[2.7][WebProfilerBundle] Fix CORS ajax security issues #18413

romainneutron · Apr 2, 2016

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

The WebProfiler toolbar monitors ajax requests. However, when using cross domain ajax requests, it triggers a security issues Refused to get unsafe header "X-Debug-Token" Refused to get unsafe header "X-Debug-Token-Link" because if the other app is not a Symfony App configured to expose these headers in CORS.

This fixes the issue. It adds a new configuration node to explicitly activate it on purpose.

fabpot · Apr 2, 2016

src/Symfony/Bundle/WebProfilerBundle/DependencyInjection/Configuration.php

@@ -45,6 +45,7 @@ public function getConfigTreeBuilder()
                    ->end()
                ->end()
                ->booleanNode('intercept_redirects')->defaultFalse()->end()
+                ->booleanNode('enable_cors')->defaultFalse()->end()


Any downside to always enable it?

No downside, even if you're doing cross domain calls without exposing the related headers. However, you'll have security logs in your console.

This should be enabled if you're doing cross domain calls on Symfony apps that enabled cors and have been configured to expose these headers through CORS. Moreover, even if you do that, I'm not sure you would be able to display profiles (to be tested)

Oh, I just realized I read your comment to fast :) Of course, it's better to enable it by default, I'm gonna update this

romainneutron · Apr 2, 2016

PR updated, comments addressed

kroshp · Apr 6, 2016

Same issue in 2.8

xabbuh · Apr 6, 2016

@kroshp All fixes will be merged up to all maintained branches after being merged into the lowest affected branch.

stof · Apr 7, 2016

src/Symfony/Bundle/WebProfilerBundle/EventListener/WebDebugToolbarListener.php

@@ -40,8 +40,9 @@ class WebDebugToolbarListener implements EventSubscriberInterface
    protected $mode;
    protected $position;
    protected $excludedAjaxPaths;
+    protected $enableCors;


should be private (even if older properties are not for legacy reasons)

stof · Apr 7, 2016

is there a way in JS to check whether a header can be accessed according to the CORS rules, to be able to fallback ? It would be great to avoid this configuration setting entirely, and just remove it from the display (or display it in a special way) when the token cannot be retrieved due to CORS restrictions

stof · Apr 7, 2016

btw, a try/catch around the statement reading the header might be an acceptable way to implement the fallback if there is no better one.

romainneutron · Apr 14, 2016

I just tried to detect if we had access to these headers in CORS using Access-Control-Expose-Headers, but a security warning is still issued when accessing this header.

Please note that these are not exception but Browser security warnings. You can try/catch these warnings but they still appear in the console.

I'm not in favor of adding a new configuration, so this PR is stalled while we found something better :/

romainneutron · Apr 16, 2016

I'm a bit stuck with this here...
If anybody's got an idea on how to solve this problem without a config option, help is welcome :)

stof · Apr 20, 2016

@romainneutron have to tried to ask on SO to get some help from the JS community ?

…equest occured from another domain (romainneutron) This PR was merged into the 2.7 branch. Discussion ---------- [2.7][WebProfilerBunde] Give an absolute url in case the request occured from another domain | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a This is related to #18413: Using two symfony application ProjectA and MicroServiceB, if ProjectA issues XHR to MicroServiceB and both have the WebProfilerBundle enabled, then Ajax requests will be monitored in ProjectA. But - at the moment - it will try to look for profiles under ProjectA domain whereas data are stored on MicroServiceB domain. This PR fixes this issue Commits ------- 1a5d4ad [WebProfilerBunde] Give an absolute url in case the request occured from another domain

weaverryan · May 5, 2016

Could why just avoid checking the headers entirely (and therefore showing the profiler link) when the AJAX request is for another domain? As nice as it is to show a profiler link that's coming from another Symfony app on some other domain, I'm not sure it's necessary - I'd expect the profiler link only for AJAX requests that are coming from this application.

romainneutron · May 12, 2016

I like @weaverryan 's idea, WDYT?

xabbuh · May 13, 2016

@weaverryan But that would mean that AJAX requests to the same application but with another domain also wouldn't be displayed, right? If so this is a BC break to me as someone might already rely on it (you could serve your internal API with a different domain for whatever reason for example).

stof · May 13, 2016

@weaverryan this would not be very convenient if you have an app hitting your API hosted on a different domain.
This is why we should first try to get input from the community of JS devs/browsers to know whether there is a way to know that we are allowed (or not allowed) to read a header. But @romainneutron has not replied to my question above

stof · May 13, 2016

@romainneutron can you try using getAllResponseHeaders() which will give you all headers ? AFAIU, this should always be allowed by CORS (but giving you only the headers you are allowed to see). You could then check in this object whether there is the profiler header

stof · May 13, 2016

@romainneutron I confirm you that xhr.getAllResponseHeaders() does not trigger a security warning in chromium

stof · May 13, 2016

though it returns a string representation of HTTP headers, so you will need to extract the header from it.

stof · May 13, 2016

or simpler: use the regex only to know whether the Symfony headers are exposed, and then use getResponseHeader to get their value when exposed (avoiding to parse headers manually)

romainneutron · May 14, 2016

