Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(23)

Issue 2368593002: Prevent interpretating userinfo as url scheme when editing bookmarks (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 months ago by elawrence
Modified:
10 months, 1 week ago
Reviewers:
*sky
CC:
chromium-reviews, tfarina
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Patch Set 2 : Add unittests #

Patch Set 3 : Fix unittest #

Total comments: 6

Patch Set 4 : Fix Mac Unit tests to use default CocoaProfileTest fixture #

Patch Set 5 : Address review feedback; follow style guide #

Total comments: 2

Patch Set 6 : Fix ordering of arguments to EXPECT_EQ #

Total comments: 1

Patch Set 7 : Don't NULL when going away #

Messages

Total messages: 38 (25 generated)
elawrence
PTAL, thanks!
11 months ago (2016-09-23 19:55:52 UTC) #7
sky
Please add test coverage.
11 months ago (2016-09-23 20:10:39 UTC) #8
elawrence
On 2016/09/23 20:10:39, sky wrote: > Please add test coverage. Added unit tests.
11 months ago (2016-09-26 21:13:24 UTC) #13
sky
https://codereview.chromium.org/2368593002/diff/40001/chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm File chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm (right): https://codereview.chromium.org/2368593002/diff/40001/chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm#newcode262 chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm:262: class BookmarkEditorControllerEditKeepsSchemeTest : public CocoaProfileTest { I wouldn't bother ...
11 months ago (2016-09-26 23:03:02 UTC) #14
elawrence
Thanks for the feedback on the tests! Please have a look. https://codereview.chromium.org/2368593002/diff/40001/chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm File chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm (right): ...
10 months, 4 weeks ago (2016-09-29 21:53:03 UTC) #21
sky
Thanks for the patience and cleanup. Almost there. https://codereview.chromium.org/2368593002/diff/120001/chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm File chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm (right): https://codereview.chromium.org/2368593002/diff/120001/chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm#newcode295 chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm:295: ASSERT_EQ(kParent->child_count(), ...
10 months, 4 weeks ago (2016-09-29 22:20:54 UTC) #22
elawrence
Thanks. I've corrected the ordering in the assertions. I assume I should not attempt to ...
10 months, 3 weeks ago (2016-09-30 15:50:09 UTC) #25
sky
I'm not entirely sure what version of gtest chrome has. It's entirely possible Chrome is ...
10 months, 3 weeks ago (2016-09-30 16:03:31 UTC) #26
elawrence
Please let me know if anything else is needed here. I'm hoping to land this ...
10 months, 3 weeks ago (2016-10-03 17:06:27 UTC) #29
sky
LGTM - thanks! https://codereview.chromium.org/2368593002/diff/140001/chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm File chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm (right): https://codereview.chromium.org/2368593002/diff/140001/chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm#newcode300 chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm:300: controller = NULL; remove as not ...
10 months, 3 weeks ago (2016-10-03 17:46:39 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2368593002/160001
10 months, 3 weeks ago (2016-10-03 17:55:57 UTC) #35
commit-bot: I haz the power
Committed patchset #7 (id:160001)
10 months, 3 weeks ago (2016-10-03 18:41:24 UTC) #36
commit-bot: I haz the power
10 months, 3 weeks ago (2016-10-03 18:43:54 UTC) #38
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/fa34e547d6ee25ea0692436ba7462ed0a0ef45f4
Cr-Commit-Position: refs/heads/master@{#422467}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b