Prevent interpretating userinfo as url scheme when editing bookmarks

Prevent interpretating userinfo as url scheme when editing bookmarks

Chrome's Edit Bookmark dialog formats urls for display such that a
url of http://javascript:scripttext@host.com is later converted to a
javascript url scheme, allowing persistence of a script injection
attack within the user's bookmarks.

This fix prevents such misinterpretations by always showing the
scheme when a userinfo component is present within the url.

BUG=639126
Committed: https://crrev.com/fa34e547d6ee25ea0692436ba7462ed0a0ef45f4
Cr-Commit-Position: refs/heads/master@{#422467}

[elawrence, 2016-09-23 16:46:29]

The CQ bit was checked by elawrence@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 16:47:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 17:06:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 17:06:30]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[elawrence, 2016-09-23 19:55:51]

elawrence@chromium.org changed reviewers:
+ sky@chromium.org

[elawrence, 2016-09-23 19:55:51]

elawrence@chromium.org changed required reviewers:
+ sky@chromium.org

[elawrence, 2016-09-23 19:55:52]

PTAL, thanks!

[sky, 2016-09-23 20:10:39]

Please add test coverage.

[elawrence, 2016-09-26 19:47:54]

The CQ bit was checked by elawrence@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 19:49:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 19:59:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 19:59:53]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[elawrence, 2016-09-26 21:13:24]

On 2016/09/23 20:10:39, sky wrote:
> Please add test coverage.

Added unit tests.

[sky, 2016-09-26 23:03:02]

https://codereview.chromium.org/2368593002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm
(right):

https://codereview.chromium.org/2368593002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm:262:
class BookmarkEditorControllerEditKeepsSchemeTest : public CocoaProfileTest {
I wouldn't bother with a separate test facade and just use TEST() for this. The
separate facade is nice if you're going to do more than one test that wants this
setup, but that doesn't seem to be the case (doing that also means you'll fix
controller_ not being initialiazed and controller_ should be protected in style
guide).

https://codereview.chromium.org/2368593002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc (right):

https://codereview.chromium.org/2368593002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc:347: GURL
script_url = GURL("http://javascript:scripttext@example.com/");
It would be nice if the tests were more similar than different. For example,
here you use GURL, in the cocoa one you don't.

https://codereview.chromium.org/2368593002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc:357:
SetTitleText(ASCIIToUTF16("EditingKeepsScheme"));
Make local variable (constant) so you can compare against constant on 365.

[elawrence, 2016-09-27 21:24:24]

Patchset #4 (id:60001) has been deleted

[elawrence, 2016-09-29 17:29:53]

Patchset #4 (id:80001) has been deleted

[elawrence, 2016-09-29 19:56:06]

The CQ bit was checked by elawrence@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 19:56:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 20:44:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 20:44:03]

Dry run: This issue passed the CQ dry run.

[elawrence, 2016-09-29 21:53:03]

Thanks for the feedback on the tests! Please have a look.

https://codereview.chromium.org/2368593002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm
(right):

https://codereview.chromium.org/2368593002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm:262:
class BookmarkEditorControllerEditKeepsSchemeTest : public CocoaProfileTest {
On 2016/09/26 23:03:02, sky wrote:
> I wouldn't bother with a separate test facade and just use TEST() for this.
The
> separate facade is nice if you're going to do more than one test that wants
this
> setup, but that doesn't seem to be the case (doing that also means you'll fix
> controller_ not being initialiazed and controller_ should be protected in
style
> guide).

Done.

https://codereview.chromium.org/2368593002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc (right):

https://codereview.chromium.org/2368593002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc:347: GURL
script_url = GURL("http://javascript:scripttext@example.com/");
On 2016/09/26 23:03:02, sky wrote:
> It would be nice if the tests were more similar than different. For example,
> here you use GURL, in the cocoa one you don't.

Done.

https://codereview.chromium.org/2368593002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc:357:
SetTitleText(ASCIIToUTF16("EditingKeepsScheme"));
On 2016/09/26 23:03:02, sky wrote:
> Make local variable (constant) so you can compare against constant on 365.

Done.

[sky, 2016-09-29 22:20:54]

Thanks for the patience and cleanup. Almost there.

https://codereview.chromium.org/2368593002/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm
(right):

https://codereview.chromium.org/2368593002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm:295:
ASSERT_EQ(kParent->child_count(), 1);
Over of assertions is expected, actual. 1 should be first. Same comment for the
remaining assertions.

[elawrence, 2016-09-30 15:46:50]

The CQ bit was checked by elawrence@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 15:47:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[elawrence, 2016-09-30 15:50:09]

