Closed Bug 1184798 Opened 9 years ago Closed 9 years ago

test various types of interceptions of worker script network requests

Categories

(Core :: DOM: Service Workers, defect)

Product:
Core
Component:
DOM: Service Workers
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: bkelly, Assigned: nsm)

References

(
URL
)

Details

Attachments

(2 files)

MozReview Request: Bug 1184798 - Ensure workers loads are treated as non-subresource fetches. r?jdm
40 bytes, text/x-review-board-request
jdm
: review+
Details
MozReview Request: Bug 1184798 - same origin, cors and no-cors load tests. r=bkelly
40 bytes, text/x-review-board-request
bkelly
: review+
Details 
Write tests to validate that we DTRT for this call site in the worker ScriptLoader:

  https://www.google.com/url?q=http%3A%2F%2Fhg.mozilla.org%2Fmozilla-central%2Ffile%2F0b901209064c%2Fdom%2Fworkers%2FScriptLoader.cpp%23l165

The top level worker scripts are considered client requests, so opaque responses should be blocked.

Worker scripts are supposed to same-origin, though, so its a bit unclear if CORS and synthetic responses should be allowed.

For example, we allow an "eval" of a worker script now via data urls, but we downgrade the worker to the null principal so it can do no further network requests.

Do we need to do something similar here?  Synthetic responses are effectively an eval.
Anne, Jonas, can you weigh in here?

Given that we restrict the capabilities of a data URI worker, should we enforce the same restrictions on a synthetic Response?

And should we do the same for a CORS response since it can be easily made into a synthetic response?

I think the absolute safest thing would be to block CORS responses and treat synthetic responses the same as a data URI.
Flags: needinfo?(jonas)
Flags: needinfo?(annevk)
The specification currently blocks CORS for client requests, but that seems wrong.

A data URL has restrictions because it has no origin. We actually want to restrict data URLs all over our codebase.

That's not the case for a synthetic or CORS responses though, they should be fine here.
Flags: needinfo?(annevk)
ServiceWorkers aside, there's not really any strong reason why we're requiring that Dedicated and Shared workers are same-origin. I think the main reason we do is to make it very clear that the worker script is executing in the origin of the page creating the worker.

But security-wise it's basically the same thing to do:

page.html
...
w = new Worker("script.js");
...

script.js
importScript("https://other.website.com/script.js");


Which is totally permitted by the spec. Which makes sense given that that matches how <script src="..."> works.

The only advantage that I can see with the current same-origin restriction for Workers, is that it means that the URL of a worker also matches origin of the worker. But we don't really expose the worker URL anywhere in the DOM currently, so the advantage is quite small.


So from a security point of view it'd actually be quite fine to respond with an opaque response object. Especially if we don't treat it as a redirect but rather as a "act as if the server of the URL that was originally requested responded with this content".

Only issue is that I think that we for cross-origin <script>s and importScripts suppress part of the error messages when they are raised, to avoid exposing the source code of the cross-origin script. So we'd have to do that here too.


