fix: several Windows-checkout issues found while running the platform's own checks locally - #173
Open
aakashkumarjaiswal0987-ops wants to merge 4 commits into
Conversation
…inks git rev-parse --show-toplevel always returns forward-slash paths, even on Windows, while path.resolve()/path.sep use backslashes there. The guard compared the two directly with startsWith(), so the containment check never matched on Windows and every local Markdown link in the repo was reported as missing, even though the files exist. Wrapping the git output in resolve() normalizes it to the platform's native separators before the comparison. Verified locally: the guard fails with 58 false positives on a clean Windows checkout before this change, and passes with 0 failures after it.
dependencies-security.test.mjs rebuilt the pnpm store path for the patched image-size@2.0.2 package by string-interpolating the patch hash read from pnpm-lock.yaml directly into a hardcoded "patch_hash=<hash>" directory name. On this Windows checkout the actual installed directory is named "patch_hash_72198d98..." (a different separator character AND a different hash), because patches/image-size@2.0.2.patch checks out with CRLF line endings here, so pnpm computes a different content hash for it than the lockfile recorded on the machine that produced the lockfile. The test then threw ERR_MODULE_NOT_FOUND trying to import a path that never existed, which is a false negative on a DoS-hardening regression test: the underlying security patch (rejecting non-advancing ICNS/HEIF/JXL boxes in dist/fromFile.cjs) is verified to be correctly applied in the actual installed package, but the test itself could not find it to check. Fix: scan node_modules/.pnpm for an entry matching image-size@2.0.2_patch_hash[=_]<hash> instead of constructing the exact path from the lockfile hash. This verifies the invariant the test actually cares about — a patched image-size@2.0.2 is installed — without depending on an exact hash match or separator character that can legitimately differ by platform/line-ending handling. Verified locally by toggling the change off/on with git stash in the same session: 0/3 passing before, 3/3 passing after, both times confirmed against the same real installed package.
aakashkumarjaiswal0987-ops
changed the title
Both prior fixes in this branch trace back to the same root cause: this repository has no .gitattributes, so whether a checked-out file ends up with LF or CRLF line endings depends entirely on a contributor's local core.autocrlf setting. On a Windows machine with the (very common) core.autocrlf=true default, this silently introduces CRLF bytes into files the reviewed source never had them in, breaking anything that hashes or pattern-matches file content byte-for-byte: - guards/markdown-links.mjs and patches/image-size@2.0.2.patch (fixed earlier in this branch) - scripts/release.test.mjs and scripts/release.integration.test.mjs, which assert exact regex matches against generated workflow YAML and fail with 11 additional false negatives once CRLF is introduced Pinning `* text=auto eol=lf` makes every tracked text file check out with LF regardless of the contributor's autocrlf setting, so the guards, patches, and workflow-content tests behave identically for every contributor regardless of platform — closing off this entire class of Windows-only false positive/negative, not just the two instances already fixed in this branch.
…'t parsed as remote hosts Both tar invocations in the release path build paths from os.tmpdir()/path.resolve(), which return drive-letter paths like C:\Users\... on Windows. GNU tar (bundled with Git for Windows, commonly the only tar on a Windows PATH) interprets a colon in a path as a remote-host separator unless told otherwise, so it tries to open an SSH-style connection to a host literally named "C" and fails with "Cannot connect to C: resolve failed" instead of reading the local path. Affected: - scripts/release.integration.test.mjs: the test fixture that builds a fake package tarball to exercise publishTarball()/registryState() - scripts/release.mjs inspectTarball(): reads package/package.json out of the real tarball before publishing — this is production release logic, not just test infrastructure, so this bug would also break running the actual release script from a Windows machine, not only the test suite --force-local tells tar to always treat the path as local regardless of any colon in it. It's a no-op on Linux/macOS where this ambiguity doesn't exist, so this doesn't change behavior in CI. Verified: reproduced the exact failure in isolation with a minimal tar invocation against a C:\ path, confirmed --force-local resolves it, then confirmed scripts/release.test.mjs and scripts/release.integration.test.mjs both regain previously-failing tests after applying it (13/31 passing before this fix in this session -> 24/31 after; remaining failures are a separate, pre-existing Windows npm-spawn issue noted in the PR description, not caused by this change).
aakashkumarjaiswal0987-ops
changed the title
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
2 participants
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.
Summary
While setting up this project locally (no Docker available, so only the Node-only paths: guards, typecheck, and the dev test suite), I found and fixed three related issues that are all invisible in CI because every job in
.github/workflows/ci.ymlruns onubuntu-latest. I also found a fourth issue that I'm reporting but deliberately did not fix — see the note at the end.1.
guards/markdown-links.mjs— false-positive "missing" links on WindowsThe guard resolves the repository root via
git rev-parse --show-toplevel, which always returns forward-slash paths, even on Windows, then compares it against locally-resolved link targets withstartsWith. Node'spath.resolve()/path.sepreturn backslashes on Windows, so the containment check never matches — every local Markdown link in the repo (README.md, CONTRIBUTING.md, docs) gets reported as missing, even though the files exist.Fix: wrap the git output in
resolve()so it normalizes to the platform's native separators before comparison.2. No
.gitattributes— checkout line endings depend on contributor configThis repo has no
.gitattributes, so whether a checked-out file ends up LF or CRLF depends entirely on a contributor's localcore.autocrlf. On Windows with the commoncore.autocrlf=truedefault, this silently introduces CRLF bytes into files the reviewed source never had them in, and breaks anything that hashes or pattern-matches file content byte-for-byte. This is the root cause behind issue #3 below and behind 11 additional (already-fixed-by-normalization) false negatives inscripts/release.test.mjs/scripts/release.integration.test.mjsthat compared regex patterns against generated workflow YAML.Fix: added
.gitattributespinning* text=auto eol=lf, so every tracked text file checks out identically regardless of contributor platform.3.
scripts/dependencies-security.test.mjs— false-negative DoS-regression testThis test verifies the local security patch for
image-size@2.0.2(rejecting non-advancing ICNS/HEIF/JXL boxes) is applied, by string-interpolating the patch hash frompnpm-lock.yamlinto a hardcodednode_modules/.pnpm/image-size@2.0.2_patch_hash=<hash>path. Before fix #2,patches/image-size@2.0.2.patchchecked out with CRLF on Windows, so pnpm computed a different content hash for it than the lockfile recorded, and the installed directory didn't match the hardcoded path — throwingERR_MODULE_NOT_FOUNDinstead of actually checking the patch.I confirmed the underlying patch was correctly applied in the real installed package the whole time (
grep "does not advance" .../dist/fromFile.cjsfinds all three guards) — this was purely a test bug, not a broken patch, but it's a false negative on a security-relevant test, which seemed worth fixing regardless of #2.Fix: scan
node_modules/.pnpmfor an entry matchingimage-size@2.0.2_patch_hash[=_]<hash>instead of constructing the exact path, so the test checks the actual invariant ("a patched image-size@2.0.2 is installed") without depending on an exact hash/separator.4.
tarinvoked without--force-localmisparses Windows drive letters as remote hostsscripts/release.mjs(inspectTarball, real release logic — not just a test) andscripts/release.integration.test.mjs's fixture both calltarwith paths frompath.resolve()/os.tmpdir(), which are drive-letter paths on Windows (e.g.C:\Users\...). GNU tar (what Git for Windows puts onPATH) interprets the colon as a remote-host separator unless told otherwise, and fails withtar (child): Cannot connect to C: resolve failedinstead of reading the local path. This means running the actual release script — not only its tests — would fail on a Windows machine today.Fix: pass
--force-localto both invocations. It's a no-op on Linux/macOS, so no CI behavior changes.Found, not fixed:
spawnSync("npm", ...)inrelease.mjsthrows ENOENT on WindowsThe remaining 7 failing release tests all fail with
spawnSync npm ENOENT. On Windows,npmonPATHisnpm.cmd, andspawnSync/execFileSyncdon't resolve.cmdshims withoutshell: true(confirmed:npm.cmddirectly throwsEINVALhere;shell: trueworks but Node emits aDEP0190warning because unescaped shell args are a real risk). This is insidepublishTarball(), which also builds npm auth config and handlesNODE_AUTH_TOKEN— I didn't feel comfortable proposing a fix to that code path without your input on the right tradeoff (shell: truewith careful arg escaping vs. across-spawn-style dependency vs. something else), so I'm flagging it rather than guessing. Happy to take a pass at it if you'd like a specific direction.Verification
Every fix above was verified by toggling it off and back on with
git stash/git stash popin the same terminal session and re-running the affected check each time, to confirm the before/after wasn't coincidental — not just eyeballing a single run.Test plan
node guards/run.mjs— 2/2 passing (was 1/2)node --test scripts/dependencies-security.test.mjs— 3/3 passing (was 0/3), underlying patch confirmed genuinely applied independent of the test bugnode --test scripts/release.test.mjs scripts/release.integration.test.mjs— 24/31 passing (was 13/31); remaining 7 are the documented npm ENOENT issue, unrelated to this PR's fixespnpm typecheck— 16/16 packages, unaffected.gitattributesnormalizes to what Linux already produces;resolve()and--force-localare no-ops when paths already use/)