Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Avoid calling eval when there is no script embedded in the toolbar#27584
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| returnscript.firstChild.nodeValue; | ||
| }).join(';\n'))); | ||
| var i, scripts= [].slice.call(el.querySelectorAll('script')); | ||
| for (i=0; i<scripts.length;++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why not definei inside the loop?for (var i..., that way you can be sure you won't get any scope bleeding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Because this variable is not only used by this loop but also by next ones. Putting it inside the loop might make us remove the variable declaration by mistake in a refactoring, which would make us use a global variable here.
that way you can be sure you won't get any scope bleeding.
That's wrong. It would be true when usinglet, but not forvar (which is scoped only by functions, not by loops)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I was confusing it withlet indeed. In case ofvar this is indeed better 👍
fabpot commentedJun 13, 2018
Thank you@stof. |
… toolbar (stof)This PR was merged into the 4.1 branch.Discussion----------Avoid calling eval when there is no script embedded in the toolbar| Q | A| ------------- | ---| Branch? | 4.1| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#27583| License | MIT| Doc PR | n/a#27189 changed the way embedded scripts were eval'd for the toolbar. But it also refactored the code in a way triggering `eval` all the time, even when there is no embedded script, which was reported several times as an issue with CSP.While the debug panel (showing dumps) still requires having `unsafe-eval` in the CSP header (due to embedding scripts that we eval), this PR reverts back to the behavior of Symfony 4.0 and older, where only toolbars actually embedding scripts have this CSP compat issue.Commits-------a0f78a5 Avoid calling eval when there is no script embedded in the toolbar
ogizanagi commentedJun 13, 2018
I'm late but thank you taking care of this@stof 👍 |
#27189 changed the way embedded scripts were eval'd for the toolbar. But it also refactored the code in a way triggering
evalall the time, even when there is no embedded script, which was reported several times as an issue with CSP.While the debug panel (showing dumps) still requires having
unsafe-evalin the CSP header (due to embedding scripts that we eval), this PR reverts back to the behavior of Symfony 4.0 and older, where only toolbars actually embedding scripts have this CSP compat issue.