Open Bug 604289 Opened 14 years ago Updated 4 years ago

Content still can steal focus from chrome by window.focus();

Categories

(Core :: DOM: Core & HTML, defect, P3)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
blocking2.0 --- .x+
status2.0 --- wanted

People

(Reporter: masayuki, Unassigned)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(2 files, 3 obsolete files)

one testcase
315 bytes, text/html
Details
Patch v1.0
8.65 KB, patch
Details | Diff | Splinter Review
Attached patch Patch v0.1 (obsolete) — — Splinter Review 
> data:text/html,<body onload="setTimeout(function () { window.focus(); document.getElementById('i').focus(); }, 3000);"><input id="i">

Like this, window.focus() can steal focus from chrome.

Here is a draft patch. I'll clean up it after bug 591890.
Flags: in-testsuite?
blocking2.0: --- → ?
Comment on attachment 483079 [details] [diff] [review]
Patch v0.1


>+PRBool
>+nsFocusManager::AllowsToBeStolenFocusBy(nsPIDOMWindow* aNewFocusWindow)
>+{
>+  NS_ENSURE_TRUE(aNewFocusWindow, PR_FALSE);
>+
>+  // When the following conditions are true:
>+  //  * an element has focus
>+  //  * isn't called by trusted event (i.e., called by untrusted event or by js)
>+  //  * the focus is moved to a different window
>+  // then, we need to check the permission.
>+
>+  if (!mFocusedContent ||
>+      mFocusedContent->GetOwnerDoc()->GetWindow() == aNewFocusWindow) {
>+    return PR_TRUE;
>+  }

I'm not quite sure I understand this. What if no element is focused, but only window?
Shouldn't you use mFocusedWindow?
> I'm not quite sure I understand this. What if no element is focused, but only
> window?
> Shouldn't you use mFocusedWindow?

Doesn't that break focus() methods excessively? I'll try to create such patch, wait a couple of hours.
Attached patch Patch v0.2 (obsolete) — — Splinter Review 
posted to tryserver.
Attachment #483079 - Attachment is obsolete: true
Comment on attachment 483126 [details] [diff] [review]
Patch v0.2


>+PRBool
>+nsFocusManager::AllowsToBeStolenFocusBy(nsPIDOMWindow* aNewFocusWindow)
>+{
>+  // When the following conditions are true:
>+  //  * an element has focus
>+  //  * isn't called by trusted event (i.e., called by untrusted event or by js)
>+  //  * the focus is moved to a different window
>+  // then, we need to check the permission.
>+
>+  // If there is a focused window, caller should be able to access the window
>+  // when it can steal focus from the window.
>+  if (mFocusedWindow != aNewFocusWindow &&
>+      !nsContentUtils::CanCallerAccess(mFocusedWindow)) {
Just wondering. Do we really want to be this strict.
Would it be enough to do this check only if mFocusedWindow is a chrome window?
Should a web site be able to steal focus from another web site?
And I'm not sure the mFocusedContent check is needed or not after the check for mFocusedWindow.
(In reply to comment #5)
> Should a web site be able to steal focus from another web site?
Yeah, this is the question.
Better to test how current browsers work.
Looks google chrome doesn't steal focus from another site.

I opened two windows. One is:

> data:text/html,<body onload="setTimeout(function () { window.focus(); document.getElementById('i').focus(); }, 10000);"><input id="i">

The other is google top page.

If I focus to google top page's window, the data page cannot steal focus.
But can an iframe steal focus from the window.parent? That is IMO
more interesting case.
And what does IE do?
Chrome has a bit strange behavior in some cases.
The testcase shows a regression.
FF3.6, Opera and Chrome, at least, do allow moving focus. FF4 doesn't.
How about these behavior?

* chrome can steal focus from everyone
* content cannot steal focus from chrome
* content can steal focus from its ancestor or descendant content window
* content can steal focus from its accessible content window
Sounds reasonable.
Jst or Jonas may have something to say here.
Attached patch Patch v0.3 (obsolete) — — Splinter Review 
I'm not sure whether I can write iframe test for third condition of comment 13...
Attachment #483126 - Attachment is obsolete: true
The behavior described in comment 13 sounds reasonable to me. And blocking on this.
blocking2.0: ? → betaN+
Depends on: 591890
Attached patch Patch v1.0 — — Splinter Review 
nsContentUtils::GetDocumentFromCaller() crashes during running browser_focus_steal_from_chrome.js...
Attachment #483391 - Attachment is obsolete: true
Ah, hmm, could you not use nsContentUtils::GetDocumentFromCaller(), but
subject principal from security manager? Would that work?
Then do the comparison using nsIPrincipal::Subsumes.

(The crash is still a valid bug, IMHO. nsContentUtils::GetDocumentFromCaller() should just return null in that case.)
Hmm, no, you really need the document...
Does GetDocumentFromContext() fail too?
Or if nothing else works, perhaps we need to prevent few methods 
to be quickstubbed.
Like 'nsIDOMHTMLInputElement.select' in dom_quickstubs.qsconf, perhaps
also other selection related methods.
Maybe there aren't that many problematic cases after all.
(In reply to comment #18)
> (The crash is still a valid bug, IMHO. nsContentUtils::GetDocumentFromCaller()
> should just return null in that case.)

I think I disagree (and the documentation disagrees with the implementation, see also the assert in GetDocumentFromCaller).

(In reply to comment #22)
> Like 'nsIDOMHTMLInputElement.select' in dom_quickstubs.qsconf, perhaps
> also other selection related methods.
> Maybe there aren't that many problematic cases after all.

If you're whitelisting, why can't you get the right document in the C++ callers anyway? nsHTMLInputElement::Select has easy access to the ownerDocument, which should be equivalent to GetDocumentFromCaller, no?
Chrome should be able to call somecontentobject.focus(). In that case
we need to detect that it is chrome which is calling.
Sure, but why would that need a document to check for chrome?
Doesn't need to be document, principal would be ok too.
Hmm, can we always get the subject principal?
Security manager does use jsstack.


For nsContentUtils::ContentIsCrossDocDescendantOf calls we could use the
to-be-focused content object.
(In reply to comment #20)
> Does GetDocumentFromContext() fail too?

No, GetDocumentFromContext() works fine (doesn't return NULL). And please note that my patch doesn't call GetDocumentFromCaller() when the caller is chrome. If the caller is chrome, it can always steal focus. Therefore, we can return from the method early.
If a sub-window's function which tries to move focus is kicked by opener window's javascript, should it succeed to steal focus? If so, we should use GetDocumentFromContext() rather than GetDocumentFromCaller()?
If site A has an iframe and site B is loaded in the iframe. Furthermore, B opens a new window which loads a B's page this is called as C. Then, there are 4 complex cases when A has focus.

1. B calls C's function which calls focus() of C.
2. B calls C's function which calls focus() of B.
3. C calls B's function which calls focus() of B.
4. C calls B's function which calls focus() of C.

If only 1 and 2 should succeed, GetDocumentFromContext() is better. Which cases should succeed?
Yeah, I think GetDocumentFromContext() makes more sense than GetDocumentFromCaller(), but I'm still worried that does it really work in
cases like bug 612251. Does it really return some reasonable document?
Or do we still need to change dom_quickstubs.qsconf - as far as I understand, 
we do need to change that.
We'd really like to get this fixed, and if a patch shows up we should take it (assuming its safe enough etc). But given that this behavior has been around for a while we can't block on this.
blocking2.0: betaN+ → .x
status2.0: --- → wanted
Some interesting questions here for sure, but I would like to plead that at least one aspect of this problem be fixed.

Just don't let javascript in a web page steal the focus from the address bar. Just fix that. 

It is not possible to implement good keyboard navigation support that uses the native focus mechanism until you fix this. If the user clicks some whitespace on the page, it causes the focused element to lose focus, I want that element to grab it back so that the keyboard continues to work as intended, but this case is indistinguishable from the user clicking in the address bar, so I steal the focus from the address bar! When could it possibly be correct that the page can do this?
(In reply to comment #32)
> It is not possible to implement good keyboard navigation support that uses the
> native focus mechanism until you fix this. If the user clicks some whitespace
> on the page, it causes the focused element to lose focus, I want that element
> to grab it back so that the keyboard continues to work as intended, but this
> case is indistinguishable from the user clicking in the address bar, so I steal
> the focus from the address bar! When could it possibly be correct that the page
> can do this?

When user clicks an unfocusable element, its window will get focus. Therefore, the previous focused element lost it.
Thank you, it did not occur to me that I was listening for focus events at the document and below but not at the window.

Still, if I put one button and this javascript in a web page, the user can not use the address bar or search box:

setInterval(function(){document.getElementsByTagName('button')[0].focus();},100);

It doesn't seem that a web page should be able to do that, and it can't do that in any other browser.
Flags: in-testsuite? → in-testsuite+
Assignee: masayuki → nobody
Severity: normal → S3
Status: ASSIGNED → NEW
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: