content/browser/resource_request_body_browser_utils.h - Issue 2908433003: RenderFrameProxyHost::OnOpenURL needs to validate resource request body.

Side by Side Diff: content/browser/resource_request_body_browser_utils.h

Issue 2908433003: RenderFrameProxyHost::OnOpenURL needs to validate resource request body. (Closed)

Patch Set: Covering RenderFrameHostImpl as well and rearranging the test to pass on Android. Created 3 years, 6 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

« content/browser/frame_host/render_frame_host_impl.cc ('K') | « content/browser/frame_host/render_frame_proxy_host.cc ('k') | content/browser/resource_request_body_browser_utils.cc » ('j') | content/browser/resource_request_body_browser_utils.cc » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 // Copyright 2017 The Chromium Authors. All rights reserved.

	2 // Use of this source code is governed by a BSD-style license that can be

	3 // found in the LICENSE file.

	4

	5 #ifndef CONTENT_BROWSER_RESOURCE_REQUEST_BODY_BROWSER_UTILS_H_

	6 #define CONTENT_BROWSER_RESOURCE_REQUEST_BODY_BROWSER_UTILS_H_

	7

	8 #include "base/memory/ref_counted.h"

	9

	10 namespace content {

	11

	12 class SiteInstance;

	13 class ResourceRequestBodyImpl;

	14

	15 // Checks if \|site_instance\| can read all elements of \|body\|.
	alexmos 2017/05/25 23:44:06 nit: Maybe elaborate a little bit? I.e., validate nit: Maybe elaborate a little bit? I.e., validate that the renderer process for \|site_instance\| is allowed to access files in the POST upload data specified by \|body\|. Also, probably more for your other CL, but maybe also mention what can be called on what thread (since you'll need the deduplicated helper on both UI and IO threads, though this one takes a SiteInstance so should be UI thread). Łukasz Anforowicz 2017/05/26 00:05:14 Done. Show quoted text On 2017/05/25 23:44:06, alexmos wrote: > nit: Maybe elaborate a little bit? I.e., validate that the renderer process for > \|site_instance\| is allowed to access files in the POST upload data specified by > \|body\|. Done. Show quoted text > > Also, probably more for your other CL, but maybe also mention what can be called > on what thread (since you'll need the deduplicated helper on both UI and IO > threads, though this one takes a SiteInstance so should be UI thread). Done.
	16 // TODO(lukasza): Remove code duplication - the function below should be reused

	17 // by RenderFrameHostImpl::OnBeginNavigation and

	18 // ResourceDispatcherHostImpl::ShouldServiceRequest.

	19 bool CanReadRequestBody(SiteInstance* site_instance,

	20 const scoped_refptr<ResourceRequestBodyImpl>& body);
	Łukasz Anforowicz 2017/05/25 19:56:01 Does this look okay? I had some comments about th Does this look okay? I had some comments about the file/dir for this new function in https://codereview.chromium.org/2905763002/diff/20001/content/browser/resourc... mmenke@ had some (optional) suggestions about possibly putting the new function into a static-only class or namespace. I'll rebase https://codereview.chromium.org/2905763002 in a minute. alexmos 2017/05/25 23:44:06 Hmm, this might be ok. A static-only class might Show quoted text On 2017/05/25 19:56:01, Łukasz A. wrote: > Does this look okay? > > I had some comments about the file/dir for this new function in > https://codereview.chromium.org/2905763002/diff/20001/content/browser/resourc... > > mmenke@ had some (optional) suggestions about possibly putting the new function > into a static-only class or namespace. > > I'll rebase https://codereview.chromium.org/2905763002 in a minute. Hmm, this might be ok. A static-only class might make it easier to discover, but I think it'd make more sense if you had multiple of these helpers that it could help group together. But maybe also run this by nick@? Łukasz Anforowicz 2017/05/26 00:05:14 Ok - see https://codereview.chromium.org/290843300 Show quoted text On 2017/05/25 23:44:06, alexmos wrote: > On 2017/05/25 19:56:01, Łukasz A. wrote: > > Does this look okay? > > > > I had some comments about the file/dir for this new function in > > > https://codereview.chromium.org/2905763002/diff/20001/content/browser/resourc... > > > > mmenke@ had some (optional) suggestions about possibly putting the new > function > > into a static-only class or namespace. > > > > I'll rebase https://codereview.chromium.org/2905763002 in a minute. > > Hmm, this might be ok. A static-only class might make it easier to discover, > but I think it'd make more sense if you had multiple of these helpers that it > could help group together. But maybe also run this by nick@? Ok - see https://codereview.chromium.org/2908433003/diff/80001/content/browser/resourc...
	21

	22 } // namespace content

	23

	24 #endif // CONTENT_BROWSER_RESOURCE_REQUEST_BODY_BROWSER_UTILS_H_

OLD	NEW