Release 1.5.14 - #184
Open
07souravkunda wants to merge 8 commits into
Open
Conversation
…env) The binary-download retry/fallback state was signalled through three process.env vars (BINARY_DOWNLOAD_FALLBACK_ENABLED / _ERROR_MESSAGE / _SOURCE_URL). process.env is a process-global mutable store, which caused two problems on the binary download path: - Cross-instance state bleed (CWE-362): a download/exec failure on one Local instance set these globals for the whole process, so every other concurrent Local instance (e.g. in a parallel test runner) inherited the failed instance's fallback flag, error text, and cached source URL - instances silently downloaded from another instance's request-context URL and reported another instance's error as their own telemetry. - Unvalidated download source (CWE-494): getSourceUrl(Sync) returned process.env.BINARY_DOWNLOAD_SOURCE_URL verbatim, with no scheme/host check, before contacting the endpoint API. A value planted in the environment before the process booted therefore steered the binary download to an arbitrary host, which is then chmod 0755'd and executed. Replace the globals with a per-Local-instance state object, shared by reference across the LocalBinary objects a single instance recreates during its retry loop. This ends the cross-instance bleed and removes the environment shortcut, while preserving the same per-instance retry/fallback behaviour (the resolved fallback URL is still cached to avoid re-requesting the endpoint API within one instance). Adds regression tests that fail before this change and pass after it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rename the describe and strip the leading comment's internal ids so the regression suite carries no internal reference (this is a public repo whose test file also ships in the published npm tarball). No test behaviour change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t a proxy Four hardening fixes to the binary download path, all in the same area: - The access key was passed to lib/fetchDownloadSourceUrl.js as a positional argv element, so it was readable by any local user via `ps` or /proc/<pid>/cmdline for the lifetime of the spawn. It now travels in the child's environment (/proc/<pid>/environ is restricted to the owning user) and the remaining argv slots shift down by one accordingly. (CWE-214) - lib/download.js only applied `useCaCertificate` inside the `if (proxyHost && proxyPort)` branch, so a caller-supplied TLS trust anchor was ignored whenever no proxy was configured. Worse, the parent passes the literal `undefined` placeholders for the proxy slots in exactly that case, which arrive as the truthy *string* "undefined" — so the sync download built a proxy agent for host "undefined" and failed outright with `getaddrinfo ENOTFOUND undefined`. Both are fixed: the CA is applied unconditionally, and the proxy slots are compared with the existing `isUndefined` helper (as lib/fetchDownloadSourceUrl.js already does). (CWE-295) - retryBinaryDownload did an async fs.stat followed by a synchronous fs.unlinkSync inside the callback. Collapsed to a single ENOENT-tolerant fs.unlink, removing the window between the two and the uncatchable throw a failing unlinkSync raised from within the stat callback. (CWE-362) - getAvailableDirs fell back to os.tmpdir() itself — /tmp on Linux, which is world-writable — under a fixed, predictable binary name. It now uses a per-uid subdirectory created 0700, and that fallback is rejected unless it is a real directory owned by us and not group/world-writable, so a pre-created symlink or shared directory cannot be used as the destination for a binary we are about to execute. Only the temp fallback is subjected to this check; $HOME/.browserstack and cwd are unchanged. (CWE-377) Also pins the Semgrep CI container to an immutable digest so a mutated tag cannot redirect the workflow to a different image. (CWE-829)
…start
`start()` handles the binary's output inside an `execFile` callback. The
empty-output branch called back with 'No output received' but did not
return, so control fell through to `data['message']['message']` on
`data = {}`. That threw a TypeError, and because the throw happens inside
a callback invoked by node's internal exithandler, no try/catch around
`local.start(...)` could intercept it — it surfaced as an
uncaughtException in the host process.
Three paths reached the same unguarded deref:
- empty stdout and stderr (the reported one) — now returns after the
callback, so it fires exactly once
- the terminal branch of the `error` handler, which also fell through
- any non-connected payload with no `message` key
Also guards `JSON.parse`: non-JSON output threw a SyntaxError from the
same uncatchable position, and is now reported through the callback with
the raw output attached as `extra`.
`startSync` shared the unguarded deref and now uses the same helper. Its
empty-output branch already returned, so it was not exposed to the
fall-through.
Adds regression tests driving start() with stub binaries for each output
shape, asserting the callback fires exactly once and nothing escapes as
an uncaughtException. They need no credentials or network. Three of the
four fail on master with the TypeError from the ticket.
…llback-fallthrough LOC-7325: stop uncatchable TypeError on empty binary output in Local.start
Scope binary-download fallback state per Local instance
Keep the access key out of child argv; honour useCaCertificate without a proxy
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited), Workspace UI (inherited) Review profile: ASSERTIVE Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
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.
No description provided.