security(mcp): GitHub MCP servers return raw Octokit error messages without token redaction

[chrisleekr]

Finding

The five src/mcp/servers/*.ts GitHub-touching MCP servers all return raw Octokit error messages to the agent's tool-result channel, and four of them apply no secret redaction before doing so. The fifth (merge-readiness.ts) does redact, but with a hand-rolled inline regex that has already drifted from the canonical helper used elsewhere in the codebase. The leak vector is real: Octokit RequestError.message (and err.request) routinely echoes the request URL, which carries the ghs_… installation token as https://x-access-token:<token>@api.github.com/... for git operations, and the Authorization: Bearer ghs_… header is attached to err.request.headers on every REST/GraphQL failure path (see external references). The merge-readiness server already calls out this concern in a code comment ("an octokit GraphQL error can echo the request URL, which carries the installation token") but the same code does not exist in the four sibling servers.

Verified line-accurate callsites in this repo:

• src/mcp/servers/comment.ts:127-133 — catch returns `Error: ${errorMessage}` with no redaction.
• src/mcp/servers/inline-comment.ts:162-180 — catch returns `Error creating inline comment: ${errorMessage}${helpMessage}` with no redaction.
• src/mcp/servers/github-state.ts:61-67 — fail() returns JSON.stringify({ error: message }) with no redaction; reachable from all seven read-only tools.
• src/mcp/servers/resolve-review-thread.ts:227-240 — catch returns JSON.stringify({ code, message, thread_id, pr_number }) with no redaction.
• src/mcp/servers/merge-readiness.ts:57-68 — uses a divergent inline regex /gh[a-z]_[A-Za-z0-9_]{20,}|x-access-token:[^@\s]+/g. This regex misses three canonical token shapes the central helper covers: github_pat_…, Anthropic sk-ant-api03-…, and Anthropic OAuth sk-ant-oat##-…, plus database-URL credentials and PEM blocks.

A canonical helper already exists at src/utils/sanitize.ts:88 — redactGitHubTokens() matches all five GitHub token shapes (ghp_, gho_, ghs_, ghr_, github_pat_) and is the same regex the log redactor at src/utils/log-redaction.ts:114 calls through scrubString on the err.message / err.stack / err.request.headers paths of every err log entry. The MCP servers mcp-logger.ts already imports errSerializer from the same file (src/mcp/mcp-logger.ts:17), so the log path is covered — only the tool-result path leaks.

The downstream output guard at src/utils/github-output-guard.ts:79 plus the regex pass in src/utils/sanitize.ts redactSecrets() will catch a token if the agent re-quotes it into a tracking-comment update, but defense-in-depth requires stripping at the source: redaction should run as the error crosses the trust boundary out of Octokit, not seven hops downstream. The CLAUDE.md security invariant 2 makes this explicit ("Apply to every body about to be posted to GitHub") and the doc comment in merge-readiness.ts already encodes the right pattern; this issue is the gap that prevents it from being uniformly applied.

Diagram

flowchart TD
  GHE[GitHub API error<br/>URL with x-access-token and ghs token]:::ext
  OCT[Octokit RequestError<br/>err.message echoes the request URL]:::oct

  C1[comment.ts L127-L133<br/>raw error.message]:::bad
  C2[inline-comment.ts L162-L180<br/>raw errorMessage]:::bad
  C3[github-state.ts L61-L67<br/>raw err.message]:::bad
  C4[resolve-review-thread.ts L227-L240<br/>raw err.message]:::bad
  C5[merge-readiness.ts L57-L68<br/>divergent hand-rolled regex]:::warn

  AGT[Claude Agent SDK<br/>raw token enters model context]:::agent

  CANON[utils/sanitize.ts L88<br/>redactGitHubTokens canonical helper<br/>matches ghp gho ghs ghr github_pat]:::good

  GHE --> OCT
  OCT --> C1
  OCT --> C2
  OCT --> C3
  OCT --> C4
  OCT --> C5
  C1 --> AGT
  C2 --> AGT
  C3 --> AGT
  C4 --> AGT
  C5 -. partial coverage .-> AGT
  CANON -. should wire into all 5 .-> C1
  CANON -. should wire into all 5 .-> C2
  CANON -. should wire into all 5 .-> C3
  CANON -. should wire into all 5 .-> C4
  CANON -. should replace inline regex .-> C5

  classDef ext fill:#7d2105;color:#ffffff
  classDef oct fill:#1a5276;color:#ffffff
  classDef bad fill:#922b21;color:#ffffff
  classDef warn fill:#b9770e;color:#ffffff
  classDef agent fill:#5b2c6f;color:#ffffff
  classDef good fill:#196f3d;color:#ffffff

Loading

Rationale

This is an MCP-layer instance of OWASP's freshly-published MCP01:2025 — Token Mismanagement and Secret Exposure (owasp.org). The exposure pathway is "Memory Retention — Models retain conversational data containing credentials across sessions": once a token bytes-string crosses into the agent's transcript via a tool result, the SDK can re-emit it in later turns (subsequent tool calls, prompt-cached transcript replay, error-recovery messages) until the conversation ends.

The blast radius is bounded but non-trivial:

1. GitHub installation tokens (ghs_…, 1-hour TTL) — the runtime credential for every MCP-mediated GitHub write. Exposure inside the agent's transcript means any Bash tool call the agent issues afterwards can carry it (e.g. echo "$token" > out.log, a git commit -m with the token text, a stray console.log in an Edit), each of which is a side channel that does NOT go through safePostToGitHub's regex pass.
2. GitHub Actions ecosystem precedent: the same vector caused actions/checkout #162 and follow-on advisories — installation tokens echoed in error output during transient API failures. The pattern is well-known.
3. Code-comment drift: the existence of the merge-readiness inline regex demonstrates the repo already knew about the risk in one server but did not generalise the fix; this is exactly the "audit unredacted logging of prompts/responses" detection signal OWASP MCP01 calls out. Centralising the redactor closes the drift surface permanently.
4. Cost: each call site is a one-line wrap (redactGitHubTokens(rawMessage)); no schema change, no behaviour change in the happy path, no new dependency. The existing test surface at test/utils/sanitize.test.ts:164 already covers the canonical helper.

References

Internal:

• src/mcp/servers/comment.ts:127-133 — unredacted catch in update_claude_comment.
• src/mcp/servers/inline-comment.ts:162-180 — unredacted catch in create_inline_comment.
• src/mcp/servers/github-state.ts:61-67 — unredacted fail() shared across 7 read-only tools.
• src/mcp/servers/resolve-review-thread.ts:227-240 — unredacted catch in resolve_review_thread.
• src/mcp/servers/merge-readiness.ts:57-68 — divergent inline regex that misses github_pat_, Anthropic, AWS, JWT, and DB-URL patterns.
• src/utils/sanitize.ts:88 — redactGitHubTokens() canonical helper (all five GitHub token shapes).
• src/utils/log-redaction.ts:114 — same helper applied to log error serialiser (scrubString over err.message/err.stack/headers).
• test/utils/sanitize.test.ts:164 — existing test surface for the canonical helper.
• CLAUDE.md Security invariant 2 (Output secret-strip chokepoint) — establishes the pattern that every GitHub-bound write redact at its boundary.

External:

• OWASP MCP Top 10 — MCP01:2025 Token Mismanagement & Secret Exposure — names "unredacted logging of prompts/responses" as a detection indicator and lists redaction before logging as an immediate mitigation.
• GitHub: Securing your authentication credentials — installation access token format — documents the ghs_… installation-token shape and credentialed URL format (https://x-access-token:<token>@…) embedded in git remotes and request URLs.
• Octokit GraphQL.js — error.request property — documents that error.request "contains request options such as query, variables and headers set for easier debugging", confirming the leak vector for error-path callers.

Suggested Next Steps

1. Add a shared MCP error helper at src/mcp/servers/_redact.ts (or extend src/mcp/mcp-logger.ts) exporting redactErrorMessage(err: unknown): string that calls redactGitHubTokens() plus the redactCredentialUrls() regex already in src/utils/log-redaction.ts:105 (extract or re-export it). One function, one regex set.
2. Wire it into all five servers at the verified callsites: comment.ts:127-133, inline-comment.ts:162-180, github-state.ts:61-67 (in fail()), resolve-review-thread.ts:229, merge-readiness.ts:57-68 (replacing the inline regex). Each site is a one-line change.
3. Mirror the same redaction in the shared fetcher path at src/github/state-fetchers.ts:537-540 (dispatchGithubStateTool's catch) so the inline runWithTools consumers (chat-thread, triage) get the same coverage as the MCP path.
4. Add a test in test/mcp/ that drives each server's catch path with a synthetic Octokit RequestError whose message contains a ghs_ token and asserts the returned tool-result body has it stripped. The test surface in test/utils/sanitize.test.ts:164 is the template.
5. Delete the merge-readiness inline regex comment + code once the shared helper lands; replace with redactErrorMessage(err) for parity. Avoids the divergence detector at scripts/check-docs-citations.ts flagging the obsolete line citation in CLAUDE.md later.

Areas Evaluated

This run explored the MCP focus area: src/mcp/registry.ts, all six servers in src/mcp/servers/, the shared fetcher path in src/github/state-fetchers.ts, the MCP logger at src/mcp/mcp-logger.ts, the canonical sanitisation helpers in src/utils/sanitize.ts + src/utils/log-redaction.ts, the output guard at src/utils/github-output-guard.ts, and the retry helper at src/utils/retry.ts. Cross-checked against the closed MCP issue ("fix(mcp): 4 of 5 GitHub-touching MCP servers + 7 state-fetchers bypass retryWithBackoff") — that fix addressed retry coverage and is orthogonal to error-message redaction.

Generated by the scheduled research action on 2026-06-10