Skip to content

Commit

Permalink
[Permissions Policy Wildcards] (3) Propagate node source to allow lis…
Browse files Browse the repository at this point in the history
…t parser

The next CL will actually use the propagated type, but for now the
important thing is to preserve the type so that we can differentiate
attribute policies from header policies. Why not fold this CL into
the next one which actually uses the type? This code is especially
sensitive and I want to divide off portions where possible since the
next CL has some very risky code. Why not add a test? This code is all
internal, so a DCHECK and existing tests seems sufficient.
Design doc:
w3c/webappsec-permissions-policy#482

This CL is part of a series:
(1) Add function to detect subdomain matches
(2) Use OriginWithPossibleWildcards in policy
(3) Propagate node source to allow list parser
(4) Parse wildcard subdomain matches
(5) Support wildcards in getAllowlistForFeature
(6) Add WPTs
(7) Enable by default

Bug: 1345994
Change-Id: I9f5e47f3ca04b63dce0c14c03745a187a4f3171a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3936205
Commit-Queue: Ari Chivukula <arichiv@chromium.org>
Auto-Submit: Ari Chivukula <arichiv@chromium.org>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1056236}
  • Loading branch information
arichiv authored and Chromium LUCI CQ committed Oct 7, 2022
1 parent f5970fd commit b083199
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,8 @@ class ParsingContext {
static constexpr wtf_size_t MAX_LENGTH_PARSE = 1 << 16;

absl::optional<ParsedPermissionsPolicyDeclaration> ParseFeature(
const PermissionsPolicyParser::Declaration&);
const PermissionsPolicyParser::Declaration& declaration_node,
const PermissionsPolicyParser::NodeType type);

