Skip to content

Comments

fix: use OR operator in includesCredentials per WHATWG URL Standard#4816

Merged
KhafraDev merged 1 commit intonodejs:mainfrom
jackhax:fix/includes-credentials-or-operator
Feb 7, 2026
Merged

fix: use OR operator in includesCredentials per WHATWG URL Standard#4816
KhafraDev merged 1 commit intonodejs:mainfrom
jackhax:fix/includes-credentials-or-operator

Conversation

@jackhax
Copy link
Contributor

@jackhax jackhax commented Feb 7, 2026

Summary

  • Fix logical operator in includesCredentials() (lib/web/fetch/util.js): &&||
  • Add unit tests for includesCredentials() covering all credential combinations

Problem

The includesCredentials() function uses AND (&&) instead of OR (||), contradicting both:

  1. Its own comment: "A URL includes credentials if its username or password is not the empty string."
  2. WHATWG URL Standard §4.4: "A URL includes credentials if its username is not the empty string or its password is not the empty string."

This causes the Authorization header to be silently omitted during authentication retries for URLs with only a username (e.g., http://apitoken@host) or only a password (e.g., http://:secret@host).

Fix

 function includesCredentials (url) {
   // A URL includes credentials if its username or password is not the empty string.
-  return !!(url.username && url.password)
+  return !!(url.username || url.password)
 }

Test plan

  • New test: URL with both username and password → true
  • New test: URL with only username → true
  • New test: URL with only password → true
  • New test: URL with no credentials → false
  • Existing fetch tests pass (no regressions)
  • ESLint passes

Fixes: #4815

Ref: CWE-480: Use of Incorrect Operator

The includesCredentials function used AND (&&) instead of OR (||),
causing it to return false for URLs with only a username or only a
password. Per WHATWG URL Standard §4.4, a URL includes credentials
if its username OR its password is not the empty string.

Fixes: nodejs#4815
@mcollina mcollina requested a review from KhafraDev February 7, 2026 08:53
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.25%. Comparing base (b3326b5) to head (e7ff4db).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4816   +/-   ##
=======================================
  Coverage   93.25%   93.25%           
=======================================
  Files         109      109           
  Lines       34153    34153           
=======================================
  Hits        31848    31848           
  Misses       2305     2305           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@KhafraDev KhafraDev merged commit a821c56 into nodejs:main Feb 7, 2026
36 of 37 checks passed
@github-actions github-actions bot mentioned this pull request Feb 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

includesCredentials() silently drops Authorization header for username-only or password-only URLs on 401 retry

4 participants