Check that the files the renderer wants to preserve as part of a session restore are already availa…

Check that the files the renderer wants to preserve as part of a session restore are already available to the renderer.

The fix involves cracking open the WebHistoryItem, extracting its file paths,
and checking them against ChildProcessSecurityPolicy for the messages
ViewHostMsg_UpdateState and ViewHostMsg_FrameNavigate.

BUG=237263
R=darin@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198965

[Tom Sepez, 2013-05-02 00:54:42]

Darin, do you have a suggestion on how to test this?

[darin (slow to review), 2013-05-02 22:15:35]

https://codereview.chromium.org/14727006/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/14727006/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_view_host_impl.cc:1246: bool
RenderViewHostImpl::CanAccessFilesOfSerializedState(
nit: this method should be listed after the ClearPowerSaveBlockers method to
preserve sort order of methods in the header file.

[Tom Sepez, 2013-05-06 22:12:26]

On 2013/05/02 22:15:35, darin wrote:
>
https://codereview.chromium.org/14727006/diff/1/content/browser/renderer_host...
> File content/browser/renderer_host/render_view_host_impl.cc (right):
> 
>
https://codereview.chromium.org/14727006/diff/1/content/browser/renderer_host...
> content/browser/renderer_host/render_view_host_impl.cc:1246: bool
> RenderViewHostImpl::CanAccessFilesOfSerializedState(
> nit: this method should be listed after the ClearPowerSaveBlockers method to
> preserve sort order of methods in the header file.

Done.

[Tom Sepez, 2013-05-06 22:14:14]

Darin, please review.  This patch set adds a test that fabricates an update
state message with a file name as part of the item, and checks for access before
and after child process security policy is called to permit it.

The ReceivedBadMessage() call should terminate the renderer in real life, but
merely counts bad messages in this test.

[darin (slow to review), 2013-05-06 23:39:47]

For completeness, is it possible to write a similar test for
RenderViewHostImpl::OnNavigate?

https://codereview.chromium.org/14727006/diff/7001/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_unittest.cc (right):

https://codereview.chromium.org/14727006/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_unittest.cc:233: std::string
file_name = "/tmp/foo";
I don't think this is going to work very well on Windows.  You should use the
PathService and pass FilePath to TestOnUpdateStateWithFile.

https://codereview.chromium.org/14727006/diff/7001/content/browser/renderer_h...
File content/browser/renderer_host/test_render_view_host.cc (right):

https://codereview.chromium.org/14727006/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/test_render_view_host.cc:369:
http_body.appendData(WebKit::WebData(char_data, sizeof(char_data)-1));
nit: use arraysize() instead of sizeof() just to be extra pedantic.

https://codereview.chromium.org/14727006/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/test_render_view_host.cc:374:
history_item.setURLString("about::blank");
nit: Using "about:blank" here might make this test fragile if we later on add
assertions that this couldn't possibly be URLs like "about:blank".  Maybe use a
more realistic HTTP URL?

[Tom Sepez, 2013-05-07 00:09:52]

On 2013/05/06 23:39:47, darin wrote:
> For completeness, is it possible to write a similar test for
> RenderViewHostImpl::OnNavigate?
> 
Possibly. Setting up the arguments is considerably more complicated here, which
is why I picked the other one to start.

>
https://codereview.chromium.org/14727006/diff/7001/content/browser/renderer_h...
> File content/browser/renderer_host/render_view_host_unittest.cc (right):
> 
>
https://codereview.chromium.org/14727006/diff/7001/content/browser/renderer_h...
> content/browser/renderer_host/render_view_host_unittest.cc:233: std::string
> file_name = "/tmp/foo";
> I don't think this is going to work very well on Windows.  You should use the
> PathService and pass FilePath to TestOnUpdateStateWithFile.
> 
ChildProcessSecuirytPolicy isn't too picky about platform-correct file strings,
so long as we don't try to access these for real, we should be OK.  Nonetheless,
I'll fix this.
>
https://codereview.chromium.org/14727006/diff/7001/content/browser/renderer_h...
> File content/browser/renderer_host/test_render_view_host.cc (right):
> 
>
https://codereview.chromium.org/14727006/diff/7001/content/browser/renderer_h...
> content/browser/renderer_host/test_render_view_host.cc:369:
> http_body.appendData(WebKit::WebData(char_data, sizeof(char_data)-1));
> nit: use arraysize() instead of sizeof() just to be extra pedantic.
> 
> 
Done.

https://codereview.chromium.org/14727006/diff/7001/content/browser/renderer_h...
> content/browser/renderer_host/test_render_view_host.cc:374:
> history_item.setURLString("about::blank");
> nit: Using "about:blank" here might make this test fragile if we later on add
> assertions that this couldn't possibly be URLs like "about:blank".  Maybe use
a
> more realistic HTTP URL?
Sure. I wanted to be sure we didn't really load anything, though.

[Tom Sepez, 2013-05-07 01:24:49]

Darin, please review: Added new test.

[darin (slow to review), 2013-05-07 04:36:19]

LGTM, thanks for those extra improvements!  My goal was to just make things a
bit more robust in case folks make reasonable code changes elsewhere (e.g.,
validating URLs or file paths).

[commit-bot: I haz the power, 2013-05-07 17:19:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/14727006/16002

[commit-bot: I haz the power, 2013-05-07 17:38:45]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-07 21:08:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/14727006/28003

[Tom Sepez, 2013-05-08 19:31:38]

Committed patchset #7 manually as r198965 (presubmit successful).