1195820 - fetch() and new Request() should throw TypeError on URL with username/password

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Step 13.3 says:

  If parsedURL includes credentials, throw a TypeError. 

Here:

  https://fetch.spec.whatwg.org/#dom-request

As far as I can tell we don't do this.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Proposed patch

I don't know if this is the official way to submit a potential patch (this is my first contribution to Firefox), but I hope so!

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I also throw the type error on parse failure, like the spec says in the same step.

I don't know if I've made any mistakes, let me know if I did please :)

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: firefox-1195820.diff

v2 (sorry, submitted it to quickly! :P)

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Thanks Emilio!  I'll review this tomorrow.

Also, can you add a test?  It should be straightforward to add something to:

  https://dxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/fetch/test_request.js

You can run the test locally by executing:

  ./mach mochitest dom/tests/mochitest/fetch/test_request.html

Thanks again.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Great, sure!

It actually breaks some tests from wpt (service-workers/cache-storage) when the url has credentials, even though I think it's the correct behaviour after reading http://www.w3.org/TR/service-workers/#cache-put.

Should I modify those too? (I guess not because the tests live in https://github.com/w3c/web-platform-tests but...)

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Nice catch!  I think the way to handle this is to mark the tests "expected fail" for now and write a new bug to get the upstream tests fixed.  You can mark them expected fail by adding to the .ini files like:

  testing/web-platform/meta/service-workers/cache-storage/window/cache-put.https.html.ini

And add an entry like:

  [Cache.put with request URLs containing embedded credentials]
    expected: FAIL
    bug: <url to the bug you write to fix the upstream tests>

If you're interested I can help you write a PR to the upstream WPT tests to fix the tests as well.

Thanks again!

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: firefox-1195820.diff

v3 (hope this is all!)

I think I'll have no problems with writing the PR to web-platform-tests, even though I don't have more time today (It's almost 6 A.M. here!).

I've filled an issue instead to keep track of it until I write it (https://github.com/w3c/web-platform-tests/issues/2098).

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: firefox-1195820.diff

Arrgg!!! Little whitespace fix in the `hasCredentials` assignment.

Sorry for your inbox!

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8650276 [details] [diff] [review]
firefox-1195820.diff

Review of attachment 8650276 [details] [diff] [review]:
-----------------------------------------------------------------

This looks really good!  I have some style/structure comments, but otherwise it seems correct.  I think maybe a couple of the wpt test annotations might be off, though.

Please re-flag me for review after addressing the comments.  Thanks!

::: dom/fetch/Request.cpp
@@ +72,4 @@
>  namespace {
>  void
>  GetRequestURLFromDocument(nsIDocument* aDocument, const nsAString& aInput,
> +                          nsAString& aRequestURL, ErrorResult& aRv, bool& hasCredentials)

So, I think it might be a little cleaner to just throw the TypeError directly instead of using a new hasCredentials out param.  After all, we already have the ErrorResult for communicating failures out of the functions.

Also, some code style notes for future reference:

Gecko style uses a convention where arguments are prefixed with an "a".  So "aHasCredentials".

I also have a personal preference to append "Out" for arguments that are passing information back out of the function.  So "aHasCredentialsOut".

Finally, I think we normally have the convention of putting ErrorResult last in the list.

But again, that's just for future reference.  Lets throw the error directly.

@@ +92,5 @@
>      return;
>    }
>  
> +  nsAutoString credentials;
> +  aRv = resolvedURIClone->GetUserPass(credentials);

Please do the userpass check before CloneIgnoringRef() call.  No need to allocate more memory if we are going to fail.

@@ +97,5 @@
> +  if (NS_WARN_IF(aRv.Failed())) {
> +    return;
> +  }
> +
> +  hasCredentials = !credentials.IsEmpty();

So here:

  if (!credentials.IsEmpty()) {
    aRv.ThrowTypeError(MSG_URL_HAS_CREDENTIALS, &aInput);
  }

Note, I think we should have a new message here to be more specific.  You can add the MSG_URL_HAS_CREDENTIALS message to dom/bindings/Errors.msg.  (Sorry, this will cause a lot of stuff to recompile.)

@@ +129,5 @@
>      return;
>    }
>  
> +  nsAutoString credentials;
> +  aRv = uriClone->GetUserPass(credentials);

Again, please do this before the CloneIgnoringRef().

@@ +134,5 @@
> +  if (NS_WARN_IF(aRv.Failed())) {
> +    return;
> +  }
> +
> +  hasCredentials = !credentials.IsEmpty();

Again, I think its better to just throw the TypeError here.

@@ +166,5 @@
>      return;
>    }
>  
> +  nsString username;
> +  url->GetUsername(username, aRv);

Please do the username and password checks before the SetHash() here.

