Integrate fetch with subresource integrity

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: bkelly, Assigned: tt)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [tw-dom] btpp-active)

Attachments

(7 attachments, 86 obsolete attachments)

9.04 KB, patch
Details | Diff | Splinter Review
9.49 KB, patch
Details | Diff | Splinter Review
27.52 KB, patch
Details | Diff | Splinter Review
41.07 KB, patch
Details | Diff | Splinter Review
14.90 KB, patch
Details | Diff | Splinter Review
10.53 KB, patch
Details | Diff | Splinter Review
30.79 KB, patch
Details | Diff | Splinter Review
The fetch spec is now better integrated with SRI:

  https://github.com/whatwg/fetch/issues/85

Our SRI implementation is still in progress in bug 992096, but we should make sure its integrated properly once its done.
I've just documented how the SRI code hooks into the script and style elements: https://wiki.mozilla.org/Security/Subresource_Integrity#Engineering
Whiteboard: [tw-dom]
Assignee

Comment 2

3 years ago
I would like to try on this bug.
Assignee: nobody → ttung
Tom, what's the timeline for this bug?
Flags: needinfo?(ttung)
Assignee

Comment 4

3 years ago
Hi Frederik,

Currently, I'm working on another bug and just investigating the background of this bug. I will focus on this bug recently. Does this bug blocks something important?
Flags: needinfo?(ttung)
Nah, just curious.
Assignee

Comment 6

