CSP: Improve compatibility of source matching
Created attachment 450365 [details] Patch
Created attachment 450366 [details] Patch
Comment on attachment 450366 [details] 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.
Created attachment 450564 [details] Patch
(In reply to Darin Adler from comment #3) I applied all of your fixes. > 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. I ended up limiting this patch is scope a bit by removing the default port changes, as WebKit tests asserted otherwise and I honestly think its correct. I'll try to have a discussion with some Chromium folks about this. In the end the test cases covers http->wss and http->ws which is the majority of this change. I don't see an obvious way to set up WPT to use non-http custom protocols that can upgrade to https. If that sounds reasonable to you I'll commit it soon.
It’s always risky to make improvements that don’t have regression tests. In past, when that happened, we expanded the capabilities of WebKit regression tests so we could test the improvements, sometimes using tests written in C++ or Objective-C if we couldn’t do them in LayoutTests-style tests. But that would not block landing the fix. It’s important follow-through for the health of the product, though. We don’t want a future contributor to unknowingly break this.
Comment on attachment 450564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450564&action=review > Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:120 > + bool isUpgradeSecure = (port == 443) || (!port && WTF::isDefaultPortForProtocol(443, url.protocol())); I don’t think the WTF:: prefix is needed here. > Source/WebCore/page/csp/ContentSecurityPolicySourceList.h:64 > - bool hasWildcard { false }; > + HasWildcard hasWildcard { HasWildcard::No }; Is this change helpful? I don’t strongly object, after all it’s consistent with what we have to do to make function arguments look good, and is helpful if we are using structure initialization syntax, but I’m not sure how you decided. > Source/WebCore/page/csp/ContentSecurityPolicySourceList.h:68 > - bool hasWildcard { false }; > + HasWildcard hasWildcard { HasWildcard::No }; Ditto.
(In reply to Darin Adler from comment #7) > Is this change helpful? I don’t strongly object, after all it’s consistent > with what we have to do to make function arguments look good, and is helpful > if we are using structure initialization syntax, but I’m not sure how you > decided. It was only for consistency in the area.
Well the data member is just a bool, not a HasWildcard, so it’t not completely consistent.
Alright, I'll change it back. Also found bug 235992 while in the area.
rdar://83788668
(In reply to Patrick Griffis from comment #10) > Alright, I'll change it back. > > Also found bug 235992 while in the area. Did you intend to do more here? I'm not sure if we should review either of these patches, or if the work was done elsewhere?
(In reply to Brent Fulgham from comment #12) > (In reply to Patrick Griffis from comment #10) > > Alright, I'll change it back. > > > > Also found bug 235992 while in the area. > > Did you intend to do more here? I'm not sure if we should review either of > these patches, or if the work was done elsewhere? I was intending for another revision. Just got sick and pulled to other tasks. It's on my list.
*** Bug 201591 has been marked as a duplicate of this bug. ***
Created attachment 456375 [details] Patch
Created attachment 456376 [details] Patch
Comment on attachment 456376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456376&action=review Looks good! I see Darin already reviewed it but I just had one spec question. > Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:70 > + return true; I'm looking at the spec: " More formally, two ASCII strings (A and B) are said to scheme-part match if the following algorithm returns "Matches": 1. If one of the following is true, return "Matches": 1. A is an ASCII case-insensitive match for B. 2. A is an ASCII case-insensitive match for "http", and B is an ASCII case-insensitive match for "https". 3. A is an ASCII case-insensitive match for "ws", and B is an ASCII case-insensitive match for "wss", "http", or "https". 4. A is an ASCII case-insensitive match for "wss", and B is an ASCII case-insensitive match for "https". 2. Return "Does Not Match". " It seems we directly cover (1) with the first check of (scheme == urlScheme). We cover (2) with (scheme == "http" && urlScheme == "https"). And we cover part of (3) with (scheme == "ws" && urlScheme == "wss"). Do we also need a check for scheme == 'ws' && urlScheme == 'http'/'https' and (4)? > Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp:125 > + // Also not allowed by the Content Security Policy Level 3 spec., we allow a data URL to match Tiny nit that already existed before your patch: we usually try to make comments full sentences with periods at the end.
Created attachment 456390 [details] Patch
(In reply to Kate Cheney from comment #17) > It seems we directly cover (1) with the first check of (scheme == > urlScheme). We cover (2) with (scheme == "http" && urlScheme == "https"). > And we cover part of (3) with (scheme == "ws" && urlScheme == "wss"). Do we > also need a check for scheme == 'ws' && urlScheme == 'http'/'https' and (4)? You're right. Updated to handle this. > > Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp:125 > > + // Also not allowed by the Content Security Policy Level 3 spec., we allow a data URL to match > > Tiny nit that already existed before your patch: we usually try to make > comments full sentences with periods at the end. This was a multi-line comment that ended with a period.
Comment on attachment 456390 [details] Patch > > Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp:125 > > > + // Also not allowed by the Content Security Policy Level 3 spec., we allow a data URL to match > > > > Tiny nit that already existed before your patch: we usually try to make > > comments full sentences with periods at the end. > This was a multi-line comment that ended with a period. Nice! I missed the second line.
Committed r292266 (249164@main): <https://commits.webkit.org/249164@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 456390 [details].