Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2358463002:
Views::Textfield: Prevent revealing password text. (Closed)
|
Created:
4 years, 3 months ago by karandeepb Modified:
4 years, 3 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, James Su, shuchen+watch_chromium.org, yusukes+watch_chromium.org, nona+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionViews::Textfield: Prevent revealing password text.
Currently, the Yank command on Mac and the selection clipboard on Linux can
reveal the text of an obscured/password Views::Textfield. This CL modifies
Textfield::UpdateSelectionClipboard() to ensure that the selection clipboard is
not modified for a password textfield. Textfield::ExecuteTextEditCommand(..) is
also modified to only update the yank kill buffer for a non-password textfield.
Unit tests which demonstrate the problem are also added.
BUG=648511, 648509
Committed: https://crrev.com/27bac7b1a96717471167acd71d9d8aa5624fa904
Cr-Commit-Position: refs/heads/master@{#419977}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address review comments. #Patch Set 3 : Address review comments. #
Messages
Total messages: 26 (16 generated)
Description was changed from ========== Password textfields. ========== to ========== Views::Textfield: Prevent revealing password text. Currently, the Yank command on Mac and the selection clipboard on Linux can reveal the text of an obscured/password Views::Textfield. This CL modifies Textfield::UpdateSelectionClipboard() to ensure that the selection clipboard is not modified for a password textfield. Textfield::ExecuteTextEditCommand(..) is also modified to only update the yank kill buffer for a non-password textfield. Unit tests which demonstrate the problem are also added. BUG=648511, 648509 ==========
Patchset #3 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
karandeepb@chromium.org changed reviewers: + pkasting@chromium.org
PTAL pkasting@. Thanks.
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2358463002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_model.cc (right): https://codereview.chromium.org/2358463002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_model.cc:352: // |add_to_kill_buffer| should never be true for an obscured textfield. Nit: Why not DCHECK this atop the function instead of below the first block? Does that block sometimes violate this invariant? Newline between this block and surrounding code (in this case, above) https://codereview.chromium.org/2358463002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_model.cc:353: DCHECK(!(add_to_kill_buffer && render_text_->obscured())); Nit: Distribute "!" through and eliminate parens https://codereview.chromium.org/2358463002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_model.cc:381: DCHECK(!(add_to_kill_buffer && render_text_->obscured())); Same comments https://codereview.chromium.org/2358463002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2358463002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:2085: textfield2->SetTextInputType(ui::TEXT_INPUT_TYPE_PASSWORD); Nit: I would move this line down to be part of (and at the top of) the first new block you add below, if that doesn't break anything. This is an implementation detail of "Verify deletion in a password textfield...". https://codereview.chromium.org/2358463002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:2758: textfield2->SetTextInputType(ui::TEXT_INPUT_TYPE_PASSWORD); Nit: Again, I'd just merge this block into the next one.
https://codereview.chromium.org/2358463002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_model.cc (right): https://codereview.chromium.org/2358463002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_model.cc:352: // |add_to_kill_buffer| should never be true for an obscured textfield. On 2016/09/20 17:29:30, Peter Kasting wrote: > Nit: Why not DCHECK this atop the function instead of below the first block? > Does that block sometimes violate this invariant? > > Newline between this block and surrounding code (in this case, above) Done. https://codereview.chromium.org/2358463002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_model.cc:353: DCHECK(!(add_to_kill_buffer && render_text_->obscured())); On 2016/09/20 17:29:30, Peter Kasting wrote: > Nit: Distribute "!" through and eliminate parens Done. https://codereview.chromium.org/2358463002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_model.cc:381: DCHECK(!(add_to_kill_buffer && render_text_->obscured())); On 2016/09/20 17:29:30, Peter Kasting wrote: > Same comments Done. https://codereview.chromium.org/2358463002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2358463002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:2085: textfield2->SetTextInputType(ui::TEXT_INPUT_TYPE_PASSWORD); On 2016/09/20 17:29:30, Peter Kasting wrote: > Nit: I would move this line down to be part of (and at the top of) the first new > block you add below, if that doesn't break anything. This is an implementation > detail of "Verify deletion in a password textfield...". I'd like to keep it here since this also tests that yanking into a password textfield works. https://codereview.chromium.org/2358463002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:2758: textfield2->SetTextInputType(ui::TEXT_INPUT_TYPE_PASSWORD); On 2016/09/20 17:29:30, Peter Kasting wrote: > Nit: Again, I'd just merge this block into the next one. Done.
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2358463002/#ps80001 (title: "Address review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2358463002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2358463002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:2085: textfield2->SetTextInputType(ui::TEXT_INPUT_TYPE_PASSWORD); On 2016/09/21 03:36:12, karandeepb wrote: > On 2016/09/20 17:29:30, Peter Kasting wrote: > > Nit: I would move this line down to be part of (and at the top of) the first > new > > block you add below, if that doesn't break anything. This is an > implementation > > detail of "Verify deletion in a password textfield...". > > I'd like to keep it here since this also tests that yanking into a password > textfield works. Perhaps in that case do this: // Verify yanked text persists across multiple textfields, and that yanking // into a password textfield works. textfield2->SetTextInputType(ui::TEXT_INPUT_TYPE_PASSWORD); SendKeyPress(ui::VKEY_Y, ui::EF_CONTROL_DOWN); ...
The CQ bit was unchecked by karandeepb@chromium.org
On 2016/09/21 04:12:30, Peter Kasting wrote: > https://codereview.chromium.org/2358463002/diff/60001/ui/views/controls/textf... > File ui/views/controls/textfield/textfield_unittest.cc (right): > > https://codereview.chromium.org/2358463002/diff/60001/ui/views/controls/textf... > ui/views/controls/textfield/textfield_unittest.cc:2085: > textfield2->SetTextInputType(ui::TEXT_INPUT_TYPE_PASSWORD); > On 2016/09/21 03:36:12, karandeepb wrote: > > On 2016/09/20 17:29:30, Peter Kasting wrote: > > > Nit: I would move this line down to be part of (and at the top of) the first > > new > > > block you add below, if that doesn't break anything. This is an > > implementation > > > detail of "Verify deletion in a password textfield...". > > > > I'd like to keep it here since this also tests that yanking into a password > > textfield works. > > Perhaps in that case do this: > > // Verify yanked text persists across multiple textfields, and that yanking > // into a password textfield works. > textfield2->SetTextInputType(ui::TEXT_INPUT_TYPE_PASSWORD); > SendKeyPress(ui::VKEY_Y, ui::EF_CONTROL_DOWN); > ... Will update the comment.
https://codereview.chromium.org/2358463002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2358463002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:2085: textfield2->SetTextInputType(ui::TEXT_INPUT_TYPE_PASSWORD); On 2016/09/21 04:12:30, Peter Kasting wrote: > On 2016/09/21 03:36:12, karandeepb wrote: > > On 2016/09/20 17:29:30, Peter Kasting wrote: > > > Nit: I would move this line down to be part of (and at the top of) the first > > new > > > block you add below, if that doesn't break anything. This is an > > implementation > > > detail of "Verify deletion in a password textfield...". > > > > I'd like to keep it here since this also tests that yanking into a password > > textfield works. > > Perhaps in that case do this: > > // Verify yanked text persists across multiple textfields, and that yanking > // into a password textfield works. > textfield2->SetTextInputType(ui::TEXT_INPUT_TYPE_PASSWORD); > SendKeyPress(ui::VKEY_Y, ui::EF_CONTROL_DOWN); > ... Done.
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2358463002/#ps100001 (title: "Address review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Views::Textfield: Prevent revealing password text. Currently, the Yank command on Mac and the selection clipboard on Linux can reveal the text of an obscured/password Views::Textfield. This CL modifies Textfield::UpdateSelectionClipboard() to ensure that the selection clipboard is not modified for a password textfield. Textfield::ExecuteTextEditCommand(..) is also modified to only update the yank kill buffer for a non-password textfield. Unit tests which demonstrate the problem are also added. BUG=648511, 648509 ========== to ========== Views::Textfield: Prevent revealing password text. Currently, the Yank command on Mac and the selection clipboard on Linux can reveal the text of an obscured/password Views::Textfield. This CL modifies Textfield::UpdateSelectionClipboard() to ensure that the selection clipboard is not modified for a password textfield. Textfield::ExecuteTextEditCommand(..) is also modified to only update the yank kill buffer for a non-password textfield. Unit tests which demonstrate the problem are also added. BUG=648511, 648509 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Views::Textfield: Prevent revealing password text. Currently, the Yank command on Mac and the selection clipboard on Linux can reveal the text of an obscured/password Views::Textfield. This CL modifies Textfield::UpdateSelectionClipboard() to ensure that the selection clipboard is not modified for a password textfield. Textfield::ExecuteTextEditCommand(..) is also modified to only update the yank kill buffer for a non-password textfield. Unit tests which demonstrate the problem are also added. BUG=648511, 648509 ========== to ========== Views::Textfield: Prevent revealing password text. Currently, the Yank command on Mac and the selection clipboard on Linux can reveal the text of an obscured/password Views::Textfield. This CL modifies Textfield::UpdateSelectionClipboard() to ensure that the selection clipboard is not modified for a password textfield. Textfield::ExecuteTextEditCommand(..) is also modified to only update the yank kill buffer for a non-password textfield. Unit tests which demonstrate the problem are also added. BUG=648511, 648509 Committed: https://crrev.com/27bac7b1a96717471167acd71d9d8aa5624fa904 Cr-Commit-Position: refs/heads/master@{#419977} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/27bac7b1a96717471167acd71d9d8aa5624fa904 Cr-Commit-Position: refs/heads/master@{#419977} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
