1624449 - Muted errors still have an URL/filename

Status: NEW

(Core :: JavaScript Engine, defect, P2)

Description

[Tom S]

I don't think the filename/URL is actually private, because this is set by the "loader". Maybe it contains the URL after redirect? So this might not be a real security issue.

The spec says https://html.spec.whatwg.org/multipage/webappapis.html#report-the-error urlString should be an empty string.
We however always assign the filename: https://searchfox.org/mozilla-central/rev/202a285024f174c2d2bf2152d9cba90a03723eab/dom/base/nsJSEnvironment.cpp#510

Comment 1

[Tom S]

I have a fix and test for this.

Comment 2

[Anne (:annevk)]

I think it's a security problem if it's the URL after redirects (that's the one the embedder does not know).

Comment 3

[Tom S]

Attachment: testcase.html

At least this test-case shows that we seem to use the pre-redirect URL.

Somewhat related probably at http://wpt.live/html/semantics/scripting-1/the-script-element/muted-errors.sub.html, we fail the test:
"Syntax error for same-origin script redirected to a cross-origin URL and redirected back to same-origin should be muted"

Comment 4

[Tom S]

Attachment: Bug 1624449 - Muted errors still have an URL/filename

Comment 5

[Tom S]

I can't test this on try, so it's quite likely that there are some other test failures.

Comment 6

[Tom S]

Can I push this to try and land on autoland afterwards, or do I need to wait?

Comment 7

[Andrew McCreight [:mccr8]]

It depends on what the security rating is. I'm not sure what it should be.

Comment 8

[Daniel Veditz [:dveditz]]

If it's the pre-redirect URL then it's not a security bug and you can just land. I don't get either URL with your testcase though. Am I doing it wrong? Have too many paranoid prefs turned on?

If it's the post-redirect URL then it's a sec-high same-origin policy violation. You could push the patch to try, but take the testcase out first (which might make pushing to try pointless?).

Comment 9

[Tom S]

(In reply to Daniel Veditz [:dveditz] from comment #8)

If it's the pre-redirect URL then it's not a security bug and you can just land. I don't get either URL with your testcase though. Am I doing it wrong? Have too many paranoid prefs turned on?

The test-case doesn't work when directly running on bugzilla.mozilla.org, because the CSP blocks the JS. I am going to try and figure out where the filename comes from. As I said for me with that test it's the pre-redirect bit.ly URL instead of example.org

If it's the post-redirect URL then it's a sec-high same-origin policy violation. You could push the patch to try, but take the testcase out first (which might make pushing to try pointless?).

That would work for me, I just want to find other potential test failures (or new passes more likely) caused by this change.

Comment 10

[Tom S]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=695f2fc11041155a3a2d178ba1b8be5676c8f158

Comment 11

[Tom S]

I suspect this is not actually a security issue:

• We use the initial URL and not the final URL: https://searchfox.org/mozilla-central/source/dom/script/ScriptLoader.cpp#2435-2436
• Boris purposefully chose this behavior it seems: https://searchfox.org/mozilla-central/source/dom/tests/mochitest/bugs/test_bug873229.html#46-48

We should still fix it to match the spec and other browsers.

Comment 12

[Anne (:annevk)]

I wonder if it's better to not make this change until we sort out https://github.com/whatwg/html/issues/958#issuecomment-585900125 onward in combination with https://github.com/whatwg/html/issues/958#issuecomment-226309871. In particular as we're revisiting what needs to be muted we might well decide to not mute the URL as it's harmless and give at least a hint as to where people need to look for errors.

Comment 13

[Tom S]

Sounds good to me. We should maybe rise a spec issue about not censoring the filename.

Comment 14

[Anne (:annevk)]

So looking at https://html.spec.whatwg.org/#runtime-script-errors <del title=oops>I don't think</del> filename is censored. Unfortunately the way filename is defined is too vague and would allow for an interpretation that it's the one after redirects.

Comment 15

[Tom S]

So looking at https://html.spec.whatwg.org/#runtime-script-errors I don't think filename is censored.

I think it's being censored:
Step 6. If script's muted errors is true, ... set ... urlString to the empty string ...
Step 7. ... the filename attribute initialized to urlString ...

Unfortunately the way filename is defined is too vague and would allow for an interpretation that it's the one after redirects.

Definitely. It almost sounds like it's actually the final URL: "resource from which script was obtained"

Comment 16

[Tom S]

I think we should open this up. I want to comment with some more details in html#958, which would basically reveal everything here.

Comment 17

[Tom S]

Added a comment to the github issue. I guess we just have to wait for other vendors now.

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:evilpie, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 19

[Tom S]

Unassinging myself. There doesn't seem to be any movement on this.