Skip to content

fix(fetch): fix SOCKS5 proxy URL validation logic#31604

Merged
bartlomieju merged 1 commit intodenoland:mainfrom
majiayu000:fix/issue-31356-socks5-proxy-check
Dec 18, 2025
Merged

fix(fetch): fix SOCKS5 proxy URL validation logic#31604
bartlomieju merged 1 commit intodenoland:mainfrom
majiayu000:fix/issue-31356-socks5-proxy-check

Conversation

@majiayu000
Copy link
Copy Markdown
Contributor

@majiayu000 majiayu000 commented Dec 14, 2025 •
edited
Loading

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
Copy link
Copy Markdown

CLAassistant commented Dec 14, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Dec 14, 2025 •
edited
Loading

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

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

@majiayu000
fix(fetch): fix SOCKS5 proxy URL validation logic
e7f1ed9 
The condition for validating SOCKS5 proxy URLs was using `||` (OR) instead
of `&&` (AND), which caused valid socks5:// and socks5h:// URLs to always
throw a TypeError. The correct logic should only throw when the URL does
NOT start with "socks5:" AND does NOT start with "socks5h:".

Closes denoland#31356
@majiayu000 majiayu000 force-pushed the fix/issue-31356-socks5-proxy-check branch from 0efde7f to e7f1ed9 Compare December 14, 2025 08:24
bartlomieju
bartlomieju approved these changes Dec 18, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

I verified that this fails on main. Thanks!

@bartlomieju bartlomieju merged commit 1d77081 into denoland:main Dec 18, 2025
21 checks passed
@majiayu000
Copy link
Copy Markdown
Contributor Author

majiayu000 commented Dec 18, 2025

Happy to help.

@majiayu000 majiayu000 deleted the fix/issue-31356-socks5-proxy-check branch December 18, 2025 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bartlomieju bartlomieju bartlomieju approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Deno.HttpClient cannot use a SOCKS5 proxy

3 participants

@majiayu000 @CLAassistant @bartlomieju