NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[2.8][WebProfilerBundle] Fix bundle usage in Content-Security-Policy context#18434

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

romainneutron wants to merge1 commit intosymfony:2.8fromromainneutron:fix-csp-webprofiler-2.8

Closed

[2.8][WebProfilerBundle] Fix bundle usage in Content-Security-Policy context#18434

romainneutron wants to merge1 commit intosymfony:2.8fromromainneutron:fix-csp-webprofiler-2.8

Conversation

Copy link

Contributor

romainneutron commentedApr 4, 2016

Q	A
Branch?	2.8
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#15397
License	MIT
Doc PR	N/A

Hello, this PR fixes the compatibility of the WebprofilerBundle in a context where Content-Security-Policy headers are could preventunsafe-inline ofscript-src orstyle-src directives.

I intentionally open this PR against 2.8 instead of 2.3. Fixing this in 2.3/2.7 is a PITA due to the legacy codebase of the WebProfiler. It would imply doing the job twice. Moreover, doing this is not trivial at all. I hope you all understand.

Job is done at 80%, so I open the PR to get feedback.

Add tests to theContentSecurityPolicyHandler andWebDebugToolbarListener

romainneutron added WebProfilerBundle Status: Needs Review labels

Apr 4, 2016

romainneutron force-pushed thefix-csp-webprofiler-2.8 branch 3 times, most recently fromb2ff331 to20b4c7fCompare

April 4, 2016 12:55

hhamon reviewed

Apr 4, 2016

View reviewed changes

src/Symfony/Bundle/WebProfilerBundle/CSP/ContentSecurityPolicyHandler.php Outdated


		$headers = $this->getCSPHeaders($response);

		foreach($headers as $header => $directives) {

Copy link

Contributor

hhamonApr 4, 2016

Choose a reason for hiding this comment

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

missing space afterforeach.

Copy link

Member

stof commentedApr 4, 2016

IMO, this should be considered as a new feature rather than a bugfix

javiereguiluz reviewed

Apr 4, 2016

View reviewed changes

src/Symfony/Bundle/WebProfilerBundle/CSP/ContentSecurityPolicyHandler.php Outdated

		*
		* @return string The Content-Security-Policy header
		*/
		private function generateCSPHeader(array $directives)

Copy link

Member

javiereguiluzApr 4, 2016

Choose a reason for hiding this comment

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

Given that this method just reduces an array to a string, maybe we could use thearray_reduce() method.

Copy link

Member

stof commentedApr 4, 2016

thus, another reason to consider it a new feature is that any bundle extending the profiler will have to consider it too.

javiereguiluz reviewed

Apr 4, 2016

View reviewed changes

src/Symfony/Bundle/WebProfilerBundle/Controller/ProfilerController.php Outdated

		{
		$response =newResponse('',$code,$headers);

		$nonces =$this->cspHandler ?$this->cspHandler->getNonces($request,$response) : [];

Copy link

Member

javiereguiluzApr 4, 2016

Choose a reason for hiding this comment

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

Forbidden short array notation in this line

Copy link

Member

javiereguiluz commentedApr 4, 2016

I have a global concern about this code. As usually happens with acronyms, it's hard to name methods and variables. But right now there are different criteria:

updateCSPheaders()getCSPHeaders()$cspHandler;

I propose to treatCSP asCsp word, so:

updateCspHeaders()getCspHeaders()$cspHandler;

javiereguiluz added the Bug label

Apr 4, 2016

hhamon reviewed

Apr 4, 2016

View reviewed changes

src/Symfony/Bundle/WebProfilerBundle/Resources/config/profiler.xml

		</service>

		<serviceid="web_profiler.csp_handler"class="%web_profiler.csp_handler.class%">
		</service>

Copy link

Contributor

hhamonApr 4, 2016

Choose a reason for hiding this comment

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

Can be removed and replaced with a single<service/> monotag.

Copy link

Member

javiereguiluzApr 4, 2016

Choose a reason for hiding this comment

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

@hhamon I think they are called "empty tags" ... but I absolutely love the "monotag" word 😁

Copy link

Contributor

hhamonApr 4, 2016

Choose a reason for hiding this comment

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

I never knew how to call them and this is why I also end up calling them "monotag" :D

romainneutron force-pushed thefix-csp-webprofiler-2.8 branch 4 times, most recently fromf81a05c to455cbd5Compare

April 4, 2016 14:24

Copy link

ContributorAuthor

romainneutron commentedApr 4, 2016

@stof I proposed this as a bug fix because it prevents usage of Symfony in a content-security-policy context. This is fully BC with other usage (Silex and any bundle extending it)
Bundles extending this one should only care about this in case they are not CSP compatible (some of them are already compatible) and want to be

Copy link

ContributorAuthor

romainneutron commentedApr 4, 2016

For the record, I updated the PR according to your comments

romainneutron force-pushed thefix-csp-webprofiler-2.8 branch 7 times, most recently from8de3cdf toff1cccdCompare

April 6, 2016 14:44

stof reviewed

Apr 8, 2016

View reviewed changes

src/Symfony/Bundle/WebProfilerBundle/Controller/ProfilerController.php

		return$this->templateManager;
		}

		privatefunctionrenderWithCspNonces(Request$request,$template,$variables,$code =200,$headers =array('Content-Type' =>'text/html'))

Copy link

Member

stofApr 8, 2016

Choose a reason for hiding this comment

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

according to your talk at the SymfonyLive right now, wouldn't it be better to use signatures instead ?

Copy link

ContributorAuthor

romainneutronApr 16, 2016

Choose a reason for hiding this comment

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

Actually this PR provides CSP support for setups whereunsafe-inline is disabled indevelopment environment (because it only affects the WebProfilerBundle that should not be enabled in prod in any case)

So, even if it's recommended to use signature instead of nonce, it's a recommendation for production or staging. In development, it enables user to fully uses test with their CSP and uses the WebProfilerBundle.

So, no, it's not a problem :)

romainneutron force-pushed thefix-csp-webprofiler-2.8 branch fromff1cccd toe1b2404Compare

April 16, 2016 09:49

[WebProfilerBundle] Fix bundle usage in Content-Security-Policy context

0c61a7b

romainneutron force-pushed thefix-csp-webprofiler-2.8 branch frome1b2404 to0c61a7bCompare

April 16, 2016 10:01

Copy link

ContributorAuthor

romainneutron commentedApr 16, 2016•
edited
Loading

I just amended the PR according to Javier's comment aboutarray_reduce. It's now ready, feedback are welcome :)

