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

RESOLVED FIXED in Firefox 43

Status

()

Core
DOM: Service Workers
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: nsm, Assigned: nsm)

Tracking

Trunk
mozilla43
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 affected, firefox43 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

https://github.com/slightlyoff/ServiceWorker/issues/630
Assignee: nobody → amarchesini
Duplicate of this bug: 1187347
Created attachment 8646977 [details] [diff] [review]
slashes.patch
Attachment #8646977 - Flags: review?(bkelly)

Comment 3

2 years ago
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.
Attachment #8646977 - Flags: review?(bkelly) → review-
Created attachment 8647069 [details] [diff] [review]
slashes.patch
Attachment #8646977 - Attachment is obsolete: true
Attachment #8647069 - Flags: review?(bkelly)

Comment 5

2 years ago
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.
Attachment #8647069 - Flags: review?(bkelly) → review+
> 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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a8f45990368
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
it seems that the dependences are fully landed. We can land this patch too.
Keywords: checkin-needed
Blocks: 1180861
https://hg.mozilla.org/integration/mozilla-inbound/rev/f447f7b8190b
Keywords: checkin-needed
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.
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d72462f193b4
nsm, can you take this and land it with the other fixes?
Flags: needinfo?(nsm.nikhil)
Assignee: amarchesini → nsm.nikhil
Flags: needinfo?(nsm.nikhil)
Status: NEW → ASSIGNED
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/7c5cc8cfca19afaeb4af08c669a78219edea72c4
changeset:  7c5cc8cfca19afaeb4af08c669a78219edea72c4
user:       Andrea Marchesini <amarchesini@mozilla.com>
date:       Wed Aug 19 13:23:58 2015 -0700
description:
Bug 1185640 - serviceworker register() should not accept escaped slashes. r=bkelly
https://hg.mozilla.org/mozilla-central/rev/7c5cc8cfca19
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.