fix(scripts): accept CRLF tar listings when inspecting npm tarballs - #217
Open
rohanpoudel2 wants to merge 2 commits into
Open
fix(scripts): accept CRLF tar listings when inspecting npm tarballs#217rohanpoudel2 wants to merge 2 commits into
rohanpoudel2 wants to merge 2 commits into
Conversation
check-package.mjs validated the `tar -tvzf` listing with a single multiline regex (`/^[^d-]/mu`) to assert every entry begins with `-` (regular file) or `d` (directory). With the `m` flag, JavaScript treats both `\r` and `\n` as line terminators, so on a CRLF listing (as emitted by tar on Windows) `^` matches the position between `\r` and `\n`, and the bare `\n` is then evaluated as the first character of that "line". It is neither `d` nor `-`, so the regex matched and the script rejected a perfectly valid tarball with: Error: npm tarball contains a non-regular entry (symbolic or hard link, device, or pipe). The listing is already normalized correctly a few lines later via `split(/\r?\n/u)`; the fix does that split once and checks each line's first character individually instead of regexing the whole blob, matching how the rest of the script already handles line endings. Audited the remainder of the script for the same class of bug; no other multiline regex or line-handling logic assumes LF. Added a regression test that packs a real tarball with `pnpm pack` and runs check-package.mjs through a `tar` shim that reproduces the CRLF verbose listing, plus a companion test proving a real non-regular entry is still rejected. Fixes openai#32
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #32
Problem
scripts/check-package.mjsvalidated the wholetar --ignore-zeros -tvzflisting in one shot:With the
mflag, JavaScript treats both\rand\nas line terminators. On a CRLF listing,^therefore matches the position between the\rand the\n, and the\nis inspected as if it were the first character of an entry line. It is neitherdnor-, so a perfectly valid archive is rejected:Windows contributors whose
taremits CRLF cannot runpnpm run check:packageat all, as reported in #32 with bsdtar 3.5.2.The bug is only in this early check. A few lines further down the same listing is already normalized correctly with
split(/\r?\n/u).Change
Hoist the split that already exists and check each line individually, so the entry-type check and the entry-count check now read the same normalized lines:
I audited the rest of the script for the same class of defect — the
sizeFieldcheck, theentriessplit,unsafePath, the launcher-permission split,internalMarker, and the.br/PNG filename tests. None of them combine themflag with a multi-line input, so this was the only occurrence.Verification
New
tests-ts/check-package.test.tspacks the real tarball and runs the real script as a child process, with atarshim onPATHthat forwards every invocation to the real binary untouched except-tvzf, whose output it CRLF-terminates. That reproduces the Windows listing behavior exactly while leaving the archive itself valid.Against the unfixed script:
With the fix:
A second test shims the listing with
sed 's#^-#l#'to confirm a genuinely non-regular entry is still rejected, so the fix cannot silently turn into a no-op.Full suite: 719 pass / 5 skip / 0 fail.
pnpm run typesandpnpm run formatare clean.The shims are POSIX shell scripts, so the tests use the repository's existing
testPosixhelper and skip on Windows — the same patterntests-ts/container-entrypoint.test.tsuses. The Windows listing behavior is what is being reproduced, and the fix itself is platform-independent.