Thanks. I've corrected the ordering in the assertions. 

I assume I should not attempt to correct in this CL the other 16 incorrect
orderings unrelated to my change elsewhere in the .mm file?

Interestingly, GTest's documentation says "Historical note: Before February 2016
*_EQ had a convention of calling it as ASSERT_EQ(expected, actual), so lots of
existing code uses this order. Now *_EQ treats both parameters in the same way."
but looking at the output from the unit-test runs that doesn't seem to be true
for GTest as used in Chromium (the output explicitly calls the first parameter
"Expected"). Does this mean that we're using an older version of GTest?

https://codereview.chromium.org/2368593002/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm
(right):

https://codereview.chromium.org/2368593002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm:295:
ASSERT_EQ(kParent->child_count(), 1);
On 2016/09/29 22:20:54, sky wrote:
> Over of assertions is expected, actual. 1 should be first. Same comment for
the
> remaining assertions.

Done.

[sky, 2016-09-30 16:03:31]

I'm not entirely sure what version of gtest chrome has. It's entirely
possible Chrome is using an older version.

  -Scott

On Fri, Sep 30, 2016 at 8:50 AM,  <elawrence@chromium.org> wrote:
> Thanks. I've corrected the ordering in the assertions.
>
> I assume I should not attempt to correct in this CL the other 16 incorrect
> orderings unrelated to my change elsewhere in the .mm file?
>
> Interestingly, GTest's documentation says "Historical note: Before February
> 2016
> *_EQ had a convention of calling it as ASSERT_EQ(expected, actual), so lots
> of
> existing code uses this order. Now *_EQ treats both parameters in the same
> way."
> but looking at the output from the unit-test runs that doesn't seem to be
> true
> for GTest as used in Chromium (the output explicitly calls the first
> parameter
> "Expected"). Does this mean that we're using an older version of GTest?
>
>
>
>
https://codereview.chromium.org/2368593002/diff/120001/chrome/browser/ui/coco...
> File
> chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm
> (right):
>
>
https://codereview.chromium.org/2368593002/diff/120001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm:295:
> ASSERT_EQ(kParent->child_count(), 1);
> On 2016/09/29 22:20:54, sky wrote:
>> Over of assertions is expected, actual. 1 should be first. Same
> comment for the
>> remaining assertions.
>
> Done.
>
> https://codereview.chromium.org/2368593002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-09-30 17:07:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 17:07:10]

Dry run: This issue passed the CQ dry run.

[elawrence, 2016-10-03 17:06:27]

Please let me know if anything else is needed here. I'm hoping to land this
security fix in M55. Thanks!

[sky, 2016-10-03 17:46:39]

LGTM - thanks!

https://codereview.chromium.org/2368593002/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm
(right):

https://codereview.chromium.org/2368593002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm:300:
controller = NULL;
remove as not necessary.

[elawrence, 2016-10-03 17:47:50]

The CQ bit was checked by elawrence@chromium.org

[elawrence, 2016-10-03 17:47:56]

The CQ bit was unchecked by elawrence@chromium.org

[elawrence, 2016-10-03 17:55:10]

The CQ bit was checked by elawrence@chromium.org

[elawrence, 2016-10-03 17:55:10]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2368593002/#ps160001
(title: "Don't NULL when going away")

[commit-bot: I haz the power, 2016-10-03 17:55:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 18:41:24]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2016-10-03 18:43:53]

Description was changed from

==========
Prevent interpretating userinfo as url scheme when editing bookmarks

Chrome's Edit Bookmark dialog formats urls for display such that a
url of http://javascript:scripttext@host.com is later converted to a
javascript url scheme, allowing persistence of a script injection
attack within the user's bookmarks.

This fix prevents such misinterpretations by always showing the
scheme when a userinfo component is present within the url.

BUG=639126
==========

to

==========
Prevent interpretating userinfo as url scheme when editing bookmarks

Chrome's Edit Bookmark dialog formats urls for display such that a
url of http://javascript:scripttext@host.com is later converted to a
javascript url scheme, allowing persistence of a script injection
attack within the user's bookmarks.

This fix prevents such misinterpretations by always showing the
scheme when a userinfo component is present within the url.

BUG=639126

Committed: https://crrev.com/fa34e547d6ee25ea0692436ba7462ed0a0ef45f4
Cr-Commit-Position: refs/heads/master@{#422467}
==========

[commit-bot: I haz the power, 2016-10-03 18:43:54]

Patchset 7 (id:??) landed as
https://crrev.com/fa34e547d6ee25ea0692436ba7462ed0a0ef45f4
Cr-Commit-Position: refs/heads/master@{#422467}