@@ +177,5 @@
> +  if (NS_WARN_IF(aRv.Failed())) {
> +    return;
> +  }
> +
> +  hasCredentials = !username.IsEmpty() || !password.IsEmpty();

Again, throw the TypeError here instead of using a hasCredentials out param.

@@ +245,5 @@
>  
> +    // Either if is a parse failure, or has credentials in the url
> +    // we throw a TypeError
> +    if (NS_WARN_IF(aRv.Failed()) || hasCredentials) {
> +      aRv.ThrowTypeError(MSG_INVALID_URL, &input);

And then we can lose this block since we're throwing in the individual methods.

I realize we end up with slightly more code, but its nice to avoid the added complexity of an out param if we don't really need it.

::: dom/tests/mochitest/fetch/test_request.js
@@ +223,5 @@
> +  try {
> +    var req = new Request("http://[e-mail]");
> +    ok(false, "URLs with credentials should be rejected");
> +  } catch(e) {
> +    is(e.name, "TypeError", "URLs with credentials should be rejected");

Nice!

::: testing/web-platform/meta/service-workers/cache-storage/serviceworker/cache-match.https.html.ini
@@ +2,5 @@
>    type: testharness
>    prefs: [dom.serviceWorkers.enabled: true, dom.serviceWorkers.interception.enabled: true, dom.serviceWorkers.exemptFromPerDomainMax:true, dom.caches.enabled:true]
> +  [Cache.match and Cache.matchAll]
> +    expected: FAIL
> +    bug: https://github.com/w3c/web-platform-tests/issues/2098

Thanks!

::: testing/web-platform/meta/service-workers/cache-storage/window/cache-match.https.html.ini
@@ +1,4 @@
>  [cache-match.https.html]
>    type: testharness
>    prefs: [dom.caches.enabled:true]
> +  expected: ERROR

Is there something more breaking in the window and worker versions of the cache-match.https.html test?  Can this just be the same single test case annotation for FAIL that you have in serviceworkers/cache-match.https.html?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Ben Kelly [:bkelly] from comment #9)
> Also, some code style notes for future reference:
> 
> Gecko style uses a convention where arguments are prefixed with an "a".  So
> "aHasCredentials".
> 
> I also have a personal preference to append "Out" for arguments that are
> passing information back out of the function.  So "aHasCredentialsOut".
> 
> Finally, I think we normally have the convention of putting ErrorResult last
> in the list.
Thanks! Will have it in account in the future :)

> Is there something more breaking in the window and worker versions of the
> cache-match.https.html test?  Can this just be the same single test case
> annotation for FAIL that you have in serviceworkers/cache-match.https.html?

Yes, there is: In the window and worker version the script is evaluated in the global context, so the incorrect `new Request()` throws and breaks the test.

In serviceworkers it's encapsulated in a global promise test, so it breaks just that test (which is really all the tests but...).

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: firefox-1195820.diff

I catched the parse failures in the constructor with aRv.ErrorCodeIs, to be complaint with the spec (if the parse fails, throw a TypeError), hope it's fine :P

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: firefox-1195820.diff

Updated it to ignore the failure of GetUserPass.

Calling it on an URI like `javascript://abc` fails with NS_ERROR_FAILURE, even when it's a perfectly valid URI.

It was making a cache test fail (Cache.add should throw a TypeError for non-HTTP/HTTPS URLs.)

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8651017 [details] [diff] [review]
firefox-1195820.diff

Review of attachment 8651017 [details] [diff] [review]:
-----------------------------------------------------------------

This looks really good, but there are a few tweaks to the error handling I'd like to see.  And nice catch on properly handling URLs with javascript:// scheme!

::: dom/fetch/Request.cpp
@@ +87,5 @@
> +  nsAutoCString credentials;
> +  aRv = resolvedURI->GetUserPass(credentials);
> +  // This fails with URIs with weird protocols, even when they are valid,
> +  // so we ignore the failure
> +  if (!NS_WARN_IF(aRv.Failed())) {

I think a better way to do this would be to ignore the return value from GetUserPass() and rely on the credentials string being left with the default empty value.  You can do that with something like:

  // This fails with URIs with weird protocols, even when they are valid,
  // so we ignore the failure
  nsAutoCString credentials;
  unused << resolvedURI->GetUserPass(credentials)
  if (!credentials.IsEmpty()) {
    // ...
  }

@@ +127,5 @@
> +  nsAutoCString credentials;
> +  aRv = uri->GetUserPass(credentials);
> +  // This fails with URIs with weird protocols, even when they are valid,
> +  // so we ignore the failure
> +  if (!NS_WARN_IF(aRv.Failed())) {

Same pattern here for ignoring the error value.

@@ +252,4 @@
>      }
>  
>      if (NS_WARN_IF(aRv.Failed())) {
> +      if (aRv.ErrorCodeIs(NS_ERROR_MALFORMED_URI)) {

Rather than convert the error here, I think it would be preferable to call aRv.ThrowTypeError() directly when the NS_NewURI() or URI::Constructor() calls fail.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: firefox-1195820.diff

There it goes again, with the suggestions applied :)

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8651843 [details] [diff] [review]
firefox-1195820.diff

Review of attachment 8651843 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great.  Thanks! r=me

Are you setup to do a try build?

  https://wiki.mozilla.org/Build:TryServer

To do so you need Level 1 commit access:

  https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/

If you don't have this, I can push a try for you on this bug.  Feel free to NI me on a level 1 commit access bug, though, and I will vouch for you.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

I went ahead and pushed a try build:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=7241ace08d55

Also, can you configure your mercurial to provide author information?  For example, in my .hgrc I have:

  [ui]  
  username = Ben Kelly <[e-mail]>

Also, please provide a commit message on the patch.  Something like:

  Bug 1195820 Request constructor should throw TypeError if URL has credentials. r=bkelly

I use mq to track patches:

  https://developer.mozilla.org/en-US/docs/Archive/Mozilla/Mercurial/Queues

Although I guess there is a new way to do this with the evolve plugin.  I'm not familiar with that yet, though.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Looks like there may be another test that needs fixing:

23 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/fetch/test_fetch_cors.html | Some test failed in test_fetch_cors.js
27 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/fetch/test_fetch_cors.html | Expected test failure for ({pass:1, method:"GET", noAllowPreflight:1})
29 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/fetch/test_fetch_cors.html | Expected test failure for ({pass:1, method:"GET", noAllowPreflight:1}) - Result logged after SimpleTest.finish()
30 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/fetch/test_fetch_cors.html | Exception should be TypeError for ({pass:1, method:"GET", noAllowPreflight:1}) - Result logged after SimpleTest.finish()
43 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/fetch/test_fetch_cors_sw_reroute.html | Some test failed in test_fetch_cors.js
48 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/fetch/test_formdataparsing.html | Expected test failure for ({pass:1, method:"GET", noAllowPreflight:1})
50 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/fetch/test_formdataparsing.html | Expected test failure for ({pass:1, method:"GET", noAllowPreflight:1, username:(void 0)})
52 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/fetch/test_formdataparsing.html | Expected test failure for ({pass:1, method:"GET", noAllowPreflight:1, username:(void 0), password:(void 0)})

Comment 18

[Emilio Cobos Álvarez (:emilio)]

(In reply to Ben Kelly [:bkelly] from comment #16)
> I went ahead and pushed a try build:
> 
>   https://treeherder.mozilla.org/#/jobs?repo=try&revision=7241ace08d55
> 
> Also, can you configure your mercurial to provide author information?  For
> example, in my .hgrc I have:
> 
>   [ui]  
>   username = Ben Kelly <[e-mail]>
> 
> Also, please provide a commit message on the patch.  Something like:
> 
>   Bug 1195820 Request constructor should throw TypeError if URL has
> credentials. r=bkelly

Done! Sorry, previous patches were created using `git diff`. I'll submit a new one right now.

(In reply to Ben Kelly [:bkelly] from comment #15)
> If you don't have this, I can push a try for you on this bug.  Feel free to NI me on a level 1 commit access bug, though, and I will vouch for you.
I definitely don't have any commit access... I guess NI is need more information? I suppose I'd have to do something like Bug 1187909, am I right?

Comment 19

[Emilio Cobos Álvarez (:emilio)]

(In reply to Ben Kelly [:bkelly] from comment #17)
> Looks like there may be another test that needs fixing:
> 
> 23 INFO TEST-UNEXPECTED-FAIL |
> dom/tests/mochitest/fetch/test_fetch_cors.html | Some test failed in
> test_fetch_cors.js
> 27 INFO TEST-UNEXPECTED-FAIL |
> dom/tests/mochitest/fetch/test_fetch_cors.html | Expected test failure for
> ({pass:1, method:"GET", noAllowPreflight:1})
> 29 INFO TEST-UNEXPECTED-FAIL |
> dom/tests/mochitest/fetch/test_fetch_cors.html | Expected test failure for
> ({pass:1, method:"GET", noAllowPreflight:1}) - Result logged after
> SimpleTest.finish()
> 30 INFO TEST-UNEXPECTED-FAIL |
> dom/tests/mochitest/fetch/test_fetch_cors.html | Exception should be
> TypeError for ({pass:1, method:"GET", noAllowPreflight:1}) - Result logged
> after SimpleTest.finish()
> 43 INFO TEST-UNEXPECTED-FAIL |
> dom/tests/mochitest/fetch/test_fetch_cors_sw_reroute.html | Some test failed
> in test_fetch_cors.js
> 48 INFO TEST-UNEXPECTED-FAIL |
> dom/tests/mochitest/fetch/test_formdataparsing.html | Expected test failure
> for ({pass:1, method:"GET", noAllowPreflight:1})
> 50 INFO TEST-UNEXPECTED-FAIL |
> dom/tests/mochitest/fetch/test_formdataparsing.html | Expected test failure
> for ({pass:1, method:"GET", noAllowPreflight:1, username:(void 0)})
> 52 INFO TEST-UNEXPECTED-FAIL |
> dom/tests/mochitest/fetch/test_formdataparsing.html | Expected test failure
> for ({pass:1, method:"GET", noAllowPreflight:1, username:(void 0),
> password:(void 0)})

I'll take a look at them ASAP :P

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Emilio Cobos Álvarez from comment #18)
> I definitely don't have any commit access... I guess NI is need more
> information? I suppose I'd have to do something like Bug 1187909, am I right?

Right.  File a bug like that and then you can NI me on it.  That way I can find the bug to comment and vouch for you.

Thanks!

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Attachment: firefox-1195820.patch

There it goes with the tests fixed. I kept the tests and just enclosed the request constructor in a try/catch because the expected results are the same.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8651913 [details] [diff] [review]
firefox-1195820.patch

Review of attachment 8651913 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable, but I think it would be a bit better to handle the error within the promise chain.

::: dom/tests/mochitest/fetch/test_fetch_cors.js
@@ +741,5 @@
> +    try {
> +      var request = new Request(req.url, { method: req.method, mode: "cors",
> +                                           headers: req.headers, body: req.body });
> +      fetches.push((function(test, request) {
> +        return fetch(request).then(function(res) {

Rather than use a try/catch, can you create the Request object in a promise at the start of this chain?  Something like:

  return new Promise(function(resolve) {
    resolve(new Request(req.url, {method: req.method, mode: "cors",
                       headers: req.headers, body: req.body }));
  }).then(function(request) {
    return fetch(request);
  }).then(res) {

Then the existing .catch() should work.

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Attachment: firefox-1195820.patch

Sorry for being a bit slow :P

There it is, I hope ;)

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8651961 [details] [diff] [review]
firefox-1195820.patch

Review of attachment 8651961 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/tests/mochitest/fetch/test_fetch_cors.js
@@ +743,5 @@
> +      return new Promise(function(resolve) {
> +        resolve(new Request(req.url, { method: req.method, mode: "cors",
> +                                         headers: req.headers, body: req.body }));
> +      }).then(function(request) {
> +        return fetch(request).then(function(res) {

Sorry for being nit-picky here, but we shouldn't need to change the indentation of all this code.  I think the preferred style in a promise chain like this is to return fetch() and add the next .then() on the main chain.  So:

  }).then(function(request) {
    return fetch(request);
  }).then(function(res) {

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Attachment: firefox-1195820.patch

Yeah, sorry, I realised (I was not too used to work with promises) and I was working on it while you were replying.

I dedented everything the most I could, I think that way is more clear.

Thanks and sorry again :/

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8651968 [details] [diff] [review]
firefox-1195820.patch

Review of attachment 8651968 [details] [diff] [review]:
-----------------------------------------------------------------

No worries. Looks good!  Thanks for tweaking it for me. r=me

I'll push another try run for you.

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d36e91b9ed7e

Comment 28

[Emilio Cobos Álvarez (:emilio)]

I guess all went fine?

Can I do anything else? :)

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Yea, the try build looks green.  So after that you can add the checkin-needed keyword to the bug to get the sheriffs to land your patch.

Thanks again!

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba8758c89322

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ba8758c89322

Comment 32

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/fetch-and-new-request-now-throws-if-url-includes-credentials/

Comment 33

[Saurabh Nair [:jsx] (not reading bugmail. ni? if you need me)]

Updated:
https://developer.mozilla.org/en-US/docs/Web/API/GlobalFetch/fetch
https://developer.mozilla.org/en-US/docs/Web/API/Request/Request

Dev notes:
https://developer.mozilla.org/en-US/Firefox/Releases/43#Miscellaneous

Comment 34

[Chris Mills [:cmills]]

(In reply to Saurabh Nair [:jsx] from comment #33)
> Updated:
> https://developer.mozilla.org/en-US/docs/Web/API/GlobalFetch/fetch
> https://developer.mozilla.org/en-US/docs/Web/API/Request/Request
> 
> Dev notes:
> https://developer.mozilla.org/en-US/Firefox/Releases/43#Miscellaneous

Thanks, jsx! I've updated the pages to include an "Error" section, with an error table, as I think that's how we more commonly specify a method's particular errors now.