updated discord workflow #467
Conversation
📝 WalkthroughWalkthroughThis PR updates the Discord notification workflow to use Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/discord.yaml (1)
181-197: duplicate alert-webhook logic — dry it up.
sendFailureAlertat line 181 and the inline block in the outercatch(414-430) are basically the same fetch with slightly different copy ("PR Discord sync failed" vs "PR->Discord sync failed"). also,sendFailureAlertis defined but never actually called anywhere — the catch block rolls its own instead. kinda cursed at 2am but harmless.♻️ suggested cleanup
} catch (err) { const msg = err && err.message ? err.message : String(err); core.setFailed(msg); - - const alertWebhook = process.env.DISCORD_ALERT_WEBHOOK_URL; - if (alertWebhook) { - try { - await fetch(alertWebhook, { - method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ - username: "OpenScreen", - avatar_url: WEBHOOK_AVATAR, - content: `⚠️ PR->Discord sync failed\n${msg}\nRun: ${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`, - allowed_mentions: { parse: [] } - }) - }); - } catch { - core.warning("Failed to send alert webhook."); - } - } + await sendFailureAlert(msg); }Also applies to: 410-431
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/discord.yaml around lines 181 - 197, The sendFailureAlert function duplicates the inline fetch logic in the outer catch and is never used; remove the duplicated fetch and call sendFailureAlert from that catch, refactor sendFailureAlert to accept a message string (used for "PR Discord sync failed" vs "PR->Discord sync failed") and reuse common fields (username, avatar_url, allowed_mentions) while preserving the alertWebhookUrl presence check and its try/catch that logs core.warning on failure; update the outer catch to call sendFailureAlert with the appropriate message instead of rolling its own fetch block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/discord.yaml:
- Around line 4-5: The workflow currently trusts a discord-thread-id marker in
the PR body (parsed by extractThreadId) and then uses it in shouldCreateThread
and patchDiscordThread; to fix, reject or ignore PR-body markers when the PR is
from a fork (check pr.head.repo.full_name !== pr.base.repo.full_name) and only
accept markers for same-repo PRs or markers previously written by the workflow
(persist mapping in an action-owned issue comment or other write-protected
store), additionally make extractThreadId robust (use a global/regexp that finds
the workflow-authored marker or include a validation step that checks the
resolved threadId belongs to the expected parent forum channel before any
POST/PATCH in patchDiscordThread), so that attackers cannot inject arbitrary
thread_ids from forked PRs.
- Around line 21-24: The actor filter is too broad: change the conditional that
currently checks github.actor != 'github-actions[bot]' so it only ignores
actions triggered by this same workflow instead of all bot actors; update the
workflow "if" expression (the if: key) to compare github.actor to
github.triggering_actor or otherwise skip only when github.actor ==
github.triggering_actor, leaving the concurrency block (concurrency:, group:,
cancel-in-progress:) untouched; alternatively implement a secondary check later
that inspects the event payload (e.g., comment body marker) and short-circuits
only known self-trigger markers rather than excluding all 'github-actions[bot]'.
---
Nitpick comments:
In @.github/workflows/discord.yaml:
- Around line 181-197: The sendFailureAlert function duplicates the inline fetch
logic in the outer catch and is never used; remove the duplicated fetch and call
sendFailureAlert from that catch, refactor sendFailureAlert to accept a message
string (used for "PR Discord sync failed" vs "PR->Discord sync failed") and
reuse common fields (username, avatar_url, allowed_mentions) while preserving
the alertWebhookUrl presence check and its try/catch that logs core.warning on
failure; update the outer catch to call sendFailureAlert with the appropriate
message instead of rolling its own fetch block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 973c3e86-36b5-485f-8555-1b8000010552
📒 Files selected for processing (1)
.github/workflows/discord.yaml
| pull_request_target: | ||
| types: [opened, reopened, ready_for_review, converted_to_draft, synchronize, edited, labeled, unlabeled, closed] |
There was a problem hiding this comment.
pull_request_target + attacker-controlled PR body = thread hijack, lowkey risky.
switching to pull_request_target means fork PRs now get your secrets (webhook + bot token). that's fine on its own, but downstream this workflow trusts pr.body to carry the discord thread id via extractThreadId. a fork PR author can drop <!-- discord-thread-id:<some-channel-id> --> into their PR description and:
shouldCreateThread(line 230-233) short-circuits becausethreadIdis truthy,- every subsequent webhook post goes to the attacker-chosen
thread_id(line 93), patchDiscordThreadhappily PATCHes that channel with your bot token — renaming it, rewritingapplied_tags, and onclosed/merge evenarchived: true, locked: true(lines 305, 341, 381).
also note the regex isn't global, so on edited a malicious body can prepend a new marker ahead of the legit one and take over an existing thread mapping.
mitigations to consider:
- persist the PR→thread mapping somewhere the author can't write (repo variable, gist, issue comment authored by the action, or an external kv) instead of the PR body, OR
- reject/ignore markers on events where
pr.head.repo.full_name !== pr.base.repo.full_nameunless the marker was verified to have been written by the workflow (e.g., cross-check against a trusted source), OR - at minimum, validate
threadIdagainst the parent forum channel id before PATCH/POST so arbitrary channels can't be addressed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/discord.yaml around lines 4 - 5, The workflow currently
trusts a discord-thread-id marker in the PR body (parsed by extractThreadId) and
then uses it in shouldCreateThread and patchDiscordThread; to fix, reject or
ignore PR-body markers when the PR is from a fork (check pr.head.repo.full_name
!== pr.base.repo.full_name) and only accept markers for same-repo PRs or markers
previously written by the workflow (persist mapping in an action-owned issue
comment or other write-protected store), additionally make extractThreadId
robust (use a global/regexp that finds the workflow-authored marker or include a
validation step that checks the resolved threadId belongs to the expected parent
forum channel before any POST/PATCH in patchDiscordThread), so that attackers
cannot inject arbitrary thread_ids from forked PRs.
| if: github.event_name != 'schedule' && github.actor != 'github-actions[bot]' | ||
| concurrency: | ||
| group: discord-pr-sync-${{ github.repository }}-${{ github.event.pull_request.number || github.event.issue.number || github.run_id }} | ||
| cancel-in-progress: false |
There was a problem hiding this comment.
actor filter is a bit blunt; concurrency looks good.
concurrency block is fine — cancel-in-progress: false is the right call for a notification pipeline, and the run_id fallback keeps non-PR triggers from colliding.
nit on the actor check: github.actor != 'github-actions[bot]' will also drop legitimate issue_comment / pull_request_review events authored by the repo's own actions (release bots, stale-bot comments, etc.). if the intent is just "don't react to things this very workflow did," a github.triggering_actor check or filtering further down (e.g., skip only when the comment body is a known marker) is cleaner. not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/discord.yaml around lines 21 - 24, The actor filter is too
broad: change the conditional that currently checks github.actor !=
'github-actions[bot]' so it only ignores actions triggered by this same workflow
instead of all bot actors; update the workflow "if" expression (the if: key) to
compare github.actor to github.triggering_actor or otherwise skip only when
github.actor == github.triggering_actor, leaving the concurrency block
(concurrency:, group:, cancel-in-progress:) untouched; alternatively implement a
secondary check later that inspects the event payload (e.g., comment body
marker) and short-circuits only known self-trigger markers rather than excluding
all 'github-actions[bot]'.
![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: 63c850bc08
ℹ️ 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".
| context.eventName === "pull_request_target" && | ||
| ["opened", "reopened", "ready_for_review"].includes(action) && | ||
| !threadId; |
There was a problem hiding this comment.
Now that this job is triggered by pull_request_target, fork PR authors can control pr.body while repository secrets are available. This condition trusts any existing discord-thread-id marker in the PR body and skips creating a fresh mapping, so an attacker can pre-seed an arbitrary numeric thread ID and make subsequent events post to or patch unrelated Discord threads via the webhook/bot token. Please treat PR-body markers as untrusted on fork-originated PRs (or verify the marker belongs to this PR) before reusing them.
Useful? React with 👍 / 👎.
2 participants
Summary by CodeRabbit
Bug Fixes
Changes