fix: exclude job-output credential env vars from agent sandbox (--exclude-env)#46076
Conversation
…lude-env)
Extend ComputeAWFExcludeEnvVarNames to treat needs.*.outputs.* expressions
as credential-bearing (same as secrets.*), so env vars set from job outputs
(e.g. GH_TOKEN: ${{ needs.fetch_token.outputs.token }}) are excluded from
the agent container via AWF --exclude-env.
Closes #46012
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Extends AWF sandbox credential protection to job-output-derived environment variables.
Changes:
- Detects
needs.JOB.outputs.OUTPUTexpressions. - Excludes matching MCP, engine, and agent environment variables.
- Adds unit tests and a patch changeset.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/secret_extraction.go |
Adds job-output expression detection. |
pkg/workflow/secret_extraction_test.go |
Tests expression matching. |
pkg/workflow/awf_helpers.go |
Applies detection to sandbox exclusions. |
pkg/workflow/awf_helpers_test.go |
Tests environment-variable exclusions. |
.changeset/patch-exclude-job-output-credential-env-vars.md |
Documents the security fix. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Medium
| // jobOutputExprPattern matches a ${{ needs.JOB.outputs.OUTPUT }} expression anywhere in a string. | ||
| // Used to detect job-output credential references for --exclude-env, treating them | ||
| // the same as ${{ secrets.* }} references. | ||
| jobOutputExprPattern = regexp.MustCompile(`\$\{\{\s*needs\.[A-Za-z_][A-Za-z0-9_-]*\.outputs\.[A-Za-z_][A-Za-z0-9_-]*`) |
There was a problem hiding this comment.
Fixed in the latest commit. ContainsJobOutputExpr now uses InlineExpressionPattern to extract all ${{ ... }} blocks in the value, then searches within each expression body using two patterns:
jobOutputBodyDotPattern—\bneeds\.[A-Za-z_][A-Za-z0-9_-]*\.outputs\.[A-Za-z_][A-Za-z0-9_-]*— matches dot notation anywhere in the expression body, so${{ github.ref && needs.auth.outputs.token }}now returnstrue.jobOutputBodyBracketPattern—\bneeds\[['"][A-Za-z_][A-Za-z0-9_-]*['"]\](?:\.outputs\b|\[['"]outputs['"]\])— matches single- and double-quote bracket notation, so${{ needs['auth'].outputs['token'] }}and${{ needs['auth']['outputs']['token'] }}now returntrue.
Regression tests added for: sub-expression with &&, sub-expression with ||, bracket with single quotes, bracket with double quotes, full bracket notation (['JOB']['outputs']['OUTPUT']), and bracket notation after another token.
This comment has been minimized.
This comment has been minimized.
|
Hey The PR looks well-structured: the change is tightly scoped to the credential-detection path, the new
|
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Skills-Based Review
Applied /diagnosing-bugs — requesting changes for one broken test.
📋 Key Themes & Highlights
Issue
- Broken test case:
"job output embedded in a larger string"uses a value without${{, soContainsJobOutputExprreturnsfalsebutwant: true— the test will always fail.
Positive Highlights
- ✅ Clean, focused fix addressing a real security gap (ephemeral token leakage).
- ✅ Good table-driven test coverage across all three env sections (mcp-scripts, engine.env, agent.env).
- ✅ Regex correctly handles extra whitespace and hyphenated job names.
- ✅ Doc comment on
ComputeAWFExcludeEnvVarNamesis updated accurately.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 17.7 AIC · ⌖ 7.73 AIC · ⊞ 6.7K
Comment /matt to run again
🤖 PR Triage
Score breakdown: Impact 45 + Urgency 30 + Quality 16 Rationale: Security-sensitive fix — credential env vars from job outputs were not excluded from agent sandbox
|
|
@copilot add frontmatter field to specify additional envs to exclude. those get added to the awf excludes env list |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit Imported workflow
The |
|
@copilot please run the Unresolved review feedback:
|
…ainsJobOutputExpr
- Replace `jobOutputExprPattern` (anchored to `${{ needs.`) with two body
patterns checked within `InlineExpressionPattern` extractions:
* `jobOutputBodyDotPattern`: `\bneeds.JOB.outputs.OUTPUT` (dot notation,
anywhere in expression body)
* `jobOutputBodyBracketPattern`: `needs['JOB'].outputs` / `needs['JOB']['outputs']`
(single- and double-quote bracket notation)
- `ContainsJobOutputExpr` now iterates `${{ ... }}` blocks found by
`InlineExpressionPattern` and tests each body, matching the approach
used by `ExtractSecretsFromValue`
- Adds 7 new test cases covering sub-expressions (`github.ref && needs.*`),
bracket notation (single/double quote, full bracket), and embedded bracket
after another token
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot update main json schema with new field |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit |
|
🎉 This pull request is included in a new release. Release: |
Add excluded-env frontmatter field definition introduced in fix: exclude job-output credential env vars from agent sandbox (#46076). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add excluded-env frontmatter field definition introduced in fix: exclude job-output credential env vars from agent sandbox (#46076). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ComputeAWFExcludeEnvVarNamesonly detected${{ secrets.* }}as credential-bearing, so env vars set via job outputs (e.g.GH_TOKEN: ${{ needs.fetch_token.outputs.token }}) were never passed to--exclude-env, leaving ephemeral tokens fully visible inside the AWF container.Changes
secret_extraction.go— addsjobOutputExprPatternregex andContainsJobOutputExpr()helper that detects${{ needs.JOB.outputs.OUTPUT }}anywhere in a value stringawf_helpers.go— extends the credential check in all three env sections (mcp-scripts,engine.env,agent.env) to ORContainsJobOutputExpr()with the existingsecrets.check; updates doc comment onComputeAWFExcludeEnvVarNamesawf_helpers_test.goandsecret_extraction_test.gocovering job-output exclusion, static-value pass-through, and mixed secrets+outputsExample: the following
mcp-scriptsenv block now produces--exclude-env GH_TOKENin the compiled lock file, matching the behaviour ofGH_TOKEN: ${{ secrets.SOME_PAT }}: