|
|
DescriptionIgnore Javascript urls dropped on tabs
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.
BUG=639750
Committed: https://crrev.com/d1a8cbeec4d9f9520a4bb1f4db6d3f62900ee709
Cr-Commit-Position: refs/heads/master@{#418620}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address nits #Messages
Total messages: 30 (15 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: + dcheng@chromium.org, pkasting@chromium.org
elawrence@chromium.org changed required reviewers: + pkasting@chromium.org
Please take a look. Thanks!
Description was changed from ========== Ignore Javacript urls dropped on tabs 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. BUG=639750 ========== to ========== Ignore Javascript urls dropped on tabs 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. BUG=639750 ==========
What happens if the URL is dropped on web content area instead of the tab strip?
Can this be tested? LGTM otherwise. https://codereview.chromium.org/2323273003/diff/1/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/tab_strip.cc (right): https://codereview.chromium.org/2323273003/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab_strip.cc:1632: if ((drop_info_->url).SchemeIs(url::kJavaScriptScheme)) Nit: Remove unnecessary first set of parens. Combine with conditional above. https://codereview.chromium.org/2323273003/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab_strip.cc:1653: // Do nothing if the file was unsupported or the URL is invalid. The URL may Nit: "...the file is unsupported, the URL is invalid, or this is a javascript: URL (for self-XSS prevention)."
On 2016/09/09 20:30:09, dcheng wrote: > What happens if the URL is dropped on web content area instead of the tab strip? If the JavaScript URL Object is dropped on the web content area, that content area is navigated to about:blank, the same behavior that exists in the build today for both javascript: and file: drops. The code that performs the on-content-drop navigation is in DragController::performDrag function: if (m_page->settings().navigateOnDragDrop()) { m_page->deprecatedLocalMainFrame()->loader().load(FrameLoadRequest(nullptr, ResourceRequest(dragData->asURL()))); } However, when dropping a JavaScript URL, by the time you get here, dragData->asURL() returns "about:blank" while dragData->asPlainText() contains the original Javascript URI, so the URL is getting sanitized before we even get here.
On 2016/09/09 22:03:21, elawrence wrote: > On 2016/09/09 20:30:09, dcheng wrote: > > What happens if the URL is dropped on web content area instead of the tab > strip? > > If the JavaScript URL Object is dropped on the web content area, that content > area is navigated to about:blank, the same behavior that exists in the build > today for both javascript: and file: drops. > > The code that performs the on-content-drop navigation is in > DragController::performDrag function: > > if (m_page->settings().navigateOnDragDrop()) > { > > m_page->deprecatedLocalMainFrame()->loader().load(FrameLoadRequest(nullptr, > ResourceRequest(dragData->asURL()))); > } > > However, when dropping a JavaScript URL, by the time you get here, > dragData->asURL() returns "about:blank" while dragData->asPlainText() contains > the original Javascript URI, so the URL is getting sanitized before we even get > here. Hmm.... interesting and slightly surprising. Non-owner LGTM.
>> However, when dropping a JavaScript URL, by the time you get here, >> dragData->asURL() returns "about:blank" while dragData->asPlainText() contains >> the original Javascript URI, so the URL is getting sanitized before we even >> get here. > > Hmm.... interesting and slightly surprising. Non-owner LGTM. After a silly degree of flailing about, I've determined that this behavior occurs because the RenderViewHost restricts inbound URLs via the callstack: WebContentsViewAura::OnDragEntered RenderViewHostImpl::FilterDropData RenderProcessHostImpl::FilterURL ChildProcessSecurityPolicyImpl::IsWebSafeScheme JavaScript: is not a WebSafe scheme and thus FilterURL sanitizes it to about:blank
On 2016/09/09 20:43:06, Peter Kasting wrote: > Can this be tested? > > LGTM otherwise. I've done manual testing using every pattern I can think of, and I've confirmed that it impacts only Javascript: URLs and not other types. I was not able to find any existing browser tests that exercise the "drop URL on tab" functionality of the tab strip.
https://codereview.chromium.org/2323273003/diff/1/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/tab_strip.cc (right): https://codereview.chromium.org/2323273003/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab_strip.cc:1632: if ((drop_info_->url).SchemeIs(url::kJavaScriptScheme)) On 2016/09/09 20:43:06, Peter Kasting wrote: > Nit: Remove unnecessary first set of parens. Combine with conditional above. Done. https://codereview.chromium.org/2323273003/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab_strip.cc:1653: // Do nothing if the file was unsupported or the URL is invalid. The URL may On 2016/09/09 20:43:06, Peter Kasting wrote: > Nit: "...the file is unsupported, the URL is invalid, or this is a javascript: > URL (for self-XSS prevention)." Done.
On 2016/09/12 21:42:42, elawrence wrote: > On 2016/09/09 20:43:06, Peter Kasting wrote: > > Can this be tested? > > > > LGTM otherwise. > > I've done manual testing using every pattern I can think of, and I've confirmed > that it impacts only Javascript: URLs and not other types. > > I was not able to find any existing browser tests that exercise the "drop URL on > tab" functionality of the tab strip. Do we have drag-and-drop tests in general? I'm reluctant to block such a simple fix on something like that, but it does seem like we should add tests if we can.
> Do we have drag-and-drop tests in general? I'm reluctant to block such a simple > fix on something like that, but it does seem like we should add tests if we can. Any chance there's developer more familiar that TabStrip that I might ask? We do have a bunch of drag/drop tests across the product, but they seem to fall into two buckets: 1. Drag/drop reordering of tabs 2. Drag/drop of web content to web content I wasn't able to find any tests that exercise dropping anything to the tabstrip itself. \ui\views\widget\desktop_aura\desktop_drag_drop_client_aurax11_unittest.cc This looks like the most promising candidate, but as far as I can discern it's a Linux-specific test that only checks for the behavior of strings dropped into Renderers ------ \chrome\browser\ui\views\tabs\tab_unittest.cc Testing of tab's indicator animation as it simulates calls that would occur as a tab itself is moved ------ \chrome\browser\ui\views\toolbar\browser_actions_container_browsertest.cc Testing of drag/drop of "Browser Actions" icons (from extensions) ------ \src\components\bookmarks\browser\bookmark_node_data_unittest.cc Testing of drag/drop of bookmarks ------ \ash\drag_drop\drag_drop_interactive_uitest.cc \ash\drag_drop\drag_drop_controller_unittest.cc Testing of dragging of Renderer content without dropping it anywhere ------ \ui\views\controls\button\menu_button_unittest.cc \ui\views\controls\menu\menu_controller_unittest.cc \ui\views\test\menu_test_utils.cc Testing of draggable menu items ------
On 2016/09/13 20:00:49, elawrence wrote: > > Do we have drag-and-drop tests in general? I'm reluctant to block such a > simple > > fix on something like that, but it does seem like we should add tests if we > can. > > Any chance there's developer more familiar that TabStrip that I might ask? You can try sky. If he doesn't know, no one does. If it's not easy to add tests for this, I would consider just landing as-is and filing a bug (maybe against sky to triage) that we should have tests for this.
> If it's not easy to add tests for this, I would consider just landing as-is and > filing a bug (maybe against sky to triage) that we should have tests for this. Done. I spoke to sky and I've created a new bug to track the creation of tests for this area (https://bugs.chromium.org/p/chromium/issues/detail?id=646911) to unblock landing of this CL.
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 pkasting@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2323273003/#ps20001 (title: "Address nits")
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 ========== Ignore Javascript urls dropped on tabs 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. BUG=639750 ========== to ========== Ignore Javascript urls dropped on tabs 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. BUG=639750 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Ignore Javascript urls dropped on tabs 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. BUG=639750 ========== to ========== Ignore Javascript urls dropped on tabs 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. BUG=639750 Committed: https://crrev.com/d1a8cbeec4d9f9520a4bb1f4db6d3f62900ee709 Cr-Commit-Position: refs/heads/master@{#418620} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d1a8cbeec4d9f9520a4bb1f4db6d3f62900ee709 Cr-Commit-Position: refs/heads/master@{#418620} |