Skip to content

content security policy audit#1331

Merged
rBangay merged 5 commits into
mainfrom
content-security-policy-audit
Apr 23, 2024
Merged

content security policy audit#1331
rBangay merged 5 commits into
mainfrom
content-security-policy-audit

Conversation

@rBangay

@rBangay rBangay commented Apr 11, 2024

Copy link
Copy Markdown
Contributor

What does this change?

Adds a content security policy report header to all requests so that we can audit which endpoints need adding to a future content-security-policy header

as per docs at https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy-Report-Only

Example of csp report in cloudwatch logs

Screenshot 2024-04-19 at 13 04 22

…hat we can audit which endpoints need adding to a future content-security-policy header
Comment thread server/routes/core.ts Outdated
Comment thread server/routes/core.ts Outdated
const router = Router();

router.get('/csp-audit-report-endpoint', (req: Request, res: Response) => {
log.info(`CSP_AUDIT: ${req.body}`);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sometimes cloudwatch is a bit irritating and splits multi line log statements across more than one log line. It does have smarts to spot json, but if we hit this problem, we can always indent subsequent lines (which should help)

Comment thread server/server.ts Outdated
server.use(function (_: Request, res: Response, next: NextFunction) {
res.setHeader(
'Content-Security-Policy-Report-Only',
'report-to /csp-audit-report-endpoint',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the documentation is inconsistent about how report-to works https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/report-to
it sounds like report-uri has recently been deprecated so it may not be completely up to date in the docs?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated it to send both report-uri and report-to ... which should give us the best browser coverage (report-uri is ignored for browsers that support report-to apparently, also firefox at the time of writing doesn't support report-to)

…2 to 204 as it is not returning any content

modify the header setting to use both report-uri and the newer report-to to ensure cross browser support

@tjmw tjmw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this looks great 👍 I've not used Content-Security-Policy-Report-Only before so it's cool to see this in practice.

At the moment the policy added here is specifying https for all content and reporting violations, is the next step to add the more exhaustive policy defined in #1330 (but report only, not enforce)?

@johnduffell

Copy link
Copy Markdown
Member

At the moment the policy added here is specifying https for all content and reporting violations, is the next step to add the more exhaustive policy defined in #1330 (but report only, not enforce)?

Good question - when we discussed it we were imagining to start from zero and only add the ones we saw used, because we didn't even know why some of them were there, and they may only be relevant to acquisition (and may not even be relevant there ay more - after all, who would think to delete things?)
But once we see some results we could take a judgement call in the name of efficiency?

@rBangay
rBangay force-pushed the content-security-policy-audit branch 2 times, most recently from 8df2d68 to 75f2f6f Compare April 19, 2024 11:38
…s that do no require sign in.

use express json middleware for the csp audit endpoint request object so that you can log the json in human readable form
@rBangay
rBangay force-pushed the content-security-policy-audit branch from 75f2f6f to fce7ef4 Compare April 19, 2024 12:01

@tjmw tjmw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great!

One thought I had about the potential noise from the new endpoint - does MMA have an existing feature flag or switch mechanism? If so I wonder if it'd be worth wrapping the log line in a switch that we can easily toggle the logging back and forth without deploying. No worries if not.

@rBangay

rBangay commented Apr 22, 2024

Copy link
Copy Markdown
Contributor Author

This looks great!

One thought I had about the potential noise from the new endpoint - does MMA have an existing feature flag or switch mechanism? If so I wonder if it'd be worth wrapping the log line in a switch that we can easily toggle the logging back and forth without deploying. No worries if not.

Good question ... there are feature switches of sort in manage-frontend, but they are not controlled by RRCP or any sort of CMS, they're just variables in a feature switch ts file. I'll put the audit stuff behind a switch though 👍

Comment thread server/routes/api.ts
if (featureSwitches.cspSecurityAudit) {
router.post('/csp-audit-report-endpoint', (req, res) => {
const parsedBody = JSON.parse(req.body.toString());
log.warn(JSON.stringify(parsedBody));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at the screenshot in the description, it ends up as double nested JSON. Is there a way to inline this JSON? or at least prettyPrint i.e. 2 space indent etc?
No worries if not but it might help!

@johnduffell johnduffell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fab I'm impressed how in depth that was / how much we learnt! 👍

@rBangay
rBangay merged commit 158376a into main Apr 23, 2024
@rBangay
rBangay deleted the content-security-policy-audit branch April 23, 2024 08:58
@prout-bot

Copy link
Copy Markdown

Seen on PROD (merged by @rBangay 9 minutes and 30 seconds ago) Please check your changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants