Bug 210256 - Remove legacy X-WebKit-CSP header support
Summary: Remove legacy X-WebKit-CSP header support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-09 01:43 PDT by Keith Miller
Modified: 2020-04-09 20:56 PDT (History)
10 users (show)

See Also:


Attachments
Patch (4.11 KB, patch)
2020-04-09 01:44 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (66.58 KB, patch)
2020-04-09 10:57 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (65.41 KB, patch)
2020-04-09 11:27 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (63.67 KB, patch)
2020-04-09 14:00 PDT, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2020-04-09 01:43:12 PDT
Remove legacy X-WebKit-CSP header support
Comment 1 Keith Miller 2020-04-09 01:44:10 PDT
Created attachment 395923 [details]
Patch
Comment 2 Daniel Bates 2020-04-09 08:11:54 PDT
Comment on attachment 395923 [details]
Patch

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

Patch looks good. Tests should be updated.

> Source/WebCore/dom/Document.cpp:-3650
> -            contentSecurityPolicy()->didReceiveHeader(content, ContentSecurityPolicyHeaderType::PrefixedEnforce, ContentSecurityPolicy::PolicyFrom::HTTPEquivMeta, referrer(), httpStatusCode);

OK as-is. No change needed. The optimal solution also removes ContentSecurityPolicyHeaderType::PrefixedEnforce and related code.

> Source/WebCore/page/csp/ContentSecurityPolicyResponseHeaders.cpp:-50
> -        m_headers.append({ policyValue, ContentSecurityPolicyHeaderType::PrefixedReport });

OK as-is. No change needed. The optimal solution also removes ContentSecurityPolicyHeaderType::PrefixedReport and related code.
Comment 3 Geoffrey Garen 2020-04-09 09:02:38 PDT
Would be good to add an http regression test.
Comment 4 Keith Miller 2020-04-09 09:07:20 PDT
(In reply to Geoffrey Garen from comment #3)
> Would be good to add an http regression test.

My plan is to change the existing tests from reporting a failure with the current code to reporting success. That's assuming these tests are not testing both legacy and non-legacy headers in the same test.
Comment 5 Keith Miller 2020-04-09 10:57:39 PDT
Created attachment 395976 [details]
Patch
Comment 6 Geoffrey Garen 2020-04-09 11:07:13 PDT
Comment on attachment 395976 [details]
Patch

r=me
Comment 7 Daniel Bates 2020-04-09 11:12:20 PDT
This patch looks like it's reducing test coverage. Are there tests using the non legacy header that cover all tested functionality?
Comment 8 Keith Miller 2020-04-09 11:21:22 PDT
(In reply to Daniel Bates from comment #7)
> This patch looks like it's reducing test coverage. Are there tests using the
> non legacy header that cover all tested functionality?

I believe so, as there are non-legacy versions of the tests. Although, looking through it again I incorrectly rebaselined one of the tests (and its expectation). So will fix.
Comment 9 Keith Miller 2020-04-09 11:27:33 PDT
Created attachment 395981 [details]
Patch for landing
Comment 10 EWS 2020-04-09 13:04:55 PDT
Found 1 new test failure: http/tests/security/contentSecurityPolicy/1.1/scripthash-blocked-by-enforced-policy-and-allowed-by-report-policy.php
Comment 11 Keith Miller 2020-04-09 14:00:20 PDT
Created attachment 396005 [details]
Patch for landing
Comment 12 EWS 2020-04-09 14:31:09 PDT
Committed r259829: <https://trac.webkit.org/changeset/259829>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396005 [details].
Comment 13 Radar WebKit Bug Importer 2020-04-09 14:32:16 PDT
<rdar://problem/61540361>
Comment 14 Alex Christensen 2020-04-09 20:55:34 PDT
This appears to have broken the WebKit1 test http/tests/security/contentSecurityPolicy/1.1/form-action-src-redirect-blocked.html

See https://build.webkit.org/results/Apple-Catalina-Release-WK1-Tests/r259843%20(4724)/http/tests/security/contentSecurityPolicy/1.1/form-action-src-redirect-blocked-pretty-diff.html
Comment 15 Keith Miller 2020-04-09 20:56:56 PDT
(In reply to Alex Christensen from comment #14)
> This appears to have broken the WebKit1 test
> http/tests/security/contentSecurityPolicy/1.1/form-action-src-redirect-
> blocked.html
> 
> See
> https://build.webkit.org/results/Apple-Catalina-Release-WK1-Tests/
> r259843%20(4724)/http/tests/security/contentSecurityPolicy/1.1/form-action-
> src-redirect-blocked-pretty-diff.html

See, https://bugs.webkit.org/show_bug.cgi?id=210310.