Remove "open in new tab" items from context menu if the process doesn't

Remove "open in new tab" items from context menu if the process doesn't
have permission to open them directly.  For example, right-click on a
"chrome://" link in an ordinary window should show two items: Copy link
location and inspect element, but a full menu from a WebUI window itself.

NOTE: Fixing this issue requires a fix to ChildProcessSecurityPolicy, which as been silently too permissive.

BUG=104466
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113818

[Tom Sepez, 2011-11-17 21:29:31]

Since the browser_process_impl is already adding a scheme from its lofty
position in /chrome to the child process security policy in /content, seems
reasonable to make it inform the child about the other schemes that content
can't know about here, even if there is no handler for them registered later on
via the per-profile code.

Change the handling of these pseudo-schemes so that they must be blessed, apart
from the 3 special cases we already had.

[Tom Sepez, 2011-11-18 00:35:07]

So it looks like ChildProcessSecurityPolicy has been permissive since the
beginning, since it didn't realize the chrome:// wasn't an external URL.

Now that ChildProcessSecurityPolicy is actually returning "no" for chrome://
URLs unless the webui has been granted, the next test to bomb is
AllUrlsApiTest.WhitelistedExtension, which seems to be hitting a denial when it
tries to call ui_test_utils::Navigate("chrome://") while showing a data: url.

Need to figure out if we should be giving the test framework an exemption.

[Tom Sepez, 2011-11-18 17:42:49]

The first place the WhiteListedExtension is tripping over this is:

	ChildProcessSecurityPolicy::SecurityState::CanRequestURL() [0x36c2116]
	ChildProcessSecurityPolicy::CanRequestURL() [0x36c19c2]
	RenderViewHost::FilterURL() [0x37a641c]
	TabContents::OnDidStartProvisionalLoadForFrame() [0x380b8eb]

[Tom Sepez, 2011-11-18 20:26:34]

Part of the problem is that when transitioning from a data: URL to the
chrome://version URL, the security test is being made against the RVH associated
with data:, not the pending one associated with the navigation to chrome://

Unfortunately, the new RVH doesn't have its WebUI permission bits set either.

[Tom Sepez, 2011-11-18 20:59:54]

RenderViewHostManager::UpdateRendererStateForNavigate() isn't setting
pending_web_ui_ because chrome://version is a browser about handler, not a
full-fledged web_ui.  Thus RenderViewHostManager::InitRenderView() doesn't call
allowBindings() on the RVH.

[Cris Neckar, 2011-11-18 22:15:57]

I am absolutely certain that I don't even approach understanding this well
enough to review it. Adam I think this is on you :)

On 2011/11/18 20:59:54, Tom Sepez wrote:
> RenderViewHostManager::UpdateRendererStateForNavigate() isn't setting
> pending_web_ui_ because chrome://version is a browser about handler, not a
> full-fledged web_ui.  Thus RenderViewHostManager::InitRenderView() doesn't
call
> allowBindings() on the RVH.

[Tom Sepez, 2011-12-01 00:13:30]

Adam, might as well kick off a review.  Need to wait for LKGR to advance a bit
before the trybots have a chance at this ...

[abarth-chromium, 2011-12-01 00:22:35]

Hum...  Can you ping me again about this tomorrow?  I'm a bit too frazzled right
now to think it through clearly.

http://codereview.chromium.org/8588039/diff/16001/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/8588039/diff/16001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:152:
policy->RegisterWebSafeScheme(chrome::kChromeDevToolsScheme);
Why is dev tools web-safe?  You shouldn't be able to follow hyperlinks into the
inspector.  That would be strange.

http://codereview.chromium.org/8588039/diff/16001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:153:
policy->RegisterPseudoScheme(chrome::kChromeUIScheme);
Why is ChromeUIScheme a pseudo scheme?  That doesn't seem right.

[Tom Sepez, 2011-12-01 00:43:35]

http://codereview.chromium.org/8588039/diff/16001/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/8588039/diff/16001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:152:
policy->RegisterWebSafeScheme(chrome::kChromeDevToolsScheme);
Looks like this is in response to breaking the devtools.  Will remove this and
retest.

