fix(package): scan semantic content for internal references - #618
fix(package): scan semantic content for internal references#618mldangelo-oai wants to merge 10 commits into
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…ckage-semantic-content-scan
…ckage-semantic-content-scan
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
kmbroai
left a comment
kmbroai
left a comment
There was a problem hiding this comment.
Critical review
Reviewed head 35cc277e85843e61a6cd55a051f4425bc9fdd6d7.
Recommendation: keep semantic-content scanning; no new blocking correctness defect found in the reviewed changes. Decoding compressed archive bytes as text can reject a clean package because of coincidental byte sequences. Scanning filenames, actual text, and decompressed bundled code fixes the right problem without weakening the intended disclosure check.
Correctness
check-package.mjs validates the closed archive format before extraction, compares the native tar listing with parsed paths, verifies the extracted tree and sizes, and cleans up in finally. Scanning complete headers retains metadata coverage, while PNGs remain restricted to the known digest instead of arbitrary text interpretation. The Brotli helper checks decompression consumption, so trailing data is not silently ignored.
Passing the already-read archive on stdin avoids Windows drive-letter interpretation and a separate path reread. Keeping the caller's working directory also avoids accidentally selecting a relative-PATH executable from beside the archive. The --ignore-zeros use is acceptable here specifically because the preceding parser proves the remainder is zero padding; it should not be copied to an unconstrained archive reader.
Simplification and necessity
The semantic scanner is the essential fix. The extra parser/extractor agreement is defensible only as validation of this package's controlled plain-ustar contract—not as a general tar library. Preserve that narrow scope. An npm pack-format change should cause one explicit compatibility update, not accumulate ad hoc support for every extension.
There are three native tar passes plus the in-memory parser and filesystem walk. I would avoid adding further independent interpretations. If this becomes costly to maintain, evaluate a single maintained archive reader with equivalent path/type/metadata guarantees; do not simply remove checks while retaining native extraction. The existing expanded-size bound is unchanged by this PR and is not a newly introduced scan-input cap.
Verification
Ran the five focused package helper suites: 25 passed, 0 failed, 72 assertions, including binary-marker false positives, decompressed content, tar forms/listing, provenance, and timeout helpers. Linux with Bun 1.3.14 / Node 22.13.1 and cached dependencies. I did not independently rebuild the full npm archive, run installed-package smoke, or execute native Windows/macOS packaging, so the author's broader artifact claims are not my verification results.
…ckage-semantic-content-scan
…ckage-semantic-content-scan
…ckage-semantic-content-scan
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![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: 1239beeb4e
ℹ️ 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".
|
|
||
| assertNoInternalReference(header); | ||
| if ( | ||
| header[156] !== 0x30 || |
There was a problem hiding this comment.
When an otherwise valid npm tarball uses the POSIX-compatible NUL typeflag for a regular file, this condition rejects it even though the system tar command accepts and extracts it; the previous parser explicitly treated both NUL and "0" as regular-file encodings. This representation-only restriction can therefore break check:package for installable archives produced by a different tar writer, so accept both regular-file typeflags while scanning their semantic content.
AGENTS.md reference: AGENTS.md:L19-L22
Useful? React with 👍 / 👎.
2 participants
Summary
Scan the semantic contents of the npm package so marker-like compressed bytes do not reject a clean release. Continue checking paths, headers, text and decompressed bundled code.
Changes
Testing
Final release integration updates the synthetic archive fixture to include the executable MCP launcher now required by main, sharing its plugin contract across tests. The two failing positive cases pass after the fixture correction; the permission check and archive/disclosure protections are unchanged.
Five focused suites, seed 618: 27 passed. The npm size-field unit and end-to-end regressions failed before the fix and passed afterward.
Fresh npm 11.12.1 and pnpm 11.9.0 artifacts both passed the 282-entry package check. Full installed npm smoke passed with 118 plugin files.
Types, generated models, formatting and build passed.
The full pnpm
check-packageentrypoint, including installed smoke, passed. Full suite, native Windows and Node 22 were not rerun.The prior workflow was cancelled; its four red checks were aggregate coverage gates. New-head CI is left for a separate pass.
Final main refresh (
fd98a90): package 0.1.21 includes the MCP launcher-permission fix; SDK implementation and bundled payload are unchanged. Types/model generation, formatting, build, 44 focused tests, static artifact verification and full installed-package smoke passed, including MCP initialization. CI was not awaited.Risk and rollout
This is a package-validation change, not a general tar parser. Unsupported tar forms still fail; path/type checks, extraction agreement, expanded-size bounds, PNG digests and complete Brotli consumption remain. Keep the existing squash-only integration guidance for this branch. No package was published and no PR was merged.
Public disclosure review
Existing automated comments link to account-gated Codex settings, so the second attestation is left unchecked. This update uses synthetic fixtures and a GitHub noreply commit identity; other authors' comments are unchanged.