Closed
Bug 625187
Opened 13 years ago
Closed 13 years ago
Alert origin is no longer shown for third-party iframes
Categories
(Toolkit :: General, defect)
Toolkit
General
Tracking
()
RESOLVED
FIXED
mozilla2.0b12
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
People
(Reporter: jruderman, Assigned: msucan)
References
Details
(Keywords: regression, testcase, Whiteboard: [sg:low spoof][good first bug][softblocker])
Attachments
(2 files, 3 obsolete files)
61 bytes,
text/html
|
Details | |
7.79 KB,
patch
|
Details | Diff | Splinter Review |
The fix for bug 59314 removed the origin indication in alerts. Most of the time, this is reasonable, since the address bar shows the right thing. But for alerts that come from third-party iframes, maybe we should re-add it, to prevent the third-party iframe content from making it seem like the main page is saying something.
Updated•13 years ago
|
Assignee: nobody → dolske
Whiteboard: [sg:want?] → [sg:low spoof]
I found a way to do spoofing without iframes. May I attach the testcase here?
Reporter | ||
Comment 3•13 years ago
|
||
shutdown, please file a separate bug for that, possibly security-sensitive.
(In reply to comment #3) > shutdown, please file a separate bug for that, possibly security-sensitive. OK. I filed bug 626775.
Comment 5•13 years ago
|
||
Yeah, we should probably readd the origin text -- at least when the the prompting window's origin != window.top. It's unlikely to have much practical effect, as I suspect most users ignore the text anyway.
Comment 6•13 years ago
|
||
This shouldn't actually be very hard to do -- the text (prompt title) is already there but invisible, it's the <description anonid="info.title" hidden="true"/> in tabprompts.xml.
Whiteboard: [sg:low spoof] → [sg:low spoof][good first bug]
Does this block? It is a regression, and not fixing it poses at least some amount of security risk.
blocking2.0: --- → ?
Updated•13 years ago
|
blocking2.0: ? → final+
Whiteboard: [sg:low spoof][good first bug] → [sg:low spoof][good first bug][softblocker]
Assignee | ||
Comment 8•13 years ago
|
||
As a user I'd like the title to return in some way or another. I do find web pages that show alerts annoying and not knowing the origin is problematic - I actually use such information to determine what action I shall take - being aware that a site can have hidden iframes and stuff like that. Given there is no patch yet, I am submitting a quick fix, a patch that shows the title in tab prompts. Feedback is welcome. Thanks!
Attachment #511354 -
Flags: feedback?(dolske)
Comment 9•13 years ago
|
||
Comment on attachment 511354 [details] [diff] [review] quick fix I think we need to something like what I suggested in comment 5, and not just always show the alert origin.
Attachment #511354 -
Flags: feedback?(dolske) → feedback-
Assignee | ||
Comment 10•13 years ago
|
||
Updated the patch as per comment 5. Thanks for your feedback! My rationale was that it might be confusing to users who get such messages, not knowing their origin, even if they come from the main page (the window.top location). It might be confusing to only show the location at times. It's like showing a menu item only when a specific condition is met. The train of thought was that users should always expect to see the domain name and protocol, if not... something is wrong. In the rare case when a prompt from a different domain shows up, users won't expect to look for the location - they might even believe the title is part of the message. (or maybe I am wrong :) ) Nonetheless, this is better than not showing the title at all (like now). Please let me know if I have to make further changes to the patch. Thanks!
Attachment #511354 -
Attachment is obsolete: true
Attachment #511954 -
Flags: review?(dolske)
Updated•13 years ago
|
Assignee: dolske → mihai.sucan
Comment 11•13 years ago
|
||
(In reply to comment #10) > My rationale was that it might be confusing to users who get such messages, not > knowing their origin, even if they come from the main page (the window.top > location). I think it's more likely that people won't understand the notion of mixed-origin pages, and would be surprised to learn that when siteA.com is showing in the URL bar, something from siteB.org might be still involved. > It might be confusing to only show the location at times. Feature, not a bug. ;) When it shows up, it's a sign that there's something different going on. Of course, most people will still ignore it, but there's the opportunity for others to see and act accordingly.
Comment 12•13 years ago
|
||
Comment on attachment 511954 [details] [diff] [review] updated patch >--- a/toolkit/components/prompts/content/tabprompts.xml >+ // Show the tabprompt title that shows the prompt origin when >+ // the prompt location is not the same as that of the top >+ // window. >+ let topLocation = this.args.domWindow.top.location; >+ let promptLocation = this.args.domWindow.location; >+ if (topLocation.host != promptLocation.host || >+ topLocation.protocol != promptLocation.protocol) >+ this.ui.infoTitle.removeAttribute("hidden"); >+ Right idea, though now that I think about it more I think we may actually want a nsIPrincipal check here instead... var topPrincipal = this.args.domWindow.top.document.nodePrincipal; var winPrincipal = this.args.domWindow.document.nodePrincipal; if (topPrincipal.equal(winPrincipal)) this.ui.infoTitle.removeAttribute("hidden"); [I guess there are technically 3 principals involved here? The top window, the frame window, and the calling script. But the calling script should only be able to touch window.alert() on one of those if it's same-origin, so therefore there can be at most 2 different principals?]
Attachment #511954 -
Flags: review?(dolske) → review-
Comment 13•13 years ago
|
||
CCing bz because principals still confuse me.
Comment 14•13 years ago
|
||
...oh, and a test to exercise this case would be super-swell.
Assignee | ||
Comment 15•13 years ago
|
||
Updated the patch based on review. Thanks! Changes: - using the nsIPrincipal of the documents to check if the origin should show or not. Having read MDN I think this is fine - since it does check domain name, port and protocol - pretty much what I did, but it also does some certificate fingerprint checks and overall I think the comparison is better (safer). - added a mochitest-plain which checks that the origin is shown when needed. Please let me know if further changes are needed. Thanks!
Attachment #511954 -
Attachment is obsolete: true
Attachment #512183 -
Flags: review?(dolske)
Comment 16•13 years ago
|
||
There are actually 4 semi-relevant principals here: 1) The principal of the toplevel window. 2) The principal of the window whose alert method is called. 3) The principal of the script that made the alert() call. 4) The principal of the script in which we entered JS. These can all be different, in general, though 2,3,4 all have to test equal to each other in this case, as long as we ignore enablePrivilege (whether because they're same-origin to start with, or because of document.domain changes). Usually, at least. It's possible to construct cases where .domain is set and then unset in which 2,3,4 don't all test equal but can touch each other... I wouldn't worry about those. The attached patch (update 3) looks reasonable to me.
Comment 17•13 years ago
|
||
Comment on attachment 512183 [details] [diff] [review] patch update 3 >--- a/toolkit/components/prompts/content/tabprompts.xml >+ // Show the tabprompt title that shows the prompt origin when >+ // the prompt origin is not the same as that of the top window. Nit: Change first "Show" to "Display" so it reads a bit smoother. >+++ b/toolkit/components/prompts/test/test_bug625187.html >+<iframe src="http://example.com/tests/toolkit/components/prompts/test/bug625187_iframe.html"></iframe> I think it would be useful (and simple) to extend the test to also check: * iframe is same-origin as parent, iframe calls window.alert() * iframe is same-origin as parent, iframe calls window.parent.alert() I'd just modify your iframe page to have another button for calling window.parent.alert(), and then load it in <iframe src="bug625187_iframe.html"></iframe>. r+a=me with that.
Attachment #512183 -
Flags: review?(dolske)
Attachment #512183 -
Flags: review+
Attachment #512183 -
Flags: approval2.0+
Assignee | ||
Comment 18•13 years ago
|
||
Updated the patch as requested, to include more tests. Thanks for the review+ and for the approval+! Will push this patch to the try server and post results.
Attachment #512183 -
Attachment is obsolete: true
Assignee | ||
Comment 19•13 years ago
|
||
Try server builds and logs: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mihai.sucan@gmail.com-f4324a3062de Try server results: http://tbpl.mozilla.org/?tree=MozillaTry&rev=f4324a3062de They look good to me, but please confirm. There are some test failures which look like the usual noise, seemingly unrelated to the patch.
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Comment 20•13 years ago
|
||
Looks fine to me.
Comment 22•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/a23d25fcc25d
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•