From e9ab013792ab053f6ac22f1ec76aeeb404136085 Mon Sep 17 00:00:00 2001 From: Chris Lee Date: Sat, 20 Jun 2026 21:43:41 +1000 Subject: [PATCH] fix(mcp): redact Octokit error tool-results and widen GitHub token regex Five GitHub-touching MCP servers and the shared state-fetchers fetcher returned raw Octokit error messages to the agent tool-result channel, leaking the installation token echoed in the credential URL and Bearer header (OWASP MCP01). Add config-free redactErrorMessage() and wire it into all six catch sites; delete the divergent merge-readiness inline regex. Also widen the canonical ghp_/gho_/ghs_/ghr_ token regexes in sanitize.ts from {36} to open-ended [A-Za-z0-9._-]{36,} so the new ~520-char ghs_APPID_JWT installation token (GitHub rollout 2026-04-27) is scrubbed. Closes #224 Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_015v2bspPF1ZTTG9ZZrbBgA1 --- src/github/state-fetchers.ts | 3 +- src/mcp/servers/comment.ts | 3 +- src/mcp/servers/github-state.ts | 3 +- src/mcp/servers/inline-comment.ts | 3 +- src/mcp/servers/merge-readiness.ts | 8 +-- src/mcp/servers/resolve-review-thread.ts | 3 +- src/utils/log-redaction.ts | 13 +++- src/utils/sanitize.ts | 33 +++++++--- test/mcp/error-redaction-wiring.test.ts | 45 +++++++++++++ test/utils/log-redaction.test.ts | 82 ++++++++++++++++++++++++ test/utils/sanitize.test.ts | 7 ++ 11 files changed, 181 insertions(+), 22 deletions(-) create mode 100644 test/mcp/error-redaction-wiring.test.ts create mode 100644 test/utils/log-redaction.test.ts diff --git a/src/github/state-fetchers.ts b/src/github/state-fetchers.ts index 5dd4149..59a4d53 100644 --- a/src/github/state-fetchers.ts +++ b/src/github/state-fetchers.ts @@ -14,6 +14,7 @@ import type { Octokit } from "octokit"; import type { LLMTool, LLMToolCall, LLMToolResult } from "../ai/llm-client"; import type { Logger } from "../logger"; +import { redactErrorMessage } from "../utils/log-redaction"; import { retryWithBackoff } from "../utils/retry"; import { PROBE_QUERY } from "./queries"; @@ -535,7 +536,7 @@ export async function dispatchGithubStateTool( }; } } catch (err) { - const message = err instanceof Error ? err.message : String(err); + const message = redactErrorMessage(err); return { content: JSON.stringify({ error: message }), isError: true }; } } diff --git a/src/mcp/servers/comment.ts b/src/mcp/servers/comment.ts index fdf40a8..0387b90 100644 --- a/src/mcp/servers/comment.ts +++ b/src/mcp/servers/comment.ts @@ -3,6 +3,7 @@ import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js" import { Octokit } from "octokit"; import { z } from "zod"; +import { redactErrorMessage } from "../../utils/log-redaction"; import { retryWithBackoff } from "../../utils/retry"; import { redactSecrets, sanitizeContent } from "../../utils/sanitize"; import { createMcpLogger } from "../mcp-logger"; @@ -125,7 +126,7 @@ server.tool( ], }; } catch (error) { - const errorMessage = error instanceof Error ? error.message : String(error); + const errorMessage = redactErrorMessage(error); return { content: [{ type: "text" as const, text: `Error: ${errorMessage}` }], isError: true, diff --git a/src/mcp/servers/github-state.ts b/src/mcp/servers/github-state.ts index 6317cc1..a9c0aab 100644 --- a/src/mcp/servers/github-state.ts +++ b/src/mcp/servers/github-state.ts @@ -12,6 +12,7 @@ import { getWorkflowRun, listPrComments, } from "../../github/state-fetchers"; +import { redactErrorMessage } from "../../utils/log-redaction"; import { createMcpLogger } from "../mcp-logger"; /** @@ -59,7 +60,7 @@ function ok(text: string): { content: { type: "text"; text: string }[] } { } function fail(err: unknown): { content: { type: "text"; text: string }[]; isError: true } { - const message = err instanceof Error ? err.message : String(err); + const message = redactErrorMessage(err); return { content: [{ type: "text" as const, text: JSON.stringify({ error: message }) }], isError: true, diff --git a/src/mcp/servers/inline-comment.ts b/src/mcp/servers/inline-comment.ts index 99e4ed2..3d9233f 100644 --- a/src/mcp/servers/inline-comment.ts +++ b/src/mcp/servers/inline-comment.ts @@ -3,6 +3,7 @@ import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js" import { Octokit } from "octokit"; import { z } from "zod"; +import { redactErrorMessage } from "../../utils/log-redaction"; import { retryWithBackoff } from "../../utils/retry"; import { redactSecrets, sanitizeContent } from "../../utils/sanitize"; import { createMcpLogger } from "../mcp-logger"; @@ -161,7 +162,7 @@ server.tool( ], }; } catch (error) { - const errorMessage = error instanceof Error ? error.message : String(error); + const errorMessage = redactErrorMessage(error); let helpMessage = ""; if (errorMessage.includes("Validation Failed")) { diff --git a/src/mcp/servers/merge-readiness.ts b/src/mcp/servers/merge-readiness.ts index 0da673c..c53130d 100644 --- a/src/mcp/servers/merge-readiness.ts +++ b/src/mcp/servers/merge-readiness.ts @@ -4,6 +4,7 @@ import { Octokit } from "octokit"; import { z } from "zod"; import { PROBE_QUERY } from "../../github/queries"; +import { redactErrorMessage } from "../../utils/log-redaction"; import { retryWithBackoff } from "../../utils/retry"; import { computeVerdict, type ProbeResponseShape } from "../../workflows/ship/verdict"; import { createMcpLogger } from "../mcp-logger"; @@ -55,12 +56,7 @@ function ok(text: string): { content: { type: "text"; text: string }[] } { } function fail(err: unknown): { content: { type: "text"; text: string }[]; isError: true } { - // Silently strip a token-shaped substring: an octokit GraphQL error can - // echo the request URL, which carries the installation token. Output-path - // redaction strips matched bytes rather than leaving a marker (a marker - // leaks probing signal to an attacker, CLAUDE.md security invariant 2). - const raw = err instanceof Error ? err.message : String(err); - const message = raw.replace(/gh[a-z]_[A-Za-z0-9_]{20,}|x-access-token:[^@\s]+/g, ""); + const message = redactErrorMessage(err); return { content: [{ type: "text" as const, text: JSON.stringify({ error: message }) }], isError: true, diff --git a/src/mcp/servers/resolve-review-thread.ts b/src/mcp/servers/resolve-review-thread.ts index 48f44e3..50508c6 100644 --- a/src/mcp/servers/resolve-review-thread.ts +++ b/src/mcp/servers/resolve-review-thread.ts @@ -19,6 +19,7 @@ import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js" import { Octokit } from "octokit"; import { z } from "zod"; +import { redactErrorMessage } from "../../utils/log-redaction"; import { retryWithBackoff } from "../../utils/retry"; import { createMcpLogger } from "../mcp-logger"; @@ -226,7 +227,7 @@ server.tool( }; } catch (err) { const code = classifyError(err); - const message = err instanceof Error ? err.message : String(err); + const message = redactErrorMessage(err); return { content: [ { diff --git a/src/utils/log-redaction.ts b/src/utils/log-redaction.ts index 8bcdb4f..92c5bdc 100644 --- a/src/utils/log-redaction.ts +++ b/src/utils/log-redaction.ts @@ -11,7 +11,7 @@ */ import pino, { type SerializedError } from "pino"; -import { redactGitHubTokens } from "./sanitize"; +import { redactGitHubTokens, redactSecrets } from "./sanitize"; const VALID_PINO_LEVELS = new Set(["trace", "debug", "info", "warn", "error", "fatal", "silent"]); @@ -106,6 +106,17 @@ function redactCredentialUrls(text: string): string { return text.replace(/\b([a-z][a-z0-9+\-.]*:\/\/)([^@/\s:]+):([^@/\s]+)@/gi, "$1***:***@"); } +/** + * Redact secrets from an error before it crosses the trust boundary into the + * agent's tool-result channel. Strips matched bytes with no marker (security + * invariant 2: a marker leaks probing signal). Covers every redactSecrets + * shape plus the x-access-token:@host credential URLs Octokit echoes. + */ +export function redactErrorMessage(err: unknown): string { + const raw = err instanceof Error ? err.message : String(err); + return redactSecrets(redactCredentialUrls(raw)).body; +} + /** Censor placeholder, matches pino's default so output is uniform. */ const CENSOR = "[Redacted]"; diff --git a/src/utils/sanitize.ts b/src/utils/sanitize.ts index 311fd60..3535d9c 100644 --- a/src/utils/sanitize.ts +++ b/src/utils/sanitize.ts @@ -86,14 +86,20 @@ export function stripHtmlComments(content: string): string { * attacker any probing feedback. */ export function redactGitHubTokens(content: string): string { + // ghp_/gho_/ghs_/ghr_ tokens: open-ended body, no trailing \b. The legacy + // shape was exactly 36 alphanumerics, but GitHub's stateless installation + // token (ghs_APPID_JWT, ~520 chars with dots/underscores; staged rollout from + // 2026-04-27) breaks any /ghs_[A-Za-z0-9]{36}/ assumption. Match the whole + // opaque run instead so the new format is scrubbed too. Over-matching is the + // safe direction for redaction. // GitHub Personal Access Tokens (classic) - content = content.replace(/\bghp_[A-Za-z0-9]{36}\b/g, "[REDACTED_GITHUB_TOKEN]"); + content = content.replace(/\bghp_[A-Za-z0-9._-]{36,}/g, "[REDACTED_GITHUB_TOKEN]"); // GitHub OAuth tokens - content = content.replace(/\bgho_[A-Za-z0-9]{36}\b/g, "[REDACTED_GITHUB_TOKEN]"); - // GitHub installation tokens - content = content.replace(/\bghs_[A-Za-z0-9]{36}\b/g, "[REDACTED_GITHUB_TOKEN]"); + content = content.replace(/\bgho_[A-Za-z0-9._-]{36,}/g, "[REDACTED_GITHUB_TOKEN]"); + // GitHub installation tokens (legacy 36-char body + new ghs_APPID_JWT stateless) + content = content.replace(/\bghs_[A-Za-z0-9._-]{36,}/g, "[REDACTED_GITHUB_TOKEN]"); // GitHub refresh tokens - content = content.replace(/\bghr_[A-Za-z0-9]{36}\b/g, "[REDACTED_GITHUB_TOKEN]"); + content = content.replace(/\bghr_[A-Za-z0-9._-]{36,}/g, "[REDACTED_GITHUB_TOKEN]"); // GitHub fine-grained personal access tokens content = content.replace(/\bgithub_pat_[A-Za-z0-9_]{11,221}\b/g, "[REDACTED_GITHUB_TOKEN]"); return content; @@ -122,11 +128,14 @@ interface SecretPattern { // Patterns are applied in fixed order. Most-specific first so a value that // would match multiple patterns is attributed to its true kind. const SECRET_PATTERNS: SecretPattern[] = [ - // GitHub tokens, same five formats as the input-side redactor. - { kind: "GITHUB_TOKEN", re: /\bghp_[A-Za-z0-9]{36}\b/g }, - { kind: "GITHUB_TOKEN", re: /\bgho_[A-Za-z0-9]{36}\b/g }, - { kind: "GITHUB_TOKEN", re: /\bghs_[A-Za-z0-9]{36}\b/g }, - { kind: "GITHUB_TOKEN", re: /\bghr_[A-Za-z0-9]{36}\b/g }, + // GitHub tokens, same five formats as the input-side redactor. Open-ended + // bodies (no trailing \b) so the new ghs_APPID_JWT stateless installation + // token (~520 chars with dots/underscores, GitHub rollout from 2026-04-27) + // is matched, not just the legacy 36-char shape. + { kind: "GITHUB_TOKEN", re: /\bghp_[A-Za-z0-9._-]{36,}/g }, + { kind: "GITHUB_TOKEN", re: /\bgho_[A-Za-z0-9._-]{36,}/g }, + { kind: "GITHUB_TOKEN", re: /\bghs_[A-Za-z0-9._-]{36,}/g }, + { kind: "GITHUB_TOKEN", re: /\bghr_[A-Za-z0-9._-]{36,}/g }, { kind: "GITHUB_TOKEN", re: /\bgithub_pat_[A-Za-z0-9_]{11,221}\b/g }, // Anthropic API keys and OAuth tokens. { kind: "ANTHROPIC_API_KEY", re: /\bsk-ant-api03-[A-Za-z0-9_-]{80,}\b/g }, @@ -149,6 +158,10 @@ const SECRET_PATTERNS: SecretPattern[] = [ kind: "DB_URL_WITH_PASSWORD", re: /\b(?:postgres|postgresql|redis|valkey|rediss|mongodb|mongodb\+srv|mysql):\/\/[^\s:@/]+:[^\s@/]+@[^\s)\]"'`<>]+/g, }, + // JWT must stay BELOW the GITHUB_TOKEN patterns: a new ghs_APPID_JWT stateless + // token embeds a JWT, and the ghs_ pattern above must claim the whole run + // first. Reordering JWT above would strip only the JWT portion and leak the + // ghs_APPID_ prefix bytes. // JSON Web Tokens. Three base64url segments separated by `.`. Each segment // requires ≥20 chars to keep us above the noise floor of arbitrary base64 // chunks that happen to start with `eyJ` (the literal `{"`). Real-world diff --git a/test/mcp/error-redaction-wiring.test.ts b/test/mcp/error-redaction-wiring.test.ts new file mode 100644 index 0000000..aab218c --- /dev/null +++ b/test/mcp/error-redaction-wiring.test.ts @@ -0,0 +1,45 @@ +import { readFileSync } from "node:fs"; +import { join } from "node:path"; + +import { describe, expect, it } from "bun:test"; + +// Source-assertion guard (non-brittle) that the six leak sites route Octokit +// error messages through `redactErrorMessage` before they reach the agent +// tool-result channel. import.meta.dir is test/mcp, so the repo root is two +// levels up. Assertions bind on the assignment form `= redactErrorMessage(` +// so an import line or a stray comment mentioning the helper cannot satisfy +// the check; only the actual catch-path usage does. +const repoRoot = join(import.meta.dir, "..", ".."); +const ASSIGN = "= redactErrorMessage("; + +function readSource(relPath: string): string { + return readFileSync(join(repoRoot, relPath), "utf8"); +} + +describe("error-redaction wiring", () => { + it("comment.ts routes errors through redactErrorMessage", () => { + expect(readSource("src/mcp/servers/comment.ts")).toContain(ASSIGN); + }); + + it("inline-comment.ts routes errors through redactErrorMessage", () => { + expect(readSource("src/mcp/servers/inline-comment.ts")).toContain(ASSIGN); + }); + + it("github-state.ts routes errors through redactErrorMessage", () => { + expect(readSource("src/mcp/servers/github-state.ts")).toContain(ASSIGN); + }); + + it("resolve-review-thread.ts routes errors through redactErrorMessage", () => { + expect(readSource("src/mcp/servers/resolve-review-thread.ts")).toContain(ASSIGN); + }); + + it("merge-readiness.ts uses redactErrorMessage and drops the inline regex", () => { + const src = readSource("src/mcp/servers/merge-readiness.ts"); + expect(src).toContain(ASSIGN); + expect(src).not.toContain("x-access-token:"); + }); + + it("state-fetchers.ts routes errors through redactErrorMessage", () => { + expect(readSource("src/github/state-fetchers.ts")).toContain(ASSIGN); + }); +}); diff --git a/test/utils/log-redaction.test.ts b/test/utils/log-redaction.test.ts new file mode 100644 index 0000000..66a951a --- /dev/null +++ b/test/utils/log-redaction.test.ts @@ -0,0 +1,82 @@ +import { describe, expect, it } from "bun:test"; + +import { redactErrorMessage } from "../../src/utils/log-redaction"; + +describe("redactErrorMessage", () => { + it("strips a ghs_ token and credentialed URL from an Octokit RequestError", () => { + // The authoritative red->green case: an Octokit error message echoes both a + // bare installation token and the credentialed clone/API URL that carries it. + const token = `ghs_${"a".repeat(36)}`; + const err = new Error( + `HttpError: request failed for https://x-access-token:${token}@api.github.com/repos/o/r/issues/1`, + ); + const out = redactErrorMessage(err); + expect(out).not.toContain("ghs_"); + expect(out).not.toContain(token); + }); + + it("strips the new ghs_APPID_JWT stateless installation token (~520 chars, dots + underscores)", () => { + // GitHub's 2026-04-27 rollout changes ghs_ tokens to ghs_APPID_JWT, far + // longer than 36 chars and containing underscores + JWT dots. The legacy + // /ghs_[A-Za-z0-9]{36}/ shape cannot match it. Cover the bare form, the + // Authorization: Bearer header form, and the credentialed-URL form. + const newToken = `ghs_1234567_eyJ${"a".repeat(200)}.${"b".repeat(160)}.${"c".repeat(140)}`; + expect(newToken.length).toBeGreaterThan(500); + + const bare = redactErrorMessage(new Error(`Bad credentials: ${newToken}`)); + expect(bare).not.toContain(newToken); + expect(bare).not.toContain("ghs_"); + + const bearer = redactErrorMessage( + new Error(`request failed Authorization: Bearer ${newToken}`), + ); + expect(bearer).not.toContain(newToken); + expect(bearer).not.toContain("ghs_"); + + const url = redactErrorMessage( + new Error(`clone failed https://x-access-token:${newToken}@github.com/o/r.git`), + ); + expect(url).not.toContain(newToken); + expect(url).not.toContain("ghs_"); + }); + + it("strips a github_pat_ fine-grained token (shape the old inline regex missed)", () => { + const token = `github_pat_${"a".repeat(40)}`; + const out = redactErrorMessage(new Error(`auth failed with ${token}`)); + expect(out).not.toContain(token); + expect(out).not.toContain("github_pat_"); + }); + + it("strips an Anthropic sk-ant-api03- key", () => { + const key = `sk-ant-api03-${"x".repeat(80)}`; + const out = redactErrorMessage(new Error(`bad key: ${key}`)); + expect(out).not.toContain(key); + expect(out).not.toContain("sk-ant-api03-"); + }); + + it("masks a DB URL password so the secret is absent", () => { + const out = redactErrorMessage( + new Error("connect failed: postgres://user:secret@db.local:5432/app"), + ); + expect(out).not.toContain("secret"); + }); + + it("passes a benign message through unchanged", () => { + const benign = "ENOENT: no such file or directory, open '/tmp/missing'"; + expect(redactErrorMessage(new Error(benign))).toBe(benign); + }); + + it("passes a benign non-Error input through unchanged", () => { + // Locks the String(err) branch against over-redaction of clean input. + expect(redactErrorMessage("plain string, no secrets here")).toBe( + "plain string, no secrets here", + ); + }); + + it("handles a non-Error input (plain string) without throwing and redacts a token", () => { + const token = `ghs_${"b".repeat(36)}`; + const out = redactErrorMessage(`raw string with ${token} inside`); + expect(out).not.toContain(token); + expect(out).not.toContain("ghs_"); + }); +}); diff --git a/test/utils/sanitize.test.ts b/test/utils/sanitize.test.ts index 6e4b5b6..47730c3 100644 --- a/test/utils/sanitize.test.ts +++ b/test/utils/sanitize.test.ts @@ -187,6 +187,13 @@ describe("redactGitHubTokens", () => { expect(redactGitHubTokens(token)).toBe("[REDACTED_GITHUB_TOKEN]"); }); + it("redacts the new ghs_APPID_JWT stateless installation token", () => { + // GitHub's 2026-04-27 rollout: ghs_ tokens become ghs_APPID_JWT (~520 + // chars, dots + underscores). The legacy {36} pattern cannot match it. + const token = `ghs_1234567_eyJ${"a".repeat(200)}.${"b".repeat(160)}.${"c".repeat(140)}`; + expect(redactGitHubTokens(`auth: ${token} done`)).toBe("auth: [REDACTED_GITHUB_TOKEN] done"); + }); + it("preserves non-token text", () => { expect(redactGitHubTokens("normal text")).toBe("normal text"); });