Prevent usage of Basic Auth in URL about got

[stevenvachon]

Breaking the Internet is not a good idea.

As for supporting it in this library, consider this example:

function doSomething(url) {
  let auth = '';

  // shouldn't be necessary
  if (url.username || url.password) {
    url = new URL(url);  // wasted resources
    auth = `${url.username}:${url.password}`;
    url.username = url.password = '';
  }

  return got(url, {auth});
}

const url = new URL('http://user:pass@host/');  // from an HTML document

doSomething(url).then(() => {});

This library's goal is to make requests easier, not annoying.

[alextes]

Hm, I read over this, the RFC, our code, node's code, and I'm not sure why we throw when we do. Node seems to already turn user:[email protected] into an Authorization header. So at least the current suggestion: Basic authentication must be done with the 'auth' option doesn't make much sense to me. If I'm reading our code right, we parse the url, and when we do, both old and new url parsing from node build an auth option for you. No reason to force the user to do this. I'd suggest removing that part.

What would be cool is to help people not expose credentials by using basic auth over http. It feels like that was the original intention, in that case, I'd suggest we console.warn or throw an error, that helpfully explains we try to help you keep your users safe in the wild web west, but if you know what you're doing, set unsafeAuth to true, and we'll get out of your way.

Forced or annoying security usually leads to bypassed security.

[paulmelnikow]

And – thank you all so much for your work on this useful library! I've just adopted it in another project and appreciate how easy it is to use, and for other contributors to pick up.

[alextes]

I realize I'm showing up after the work is done and then complaining about the outcome
We still appreciate the feedback 😊 .

Maybe I can help conclude the discussion. I think I speak for all got maintainers when I say, we want to help devs as much as possible 😚 . With basic auth we faced an interesting dilemma. Would we help users more by silently exposing their credentials, or by loudly telling them there are better ways for what they're trying to do? We initially went with the latter, now we switched to the former.

Both your positions are noted. As soon as people show up sad that their credentials got exposed, in numbers greater than the one or two upset that an error got thrown, I'm sure we'd be happy to reconsider.

[stevenvachon]

It was only removed from Chrome for a few versions. The feature was re-added https://bugs.chromium.org/p/chromium/issues/detail?id=123150

I think this change should be reversed. @sindresorhus

[sindresorhus]

Chrome caving to users doesn't mean deprecating it wasn't the right move and doesn't mean we should follow them.

[alextes]

@stevenvachon that doesn't change anything about the fact it's deprecated though. Are you saying we should ignore the RFC?

Page hadn't refreshed yet. Sorry!

[alextes]

I'm with Steven on this one. I feel Chrome was very wrong to cave and other 'expectation setters' like browsers wrong to ignore the RFC. Node and other HTTP communicators should do more to protect their users' security, especially using plain HTTP. I also feel this is not our battle.

Question: the only strongly worded issue I can find is the use of auth in URLs and if I understand correctly Node 4 is already silently rewriting those. Technically we'd still comply with the RFC no?

[lpinca]

Would you accept a PR that reverts 62ff082? It doesn't make sense imho as the auth option is a valid option for http.request().

On top of this #329 introduced a breaking change in version 7.1.0. Previously the error was thrown only when using a URL string. Now it is thrown even when using a URL object.

got({ hostname: 'example.com', auth: 'foo:bar' })

should not throw/reject.

[felixfbecker]

I have a Node backend that is doing requests on arbitrary provided URLs. It seems pointless to me that I have to manually parse the URL, check if there is auth in it, then put it into the auth option. It's nothing but unneeded code and trips up users because they don't know/expect they need to handle this manually.

[felixfbecker]

const auth = [url.username, url.password].filter(Boolean).join(':') || undefined
const u = new URL(url.href)
u.password = ''
u.username = ''
await got(u.href, { auth })

[felixfbecker]

Note that auth and auth in the URL are not the same. There are APIs (e.g. GitHub, Sourcegraph) that allow you give an access token in the URL, which is convenient in environments where you can only configure a URL but not e.g. headers. But when put into the Authorization header, these APIs usually require it to be with the Bearer scheme (or Token), not Basic. auth however sets Basic, which is usually not recognized.

I also don't understand why this decision was made based on Chrome's decision despite the readme saying

Got is for Node.js. For browsers, we recommend Ky.

when Node has never deprecated this (and I doubt ever will).

[felixfbecker]

Node seems to already turn user:[email protected] into an Authorization header.

Are you sure? I used request and axios against an API that supports an auth token as the "username" in the URL, but not Authorization: Basic (only Authorization: token), and they both work (while got throws).

[felixfbecker]

A console.warn() (made to only log once) and an option unsafeAuth sounds fine to me.

[szmarczak]

@sindresorhus What's your thoughts?

[sindresorhus]

You're right that it's not our battle, and I don't really care enough to do all this. Let's just remove the check.

[paulmelnikow]

I noticed this discussion via the changelog. I realize I'm showing up after the work is done and then complaining about the outcome… so take this heavily salted!

For what it's worth, I appreciated the check, and wanted to offer support of the intent behind it. In-URL auth is obscure. Most devs probably do not think about the fact that a URL can contain a username and password which ends up in a header.

I'm also passing off user-provided URLs to make request on the server. I don't want to accept in-url auth, which I can handle with validation. Unfortunately the validation library I'm using (Joi) doesn't support this yet, so this is in the backlog.

In the rare case someone wants to use in-URL auth, suppressing the warning could be handled using an option like unsafeAuth: true as @alextes suggested.

But when put into the Authorization header, these APIs usually require it to be with the Bearer scheme (or Token), not Basic. auth however sets Basic, which is usually not recognized.

Is that really true? It sounds like the auth string is not sent to the server in the URL. Rather, it's the requester's job to translate the username and password to a Basic auth header. https://serverfault.com/a/371918

[felixfbecker]

Imo if you want to prevent that you should add this validation at the top layer where the user input enters your application, not at the lowest layer where you start making the request.
It's a oneliner: url => !!new URL(url).username. Iirc joi supports adding your own validation functions.

[paulmelnikow]

Yea. It's in the backlog :)

[felixfbecker]

Note that for example OAuth2 also specs the access_token query parameter, which is also part of the URL, but got doesn't throw or warn on that :)

[alextes]

Noted. Although that doesn't seem to be an example of accidentally exposing credentials 😉 .