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

Issue 2062523002: Fixing renderer's access to a file from HTTP POST (after a xsite transfer). (Closed)

Created:
4 years, 6 months ago by Łukasz Anforowicz
Modified:
4 years, 6 months ago
Reviewers:
Charlie Reis
CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, site-isolation-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fixing renderer's access to a file from HTTP POST (after a xsite transfer). Renderer needs to access files from HTTP POST (i.e. from XSSAuditor). This CL makes sure that file access is preserved across xsite transfers. BUG=101395, 613260 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/bd82f922853ea6e61c4d907f2b51786f3ed9f5a2 Cr-Commit-Position: refs/heads/master@{#400460}

Patch Set 1 #

Patch Set 2 : One more test expectation. #

Total comments: 2

Patch Set 3 : Got the fix. #

Patch Set 4 : More self-review + the CL actually builds this time around... :-/ #

Patch Set 5 : Refactoring / deduplicating a bit of code. #

Patch Set 6 : Removed unnecessary logging + simplified condition of an "if" statement. #

Total comments: 12

Patch Set 7 : Fixing Windows build. #

Total comments: 8

Patch Set 8 : Rebasing... #

Patch Set 9 : Moved the test to another browser tests suite. #

Patch Set 10 : Removed the new (redundant) access check from RDHI. #

Total comments: 2

Patch Set 11 : Moving FileChooserDelegate into content_browser_test_utils_*internal*.h #

