| Summary: | location.href does not throw SECURITY_ERR when accessed across origins with JSC bindings | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mihai Parparita <mihaip> | ||||||||||
| Component: | WebCore JavaScript | Assignee: | Mike West <mkwst> | ||||||||||
| Status: | REOPENED --- | ||||||||||||
| Severity: | Normal | CC: | abarth, ahmad.saleem792, annevk, ap, buildbot, ggaren, kyle, levin, mihaip, mjs, mkwst, rniwa, sam, syoichi, tnainani, webkit.review.bot | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Bug Depends on: | 109887, 110018 | ||||||||||||
| Bug Blocks: | 43504 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Mihai Parparita
2010-08-11 18:32:43 PDT
Created attachment 64286 [details]
Patch
Sam/Adam/Maciej: Since you have either reviewed or made security-relate changes to the location object in the past (r44135, r48619, r51757), could one of you take a look at this (there's more details at the parent bug 43504)? (I'm not looking to land this until the V8 side is sorted out too, that's in bug 43892). Comment on attachment 64286 [details] Patch > Gecko and IE do throw the exception. Can you add this information (and the spec reference) to the ChangeLog? Generally, it's helpful if the ChangeLog explains why we're making these changes without having to hunt through the bug database. Otherwise, looks good. Thanks! We have historically not thrown in these cases. If we want to move to throwing on cross-origin errors I would prefer to discuss it in a more general forum. (In reply to comment #4) > We have historically not thrown in these cases. If we want to move to throwing on cross-origin errors I would prefer to discuss it in a more general forum. Sure, should I start a thread on webkit-dev@ or is there a more appropriate forum? (I'd also be curious about the historical reasons, since http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#security-location is pretty explicit about the error being thrown). Comment on attachment 64286 [details]
Patch
Clearing the review flag based on the above discussion.
(In reply to comment #5) > (In reply to comment #4) > > We have historically not thrown in these cases. If we want to move to throwing on cross-origin errors I would prefer to discuss it in a more general forum. > > Sure, should I start a thread on webkit-dev@ or is there a more appropriate forum? (I'd also be curious about the historical reasons, since http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#security-location is pretty explicit about the error being thrown). webkit-dev is fine. There are two parts to why this is the current case. 1) We are generally weary of throwing exceptions. 2) There was no specification on cross-origin violations when we implemented our checks. (In reply to comment #7) > webkit-dev is fine. There are two parts to why this is the current case. 1) We are generally weary of throwing exceptions. 2) There was no specification on cross-origin violations when we implemented our checks. OK, just emailed webkit-dev. Thanks for the background information. Just wondering if there was any update on this issue. I followed the thread on webkit-dev and it never seemed to come to any sort of consensus. We're also seeing the error show up on SourceForge for many of the ads, which is annoying when trying to track down real ad issues. (In reply to comment #9) > Just wondering if there was any update on this issue. I followed the thread on webkit-dev and it never seemed to come to any sort of consensus. We're also seeing the error show up on SourceForge for many of the ads, which is annoying when trying to track down real ad issues. Re-reading the thread, it seems like folks aren't that enthusiastic about changing our behavior unless there's a compatibility reason to change. This is the kind of change that can cause compatibility problems (in either direction). Personally, I think throwing would be slightly better, but I don't feel that strongly about it. In my reading of the thread it seemed like there was a disconnect between the webkit devs and the folks who are enthusiastic about seeing it fixed. In particular, if you look at the Chromium bug (http://crbug/17325), there are a several replies to Mihai's request for real world examples at the end. Those have to be combined with some of the other real world problems (FCKEditor, Google Talk, etc.) mentioned in the thread for a more accurate picture of interest. The compatibility argument doesn't seem particularly strong given that catching the error is what IE and Firefox already do. (In reply to comment #11) > The compatibility argument doesn't seem particularly strong given that catching the error is what IE and Firefox already do. Or more accurately, compatibility seems to argue in favor of fixing this rather than ignoring it :-) > In my reading of the thread it seemed like there was a disconnect between the webkit devs and the folks who are enthusiastic about seeing it fixed. Yes. :) > In particular, if you look at the Chromium bug (http://crbug/17325), there are a several replies to Mihai's request for real world examples at the end. Those seem to be that folks at Sprout and Facebook have run into this problem and worked around it somehow (or just their sites work less well in WebKit-based browsers). > The compatibility argument doesn't seem particularly strong given that catching the error is what IE and Firefox already do. Generally, we like to align our behavior with IE and Firefox. You make a good case. Maybe we should give this a try and see if it causes problems. I agree that (In reply to comment #13) > You make a good case. Maybe we should give this a try and see if it causes problems. I agree that this is worth trying, but I won't have time to resurrect my patch, so I'm leaving this up for grabs. Grabbing this as part of my nacient error message cleanup extravaganza. (In reply to comment #9) > Just wondering if there was any update on this issue. I followed the thread on webkit-dev and it never seemed to come to any sort of consensus. We're also seeing the error show up on SourceForge for many of the ads, which is annoying when trying to track down real ad issues. Kyle, you may find ancestorOrigins useful (http://trac.webkit.org/changeset/113945). It helps in some of these scenarios but not all of course. Created attachment 186342 [details]
Patch
Comment on attachment 186342 [details] Patch Attachment 186342 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16367223 New failing tests: http/tests/security/sandboxed-iframe-blocks-access-from-parent.html Comment on attachment 186342 [details] Patch Attachment 186342 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16371202 New failing tests: http/tests/security/sandboxed-iframe-blocks-access-from-parent.html Comment on attachment 186342 [details] Patch Attachment 186342 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16378092 Created attachment 186362 [details]
Patch
Comment on attachment 186362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186362&action=review > Source/WebCore/ChangeLog:25 > + This is the JSC half of the patch. V8 is coming in http://wkbug.com/43892 Perhaps we should have an ENABLE flag for this change in behavior since we have some reason to believe it comes with compatibility risks. (In reply to comment #22) > (From update of attachment 186362 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186362&action=review > > > Source/WebCore/ChangeLog:25 > > + This is the JSC half of the patch. V8 is coming in http://wkbug.com/43892 > > Perhaps we should have an ENABLE flag for this change in behavior since we have some reason to believe it comes with compatibility risks. Do we? The claim made here is that the web must already be checking for thrown exceptions, as that's what other major vendors seem to be doing already. Spot checks against a few ad servers line up with this assumption. I don't think an ENABLE flag is necessary, but I'll certainly add it if you disagree. > Do we? The claim made here is that the web must already be checking for thrown exceptions, as that's what other major vendors seem to be doing already. Spot checks against a few ad servers line up with this assumption.
My worry is more about non-web uses of WebKit, such as by Mac Apps and Dashboard widgets. The change is very localized, so I guess it will be easy to revisit if we do run into compat problems.
Comment on attachment 186362 [details]
Patch
I would wait until Tuesday to land the V8 side of this patch for the same reason as the nosniff patch: that will give us a full Dev cycle to find compat problems before the change is promoted to Beta.
(In reply to comment #25) > (From update of attachment 186362 [details]) > I would wait until Tuesday to land the V8 side of this patch for the same reason as the nosniff patch: that will give us a full Dev cycle to find compat problems before the change is promoted to Beta. I'm happy to wait, thanks for the review. Created attachment 188038 [details]
Patch for landing
(In reply to comment #27) > Created an attachment (id=188038) [details] > Patch for landing Chromium has branched; I'll go ahead and land the JSC side now, and work with the V8 folks to being the Chromium side to parity in http://wkbug.com/43892 Comment on attachment 188038 [details] Patch for landing Clearing flags on attachment: 188038 Committed r142734: <http://trac.webkit.org/changeset/142734> All reviewed patches have been landed. Closing bug. This patch seems to have caused a crash: https://bugs.webkit.org/show_bug.cgi?id=110017. Re-opened since this is blocked by bug 110018 The ASSERT in bug 109838 might explain why this was crashing -- it seems that, in at least one code path, if this exception is thrown, it is never processed by the VM. (In reply to comment #33) > The ASSERT in bug 109838 might explain why this was crashing -- it seems that, in at least one code path, if this exception is thrown, it is never processed by the VM. :( I'm not having much luck constructing a layout test that crashes. Likewise, running the relevant existing layout tests 10000 times hasn't produced a single crash locally. I'd appreciate some help tracking this down. :) (In reply to comment #34) > (In reply to comment #33) > > The ASSERT in bug 109838 might explain why this was crashing -- it seems that, in at least one code path, if this exception is thrown, it is never processed by the VM. > > :( > > I'm not having much luck constructing a layout test that crashes. Likewise, running the relevant existing layout tests 10000 times hasn't produced a single crash locally. I'd appreciate some help tracking this down. :) https://bugs.webkit.org/show_bug.cgi?id=110017 mentions a number of sites that crashed pretty reliably when this change was in the tree. |