Redact MCP gateway bearers from diagnostic artifacts - #50961
Conversation
|
Thanks for tackling this security issue! 🔒 The PR is properly framed and addresses a critical credential-exposure vulnerability (MCP gateway bearer tokens appearing in diagnostic logs). Since this is a draft by the core team working through the implementation plan, you're on the right track. Once you move out of draft, here are the key areas from the issue to keep in mind:
Looks good! Keep the PR description updated as you make progress.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (33 additions across 4 files). |
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
|
|
|
There was a problem hiding this comment.
Pull request overview
Ensures MCP gateway bearer credentials are scrubbed from diagnostic artifacts.
Changes:
- Always invokes secret redaction.
- Discovers bearer tokens across JSON and Codex TOML configurations.
- Tests redaction across diagnostic logs and secret-free workflows.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/redact_secrets.go |
Always generates the redaction action step. |
pkg/workflow/redact_secrets_test.go |
Covers workflows without declared secrets. |
actions/setup/js/redact_secrets.cjs |
Discovers gateway credentials from additional configurations. |
actions/setup/js/redact_secrets.test.cjs |
Covers Codex tokens and diagnostic log streams. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
| // Add environment variables | ||
| yaml.WriteString(" env:\n") | ||
|
|
||
| if len(secretReferences) > 0 { |
There was a problem hiding this comment.
Fixed in 8eea487 — the env: block is now emitted inside the len(secretReferences) > 0 guard, so the key is omitted entirely when a workflow declares no secrets. Hardened further in bfefcc2: the tests now parse the generated step as YAML and assert env is absent for secret-free workflows and is a populated mapping when secrets exist, so a null env is caught structurally rather than by substring matching.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Redact MCP gateway bearers from diagnostic artifacts
This PR correctly addresses the MCP bearer token leak in diagnostic artifacts. The approach — always running the redaction step, extracting tokens from multiple config formats (JSON + TOML), and covering all relevant log files — is sound.
One blocking bug found:
Empty env: block → invalid YAML
In pkg/workflow/redact_secrets.go, the refactor always emits:
env:...but only conditionally writes child entries (when secretReferences is non-empty). A bare env: key with no mappings is invalid YAML and will cause GitHub Actions to reject the generated workflow file for all workflows that declare no secrets.
The env: header write must be guarded by the same if len(secretReferences) > 0 condition that wraps its content (or moved inside that block).
The new test TestSecretRedactionRunsWithoutWorkflowSecrets doesn't catch this because it doesn't validate YAML structure.
Other observations (non-blocking)
- TOML regex scope (
redact_secrets.cjsline 121):/Authorization\s*=\s*"([^"]+)"/gis intentionally broad to cover Codex TOML headers — this is acceptable given the security goal, but worth documenting. - Test coverage for empty env block in Go: add an assertion in
TestSecretRedactionRunsWithoutWorkflowSecretsthat the output does not contain a bareenv:with no following indented key.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 28.8 AIC · ⊞ 5.3K
| yaml.WriteString(" await main();\n") | ||
|
|
||
| // Add environment variables | ||
| yaml.WriteString(" env:\n") |
There was a problem hiding this comment.
Bug: empty env: block generates invalid YAML when no secrets are declared
The env: key is always written (this line), but when secretReferences is empty the block has no child entries. GitHub Actions rejects a step with a bare env: key and no mappings — this will break all compiled workflows where no workflow secrets are declared.
Suggested fix — guard the env: header inside the secrets block:
if len(secretReferences) > 0 {
yaml.WriteString(" env:\n")
// ... existing env entries ...
}The new test TestSecretRedactionRunsWithoutWorkflowSecrets doesn't catch this because it only asserts the presence of the main() call, not that the generated YAML is structurally valid.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 8eea487 — the env: header is now written inside the len(secretReferences) > 0 guard, and TestSecretRedactionRunsWithoutWorkflowSecrets now asserts no env: block is emitted when there are no secrets.
There was a problem hiding this comment.
Follow-up on the test-adequacy point: bfefcc2 replaces the substring assertions with a real YAML parse of the generated step. TestSecretRedactionRunsWithoutWorkflowSecrets now asserts the env key is absent, and a new TestSecretRedactionEmitsEnvMappingWithWorkflowSecrets asserts env unmarshals as a populated mapping. I verified the test catches the original bug by temporarily reintroducing the unconditional env: write — it fails with got <nil>, confirming the null-mapping case is now detected structurally.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on one correctness issue and three test/robustness improvements.
📋 Key Themes & Highlights
Key Themes
- Empty
env:block (correctness): The refactored Go code always writesenv:but only populates it when secrets exist — generating invalid YAML for secret-free workflows (the very case this PR targets). - TOML regex case sensitivity: The
Authorizationkey regex is case-sensitive; lowercase variants used by some Codex versions would silently skip redaction. - Token length guard: The 6-char minimum applies to the full header value including the
Bearerprefix, not just the credential — very loose and may capture short noise values. - Test fixture realism: The TOML test doesn't exercise the
Bearer-prefix stripping path.
Positive Highlights
- ✅ Removing the
len(secretReferences) == 0early-exit is the right fix and is clearly motivated - ✅
Set-deduplication for config paths prevents duplicate token entries - ✅ Good regression test in
redact_secrets_test.goverifying the no-secrets path - ✅ Multi-log diagnostic test covers the full set of artifact files
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 39.1 AIC · ⊞ 7.1K
Comment /matt to run again
| yaml.WriteString(" await main();\n") | ||
|
|
||
| // Add environment variables | ||
| yaml.WriteString(" env:\n") |
There was a problem hiding this comment.
[/diagnosing-bugs] Empty env: block when no secrets are declared — this generates invalid YAML with a dangling env: key that has no child entries, causing the generated workflow to fail at runtime.
💡 Suggested fix
Only emit the env: block when there is content for it:
if len(secretReferences) > 0 {
yaml.WriteString(" env:\n")
// ... write secret env vars ...
}Right now env: is always written (line 167) but only populated when len(secretReferences) > 0 (line 169). A bare env: with no children is invalid YAML.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 8eea487 — the env: header is now emitted only when secret variables exist, with a test asserting the block is absent otherwise.
| try { | ||
| config = /** @type {Record<string, any>} */ JSON.parse(raw); | ||
| } catch { | ||
| for (const match of raw.matchAll(/\bAuthorization\s*=\s*"([^"]+)"/g)) { |
There was a problem hiding this comment.
[/tdd] The TOML Authorization regex is case-sensitive and only matches the key Authorization — real Codex TOML uses authorization (lower-case) in some versions. A test with authorization = "token" would currently fail silently.
💡 Suggested fix
Use a case-insensitive flag:
for (const match of raw.matchAll((bauthorization/redacted)\s*=\s*"([^"]+)"/gi)) {Also consider adding a test with a lowercase key to document the intended contract.
@copilot please address this.
There was a problem hiding this comment.
Fixed in c5ab33b — the TOML scan now uses the i flag, and a regression test covers a lowercase authorization key.
| tokens.add(auth); | ||
| if (/^[Bb]earer /.test(auth)) { | ||
| const tokenPart = auth.slice(7).trim(); | ||
| if (tokenPart.length >= 6) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The auth.length >= 6 guard is applied to the full Authorization header value (e.g. Bearer ghs_abc123). The 6-character minimum is very loose — a malformed or short value could still be added to tokens and trigger redaction of unintended strings in logs.
💡 Suggestion
Apply the minimum length check to the extracted tokenPart only, not the raw header value (which includes the Bearer prefix). And consider a slightly higher threshold (e.g. 16) consistent with real gateway token lengths to reduce false positives:
const auth = match[1].trim();
if (/^[Bb]earer /.test(auth)) {
const tokenPart = auth.slice(7).trim();
if (tokenPart.length >= 16) tokens.add(tokenPart);
} else if (auth.length >= 16) {
tokens.add(auth);
}@copilot please address this.
There was a problem hiding this comment.
Fixed in c5ab33b — both the JSON and TOML paths now share one helper that strips the bearer prefix first and applies the minimum-length guard to the credential itself, so a short value can no longer pass by being prefixed. I kept the threshold at 6 rather than raising it to 16, since that value is already the JSON path's contract and existing tests depend on it; raising it is a separate behavior change. A test covers a short bearer-prefixed value producing no tokens.
|
|
||
| it("should extract Authorization tokens from Codex TOML configuration", () => { | ||
| const configPath = path.join(tempDir, "config.toml"); | ||
| fs.writeFileSync(configPath, '[mcp_servers.github]\nhttp_headers = { Authorization = "codex-gateway-token-xyz789" }\n'); |
There was a problem hiding this comment.
[/tdd] The Codex TOML test uses http_headers = { Authorization = "codex-gateway-token-xyz789" } but the actual TOML format for Codex uses [tool.codex.mcp_servers.github] with http_headers.Authorization. Consider aligning the test fixture to a realistic Codex config shape to guard against format drift.
💡 Suggestion
A more realistic fixture:
[tool.codex.mcp_servers.github]
http_headers = { Authorization = "Bearer codex-gateway-token-xyz789" }This would also exercise the Bearer-prefix stripping path, verifying both the full Bearer <token> and bare <token> end up in the token set.
@copilot please address this.
There was a problem hiding this comment.
Fixed in c5ab33b — the fixture now matches the shape emitted by actions/setup/sh/convert_gateway_config_codex.sh ([mcp_servers.github] with url and an http_headers inline table) and uses a bearer-prefixed value, so the test asserts both the full header value and the bare token are extracted. Note Codex's generated config uses top-level [mcp_servers.*], not [tool.codex.mcp_servers.*].
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (10 tests)
Verdict
References: Run 31140894478
|
|
@copilot run pr-finisher skill |
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>
|
@copilot Please triage this PR, refresh the branch if possible, address the latest reviewer feedback, and run the Latest review signals:
Run context: https://github.com/github/gh-aw/actions/runs/31142787192
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
MCP gateway session bearers could persist in uploaded diagnostic logs. The bearer is runtime credential material and must not be retained in workflow artifacts.
Always run redaction
Discover gateway credentials
/tmp, runner temp, Copilot, Gemini, and Codex formats.Authorizationentries.Scrub diagnostic logs
github.log,mcp-gateway.log,safeoutputs.log, andstderr.log.Run context: https://github.com/github/gh-aw/actions/runs/31142787192> Generated by 👨🍳 PR Sous Chef · gpt54 · 11.8 AIC · ⊞ 8.3K · ◷