fix(fetch): support basic auth from URL#10896
Merged
Merged
Conversation
8 tasks
Contributor
There was a problem hiding this comment.
1 issue found across 2 files
Confidence score: 3/5
- There is a concrete regression risk in
lib/adapters/fetch.js: usingbtoa(auth)can throw aDOMExceptionfor non-Latin1 credentials, which may break Basic Auth for affected users. - I’m scoring this as moderate risk because the issue is fairly severe (7/10) with strong confidence (8/10) and can cause user-facing request failures, even though the change surface appears limited.
- Pay close attention to
lib/adapters/fetch.js- align credential encoding behavior with the HTTP adapter to avoid non-Latin1 auth failures.
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/adapters/fetch.js">
<violation number="1" location="lib/adapters/fetch.js:242">
P1: `btoa(auth)` may throw DOMException for non-Latin1 credentials, diverging from http adapter behavior</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Member
|
@cubic-dev-ai please review in depth again |
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 2 files
Confidence score: 3/5
- There is a concrete, user-facing behavior risk in
lib/adapters/fetch.js: password-only URL credentials (likehttps://:[email protected]) are currently dropped because fallback logic checks onlyparsedURL.username. - Given the medium severity (5/10) with high confidence (9/10), this is more than a cosmetic issue and could cause authentication failures for a valid credential format.
- This looks scoped to a specific credential edge case rather than a broad breakage, so risk is moderate rather than critical.
- Pay close attention to
lib/adapters/fetch.js- credential parsing/fallback should preserve password-only credentials instead of silently omitting them.
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/adapters/fetch.js">
<violation number="1" location="lib/adapters/fetch.js:244">
P2: URL credential fallback ignores password-only credentials (e.g. `https://:[email protected]`) because it checks `parsedURL.username` only, causing credentials to be silently dropped.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
Author
Contributor
|
@niksy I have started the AI code review. It will take a few minutes to complete. |
jasonsaayman
previously requested changes
May 24, 2026
Member
jasonsaayman
left a comment
There was a problem hiding this comment.
Just one small issues, thanks
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
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
The fetch adapter is now aligned with the
httpadapter forBasicauth: it first respectsconfig.auth, and when that is not provided, it falls back to credentials embedded in the request URL, setsAuthorization: Basic ..., and clears any existingauthorizationheader so precedence matches Node behavior.It also sanitizes the URL by stripping credentials before constructing
Request, which prevents theTypeError: Request cannot be constructed from a URL that includes credentials.Existing test was covering classic
httpadapter instead offetchso it was passing which is wrong and is now fixed. New tests cover URL-based auth fallback, decoding of percent-encoded credentials, safe handling of malformed percent-encoding without throwing, and confirmation that theauthoption overrides a manually providedAuthorizationheader regardless of header casing.Checklist
index.d.tsandindex.d.cts)Summary by cubic
Fixes Basic auth in the
fetchadapter to matchhttp, including URL-embedded credentials (username, password, or password-only). Credentials are stripped from the URL and sent via a UTF‑8‑safeAuthorization: Basic ...header to avoidRequestconstruction errors.Description
config.auth> URL creds; clear anyAuthorizationheader (any casing).http(smallhttpfix included).Authorization: Basic ....Request.fetchURL-with-credentials errors.httpand WHATWG URL.Docs
Please update
/docs/(and README where relevant) to cover:Authorization.Request.Testing
fetchadapter.fetchtests for UTF‑8 credentials, URL-based fallback, percent-decoding (valid/malformed), password-only creds, andauthoverriding anAuthorizationheader (any casing).httptest for password-only URL creds.Semantic version impact
Patch: bug fix and behavior alignment; no public API changes.
Written for commit d3d6e60. Summary will update on new commits.