Closed Bug 1000175 Opened 6 years ago Closed 6 years ago

window.onerror catching events cross domain


(Core :: DOM: Core & HTML, defect)

Not set





(Reporter: jacob_champlin, Assigned: bzbarsky)



(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140327172601

Steps to reproduce:

We bind to window.onerror on all our company web pages, and then send ourselves a email whenever our pages have a javascript error.

On one of our pagest we have the following link to a outside domain:
<a href="">Fidelity Charitable Donor Advised Fund Program </a>

Actual results:

On Seamonkey 2.25 window.onerror caught the following exception. 

Script error. in s_code.js:0

Which suggests that our page was trying to execute s_code.js but couldn't find it.  The problem is that s_code.js is not something our page is loading, it something the website is loading.

We have not been able to reproduce this reliably.  It only happened once, but thought it was risky enough to report.

Expected results:

It is my expectation that window.onerror on domain A should never attempt to execute javascript from domain B.  I think this is a security risk.  Note the script didn't technically execute, so this just maybe a issue with window.onerror events.
Jacob, thank you for reporting this!

That looks like a sanitized script error: the script that threw is not same-origin with the onerror handler.  That's why the error text and line number are hidden; the only thing left is the filename.

It looks to me like we could in fact get in this state this in the following situation:

1)  User loads page A.
2)  User clicks link to page B.
3)  User hits back button.
4)  Exception thrown on page B, async error reporter event posted.
5)  Navigation back to page A completes.
6)  Error event fires.

because the ScriptErrorEvent doesn't check whether the inner window when it was posted matches the inner window when it's firing.  Seems to me like it should, no?  mScriptGlobal in this case is an _outer_ window, fwiw.
Component: Security → DOM
Flags: needinfo?(bugs)
Flags: needinfo?(bobbyholley)

Thank you for the quick response.

Your explanation seems like exactly what happened.  I was missing steps #5 and #6.  The only problem is I still can't seem to reproduce, because page A loads faster than I can hit link to page B.  I will try to put a sleep in and see if I can reproduce.

Jacob, I wouldn't worry about trying to reproduce this.  I think our code here is clearly buggy.
What behavior do we want with document.write?
Flags: needinfo?(bugs)
That's a good question.  open() blows away event listeners on the window, so I would argue we want an actual inner window identity check, not an active document check.
I don't see anything truly async there, but a script runner. Sure, that is enough to cause this issue though.
Hmm.  If it's just a script runner that makes the scenario in comment 1 less likely.  :( We should still fix, but there might be something else going on.
Yeah, it seems like ScriptErrorEvent should hold onto the inner, rather than the outer, and do some kind of check against the current inner. There are two bits of behavior that I'm not sure about though:

* Do we want to invoke the old onerror, or nothing at all?
* Do we want to report to the outer window's console, or not at all?
Flags: needinfo?(bobbyholley)
> * Do we want to invoke the old onerror, or nothing at all?

I would say nothing at all.  Typically an event listener wouldn't run in a non-current inner anyway.

>* Do we want to report to the outer window's console, or not at all?

I think we do want to report to the outer's console.

Make sense?
Ever confirmed: true
Flags: needinfo?(bobbyholley)
(In reply to Boris Zbarsky [:bz] (on PTO, reviews are slow) from comment #9)
> Make sense?

Flags: needinfo?(bobbyholley)
Assignee: nobody → bzbarsky
Jacob, can you reproduce this reliably enough that you could test this patch if I pointed you to some binaries (Linux 64-bit, right?) that include it?
Flags: needinfo?(jacob_champlin)
Whiteboard: [need review]
Comment on attachment 8422022 [details] [diff] [review]
Make sure error events on window only fire if the active document has not changed from when the exception was thrown

Why do we need mScriptGlobal anymore?
mWindow should be enough for getting docshell
Attachment #8422022 - Flags: review?(bugs) → review-
Attached patch Updated patchSplinter Review
Right you are!
Attachment #8422022 - Attachment is obsolete: true
Attachment #8425240 - Flags: review?(bugs)
Attachment #8425241 - Attachment description: test.patch → Interdiff from first patch
Comment on attachment 8425240 [details] [diff] [review]
Updated patch

The whole error dispatching code is a bit ugly :/
Attachment #8425240 - Flags: review?(bugs) → review+
Flags: needinfo?(jacob_champlin) → in-testsuite?
Whiteboard: [need review]
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.