Closed Bug 1523706 Opened 2 years ago Closed 1 year ago

Consider strictly enforcing MIME checks for Worker scripts

Categories

(Core :: DOM: Workers, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(4 files, 1 obsolete file)

It would be good to figure out if we could also restrict the MIME types of worker scripts, instead of just those loaded via importScripts() (Bug 1514680).

I think this issue also still applies: https://github.com/whatwg/html/issues/3255

Component: DOM → DOM: Workers
Assignee: nobody → evilpies

Unless something is wrong with our telemetry I think we should at least try doing this.

This causes quite a lot of test failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=96da6a532b3d2b2fe1e7caea6b79a1ff9f06b29b. I do think some of those might be unrelated.

Interesting find, seems like for some reason tests in WPT use polygot HTML/JS files ... !!

For example:
workers/interfaces/WorkerGlobalScope/self.html
workers/semantics/interface-objects/003.html
workers/semantics/interface-objects/004.html
workers/interfaces/WorkerUtils/WindowTimers/003.html
workers/interfaces/WorkerUtils/WindowTimers/005.html

https://searchfox.org/mozilla-central/search?q=new+(Shared)%3FWorker..%23&case=false&regexp=true&path=

Should we change those tests or just disable them for us?

Flags: needinfo?(ckerschb)

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

Should we change those tests or just disable them for us?

From my quick check it seems those usages are not intentional by the authors of the tests and I think we should update them.

Flags: needinfo?(ckerschb)
Depends on: 1557736

I am waiting for review to update some of the WPT tests, but at least a few need to be disabled, because they just don't work when blocking other MIME types.

Keywords: leave-open
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6782caf07c7d
Use JavaScript mime type for two worker tests. r=ckerschb
Attachment #9067976 - Attachment description: Bug 1523706 - Consider strictly enforcing MIME checks for Worker scripts → Bug 1523706 - Consider strictly enforcing MIME checks for Worker scripts. r?ckerschb

Depends on D37911

I am going to send an Intent to Ship for this, considering that Chrome seems disinclined to implement this.

Keywords: site-compat
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8fcae0a0d731
Consider strictly enforcing MIME checks for Worker scripts. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/122642699fc5
Disable WPT Worker tests that require a non JavaScript mime. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/09edf04895b6
Extend devtools test. r=ckerschb
Status: NEW → RESOLVED
Closed: 1 year ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Blocks: 1569122
Blocks: 1569123

Is there a bug tracking re-enabling the tests that got disabled here?

Flags: needinfo?(evilpies)

Yes, see bug 1569123.

Flags: needinfo?(evilpies)

That's about re-enabling the type checks (which got disabled in bug 1569122). I am talking about things like https://searchfox.org/mozilla-central/rev/153feabebc2d13bb4c29ef8adf104ec1ebd246ae/testing/web-platform/meta/workers/Worker_script_mimetype.htm.ini#2 which disabled various tests.

Flags: needinfo?(evilpies)

hg backout 122642699fc5 worked. See review request.

Flags: needinfo?(evilpies)
Regressions: 1583657

ni myself to fix those tests.

Flags: needinfo?(evilpies)

What should I do about workers/Worker_script_mimetype.htm? It seems like that test uses plaintext on purpose.

Flags: needinfo?(bzbarsky)

That one we should mark as failing on the branches where we have the strict MIME type enforcement (and keep testing that it passes on the branches where we do not have that turned on).

Flags: needinfo?(bzbarsky)

Do we actually have branches with the test changes and not the code/pref changes? Is that situation expected to persist (i.e. is there some part of this that isn't expected to ride the trains?). Typically for things that are on on nightly but not on beta/release we either set the pref unconditionally so the feature is on on all branches, or annotate things like

if release_or_beta: PASS
FAIL

It sounds like the first option is undesirable here, so the second might be possible. If there are differences between release and beta that need to be annotated I'm not sure what you can do; I can't see a related mozinfo field, but I may be overlooking something obvious.

Blocks: 1595297

Opened bug 1595297 to re-enable those tests.

Flags: needinfo?(evilpies)
Attachment #9094639 - Attachment is obsolete: true
See Also: → 1616237
You need to log in before you can comment on or make changes to this bug.