Commit 0d2fc4e4 authored by Devlin Cronin's avatar Devlin Cronin Committed by Commit Bot

[Extensions Click-to-Script] Support URL patterns in the extensions page

Rather than only displaying hosts, display the actual URLPatterns
granted to an extension. Given this is a power-user surface, we can be
a little less concerned with the technicality shown. Additionally,
since we don't use a full string (e.g., "Read and change your data on
all google.com sites"), we need to have the differentiation of which
hosts the extension is granted, such as whether it includes subdomains.

Also allow the user to enter patterns as an URLPattern (thus including
subdomains if they so desire). Enforce that paths are always '*', since
we don't use path-based permissions. To make it easier on users, also
support just entering the host name (e.g., 'google.com'), which will be
expanded to '*://google.com/*'.

This also properly fixes the issue of clicking 'remove' or 'edit' on
existing host options.

Add JS tests for pattern validation, and update unittests to support
pattern-based host manipulation.

Bug: 844128

Change-Id: I599151bb6b89bdb113af4c39dcf0e21d152aa0fe
Reviewed-on: https://chromium-review.googlesource.com/1166596
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: default avatarKaran Bhatia <karandeepb@chromium.org>
Reviewed-by: default avatarSteven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585299}
parent 0fe599e9
......@@ -34,6 +34,7 @@
#include "chrome/browser/extensions/extension_ui_util.h"
#include "chrome/browser/extensions/extension_util.h"
#include "chrome/browser/extensions/install_verifier.h"
#include "chrome/browser/extensions/permissions_updater.h"
#include "chrome/browser/extensions/scripting_permissions_modifier.h"
#include "chrome/browser/extensions/shared_module_service.h"
#include "chrome/browser/extensions/unpacked_installer.h"
......@@ -267,6 +268,25 @@ developer::LoadError CreateLoadError(
return response;
}
base::Optional<URLPattern> ParseRuntimePermissionsPattern(
const std::string& pattern_str) {
constexpr int kValidRuntimePermissionSchemes = URLPattern::SCHEME_HTTP |
URLPattern::SCHEME_HTTPS |
URLPattern::SCHEME_FILE;
URLPattern pattern(kValidRuntimePermissionSchemes);
if (pattern.Parse(pattern_str) != URLPattern::PARSE_SUCCESS)
return base::nullopt;
// We don't allow adding paths for permissions, because they aren't meaningful
// in terms of origin access. The frontend should validate this, but there's
// a chance something can slip through, so we should fail gracefully.
if (pattern.path() != "/*")
return base::nullopt;
return pattern;
}
} // namespace
namespace ChoosePath = api::developer_private::ChoosePath;
......@@ -1969,30 +1989,27 @@ DeveloperPrivateAddHostPermissionFunction::Run() {
developer::AddHostPermission::Params::Create(*args_));
EXTENSION_FUNCTION_VALIDATE(params);
GURL host(params->host);
if (!host.is_valid() || host.path_piece().length() > 1 || host.has_query() ||
host.has_ref()) {
base::Optional<URLPattern> pattern =
ParseRuntimePermissionsPattern(params->host);
if (!pattern)
return RespondNow(Error(kInvalidHost));
}
const Extension* extension = GetExtensionById(params->extension_id);
if (!extension)
return RespondNow(Error(kNoSuchExtensionError));
ScriptingPermissionsModifier scripting_modifier(browser_context(), extension);
if (!scripting_modifier.CanAffectExtension())
if (!ScriptingPermissionsModifier(browser_context(), extension)
.CanAffectExtension()) {
return RespondNow(Error(kCannotChangeHostPermissions));
// Only grant withheld permissions. This also ensures that we won't grant
// any permission for a host that shouldn't be accessible to the extension,
// like chrome:-scheme urls.
if (!extension->permissions_data()
->withheld_permissions()
.HasEffectiveAccessToURL(host)) {
return RespondNow(Error("Cannot grant a permission that wasn't withheld."));
}
scripting_modifier.GrantHostPermission(host);
URLPatternSet new_host_permissions({*pattern});
PermissionsUpdater(browser_context())
.GrantRuntimePermissions(
*extension,
PermissionSet(APIPermissionSet(), ManifestPermissionSet(),
new_host_permissions, new_host_permissions));
return RespondNow(NoArguments());
}
......@@ -2007,11 +2024,10 @@ DeveloperPrivateRemoveHostPermissionFunction::Run() {
developer::RemoveHostPermission::Params::Create(*args_));
EXTENSION_FUNCTION_VALIDATE(params);
GURL host(params->host);
if (!host.is_valid() || host.path_piece().length() > 1 || host.has_query() ||
host.has_ref()) {
base::Optional<URLPattern> pattern =
ParseRuntimePermissionsPattern(params->host);
if (!pattern)
return RespondNow(Error(kInvalidHost));
}
const Extension* extension = GetExtensionById(params->extension_id);
if (!extension)
......@@ -2021,10 +2037,18 @@ DeveloperPrivateRemoveHostPermissionFunction::Run() {
if (!scripting_modifier.CanAffectExtension())
return RespondNow(Error(kCannotChangeHostPermissions));
if (!scripting_modifier.HasGrantedHostPermission(host))
URLPatternSet host_permissions_to_remove({*pattern});
std::unique_ptr<const PermissionSet> permissions_to_remove =
PermissionSet::CreateIntersection(
PermissionSet(APIPermissionSet(), ManifestPermissionSet(),
host_permissions_to_remove, host_permissions_to_remove),
*scripting_modifier.GetRevokablePermissions(),
URLPatternSet::IntersectionBehavior::kDetailed);
if (permissions_to_remove->IsEmpty())
return RespondNow(Error("Cannot remove a host that hasn't been granted."));
scripting_modifier.RemoveGrantedHostPermission(host);
PermissionsUpdater(browser_context())
.RevokeRuntimePermissions(*extension, *permissions_to_remove);
return RespondNow(NoArguments());
}
......
......@@ -62,6 +62,7 @@
#include "extensions/common/extension_set.h"
#include "extensions/common/manifest_constants.h"
#include "extensions/common/permissions/permission_set.h"
#include "extensions/common/permissions/permissions_data.h"
#include "extensions/common/value_builder.h"
#include "extensions/test/test_extension_dir.h"
#include "services/data_decoder/data_decoder_service.h"
......@@ -1346,19 +1347,33 @@ TEST_F(DeveloperPrivateApiUnitTest, GrantHostPermission) {
}
};
GURL host("https://example.com");
EXPECT_FALSE(modifier.HasGrantedHostPermission(host));
run_add_host_permission(host.spec(), true, nullptr);
const GURL kExampleCom("https://example.com/");
EXPECT_FALSE(modifier.HasGrantedHostPermission(kExampleCom));
run_add_host_permission("https://example.com/*", true, nullptr);
EXPECT_TRUE(modifier.HasGrantedHostPermission(kExampleCom));
const GURL kGoogleCom("https://google.com");
const GURL kMapsGoogleCom("https://maps.google.com/");
EXPECT_FALSE(modifier.HasGrantedHostPermission(kGoogleCom));
EXPECT_FALSE(modifier.HasGrantedHostPermission(kMapsGoogleCom));
run_add_host_permission("https://*.google.com/*", true, nullptr);
EXPECT_TRUE(modifier.HasGrantedHostPermission(kGoogleCom));
EXPECT_TRUE(modifier.HasGrantedHostPermission(kMapsGoogleCom));
run_add_host_permission(kInvalidHost, false, kInvalidHostError);
// Path of the pattern must exactly match "/*".
run_add_host_permission("https://example.com/", false, kInvalidHostError);
run_add_host_permission("https://example.com/foobar", false,
kInvalidHostError);
run_add_host_permission("https://example.com/#foobar", false,
kInvalidHostError);
run_add_host_permission("https://example.com/*foobar", false,
kInvalidHostError);
// Cannot grant chrome:-scheme URLs.
GURL chrome_host("chrome://settings/*");
run_add_host_permission(chrome_host.spec(), false, kInvalidHostError);
GURL chrome_host("chrome://settings");
run_add_host_permission(chrome_host.spec(), false,
"Cannot grant a permission that wasn't withheld.");
EXPECT_FALSE(modifier.HasGrantedHostPermission(chrome_host));
}
......@@ -1391,22 +1406,41 @@ TEST_F(DeveloperPrivateApiUnitTest, RemoveHostPermission) {
}
};
GURL host("https://example.com");
run_remove_host_permission(host.spec(), false,
run_remove_host_permission("https://example.com/*", false,
"Cannot remove a host that hasn't been granted.");
modifier.GrantHostPermission(host);
EXPECT_TRUE(modifier.HasGrantedHostPermission(host));
const GURL kExampleCom("https://example.com");
modifier.GrantHostPermission(kExampleCom);
EXPECT_TRUE(modifier.HasGrantedHostPermission(kExampleCom));
// Path of the pattern must exactly match "/*".
run_remove_host_permission("https://example.com/", false, kInvalidHostError);
run_remove_host_permission("https://example.com/foobar", false,
kInvalidHostError);
run_remove_host_permission("https://example.com/#foobar", false,
kInvalidHostError);
run_remove_host_permission("https://example.com/*foobar", false,
kInvalidHostError);
run_remove_host_permission(kInvalidHost, false, kInvalidHostError);
EXPECT_TRUE(modifier.HasGrantedHostPermission(host));
EXPECT_TRUE(modifier.HasGrantedHostPermission(kExampleCom));
run_remove_host_permission(host.spec(), true, nullptr);
EXPECT_FALSE(modifier.HasGrantedHostPermission(host));
run_remove_host_permission("https://example.com/*", true, nullptr);
EXPECT_FALSE(modifier.HasGrantedHostPermission(kExampleCom));
URLPattern new_pattern(Extension::kValidHostPermissionSchemes,
"https://*.google.com/*");
PermissionsUpdater(profile()).GrantRuntimePermissions(
*extension, PermissionSet(APIPermissionSet(), ManifestPermissionSet(),
URLPatternSet({new_pattern}), URLPatternSet()));
const GURL kGoogleCom("https://google.com/");
const GURL kMapsGoogleCom("https://maps.google.com/");
EXPECT_TRUE(modifier.HasGrantedHostPermission(kGoogleCom));
EXPECT_TRUE(modifier.HasGrantedHostPermission(kMapsGoogleCom));
run_remove_host_permission("https://*.google.com/*", true, nullptr);
EXPECT_FALSE(modifier.HasGrantedHostPermission(kGoogleCom));
EXPECT_FALSE(modifier.HasGrantedHostPermission(kMapsGoogleCom));
}
TEST_F(DeveloperPrivateApiUnitTest, UpdateHostAccess) {
......@@ -1499,6 +1533,71 @@ TEST_F(DeveloperPrivateApiUnitTest,
EXPECT_FALSE(modifier.HasGrantedHostPermission(example_com));
}
TEST_F(DeveloperPrivateApiUnitTest,
UpdateHostAccess_GrantScopeGreaterThanRequestedScope) {
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeature(features::kRuntimeHostPermissions);
scoped_refptr<const Extension> extension =
ExtensionBuilder("test").AddPermission("http://*/*").Build();
service()->AddExtension(extension.get());
ScriptingPermissionsModifier modifier(profile(), extension.get());
modifier.SetWithholdHostPermissions(true);
ExtensionPrefs* extension_prefs = ExtensionPrefs::Get(profile());
EXPECT_EQ(PermissionSet(),
extension->permissions_data()->active_permissions());
EXPECT_EQ(PermissionSet(),
*extension_prefs->GetRuntimeGrantedPermissions(extension->id()));
{
scoped_refptr<UIThreadExtensionFunction> function =
base::MakeRefCounted<api::DeveloperPrivateAddHostPermissionFunction>();
std::string args = base::StringPrintf(
R"(["%s", "%s"])", extension->id().c_str(), "*://chromium.org/*");
EXPECT_TRUE(api_test_utils::RunFunction(function.get(), args, profile()))
<< function->GetError();
}
// The active permissions (which are given to the extension process) should
// only include the intersection of what was requested by the extension and
// the runtime granted permissions - which is http://chromium.org/*.
URLPattern http_chromium(Extension::kValidHostPermissionSchemes,
"http://chromium.org/*");
const PermissionSet http_chromium_set(
APIPermissionSet(), ManifestPermissionSet(),
URLPatternSet({http_chromium}), URLPatternSet());
EXPECT_EQ(http_chromium_set,
extension->permissions_data()->active_permissions());
// The runtime granted permissions should include all of what was approved by
// the user, which is *://chromium.org/*, and should be present in both the
// scriptable and explicit hosts.
URLPattern all_chromium(Extension::kValidHostPermissionSchemes,
"*://chromium.org/*");
const PermissionSet all_chromium_set(
APIPermissionSet(), ManifestPermissionSet(),
URLPatternSet({all_chromium}), URLPatternSet({all_chromium}));
EXPECT_EQ(all_chromium_set,
*extension_prefs->GetRuntimeGrantedPermissions(extension->id()));
{
scoped_refptr<UIThreadExtensionFunction> function = base::MakeRefCounted<
api::DeveloperPrivateRemoveHostPermissionFunction>();
std::string args = base::StringPrintf(
R"(["%s", "%s"])", extension->id().c_str(), "*://chromium.org/*");
EXPECT_TRUE(api_test_utils::RunFunction(function.get(), args, profile()))
<< function->GetError();
}
// Removing the granted permission should remove it entirely from both
// the active and the stored permissions.
EXPECT_EQ(PermissionSet(),
extension->permissions_data()->active_permissions());
EXPECT_EQ(PermissionSet(),
*extension_prefs->GetRuntimeGrantedPermissions(extension->id()));
}
TEST_F(DeveloperPrivateApiUnitTest, ExtensionUpdatedEventOnPermissionsChange) {
DeveloperPrivateAPI::Get(profile());
const ExtensionId listener_id = crx_file::id_util::GenerateId("listener");
......
......@@ -273,24 +273,33 @@ void AddPermissionsInfo(content::BrowserContext* browser_context,
if (!permissions_modifier.HasWithheldHostPermissions()) {
permissions->host_access = developer::HOST_ACCESS_ON_ALL_SITES;
} else {
constexpr bool include_rcd = true;
constexpr bool exclude_file_scheme = true;
std::set<std::string> distinct_hosts =
permission_message_util::GetDistinctHosts(
active_permissions.effective_hosts(), include_rcd,
exclude_file_scheme);
// TODO(devlin): This isn't quite right - it's possible the user just
// selected "on specific sites" from the dropdown, and hasn't yet added
// any sites. We'll need to handle this.
// https://crbug.com/844128.
if (!distinct_hosts.empty()) {
ExtensionPrefs* extension_prefs = ExtensionPrefs::Get(browser_context);
std::unique_ptr<const PermissionSet> runtime_granted_permissions =
extension_prefs->GetRuntimeGrantedPermissions(extension.id());
if (runtime_granted_permissions->effective_hosts().is_empty()) {
// TODO(devlin): This isn't quite right - it's possible the user just
// selected "on specific sites" from the dropdown, and hasn't yet added
// any sites. We'll need to handle this.
// https://crbug.com/844128.
permissions->host_access = developer::HOST_ACCESS_ON_CLICK;
} else {
permissions->host_access = developer::HOST_ACCESS_ON_SPECIFIC_SITES;
std::set<std::string> distinct_hosts;
for (auto pattern : runtime_granted_permissions->effective_hosts()) {
// We only allow addition/removal of full hosts (since from a
// permissions point of view, path is irrelevant). We always make the
// path wildcard when adding through this UI, but the optional
// permissions API may allow adding permissions with paths.
// TODO(devlin): Investigate, and possibly change the optional
// permissions API.
pattern.SetPath("/*");
distinct_hosts.insert(pattern.GetAsString());
}
permissions->runtime_host_permissions =
std::make_unique<std::vector<std::string>>(
std::make_move_iterator(distinct_hosts.begin()),
std::make_move_iterator(distinct_hosts.end()));
} else {
permissions->host_access = developer::HOST_ACCESS_ON_CLICK;
}
}
}
......
......@@ -36,7 +36,10 @@
#include "extensions/common/extension_features.h"
#include "extensions/common/feature_switch.h"
#include "extensions/common/permissions/permission_message.h"
#include "extensions/common/permissions/permission_set.h"
#include "extensions/common/permissions/permissions_data.h"
#include "extensions/common/url_pattern.h"
#include "extensions/common/url_pattern_set.h"
#include "extensions/common/value_builder.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
......@@ -430,7 +433,7 @@ TEST_F(ExtensionInfoGeneratorUnitTest, RuntimeHostPermissions) {
info->permissions.host_access);
ASSERT_TRUE(info->permissions.runtime_host_permissions);
EXPECT_THAT(*info->permissions.runtime_host_permissions,
testing::UnorderedElementsAre("example.com"));
testing::UnorderedElementsAre("https://example.com/*"));
EXPECT_THAT(info->permissions.simple_permissions, testing::IsEmpty());
// An extension that doesn't request any host permissions should not have
......@@ -457,6 +460,50 @@ TEST_F(ExtensionInfoGeneratorUnitTest, RuntimeHostPermissionsWithoutFeature) {
info->permissions.simple_permissions[0].message);
}
// Tests that runtime_host_permissions is correctly populated when permissions
// are granted by the user beyond what the extension originally requested in the
// manifest.
TEST_F(ExtensionInfoGeneratorUnitTest,
RuntimeHostPermissionsBeyondRequestedScope) {
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeature(features::kRuntimeHostPermissions);
scoped_refptr<const Extension> extension =
CreateExtension("extension", ListBuilder().Append("http://*/*").Build(),
Manifest::INTERNAL);
std::unique_ptr<developer::ExtensionInfo> info =
GenerateExtensionInfo(extension->id());
// Withhold permissions, and grant *://chromium.org/*.
ScriptingPermissionsModifier permissions_modifier(profile(), extension);
permissions_modifier.SetWithholdHostPermissions(true);
URLPattern all_chromium(Extension::kValidHostPermissionSchemes,
"*://chromium.org/*");
PermissionSet all_chromium_set(APIPermissionSet(), ManifestPermissionSet(),
URLPatternSet({all_chromium}),
URLPatternSet({all_chromium}));
PermissionsUpdater(profile()).GrantRuntimePermissions(*extension,
all_chromium_set);
// The extension should only be granted http://chromium.org/* (since that's
// the intersection with what it requested).
URLPattern http_chromium(Extension::kValidHostPermissionSchemes,
"http://chromium.org/*");
EXPECT_EQ(PermissionSet(APIPermissionSet(), ManifestPermissionSet(),
URLPatternSet({http_chromium}), URLPatternSet()),
extension->permissions_data()->active_permissions());
// The generated info should use the entirety of the granted permission,
// which is *://chromium.org/*.
info = GenerateExtensionInfo(extension->id());
EXPECT_EQ(developer::HOST_ACCESS_ON_SPECIFIC_SITES,
info->permissions.host_access);
ASSERT_TRUE(info->permissions.runtime_host_permissions);
EXPECT_THAT(*info->permissions.runtime_host_permissions,
testing::UnorderedElementsAre("*://chromium.org/*"));
}
// Test that file:// access checkbox does not show up when the user can't
// modify an extension's settings. https://crbug.com/173640.
TEST_F(ExtensionInfoGeneratorUnitTest, ExtensionInfoLockedAllUrls) {
......
......@@ -5,6 +5,39 @@
cr.define('extensions', function() {
'use strict';
// A RegExp to roughly match acceptable patterns entered by the user.
// exec'ing() this RegExp will match the following groups:
// 0: Full matched string.
// 1: Scheme + scheme separator (e.g., 'https://').
// 2: Scheme only (e.g., 'https').
// 3: Match subdomains ('*.').
// 4: Hostname (e.g., 'example.com').
// 5: Port, including ':' separator (e.g., ':80').
// 6: Path, include '/' separator (e.g., '/*').
const patternRegExp = new RegExp(
'^' +
// Scheme; optional.
'((http|https|\\*)://)?' +
// Include subdomains specifier; optional.
'(\\*\\.)?' +
// Hostname, required.
'([a-z0-9\\.-]+\\.[a-z0-9]+)' +
// Port, optional.
'(:[0-9]+)?' +
// Path, optional but if present must be '/' or '/*'.
'(\\/\\*|\\/)?' +
'$');
function getPatternFromSite(site) {
let res = patternRegExp.exec(site);
assert(res);
let scheme = res[1] || '*://';
let host = (res[3] || '') + res[4];
let port = res[5] || '';
let path = '/*';
return scheme + host + port + path;
}
const RuntimeHostsDialog = Polymer({
is: 'extensions-runtime-hosts-dialog',
......@@ -62,17 +95,7 @@ cr.define('extensions', function() {
return;
}
let valid = true;
try {
// Check if the input parses as a valid URL. JS URL parsing isn't
// great, but it's quick and easy and better than nothing. If it's still
// not valid, the API will throw an error and the embedding element will
// call setInputInvalid().
let testUrl = new URL(this.site_);
} catch (e) {
valid = false;
}
let valid = patternRegExp.test(this.site_);
this.inputInvalid_ = !valid;
},
......@@ -138,7 +161,8 @@ cr.define('extensions', function() {
* @private
*/
addPermission_: function() {
this.delegate.addRuntimeHostPermission(this.itemId, this.site_)
let pattern = getPatternFromSite(this.site_);
this.delegate.addRuntimeHostPermission(this.itemId, pattern)
.then(
() => {
this.$.dialog.close();
......@@ -149,5 +173,8 @@ cr.define('extensions', function() {
},
});
return {RuntimeHostsDialog: RuntimeHostsDialog};
return {
RuntimeHostsDialog: RuntimeHostsDialog,
getPatternFromSite: getPatternFromSite
};
});
......@@ -36,7 +36,7 @@ suite('RuntimeHostsDialog', function() {
let id = args[0];
let input = args[1];
assertEquals(ITEM_ID, id);
assertEquals(site, input);
assertEquals('http://www.example.com/*', input);
});
});
......@@ -66,7 +66,7 @@ suite('RuntimeHostsDialog', function() {
delegate.acceptRuntimeHostPermission = false;
const input = dialog.$$('cr-input');
const site = 'http://http://http://';
const site = 'http://....a';
input.value = site;
input.fire('input');
assertFalse(input.invalid);
......@@ -81,12 +81,12 @@ suite('RuntimeHostsDialog', function() {
});
test('editing current entry', function() {
const oldSite = 'http://example.com';
const newSite = 'http://chromium.org';
const oldPattern = 'http://example.com/*';
const newPattern = 'http://chromium.org/*';
dialog.currentSite = oldSite;
dialog.currentSite = oldPattern;
const input = dialog.$$('cr-input');
input.value = newSite;
input.value = newPattern;
input.fire('input');
const submit = dialog.$.submit;
......@@ -94,12 +94,32 @@ suite('RuntimeHostsDialog', function() {
return delegate.whenCalled('removeRuntimeHostPermission')
.then((args) => {
expectEquals(ITEM_ID, args[0] /* id */);
expectEquals(oldSite, args[1] /* site */);
expectEquals(oldPattern, args[1] /* pattern */);
return delegate.whenCalled('addRuntimeHostPermission');
})
.then((args) => {
expectEquals(ITEM_ID, args[0] /* id */);
expectEquals(newSite, args[1] /* site */);
expectEquals(newPattern, args[1] /* pattern */);
});
});
test('get pattern from url', function() {
expectEquals(
'https://example.com/*',
extensions.getPatternFromSite('https://example.com/*'));
expectEquals(
'https://example.com/*',
extensions.getPatternFromSite('https://example.com/'));
expectEquals(
'https://example.com/*',
extensions.getPatternFromSite('https://example.com'));
expectEquals(
'https://*.example.com/*',
extensions.getPatternFromSite('https://*.example.com/*'));
expectEquals(
'*://example.com/*', extensions.getPatternFromSite('example.com'));
expectEquals(
'https://example.com:80/*',
extensions.getPatternFromSite('https://example.com:80/*'));
});
});
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment