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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=235873

Attachment 450366: Patch

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




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

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() :
m_scheme;

auto&

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

auto

> 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
.value().

> 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.

>
LayoutTests/imported/w3c/web-platform-tests/content-security-policy/connect-src
/connect-src-websocket-self.sub-expected.txt:2
> +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.