Skip to content
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

[WebProfiler] Do not add src-elem CSP directives if they do not exist #36678

Merged
merged 1 commit into from May 4, 2020

Conversation

ndench
Copy link
Contributor

@ndench ndench commented May 4, 2020

Q A
Branch? 3.4, 4.4, 5.0
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #36643
License MIT
Doc PR n/a

In the latest 3.4., 4.4. and 5.0.* branches the script-src-elem and style-src-elem directives are added to the Content-Security-Policy header if they don't exist by copying the default-src. This causes browsers to ignore the script-src and style-src directives which likely contain scripts and styles the developer wanted to allow.

As mentioned in the fixed ticket, we shouldn't be adding these directives if they don't exist because the browser will automatically fallback to script-src and style-src which we have already added unsafe-inlen and the nonce-* to.

This will need to be merged into 3.4, 4.4 and 5.0, but I was unsure which branch I am meant to base it off to start with. I've put it on 4.4 but can move it to another if required.

@ndench
Copy link
Contributor Author

ndench commented May 4, 2020

I can't see how my change would have caused Appveyor to fail. Here's the error:

There was 1 failure:
1) Symfony\Bundle\FrameworkBundle\Tests\Functional\DebugAutowiringCommandTest::testNotConfusedByClassAliases
Failed asserting that '\r\n
Autowirable Types\r\n
=================\r\n
\r\n
 The following classes & interfaces can be used as type-hints when autowiring:\r\n
 (only showing classes/interfaces matching ClassAlias)\r\n
 \r\n
 Symfony\Bundle\FrameworkBundle\Tests\Fixtures\ClassAliasExampleClass (public)\r\n
\r\n
\r\n
' contains "Symfony\Bundle\FrameworkBundle\Tests\Fixtures\ClassAliasExampleClass (public)".
C:\projects\symfony\src\Symfony\Bundle\FrameworkBundle\Tests\Functional\DebugAutowiringCommandTest.php:110

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone May 4, 2020
@nicolas-grekas nicolas-grekas changed the base branch from 4.4 to 3.4 May 4, 2020 13:18
@nicolas-grekas
Copy link
Member

nicolas-grekas commented May 4, 2020

I rebased this PR for 3.4.

@cs278 / @ampaze can you please have a look?

@ndench would you mind having a look at #36645 too?

@nicolas-grekas
Copy link
Member

Thank you @ndench.

@nicolas-grekas nicolas-grekas merged commit cf0d086 into symfony:3.4 May 4, 2020
@ndench ndench deleted the fix-csp-src-elem branch May 5, 2020 00:29
@cs278
Copy link
Contributor

cs278 commented May 5, 2020

Great thanks, just tested this works as expected. 👍

fabpot added a commit that referenced this pull request May 13, 2020
…nch)

This PR was merged into the 4.4 branch.

Discussion
----------

[WebProfiler] Remove 'none' when appending CSP tokens

| Q             | A
| ------------- | ---
| Branch?       | 3.4, 4.4, 5.0
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #36645
| License       | MIT
| Doc PR        | n/a

@nicolas-grekas  asked me to to have a look at this after #36678.

If a user has a CSP policy of `default-src 'none'`, then the WebProfiler copies `'none'` to `script-src` and `style-src` then adds other sources. This creates an invalid policy since `'none'` is only allowed when it's the only item in the source list.

This will probably need to be merged into 3.4 first, I started on 4.4 so I can test in my current symfony project which requires 4.4.

Commits
-------

967bc4a [WebProfiler] Remove 'none' when appending CSP tokens
This was referenced May 31, 2020
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.

None yet

4 participants