That said, there might be consistency or simplicity reasons to not allow opaque responses when loading a worker. I don't know the spec well enough to say.
Actually, it wouldn't be quite the same since the code that sets up the script environment doesn't anticipate an opaque response and therefore wouldn't hide exceptions properly. We shouldn't allow it until we actually change workers (and their specification) to deal with opaque responses (if ever).
(In reply to Anne (:annevk) from comment #2)
> The specification currently blocks CORS for client requests, but that seems
> wrong.
> 
> A data URL has restrictions because it has no origin. We actually want to
> restrict data URLs all over our codebase.
> 
> That's not the case for a synthetic or CORS responses though, they should be
> fine here.

When you grab a response from another URL, and use to satisfy a request, does that act as a redirect? Or does it simply act as if the server had responded with that contents?

If it simply acts as if the server had responded, then wouldn't it make sense to treat all responses that the SW can read the contents of as the the same? I.e. a same-origin response, a synthetic response, a CORS response and a response from a data:-URL would all seem equivalent?
Yes, if we were to allow opaque responses for workers, we'd need to be able to hide exception details. I don't know how easy that would be to do.
Flags: needinfo?(jonas)
In reply to comment #5, you act as if the server had responded with those contents. If the service worker can get contents out of a data URL that's fine, but I was talking about data URL policy in general, when you pass one to e.g., new Worker(), without service workers being involved.

Which reminds me of another point, allowing cross-origin URLs for new Worker() changes the attack surface for sites that use variables for the new Worker() URL. But that whole discussion is somewhat off-topic anyway.
Ok, it sounds like the security concerns aren't there for worker scripts where the service worker can view the contents of the Response.

So for the time being I will just verify we follow the current spec:

 - only basic and default response types are allow (opaque and cors responses are blocked)

If the spec opens up to allow CORS responses for client requests then we can fix the test.

Thanks guys.
I updated the specification earlier today to allow CORS responses: https://github.com/whatwg/fetch/commit/1612905aae06fdb912779b308d71bfc13422833f
This still needs to be done, but I need to go work bug 1093357 right now.  Hopefully someone else can pick this up.
Assignee: bkelly → nobody
Status: ASSIGNED → NEW
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Sorry to be a bearer of bad news but our worker interception is broken. We don't treat workers as 'navigation requests', which means we use the worker's document to determine interception and not the worker script's location. This means that if document at /foo/uncontrolled creates |new Worker("/foo/controlled.js")| it won't get intercepted. Presumably we need to change nsINetworkInterceptController or something. Josh, what would be the way to fix this?

http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on-fetch-request-algorithm step 12 says "if request is a non-subresource request" which workers are (there destination is "worker" or "sharedworker").
Flags: needinfo?(josh)
I thought worker scripts were client requests, not navigations.  But maybe that distinction is a fetch thing and not relevant here.
In fetch spec terms workers are "non-subresources" requests along with documents.
Bug 1184798 - Ensure workers loads are treated as non-subresource fetches. r?jdm

Right now we treat all of these as 'navigations' but I can file a followup to
change the terminology if required.
Attachment #8663071 - Flags: review?(josh)
Comment on attachment 8663072 [details]
MozReview Request: Bug 1184798 - same origin, cors and no-cors load tests. r=bkelly

https://reviewboard.mozilla.org/r/19697/#review17733

It would be nice to factor out the common parts of the test cases to reduce duplication, but thats a nit.  Please fix the issue in the service worker preventing you from actually passing an opaque response, though.

::: testing/web-platform/mozilla/tests/service-workers/service-worker/resources/dummy-worker-interceptor.js:16
(Diff revision 1)
> +    event.respondWith(fetch(url));

This will fail in the no-cors case before you even get an opaque response because the fetch() is using "cors" mode.  I think you need to set the mode of the fetch() based on what script you are trying to load.
Attachment #8663072 - Flags: review?(bkelly) → review+
Attachment #8663071 - Flags: review?(josh)
Comment on attachment 8663071 [details]
MozReview Request: Bug 1184798 - Ensure workers loads are treated as non-subresource fetches. r?jdm

https://reviewboard.mozilla.org/r/19695/#review17729

I don't feel like this new code belongs in the HTTP channel code.

::: netwerk/protocol/http/HttpBaseChannel.cpp:2159
(Diff revision 1)
> -  return mLoadFlags & LOAD_DOCUMENT_URI;
> +  return (mLoadFlags & LOAD_DOCUMENT_URI) || IsWorkerLoad();

This feels like a weird place for this. I would rather add this check to nsDocShell, and move IsWorkerLoad elsewhere.

::: netwerk/protocol/http/HttpBaseChannel.cpp:2170
(Diff revision 1)
> +    mURI->GetSpec(url);

?
Comment on attachment 8663071 [details]
MozReview Request: Bug 1184798 - Ensure workers loads are treated as non-subresource fetches. r?jdm

Bug 1184798 - Ensure workers loads are treated as non-subresource fetches. r?jdm

Right now we treat all of these as 'navigations' but I can file a followup to
change the terminology if required.
Attachment #8663071 - Flags: review?(josh)
Comment on attachment 8663072 [details]
MozReview Request: Bug 1184798 - same origin, cors and no-cors load tests. r=bkelly

Bug 1184798 - same origin, cors and no-cors load tests. r=bkelly
Attachment #8663072 - Attachment description: MozReview Request: Bug 1184798 - same origin, cors and no-cors load tests. r?bkelly → MozReview Request: Bug 1184798 - same origin, cors and no-cors load tests. r=bkelly
Comment on attachment 8663071 [details]
MozReview Request: Bug 1184798 - Ensure workers loads are treated as non-subresource fetches. r?jdm

https://reviewboard.mozilla.org/r/19695/#review17871

::: docshell/base/nsDocShell.cpp:13920
(Diff revision 2)
> +                               nsContentPolicyType aLoadContentType)

I think adding this as an attribute to nsIInterceptedChannel makes more sense than passing it to ChannelIntercepted and subsequent users. Alternatively, we can even use `aChannel->GetChannel` and extract the load type ourselves without changing the interface.

::: dom/workers/ServiceWorkerManager.cpp:3896
(Diff revision 2)
> -  if (!isNavigation) {
> +  if (!isNavigation && !nsContentUtils::IsWorkerLoad(aLoadType)) {

We could just accept a new boolean argument here rather than calling GetIsNavigation and IsWorkerLoad again.

::: netwerk/protocol/http/HttpBaseChannel.h:261
(Diff revision 2)
> +    bool IsWorkerLoad() const;

This can be removed.
Attachment #8663071 - Flags: review?(josh)
Comment on attachment 8663071 [details]
MozReview Request: Bug 1184798 - Ensure workers loads are treated as non-subresource fetches. r?jdm

Bug 1184798 - Ensure workers loads are treated as non-subresource fetches. r?jdm

Right now we treat all of these as 'navigations' but I can file a followup to
change the terminology if required.
Attachment #8663071 - Flags: review?(josh)
Comment on attachment 8663072 [details]
MozReview Request: Bug 1184798 - same origin, cors and no-cors load tests. r=bkelly

Bug 1184798 - same origin, cors and no-cors load tests. r=bkelly
Comment on attachment 8663071 [details]
MozReview Request: Bug 1184798 - Ensure workers loads are treated as non-subresource fetches. r?jdm

https://reviewboard.mozilla.org/r/19695/#review17887

::: netwerk/protocol/http/InterceptedChannel.h:59
(Diff revision 3)
> +  NS_IMETHOD GetInternalContentPolicyType(nsContentPolicyType *aInternalContentPolicyType) override;

I believe you can leave this out.

::: netwerk/protocol/http/InterceptedChannel.cpp:78
(Diff revision 3)
> +InterceptedChannelBase::GetInternalContentPolicyType(nsContentPolicyType* aPolicyType)

I don't think this implementation is necessary?
Attachment #8663071 - Flags: review?(josh) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: