Comments (21)
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.
from got.
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.
from got.
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.
from got.
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.
from got.
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
from got.
Chrome caving to users doesn't mean deprecating it wasn't the right move and doesn't mean we should follow them.
from got.
@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!
from got.
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?
from got.
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.
from got.
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.
from got.
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 })
from got.
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).
from got.
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).
from got.
A console.warn()
(made to only log once) and an option unsafeAuth
sounds fine to me.
from got.
@sindresorhus What's your thoughts?
from got.
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.
from got.
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 theBearer
scheme (orToken
), notBasic
.auth
however setsBasic
, 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
from got.
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.
from got.
Yea. It's in the backlog :)
from got.
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 :)
from got.
Noted. Although that doesn't seem to be an example of accidentally exposing credentials π .
from got.
Related Issues (20)
- `Response<string>` returned rather than a json response HOT 2
- Is v13 still maintained/supported? HOT 1
- Hangs on revalidated response from cache
- Infinite redirection with special characters in the redirected URL HOT 1
- Support `servername` https option
- How to get traffic usage per request? HOT 1
- Is it possible to skip/disable normalization of a request body? HOT 1
- afterResponse fired 3 times on error HOT 3
- Getting the IP address for failed requests due to timeout
- Post request to API returns response code 500 (Internal Server Error) whereas CURL returns 200 status code HOT 2
- Timeouts are longer than expected HOT 1
- NPM Says: Types are not integrated HOT 1
- FormData getLength can error out when some file input is unknown stream.
- POSTs whose bodys are instances of createReadStream hang with latest node HOT 5
- Apply `applyDestroyPatch` for v11.x
- Stream report downloadProgress when redirects HOT 1
- Is TypeScript support outdated? HOT 1
- Good News for CJS
- afterResponse hook is not called after a retryWithMergedOptions() HOT 2
- HTTP2 with proxy doesn't work? (http2-wrapper)
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
π Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. πππ
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google β€οΈ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from got.