Closed Bug 1195820 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: bkelly, Assigned: emilio, Mentored)

References

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [good first bug][lang=c++])

Attachments

(1 file, 9 obsolete files)

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.
Attached patch Proposed patch (obsolete) — Splinter Review
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!
Attachment #8650217 - Flags: review?(bkelly)
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 :)
Attached patch firefox-1195820.diff (obsolete) — Splinter Review
v2 (sorry, submitted it to quickly! :P)
Attachment #8650217 - Attachment is obsolete: true
Attachment #8650217 - Flags: review?(bkelly)
Attachment #8650226 - Flags: review?(bkelly)
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.
Assignee: nobody → ecoal95
Status: NEW → ASSIGNED
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...)
Flags: needinfo?(bkelly)
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!
Flags: needinfo?(bkelly)
Attached patch firefox-1195820.diff (obsolete) — Splinter Review
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).
Attachment #8650226 - Attachment is obsolete: true
Attachment #8650226 - Flags: review?(bkelly)
Attachment #8650275 - Flags: review?(bkelly)
Attached patch firefox-1195820.diff (obsolete) — Splinter Review
Arrgg!!! Little whitespace fix in the `hasCredentials` assignment.

Sorry for your inbox!
Attachment #8650275 - Attachment is obsolete: true
Attachment #8650275 - Flags: review?(bkelly)
Attachment #8650276 - Flags: review?(bkelly)
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://user@example.com");
> +    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?
Attachment #8650276 - Flags: review?(bkelly)
(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...).
Attached patch firefox-1195820.diff (obsolete) — Splinter Review
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
Attachment #8650276 - Attachment is obsolete: true
Attachment #8651000 - Flags: review?(bkelly)
Attached patch firefox-1195820.diff (obsolete) — Splinter Review
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.)
Attachment #8651000 - Attachment is obsolete: true
Attachment #8651000 - Flags: review?(bkelly)
Attachment #8651017 - Flags: review?(bkelly)
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.
Attachment #8651017 - Flags: review?(bkelly)
Attached patch firefox-1195820.diff (obsolete) — Splinter Review
There it goes again, with the suggestions applied :)
Attachment #8651017 - Attachment is obsolete: true
Attachment #8651843 - Flags: review?(bkelly)
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.
Attachment #8651843 - Flags: review?(bkelly) → review+
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 <ben@wanderview.com>

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.
Flags: needinfo?(ecoal95)
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)})
(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 <ben@wanderview.com>
> 
> 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?
Flags: needinfo?(ecoal95)
(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
(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!
Attached patch firefox-1195820.patch (obsolete) — Splinter Review
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.
Attachment #8651843 - Attachment is obsolete: true
Attachment #8651913 - Flags: review?(bkelly)
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.
Attachment #8651913 - Flags: review?(bkelly)
Attached patch firefox-1195820.patch (obsolete) — Splinter Review
Sorry for being a bit slow :P

There it is, I hope ;)
Attachment #8651913 - Attachment is obsolete: true
Attachment #8651961 - Flags: review?(bkelly)
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) {
Attachment #8651961 - Flags: review?(bkelly)
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 :/
Attachment #8651961 - Attachment is obsolete: true
Attachment #8651968 - Flags: review?(bkelly)
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.
Attachment #8651968 - Flags: review?(bkelly) → review+
I guess all went fine?

Can I do anything else? :)
Flags: needinfo?(bkelly)
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!
Flags: needinfo?(bkelly)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ba8758c89322
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.