Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(70)

Issue 2347163004: Disallow navigations to blob URLs with non-canonical origins. (Closed)

Created:
4 years, 3 months ago by ncarter (slow)
Modified:
4 years, 3 months ago
Reviewers:
Charlie Reis, nasko
CC:
chromium-reviews, jam, darin-cc_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Disallow navigations to blob URLs with non-canonical origins. BUG=646278 TEST=content_browsertests, included Committed: https://crrev.com/654b9b9e4b7bdec1366e1ab378b7a8f5c63fc697 Cr-Commit-Position: refs/heads/master@{#420103}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Add tests. #

Total comments: 14

Patch Set 3 : Fix issues and add unittest. #

Total comments: 6

Patch Set 4 : Charlie fixes. #

Patch Set 5 : Fix XR to blob in isolated world. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+253 lines, -2 lines) Patch
A content/browser/blob_storage/blob_url_browsertest.cc View 1 2 3 1 chunk +188 lines, -0 lines 0 comments Download
M content/browser/child_process_security_policy_impl.cc View 1 2 2 chunks +16 lines, -0 lines 0 comments Download
M content/browser/child_process_security_policy_unittest.cc View 1 2 3 1 chunk +43 lines, -0 lines 0 comments Download
M content/test/BUILD.gn View 1 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/xhr-to-blob-in-isolated-world.html View 1 2 3 4 1 chunk +5 lines, -2 lines 0 comments Download

Messages

Total messages: 38 (23 generated)
ncarter (slow)
Nasko or Charlie -- What do you think of this approach? I'm working on tests; ...
4 years, 3 months ago (2016-09-16 21:18:13 UTC) #6
ncarter (slow)
https://codereview.chromium.org/2347163004/diff/1/content/browser/child_process_security_policy_impl.cc File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2347163004/diff/1/content/browser/child_process_security_policy_impl.cc#newcode607 content/browser/child_process_security_policy_impl.cc:607: Probably need this here too.
4 years, 3 months ago (2016-09-16 21:21:04 UTC) #7
Charlie Reis
On 2016/09/16 21:18:13, ncarter wrote: > Nasko or Charlie -- > > What do you ...
4 years, 3 months ago (2016-09-16 21:45:03 UTC) #8
nasko
On 2016/09/16 21:45:03, Charlie Reis (slow) wrote: > On 2016/09/16 21:18:13, ncarter wrote: > > ...
4 years, 3 months ago (2016-09-16 22:02:57 UTC) #9
ncarter (slow)
PTAL. Test added.
4 years, 3 months ago (2016-09-19 23:53:36 UTC) #13
Charlie Reis
Thanks! LGTM with nits. https://codereview.chromium.org/2347163004/diff/20001/content/browser/blob_storage/blob_url_browsertest.cc File content/browser/blob_storage/blob_url_browsertest.cc (right): https://codereview.chromium.org/2347163004/diff/20001/content/browser/blob_storage/blob_url_browsertest.cc#newcode70 content/browser/blob_storage/blob_url_browsertest.cc:70: // Using an http page, ...
4 years, 3 months ago (2016-09-20 16:28:55 UTC) #16
ncarter (slow)
https://codereview.chromium.org/2347163004/diff/20001/content/browser/child_process_security_policy_impl.cc File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2347163004/diff/20001/content/browser/child_process_security_policy_impl.cc#newcode584 content/browser/child_process_security_policy_impl.cc:584: if (url.SchemeIsBlob() && On 2016/09/20 16:28:55, Charlie Reis (slow) ...
4 years, 3 months ago (2016-09-20 17:46:55 UTC) #17
ncarter (slow)
Significant changes since approval. PTAL https://codereview.chromium.org/2347163004/diff/20001/content/browser/blob_storage/blob_url_browsertest.cc File content/browser/blob_storage/blob_url_browsertest.cc (right): https://codereview.chromium.org/2347163004/diff/20001/content/browser/blob_storage/blob_url_browsertest.cc#newcode70 content/browser/blob_storage/blob_url_browsertest.cc:70: // Using an http ...
4 years, 3 months ago (2016-09-20 23:01:43 UTC) #20
Charlie Reis
Thanks! Updated patch LGTM with minor nits. https://codereview.chromium.org/2347163004/diff/20001/content/browser/child_process_security_policy_impl.cc File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2347163004/diff/20001/content/browser/child_process_security_policy_impl.cc#newcode566 content/browser/child_process_security_policy_impl.cc:566: bool ChildProcessSecurityPolicyImpl::CanRequestURL( ...
4 years, 3 months ago (2016-09-20 23:19:06 UTC) #21
ncarter (slow)
https://codereview.chromium.org/2347163004/diff/40001/content/browser/blob_storage/blob_url_browsertest.cc File content/browser/blob_storage/blob_url_browsertest.cc (right): https://codereview.chromium.org/2347163004/diff/40001/content/browser/blob_storage/blob_url_browsertest.cc#newcode141 content/browser/blob_storage/blob_url_browsertest.cc:141: // Location.replace shouldn't be allowed . On 2016/09/20 23:19:06, ...
4 years, 3 months ago (2016-09-21 17:03:16 UTC) #24
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/2347163004/60001
4 years, 3 months ago (2016-09-21 17:04:21 UTC) #27
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 3 months ago (2016-09-21 18:09:10 UTC) #29
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/654b9b9e4b7bdec1366e1ab378b7a8f5c63fc697 Cr-Commit-Position: refs/heads/master@{#420103}
4 years, 3 months ago (2016-09-21 18:11:51 UTC) #31
dewittj
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2358193002/ by dewittj@chromium.org. ...
4 years, 3 months ago (2016-09-21 19:18:40 UTC) #32
ncarter (slow)
4 years, 3 months ago (2016-09-21 19:30:06 UTC) #33
Message was sent while issue was closed.
Thanks for the revert. Definitely this is the reason for the layouttest
failures.

Powered by Google App Engine
This is Rietveld 408576698