Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2346023002:
Ignore Javascript urls dropped on tabs (Mac version) (Closed)
|
Created:
4 years, 3 months ago by elawrence Modified:
4 years, 3 months ago Reviewers:
*Avi (use Gerrit) CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIgnore Javascript urls dropped on tabs (Mac version)
When a Javascript: url is dropped on a tab, it executes in the
security context of the selected tab, representing a script
injection attack ("Dropjacking"). We will match other browsers
and disallow such drops.
Mac handles URL drops using a different codepath than Windows;
this change fixes the Mac codepath.
BUG=639750
Committed: https://crrev.com/a2525d33673bd4a5987a9aa3dfb516f6c89fb6de
Cr-Commit-Position: refs/heads/master@{#419266}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address feedback #
Total comments: 2
Patch Set 3 : Move return to a new line #
Messages
Total messages: 21 (13 generated)
The CQ bit was checked by elawrence@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.
elawrence@chromium.org changed reviewers: + avi@chromium.org
elawrence@chromium.org changed required reviewers: + avi@chromium.org
PTAL, thanks!
Some minor nits. https://codereview.chromium.org/2346023002/diff/1/chrome/browser/ui/cocoa/tab... File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): https://codereview.chromium.org/2346023002/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2069: // Security: Block JavaScript to prevent self-xss Comments are full sentences; end them with a full-stop. https://codereview.chromium.org/2346023002/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2073: Why not put this right at the beginning of the function? If we're going to bail early, bail as early as possible. And no {} around the one-line body of the if. https://codereview.chromium.org/2346023002/diff/1/chrome/browser/ui/cocoa/too... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/2346023002/diff/1/chrome/browser/ui/cocoa/too... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:1153: // Security: Sanitize text to prevent self-XSS Comments are full sentences; end them with a full-stop. Also, why "self-XSS" here and "self-xss" above? Pick one and use it everywhere. https://codereview.chromium.org/2346023002/diff/1/chrome/browser/ui/cocoa/too... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:1178: // Security: Block JavaScript to prevent self-XSS ditto.
Thanks for the fast review! https://codereview.chromium.org/2346023002/diff/1/chrome/browser/ui/cocoa/tab... File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): https://codereview.chromium.org/2346023002/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2069: // Security: Block JavaScript to prevent self-xss On 2016/09/16 18:31:09, Avi wrote: > Comments are full sentences; end them with a full-stop. Done. https://codereview.chromium.org/2346023002/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2073: > Why not put this right at the beginning of the function? If we're going to bail early, bail as early as possible. Makes sense. > And no {} around the one-line body of the if. I was trying to match the style elsewhere in this file, but I'm happy to remove. https://codereview.chromium.org/2346023002/diff/1/chrome/browser/ui/cocoa/too... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/2346023002/diff/1/chrome/browser/ui/cocoa/too... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:1153: // Security: Sanitize text to prevent self-XSS On 2016/09/16 18:31:09, Avi wrote: > Comments are full sentences; end them with a full-stop. > > Also, why "self-XSS" here and "self-xss" above? Pick one and use it everywhere. Done. https://codereview.chromium.org/2346023002/diff/1/chrome/browser/ui/cocoa/too... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:1178: // Security: Block JavaScript to prevent self-XSS On 2016/09/16 18:31:09, Avi wrote: > ditto. Done.
LGTM with nits. https://codereview.chromium.org/2346023002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): https://codereview.chromium.org/2346023002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2063: if (url->SchemeIs(url::kJavaScriptScheme)) return; Strictly speaking this is allowed by the style guide, but I haven't seen it done this way before. Can you put the "return" onto its own line? (Here and in the other file.)
Fixed. Thanks again! https://codereview.chromium.org/2346023002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): https://codereview.chromium.org/2346023002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2063: if (url->SchemeIs(url::kJavaScriptScheme)) return; On 2016/09/16 19:07:49, Avi wrote: > Strictly speaking this is allowed by the style guide, but I haven't seen it done > this way before. Can you put the "return" onto its own line? (Here and in the > other file.) Done.
The CQ bit was checked by elawrence@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.
The CQ bit was checked by elawrence@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org Link to the patchset: https://codereview.chromium.org/2346023002/#ps40001 (title: "Move return to a new line")
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from
==========
Ignore Javascript urls dropped on tabs (Mac version)
When a Javascript: url is dropped on a tab, it executes in the
security context of the selected tab, representing a script
injection attack ("Dropjacking"). We will match other browsers
and disallow such drops.
Mac handles URL drops using a different codepath than Windows;
this change fixes the Mac codepath.
BUG=639750
==========
to
==========
Ignore Javascript urls dropped on tabs (Mac version)
When a Javascript: url is dropped on a tab, it executes in the
security context of the selected tab, representing a script
injection attack ("Dropjacking"). We will match other browsers
and disallow such drops.
Mac handles URL drops using a different codepath than Windows;
this change fixes the Mac codepath.
BUG=639750
Committed: https://crrev.com/a2525d33673bd4a5987a9aa3dfb516f6c89fb6de
Cr-Commit-Position: refs/heads/master@{#419266}
==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a2525d33673bd4a5987a9aa3dfb516f6c89fb6de Cr-Commit-Position: refs/heads/master@{#419266} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
