New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ContentSecurityPolicy headers overwrite each other #159

Open
markushausammann opened this Issue Oct 21, 2018 · 6 comments

Comments

Projects
None yet
2 participants
@markushausammann

markushausammann commented Oct 21, 2018

  • I was not able to find an open or closed issue matching what I'm seeing.
  • This is not a question. (Questions should be asked on chat (Signup here) or our forums.)

It's often not allowed or recommended to have several headers with the same name. But there are situations where it is allowed or even necessary. The CSP is one of these.

https://w3c.github.io/webappsec-csp/#multiple-policies

and

https://www.w3.org/TR/CSP2/#content-security-policy-header-field

Imagine a case where a main application creates a CSP and different modules also independently add their own CSPs. The framework MUST render them all separately OR do a preemptive union merge which is what the client would otherwise do. It will (hopefully) quickly become standard practice that modules provide their own CSPs.

Code to reproduce the issue

$headers = $controller->getResponse()->getHeaders();
$headers->addHeader(new ContentSecurityPolicy($someDirectives));
$headers->addHeader(new ContentSecurityPolicy($someOtherDirectives));

Expected results

The expected result is a response with two CSP headers (OR a union merged CSP).

Actual results

The second addition overwrites the first, the response only contains that one CSP.

@markushausammann

This comment has been minimized.

markushausammann commented Oct 21, 2018

PS: It's not enough to have ContentSecurityPolicy implement MultipleHeaderInterface I think. The framework also needs to make sure to check for existing policies and append them automatically.

@markushausammann

This comment has been minimized.

markushausammann commented Oct 21, 2018

Also I'm trying to understand the MultipleHeaderInterface and I've tried to add such a multi header string via addHeaderLine but that makes Headers throw an exception on line 192: A field name was provided without a field value.

:-o

@markushausammann

This comment has been minimized.

markushausammann commented Oct 29, 2018

Am I even in the right repository? Seems this one hasn't really been touched for quite some time.

@Ocramius

This comment has been minimized.

Member

Ocramius commented Oct 29, 2018

Yeah, the repo is correct, but unless someone picks up a failing test case + patch, it's not gonna be solved. If you can manage to create a failing test, that already simplifies things enormously.

@markushausammann

This comment has been minimized.

markushausammann commented Oct 29, 2018

Ok, I'll try to find time for something... not that I'm the right person to do that.

@Ocramius

This comment has been minimized.

Member

Ocramius commented Oct 29, 2018

Yes you are: you know exactly what the bug looks like 👍

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