This repository has been archived by the owner. It is now read-only.
feat(server): Set security headers on all HTML pages. #4750
Conversation
What is the problem? The 404 page didn't have x-frame-options, content-security-policy, and x-robots-tag headers set. How does this fix it? Set up the frame-guard, csp, and noindex middlewares to set their headers on *every* HTML response. This is done by wrapping existing middlewares that only work with HTML responses in the `onHeaders` middleware. Using the `onHeaders` middleware, check if the content-type is HTML, and if so, call the original middleware to set the header. Why are all those tests updated? I updated the unit tests of frame-guard, csp, and noindex to ignore the onHeaders middleware. Additionally, since we know definitively the response is HTML, the checks to see if the middleware is applicable is no longer needed, except for CSP where no CSP headers are set for unit tests. This allowed me to remove utils.js too. The unit tests for the x-robots-tag is different, why? The test for the x-robots tag was done for all "non-JSON" responses, yet the middleware itself used to check whether the resource was HTML. I went with the middleware's view of the world and moved the x-robots-tag check to be done on every html page. The unit tests also ditch the check for `content-security-policy-rport-only` Good eye. Those tests *should have been failing*. We have not served the report-only header for quite a while, which leads me to believe that the call to csp.isCspRequired was always returning `false`. Instead of relying upon `csp.isCspRequired`, I encode whether CSP is expected within the route declarations. Assume CSP is required for all HTML resources, then add `csp: false` to those that do not.
c259c61
to
688a3f8
|
@mozilla/fxa-devs - r? @rfk - can you verify that I didn't break the x-robots-tag work you did? More info in the PR comment. |
|
Looks fine to me |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
@g-k - here's what I came up with today, not as bad as I expected.
What is the problem?
The 404 page didn't have x-frame-options, content-security-policy,
and x-robots-tag headers set.
How does this fix it?
Set up the frame-guard, csp, and noindex middlewares to set their
headers on every HTML response. This is done by wrapping existing
middlewares that only work with HTML responses in the
onHeadersmiddleware. Using the
onHeadersmiddleware, check if thecontent-type is HTML, and if so, call the original middleware to
set the header.
Why are all those tests updated?
I updated the unit tests of frame-guard, csp, and noindex to ignore
the onHeaders middleware. Additionally, since we know definitively
the response is HTML, the checks to see if the middleware is applicable
is no longer needed, except for CSP where no CSP headers are set for unit
tests. This allowed me to remove utils.js too.
The unit tests for the x-robots-tag is different, why?
The test for the x-robots tag was done for all "non-JSON" responses,
yet the middleware itself used to check whether the resource
was HTML. I went with the middleware's view of the world and moved
the x-robots-tag check to be done on every html page.
The unit tests also ditch the check for
content-security-policy-report-onlyGood eye. Those tests should have been failing. We have not served
the report-only header for quite a while, which leads me to believe that
the call to csp.isCspRequired was always returning
false. Insteadof relying upon
csp.isCspRequired, I encode whether CSP is expectedwithin the route declarations. Assume CSP is required for all HTML resources,
then add
csp: falseto those that do not.