Closed Bug 327109 Opened 19 years ago Closed 18 years ago

[FIX]SameOrSubdomainOfTarget should die


(Core :: Security, defect, P1)






(Reporter: bzbarsky, Assigned: bzbarsky)




(2 files, 2 obsolete files)

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.
We should fix this for 1.9, imo.
Flags: blocking1.9a1+
Flags: blocking1.9-
Whiteboard: [wanted-1.9]
Assignee: dveditz → bzbarsky
Priority: -- → P1
Target Milestone: --- → mozilla1.9alpha
Attached file 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).
Attached patch Patch. Still needs testing. (obsolete) — Splinter Review
I'll test this once is actually alive again.  :(
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....
Attachment #232455 - Flags: superreview?(jst)
Attachment #232455 - Flags: review?(dveditz)
Summary: SameOrSubdomainOfTarget should die → [FIX]SameOrSubdomainOfTarget should die
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.

Attachment #232455 - Flags: superreview?(jst) → superreview+
Yeah, this is trunk-only as far as I'm concerned, unless something _really_ weird happens.
Depends on: 332182
Attached patch Updated to trunkSplinter Review
Attachment #232287 - Attachment is obsolete: true
Attachment #232455 - Attachment is obsolete: true
Attachment #233913 - Flags: review?(dveditz)
Attachment #232455 - Flags: review?(dveditz)
Comment on attachment 233913 [details] [diff] [review]
Updated to trunk

Looks good, much better to have the rules enforced in a common spot.
Attachment #233913 - Flags: review?(dveditz) → review+
Blocks: 355948
Closed: 18 years ago
Resolution: --- → FIXED
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
You need to log in before you can comment on or make changes to this bug.