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

URLs with username/password #26

Closed
hiroshige-g opened this issue Mar 26, 2015 · 36 comments
Closed

URLs with username/password #26

hiroshige-g opened this issue Mar 26, 2015 · 36 comments

Comments

@hiroshige-g
Copy link

When execute fetch() on origin http://a.com, the spec seems to allow us to fetch or redirect to a URL with username/password, if the all URLs in the redirect chain before that URL are on the origin http://a.com.
Particularly, if other conditions are OK (e.g. Access-Control-Allow-Origin is properly set),
fetch('http://user:pass@a.com/', {mode: 'cors'}): OK
fetch('http://user:pass@b.com/', {mode: 'cors'}): OK
fetch('http://a.com/', {mode: 'cors'}), which is redirected to http://user:pass@a.com/: OK
fetch('http://a.com/', {mode: 'cors'}), which is redirected to http://user:pass@b.com/: OK
fetch('http://b.com/', {mode: 'cors'}), which is redirected to http://user:pass@a.com/: Network Error
fetch('http://b.com/', {mode: 'cors'}), which is redirected to http://user:pass@b.com/: Network Error

Is my understanding is correct and is this behavior intentional?
(e.g. the spec doesn't intend to reject all cross-origin requests/redirects to URLs with username/password, right?)

@annevk
Copy link
Member

annevk commented Mar 26, 2015

Yeah, though maybe we want to change this to simply reject such URLs. Nobody seems to really like this URL format much so supporting it might not be good.

@annevk
Copy link
Member

annevk commented Apr 3, 2015

@ehsan @nikhilm do you think we can still change fetch() / Request to reject when the URL contains credentials? It's an anti-pattern and we should probably not spread it further.

@nikhilm
Copy link

nikhilm commented Apr 3, 2015

yes. we could request uplift to aurora

@inexorabletash
Copy link
Member

@hiroshige-g pointed out an interesting case in https://crbug.com/474439

If a page accesses a resource (e.g. an image) via a URL with credentials, Service Worker can intercept the fetch and will see a Request object with credentials. This request can then be used as a key in a Cache for reading or writing.

It feels slightly odd to not be able to mint such Request objects; I suppose there are other such properties (e.g. destination) that can't be synthesized from script, but those aren't considered in the Cache matching algorithm.

@wanderview
Copy link
Member

Of course, you can't run anything like the fetch() method with the intercepted request since fetch() runs the Constructor algorithm as step 2. Cache.add() also runs the Request constructor explicitly.

The only way it could end up in the Cache is if you do something like:

addEventListener('fetch', function(evt) {
  evt.respondWith(fetch(someOtherUrl).then(function(response) {
    cache.put(evt.request, response.clone());
    return response;
  }));
});

@inexorabletash
Copy link
Member

fetch() runs the Constructor algorithm

But if you pass a Request to fetch() the associated request is passed as input, not a string. So in step 12 the substeps are not run. The URL buried inside makes it through unscathed.

Same thing applies to Cache.add() - the passed Request's (API) associated request (concept) is what gets used, and it's not rejected based on credentials.

@annevk
Copy link
Member

annevk commented Sep 4, 2015

Right, I think the same applies to a page using XMLHttpRequest right now. The reason I added the restriction was that I thought we wanted to move away from such URLs so I figured I should restrict them here even though they can end up in Request objects in other ways.

There's other such cases. E.g. the only way to get a cross-origin referrer in a Request object is through a cross-origin "cors" stylesheet.

I don't think either is problematic personally, but I'm happy to make changes here obviously provided the argument is somewhat compelling.

@wanderview
Copy link
Member

But if you pass a Request to fetch() the associated request is passed as input, not a string. So in step 12 the substeps are not run. The URL buried inside makes it through unscathed.

Right. Sorry for my confusion.

@inexorabletash
Copy link
Member

I don't think either is problematic personally

Ditto. We can relax the restriction later easily enough.

@hmottestad
Copy link

I've run across an issue with using fetch because of the restrictions on credentials in urls. I posted it to the chrome issue tracker: https://bugs.chromium.org/p/chromium/issues/detail?id=596261#c7

My scenario is the following:

  • Go to a page using inline basic auth
  • Use fetch to load a resource from a relative url

Example:

The reason I want to be able to do this, is because I run a server in my house that is accessible outside of my home. To make things easy for myself I have links to the server that use inline basic auth, so it logs me in automatically. I use https to encrypt all traffic. But now I can't fetch relative urls anymore.

@annevk
Copy link
Member

annevk commented Mar 22, 2016

@hmottestad what happens for that scenario in other browsers? Do they keep the URL, including its credentials, as the base URL? I thought that maybe if you navigate to such a URL the URL would be normalized in some way. I think that's what I'd prefer (apart from removing all support for such URLs which still seems not feasible).

@hmottestad
Copy link

@annevk

With the following example:

Older version of chrome (v48): Request URL: https://test:test@www.google.no/hello
Firefox: TypeError: hello is an url with embedded credentials.
New version of Chrome fails like Firefox.

However, XMLHttpRequest works fine in Firefox:

xhttp = new XMLHttpRequest();
xhttp.open("GET", "hello", true);
xhttp.send();

Request url: https://test:test@www.google.no/hello

I would propose that the credentials checking happens before expanding relative urls.

@annevk
Copy link
Member

annevk commented Mar 23, 2016

That check cannot be done before a URL is parsed. A "relative URL" is just an ordinary string. Either we allow credentials in URLs or we don't. There's no middle ground here I think.

Presumably this also fails in Edge, which does not allow credentials in URLs to begin with. @mikewest you looked at this at some point, right, opinions?

@hmottestad
Copy link

@annevk What I meant was, allow relative urls where the base has credentials, but not absolute urls with credentials. Which can be done by checking for credentials before expanding.

Alternatively, just strip the credentials from the base when expanding relative urls. This is my work-around for the issue at the moment (by prepending window.location.origin to my relative url before performing the fetch).

@hmottestad
Copy link

@annevk @mikewest

I'm following up on this. For me this is about using relative urls with the fetch api, regardless of the actual page I am on.

@annevk
Copy link
Member

annevk commented Mar 31, 2016

Okay, let's reopen this for a couple of weeks to see if there's any implementer interest in changing this behavior.

@annevk annevk reopened this Mar 31, 2016
@hiroshige-g
Copy link
Author

Either we allow credentials in URLs or we don't. There's no middle ground here I think.

I agree with this.

Perhaps we can allow URLs with embedded credentials only if it is created by a relative URL in the Request constructor, but it might introduce subtle corner cases and I'm not sure it is more strict than allowing all URLs with embedded credentials.

Alternatively, just strip the credentials from the base when expanding relative urls.

@annevk, how about stripping credentials from the base for all cases, not only for Fetch API?
We have two mechanisms that can be used for authenticating in multiple pages: (1) authentication entries for Authorization headers and (2) propagation of the embedded credentials in URLs.
(In Chrome 49/Firefox 45, the credentials embedded in URLs are propagated: if the URL of the main HTML contains credentials, the URLs of iframe's, img's, and XHRs etc. in the page also contain the same credentials.)
If we can stop propagating credentials in URLs and rely only on authentication entries, we can make the spec/behavior simpler, prevent unintended propagation of credentials, and resolve this issue.

@annevk
Copy link
Member

annevk commented Apr 11, 2016

The URL Standard unintentionally had that behavior for a while, see whatwg/url#22 and whatwg/url#25 for when we changed it back.

I wonder what @sleevi and @valenting think about that.

@sleevi
Copy link

sleevi commented Apr 11, 2016

@annevk I'm not sure I fully grok the terminology being used here; in particular, I'm not sure what @hiroshige-g is referring to a authentication entries.

In general, Chrome is supportive of trying to get rid of credentials in URLs (see https://bugs.chromium.org/p/chromium/issues/detail?id=585109 which is perhaps most relevant to this, which comments 11/12/13/14 get to the general thinking). However, we haven't really formulated a plan of action, nor of consistency - for example, credentials from URLs can end up as internal authentication cache entries (e.g. if it sees another Authorization challenge, and it had a user/pass that worked, continue using it - but that logic itself is contingent upon where the user/pass came from; I don't believe this is covered by Fetch logic AFAIK, same as things like prompts for client certificates, as both are more 'UI' decisions then Fetch decisions).

@valenting
Copy link

I tend to agree with @hiroshige-g - it would be great if we could stop propagating credentials via URLs.
If we wanted to only disallow credentials in the Fetch API, that would be easy enough.
Removing credentials from URLs in general I think is also possible. @mcmanus has the final word on this, but I think coordinating with Chrome via the spec would be the path forward on this issue.

@annevk
Copy link
Member

annevk commented Apr 12, 2016

@sleevi @valenting sorry for being unclear, the proposal @hiroshige-g would affect how we parse URLs as I understand it. Rather than copying the username/password from a base URL that has them, we would stop doing that. This means that relative URLs in a document loaded from a URL with credentials, would no longer contain the credentials directly once parsed into a URL record. Those resources identified by those URL records would however still load since the document that was loaded ended up creating an authentication entry for the URLs.

@hmottestad
Copy link

@annevk Removing url credentials when using fetch for relative urls is an approach that will work for me.

@tyoshino
Copy link
Member

@sleevi Yes. When I was discussing this with @hiroshige-g, I was thinking about http_auth_cache_ of Chromium net/. At least, I think that use cases like the original reporter's could be resolved by hiroshige-g's proposal.

@sleevi
Copy link

sleevi commented Apr 13, 2016

Right, I would be fairly opposed to adding any implicit or explicit dependencies in the http_auth_cache_ - especially with sub resource fetches. I don't feel confident that we could offer any form of deterministic behaviour then; it already isn't guaranteed that credentials from URLs will enter that cache. In that sense, I feel @hiroshige-g's approach would be fairly complicated to implement properly, and also have subtle edge cases.

Naively, I like the proposal of only allowing it if it propagated from a relative URL, but that's mostly because it makes it invisible to the layer of the network I deal with, thus "somebody else's problem". My concern is that we've had enough subtle bugs and regressions with the cache that this feels like a very risky approach, with limited value given the overall desire of the team to obsolete/ignore credentials in URLs entirely.

That said, I would be curious what @mcmanus thinks for Firefox.

To be clear, I'm not trying to rule it out, but knowing the issues in this area, and knowing that the people I would trust to know the edge cases and test thoroughly are all rather busy, and knowing the complexities that would propagate to the rest of the stack, I'm not enthused about it, but it shouldn't be impossible.

@tyoshino
Copy link
Member

it already isn't guaranteed that credentials ...

Oh, ok. So ... I'm curious whether/where in the HTML spec, it's defined how to propagate username/password entered in a dialog propagated to sub resources. Anne.

@sleevi
Copy link

sleevi commented Apr 13, 2016

You're confusing things, @tyoshino. We track the source of authentication data and it has different processing logic. Dialog-entered credentials enter the cache, but URL or explicitly sourced credentials don't necessarily, and there's entirely separate logic for ambient credentials that may be available from secondary sources (e.g. inferred from the OS user for things like Negotiate, or sourced from low-level credential stores independent of the password manager). For Chromium, please be aware that //net has no relationship to the Fetch API or specification - that is handled by Blink, using the primitives //net exposes. However, that's also part of the concern I am expressing here - to change it as proposed would requiring auditing and changing the //net behaviour to match Fetch, which is not desirable in general, and may also have secondary effects for all the non-Fetch use cases. Further, because it is so low-level, there is greater risk of introducing regressions, in both Chrome and other //net embedders.

@tyoshino
Copy link
Member

I wanted to know whether/how the HTML spec side is taking care of caching authentication data provided by user in some common way (sorry about using "entered in a dialog" carelessly which is confusing), not URL, and using it for (not only sub but also any kind of) resource loading.

@tyoshino
Copy link
Member

I understood that net/'s authentication data caching isn't the right place to burden to work out an alternative for the URL-embeded credentials based credential propagation.

My point was, in case the HTML or Fetch spec are trying to cover any of the various logics/behaviors Ryan explained in non realistic way, we need to fix the spec to be realistic or kick it out from the scope of the HTML or Fetch spec.

@tyoshino
Copy link
Member

I had further discussion with hiroshige-g and yutakahirano offline:

  • We'd like to support the policy that we're deprecating URL with embedded credentials
  • There's a work around (build a credential-less absolute URL manually and pass it to fetch()) for the inconvenience @hmottestad is experiencing
  • We don't want to complicate spec/impl only for resolving the issue. We'd like him/her to use the work around given that fetch() with credential is not needed (URLs with username/password #26 (comment))

@annevk
Copy link
Member

annevk commented Apr 13, 2016

Fetch does say when to prompt the user for HTTP auth credentials (and when not), but at this point I'm a bit lost as to what we should do here. The last comment by @tyoshino seems to suggest we preserve the status quo, both for URL parsing and Fetch. That works for me.

@hmottestad
Copy link

@tyoshino could you make the spec ignore the credentials instead, by either having the underlying http request ignore the credentials (as was the behaviour in chrome up until a few weeks ago), or strip out the credentials when expanding the url?

My own workaround is actually what you mention. I went through every single use of fetch that I have and added window.location to make absolute urls. Which consequently broke my server side rendering in node. So it's no exactly a very good workaround.

@tyoshino
Copy link
Member

@annevk Yes. We should pursue what's the right thing to do on both spec and impl. But we don't want to add exceptional logic to fetch()'s base URL handling at this point.

Which consequently broke my server side rendering in node.

@hmottestad Could you please elaborate this?

@hmottestad
Copy link

@tyoshino the window object doesn't exist in nodejs, so when my react code gets rendered on the server it throws an undefined objects exception. So I actually have two workarounds at this time, the first is to create absolute urls with window.location, and the other is to inject an empty window object if it is null. All of this looks really ugly.

Btw. When do you think we will see browsers drop support for urls with inline credentials?

@tyoshino
Copy link
Member

How about building the hostname value inside the React JS based server code maybe by using the HOST header value?

I have no idea about other developers' plan.

@hmottestad
Copy link

@tyoshino How about a softer approach to credentials in URLs. At least until support for them is removed from modern browsers.

@annevk annevk added needs implementer interest Moving the issue forward requires implementers to express interest and removed needs implementer interest Moving the issue forward requires implementers to express interest labels Jul 22, 2016
@annevk
Copy link
Member

annevk commented Jul 22, 2016

I'm going to close this since we haven't really come up with something workable. If an implementer sees value in further change here, please file a new targeted issue. Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

9 participants