Bug 2061264 - Remove CSP header from REST API responses - #2695
Conversation
| # Compared against the type alone, with any parameters (charset, boundary) | ||
| # stripped first, so entries here must not carry any. | ||
| use constant CSP_DOCUMENT_TYPES => | ||
| ('text/html', 'application/xhtml+xml', 'image/svg+xml'); |
There was a problem hiding this comment.
This list leaves out the XML variants, do they belong here?
| # Compared against the type alone, with any parameters (charset, boundary) | ||
| # stripped first, so entries here must not carry any. | ||
| use constant CSP_DOCUMENT_TYPES => | ||
| ('text/html', 'application/xhtml+xml', 'image/svg+xml'); |
There was a problem hiding this comment.
Also worth cross-referencing %_SAFE_INLINE_TYPES / is_executable_content_type in Bugzilla::Attachment from here. We now have two lists in the tree answering roughly "what does a browser render as a scriptable document", and they disagree; a pointer in each direction makes the divergence deliberate rather than accidental.
|
|
||
| # End to end. A native /rest route: JSON, and the route that would otherwise | ||
| # enforce. | ||
| has_no_csp($t->get_ok('/rest/config/component_teams'), 'native REST route'); |
There was a problem hiding this comment.
Add a ->status_is(200) here. Without it a 401 or 500 REST error still passes (the error body is JSON too), so the test could quietly stop covering the real payload path.
| # policy there is read by nobody: it just adds ~1KB to every response and | ||
| # costs a Bugzilla->user lookup plus a nonce to build (see the | ||
| # content_security_policy helper in Bugzilla::App::Plugin::Glue). | ||
| if (_response_is_document($res) && !$res->headers->content_security_policy) { |
There was a problem hiding this comment.
Before this change, every response without an existing CSP header got one. Now only responses _response_is_document() classifies as documents do.
3xx redirects aren't excluded by $res->is_empty, only 1xx/204/304 do, but typically have no Content-Type set either, so they fall into "not a document" and silently lose the CSP header (I think at the login redirect but maybe others are concerned too).
If it was intentional, it's worth a line in the comment above, if not, redirects should probably be treated as documents too.
| # or renderer happened to use. | ||
| my ($type) = split /;/, $res->headers->content_type // ''; | ||
| $type = lc trim($type // ''); | ||
| return 1 if any { $_ eq $type } CSP_DOCUMENT_TYPES; |
There was a problem hiding this comment.
No Content-Type at all (redirects) currently falls through to "not a document" and loses CSP (cf. my previous comment on line 197). I suggest to treat an absent/empty type as a document by default (REST/static/rendered responses always set one explicitly, so this only affects the redirect case).
Add something like return 1 unless length $type; after line 228:
my ($type) = split /;/, $res->headers->content_type // '';
$type = lc trim($type // '');
# No Content-Type at all default to document rather than silently dropping CSP
return 1 unless length $type;
return 1 if any { $_ eq $type } CSP_DOCUMENT_TYPES;
| 'the document policy still points at the collector' | ||
| ); | ||
|
|
||
| done_testing; |
There was a problem hiding this comment.
This can be pinned directly rather than left to a comment in Constants.pm at line 912: assert every CSP-scoped type is also treated as executable by Bugzilla::Attachment
I suggest to add this test at the end, before done_testing;:
require Bugzilla::Attachment;
ok(Bugzilla::Attachment::is_executable_content_type($_),
"$_ is treated as executable, matching its CSP-document scope")
for (CSP_DOCUMENT_TYPES, qw(application/xhtml+xml image/svg+xml application/rdf+xml application/atom+xml application/rss+xml));
And a comment explaining that "any type CSP-scope treats as a document must also be one Attachment refuses to call 'safe to inline' (cf. comment at Constants.pm:912)"
Xzzz
left a comment
Xzzz
left a comment
There was a problem hiding this comment.
I’ve suggested a few changes in my comments. They aren’t necessary, but are just a precaution.
Feel free to add or modify them
Summary
CSP headers were being sent on every response, including REST API responses. This limits them to responses a browser actually renders as a document.
The header is added in one place, the
after_dispatchhook inBugzilla::App, and that hook runs for every response: native Mojo routes, legacy CGI scripts, static files and the REST API alike. The only guard was "is a CSP header alreadyset?", so
/rest,/api,/bzapi,/latest,rest.cgiand the static JSON endpoints all received one. Native routes enforce rather than report, so REST responses were getting a full enforcing policy with areport-uri.Nothing was broken by this, since CSP is inert on a JSON response and API clients send no violation reports. It was pure overhead: roughly 1KB on every API response, plus a
Bugzilla->userlookup and a nonce generated per requestto build a policy that no client would ever read.
Changes
Bugzilla/Constants.pm: new exportedCSP_DOCUMENT_TYPESlisting the content types a browser renders as a document (text/html,application/xhtml+xml,image/svg+xml), alongside the other CSP constants.Bugzilla/App.pm: new_response_is_document, consulted by theafter_dispatchhook before the header is added. It skips bodiless responses, then compares the content type against the list with parameters stripped andcase normalized.
t/csp-scope.t: new test pinning which responses carry a policy at all, as opposed tot/csp-enforce.twhich pins enforcing vs report-only for the ones that do.t/csp-enforce.t: the collector's own 204 no longer asserts an enforcing header, with a comment pointing at the new test.Behavior
/home, legacy CGI)show_bug.cgi?format=modal,report.cgi,chart.cgi)/rest,/api,/bzapi,/latest,rest.cgi/__version__)/csp_report(204)text/plain,application/octet-stream)Dropping the policy from downloads is safe because X-Content-Type-Options: nosniff` is set globally in the same hook, so a
download cannot be re-sniffed into a document.
The status check in
_response_is_documentis not redundant with the content type check. A response with no body still gets a content type from the renderer and it defaults totext/html. The CSP collector is exactly that shape,render(data => '', status => 204), so without the status check it looks like a document and gets handed a policy governing a page that does not exist. The first test run caught this.Testing
t/csp-scope.t,t/csp-enforce.t,t/csp-report.t: 107 tests, all pass.t/app-cgi-request-limit.t,t/critic.t,t/markdown.t), all confirmedpre-existing by running them against a clean checkout of the base commit and getting byte-identical failures, including the same subtest numbers.