docs(operations): record runner audit credential and assignment contract - #401
docs(operations): record runner audit credential and assignment contract#401cursor[bot] wants to merge 23 commits into
Conversation
Name capability-file ingress, exact token redaction, and started_at losing assignment authority in Unreleased so operators can apply the hardened production command without treating PASS as merge authority. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review of #401 (64c6213)
This head is the right landing vehicle for the runner-assignment hardening that started in #394. Prefer this branch over merging #394 at f4a4bc9. Do not treat this COMMENT as approval, required-check success, or merge authority.
CodeRabbit CLI 0.7.3 is installed here, but coderabbit auth login --agent timed out on the browser callback (automatic_login_failed). This assessment is from the a634066...64c6213 range, not a CodeRabbit CLI result.
What this PR gets right
- Production
operations:runner-assignmentreads GitHub transport authority only from the owner-only file named byNOEMA_MAINTAINER_TOKEN_PATH. AmbientGH_TOKENalone fails closed. ghspawn/stderr diagnostics exact-match redact the active token as[REDACTED].- Assignment authority is a positive
runner_idor a non-emptyrunner_name. Queuedstarted_atwithrunner_id=0stays unassigned. - Unreleased now names the three production behavior changes and the next operator action: prepare the capability file; do not treat PASS as required Check, formal review, or merge authority.
- Tests inject
gh_apiwithout a filesystem credential, and the production path uses a real capability file against an ambient decoy token.
Residual risk (not merge-blocking)
redactExactSecretwill explode a diagnostic if it is ever called with an empty secret. Unreachable today becausecreateGhSubprocessEnvironmentrejects emptyGH_TOKENfirst. Guard the empty-secret case before reuse.- Test
ghshims interpolate the expected token into a shelliftest. Current literals are safe; a later$/ backtick / quote token would break the shim. Compare against a side file instead. test/actions-runner-assignment-cli.test.tsis missing a trailing newline.- First-run operator trap:
echo TOKEN > filewrites a trailing newline, and the capability reader then fails closed on control characters. Doctoring already usesprintf '%s', but it should say thatecho/printf '%s 'will fail and that the token needs Actions workflow-run and workflow-job read.
Next action
Keep #401 as the review surface for this slice. Wait for exact-head verify and Security Scan (trivy-fs is a real finding, not a flake) to terminate on this SHA. Do not merge #394 once this PR exists unless that head is fast-forwarded to include the Unreleased bullet. This does not close issue #30.
Queued checks and this COMMENT are not GREEN and are not merge authority.
Sent by Cursor Automation: Fix Issues
|
|
||
| function redactExactSecret(value, secret) { | ||
| const text = typeof value === "string" ? value : String(value ?? ""); | ||
| return text.split(secret).join("[REDACTED]"); |
There was a problem hiding this comment.
text.split(secret).join("[REDACTED]") is correct for a non-empty active token, and createGhSubprocessEnvironment currently rejects empty GH_TOKEN before this runs.
If this helper is reused with an empty secret, split("") inserts [REDACTED] between every character. Guard with if (typeof secret !== "string" || secret.length === 0) return text so a later caller cannot take that landmine.
| const executable = join(directory, "gh"); | ||
| const tokenGuard = expectedToken === undefined | ||
| ? "" | ||
| : `if [ "$GH_TOKEN" != "${expectedToken}" ]; then\n printf '%s' 'unexpected delegated GH_TOKEN' >&2\n exit 91\nfi\n`; |
There was a problem hiding this comment.
This shim interpolates expectedToken into a POSIX if test. The current literals (read-only-capability-token) are shell-safe, so this is not a production leak.
A later call with $, backticks, or " would break the comparison or write a real token into a script. Compare $GH_TOKEN to a side file, or keep the guard in JavaScript.
| await expect(startCli({ execute: success, write_error: writeError, set_exit_code: setExitCode })).resolves.toEqual({ exit_code: 0 }); | ||
| }); | ||
| }); | ||
| }); No newline at end of file |
There was a problem hiding this comment.
This file lost its trailing newline in the range diff (\\ No newline at end of file). Restore the POSIX text-file terminator so later patches do not keep carrying a noisy EOF hunk.
|
Superseded by #402. Exact ancestry is preserved: #401 head |
2 participants
Purpose
Land the #394 runner-assignment hardening together with the missing Unreleased operator contract. #394 at
f4a4bc9already implements capability-file ingress, exact token redaction, andstarted_atlosing assignment authority, butCHANGELOG.mdstill described only the original audit addition. Operators reading Unreleased could not see the three production behavior changes or the next action (prepareNOEMA_MAINTAINER_TOKEN_PATH, do not treat PASS as merge authority).What to do with this PR
Prefer this branch over merging #394 at
f4a4bc9. This head is #394 plus one documentation/contract commit. Do not merge #394 once this PR exists unless that head is fast-forwarded to include the Unreleased bullet.Test-first evidence
test/actions-runner-assignment-docs.test.tsnow requires Unreleased to nameNOEMA_MAINTAINER_TOKEN_PATH,[REDACTED], queuedstarted_at,runner_id, andrunner_name, while keeping the existingoperations:runner-assignment/ required Check / formal review phrases. Local Node 24.19.0 / npm 11.17.0: eight runner-assignment and changelog contract files, 44 tests, all passed.Authority boundary
This does not close issue #30. It does not change GitHub permissions, reviewer/maintainer identities, workflow authority, release/deployment authority, or any central repository. Queued exact-head checks on this successor are not GREEN and are not merge authority.
Related: #394, #30.