Set even more strict CSP header in redirect response

[ljeda]

suggested fix for the too broad (wildcard) CSP directives (frame-ancestors and form-action) in redirect response.

fixes #187

[UlisesGascon]

Thanks for prepare this PR and for the context @ljeda!

I think that impose unnecessary restrictions that might conflict with how the consuming applications are designed to operate. For example:

• If a consuming app relies on iframe embedding (e.g., for previewing content), frame-ancestors 'none' could break their functionality.
• If forms are a core part of their app, form-action 'none' will prevent all form submissions, potentially breaking workflows.

It is possible to use setHeaders (docs) to add this limits when using the library.

const express = require('express');
const serveStatic = require('serve-static');

const app = express();

// Serve static files with custom headers
app.use(
  serveStatic('public', {
    setHeaders: (res, path, stat) => {
      // Set custom headers
      res.setHeader(
        'Content-Security-Policy',
        "default-src 'self'; frame-ancestors 'none'; form-action 'none';"
      );
    },
  })
);

app.listen(3000, () => {
  console.log('Server running on http://localhost:3000');
});

[wesleytodd]

I agree, and think this is what folks should use helmet for: https://www.npmjs.com/package/helmet#content-security-policy

[ljeda]

Thanks @UlisesGascon and @wesleytodd for the comments!

I have one remark though. Please notice that the part of code I'm referring to is related with the behavior of the serve-static in which every call to the URL that does not end with a slash is redirected to a path ending with a slash:

serve-static/index.js

Lines 181 to 207 in e2bf828

	function createRedirectDirectoryListener () {
	return function redirect (res) {
	if (this.hasTrailingSlash()) {
	this.error(404)
	return
	}
	// get original URL
	var originalUrl = parseUrl.original(this.req)
	// append trailing slash
	originalUrl.path = null
	originalUrl.pathname = collapseLeadingSlashes(originalUrl.pathname + '/')
	// reformat the URL
	var loc = encodeUrl(url.format(originalUrl))
	var doc = createHtmlDocument('Redirecting', 'Redirecting to ' + escapeHtml(loc))
	// send redirect response
	res.statusCode = 301
	res.setHeader('Content-Type', 'text/html; charset=UTF-8')
	res.setHeader('Content-Length', Buffer.byteLength(doc))
	res.setHeader('Content-Security-Policy', "default-src 'none'")
	res.setHeader('X-Content-Type-Options', 'nosniff')
	res.setHeader('Location', loc)
	res.end(doc)
	}

As far as I understand when serving redirect we shouldn't worry about any form actions or iframes (happy to be wrong) as we're just serving the simple redirect page that takes us to the final page.

I've already tried to play around with setHeaders mentioned but looking at the code function createRedirectDirectoryListener does not get called while serving the redirect while the headers are explicitly being set there, so whatever I put to setHeaders, it's not effective.

[wesleytodd]

we shouldn't worry about any form actions or iframes (happy to be wrong)

I am honestly not sure about this, but I think it is reasonable for someone to serve a static asset in an iframe. And while I have not played around with this behavior at all to see what this new restriction might break I think unless someone comes with a clear and present risk around this default I believe we are better off recommending folks tighten their security posture with helmet than to mistakenly restrict a legitimate use case with a lower level package like this one.

[ljeda]

@wesleytodd I see your point. I was assuming the CSP headers will not prevent the redirect, but seems like I was wrong:
https://security.stackexchange.com/questions/264277/do-i-need-csp-frame-ancestors-on-a-redirect-page

I was hoping that maybe alternatively we could just add the same ability to customize the headers by using the mentioned setHeaders param (docs), it seemed for me a bit counter-intuitive that these are not applied when serve-static is responding with redirect.

I've tried using helmet, but seems like due to the way the redirect is constructed in serve-static, the redirect response does not get the CSP headers I've specified in helmet config.

[wesleytodd]

I've tried using helmet, but seems like due to the way the redirect is constructed in serve-static, the redirect response does not get the CSP headers I've specified in helmet config.

Ah, interesting. Yes this would set the header in a way that overrides helmet. As it stands with this code there is no way to tighten the security in user-land code without fully overriding this behavior. That is not good and is something I would love to see a PR for. It might be tricky from a design perspective as we would want to have similar handling for all of the headers this is setting. I could see this going two ways:

1. Allow overriding the redirect handler method entirely (and then working with helmet to document this usage)
2. Only setting the headers if not already set

If you are interested in this work, please open a new issue or PR and we can discuss further in that.