From 9d49c4b13586656cb127fc69d380fc64b9f703b1 Mon Sep 17 00:00:00 2001 From: Morgan Lunt Date: Mon, 8 Jun 2026 15:28:35 -0700 Subject: [PATCH] code-modernization: close remaining credential-leak paths A red-team pass found four ways credential values still reached shareable artifacts after the initial redaction: - the remediation patch: a diff removing a hardcoded secret carries the raw value on its '-' lines by construction. harden now splits output: non-credential hunks in the shareable security_remediation.patch, credential hunks in a gitignored security_remediation.local.patch with comment-only placeholders in the shareable file - the other four agents had no secret-handling rules. legacy-analyst (hardcoded-config evidence in tech-debt findings), business-rules-extractor (credentials recorded as rule parameters), test-engineer (legacy literals becoming committed test fixtures), and architecture-critic (quoted code in notes files) now all mask values and cite file:line; assess's tech-debt prompt and ASSESSMENT.md masking now cover every section, not just Security Findings - non-git projects: a .gitignore protects nothing under SVN/Mercurial. Both commands now refuse --show-secrets without git and write the quarantine file to ~/.modernize// outside the project tree - the patch-apply instruction was wrong in both documented layouts (symlinked legacy/ broke relative paths). Patches are now written with project-root-relative paths and applied from the project root Also: --show-secrets is now position-independent in both commands, and the README documents the full model. --- plugins/code-modernization/README.md | 2 +- .../agents/architecture-critic.md | 6 ++ .../agents/business-rules-extractor.md | 9 +++ .../agents/legacy-analyst.md | 9 +++ .../agents/test-engineer.md | 9 +++ .../commands/modernize-assess.md | 24 ++++-- .../commands/modernize-extract-rules.md | 2 +- .../commands/modernize-harden.md | 74 +++++++++++++------ 8 files changed, 103 insertions(+), 32 deletions(-) diff --git a/plugins/code-modernization/README.md b/plugins/code-modernization/README.md index 9c1903b7..12a89322 100644 --- a/plugins/code-modernization/README.md +++ b/plugins/code-modernization/README.md @@ -26,7 +26,7 @@ mkdir -p legacy && ln -s /path/to/your/legacy/codebase legacy/billing ## Secret handling -Legacy systems routinely contain live credentials, and assessment artifacts get committed and shared. `/modernize-assess` and `/modernize-harden` therefore **never write discovered credential values into findings files** — findings cite `file:line` with a masked preview (`AKIA****`). When credentials are found, a per-credential inventory (type, location, blast radius, rotation recommendation) is written to `analysis//SECRETS.local.md`, which the commands gitignore before writing. Pass `--show-secrets` to include raw values in that quarantine file (and only there). If you ran an earlier version of this plugin on a real system, check whether `analysis/` artifacts containing credentials were committed or shared, and rotate anything that was. +Legacy systems routinely contain live credentials, and assessment artifacts get committed and shared. **Every agent in this plugin masks credential values** — findings, rule-card parameters, architecture notes, and test fixtures cite `file:line` with a masked preview (`AKIA****`), never the value. When credentials are found, a per-credential inventory (type, location, blast radius, rotation recommendation) is written to `analysis//SECRETS.local.md`, which the commands gitignore before writing; on non-git projects the quarantine file goes to `~/.modernize//` instead. `/modernize-harden` splits its remediation diff so credential-removal hunks (which necessarily contain the raw value) land in a gitignored `security_remediation.local.patch`, never the shareable patch. Pass `--show-secrets` to include raw values in the quarantine file (and only there). If you ran an earlier version of this plugin on a real system, check whether `analysis/` artifacts containing credentials were committed or shared, and rotate anything that was. ## Commands diff --git a/plugins/code-modernization/agents/architecture-critic.md b/plugins/code-modernization/agents/architecture-critic.md index 08ba03b1..cf45ec6a 100644 --- a/plugins/code-modernization/agents/architecture-critic.md +++ b/plugins/code-modernization/agents/architecture-critic.md @@ -29,6 +29,12 @@ For **transformed code**: - Does the test suite actually pin behavior, or just exercise code paths? - What would the on-call engineer need at 3am that isn't here? +## Secret handling (mandatory) + +When a finding quotes code containing a credential, key, token, or +connection string, mask the value (`'Pr0d****'`) and cite `file:line` — +findings get appended verbatim to committed notes files. + ## Output Findings ranked **Blocker / High / Medium / Nit**. Each with: what, where, diff --git a/plugins/code-modernization/agents/business-rules-extractor.md b/plugins/code-modernization/agents/business-rules-extractor.md index 31eca621..c0d0bb21 100644 --- a/plugins/code-modernization/agents/business-rules-extractor.md +++ b/plugins/code-modernization/agents/business-rules-extractor.md @@ -40,6 +40,15 @@ of the technology, skip it. from structure/names), **Low** (ambiguous; needs SME). 6. If confidence < High, write the exact question an SME must answer. +## Secret handling (mandatory) + +Rule parameters sometimes *are* credentials — hardcoded passwords in auth +checks, API keys in partner-service calls, connection strings in batch +routines. Record the **rule**, never the **value**: write the parameter as +`` with at most a 2–4 character +preview. Rule cards flow into briefs and steering decks; a raw credential +in a parameter list is a leak. + ## Output format One "Rule Card" per rule (see the format in the `/modernize-extract-rules` diff --git a/plugins/code-modernization/agents/legacy-analyst.md b/plugins/code-modernization/agents/legacy-analyst.md index b22e5736..a6fd5e6c 100644 --- a/plugins/code-modernization/agents/legacy-analyst.md +++ b/plugins/code-modernization/agents/legacy-analyst.md @@ -32,6 +32,15 @@ and explain it in terms a modern engineer can act on. - **Note what's missing.** Unhandled error paths, TODO comments, commented-out blocks, magic numbers — these are signals about history and risk. +## Secret handling (mandatory) + +Legacy code is full of live credentials, and your findings get copied into +shareable reports. When the evidence for a finding — hardcoded config, +dead code, debt, an interface payload — includes a credential, API key, +token, connection string, or private key, **never reproduce the value**. +Cite `file:line` with a masked preview (`VALUE 'Pr0d****'`, +`password=****`). The finding is the practice, not the value. + ## Output format Default to structured markdown: tables for inventories, Mermaid for graphs, diff --git a/plugins/code-modernization/agents/test-engineer.md b/plugins/code-modernization/agents/test-engineer.md index 9f49e883..07057ad0 100644 --- a/plugins/code-modernization/agents/test-engineer.md +++ b/plugins/code-modernization/agents/test-engineer.md @@ -28,6 +28,15 @@ someone thinks it should do) so that a rewrite can be proven equivalent. `@Disabled("pending RULE-NNN")` / `@pytest.mark.skip` / `it.todo()` — never deleted. +## Secret handling (mandatory) + +Never copy credential-like literals — passwords, API keys, tokens, +connection strings — from legacy code into test fixtures. Tests live in +the deliverable codebase and get committed. Substitute clearly-fake values +of the same shape and length and note the substitution in a comment. +Anything a test genuinely needs live (e.g. a real database connection for +a dual-run harness) is read from an environment variable, never inlined. + ## Output Idiomatic tests for the requested target stack (JUnit 5 / pytest / Vitest / diff --git a/plugins/code-modernization/commands/modernize-assess.md b/plugins/code-modernization/commands/modernize-assess.md index fba8b8cc..b098f02a 100644 --- a/plugins/code-modernization/commands/modernize-assess.md +++ b/plugins/code-modernization/commands/modernize-assess.md @@ -5,7 +5,9 @@ argument-hint: [--show-secrets] | --portfolio **Mode select.** If `$ARGUMENTS` starts with `--portfolio`, run **Portfolio mode** against the directory that follows. Otherwise run **Single-system -mode** against `legacy/$1`. +mode** against the system dir. Parse flags positionally-independently: +`--show-secrets` may appear before or after the system dir — the system +dir is the first non-flag token. --- @@ -108,7 +110,9 @@ Spawn three subagents **in parallel**: 2. **legacy-analyst** — "Identify technical debt in legacy/$1: dead code, deprecated APIs, copy-paste duplication, god objects/programs, missing error handling, hardcoded config. Return the top 10 findings ranked by - remediation value, each with file:line evidence." + remediation value, each with file:line evidence. If evidence contains a + credential value, mask it per your secret-handling rules — never quote + it." 3. **security-auditor** — "Scan legacy/$1 for security vulnerabilities: injection, auth weaknesses, hardcoded secrets, vulnerable dependencies, @@ -150,14 +154,20 @@ security-auditor found any hardcoded credentials: 1. Ensure `analysis/.gitignore` exists and contains the line `SECRETS.local.md` (create or append as needed). If the project is a git repo, verify with `git check-ignore -q analysis/$1/SECRETS.local.md` - before writing any findings. -2. Write `analysis/$1/SECRETS.local.md`: one row per credential — masked - preview, `file:line`, credential type, what it grants access to, + — do not write any findings until the check passes. If there is **no + git repo** (check for `.svn`/`.hg`/`CVS` too — a `.gitignore` protects + nothing under another VCS): refuse `--show-secrets` and write + `SECRETS.local.md` to `~/.modernize/$1/` instead of the project tree, + telling the user where it went and why. +2. Write `SECRETS.local.md`: one row per credential — masked preview, + `file:line`, credential type, what it grants access to, production/test guess, rotation recommendation. Only if the user passed `--show-secrets`, add the raw value column here — this file only, never ASSESSMENT.md. -3. In ASSESSMENT.md, the Security Findings section lists credential - findings with masked values only, plus a one-line pointer: +3. Masking applies to **every section of ASSESSMENT.md**, whichever agent + produced the finding — the Technical Debt section quotes hardcoded + config; those quotes follow the same masking rule as Security Findings. + The Security Findings section adds a one-line pointer: "Credential inventory in SECRETS.local.md (gitignored; not for sharing)." Create `analysis/$1/ASSESSMENT.md` with these sections: diff --git a/plugins/code-modernization/commands/modernize-extract-rules.md b/plugins/code-modernization/commands/modernize-extract-rules.md index 1fe7979c..e842867c 100644 --- a/plugins/code-modernization/commands/modernize-extract-rules.md +++ b/plugins/code-modernization/commands/modernize-extract-rules.md @@ -46,7 +46,7 @@ Merge the three result sets. Deduplicate. For each distinct rule, write a When Then [And ] -**Parameters:** +**Parameters:** `> **Edge cases handled:** **Suspected defect:** **Confidence:** High | Medium | Low — diff --git a/plugins/code-modernization/commands/modernize-harden.md b/plugins/code-modernization/commands/modernize-harden.md index ce3a784b..64e36f3f 100644 --- a/plugins/code-modernization/commands/modernize-harden.md +++ b/plugins/code-modernization/commands/modernize-harden.md @@ -3,8 +3,11 @@ description: Security vulnerability scan with a reviewable remediation patch — argument-hint: [--show-secrets] --- -Run a **security hardening pass** on `legacy/$1`: find vulnerabilities, rank -them, and produce a reviewable patch for the critical ones. +Run a **security hardening pass** on the legacy system: find +vulnerabilities, rank them, and produce a reviewable patch for the +critical ones. Parse arguments flag-independently: the system dir +(referred to as `$1` below) is the first non-flag token in `$ARGUMENTS`; +`--show-secrets` may appear anywhere. This command never edits `legacy/` — it writes findings and a proposed patch to `analysis/$1/`. The user reviews and applies (or not). @@ -14,19 +17,25 @@ to `analysis/$1/`. The user reviews and applies (or not). Findings files get shared, committed, and pasted into decks — discovered credential values must never land in them. Before any scanning: -1. Ensure `analysis/.gitignore` exists and contains the line - `SECRETS.local.md`. Create the file or append the line if missing. +1. Ensure `analysis/.gitignore` exists and contains the lines + `SECRETS.local.md` and `*.local.patch`. Create the file or append the + missing lines. 2. If the project is a git repo, verify with `git check-ignore -q analysis/$1/SECRETS.local.md` — if that exits non-zero, fix the ignore rule before proceeding. Do not write any - findings until this check passes (skip the check only if there is no - git repo). + findings until this check passes. +3. **If there is no git repo** (check for `.svn`/`.hg`/`CVS` too — a + `.gitignore` protects nothing under another VCS): refuse + `--show-secrets`, and write `SECRETS.local.md` and any `.local.patch` + file to `~/.modernize/$1/` instead of the project tree, telling the + user where they went and why. -All secret values in every artifact this command produces are **masked** -(`AKIA****`, `password=****`) and cited by `file:line`. The one exception: -if the user passed `--show-secrets`, raw values may appear in -`analysis/$1/SECRETS.local.md` (gitignored above) and nowhere else — -never in SECURITY_FINDINGS.md or the patch commentary. +All secret values in every shareable artifact this command produces are +**masked** (`AKIA****`, `password=****`) and cited by `file:line`. Raw +values may appear in exactly two places, both gitignored: the +`*.local.patch` remediation hunks (unavoidably — see Remediate) and, only +with `--show-secrets`, `SECRETS.local.md`. Never in SECURITY_FINDINGS.md +or patch commentary. ## Scan @@ -62,23 +71,38 @@ not for sharing)." ## Remediate For each **Critical** and **High** finding, draft a minimal, targeted fix. -Do **not** edit `legacy/` — write all fixes as a single unified diff to -`analysis/$1/security_remediation.patch`, with a comment line above each -hunk citing the finding ID it addresses (`# SEC-001: parameterize the query`). +Do **not** edit `legacy/` — write fixes as unified diffs with **paths +relative to the project root** (`legacy/$1/...`), applied from the project +root, with a comment line above each hunk citing the finding ID it +addresses (`# SEC-001: parameterize the query`). + +**Credential findings split into two files.** A diff that removes a +hardcoded secret necessarily contains the raw value on its `-` and +context lines — that cannot go in the shareable patch: + +- `analysis/$1/security_remediation.patch` (shareable) — every + non-credential hunk, plus for each credential finding a comment-only + placeholder: `# SEC-NNN: credential remediation — hunk in + security_remediation.local.patch (gitignored; not for sharing)`. +- `analysis/$1/security_remediation.local.patch` (gitignored in Step 0) — + the real, applyable hunks for credential findings only. Add a **Remediation Log** section to SECURITY_FINDINGS.md mapping each -finding ID → one-line summary of the proposed fix and the patch hunk that -implements it. +finding ID → one-line summary of the proposed fix and which patch file +carries the hunk. ## Verify -Spawn the **security-auditor** again to **review the patch** against the -original code: +Spawn the **security-auditor** again to **review both patches** against +the original code: -"Review analysis/$1/security_remediation.patch against legacy/$1. For each +"Review analysis/$1/security_remediation.patch and +analysis/$1/security_remediation.local.patch against legacy/$1. For each hunk: does it fully remediate the cited finding? Does it introduce new -vulnerabilities or change behavior beyond the fix? Return one verdict per -hunk: RESOLVES / PARTIAL / INTRODUCES-RISK, with a one-line reason." +vulnerabilities or change behavior beyond the fix? Confirm no raw +credential values appear anywhere in the shareable patch. Return one +verdict per hunk: RESOLVES / PARTIAL / INTRODUCES-RISK, with a one-line +reason." Add a **Patch Review** section to SECURITY_FINDINGS.md with the verdicts. If any hunk is PARTIAL or INTRODUCES-RISK, revise the patch and re-review. @@ -87,8 +111,12 @@ If any hunk is PARTIAL or INTRODUCES-RISK, revise the patch and re-review. Tell the user the artifacts are ready: - `analysis/$1/SECURITY_FINDINGS.md` — findings, remediation log, patch review -- `analysis/$1/security_remediation.patch` — review, then apply if appropriate - with `git -C legacy/$1 apply ../../analysis/$1/security_remediation.patch` +- `analysis/$1/security_remediation.patch` — review, then apply **from the + project root**: `git apply analysis/$1/security_remediation.patch` + (if `legacy/$1` is a symlink, use `git apply --unsafe-paths` or apply + with `patch -p0` from the project root) +- `analysis/$1/security_remediation.local.patch` — the credential fixes; + apply the same way, and rotate the affected credentials regardless - Re-run `/modernize-harden $1` after applying to confirm resolution Suggest: `glow -p analysis/$1/SECURITY_FINDINGS.md`