fix: spawn the archiver without a shell in verify-vsix - #17
Merged
Conversation
CodeQL flags `js/indirect-command-line-injection` here: `process.env.SystemRoot` is interpolated into a string handed to `execSync`. **It is not exploitable and this commit should not claim otherwise.** The value is already quoted, and `SystemRoot` is a trusted value — anyone able to set it can run anything they like without this script's help. Nor does the script ship; it runs locally and in CI to check a packaged VSIX. It is worth changing anyway for one reason that has nothing to do with CodeQL: `vscode-ext-kit`'s equivalent script already uses `execFileSync`, so this is the odd one out. Passing argv removes the quoting question rather than answering it, and removes the shell that could get it wrong. The archive argument stays relative and `cwd` stays as it was — that pairing is deliberate, because bsdtar reads an absolute `C:\…` as a remote host. Verified by running the lane that actually exercises it: package, then `verify:vsix` end to end. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
CodeQL flags
js/indirect-command-line-injection:process.env.SystemRootisinterpolated into a string handed to
execSync.Not exploitable
The value is already quoted, and
SystemRootis a trusted value — anyone ableto set it can run anything they like without this script's help. The script also
does not ship: it runs locally and in CI to check a packaged VSIX.
Why change it anyway
One reason, and it has nothing to do with CodeQL:
vscode-ext-kit's equivalentscript already uses
execFileSync. This is the odd one out.Passing argv removes the quoting question rather than answering it, and removes
the shell that could get it wrong.
What is preserved
The archive argument stays relative and
cwdstays as it was. That pairing isdeliberate — bsdtar reads an absolute
C:\…as a remote host — and the commentsaying so is kept.
Verification
Ran the lane that actually exercises this:
package, thenverify:vsixend toend, extracting a real VSIX.
🤖 Generated with Claude Code