rasterstate/download-artifact-action
2
0
Fork 0
Code Issues 4 Pull requests Projects Releases Packages Wiki Activity Actions

Reject cross-repo artifact scopes #16

Closed
stephen wants to merge 1 commit from fix/scope-isolation-15 into main
pull from: fix/scope-isolation-15
merge into: rasterstate:main
Conversation 2 Commits 1 Files changed 4 +63 -4
stephen commented 2026-07-05 20:05:03 +00:00
Owner
Copy link

Fixes #15.

Confirmed the vulnerable path before the fix:

action.yml accepted repository: "Download artifacts from a different repository"
config used repo: sanitize(process.env.RASTER_ARTIFACTS_SCOPE || repo)

download-impl passed the repository input into resolveConfig, so workflow-controlled input/env could select another repository's storage prefix while all repos shared the same backend credential.

Changes:

  • Bind downloads to the current GITHUB_REPOSITORY scope.
  • Reject a mismatched repository input or RASTER_ARTIFACTS_SCOPE override before backend access.
  • Update action metadata/README to remove the cross-repo access claim.
  • Add regression tests proving mismatched repository and scope reads are rejected.

Verification:

  • npm test
  • npm run check

Upload follow-up: filed rasterstate/upload-artifact-action#10 for equivalent write-scope validation; this PR intentionally scopes only the download-side read boundary.

Fixes #15. Confirmed the vulnerable path before the fix: > action.yml accepted `repository`: "Download artifacts from a different repository" > config used `repo: sanitize(process.env.RASTER_ARTIFACTS_SCOPE || repo)` `download-impl` passed the `repository` input into `resolveConfig`, so workflow-controlled input/env could select another repository's storage prefix while all repos shared the same backend credential. Changes: - Bind downloads to the current `GITHUB_REPOSITORY` scope. - Reject a mismatched `repository` input or `RASTER_ARTIFACTS_SCOPE` override before backend access. - Update action metadata/README to remove the cross-repo access claim. - Add regression tests proving mismatched repository and scope reads are rejected. Verification: - `npm test` - `npm run check` Upload follow-up: filed rasterstate/upload-artifact-action#10 for equivalent write-scope validation; this PR intentionally scopes only the download-side read boundary.
Reject cross-repo artifact scopes
All checks were successful
ci / test (pull_request) Successful in 7s
Details
test / unit (pull_request) Successful in 5s
Details
test / e2e (pull_request) Successful in 5s
Details
654c90bd85
stephen commented 2026-07-05 20:11:56 +00:00
Author
Owner
Copy link

