Skip to content

fix(cli): enforce native asset containment - #84

Merged
viktar-b merged 4 commits into
mainfrom
codex/windows-assets
Sep 30, 2026
Merged

viktar-b merged 4 commits into
mainfrom
codex/windows-assets

Conversation

@viktar-b

@viktar-b viktar-b commented Sep 30, 2026 •

Copy link
Copy Markdown
Owner

Why

Windows asset capture can accept a directory junction that now resolves outside the entry source directory. path.relative() returns a backslash-separated parent path, but the containment check only recognizes ../.

Scope

  • Use the native path separator in captureAssets while retaining realpath resolution, exact-parent and absolute-path rejection.
  • Exercise real asset capture with contained ..notes names and a directory link retargeted to a sibling after its hash was recorded. Windows runs the junction case without a skip.
  • Check that captured bytes remain copied and that changed bytes or an incorrect media signature still fail.

Blast Radius

The change affects CLI asset capture for HTML, PDF and development reports. It preserves the function signature and existing hash and media checks.

Verification

  • macOS: all 80 CLI tests passed. The five new filesystem cases also passed before the fix, as expected for the existing POSIX check.
  • CLI lint and typecheck passed.
  • All 23 selected HTML, PDF, development-server and demo-preparation integration tests passed with an independently installed wheel.
  • Native Windows before/after verification is pending. The test-only commit precedes the production fix.

@vercel

vercel Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
calculation-source-object Ready Ready Preview Sep 30, 2026 9:55am UTC

Request Review

@viktar-b

viktar-b commented Sep 30, 2026 •

Copy link
Copy Markdown
Owner Author

Independent verifier verdict: PASS+NOTES

Reviewed PR 84 at head 3f81de69f1746029cf6ac5c59b8546ee3b0f93b0 against base 91282ab0836dfbba579b2f5e65c1d672119205a2. Stable patch ID: 7ea1adfc8fe5accf9ec66cf92936a8bb7aa56af9.

I found no correctness, API, ownership, or test-boundary defect. The production change replaces the POSIX-only ../ prefix with ..${sep} inside the existing realpathSync and relative containment check. Exact-parent and absolute-result rejection, regular-file validation, byte capture, SHA-256 validation, media checks, and DocumentPreparationError wrapping remain unchanged. No public API or test-only production helper was added.

Filesystem and behavior review

The five new cases call captureAssets over disposable files validated through ExecutionPayloadSchema. They prove that contained ..notes/diagram.svg and ..diagram.svg names remain valid, an initially contained directory link or Windows junction is rejected after retargeting to a sibling, already captured bytes remain copied after the source changes, a later hash mismatch fails, and matching bytes with an incorrect PNG declaration fail. The fixture records every temporary root before later work, and afterEach removes all recorded roots recursively.

The retarget case keeps identical SVG bytes inside and outside, so only containment can explain the expected rejection. Resolving the asset to its real path before the native relative check also keeps link retargeting from bypassing later file and hash validation.

Live proof

  • After the checkout's npm ci completed, npm run build:lib passed.
  • npm test --workspace @cs-object/cli passed 6 files and 80 tests in this independent checkout, including all five asset cases.
  • CLI typecheck passed. CLI lint exited successfully with one pre-existing warning in src/arguments.ts and one pre-existing informational diagnostic in tests/dev.test.ts; neither file is in this diff.
  • The owner's retained receipts match the head: 80 CLI tests passed, and 23 selected HTML, PDF, development-server, and demo-preparation integration tests passed with a wheel installed in that worktree's own venv. The owner's macOS baseline log passes all five new cases with the old separator check, which makes the native Windows comparison necessary.
  • Native run 36695699063 checked out the fixed exact head on Windows Server 2025 with Node 24.21.0, Python 3.11.9, and PowerShell 7.6.6. Job 109823019537 passed all 5 asset tests.
  • Native baseline job 109823019292 checked out test harness head f4de4e767747cb4d8cb3bb329b9448cc3f3b2a11, which retains the old production check. Four cases passed. The sole failure was the retargeted-junction assertion at assets.test.ts:118: captureAssets did not throw. This is the intended current-product failure.
  • The diagnostic workflow is red because its PowerShell baseline wrapper printed the expected-failure receipt but retained exit code 1. The fixed job itself succeeded; no fixed-head test failed.

Diff and comment audit

The two-file diff adds no TypeScript, lint, or test suppressions and no phase or constraint comments. pstack:no-comments results: 0 deletions, 0 restorations, 0 encoding offers, and no open comment constraints. The PR has only the Vercel status comment and no unresolved review finding. No production file was edited during this review.

Merge state and limits

The reviewed head was clean against the reviewed base when verification completed. The PR is now behind current main after unrelated merges. Update it before merge, compare the resulting stable patch ID, and rerun current-head gates; a changed patch needs fresh review. Required checks for this head report quality, isolation, dependency-review, and pr-title as passing; the classifier intentionally skipped installed-packages. CodeQL's Actions, JavaScript/TypeScript, and Python analyses pass. The owner supplied the affected installed-wheel integration receipt separately.

This native proof covers Windows Server 2025 and PowerShell 7. It does not qualify Windows 11, PowerShell 5.1, foreground Ctrl+C behavior, or human PDF inspection. Those broader claims are outside this asset-containment PR.

Commands and receipts

npm run build:lib; npm test --workspace @cs-object/cli; npm run lint --workspace @cs-object/cli; npm run typecheck --workspace @cs-object/cli; inspect the exact diff and stable patch ID; audit the owner's report.md, cli-tests.log, integration.log, and baseline-posix.log; inspect gh run view 36695699063 --job ... --log; and query gh pr checks 84 --required plus current comments and reviews.

@viktar-b

Copy link
Copy Markdown
Owner Author

Root verification PASS at 08f243d9f5c6343cadb81111e4a83e9075ca71e6 against main c710feff9768e980d55ce50f7bc6eaf8984859ab.

The independent non-author verdict is recorded above. The refreshed diff retains stable patch ID 7ea1adfc8fe5accf9ec66cf92936a8bb7aa56af9. Its base now includes the bindings, npm launcher and UTF-8 repairs. All five focused filesystem tests pass after the refresh.

Native Windows proof demonstrates the original retargeted-junction escape and passes all five cases at the reviewed fixed patch. The independent verifier also passed 80 CLI tests and audited 23 affected installed-wheel integration cases. Current-head quality, isolation, dependency-review, PR-title and CodeQL pass. Installed-packages is skipped by the maintained CI policy; the affected installed acceptance receipt was independently audited. GitHub reports CLEAN with no unresolved threads. Normal protected merge is authorized for this head only.

@viktar-b
viktar-b merged commit 76a34b5 into main Sep 30, 2026
12 checks passed
@viktar-b
viktar-b deleted the codex/windows-assets branch September 30, 2026 10:03

This branch was successfully deployed

1 active deployment
Preview — 08f243d9 Deployed Sep 30, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant