diff --git a/docs/redaction.md b/docs/redaction.md index e9eacef..7cd5c34 100644 --- a/docs/redaction.md +++ b/docs/redaction.md @@ -29,8 +29,35 @@ pass through untouched. What it catches: known token formats (OpenAI, Anthropic, GitHub, AWS, Slack, JWTs), PEM private key blocks, `Authorization` headers, URL credentials (`postgres://user:secret@host`), assignments to sensitive-looking keys -(`password`, `*_TOKEN`, `api_key`, ...), and — depending on the mode below — -emails, phone numbers, SSNs, and credit card numbers. +(`password`, `pwd`, `*_TOKEN`, `api_key`, ...), and — depending on the mode +below — emails, phone numbers, SSNs, and credit card numbers. + +### Shell expansions + +A shell expansion reads a variable; it doesn't assign one. `${PWD:t}`, +`${PASSWORD:0:3}`, `${TOKEN:?unset}`, and `$TOKEN:foo` are left alone, even +though the name looks sensitive and the `:` looks like `key: value`. So is an +assignment whose whole value names another variable (`$GH_TOKEN`, +`"${GH_TOKEN}"`), and the command name of an unquoted command, as in +`export GITHUB_TOKEN=$(gh auth token)`. Replacing either would leave a line +that no longer parses. The command's arguments still go through the other +patterns, but a bare literal argument (`$(printf %s hunter2)`) isn't +recognized as a secret, so keep secrets out of them. A quoted command +(`"$(gh auth token)"`) is still redacted whole, since `""` parses. + +The one part of an expansion that can hold a literal secret is the word of a +default, assign, or alternate (`:-`, `:=`, `:+`, and the forms without `:`). +Under a sensitive name, that word is replaced and the braces kept: + +``` +local file: psql "${DB_PASSWORD:-hunter2}" +pushed file: psql "${DB_PASSWORD:-}" +``` + +A word that is itself a reference (`${DB_PASSWORD:-$FALLBACK}`) passes +through. `$PWD` and `$OLDPWD` are the shell's working directory, so their +defaults always pass through. Outside an expansion, `PWD` is still a password +key: ODBC connection strings write `UID=sa;PWD=...;`. ## Reviewing what gets redacted @@ -43,6 +70,10 @@ Pattern-based detection has false positives, so redaction is never silent: files, so it is always exact and current. - A `redaction` event with counts (never content) lands in the machine log (`sidecar tail`). +- On a peer with [`review = "required"`](review.md), the review packet lists + every added line that carries a placeholder. The reviewer is asked to reject + any placeholder that replaced something other than a credential, or that + leaves the file broken. ## Modes @@ -59,9 +90,9 @@ Comments and in-file markers do not override redaction policy. Changing the mode re-runs every tracked file through the filter on the next sync, so the whole tree converges to the new mode — `none → secrets+pii` re-redacts everything, `secrets+pii → none` re-commits your originals. -Upgrading from a version with an in-file redaction bypass also reprocesses -tracked files on the next snapshot, including files whose contents did not -change. +Upgrading to a version whose filter behaves differently (the in-file bypass +removal, or the shell expansion fix) also reprocesses tracked files on the +next snapshot, including files whose contents did not change. The mode governs your files. Two things sidecar generates about itself are always redacted at the `secrets` level whatever the mode says: the machine-level @@ -70,6 +101,40 @@ log (`sidecar tail`) and the failure messages in a quotes remote URLs — which carry tokens. Turning redaction off is a decision about your notes, not an instruction to publish your own credentials. +## False positives break other machines, not yours + +A false positive is more than a cosmetic placeholder. The machine that wrote +the file keeps its original, so its author sees nothing wrong. Every other +machine receives the published blob, placeholder included, as its working +copy, so the file is damaged there: + +``` +on the author's machine: + [[ -n "$sub" && "${PWD:t}" != "$sub" ]] && where+=" %F{cyan}${${PWD:t}//\%/%%}%f" +pushed, and so on every other machine: + [[ -n "$sub" && "${PWD:" != "$sub" ]] && where+=" %F{cyan}${${PWD:" +``` + +That was a real incident in a dotfiles sidecar (before the +[shell expansion](#shell-expansions) fix): the line stopped parsing, so every +interactive shell on the other machines errored out. A review approved it, +because the reviewer saw `` and took it for redaction doing its job. +The same is true of real secrets in shell files: `export GITHUB_TOKEN=` +doesn't parse either. + +To limit the damage: + +- Run `sidecar redactions` after adding or editing config, scripts, or + dotfiles. It shows exactly what other machines will get. +- When a redaction is wrong, rewrite the line, or turn redaction off for that + path with a [rule](rules.md) (`redaction = "none"`). Only do that for + content you would publish verbatim. Fix it on the author's machine: once it + publishes the corrected file, the others get it on their next sync, and + editing a damaged copy elsewhere only adds a competing version. +- Upgrade every machine together after a redaction fix. An older filter on + another machine still applies the old patterns when it commits the file + again. + ## Limits worth knowing - **Redaction is one-way, and multi-machine syncs deliver redacted text.** diff --git a/docs/review.md b/docs/review.md index 8591674..2198a1e 100644 --- a/docs/review.md +++ b/docs/review.md @@ -67,8 +67,12 @@ binary, text that isn't UTF-8, or one whose attributes turn diffs off — has its complete new content under `opaque/`, and the instructions list each one: nothing publishes that isn't in the packet. The diff is of committed content, so it is already redacted: secrets redaction caught appear as `` -placeholders, never as values. Packets live in sidecar's state dir and only -your user can read them. +placeholders, never as values. The instructions list every added line that +carries a placeholder and ask the reviewer to reject any placeholder that +replaced something other than a credential, or that leaves the file broken: +a redaction false positive replaces that text on every other machine +([redaction](redaction.md#false-positives-break-other-machines-not-yours)). +Packets live in sidecar's state dir and only your user can read them. ## Deciding @@ -236,7 +240,8 @@ changes no file and is never pushed. reviewer can be wrong in either direction. - **The reviewer sees the redacted diff, not your working tree.** That keeps caught secrets away from it. It also means what redaction rewrote shows up - as placeholders. + only as placeholders: the reviewer can judge where each one landed and + whether the line still makes sense, but never sees what it replaced. - **Turning review off publishes local history.** A clone a gated peer has used remembers it, and sync refuses until you confirm with `git -C config --unset sidecar.review`. The next sync then pushes diff --git a/packages/sidecar/dist/cli.js b/packages/sidecar/dist/cli.js index 78cdb3c..0c97fbd 100755 --- a/packages/sidecar/dist/cli.js +++ b/packages/sidecar/dist/cli.js @@ -1449,15 +1449,142 @@ var init_git = __esm(() => { }); // src/redaction.ts +function redactShellDefaults(text) { + let output = ""; + let copied = 0; + for (const match of text.matchAll(SHELL_DEFAULT_REGEX)) { + const [whole, open, name, operator] = match; + if (match.index < copied) + continue; + if (SHELL_DIRECTORY_NAMES.has(name) || !isSensitiveKey(name)) + continue; + const wordStart = match.index + whole.length; + const end = shellWordEnd(text, wordStart, "}"); + const wordEnd = end === -1 ? lineEnd(text, wordStart) : end; + const word = text.slice(wordStart, wordEnd); + output += text.slice(copied, wordStart); + output += !word || isPlaceholder(word) || isShellReference(word) ? word : placeholderForKey(name); + copied = wordEnd; + } + return output + text.slice(copied); +} +function shellWordEnd(text, start, close, depth = 0) { + if (depth > SHELL_NESTING_LIMIT) + return -1; + let parens = 0; + for (let index = start;index < text.length; index += 1) { + const char = text[index]; + if (char === ` +` || char === "\r") + return -1; + if (char === "\\") { + index += 1; + } else if (char === "'" || char === '"' || char === "`") { + index = closingQuote(text, index + 1, char, depth + 1); + if (index === -1) + return -1; + } else if (char === "$" && (text[index + 1] === "{" || text[index + 1] === "(")) { + index = shellWordEnd(text, index + 2, text[index + 1] === "{" ? "}" : ")", depth + 1); + if (index === -1) + return -1; + } else if (close === ")" && char === "(") { + parens += 1; + } else if (close === ")" && char === ")" && parens > 0) { + parens -= 1; + } else if (char === close) { + return index; + } + } + return -1; +} +function closingQuote(text, start, quote, depth) { + if (depth > SHELL_NESTING_LIMIT) + return -1; + for (let index = start;index < text.length; index += 1) { + const char = text[index]; + if (char === ` +` || char === "\r") + return -1; + if (char === quote) + return index; + if (quote === "'") + continue; + if (char === "\\") { + index += 1; + } else if (quote === '"' && char === "$" && (text[index + 1] === "{" || text[index + 1] === "(")) { + index = shellWordEnd(text, index + 2, text[index + 1] === "{" ? "}" : ")", depth + 1); + if (index === -1) + return -1; + } else if (quote === '"' && char === "`") { + index = closingQuote(text, index + 1, "`", depth + 1); + if (index === -1) + return -1; + } + } + return -1; +} +function lineEnd(text, start) { + const match = /[\r\n]/.exec(text.slice(start)); + return match ? start + match.index : text.length; +} +function isShellReadOfKey(text, offset, separator) { + if (SHELL_EXPANSION_KEY_PREFIX.test(text.slice(Math.max(0, offset - 64), offset))) + return true; + return text[offset - 1] === "$" && separator.trim().startsWith(":"); +} +function isShellReference(value, depth = 0) { + if (depth > SHELL_NESTING_LIMIT) + return false; + const unquoted = value.length > 1 && value.startsWith('"') && value.endsWith('"') ? value.slice(1, -1) : value; + if (SHELL_VARIABLE_REFERENCE.test(unquoted)) + return true; + if (!unquoted.startsWith("${") || shellWordEnd(unquoted, 2, "}") !== unquoted.length - 1) + return false; + const rest = SHELL_EXPANSION_INNER.exec(unquoted.slice(2, -1))?.groups?.rest; + if (rest === undefined) + return false; + if (rest === "" || /^:?\?/.test(rest) || /^(?::[a-zA-Z0-9]+)+$/.test(rest)) + return true; + const word = /^:?[-=+](?.*)$/s.exec(rest)?.groups?.word; + return word !== undefined && (word === "" || isPlaceholder(word) || isShellReference(word, depth + 1)); +} +function startsWithShellReference(text, start, value) { + if (SHELL_COMMAND_NAME_VALUE.test(value)) + return true; + if (text[start] !== "$") + return false; + let end; + if (text[start + 1] === "{") { + end = shellWordEnd(text, start + 2, "}"); + if (end === -1) + return false; + end += 1; + } else { + end = start + 1 + (/^[A-Za-z0-9_]*/.exec(text.slice(start + 1))?.[0].length ?? 0); + } + if (end < text.length && !/[\s;&|)]/.test(text[end])) + return false; + return isShellReference(text.slice(start, end)); +} function redactText(input, mode = DEFAULT_REDACTION_MODE) { if (mode === "none") return input; - let output = input.replace(PEM_PRIVATE_KEY_REGEX, "").replace(AUTHORIZATION_HEADER_REGEX, (_match, prefix) => `${prefix}`).replace(BARE_BEARER_TOKEN_REGEX, (_match, prefix) => `${prefix}`).replace(URL_CREDENTIALS_REGEX, (_match, prefix) => `${prefix}:@`).replace(QUOTED_SECRET_REGEX, (match, ...args) => { - const { keyQuote = "", key, separator, valueQuote } = args.at(-1); - if (!isSensitiveKey(key)) + let output = redactShellDefaults(input).replace(PEM_PRIVATE_KEY_REGEX, "").replace(AUTHORIZATION_HEADER_REGEX, (_match, prefix) => `${prefix}`).replace(BARE_BEARER_TOKEN_REGEX, (_match, prefix) => `${prefix}`).replace(URL_CREDENTIALS_REGEX, (_match, prefix) => `${prefix}:@`).replace(QUOTED_SECRET_REGEX, (match, ...args) => { + const { keyQuote = "", key, separator, valueQuote, value } = args.at(-1); + const offset = args.at(-3); + const text = args.at(-2); + if (!isSensitiveKey(key) || isShellReadOfKey(text, offset + keyQuote.length, separator)) + return match; + if (valueQuote === '"' && isShellReference(value)) return match; return `${keyQuote}${key}${keyQuote}${separator}${valueQuote}${placeholderForKey(key)}${valueQuote}`; - }).replace(BARE_ASSIGNMENT_SECRET_REGEX, (match, key, separator) => isSensitiveKey(key) ? `${key}${separator}${placeholderForKey(key)}` : match); + }).replace(BARE_ASSIGNMENT_SECRET_REGEX, (match, key, separator, value, offset, text) => { + if (!isSensitiveKey(key) || isShellReadOfKey(text, offset, separator)) + return match; + if (startsWithShellReference(text, offset + key.length + separator.length, value)) + return match; + return `${key}${separator}${placeholderForKey(key)}`; + }); for (const [pattern, replacement] of TOKEN_PATTERNS) { output = output.replace(pattern, replacement); } @@ -1465,6 +1592,12 @@ function redactText(input, mode = DEFAULT_REDACTION_MODE) { return output; return output.replace(EMAIL_REGEX, "").replace(PHONE_REGEX, "").replace(SSN_REGEX, "").replace(CREDIT_CARD_CANDIDATE_REGEX, (candidate) => isLikelyCreditCard(candidate) ? "" : candidate); } +function isPlaceholder(text) { + return /^<(?:API_KEY|TOKEN|SECRET|PRIVATEKEY|EMAIL|PHONENUMBER|SSN|CREDITCARD)>$/.test(text); +} +function hasRedactionPlaceholder(text) { + return countRedactionPlaceholders(text) > 0; +} function countRedactionPlaceholders(text) { return text.match(PLACEHOLDER_REGEX)?.length ?? 0; } @@ -1518,12 +1651,20 @@ function isLikelyCreditCard(value) { } return sum % 10 === 0; } -var DEFAULT_REDACTION_MODE = "secrets", REDACTION_MODES, KEY_NAME_PATTERN, QUOTED_SECRET_REGEX, BARE_ASSIGNMENT_SECRET_REGEX, AUTHORIZATION_HEADER_REGEX, PEM_PRIVATE_KEY_REGEX, URL_CREDENTIALS_REGEX, BARE_BEARER_TOKEN_REGEX, TOKEN_PATTERNS, EMAIL_REGEX, PHONE_REGEX, SSN_REGEX, CREDIT_CARD_CANDIDATE_REGEX, PLACEHOLDER_REGEX, COMPACT_SENSITIVE_KEYS; +var DEFAULT_REDACTION_MODE = "secrets", REDACTION_MODES, KEY_NAME_PATTERN, QUOTED_SECRET_REGEX, BARE_ASSIGNMENT_SECRET_REGEX, SHELL_NAME_PATTERN, SHELL_EXPANSION_OPEN, SHELL_DEFAULT_REGEX, SHELL_EXPANSION_KEY_PREFIX, SHELL_EXPANSION_INNER, SHELL_DIRECTORY_NAMES, SHELL_VARIABLE_REFERENCE, SHELL_NESTING_LIMIT = 16, SHELL_COMMAND_NAME_VALUE, AUTHORIZATION_HEADER_REGEX, PEM_PRIVATE_KEY_REGEX, URL_CREDENTIALS_REGEX, BARE_BEARER_TOKEN_REGEX, TOKEN_PATTERNS, EMAIL_REGEX, PHONE_REGEX, SSN_REGEX, CREDIT_CARD_CANDIDATE_REGEX, PLACEHOLDER_REGEX, COMPACT_SENSITIVE_KEYS; var init_redaction = __esm(() => { REDACTION_MODES = ["none", "secrets", "secrets+pii"]; KEY_NAME_PATTERN = String.raw`[A-Za-z0-9_][A-Za-z0-9_-]*`; - QUOTED_SECRET_REGEX = new RegExp(String.raw`(?:(?["'])|\b)(?${KEY_NAME_PATTERN})\k` + String.raw`(?\s*[:=]\s*)(?["'])(?:\\[^\r\n]|(?!\k)[^\\\r\n])+\k`, "g"); + QUOTED_SECRET_REGEX = new RegExp(String.raw`(?:(?["'])|\b)(?${KEY_NAME_PATTERN})\k` + String.raw`(?\s*[:=]\s*)(?["'])(?(?:\\[^\r\n]|(?!\k)[^\\\r\n])+)\k`, "g"); BARE_ASSIGNMENT_SECRET_REGEX = new RegExp(String.raw`\b(${KEY_NAME_PATTERN})(\s*[:=]\s*)([^\s"',;` + "`" + String.raw`]+)`, "g"); + SHELL_NAME_PATTERN = String.raw`[A-Za-z_][A-Za-z0-9_]*`; + SHELL_EXPANSION_OPEN = String.raw`\$\{(?:\([^()\s]*\))?[#!^=~]*`; + SHELL_DEFAULT_REGEX = new RegExp(String.raw`(${SHELL_EXPANSION_OPEN})(${SHELL_NAME_PATTERN})(:?[-=+])`, "g"); + SHELL_EXPANSION_KEY_PREFIX = new RegExp(`${SHELL_EXPANSION_OPEN}$`); + SHELL_EXPANSION_INNER = new RegExp(String.raw`^(?:\([^()\s]*\))?[#!^=~]*${SHELL_NAME_PATTERN}(?.*)$`, "s"); + SHELL_DIRECTORY_NAMES = new Set(["PWD", "OLDPWD"]); + SHELL_VARIABLE_REFERENCE = new RegExp(String.raw`^\$(?:[A-Z_][A-Z0-9_]*)$`); + SHELL_COMMAND_NAME_VALUE = /^\$\((?:[A-Za-z0-9_./+-]+|<(?:API_KEY|TOKEN|SECRET|PRIVATEKEY|EMAIL|PHONENUMBER|SSN|CREDITCARD)>)\)?$/; AUTHORIZATION_HEADER_REGEX = /\b(authorization\s*[:=]\s*(?:bearer|basic|token)\s+)([^\s"',;`]+)/gi; PEM_PRIVATE_KEY_REGEX = /-----BEGIN [A-Z0-9 ]*PRIVATE KEY(?: BLOCK)?-----[\s\S]*?-----END [A-Z0-9 ]*PRIVATE KEY(?: BLOCK)?-----/g; URL_CREDENTIALS_REGEX = /(\/\/[^\s/:@"'`]+):([^\s/@"'`]+)@/g; @@ -4726,7 +4867,7 @@ function createReview(options) { if (guidelines) writePrivateFile(path8.join(dir, "guidelines.md"), `${guidelines} `); - writePrivateFile(path8.join(dir, "instructions.md"), reviewInstructions(record, config, guidelines)); + writePrivateFile(path8.join(dir, "instructions.md"), reviewInstructions(record, config, guidelines, addedPlaceholderLines(diff))); writePrivateJson(path8.join(dir, "review.json"), record); logSidecarEvent("review-open", { id, root, peer: config.peer, files: files.length }); pruneClosedReviews(sidecarPath); @@ -4785,6 +4926,102 @@ function pruneOrphanedReviews() { fs8.rmSync(review.dir, { recursive: true, force: true }); } } +function addedPlaceholderLines(patch) { + const found = []; + let file; + let oldLeft = 0; + let newLeft = 0; + let newLine = 0; + for (const raw of patch.split(` +`)) { + const line = raw.endsWith("\r") ? raw.slice(0, -1) : raw; + if (oldLeft > 0 || newLeft > 0) { + if (line.startsWith("+")) { + if (file && hasRedactionPlaceholder(line)) + found.push({ file, line: newLine, text: line.slice(1) }); + newLeft -= 1; + newLine += 1; + } else if (line.startsWith("-")) { + oldLeft -= 1; + } else if (!line.startsWith("\\")) { + oldLeft -= 1; + newLeft -= 1; + newLine += 1; + } + continue; + } + if (line.startsWith("diff --git ")) { + file = undefined; + } else if (line.startsWith("+++ ")) { + file = patchHeaderPath(line.slice(4)); + } else { + const hunk = /^@@ -\d+(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/.exec(line); + if (hunk) { + oldLeft = Number(hunk[1] ?? 1); + newLine = Number(hunk[2]); + newLeft = Number(hunk[3] ?? 1); + } + } + } + return found; +} +function patchHeaderPath(header) { + const target = header.endsWith("\t") ? header.slice(0, -1) : header; + if (target === "/dev/null") + return; + const name = target.length > 1 && target.startsWith('"') && target.endsWith('"') ? unquoteGitPath(target.slice(1, -1)) : target; + return name.replace(/^b\//, ""); +} +function unquoteGitPath(quoted) { + const parts = []; + let literal = ""; + for (let index = 0;index < quoted.length; index += 1) { + if (quoted[index] !== "\\" || index + 1 === quoted.length) { + literal += quoted[index]; + continue; + } + parts.push(Buffer.from(literal, "utf8")); + literal = ""; + const octal = /^[0-3][0-7]{2}/.exec(quoted.slice(index + 1))?.[0]; + if (octal) { + parts.push(Buffer.from([Number.parseInt(octal, 8)])); + index += 3; + } else { + const next = quoted[index + 1]; + parts.push(GIT_PATH_ESCAPES[next] === undefined ? Buffer.from(next, "utf8") : Buffer.from([GIT_PATH_ESCAPES[next]])); + index += 1; + } + } + parts.push(Buffer.from(literal, "utf8")); + return Buffer.concat(parts).toString("utf8"); +} +function placeholderSection(lines) { + if (!lines.length) + return ""; + const shown = lines.slice(0, PLACEHOLDER_LINES_SHOWN); + const more = lines.length - shown.length; + return ` +## Redaction placeholders + +These lines in the change carry a placeholder (numbered as in the new file). +For each one, check that the placeholder stands where a credential's value +was, and only there. Reject, naming the line, if it replaced something that is +not a credential — part of a shell expansion such as \`\${PWD:t}\`, a path, a +command, ordinary words — or if the text around it no longer holds together +(a script that no longer parses, a broken config file). That holds even when +it did replace a real credential: the other machines still receive a broken +file, and the credential belongs outside the synced file. Some placeholders +may have been typed by hand or come from content already published; judge +them the same way. + +${shown.map((entry) => `- ${entry.file}:${entry.line} + + ${entry.text} +`).join(` +`)}${more ? ` +…and ${more} more; find them in \`diff.patch\`. +` : ""}`; +} function opaqueSection(record) { const opaque = record.files.filter((file) => file.artifact); if (!opaque.length) @@ -4799,7 +5036,7 @@ ${opaque.map((file) => `- \`${file.artifact}\` — ${file.path}`).join(` `)} `; } -function reviewInstructions(record, config, guidelines) { +function reviewInstructions(record, config, guidelines, placeholders) { return `# Sidecar publish review ${record.id} A sidecar is a scratchpad repository that syncs automatically to a shared Git @@ -4807,8 +5044,9 @@ remote (${redactText(config.remote)}). This machine wants to publish the change \`diff.patch\`. Everyone with access to that remote can read what you approve, and it stays in the remote's history. -Decide whether anything in the change should not leave this machine. This is -not a code review: ignore quality, style, and correctness. +Decide whether anything in the change should not leave this machine, and +whether redaction damaged it. This is not a code review: ignore the quality, +style, and correctness of what the author wrote. Secret redaction has already run. Values it caught appear as placeholders such as \`\`, \`\`, or \`\`; they are not leaks. Look for what @@ -4819,6 +5057,11 @@ pattern matching misses: - confidential material: customer names and data, unreleased plans, internal-only details - anything the repository's guidelines below rule out +Pattern matching also has false positives, and they are not harmless. A +placeholder replaces the original text in the published file; every other +machine that syncs this repository gets the placeholder in its own copy, while +this machine keeps the original, so the author never sees the damage. +${placeholderSection(placeholders)} ## Files All in this directory (\`$SIDECAR_REVIEW_DIR\`): @@ -4916,7 +5159,7 @@ function decideReview(review, verdict, reason) { review.state = verdict; logSidecarEvent("review-decision", { id: review.record.id, verdict }); } -var REVIEW_SCHEMA = 1, KEEP_CLOSED_REVIEWS = 10, ID_PATTERN, LAUNCH_GRACE_SECONDS = 60, PACKET_DIFF_OPTIONS; +var REVIEW_SCHEMA = 1, KEEP_CLOSED_REVIEWS = 10, ID_PATTERN, LAUNCH_GRACE_SECONDS = 60, PACKET_DIFF_OPTIONS, GIT_PATH_ESCAPES, PLACEHOLDER_LINES_SHOWN = 200; var init_review = __esm(() => { init_util(); init_git(); @@ -4926,6 +5169,7 @@ var init_review = __esm(() => { init_redaction(); ID_PATTERN = /^[0-9a-f]+$/; PACKET_DIFF_OPTIONS = ["-r", "--no-renames", "--ignore-submodules=none"]; + GIT_PATH_ESCAPES = { a: 7, b: 8, t: 9, n: 10, v: 11, f: 12, r: 13, '"': 34, "\\": 92 }; }); // src/sync.ts @@ -6274,7 +6518,7 @@ function pendingInboxBranches(repo, config) { }); return [...local, ...remote].sort(); } -var REVIEW_POLICY_KEY = "sidecar.review", WRITTEN_TRAILER = "written:", WRITTEN_TRAILER_LIMIT = 500, REDACTION_FILTER_NAME = "sidecar-redact", REDACTION_FILTER_REVISION = "2", REDACTION_POLICY_FILE = "sidecar-redaction-policy"; +var REVIEW_POLICY_KEY = "sidecar.review", WRITTEN_TRAILER = "written:", WRITTEN_TRAILER_LIMIT = 500, REDACTION_FILTER_NAME = "sidecar-redact", REDACTION_FILTER_REVISION = "3", REDACTION_POLICY_FILE = "sidecar-redaction-policy"; var init_sync = __esm(() => { init_color(); init_util(); diff --git a/packages/sidecar/src/redaction.ts b/packages/sidecar/src/redaction.ts index 6d6af3b..3a0b27f 100644 --- a/packages/sidecar/src/redaction.ts +++ b/packages/sidecar/src/redaction.ts @@ -16,7 +16,7 @@ const QUOTED_SECRET_REGEX = new RegExp( // The escape branch is the only one that can consume a backslash, so the // alternation is unambiguous — an unterminated value can't trigger // exponential backtracking. - String.raw`(?\s*[:=]\s*)(?["'])(?:\\[^\r\n]|(?!\k)[^\\\r\n])+\k`, + String.raw`(?\s*[:=]\s*)(?["'])(?(?:\\[^\r\n]|(?!\k)[^\\\r\n])+)\k`, "g", ); @@ -25,6 +25,162 @@ const BARE_ASSIGNMENT_SECRET_REGEX = new RegExp( "g", ); +// Shell parameter expansion reads a variable; it never assigns one. `${PWD:t}` +// (zsh's tail modifier), `${PASSWORD:0:3}`, and `$TOKEN:foo` look like +// `key: value` to the assignment patterns above, and redacting the "value" +// eats the closing brace: the published file no longer parses, and every +// other machine receives it as its working copy. So a key read through `$` or +// `${` is skipped there, and the one part of an expansion that can carry a +// literal secret — the word of a default (`${DB_PASSWORD:-hunter2}`), assign +// (`:=`), or alternate (`:+`) — is redacted here instead, keeping the braces. +const SHELL_NAME_PATTERN = String.raw`[A-Za-z_][A-Za-z0-9_]*`; +// `${`, then zsh's `(flags)` and the `#`/`!`/`^`/`=`/`~` prefixes. One +// grammar for both rules: a form skipped as a read must be one whose default +// is redacted. +const SHELL_EXPANSION_OPEN = String.raw`\$\{(?:\([^()\s]*\))?[#!^=~]*`; +const SHELL_DEFAULT_REGEX = new RegExp( + String.raw`(${SHELL_EXPANSION_OPEN})(${SHELL_NAME_PATTERN})(:?[-=+])`, + "g", +); +const SHELL_EXPANSION_KEY_PREFIX = new RegExp(`${SHELL_EXPANSION_OPEN}$`); +const SHELL_EXPANSION_INNER = new RegExp(String.raw`^(?:\([^()\s]*\))?[#!^=~]*${SHELL_NAME_PATTERN}(?.*)$`, "s"); +// The shell's working directories. In an expansion they are always the +// variable, never an ODBC-style `PWD=` password. +const SHELL_DIRECTORY_NAMES = new Set(["PWD", "OLDPWD"]); +// Upper case only: in a `.env` file, `PASSWORD=$ecret` may be a literal. +const SHELL_VARIABLE_REFERENCE = new RegExp(String.raw`^\$(?:[A-Z_][A-Z0-9_]*)$`); +// Deeper nesting is treated as unterminated, and so redacted: hostile input +// must not make the clean filter recurse or rescan without bound. +const SHELL_NESTING_LIMIT = 16; + +function redactShellDefaults(text: string): string { + let output = ""; + let copied = 0; + for (const match of text.matchAll(SHELL_DEFAULT_REGEX)) { + const [whole, open, name, operator] = match; + // Inside a word already redacted, or already found to be only references. + if (match.index < copied) continue; + if (SHELL_DIRECTORY_NAMES.has(name) || !isSensitiveKey(name)) continue; + const wordStart = match.index + whole.length; + const end = shellWordEnd(text, wordStart, "}"); + // Unterminated on this line: there is no closing brace to keep, so the + // rest of the line goes, as the assignment pattern would have taken it. + const wordEnd = end === -1 ? lineEnd(text, wordStart) : end; + const word = text.slice(wordStart, wordEnd); + output += text.slice(copied, wordStart); + output += !word || isPlaceholder(word) || isShellReference(word) ? word : placeholderForKey(name); + copied = wordEnd; + } + return output + text.slice(copied); +} + +/** + * Where the word starting at `start` ends: the index of the `close` that + * ends its expansion, or -1 if the line ends first. Backslash escapes, + * quotes, backticks, and nested `${…}` and `$(…)` are skipped, so a brace + * inside them never ends the word early and leaves part of it behind. + */ +function shellWordEnd(text: string, start: number, close: "}" | ")", depth = 0): number { + if (depth > SHELL_NESTING_LIMIT) return -1; + let parens = 0; + for (let index = start; index < text.length; index += 1) { + const char = text[index]; + if (char === "\n" || char === "\r") return -1; + if (char === "\\") { + index += 1; + } else if (char === "'" || char === '"' || char === "`") { + index = closingQuote(text, index + 1, char, depth + 1); + if (index === -1) return -1; + } else if (char === "$" && (text[index + 1] === "{" || text[index + 1] === "(")) { + index = shellWordEnd(text, index + 2, text[index + 1] === "{" ? "}" : ")", depth + 1); + if (index === -1) return -1; + } else if (close === ")" && char === "(") { + parens += 1; + } else if (close === ")" && char === ")" && parens > 0) { + parens -= 1; + } else if (char === close) { + return index; + } + } + return -1; +} + +/** The index of the quote closing one opened before `start`; a double quote's expansions are skipped whole. */ +function closingQuote(text: string, start: number, quote: string, depth: number): number { + if (depth > SHELL_NESTING_LIMIT) return -1; + for (let index = start; index < text.length; index += 1) { + const char = text[index]; + if (char === "\n" || char === "\r") return -1; + if (char === quote) return index; + if (quote === "'") continue; + if (char === "\\") { + index += 1; + } else if (quote === '"' && char === "$" && (text[index + 1] === "{" || text[index + 1] === "(")) { + index = shellWordEnd(text, index + 2, text[index + 1] === "{" ? "}" : ")", depth + 1); + if (index === -1) return -1; + } else if (quote === '"' && char === "`") { + index = closingQuote(text, index + 1, "`", depth + 1); + if (index === -1) return -1; + } + } + return -1; +} + +function lineEnd(text: string, start: number): number { + const match = /[\r\n]/.exec(text.slice(start)); + return match ? start + match.index : text.length; +} + +/** Whether the key at `offset` is read by `${KEY…}`, or by `$KEY` before a `:`. `$key = value` is an assignment (PHP, PowerShell). */ +function isShellReadOfKey(text: string, offset: number, separator: string): boolean { + if (SHELL_EXPANSION_KEY_PREFIX.test(text.slice(Math.max(0, offset - 64), offset))) return true; + return text[offset - 1] === "$" && separator.trim().startsWith(":"); +} + +/** + * Whether a value only names another variable, which is never a literal + * secret: `$NAME`, or `${NAME}` with a modifier, an error message, or a + * default that is itself a reference or a placeholder. One pair of double + * quotes around it is allowed. A command is not a reference: its arguments + * can be literals. + */ +function isShellReference(value: string, depth = 0): boolean { + if (depth > SHELL_NESTING_LIMIT) return false; + const unquoted = value.length > 1 && value.startsWith('"') && value.endsWith('"') ? value.slice(1, -1) : value; + if (SHELL_VARIABLE_REFERENCE.test(unquoted)) return true; + if (!unquoted.startsWith("${") || shellWordEnd(unquoted, 2, "}") !== unquoted.length - 1) return false; + const rest = SHELL_EXPANSION_INNER.exec(unquoted.slice(2, -1))?.groups?.rest; + if (rest === undefined) return false; + if (rest === "" || /^:?\?/.test(rest) || /^(?::[a-zA-Z0-9]+)+$/.test(rest)) return true; + const word = /^:?[-=+](?.*)$/s.exec(rest)?.groups?.word; + return word !== undefined && (word === "" || isPlaceholder(word) || isShellReference(word, depth + 1)); +} + +// `$(gh auth token)`: an unquoted value that opens a command substitution +// with a plain command name. The value pattern stops at whitespace, so the +// name is all it would replace, leaving ` auth token)`: no secret +// removed, and a line that no longer parses. Anything less plain +// (`$(cat<<)\)?$/; + +/** Whether the unquoted value starting at `start` only refers to something else: `$NAME`, `${NAME}`, or a command. */ +function startsWithShellReference(text: string, start: number, value: string): boolean { + if (SHELL_COMMAND_NAME_VALUE.test(value)) return true; + if (text[start] !== "$") return false; + let end: number; + if (text[start + 1] === "{") { + end = shellWordEnd(text, start + 2, "}"); + if (end === -1) return false; + end += 1; + } else { + end = start + 1 + (/^[A-Za-z0-9_]*/.exec(text.slice(start + 1))?.[0].length ?? 0); + } + if (end < text.length && !/[\s;&|)]/.test(text[end])) return false; + return isShellReference(text.slice(start, end)); +} + const AUTHORIZATION_HEADER_REGEX = /\b(authorization\s*[:=]\s*(?:bearer|basic|token)\s+)([^\s"',;`]+)/gi; const PEM_PRIVATE_KEY_REGEX = /-----BEGIN [A-Z0-9 ]*PRIVATE KEY(?: BLOCK)?-----[\s\S]*?-----END [A-Z0-9 ]*PRIVATE KEY(?: BLOCK)?-----/g; @@ -50,25 +206,32 @@ const CREDIT_CARD_CANDIDATE_REGEX = /\b(?:\d[ -]*?){13,19}\b/g; export function redactText(input: string, mode: RedactionMode = DEFAULT_REDACTION_MODE): string { if (mode === "none") return input; - let output = input + let output = redactShellDefaults(input) .replace(PEM_PRIVATE_KEY_REGEX, "") .replace(AUTHORIZATION_HEADER_REGEX, (_match, prefix: string) => `${prefix}`) .replace(BARE_BEARER_TOKEN_REGEX, (_match, prefix: string) => `${prefix}`) .replace(URL_CREDENTIALS_REGEX, (_match, prefix: string) => `${prefix}:@`) .replace(QUOTED_SECRET_REGEX, (match, ...args) => { - const { keyQuote = "", key, separator, valueQuote } = args.at(-1) as { + const { keyQuote = "", key, separator, valueQuote, value } = args.at(-1) as { keyQuote?: string; key: string; separator: string; valueQuote: string; + value: string; }; - if (!isSensitiveKey(key)) return match; + const offset = args.at(-3) as number; + const text = args.at(-2) as string; + if (!isSensitiveKey(key) || isShellReadOfKey(text, offset + keyQuote.length, separator)) return match; + if (valueQuote === '"' && isShellReference(value)) return match; return `${keyQuote}${key}${keyQuote}${separator}${valueQuote}${placeholderForKey(key)}${valueQuote}`; }) .replace( BARE_ASSIGNMENT_SECRET_REGEX, - (match, key: string, separator: string) => - isSensitiveKey(key) ? `${key}${separator}${placeholderForKey(key)}` : match, + (match, key: string, separator: string, value: string, offset: number, text: string) => { + if (!isSensitiveKey(key) || isShellReadOfKey(text, offset, separator)) return match; + if (startsWithShellReference(text, offset + key.length + separator.length, value)) return match; + return `${key}${separator}${placeholderForKey(key)}`; + }, ); for (const [pattern, replacement] of TOKEN_PATTERNS) { @@ -87,6 +250,15 @@ export function redactText(input: string, mode: RedactionMode = DEFAULT_REDACTIO const PLACEHOLDER_REGEX = /<(?:API_KEY|TOKEN|SECRET|PRIVATEKEY|EMAIL|PHONENUMBER|SSN|CREDITCARD)>/g; +function isPlaceholder(text: string): boolean { + return /^<(?:API_KEY|TOKEN|SECRET|PRIVATEKEY|EMAIL|PHONENUMBER|SSN|CREDITCARD)>$/.test(text); +} + +/** Whether a line carries a redaction placeholder. */ +export function hasRedactionPlaceholder(text: string): boolean { + return countRedactionPlaceholders(text) > 0; +} + export function countRedactionPlaceholders(text: string): number { return text.match(PLACEHOLDER_REGEX)?.length ?? 0; } diff --git a/packages/sidecar/src/review.ts b/packages/sidecar/src/review.ts index d336d5d..712f108 100644 --- a/packages/sidecar/src/review.ts +++ b/packages/sidecar/src/review.ts @@ -38,7 +38,7 @@ import { reviewTimeoutSeconds, } from "./config.js"; import { logSidecarEvent, readSettings, sidecarStateDir } from "./state.js"; -import { redactText } from "./redaction.js"; +import { hasRedactionPlaceholder, redactText } from "./redaction.js"; const REVIEW_SCHEMA = 1; /** Closed reviews kept per family, newest first; older packets are deleted. */ @@ -330,7 +330,7 @@ export function createReview(options: { root: string; config: SidecarConfig; sid }; writePrivateFile(path.join(dir, "diff.patch"), diff); if (guidelines) writePrivateFile(path.join(dir, "guidelines.md"), `${guidelines}\n`); - writePrivateFile(path.join(dir, "instructions.md"), reviewInstructions(record, config, guidelines)); + writePrivateFile(path.join(dir, "instructions.md"), reviewInstructions(record, config, guidelines, addedPlaceholderLines(diff))); // The record last: a directory without one is not yet a review, so a crash // mid-packet leaves nothing a sync would wait on. writePrivateJson(path.join(dir, "review.json"), record); @@ -418,6 +418,117 @@ export function pruneOrphanedReviews(): void { } } +type PlaceholderLine = { file: string; line: number; text: string }; + +/** + * The lines a patch adds that carry a redaction placeholder, numbered as in + * the new file. From the published diff alone: it says where a placeholder + * will land, not whether redaction or a person put it there, and it never + * needs the original text. + */ +export function addedPlaceholderLines(patch: string): PlaceholderLine[] { + const found: PlaceholderLine[] = []; + let file: string | undefined; + let oldLeft = 0; + let newLeft = 0; + let newLine = 0; + for (const raw of patch.split("\n")) { + const line = raw.endsWith("\r") ? raw.slice(0, -1) : raw; + // Inside a hunk, the header's counts say where it ends: an added line + // can itself begin with `+++ ` or `@@ `. + if (oldLeft > 0 || newLeft > 0) { + if (line.startsWith("+")) { + if (file && hasRedactionPlaceholder(line)) found.push({ file, line: newLine, text: line.slice(1) }); + newLeft -= 1; + newLine += 1; + } else if (line.startsWith("-")) { + oldLeft -= 1; + } else if (!line.startsWith("\\")) { + oldLeft -= 1; + newLeft -= 1; + newLine += 1; + } + continue; + } + if (line.startsWith("diff --git ")) { + file = undefined; + } else if (line.startsWith("+++ ")) { + file = patchHeaderPath(line.slice(4)); + } else { + const hunk = /^@@ -\d+(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/.exec(line); + if (hunk) { + oldLeft = Number(hunk[1] ?? 1); + newLine = Number(hunk[2]); + newLeft = Number(hunk[3] ?? 1); + } + } + } + return found; +} + +/** + * The new path a `+++ ` header names, or undefined for a deletion. Git + * C-quotes a path with unusual bytes, and ends one containing a space with a + * tab. + */ +function patchHeaderPath(header: string): string | undefined { + const target = header.endsWith("\t") ? header.slice(0, -1) : header; + if (target === "/dev/null") return undefined; + const name = target.length > 1 && target.startsWith('"') && target.endsWith('"') ? unquoteGitPath(target.slice(1, -1)) : target; + return name.replace(/^b\//, ""); +} + +const GIT_PATH_ESCAPES: Record = { a: 7, b: 8, t: 9, n: 10, v: 11, f: 12, r: 13, '"': 34, "\\": 92 }; + +/** Undoes git's C-style quoting; octal escapes are raw bytes of a UTF-8 path. */ +function unquoteGitPath(quoted: string): string { + const parts: Buffer[] = []; + let literal = ""; + for (let index = 0; index < quoted.length; index += 1) { + if (quoted[index] !== "\\" || index + 1 === quoted.length) { + literal += quoted[index]; + continue; + } + parts.push(Buffer.from(literal, "utf8")); + literal = ""; + const octal = /^[0-3][0-7]{2}/.exec(quoted.slice(index + 1))?.[0]; + if (octal) { + parts.push(Buffer.from([Number.parseInt(octal, 8)])); + index += 3; + } else { + const next = quoted[index + 1]; + parts.push(GIT_PATH_ESCAPES[next] === undefined ? Buffer.from(next, "utf8") : Buffer.from([GIT_PATH_ESCAPES[next]])); + index += 1; + } + } + parts.push(Buffer.from(literal, "utf8")); + return Buffer.concat(parts).toString("utf8"); +} + +/** Past this many, the list points into the diff instead of growing. */ +const PLACEHOLDER_LINES_SHOWN = 200; + +function placeholderSection(lines: PlaceholderLine[]): string { + if (!lines.length) return ""; + const shown = lines.slice(0, PLACEHOLDER_LINES_SHOWN); + const more = lines.length - shown.length; + return ` +## Redaction placeholders + +These lines in the change carry a placeholder (numbered as in the new file). +For each one, check that the placeholder stands where a credential's value +was, and only there. Reject, naming the line, if it replaced something that is +not a credential — part of a shell expansion such as \`\${PWD:t}\`, a path, a +command, ordinary words — or if the text around it no longer holds together +(a script that no longer parses, a broken config file). That holds even when +it did replace a real credential: the other machines still receive a broken +file, and the credential belongs outside the synced file. Some placeholders +may have been typed by hand or come from content already published; judge +them the same way. + +${shown.map((entry) => `- ${entry.file}:${entry.line}\n\n ${entry.text}\n`).join("\n")}${more ? `\n…and ${more} more; find them in \`diff.patch\`.\n` : ""}`; +} + function opaqueSection(record: ReviewRecord): string { const opaque = record.files.filter((file) => file.artifact); if (!opaque.length) return ""; @@ -431,7 +542,7 @@ ${opaque.map((file) => `- \`${file.artifact}\` — ${file.path}`).join("\n")} `; } -function reviewInstructions(record: ReviewRecord, config: SidecarConfig, guidelines: string | undefined): string { +function reviewInstructions(record: ReviewRecord, config: SidecarConfig, guidelines: string | undefined, placeholders: PlaceholderLine[]): string { return `# Sidecar publish review ${record.id} A sidecar is a scratchpad repository that syncs automatically to a shared Git @@ -439,8 +550,9 @@ remote (${redactText(config.remote)}). This machine wants to publish the change \`diff.patch\`. Everyone with access to that remote can read what you approve, and it stays in the remote's history. -Decide whether anything in the change should not leave this machine. This is -not a code review: ignore quality, style, and correctness. +Decide whether anything in the change should not leave this machine, and +whether redaction damaged it. This is not a code review: ignore the quality, +style, and correctness of what the author wrote. Secret redaction has already run. Values it caught appear as placeholders such as \`\`, \`\`, or \`\`; they are not leaks. Look for what @@ -451,6 +563,11 @@ pattern matching misses: - confidential material: customer names and data, unreleased plans, internal-only details - anything the repository's guidelines below rule out +Pattern matching also has false positives, and they are not harmless. A +placeholder replaces the original text in the published file; every other +machine that syncs this repository gets the placeholder in its own copy, while +this machine keeps the original, so the author never sees the damage. +${placeholderSection(placeholders)} ## Files All in this directory (\`$SIDECAR_REVIEW_DIR\`): diff --git a/packages/sidecar/src/sync.ts b/packages/sidecar/src/sync.ts index eaf1609..4e0f240 100644 --- a/packages/sidecar/src/sync.ts +++ b/packages/sidecar/src/sync.ts @@ -1626,8 +1626,10 @@ export function fileRedactionDelta( const REDACTION_FILTER_NAME = "sidecar-redact"; // Bump when filter semantics change without changing the command, so Git's -// cached blobs are reprocessed too. Revision 2 adds per-path peer rules. -const REDACTION_FILTER_REVISION = "2"; +// cached blobs are reprocessed too. Revision 2 adds per-path peer rules; +// revision 3 stops reading shell expansions (`${PWD:t}`) as assignments, so +// files an earlier filter mangled are committed again from their originals. +const REDACTION_FILTER_REVISION = "3"; const REDACTION_POLICY_FILE = "sidecar-redaction-policy"; export type RedactionPolicy = { rules?: SidecarRules; rulesPath?: string }; diff --git a/packages/sidecar/tests/cli.test.ts b/packages/sidecar/tests/cli.test.ts index 336b1f7..bc3043c 100644 --- a/packages/sidecar/tests/cli.test.ts +++ b/packages/sidecar/tests/cli.test.ts @@ -669,6 +669,92 @@ describe("redaction", () => { expect(redacted).toContain("2FA_TOKEN="); }); + // The line that broke every interactive shell on two machines: `PWD` names + // a password key, and `${PWD:t}` read as the assignment `PWD: t}`. + const ZSHRC_MODS_LINE = '[[ -n "$sub" && "${PWD:t}" != "$sub" ]] && where+=" %F{cyan}${${PWD:t}//\\%/%%}%f"'; + + test("publishes a zsh line using ${PWD:t} unchanged", () => { + const file = `typeset where\n${ZSHRC_MODS_LINE}\n`; + for (const mode of ["secrets", "secrets+pii"] as const) { + expect(redactText(ZSHRC_MODS_LINE, mode)).toBe(ZSHRC_MODS_LINE); + expect(redactBuffer(Buffer.from(file), mode).toString("utf8")).toBe(file); + } + }); + + test.each([ + "echo ${PASSWORD:0:3}", + 'echo "${GITHUB_TOKEN:?set GITHUB_TOKEN first}"', + "echo $TOKEN:foo", + "echo $PWD:t", + "echo ${(L)PWD:t}", + "echo ${#API_KEY}", + "cd ${PWD:-/tmp}", + 'cd "${OLDPWD:-$HOME}"', + "echo ${PWD/#$HOME/~}", + "x=${SECRET_DIR:-/tmp/s}", + "f=${TOKEN_FILE:h}", + "echo ${DB_PASSWORD:-$FALLBACK_PASSWORD}", + "export GITHUB_TOKEN=$(gh auth token)", + "export OPENAI_API_KEY=$(security find-generic-password -s openai -w); echo ok", + "export GITHUB_TOKEN=$GH_TOKEN", + "export GITHUB_TOKEN=${GH_TOKEN}", + 'password: "${DB_PASSWORD}"', + ])("leaves shell expansions that only read a variable alone: %s", (line) => { + expect(redactText(line)).toBe(line); + }); + + test.each([ + // Literal defaults are redacted inside the braces, which survive. + ['psql "${DB_PASSWORD:-hunter2}"', 'psql "${DB_PASSWORD:-}"'], + ["x=${DB_PASSWORD:-hunter2}", "x=${DB_PASSWORD:-}"], + ['psql "${DB_PASSWORD:-hunter2$SUFFIX}"', 'psql "${DB_PASSWORD:-}"'], + ['export PGPASSWORD="${DB_PASSWORD:+hunter2}"', 'export PGPASSWORD="${DB_PASSWORD:+}"'], + ["echo ${(U)DB_PASSWORD:-hunter2}", "echo ${(U)DB_PASSWORD:-}"], + ['echo "${DB_PASSWORD:-hunter2\\}suffix}"', 'echo "${DB_PASSWORD:-}"'], + [": ${API_KEY:=notarealkey}", ": ${API_KEY:=}"], + ["url=${TOKEN-abc}def", "url=${TOKEN-}def"], + ["POSTGRES_PASSWORD: ${POSTGRES_PASSWORD:-postgres}", "POSTGRES_PASSWORD: ${POSTGRES_PASSWORD:-}"], + ["echo ${#DB_PASSWORD:-hunter2}", "echo ${#DB_PASSWORD:-}"], + ['echo ${DB_PASSWORD:-"pre$(printf "%s" "}hunter2")"}', "echo ${DB_PASSWORD:-}"], + // A command's arguments can be literals. Only a plain command name, all + // the old pattern would have replaced, is left alone. + ["echo ${DB_PASSWORD:-$(security find-generic-password -s db -w)}", "echo ${DB_PASSWORD:-}"], + ['export GITHUB_TOKEN="$(gh auth token)"', 'export GITHUB_TOKEN=""'], + ['PASSWORD="$(printf %s hunter2)"', 'PASSWORD=""'], + ["PASSWORD=$((12345678))", "PASSWORD="], + ["PASSWORD=$(cat<<"], + ["PASSWORD=$(sk-1234567890abcdef)", "PASSWORD=$()"], + ["PASSWORD=$(echo${IFS}hunter2)", "PASSWORD="], + ["export GITHUB_TOKEN=$(echo password=hunter2)", "export GITHUB_TOKEN=$(echo password="], + ["PASSWORD=$(echo password=)", "PASSWORD=$(echo password="], + // Unterminated: nothing to keep, so the rest of the line goes. + ["echo ${DB_PASSWORD:-hunter2", "echo ${DB_PASSWORD:-"], + // A `$` before a key read by `=` is an assignment (PHP, PowerShell). + ["$password = 'hunter2';", "$password = '';"], + ['$password = "hunter2";', '$password = "";'], + // A lower-case `$word` may be a literal in a .env file. + ["GITHUB_TOKEN=$ecretvalue", "GITHUB_TOKEN="], + ["GITHUB_TOKEN=${GH_TOKEN:-hunter2}", "GITHUB_TOKEN=${GH_TOKEN:-}"], + // A default under a name that is not sensitive is caught by the outer key. + ["GITHUB_TOKEN=${FALLBACK:-hunter2}", "GITHUB_TOKEN="], + // PWD outside an expansion stays a password key: ODBC connection strings. + ["Driver={ODBC};UID=sa;PWD=hunter2;", "Driver={ODBC};UID=sa;PWD=;"], + ["pwd=hunter2", "pwd="], + ["DB_PWD=hunter2", "DB_PWD="], + ])("still redacts secrets in and around shell syntax: %s", (line, expected) => { + expect(redactText(line)).toBe(expected); + expect(redactText(expected)).toBe(expected); + }); + + test("deeply nested shell expansions stay linear", () => { + const started = Date.now(); + redactText(`${"${TOKEN:-".repeat(5000)}$FALLBACK${"}".repeat(5000)}`); + redactText(`TOKEN=${"$(".repeat(5000)}x${")".repeat(5000)}`); + redactText('"$('.repeat(5000)); + redactText(`${"TOKEN=$(echo ".repeat(5000)}x${")".repeat(5000)}`); + expect(Date.now() - started).toBeLessThan(1000); + }); + integrationTest("redacts files containing the former bypass marker", () => { const repo = initRepo(); const content = "sidecar:no-redact\nGITHUB_TOKEN=ghp_abcdefghijklmnopqrstuvwxyz1234567890\n"; diff --git a/packages/sidecar/tests/review.test.ts b/packages/sidecar/tests/review.test.ts index 0b24922..20da561 100644 --- a/packages/sidecar/tests/review.test.ts +++ b/packages/sidecar/tests/review.test.ts @@ -20,6 +20,7 @@ import { logSidecarEvent, readSettings, writeSettings } from "../src/state.js"; import type { SidecarConfig } from "../src/config.js"; import { type Review, + addedPlaceholderLines, createReview, decideReview, findReview, @@ -39,10 +40,12 @@ const config: SidecarConfig = { let directory: string; let root: string; let numstat = ""; +let patch = ""; beforeEach(() => { vi.resetAllMocks(); numstat = "1\t0\tnotes.md\0"; + patch = "diff --git a/notes.md b/notes.md\n+a new line\n"; directory = fs.mkdtempSync(path.join(os.tmpdir(), "sidecar-review-")); root = path.join(directory, "repo"); fs.mkdirSync(path.join(root, "sidecar", ".git"), { recursive: true }); @@ -54,7 +57,7 @@ beforeEach(() => { let stdout = ""; if (args[0] === "diff-tree" && args.includes("--name-status")) stdout = "M\0notes.md\0A\0plans/q3.md\0"; else if (args[0] === "diff-tree" && args.includes("--numstat")) stdout = numstat; - else if (args[0] === "diff-tree") stdout = "diff --git a/notes.md b/notes.md\n+a new line\n"; + else if (args[0] === "diff-tree") stdout = patch; else if (args[0] === "ls-tree") stdout = ["100644 blob aaa\tnotes.md", "100644 blob bbb\tplans/q3.md", "160000 commit ccc\tvendor/lib"].join("\x00"); return { status: 0, stdout, stderr: "" }; }); @@ -133,6 +136,71 @@ describe("opening a review", () => { expect(review.record.files.find((file) => file.path === "notes.md")!.artifact).toBeUndefined(); }); + test("points the reviewer at every line where a placeholder lands, and what a false positive costs", () => { + patch = [ + "diff --git a/home/.zshrc_mods b/home/.zshrc_mods", + "index 1111111..2222222 100644", + "--- a/home/.zshrc_mods", + "+++ b/home/.zshrc_mods", + "@@ -10,2 +10,3 @@ prompt() {", + " local where", + '+ [[ -n "$sub" && "${PWD:" != "$sub" ]] && where+=" %F{cyan}${${PWD:"', + " print $where", + "", + ].join("\n"); + const instructions = fs.readFileSync(path.join(open().dir, "instructions.md"), "utf8"); + expect(instructions).toContain("## Redaction placeholders"); + expect(instructions).toContain('- home/.zshrc_mods:11\n\n [[ -n "$sub" && "${PWD:"'); + expect(instructions).toContain("every other\nmachine that syncs this repository gets the placeholder"); + }); + + test("leaves the placeholder section out when the change adds none", () => { + expect(fs.readFileSync(path.join(open().dir, "instructions.md"), "utf8")).not.toContain("## Redaction placeholders"); + }); + + test("numbers placeholder lines by hunk, so added lines that look like headers stay content", () => { + const lines = addedPlaceholderLines([ + "diff --git a/a.md b/a.md", + "--- a/a.md", + "+++ b/a.md", + "@@ -1,2 +1,4 @@", + "-token: ", + "+++ b/fake.md ", + "+@@ -1 +1 @@", + " context", + "+password: ", + "\\ No newline at end of file", + "diff --git a/gone.md b/gone.md", + "--- a/gone.md", + "+++ /dev/null", + "@@ -1 +0,0 @@", + "-token: ", + "diff --git a/new.md b/new.md", + "--- /dev/null", + "+++ b/new.md", + "@@ -0,0 +1 @@", + "+key: ", + "diff --git a/my notes.md b/my notes.md", + "--- a/my notes.md\t", + "+++ b/my notes.md\t", + "@@ -0,0 +1 @@", + "+token: ", + 'diff --git "a/caf\\303\\251\\tx.md" "b/caf\\303\\251\\tx.md"', + '--- "a/caf\\303\\251\\tx.md"', + '+++ "b/caf\\303\\251\\tx.md"', + "@@ -0,0 +1 @@", + "+token: ", + "", + ].join("\n")); + expect(lines).toEqual([ + { file: "a.md", line: 1, text: "++ b/fake.md " }, + { file: "a.md", line: 4, text: "password: " }, + { file: "new.md", line: 1, text: "key: " }, + { file: "my notes.md", line: 1, text: "token: " }, + { file: "café\tx.md", line: 1, text: "token: " }, + ]); + }); + test("says so when the repository has no guidelines", () => { const review = open(); expect(fs.existsSync(path.join(review.dir, "guidelines.md"))).toBe(false);