We should define how much could focus be moved with autofocus and what is already doable.

I don't quite understand this bug. Does autofocus introduce some new way to steal focus? .focus() can do that always in many cases.

I agree. But Jonas is concerned about cross-domain focus change for example. But this should probably not be a problem if we do something in bug 601031. If we do not fix bug 601031, we should probably make sure that autofocus can't be too harmful.

Ah, we should probably restrict autofocus if focus is already set to some element in the content document tree. This way loading an iframe couldn't steal focus.

We already restrict the autofocus if the focus is set to one element but we only check the document so it might be doable to move the focus to another document with iframe's. For example, if there is an iframe inserted inside the document which has an autofocus attribute, stealing the focus is easy. Though, in that case, we might be safe considering the iframe has been created by the document. Do we want to prevent that? The real issue is when an iframe as the focus and another iframe is inserted in the parent document with an autofocus attribute. In that case, we should prevent focus to move.

So, currently, we can't autofocus if: 1. The document has something focused. Example <input autofocus><input autofocus> 2. The window has focused node. Example: <iframe><input autofocus></iframe><input autofocus>, the <iframe> is a focused node. So, this is not taking into account this case: <iframe><input autofocus></iframe> then inserting the exact same <iframe> in the document. It's easily fixable (I've a patch in progress). We only have to check that window.top has a focused node instead of window. However, I really doubt this might be a security issue given that you need script to insert dynamically an <iframe> to steal user's focus and the point here is that autofocus is opening a security hole when the user has script disabled. And, to be cynical, given the level of non-workiness of autofocus, the user would have to be really unlucky to be attacked with that feature ;)

Is there a way to get a nsGlobalWindow or is that even safe to cast nsPIDOMWindow to nsGlobalWindow? Because what I'm doing is basically nsGlobalWindow::GetTop().

This is fixing a test which was using iframe to test autofocus.

Comment on attachment 494678 [details] [diff] [review] Part 1 - Don't let a frame to steal focus from another frame. I would probably QI to window, but this should be safe too.

(In reply to comment #9) > Comment on attachment 494678 [details] [diff] [review] > Patch v1.1 > > I would probably QI to window, but this should be safe too. nsGlobalWindow::GetTop doesn't QI so I guess this is safe.

Comment on attachment 494678 [details] [diff] [review] Part 1 - Don't let a frame to steal focus from another frame. Asking approval given that it's an issue that was raised during the security review of this feature. The only risk is to break websites that do use autofocus this way but we don't want them to do that and we can assume no one does that for non-malicious reasons.

The only abuse you can do of autofocus now is removing an iframe which has autofocus then append another one with autofocus. We could prevent that by checking that the top document is already loaded and decline autofocus in that case.

I'm not sure if we really want that and I'm wondering if we should use windom.mDocumentLoaded instead of nsIDocument::READYSTATE_COMPLETE. Finally, some tests will have to be fixed.

Jonas and Olli, I think this last patch is fixing all possible security holes (even if I'm not sure there would be usable without script enabled as said in comment 5). Do you see anything else?

"fixes all security holes" is sort of bold wording as I wouldn't ever say that any patch "fixes all security holes" for any feature. Anyhow, the only thing I'd like to make sure is that loading a page with <input autofocus> in a "background" tab, i.e. a tab other than the one the user is currently viewing, doesn't make us switch to that tab. Is that the case?

(In reply to comment #15) > "fixes all security holes" is sort of bold wording as I wouldn't ever say that > any patch "fixes all security holes" for any feature. Yeah, I agree. I should have weight my words. It seems to prevent all focus stealing situation I've taught about. > Anyhow, the only thing I'd like to make sure is that loading a page with <input > autofocus> in a "background" tab, i.e. a tab other than the one the user is > currently viewing, doesn't make us switch to that tab. > > Is that the case? It's actually focusing the element exactly like any other focus call. focus() doesn't switch tabs. However, the focus is correctly done when called from a background tab (we have a test checking that). I can add a test to check that the tab doesn't change if you want.

Comment on attachment 494686 [details] [diff] [review] Part 2 - Don't let a frame steal focus when the document has been loaded You could keep the check also in the bind, though I assume autofocus wont be used too often so it doesn't really matter.

Olli, I had to change all tests because mochitests are run in an iframe and we can't use autofocus in an iframe anymore if the top document is loaded. Could you review the test changes? I'm going to attach an inter diff.

Pushed: http://hg.mozilla.org/mozilla-central/rev/14ca1420a81d http://hg.mozilla.org/mozilla-central/rev/dc8dea324142