Skip to content

Reject unencoded unix socket URLs instead of silently hitting localhost - #1854

Open
afonsojanu wants to merge 1 commit into
forwardemail:masterfrom
afonsojanu:fix/unix-socket-url-percent-encoding-error
Open

Reject unencoded unix socket URLs instead of silently hitting localhost#1854
afonsojanu wants to merge 1 commit into
forwardemail:masterfrom
afonsojanu:fix/unix-socket-url-percent-encoding-error

Conversation

@afonsojanu

Copy link
Copy Markdown

Closes #1767.

If a http+unix:// (or https+unix://) URL's socket path still has literal slashes in it instead of being percent-encoded, the WHATWG URL parser can't separate the socket path from the request path at all. Everything after // just becomes the pathname and the hostname comes out empty, so options.socketPath ends up as an empty string.

Right now that falls all the way through to actually issuing an HTTP request against localhost with a null path, which is a pretty confusing thing to debug. You end up chasing an ECONNREFUSED against a host you never asked for instead of getting any hint that the URL itself was the problem.

I checked exactly what request options were being built for a URL like http+unix:///path/to/socket.sock/the/actual/url:

{ socketPath: '', host: 'localhost', port: 80, path: null, ... }

so the fix is to catch that at the point the socket path gets pulled out of url.hostname, before any of that gets used to build a request. When the hostname is empty in the +unix: branch, the request now fails immediately with a message pointing at the actual fix (percent-encode the slashes), routed through the same callback() path used for aborted requests, so it reaches whatever was passed to .end() the normal way.

Added a test alongside the existing unix socket tests covering the correctly-encoded case, using a deliberately unencoded path and asserting on the new error message. Ran the full test/node/unix-sockets.js file (still 5/5 including the two pre-existing http/https cases) and eslint -c .eslintrc src test clean (matches what CI runs).

One thing I noticed but didn't touch: the local xo-based pre-commit hook currently fails on this file for reasons unrelated to this change (missing node: protocol prefixes, a few prettier formatting nits, a couple of guard-for-in warnings elsewhere in the file) — looks like pre-existing drift between the xo config and the codebase rather than anything introduced here, so I left it alone rather than bundling an unrelated cleanup into this PR.

…calhost

If the socket path in a http+unix:// or https+unix:// URL still has literal
slashes instead of being percent-encoded, the URL parser can't tell the
socket path apart from a regular path. It ends up with an empty hostname,
and superagent was falling back to a plain HTTP request against localhost
with a null path, which just fails with a confusing ECONNREFUSED that has
nothing to do with the actual mistake.

Now this case is caught right where the socket path gets parsed out, and
the request fails with a message that tells you what to do about it.

Fixes forwardemail#1767
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.

Unix socket is expected to be encoded with %2F

1 participant