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

Disable redirects to UNIX sockets #2047

Merged
merged 5 commits into from May 25, 2022
Merged

Disable redirects to UNIX sockets #2047

merged 5 commits into from May 25, 2022

Conversation

szmarczak
Copy link
Collaborator

No description provided.

@szmarczak szmarczak marked this pull request as draft May 25, 2022 17:41
@szmarczak szmarczak marked this pull request as ready for review May 25, 2022 17:45
source/core/index.ts Outdated Show resolved Hide resolved
source/core/index.ts Outdated Show resolved Hide resolved
@lpinca
Copy link
Contributor

lpinca commented Jun 29, 2022

It seems that

  1. unix: -> http: works
  2. unix: -> unix: works
  3. http: -> unix: does not work
  4. unix: -> http: -> unix: does not work

Is 4 wanted? If so, why? The original request was made against a UNIX socket so given that 2 works I think it is ok if the client is redirected back to a UNIX socket.

Also, unrelated to this PR but 2 can leak sensitive headers if the socket path is not the same. For example http://unix/foo:/ -> http://unix/bar:/.

@szmarczak
Copy link
Collaborator Author

szmarczak commented Jul 1, 2022

@lpinca Yes, this is expected. Got has no knowledge whether to trust the http: resource or not. There's no difference when visiting directly that resource first, skipping the earlier unix: visit. Similarly, browsers have implemented CORS to prevent cross-origin requests. You can get the next unix: location via error.response.headers.location.

If that's not sufficient, I guess we could have an option to select desired behavior (#2046). Is the http: server localhost? If so, we could whitelist IPs in the localhost namespace.

Edit: or we could have an allow list, or make beforeRedirect hook accept returned value such as 'accept' or 'reject' (undefined would mean 'accept').
/cc @sindresorhus

Also, unrelated to this PR but 2 can leak sensitive headers if the socket path is not the same. For example http://unix/foo:/ -> http://unix/bar:/.

Thanks for reporting! I've opened #2069. That's rather an unlikely occurrence, since bar: needs to be a malicious process or be exploitable.

@lpinca
Copy link
Contributor

lpinca commented Jul 1, 2022

Got has no knowledge whether to trust the http: resource or not. There's no difference when visiting directly that resource first, skipping the earlier unix: visit.

The difference I was thinking about is that when the original request is made against a UNIX socket, the client already expects to hit a server running on the same machine so it is not "tricked" into doing so and the redirect should be followed (at least if the socket path is the same as the original one).

Anyway I'm not even sure if http: -> unix: should be disabled. In my opinion it is not different from a redirect to http://localhost and that is supported by all browsers.

@szmarczak
Copy link
Collaborator Author

when the original request is made against a UNIX socket, the client already expects to hit a server running on the same machine so it is not "tricked" into doing so and the redirect should be followed

What is the use case for this?

In my opinion it is not different from a redirect to http://localhost/ and that is supported by all browsers.

Servers running on UNIX sockets tend to be more "important", for example docker.

@lpinca
Copy link
Contributor

lpinca commented Jul 4, 2022

What is the use case for this?

No real use case, was just wondering why it was disabled.

Servers running on UNIX sockets tend to be more "important", for example docker.

Hmm, unconvinced. You would need a process running as root or as a member of the Docker group to access it.

Anyway I do not need support for redirects to UNIX sockets so I do not care much :).

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.

None yet

3 participants