Bug 13471 - REGRESSION (r21045): Secure keyboard entry mode remains in effect after leaving a password field by submitting
Summary: REGRESSION (r21045): Secure keyboard entry mode remains in effect after leavi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P1 Major
Assignee: Nobody
URL:
Keywords: HasReduction, InRadar, Regression
Depends on:
Blocks:
 
Reported: 2007-04-24 06:46 PDT by mitz
Modified: 2007-04-28 18:54 PDT (History)
1 user (show)

See Also:


Attachments
Test case (174 bytes, text/html)
2007-04-24 06:46 PDT, mitz
no flags Details
Reset secure keyboard entry mode in Frame::setView (2.78 KB, patch)
2007-04-24 07:18 PDT, mitz
no flags Details | Formatted Diff | Diff
Reset secure entry mode in setDocument (6.16 KB, patch)
2007-04-24 11:46 PDT, mitz
no flags Details | Formatted Diff | Diff
Reset secure entry mode in setDocument (6.16 KB, patch)
2007-04-24 11:49 PDT, mitz
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2007-04-24 06:46:05 PDT
To reproduce:
1) Use System Preferences > International > Input Menu to enable a non-Roman input source, e.g. Arabic.
2) Open the attached test case and click in the password field.
3) Check that non-Roman input sources are disabled in the Input menu (the flag on the right side of the menu bar).
4) Press Return.
5) Click in the text field.
6) Check that non-Roman input sources are enabled and selectable in the Input menu.

Expected results: disabled in 3) and enabled in 6).
Actual results: disabled in 3) and in 6).

Regressed in <http://trac.webkit.org/projects/webkit/changeset/21045>.
Comment 1 mitz 2007-04-24 06:46:42 PDT
Created attachment 14156 [details]
Test case
Comment 2 mitz 2007-04-24 07:18:17 PDT
Created attachment 14157 [details]
Reset secure keyboard entry mode in Frame::setView

The reason why you can reset to false unconditionally is that pages containing password fields are not allowed in the page cache, so you know that in setView() you're never entering a page with a focused password field.

If you think it's necessary, I can add a FIXME in FrameLoader::canCachePage() saying that if you ever decide to allow password fields in the page cache, then you should address this problem.
Comment 3 Darin Adler 2007-04-24 08:05:24 PDT
Comment on attachment 14157 [details]
Reset secure keyboard entry mode in Frame::setView

Why not spell it "submit" everywhere?

What's the key reason this needs to be called? Is it really the change in view that's the issue? Doesn't the input element lose focus? I think this fix is fine, but it would be stronger if I understood exactly why this is the right point in time to clear this. It has to have something about the input that was sponsoring secure keyboard entry.

r=me
Comment 4 mitz 2007-04-24 08:41:15 PDT
Comment on attachment 14157 [details]
Reset secure keyboard entry mode in Frame::setView

(In reply to comment #3)
> (From update of attachment 14157 [details] [edit])
> Why not spell it "submit" everywhere?

Why not say "you made a typo"? ;-)

> What's the key reason this needs to be called? Is it really the change in view
> that's the issue? Doesn't the input element lose focus?

In many senses, it never does (so I think that even prior to r21045, if you happened to be in a secure text field when the new page was loaded, you'd remain in Roman-only mode). The Frame doesn't keep track of focus, it just serves requests from nodes to switch secure mode on or off. Presumably, those nodes are in the Frame's Document. So now I'm thinking it may be more appropriate to patch setDocument() instead of setView(). It would still just reset to false unconditionally, since it can't (and doesn't need to) ask the new document's focused node if it requires secure keyboard entry. However, that could be changed in the future (if password fields can go into the page cache).
Comment 5 Darin Adler 2007-04-24 09:47:17 PDT
(In reply to comment #4)
> Why not say "you made a typo"? ;-)

Sorry, I was just in a silly mood.

> > What's the key reason this needs to be called? Is it really the change in view
> > that's the issue? Doesn't the input element lose focus?
> 
> In many senses, it never does (so I think that even prior to r21045, if you
> happened to be in a secure text field when the new page was loaded, you'd
> remain in Roman-only mode). The Frame doesn't keep track of focus, it just
> serves requests from nodes to switch secure mode on or off. Presumably, those
> nodes are in the Frame's Document. So now I'm thinking it may be more
> appropriate to patch setDocument() instead of setView().

Sounds good; easier to explain at least. I think either is fine in practice.

> It would still just
> reset to false unconditionally, since it can't (and doesn't need to) ask the
> new document's focused node if it requires secure keyboard entry. However, that
> could be changed in the future (if password fields can go into the page cache).

Makes sense. This is subtle enough that I think we need a comment or two. I really like the idea of putting a comment in FrameLoader::canCachePage -- I don't think it needs to be a FIXME, just a warning that if you change the rule then this other code will need to change too.
Comment 6 mitz 2007-04-24 11:46:09 PDT
Created attachment 14159 [details]
Reset secure entry mode in setDocument

This turned out to be a slightly bigger change.

I can go all the way and de-friend FrameLoader and Frame by adding a couple of public accessors and making cleanupScriptObjects() public. Feel free to r- if you want me to do it (or for any other reason!).
Comment 7 mitz 2007-04-24 11:48:06 PDT
Comment on attachment 14159 [details]
Reset secure entry mode in setDocument

Wait! Found another typo!
Comment 8 mitz 2007-04-24 11:49:10 PDT
Created attachment 14160 [details]
Reset secure entry mode in setDocument

See comment #6.
Comment 9 Darin Adler 2007-04-24 13:07:33 PDT
Comment on attachment 14160 [details]
Reset secure entry mode in setDocument

I do like the idea of "going all the way" and eliminating the friendship, but I don't see any need to do this as part of this patch.

setDocument should be changed to take a PassRefPtr.

r=me
Comment 10 Darin Adler 2007-04-26 08:31:40 PDT
<rdar://problem/5163498>
Comment 11 Darin Adler 2007-04-28 18:54:44 PDT
Sending        WebCore/ChangeLog
Sending        WebCore/loader/FrameLoader.cpp
Adding         WebCore/manual-tests/secure-keyboard-enabled-after-submit.html
Sending        WebCore/page/Frame.cpp
Transmitting file data ....
Committed revision 21172.