fix: Prevent Node subprocess from hanging during snyk test

[danskmt]

Pull Request Submission Checklist

• [X Follows CONTRIBUTING guidelines
• Commit messages
  are release-note ready, emphasizing
  what was changed, not how.
• Includes detailed description of changes
• Contains risk assessment (Low | Medium | High)
• Highlights breaking API changes (if applicable)
• Links to automated tests covering new functionality
• Includes manual testing instructions (if necessary)
• Updates relevant GitBook documentation (PR link: ___)
• Includes product update to be announced in the next stable release notes

What does this PR do?

Fixes snyk test hanging indefinitely when the Snyk API responds with a 302 redirect (e.g. for remote package scans like snyk test xlsx).

The root cause: needle (the HTTP client) follows redirects internally, creating an intermediate CONNECT tunnel socket that is never exposed to the callback. This socket keeps the Node.js event loop alive, preventing the subprocess from exiting, which blocks the Go CLI's snykCmd.Run() indefinitely.

The fix disables needle's built-in redirect following in makeRequest and implements manual redirect handling with explicit socket cleanup (res.socket.destroy()) after each hop. Each redirect creates a fresh agent so sockets are properly isolated and destroyed.

Where should the reviewer start?

src/lib/request/request.ts — the only file changed. The makeRequest function now:

Sets options.follow_max = 0 to disable needle's redirect following
Uses an internal sendRequest loop that handles redirects manually
Calls res?.socket?.destroy() after every response to close the CONNECT tunnel

How should this be manually tested?

1. snyk test xlsx — previously hangs indefinitely, should now complete and exit
2. snyk test <local-repo-with-package.json> — should continue to work as before (no redirect involved)
3. Any snyk test that triggers a 302 from the API (version-range vuln lookups)

What's the product update that needs to be communicated to CLI users?

Risk assessment (Low | Medium | High)?

Low. The change only affects makeRequest in the request module. Redirect behavior is preserved (same codes, same max of 5 hops), just handled manually instead of by needle. streamRequest is unaffected.

What are the relevant tickets?

CLI-1166

[snyk-io]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scanner	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[github-actions]

	Warnings
⚠️	There are multiple commits on your branch, please squash them locally before merging!
⚠️	"Merge branch 'main' into fix/CLI-1166-node-subprocess-hangs-after-snyk-test" is too long. Keep the first line of your commit message under 72 characters.

Generated by 🚫 dangerJS against 39fc99a

[danskmt]

Another solution for this issue, other than the one in this PR, is to add this line:
process.exit(process.exitCode ?? 0);
at the end of callHandlingUnexpectedErrors in src/cli/index.ts, i.e.

callHandlingUnexpectedErrors(async () => {
  testPlatformSupport();
  const { main } = await import('./main');
  await main();
  process.exit(process.exitCode ?? 0);
}, EXIT_CODES.ERROR);

I opted for the current solution in this PR since it takes into account the root cause of the issue that creates connections that never ends.

[danskmt]

PR Reviewer Guide 🔍

🧪 No relevant tests
🔒 Security concerns

Sensitive information exposure: The manual redirect logic introduced in makeRequest (lines 186-218) forwards all request headers to the redirect destination. This includes Snyk API tokens and potentially other sensitive headers. If an attacker can trigger a redirect to a malicious external domain, the Snyk CLI will inadvertently send the user's authentication credentials to that domain. To fix this, the logic should compare the origin (protocol and hostname) of the redirectUrl with the reqUrl and strip sensitive headers if they do not match.

⚡ Recommended focus areas for review

Sensitive Header Leakage 🔴 [critical]
The manual redirect implementation in sendRequest shallow-copies the original request options, including all headers, to the redirect request. If a request is redirected to a different domain (e.g., from snyk.io to a third-party host), sensitive authentication headers like Authorization or x-snyk-token will be forwarded. This is a significant security risk. Standard practice is to strip sensitive authentication and session headers when the redirect target has a different origin (protocol, host, or port) than the source.

const redirectOptions = { ...reqOptions, agent: newAgent };

Unhandled Exception 🟠 [major]
The new URL() constructor is used within the asynchronous needle callback without a try-catch block. If a server returns a malformed or invalid Location header, new URL() will throw an exception. Since this happens inside an asynchronous callback, the error will result in an unhandled exception that bypasses the Promise's rejection logic, potentially crashing the process or leaving the makeRequest promise hanging indefinitely.

const redirectUrl = new URL(
  res.headers.location,
  reqUrl,
).toString();

Incomplete Header Cleanup 🟡 [minor]
When handling redirects that change the request method to GET (301, 302, 303), the code correctly removes content-length and content-encoding. However, it leaves the content-type header intact. Sending a Content-Type header (e.g., application/json) on a GET request with no body is technically valid but can cause unexpected behavior or errors on some server implementations.

if (!preserveMethod) {
  delete redirectOptions.headers!['content-length'];
  delete redirectOptions.headers!['content-encoding'];
}

📚 Repository Context Analyzed

This review considered 7 relevant code sections from 5 files (average relevance: 1.00)

• src/lib/request/request.ts (lines 1-256)
• packages/snyk-protect/src/lib/http.ts (lines 1-50)
• CONTRIBUTING.md (2 segments, lines 301-400)
• package.json (2 segments, lines 1-100)
• dangerfile.js (lines 1-111)

Worth noting that none of these are regressions. The original code used needle's built-in redirect following (follow_max: 5), which also forwarded all headers (including auth) to redirect targets and didn't guard against malformed Location headers. Since we now own the redirect logic, it's a good opportunity to handle these properly.

[snyk-pr-review-bot]

PR Reviewer Guide 🔍

🧪 No relevant tests

🔒 Security concerns Sensitive information exposure: The manual redirect implementation in src/lib/request/request.ts (lines 221-232) only strips sensitive authentication headers when the destination host changes. This logic fails to protect credentials during protocol downgrades on the same host (e.g., a redirect from https://api.snyk.io to http://api.snyk.io). In such a scenario, the authorization header and x-snyk-token would be sent over an unencrypted connection, exposing them to potential interception.

⚡ Recommended focus areas for review Security Risk 🟠 [major] The manual redirect logic only strips sensitive headers (like authorization and x-snyk-token) if the host of the redirect URL differs from the original. It does not account for protocol downgrades (HTTPS to HTTP) on the same host. If a Snyk API or intermediate service redirects an encrypted request to an unencrypted http endpoint on the same host, sensitive credentials will be transmitted in plain text. Standard security practice is to strip sensitive headers whenever moving from a secure protocol (HTTPS) to an insecure one (HTTP), regardless of the host. if (parsedRedirect.host !== parsedOriginal.host) { const sensitiveHeaders = [ 'authorization', 'session-token', 'cookie', 'x-api-key', 'x-snyk-token', ]; for (const h of sensitiveHeaders) { delete redirectHeaders[h]; } } Missing Tests 🟠 [major] The PR replaces a core library feature (needle's internal redirect following) with a custom manual implementation but lacks functional tests for this new logic. This is a high-risk change as it affects all network communication in the CLI. The implementation should be verified with tests covering maximum redirect depth, relative vs. absolute URL handling, method preservation for 307/308 status codes, and the stripping of sensitive headers during cross-host redirects. const sendRequest = ( reqMethod: string, reqUrl: string, reqData: any, reqOptions: needle.NeedleOptions, ) => { needle.request( reqMethod as needle.NeedleHttpVerbs, reqUrl, reqData, reqOptions, (err, res, respBody) => { // Destroy the socket so the CONNECT tunnel doesn't keep the process alive. res?.socket?.destroy(); if (res?.headers?.[headerSnykAuthFailed] === 'true') { return reject(new MissingApiTokenError()); } debug('response (%s)', (res || {}).statusCode); if (err) { debug('response err: %s', err); return reject(err); } if ( res.statusCode && REDIRECT_CODES.includes(res.statusCode) && res.headers?.location && redirectsLeft > 0 ) { redirectsLeft--; let redirectUrl: string; try { redirectUrl = new URL( res.headers.location, reqUrl, ).toString(); } catch (e) { return reject( new Error(`Invalid redirect Location: ${res.headers.location}`), ); } debug('following redirect to %s', redirectUrl); const parsedRedirect = parse(redirectUrl); const parsedOriginal = parse(reqUrl); const newAgent = parsedRedirect.protocol === 'http:' ? new http.Agent({ keepAlive: false }) : new https.Agent({ keepAlive: false }); const preserveMethod = res.statusCode !== undefined && METHOD_PRESERVING_REDIRECTS.includes(res.statusCode); const redirectHeaders = { ...reqOptions.headers }; if (!preserveMethod) { delete redirectHeaders['content-length']; delete redirectHeaders['content-encoding']; } if (parsedRedirect.host !== parsedOriginal.host) { const sensitiveHeaders = [ 'authorization', 'session-token', 'cookie', 'x-api-key', 'x-snyk-token', ]; for (const h of sensitiveHeaders) { delete redirectHeaders[h]; } } const redirectOptions = { ...reqOptions, agent: newAgent, headers: redirectHeaders, }; return sendRequest( preserveMethod ? reqMethod : 'get', redirectUrl, preserveMethod ? reqData : null, redirectOptions, ); } resolve({ res, body: respBody }); }, ); }; Incomplete Header Stripping 🟡 [minor] The sensitiveHeaders list used during redirects is missing standard sensitive headers like proxy-authorization. While the list covers common Snyk and web headers, failing to strip proxy-authorization could lead to the exposure of proxy credentials if they are present in the request and a redirect occurs to an external host. const sensitiveHeaders = [ 'authorization', 'session-token', 'cookie', 'x-api-key', 'x-snyk-token', ];

📚 Repository Context Analyzed This review considered 7 relevant code sections from 5 files (average relevance: 1.01) src/lib/request/request.ts (lines 1-283) packages/snyk-protect/src/lib/http.ts (lines 1-50) CONTRIBUTING.md (2 segments, lines 301-400) package.json (2 segments, lines 1-100) dangerfile.js (lines 1-111)