feat(discord): migrate Discord integration from private repo - #439
feat(discord): migrate Discord integration from private repo#439Shubhank-Jonnada wants to merge 5 commits into
Conversation
Moves the Discord integration out of the private integrations repo and into this repository. Five actions: list_channels, get_message_history, send_message, add_reaction, and remove_reaction, all against the Discord REST API v10. Auth is platform OAuth with the bot scope. The bot token is read from the DISCORD_BOT_TOKEN environment variable at call time and is not stored in source. Channel-scoped actions verify the channel belongs to the authorized guild before acting. Changes made during the migration: * Pinned the SDK to autohive-integrations-sdk~=2.0.1, since 2.0.0 is deprecated. * Dropped the messages.read, guilds, and guilds.members.read OAuth scopes. Every call authenticates with the bot token, so no user-token scope gated any of them, and the guild id in connection metadata comes from the guild_id parameter on the OAuth callback rather than the guilds scope. * Removed a remote CDN icon URL from config.json so the bundled icon.png is used, matching every other integration here. * Rewrote the README in the public style, documenting the bot permissions each action needs. * Cleared a stale __init__.py comment that referenced a vendored SDK and a private issue number. * Added the Discord environment variables to .env.example and listed the integration in the repository README. Security review: no hardcoded secrets or credentials. The only long literals in the tests are placeholder snowflake ids. The legacy manual live-test harness (test_discord_integration.py) asserts nothing and was left in the private repo rather than migrated, matching the Teams migration in #397. Validated with hiveup: all 13 checks pass, including 12 unit tests.
🔍 Integration Validation ResultsCommit:
✅ Structure Check output✅ Code Check output✅ Tests Check output✅ README Check output✅ Version Check output |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d28123d9cf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| allowed_guild = context.metadata.get("guild") | ||
| if allowed_guild and response.data.get("guild_id") != allowed_guild: |
There was a problem hiding this comment.
When the connection metadata does not contain guild (for example if the platform stores the OAuth callback value under guild_id or metadata is missing), this condition skips the guild comparison entirely. Because the actions authenticate with Autohive's shared bot token, get_message_history, send_message, and reaction actions can then operate on any channel ID where that bot is installed instead of failing closed for the connected server.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Now fails closed. The guild is required and checked before the channel lookup, so a call without it costs no request, and the four channel-scoped actions agree with list_channels which already failed closed. Added unit coverage parametrized across all four actions plus the None and empty-string cases - 707220d
One correction on the cause: the platform does store the value under guild, not guild_id. DiscordOAuthConnectionProvisioner.cs serializes the connection metadata as guild = tokenGuild.Id, so the key matches what the integration reads. The fail-open you identified is real regardless, since absent or empty metadata skipped the comparison entirely.
| emoji = inputs["reaction"] | ||
| if not emoji.isalnum(): | ||
| emoji = quote(emoji) |
There was a problem hiding this comment.
When a workflow follows the schema/README and supplies a custom emoji ID, isalnum() leaves the raw snowflake in the reaction URL; Discord requires custom emoji reactions to be URL-encoded as name:id, so these add/remove calls fail with 10014: Unknown Emoji for custom emoji IDs. Please either require/build the name:id value or expose the emoji name alongside the ID. See Discord's reaction docs: https://docs.discord.com/developers/resources/message#create-reaction.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by building the name:id value, which is the more complete of the two options you offered. Reaction handling moved into _resolve_reaction_emoji, which accepts a Unicode emoji (URL encoded), a custom emoji already given as name:id (encoded, no extra request), or a bare custom emoji id, which is resolved against the authorized guild emoji list and sent as name:id. An id absent from the server returns an error naming the id instead of Discord 10014. A blank reaction is rejected up front - 2a8f169
Worth noting the old test_alphanumeric_emoji_not_encoded asserted the broken behaviour, so it is replaced by coverage of all the shapes above.
One thing surfaced while fixing this: adding a literal emoji to a config.json description breaks config parsing on Windows, because the SDK Integration.load opens the file without an explicit encoding and cp1252 cannot decode it. config.json is kept ASCII and the emoji examples live in the README.
Replaces the assertion-free live harness that was left behind in the private repo with real pytest integration tests against the Discord REST API. Twelve tests, nine read-only and three destructive: * list_channels: returns channels, payload structure, every channel belongs to the authorized guild, and a missing guild is rejected without a call. * get_message_history: returns messages and has_more, payload structure, limit is respected, the before cursor pages backwards and excludes itself, and a channel outside the authorized guild is rejected. * send_message and the reaction pair are marked destructive, since they post real messages and reactions. The reaction test is a lifecycle: post a message, react to it, then remove the reaction. The live context performs real HTTP through aiohttp and supplies the guild metadata the platform would normally inject. It deliberately does not add an Authorization header, because the integration builds its own from DISCORD_BOT_TOKEN, which is what these tests need to exercise. Empty 204 bodies from the reaction endpoints and non-string query values are both handled in the fetch wrapper. Tests skip rather than fail when credentials are absent. Documented the required variables and the safe and destructive run commands in the integration README, and narrowed the .env.example block to the three variables the tests actually read.
_verify_channel_guild only compared the channel's guild when a guild was present in the connection metadata, so an absent or empty value skipped the comparison and let the action proceed. Because every action authenticates with Autohive's shared bot token rather than a user token, that meant get_message_history, send_message, add_reaction, and remove_reaction could operate on any channel that bot can see in any server, not just the one the connection was authorized for. The guild is now required, and checked before the channel lookup rather than after, so an unauthorized call costs no request. This also makes the four channel-scoped actions agree with list_channels, which already failed closed on missing metadata. Adds unit coverage parametrized across all four channel-scoped actions for missing metadata, plus the None and empty-string guild cases. Reported by Codex review on #439.
Discord's reaction routes need the emoji path segment URL encoded, and a custom emoji identified as name:id. A bare id fails with 10014: Unknown Emoji. The previous check skipped encoding for anything alphanumeric, so a bare snowflake went into the URL untouched and add_reaction and remove_reaction failed for exactly the input config.json documented, a "custom emoji ID". Reaction handling now lives in _resolve_reaction_emoji and accepts three shapes: * A Unicode emoji, which is URL encoded. * A custom emoji already given as name:id, which is URL encoded and used as is, with no extra request. * A bare custom emoji id, which is resolved against the authorized guild's emoji list and sent as name:id. An id that does not exist there returns an error naming the id rather than Discord's opaque 10014. A blank reaction is now rejected up front instead of producing a malformed URL. Kept config.json ASCII: the SDK's Integration.load opens it without an explicit encoding, so a literal emoji in a description makes config parsing fail on any Windows machine with a cp1252 locale. The emoji examples live in the README instead, which documents all three accepted forms. test_alphanumeric_emoji_not_encoded asserted the old behaviour and is replaced by coverage of the encoded Unicode path, the name:id path, bare-id resolution for both add and remove, an unknown id, and a blank reaction. Reported by Codex review on #439. Format confirmed against Discord's reaction docs.
Every action authenticates with Autohive's shared bot token, so
_verify_channel_guild is the only thing keeping a connection inside its
own server. It validated `channel`, but two other caller-supplied values
land in the URL after that check has already passed, and the HTTP client
resolves dot segments before sending.
A message_id of
../../<other-channel>/messages/<id>
turned
PUT /channels/<authorized>/messages/<crafted>/reactions/<e>/@me
into
PUT /channels/<other-channel>/messages/<id>/reactions/<e>/@me
reaching a channel in a guild the connection was never authorized for,
using a bot token shared across customers. The reaction segment had the
same hole from the opposite direction: quote() leaves "/" unescaped by
default, so an emoji string could also rewrite the path.
_snowflake() now validates channel and message_id in every action that
builds a path from them, the emoji is quoted with safe="", and
config.json carries a matching pattern. A test pins the rewrite itself,
and disabling the validation makes it fail with the PUT landing on the
other channel, so the coverage is not vacuous.
|
Audited this against the Step 1 — Security / safety / secrets review
Finding: path-bound IDs could cross the guild boundaryFixed in 923f1fe.
A into reaching a channel in a guild the connection was never authorized for. The reaction segment had the same hole from the other direction: Fix: I checked the tests are not vacuous: disabling the validation makes them fail with the PUT landing on Also
|
|
@Autohive-AI review |
![autohive-ai-beta[bot]](https://avatars.githubusercontent.com/in/4631178?s=60&v=4){kind=small}
There was a problem hiding this comment.
| Verdict | Errors | Warnings | Suggestions |
|---|---|---|---|
| Reviewed | 0 | 1 | 0 |
Reviewed
Summary
Adds the Discord REST integration with guild-scoped channel actions and tests. Targeted unit tests could not run because pytest is unavailable in the review container.
PR Change Flow
sequenceDiagram
participant W as Workflow
participant D as Discord Integration
participant API as Discord API
W->>D: Execute channel action
D->>API: Verify channel guild
API-->>D: Channel guild metadata
D->>API: Read, post, or react
API-->>D: Action response
D-->>W: Action result
Review powered by Autohive
| except ValueError as e: | ||
| return ActionError(message=str(e)) | ||
|
|
||
| limit = inputs.get("limit", 100) |
There was a problem hiding this comment.
A caller can supply limit=101, 0, or 1.5: the schema accepts these values and this forwards them unchanged to Discord, which only accepts an integer from 1 through 100 and responds with a 400. Validate the range and integer type here (and constrain the schema) so invalid workflow input returns a local action error instead of a failed API request.
| limit = inputs.get("limit", 100) | |
| limit = inputs.get("limit", 100) | |
| if isinstance(limit, bool) or not isinstance(limit, int) or not 1 <= limit <= 100: | |
| return ActionError(message="'limit' must be an integer between 1 and 100.") |
1 participant
Summary
Moves the Discord integration from the private
integrationsrepo into this repository, following the pattern used for Teams (#397) and Agno Data Agent (#401).Five actions against the Discord REST API v10:
list_channelsget_message_historybeforemessage idsend_messageadd_reactionremove_reactionAuth is platform OAuth with the
botscope. Calls authenticate with Autohive's bot token, read fromDISCORD_BOT_TOKENat call time and never stored in source. Channel-scoped actions verify the target channel belongs to the authorized guild before acting, so a workflow cannot reach into an unrelated guild.Which version this is
The copy on
masterin the private repo is a legacy layout: SDK~=1.0.2with vendoredaiohttp/jsonschema, anintegration.py, and a singletests/test_discord.py. This PR migrates the modernised version instead, which was on the private repo'sfix/discord-iconbranch: SDK 2.x, current action-handler layout, and a real unit suite.Changes made during the migration
~=2.0.1. The integration was on~=2.0.0, which HiveUp flags as deprecated.messages.read,guilds, andguilds.members.read. All call sites use the bot token, so no user-token scope gated any of them. The one thing that looked scope dependent, the guild id incontext.metadata, is written by the platform asguildfrom the OAuth callback'sguild_idparameter (DiscordOAuthConnectionProvisioner.cs), which thebotscope provides, so the guild authorization check is unaffected.config.json. It pointed at Discord's old Webflow host and was the onlyiconfield of its kind in either repo. The bundledicon.pngis now used, matching every other integration here.__init__.pycomment that referenced a vendored SDK and a private issue number..env.exampleand listed the integration in the repository README.Review feedback addressed
Both raised by Codex review, each fixed in its own commit.
Guild authorization failed open (
707220d)._verify_channel_guildonly compared the channel's guild when one was present in metadata, so an absent or empty value skipped the check and the action proceeded. Since every action uses Autohive's shared bot token rather than a user token, that allowed reaching any channel that bot can see in any server. The guild is now required, and checked before the channel lookup so an unauthorized call costs no request. This also makes the four channel-scoped actions agree withlist_channels, which already failed closed.Custom emoji reactions were malformed (
2a8f169). Discord needs the emoji path segment URL encoded and a custom emoji identified asname:id; a bare id fails with10014: Unknown Emoji. The old check skipped encoding for anything alphanumeric, so a bare snowflake, precisely whatconfig.jsondocumented as a "custom emoji ID", went into the URL untouched. Reaction handling now accepts three shapes:name:idname:idAn id absent from the server returns an error naming it rather than Discord's opaque
10014. A blank reaction is rejected up front.One incidental find while fixing that:
config.jsonis kept ASCII deliberately. The SDK'sIntegration.loadopens it without an explicit encoding, so a literal emoji in a description makes config parsing fail on any Windows machine with a cp1252 locale. The emoji examples live in the README.Tests
21 unit tests (
test_discord_unit.py): every action, guild authorization including the fail-closed cases parametrized across all four channel-scoped actions,has_morepaging, threaded-reply payload, and all the emoji shapes above.12 integration tests (
test_discord_integration.py), written for this PR in2cc085b. The private repo's live harness asserted nothing, so rather than migrate a file that passes unconditionally, these were written properly.9 read-only:
list_channelsreturns channels, payload structure, every channel belongs to the authorized guild, missing guild rejected without any HTTP callget_message_historyreturnsmessages/has_more, payload structure,limitrespected,beforecursor pages backwards and excludes itself, channel outside the authorized guild rejected3 destructive, marked
@pytest.mark.destructivebecause they write real data:send_message, threaded reply, and a reaction lifecycle (post a message, react, remove the reaction).The live context performs real HTTP via
aiohttpand supplies the guild metadata the platform injects. It deliberately does not add anAuthorizationheader, since the integration builds its own fromDISCORD_BOT_TOKENand that is the path worth exercising. Empty204bodies from the reaction endpoints and non-string query values are handled in the fetch wrapper.Running them
Read-only, safe to repeat:
pytest discord/tests/test_discord_integration.py -m "integration and not destructive"DISCORD_CHANNEL_ID. Run deliberately, not as part of a review pass:pytest discord/tests/test_discord_integration.py -m "integration and destructive"Requires
DISCORD_BOT_TOKEN,DISCORD_GUILD_ID, andDISCORD_CHANNEL_ID; all skip cleanly when unset. Neither set runs in CI, by both the-m unitdefault filter andpython_filesnot matchingtest_*_integration.py. I verified all three: 9 skipped / 3 deselected read-only, 3 skipped / 9 deselected destructive, and a defaultpytest discord/collecting only the unit tests.Security review
No hardcoded secrets or credentials. The only long literals in the tests are placeholder snowflake ids (
111111111111111111and similar). The bot token is environment supplied, so nothing needs rotating as a result of this move.Validation
hiveup validate --base-ref origin/masterpasses all 13 checks.Related