Adversarial review: CHANGES (do not merge as the #15 access-boundary fix)

I did not author this. Reviewed at 654c90b, diffed vs main, against the #15 threat model (shared runner pool, one shared RASTER_ARTIFACTS_S3_* credential, scope values set inside the CI job). CI is green (ci/test, test/unit, test/e2e all pass) and the suite is 35/35 locally, but green CI here only proves the two weaker vectors are closed, not the real one.

The fix is client-side only, and the boundary is still bypassable. It moves the anchor from "repository input / RASTER_ARTIFACTS_SCOPE" to "GITHUB_REPOSITORY" and rejects the first two when they disagree with the third (config.js:65-82). But GITHUB_REPOSITORY is itself job-controlled env, read straight from process.env (config.js:48) with no server-side or signed binding. The attacker just tells the same lie one level down.

Proof (probe 1 + 3): the bypass succeeds

Malicious job sets GITHUB_REPOSITORY to the victim, leaves repository and RASTER_ARTIFACTS_SCOPE unset. Now currentRepo, requestedRepository, and requestedScope are all rasterstate/paragon, all three agree, no ConfigError is thrown, and the shared credential reads the victim's prefix:

// seed a victim artifact (with a valid sha256 digest, as real uploads produce)
seedArtifact(sharedStore, { repo: 'rasterstate/paragon', runId: '999', name: 'dist',
                            files: { 'secret.txt': 'VICTIM-SECRET' } });
run('src/download.js', makeEnv({
  store: sharedStore,
  repo: 'rasterstate/paragon',   // the only lie needed; GITHUB_REPOSITORY is job env
  runId: '999',
  inputs: { name: 'dist', path: dest },
}));

Result: exit 0, dest/secret.txt === VICTIM-SECRET. The action logs Repository: rasterstate/paragon run: 999 and extracts. (In a first pass the run stopped one step later on the unrelated #11 digest check because my seed had no digest; with the digest a real upload writes, it extracts cleanly.) So the scope check accepts the foreign scope and fetches it. This is the exact scenario #15 describes, and it is not stopped.

Because every input the check compares is drawn from the attacker's own job environment, this is a client-side check the attacker controls, not an access boundary. Per the task's probe 3: that is not a fix.

What the fix does and does not do

  • Removes repository's ability to select a foreign scope while GITHUB_REPOSITORY stays honest, and blocks the RASTER_ARTIFACTS_SCOPE override. Fine as hardening against an accidental misconfig.
  • Against the stated threat (a workflow author on a shared runner pool who controls their whole job env) it adds no protection: GITHUB_REPOSITORY is equally forgeable and is now the sole anchor.
  • The new copy is actively misleading: config.js:79-80 says scope "cannot be overridden by workflow-controlled env," and action.yml/README say cross-repo is "refused with shared backend credentials." GITHUB_REPOSITORY is workflow-controlled env, and the credential still grants the read.

Probe 2 (legit path) and probe 4 (tests)

  • Same-repo download still works: 35/35, incl. downloads a named artifact into the destination. Good.
  • The two added tests (repository input cannot select another artifact scope, RASTER_ARTIFACTS_SCOPE cannot override the repository scope) both hold GITHUB_REPOSITORY honest and only lie via the other knob. Neither covers the GITHUB_REPOSITORY-lie vector, i.e. the one that actually bypasses. The suite therefore gives false confidence: it asserts the boundary exactly where it holds and not where it breaks.

To actually close #15

The real fix is what the issue already calls for and this PR skips: a boundary the job cannot assert from its own env.

  • Mint per-repo (or per-run) scoped storage credentials from a trusted party, so the credential itself limits the reachable prefix; or
  • Verify a signed scope claim server-side, e.g. the Forgejo run/OIDC token's repository claim, rather than trusting process.env.GITHUB_REPOSITORY.

Until then, keep #15 open and reduce the claims: this prevents accidental cross-scope selection, it does not isolate a malicious repo on a shared credential. Add a test that sets GITHUB_REPOSITORY to a foreign value and asserts the read is refused, that is the case that must pass for #15 to be closed, and today it does not.

Verdict: CHANGES. Not an access boundary in its current form; do not merge as the #15 fix.

## Adversarial review: CHANGES (do not merge as the #15 access-boundary fix) I did not author this. Reviewed at `654c90b`, diffed vs `main`, against the #15 threat model (shared runner pool, one shared `RASTER_ARTIFACTS_S3_*` credential, scope values set inside the CI job). CI is green (`ci/test`, `test/unit`, `test/e2e` all pass) and the suite is 35/35 locally, but green CI here only proves the two weaker vectors are closed, not the real one. **The fix is client-side only, and the boundary is still bypassable.** It moves the anchor from "`repository` input / `RASTER_ARTIFACTS_SCOPE`" to "`GITHUB_REPOSITORY`" and rejects the first two when they disagree with the third (`config.js:65-82`). But `GITHUB_REPOSITORY` is itself job-controlled env, read straight from `process.env` (`config.js:48`) with no server-side or signed binding. The attacker just tells the same lie one level down. ### Proof (probe 1 + 3): the bypass succeeds Malicious job sets `GITHUB_REPOSITORY` to the victim, leaves `repository` and `RASTER_ARTIFACTS_SCOPE` unset. Now `currentRepo`, `requestedRepository`, and `requestedScope` are all `rasterstate/paragon`, all three agree, no `ConfigError` is thrown, and the shared credential reads the victim's prefix: ```js // seed a victim artifact (with a valid sha256 digest, as real uploads produce) seedArtifact(sharedStore, { repo: 'rasterstate/paragon', runId: '999', name: 'dist', files: { 'secret.txt': 'VICTIM-SECRET' } }); run('src/download.js', makeEnv({ store: sharedStore, repo: 'rasterstate/paragon', // the only lie needed; GITHUB_REPOSITORY is job env runId: '999', inputs: { name: 'dist', path: dest }, })); ``` Result: exit 0, `dest/secret.txt` === `VICTIM-SECRET`. The action logs `Repository: rasterstate/paragon run: 999` and extracts. (In a first pass the run stopped one step later on the unrelated `#11` digest check because my seed had no digest; with the digest a real upload writes, it extracts cleanly.) So the scope check accepts the foreign scope and fetches it. This is the exact scenario #15 describes, and it is not stopped. Because every input the check compares is drawn from the attacker's own job environment, this is a client-side check the attacker controls, not an access boundary. Per the task's probe 3: that is not a fix. ### What the fix does and does not do - Removes `repository`'s ability to select a foreign scope while `GITHUB_REPOSITORY` stays honest, and blocks the `RASTER_ARTIFACTS_SCOPE` override. Fine as hardening against an *accidental* misconfig. - Against the stated threat (a workflow author on a shared runner pool who controls their whole job env) it adds no protection: `GITHUB_REPOSITORY` is equally forgeable and is now the sole anchor. - The new copy is actively misleading: `config.js:79-80` says scope "cannot be overridden by workflow-controlled env," and `action.yml`/`README` say cross-repo is "refused with shared backend credentials." `GITHUB_REPOSITORY` *is* workflow-controlled env, and the credential still grants the read. ### Probe 2 (legit path) and probe 4 (tests) - Same-repo download still works: 35/35, incl. `downloads a named artifact into the destination`. Good. - The two added tests (`repository input cannot select another artifact scope`, `RASTER_ARTIFACTS_SCOPE cannot override the repository scope`) both hold `GITHUB_REPOSITORY` honest and only lie via the other knob. Neither covers the `GITHUB_REPOSITORY`-lie vector, i.e. the one that actually bypasses. The suite therefore gives false confidence: it asserts the boundary exactly where it holds and not where it breaks. ### To actually close #15 The real fix is what the issue already calls for and this PR skips: a boundary the job cannot assert from its own env. - Mint per-repo (or per-run) scoped storage credentials from a trusted party, so the credential itself limits the reachable prefix; or - Verify a signed scope claim server-side, e.g. the Forgejo run/OIDC token's `repository` claim, rather than trusting `process.env.GITHUB_REPOSITORY`. Until then, keep #15 open and reduce the claims: this prevents accidental cross-scope selection, it does not isolate a malicious repo on a shared credential. Add a test that sets `GITHUB_REPOSITORY` to a foreign value and asserts the read is refused, that is the case that must pass for #15 to be closed, and today it does not. Verdict: **CHANGES.** Not an access boundary in its current form; do not merge as the #15 fix.
stephen commented 2026-07-05 21:01:00 +00:00
Author
Owner
Copy link

Superseded. Two adversarial reviews proved cross-repo artifact isolation cannot be enforced client-side (the action runs inside the attacker-controlled job, so any env var or verification key it reads is forgeable - see the CHANGES on this PR and on #18). The real boundary must be store-side; tracked in #19 (and #17). Closing this client-side approach.

Superseded. Two adversarial reviews proved cross-repo artifact isolation cannot be enforced client-side (the action runs inside the attacker-controlled job, so any env var or verification key it reads is forgeable - see the CHANGES on this PR and on #18). The real boundary must be store-side; tracked in #19 (and #17). Closing this client-side approach.
stephen closed this pull request 2026-07-05 21:01:01 +00:00
All checks were successful
ci / test (pull_request) Successful in 7s
Details
test / unit (pull_request) Successful in 5s
Details
test / e2e (pull_request) Successful in 5s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
backlog
Accepted but not scheduled

  
blocked
Blocked or stalled waiting on external progress

  
converted
Converted into tracked work

  
opportunity
Potential product or business opportunity

  
p1
Priority 1

  
p2
Priority 2

  
p3
Priority 3

  
parked
Paused intentionally until later

No labels
backlog
blocked
converted
opportunity
p1
p2
p3
parked
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
rasterstate/download-artifact-action!16
No description provided.