Unified diffs Side-by-side diffs Delta from patch set Stats (+179 lines, -42 lines) Patch
M content/browser/cross_site_transfer_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +76 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.h View 1 2 3 4 5 6 7 1 chunk +7 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 2 3 4 5 6 7 8 9 2 chunks +25 lines, -4 lines 0 comments Download
M content/browser/frame_host/render_frame_host_manager_browsertest.cc View 1 2 3 4 5 6 7 8 9 3 chunks +1 line, -26 lines 0 comments Download
M content/browser/session_history_browsertest.cc View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M content/common/resource_request_body_impl.h View 1 2 3 4 5 6 7 1 chunk +3 lines, -0 lines 0 comments Download
M content/common/resource_request_body_impl.cc View 1 2 3 4 5 6 7 1 chunk +10 lines, -0 lines 0 comments Download
M content/test/content_browser_test_utils_internal.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +22 lines, -0 lines 0 comments Download
M content/test/content_browser_test_utils_internal.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +18 lines, -0 lines 0 comments Download
A content/test/data/form_that_posts_cross_site.html View 1 chunk +15 lines, -0 lines 0 comments Download
D content/test/data/session_history/form_that_posts_cross_site.html View 1 chunk +0 lines, -10 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 30 (13 generated)
Łukasz Anforowicz
Charlie, thanks for pointing me at the old CL [1] and the problem of granting ...
4 years, 6 months ago (2016-06-10 23:07:04 UTC) #3
Łukasz Anforowicz
Actually, please ignore me - last week I must have run the test without --site-per-process ...
4 years, 6 months ago (2016-06-13 16:09:11 UTC) #4
Łukasz Anforowicz
Ok Charlie, this time around this CL should be ready for a real review :-) ...
4 years, 6 months ago (2016-06-14 01:07:17 UTC) #10
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2062523002/120001
4 years, 6 months ago (2016-06-14 16:35:15 UTC) #12
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 6 months ago (2016-06-14 17:34:28 UTC) #14
Charlie Reis
Thanks! I think I agree that a second validation check in BeginRequest is a good ...
4 years, 6 months ago (2016-06-16 20:18:00 UTC) #15
Charlie Reis
https://codereview.chromium.org/2062523002/diff/100001/content/browser/loader/resource_dispatcher_host_impl.cc File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2062523002/diff/100001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1401 content/browser/loader/resource_dispatcher_host_impl.cc:1401: } On 2016/06/14 01:07:17, Łukasz Anforowicz wrote: > AFAICT ...
4 years, 6 months ago (2016-06-16 20:22:12 UTC) #16
Łukasz Anforowicz
On 2016/06/16 20:18:00, Charlie Reis (OOO til June 18) wrote: > I think I agree ...
4 years, 6 months ago (2016-06-16 22:05:04 UTC) #17
Charlie Reis
https://codereview.chromium.org/2062523002/diff/100001/content/browser/loader/resource_dispatcher_host_impl.cc File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2062523002/diff/100001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1401 content/browser/loader/resource_dispatcher_host_impl.cc:1401: } On 2016/06/16 22:05:04, Łukasz Anforowicz wrote: > On ...
4 years, 6 months ago (2016-06-16 22:20:00 UTC) #18
Łukasz Anforowicz
Thanks Charlie. I think it should be ready to land now. https://codereview.chromium.org/2062523002/diff/100001/content/browser/loader/resource_dispatcher_host_impl.cc File content/browser/loader/resource_dispatcher_host_impl.cc (right): ...
4 years, 6 months ago (2016-06-16 23:33:56 UTC) #19
Łukasz Anforowicz
https://codereview.chromium.org/2062523002/diff/100001/content/common/resource_request_body.cc File content/common/resource_request_body.cc (right): https://codereview.chromium.org/2062523002/diff/100001/content/common/resource_request_body.cc#newcode54 content/common/resource_request_body.cc:54: if (element.type() == Element::TYPE_FILE) On 2016/06/16 23:33:55, Łukasz Anforowicz ...
4 years, 6 months ago (2016-06-17 00:10:32 UTC) #20
Charlie Reis
Thanks, LGTM without the extra kill and with just handling TYPE_FILE. https://codereview.chromium.org/2062523002/diff/100001/content/common/resource_request_body.cc File content/common/resource_request_body.cc (right): ...
4 years, 6 months ago (2016-06-17 07:13:36 UTC) #21
Łukasz Anforowicz
Thanks Charlie. https://codereview.chromium.org/2062523002/diff/100001/content/common/resource_request_body.cc File content/common/resource_request_body.cc (right): https://codereview.chromium.org/2062523002/diff/100001/content/common/resource_request_body.cc#newcode54 content/common/resource_request_body.cc:54: if (element.type() == Element::TYPE_FILE) On 2016/06/17 07:13:35, ...
4 years, 6 months ago (2016-06-17 16:44:48 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2062523002/200001
4 years, 6 months ago (2016-06-17 16:53:59 UTC) #25
commit-bot: I haz the power
Committed patchset #11 (id:200001)
4 years, 6 months ago (2016-06-17 18:21:11 UTC) #27
commit-bot: I haz the power
Patchset 11 (id:??) landed as https://crrev.com/bd82f922853ea6e61c4d907f2b51786f3ed9f5a2 Cr-Commit-Position: refs/heads/master@{#400460}
4 years, 6 months ago (2016-06-17 18:23:27 UTC) #29
benwells
4 years, 6 months ago (2016-06-20 10:56:59 UTC) #30
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/2080223002/ by benwells@chromium.org.

The reason for reverting is: New test is failing on DrMemory:

First failing build:
https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Content%2...

Sample output:
CrossSiteTransferTest.PostWithFileData:

c:\b\build\slave\drm-cr\build\src\content\browser\cross_site_transfer_browsertest.cc(458):
error: Value of: delegate->file_chosen()
Actual: false
Expected: true
[2980:4872:0617/180652:2750297:WARNING:render_frame_host_impl.cc(1927)]
OnDidStopLoading was called twice.
c:\b\build\slave\drm-cr\build\src\content\browser\cross_site_transfer_browsertest.cc(480):
error: Value of: ChildProcessSecurityPolicyImpl::GetInstance()->CanReadFile(
new_process_id, file_path)
Actual: false
Expected: true
c:\b\build\slave\drm-cr\build\src\content\browser\cross_site_transfer_browsertest.cc(490):
error: Value of: actual_page_body
Expected: has substring "test-file-content"
Actual: "------WebKitFormBoundary8oQLaVhRjyBKYRPr\nContent-Disposition:
form-data; name=\"file\"; filename=\"\"\nContent-Type:
application/octet-stream\n\n\n------WebKitFormBoundary8oQLaVhRjyBKYRPr--\n\n"
c:\b\build\slave\drm-cr\build\src\content\browser\cross_site_transfer_browsertest.cc(492):
error: Value of: actual_page_body
Expected: has substring "3488.tmp"
Actual: "------WebKitFormBoundary8oQLaVhRjyBKYRPr\nContent-Disposition:
form-data; name=\"file\"; filename=\"\"\nContent-Type:
application/octet-stream\n\n\n------WebKitFormBoundary8oQLaVhRjyBKYRPr--\n\n".

Powered by Google App Engine
This is Rietveld 408576698