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)

defect
Not set
normal

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)

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.
Assignee: nobody → dolske
Whiteboard: [sg:want?] → [sg:low spoof]
I found a way to do spoofing without iframes.
May I attach the testcase here?
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.
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.
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: --- → ?
blocking2.0: ? → final+
Whiteboard: [sg:low spoof][good first bug] → [sg:low spoof][good first bug][softblocker]
Attached patch quick fix (obsolete) — Splinter Review
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 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-
Attached patch updated patch (obsolete) — Splinter Review
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)
Assignee: dolske → mihai.sucan
(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 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-
CCing bz because principals still confuse me.
...oh, and a test to exercise this case would be super-swell.
Attached patch patch update 3 (obsolete) — Splinter Review
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)
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 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+
Attached patch patch update 4Splinter Review
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
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
Looks fine to me.
Great! Then the patch can be checked-in.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/a23d25fcc25d
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla2.0b12
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 636197
You need to log in before you can comment on or make changes to this bug.