fix: backport the fixes from the v1 branch#10688
Merged
Merged
Conversation
jasonsaayman
added
priority::medium
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 8 files
Confidence score: 4/5
- This PR looks safe to merge with minimal risk: both findings are moderate/low severity and appear to be edge-case parsing/sanitization behaviors rather than broad breakages.
- Most significant issue is in
lib/helpers/sanitizeHeaderValue.js, where boundary whitespace stripping only removes a single leading/trailing space or tab, which can leave partially unsanitized header values when multiple boundary whitespace characters are present. lib/helpers/shouldBypassProxy.jsmay mis-handle an explicit:0port by treating0as falsy and falling back to the protocol default, which can cause incorrect proxy bypass decisions for that specific input.- Pay close attention to
lib/helpers/sanitizeHeaderValue.jsandlib/helpers/shouldBypassProxy.js- edge-case input handling (multi-boundary whitespace and explicit:0port parsing) should be verified.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/helpers/sanitizeHeaderValue.js">
<violation number="1" location="lib/helpers/sanitizeHeaderValue.js:6">
P2: Boundary whitespace regex only strips one leading/trailing space or tab; values with multiple boundary whitespace characters remain partially unsanitized.</violation>
</file>
<file name="lib/helpers/shouldBypassProxy.js">
<violation number="1" location="lib/helpers/shouldBypassProxy.js:95">
P2: Explicit port `:0` is misparsed as the protocol default because `0` is treated as falsy.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Member
Author
|
@cubic-dev-ai please review this again. Please be on the look out for any gaps in the fixes to the security concerns. |
Contributor
@jasonsaayman I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
1 issue found across 8 files
Confidence score: 3/5
- There is a concrete behavior risk in
lib/helpers/shouldBypassProxy.js:NO_PROXYonly treats wildcard bypass as valid when the value is exactly*, not when*appears in a list. - Because this issue is high severity with high confidence, proxy bypass decisions can be wrong in real deployments, which can cause unexpected routing through a proxy.
- Pay close attention to
lib/helpers/shouldBypassProxy.js- wildcard parsing in comma/space-separatedNO_PROXYentries can fail and change network behavior.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/helpers/shouldBypassProxy.js">
<violation number="1" location="lib/helpers/shouldBypassProxy.js:90">
P1: `NO_PROXY` wildcard entries are only honored when the value is exactly `*`; `*` inside a comma/space-separated list is ignored, so proxy bypass can fail unexpectedly.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Member
Author
|
@cubic-dev-ai please review again |
Contributor
@jasonsaayman I have started the AI code review. It will take a few minutes to complete. |
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 by cubic
Backports v1 security hardening to
axiosv0.x: sanitize outgoing header values and correctly bypass proxies viaNO_PROXY/no_proxy, including localhost, loopback, trailing dots, and bracketed IPv6. Also ensures proxy bypass is checked before parsing proxy URLs and usesparsed.hostfor correct ports/IPv6 handling.Description
sanitizeHeaderValueand applied it to all request headers before dispatch. Strips invalid bytes, CR/LF, and trims boundary whitespace. Supports arrays.shouldBypassProxyto honorNO_PROXY/no_proxy(wildcards, explicit ports, trailing dots, IPv6 including bracketed, loopback aliases). Used in the Nodehttpadapter to skip proxy when needed.parsed.hostso ports and IPv6 are handled correctly.NO_PROXYdon’t leak through proxies; aligns with common semantics, including loopback aliases.Docs
NO_PROXY/no_proxywith:*.example.com)localhost:8080)localhost,127.0.0.1,::1)[::1]) and hostnames with trailing dots (localhost.)Testing
sanitizeHeaderValue(invalid chars, boundary whitespace, arrays).shouldBypassProxy(wildcards, explicit ports, loopback aliases, trailing dots, bracketed IPv6).NO_PROXYports and wildcards, including trailing dots and bracketed IPv6.Written for commit 5e905d2. Summary will update on new commits.