From 980ce333c5d201d842da3c23535e8913601a302a Mon Sep 17 00:00:00 2001 From: Zack Story <2302315+rt2zz@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:01:25 -0700 Subject: [PATCH 1/4] fix(redaction): stop reading shell expansions as secret assignments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `${PWD:t}` matched the bare-assignment pattern as `PWD: t}` (PWD is a password key), and redacting the "value" ate the closing brace. The published dotfile no longer parsed, and every other machine received it as its working copy while the author's machine kept the original. - A key read through `${…}` or `$KEY:` is never an assignment. The one part of an expansion that can hold a literal secret, the word of a default, assign, or alternate, is redacted inside the braces instead. - Values that only refer to something else (`$(gh auth token)`, `$GH_TOKEN`, `${GH_TOKEN}`) are no longer replaced. - PWD stays a password key outside expansions (ODBC `PWD=`). - Filter revision 3, so every checkout reprocesses tracked files and the authoring machine republishes what the old filter mangled. - Review packets list every added line carrying a placeholder and tell the reviewer that a false positive replaces the text on every other machine. Co-Authored-By: Claude --- docs/redaction.md | 71 +++++++- docs/review.md | 11 +- packages/sidecar/dist/cli.js | 222 ++++++++++++++++++++++++-- packages/sidecar/src/redaction.ts | 157 +++++++++++++++++- packages/sidecar/src/review.ts | 89 ++++++++++- packages/sidecar/src/sync.ts | 6 +- packages/sidecar/tests/cli.test.ts | 66 ++++++++ packages/sidecar/tests/review.test.ts | 58 ++++++- 8 files changed, 646 insertions(+), 34 deletions(-) diff --git a/docs/redaction.md b/docs/redaction.md index e9eacef..6441fa5 100644 --- a/docs/redaction.md +++ b/docs/redaction.md @@ -29,8 +29,31 @@ 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 refers to something else — `$GH_TOKEN`, +`${GH_TOKEN}`, or a command such as `$(gh auth token)`. The value is +fetched when the line runs, and isn't written in the file to strip. + +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 +66,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 +86,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 +97,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..d3bbe36 100755 --- a/packages/sidecar/dist/cli.js +++ b/packages/sidecar/dist/cli.js @@ -1449,15 +1449,128 @@ 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; + const wordStart = match.index + whole.length; + if (match.index < copied || SHELL_DIRECTORY_NAMES.has(name) || !isSensitiveKey(name)) + continue; + const end = shellWordEnd(text, wordStart, "}"); + const wordEnd = end === -1 ? lineEnd(text, wordStart) : end; + const word = text.slice(wordStart, wordEnd); + if (!word || isShellReference(word) || isPlaceholder(word)) + continue; + output += `${text.slice(copied, match.index)}${open}${name}${operator}${placeholderForKey(name)}`; + copied = wordEnd; + } + return output + text.slice(copied); +} +function shellWordEnd(text, start, close) { + const closers = [close]; + 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 === "`") { + const quoteEnd = closingQuote(text, index + 1, char); + if (quoteEnd === -1) + return -1; + index = quoteEnd; + } else if (char === "$" && (text[index + 1] === "{" || text[index + 1] === "(")) { + closers.push(text[index + 1] === "{" ? "}" : ")"); + index += 1; + } else if (char === "(" && closers.at(-1) === ")") { + closers.push(")"); + } else if (char === closers.at(-1)) { + closers.pop(); + if (!closers.length) + return index; + } + } + return -1; +} +function closingQuote(text, start, quote) { + for (let index = start;index < text.length; index += 1) { + const char = text[index]; + if (char === ` +` || char === "\r") + return -1; + if (char === "\\" && quote !== "'") + index += 1; + else if (char === quote) + return index; + } + 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) { + 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("${") && !unquoted.startsWith("$(")) + return false; + const close = unquoted[1] === "{" ? "}" : ")"; + if (shellWordEnd(unquoted, 2, close) !== unquoted.length - 1) + return false; + if (close === ")") + return true; + const inner = new RegExp(String.raw`^(?:\([^()\s]*\))?[#!^=~]*${SHELL_NAME_PATTERN}(?.*)$`, "s").exec(unquoted.slice(2, -1)); + const rest = inner?.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)); +} +function startsWithShellReference(text, start) { + if (text[start] !== "$") + return false; + let end; + if (text[start + 1] === "{" || text[start + 1] === "(") { + end = shellWordEnd(text, start + 2, text[start + 1] === "{" ? "}" : ")"); + 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)) + return match; + return `${key}${separator}${placeholderForKey(key)}`; + }); for (const [pattern, replacement] of TOKEN_PATTERNS) { output = output.replace(pattern, replacement); } @@ -1465,6 +1578,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 +1637,18 @@ 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_DIRECTORY_NAMES, SHELL_VARIABLE_REFERENCE, 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(String.raw`\$\{(?:\([^()\s]*\))?[#!^=~]*$`); + SHELL_DIRECTORY_NAMES = new Set(["PWD", "OLDPWD"]); + SHELL_VARIABLE_REFERENCE = new RegExp(String.raw`^\$(?:[A-Z_][A-Z0-9_]*)$`); 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 +4851,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 +4910,73 @@ 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("+++ ")) { + const target = line.slice(4); + file = target === "/dev/null" ? undefined : target.replace(/^b\//, ""); + } 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 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 +4991,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 +4999,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 +5012,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 +5114,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, PLACEHOLDER_LINES_SHOWN = 200; var init_review = __esm(() => { init_util(); init_git(); @@ -6274,7 +6472,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..9a93a85 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,135 @@ 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, then the name. +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(String.raw`\$\{(?:\([^()\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_]*)$`); + +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; + const wordStart = match.index + whole.length; + // A default nested in an earlier, already-redacted word is gone with it. + if (match.index < copied || SHELL_DIRECTORY_NAMES.has(name) || !isSensitiveKey(name)) continue; + 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); + if (!word || isShellReference(word) || isPlaceholder(word)) continue; + output += `${text.slice(copied, match.index)}${open}${name}${operator}${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: "}" | ")"): number { + const closers: string[] = [close]; + 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 === "`") { + const quoteEnd = closingQuote(text, index + 1, char); + if (quoteEnd === -1) return -1; + index = quoteEnd; + } else if (char === "$" && (text[index + 1] === "{" || text[index + 1] === "(")) { + closers.push(text[index + 1] === "{" ? "}" : ")"); + index += 1; + } else if (char === "(" && closers.at(-1) === ")") { + closers.push(")"); + } else if (char === closers.at(-1)) { + closers.pop(); + if (!closers.length) return index; + } + } + return -1; +} + +function closingQuote(text: string, start: number, quote: string): number { + for (let index = start; index < text.length; index += 1) { + const char = text[index]; + if (char === "\n" || char === "\r") return -1; + if (char === "\\" && quote !== "'") index += 1; + else if (char === quote) return index; + } + 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 is only a reference to something else, which is never a + * literal secret: `$NAME`, `$(command)`, 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. + */ +function isShellReference(value: string): boolean { + 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("${") && !unquoted.startsWith("$(")) return false; + const close = unquoted[1] === "{" ? "}" : ")"; + if (shellWordEnd(unquoted, 2, close) !== unquoted.length - 1) return false; + if (close === ")") return true; + const inner = new RegExp(String.raw`^(?:\([^()\s]*\))?[#!^=~]*${SHELL_NAME_PATTERN}(?.*)$`, "s").exec(unquoted.slice(2, -1)); + const rest = inner?.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)); +} + +/** Whether the bare value starting at `start` is wholly a reference: `KEY=$(gh auth token)`. */ +function startsWithShellReference(text: string, start: number): boolean { + if (text[start] !== "$") return false; + let end: number; + if (text[start + 1] === "{" || text[start + 1] === "(") { + end = shellWordEnd(text, start + 2, text[start + 1] === "{" ? "}" : ")"); + 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 +179,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)) return match; + return `${key}${separator}${placeholderForKey(key)}`; + }, ); for (const [pattern, replacement] of TOKEN_PATTERNS) { @@ -87,6 +223,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..8647918 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,79 @@ 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("+++ ")) { + const target = line.slice(4); + file = target === "/dev/null" ? undefined : target.replace(/^b\//, ""); + } 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; +} + +/** 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 +504,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 +512,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 +525,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..2cb62a7 100644 --- a/packages/sidecar/tests/cli.test.ts +++ b/packages/sidecar/tests/cli.test.ts @@ -669,6 +669,72 @@ 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}", + "echo ${DB_PASSWORD:-$(security find-generic-password -s db -w)}", + "export GITHUB_TOKEN=$(gh auth token)", + '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:-}"], + // 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); + }); + 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..62c555d 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,59 @@ 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,3 +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: ", + "", + ].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: " }, + ]); + }); + test("says so when the repository has no guidelines", () => { const review = open(); expect(fs.existsSync(path.join(review.dir, "guidelines.md"))).toBe(false); From 0bd42ed877a9dfcd7f0d3050ff38682346b3db1b Mon Sep 17 00:00:00 2001 From: Zack Story <2302315+rt2zz@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:09:43 -0700 Subject: [PATCH 2/4] fix(redaction): close the gaps review found in the shell expansion rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `${#NAME:-word}` defaults are redacted: the read exemption and the default rule share one grammar. - Double-quoted strings skip their own expansions, so a quote inside `$(…)` no longer ends the word early and leaves a secret behind. - Only an unquoted command with nothing inside to redact is left alone; quoted commands and `$((…))` arithmetic are redacted as before. - Nesting is bounded and reference words are scanned once, so hostile input stays linear in the clean filter. - The review packet decodes git-quoted and tab-terminated paths. Co-Authored-By: Claude --- docs/redaction.md | 9 +- packages/sidecar/dist/cli.js | 127 ++++++++++++++++++-------- packages/sidecar/src/redaction.ts | 100 ++++++++++++-------- packages/sidecar/src/review.ts | 42 ++++++++- packages/sidecar/tests/cli.test.ts | 19 +++- packages/sidecar/tests/review.test.ts | 14 ++- 6 files changed, 228 insertions(+), 83 deletions(-) diff --git a/docs/redaction.md b/docs/redaction.md index 6441fa5..be309af 100644 --- a/docs/redaction.md +++ b/docs/redaction.md @@ -37,9 +37,12 @@ below — emails, phone numbers, SSNs, and credit card numbers. 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 refers to something else — `$GH_TOKEN`, -`${GH_TOKEN}`, or a command such as `$(gh auth token)`. The value is -fetched when the line runs, and isn't written in the file to strip. +assignment whose whole value names another variable (`$GH_TOKEN`, +`"${GH_TOKEN}"`), and an unquoted command such as +`export GITHUB_TOKEN=$(gh auth token)` with nothing inside it to redact. +Replacing either would leave a line that no longer parses. A quoted command +(`"$(gh auth token)"`) is still redacted: its arguments could be literal +secrets, and `""` still 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 `:`). diff --git a/packages/sidecar/dist/cli.js b/packages/sidecar/dist/cli.js index d3bbe36..164eee1 100755 --- a/packages/sidecar/dist/cli.js +++ b/packages/sidecar/dist/cli.js @@ -1454,21 +1454,24 @@ function redactShellDefaults(text) { let copied = 0; for (const match of text.matchAll(SHELL_DEFAULT_REGEX)) { const [whole, open, name, operator] = match; - const wordStart = match.index + whole.length; - if (match.index < copied || SHELL_DIRECTORY_NAMES.has(name) || !isSensitiveKey(name)) + 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); - if (!word || isShellReference(word) || isPlaceholder(word)) - continue; - output += `${text.slice(copied, match.index)}${open}${name}${operator}${placeholderForKey(name)}`; + 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) { - const closers = [close]; +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 === ` @@ -1477,33 +1480,46 @@ function shellWordEnd(text, start, close) { if (char === "\\") { index += 1; } else if (char === "'" || char === '"' || char === "`") { - const quoteEnd = closingQuote(text, index + 1, char); - if (quoteEnd === -1) + index = closingQuote(text, index + 1, char, depth + 1); + if (index === -1) return -1; - index = quoteEnd; } else if (char === "$" && (text[index + 1] === "{" || text[index + 1] === "(")) { - closers.push(text[index + 1] === "{" ? "}" : ")"); - index += 1; - } else if (char === "(" && closers.at(-1) === ")") { - closers.push(")"); - } else if (char === closers.at(-1)) { - closers.pop(); - if (!closers.length) - return index; + 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) { +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 !== "'") - index += 1; - else if (char === quote) + 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; } @@ -1516,25 +1532,21 @@ function isShellReadOfKey(text, offset, separator) { return true; return text[offset - 1] === "$" && separator.trim().startsWith(":"); } -function isShellReference(value) { +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("${") && !unquoted.startsWith("$(")) - return false; - const close = unquoted[1] === "{" ? "}" : ")"; - if (shellWordEnd(unquoted, 2, close) !== unquoted.length - 1) + if (!unquoted.startsWith("${") || shellWordEnd(unquoted, 2, "}") !== unquoted.length - 1) return false; - if (close === ")") - return true; - const inner = new RegExp(String.raw`^(?:\([^()\s]*\))?[#!^=~]*${SHELL_NAME_PATTERN}(?.*)$`, "s").exec(unquoted.slice(2, -1)); - const rest = inner?.groups?.rest; + 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)); + return word !== undefined && (word === "" || isPlaceholder(word) || isShellReference(word, depth + 1)); } function startsWithShellReference(text, start) { if (text[start] !== "$") @@ -1550,7 +1562,13 @@ function startsWithShellReference(text, start) { } if (end < text.length && !/[\s;&|)]/.test(text[end])) return false; - return isShellReference(text.slice(start, end)); + const value = text.slice(start, end); + if (!value.startsWith("$(")) + return isShellReference(value); + if (value.startsWith("$((")) + return false; + const command = value.slice(2, -1); + return redactText(command, "secrets") === command; } function redactText(input, mode = DEFAULT_REDACTION_MODE) { if (mode === "none") @@ -1637,16 +1655,17 @@ function isLikelyCreditCard(value) { } return sum % 10 === 0; } -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_DIRECTORY_NAMES, SHELL_VARIABLE_REFERENCE, 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, 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"); 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_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(String.raw`\$\{(?:\([^()\s]*\))?[#!^=~]*$`); + 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_]*)$`); AUTHORIZATION_HEADER_REGEX = /\b(authorization\s*[:=]\s*(?:bearer|basic|token)\s+)([^\s"',;`]+)/gi; @@ -4937,8 +4956,7 @@ function addedPlaceholderLines(patch) { if (line.startsWith("diff --git ")) { file = undefined; } else if (line.startsWith("+++ ")) { - const target = line.slice(4); - file = target === "/dev/null" ? undefined : target.replace(/^b\//, ""); + file = patchHeaderPath(line.slice(4)); } else { const hunk = /^@@ -\d+(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/.exec(line); if (hunk) { @@ -4950,6 +4968,36 @@ function addedPlaceholderLines(patch) { } 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 ""; @@ -5114,7 +5162,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, PLACEHOLDER_LINES_SHOWN = 200; +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(); @@ -5124,6 +5172,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 diff --git a/packages/sidecar/src/redaction.ts b/packages/sidecar/src/redaction.ts index 9a93a85..2209616 100644 --- a/packages/sidecar/src/redaction.ts +++ b/packages/sidecar/src/redaction.ts @@ -34,34 +34,41 @@ const BARE_ASSIGNMENT_SECRET_REGEX = new RegExp( // 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, then the name. -const SHELL_EXPANSION_OPEN = String.raw`\$\{(?:\([^()\s]*\))?[!^=~]*`; +// `${`, 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(String.raw`\$\{(?:\([^()\s]*\))?[#!^=~]*$`); +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; - // A default nested in an earlier, already-redacted word is gone with it. - if (match.index < copied || SHELL_DIRECTORY_NAMES.has(name) || !isSensitiveKey(name)) continue; 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); - if (!word || isShellReference(word) || isPlaceholder(word)) continue; - output += `${text.slice(copied, match.index)}${open}${name}${operator}${placeholderForKey(name)}`; + output += text.slice(copied, wordStart); + output += !word || isPlaceholder(word) || isShellReference(word) ? word : placeholderForKey(name); copied = wordEnd; } return output + text.slice(copied); @@ -73,36 +80,48 @@ function redactShellDefaults(text: string): string { * 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: "}" | ")"): number { - const closers: string[] = [close]; +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 === "`") { - const quoteEnd = closingQuote(text, index + 1, char); - if (quoteEnd === -1) return -1; - index = quoteEnd; + index = closingQuote(text, index + 1, char, depth + 1); + if (index === -1) return -1; } else if (char === "$" && (text[index + 1] === "{" || text[index + 1] === "(")) { - closers.push(text[index + 1] === "{" ? "}" : ")"); - index += 1; - } else if (char === "(" && closers.at(-1) === ")") { - closers.push(")"); - } else if (char === closers.at(-1)) { - closers.pop(); - if (!closers.length) return index; + 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: string, start: number, quote: string): number { +/** 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 !== "'") index += 1; - else if (char === quote) return index; + 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; } @@ -119,27 +138,31 @@ function isShellReadOfKey(text: string, offset: number, separator: string): bool } /** - * Whether a value is only a reference to something else, which is never a - * literal secret: `$NAME`, `$(command)`, 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. + * 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): boolean { +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("${") && !unquoted.startsWith("$(")) return false; - const close = unquoted[1] === "{" ? "}" : ")"; - if (shellWordEnd(unquoted, 2, close) !== unquoted.length - 1) return false; - if (close === ")") return true; - const inner = new RegExp(String.raw`^(?:\([^()\s]*\))?[#!^=~]*${SHELL_NAME_PATTERN}(?.*)$`, "s").exec(unquoted.slice(2, -1)); - const rest = inner?.groups?.rest; + 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)); + return word !== undefined && (word === "" || isPlaceholder(word) || isShellReference(word, depth + 1)); } -/** Whether the bare value starting at `start` is wholly a reference: `KEY=$(gh auth token)`. */ +/** + * Whether the unquoted value starting at `start` is wholly a reference, or a + * command substitution with nothing inside to redact: `KEY=$(gh auth token)`. + * Redacting either would leave a line that no longer parses, and the old + * pattern only ever caught a command's first word. Quoted, a command is + * still redacted: `KEY=""` parses, and the quotes may hold a literal. + */ function startsWithShellReference(text: string, start: number): boolean { if (text[start] !== "$") return false; let end: number; @@ -151,7 +174,12 @@ function startsWithShellReference(text: string, start: number): boolean { 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 value = text.slice(start, end); + if (!value.startsWith("$(")) return isShellReference(value); + // Arithmetic is a literal number, not a command. + if (value.startsWith("$((")) return false; + const command = value.slice(2, -1); + return redactText(command, "secrets") === command; } const AUTHORIZATION_HEADER_REGEX = /\b(authorization\s*[:=]\s*(?:bearer|basic|token)\s+)([^\s"',;`]+)/gi; diff --git a/packages/sidecar/src/review.ts b/packages/sidecar/src/review.ts index 8647918..712f108 100644 --- a/packages/sidecar/src/review.ts +++ b/packages/sidecar/src/review.ts @@ -453,8 +453,7 @@ export function addedPlaceholderLines(patch: string): PlaceholderLine[] { if (line.startsWith("diff --git ")) { file = undefined; } else if (line.startsWith("+++ ")) { - const target = line.slice(4); - file = target === "/dev/null" ? undefined : target.replace(/^b\//, ""); + file = patchHeaderPath(line.slice(4)); } else { const hunk = /^@@ -\d+(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/.exec(line); if (hunk) { @@ -467,6 +466,45 @@ export function addedPlaceholderLines(patch: string): PlaceholderLine[] { 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; diff --git a/packages/sidecar/tests/cli.test.ts b/packages/sidecar/tests/cli.test.ts index 2cb62a7..ba22597 100644 --- a/packages/sidecar/tests/cli.test.ts +++ b/packages/sidecar/tests/cli.test.ts @@ -694,9 +694,7 @@ describe("redaction", () => { "x=${SECRET_DIR:-/tmp/s}", "f=${TOKEN_FILE:h}", "echo ${DB_PASSWORD:-$FALLBACK_PASSWORD}", - "echo ${DB_PASSWORD:-$(security find-generic-password -s db -w)}", "export GITHUB_TOKEN=$(gh auth token)", - '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}", @@ -716,6 +714,15 @@ describe("redaction", () => { [": ${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 bare one that would no + // longer parse, with nothing inside to redact, 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="], + ["export GITHUB_TOKEN=$(echo password=hunter2)", "export GITHUB_TOKEN= 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). @@ -735,6 +742,14 @@ describe("redaction", () => { 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)); + 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 62c555d..20da561 100644 --- a/packages/sidecar/tests/review.test.ts +++ b/packages/sidecar/tests/review.test.ts @@ -163,7 +163,7 @@ describe("opening a review", () => { "diff --git a/a.md b/a.md", "--- a/a.md", "+++ b/a.md", - "@@ -1,3 +1,4 @@", + "@@ -1,2 +1,4 @@", "-token: ", "+++ b/fake.md ", "+@@ -1 +1 @@", @@ -180,12 +180,24 @@ describe("opening a review", () => { "+++ 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: " }, ]); }); From 68065d1c5db10244b2d8a72d0653c9ea1b1ac3f6 Mon Sep 17 00:00:00 2001 From: Zack Story <2302315+rt2zz@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:13:49 -0700 Subject: [PATCH 3/4] fix(redaction): exempt only a command's name, never what main redacted A command substitution was left alone when redacting its inside changed nothing, but `$(cat<< --- docs/redaction.md | 10 ++++----- packages/sidecar/dist/cli.js | 23 +++++++++------------ packages/sidecar/src/redaction.ts | 33 ++++++++++++++---------------- packages/sidecar/tests/cli.test.ts | 10 ++++++--- 4 files changed, 37 insertions(+), 39 deletions(-) diff --git a/docs/redaction.md b/docs/redaction.md index be309af..cc16242 100644 --- a/docs/redaction.md +++ b/docs/redaction.md @@ -38,11 +38,11 @@ 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 an unquoted command such as -`export GITHUB_TOKEN=$(gh auth token)` with nothing inside it to redact. -Replacing either would leave a line that no longer parses. A quoted command -(`"$(gh auth token)"`) is still redacted: its arguments could be literal -secrets, and `""` still parses. +`"${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. A command's arguments are not checked, so keep +literal 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 `:`). diff --git a/packages/sidecar/dist/cli.js b/packages/sidecar/dist/cli.js index 164eee1..8edd30b 100755 --- a/packages/sidecar/dist/cli.js +++ b/packages/sidecar/dist/cli.js @@ -1548,12 +1548,14 @@ function isShellReference(value, depth = 0) { const word = /^:?[-=+](?.*)$/s.exec(rest)?.groups?.word; return word !== undefined && (word === "" || isPlaceholder(word) || isShellReference(word, depth + 1)); } -function startsWithShellReference(text, start) { +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] === "{" || text[start + 1] === "(") { - end = shellWordEnd(text, start + 2, text[start + 1] === "{" ? "}" : ")"); + if (text[start + 1] === "{") { + end = shellWordEnd(text, start + 2, "}"); if (end === -1) return false; end += 1; @@ -1562,13 +1564,7 @@ function startsWithShellReference(text, start) { } if (end < text.length && !/[\s;&|)]/.test(text[end])) return false; - const value = text.slice(start, end); - if (!value.startsWith("$(")) - return isShellReference(value); - if (value.startsWith("$((")) - return false; - const command = value.slice(2, -1); - return redactText(command, "secrets") === command; + return isShellReference(text.slice(start, end)); } function redactText(input, mode = DEFAULT_REDACTION_MODE) { if (mode === "none") @@ -1582,10 +1578,10 @@ function redactText(input, mode = DEFAULT_REDACTION_MODE) { if (valueQuote === '"' && isShellReference(value)) return match; return `${keyQuote}${key}${keyQuote}${separator}${valueQuote}${placeholderForKey(key)}${valueQuote}`; - }).replace(BARE_ASSIGNMENT_SECRET_REGEX, (match, key, separator, _value, offset, text) => { + }).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)) + if (startsWithShellReference(text, offset + key.length + separator.length, value)) return match; return `${key}${separator}${placeholderForKey(key)}`; }); @@ -1655,7 +1651,7 @@ function isLikelyCreditCard(value) { } return sum % 10 === 0; } -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, 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_-]*`; @@ -1668,6 +1664,7 @@ var init_redaction = __esm(() => { 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_./+-]+\)?$/; 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; diff --git a/packages/sidecar/src/redaction.ts b/packages/sidecar/src/redaction.ts index 2209616..f613d75 100644 --- a/packages/sidecar/src/redaction.ts +++ b/packages/sidecar/src/redaction.ts @@ -156,30 +156,27 @@ function isShellReference(value: string, depth = 0): boolean { return word !== undefined && (word === "" || isPlaceholder(word) || isShellReference(word, depth + 1)); } -/** - * Whether the unquoted value starting at `start` is wholly a reference, or a - * command substitution with nothing inside to redact: `KEY=$(gh auth token)`. - * Redacting either would leave a line that no longer parses, and the old - * pattern only ever caught a command's first word. Quoted, a command is - * still redacted: `KEY=""` parses, and the quotes may hold a literal. - */ -function startsWithShellReference(text: string, start: number): boolean { +// `$(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<< { + (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)) return match; + if (startsWithShellReference(text, offset + key.length + separator.length, value)) return match; return `${key}${separator}${placeholderForKey(key)}`; }, ); diff --git a/packages/sidecar/tests/cli.test.ts b/packages/sidecar/tests/cli.test.ts index ba22597..57dbe7d 100644 --- a/packages/sidecar/tests/cli.test.ts +++ b/packages/sidecar/tests/cli.test.ts @@ -716,13 +716,16 @@ describe("redaction", () => { ["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 bare one that would no - // longer parse, with nothing inside to redact, is left alone. + // 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="], - ["export GITHUB_TOKEN=$(echo password=hunter2)", "export GITHUB_TOKEN= password="], + ["PASSWORD=$(cat<<"], + ["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). @@ -747,6 +750,7 @@ describe("redaction", () => { 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); }); From d3ba322d1ac3bdfb625b124e97d29445aaf7d634 Mon Sep 17 00:00:00 2001 From: Zack Story <2302315+rt2zz@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:16:41 -0700 Subject: [PATCH 4/4] fix(redaction): keep a token-shaped command name idempotent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `PASSWORD=$(sk-…)` passed the command-name exemption, then the token patterns replaced the name, and the next pass no longer exempted `$()`. A placeholder now passes as a command name. The docs no longer claim a command's arguments go unchecked. Co-Authored-By: Claude --- docs/redaction.md | 7 ++++--- packages/sidecar/dist/cli.js | 2 +- packages/sidecar/src/redaction.ts | 6 ++++-- packages/sidecar/tests/cli.test.ts | 1 + 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/docs/redaction.md b/docs/redaction.md index cc16242..7cd5c34 100644 --- a/docs/redaction.md +++ b/docs/redaction.md @@ -40,9 +40,10 @@ 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. A command's arguments are not checked, so keep -literal secrets out of them. A quoted command (`"$(gh auth token)"`) is -still redacted whole, since `""` parses. +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 `:`). diff --git a/packages/sidecar/dist/cli.js b/packages/sidecar/dist/cli.js index 8edd30b..0c97fbd 100755 --- a/packages/sidecar/dist/cli.js +++ b/packages/sidecar/dist/cli.js @@ -1664,7 +1664,7 @@ var init_redaction = __esm(() => { 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_./+-]+\)?$/; + 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; diff --git a/packages/sidecar/src/redaction.ts b/packages/sidecar/src/redaction.ts index f613d75..3a0b27f 100644 --- a/packages/sidecar/src/redaction.ts +++ b/packages/sidecar/src/redaction.ts @@ -160,8 +160,10 @@ function isShellReference(value: string, depth = 0): boolean { // 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 { diff --git a/packages/sidecar/tests/cli.test.ts b/packages/sidecar/tests/cli.test.ts index 57dbe7d..bc3043c 100644 --- a/packages/sidecar/tests/cli.test.ts +++ b/packages/sidecar/tests/cli.test.ts @@ -723,6 +723,7 @@ describe("redaction", () => { ['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="],