327109 - [FIX]SameOrSubdomainOfTarget should die

Status: RESOLVED FIXED

Description

[Boris Zbarsky [:bz] (no decent commit message means r-)]

This method in docshell duplicates parts of SecurityCompareURIs, but not other parts...

I think we should have a caps method that does what needs doing here; that way we can reuse SecurityCompareURIs (or factor better) and not have this weird version skew between two methods that are trying to do very similar things.

Comment 1

[Boris Zbarsky [:bz] (no decent commit message means r-)]

We should fix this for 1.9, imo.

Comment 2

[Boris Zbarsky [:bz] (no decent commit message means r-)]

Created attachment 232284 [details]
Testcase

Based on testing other UAs:

NS4 and IE6 and IE7 allow anyone to target anything.

Opera allows same-domain targeting and ignores document.domain.

I think both are wrong.  We should simply be doing a same-origin check here (which takes document.domain into account).

Comment 3

[Boris Zbarsky [:bz] (no decent commit message means r-)]

Created attachment 232287 [details] [diff] [review]
Patch.  Still needs testing.

I'll test this once landfill.bugzilla.org is actually alive again.  :(

Comment 4

[Boris Zbarsky [:bz] (no decent commit message means r-)]

Created attachment 232455 [details] [diff] [review]
OK, this gives the behavior I'd expect

The major change here, in my opinion, is that two unrelated (neither opened the other) windows from the same site with different document.domain values will not be able to target each other anymore.  I think that's what we want, frankly....

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 232455 [details] [diff] [review]
OK, this gives the behavior I'd expect

This does seem to be what we want. The code changes look good, the only thing that is a bit worrisome is that there *is* a change in behaviour, and that *could* make some sites behave differently (though they shouldn't completely break, things might simply open in new windows rather than reusing existing ones). If we'd be pushing this in on the branch I'd love to see this sit on the trunk for some time before we take it on the branch, but it doesn't look like we'll be attempting to make this change on the branch.

sr=jst

Comment 6

[Boris Zbarsky [:bz] (no decent commit message means r-)]

Yeah, this is trunk-only as far as I'm concerned, unless something _really_ weird happens.

Comment 7

[Boris Zbarsky [:bz] (no decent commit message means r-)]

Created attachment 233913 [details] [diff] [review]
Updated to trunk

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 233913 [details] [diff] [review]
Updated to trunk

Looks good, much better to have the rules enforced in a common spot.
r=dveditz

Comment 9

[Boris Zbarsky [:bz] (no decent commit message means r-)]

Fixed.