fix(fetch): fix SOCKS5 proxy URL validation logic

[majiayu000]

Summary

This PR fixes a logical error in the SOCKS5 proxy URL validation in Deno.createHttpClient().

The bug: The condition for validating SOCKS5 proxy URLs was using || (OR) instead of && (AND):

// Before (broken)
if (
  !StringPrototypeStartsWith(url, "socks5:") ||
  !StringPrototypeStartsWith(url, "socks5h:")
) { throw ... }

This meant that a valid socks5:// URL would still throw because it doesn't start with socks5h:, and vice versa. The condition would almost always be true, making SOCKS5 proxies unusable.

The fix:

// After (fixed)
if (
  !StringPrototypeStartsWith(url, "socks5:") &&
  !StringPrototypeStartsWith(url, "socks5h:")
) { throw ... }

Now the error is only thrown when the URL starts with NEITHER socks5: nor socks5h:.

Changes

• ext/fetch/22_http_client.js: Fixed the logical operator from || to &&
• tests/unit/fetch_test.ts: Added tests to verify that SOCKS5 transport accepts both socks5:// and socks5h:// URLs

Test plan

• Added test createHttpClientSocks5ProxyAcceptsSocks5Url - verifies socks5 transport accepts socks5:// URLs
• Added test createHttpClientSocks5ProxyAcceptsSocks5hUrl - verifies socks5 transport accepts socks5h:// URLs
• Manually verified with dev build that socks5:// and socks5h:// URLs now work correctly

Closes #31356

[CLAassistant]

All committers have signed the CLA.

[coderabbitai]

Walkthrough

The pull request relaxes Socks5 proxy URL validation in the HTTP client: the check now throws only if the proxy URL does not start with socks5: AND does not start with socks5h:, allowing either prefix. The transport assignment for the Socks5 proxy path remains set to "http". Two unit tests were added to verify that socks5:// and socks5h:// proxy URLs are accepted and produce a Deno.HttpClient instance. No public API declarations were changed.

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title accurately summarizes the main change: fixing SOCKS5 proxy URL validation logic by correcting a logical operator error.
Description check	✅ Passed	Description clearly explains the bug (OR vs AND operator), shows before/after code, lists changes, and references the closed issue.
Linked Issues check	✅ Passed	PR directly addresses issue #31356 by fixing the exact logical condition mentioned, changing || to && in SOCKS5 validation and adding tests.
Out of Scope Changes check	✅ Passed	All changes are scoped to SOCKS5 proxy URL validation logic and related test coverage, with no unrelated modifications.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0efde7f and e7f1ed9.

📒 Files selected for processing (2)

• ext/fetch/22_http_client.js (1 hunks)
• tests/unit/fetch_test.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.{ts,tsx,js}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with --inspect-brk flag and connect Chrome DevTools to chrome://inspect
Use console.log() for debug prints in JavaScript runtime code

Files:

• ext/fetch/22_http_client.js
• tests/unit/fetch_test.ts

🧬 Code graph analysis (1)

ext/fetch/22_http_client.js (2)

ext/fetch/lib.rs (1)

• url (457-459)

ext/http/00_serve.ts (1)

• url (242-274)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

• GitHub Check: test debug linux-x86_64
• GitHub Check: test debug linux-aarch64
• GitHub Check: test debug windows-x86_64
• GitHub Check: test debug macos-aarch64
• GitHub Check: test debug macos-x86_64
• GitHub Check: test release linux-x86_64
• GitHub Check: lint debug windows-x86_64
• GitHub Check: lint debug linux-x86_64
• GitHub Check: lint debug macos-x86_64
• GitHub Check: build libs

🔇 Additional comments (2)

ext/fetch/22_http_client.js (1)

67-68: LGTM! Logical operator fix is correct.

The change from || to && properly fixes the validation. With the old OR logic, any valid socks5:// or socks5h:// URL would incorrectly throw an error. The AND logic now correctly throws only when the URL doesn't start with either prefix.

tests/unit/fetch_test.ts (1)

2314-2344: LGTM! Tests verify the validation fix.

Both tests confirm that socks5:// and socks5h:// URLs are now accepted without throwing a TypeError. The tests use proper resource management with using and follow existing patterns.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}