Closed Bug 645115 Opened 9 years ago Closed 9 years ago
With two scripts in inner
HTML, the first one runs, even though neither should run
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:2.0.0) Gecko/20100101 Firefox/4.0 Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0.0) Gecko/20100101 Firefox/4.0 A script tag added via innerHTML won't fire, unless it is followed by another script tag (just an opening tag required) Reproducible: Always Steps to Reproduce: 1. Load attached example 2. Click button "Add one script element" -> script element added, but script doesn't fire 3. Click button "Add two script elements" -> script elements added, first one fires It doesn't matter how many script elements you add, all but the last element execute correctly. The last element just needs an opening script tag to make the previous script fire, a closing tag by itself won't work
Slightly simpler test case
Attachment #521891 - Attachment is obsolete: true
Should not execute at all!? Other browsers don't, Fx3.6 did in both cases.
Severity: major → normal
Component: General → HTML: Parser
OS: Linux → All
Product: Firefox → Core
QA Contact: general → parser
Hardware: x86_64 → All
Version: 4.0 Branch → Trunk
Toggling html5.parser.enable makes the 3.6.x style behaviour return. Therefore caused by the new html 5 parser in Firefox 4, bug 373864. The testcase 'doesn't work' in Chrome 12.0.712.0 dev either. As jj said, surely this should have never worked, so this is presumably invalid?
Agreed - to me it doesn't matter if it works one way or the other, as long as it is consistent. We have to handle both cases anyway: browsers-that-do and browsers-that-don't.
Uh... this looks broken. Henri, what's going on?
Assignee: nobody → hsivonen
Status: UNCONFIRMED → NEW
Ever confirmed: true
This is a security bug in the sense that a script that shouldn't run runs, which could, in theory, lead to XSS if a site was relying on spec compliance to avoid XSS. In practice, the security impact is very, very low for the time being, because any site vulnerable because of this would be vulnerable in Firefox 3.6 anyway where the behavior is even expected. Hence, not marking confidential but nominating for a point release anyway. The bug here is that tree ops aren't flushed in the fragment case when the parser core yields due to a script, but the script execution prevention code assumes that ops get flushed when the parser core yields due to a script.
Nominating for point release, because a script gets run when scripts aren't supposed to run. (Mitigating factor: Firefox 3.6 always runs scripts in that case, so sites shouldn't be relying on scripts not running.)
Status: NEW → ASSIGNED
blocking2.0: --- → ?
Summary: script tag added with innerHTML requires a second script tag before it is executed → With two scripts in innerHTML, the first one runs, even though neither should run
Now with the mochitest actually hg added.
Comment on attachment 522327 [details] [diff] [review] Flush tree ops in the fragment case when the tree builder yields due to a script, v2 The assertions I added fire when I expected them not to. I need to look at this again.
The fix itself was ok. The assertions I added were bogus, so I removed the assertions.
Comment on attachment 522650 [details] [diff] [review] Flush tree ops in the fragment case when the tree builder yields due to a script, v3 r=me, but add a comment explaining why this block is needed.
Attachment #522650 - Flags: review?(bzbarsky) → review+
(In reply to comment #14) > Comment on attachment 522650 [details] [diff] [review] > Flush tree ops in the fragment case when the tree builder yields due to a > script, v3 > > r=me, but add a comment explaining why this block is needed. Thanks. Comment added and landed into cedar: http://hg.mozilla.org/projects/cedar/rev/0e91598cc3f9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
If 3.6 was running scripts in this case then it's not clear why this can't wait for Firefox 5. If we really need it in macaw please explain.
blocking2.0: ? → -
It can't wait till FF 5 because it's a clear regression from FF 3.6. For us ( SugarCRM ) it is blocking official FF 4 support.
John, what's a regression from 3.6, exactly? In 3.6 both scripts ran. In 4.0 only one of them runs. In 5.0 neither one will run. If you code is assuming the 3.6 behavior, it won't work in 5.0. If it's not assuming that, then what _is_ it assuming?
The code is assuming the 3.6 behavior, which works across Safari, IE, and Chrome, but not FF 4+. If the plan in FF 5 for none of these scripts to run, I think many more applications will break as well.
John, the behavior the HTML5 spec calls for is that neither of those scripts should run. So yes, that's the behavior we will be shipping in Firefox 5.
And for what it's worth, on the testcase attached to this bug Chrome 10 and Chrome 11 beta and Chrome 12 dev don't run the scripts. Neither does Safari 5. Neither does Opera 11. In fact, Firefox 3.6 is the only browser I have here that runs the scripts in that situation (modulo the bug in Firefox 4). So I have to assume that whatever your app is doing is completely different from what this bug is about. Might be worth reporting a separate bug on whatever it's actually doing.
OK, thanks for the feedback. You are right, that our use-case was slightly different than the attached case, but for some reason we were seeing the same behavior as the original poster. I did some testing with the latest trunk build of FF ( Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.2a1pre) Gecko/20110411 Firefox/4.2a1pre ) and our use-case seems to work correctly there, so it sounds like we have some handling in place for script tags that works for all browsers except FF 4.. I'm guessing some form of this bug was present in FF 4 that was causing the problem seen. Is there anyway this could be backported to FF 4? I'd be more than happy to test out a build with this fix in it to see if it fixes the problem. We'd like to be able to add FF 4 support ASAP without having to add one-off hacks in our codebase.
John, I'm not sure about backporting. It looks like there will be only one security update between Firefox 4 and the Firefox 5 release in June. As far as testing, I pushed just this change applied on top of the Firefox 4 release changset to the try server to build a corresponding Mac release build. Once there's a link to the resulting build, I'll post it here.
That seems to fix the issue :-). Thanks for creating the build for me. If there was anyway this could make it into a security update for FF4, it would mean we could be supporting FF 4 officially. What sort of timetable would that be in?
I think there's only one security update planned for Fx4 at the moment, as I said, and that's locking down soon (matter of days). Henri, do you think this patch is appropriate for branch?
Comment on attachment 522650 [details] [diff] [review] Flush tree ops in the fragment case when the tree builder yields due to a script, v3 (In reply to comment #27) > Henri, do you think this patch is appropriate for branch? Yes, I think this is appropriate for the branch. It's very low risk and it fixes pretty bogus behavior. See comment 8 for how this can be rationalized as a security fix, but see also comment 17.
Attachment #522650 - Flags: approval2.0?
Note to the person processing approvals: Per comment 18 and comment 26, getting this fix into Macaw would help SugarCRM support Firefox 4.
Thanks everyone for all your help on this; I really appreciate it!
As far as I can tell, this missed the chance to be in a point release unless the point release is respun with ridealongs.
Comment on attachment 522650 [details] [diff] [review] Flush tree ops in the fragment case when the tree builder yields due to a script, v3 Not doing another 4.0.x release, 5.0 will be out soon.
Attachment #522650 - Flags: approval2.0? → approval2.0-
You need to log in before you can comment on or make changes to this bug.