Skip to content

fix: unrestricted cloud metadata exfiltration via header injection chain#10660

Merged
jasonsaayman merged 5 commits intov1.xfrom
sec/ghsa-fvcv-3m26-pcqx
Apr 6, 2026
Merged

fix: unrestricted cloud metadata exfiltration via header injection chain#10660
jasonsaayman merged 5 commits intov1.xfrom
sec/ghsa-fvcv-3m26-pcqx

Conversation

@jasonsaayman
Copy link
Copy Markdown
Member

@jasonsaayman jasonsaayman commented Apr 6, 2026

Summary by cubic

Blocks CRLF header injection chains that could exfiltrate cloud metadata by rejecting invalid header values. Also hardens error stack merging logic in Axios.

Description

  • Summary of changes
    • Added assertValidHeaderValue in AxiosHeaders to throw on \r or \n in any header value (including array elements).
    • Enforced validation on all header-set paths; updated XHR mock to record request headers.
    • Reworked Axios error stack merging to use index-based slicing with a duplicate-tail guard (no fragile regex).
  • Reasoning
    • CRLF-enabled header splitting could be chained to access IMDS and leak metadata. Invalid input now hard-fails.
  • Additional context
    • Behavior change: headers containing CR/LF are rejected with “Invalid character in header content”.

Docs

  • User-provided header values must not include CR or LF.
  • Requests with invalid headers are rejected. No API surface changes.

Testing

  • Added/updated tests:
    • Browser adapter: rejects CRLF headers; ensures no request is sent; mock captures headers.
    • Node fetch adapter: rejects CRLF headers.
    • Node http adapter: rejects CRLF headers.
    • AxiosHeaders: throws on CRLF in single and array values; preserves iterable multi-value behavior.

Written for commit c16da40. Summary will update on new commits.

@jasonsaayman jasonsaayman self-assigned this Apr 6, 2026
@jasonsaayman jasonsaayman added priority::medium A medium priority commit::fix The PR is related to a bugfix type::security The PR is a secuirty related changed normally from a CVE labels Apr 6, 2026
Comment thread lib/core/AxiosHeaders.js Fixed
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 5 files

Confidence score: 4/5

  • This PR appears safe to merge; the only noted issue is a low-severity test fragility rather than a functional bug.
  • tests/unit/adapters/fetch.test.js uses a runtime-specific rejection string, which could make the security test flaky across Node/fetch implementations.
  • Pay close attention to tests/unit/adapters/fetch.test.js - broaden the rejection matcher to avoid cross-runtime flakiness.
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="tests/unit/adapters/fetch.test.js">

<violation number="1" location="tests/unit/adapters/fetch.test.js:36">
P2: The rejection matcher is too specific to one runtime error string, which makes this security test flaky across Node/fetch implementations. Match a broader invalid-header pattern (or just assert rejection) instead.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread tests/unit/adapters/fetch.test.js Outdated
@caugner
Copy link
Copy Markdown
Contributor

caugner commented Apr 10, 2026

According to https://github.com/axios/axios#semver, Axios follows semver.

My understanding of this change is that the setHeader() function started throwing an error now, which it didn't before, and that this is generally considered a breaking change.

I noticed this in mdn/mdn-http-observatory#489, because our tests failed. For our use case in the MDN HTTP Observatory, we would prefer if setHeader() didn't throw, or if there was an opt-out mechanism.

@jasonsaayman
Copy link
Copy Markdown
Member Author

Thanks, @caugner. I should have checked more cases, as I had determined this to be quite low risk for 99% of users. I will add an explicit opt-out and make a PR to the MDN lib as soon as it is ready.

@caugner
Copy link
Copy Markdown
Contributor

caugner commented Apr 10, 2026

Thanks. I'm wondering how other HTTP request frameworks handle invalid headers. Do they throw, or do they ignore or sanitize the value instead?

@jasonsaayman
Copy link
Copy Markdown
Member Author

I don't actually know. I will do some research. Would it be fine in your case if this is just sanitised and does not throw? That would be a better approach for us. In the past, security opt-in has led to the same CVEs being opened repeatedly.

@caugner
Copy link
Copy Markdown
Contributor

caugner commented Apr 10, 2026

Would it be an option to have two modes for handling \r and \n in headers:

  • loose (default) replaces them with space, and
  • strict (opt-in) throws an error.

You could then make strict the default option in Axios v2.

@jasonsaayman
Copy link
Copy Markdown
Member Author

Sure, that can work! Thanks for the suggestion. I will do that asap.

@caugner
Copy link
Copy Markdown
Contributor

caugner commented Apr 10, 2026

RFC 9110 defines the grammar of HTTP field values as follows:

  field-value    = *field-content
  field-content  = field-vchar
                   [ 1*( SP / HTAB / field-vchar ) field-vchar ]
  field-vchar    = VCHAR / obs-text
  obs-text       = %x80-FF

(VCHAR is defined in RFC 5234)

So strictly speaking the isValidHeaderValue() function is not complete, and could be something like this:

const isValidHeaderValue = (value) =>
  // HTAB:    \x09 (not allowed as first/last character)
  // SP:      \x20 (not allowed as first/last character)
  // VCHAR:   \x20-\x7E
  // obs-text \x80-\xFF
  /^[\x09\x20-\x7E\x80-\xFF]+$/.test(value) && !/^[\x09\x20]|[\x09\x20]$/.test(value);

Edit: Loose mode could strip forbidden characters.

const sanitizeHeaderValue = (value) =>
  value
    .replace(/[^\x09\x20-\x7E\x80-\xFF]/g, '')
    .replace(/^[\x09\x20]|[\x09\x20]$/g, '');

@jasonsaayman
Copy link
Copy Markdown
Member Author

jasonsaayman commented Apr 10, 2026

Thanks @caugner, this is very helpful. I will implement as soon as I have a gap and do a release. May bundle some more fixes with this, but I do quite like just doing the "Loose mode" as this is a better solution.

@ThiefMaster
Copy link
Copy Markdown

If your application relied on including raw line breaks in headers then it was broken before. Asking for a major version bump just to make this throw an error this is kind of strange... There's simply no valid reason to include a line break when setting a header.

iamnaii added a commit to iamnaii/BESTCHOICE that referenced this pull request Apr 11, 2026
Pulls in two CVE fixes (both Node.js adapter — low impact on current
browser-only usage but hardens future server-side use):
  - SSRF via no_proxy hostname normalisation bypass (axios/axios#10661)
  - Cloud metadata exfiltration via header injection (axios/axios#10660)

Also gets us past the broken 1.13.3 where AxiosError.status was dropped —
our new LiffPayment slip upload error classification relies on
response.status (413 → too large, 400 → backend validator). Installed
was 1.13.6 (already safe) but pinning ^1.15.0 prevents future regressions.

No breaking changes to timeout/error API touched in
apps/web/src/pages/liff/LiffPayment.tsx.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@openshift-merge-robot
Copy link
Copy Markdown

Fix included in release 4.14.0-0.nightly-2026-04-22-065932

@openshift-merge-robot
Copy link
Copy Markdown

Fix included in release 4.15.0-0.nightly-2026-04-22-170323

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commit::fix The PR is related to a bugfix priority::medium A medium priority type::security The PR is a secuirty related changed normally from a CVE

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants