1185640 - Passing a scope or scriptURL to register() with escaped '/' or '\' should fail

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://github.com/slightlyoff/ServiceWorker/issues/630

Comment 2

[Andrea Marchesini [:baku]]

Attachment: slashes.patch

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8646977 [details] [diff] [review]
slashes.patch

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

Sorry, but this needs to only check for the escaped slashes in the path part of the URL/scope.  Escaped slashes in the query part of the string are allowed, for example.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: slashes.patch

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8647069 [details] [diff] [review]
slashes.patch

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

r=me with comments addressed.  Thanks.

::: dom/workers/ServiceWorkerContainer.cpp
@@ +105,5 @@
> +CheckForSlashEscapedCharsInPath(nsIURI* aURI)
> +{
> +  MOZ_ASSERT(aURI);
> +
> +  nsCOMPtr<nsIURL> url(do_QueryInterface(aURI));

Assert main thread since nsIURI is being used?

@@ +116,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  ToLowerCase(path);

You don't need to explicitly lower case here.  You can just pass true for aIgnoreCase param in Find:

  path.Find("%2f", true /* ignore case */)

I guess doing a single lower case operation is marginally more CPU efficient, though.

So I don't feel strongly either way here.

::: dom/workers/test/serviceworkers/test_escapedSlashes.html
@@ +19,5 @@
> +
> +var tests = [
> +  { status:  true,
> +    scriptURL: "a.js?foo%2fbar",
> +    scopeURL: null },

Can you add a couple cases that test encoded slashes in the #anchor url ref section?

@@ +84,5 @@
> +      } else {
> +        ok(test.status, "Register should fail");
> +      }
> +    })
> +    .then(runTest);

I think we should unregister the previous service worker before going on to the next one.  Otherwise we are going to build up a lot of registrations/workers.

Comment 6

[Andrea Marchesini [:baku]]

> I think we should unregister the previous service worker before going on to
> the next one.  Otherwise we are going to build up a lot of
> registrations/workers.

All these registrations should fail because the scripts do not exist.

Comment 7

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a8f45990368

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Backed out for wpt permacrashes.
https://treeherder.mozilla.org/logviewer.html#?job_id=12821154&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=12820763&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/26e1446feb64

Comment 9

[Andrea Marchesini [:baku]]

it seems that the dependences are fully landed. We can land this patch too.

Comment 10

[Andrea Marchesini [:baku]]

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

Comment 11

[Nigel Babu [:nigelb]]

This has caused W8 bustage like this:

11. '/tools/buildbot/bin/python scripts/scripts/web_platform_tests.py ...' warnings 30m 40s 18ms
15022 01:58:01 INFO - TEST-UNEXPECTED-FAIL | /_mozilla/service-workers/service-worker/registration.https.html | Script URL is same-origin filesystem: URL - assert_throws: Registering a script which has same-origin filesystem: URL should fail with SecurityError. function "function () { throw e; }" threw object "[Exception... "Failure" nsresult: "0x80004005 (NS_ERROR_..." that is not a DOMException SecurityError: property "code" is equal to undefined, expected 18
15029 01:58:01 INFO - TEST-UNEXPECTED-FAIL | /_mozilla/service-workers/service-worker/registration.https.html | Scope URL is same-origin filesystem: URL - assert_throws: Registering with the scope that has same-origin filesystem: URL should fail with SecurityError. function "function () { throw e; }" threw object "[Exception... "Failure" nsresult: "0x80004005 (NS_ERROR_..." that is not a DOMException SecurityError: property "code" is equal to undefined, expected 18
15999 02:03:00 ERROR - Return code: 1 

https://treeherder.mozilla.org/logviewer.html#?job_id=13081326&repo=mozilla-inbound

I'll be backing out unless I hear otherwise.

Comment 12

[Nigel Babu [:nigelb]]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d72462f193b4

Comment 13

[Andrea Marchesini [:baku]]

nsm, can you take this and land it with the other fixes?

Comment 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/7c5cc8cfca19afaeb4af08c669a78219edea72c4
changeset:  7c5cc8cfca19afaeb4af08c669a78219edea72c4
user:       Andrea Marchesini <[e-mail]>
date:       Wed Aug 19 13:23:58 2015 -0700
description:
Bug 1185640 - serviceworker register() should not accept escaped slashes. r=bkelly

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7c5cc8cfca19