1948889 - Change SRI message to take referrer url for warnings in a thread-safe way.

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Emilio Cobos Álvarez (:emilio)]

https://searchfox.org/mozilla-central/rev/a80b3969c6d78ac5ba2552a5bce5e5c7a74f3a9b/dom/script/ScriptLoader.cpp#932 (and similar code in the fetch driver / sheet loader / etc) uses the doc URI for SRI warnings.

That feels wrong? I guess the idea is that you can only specify SRI via attributes? But still wouldn't it be more helpful to point to the subresource URI that's actually failing to validate?

Comment 1

[Frederik Braun [:freddy]]

Oh, good catch. Looks like this got regressed by bug 1839316.

However, I can not come up with a test case that verifies this being wrong.

I have already looked at

• https://www.srihash.org/
• wpt in subresource-integrity/subresource-integrity.html
• wpt in content-security-policy/unsafe-hashes
  ... and I could not get to any error condition that reports the wrong URL.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Ah, I was writing a patch :)

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1948889 - Simplify SRICheck by getting location from http referrer info. r=freddyb

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(In reply to Frederik Braun [:freddy] from comment #1)

Oh, good catch. Looks like this got regressed by bug 1839316.

However, I can not come up with a test case that verifies this being wrong.

I mean it only affects console messages, so no WPT impact.

• https://www.srihash.org/

With my patch, devtools points to must-fail.css rather than srihash.org in the console.

Comment 5

[Frederik Braun [:freddy]]

Oh, now I get it. This is about the logged URL at the right side of the devtools console. Not as part of the message text.
Before the patch, the message logged that the error is at the HTML file which includes the hashed resources. With your patch it is now logging the URL of the file that was meant to be included.

We can't really tell if the provided hash was wrong or if the response was wrong. I'd slightly lean that most cases have a wrong hash in the URL rather than an unexpected resource showing up. But 's really just... a choice? I am OK with either.

Tom has done most of the recent SRI work so I'd like him to weigh in.

Comment 6

[Tom Schuster]

In bug 1938307 I added an explicit reference in the error message to the SRI using resource URL that failed to load. To me this makes more sens than changing the source location. The SRI is actually not an intrinsic part of the requested resource, but is actually defined by the place (source) requesting the resource.

Comment 7

[Frederik Braun [:freddy]]

I think I want to stick with what I was leaning to.

The log message already contains failing resource URL and the computed hash in the message. I believe the source URL should point to whatever closest match we have for what was requesting the incorrect message. I think it's perfect.

But the source url (right bit in the console) should continue point to the document URL as a "source URL".
If we can provide more info (like the exact source line in HTML / JS), we should ideally point to that.

@emilio: I think we should wontfix this bug. Does that reasoning make sense? Anything we overloked?

Comment 8

[Tom Schuster]

I guess we could explicitly include the resource URI in all error messages that don't have it yet?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Ok, I think it's a bit weird, but let's repurpose this to take the referrer url from the channel, since I need that to make the stylesheet SRI stuff work off the main thread.

Comment 10

[Frederik Braun [:freddy]]

As this is a regression, I am setting release status for affected releases. However, given this only affects devtools I am quite happy with only fixing this in Nightly.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e461094881c
Simplify SRICheck by getting location from http referrer info. r=freddyb,tschuster

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb800db4ae96
Fix test when run under xorig.

Comment 13

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/3e461094881c
https://hg.mozilla.org/mozilla-central/rev/bb800db4ae96