diff --git a/plugins/codex-security/references/final-report.md b/plugins/codex-security/references/final-report.md index 5886eff61..8fa17bd1e 100644 --- a/plugins/codex-security/references/final-report.md +++ b/plugins/codex-security/references/final-report.md @@ -118,7 +118,7 @@ Affected lines must include the root broken control or dangerous sink line when Then render these subsections under each finding: - `#### Summary` - - Explain why the issue matters, what the vulnerable path is, and why the current controls are insufficient. + - Start with how to reproduce the issue and what happens in the product, then explain why the code causes that behavior. - Wrap code identifiers, RPC names, functions, types, fields, parameters, configuration keys, and literal values in single backticks. - `#### Root Cause` - State the violated security invariant and explain exactly how the implementation breaks it. diff --git a/plugins/codex-security/references/finding-detail-fields.md b/plugins/codex-security/references/finding-detail-fields.md index da6979888..df5d5fa82 100644 --- a/plugins/codex-security/references/finding-detail-fields.md +++ b/plugins/codex-security/references/finding-detail-fields.md @@ -4,6 +4,9 @@ For every reportable finding in `findings.json`, preserve the validated reasonin ## Writing Rules +- Lead the title and `summary` with the user action and product impact. +- Use `attackPath.summary` to briefly explain how to reproduce the issue. +- Explain how the code causes that product behavior in `rootCause.summary`. Use plain language and avoid repetition. - Wrap RPC names, functions, types, fields, parameters, configuration keys, literal identifiers, and short expressions in single backticks. For example: `environment/add`, `environmentId`, `execServerUrl`, and `EnvironmentManager::upsert_environment()`. - Keep code out of prose. Put source snippets in `codeEvidence[].code`, then reference them from the section that explains why the snippet matters. The workspace consolidates those referenced snippets under **Root cause** so the violated invariant and its source proof stay together. - Root cause must be a source-backed walkthrough, not a verdict paragraph. Start with the code where user-controlled data is declared, decoded, or read; follow each meaningful call, transformation, or state transition; then show the missing control, dangerous operation, and later consumer when it affects impact. @@ -106,11 +109,7 @@ The following shape shows how to encode the `environment/add` reserved-environme "validation": { "method": "static source trace", "summary": "The source trace confirms that an `environment/add` caller controls both inputs, the RPC forwards them unchanged, and runtime insertion accepts `local`.", - "evidenceRefs": [ - "rpc-input", - "rpc-forward", - "runtime-upsert" - ], + "evidenceRefs": ["rpc-input", "rpc-forward", "runtime-upsert"], "assertions": [ "The runtime path lacks the reserved-ID check present during startup.", "Inserting `local` replaces the existing `HashMap` entry." @@ -139,11 +138,7 @@ The following shape shows how to encode the `environment/add` reserved-environme "entrypoint": "experimental `environment/add` RPC", "outcome": "future operations selected for `local` are routed to the remote executor" }, - "evidenceRefs": [ - "rpc-forward", - "runtime-upsert", - "default-lookup" - ], + "evidenceRefs": ["rpc-forward", "runtime-upsert", "default-lookup"], "impact": { "level": "medium", "why": "Later commands and filesystem requests selected for `local` can be routed to the attacker-controlled remote executor." diff --git a/plugins/codex-security/skills/track-findings/SKILL.md b/plugins/codex-security/skills/track-findings/SKILL.md index 93608afa0..e546e2c16 100644 --- a/plugins/codex-security/skills/track-findings/SKILL.md +++ b/plugins/codex-security/skills/track-findings/SKILL.md @@ -132,6 +132,8 @@ Treat failed requests, incomplete exact-identifier searches, unread plausible ma ### 4. Preview The Exact Writes +Follow the Writing Rules in `../../references/finding-detail-fields.md`. Explain the problem, how to reproduce it, the cause, the proposed fix, and validation. Put source locations and scan identifiers last. + Present a compact review before any mutation. For every finding show: - finding id and fingerprint diff --git a/sdk/typescript/src/publication.ts b/sdk/typescript/src/publication.ts index dfa3bef36..03f8a6719 100644 --- a/sdk/typescript/src/publication.ts +++ b/sdk/typescript/src/publication.ts @@ -112,7 +112,50 @@ function renderFindingDescription( ): string { const { coverage } = contract; const { scan } = contract.manifest; - const lines = [ + const lines = ["## Summary", "", finding.summary]; + + if (finding.attackPath?.summary !== undefined) { + lines.push("", "## Reproduction summary", "", finding.attackPath.summary); + } + + const rootCause = finding.rootCause; + if (typeof rootCause === "string") { + lines.push("", "## Root cause", "", rootCause); + } else if (rootCause !== undefined) { + lines.push("", "## Root cause", "", rootCause.summary); + if (rootCause.code !== undefined) { + lines.push("", fencedCode(rootCause.code, rootCause.language)); + } + } + + lines.push("", "## Remediation", "", finding.remediation); + + const validation = finding.validation; + const limits = [ + ...new Set([ + ...(validation?.limitations ?? []), + ...(validation?.counterEvidence ?? []), + ...(finding.attackPath?.limitations ?? []), + ]), + ]; + if (validation?.summary || validation?.method || limits.length > 0) { + lines.push("", "## Validation", ""); + if (validation?.summary) lines.push(validation.summary, ""); + if (validation?.method) lines.push(`**Method:** ${validation.method}`, ""); + lines.push(...limits.map((limit) => `- ${limit}`)); + } + + if (finding.codeEvidence !== undefined && finding.codeEvidence.length > 0) { + lines.push("", "## Source-code evidence"); + for (const evidence of finding.codeEvidence) { + lines.push("", ...renderCodeEvidence(scan.target, evidence)); + } + } + + lines.push( + "", + "---", + "", "## Codex Security finding", "", `**Scan ID:** ${scan.id}`, @@ -148,30 +191,7 @@ function renderFindingDescription( ...finding.locations.map((location) => renderLocation(scan.target, location), ), - "", - "## Summary", - "", - finding.summary, - ]; - - const rootCause = finding.rootCause; - if (typeof rootCause === "string") { - lines.push("", "## Root cause", "", rootCause); - } else if (rootCause !== undefined) { - lines.push("", "## Root cause", "", rootCause.summary); - if (rootCause.code !== undefined) { - lines.push("", fencedCode(rootCause.code, rootCause.language)); - } - } - - if (finding.codeEvidence !== undefined && finding.codeEvidence.length > 0) { - lines.push("", "## Source-code evidence"); - for (const evidence of finding.codeEvidence) { - lines.push("", ...renderCodeEvidence(scan.target, evidence)); - } - } - - lines.push("", "## Remediation", "", finding.remediation); + ); return `${lines.join("\n")}\n`; } diff --git a/sdk/typescript/tests-ts/publication.test.ts b/sdk/typescript/tests-ts/publication.test.ts index 5581d118e..ac969d4e5 100644 --- a/sdk/typescript/tests-ts/publication.test.ts +++ b/sdk/typescript/tests-ts/publication.test.ts @@ -150,9 +150,58 @@ describe("scan publication preparation", () => { expect(issue.description).toContain("**Uploaded:** 2026-06-01T10:30:00Z"); expect(issue.description).toContain("without containment validation"); expect(issue.description).toContain("Normalize destinations"); + expect( + issue.description.indexOf("without containment validation"), + ).toBeLessThan(issue.description.indexOf("**Scan ID:**")); + expect(issue.description.indexOf("Normalize destinations")).toBeLessThan( + issue.description.indexOf("**Scan ID:**"), + ); expect(issue.description).not.toContain("/blob/deadbeef/"); }); + test("keeps the reproduction and validation limits ahead of scan metadata", async () => { + const scanDirectory = await copyExample(); + const findingsPath = join(scanDirectory, "findings.json"); + const findings = await readJson(findingsPath); + const finding = findings.findings[0]!; + const reproduction = + "Import an archive containing ../escape.txt. The importer should reject it; source review shows it writing outside the selected directory. This was not run."; + const unrun = "No runtime reproduction was run."; + const attackPath = { + summary: reproduction, + limitations: [unrun, "Files must be writable by the importing process."], + }; + const validation = { + method: "Source review", + summary: "The importer passes the entry name to the filesystem write.", + limitations: [unrun], + counterEvidence: ["The user must first choose to import the archive."], + }; + finding.attackPath = attackPath; + finding.validation = validation; + await writeJson(findingsPath, findings); + await reseal(scanDirectory); + + const { description } = ( + await prepareScanPublication(scanDirectory, DESTINATION) + ).issues[0]!; + + const metadata = description.indexOf("**Scan ID:**"); + for (const text of [ + finding.summary, + reproduction, + validation.summary, + validation.method, + ...validation.limitations, + ...validation.counterEvidence, + ...attackPath.limitations, + ]) { + expect(description).toContain(text); + expect(description.indexOf(text)).toBeLessThan(metadata); + } + expect(description.split(unrun)).toHaveLength(2); + }); + test("uses the canonical scan directory beneath an aliased parent", async () => { const root = await mkdtemp( join(tmpdir(), "codex-security-publication-alias-"),