Fix bug where we'd allow chrome-extension URLs to be loaded in incognito mode

Fix bug where we'd allow chrome-extension URLs to be loaded in incognito mode
as subresources, even though the extension is not allowed in incognito.

BUG=118721
TEST=see bug for repro; try the proof of concept in incognito mode with adblock installed but not allowed in incognito. adblock should not be detected.


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

[jam, 2012-03-20 00:52:36]

content lgtm

[Aaron Boodman, 2012-03-20 02:19:29]

https://chromiumcodereview.appspot.com/9726025/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/extension_protocols.cc (right):

https://chromiumcodereview.appspot.com/9726025/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols.cc:177: if
(info->GetResourceType() == ResourceType::MAIN_FRAME) {
Why only the main frame? Shouldn't we prevent all resources?

https://chromiumcodereview.appspot.com/9726025/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/extension_protocols_unittest.cc (right):

https://chromiumcodereview.appspot.com/9726025/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
_unittest.cc!? Mind. Blown.

https://chromiumcodereview.appspot.com/9726025/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:26: FilePath
path(FILE_PATH_LITERAL("c:\\"));
Don't need this anymore. Can create extensions w/ empty path.

https://chromiumcodereview.appspot.com/9726025/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:122: // is blocked, we
should see a different error code than "file not found".
Can we make it so that the resource does exist? I want to fix this someday to
actually return 404, and then this test will break.

[Matt Perry, 2012-03-20 17:27:33]

https://chromiumcodereview.appspot.com/9726025/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/extension_protocols.cc (right):

https://chromiumcodereview.appspot.com/9726025/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols.cc:177: if
(info->GetResourceType() == ResourceType::MAIN_FRAME) {
On 2012/03/20 02:19:29, Aaron Boodman wrote:
> Why only the main frame? Shouldn't we prevent all resources?

We prevent all resources if the extension is not incognito-enabled (line 171).
In addition, we prevent main frame resources if the extension is not split mode.

https://chromiumcodereview.appspot.com/9726025/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/extension_protocols_unittest.cc (right):

https://chromiumcodereview.appspot.com/9726025/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:26: FilePath
path(FILE_PATH_LITERAL("c:\\"));
On 2012/03/20 02:19:29, Aaron Boodman wrote:
> Don't need this anymore. Can create extensions w/ empty path.

But then GetResourceURL returns a bogus URL.

https://chromiumcodereview.appspot.com/9726025/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:122: // is blocked, we
should see a different error code than "file not found".
On 2012/03/20 02:19:29, Aaron Boodman wrote:
> Can we make it so that the resource does exist? I want to fix this someday to
> actually return 404, and then this test will break.

We could, but that would complicate the test and make it rely on the filesystem.
Doesn't seem worth it, since we can easily update the test when the expectations
change.

[Matt Perry, 2012-03-21 19:12:01]

ping

[Aaron Boodman, 2012-03-21 19:29:43]

LGTM, but can you try and figure out the GetResourceURL thing?

https://chromiumcodereview.appspot.com/9726025/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/extension_protocols.cc (right):

https://chromiumcodereview.appspot.com/9726025/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols.cc:177: if
(info->GetResourceType() == ResourceType::MAIN_FRAME) {
On 2012/03/20 17:27:33, Matt Perry wrote:
> On 2012/03/20 02:19:29, Aaron Boodman wrote:
> > Why only the main frame? Shouldn't we prevent all resources?
> 
> We prevent all resources if the extension is not incognito-enabled (line 171).
> In addition, we prevent main frame resources if the extension is not split
mode.

Ah, I remember. Thanks.

https://chromiumcodereview.appspot.com/9726025/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/extension_protocols_unittest.cc (right):

https://chromiumcodereview.appspot.com/9726025/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:122: // is blocked, we
should see a different error code than "file not found".
On 2012/03/20 17:27:33, Matt Perry wrote:
> On 2012/03/20 02:19:29, Aaron Boodman wrote:
> > Can we make it so that the resource does exist? I want to fix this someday
to
> > actually return 404, and then this test will break.
> 
> We could, but that would complicate the test and make it rely on the
filesystem.
> Doesn't seem worth it, since we can easily update the test when the
expectations
> change.

Fair enough.

[Aaron Boodman, 2012-03-21 21:18:10]

LGTM

[commit-bot: I haz the power, 2012-03-21 21:39:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpcomplete@chromium.org/9726025/19001

[commit-bot: I haz the power, 2012-03-21 23:29:25]

Change committed as 128097