http://codereview.chromium.org/8588039/diff/16001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:153:
policy->RegisterPseudoScheme(chrome::kChromeUIScheme);
On 2011/12/01 00:22:35, abarth wrote:
> Why is ChromeUIScheme a pseudo scheme?  That doesn't seem right.

This has to do with ChildProcessSecurityPolicy::CanRequestURL() and its
overzealous desire to conclude that URLs are destined for the shell if
!net::URLRequest::IsHandledURL(url).  We change this to conclude that shell
requests also aren't pseudo-schemes, and add chrome to that list.  chrome smells
like a pseudo scheme if its not being handled by URLRequest, no?

[abarth-chromium, 2011-12-01 00:45:49]

The ChromeUIScheme is handled by URLRequest.

[tsepez (do not use), 2011-12-01 01:13:55]

Seems like it should be.  But adding some tracing to
child_process_security_policy (just to get this in print so I don't have to
trust my gdb-fu):

 if (!net::URLRequest::IsHandledURL(url))
    LOG(WARNING) << "IsHandledURL() is false for " << url.spec();

results in the following when I right-click on a chrome link on an http
page:

[7880:7880:1740865125093:WARNING:child_process_security_policy.cc(387)]
IsHandledURL() is false for chrome://flags/

Maybe that's where the bug lies.  But does the net:: layer 's idea of
URLRequest grok a chrome:// ?? seems like layering is off if it does

On Wed, Nov 30, 2011 at 4:45 PM, <abarth@chromium.org> wrote:

> The ChromeUIScheme is handled by URLRequest.
>
>
http://codereview.chromium.**org/8588039/<http://codereview.chromium.org/8588...
>

[abarth-chromium, 2011-12-01 01:32:52]

I feel like this bug keeps coming up.  The "chrome" scheme should be a
handled URL.  I'm nto sure what's involved in fixing that.  Whatever
registers the chrome scheme to get URLRequests should mark it as
handled.

Adam


On Wed, Nov 30, 2011 at 5:13 PM, Thomas Sepez <tsepez@google.com> wrote:
> Seems like it should be.  But adding some tracing to
> child_process_security_policy (just to get this in print so I don't have to
> trust my gdb-fu):
>
>  if (!net::URLRequest::IsHandledURL(url))
>     LOG(WARNING) << "IsHandledURL() is false for " << url.spec();
>
> results in the following when I right-click on a chrome link on an http
> page:
>
> [7880:7880:1740865125093:WARNING:child_process_security_policy.cc(387)]
> IsHandledURL() is false for chrome://flags/
>
> Maybe that's where the bug lies.  But does the net:: layer 's idea of
> URLRequest grok a chrome:// ?? seems like layering is off if it does
>
> On Wed, Nov 30, 2011 at 4:45 PM, <abarth@chromium.org> wrote:
>>
>> The ChromeUIScheme is handled by URLRequest.
>>
>> http://codereview.chromium.org/8588039/
>
>

[Tom Sepez, 2011-12-02 02:07:36]

Here's why this is hard:
There's a single static net::URLRequest::IsHandledURL() for the whole browser. 
Schemes never get added to it.
There's a per-profile net::URLRequestJobFactory object which schemes get added
to when the profile is opened.  It already has an isHandledURL() function that
is updated with the new protocols. So we want to call this function instead of
the net::URLRequest::IsHandledURL.

This means that the per-process ChildProcessSecurityPolicy object must determine
the appropriate profile to retrieve the URLRequestJobFactory.

I haven't been able to convince myself that a single per-process CPSP will
always correspond to a single profile.  Do we launch separate renderers etc for
the same site under different profiles?  If we do keep this separation, then it
should be possible to link them.

Even so,  profiles (and hence the path to the URJF) is a chrome layer concept,
and CPSP is a content layer object.  There's some layering work to overcome.

[abarth-chromium, 2011-12-02 21:25:46]

I believe there is only one profile per render process.

Adam


