Deduplicate security headers

[sadiqkhoja]

• currently security headers are defined in common-headers.conf and in the enketo location block in main conf file. This commit introduces $enketo variable, whose value is set to true in enketo location block and using this variable in common-headers to decide which CSP policy should be applied if it is enketo or otherwise.
• Added tests to ensure that all paths have correct CSP headers.

Before submitting this PR, please make sure you have:

• branched off and targeted the next branch OR only changed documentation/infrastructure (master is stable and used in production)
• verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

[alxndrsn]

It took me a while to see that this line is different (/google/ vs /github/).

I think it would be helpful to test inverse expectations as well.

Would it make sense to have expectedPolicy: 'enketo' vs expectedPolicy: 'central-frontend' or something?

One of the big frustrations with defining the CSP policy in nginx config files is that there seems to be no way to split the policies over multiple lines, which makes tracking changes to the CSPs really difficult. So it might actually provide value to redefine the full CSP for each of enketo and central-frontend policies in these tests.

E.g.:

const centralFrontendPolicy = [
  `"default-src 'none'`,
  `connect-src 'self'`,
  `font-src 'self'`,
  `frame-src 'self' https://getodk.github.io/central/news.html`,
  `img-src * data:`,
  `manifest-src 'none'`,
  `media-src 'none'`,
  `object-src 'none'`,
  `script-src 'self'`,
  `style-src 'self'`,
  `style-src-attr 'unsafe-inline'`,
  `report-uri /csp-report"`,
];

...

[
  ...
  { path: '/client-config.json', expectedPolicy: centralFrontendPolicy },
].forEach(({ path, expectedPolicy }) => {
  // when
  const res = await fetchHttps(t.request);

  // then
  assert.equal(res.headers.get('content-security-policy-report-only'), expectedPolicy.join('; '));
}

[sadiqkhoja]

I will update the tests.

I think CSP can be defined in multiple lines like this:

add_header Content-Security-Policy-Report-Only 
        "default-src 'none';
        connect-src 'self';
        font-src 'self';
        frame-src 'self' https://getodk.github.io/central/news.html;
        img-src * data:;
        manifest-src 'none';
        media-src 'none';
        object-src 'none';
        script-src 'self';
        style-src 'self';
        style-src-attr 'unsafe-inline';
        report-uri /csp-report";

Spec says that string is split by semi-colon and then each part is trimmed of whitespaces including CR

[alxndrsn]

I'd be very nervous introducing linebreaks into headers.

[alxndrsn]

I think header content is defined in RFC 9110 at https://www.rfc-editor.org/rfc/rfc9110.html#name-field-values:

  field-value    = *field-content
  field-content  = field-vchar
                   [ 1*( SP / HTAB / field-vchar ) field-vchar ]
  field-vchar    = VCHAR / obs-text
  obs-text       = %x80-FF

VCHAR is defined in RFC 5234 at https://www.rfc-editor.org/rfc/rfc5234.html#appendix-B.1:

  VCHAR          =  %x21-7E
                         ; visible (printing) characters

\n is 0x0a , so not within either range allowed for header values.

[alxndrsn]

It would be great to hear more about the motivation for this change. I think testing the CSP headers is a great idea, but I don't yet see the value in moving the enketo policy alongside the central-frontend policy - there is no technical reason why they should share any common policy.

[sadiqkhoja]

I don't yet see the value in moving the enketo policy alongside the central-frontend policy - there is no technical reason why they should share any common policy.

My motivation for this PR is to have following headers in one place, currently there are in two places i.e. odk.conf.template and common-headers.conf with the expectation that both should be modified in tandem:

add_header Referrer-Policy same-origin;
add_header Strict-Transport-Security "max-age=63072000" always;
add_header X-Frame-Options "SAMEORIGIN";
add_header X-Content-Type-Options nosniff;

[alxndrsn]

with the expectation that both should be modified in tandem

Good point.

Maybe there should be two files, e.g. global-headers.conf and central-frontend-headers.conf?

If there's a way of eliminating the separate files, that might also make life simpler.