Copy link

ContributorAuthor

romainneutron commentedApr 16, 2016

A side note argument about "new feature" vs "bug fix":

Considering this a "new feature" would disable the WebProfilerBundle to any user that uses Content Security Policy in their Symfony Application until 3.2. Moreover, it would mean that Symfony would not provide a LTS version compatible with Content Security Policy before the end of 2017 (3.4 if I'm not wrong).
I definitely think it's a bug fix because Content Security Policies are a standard supported by most of the modern browsers (http://caniuse.com/#feat=contentsecuritypolicy2) and used by many applications, GitHub included.
Last but not least, I consider this one of the key feature to prevent XSS, and it should not be unusable with Symfony.

romainneutron mentioned this pull request

Apr 16, 2016

[3.2][WebProfilerBundle] Fix bundle usage in Content-Security-Policy context without unsafe-inline#18568

Merged

Copy link

ContributorAuthor

romainneutron commentedApr 16, 2016

Replaced by#18568 that proposes this has a new feature in 3.2

romainneutron closed this

Apr 16, 2016

romainneutron deleted the fix-csp-webprofiler-2.8 branch

April 16, 2016 17:32

fabpot added a commit that referenced this pull request

Jun 9, 2016

feature#18568[3.2][WebProfilerBundle] Fix bundle usage in Content-S…

856c9f6

…ecurity-Policy context without unsafe-inline (romainneutron)This PR was merged into the 3.2-dev branch.Discussion----------[3.2][WebProfilerBundle] Fix bundle usage in Content-Security-Policy context without unsafe-inline| Q             | A| ------------- | ---| Branch?       | 3.2| Bug fix?      | yes| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#15397| License       | MIT| Doc PR        | N/AHello, this PR fixes the compatibility of the WebprofilerBundle in a context where Content-Security-Policy headers are could prevent `unsafe-inline` of `script-src` or `style-src` directives.This PR has been originally proposed in 2.8 in#18434Commits-------571a1f2 [WebProfilerBundle] Fix bundle usage in Content-Security-Policy context without unsafe-inline

Labels

Bug Status: Needs Review WebProfilerBundle

5 participants

Movatterモバイル変換

Uh oh!

[2.8][WebProfilerBundle] Fix bundle usage in Content-Security-Policy context#18434

[2.8][WebProfilerBundle] Fix bundle usage in Content-Security-Policy context#18434

Uh oh!

Conversation

romainneutron commentedApr 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commentedApr 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commentedApr 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javiereguiluz commentedApr 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romainneutron commentedApr 4, 2016

Uh oh!

romainneutron commentedApr 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romainneutron commentedApr 16, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

romainneutron commentedApr 16, 2016

Uh oh!

romainneutron commentedApr 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

romainneutron commentedApr 16, 2016•
edited
Loading