Skip to content

[WebProfilerBundle] [VarDumper] Inject "unsafe-eval" into the CSP if the VarDumper was used #29155

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[WebProfilerBundle] [VarDumper] Inject "unsafe-eval" into the CSP if the VarDumper was used #29155

wants to merge 1 commit into from

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Nov 9, 2018 •
edited
Loading

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #29084
License MIT
Doc PR -

Adapts the VarDumper so that it accepts a callable which is called after something has been dumped. This can be used by the ContentSecurityPolicyHandler in the WebProfilerBundle to modify CSPs to allow unsafe-eval which is needed for the profiler toolbar to evaluate injected JS.
This is kind of a small feature/extension to #18568. I ommited the changelog and doc changes because of that but they can be added of course if needed.

@herndlm
Copy link
Contributor Author

herndlm commented Nov 9, 2018

Travis seems to be failing because it is not using my modified VarDumper. Should this be separated?

@nicolas-grekas nicolas-grekas added this to the next milestone Nov 9, 2018
@herndlm
Copy link
Contributor Author

herndlm commented Dec 3, 2018

Hello, what do you think of this change? @nicolas-grekas maybe? :)

@stof
Copy link
Member

stof commented Dec 4, 2018

The static property which is never reset will not play well with cases like php-pm, which reuse the same process for multiple HTTP requests (dumping once will authorize unsafe-eval for all subsequent requests in that case with this PR)

@herndlm
Copy link
Contributor Author

herndlm commented Dec 4, 2018

Interesting, I have apparently not thought about that. I am not familiar with php-pm, do you have any suggestions to avoid that problem or when to clear the property?

@herndlm
Copy link
Contributor Author

herndlm commented Dec 4, 2018 •
edited
Loading

I have pushed a new solution which seems cleaner to me and I guess it should handle the php-pm problems better. Basically I extended the VarDumper so that it can call functions after dumping and use it to enable unsafe-eval in the CSP. This is set up in a request listener in WebDebugToolbarListener. I'm waiting for your input :)

@herndlm
Copy link
Contributor Author

herndlm commented Dec 6, 2018

Adapted a few small things and added more CSP edge cases. @stof What do you think?

@althaus
Copy link
Contributor

althaus commented Jan 14, 2019

Any news on this? The missing rule makes our dumps unreadable in the toolbar as the missing CSS evaluation results in black-on-black rendering in our project. :(

@stof
Copy link
Member

stof commented Jan 14, 2019

this PR conflicts with the current state of the branch.

@dschoschu dschoschu mentioned this pull request Jan 17, 2019
Closed
@herndlm
Copy link
Contributor Author

herndlm commented Jan 20, 2019

@stof PR is adapted

@herndlm
Copy link
Contributor Author

herndlm commented Feb 2, 2019

Friendly request for a review :) What do you think?

@herndlm
Copy link
Contributor Author

herndlm commented Feb 26, 2019

@nicolas-grekas @stof this is rebased on the current master, everything still applies, all tests are working and looking good. What do you think?

fabpot
fabpot requested changes Mar 4, 2019
View reviewed changes
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

Looks good to me feature-wise. About the new $afterDumpHandler property, it should be reset somehow after being used; if not, it will persist for any other requests managed by the same PHP process.

@herndlm
Copy link
Contributor Author

herndlm commented Mar 15, 2019

Looks good to me feature-wise. About the new $afterDumpHandler property, it should be reset somehow after being used; if not, it will persist for any other requests managed by the same PHP process.

Hi, thank you for the review and input! I pushed a new version which is using the handler only once and resetting it afterwards which is also covered by a test.

@Simperfit
Copy link
Contributor

@monojp could you please rebase with master ?
@fabpot @stof I think this is ready to be merged, CI failures seems not related.

@herndlm
Copy link
Contributor Author

herndlm commented Apr 10, 2019

@Simperfit done

@herndlm
Copy link
Contributor Author

herndlm commented Nov 23, 2019

Did the requested changes by resetting evaluationEnabled at the end when updating the headers and adapted the tests for that, rebased on master as well. AppVeyor changes seem to be unrelated? (table from other components already existing in sqlite)

@herndlm herndlm requested a review from fabpot November 23, 2019 19:56
@herndlm herndlm requested a review from stof November 23, 2019 19:57
nicolas-grekas
nicolas-grekas reviewed Feb 4, 2020
View reviewed changes
src/Symfony/Component/VarDumper/VarDumper.php
@@ -57,4 +65,12 @@ public static function setHandler(callable $callable = null)

return $prevHandler;
}

public static function setAfterDumpHandlerOnce(callable $callable = null)
Copy link
Member

Choose a reason for hiding this comment

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

this looks like something we'd better avoid IMHO.
I think it's fine if we call setEvaluationEnabled(true) all the time when dump() is called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for getting back in touch so late. I'm aware this a not-important edge-case and I actually needed this at my old company and currently I don't. Anyways but I still want to finish this :)

Do you mean to adapt this callback to be called every-time instead of that only once logic which simplifies it or something completely different?

@nicolas-grekas
Copy link
Member

I'm closing because the approach here is too invasive IMHO as it relies on global state.
Instead, I'm proposing a few alternatives in #29084 (comment) if you're up to give it a try.

@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@herndlm herndlm mentioned this pull request Mar 10, 2021
Merged
fabpot added a commit that referenced this pull request Mar 12, 2021
@fabpot
feature #40441 [WebProfilerBundle] Disable CSP if dumper was used (mo…
bbb4d9f 
…nojp)

This PR was squashed before being merged into the 5.3-dev branch.

Discussion
----------

[WebProfilerBundle] Disable CSP if dumper was used

| Q             | A
| ------------- | ---
| Branch?       | 5.x for features
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #29084
| License       | MIT
| Doc PR        | -

Disables a configured Content Security Policy if the dumper was used to avoid breaking the toolbar. This is a less invasive alternative to #29155 which fixes #29084 (there is a project with a test case linked there).

Commits
-------

5dc2637 [WebProfilerBundle] Disable CSP if dumper was used
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@ro0NL ro0NL ro0NL left review comments

@Simperfit Simperfit Simperfit approved these changes

@fabpot fabpot Awaiting requested review from fabpot

@stof stof Awaiting requested review from stof

Assignees
No one assigned
Labels
Feature Status: Needs Review VarDumper WebProfilerBundle
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
8 participants
@herndlm @stof @althaus @Simperfit @nicolas-grekas @fabpot @ro0NL @carsonbot