Bug 210256 – Remove legacy X-WebKit-CSP header support

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

Remove legacy X-WebKit-CSP header support

[Keith Miller]

Created attachment 395923 [details]
Patch

[Daniel Bates]

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.

[Geoffrey Garen]

Would be good to add an http regression test.

[Keith Miller]

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

[Keith Miller]

Created attachment 395976 [details]
Patch

[Geoffrey Garen]

Comment on attachment 395976 [details]
Patch

r=me

[Daniel Bates]

This patch looks like it's reducing test coverage. Are there tests using the non legacy header that cover all tested functionality?

[Keith Miller]

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

[Keith Miller]

Created attachment 395981 [details]
Patch for landing

[EWS]

Found 1 new test failure: http/tests/security/contentSecurityPolicy/1.1/scripthash-blocked-by-enforced-policy-and-allowed-by-report-policy.php

[Keith Miller]

Created attachment 396005 [details]
Patch for landing

[EWS]

Committed r259829: <https://trac.webkit.org/changeset/259829>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396005 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61540361>

[Alex Christensen]

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

[Keith Miller]

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