fix(contract): seal finding write-ups and the hardening portfolio - #240
fix(contract): seal finding write-ups and the hardening portfolio#240rohanpoudel2 wants to merge 3 commits into
Conversation
Every entry in manifest.scan.artifacts is digest-verified when a scan loads, and every coverage receipt is required to be one of those sealed artifacts. finding.writeup.reportPath and manifest.scan.hardening.portfolioPath got neither treatment. Both were opened and immediately closed, an existence check only, so the human-readable write-up a person actually reads could be replaced wholesale after sealing and the scan still loaded as intact. The producer built the sealed set from findings.json, coverage.json and the coverage receipt refs, and handled write-ups separately in _require_derived_writeup_files, which only asserted that the files exist. Add the referenced derived Markdown to the sealed set so its contents are covered by the digest verification that already runs for every listed artifact. A path that is also a coverage receipt keeps a single record, because duplicate artifact paths are rejected on load. On the SDK side, require every referenced derived document to be one of the sealed artifacts, so a manifest cannot reference a write-up while leaving it out of the seal. Gate that on the manifest listing at least one derived document, so scans sealed before this keep loading with the existence check they were sealed with instead of becoming unreadable. That gate rests on manifest authenticity, which is what the seal already assumes: artifact digests live in the manifest, so a rewritten manifest can always re-seal itself, and the guarantee being restored is that artifacts on disk match what an authentic manifest recorded. Fixes openai#230
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ff045fde5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (sealsDerivedDocuments) { | ||
| const normalized = safeRelativePath(document.path, document.context); | ||
| if (!artifactPaths.has(normalized)) { | ||
| throw new ContractValidationError( | ||
| `Derived document is missing from sealed artifacts: ${document.path}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
When a sealed manifest references multiple derived documents but lists only one in scan.artifacts, this branch makes loadContract reject it, but the bundled Python paths do not apply the equivalent all-or-none gate: _prepare_scan_finalization performs only existence checks, while _read_sealed_scan and validate_scan_contract.py validate artifact hashes and coverage receipts without requiring derived references to be sealed. Consequently, direct CLI exports, workbench exports/comparisons, and Python contract validation can accept the same partially unsealed contract that the SDK rejects, leaving this integrity gap open outside the SDK; add the same legacy-compatible gate to the shared Python validation path.
Useful? React with 👍 / 👎.
| for ref in _derived_document_refs(scan, findings): | ||
| if _require_safe_relative_path(ref, "derived document path") in sealed_paths: | ||
| continue | ||
| sealed_artifacts.append(_artifact_record(scan_dir, ref, "text/markdown")) |
There was a problem hiding this comment.
For every completed scan with hardening guidance or finding write-ups, this loop now adds those documents to the digest seal, but the generated report in report_projection.py still labels the portfolio “unsealed,” and the bundled reporting/artifact guidance continues to describe these outputs as unsealed or revisable. A user or agent following that published contract can edit a document after completion and then find that loading or exporting the scan fails its digest check. Update the generated wording and bundled instructions as part of this contract change.
Useful? React with 👍 / 👎.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Sealing the referenced write-ups and hardening portfolio only closed the integrity gap for SDK readers. `loadContract` rejected a manifest that referenced several derived documents but sealed only some of them, while the bundled Python paths still accepted it: `_read_sealed_scan` and `validate_scan_contract.py` verified artifact digests and coverage receipts without requiring the referenced documents to be among them, and `_prepare_scan_finalization` only checked that the files existed. Direct CLI exports, SARIF projection, workbench exports and comparisons, and Python contract validation therefore loaded a partially unsealed contract that the SDK refused. `_validate_sealed_derived_documents` now applies the same all-or-none gate next to `_validate_sealed_coverage_receipts`, and runs from every place that validates a seal: `_read_sealed_scan`, both branches of `_prepare_scan_finalization`, and `validate_scan_contract.py`. The condition matches `validateSeal` exactly: when the manifest lists at least one referenced derived document in `scan.artifacts`, every referenced document must be there; otherwise the manifest predates the change and keeps the existence check it was sealed with, so old sealed scans stay readable. The published contract also stopped matching the producer. The generated report called the portfolio unsealed, and the bundled guidance told authors these documents were unsealed or revisable and must not join the sealed artifact list - following it, an agent would edit a document after completion and then find loading or exporting fails the digest check. The report now says the portfolio is covered by the scan seal, and the reporting and artifact references, the hardening skill, and the diff-scan skill describe finalization sealing these documents. Wording about the `report.md` projection and the unsealed draft manifest is unchanged, because both remain outside the seal.
2 participants
Fixes #230
I filed that issue rather than opening a PR because the fix spans the bundled plugin and the migration story for already-sealed scans was yours to choose. This implements it with the most conservative migration I could find — no schema change, no version bump, and no previously-sealed scan becomes unreadable. If you would rather gate on the producer version, that shape is discussed under "Why the gate is data-derived" below.
Problem
Every entry in
manifest.scan.artifactsis digest-verified on load, and every coverage receipt is required to be one of those sealed artifacts:finding.writeup.reportPathandmanifest.scan.hardening.portfolioPathgot neither treatment — they were opened and immediately closed:That is an existence check. The write-up is the artifact a person actually reads, and it could be replaced wholesale after sealing while the scan still loaded as intact.
It was by construction rather than an oversight in one layer.
finalize_scan_contract.pybuilt the sealed set from three sources —findings.json,coverage.json, and the coverage receipt refs — and handled write-ups separately in_require_derived_writeup_files, which only asserts the files exist.Change
Producer — add the referenced derived Markdown to the sealed set, so its contents fall under the digest verification that already runs for every listed artifact. A path that is also a coverage receipt keeps a single record, because duplicate artifact paths are rejected on load.
SDK — require every referenced derived document to be one of the sealed artifacts, so a manifest cannot reference a write-up while leaving it outside the seal.
The two halves fix different things, and it is worth being precise about which does what:
artifactswould otherwise fall back to the existence check.Migration
Tightening the SDK unconditionally would reject every scan sealed by an older plugin, and
loadContractaccepts scans without aScanExpectation, so historical scans on disk do load today andexportdepends on that. So the SDK requirement is gated on the manifest listing at least one derived document. New scans list them and are strictly verified; older manifests list none and keep the existence check they were sealed with.The gate is not weaker than the alternative. It rests on manifest authenticity, and that is what the seal already assumes: artifact digests live in the same manifest they protect, so anyone who can rewrite the manifest can re-seal anything. The seal's real guarantee is that the artifacts on disk match what an authentic manifest recorded, and that is exactly the guarantee this restores for write-ups.
The gate also degrades in the safe direction. Stripping one write-up from a manifest that seals others leaves the gate active, so the missing one is rejected. Only stripping every derived entry reaches the lenient path, and that is manifest rewriting.
Why the gate is data-derived rather than version-gated
The obvious alternative is to gate on
manifest.scan.producer.version. I did not take it for two reasons.It is no stronger. An attacker who can rewrite the manifest to drop artifact entries can equally rewrite
producer.versionto an older value, so both gates rest on the same assumption.It is more invasive. It would mean choosing and hardcoding a plugin version, bumping
plugin.jsonandBUNDLED_PLUGIN_VERSION, and touching the release cut — version and release mechanics I would rather not decide from outside the canonical repo, especially so soon after #183. Say the word and I will switch it.Not a bug, for the record
Repeating this from the issue since it is the natural next question: a manifest cannot dodge verification by omitting
findings.jsonorcoverage.jsonfromscan.artifacts. The schema'sallOf/contains/maxContains: 1clauses force both into the array. That part is sound, andmediaTypeis only constrained to a non-empty string, sotext/markdownon the new records needs no schema change.Verification
Four tests, and each half was reverted independently to confirm the tests fail for the right reason.
scan-recovery.test.ts— a new test drives the real workbench end to end: a draft with a finding write-up and a hardening portfolio is sealed, and the resulting manifest is asserted to carry both as artifacts with the correct SHA-256 andtext/markdown. With only the producer change reverted it fails, reporting the write-up artifact asundefined:contract.test.ts— three new tests: a write-up replaced after the seal covered it is rejected, a hardening portfolio replaced after the seal covered it is rejected, and a referenced write-up left out ofartifactsis rejected with the new message. With only the SDK change reverted, exactly the third fails, which is the correct split — the first two are the producer change being exercised through the pre-existing digest loop:The pre-existing test
accepts regular derived artifacts and a schema-valid scope summarybuilds a manifest that references a write-up and a portfolio without sealing either, which is the historical shape. It still passes untouched, so the migration path is covered by a test that predates this change.Full suite on this base: 730 pass / 5 skip / 0 fail.
pnpm run typesandpnpm run formatare clean, andfinalize_scan_contract.pycompiles underpython3 -m py_compile.