3 years ago
Posted patch sri_v1.patch (obsolete) — Splinter Review
WIP patch
current progress: 
1. add integrity metadata attribute in Request & InternalRequest
2. part of Step 15 in "Main fetch" (not in this patch since it haven't been completed yet) 
Next Step:
1. Modify type of mIntegrity from "nsString" to "SRIMetadata" in |InternalRequest|
2. Finish the Step 15 in "Main fetch"
Assignee

Comment 7

3 years ago
Current progress: 
1. It seems that we don't to store Integrity via SRIMetadata in InternalRequest, so keeping store it via nsString.
2. Get stuck at the final step of "3.3.5 Does response match metadataList?" [1]

At step 15 in Main-Fetch, we need to check whether does response match request's integrity metadata or not. To do this, we need to implement [1] and steps among it. However, I cannot find representation data for response to match request's integrity metadata [2]. It seems that we don't have it in fetch api. Could you provide any hint for this, Ben? Thanks!

[1] https://w3c.github.io/webappsec-subresource-integrity/#does-response-match-metadatalist
[2] https://w3c.github.io/webappsec-subresource-integrity/#apply-algorithm-to-response
Flags: needinfo?(bkelly)
Whiteboard: [tw-dom] → [tw-dom] btpp-active
(In reply to Tom Tung from comment #7)
> Current progress: 
> 1. It seems that we don't to store Integrity via SRIMetadata in
> InternalRequest, so keeping store it via nsString.

The SRIMetadata constructor takes a string and converts it to the right structure:

https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/dom/security/SRIMetadata.cpp#27-72

> 2. Get stuck at the final step of "3.3.5 Does response match metadataList?"
> [1]
> 
> At step 15 in Main-Fetch, we need to check whether does response match
> request's integrity metadata or not. To do this, we need to implement [1]
> and steps among it.

That algorithm is already implemented here in SRICheck::Verify():

https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/dom/security/SRICheck.cpp#340-385
Flags: needinfo?(bkelly)
Assignee

Comment 9

3 years ago
(In reply to François Marier [:francois] from comment #8)
Thanks, I'll try to use it!
Assignee

Comment 10

3 years ago
Thanks for hints from Francois, but I still have somethings need to be figured out. I address them with my solutions for them below.

1. In Main fetch step 15-1, "Wait for response's body". In my native think, I need to implement it in funtion "OnStreamComplete()". However, we don't have it in FetchDriver, so I'll try to implement one in FetchDriver.cpp.

2. Although function SRICheck::Verify() can verify response, I still need to update the hash value as [1]. Thus, I'll try to call VerifyIntegrity() for verify the response. The problem is I don't know what should be put into this function, such as "aLoader", "aString" and "aCORSMode". "aLoader" could be a argument from funtion OnStreamComplete(). I guess "aString" could be the stream value from response's body and "aCORSMode" could be get like [3]. I'll check whether it's correct or not.

[1] https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/dom/security/SRICheck.cpp#208
[2] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#1895
[3] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#1890
Assignee

Comment 11

3 years ago
Posted patch WIP patch -v2 (obsolete) — Splinter Review
Sorry for late reply, I was working on another bug and study another api. 

Thanks for François! After face-to-face teaching, I figure out more about this bug and how should SRI works with fetch. This patch have almost been done. The only thing left is setting response and internalResponse to a network error. I believe that I could finish it soon.
Attachment #8739387 - Attachment is obsolete: true
Assignee

Comment 12

3 years ago
Posted patch WIP patch - v3 (obsolete) — Splinter Review
Current status:
Can pass all the test in web-platform/tests/fetch/api/basic/integrity.html but fail in integrity-worker.html. The reason is worker don't set a document in FetchDriver because of bug 1224855. However, SRICheck::IntegrityMetadata() need document to be set.
Attachment #8744268 - Attachment is obsolete: true
As far as I can tell that's just needed for a couple ReportToConsole() calls in SRICheck::IntegrityMetadata(), right?
Assignee

Comment 14

3 years ago
Yes, it looks like that. So do we need documnet object in worker or just create another SRICheck function without ReportToConsole()? I'm curious about whether we need ReportToConsole() for worker?
Debugging would be hard if there's no devtools reporting for stuff happening in a worker.
But maybe this could be done as a follow-up?
Assignee

Comment 16

3 years ago
Do you means I could write a patch(write another SRICheck::IntegrityMetadata() without calling ReportToConsole()) and file a follow-up bug for finding another way to have devtools report?
You could probably change SRICheck::Metadata() to take an nsIConsoleReportCollector and then flush it to the right place.  If the FetchDriver has an mDocument, it can flush it there.  If it doesn't then it would need to get it to the right worker-related documents.  Thats a bit complicated.

For a service worker we actually have a heuristic for finding documents defined in ServiceWorkerManager::ReportToAllClients().  We could create a new version of that method that takes an nsIConsoleReportCollector instead of the message directly.  It could then flush to each client document.

Dedicated workers might have their own document associated with them.  Shared workers also have a number of shared documents.  We could make a method on WorkerPrivate that takes an nsIConsoleReportCollector and flushes the reports to the right place.

It would be nice to do that here, but it could also be a follow-up.  If we allow a nullptr document to be passed to ReportToConsole() it will just send the message to the browser console instead of the window's web console.
Assignee

Comment 18

3 years ago
Sure, I'll try to use nsIConsoleReportCollector here. Thanks!
Assignee

Comment 19

3 years ago
(In reply to Tom Tung from comment #12)
> in FetchDriver because of bug 1224855. However,
It's bug 1224865. Sorry for that.
Assignee

Comment 20

3 years ago
Hi Ben,

  I divided this bug into two part and this patch is for part one. Part one will be responsible for integrating SRI with Fetch API and part two will be responsible for handling Console message. I'm working on part two. Hopefully, I could complete part two soon, but I would like to ask to review for part one first. Could you help me to review this patch, Ben?

  In this patch, I add an attribute "integrity" into Request/InternalRequest and implement step 17 in Main-fetch. I init the Integrity in OnStartRequest(), copy the stream among OnDataAvilable() and verify the integrity in OnStopRequest(). 
  Secondly, I move the code [1] from HttpFetch() into OnStopRequest(). The main reason is that we should process response after checking integrity and integrity should be verified after OnStopRequest(). 
  Then, in order to allow function IsEligible() SRICheck to check whether is same-origin [2], I add SetDocumentURI() in FetchDriver and SRICheck. 
  Finally, after the changes, we should pass relevant tests in wpt, so I remove the code in testing/web-platform/meta/fetch/api/basic/integrity-worker.html.ini & testing/web-platform/meta/fetch/api/basic/integrity.html.ini

[1] https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp?from=fetchdriver.cpp#408-414
[2] https://dxr.mozilla.org/mozilla-central/source/dom/security/SRICheck.cpp#77
Attachment #8748470 - Attachment is obsolete: true
Attachment #8750215 - Flags: review?(bkelly)
Comment on attachment 8750215 [details] [diff] [review]
Bug 1243791 - v1 - P1 Integrate fetch with SRI - Fetch API & SRICheck changes.

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

I'm sorry, but I don't think we can delay resolving the Response for all fetch() calls to OnStopRequest().  I also would like to change how we are passing data down to IsEligible.

I really think we should do the nsIConsoleLogCollector changes in this bug.  It would make it clearer where the document is actually required for other things vs just reporting.

Finally, I think this should ultimately get review from Christoph or Jonas for the SRI changes we are making.

Thanks for working on this!

::: dom/fetch/Fetch.cpp
@@ +137,5 @@
>      MOZ_ASSERT(loadGroup);
>      RefPtr<FetchDriver> fetch = new FetchDriver(mRequest, principal, loadGroup);
> +    nsIDocument* doc = proxy->GetWorkerPrivate()->GetDocument();
> +    if (doc) {
> +      fetch->SetDocumentURI(doc->GetDocumentURI());

I don't think this is correct.  Not all workers have a document.

Also, for the purposes of the SRI IsEligible() check, I think you could just use the worker script.

Ultimately, though, I think what we should really be doing is passing the LoadTainting value down to IsEligible():

  https://dxr.mozilla.org/mozilla-central/source/netwerk/base/LoadTainting.h#34

You can get this value from here:

  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/InternalResponse.cpp#122

If the tainting is Basic or CORS, then IsEligible should return NS_OK.  If its Opaque, then IsEligible() should return NS_ERROR_SRI_NOT_ELIGIBLE.

Again we have the logging issue if IsEligible() fails.

You probably will need to make a different version of SRICheckDataVerifier() that takes the LoadTainting as well in order to pass the value down.

::: dom/fetch/FetchDriver.cpp
@@ +696,5 @@
>    } else {
>      MOZ_ASSERT(mResponse);
>      MOZ_ASSERT(!mResponse->IsError());
>  
> +    // From "Main Fetch" step 17: SRI-part3.

See my note below, but this needs to be done earlier than OnStopRequest().

@@ +729,5 @@
>  
>    if (mObserver) {
> +    //From "Main Fetch" step 23: Process response.
> +    MOZ_ASSERT(mResponse);
> +    mObserver->OnResponseAvailable(mResponse);

So, this is implementing step 17.1 from Main Fetch:

  "Wait for response body"

But its doing it unconditionally.  This will block resolving the Response until the body is fully present even for requests not using SRI.

I don't think we should include this perf hit here.  We need to implement the Main Fetch step 17 conditional check:

  "If response is not a network error and request's integrity metadata is not the empty string, run these substeps:"

So we resolve the Response in BeginAndGetFilteredResponse() normally, but wait to resolve it until OnStopRequest() if SRI is in play.
Attachment #8750215 - Flags: review?(bkelly) → review-
Assignee

Comment 22

3 years ago
Thanks for the feedback! I'm sorry about wrong direction about patch. I'll make it better next time. I have some questions for comments and I leave them below inline. Could you help me to figure them out?

(In reply to Ben Kelly [:bkelly] from comment #21)
> I'm sorry, but I don't think we can delay resolving the Response for all
> fetch() calls to OnStopRequest().  I also would like to change how we are
> passing data down to IsEligible.
> 
> I really think we should do the nsIConsoleLogCollector changes in this bug. 
> It would make it clearer where the document is actually required for other
> things vs just reporting.

Yeah, I plan to do it in another patch(P2).

> Finally, I think this should ultimately get review from Christoph or Jonas
> for the SRI changes we are making.

Sure, thanks for reminder!
 
> ::: dom/fetch/Fetch.cpp
> @@ +137,5 @@
> >      MOZ_ASSERT(loadGroup);
> >      RefPtr<FetchDriver> fetch = new FetchDriver(mRequest, principal, loadGroup);
> > +    nsIDocument* doc = proxy->GetWorkerPrivate()->GetDocument();
> > +    if (doc) {
> > +      fetch->SetDocumentURI(doc->GetDocumentURI());
> 
> I don't think this is correct.  Not all workers have a document.
> 
> Also, for the purposes of the SRI IsEligible() check, I think you could just
> use the worker script.
> 
> Ultimately, though, I think what we should really be doing is passing the
> LoadTainting value down to IsEligible():
> https://dxr.mozilla.org/mozilla-central/source/netwerk/base/LoadTainting.h#34
> 
> You can get this value from here:
>  
> https://dxr.mozilla.org/mozilla-central/source/dom/fetch/InternalResponse.
> cpp#122

Could you explain more about why we should use LoadTainting rather than ResponseType since SRI spec, step 2 [1] links to response type.

[1] https://www.w3.org/TR/SRI/#is-response-eligible-for-integrity-validation

> 
> If the tainting is Basic or CORS, then IsEligible should return NS_OK.  If
> its Opaque, then IsEligible() should return NS_ERROR_SRI_NOT_ELIGIBLE.
> 
> Again we have the logging issue if IsEligible() fails.
> 
> You probably will need to make a different version of SRICheckDataVerifier()
> that takes the LoadTainting as well in order to pass the value down.

I do the mapping then call verify because I asked François before. He suggested that I could map from ResponseType::Cors to CORS_ANONYMOUS and translate others to CORS_NONE then call SRIDataVerifier::Verify(). 

Did you mean I should translate these enum(CORS, DEFAULT, ... etc) into CORS_xxx in SRIDataVerifier::Verify() instead of in FetchDriver::OnStopRequest()?

> @@ +729,5 @@
> >  
> >    if (mObserver) {
> > +    //From "Main Fetch" step 23: Process response.
> > +    MOZ_ASSERT(mResponse);
> > +    mObserver->OnResponseAvailable(mResponse);
> 
> So, this is implementing step 17.1 from Main Fetch:
> 
>   "Wait for response body"
> 
> But its doing it unconditionally.  This will block resolving the Response
> until the body is fully present even for requests not using SRI.
> 
> I don't think we should include this perf hit here.  We need to implement
> the Main Fetch step 17 conditional check:
> 
>   "If response is not a network error and request's integrity metadata is
> not the empty string, run these substeps:"
> 
> So we resolve the Response in BeginAndGetFilteredResponse() normally, but
> wait to resolve it until OnStopRequest() if SRI is in play.

Sure. Just want to make sure that I understand. I need to add if condition at [1] to check if response is not a network error and request's integrity metadata is not the empty string. If it's true then do it in OnStopRequest(), otherwise process filteredResponse as usual, right? 

[1] https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#408-413
(In reply to Tom Tung from comment #22)
> Could you explain more about why we should use LoadTainting rather than
> ResponseType since SRI spec, step 2 [1] links to response type.
> 
> [1] https://www.w3.org/TR/SRI/#is-response-eligible-for-integrity-validation

I'm saying to use LoadTainting instead of calling CheckSameOriginURI() in IsEligible().  So you don't need the documentURI.  The LoadTainting is already the result of performing same origin checks, so it provides the same information.

I prefer LoadTainting over ResponseType because it makes the SRICheck API less specific to Fetch.  LoadTainting is defined in our netwerk stack regardless of fetch().  Any nsIChannel created with AsyncOpen2() will have a valid LoadTainting value that can be used with the API.

The LoadTainting also combines the information in the documentURI and CORSMode into a single enum:

1) LoadTainting::Basic == CORS_NONE with same-origin documentURI
2) LoadTainting::CORS == CORS_ANONYMOUS or CORS_USE_CREDENTIALS with cross-origin documentURI
3) LoadTainting::Opaque == CORS_NONE with cross-origin documentURI

We basically just need to prevent SRI when we have LoadTainting::Opaque.

> Sure. Just want to make sure that I understand. I need to add if condition
> at [1] to check if response is not a network error and request's integrity
> metadata is not the empty string. If it's true then do it in
> OnStopRequest(), otherwise process filteredResponse as usual, right? 
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#408-
> 413

Correct.  You only want to delay resolving the Response if SRI is actually going to be checked.
(In reply to Ben Kelly [:bkelly] from comment #21)
> Finally, I think this should ultimately get review from Christoph or Jonas
> for the SRI changes we are making.

I'm happy to review the SRI changes once they're ready.
Assignee

Comment 25

3 years ago
I'm trying on change arguments for IsEligible from CORSMode to LoadTainting. After I changed that, I got failed at test_style_crossdomain.html, test "valid non-CORS sha256 hash in a data: URL" [1].  I expected that I can get LoadTainting::opaque here but I can only get LoadTainting::Basic. Still try to figure out why...


[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/test/sri/iframe_style_crossdomain.html#78
(In reply to Tom Tung [:tt] from comment #25)
> I'm trying on change arguments for IsEligible from CORSMode to LoadTainting.
> After I changed that, I got failed at test_style_crossdomain.html, test
> "valid non-CORS sha256 hash in a data: URL" [1].  I expected that I can get
> LoadTainting::opaque here but I can only get LoadTainting::Basic. Still try
> to figure out why...
> 
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/security/test/sri/
> iframe_style_crossdomain.html#78

I think that test is wrong.  Our stylesheet loaded is supposed to inherit the origin:

https://dxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp#1617

I think this is a bug in our existing SRI implementation.

Francois, what do you think?
Flags: needinfo?(francois)
Sorry, I was trying to say the origin should be inherited when loading a stylesheet with a data URI.
Also note the SRI spec says to check the result tainting and not just compare the URI:

https://w3c.github.io/webappsec-subresource-integrity/#is-response-eligible-for-integrity-validation

So I'm fairly confident that test is wrong.
Assignee

Comment 29

3 years ago
Thanks for helping, Ben. 
That test expected to get false because it is not eligible. I thought the test should be correct but something is wrong due to the fourth note "Note Since the response type for data URLs will always be “opaque” for script and link elements ..." [1]. Thus, I expected to get LoadTainting::Opaque since response type is Opaque rather than LoadTainting::Basic. 

[1] https://w3c.github.io/webappsec-subresource-integrity/#is-response-eligible-for-integrity-validation
Hmm, interesting.  I think that note overstates things a bit.  AFAIK the stylesheet loading is not specified here.  In gecko we do inherit the origin and treat them as same-origin.

I wrote a spec issue against SRI:

https://github.com/w3c/webappsec-subresource-integrity/issues/35

In any case, whether stylesheet treats data URL as same-origin or not is a separate issue.  SRI should simply work based on the tainting value.  If we change stylesheet not to inherit the origin for data URL in the future then it will start getting blocked automatically.

I think we should change the two data URL test cases to pass.  We should then add another test case for "http://example.com/tests/dom/security/test/sri/style1.css" that does not use CORS to show that an opaque response is blocked.

Does that sound good?
Assignee

Comment 31

3 years ago
Ok, I see. I'll try to do that. Thanks!
(In reply to Ben Kelly [:bkelly] from comment #30)
> I think we should change the two data URL test cases to pass.  We should
> then add another test case for
> "http://example.com/tests/dom/security/test/sri/style1.css" that does not
> use CORS to show that an opaque response is blocked.
> 
> Does that sound good?

Pending resolution of the spec issue you raised, I'd say that's a reasonable plan for now.
Flags: needinfo?(francois)
Assignee

Comment 33

3 years ago
WIP patch for SRI changes. Addressed comments expect changing method for ReportToConsole.
Assignee

Comment 34

3 years ago
This is also WIP patch and it's for change about SRI test.
Assignee

Comment 35

3 years ago
WIP patch for Fetch API modify. 

Sorry for the late reply, I was working on another bug and studying storage API. I'll try to solve this bug as soon as possible.
Attachment #8750215 - Attachment is obsolete: true
Assignee

Comment 36

3 years ago
WIP patch for wpt tests enable
Assignee

Comment 37

3 years ago
WIP patch. Add a function FlushReportToAllClients() in order to provide a method to report to all SW's clients.
Assignee

Comment 38

3 years ago
WIP patch. The changes in this patch is mainly for report to worker's console. 

Next Step: Provide a mechanism for workers(dedicated and shared) Fetch API to pass document to SRICheck and servicewroker to pass its scope.
Attachment #8754762 - Attachment is obsolete: true
(In reply to Tom Tung [:tt] from comment #39)
> Sorry for delay these day. I have few questions about how to pass document
> object to SRICheck API in worker case. I address them below inline.
> Could you give me some hints about these, Ben? Thanks!

So what I was proposing was that code that wants to use SRI do something like:

1) Create a ConsoleReportCollector object.
2) Pass the collector to the SRI API.
3) After returning from the SRI API, call ConsoleReportCollector::FlushConsoleReports(document).

The document in (3) would depend on who is using SRI.

- In a window you can just use the windows document.
- If the WorkerPrivate or its parent change has a document, then you can use that.
- On shared worker you would need to call it once for each document attached to the SharedWorker.
- For a service worker you would need to make a new version of ServiceWorkerManager::ReportToAllClients() that takes an nsIConsoleReportCollector and flushes repeatedly to each document found.

For the last two here, you need to make a way for the collector to "flush" without losing its list of reports.  Maybe an optional second argument to FlushConsoleReport(document, forgetReports=true).

> 1. In order to flush report to worker's document, I should pass dedicated
> worker's "loading" document(which is from WorkerPrivate), right? Or is there
> any other ways to get directly dedicated worker's document from
> main-thread(FetchDriver)?
> 2. If right, for dedicated worker case, should I pass a "loading" document
> from FetchRequest() into FetchDriver at [1]? I ask this question because bug
> bug 1224865 remove it.

You probably want to add a virtual method to FetchDriverObserver like GetReportCollector().  The FetchDriverObserver implementation can then be responsible for flushing any reports when it sees the FetchDriver complete.  The WorkerFetchResolver implementation of FetchDriverObserver should have access to the WorkerPrivate in question.

Sorry its still a bit hand wavy.  Does that make sense?
Flags: needinfo?(bkelly)
Assignee

Comment 40

3 years ago
Sorry for delay these day. I have few questions about how to pass document object to SRICheck API in worker case. I address them below inline.
Could you give me some hints about these, Ben? Thanks!

1. In order to flush report to worker's document, I should pass dedicated worker's "loading" document(which is from WorkerPrivate), right? Or is there any other ways to get directly dedicated worker's document from main-thread(FetchDriver)?
2. If right, for dedicated worker case, should I pass a "loading" document from FetchRequest() into FetchDriver at [1]? I ask this question because bug bug 1224865 remove it.
[1] https://dxr.mozilla.org/mozilla-central/source/dom/fetch/Fetch.cpp?from=fetch.cpp#139
Flags: needinfo?(bkelly)
Assignee

Comment 41

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #40)
> Sorry its still a bit hand wavy.  Does that make sense?
Yes, it much clearer for me. Thanks for teaching me that!
Assignee

Comment 45

3 years ago
Attachment #8754764 - Attachment is obsolete: true
Assignee

Comment 48

3 years ago
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e3e4d0c558e&selectedJob=21670534

It seems that it still have few problems when running try, but I could pass tests in my environment (mac debug mode). I'll send them to review once I fix them.
Assignee

Comment 49

3 years ago
Comment on attachment 8757860 [details] [diff] [review]
Bug-1187335 - P4 - Integrity fetch with SRI & add nsIConsoleReportCollector to show console report - fetch API changed.

Hi Ben,

In this patch, I find a problem when I implement nsIConsoleReportCollector as your comment#40. I could not pass a collector to FetchDriverObserver since the collector is reference to nsIChannel which cannot run on different thread(main thread and worker thread). Besides, I cannot pass nsIDocument to FetchDriverObserver either for same reason. Thus, I pass worker's document to FetchDriver and flush it at OnStopRequest() after finishing SRICheck::Verify().

However, when I run test integrity-worker.html [1], I can only find the logs in browser console rather than web console. It seems that I flush report in wrong timing. Could you give some hint to solve that? Thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/fetch/api/basic/integrity-worker.html?from=integrity-worker.html
Attachment #8757860 - Flags: feedback?(bkelly)
Assignee

Comment 50

3 years ago
(In reply to Tom Tung [:tt] from comment #48)
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=1e3e4d0c558e&selectedJob=21670534

The failed tests happened because I call SRIDataVerifier::Update() on FetchDriver::OnDataAvailable() which is not called on the main thread. However, I call SRIDataVerifier::Verify() on FetchDriver::OnStopRequest which is called on the main thread and 'nsCryptoHash' is not threadsafe. Thus, I hit the MOZ_CRASH.

I'll try to implement a Runnable to avoid they will be called on different threads.

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=21673090&repo=try#L17146
The other option is to just not do the retarget if SRI is play:

https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp?from=FetchDriver.cpp#604

It should all work without retargeting to the stream transport service since some channel configurations don't support it anyway.
Assignee

Comment 52

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #51)
> The other option is to just not do the retarget if SRI is play:
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.
> cpp?from=FetchDriver.cpp#604
> 
> It should all work without retargeting to the stream transport service since
> some channel configurations don't support it anyway.

This approach is much better than create a runnable to dispatch another thread. I'll implement that! Thanks!
Comment on attachment 8757860 [details] [diff] [review]
Bug-1187335 - P4 - Integrity fetch with SRI & add nsIConsoleReportCollector to show console report - fetch API changed.

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

This is moving in the right direction, but I think it would be preferable to remove conditional logic where possible.  So I'd rather we just always used a nsIConsoleReportCollector.  The SRI code should just be document agnostic.  Where the collector is flushed to should totally be determined with the FetchObserver.

::: dom/fetch/FetchDriver.cpp
@@ +602,5 @@
> +  mRequest->GetIntegrity(integrity);
> +  if (mResponse->Type() != ResponseType::Error && !integrity.IsEmpty() &&
> +      mSRIMetadata.IsEmpty()) {
> +    if (!mDocument) {
> +      nsCOMPtr<nsIConsoleReportCollector> collector = do_QueryInterface(channel);

I was more thinking you would do:

  nsCOMPtr<nsIConsoleReportCollector> collector = new ConsoleReportCollector();

From here:

  https://dxr.mozilla.org/mozilla-central/source/dom/base/ConsoleReportCollector.h#16

And I think the observer should probably own the collector.  So here you would call mObserver->GetCollector() and the collector would be responsible for creating/returning a collector.  It can then decide to flush when it is convenient for it (probably in OnResponseEnd).

@@ +604,5 @@
> +      mSRIMetadata.IsEmpty()) {
> +    if (!mDocument) {
> +      nsCOMPtr<nsIConsoleReportCollector> collector = do_QueryInterface(channel);
> +      MOZ_ASSERT(collector);
> +      mSRIMetadata.SetReporter(collector);

I don't see the changes to SRIMetadata in this patch.

@@ +701,5 @@
> +    if (mResponse->Type() != ResponseType::Error && !integrity.IsEmpty()) {
> +      MOZ_ASSERT(mSRIDataVerifier);
> +
> +      nsCOMPtr<nsIChannel> channel = do_QueryInterface(aRequest);
> +      nsresult rv = mSRIDataVerifier->Verify(mSRIMetadata, channel, mDocument);

I was thinking we would pass the collector explicitly to SRI instead of a document.  Just make the SRI code document-agnostic.

@@ +705,5 @@
> +      nsresult rv = mSRIDataVerifier->Verify(mSRIMetadata, channel, mDocument);
> +
> +      // Only worker need to flush console report.
> +      if (mObserver && !mDocument) {
> +        mObserver->FlushConsoleReport(channel, mWorkerDocument);

I think it would be simpler to just always flush through the reporter whether you are main thread or worker thread.
Attachment #8757860 - Flags: feedback?(bkelly) → feedback-
Assignee

Comment 54

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #53)

Thanks for feedback! 

> Comment on attachment 8757860 [details] [diff] [review]
> Bug-1187335 - P4 - Integrity fetch with SRI & add nsIConsoleReportCollector
> to show console report - fetch API changed.
> This is moving in the right direction, but I think it would be preferable to
> remove conditional logic where possible.  So I'd rather we just always used
> a nsIConsoleReportCollector.  The SRI code should just be document agnostic.
> Where the collector is flushed to should totally be determined with the
> FetchObserver.

Does "SRI code be document agnostic" means removing nsIDocument argument for all the function in SRI?
If it is, should I file follow-up bug for that since it need to few changes in Loader.cpp and nsScriptLoader.cpp (find a place to flush the report)?

> ::: dom/fetch/FetchDriver.cpp
> @@ +602,5 @@
> > +      nsCOMPtr<nsIConsoleReportCollector> collector = do_QueryInterface(channel);
> I was more thinking you would do:
> 
>   nsCOMPtr<nsIConsoleReportCollector> collector = new
> ConsoleReportCollector();
> 
> From here:
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/base/
> ConsoleReportCollector.h#16 


> And I think the observer should probably own the collector.  So here you
> would call mObserver->GetCollector() and the collector would be responsible
> for creating/returning a collector.  It can then decide to flush when it is
> convenient for it (probably in OnResponseEnd).
> 
> @@ +604,5 @@
> > +      mSRIMetadata.IsEmpty()) {
> > +    if (!mDocument) {
> > +      nsCOMPtr<nsIConsoleReportCollector> collector = do_QueryInterface(channel);
> > +      MOZ_ASSERT(collector);
> > +      mSRIMetadata.SetReporter(collector);
> 
> I don't see the changes to SRIMetadata in this patch.

Oh, I do them in P2. Sorry for that, but I wants to check whether I do the right way for flushing report to worker first. I would like to send review request for them after I complete that.

> 
> @@ +701,5 @@
> > +    if (mResponse->Type() != ResponseType::Error && !integrity.IsEmpty()) {
> > +      MOZ_ASSERT(mSRIDataVerifier);
> > +
> > +      nsCOMPtr<nsIChannel> channel = do_QueryInterface(aRequest);
> > +      nsresult rv = mSRIDataVerifier->Verify(mSRIMetadata, channel, mDocument);
> 
> I was thinking we would pass the collector explicitly to SRI instead of a
> document.  Just make the SRI code document-agnostic.

The reason is that I want to use nsContentUtils::ReportToConsole() if having a nsIDocument. Actually I'm not sure about different between using nsContentUtils::ReportToConsole() directly and calling AddConsoleReport() then FlushConsoleReports() if we can get a nsIDocument object. Thus, I call AddConsoleReport() only if we cannot get a nsIDocument object. Besides, it makes less changes for Loader and ScriptLoader. :P

Could you tell me more about different between them when we already have a nsDocument? Thanks!
(In reply to Tom Tung [:tt] from comment #54)
> > Comment on attachment 8757860 [details] [diff] [review]
> > Bug-1187335 - P4 - Integrity fetch with SRI & add nsIConsoleReportCollector
> > to show console report - fetch API changed.
> > This is moving in the right direction, but I think it would be preferable to
> > remove conditional logic where possible.  So I'd rather we just always used
> > a nsIConsoleReportCollector.  The SRI code should just be document agnostic.
> > Where the collector is flushed to should totally be determined with the
> > FetchObserver.
> 
> Does "SRI code be document agnostic" means removing nsIDocument argument for
> all the function in SRI?
> If it is, should I file follow-up bug for that since it need to few changes
> in Loader.cpp and nsScriptLoader.cpp (find a place to flush the report)?

I mean the main methods with significant code in them in SRI should not reference nsIDocument if we can just use nsIConsoleReportCollector instead.

There are two options to handle Loader.cpp and nsScriptLoader.cpp:

1) Make them create a ConsoleReportCollector and flush to their document manually.
2) Make a wrapper method in SRI that does this, but leaves the main SRI algorithms document-agnostic.

> > @@ +701,5 @@
> > > +    if (mResponse->Type() != ResponseType::Error && !integrity.IsEmpty()) {
> > > +      MOZ_ASSERT(mSRIDataVerifier);
> > > +
> > > +      nsCOMPtr<nsIChannel> channel = do_QueryInterface(aRequest);
> > > +      nsresult rv = mSRIDataVerifier->Verify(mSRIMetadata, channel, mDocument);
> > 
> > I was thinking we would pass the collector explicitly to SRI instead of a
> > document.  Just make the SRI code document-agnostic.
> 
> The reason is that I want to use nsContentUtils::ReportToConsole() if having
> a nsIDocument. Actually I'm not sure about different between using
> nsContentUtils::ReportToConsole() directly and calling AddConsoleReport()
> then FlushConsoleReports() if we can get a nsIDocument object. Thus, I call
> AddConsoleReport() only if we cannot get a nsIDocument object. Besides, it
> makes less changes for Loader and ScriptLoader. :P
> 
> Could you tell me more about different between them when we already have a
> nsDocument? Thanks!

Yea, I just think it makes for a lot of special case logic that makes it hard to follow when the document is used or not.  I really think it would be easier to maintain if we always do the same thing (nsIConsoleReportCollector::AddConsoleReport()) and then flush to the document after the SRI call.
Assignee

Comment 56

3 years ago
Hi Ben, 

  Sorry for the really late reply, I was stuck with cannot find the debug messages in web console. :( 
  I find the reason causing this problem finally. The reason is that I didn't pass SourceFileURI for AddConsoleReport(...) [1] and the pending reports are ignore while calling FlushConsoleReposrts(...) [2]. I did this becuase a SourceFileURI is not provide in SRI API [3], and ReportToConsole(...) can be called when there is no SourceFileURI object. Therefore I pass a EmptyCString() for no SourceFileURI as calling AddConsoleReport. 
  Right now, I just comment [2] to make the debugging reports flush to console. Is there anything wrong for passing a EmptyCString() for SourceFileURI? Could you help me to fix that? Besides, would you mind taking a look at the Console reporting mechanism that I implement? Thanks!

I create this patch for modifying the mechanism to report to console in SRI API and corresponding caller. 
In this patch: 1. Add a function for reporting to console in SRI API.
               2. Using LoadTainting to decide whehter is CORS or not.
               3. Removing passing all nsIDocument object for SRI API.
               4. Let caller class(Loader/nsScriptLoader) hold a nsIConsoleReportCollector pointer, create it as the class init, flush the report at suitable timing and pass it into SRI when call SRI's API.

Note: The reason that "I implement #4 rather than let SRIMetadata hold a ConsoleReportCollector" is there may have more than one SRIMetadata while Checking SRI [4] and I believe that it(#4) is also a well mechanism for reporting to console. I'll make fetch API(Patch 4) do the same mechanism if it is ok for you.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/ConsoleReportCollector.h?from=ConsoleReportCollector#24
[2] https://dxr.mozilla.org/mozilla-central/source/dom/base/ConsoleReportCollector.cpp#58-60
[3] https://dxr.mozilla.org/mozilla-central/source/dom/security/SRICheck.cpp#86-91
[4] https://dxr.mozilla.org/mozilla-central/source/dom/security/SRICheck.cpp#160
Attachment #8757857 - Attachment is obsolete: true
Attachment #8760056 - Flags: feedback?(bkelly)
Comment on attachment 8760056 [details] [diff] [review]
Bug-1187335 - P2 - Modify the way to report to console for worker and use LoadTainting to decide CORS or not

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

Looks great!  Thanks.

::: dom/base/ConsoleReportCollector.cpp
@@ +55,5 @@
>      // just turns around and converts it back to a spec.
>      nsCOMPtr<nsIURI> uri;
>      nsresult rv = NS_NewURI(getter_AddRefs(uri), report.mSourceFileURI);
>      if (NS_FAILED(rv)) {
> +    //  continue;

So this fails when mSourceFileURI is empty string?  I guess wrap the NS_NewURI() call and its error checking in a "if (!report.mSourceFileURI.IsEmpty())".  And then maybe change the error checking to an assertion with MOZ_ALWAYS_SUCCEEDS(rv).

What do you think?

::: dom/security/SRIMetadata.h
@@ +8,5 @@
>  #define mozilla_dom_SRIMetadata_h
>  
>  #include "nsTArray.h"
>  #include "nsString.h"
> +#include "nsIConsoleReportCollector.h"

I don't think this is the right place for this include.  You probably want to forward declare in SRICheck.h and include the header in SRICheck.cpp.
Attachment #8760056 - Flags: feedback?(bkelly) → feedback+
Assignee

Comment 58

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #57)
Thanks for the feedback!
 
> ::: dom/base/ConsoleReportCollector.cpp
> @@ +55,5 @@
> >      // just turns around and converts it back to a spec.
> >      nsCOMPtr<nsIURI> uri;
> >      nsresult rv = NS_NewURI(getter_AddRefs(uri), report.mSourceFileURI);
> >      if (NS_FAILED(rv)) {
> > +    //  continue;
> 
> So this fails when mSourceFileURI is empty string?  I guess wrap the

Yes, it fails when mSourceFileURI is empty string.

> NS_NewURI() call and its error checking in a "if
> (!report.mSourceFileURI.IsEmpty())".  And then maybe change the error
> checking to an assertion with MOZ_ALWAYS_SUCCEEDS(rv).
> 
> What do you think?

Yeah, adding an "if" condition can avoid failure at empty string. However, I wonder about once we change the assert, It will also change the current behavior. If we change the assertion to MOZ_ALWAYS_SUCCEEDS, it may affect all the other pending reports once a bad report hit the assert. The current behavior will leave bad report behind and keep process the other pending reports. Should we do that or I misunderstand something?
 
> ::: dom/security/SRIMetadata.h
> @@ +8,5 @@
> >  #define mozilla_dom_SRIMetadata_h
> >  
> >  #include "nsTArray.h"
> >  #include "nsString.h"
> > +#include "nsIConsoleReportCollector.h"
> 
> I don't think this is the right place for this include.  You probably want
> to forward declare in SRICheck.h and include the header in SRICheck.cpp.

Oh, I forget to remove it. Thanks for the notice! It was used to let the SRIMetadata carry a consolereportcollector.
Assignee

Comment 59

3 years ago
(In reply to Tom Tung [:tt][Dayoff: 6/9~6/10, PTO: 6/20] from comment #58)
> > What do you think?
> Should we do that or I misunderstand something?

I just find that I may misunderstand something. Did you mean that just change the assertion and keep "if(..) {continue;}" still there?
Assignee

Comment 60

3 years ago
Hi Ben, 

  Could you help me to review this patch? I address your comment, but I put ConsoleReportCollector into P2-1. Thanks!

  As I said in comment #56, this patch is for:
   1. Add a function for reporting to console in SRI API.
   2. Using LoadTainting to decide whehter is CORS or not.
   3. Removing passing all nsIDocument object for SRI API.
   4. Let caller class(Loader/nsScriptLoader) hold a nsIConsoleReportCollector pointer, create it as the class init, flush the report at suitable timing and pass it into SRI when call SRI's API.
Attachment #8760056 - Attachment is obsolete: true
Attachment #8760750 - Flags: review?(bkelly)
Assignee

Comment 61

3 years ago
This patch is for ConsoleReportCollector accept an empty SourceURI and add an assertion as SourceURI fail to convert. 

Could you help to review this patch, Ben? Thanks!
Attachment #8760753 - Flags: review?(bkelly)
Assignee

Comment 62

3 years ago
Comment on attachment 8757856 [details] [diff] [review]
Bug-1187335 - P1 - Add FlushReportToAllClient() to ServiceWorkerManager for flushing report to all clients.

Hi Ben,

This patch is for ServiceWorker to Flush Reports to right windows and it will be used in Fetch API at Patch #4.  Would you mind helping me to review this patch? Thanks!
Attachment #8757856 - Flags: review?(bkelly)
Assignee

Comment 63

3 years ago
Hi Ben,

Sorry for send few review request in a short time, but I would like to ask you review this patch, too. Would you mind helping me to review it? Thanks!

This your patch is mainly for integrity fetch with SRI. 
In this patch, 1. add Integirty attribute to Fetch (Request/InternalRequest)
               2. handle ConsoleReportCollector in FetchDriverObserver and its derived class (create as ctor, pass it to SRI API and flush it at OnRespondEnd())
               3. handle integrity with SRI API in FetchDriver.
Attachment #8757860 - Attachment is obsolete: true
Attachment #8760796 - Flags: review?(bkelly)
Assignee

Comment 64

3 years ago
Comment on attachment 8757859 [details] [diff] [review]
Bug-1187335 - P3 - Modify SRI test to match current behavior.

This patch is addressing the comment #30 for correcting the SRI tests. Could you help me to review it, Ben? Thanks!!
Attachment #8757859 - Flags: review?(bkelly)
Assignee

Comment 65

3 years ago
Comment on attachment 8757861 [details] [diff] [review]
Bug-1187335- P5 - Enable SRI's tests and Request's integrity attribute in wpt.

Hi Ben,

This patch is for removing the masks (expect fail) in web-platform test after adding SRI into Fetch API. Could you help me to review this one? Thanks!
Attachment #8757861 - Attachment description: Bug-1187335-P5 - Enable SRI's tests and Request's integrity attribute in wpt. → Bug-1187335- P5 - Enable SRI's tests and Request's integrity attribute in wpt.
Attachment #8757861 - Flags: review?(bkelly)
Comment on attachment 8757856 [details] [diff] [review]
Bug-1187335 - P1 - Add FlushReportToAllClient() to ServiceWorkerManager for flushing report to all clients.

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +1472,5 @@
> +    }
> +
> +    windows.AppendElement(doc->InnerWindowID());
> +
> +    aReporter->FlushConsoleReports(doc);

So this will flush the reports to the first document seen, free the reports, and then the remaining documents will not get any reports.  See:

https://dxr.mozilla.org/mozilla-central/source/dom/base/ConsoleReportCollector.cpp#48

You need to add an argument to FlushConsoleReports() that lets the caller avoid dropping the reports.  Something like:

enum class ReportAction {
  Forget,
  Save
};

void
FlushConsoleReports(nsIDocument* aDoc, ReportAction aAction = ReportAction::Forget);
Attachment #8757856 - Flags: review?(bkelly) → review-
Attachment #8757859 - Flags: review?(bkelly) → review+
Attachment #8757861 - Flags: review?(bkelly) → review+
Attachment #8760750 - Flags: review?(bkelly) → review+
Comment on attachment 8760753 [details] [diff] [review]
Bug 1187335 - P2-1 - Add an assertion as SourceURI fail and accept an empty SourceURI for ConsoleReportCollector.

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

::: dom/base/ConsoleReportCollector.cpp
@@ +56,5 @@
>      nsCOMPtr<nsIURI> uri;
> +    if (!report.mSourceFileURI.IsEmpty()) {
> +      nsresult rv = NS_NewURI(getter_AddRefs(uri), report.mSourceFileURI);
> +      if (NS_FAILED(rv)) {
> +        MOZ_ALWAYS_SUCCEEDS(rv);

nit: I would put this before if NS_FAILED(rv).  Or if you want to keep it in the if-statement make it MOZ_ASSERT_UNREACHABLE().
Attachment #8760753 - Flags: review?(bkelly) → review+
Comment on attachment 8760796 [details] [diff] [review]
Bug 1187335 - P4 - Integrity fetch with SRI & add nsIConsoleReportCollector to show console report - fetch API changed

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

::: dom/fetch/Fetch.cpp
@@ +494,5 @@
> +      swm->FlushReportToAllClients(worker->WorkerName(), mReporter);
> +    }
> +  } else {
> +    MOZ_ASSERT(worker->GetDocument());
> +    mReporter->FlushConsoleReports(worker->GetDocument());

You're missing a few cases here:

1) Nested workers will have a parent that ultimately chains to a document, but GetDocument() here will return nullptr.  You probably need to walk this chain.

2) SharedWorkers will have multiple documents.  You probably want to use the same FlushConsoleReports() call on each document that SWM does.

I would recommend making a WorkerPrivate::FlushConsoleReports(nsIConsoleReporter* aReporter) that does the right thing depending on the worker type.  That way we can reuse this logic elsewhere.

::: dom/fetch/FetchDriver.cpp
@@ +672,5 @@
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return rv;
> +      }
> +
> +      rv = mSRIDataVerifier->Update(aCount, (uint8_t*)buffer.Elements());

This should be passing aRead instead of aCount.

@@ +676,5 @@
> +      rv = mSRIDataVerifier->Update(aCount, (uint8_t*)buffer.Elements());
> +      NS_ENSURE_SUCCESS(rv, rv);
> +
> +      rv = mPipeOutputStream->Write(reinterpret_cast<char*>(buffer.Elements()),
> +                                    aCount, &aWrite);

Same here.  aRead instead of aCount.

::: dom/fetch/Request.cpp
@@ +494,5 @@
> +    if (!integrity.IsEmpty()) {
> +      nsAutoCString method;
> +      request->GetMethod(method);
> +      NS_ConvertUTF8toUTF16 label(method);
> +      aRv.ThrowTypeError<MSG_INVALID_REQUEST_METHOD>(label);

This error message seems cut and paste?

::: dom/workers/ServiceWorkerPrivate.cpp
@@ +1259,5 @@
>      , mRequestCredentials(RequestCredentials::Same_origin)
>      , mContentPolicyType(nsIContentPolicy::TYPE_INVALID)
>      , mReferrer(kFETCH_CLIENT_REFERRER_STR)
>      , mReferrerPolicy(ReferrerPolicy::_empty)
> +    , mIntegrity(EmptyString())

Shouldn't we be pulling this from the nsIChannel so that we reflect the requests real integrity value?  We should have a test case that shows this as well.
Attachment #8760796 - Flags: review?(bkelly) → review-
Assignee

Comment 69

3 years ago
Hi Ben,

Thanks for the review and the feedback!

(In reply to Ben Kelly [:bkelly] from comment #66) 
> ::: dom/workers/ServiceWorkerManager.cpp
> @@ +1472,5 @@
> > +    }
> > +
> > +    windows.AppendElement(doc->InnerWindowID());
> > +
> > +    aReporter->FlushConsoleReports(doc);
> 
> So this will flush the reports to the first document seen, free the reports,
> and then the remaining documents will not get any reports.  See:
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/base/
> ConsoleReportCollector.cpp#48
> 
> You need to add an argument to FlushConsoleReports() that lets the caller
> avoid dropping the reports.  Something like:
> 
> enum class ReportAction {
>   Forget,
>   Save
> };
> 
> void
> FlushConsoleReports(nsIDocument* aDoc, ReportAction aAction =
> ReportAction::Forget);

Oh, I should notice the report must be clear once it was flushed. Sorry for that. 
I'll implement that.

(In reply to Ben Kelly [:bkelly] from comment #67)
> Comment on attachment 8760753 [details] [diff] [review]
> Bug 1187335 - P2-1 - Add an assertion as SourceURI fail and accept an empty
> SourceURI for ConsoleReportCollector.
> 
> Review of attachment 8760753 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/ConsoleReportCollector.cpp
> @@ +56,5 @@
> >      nsCOMPtr<nsIURI> uri;
> > +    if (!report.mSourceFileURI.IsEmpty()) {
> > +      nsresult rv = NS_NewURI(getter_AddRefs(uri), report.mSourceFileURI);
> > +      if (NS_FAILED(rv)) {
> > +        MOZ_ALWAYS_SUCCEEDS(rv);
> 
> nit: I would put this before if NS_FAILED(rv).  Or if you want to keep it in
> the if-statement make it MOZ_ASSERT_UNREACHABLE().

Sure, I would like to put MOZ_ALWAYS_SUCCEEDS before the if condition.

(In reply to Ben Kelly [:bkelly] from comment #68)
> Comment on attachment 8760796 [details] [diff] [review]
> Bug 1187335 - P4 - Integrity fetch with SRI & add nsIConsoleReportCollector
> to show console report - fetch API changed
> 
> Review of attachment 8760796 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/fetch/Fetch.cpp
> @@ +494,5 @@
> > +      swm->FlushReportToAllClients(worker->WorkerName(), mReporter);
> > +    }
> > +  } else {
> > +    MOZ_ASSERT(worker->GetDocument());
> > +    mReporter->FlushConsoleReports(worker->GetDocument());
> 
> You're missing a few cases here:
> 
> 1) Nested workers will have a parent that ultimately chains to a document,
> but GetDocument() here will return nullptr.  You probably need to walk this
> chain.
> 
> 2) SharedWorkers will have multiple documents.  You probably want to use the
> same FlushConsoleReports() call on each document that SWM does.
> 
> I would recommend making a
> WorkerPrivate::FlushConsoleReports(nsIConsoleReporter* aReporter) that does
> the right thing depending on the worker type.  That way we can reuse this
> logic elsewhere.

Sorry for missing them. I'll try to implement them in next patch! 
BTW, should I create few tests for make sure that I do the behavior correctly? But, I have no idea how to do that. Could you show some existing tests for me or teach me how to write tests for ensuring I report to correct console for each kind of workers? 

> ::: dom/fetch/FetchDriver.cpp
> @@ +672,5 @@
> > +      if (NS_WARN_IF(NS_FAILED(rv))) {
> > +        return rv;
> > +      }
> > +
> > +      rv = mSRIDataVerifier->Update(aCount, (uint8_t*)buffer.Elements());
> 
> This should be passing aRead instead of aCount.
> @@ +676,5 @@
> > +      rv = mSRIDataVerifier->Update(aCount, (uint8_t*)buffer.Elements());
> > +      NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +      rv = mPipeOutputStream->Write(reinterpret_cast<char*>(buffer.Elements()),
> > +                                    aCount, &aWrite);
> 
> Same here.  aRead instead of aCount.

Yeah, sorry of that. I'll correct them. 

> 
> ::: dom/fetch/Request.cpp
> @@ +494,5 @@
> > +    if (!integrity.IsEmpty()) {
> > +      nsAutoCString method;
> > +      request->GetMethod(method);
> > +      NS_ConvertUTF8toUTF16 label(method);
> > +      aRv.ThrowTypeError<MSG_INVALID_REQUEST_METHOD>(label);
> 
> This error message seems cut and paste?

Yes, I thought it is kind of general type error message, but I was wrong. I'll create a new error message for empty integrity.
> 
> ::: dom/workers/ServiceWorkerPrivate.cpp
> @@ +1259,5 @@
> >      , mRequestCredentials(RequestCredentials::Same_origin)
> >      , mContentPolicyType(nsIContentPolicy::TYPE_INVALID)
> >      , mReferrer(kFETCH_CLIENT_REFERRER_STR)
> >      , mReferrerPolicy(ReferrerPolicy::_empty)
> > +    , mIntegrity(EmptyString())
> 
> Shouldn't we be pulling this from the nsIChannel so that we reflect the
> requests real integrity value?  We should have a test case that shows this
> as well.
Could you tell me more about this? I am not sure how to do that. Thanks!
(In reply to Tom Tung [:tt][Dayoff: 6/9~6/10, PTO: 6/20] from comment #69)
> > > +    , mIntegrity(EmptyString())
> > 
> > Shouldn't we be pulling this from the nsIChannel so that we reflect the
> > requests real integrity value?  We should have a test case that shows this
> > as well.
> Could you tell me more about this? I am not sure how to do that. Thanks!

Oh, I see.  The integrity value is never passed through the channel.  The thing doing the integrity check has to live on top of the channel to and do the comparison manually itself.

So what we need to do is pass the integrity metadata string through the nsIChannel.  You can add a SetIntegrityMetadata() and GetIntegrityMetadata() in nsIHttpChannelInternal.  (Maybe intregrity applies to other schemes, but we only support SW interception on HTTP at the moment).

Then the code initiating the nsIChannel that will have its integrity checked must call SetIntegrityMetadata().  The ServiceWorker code would then call GetIntegrityMetadata() when creating a Request object from the intercepted channel.

To avoid missing the SetIntegrityMetadata() call, I would probably make the existing code use GetIntegrityMetadata() as well instead of looking at its local variable.

Does that make sense?
Assignee

Comment 73

3 years ago
Hi François,

Would you mind helping me to review the modifications for SRI API? Thanks! The reason that I did this patch is trying not to use nsIDocument to reporting to console since workers' Document is loading and it's not suitable for reporting to console.
This patch is mainly done:
   1. Add a function for reporting to console in SRI API.
   2. Using LoadTainting to decide whehter is CORS or not.
   3. Removing passing all nsIDocument object for SRI API.
   4. Let caller class(Loader/nsScriptLoader) hold a nsIConsoleReportCollector pointer, create it as the class init, flush the report at suitable timing and pass it into SRI when call SRI's API.
Attachment #8760750 - Attachment is obsolete: true
Attachment #8763005 - Flags: review?(francois)
Assignee

Comment 74

3 years ago
Hi François,

Could you also take a look for this patch? Thanks! 
This patch is addressing the comment #30 for correcting the SRI tests.
Attachment #8757859 - Attachment is obsolete: true
Attachment #8763006 - Flags: review?(francois)
Assignee

Updated

3 years ago
Attachment #8762839 - Attachment description: [Final] Bug 1187335 - P5 - Enable SRI's tests and Request's integrity attribute in wpt. r=bkelly. → [Final] Bug 1187335 - P5-part1 - Enable SRI's tests and Request's integrity attribute in wpt. r=bkelly.
Assignee

Comment 76

3 years ago
Hi Ben,

I not sure about whether the direction of this patch is correct or not. Thus, I send a feedback request to you for checking. Could you give me some feedback? Thanks!

In this patch, I addressed the comments(add an enum and a clearing function into nsConsoleReportCollector, and add a flushing reports for sharedworekr). I list the problems which I'm not sure below inline. 

1. Should I modify nsIConsoleReportCollector.h?
To address comment #66 (add enum Action), I also implement a function to clear the pending reports. I implement them into nsIConsoleReportCollector.h and do the corresponding change to its derived classes. 

2. Does preventDefault have any effect on ConsoleReportCollector? There are few code to deal with it when sharedworker calls BroadcastErrorToSharedWorkers(..)  as [1]. 
I do not deal with preventDefault case in this patch. If it is necessary, I'll implement it in next patch.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp?q=WorkerPrivate.cpp&redirect_type=direct#3351-3378
Besides, I also add a function to flush report for sharedworker
Attachment #8757856 - Attachment is obsolete: true
Attachment #8765368 - Flags: feedback?(bkelly)
Assignee

Updated

3 years ago
Attachment #8765368 - Attachment description: Bug-1187335-P1.patch → Bug 1187335 - P1 - Add FlushReportToAllClient() to ServiceWorkerManager and SharedWorker.
Assignee

Comment 77

3 years ago
Could you help me to review this patch, Ben? Thanks!

In this patch, I addressed the comment #68 except the test case for pulling integrity form nsIChannel. I'll do it later.
Attachment #8760796 - Attachment is obsolete: true
Attachment #8765370 - Flags: review?(bkelly)
Assignee

Updated

3 years ago
Attachment #8765370 - Attachment description: Bug 1187335 - P4-part1 - Integrity fetch with SRI & add nsIConsoleReportCollector to show console report - fetch API changed → Bug 1187335 - P4-part1 - Integrate fetch with SRI & add nsIConsoleReportCollector to show console report - fetch API changed
Assignee

Comment 78

3 years ago
I create this patch for implementing request.integrity into Cache API. Do you mind helping me to review it also, Ben? Thanks!

BTW, should I add a test for it at test_migration.js?
Attachment #8765372 - Flags: review?(bkelly)
Assignee

Updated

3 years ago
Attachment #8763005 - Attachment description: Bug 1187335 - P2 - Modify the way to report to console for worker and use LoadTainting to decide CORS or not. r=bkelly. → Bug 1187335 - P2-part1 - Modify the way to report to console for worker and use LoadTainting to decide CORS or not. r=bkelly.
Assignee

Comment 79

3 years ago
I add a wpt test for testing request.integrity to shared worker. Could you help me to review this patch, Ben? Thanks!
Attachment #8765376 - Flags: review?(bkelly)
Assignee

Updated

3 years ago
Attachment #8765376 - Attachment description: Add a SharedWorker integrity test in wpt. → Bug - 1187335 - P5-part2 - Add a SharedWorker integrity test in wpt.
Comment on attachment 8765368 [details] [diff] [review]
Bug 1187335 - P1 - Add FlushReportToAllClient() to ServiceWorkerManager and SharedWorker.

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

::: dom/console/nsIConsoleReportCollector.h
@@ +93,5 @@
>    //                ownership of our currently console reports.
>    virtual void
>    FlushConsoleReports(nsIConsoleReportCollector* aCollector) = 0;
> +
> +  // Clear all pending reports. Main thread only.

Why is this documented as main thread only?  It seems it could be safely called from any thread.

::: dom/workers/WorkerPrivate.cpp
@@ +3604,5 @@
> +
> +  if (sharedWorkers.IsEmpty()) {
> +    aReporter->FlushConsoleReports((nsIDocument*)nullptr);
> +    return;
> +  }

Lets just let it fall through the loops below and trigger the final flush to null document.  I don't think this short circuit really buys us much here.

@@ +3608,5 @@
> +  }
> +
> +  // First fire the error event at all SharedWorker objects. This may include
> +  // multiple objects in a single window as well as objects in different
> +  // windows.

This comment seems wrong.  Cut&paste mistake?

@@ +3626,5 @@
> +  // If there are no windows to consider further then we're done.
> +  if (windowActions.IsEmpty()) {
> +    aReporter->FlushConsoleReports((nsIDocument*)nullptr);
> +    return;
> +  }

Same here.  Lets remove the short circuit to simplify this method.

@@ +3635,5 @@
> +  for (uint32_t index = 0; index < windowActions.Length(); index++) {
> +    WindowAction& windowAction = windowActions[index];
> +
> +    // If there is no window or the script already called preventDefault then
> +    // skip this window.

Is this comment about preventDefault also stale from a copy/paste?
Attachment #8765368 - Flags: feedback?(bkelly) → feedback+
Comment on attachment 8765370 [details] [diff] [review]
Bug 1187335 - P4-part1 - Integrate fetch with SRI & add nsIConsoleReportCollector to show console report - fetch API changed

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

r=me with comments addressed.

::: dom/fetch/Fetch.cpp
@@ +463,5 @@
> +  } else if (worker->IsSharedWorker()) {
> +    worker->FlushReportToShraredWorkers(mReporter);
> +  } else {
> +    mReporter->FlushConsoleReports(worker->GetDocument());
> +  }

nit: Can you break up this if/else chain into if-statements with early return?  So:

if (IsServiceWorker) {
  // flush to service worker
  return;
}

if (IsSharedWorker) {
  // flush to shared worker
  return;
}

// flush to dedicated worker

::: dom/fetch/FetchDriver.cpp
@@ +318,5 @@
>      mRequest->MaybeSkipCacheIfPerformingRevalidation();
>      internalChan->SetFetchCacheMode(static_cast<uint32_t>(mRequest->GetCacheMode()));
> +    nsString integrity;
> +    mRequest->GetIntegrity(integrity);
> +    internalChan->SetIntegrityMetadata(integrity);

See my note about making InternalRequest::GetIntegrity() just return const nsString& to make this a bit nicer here.

::: dom/fetch/FetchDriver.h
@@ +97,5 @@
>    nsCOMPtr<nsIOutputStream> mPipeOutputStream;
>    RefPtr<FetchDriverObserver> mObserver;
>    nsCOMPtr<nsIDocument> mDocument;
> +  nsAutoPtr<mozilla::dom::SRICheckDataVerifier> mSRIDataVerifier;
> +  mozilla::dom::SRIMetadata mSRIMetadata;

nit: I don't think you need the mozilla::dom namespace here.  FetchDriver is already in the mozilla::dom namespace itself.

::: dom/fetch/InternalRequest.h
@@ +99,5 @@
>      , mCredentialsMode(RequestCredentials::Omit)
>      , mResponseTainting(LoadTainting::Basic)
>      , mCacheMode(RequestCache::Default)
>      , mRedirectMode(RequestRedirect::Follow)
> +    , mIntegrity(EmptyString())

This isn't necessary.  nsString already defaults to EmptyString().

@@ +349,5 @@
>      mRedirectMode = aRedirectMode;
>    }
>  
> +  void
> +  GetIntegrity(nsAString& aIntegrity) const

This might be a little nicer if it was:

const nsString& GetIntegrity() const;

::: dom/fetch/Request.cpp
@@ +502,5 @@
> +    if (!integrity.IsEmpty()) {
> +      nsAutoCString method;
> +      request->GetMethod(method);
> +      NS_ConvertUTF8toUTF16 label(method);
> +      aRv.ThrowTypeError<MSG_INVALID_REQUEST_INTEGRITY>(label);

This doesn't seem quite right.  It will produce a message like:

  Invalid request integrity POST.

It seems we should either be setting the integrity value as the message label or the message should mention the method is wrong.

::: netwerk/protocol/http/nsIHttpChannelInternal.idl
@@ +289,5 @@
> +
> +    /**
> +     * Set to indicate Request.integrity.
> +     */
> +    attribute AString IntegrityMetadata;

This patch sets the integrity metadata when its added via fetch(), but we also need to set this attribute when SRI is being used for scripts, stylesheets, etc.  Please set it in those cases as well.

You can do this in another patch, though.  Might be easier than reviewing this whole patch again.

nit: We normally make idl attributes begin with lower cases letters.  So integrityMetadata.  This then gets automatically converted to uppercase for c++ accessors to it.
Attachment #8765370 - Flags: review?(bkelly) → review+
Comment on attachment 8765372 [details] [diff] [review]
Bug 1187335 - P4-part2 - Integrate cache with request.integrity

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

Did anything change in this patch?  I feel like I reviewed it before.
Attachment #8765372 - Flags: review?(bkelly) → review+
Attachment #8765376 - Flags: review?(bkelly) → review+
Can you add some test cases here that verify the service worker sees the correct value in event.request.integrity?

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/fetch-event.https.html

That test is mostly working with fetch(), but it would be nice to also have a case for stylesheets and scripts using SRI.  I suppose those could go in that same test or this test might work:

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/fetch-request-resources.https.html
Assignee

Comment 84

3 years ago
Hi Ben,

Thanks for the reviews and feedback! Your comments help me a lot! I will address them in next patch.

(In reply to Ben Kelly [:bkelly] from comment #80)
> Comment on attachment 8765368 [details] [diff] [review]
> Bug 1187335 - P1 - Add FlushReportToAllClient() to ServiceWorkerManager and
> SharedWorker.
> 
> Review of attachment 8765368 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/console/nsIConsoleReportCollector.h
> @@ +93,5 @@
> >    //                ownership of our currently console reports.
> >    virtual void
> >    FlushConsoleReports(nsIConsoleReportCollector* aCollector) = 0;
> > +
> > +  // Clear all pending reports. Main thread only.
> 
> Why is this documented as main thread only?  It seems it could be safely
> called from any thread.

Oh, it shouldn't. I commented that because I thought it should be called at the same thread as |FlushConsoleReports(document)|. 

> ::: dom/workers/WorkerPrivate.cpp
> @@ +3604,5 @@
> > +
> > +  if (sharedWorkers.IsEmpty()) {
> > +    aReporter->FlushConsoleReports((nsIDocument*)nullptr);
> > +    return;
> > +  }
> 
> Lets just let it fall through the loops below and trigger the final flush to
> null document.  I don't think this short circuit really buys us much here.

Sure. 
 
> @@ +3608,5 @@
> > +  }
> > +
> > +  // First fire the error event at all SharedWorker objects. This may include
> > +  // multiple objects in a single window as well as objects in different
> > +  // windows.
> 
> This comment seems wrong.  Cut&paste mistake?

Sorry for that. I just want to write a simple version of flushing report for sharedworker to ask for the direction about this patch and make sure I don't need to worry about preventDefault. I should be more careful about these before sending the requests.

(In reply to Ben Kelly [:bkelly] from comment #81)
> Comment on attachment 8765370 [details] [diff] [review]
> Bug 1187335 - P4-part1 - Integrate fetch with SRI & add
> nsIConsoleReportCollector to show console report - fetch API changed
> 
> Review of attachment 8765370 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed.
> 
> ::: dom/fetch/Request.cpp
> @@ +502,5 @@
> > +    if (!integrity.IsEmpty()) {
> > +      nsAutoCString method;
> > +      request->GetMethod(method);
> > +      NS_ConvertUTF8toUTF16 label(method);
> > +      aRv.ThrowTypeError<MSG_INVALID_REQUEST_INTEGRITY>(label);
> 
> This doesn't seem quite right.  It will produce a message like:
> 
>   Invalid request integrity POST.
> 
> It seems we should either be setting the integrity value as the message
> label or the message should mention the method is wrong.

Sure, I prefer to pass the integrity value as the message label one.

> ::: netwerk/protocol/http/nsIHttpChannelInternal.idl
> @@ +289,5 @@
> > +
> > +    /**
> > +     * Set to indicate Request.integrity.
> > +     */
> > +    attribute AString IntegrityMetadata;
> 
> This patch sets the integrity metadata when its added via fetch(), but we
> also need to set this attribute when SRI is being used for scripts,
> stylesheets, etc.  Please set it in those cases as well.
> 
> You can do this in another patch, though.  Might be easier than reviewing
> this whole patch again.
>
> nit: We normally make idl attributes begin with lower cases letters.  So
> integrityMetadata.  This then gets automatically converted to uppercase for
> c++ accessors to it.

Ok! I'll create another patch to implement that!

(In reply to Ben Kelly [:bkelly] from comment #82)
> Comment on attachment 8765372 [details] [diff] [review]
> Bug 1187335 - P4-part2 - Integrate cache with request.integrity
> 
> Review of attachment 8765372 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Did anything change in this patch?  I feel like I reviewed it before.

No, I implement them after the fetch api part is almost done.
Assignee

Comment 85

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #83)
> Can you add some test cases here that verify the service worker sees the
> correct value in event.request.integrity?
> 
> https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/
> service-workers/service-worker/fetch-event.https.html
> 
> That test is mostly working with fetch(), but it would be nice to also have
> a case for stylesheets and scripts using SRI.  I suppose those could go in
> that same test or this test might work:
> 
> https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/
> service-workers/service-worker/fetch-request-resources.https.html

Sure, I'll write a patch for adding test cases for this bug.
Assignee

Comment 86

3 years ago
Hi Ben,

Sorry for the late reply. This patch address the comments. It mainly to implement to flush report to all service worker' clients and to all shared worker. 

Could you help me to review this patch? Thanks!
Attachment #8765368 - Attachment is obsolete: true
Attachment #8767122 - Flags: review?(bkelly)
Assignee

Comment 87

3 years ago
I addressed the comments, but there are few of them that I want to ensure that they are correct. Could you help me to review this patch again?

note: For error message log (Errors,msg), I changed my mind. I think that it's better to tell users realize that request.integrity should be empty as no-cors mode.
Attachment #8765370 - Attachment is obsolete: true
Attachment #8767123 - Flags: review?(bkelly)
Assignee

Comment 88

3 years ago
Rebase this patch since we changed the method to get integrity.
Attachment #8765372 - Attachment is obsolete: true
Assignee

Comment 91

3 years ago
Comment on attachment 8767126 [details] [diff] [review]
Bug 1187335 - P5-part3 - Add test for SRI in service worker

This patch is to add a test for ensuring that service worker get correct value of request.integrity. Could you help me to review this one? Thanks!
Comment on attachment 8767122 [details] [diff] [review]
Bug 1187335 - P1 - Add FlushReportToAllClient() to ServiceWorkerManager and SharedWorker.

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

r=me with comments addressed.  Thanks!

::: dom/workers/ServiceWorkerManager.cpp
@@ +1513,5 @@
> +
> +    windows.AppendElement(doc->InnerWindowID());
> +
> +    aReporter->FlushConsoleReports(doc,
> +                                   ConsoleReportCollector::ReportAction::Save);

nit: I think you could shorten this to aReporter::ReportAction::Save.  Here and elsewhere in the patch.

::: dom/workers/ServiceWorkerManager.h
@@ +189,5 @@
>                       uint32_t aColumnNumber,
>                       uint32_t aFlags);
>  
> +  void
> +  FlushReportToAllClients(const nsCString& aScope,

nit: s/FlushReport/FlushReports/g

::: dom/workers/WorkerPrivate.cpp
@@ +3592,5 @@
>  
>  template <class Derived>
> +void
> +WorkerPrivateParent<Derived>::FlushReportToShraredWorkers(
> +                                           nsIConsoleReportCollector* aReporter)

Have you at least tested this function locally to see if it works for a SharedWorker attached to two different windows?  Having an automated test would be ideal, but I just wanted to make sure we at least verified with local testing.

@@ +3608,5 @@
> +    // May be null.
> +    nsPIDOMWindowInner* window = sharedWorker->GetOwner();
> +
> +    // Add the owning window to our list so that we will flush the reports later.
> +    if (!windowActions.Contains(window)) {

If window is nullptr, we probably don't want to add it to windowActions, right?

@@ +3613,5 @@
> +      windowActions.AppendElement(WindowAction(window));
> +    }
> +  }
> +
> +  bool reportErrorToConsole = false;

nit: Maybe clarify this variable bit by indicating its a fallback to the browser console.  Maybe `reportErrorToBrowserConsole`.

@@ +3621,5 @@
> +    WindowAction& windowAction = windowActions[index];
> +
> +    if (!windowAction.mWindow) {
> +      continue;
> +    }

If we just avoid adding nullptr's to windowActions above, then we can remove this check.
Attachment #8767122 - Flags: review?(bkelly) → review+
Comment on attachment 8767123 [details] [diff] [review]
Bug 1187335 - P4-part1 - Integrate fetch with SRI & add nsIConsoleReportCollector to show console report - fetch API changed.

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

r=me with comments addressed.

::: dom/bindings/Errors.msg
@@ +58,5 @@
>  MSG_DEF(MSG_INVALID_HEADER_VALUE, 1, JSEXN_TYPEERR, "{0} is an invalid header value.")
>  MSG_DEF(MSG_INVALID_HEADER_SEQUENCE, 0, JSEXN_TYPEERR, "Headers require name/value tuples when being initialized by a sequence.")
>  MSG_DEF(MSG_PERMISSION_DENIED_TO_PASS_ARG, 1, JSEXN_TYPEERR, "Permission denied to pass cross-origin object as {0}.")
>  MSG_DEF(MSG_MISSING_REQUIRED_DICTIONARY_MEMBER, 1, JSEXN_TYPEERR, "Missing required {0}.")
> +MSG_DEF(MSG_REQUEST_INTEGRITY_METADATA_NOT_EMPTY, 0, JSEXN_TYPEERR, "Request integrity metadata should be a empty string when no-cors mode.")

s/a empty/an empty/g
s/when no-cors/when in no-cors/g

::: dom/fetch/Fetch.cpp
@@ +466,5 @@
> +  }
> +
> +  if (worker->IsSharedWorker()) {
> +    // Flush to shared worker
> +    worker->FlushReportToShraredWorkers(mReporter);

s/Shrared/Shared/g

Sorry I missed this typo in the other patch.

::: dom/fetch/InternalRequest.h
@@ +356,5 @@
> +
> +  void
> +  SetIntegrity(const nsAString& aIntegrity)
> +  {
> +    mIntegrity.Assign(aIntegrity);

Is there every any reason the integrity value could change?  Can we MOZ_ASSERT(mIntegrity.IsEmpty()) here?

::: dom/fetch/Request.h
@@ +87,5 @@
> +    aIntegrity = mRequest->GetIntegrity();
> +  }
> +
> +  void
> +  SetIntegrity(const nsAString& aIntegrity)

Is this actually called from anywhere?  The binding is read-only and our internal code can use InternalRequest::SetIntegrity().  Can we remove this?
Attachment #8767123 - Flags: review?(bkelly) → review+
Comment on attachment 8767126 [details] [diff] [review]
Bug 1187335 - P5-part3 - Add test for SRI in service worker

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

Looks good!  I think we still need tests to verify event.request.integrity is set properly when a service worker intercepts <script integrity="foo"> and <link rel="stylesheet" integrity="foo">.
Attachment #8767126 - Flags: review?(bkelly) → review+
Assignee

Comment 95

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #93)
Hi Ben, 

Thanks for your time and the review! I address comments except one and I leave my question below inline. 

> Comment on attachment 8767123 [details] [diff] [review]
> Bug 1187335 - P4-part1 - Integrate fetch with SRI & add
> nsIConsoleReportCollector to show console report - fetch API changed.
> 
> Review of attachment 8767123 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed.
> 
> ::: dom/fetch/InternalRequest.h
> @@ +356,5 @@
> > +
> > +  void
> > +  SetIntegrity(const nsAString& aIntegrity)
> > +  {
> > +    mIntegrity.Assign(aIntegrity);
> 
> Is there every any reason the integrity value could change?  Can we
> MOZ_ASSERT(mIntegrity.IsEmpty()) here?

I did similar thing (like [1]) here due to step 24 [2]. Hence, the integrity could be changed. I think it could be passed a null string?  If it can, we cannot put this assert here.

[1] http://searchfox.org/mozilla-central/source/dom/fetch/Request.cpp#441-443
[2] https://fetch.spec.whatwg.org/#dom-request

> ::: dom/fetch/Request.h
> @@ +87,5 @@
> > +    aIntegrity = mRequest->GetIntegrity();
> > +  }
> > +
> > +  void
> > +  SetIntegrity(const nsAString& aIntegrity)
> 
> Is this actually called from anywhere?  The binding is read-only and our
> internal code can use InternalRequest::SetIntegrity().  Can we remove this?

Yes, it should not be existed for a read-only attribute. Sorry for that.
Attachment #8767123 - Attachment is obsolete: true
Assignee

Comment 96

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #92)
> Comment on attachment 8767122 [details] [diff] [review]
> Bug 1187335 - P1 - Add FlushReportToAllClient() to ServiceWorkerManager and
> SharedWorker.
> 
> Review of attachment 8767122 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed.  Thanks! 

Hi Ben, 

Thanks again!

> ::: dom/workers/ServiceWorkerManager.cpp
> @@ +1513,5 @@
> > +
> > +    windows.AppendElement(doc->InnerWindowID());
> > +
> > +    aReporter->FlushConsoleReports(doc,
> > +                                   ConsoleReportCollector::ReportAction::Save);
> 
> nit: I think you could shorten this to aReporter::ReportAction::Save.  Here
> and elsewhere in the patch.

I'm not sure what you meant. I will get build failed if I do so. I change them into |nsIConsoleReportCollector::ReportAction::Save| in this patch.

> ::: dom/workers/WorkerPrivate.cpp
> @@ +3592,5 @@
> >  
> >  template <class Derived>
> > +void
> > +WorkerPrivateParent<Derived>::FlushReportToShraredWorkers(
> > +                                           nsIConsoleReportCollector* aReporter)
> 
> Have you at least tested this function locally to see if it works for a
> SharedWorker attached to two different windows?  Having an automated test
> would be ideal, but I just wanted to make sure we at least verified with
> local testing.

I did it and I could get expected results (flush reports to each of documents attaching to the sharedworker) when opening the browser console, but hit an assertion when opening the web console. 
Furthermore, I also found another testcase(a sharedworker with windows) will also hit this assertion. Thus, I file Bug 1284420 for that.

> @@ +3608,5 @@
> > +    // May be null.
> > +    nsPIDOMWindowInner* window = sharedWorker->GetOwner();
> > +
> > +    // Add the owning window to our list so that we will flush the reports later.
> > +    if (!windowActions.Contains(window)) {
> 
> If window is nullptr, we probably don't want to add it to windowActions,
> right?
Yes, I address that in this patch.
Attachment #8767122 - Attachment is obsolete: true
Assignee

Comment 97

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #94)
> Looks good!  I think we still need tests to verify event.request.integrity
> is set properly when a service worker intercepts <script integrity="foo">
> and <link rel="stylesheet" integrity="foo">.

There is a problem when I try to store script's and stylesheet's integrity into ServiceWorker. The problem is they store their integrity metadata in SRIMetadata and I store it in nsString for InternalRequst and ServiceWorker. The reason is it may need to be called by GetIntegrity() in Request and it's easier to test.

Currently, I have two simple solution for this problem. One is either to modify storing type in both script and stylesheet from SRIMetadata to nsString or modify storing type in InternalRequest and ServiceWorker. The other is to add a helper function (maybe ToString) in SRIMetadata. But I am not sure about which one is better.

How do you think about this, François? Any suggestion?
Flags: needinfo?(francois)
Assignee

Comment 98

3 years ago
Posted patch simple_local_test.patch (obsolete) — Splinter Review
Hi Ben,

This is a patch for sharedWokrer local test and it need to based on patch P5-part2. As I mention in comment #96, I can get error reports in browser console but get an assertion failure in web console. 

I am not sure about whether the test is as your expected. Could you take a look at this patch? Thanks!
Attachment #8767893 - Flags: feedback?(bkelly)
(In reply to Tom Tung [:tt] from comment #95)
> > > +  void
> > > +  SetIntegrity(const nsAString& aIntegrity)
> > > +  {
> > > +    mIntegrity.Assign(aIntegrity);
> > 
> > Is there every any reason the integrity value could change?  Can we
> > MOZ_ASSERT(mIntegrity.IsEmpty()) here?
> 
> I did similar thing (like [1]) here due to step 24 [2]. Hence, the integrity
> could be changed. I think it could be passed a null string?  If it can, we
> cannot put this assert here.
> 
> [1] http://searchfox.org/mozilla-central/source/dom/fetch/Request.cpp#441-443
> [2] https://fetch.spec.whatwg.org/#dom-request

I think it should be safe to put the MOZ_ASSERT(mIntegrity.IsEmpty()) here.  Generally this SetIntegrity() method should only be used when initializing a new InternalRequest.  For now its an error to change the integrity value once its set.

(In reply to Tom Tung [:tt] from comment #97)
> (In reply to Ben Kelly [:bkelly] from comment #94)
> > Looks good!  I think we still need tests to verify event.request.integrity
> > is set properly when a service worker intercepts <script integrity="foo">
> > and <link rel="stylesheet" integrity="foo">.
> 
> There is a problem when I try to store script's and stylesheet's integrity
> into ServiceWorker. The problem is they store their integrity metadata in
> SRIMetadata and I store it in nsString for InternalRequst and ServiceWorker.
> The reason is it may need to be called by GetIntegrity() in Request and it's
> easier to test.

I think you can just use SRIMetadata::GetHash() and concatenate each hash together separated by spaces.  This is the format specified in the SRI spec and what the SRIMetadata constructor expects:

https://dxr.mozilla.org/mozilla-central/source/dom/security/SRIMetadata.cpp#42

As you said, this could be added as a simple ToString() on SRIMetadata as well.
Comment on attachment 8767893 [details] [diff] [review]
simple_local_test.patch

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

I guess this is useful for checking how we handle a SharedWorker attached to two windows, but its not quite what I was going for.

It would be really nice if we could validate the logging actually happens.  Do to this you would probably need a browser-chrome test like this:

https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/test/test_console_serviceworker.html

Also, can you check the console output locally in a non-debug build?  Maybe the logging will work even though we are hitting an unexpected assertion if you turn assertions off.
Attachment #8767893 - Flags: feedback?(bkelly)
Assignee

Comment 101

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #99)
> I think it should be safe to put the MOZ_ASSERT(mIntegrity.IsEmpty()) here. 
> Generally this SetIntegrity() method should only be used when initializing a
> new InternalRequest.  For now its an error to change the integrity value
> once its set.

Oh, I see. I misunderstood that you ask me to put the MOZ_ASSERT('a'Integrity.IsEmpty()), and I was wrong.
Sure, it's ok to put the MOZ_ASSERT(mIntegrity.IsEmpty()) here. I'll apply that in next patch.

> I think you can just use SRIMetadata::GetHash() and concatenate each hash
> together separated by spaces.  This is the format specified in the SRI spec
> and what the SRIMetadata constructor expects:
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/security/SRIMetadata.
> cpp#42
> 
> As you said, this could be added as a simple ToString() on SRIMetadata as
> well.

I just find that SRIMetadata may modify the original integrity metadata when its value is invalid. For example, a integrity metadata with a question mark will be truncated when store into class SRIMetadata [1]. Do we need to take care about these cases (e.g. [2])? 

If we need, maybe store them in nsString at class Loader/ScriptLoader is better? If it's not necessary, ToString() is enough since we just need to store the valid value.  

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/SRIMetadata.cpp?q=SRIMetadata.cpp&redirect_type=direct#48-60
[2] https://dxr.mozilla.org/mozilla-central/source/dom/security/test/sri/iframe_script_sameorigin.html#133
Assignee

Comment 102

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #100)
> Comment on attachment 8767893 [details] [diff] [review]
> simple_local_test.patch
> 
> Review of attachment 8767893 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess this is useful for checking how we handle a SharedWorker attached to
> two windows, but its not quite what I was going for.
> 
> It would be really nice if we could validate the logging actually happens. 
> Do to this you would probably need a browser-chrome test like this:
> 
> https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/
> test/test_console_serviceworker.html

Sure, I'll do that latter.
 
> Also, can you check the console output locally in a non-debug build?  Maybe
> the logging will work even though we are hitting an unexpected assertion if
> you turn assertions off.

Sure, I have checked the console output in a non-debug build and each of messages is presented in twice(one for whole html, the other for iframe). I think the behavior is shown as expected?
Assignee

Comment 103

3 years ago
(In reply to Tom Tung [:tt] from comment #101)
> I just find that SRIMetadata may modify the original integrity metadata when
> its value is invalid. For example, a integrity metadata with a question mark
> will be truncated when store into class SRIMetadata [1]. Do we need to take
> care about these cases (e.g. [2])? 

I found that I can get the entire integrity attribute for nsIContent object. I'm working on this way and the script part is done. For stylesheet, I still need to find out the current Element for getting nsIContent object.
(In reply to Tom Tung [:tt] from comment #103)
> (In reply to Tom Tung [:tt] from comment #101)
> > I just find that SRIMetadata may modify the original integrity metadata when
> > its value is invalid. For example, a integrity metadata with a question mark
> > will be truncated when store into class SRIMetadata [1]. Do we need to take
> > care about these cases (e.g. [2])? 
> 
> I found that I can get the entire integrity attribute for nsIContent object.
> I'm working on this way and the script part is done. For stylesheet, I still
> need to find out the current Element for getting nsIContent object.

I think we only need to set the integrity value on the nsIChannel before it does its AsyncOpen() or AsyncOpen2().  This should happen before the script/stylesheet code checks to see if the integrity matches.  So before OnStartRequest().
Also, Andrew is adding a new console reporting test in bug 1267473.  It seems like maybe you could copy that for a console test in SRI.  Its much simpler than the devtools test.
(In reply to Tom Tung [:tt] from comment #101)
> (In reply to Ben Kelly [:bkelly] from comment #99)
> > I think you can just use SRIMetadata::GetHash() and concatenate each hash
> > together separated by spaces.  This is the format specified in the SRI spec
> > and what the SRIMetadata constructor expects:
> > 
> > https://dxr.mozilla.org/mozilla-central/source/dom/security/SRIMetadata.
> > cpp#42
> > 
> > As you said, this could be added as a simple ToString() on SRIMetadata as
> > well.
> 
> I just find that SRIMetadata may modify the original integrity metadata when
> its value is invalid. For example, a integrity metadata with a question mark
> will be truncated when store into class SRIMetadata [1]. Do we need to take
> care about these cases (e.g. [2])? 
> 
> If we need, maybe store them in nsString at class Loader/ScriptLoader is
> better? If it's not necessary, ToString() is enough since we just need to
> store the valid value.  

The invalid parts need to be ignored (as per the spec) so I think you can just leave them out. If these parts ever become valid, we'll change SRIMetadata to stop discarding them.
Flags: needinfo?(francois)
Comment on attachment 8763005 [details] [diff] [review]
Bug 1187335 - P2-part1 - Modify the way to report to console for worker and use LoadTainting to decide CORS or not. r=bkelly.

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

That looks good, I just pointed out a few minor things.

::: dom/security/SRICheck.cpp
@@ +17,3 @@
>  #include "nsIProtocolHandler.h"
>  #include "nsIScriptError.h"
>  #include "nsIScriptSecurityManager.h"

Do we still need this include now that the Same-Origin check no longers uses the security manager?

@@ +31,5 @@
>  namespace mozilla {
>  namespace dom {
>  
> +static void
> +ReportToConsole(uint32_t aErrorFlags, const nsACString& aCategory,

The category is always "Sub-resource integrity", so we may as well remove that parameter and hard-code that string into the function.

@@ +33,5 @@
>  
> +static void
> +ReportToConsole(uint32_t aErrorFlags, const nsACString& aCategory,
> +                nsContentUtils::PropertiesFile aFile, const char *aMessageName,
> +                const nsACString& aParams, nsIConsoleReportCollector* aReporter)

While we don't currently pass in more than one parameter to nsContentUtils::ReportToConsole(), it's possible we'll do that in the future if we want to improve the error messages.

So I would prefer if "aParams" were an array of strings.

@@ +348,5 @@
>    nsresult rv = Finish();
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  nsCOMPtr<nsILoadInfo> loadInfo = aChannel->GetLoadInfo();
> +  LoadTainting tainting = loadInfo->GetTainting();

Please add a NULL check before this line.

NS_ENSURE_TRUE(loadInfo, <some appropriate NS_ERROR_ code>);

::: dom/security/SRICheck.h
@@ +8,3 @@
>  #define mozilla_dom_SRICheck_h
>  
>  #include "mozilla/CORSMode.h"

Do we still need this include?
Attachment #8763005 - Flags: review?(francois) → review-
Attachment #8763006 - Flags: review?(francois) → review+
Assignee

Comment 108

3 years ago
(In reply to François Marier [:francois] from comment #107)
Hi  François,

  Thanks for your feedback! I addressed them all and rebased the patch to the newest inbound. Could you help me to review this patch, again? Thanks!
Attachment #8763005 - Attachment is obsolete: true
Attachment #8769725 - Flags: review?(francois)
Assignee

Comment 109

3 years ago
Rebased the patch since there are few changes in ServiceWorkerManager.
Attachment #8767879 - Attachment is obsolete: true
Assignee

Comment 110

3 years ago
Hi Ben,

This patch is to add the integrity metadata's script/css loader into serviceWorker via HttpChannelInternal. Could you help me to review this patch? Thanks! 

Note:
  1. The integrity metadata is get from SRIMetadata. 
  2. Due to 1, the value of integrity metadata is processed by SRICheck::IntegrityMetadata(...) and the test will show the differences between whole integrity metadata and processed metadata.
  3. Since 1 (which is different from the patch for Fetch API), I will change the implementation in patch for Fetch API if this patch gets r+.
Attachment #8769740 - Flags: review?(bkelly)
Assignee

Comment 111

3 years ago
(In reply to François Marier [:francois] from comment #106)
> The invalid parts need to be ignored (as per the spec) so I think you can
> just leave them out. If these parts ever become valid, we'll change
> SRIMetadata to stop discarding them.

Thanks for the comment and sorry for missing that. 
I decided to utilized the value stored in SRIMetadata and covert it back to nsString in order to store in HTTPChannelInternal. So, I added a ToString() function in that patch(attachment 8769740 [details] [diff] [review]).
Comment on attachment 8769740 [details] [diff] [review]
Bug 1187335 - P4-part3 - Support script/css to set integrity metadata to serviceWorker.

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

Looks good, but I think we could refactor the ToString() a bit cleaner.

::: dom/security/SRIMetadata.cpp
@@ +171,5 @@
> +  MOZ_ASSERT(aString.IsEmpty()); // caller should pass empty string
> +  aString.Truncate();
> +
> +  if (!mAlgorithm.IsEmpty()) {
> +    aString.Assign(NS_ConvertUTF8toUTF16(mAlgorithm) + NS_LITERAL_STRING("-"));

If all the internal representations are in nsCString, then I suggest just making ToString() write to an nsACString&.  The caller can convert to UTF16 if they need to.

@@ +174,5 @@
> +  if (!mAlgorithm.IsEmpty()) {
> +    aString.Assign(NS_ConvertUTF8toUTF16(mAlgorithm) + NS_LITERAL_STRING("-"));
> +  }
> +
> +  if (mHashes.Length() == 0) {

So if hashes is zero we might append the "algorithm-" with nothing after it?  That seems wrong.

Can we instead make a helper routine like:

AppendHashString(nsACString& aString)
{
  // append alorithm if necessary
  // append hash
}

Then you can just loop over mHashes and call AppendHashString() on each one.

::: testing/web-platform/tests/service-workers/service-worker/fetch-request-resources.https.html
@@ +61,5 @@
>      };
>    return frame.contentWindow.load_font(actual_url);
>  }
>  
> +function script_integrity_test(frame, url, integrity, expexted_integrity) {

s/expexted/expected/g

::: testing/web-platform/tests/service-workers/service-worker/resources/fetch-request-resources-iframe.https.html
@@ +50,5 @@
> +function load_script_with_integrity(url, integrity) {
> +  var script = document.createElement('script');
> +  script.src = url;
> +  if (integrity != '') {
> +    script.integrity = integrity;

Does it matter if you set an empty string integrity value?
Attachment #8769740 - Flags: review?(bkelly) → feedback+
Assignee

Comment 113

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #112)
Hi Ben,

Thanks for the feedback! 

> Comment on attachment 8769740 [details] [diff] [review]
> Bug 1187335 - P4-part3 - Support script/css to set integrity metadata to
> serviceWorker.
> 
> Review of attachment 8769740 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, but I think we could refactor the ToString() a bit cleaner.
> 
> ::: dom/security/SRIMetadata.cpp
> @@ +171,5 @@
> > +  MOZ_ASSERT(aString.IsEmpty()); // caller should pass empty string
> > +  aString.Truncate();
> > +
> > +  if (!mAlgorithm.IsEmpty()) {
> > +    aString.Assign(NS_ConvertUTF8toUTF16(mAlgorithm) + NS_LITERAL_STRING("-"));
> 
> If all the internal representations are in nsCString, then I suggest just
> making ToString() write to an nsACString&.  The caller can convert to UTF16
> if they need to.

Sure. It can reduce lots of string conversions.

> @@ +174,5 @@
> > +  if (!mAlgorithm.IsEmpty()) {
> > +    aString.Assign(NS_ConvertUTF8toUTF16(mAlgorithm) + NS_LITERAL_STRING("-"));
> > +  }
> > +
> > +  if (mHashes.Length() == 0) {
> 
> So if hashes is zero we might append the "algorithm-" with nothing after it?
> That seems wrong.

I check the mAlgorithm at first so it will happen only when we have an algorithm without any hash value. Here is existing example for that [1].

If the integrity metadata is an empty string, it will get empty integrity here rather than "algo-".

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/test/sri/iframe_script_sameorigin.html#195 

> Can we instead make a helper routine like:
> 
> AppendHashString(nsACString& aString)
> {
>   // append alorithm if necessary
>   // append hash
> }
> 
> Then you can just loop over mHashes and call AppendHashString() on each one.

Sure.

> :::
> testing/web-platform/tests/service-workers/service-worker/fetch-request-
> resources.https.html
> @@ +61,5 @@
> >      };
> >    return frame.contentWindow.load_font(actual_url);
> >  }
> >  
> > +function script_integrity_test(frame, url, integrity, expexted_integrity) {
> 
> s/expexted/expected/g

I just find out that we have same mistakes in other function. I'll correct them in next patch.
 
> :::
> testing/web-platform/tests/service-workers/service-worker/resources/fetch-
> request-resources-iframe.https.html
> @@ +50,5 @@
> > +function load_script_with_integrity(url, integrity) {
> > +  var script = document.createElement('script');
> > +  script.src = url;
> > +  if (integrity != '') {
> > +    script.integrity = integrity;
> 
> Does it matter if you set an empty string integrity value?

Nope, I'll remove the if condition to be more clearly.
Assignee

Comment 114

3 years ago
I addressed the comment expect few of them and I put my reason in comment #113. Would you mind reviewing this patch, Ben? Thanks!
Attachment #8769740 - Attachment is obsolete: true
Attachment #8770374 - Flags: review?(bkelly)
Comment on attachment 8770374 [details] [diff] [review]
Bug 1187335 - P4-part3 - Support script/css to set integrity metadata to serviceWorker.

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

So, the "algo-" thing made me look at the spec more closely.  I don't think this approach is correct any more.  Sorry for sending you down the wrong path here!

::: dom/security/SRIMetadata.cpp
@@ +179,5 @@
> +  aString.Append(mHashes[aIndex]);
> +}
> +
> +void
> +SRIMetadata::ToString(nsACString& aString) const

Actually, looking at the spec closer I think this is the wrong approach.

The fetch spec does not actually parse the integrity metadata until step 18.2 of Main Fetch.  This is after the network request has been made and a Response is available.

So I believe the service worker FetchEvent.request.integrity should reflect the exact value set in the content integrity attribute.  It should not be the result of the parsing operation which discards invalid values.

So I think we need to save the original integrity attribute token string somewhere and just return it.

Sorry for my earlier confusion and sending you on this wrong path.
Attachment #8770374 - Flags: review?(bkelly) → review-
Comment on attachment 8769725 [details] [diff] [review]
Bug 1187335 - P2-part1 - Modify the way to report to console for worker and use LoadTainting to decide CORS or not. r=bkelly.

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

::: dom/security/SRICheck.cpp
@@ +32,5 @@
>  
> +static void
> +ReportToConsole(uint32_t aErrorFlags, nsContentUtils::PropertiesFile aFile,
> +                const char *aMessageName, nsTArray<nsString>& aParams,
> +                //const nsACString& aParams,

You should remove this line.
Attachment #8769725 - Flags: review?(francois) → review+
Assignee

Comment 117

3 years ago
(In reply to François Marier [:francois] from comment #116)
Hi François,

Thanks for your time and review!

> Comment on attachment 8769725 [details] [diff] [review]
> Bug 1187335 - P2-part1 - Modify the way to report to console for worker and
> use LoadTainting to decide CORS or not. r=bkelly.
> 
> Review of attachment 8769725 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/security/SRICheck.cpp
> @@ +32,5 @@
> >  
> > +static void
> > +ReportToConsole(uint32_t aErrorFlags, nsContentUtils::PropertiesFile aFile,
> > +                const char *aMessageName, nsTArray<nsString>& aParams,
> > +                //const nsACString& aParams,
> 
> You should remove this line.
Sorry for this. I forget to remove it.
Assignee

Comment 120

3 years ago
(In reply to Tom Tung [:tt] from comment #118)
> Created attachment 8771403 [details] [diff] [review]
> [Final] Bug 1187335 - P2-part1 - Modify the way to report to console for
> worker and use LoadTainting to decide CORS or not. r=bkelly, r=francois.
Addressed #116 comment and rebased the patch

(In reply to Tom Tung [:tt] from comment #119)
> Created attachment 8771406 [details] [diff] [review]
> [Final] Bug 1187335 - P3 - modify SRI test to match current behavior.
> r=bkelly, r=francois.
Modify header since it was finished.
Assignee

Comment 122

3 years ago
Hi Ben,

I addressed the comment #115 and your feedback in IRC. Could you help me to review this patch? Thanks!
Attachment #8770374 - Attachment is obsolete: true
Attachment #8771411 - Flags: review?(bkelly)
Assignee

Comment 123

3 years ago
Change the header since it have already get r+.
Attachment #8767124 - Attachment is obsolete: true
Comment on attachment 8771411 [details] [diff] [review]
Bug 1187335 - P4-part3 - Support script/css to set integrity metadata to serviceWorker.

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

Thanks!  r=me with comments addressed.

::: dom/security/SRIMetadata.h
@@ +60,5 @@
>    void GetHash(uint32_t aIndex, nsCString* outHash) const;
>    void GetAlgorithm(nsCString* outAlg) const { *outAlg = mAlgorithm; }
>    void GetHashType(int8_t* outType, uint32_t* outLength) const;
>  
> +  void SetOriginalIntegrityString(const nsAString& aString) { mOriginalIntegrityString.Assign(aString); };

Please don't expose a public setter like this.  I think we should just pass the original string to the constructor alongside the token being parsed.  We should be able to make mOriginalIntegrityString a const nsString.

::: testing/web-platform/tests/service-workers/service-worker/fetch-request-resources.https.html
@@ +168,5 @@
>  
> +        script_integrity_test(f, LOCAL_URL, '     ', '     ');
> +        script_integrity_test(f, LOCAL_URL, 'sha256-foo?123', 'sha256-foo?123');
> +        script_integrity_test(f, LOCAL_URL, 'sha256-foo sha384-abc', 'sha256-foo sha384-abc');
> +        script_integrity_test(f, LOCAL_URL, 'sha256-foo sha256-abc', 'sha256-foo sha256-abc');

Can you add a script and css test case for a completely garbage integrity value.  Something like "this is not a valid integrity because it has no dashes".
Attachment #8771411 - Flags: review?(bkelly) → review+
Assignee

Comment 126

3 years ago
Hi Ben,

I addressed the comment, but I'm afraid of misunderstanding what you meant. Thus, could you help me to review it again? Thanks!
Attachment #8771411 - Attachment is obsolete: true
Attachment #8771885 - Flags: review?(bkelly)
Comment on attachment 8771885 [details] [diff] [review]
Bug 1187335 - P4-part3 - Support script/css to set integrity metadata to serviceWorker.

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

r=me with comments addressed.

::: dom/base/nsScriptLoader.cpp
@@ +1362,5 @@
>  
> +      nsAutoString integrity;
> +      scriptContent->GetAttr(kNameSpaceID_None, nsGkAtoms::integrity,
> +                             integrity);
> +      SRIMetadata sriMetadata = SRIMetadata(integrity);

See my comment in SRIMetadata file.  I don't think this change is necessary.

@@ +2608,5 @@
>    if (nsContentUtils::IsChromeDoc(mDocument) && aType.LowerCaseEqualsASCII("module")) {
>      return;
>    }
>  
> +  SRIMetadata sriMetadata = SRIMetadata(aIntegrity);

See my comment in SRIMetadata file.  I don't think this change is necessary.

::: dom/security/SRIMetadata.h
@@ +26,5 @@
>  
>    /**
> +   * Create an empty metadata object with an integrity metadata.
> +   */
> +  SRIMetadata(const nsAString& aIntegrity)

Is this constructor really necessary?  Can't we just let mOriginalIntegrityString initialize to the empty string?  When SRICheck::IntegrityMetadata() assigns its own SRIMetadata to the out param then it should overwrite the mOriginalIntegrityString with the correct value.

https://dxr.mozilla.org/mozilla-central/source/dom/security/SRICheck.cpp#160

@@ +77,5 @@
>    nsCString mAlgorithm;
>    int8_t mAlgorithmType;
>    bool mEmpty;
> +
> +  nsString mOriginalIntegrityString;

const nsString

::: layout/style/Loader.cpp
@@ +1257,5 @@
>        sheetURI = aURI;
>        originalURI = aURI;
>      }
>  
> +    SRIMetadata sriMetadata = SRIMetadata(aIntegrity);

See my comment in SRIMetadata file.  I don't think this change is necessary.
Attachment #8771885 - Flags: review?(bkelly) → review+
Comment on attachment 8771885 [details] [diff] [review]
Bug 1187335 - P4-part3 - Support script/css to set integrity metadata to serviceWorker.

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

r=me with comments addressed.

::: dom/base/nsScriptLoader.cpp
@@ +1362,5 @@
>  
> +      nsAutoString integrity;
> +      scriptContent->GetAttr(kNameSpaceID_None, nsGkAtoms::integrity,
> +                             integrity);
> +      SRIMetadata sriMetadata = SRIMetadata(integrity);

See my comment in SRIMetadata file.  I don't think this change is necessary.

@@ +2608,5 @@
>    if (nsContentUtils::IsChromeDoc(mDocument) && aType.LowerCaseEqualsASCII("module")) {
>      return;
>    }
>  
> +  SRIMetadata sriMetadata = SRIMetadata(aIntegrity);

See my comment in SRIMetadata file.  I don't think this change is necessary.

::: dom/security/SRIMetadata.h
@@ +26,5 @@
>  
>    /**
> +   * Create an empty metadata object with an integrity metadata.
> +   */
> +  SRIMetadata(const nsAString& aIntegrity)

Is this constructor really necessary?  Can't we just let mOriginalIntegrityString initialize to the empty string?  When SRICheck::IntegrityMetadata() assigns its own SRIMetadata to the out param then it should overwrite the mOriginalIntegrityString with the correct value.

https://dxr.mozilla.org/mozilla-central/source/dom/security/SRICheck.cpp#160

@@ +77,5 @@
>    nsCString mAlgorithm;
>    int8_t mAlgorithmType;
>    bool mEmpty;
> +
> +  nsString mOriginalIntegrityString;

But ignore my suggestion to make mOriginalIntegrityString const.  It needs to be assignable.

::: layout/style/Loader.cpp
@@ +1257,5 @@
>        sheetURI = aURI;
>        originalURI = aURI;
>      }
>  
> +    SRIMetadata sriMetadata = SRIMetadata(aIntegrity);

See my comment in SRIMetadata file.  I don't think this change is necessary.
Sorry for the duplicate review output there.  I was trying to add just the additional comment about not making mOriginalIntegrityString const.
Assignee

Comment 130

3 years ago
Addressed the comment #128 and assigning the integrity string when the metadata is invalid and empty as we discuss in IRC. 

This patch is mainly to let css and script pass exact integrity metadata to service worker through HttpChannelInternal. To do this, I did few things and I put them below inline.
   1. modify SRIMetadata to let it be able to carry an originalIntegrityString.
   2. modify SRICheck::IntegrityMetadata(..) to make a SRIMetadata carry an originalIntegrityString
   3. set integrity metadata into httpChannelInternal in nsScript.cpp and Loader.cpp
   4. write few simple tests for it.
Attachment #8771885 - Attachment is obsolete: true
Assignee

Comment 131

3 years ago
Comment on attachment 8772258 [details] [diff] [review]
Bug 1187335 - P4-part3 - Support script/css to set integrity metadata to serviceWorker. r=bkelly.

Hi François,

As I mention in comment #130, I did few changes in nsScriptLoader, Loader, SRIMetadata and SRICheck to pass exact integrity string to service worker. Could you help to review this patch? Thanks!
Attachment #8772258 - Flags: review?(francois)
Comment on attachment 8772258 [details] [diff] [review]
Bug 1187335 - P4-part3 - Support script/css to set integrity metadata to serviceWorker. r=bkelly.

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

I feel like this change introduces too many unnecessary string copies. Every token in the loop of SRICheck::IntegrityMetadata() will create a copy the whole integrity attribute just to (probably) throw it away later.

Correct me if I'm wrong, but what you really need is for IntegrityMetadata() to set mOriginalIntegrity just before it returns (https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/dom/security/SRICheck.cpp#175):

    outMetadata->mOriginalIntegrityString = aMetadataList;
    return NS_OK;

If you make that change, you probably won't need the new constructors.

::: dom/security/SRIMetadata.h
@@ +79,5 @@
>    nsCString mAlgorithm;
>    int8_t mAlgorithmType;
>    bool mEmpty;
> +
> +  nsString mOriginalIntegrityString;

You should maintain the size order here (smallest member variable at the end). mOriginalIntegrityString should go in between mHashes and mAlgorithm.

Also (nit), why not just mIntegrityString?
Attachment #8772258 - Flags: review?(francois) → review-
(In reply to François Marier [:francois] from comment #132)
> I feel like this change introduces too many unnecessary string copies. Every
> token in the loop of SRICheck::IntegrityMetadata() will create a copy the
> whole integrity attribute just to (probably) throw it away later.
> 
> Correct me if I'm wrong, but what you really need is for IntegrityMetadata()
> to set mOriginalIntegrity just before it returns
> (https://dxr.mozilla.org/mozilla-central/rev/
> 4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/dom/security/SRICheck.cpp#175):
> 
>     outMetadata->mOriginalIntegrityString = aMetadataList;
>     return NS_OK;

I'm ok with this as long as we don't have a public setter on SRIMetadata.

Keep in mind, though, that we have copy-on-write strings.  We're not actually copying the entire contents of the string here.  Its just copying a reference to the string buffer.
(In reply to Ben Kelly [:bkelly] from comment #133)
> (In reply to François Marier [:francois] from comment #132)
> >     outMetadata->mOriginalIntegrityString = aMetadataList;
> >     return NS_OK;
> 
> I'm ok with this as long as we don't have a public setter on SRIMetadata.

Indeed, we should avoid that. I was thinking it would be reasonable to make SRICheck a friend class of SRIMetadata.

> Keep in mind, though, that we have copy-on-write strings.  We're not
> actually copying the entire contents of the string here.  Its just copying a
> reference to the string buffer.

Ah good point.
Assignee

Comment 135

3 years ago
(In reply to François Marier [:francois] from comment #132)
Hi François,

Thanks for your review and your feedback! I learn a lot from them!

> Comment on attachment 8772258 [details] [diff] [review]
> Bug 1187335 - P4-part3 - Support script/css to set integrity metadata to
> serviceWorker. r=bkelly.
> 
> Review of attachment 8772258 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I feel like this change introduces too many unnecessary string copies. Every
> token in the loop of SRICheck::IntegrityMetadata() will create a copy the
> whole integrity attribute just to (probably) throw it away later.

Yeah, I forgot that we could set private class variable via becoming its friend class without a public setter. Making SRICheck a friend class of SRIMetadata is a good idea to solve this problem. 
I'll address that in next patch!

> Correct me if I'm wrong, but what you really need is for IntegrityMetadata()
> to set mOriginalIntegrity just before it returns
> (https://dxr.mozilla.org/mozilla-central/rev/
> 4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/dom/security/SRICheck.cpp#175):
> 
>     outMetadata->mOriginalIntegrityString = aMetadataList;
>     return NS_OK;
> 
> If you make that change, you probably won't need the new constructors.
> 
> ::: dom/security/SRIMetadata.h
> @@ +79,5 @@
> >    nsCString mAlgorithm;
> >    int8_t mAlgorithmType;
> >    bool mEmpty;
> > +
> > +  nsString mOriginalIntegrityString;
> 
> You should maintain the size order here (smallest member variable at the
> end). mOriginalIntegrityString should go in between mHashes and mAlgorithm.

Sure!

> Also (nit), why not just mIntegrityString?

It's okay for me to name it "mIntegrityString" and I'll apply it to next patch. I thought "mOriginalIntegrityString" may be clearer to stand for whole integrity string but it's too long.
Assignee

Comment 136

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #133)
Thanks for reminding me that, Ben. I would like to address comment #134 and we'll not have a public setter in this method.
Assignee

Comment 137

3 years ago
Hi François,

I address the comment #132 and #134. Could you help me to review this again? Thanks!
Attachment #8772258 - Attachment is obsolete: true
Attachment #8773129 - Flags: review?(francois)
See Also: → 1212979
Comment on attachment 8771406 [details] [diff] [review]
[Final] Bug 1187335 - P3 - modify SRI test to match current behavior. r=bkelly, r=francois.

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

I missed two things in my previous review of this patch.

I think we also need to change this test so that it's expected to pass:

https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_style_crossdomain.html#15

And wouldn't your changes to the eligibility algorithm also affect these test cases?

https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_script_crossdomain.html#61-72
Attachment #8771406 - Flags: review-
Attachment #8773129 - Flags: review?(francois) → review+
Assignee

Comment 139

3 years ago
(In reply to François Marier [:francois] from comment #138)

Sorry for the late reply, I somehow miss this comment. :(

> Comment on attachment 8771406 [details] [diff] [review]
> [Final] Bug 1187335 - P3 - modify SRI test to match current behavior.
> r=bkelly, r=francois.
> 
> Review of attachment 8771406 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I missed two things in my previous review of this patch.
> 
> I think we also need to change this test so that it's expected to pass:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/
> iframe_style_crossdomain.html#15

I'm not sure what should I do. I guess I should remove |,.red-text{color:red}| in [1]? Could you give me some hints about that, François? Thanks! 

[1] https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_style_crossdomain.html#79

> And wouldn't your changes to the eligibility algorithm also affect these
> test cases?
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/
> iframe_script_crossdomain.html#61-72

Oh, they should be changed. I forgot [2] because it is marked as "todo". 
Thanks for reminding me that but I found one thing interesting. I thought [1] should be good_correctDataLoaded(), but I found its LoadTainting is opaque and still try to find out why... 

[1] https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_script_crossdomain.html#61-66
[2] https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_script_crossdomain.html#67-72
Flags: needinfo?(francois)
(In reply to Tom Tung [:tt] from comment #139)
> (In reply to François Marier [:francois] from comment #138)
> > I think we also need to change this test so that it's expected to pass:
> > 
> > https://dxr.mozilla.org/mozilla-central/rev/
> > db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/
> > iframe_style_crossdomain.html#15
> 
> I'm not sure what should I do. I guess I should remove
> |,.red-text{color:red}| in [1]? Could you give me some hints about that,
> François? Thanks! 
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/
> iframe_style_crossdomain.html#79

That particular one is already blocked:

  https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_style_crossdomain.html#45-50

so the ",.red-text{color:red}" will not be applied.

However, this one is not currently blocked:

  https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_style_crossdomain.html#51-56

and it also tries to apply ",.red-text{color:red}".

So I think all we need to do is to change the "todo()" to "ok()" on both of these lines:

  https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_style_crossdomain.html#15
  https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_style_crossdomain.html#55
Flags: needinfo?(francois)
Assignee

Comment 141

3 years ago
(In reply to François Marier [:francois] from comment #140)

Thanks for feedback, François! But it's still not clear to me. I might misunderstand something. :(

> (In reply to Tom Tung [:tt] from comment #139)
> That particular one is already blocked: 
>  
> https://dxr.mozilla.org/mozilla-central/rev/
> db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/
> iframe_style_crossdomain.html#45-50
> so the ",.red-text{color:red}" will not be applied.

I think I may misunderstand something. Please correct me if I'm wrong. 
This testcase should be loaded since it is same-origin [1]. Thus, ",.red-text{color:red}" should be applied and so does #51-56. We've discussed that in comment #30 and it is the reason why I wrote this patch. Hence, I don't think only changing from "todo()" to "ok()" will be correct.

[1] https://github.com/whatwg/html/issues/1243

However, it's strange to me that I find out the "LoadTainting" in [2] is opaque since I reckon it should be same-origin and the testcase [3] has proved that with the "LoadTainting" is basic. There may exist a bug for "LoadTainting" because either same-origin or cross-origin cannot have correct "LoadTainting" value?

[2] https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_script_crossdomain.html#115-119
[3] https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_script_crossdomain.html#121-126
Flags: needinfo?(francois)
(In reply to Tom Tung [:tt] from comment #141)
> I think I may misunderstand something. Please correct me if I'm wrong. 
> This testcase should be loaded since it is same-origin [1]. Thus,
> ",.red-text{color:red}" should be applied and so does #51-56. We've
> discussed that in comment #30 and it is the reason why I wrote this patch.
> Hence, I don't think only changing from "todo()" to "ok()" will be correct.

You're right. This entire test was written while the spec was wrong.

So I think instead what we need to do is change it to something like this:

      var redText = document.getElementById('red-text');
      var greenText = document.getElementById('green-text');
      var blueText = document.getElementById('blue-text');
      var redTextColor = window.getComputedStyle(redText, null).getPropertyValue('color');
      var greenTextColor = window.getComputedStyle(greenText, null).getPropertyValue('color');
      var blueTextColor = window.getComputedStyle(blueText, null).getPropertyValue('color');

      ok(redTextColor == 'rgb(255, 0, 0)', "The first part should be red.");
      ok(greenTextColor == 'rgb(0, 255, 0)', "The second part should be green.");
      ok(blueTextColor == 'rgb(0, 0, 255)', "The third part should be blue.");


<p><span id="red-text">This should be red</span>, 
  <span id="green-text">this should be green</span> and
  <span id="blue-text">this should be blue</span>.</p>


  <!-- valid non-CORS sha256 hash in a data: URL -->
  <link rel="stylesheet" href="data:text/css,#green-text{color:green}"
        integrity="sha256-<updated hash>"

  <!-- valid CORS sha256 hash in a data: URL -->
  <link rel="stylesheet" href="data:text/css,#blue-text{color:blue}"
        crossorigin="anonymous"
        integrity="sha256-<updated hash>"

to account for the fact that both data: URIs will get loaded properly now.
(In reply to Tom Tung [:tt] from comment #141)
> However, it's strange to me that I find out the "LoadTainting" in [2] is
> opaque since I reckon it should be same-origin and the testcase [3] has
> proved that with the "LoadTainting" is basic. There may exist a bug for
> "LoadTainting" because either same-origin or cross-origin cannot have
> correct "LoadTainting" value?
> 
> [2]
> https://dxr.mozilla.org/mozilla-central/rev/
> db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/
> iframe_script_crossdomain.html#115-119
> [3]
> https://dxr.mozilla.org/mozilla-central/rev/
> db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/
> iframe_script_crossdomain.html#121-126

Right, I would expect stylesheets and scripts to behave in the same way here. So we should make the same changes to the tests and then fix the underlying bug.
Flags: needinfo?(francois)
Assignee

Comment 144

3 years ago
(In reply to François Marier [:francois] from comment #142)
(In reply to François Marier [:francois] from comment #143)
Hi François,

Thanks for the feedback and finding out missing part in this patch. Could you help me to review this patch? Thanks! I will upload the interdiff patch for easier to review.

In this patch, I addressed your comment except the |xxx-text{color:xxx}| one. The reason is that green is not the same as rgb(0, 255, 0) at css rule [1] so I use |color:rgb(0, 255, 0)| directly. Besides, I leave [2] and mark it as todo. 

[1] http://www.w3schools.com/cssref/css_colors.asp 
[2] https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_script_crossdomain.html#115-119
Attachment #8771406 - Attachment is obsolete: true
Attachment #8778037 - Flags: review?(francois)
Assignee

Comment 145

3 years ago
Posted file interdiff-P3.txt (obsolete) —
interdiff patch for P3.
Comment on attachment 8778037 [details] [diff] [review]
Bug 1187335 - P3 - modify SRI test to match current behavior. r=bkelly, r?francois.

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

r+ once you fix these two small issues.

::: dom/security/test/sri/iframe_script_crossdomain.html
@@ +69,3 @@
>    }
> +  function bad_correctDataCORSBlocked() {
> +    ok(false, "We should not load scripts in data: URIs regardless of CORS mode!");

I think you meant to say "We should not BLOCK scripts" here.

::: dom/security/test/sri/iframe_style_crossdomain.html
@@ +13,5 @@
>        var redTextColor = window.getComputedStyle(redText, null).getPropertyValue('color');
> +      var greenTextColor = window.getComputedStyle(greenText, null).getPropertyValue('color');
> +      var blueTextColor = window.getComputedStyle(blueText, null).getPropertyValue('color');
> +      console.log(redTextColor);
> +      console.log(greenTextColor);

Did you forget to remove these console log messages?
Attachment #8778037 - Flags: review?(francois) → review+
Assignee

Comment 148

3 years ago
(In reply to François Marier [:francois] from comment #147)
Thanks for your review and feedback, François! I addressed the comment in this patch.
Attachment #8778037 - Attachment is obsolete: true
Assignee

Comment 149

3 years ago
Add a patch for testing error report in serviceworker and sharedworker.
Assignee

Comment 150

3 years ago
Sorry for modifying this patch.
I found I miss the flow for report error to serviceworker from InterceptedChannel, so remove [Final] tag and rewrite the patch. I added few function to achieve that and the patch is almost done. I'll send to review again once I finish it.
Attachment #8769727 - Attachment is obsolete: true
Assignee

Comment 151

3 years ago
Hi Ben,

Sorry for delaying so long. I was thinking which file(filename) is much reasonable to send error reports. However, I realized that I should match the behavior as |ServiceWorkerManager::ReportToAllClients|.

I modified this patch to follow the behavior as |ServiceWorkerManager::ReportToAllClients|. Since I forgot to send report to InterceptedChannel, this patch is mainly add function to achieve that. Could you help me to review this patch? Thanks! I'll send a inter-diff patch to make this patch easier to review.
Attachment #8779278 - Attachment is obsolete: true
Attachment #8780049 - Flags: review?(bkelly)
Assignee

Comment 152

3 years ago
Posted file interdiff-P1 (obsolete) —
Inter-diff patch for P1.
Assignee

Comment 153

3 years ago
Posted file interdiff-P1.txt (obsolete) —
Sorry for sending wrong file. Resend the correct one.
Attachment #8780050 - Attachment is obsolete: true
Comment on attachment 8780049 [details] [diff] [review]
Bug 1187335 - P1 - Add FlushReportToAllClient() to ServiceWorkerManager and SharedWorker.

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

::: dom/console/ConsoleReportCollector.cpp
@@ +108,5 @@
>  }
>  
> +void
> +ConsoleReportCollector::FlushReportsByWindowId(uint64_t aWindowId,
> +                                               const nsString& aFilename,

The file name belongs to the individual reports.  You could have many reports coming from different files.  I don't understand why we are overriding the filename here.
Attachment #8780049 - Flags: review?(bkelly) → review-
Assignee

Comment 155

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #154)
> The file name belongs to the individual reports.  You could have many
> reports coming from different files.  I don't understand why we are
> overriding the filename here.

Oh, I see. I thought the filename should be the same as the spec of destination document's URI [1] but I was wrong. It should be source document's URI.

So what I need to do is that modify P2-part1 (giving a valid SourceFileURI) and the part for replacing filename in this patch.

[1] http://searchfox.org/mozilla-central/source/dom/base/nsContentUtils.cpp#3562
Assignee

Comment 156

3 years ago
Hi Ben,

I remove the code for replace filename. Could you help me to review this patch? Thanks! I'll upload inter-diff patch.
Attachment #8780049 - Attachment is obsolete: true
Assignee

Comment 157

3 years ago
Posted file interdiff-P1.txt (obsolete) —
inter-diff between last r+ and current r? patch for P1.
Attachment #8780051 - Attachment is obsolete: true
Assignee

Comment 158

3 years ago
Hi Ben,

Sorry for asking to review this patch again. As I mention in comment 155, I think we need to input a valid sourceFileURI for ConsoleReportCollector. Thus, I revised this patch for make functions in SRICheck have a nsIDocument object as a argument. Could you help me to review this patch? Thanks! I'll upload the inter-diff patch for this patch, too.
Attachment #8771403 - Attachment is obsolete: true
Attachment #8780474 - Flags: review?(bkelly)
Assignee

Comment 159

3 years ago
Posted patch interdiff-P2-part1.txt (obsolete) — Splinter Review
interdiff patch for SRICheck patch.
Attachment #8780475 - Attachment is patch: true
I'm getting confused about what the actual patch queue looks like here.  Can you combine the different parts into single patches for stuff thats already been reviewed?  And then obsolete the old patches?
Comment on attachment 8780475 [details] [diff] [review]
interdiff-P2-part1.txt

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

::: b/dom/security/SRICheck.h
@@ +30,4 @@
>     * return the strongest supported hash.
>     */
>    static nsresult IntegrityMetadata(const nsAString& aMetadataList,
> +                                    const nsIDocument* aDocument,

We went to the trouble of adding the ConsoleReportCollector because there isn't always a document.  It seems dubious to add the document back now.

Can we pass an nsIURI instead?  Code with a document can then pass doc->GetDocumentURI().  It would also let workers pass their worker script URL if there is no document available.
Attachment #8780474 - Flags: review?(bkelly)
Assignee

Comment 162

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #160)
> I'm getting confused about what the actual patch queue looks like here.  Can
> you combine the different parts into single patches for stuff thats already
> been reviewed?  And then obsolete the old patches?

Sure. Sorry for the confusing and changing patches recklessly.

(In reply to Ben Kelly [:bkelly] from comment #161)
> Comment on attachment 8780475 [details] [diff] [review]
> interdiff-P2-part1.txt
> 
> Review of attachment 8780475 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b/dom/security/SRICheck.h
> @@ +30,4 @@
> >     * return the strongest supported hash.
> >     */
> >    static nsresult IntegrityMetadata(const nsAString& aMetadataList,
> > +                                    const nsIDocument* aDocument,
> 
> We went to the trouble of adding the ConsoleReportCollector because there
> isn't always a document.  It seems dubious to add the document back now.
> 
> Can we pass an nsIURI instead?  Code with a document can then pass
> doc->GetDocumentURI().  It would also let workers pass their worker script
> URL if there is no document available.

Oh, I should use nsIURI instead of nsIDocument. Thanks for noticing me that sorry for trying to add the document back.
Assignee

Updated

3 years ago
Attachment #8767893 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8778038 - Attachment is obsolete: true
Assignee

Comment 163

3 years ago
Combine with different parts of r+ patches (P5-part1, P5-part2, P5-part3) for Patch 5.
Attachment #8762839 - Attachment is obsolete: true
Attachment #8767125 - Attachment is obsolete: true
Attachment #8767126 - Attachment is obsolete: true
Assignee

Comment 164

3 years ago
Combine with different parts of r+ patches (P4-part1, P4-part3) for Patch 4. Not adding [Final] flag here since SRI functions may be changed.
Attachment #8771880 - Attachment is obsolete: true
Attachment #8778063 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8778063 - Attachment is obsolete: false
Assignee

Comment 166

3 years ago
Comment on attachment 8771413 [details] [diff] [review]
[Final] Bug 1187335 - P4-part2 - Integrate cache with request.integrity. r=bkelly.

(In reply to Tom Tung [:tt] from comment #164)
> Created attachment 8780961 [details] [diff] [review]
> Integrate fetch and cache with SRI & add nsIConsoleReportCollector to show
> console report - fetch API changed. r=bkelly.
> 
> Combine with different parts of r+ patches (P4-part1, P4-part3) for Patch 4.
> Not adding [Final] flag here since SRI functions may be changed.

Should be P4-part1 and P4-part2.
Attachment #8771413 - Attachment is obsolete: true
Assignee

Comment 167

3 years ago
Hi Ben,

I rewrite this patch for adding nsIURI into functions in SRICheck since we need a correct SourceURI for each reports. Could you help me to review this patch? I'll update the inter-diff patch, too.
Attachment #8780474 - Attachment is obsolete: true
Attachment #8781047 - Flags: review?(bkelly)
Assignee

Comment 168

3 years ago
Posted file interdiff-P2-part1.txt (obsolete) —
This patch is inter-diff patch between current patch and the r+ one.
Attachment #8780475 - Attachment is obsolete: true
Comment on attachment 8781047 [details] [diff] [review]
Bug 1187335 - P2-part1 - Modify the way to report to console for worker and use LoadTainting to decide CORS or not.

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

Doesn't this require changes to fetch() as well?  I assume it needs to pass an nsIURI if it can as well.

Overall this is going in the right direction, but there seems to be some confusion over the handling of nullptr nsIURI here.  Currently this just doesn't perform SRI if there is no nsIDocument or DocumentURL().  That seems incorrect.

::: dom/security/SRICheck.cpp
@@ +43,5 @@
> +        params[i] = aParams[i].get();
> +      }
> +    }
> +
> +    // If we don't have a reporter, report log to broswer console.

When does this happen?  Can't we require all callers to pass a reporter?

@@ +55,5 @@
> +  nsCString messageName;
> +  messageName.AssignASCII(aMessageName);
> +
> +  nsCString sourceFileUri;
> +  aURI->GetAsciiSpec(sourceFileUri);

This needs to check for nullptr nsIURI.

@@ +131,2 @@
>  {
> +  NS_ENSURE_ARG_POINTER(aURI);

It seems we always require the nsIURI to be passed?  Looking at Loader.cpp, though, we can pass nullptr here.  Shouldn't these methods accomodate nullptr nsIURI values?

::: dom/security/SRICheck.h
@@ +29,5 @@
>     * Parse the multiple hashes specified in the integrity attribute and
>     * return the strongest supported hash.
>     */
>    static nsresult IntegrityMetadata(const nsAString& aMetadataList,
> +                                    nsIURI* aURI,

I think we should distinguish these arguments from the nsIChannel URI.  So maybe name them something like:

  nsISourceURI or
  nsITriggeringURI

::: layout/style/Loader.cpp
@@ +964,5 @@
>        return NS_OK;
>      }
>    } else {
> +    nsIURI* uri = mLoader->mDocument? mLoader->mDocument->GetDocumentURI()
> +                                    : nullptr;

For example, nullptr uri is passed here.
Attachment #8781047 - Flags: review?(bkelly) → review-
Assignee

Comment 171

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #170)

Thanks for the feedback! I'll address them in next patch but I have questions for few of them.

> Comment on attachment 8781047 [details] [diff] [review]
> Bug 1187335 - P2-part1 - Modify the way to report to console for worker and
> use LoadTainting to decide CORS or not.
> 
> Review of attachment 8781047 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Doesn't this require changes to fetch() as well?  I assume it needs to pass
> an nsIURI if it can as well.

Yes, you are right but I would like to finish this patch first(getting r+) and then modify the patch for fetch().
 
> Overall this is going in the right direction, but there seems to be some
> confusion over the handling of nullptr nsIURI here.  Currently this just
> doesn't perform SRI if there is no nsIDocument or DocumentURL().  That seems
> incorrect.

Yeah, it should work even with nullptr nsIURI. I'll correct it in next patch. 
 
> ::: dom/security/SRICheck.cpp
> @@ +43,5 @@
> > +        params[i] = aParams[i].get();
> > +      }
> > +    }
> > +
> > +    // If we don't have a reporter, report log to broswer console.
> 
> When does this happen?  Can't we require all callers to pass a reporter?

I just want to make SRI function work without needing to pass ConsoleReportCollector. It's okay to remove them but we may need to add assertions (e.g. MOZ_ASSERT(consoleReportCollector)) to each function?
 
> ::: dom/security/SRICheck.h
> @@ +29,5 @@
> >     * Parse the multiple hashes specified in the integrity attribute and
> >     * return the strongest supported hash.
> >     */
> >    static nsresult IntegrityMetadata(const nsAString& aMetadataList,
> > +                                    nsIURI* aURI,
> 
> I think we should distinguish these arguments from the nsIChannel URI.  So
> maybe name them something like:
> 
>   nsISourceURI or
>   nsITriggeringURI

You are right. We should distinguish them. I guess you meant aSourceURI or aTriggeringURI? I'll use aSourceURI in next patch.
Assignee

Comment 172

3 years ago
Hi Ben,

Could you help me to review this patch? Thanks! I address the comment and I'll provide interdiff patch between last r+one and this patch.
Attachment #8781047 - Attachment is obsolete: true
Attachment #8782792 - Flags: review?(bkelly)
Assignee

Comment 173

3 years ago
Posted file interdiff-P2-part1.txt (obsolete) —
interdiff patch for P2-part1
Attachment #8781059 - Attachment is obsolete: true
Assignee

Comment 174

3 years ago
Hi Ben,

Sorry for changing the review request and patch. I just find out that we should use nsCString rahter than nsIURI for souceFileURI since the workerScript [1] is nsString. So I write a patch and want to ask your feedback for this changes. Thanks!

[1] http://searchfox.org/mozilla-central/source/dom/workers/WorkerPrivate.h#455
Attachment #8782792 - Attachment is obsolete: true
Attachment #8782792 - Flags: review?(bkelly)
Attachment #8782834 - Flags: feedback?(bkelly)
Assignee

Comment 175

3 years ago
Posted file interdiff-P2-part1.txt (obsolete) —
inter-diff patch
Comment on attachment 8782834 [details] [diff] [review]
Bug 1187335 - P2-part1v4.1 - Modify the way to report to console for worker and use LoadTainting to decide CORS or not.

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

Looks good.  A few comments to address though.  Thanks.

::: dom/base/nsScriptLoader.cpp
@@ +1164,5 @@
>    }
>  
>    nsAutoPtr<mozilla::dom::SRICheckDataVerifier> sriDataVerifier;
>    if (!aRequest->mIntegrity.IsEmpty()) {
> +    nsCString sourceUri;

I think you can use nsAutoCString for these.

@@ +1166,5 @@
>    nsAutoPtr<mozilla::dom::SRICheckDataVerifier> sriDataVerifier;
>    if (!aRequest->mIntegrity.IsEmpty()) {
> +    nsCString sourceUri;
> +    if (mDocument) {
> +      mDocument->GetDocumentURI()->GetAsciiSpec(sourceUri);

GetDocumentURI() can return nullptr.  You should check that before de-referencing the pointer.

::: dom/security/SRICheck.cpp
@@ +142,5 @@
> +      nsTArray<nsString> params;
> +      params.AppendElement(tokenUTF16);
> +      ReportToConsole(nsIScriptError::warningFlag,
> +                      nsContentUtils::eSECURITY_PROPERTIES,
> +                      "MalformedIntegrityHash", params, aSourceFileURI,

If you use NS_LITERAL_CSTRING("MalformedIntegrityHash") you can just call aReporter->AddConsoleReport() directly.  The ReportToConsole() helper function isn't doing to much here any more.

::: layout/style/Loader.cpp
@@ +965,5 @@
>      }
>    } else {
> +    nsCString sourceUri;
> +    if (mLoader->mDocument) {
> +      mLoader->mDocument->GetDocumentURI()->GetAsciiSpec(sourceUri);

Same issue with GetDocumentURI() here.
Attachment #8782834 - Flags: feedback?(bkelly) → feedback+
Assignee

Comment 177

3 years ago
Hi Ben,

Thanks for the feedback. I addressed the comment. Could you help me to review this patch? Thanks!
Attachment #8782834 - Attachment is obsolete: true
Attachment #8783386 - Flags: review?(bkelly)
Assignee

Comment 178

3 years ago
Posted patch interdiff-P2-part1.txt (obsolete) — Splinter Review
Inter-diff patch between current patch and previous r+ one.
Attachment #8782793 - Attachment is obsolete: true
Attachment #8782835 - Attachment is obsolete: true
Assignee

Comment 179

3 years ago
I modify this patch since I modify the SRI function.
Attachment #8780962 - Attachment is obsolete: true
Attachment #8783465 - Flags: review?(bkelly)
Assignee

Comment 180

3 years ago
Posted patch interdiff-P4.txt (obsolete) — Splinter Review
Inter-diff patch between r? and previous r+ for P4 patch.
Attachment #8783465 - Flags: review?(bkelly) → review+
Attachment #8783386 - Flags: review?(bkelly) → review+
Assignee

Comment 181

3 years ago
Update and rebase the patch.
Attachment #8783465 - Attachment is obsolete: true
Attachment #8783466 - Attachment is obsolete: true
Assignee

Comment 182

3 years ago
Hi François,

I re-write this patch for passing SourceFileURI to SRI function. Could you help me to review this patch again? Thanks!
Attachment #8783386 - Attachment is obsolete: true
Attachment #8783389 - Attachment is obsolete: true
Attachment #8784650 - Flags: review?(francois)
Assignee

Comment 183

3 years ago
Hi Ben,

I rewrite ConsoleReportCollector::FlushReportsByWindowId in this patch. Could you help me to review this patch? Thanks!
Attachment #8780470 - Attachment is obsolete: true
Attachment #8780472 - Attachment is obsolete: true
Attachment #8784766 - Flags: review?(bkelly)
Assignee

Comment 185

3 years ago
(In reply to Tom Tung [:tt] from comment #184)
> Created attachment 8784767 [details] [diff] [review]
> Bug 1187335 - P7 - A test for serviceworker and sharedworker on webconsole.

Sorry for sending review request without finishing comment. I create this patch for ensuring error reports will be send into each clients.
Assignee

Updated

3 years ago
Attachment #8779263 - Attachment is obsolete: true
Can you post interdiffs for these last two reviews please?  Thanks.
Flags: needinfo?(ttung)
Assignee

Comment 187

3 years ago
Posted patch interdiff-P1.txt (obsolete) — Splinter Review
Sorry for forgetting upload inter-diff patch. 
This file is inter-diff patch for P1.
It's first time for P7 to review so there is no inter-diff patch for P7.
Flags: needinfo?(ttung)
Assignee

Comment 188

3 years ago
Posted patch interdiff-P2-part1.txt (obsolete) — Splinter Review
This file is inter-diff patch for P2-part1 between previous r+ patch and current r? one.
Comment on attachment 8784854 [details] [diff] [review]
interdiff-P2-part1.txt

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

I'm confused, though.  This interdiff does not seem to be in attachment 8784766 [details] [diff] [review].

::: b/dom/security/SRICheck.cpp
@@ +86,5 @@
> +                              NS_LITERAL_CSTRING("Sub-resource Integrity"),
> +                              nsContentUtils::eSECURITY_PROPERTIES,
> +                              aSourceFileURI, 0, 0,
> +                              NS_LITERAL_CSTRING("IneligibleResource"),
> +                              const_cast<const nsTArray<nsString>&>(params));

Why do you need this const_cast?  Seems like it should not be necessary.  Here and the rest of the calls in this patch.
Comment on attachment 8784851 [details] [diff] [review]
interdiff-P1.txt

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

r=me with comments addressed.  Thanks.

::: b/dom/console/ConsoleReportCollector.cpp
@@ +110,5 @@
>  void
> +ConsoleReportCollector::FlushReportsByWindowId(uint64_t aWindowId,
> +                                               ReportAction aAction)
> +{
> +  nsTArray<PendingReport> reports;

MOZ_ASSERT(NS_IsMainThread())

@@ +145,5 @@
> +    // Utilize URI to get filename.
> +    nsCOMPtr<nsIURI> uri;
> +    nsAutoCString filename;
> +    if (!report.mSourceFileURI.IsEmpty()) {
> +      rv = NS_NewURI(getter_AddRefs(uri), report.mSourceFileURI);

Why do we need to parse the URI here?  Can't we just pass the mSourceFileURI through directly?  There is no base URL being provided so it has to be an absolute URL already.

@@ +171,5 @@
> +      continue;
> +    }
> +
> +    if (!consoleService) {
> +      rv = CallGetService(NS_CONSOLESERVICE_CONTRACTID, &consoleService);

Lets just move this up to where we declare consoleService.  If we can't get the service then we can just early exit before entering the loop.

::: b/dom/console/nsIConsoleReportCollector.h
@@ +95,5 @@
>    FlushConsoleReports(nsIConsoleReportCollector* aCollector) = 0;
>  
> +  // Flush all pending reports to the console accroding to window ID.
> +  //
> +  // aWindowId      A window ID representing where to flush the reports.

Document that this is typically the inner window ID.
Attachment #8784851 - Flags: review+
Comment on attachment 8784767 [details] [diff] [review]
Bug 1187335 - P7 - A test for serviceworker and sharedworker on webconsole.

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

::: dom/workers/test/serviceworkers/test_fetch_integrity.html
@@ +94,5 @@
> +    function() {
> +      registration.active.postMessage("end");
> +    });
> +
> +  yield waitForFetch;

Can you just do `yield fetch("fail.html")` here?

@@ +134,5 @@
> +  info("Create a SharedWorker.");
> +  let sw = new SharedWorker(filename);
> +
> +  let waitForFetch = new Promise((resolve) => {
> +    sw.port.onmessage = resolve;

It would make it easier to understand if this checked for the message data being postMessage()'d in the service worker.
Attachment #8784767 - Flags: review?(bkelly) → review+
Comment on attachment 8784766 [details] [diff] [review]
Bug 1187335 - P1 - Add FlushReportToAllClient() to ServiceWorkerManager and SharedWorker.

r+ based on review of interdiff.
Attachment #8784766 - Flags: review?(bkelly) → review+
Assignee

Comment 194

3 years ago
Thanks for your review! Addressed the comment and integrate with path P2-part2.
Attachment #8765366 - Attachment is obsolete: true
Attachment #8784766 - Attachment is obsolete: true
Attachment #8784851 - Attachment is obsolete: true
Assignee

Comment 195

3 years ago
Hi Ben,

I addressed your comment but I find out that the second task is not clear. Hence, I rewrite the second task. Could you help me to review? Thanks! The interdiff will be provided!
Attachment #8784767 - Attachment is obsolete: true
Attachment #8785863 - Flags: review?(bkelly)
Assignee

Comment 196

3 years ago
Posted patch interdiff-P7.txt (obsolete) — Splinter Review
interdiff for P7.
Comment on attachment 8785863 [details] [diff] [review]
Bug 1187335 - P7-v2 - A test for serviceworker and sharedworker on webconsole.

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

Sorry I didn't notice this before, but shouldn't fetch() be rejecting its promise if the integrity fails to match?

::: dom/workers/test/serviceworkers/sharedWorker_fetch.js
@@ +16,5 @@
> +  } else if (clients.length == 2) {
> +    broadcast("BothConnected");
> +    clients[0].onmessage = function(e) {
> +      if (e.data == "StartFetchWithWrongIntegrity") {
> +        fetch("SharedWorker_SRIFailed.html", {"integrity": "abc"}).then(

If this fails the integrity check, shouldn't it reject instead of resolving?

::: dom/workers/test/serviceworkers/test_fetch_integrity.html
@@ +85,5 @@
> +    clientWindowID
> +  );
> +
> +  info("Test for mControlledDocuments and report error message to console.")
> +  yield fetch("fail.html");

Shouldn't this throw?  Seems like the remainder of this function should not execute.

The spec says if the integrity value does not match then a network error should be returned.  The fetch method spec then converts network error responses into TypeError rejections.
Attachment #8785863 - Flags: review?(bkelly) → review-
Assignee

Comment 198

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #197)
> Shouldn't this throw?  Seems like the remainder of this function should not
> execute.
> 
> The spec says if the integrity value does not match then a network error
> should be returned.  The fetch method spec then converts network error
> responses into TypeError rejections.

Hi Ben,

  The spec is a bit strange to me here but if we follow the spec, we should not throw error here. We throw error only when the integrity metadata is valid but it's doesn't match the hash (e.g. integrity="sha256-abc").

  This integrity metadata is invalid and it will let 'parsedMetadata' be empty. According to SRI spec [1] step 2, we will return true here which makes promise resolve. Thus, if we check SRI with an invalid integrity metadata, it will return NS_OK [2] with report the error report. 

  If you don't agree with that, I will rewrite a test for the cases like |integrity="sha256-abc"| and file a bug for changing [2] to make invalid integrity metadata (e.g. integrity="abc") return error.

[1] https://w3c.github.io/webappsec-subresource-integrity/#does-response-match-metadatalist
[2] https://dxr.mozilla.org/mozilla-central/source/dom/security/SRICheck.cpp#370
Flags: needinfo?(bkelly)
Comment on attachment 8785863 [details] [diff] [review]
Bug 1187335 - P7-v2 - A test for serviceworker and sharedworker on webconsole.

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

Ah, you are correct.  Can you add a comment indicating that the fetch will succeed because the integrity value is invalid and we are looking for the console message regarding the bad integrity value?  Does that describe what you are going for here?
Attachment #8785863 - Flags: review- → review+
Flags: needinfo?(bkelly)
Assignee

Comment 201

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #200)
> Ah, you are correct.  Can you add a comment indicating that the fetch will
> succeed because the integrity value is invalid and we are looking for the
> console message regarding the bad integrity value?  Does that describe what
> you are going for here?

Yes, thank you. I'll add comment in both test to make them clearer.
Assignee

Comment 202

3 years ago
Sorry for the late reply, I tested my patches in the try server and got few errors. I was working on resolving them. I fixed most of them except one.

I hit the test [1] because attachment 8785858 [details] [diff] [review] remove the code for nulling mChannel and mReleaseHandler in InterceptedChannelChrome::FinishSynthesizedResponse(). The reason for removing the code for nulling mChannel and mReleaseHandler is we want to send error reports to InterceptedChannel for ServiceWorker.

Currently, I would like to recover [2] to make the test [1] pass but I am not sure about that. Do you have any hints for this, Ben? Thanks!  

[1] http://searchfox.org/mozilla-central/source/devtools/shared/webconsole/test/test_console_serviceworker.html
[2] http://searchfox.org/mozilla-central/source/netwerk/protocol/http/InterceptedChannel.cpp#303-304
Flags: needinfo?(bkelly)
Assignee

Comment 204

3 years ago
(In reply to Tom Tung [:tt] from comment #202)
I find out that I received the console log for message event so I cannot pass the test(test_console_serviceworker.html). I guess the reason is that I remove the code for cleaning mReleaseHandler and mChannel in InterceptedChannelChrome::FinishSynthesizedResponse().

Maybe I should modify the test(test_console_serviceworker.html) to assume that we should receive message event rather than getting cleaning code back.
The console log for which message event?  If you think the console report is correct, then updating the test expectations seems reasonable.
Flags: needinfo?(bkelly)
Assignee

Comment 206

3 years ago
Addressed comments and add another comment that console log should match the innerWindowID rather than outerWindowID.
Attachment #8785863 - Attachment is obsolete: true
Attachment #8785867 - Attachment is obsolete: true
Assignee

Comment 207

3 years ago
Hi Ben, 

Sorry for asking review this patch again. I modify this patch due to failure on try server. In this patch, I change the raw pointer consoleService to nsCOMPtr. Besides, I modify the test(test_console_serviceworker.html) since I make mReleaseHandler and mChannel not to be cleaned until dtor is called. Could you help me to review this patch? Thanks! The interdiff patch will be provided.

I'll obsolete the old patch(attachment#8785858 [details] [diff] [review]) once this patch get r+.
Attachment #8788124 - Flags: review?(bkelly)
Assignee

Comment 208

3 years ago
Posted patch interdiff for P1. (obsolete) — Splinter Review
interdiff for P1(attachment#8788124 [details] [diff] [review]).
Comment on attachment 8788125 [details] [diff] [review]
interdiff for P1.

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

::: devtools/shared/webconsole/test/test_console_serviceworker.html
@@ +121,5 @@
>      // Now postMessage() the service worker to trigger its message event
>      // handler.  This will generate 1 or 2 to console.log() statements
> +    // depending on if the worker thread needs to spin up again.  Although we
> +    // don't have a controlled or registering document in both cases, we still
> +    // could get console calls since we only clean the channel when destructor

Maybe change this to "since we only flush reports when the channel is finally destroyed".
Attachment #8788124 - Flags: review?(bkelly) → review+
Assignee

Comment 211

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #210)

Thanks for your review and feedback, Ben. I learn a lot from them! Addressed the comment.
Attachment #8785858 - Attachment is obsolete: true
Attachment #8788124 - Attachment is obsolete: true
Attachment #8788125 - Attachment is obsolete: true
Comment on attachment 8784650 [details] [diff] [review]
Bug 1187335 - P2-part1v5.1 - Modify the way to report to console for worker and use LoadTainting to decide CORS or not. r=bkelly.

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

This looks good to me. I would suggest changing `MOZ_ASSERT` to `NS_ENSURE_ARG_POINTER` to get a more appropriate error code and so that we don't crash should a null pointer be passed there.

::: dom/security/SRICheck.cpp
@@ +101,2 @@
>    MOZ_ASSERT(outMetadata->IsEmpty()); // caller must pass empty metadata
> +  MOZ_ASSERT(aReporter);

Any reason not to use `NS_ENSURE_ARG_POINTER` here?

@@ +186,3 @@
>  {
>    NS_ENSURE_ARG_POINTER(aLoader);
> +  MOZ_ASSERT(aReporter);

Ditto

@@ +225,5 @@
>  
>    // IntegrityMetadata() checks this and returns "no metadata" if
>    // it's disabled so we should never make it this far
>    MOZ_ASSERT(Preferences::GetBool("security.sri.enable", false));
> +  MOZ_ASSERT(aReporter);

Ditto

@@ +306,2 @@
>  {
> +  MOZ_ASSERT(aReporter);

Ditto

@@ +362,2 @@
>  {
> +  MOZ_ASSERT(aReporter);

Ditto
Attachment #8784650 - Flags: review?(francois) → review+
Assignee

Comment 217

3 years ago
(In reply to François Marier [:francois] from comment #216)

Thanks for your review and your time! I learn a lot from your feedback!

> Comment on attachment 8784650 [details] [diff] [review]
> Bug 1187335 - P2-part1v5.1 - Modify the way to report to console for worker
> and use LoadTainting to decide CORS or not. r=bkelly.
> 
> Review of attachment 8784650 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me. I would suggest changing `MOZ_ASSERT` to
> `NS_ENSURE_ARG_POINTER` to get a more appropriate error code and so that we
> don't crash should a null pointer be passed there.
> 

Got it! 

> ::: dom/security/SRICheck.cpp
> @@ +101,2 @@
> >    MOZ_ASSERT(outMetadata->IsEmpty()); // caller must pass empty metadata
> > +  MOZ_ASSERT(aReporter);
> 
> Any reason not to use `NS_ENSURE_ARG_POINTER` here?

No, so I will address that in next patch.

> @@ +225,5 @@
> >  
> >    // IntegrityMetadata() checks this and returns "no metadata" if
> >    // it's disabled so we should never make it this far
> >    MOZ_ASSERT(Preferences::GetBool("security.sri.enable", false));
> > +  MOZ_ASSERT(aReporter);
> 
> Ditto

I cannot use |NS_ENSURE_ARG_POINTER| here because it's in the SRICheckDataVerifier's constructor and it cannot return a value(nsresult). Thus, I'll keep using |MOZ_ASSERT| only for this one.
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 220

3 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8510e1c0f42a
P1 - Add a way to report error to all clients for ServiceWorker and SharedWorker. r=bkelly.
https://hg.mozilla.org/integration/mozilla-inbound/rev/89c1c3294bee
P2 - Modify the way to report to console for worker and use LoadTainting to decide CORS or not. r=bkelly. r=francois.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c586b0983344
P3 - modify SRI test to match current behavior. r=bkelly, r=francois.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd24ae54ea42
P4 - Integrate fetch and cache with SRI & add nsIConsoleReportCollector to show console report. r=bkelly.
https://hg.mozilla.org/integration/mozilla-inbound/rev/46b85380cb2d
P5 - Adding new tests for SRI and enabling test for Request's integrity attribute in wpt. r=bkelly.
https://hg.mozilla.org/integration/mozilla-inbound/rev/126d0c86f8ee
P6 - Support script/css to set integrity metadata to serviceWorker. r=bkelly. r=francois.
https://hg.mozilla.org/integration/mozilla-inbound/rev/54e0b3c264e5
P7 - A test for serviceworker and sharedworker on webconsole. r=bkelly.
Keywords: checkin-needed
Assignee

Comment 222

3 years ago
Thanks for all the efforts from Ben and François!
Depends on: 1301678

Updated

3 years ago
Depends on: 1304197

Updated

3 years ago
No longer depends on: 1304197