[refactor] Split helpers/file.ts along its actual seams - #108
Merged
Conversation
file.ts conflated four unrelated concerns in one 670+ line module: generic fs primitives, archive extraction/creation (incl. filesMove's package-format sorting), OS-default install-directory resolution, and privileged installer execution/admin elevation. Split into fs.ts, archive.ts, paths.ts, and installer.ts along those seams. file.ts itself becomes a pure re-export barrel (`export * from './fs.js'` etc.), so every existing import of '../helpers/file.js' - across ManagerLocal.ts's remaining callers, all test files, and the package's public index.ts/index-browser.ts exports - keeps working unchanged with zero behavior difference. Updated the primary internal consumers (ManagerLocal.ts, ConfigLocal.ts, RegistryLocal.ts, admin.ts, packageLocal.ts) to import from the specific new module they actually need instead of the barrel, since that's the actual point of the split - a reader can now tell what a file touches (generic fs vs. archive handling vs. privileged execution) from its imports alone. NOTE: local verification of this branch is currently blocked - see conversation for details on an unrelated live-registry-data-drift issue being fixed on other branches first. `npx tsc --noEmit` and `eslint .` both pass cleanly; full `npm test` has not yet been re-verified after rebasing onto the updated main.
…export them After splitting helpers/file.ts, ManagerLocal.ts imports isAdmin/ runCliAsAdmin/fileInstall from the new installer.ts and fileOpen from fs.ts, but the "elevates when unprivileged" tests still spied on helpers/file.ts's re-export barrel (`vi.spyOn(fileHelpers, 'isAdmin')` etc). vi.spyOn patches a property on the exact namespace object it's given; a re-exporting barrel is a different namespace object even though `export *` makes the same underlying binding reachable through it. The mocks were silently not intercepting anything - the real, unmocked isAdmin()/runCliAsAdmin()/fileOpen() ran during test execution. This is what produced real macOS admin prompts and root-owned files under test/ during local runs of this branch (confirmed: this fix alone took a run from 3 failures citing "User did not grant permission" / real sudo-prompt errors to 218/218 passing with zero root-owned output). Retargeted the three affected spies to installerHelpers/ fsHelpers - the modules ManagerLocal.ts now actually imports these from - and added a comment on the first affected test explaining the barrel/spyOn interaction so it isn't reintroduced by a future test.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…anch CodeQL flagged this PR with 1 new high-severity js/zipslip alert at archive.ts's AdmZip Windows-fallback branch. The code itself is byte-identical to what's on main (pre-existing, still-open alert #16 at the old file.ts:67) - not a vulnerability this refactor introduced, but CodeQL treats it as "new" since the code moved to a new file/path. The validation (isSafeArchiveEntryPath()) was already correct at runtime, but CodeQL's dataflow analysis wasn't recognizing it as a guard: each entry was validated and written in the same forEach iteration, one entry at a time. The adjacent .7z branch a few lines below - which isn't flagged - instead validates every entry up front and only writes afterward if all of them pass. Restructured the AdmZip branch to match that already-safe shape: map entries to their sanitized names, find any unsafe one and throw before writing anything, then write. Same validation logic, same error message, same result - verified via the full test suite (218/218 passing) - just reshaped so the guard-then-sink relationship is unambiguous to static analysis.
The previous restructuring (validate-all-then-write-all) didn't clear the CodeQL alert either - it was still tracing entry.entryName through to writeFileSync. Comparing against the adjacent .7z branch (which isn't flagged): that branch's untrusted entry.name only ever reaches a validation check - 7zip-min's unpack() gets the whole archive file and target directory (both trusted), never an individual entry name, so nothing untrusted reaches a filesystem-writing call in our own code. In the AdmZip branch, sanitizedName genuinely does flow into our own writeFileSync/dirCreate calls, so no restructuring of *when* the check runs changes that - what matters is *how* CodeQL can verify the guard. isSafeArchiveEntryPath()/dirContains() are correct at runtime, but the check reaches the sink through a call three functions deep, which static analysis can't verify as a guard. Replaced it with the containment check inlined directly ahead of the sink, computed on outputPath itself (the exact value passed to writeFileSync/dirCreate moments later) via path.resolve()/startsWith() - the same primitives dirContains() uses internally, just written at the point of use instead of indirected through a helper. Same behavior, verified via the full test suite (218/218 passing).
Still not clearing the CodeQL alert with the inline check from the
previous commit. Hypothesis: CodeQL's local guard-node recognition for
this query traces direct control flow (if/for/while) within a function
reliably, but not necessarily across a closure boundary - the
containment check and the sink it guards were both inside a callback
passed to Array.prototype.forEach(), an indirection through a library
function between the parameter binding and the guard. Replaced
entries.forEach(entry => {...}) with a plain for...of loop over the
same entries array - identical logic and behavior (218/218 tests still
passing), but the guard now sits in this function's own direct control
flow instead of inside a passed-in callback.
…pattern
Fetched the full js/zipslip rule documentation via the code-scanning
API - its own "GOOD" example checks the untrusted entry path directly
for a ".." substring (fileName.indexOf('..') == -1) immediately before
the write, not a resolved-path containment check. Every previous
attempt here used the latter (path.resolve() + startsWith()), which is
the more robust check (also catches absolute-path entries and
path-separator edge cases) but apparently isn't what this query's
guard-recognizer verifies on its own.
Added the substring check as an explicit first guard, directly on
sanitizedName, immediately before the existing containment check -
defense in depth, not a replacement; the containment check remains the
actually load-bearing defense. 218/218 tests still passing.
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
helpers/file.tswas a 670+ line module conflating four unrelated concerns: generic fs primitives, archive extraction/creation (incl.filesMove()'s package-format sorting), OS-default install-directory resolution, and privileged installer execution/admin elevation. Impossible to import or reason about one concern without pulling in all the others.fs.ts(generic fs primitives),archive.ts(archive extraction/creation +filesMove, grouped together sincefilesMove()only ever runs immediately afterarchiveExtract()on content it just produced),paths.ts(OS-default install directories, per specification.md), andinstaller.ts(privileged installer execution + the admin-elevation bridge).file.tsitself becomes a pure re-export barrel (export * from './fs.js'etc.), so every existing import of'../helpers/file.js'- across remaining callers and the package's publicindex.ts/index-browser.tsexports - keeps working unchanged.ManagerLocal.ts,ConfigLocal.ts,RegistryLocal.ts,admin.ts,packageLocal.ts) to import from the specific new module they actually need instead of the barrel, since that's the actual point of the split - a reader can now tell what a file touches (generic fs vs. archive handling vs. privileged execution) from its imports alone.This is item 6 of the architectural review in
review.md(Structural Warning #5).Moving
ManagerLocal.ts's imports ofisAdmin/runCliAsAdmin/fileInstall/fileOpenoff thefile.jsbarrel broke three tests in a way that wasn't a type error or an assertion failure:vi.spyOn(fileHelpers, 'isAdmin')(spying on the barrel's re-exported view) silently stopped intercepting the call, becauseManagerLocal.tsnow importsisAdmindirectly frominstaller.js- a different namespace object from the barrel, even thoughexport *makes them resolve to the same underlying binding at import time.vi.spyOnpatches a property on the specific object you hand it; re-export barrels don't get that patch applied retroactively.The practical effect: three "elevates when unprivileged" tests that are supposed to run with elevation mocked out instead ran the real
isAdmin()/runCliAsAdmin()/fileOpen()during a local test run - which triggered a genuine macOS admin-password prompt (confirmed by the repo owner, who saw and approved it, not realizing it was part of an automated test run) and left root-owned files under the gitignoredtest/scratch directory. This was caught, reproduced, and fixed as part of this PR (commit "Retarget elevation/fileOpen mocks...") - verified by re-running the full suite before/after: before, 3 failures citing real sudo-prompt errors; after, 218/218 passing with zero root-owned output (find test -user rootempty).Consequence for review: this is a real, demonstrated failure mode of splitting a module that other tests mock via
vi.spyOnon its old namespace - worth double-checking for any other module split in this codebase that touches functions covered by existingvi.spyOn(someBarrel, 'x')calls elsewhere in the test suite. I checked this repo's test suite for other spies targeting the four relocated functions (grep -rn "spyOn.*isAdmin\|spyOn.*runCliAsAdmin\|spyOn.*fileInstall\|spyOn.*fileOpen\|spyOn.*archiveExtract\|spyOn.*filesMove" tests/) and confirmedManagerLocal.test.tswas the only affected file.Test plan
npx tsc --noEmit: clean.npm run check(format, lint, build, test): 218/218 tests, 18/18 files pass.npm audit: 0 vulnerabilities.sudo-prompterrors), then verified the fix resolves it (218/218 passing,find test -user rootreturns nothing).🤖 Generated with Claude Code