feat(secrets): detect a github_secret target that names the wrong secret - #2832
Conversation
…cret
A manifest entry declares a source label and a set of targets, and the
generators emit the GitHub target FROM that label -- both
generate_chit_v2.py:143 and chit_manifest_register.py:217 write
`{"github_secret": label}`. A freshly generated entry therefore always agrees
with itself. EXISTING entries can drift, and nothing noticed.
Measured on the live manifest: 160 entries carry a github_secret target, 158
match their label, and 2 do not.
minimax_token_plan_api_key label MINIMAX_TOKEN_PLAN_API_KEY
target MINIMAX_API_KEY
That routes the token-plan credential to the PAY-AS-YOU-GO secret name. It is
issue #2748's defect -- "_infer_key_env silently substituted the fallback, so
every plan-backed route billed on the PAYG key" -- reproduced one layer down,
in the file that is supposed to be the record of truth. Two entries then
declare the same github_secret, and MINIMAX_TOKEN_PLAN_API_KEY is declared by
nothing, which is exactly why github_secret_capacity_audit.py reports it as an
orphan.
A CHECK, NOT A FIX, and that is the design. The other divergence is
service_password_postgres label SUPABASE_DB_PASSWORD
target SERVICE_PASSWORD_POSTGRES
which reads as a deliberate alias: one credential stored in GitHub under the
name of the service that owns its rotation. A tool enforcing `target == label`
would have rewritten that silently and broken an existing secret. This check
cannot tell a misroute from an alias; a human can, once, so each divergence
must be DECLARED WITH A REASON or it fails.
WHY THE DRIFT SURVIVED, which is the part worth keeping: the doctrine says the
manifests are MACHINE-EMITTED and must never be hand-edited, while
chit_manifest_register.py is additive-only with
RECONCILED_FIELDS = ("min_length",). So a drifted github_secret target had NO
LEGAL PATH to repair -- the rule forbade the edit and the tooling did not cover
the fix. It was not neglected; it was unreachable. This does not close that
hole (the registry still needs to learn to reconcile targets); it makes the
drift visible so the next one is caught in a PR instead of in a bill.
Unreadable or empty manifest exits 3, not 0 -- reporting "no drift" because
nothing parsed is worse than no check.
11 tests. The two that keep it honest pull in opposite directions: an
undeclared divergence must FAIL, and a declared one must PASS. Plus the
controls that stop an acceptance leaking -- accepting one entry does not excuse
another, and re-pointing a target does not inherit its old approval. One test
is anchored to the live tree so a NEW drift fails here rather than being
absorbed into a count nobody reads.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9mpK1ZFjexjthZbvcb94f
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 85a8193567
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isinstance(item, dict) and item.get("entry") and item.get("github_secret"): | ||
| key = f"{item['entry']}|{item['github_secret']}" | ||
| out[key] = str(item.get("reason") or "").strip() |
There was a problem hiding this comment.
When an acceptance row omits reason or supplies only whitespace, this stores an empty string but still considers the entry declared because audit() checks only whether the key exists. Such a row therefore suppresses a potentially misrouted secret without the human justification the check and _accepted.yaml explicitly require; reject or ignore acceptance rows whose normalized reason is empty.
Useful? React with 👍 / 👎.
| label = str((entry.get("source") or {}).get("label") or "") | ||
| if not label: | ||
| continue |
There was a problem hiding this comment.
If a manifest entry still has a github_secret target but its source.label is absent or empty, this branch silently skips it and the audit can report ok: true. That is precisely an unmeasurable target—the tool has no label against which to detect a misroute—so malformed or partially generated manifests can yield a false pass instead of exit 3; treat such target-bearing entries as unmeasured.
Useful? React with 👍 / 👎.
github-actions
Bot
added
chit-aware
🔏 CHIT-aware change — control-body routingThis PR touches a CHIT-aware service (ports 8086/8087/8103/8106/8113/9224 surface). Before a Control-Body ACK:
Advisory routing only — the blocking contract check is CHIT Contract. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| f"github_secret target, {report['diverged']} diverging from their label." | ||
| ) | ||
| for row in report["declared"]: | ||
| print(f" accepted {row['entry']}: {row['label']} -> {row['github_secret']}") |
| f" {row['entry']}\n" | ||
| f" source label {row['label']}\n" | ||
| f" github_secret {row['github_secret']}", |
|
CodeQL blocker triage — 2 alerts, both names-only false positives (dismiss with justification) CodeQL is the only red check on this otherwise-green PR (41 checks). Both alerts are in the new drift-report output:
Why this is a false positive by design: the tool's entire output is secret names and routing labels — never values (the GitHub API never returns values, and the manifest parse only reads names). That is exactly the doctrine #2807 codified: "Names only; the GitHub API never returns values and neither does this." CodeQL's taint heuristic can't tell a secret's name from its value when the source is a file named Recommended handling (same shape as #2834's evidence-based dismissal):
Do not mask the names — a drift check whose output hides which secret is misrouted defeats itself. Everything else on this PR is green; after dismissal it's merge-clean. |
…lisions main moved while #2818 sat blocked: #2813, #2822, #2825, #2826, #2827, #2830, #2831, #2832, #2837, #2838 all landed, plus the CodeQL alert cleanup on main. Automatic merge, no conflicts. Re-ran the checks that matter for a merge of this size rather than assuming a clean `git merge` means a correct one: duplicate top-level definitions 4 modules, all 4 already shadowed on main. mcp_server.py stays clean, so the collision fixed earlier held and nothing new joined it. hardening_ratchet debt=6, no new, no stale validate-dockerfile-paths OK (93 dockerfiles, 83 referenced) validate-command-anchors 498 baselined, 0 new, no stale Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X9mpK1ZFjexjthZbvcb94f
2 participants
A manifest entry declares a source label and a set of targets, and the generators emit the GitHub target from that label — both
generate_chit_v2.py:143andchit_manifest_register.py:217write{"github_secret": label}. So a freshly generated entry always agrees with itself. Existing entries can drift, and nothing noticed.Measured on the live manifest: 160 entries carry a
github_secrettarget, 158 match their label, 2 do not.That routes the token-plan credential to the pay-as-you-go secret name. It is issue #2748's defect — "
_infer_key_envsilently substituted the fallback, so every plan-backed route billed on the PAYG key" — reproduced one layer down, in the file that is supposed to be the record of truth.Two entries then declare the same
github_secret, andMINIMAX_TOKEN_PLAN_API_KEYis declared by nothing — which is exactly whygithub_secret_capacity_audit.pyreports it as an orphan.A check, not a fix — and that's the design
The other divergence is:
That reads as a deliberate alias: one credential stored in GitHub under the name of the service that owns its rotation. A tool enforcing
target == labelwould have rewritten it silently and broken an existing secret.This check cannot tell a misroute from an alias. A human can, once — so each divergence must be declared with a reason or it fails.
Why the drift survived
This is the part worth keeping. The doctrine says the manifests are machine-emitted and must never be hand-edited. Meanwhile
chit_manifest_register.pyis additive-only, with:So a drifted
github_secrettarget had no legal path to repair — the rule forbade the edit and the tooling didn't cover the fix. It wasn't neglected; it was unreachable.This PR does not close that hole. The registry still needs to learn to reconcile targets. What it does is make the drift visible, so the next one is caught in a PR instead of in a bill.
Tests — 11
The two that keep it honest pull in opposite directions: an undeclared divergence must FAIL (or the misroute stays invisible), and a declared one must PASS (or the tool rewrites a legitimate alias).
Plus the controls that stop an acceptance leaking — accepting one entry does not excuse another, and re-pointing a target does not inherit its old approval. One test is anchored to the live tree, so a new drift fails there rather than being absorbed into a count nobody reads.
Unreadable or empty manifest exits 3, not 0: reporting "no drift" because nothing parsed is worse than no check.
🤖 Generated with Claude Code
https://claude.ai/code/session_01X9mpK1ZFjexjthZbvcb94f