On Thu, Dec 1, 2011 at 6:07 PM,  <tsepez@chromium.org> wrote:
> Here's why this is hard:
> There's a single static net::URLRequest::IsHandledURL() for the whole
> browser.
> Schemes never get added to it.
> There's a per-profile net::URLRequestJobFactory object which schemes get
> added
> to when the profile is opened.  It already has an isHandledURL() function
> that
> is updated with the new protocols. So we want to call this function instead
> of
> the net::URLRequest::IsHandledURL.
>
> This means that the per-process ChildProcessSecurityPolicy object must
> determine
> the appropriate profile to retrieve the URLRequestJobFactory.
>
> I haven't been able to convince myself that a single per-process CPSP will
> always correspond to a single profile.  Do we launch separate renderers etc
> for
> the same site under different profiles?  If we do keep this separation, then
> it
> should be possible to link them.
>
> Even so,  profiles (and hence the path to the URJF) is a chrome layer
> concept,
> and CPSP is a content layer object.  There's some layering work to overcome.
>
>
>
> http://codereview.chromium.org/8588039/

[Tom Sepez, 2011-12-07 22:34:40]

Some comments for Patch Set #10:  Not happy with content API but lets do some
testing to find what's broken.

Condensed summary about why I'm doing this:

CPSP is still relying on net::URLRequest::isHandledURL() to see if a request is
destined for an external handler.  This is wrong in the new world, where only
the URLRequestJobFactory knows the real answer.  As a result CPSP thinks too
many protocols are externally handled, and thus has been silently too permissive
for some time.  Part of the fix to this can be to pass an URJF to CPSP, which
allows the correct result to be obtained -- so long as the JobFactory is
available and one is on the IO thread.

Where I get into trouble is with RenderViewHost::FilterURL, which wants to call
into CPSP as part of its filtering.  This happens on the UI thread, so I don't
want to bounce over the the IO thread to get the correct answer. 

One approach would be to make a thread-safe URJF, but that is too large a change
for this purpose.  Instead add an API to allow CPSP to get at
ProfileIOData::IsHandledURL() for the case where we aren't allowed to poke at
the job factory.

[jam, 2011-12-07 23:37:04]

api lgtm. just one question

http://codereview.chromium.org/8588039/diff/31003/content/browser/mock_conten...
File content/browser/mock_content_browser_client.cc (right):

http://codereview.chromium.org/8588039/diff/31003/content/browser/mock_conten...
content/browser/mock_content_browser_client.cc:94: return
net::URLRequest::IsHandledURL(url);
why isn't the mock impl returning false?

[Tom Sepez, 2011-12-07 23:43:01]

yeah, I'll fix that.  Just stuck it there to get a test in.  Probably should be
something like:

bool OverrideIsHandledURL(const GURL&, bool *is_handled);

Returning true if it was overriden with *is_handled set or false if the default
should continue in CPSP.

[Tom Sepez, 2011-12-07 23:51:34]

(Trying to avoid two calls to check the same thing since the one in
profileIOData covers the cases in net::)

[Tom Sepez, 2011-12-08 00:30:12]

But I guess there's no reason to believe that an embedder is always going to
have this property of covering for net:: itself, so  I think its back to having
the IsHandledURL() API and making a second call on false in CPSP.

[Tom Sepez, 2011-12-08 22:06:01]

I don't think that its right that CPSP can return different results depending
upon whether URJFs are present.  So until we get a thread safe URJF, I'm just
going to fall back to the new Content Client API.  This leads to a vast
simplification.

Rework the CPSP tests to stop using the deprecated global registry functions --
these don't happen in the real world anymore.  By continuing to call them, they
masked the broken real-word CPSP behaviour in the tests.  Instead, mock the new
Client Content API.

[Tom Sepez, 2011-12-08 22:24:41]

+AVI to get a reviewer for chrome/browser/tab_contents

[Avi (use Gerrit), 2011-12-08 23:20:30]

On 2011/12/08 22:24:41, Tom Sepez wrote:
> +AVI to get a reviewer for chrome/browser/tab_contents

jam@ still wants to review all API changes. Poke him.

[Tom Sepez, 2011-12-08 23:29:00]

John, would you take a final look when you get a chance?  thanks.

[jam, 2011-12-09 01:46:53]

On 2011/12/08 23:29:00, Tom Sepez wrote:
> John, would you take a final look when you get a chance?  thanks.

lgtm

Avi: just to be clear, I'd like to review new interfaces when they go in to make
sure they belong in content. but for updates to existing APIs, feel free to
review.

[Avi (use Gerrit), 2011-12-09 06:26:21]

OK, no problem.