Bug 43891 - location.href does not throw SECURITY_ERR when accessed across origins with JSC bindings
Summary: location.href does not throw SECURITY_ERR when accessed across origins with J...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Mike West
URL:
Keywords:
Depends on: 109887 110018
Blocks: 43504
  Show dependency treegraph
 
Reported: 2010-08-11 18:32 PDT by Mihai Parparita
Modified: 2013-03-06 01:46 PST (History)
14 users (show)

See Also:


Attachments
Patch (15.00 KB, patch)
2010-08-12 17:36 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (28.93 KB, patch)
2013-02-04 04:00 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (31.32 KB, patch)
2013-02-04 06:00 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch for landing (31.48 KB, patch)
2013-02-13 02:36 PST, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Parparita 2010-08-11 18:32:43 PDT
See bug 43504 for details and test case. This is about fixing the JSC bindings.
Comment 1 Mihai Parparita 2010-08-12 17:36:18 PDT
Created attachment 64286 [details]
Patch
Comment 2 Mihai Parparita 2010-08-12 17:48:33 PDT
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 3 Adam Barth 2010-08-12 17:52:14 PDT
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!
Comment 4 Sam Weinig 2010-08-12 17:58:23 PDT
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.
Comment 5 Mihai Parparita 2010-08-12 18:00:07 PDT
(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 6 Adam Barth 2010-08-12 18:19:06 PDT
Comment on attachment 64286 [details]
Patch

Clearing the review flag based on the above discussion.
Comment 7 Sam Weinig 2010-08-12 19:08:02 PDT
(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.
Comment 8 Mihai Parparita 2010-08-12 20:08:50 PDT
(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.
Comment 9 Kyle Adams 2011-06-15 18:23:48 PDT
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.
Comment 10 Adam Barth 2011-06-15 18:44:45 PDT
(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.
Comment 11 Kyle Adams 2011-06-16 06:39:59 PDT
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.
Comment 12 Kyle Adams 2011-06-16 06:40:58 PDT
(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 :-)
Comment 13 Adam Barth 2011-06-16 10:37:06 PDT
> 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.
Comment 14 Mihai Parparita 2011-06-16 14:16:52 PDT
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.
Comment 15 Mike West 2012-09-28 00:39:05 PDT
Grabbing this as part of my nacient error message cleanup extravaganza.
Comment 16 David Levin 2012-09-28 01:21:14 PDT
(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.
Comment 17 Mike West 2013-02-04 04:00:55 PST
Created attachment 186342 [details]
Patch
Comment 18 Build Bot 2013-02-04 04:43:32 PST
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 19 Build Bot 2013-02-04 05:32:15 PST
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 20 Build Bot 2013-02-04 05:40:18 PST
Comment on attachment 186342 [details]
Patch

Attachment 186342 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16378092
Comment 21 Mike West 2013-02-04 06:00:07 PST
Created attachment 186362 [details]
Patch
Comment 22 Adam Barth 2013-02-06 12:47:07 PST
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.
Comment 23 Mike West 2013-02-07 02:27:18 PST
(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.
Comment 24 Adam Barth 2013-02-07 11:03:02 PST
> 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 25 Adam Barth 2013-02-07 11:04:05 PST
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.
Comment 26 Mike West 2013-02-07 11:22:35 PST
(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.
Comment 27 Mike West 2013-02-13 02:36:41 PST
Created attachment 188038 [details]
Patch for landing
Comment 28 Mike West 2013-02-13 02:39:27 PST
(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 29 WebKit Review Bot 2013-02-13 03:20:48 PST
Comment on attachment 188038 [details]
Patch for landing

Clearing flags on attachment: 188038

Committed r142734: <http://trac.webkit.org/changeset/142734>
Comment 30 WebKit Review Bot 2013-02-13 03:20:53 PST
All reviewed patches have been landed.  Closing bug.
Comment 31 Geoffrey Garen 2013-02-16 08:13:43 PST
This patch seems to have caused a crash: https://bugs.webkit.org/show_bug.cgi?id=110017.
Comment 32 WebKit Review Bot 2013-02-16 08:28:29 PST
Re-opened since this is blocked by bug 110018
Comment 33 Geoffrey Garen 2013-02-16 14:24:44 PST
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.
Comment 34 Mike West 2013-02-20 06:49:12 PST
(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. :)
Comment 35 Maciej Stachowiak 2013-03-06 01:46:29 PST
(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.