Clean up error handling, fix a proto-pollution gap, and seal a few loose ends#10922
Merged
Conversation
…ose ends.
Been poking around the codebase and found a handful of things that needed tidying up:
- resolveConfig.js - config.params and config.paramsSerializer were being accessed directly off user input instead of going through the own() guard. If someone crafted a config with inherited params from the prototype, you'd get unexpected behavior. Swapped to own('params') / own('paramsSerializer') like the rest of the module does.
- http.js - there was a stray console.warn('emit error', err) in an abort-event catch block. Debug leftover, shouldn't reach production. Replaced with a quiet catch.
- AxiosHeaders.js - three places were throwing raw Error or TypeError instead of AxiosError. Swapped them over with ERR_BAD_OPTION_VALUE. Also added the missing AxiosError import (creates a circular dep with AxiosError.js which imports AxiosHeaders, but it works fine at runtime since the throws are inside method bodies, not at module eval time).
- toFormData.js - the circular reference detection was throwing a bare Error. Changed to AxiosError without a code, so it stays distinguishable from the depth-exceeded error that uses ERR_FORM_DATA_DEPTH_EXCEEDED. (There's a test that explicitly checks this distinction.)
- formDataToStream.js - two more raw throws (TypeError and Error) → AxiosError.
- buildURL.js - import self-path ../helpers/AxiosURLSearchParams.js when it lives in the same directory as the importer. Changed to ./AxiosURLSearchParams.js.
- index.d.cts - CanceledError was missing readonly name: 'CanceledError' that index.d.ts already has. Added it to keep the CJS declarations in sync.
Lint passes clean, all 770 unit tests green. Nothing breaking - all changes are either internal (no consumer-facing API change) or type-only.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
jasonsaayman
requested changes
May 26, 2026
jasonsaayman
left a comment
Member
There was a problem hiding this comment.
I don't think this could land as it is. We would have a couple of breaking changes.
Reverts AxiosError throws back to native Error/TypeError in AxiosHeaders,
formDataToStream, and toFormData to avoid breaking existing consumers
who catch by constructor name or check isAxiosError().
Adds regression tests for resolveConfig own('params')/own('paramsSerializer')
guard as requested in review.
Removes unused AxiosError imports from AxiosHeaders and formDataToStream.
Contributor
Author
|
Thanks for the review, @jasonsaayman. I've addressed all the feedback: Reverted - all 6 error-type changes that were breaking:
Removed the unused AxiosError imports that the PR had added alongside those throws. Added - 2 regression tests in prototypePollution.test.js for the resolveConfig own('params') / own('paramsSerializer') guard, verifying that polluted prototype values are correctly ignored. Kept the non-breaking fixes (buildURL import path, http.js quiet catch, index.d.cts CanceledError name sync). |
Contributor
There was a problem hiding this comment.
3 issues found across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
jasonsaayman
approved these changes
May 26, 2026
Contributor
Author
|
The three files flagged in the description as switching to AxiosError (AxiosHeaders.js, formDataToStream.js, toFormData.js) were reverted back to native Error/TypeError per maintainer review those are non-breaking now. The AxiosError imports were removed too. What was actually changed in f3a9873:
All 732 unit tests pass, lint clean. No breaking changes. |
jasonsaayman
added a commit
that referenced
this pull request
May 28, 2026
…ew loose ends (#10922) * Clean up error handling, fix a proto-pollution gap, and seal a few loose ends. Been poking around the codebase and found a handful of things that needed tidying up: - resolveConfig.js - config.params and config.paramsSerializer were being accessed directly off user input instead of going through the own() guard. If someone crafted a config with inherited params from the prototype, you'd get unexpected behavior. Swapped to own('params') / own('paramsSerializer') like the rest of the module does. - http.js - there was a stray console.warn('emit error', err) in an abort-event catch block. Debug leftover, shouldn't reach production. Replaced with a quiet catch. - AxiosHeaders.js - three places were throwing raw Error or TypeError instead of AxiosError. Swapped them over with ERR_BAD_OPTION_VALUE. Also added the missing AxiosError import (creates a circular dep with AxiosError.js which imports AxiosHeaders, but it works fine at runtime since the throws are inside method bodies, not at module eval time). - toFormData.js - the circular reference detection was throwing a bare Error. Changed to AxiosError without a code, so it stays distinguishable from the depth-exceeded error that uses ERR_FORM_DATA_DEPTH_EXCEEDED. (There's a test that explicitly checks this distinction.) - formDataToStream.js - two more raw throws (TypeError and Error) → AxiosError. - buildURL.js - import self-path ../helpers/AxiosURLSearchParams.js when it lives in the same directory as the importer. Changed to ./AxiosURLSearchParams.js. - index.d.cts - CanceledError was missing readonly name: 'CanceledError' that index.d.ts already has. Added it to keep the CJS declarations in sync. Lint passes clean, all 770 unit tests green. Nothing breaking - all changes are either internal (no consumer-facing API change) or type-only. * Update AxiosHeaders.js * Update AxiosHeaders.js * fix: revert breaking error-type changes per review feedback Reverts AxiosError throws back to native Error/TypeError in AxiosHeaders, formDataToStream, and toFormData to avoid breaking existing consumers who catch by constructor name or check isAxiosError(). Adds regression tests for resolveConfig own('params')/own('paramsSerializer') guard as requested in review. Removes unused AxiosError imports from AxiosHeaders and formDataToStream. * docs: add pre-release notes for config hardening --------- Co-authored-by: Jason Saayman <jasonsaayman@gmail.com>
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.
Been poking around the codebase and found a handful of things that needed tidying up:
resolveConfig.js - config.params and config.paramsSerializer were being accessed directly off user input instead of going through the own() guard. If someone crafted a config with inherited params from the prototype, you'd get unexpected behavior. Swapped to own('params') / own('paramsSerializer') like the rest of the module does.
http.js - there was a stray console.warn('emit error', err) in an abort-event catch block. Debug leftover, shouldn't reach production. Replaced with a quiet catch.
AxiosHeaders.js - three places were throwing raw Error or TypeError instead of AxiosError. Swapped them over with ERR_BAD_OPTION_VALUE. Also added the missing AxiosError import (creates a circular dep with AxiosError.js which imports AxiosHeaders, but it works fine at runtime since the throws are inside method bodies, not at module eval time).
toFormData.js - the circular reference detection was throwing a bare Error. Changed to AxiosError without a code, so it stays distinguishable from the depth-exceeded error that uses ERR_FORM_DATA_DEPTH_EXCEEDED. (There's a test that explicitly checks this distinction.)
formDataToStream.js - two more raw throws (TypeError and Error) → AxiosError.
buildURL.js - import self-path ../helpers/AxiosURLSearchParams.js when it lives in the same directory as the importer. Changed to ./AxiosURLSearchParams.js.
index.d.cts - CanceledError was missing readonly name: 'CanceledError' that index.d.ts already has. Added it to keep the CJS declarations in sync.
Lint passes clean, all 770 unit tests green. Nothing breaking - all changes are either internal (no consumer-facing API change) or type-only.
Summary by cubic
Hardened config resolution to block prototype pollution in
params/paramsSerializer, kept native error types, and tidied a few minor issues. Also updated pre-release notes.Description
resolveConfigwithown('params')andown('paramsSerializer')to ignore inherited values.Error/TypeErrorinAxiosHeaders,formDataToStream, andtoFormData(usenewwhere missing).buildURLimport to./AxiosURLSearchParams.js; silence strayconsole.warnin the Node HTTP abort handler.readonly name: 'CanceledError'to CJS typings inindex.d.cts.Docs
CanceledErrortyping./docs/that config resolution ignores inheritedparams/paramsSerializer.Testing
resolveConfigignores inheritedparamsandparamsSerializerand does not call polluted serializers.Semantic version impact
Written for commit d537120. Summary will update on new commits. Review in cubic