Update test.yml with ci-secrets env in mind#252
Update test.yml with ci-secrets env in mind#252architect-dev wants to merge 4 commits intomasterfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| repo, | ||
| issue_number, | ||
| per_page: 100, | ||
| }); | ||
|
|
||
| if (comments.some((c) => c.body && c.body.includes(marker))) { | ||
| core.info('Fork PR notice already present, skipping.'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟡 Nit: the marker check on line 45 (comments.some((c) => c.body && c.body.includes(marker))) inspects every comment body, not just bot-authored ones. Since the marker <!-- cofhe-fork-pr-notice --> is publicly visible in this workflow file, any commenter who posts a comment containing that string will silently suppress the notice. Consider also gating on c.user && c.user.login === 'github-actions[bot]' — the standard pattern for marker-based one-time bot comments. Impact is purely cosmetic (the actual secret gating is enforced independently by the ci-secrets environment), so this is a one-line hardening rather than a security fix.
Extended reasoning...
What the bug is
The new pr-fork-notice.yml posts a one-time comment on fork PRs, using a hidden HTML marker <!-- cofhe-fork-pr-notice --> to avoid duplicates. The dedup check is:
if (comments.some((c) => c.body && c.body.includes(marker))) {
core.info('Fork PR notice already present, skipping.');
return;
}This inspects the body of every comment on the PR, regardless of author. The marker string is plainly visible in the public workflow YAML, so any user — including the fork PR opener — can post a comment containing the marker substring and cause the bot to skip its notice on subsequent opened/reopened events.
Trigger
For an opened event, the race window is narrow but real: the PR author can open the PR via the API and post a comment containing the marker before the queued workflow run reaches the listComments step (queue + cold start is typically several seconds). For reopened, the window is wide open — any pre-existing comment from anyone that happens to contain the marker substring will suppress the notice when the PR is reopened.
Step-by-step
- Attacker (or just a curious contributor) opens a fork PR via API.
- Within the same script, immediately POSTs an issue comment with body
<!-- cofhe-fork-pr-notice --> hello. - The
pull_request_targetworkflow eventually runs, callslistComments, finds the marker, logsFork PR notice already present, skipping.and exits. - The maintainer-facing notice is never posted.
Addressing the refutation
The refuting verifier is largely right, and this is why I'm filing as a nit, not a normal bug:
- No security impact. The actual secret gating is the
environment: ci-secretsdirective intest.yml, which is enforced by GitHub independently of this notice. A successful suppression yields zero secret exposure. - Maintainers don't depend on this comment. Pending workflow approvals are surfaced in the Actions tab and via GitHub notifications. The notice is a friendly reminder, not a control.
- The trigger for the malicious case is implausible (must race a workflow queue and gain nothing).
However, I disagree with two refutation points:
- "Maintainer quoting the bot" is not a real concern. A maintainer quoting the bot would only matter if they did so before the bot's first run (otherwise the bot has already posted its own marker comment and the dedup is correct). That ordering is rare. And even in that edge case, an author filter is the standard pattern used widely for marker-based bot comments — losing the ability to quote-suppress is not a meaningful regression.
- The
reopenedpath widens the window considerably. Any historical comment that quotes the marker (e.g., a maintainer pasting the workflow file in a discussion) will permanently suppress the notice on reopen. This is more than a millisecond race.
Fix
One-line change in the some predicate:
const BOT = 'github-actions[bot]';
if (comments.some((c) => c.user && c.user.login === BOT && c.body && c.body.includes(marker))) {Optional belt-and-suspenders: make the marker more unique (e.g., include a UUID or a per-repo nonce) so it's less likely to appear by accident. Either change is a few lines and addresses the issue cleanly.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c54af91. Configure here.
|
Doesn't work, env secrets not exposed, to get it working would be way too much security risk. Closing for now |
1 participant
Note
Medium Risk
Touches CI secret exposure and adds a
pull_request_targetworkflow with write PR permissions; safe if the notice workflow never checks out/executes PR code, but misconfiguration could leak secrets or block CI.Overview
Adds a fork-PR approval flow for CI secrets:
test.ymlnow conditionally runs in the gatedci-secretsGitHub Environment for fork-basedpull_requestruns, while internal PRs and pushes keep using repo-level secrets.Introduces a new
pull_request_targetworkflow (pr-fork-notice.yml) that posts a one-time comment on fork PRs instructing maintainers to Approve and run to release environment-scoped secrets.Adds a short doc (
.memory/ci-secrets-gating.md) describing the routing logic, security constraints for the notice workflow, and common failure modes.Reviewed by Cursor Bugbot for commit ed9b025. Bugbot is set up for automated code reviews on this repo. Configure here.