Closed
Bug 1042798
Opened 11 years ago
Closed 11 years ago
document.write throws a security exception when called through NPAPI
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
People
(Reporter: jmullenix.estream, Assigned: bobowen)
References
Details
(Keywords: dev-doc-needed, regression, site-compat)
Attachments
(3 files)
703 bytes,
text/html
|
Details | |
1.10 KB,
patch
|
bholley
:
review+
lmandel
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.42 KB,
patch
|
bzbarsky
:
review+
lmandel
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140716183446
Steps to reproduce:
Run this snippet:
_docWin=window.open('about:blank','newWindow','width='+w+',height='+h+',menubar=0'+',toolbar=0'+',status=0'+',scrollbars=1'+',resizable=1')
_docWin.moveTo(Math.max(0, window.screenX - _docWin.outerWidth), window.screenY);
var winContent = window.document;
var winContent_Head = winContent.head.innerHTML;
var winContent_Body = winContent.body.innerHTML;
var winContent_Head = winContent_Head.replace(/A.swf/g,"B.swf");
var winContent_Body = winContent_Body.replace(/A.swf/g,"B.swf");
_docWin.document.writeln(winContent_Head);
Actual results:
This now (as of FF 31) throws the following exception, and aborts:
code: 18
message: "The operation is insecure."
name: "SecurityError"
result: 2152923154
Expected results:
This did not throw in previous versions.
![]() |
||
Comment 1•11 years ago
|
||
This seems to work just fine for me in Firefox 31.
> Run this snippet:
How exactly are you running it?
![]() |
||
Updated•11 years ago
|
Flags: needinfo?(jmullenix.estream)
Reporter | ||
Comment 2•11 years ago
|
||
Thanks for the help!
This snippet is part of a function called by a .SWF via ActionScript 3's ExternalInterface.call. The main .SWF is loaded by an HTML file, and this bit of JavaScript lives in a separate .JS file on the same machine. The structure looks like this:
..\main.html
..\assets\js\problem_file.js
It's worked well for a few years.
Flags: needinfo?(jmullenix.estream)
![]() |
||
Comment 3•11 years ago
|
||
Is this running over file:// or http://?
If the former, are you able to provide the scripts to someone else to run from file:// ?
If the latter, can you provide a link to the page? If you can't link to the page, are you willing to use http://mozilla.github.io/mozregression/ to narrow down when the problem first appears for you?
Flags: needinfo?(jmullenix.estream)
Keywords: regression,
regressionwindow-wanted
Reporter | ||
Comment 4•11 years ago
|
||
It happens over both, file:// and http://.
I can't provide scripts at this time, but I will go through the mozregression process today.
Thanks again!
Flags: needinfo?(jmullenix.estream)
Reporter | ||
Comment 5•11 years ago
|
||
I've run through mozregression, and the result is below. It appears this issue was introduced ~March 21st of this year, although I could swear I hit a working nightly from March 31st, as well.
The tool wasn't able to entirely complete its work. I tried a few times, and it always ran into server erros 500, 503 or 504, and exited with an error message like 'Back-end server is at capacity'.
Please let me know whether I need to complete the process. I'd be happy to try another time.
MOZREGRESSION OUTPUT:
Ensuring we have enough metadata to get a pushlog...
Last good revision: e84a391b604b (2014-03-20)
First bad revision: c148f0b0c8b4 (2014-03-21)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e84a391b604b&tochange=c148f0b0c8b4
... attempting to bisect inbound builds (starting from previous day, to make sure no inbound revision is missed)
Getting http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/03/2014-03-19-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.txt
Getting inbound builds between 3bc3b9e2cd99 and c148f0b0c8b4
![]() |
||
Comment 6•11 years ago
|
||
Thanks! In that range, https://hg.mozilla.org/mozilla-central/rev/496ae3bc07aa looks like the most likely culprit.
Are you willing to try two builds for me? The one in https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bzbarsky@mozilla.com-cbb266205ea7/try-win32/ is the one without that changeset and the one in https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bzbarsky@mozilla.com-81168f57c51f/try-win32/ is with that changeset.
Flags: needinfo?(jmullenix.estream)
Reporter | ||
Comment 7•11 years ago
|
||
I'm seeing the security exception in both of these builds. I'm not certain I'm running them correctly, however. In their about menus, I see version 31.0. Is that correct, or am I accidentally running my normal Firefox, somehow?
This is what I did for each:
- download and extract 'firefox-31.0a1.en-US.win32.zip'
- from the extracted folder, run firefox.exe
Flags: needinfo?(jmullenix.estream)
![]() |
||
Comment 8•11 years ago
|
||
> In their about menus, I see version 31.0.
It should say 31.0a1...
> - from the extracted folder, run firefox.exe
Did you quit Firefox first?
Flags: needinfo?(jmullenix.estream)
Reporter | ||
Comment 9•11 years ago
|
||
I did have Firefox open. That was the problem, thanks!
Yes, the exception is thrown in the second Nightly build, but not the first.
Flags: needinfo?(jmullenix.estream)
![]() |
||
Comment 10•11 years ago
|
||
[Tracking Requested - why for this release]:
[Tracking Requested - why for this release]:
[Tracking Requested - why for this release]:
Alright, great. Bob, Bobby, could you take a look, please? Looks like we're not ending up with the right JSContext in nsNPAPIPlugin.cpp anymore, so when document.open (called by document.writeln) does its GetDocumentFromContext bit it doesn't get one...
Blocks: 978042
Status: UNCONFIRMED → NEW
status-firefox31:
--- → wontfix
status-firefox32:
--- → affected
status-firefox33:
--- → affected
status-firefox34:
--- → affected
tracking-firefox32:
--- → ?
tracking-firefox33:
--- → ?
tracking-firefox34:
--- → ?
Ever confirmed: true
Flags: needinfo?(bobowencode)
Flags: needinfo?(bobbyholley)
![]() |
||
Updated•11 years ago
|
Summary: document.write throws a security exception when called through a new window created with window.open. → document.write throws a security exception when called through NPAPI
Updated•11 years ago
|
Assignee | ||
Comment 11•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #10)
> Alright, great. Bob, Bobby, could you take a look, please? Looks like
> we're not ending up with the right JSContext in nsNPAPIPlugin.cpp anymore,
> so when document.open (called by document.writeln) does its
> GetDocumentFromContext bit it doesn't get one...
Ah, yes, this was one of the early changes, we decided on AutoSafeJSContext instead of an AutoEntryScript.
Unfortunately, we can't just revert that change because I got rid of nsIScriptContext::Evaluate string in changeset d2730edb36c4.
We would have to revert that one as well and that changed interface ID (not sure if that would cause a problem).
Anyway we probably just need to decide if we need an AutoJSAPI initialised with legacy error reporting or an AutoEntryScript.
Bobby - which do you think it should be? We didn't have the AutoJSAPI around when I did this one.
Or maybe we're just entering the wrong compartment or something?
I should be able to pick this up pretty quickly on Monday, if I don't get chance before (at a wedding today and Birthday meal for someone tomorrow).
Flags: needinfo?(bobowencode)
![]() |
||
Comment 12•11 years ago
|
||
This isn't about compartments. Right now document.open looks at the JSContext* passed in and tries to get to an nsIScriptContext from it. So you need to have the right JSContext. See bug 1041602 comment 3 and following...
Comment 13•11 years ago
|
||
Yeah, looks like we need an AutoEntryScript after all - bug 978042 comment 28 was totally wrong even at the time, and even more wrong in the light of the semantics that we (later) invented in bug 989528. We're planning to execute script, so we need an AutoEntryScript.
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 15•11 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #14)
> Bob, mind writing up the patch?
No problem at all, I get onto it right now.
Flags: needinfo?(bobowencode)
Assignee | ||
Comment 16•11 years ago
|
||
Assignee | ||
Comment 17•11 years ago
|
||
Oops ... sorry:
https://tbpl.mozilla.org/?tree=Try&rev=c512b5566b09
Assignee | ||
Comment 18•11 years ago
|
||
Just a straight switch for the AutoEntryScript, which will enter the right compartment for us.
Attachment #8463291 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
![]() |
||
Comment 19•11 years ago
|
||
Comment on attachment 8463291 [details] [diff] [review]
Use an AutoEntryScript in nsNPAPIPlugin _evaluate as we are about to run script and need to ensure the correct JSContext* gets pushed v1
Can we please add a test to ensure this doesn't break again?
Keywords: dev-doc-needed,
site-compat
Updated•11 years ago
|
Attachment #8463291 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 20•11 years ago
|
||
This fails before the fix and passes after, and seems fairly minimal.
I did try to compile back to the revision before the regression, but had trouble running the test.
I think that mach and the test harness have changed quite a bit since then.
Attachment #8463557 -
Flags: review?(bzbarsky)
![]() |
||
Comment 21•11 years ago
|
||
Comment on attachment 8463557 [details] [diff] [review]
Test: ensure window.open followed by document.writeln doesn't throw security error when called through NPN evaluate v1
Looks lovely.
Attachment #8463557 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 22•11 years ago
|
||
Thanks Bobby, Boris.
Try push with test:
https://tbpl.mozilla.org/?tree=Try&rev=ce044fe8d172
Assignee | ||
Comment 23•11 years ago
|
||
Just thought I ought to try the test on more platforms as it uses window.open.
These tests are skipped for some things, but just to be safe:
https://tbpl.mozilla.org/?tree=Try&rev=10bde00304bc
Assignee | ||
Comment 24•11 years ago
|
||
And this time with some actual tests running, sorry:
https://tbpl.mozilla.org/?tree=Try&rev=93152e0314a6
Assignee | ||
Comment 25•11 years ago
|
||
Try push from comment 24 looks good.
Please land in the following order (fix then test), thanks:
https://bugzilla.mozilla.org/attachment.cgi?id=8463291
https://bugzilla.mozilla.org/attachment.cgi?id=8463557
Keywords: checkin-needed
Comment 26•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0debc766d237
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e9f64aa7434
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0debc766d237
https://hg.mozilla.org/mozilla-central/rev/1e9f64aa7434
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Assignee | ||
Comment 28•11 years ago
|
||
Hopefully, we have a fix for this problem in the latest Nightly build:
https://nightly.mozilla.org/
Would you mind trying it out?
Once we're sure it's working, I'll request that we get this uplifted.
Flags: needinfo?(jmullenix.estream)
Reporter | ||
Comment 29•11 years ago
|
||
Looks good! Running the Windows Standard version of Nightly, the code in question no longer throws a security exception.
Thanks!!
Flags: needinfo?(jmullenix.estream)
Assignee | ||
Comment 30•11 years ago
|
||
(In reply to jmullenix.estream from comment #29)
> Looks good! Running the Windows Standard version of Nightly, the code in
> question no longer throws a security exception.
Excellent, thanks for testing this.
> Thanks!!
No problem, I'll request this is uplifted to Beta.
Assignee | ||
Comment 31•11 years ago
|
||
Comment on attachment 8463291 [details] [diff] [review]
Use an AutoEntryScript in nsNPAPIPlugin _evaluate as we are about to run script and need to ensure the correct JSContext* gets pushed v1
Approval Request Comment
[Feature/regressing bug #]: Introduced by patch Part 6 in Bug 978042. Where change meant a different JSContext was being pushed.
[User impact if declined]: Any similar script called via plugins will cause exceptions.
[Describe test coverage new/current, TBPL]: New test added to dom/plugins/test/mochitest/test_npruntime_npnevaluate.htm
Also manually tested be original reporter.
[Risks and why]: Medium - this is a simple fix which should duplicate the original context pushing. However, given where the change is, I think medium is appropriate.
[String/UUID change made/needed]: None
Attachment #8463291 -
Flags: approval-mozilla-beta?
Attachment #8463291 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 32•11 years ago
|
||
Comment on attachment 8463557 [details] [diff] [review]
Test: ensure window.open followed by document.writeln doesn't throw security error when called through NPN evaluate v1
Test mentioned in approval request in comment 31.
Attachment #8463557 -
Flags: approval-mozilla-beta?
Attachment #8463557 -
Flags: approval-mozilla-aurora?
Comment 33•11 years ago
|
||
Given that medium risk, can you verify that the fix is good on m-c before we decide on whether to uplift? If you can verify this week, we can get this into beta4 on Monday.
Flags: needinfo?(bobowencode)
Comment 34•11 years ago
|
||
(In reply to Lawrence Mandel [:lmandel] from comment #33)
> Given that medium risk, can you verify that the fix is good on m-c before we
> decide on whether to uplift? If you can verify this week, we can get this
> into beta4 on Monday.
Oop. I see that the reporter did test in comment 29 and all is well.
Updated•11 years ago
|
Flags: needinfo?(bobowencode)
Comment 35•11 years ago
|
||
Comment on attachment 8463291 [details] [diff] [review]
Use an AutoEntryScript in nsNPAPIPlugin _evaluate as we are about to run script and need to ensure the correct JSContext* gets pushed v1
Given that the reporter verified that the fix is good in comment 29, I'm going to approve for Aurora and Beta in order to get this into beta3 and give a few more days to catch any fallout.
Attachment #8463291 -
Flags: approval-mozilla-beta?
Attachment #8463291 -
Flags: approval-mozilla-beta+
Attachment #8463291 -
Flags: approval-mozilla-aurora?
Attachment #8463291 -
Flags: approval-mozilla-aurora+
Updated•11 years ago
|
Attachment #8463557 -
Flags: approval-mozilla-beta?
Attachment #8463557 -
Flags: approval-mozilla-beta+
Attachment #8463557 -
Flags: approval-mozilla-aurora?
Attachment #8463557 -
Flags: approval-mozilla-aurora+
Comment 36•11 years ago
|
||
Updated•11 years ago
|
Flags: in-testsuite+
Comment 37•11 years ago
|
||
This doesn't meet ESR criteria.
Comment 38•10 years ago
|
||
I'd like to request ESR approval again. I have an enterprise client that is hitting it and it's a tiny fix.
And it's a regression (which does meat ESR criteria).
Updated•10 years ago
|
Keywords: regressionwindow-wanted
You need to log in
before you can comment on or make changes to this bug.
Description
•