If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[FIX]SameOrSubdomainOfTarget should die

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
Security
P1
normal
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla1.9alpha1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
wanted1.9 +
blocking1.9a1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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
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).
Created attachment 232287 [details] [diff] [review]
Patch.  Still needs testing.

I'll test this once landfill.bugzilla.org is actually alive again.  :(
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....
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.

sr=jst
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
Created attachment 233913 [details] [diff] [review]
Updated to trunk
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.
r=dveditz
Attachment #233913 - Flags: review?(dveditz) → review+
Blocks: 355948
Fixed.
Status: NEW → RESOLVED
Last Resolved: 11 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.