[fix] Let open() propagate errors instead of swallowing them - #98
Merged
Conversation
open() was the only mutating method on ManagerLocal that caught its own errors and downgraded them to a `false` return - every other method (install, uninstall, installDependency, ...) throws on failure. Since logging is a no-op unless debug mode is explicitly enabled, a caller in the default configuration got `false` with zero information about why opening a package failed. Flagged in an internal spec-compliance audit. Removed the try/catch so failures (a missing file, a fileOpen() error, ...) propagate as thrown Errors like everywhere else in the class. Also adds the first test coverage open() has ever had: not found, version not found, not installed, and a regression test that specifically asserts a fileOpen() failure now throws instead of returning false. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
… Windows
GitHub Advanced Security (CodeQL) flagged js/shell-command-constructed-from-input
on this PR: fileOpen()'s Windows branch called
execFileSync('cmd.exe', ['/c', 'start', '""', filePath]) with a filePath
that (via ManagerLocal.open(), which this PR touches) ultimately comes
from a package's `open` field in registry metadata - untrusted,
community-submitted content.
execFileSync itself never invokes a shell, but the target of that
specific call is cmd.exe, which *is* a command interpreter: it
re-parses its /c command line using cmd's own grammar, where &, |, ^
etc are metacharacters, regardless of how Node quoted the argv it was
given. A filePath containing one of those characters could still be
reinterpreted as a second command.
Both fileOpen() and dirOpen() (same pattern, same false safety
comment, though CodeQL only traced a reachable untrusted-input path to
fileOpen) now use explorer.exe on Windows instead of cmd.exe /c start.
explorer.exe treats its argument as a literal path with no shell
reinterpretation. Its exit code is unreliable (frequently non-zero
even on success), so both now use spawn() and don't wait on/check the
result, rather than execFileSync() which would throw on that.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 task
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
open()was the only mutating method onManagerLocalthat caught its own errors and downgraded them to afalsereturn — every other method (install,uninstall,installDependency, ...) throws on failure. Sincethis.log()is a no-op unless debug mode is explicitly enabled, a caller in the default configuration gotfalsewith zero information about why opening a package failed. Flagged in an internal spec-compliance audit.fileOpen()error, ...) propagate as thrownErrors like everywhere else in the class.open()has ever had: package not found, version not found, not installed, and a regression test that specifically asserts afileOpen()failure now throws instead of returningfalse.Test plan
npm run checkpasses (format, lint, build, tests — 202/202)open()'s error paths plus the happy path, withfileOpen()mocked so nothing actually launches during the test run🤖 Generated with Claude Code