Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use non-compliant URL from 'url' for socks urls #242

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

indutny-signal
Copy link
Contributor

new URL('socks://host:port') gives different results in browser and Node.js because socks is not among the "special schemes" specified by WHATWG and thus the hostname is treated as an opaque value (without parsing the port). Node.js implementation, however, parses the port.

Since proxy-agent can be used in Electron, unless URL is imported directly from the Node's url module - it is going to use the Browser's version of it which won't work with socks urls.

@vercel
Copy link

vercel bot commented Aug 29, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
proxy-agents ✅ Ready (Inspect) Visit Preview Aug 29, 2023 7:14pm

@changeset-bot
Copy link

changeset-bot bot commented Aug 29, 2023

🦋 Changeset detected

Latest commit: d51474a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
socks-proxy-agent Patch
pac-proxy-agent Patch
proxy-agent Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

`new URL('socks://host:port')` gives different results in browser and
Node.js because `socks` is not among the "special schemes" specified by
WHATWG and thus the hostname is treated as an opaque value (without
parsing the port). Node.js implementation, however, parses the `port`.

Since `proxy-agent` can be used in Electron, unless `URL` is imported
directly from the Node's `url` module - it is going to use the Browser's
version of it which won't work with socks urls.
@TooTallNate TooTallNate merged commit 1d39f6c into TooTallNate:main Sep 4, 2023
14 checks passed
@TooTallNate
Copy link
Owner

Makes sense. Thank you!

@indutny-signal indutny-signal deleted the fix/electron-support branch September 4, 2023 17:59
@indutny-signal
Copy link
Contributor Author

Yay, thanks for merging this!

@lukekarrys
Copy link
Collaborator

It might be helpful to use a lint rule for this as well to keep it from possibly regressing.

I think no-restricted-globals should do the trick:

{
    "rules": {
        "no-restricted-globals": ["error", { "name": "URL", "message": "Use url.URL instead" }]
    }
}

@TooTallNate
Copy link
Owner

@lukekarrys would be happy to accept a PR!

@lukekarrys
Copy link
Collaborator

lukekarrys commented Sep 8, 2023

would be happy to accept a PR!

Done 😄! #246

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.

3 participants