Filter more incoming URLs in the CreateWindow path.

Filter more incoming URLs in the CreateWindow path.

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

[Chris Evans, 2013-01-20 09:49:43]

A follow-up, as requested in the review of the previous related change.

[Charlie Reis, 2013-01-22 22:38:30]

Seems good to me, but one question about turning the security origin into a
GURL.

https://codereview.chromium.org/12036002/diff/8001/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/12036002/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.cc:1098: FilterURL(policy,
GetProcess(), false, &validated_params.opener_url);
Interesting.  I thought opener_url might be empty (requiring true for
|empty_allowed|) in cases that opener_suppressed is true, but that doesn't
appear to be the case.  This seems good to me if it doesn't fail in the tests.

https://codereview.chromium.org/12036002/diff/8001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12036002/diff/8001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:1847:
GURL(creator->document().securityOrigin().toString().utf8());
The SecurityOrigin documentation says that toString() may return "null" for
things like unique origins.  Does that do unexpected things when we pass it to
GURL or FilterURL?

[Chris Evans, 2013-01-24 21:58:22]

On 2013/01/22 22:38:30, creis wrote:
> Seems good to me, but one question about turning the security origin into a
> GURL.
> 
>
https://codereview.chromium.org/12036002/diff/8001/content/browser/renderer_h...
> File content/browser/renderer_host/render_view_host_impl.cc (right):
> 
>
https://codereview.chromium.org/12036002/diff/8001/content/browser/renderer_h...
> content/browser/renderer_host/render_view_host_impl.cc:1098: FilterURL(policy,
> GetProcess(), false, &validated_params.opener_url);
> Interesting.  I thought opener_url might be empty (requiring true for
> |empty_allowed|) in cases that opener_suppressed is true, but that doesn't
> appear to be the case.  This seems good to me if it doesn't fail in the tests.
> 
>
https://codereview.chromium.org/12036002/diff/8001/content/renderer/render_vi...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/12036002/diff/8001/content/renderer/render_vi...
> content/renderer/render_view_impl.cc:1847:
> GURL(creator->document().securityOrigin().toString().utf8());
> The SecurityOrigin documentation says that toString() may return "null" for
> things like unique origins.  Does that do unexpected things when we pass it to
> GURL or FilterURL?

Thanks for the thoughtful comments, Charlie. Because of them, I spend some time
with "unique" origins in both the file:/// and iframe sandbox context!

1) opener_url is indeed set correctly. (And the tests were passing from the try
run)
2) SecurityOrigin.toString() does return "null". Previously, the browser-side
would use GURL("null") and pass it to who knows where(!) Fortunately the GURL
would be flagged as invalid but still not ideal.
3) Although the effect is the same, I now explicitly check for an invalid GURL
and fully blank it. This is much more future-proof.
4) So for a unique origin, we can indeed have an empty GURL as the
opener_security_origin on the browser side. I changed a "false" to a "true" in
order to adhere to old behaviour.

PTAL?

[Charlie Reis, 2013-01-24 22:13:49]

Wow, thanks for tracing through all that.  That sounds like the right plan to
me.  LGTM!

[commit-bot: I haz the power, 2013-01-24 22:29:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cevans@chromium.org/12036002/2003

[commit-bot: I haz the power, 2013-01-24 22:29:59]

Presubmit check for 12036002-2003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/common/view_messages.h

[palmer, 2013-01-24 22:33:31]

lgtm

[commit-bot: I haz the power, 2013-01-24 22:39:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cevans@chromium.org/12036002/2003

[commit-bot: I haz the power, 2013-01-25 01:48:32]

Change committed as 178728