Warn about scripts with wrong MIME type

RESOLVED FIXED in mozilla66

Status

()

enhancement
P2
normal
RESOLVED FIXED
9 months ago
7 months ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

(Blocks 2 bugs, {site-compat})

66 Branch
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [domsecurity-active])

Attachments

(3 attachments, 1 obsolete attachment)

We already block a selected number of MIME types when trying to load scripts, hopefully we can block more stuff in future if we start warning.
Posted patch WIP no tests (obsolete) — Splinter Review
This definitely needs some tests. The bigger problem for me right now is that the warning only seems to appear in the Browser Console and not in the Web Console.
Assignee: nobody → evilpies
Attachment #9027867 - Flags: feedback?(ckerschb)
Comment on attachment 9027867 [details] [diff] [review]
WIP no tests

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

::: dom/locales/en-US/chrome/security/security.properties
@@ +84,5 @@
>  # LOCALIZATION NOTE: Do not translate "X-Content-Type-Options" and also do not trasnlate "nosniff".
>  XCTOHeaderValueMissing=X-Content-Type-Options header warning: value was “%1$S”; did you mean to send “nosniff”?
>  
>  BlockScriptWithWrongMimeType=Script from “%1$S” was blocked because of a disallowed MIME type.
> +ScriptWithWrongMimeTypeWarning=Script from “%1$S” is using a non-JavaScript MIME type: “%2$S”

nit: add a "." at the end like BlockScriptWithWrongMimeType does and probably rename the warning to WarnScriptWithWrongMimeType.

I guess we should also say something that this is only a warning and not actually blocking anything, right?

Maybe preface with 'Warning: Script from ...'

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1640,5 @@
> +                                        doc,
> +                                        nsContentUtils::eSECURITY_PROPERTIES,
> +                                        "ScriptWithWrongMimeTypeWarning",
> +                                        params, ArrayLength(params));
> +    }

that looks good actually. Is the doc null here? Usually we log to the browser console and not to the web console in case we have a null document.
Attachment #9027867 - Flags: feedback?(ckerschb) → feedback+
Maybe you need to do some workaround like we did in CSP [1] - you should be able to query the innerwindowID from the loadinfo.


[1] https://searchfox.org/mozilla-central/source/dom/security/nsCSPUtils.cpp#170
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Depends on: 1510241
Attachment #9027867 - Attachment is obsolete: true
With XCTO we can into a bit of an unfortunate situation, first we warn that we didn't block the resource and then we do block it.
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/901e7ee24162
Warn about scripts with wrong MIME type. r=ckerschb,nchevobbe
Keywords: leave-open
As suggested we should add some test for this. One of the devtools test failures might actually be reusable for this, unless we already have some framework for testing that a certain warning message is triggered?

We might also want to move ProcessXCTO, EnsureMIMEOfScript and WarnWrongMIMEOfScript to a class method?
Working on some test coverage, it looks like we even run this code when the script is not even found?
Turns out we also produced an error instead of a warnings.
Good that we added those test \o/

Please r+ the last patch.

Flags: needinfo?(ckerschb)

(In reply to Tom Schuster [:evilpie] from comment #11)

Please r+ the last patch.

Sorry, haven't seen that on phab - r+.

Flags: needinfo?(ckerschb)
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3e906dc0b1e8
Test for MIME type warning. r=ckerschb,nchevobbe
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Version: unspecified → 66 Branch

It seems like Chrome is not interested in adding a warning for this, because it would be hard to actually enforce it in any reasonable time and they want to avoid warning fatigue.

Does that bother us?

Flags: needinfo?(ckerschb)

(In reply to Tom Schuster [:evilpie] from comment #15)

It seems like Chrome is not interested in adding a warning for this, because it would be hard to actually enforce it in any reasonable time and they want to avoid warning fatigue.

Does that bother us?

I think we can keep and should keep the warning. Developers like our Devtools/Console messages and warning and they tend to incorporate suggestions. If people do so because of the warning, then that's a win in my opinion. But thanks for bringing it to my attention.

Flags: needinfo?(ckerschb)
You need to log in before you can comment on or make changes to this bug.