[webkit-reviews] review granted: [Bug 235873] CSP: Improve compatibility of source matching : [Attachment 450366] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Jan 30 13:56:39 PST 2022

Darin Adler <darin at apple.com> has granted Patrick Griffis
<pgriffis at igalia.com>'s request for review:
Bug 235873: CSP: Improve compatibility of source matching

Attachment 450366: Patch


--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 450366
  --> https://bugs.webkit.org/attachment.cgi?id=450366

View in context: https://bugs.webkit.org/attachment.cgi?id=450366&action=review

Generally good; room for improvement

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:240
> +    m_selfSource = makeUnique<ContentSecurityPolicySource>(*this,
m_selfSourceProtocol, securityOrigin.host(), effectivePort, emptyString(),
false, false, true);

This “false, false, true” is not a good pattern: difficult to spot mistakes.
This is why we end up using the cumbersome techniques that involve
enumerations. Otherwise there is just super subtle code like this.

> Source/WebCore/page/csp/ContentSecurityPolicy.h:173
> +    const String& selfScheme() const { return m_selfSourceProtocol; };

Why mix terminology between “scheme“ and “protocol“?

> Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:61
> +    const String& scheme = m_scheme.isEmpty() ? m_policy.selfScheme() :


> Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:62
> +    String urlScheme = url.protocol().convertToASCIILowercase();


> Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:120
> +    bool isUpgradeSecure = (port && port.value() == 443) || (!port &&
WTF::isDefaultPortForProtocol(443, url.protocol()));

“port == 443” should so the same thing as “port && port.value() == 443”

> Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:121
> +    bool isCurrentUpgradable = (m_port && m_port.value() == 80) || (m_scheme
== "http" && (!m_port || m_port.value() == 443));

Same thing here. “m_port == 80”

By the way, could also do m_port.value_or(443) == 443 or just leave out

> Source/WebCore/page/csp/ContentSecurityPolicySource.h:39
> +    ContentSecurityPolicySource(const ContentSecurityPolicy&, const String&
scheme, const String& host, std::optional<uint16_t> port, const String& path,
bool hostHasWildcard, bool portHasWildcard, bool isSelfSource);

This new argument should be an enumeration, not a bool. All callers are passing
constant values, and this is just the situation where WebKit style calls for
enumerations so that you can see at the call site what the value means.

> +PASS Expecting logs: ["allowed", "allowed"]

Not enough test coverage. This progression is great, but we need tests covering
all the improvements this patch makes.

If not already in WPT, then add to WPT or at least to WebKit tests.

More information about the webkit-reviews mailing list