From b73f86039d7a6d365d998f7f1ceab0ba6c17047f Mon Sep 17 00:00:00 2001 From: oktofeesh1 <287075021+oktofeesh1@users.noreply.github.com> Date: Sat, 27 Jun 2026 21:48:20 -0700 Subject: [PATCH] feat(enrichment): secrets-in-logs & PII-egress scanner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a REES analyzer that flags added lines passing sensitive data into a logging or stdout sink — console.log(req.headers.authorization), logger.info(`token=${apiKey}`), console.log(req) — distinct from the hardcoded-secret scan (which inspects literal values; this inspects the data flow into a sink). Pure compute, no network. Precision-first: string-literal messages are blanked by a linear hand-scan before matching, so console.log("password reset") is not flagged; only a sensitive name used as code (property access, a ${…} interpolation, or a dumped request/session object) fires. Innocuous request scalars (req.method/url/path) are excluded. All matchers are flat, linear-time regexes. Closes #1507. --- review-enrichment/src/analyzers/secret-log.ts | 144 ++++++++++++++++++ review-enrichment/src/brief.ts | 2 + review-enrichment/src/render.ts | 18 +++ review-enrichment/src/types.ts | 10 ++ review-enrichment/test/enrichment.test.ts | 119 +++++++++++++++ 5 files changed, 293 insertions(+) create mode 100644 review-enrichment/src/analyzers/secret-log.ts diff --git a/review-enrichment/src/analyzers/secret-log.ts b/review-enrichment/src/analyzers/secret-log.ts new file mode 100644 index 0000000000..acdd42f3a4 --- /dev/null +++ b/review-enrichment/src/analyzers/secret-log.ts @@ -0,0 +1,144 @@ +// Secrets-in-logs / PII-egress analyzer (#1507). Flags added lines that pass sensitive data INTO a logging or +// stdout sink — `console.log(req.headers.authorization)`, `logger.info(`token=${apiKey}`)`, `console.log(req)` — +// distinct from the hardcoded-secret scan (which inspects literal VALUES; this inspects the data FLOW into a +// sink). Pure compute, no network. Precision-first: string-literal *messages* are stripped before matching, so +// `console.log("password reset")` is NOT flagged — a hit requires a sensitive name used as CODE (property access, +// a `${…}` interpolation, or a dumped request object). Line-cited via hunk headers, mirroring the other analyzers. +import type { EnrichRequest, SecretLogFinding } from "../types.js"; + +const MAX_FINDINGS = 25; // keep the brief bounded +const MAX_LINE_CHARS = 2000; // skip pathologically long lines (defensive) + +// All matchers below are FLAT alternations (no group is itself quantified), so each is linear-time — the analyzer +// can never be the DoS class it sits beside (#1503). Logging / stdout sinks: +const SINK_RE = + /\b(?:console|logger|log|winston|pino|bunyan)\s*\.\s*(?:log|info|warn|error|debug|trace|fatal|verbose|silly)\s*\(|\bprocess\s*\.\s*std(?:out|err)\s*\.\s*write\s*\(/; + +// Sensitive names, matched only against CODE (after string messages are stripped) so a hit means the value is +// actually referenced, not merely named in a log message. +const SECRET_RE = + /\b(?:passwords?|passwd|secret|api[_-]?key|access[_-]?token|refresh[_-]?token|id[_-]?token|client[_-]?secret|authorization|auth[_-]?token|bearer[_-]?token|private[_-]?key|credentials?|session[_-]?token|set[_-]?cookie)\b/i; +const PII_RE = + /\b(?:ssn|social[_-]?security(?:[_-]?number)?|credit[_-]?card(?:[_-]?number)?|card[_-]?number|cvv|cvc|passport[_-]?(?:no|number)?|tax[_-]?id|national[_-]?id|date[_-]?of[_-]?birth)\b/i; +// A whole request/session object — or one of its sensitive sub-objects (headers/body/cookies/session/auth) — +// dumped into the sink. Innocuous scalar fields (`req.method`, `req.url`, `req.path`) are deliberately excluded to +// keep the signal high, and the object must be referenced as code so `console.log("request received")` is not a hit. +const REQUEST_OBJECT_RE = + /\b(?:req|request)\s*(?:\)|\.\s*(?:headers|body|cookies|session|auth|rawheaders)\b)|\b(?:headers|session|cookies)\s*(?:\)|\.\s*[\w$])/i; + +/** Blank out string-literal MESSAGE content (keeping `${…}` interpolation bodies, which are real code) in a single + * linear pass — no regex, so it can never backtrack. Lets the matchers above run against code, not log prose. */ +export function codeOnly(s: string): string { + let out = ""; + let i = 0; + const n = s.length; + while (i < n) { + const c = s[i]!; + if (c === '"' || c === "'") { + i++; + while (i < n && s[i] !== c) { + if (s[i] === "\\") i++; + i++; + } + i++; // closing quote + out += " "; + continue; + } + if (c === "`") { + i++; + while (i < n && s[i] !== "`") { + if (s[i] === "\\") { + i += 2; + continue; + } + if (s[i] === "$" && s[i + 1] === "{") { + i += 2; + let depth = 1; + while (i < n && depth > 0) { + if (s[i] === "{") depth++; + else if (s[i] === "}") depth--; + if (depth > 0) out += s[i]; + i++; + } + continue; + } + i++; // ordinary template-literal char — drop it + } + i++; // closing backtick + out += " "; + continue; + } + out += c; + i++; + } + return out; +} + +function sinkLabel(match: string): string { + return match.replace(/\s+/g, "").replace(/\($/, ""); +} + +/** Classify one line: does it pass sensitive data into a log/stdout sink? Returns the sink + category, or null. */ +export function detectSecretLog( + line: string, +): { sink: string; category: SecretLogFinding["category"] } | null { + const m = SINK_RE.exec(line); + if (!m) return null; + const code = codeOnly(line.slice(m.index + m[0].length)); + if (SECRET_RE.test(code)) + return { sink: sinkLabel(m[0]), category: "secret" }; + if (PII_RE.test(code)) return { sink: sinkLabel(m[0]), category: "pii" }; + if (REQUEST_OBJECT_RE.test(code)) + return { sink: sinkLabel(m[0]), category: "request-object" }; + return null; +} + +/** Scan one file patch's added lines for sensitive-data-into-a-sink, line-cited via hunk headers. Pure. */ +export function scanPatchForSecretLog( + path: string, + patch: string, +): SecretLogFinding[] { + const findings: SecretLogFinding[] = []; + let newLine = 0; + for (const line of patch.split("\n")) { + if (line.startsWith("+++") || line.startsWith("---")) continue; + const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(line); + if (hunk) { + newLine = Number(hunk[1]); + continue; + } + if (line.startsWith("+")) { + const body = line.slice(1); + if (body.length <= MAX_LINE_CHARS) { + const hit = detectSecretLog(body); + if (hit) { + findings.push({ + file: path, + line: newLine, + sink: hit.sink, + category: hit.category, + }); + } + } + newLine++; + } else if (!line.startsWith("-")) { + newLine++; + } + } + return findings; +} + +/** Analyzer entrypoint: scan every changed file's added lines for secrets/PII reaching a log or stdout sink. */ +export async function scanSecretLog( + req: EnrichRequest, +): Promise { + const findings: SecretLogFinding[] = []; + for (const file of req.files ?? []) { + if (!file.patch) continue; + for (const finding of scanPatchForSecretLog(file.path, file.patch)) { + findings.push(finding); + if (findings.length >= MAX_FINDINGS) return findings; + } + } + return findings; +} diff --git a/review-enrichment/src/brief.ts b/review-enrichment/src/brief.ts index 4c3b483775..58c6a1fbc0 100644 --- a/review-enrichment/src/brief.ts +++ b/review-enrichment/src/brief.ts @@ -15,6 +15,7 @@ import { scanActionPins } from "./analyzers/actions-pin.js"; import { scanEol } from "./analyzers/eol-check.js"; import { scanRedos } from "./analyzers/redos.js"; import { scanCodeowners } from "./analyzers/codeowners.js"; +import { scanSecretLog } from "./analyzers/secret-log.js"; import { renderBrief } from "./render.js"; type AnalyzerFn = (req: EnrichRequest, signal: AbortSignal) => Promise; @@ -29,6 +30,7 @@ const ANALYZERS: Record = { eol: (req) => scanEol(req), redos: (req) => scanRedos(req), codeowners: (req, signal) => scanCodeowners(req, fetch, { signal }), + secretLog: (req) => scanSecretLog(req), }; function runWithTimeout( diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index 0d58263b76..5270f795e0 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -142,6 +142,24 @@ export function renderBrief( } } + const secretLogs = findings.secretLog ?? []; + if (secretLogs.length) { + lines.push( + "### Secrets / PII reaching a log or stdout sink (redact before merging)", + ); + for (const item of secretLogs) { + const what = + item.category === "secret" + ? "a secret/credential" + : item.category === "pii" + ? "PII" + : "a full request/session object"; + lines.push( + `- ${safeCodeSpan(`${item.file}:${item.line}`)} — ${safeCodeSpan(item.sink)} writes ${what} to a log/stdout sink; redact or remove`, + ); + } + } + if (!lines.length) return { promptSection: "", systemSuffix: "" }; const header = diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 004443463d..0136d320af 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -100,6 +100,15 @@ export interface CodeownersFinding { owners: string[]; // sorted owners from the last-matching CODEOWNERS rule; always non-empty } +/** An added line that passes sensitive data into a logging/stdout sink (a secret, PII, or a dumped request + * object). Reports the location + sink + category only — never the logged value. */ +export interface SecretLogFinding { + file: string; + line: number; + sink: string; + category: "secret" | "pii" | "request-object"; +} + /** Structured analyzer output. Each analyzer fills its own key; more land as analyzers ship (#1477/#1478). */ export interface BriefFindings { dependency?: DependencyFinding[]; @@ -110,6 +119,7 @@ export interface BriefFindings { eol?: EolFinding[]; redos?: RedosFinding[]; codeowners?: CodeownersFinding[]; + secretLog?: SecretLogFinding[]; } export type AnalyzerStatus = "ok" | "degraded" | "skipped"; diff --git a/review-enrichment/test/enrichment.test.ts b/review-enrichment/test/enrichment.test.ts index 6f189d1143..e313f93d4b 100644 --- a/review-enrichment/test/enrichment.test.ts +++ b/review-enrichment/test/enrichment.test.ts @@ -21,6 +21,12 @@ import { scanPatchForRedos, scanRedos, } from "../dist/analyzers/redos.js"; +import { + codeOnly, + detectSecretLog, + scanPatchForSecretLog, + scanSecretLog, +} from "../dist/analyzers/secret-log.js"; const NOW = new Date("2026-06-26").getTime(); const eolFetch = @@ -937,3 +943,116 @@ test("buildBrief: eol analyzer runs (real now, 2023 cycle is past)", async () => globalThis.fetch = realFetch; } }); + +test("codeOnly: blanks string messages, keeps ${...} interpolation bodies", () => { + assert.equal(codeOnly('"a secret here"'), " "); + assert.equal(codeOnly("'plain'"), " "); + assert.ok(codeOnly("`x=${apiKey}`").includes("apiKey")); + assert.ok(!codeOnly("`logging the password now`").includes("password")); + assert.equal( + codeOnly("req.headers.authorization"), + "req.headers.authorization", + ); +}); + +test("detectSecretLog: flags sensitive data into a sink as CODE, not string messages", () => { + assert.equal( + detectSecretLog("console.log(req.headers.authorization);")?.category, + "secret", + ); + assert.equal( + detectSecretLog("logger.info(`token=${apiKey}`);")?.category, + "secret", + ); + assert.equal(detectSecretLog("log.error(user.password);")?.category, "secret"); + assert.equal(detectSecretLog("console.debug(account.ssn);")?.category, "pii"); + assert.equal(detectSecretLog("console.log(req);")?.category, "request-object"); + assert.equal( + detectSecretLog("process.stdout.write(session.cookie);")?.sink, + "process.stdout.write", + ); + // NOT flagged — sensitive word only in a string message, no sink, or a benign interpolation: + assert.equal( + detectSecretLog('console.log("password reset email sent");'), + null, + ); + assert.equal(detectSecretLog('logger.info("request received");'), null); + assert.equal(detectSecretLog("const token = readToken();"), null); + assert.equal(detectSecretLog("logger.info(`user ${id} signed in`);"), null); + assert.equal(detectSecretLog("console.error(error);"), null); + // innocuous request scalars are NOT dumps: + assert.equal(detectSecretLog("console.log(req.method, req.url);"), null); + assert.equal(detectSecretLog("console.log(req.path);"), null); + // but a whole request or a sensitive sub-object IS: + assert.equal( + detectSecretLog("console.log(req.body);")?.category, + "request-object", + ); +}); + +test("scanPatchForSecretLog: line-cited via hunk header; ignores context + safe lines", () => { + const patch = [ + "@@ -1,1 +1,4 @@", + " const ok = true;", + "+console.log(req.headers.authorization);", + '+console.log("user signed in");', + "+logger.info(`ssn=${user.ssn}`);", + ].join("\n"); + const findings = scanPatchForSecretLog("src/a.ts", patch); + assert.deepEqual( + findings.map(({ file, line, category }) => ({ file, line, category })), + [ + { file: "src/a.ts", line: 2, category: "secret" }, + { file: "src/a.ts", line: 4, category: "pii" }, + ], + ); + assert.equal(findings[0].sink, "console.log"); +}); + +test("scanSecretLog: scans every changed file's added lines, caps to its budget", async () => { + const findings = await scanSecretLog({ + repoFullName: "o/r", + prNumber: 1, + files: [ + { path: "a.ts", patch: "@@ -1,0 +1,1 @@\n+console.log(user.password);" }, + { path: "b.ts", patch: "@@ -1,0 +1,1 @@\n+console.log('hello world');" }, + { path: "c.md", patch: undefined }, + ], + }); + assert.equal(findings.length, 1); + assert.equal(findings[0].file, "a.ts"); +}); + +test("renderBrief: renders the secret-log block, code-spanning + sanitizing", () => { + const r = renderBrief({ + secretLog: [ + { file: "src/a.ts", line: 9, sink: "console.log", category: "secret" }, + ], + }); + assert.match(r.promptSection, /Secrets \/ PII reaching a log/); + assert.match(r.promptSection, /`src\/a\.ts:9`/); + assert.match(r.promptSection, /`console\.log`/); + assert.match(r.promptSection, /a secret\/credential/); +}); + +test("buildBrief: secret-log analyzer runs (pure, no network)", async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = async () => ({ ok: true, json: async () => ({}) }); + try { + const brief = await buildBrief({ + repoFullName: "o/r", + prNumber: 1, + files: [ + { + path: "src/a.ts", + patch: "@@ -1,0 +1,1 @@\n+console.log(req.headers.authorization);", + }, + ], + }); + assert.equal(brief.analyzerStatus.secretLog, "ok"); + assert.equal(brief.findings.secretLog.length, 1); + assert.match(brief.promptSection, /Secrets \/ PII reaching a log/); + } finally { + globalThis.fetch = realFetch; + } +});