@stof thanks for the hint! I'll try this asap

romainneutron · May 16, 2016

PR has been updated, ready for review

aitboudad · May 16, 2016

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

+                if (ret = allHeaders.match(/^x-debug-token-link:\s+(.*)$/im)) {
+                    stackElement.profilerUrl = ret[1];
+                }
+            }


missing comma

good catch, fixed

nicolas-grekas · May 17, 2016

👍

fabpot · May 17, 2016

Thank you @romainneutron.

…mainneutron) This PR was merged into the 2.7 branch. Discussion ---------- [2.7][WebProfilerBundle] Fix CORS ajax security issues | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A The WebProfiler toolbar monitors ajax requests. However, when using cross domain ajax requests, it triggers a security issues `Refused to get unsafe header "X-Debug-Token"` `Refused to get unsafe header "X-Debug-Token-Link"` because if the other app is not a Symfony App configured to expose these headers in CORS. ![image](https://cloud.githubusercontent.com/assets/137574/14225799/f462c09c-f8cf-11e5-925d-88be99945a92.png) This fixes the issue. It adds a new configuration node to explicitly activate it on purpose. Commits ------- f8dd87d [WebProfilerBundle] Fix CORS ajax security issues

This PR was merged into the 2.7 branch. Discussion ---------- Fix js comment in profiler | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Single line comment introduced in #18413 causes the toolbar to fail to load with a syntax error. Commits ------- 91a2f5d Fix js comment in profiler

romainneutron added WebProfilerBundle Status: Needs Review labels Apr 2, 2016

fabpot reviewed Apr 2, 2016
View reviewed changes

javiereguiluz added the Bug label Apr 2, 2016

romainneutron force-pushed the web-developer-fix-cors-profiling branch from cd33ed0 to cc11f26 Compare April 2, 2016 23:33

stof reviewed Apr 7, 2016
View reviewed changes

romainneutron mentioned this pull request Apr 16, 2016

[2.7][WebProfilerBunde] Give an absolute url in case the request occured from another domain #18562

Merged

romainneutron force-pushed the web-developer-fix-cors-profiling branch from cc11f26 to 9b67024 Compare April 16, 2016 10:59

romainneutron force-pushed the web-developer-fix-cors-profiling branch 2 times, most recently from 7b83000 to 20a26ee Compare May 16, 2016 14:53

aitboudad reviewed May 16, 2016
View reviewed changes

romainneutron force-pushed the web-developer-fix-cors-profiling branch from 20a26ee to cccc420 Compare May 16, 2016 15:16

[WebProfilerBundle] Fix CORS ajax security issues

f8dd87d

romainneutron force-pushed the web-developer-fix-cors-profiling branch from cccc420 to f8dd87d Compare May 16, 2016 16:19

fabpot merged commit f8dd87d into symfony:2.7 May 17, 2016

romainneutron deleted the web-developer-fix-cors-profiling branch May 17, 2016 21:30

linnaea mentioned this pull request May 26, 2016

Fix js comment in profiler #18883

Merged

fabpot mentioned this pull request May 26, 2016

Release v3.1.0-RC1 #18885

Merged

This was referenced Jun 6, 2016

Release v2.7.14 #18981

Merged

Release v2.8.7 #18982

Merged

Release v3.0.7 #18983

Merged

Search code, repositories, users, issues, pull requests...

Uh oh!

[2.7][WebProfilerBundle] Fix CORS ajax security issues #18413

[2.7][WebProfilerBundle] Fix CORS ajax security issues #18413

Uh oh!

Conversation

romainneutron commented Apr 2, 2016

Uh oh!

fabpot Apr 2, 2016

Choose a reason for hiding this comment

Uh oh!

romainneutron Apr 2, 2016

Choose a reason for hiding this comment

Uh oh!

romainneutron Apr 2, 2016

Choose a reason for hiding this comment

Uh oh!

romainneutron commented Apr 2, 2016

Uh oh!

kroshp commented Apr 6, 2016

Uh oh!

xabbuh commented Apr 6, 2016

Uh oh!

stof Apr 7, 2016

Choose a reason for hiding this comment

Uh oh!

stof commented Apr 7, 2016

Uh oh!

stof commented Apr 7, 2016

Uh oh!

romainneutron commented Apr 14, 2016

Uh oh!

romainneutron commented Apr 16, 2016

Uh oh!

stof commented Apr 20, 2016

Uh oh!

weaverryan commented May 5, 2016

Uh oh!

romainneutron commented May 12, 2016

Uh oh!

xabbuh commented May 13, 2016

Uh oh!

stof commented May 13, 2016

Uh oh!

stof commented May 13, 2016

Uh oh!

stof commented May 13, 2016

Uh oh!

stof commented May 13, 2016

Uh oh!

stof commented May 13, 2016

Uh oh!

romainneutron commented May 14, 2016

Uh oh!

romainneutron commented May 16, 2016

Uh oh!

aitboudad May 16, 2016

Choose a reason for hiding this comment

Uh oh!

romainneutron May 16, 2016

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented May 17, 2016

Uh oh!

fabpot commented May 17, 2016

Uh oh!

Uh oh!