struct ParsedAllowlist {
std::vector<blink::OriginWithPossibleWildcards> allowed_origins;
Expand All @@ -123,7 +124,8 @@ class ParsingContext {
const String& feature_name);

// Parse allowlist for feature.
ParsedAllowlist ParseAllowlist(const Vector<String>& origin_strings);
ParsedAllowlist ParseAllowlist(const Vector<String>& origin_strings,
const PermissionsPolicyParser::NodeType type);

void ReportFeatureUsage(mojom::blink::PermissionsPolicyFeature feature);
void ReportFeatureUsageLegacy(mojom::blink::PermissionsPolicyFeature feature);
Expand Down Expand Up @@ -275,7 +277,11 @@ ParsingContext::ParseFeatureName(const String& feature_name) {
}

ParsingContext::ParsedAllowlist ParsingContext::ParseAllowlist(
const Vector<String>& origin_strings) {
const Vector<String>& origin_strings,
const PermissionsPolicyParser::NodeType type) {
// The source of the PermissionsPolicyParser::Node must have an explicit
// source so that we know which wildcards can be enabled.
DCHECK_NE(PermissionsPolicyParser::NodeType::kUnknown, type);
ParsedAllowlist allowlist;
if (origin_strings.empty()) {
// If a policy entry has no listed origins (e.g. "feature_name1" in
Expand Down Expand Up @@ -360,7 +366,8 @@ ParsingContext::ParsedAllowlist ParsingContext::ParseAllowlist(
} else if (target_is_opaque) {
allowlist.matches_opaque_src = true;
} else {
// TODO(crbug.com/1345994): Support wildcard matching.
// TODO(crbug.com/1345994): Support wildcard matching when type is
// kHeader.
allowlist.allowed_origins.emplace_back(
target_origin,
/*has_subdomain_wildcard=*/false);
Expand All @@ -381,13 +388,15 @@ ParsingContext::ParsedAllowlist ParsingContext::ParseAllowlist(
}

absl::optional<ParsedPermissionsPolicyDeclaration> ParsingContext::ParseFeature(
const PermissionsPolicyParser::Declaration& declaration_node) {
const PermissionsPolicyParser::Declaration& declaration_node,
const PermissionsPolicyParser::NodeType type) {
absl::optional<mojom::blink::PermissionsPolicyFeature> feature =
ParseFeatureName(declaration_node.feature_name);
if (!feature)
return absl::nullopt;

ParsedAllowlist parsed_allowlist = ParseAllowlist(declaration_node.allowlist);
ParsedAllowlist parsed_allowlist =
ParseAllowlist(declaration_node.allowlist, type);

// If same feature appeared more than once, only the first one counts.
if (feature_observer_.FeatureObserved(*feature))
Expand All @@ -414,9 +423,10 @@ ParsedPermissionsPolicy ParsingContext::ParsePermissionsPolicy(
ParsedPermissionsPolicy ParsingContext::ParsePolicyFromNode(
const PermissionsPolicyParser::Node& root) {
ParsedPermissionsPolicy parsed_policy;
for (const PermissionsPolicyParser::Declaration& declaration_node : root) {
for (const PermissionsPolicyParser::Declaration& declaration_node :
root.declarations) {
absl::optional<ParsedPermissionsPolicyDeclaration> parsed_feature =
ParseFeature(declaration_node);
ParseFeature(declaration_node, root.type);
if (parsed_feature) {
ReportFeatureUsage(parsed_feature->feature);
ReportFeatureUsageLegacy(parsed_feature->feature);
Expand All @@ -429,7 +439,8 @@ ParsedPermissionsPolicy ParsingContext::ParsePolicyFromNode(

PermissionsPolicyParser::Node ParsingContext::ParseFeaturePolicyToIR(
const String& policy) {
PermissionsPolicyParser::Node root;
PermissionsPolicyParser::Node root{
PermissionsPolicyParser::NodeType::kAttribute};

if (policy.length() > MAX_LENGTH_PARSE) {
logger_.Error("Feature policy declaration exceeds size limit(" +
Expand Down Expand Up @@ -485,7 +496,7 @@ PermissionsPolicyParser::Node ParsingContext::ParseFeaturePolicyToIR(
declaration_node.feature_name = std::move(tokens.front());
tokens.erase(tokens.begin());
declaration_node.allowlist = std::move(tokens);
root.push_back(declaration_node);
root.declarations.push_back(declaration_node);
}
}

Expand All @@ -509,7 +520,8 @@ PermissionsPolicyParser::Node ParsingContext::ParsePermissionsPolicyToIR(
return {};
}

PermissionsPolicyParser::Node ir_root;
PermissionsPolicyParser::Node ir_root{
PermissionsPolicyParser::NodeType::kHeader};
for (const auto& feature_entry : root.value()) {
const auto& key = feature_entry.first;
const char* feature_name = key.c_str();
Expand Down Expand Up @@ -560,7 +572,7 @@ PermissionsPolicyParser::Node ParsingContext::ParsePermissionsPolicyToIR(
if (allowlist.empty())
allowlist.push_back("'none'");

ir_root.push_back(
ir_root.declarations.push_back(
PermissionsPolicyParser::Declaration{feature_name, allowlist});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,13 @@ class CORE_EXPORT PermissionsPolicyParser {
String feature_name;
Vector<String> allowlist;
};
using Node = Vector<Declaration>;
enum NodeType { kHeader, kAttribute, kUnknown };
// We need to keep track of the source of the list of declarations as
// different features (e.g., wildcards) might be active per-context.
struct Node {
NodeType type{kUnknown};
Vector<Declaration> declarations;
};

// Converts a header policy string into a vector of allowlists, one for each
// feature specified. Unrecognized features are filtered out. The optional
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1612,7 +1612,8 @@ bool ManifestParser::ParseIsolatedStorage(const JSONObject* object) {

Vector<blink::ParsedPermissionsPolicyDeclaration>
ManifestParser::ParseIsolatedAppPermissions(const JSONObject* object) {
PermissionsPolicyParser::Node policy;
PermissionsPolicyParser::Node policy{
PermissionsPolicyParser::NodeType::kHeader};

JSONValue* json_value = object->Get("permissions_policy");
if (!json_value)
Expand Down Expand Up @@ -1649,7 +1650,7 @@ ManifestParser::ParseIsolatedAppPermissions(const JSONObject* object) {
String wrapped_origin = (origin == "*" ? origin : "'" + origin + "'");
new_policy.allowlist.push_back(wrapped_origin);
}
policy.push_back(new_policy);
policy.declarations.push_back(new_policy);
}

PolicyParserMessageBuffer logger(
Expand Down

0 comments on commit b083199

Please sign in to comment.