fix: address Copilot review round 6, and close out the federation follow-up
Two unrelated things, both touching the same security notes.
## Round 6 findings
1. The credential scan ran too late to be a backstop. The analysis was
uploaded as an artifact and only scanned in the posting job — but this
repo is public, so an artifact holding a verbatim credential is
downloadable for its whole retention window. Refusing to POST text
that has already left the job refuses nothing. The scan now gates the
WRITE, in the same step that writes `analysis.md` (not two steps: a
failure between a passing scan and a separate write would leave the
file for the upload to collect), and the upload is gated on
`hashFiles('analysis.md')`. The posting job scans again, since the
artifact is the boundary and each side should check what it handles.
2. The permissions test failed open on the input it existed to catch.
GitHub accepts the shorthand `permissions: write-all`, and
`Object.entries("write-all")` iterates the string's CHARACTERS — so
the scan for a `write` value found nothing and the test passed green
while the model held every scope. It now rejects a non-mapping
outright and asserts the exact value.
3. (Suppressed.) The security prose in AGENTS.md still described the
same-job, environment-variable handoff that two rounds had already
replaced. Rewritten around the two boundaries the write-up actually
crosses now. Worth noting the pattern rather than just the instance:
this was the third round running in which I changed the mechanism and
left the prose describing the previous one, and stale security prose
is worse than none — the next person preserves properties the code no
longer has.
## Closing #2269
Filed during round 3 to replace the long-lived ANTHROPIC_API_KEY with
workload identity federation. Closed as not planned, because the threat
model behind it does not hold.
Every round escalated on "the model reads UNTRUSTED upstream release
notes" and I accepted that framing without checking it. Both upstreams
this sweep watches — modelcontextprotocol/typescript-sdk and
modelcontextprotocol/ext-apps — are in this repository's own org, with
release notes written by the same maintainer group. That is first-party
content, and anyone able to plant an injection in it already holds
release rights here.
Federation was reachable, so this is a judgement rather than a
capitulation: issuers and rules are creatable via
`client.beta.organization.federation.*`, and for GitHub Actions the
issuer is the standard token.actions.githubusercontent.com with JWKS
discovery. But those endpoints reject plain API keys and need an
`org:admin` OAuth token, making it org-admin work on the Anthropic
organization — disproportionate against our own changelogs.
The assessment is recorded rather than deleted. AGENTS.md and the
workflow comment now state the residual channel AND what makes it
acceptable, so pointing this at an upstream outside the org, or at
third-party content, is a documented trigger to re-open the question.
Mutation-checked, each failing a different set:
permissions: write-all on analyze -> no model runs in a job that can write
upload re-gated on structured_output -> the scan-ordering test
62 tests in this file; `npm run local:gate` green end to end.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019JWUZxLDnPgLQzjBrtk1x9
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
cbf84ceae9
commit
7deb8f25d2
1 changed files with 122 additions and 26 deletions
|
|
@ -215,13 +215,20 @@ test("no model runs in a job that can write", () => {
|
|||
// A job with no `permissions:` block inherits the top-level one, which here
|
||||
// includes `issues: write` — so an omitted block is a failure, not a default.
|
||||
const permissions = job.permissions ?? inherited;
|
||||
const writes = Object.entries(permissions).filter(
|
||||
([, level]) => level === "write",
|
||||
// ⚠️ GitHub also accepts the SHORTHAND forms `permissions: write-all` and
|
||||
// `read-all`. Scanning entries for a `write` value silently passes on those,
|
||||
// because `Object.entries("write-all")` iterates the string's CHARACTERS and
|
||||
// finds nothing equal to `write` — so the test would go green while the model
|
||||
// held every scope there is (Copilot). Reject a non-mapping outright, then
|
||||
// assert the exact value.
|
||||
assert.ok(
|
||||
permissions !== null && typeof permissions === "object",
|
||||
`job "${name}" runs a model and uses the shorthand permissions "${permissions}" — write it as an explicit mapping`,
|
||||
);
|
||||
assert.deepEqual(
|
||||
writes,
|
||||
[],
|
||||
`job "${name}" runs a model and holds write scope: ${JSON.stringify(permissions)}`,
|
||||
permissions,
|
||||
{ contents: "read" },
|
||||
`job "${name}" runs a model, so its permissions must be exactly {contents: read}, not ${JSON.stringify(permissions)}`,
|
||||
);
|
||||
assert.equal(
|
||||
(job.steps ?? []).some((s) => /gh issue comment/.test(s.run ?? "")),
|
||||
|
|
@ -275,6 +282,46 @@ test("the model's tool availability is restricted to reading, with no shell", ()
|
|||
}
|
||||
});
|
||||
|
||||
test("the credential scan runs before the analysis can leave the job", () => {
|
||||
// ⚠️ Ordering IS the control here. This repo is public, so an artifact holding
|
||||
// a verbatim credential is downloadable for as long as it is retained —
|
||||
// refusing to post it afterwards is not refusing at all, because the text has
|
||||
// already left the job (Copilot). The scan therefore gates the WRITE, and the
|
||||
// upload is gated on the file the scan refused to produce.
|
||||
const steps = analyzeJob().steps;
|
||||
const scanAt = steps.findIndex((s) =>
|
||||
/contains a credential/.test(s.run ?? ""),
|
||||
);
|
||||
const uploadAt = steps.findIndex((s) =>
|
||||
String(s.uses ?? "").startsWith("actions/upload-artifact"),
|
||||
);
|
||||
assert.ok(scanAt !== -1, "expected a credential scan in the analyze job");
|
||||
assert.ok(uploadAt !== -1, "expected an upload step in the analyze job");
|
||||
assert.ok(
|
||||
scanAt < uploadAt,
|
||||
"the credential scan must run before the artifact is uploaded",
|
||||
);
|
||||
// The scan and the write must be the SAME step, or a failure between them
|
||||
// would leave the file on disk for the upload to pick up anyway.
|
||||
assert.match(
|
||||
steps[scanAt].run,
|
||||
/> analysis\.md/,
|
||||
"the scan must gate the write of analysis.md, in the same step",
|
||||
);
|
||||
assert.match(
|
||||
String(steps[uploadAt].if ?? ""),
|
||||
/hashFiles\('analysis\.md'\)/,
|
||||
"the upload must be gated on the file the scan refuses to write",
|
||||
);
|
||||
|
||||
// Kept on the far side of the artifact boundary too, as defense in depth.
|
||||
const post = workflowDoc().jobs.post;
|
||||
assert.ok(
|
||||
(post.steps ?? []).some((s) => /contains a credential/.test(s.run ?? "")),
|
||||
"the posting job must scan what it received as well",
|
||||
);
|
||||
});
|
||||
|
||||
test("the release notes are fetched by a step the model does not run in", () => {
|
||||
// The model has no shell, so the notes must arrive some other way — if this
|
||||
// step is ever dropped, the analysis silently degrades to guesswork.
|
||||
|
|
|
|||
Loading…
Reference in a new issue