Skip to content

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.

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

Merged
merged 1 commit into from
Jun 13, 2018
+5 −4
Merged

Avoid calling eval when there is no script embedded in the toolbar #27584

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,9 +419,10 @@
function(xhr, el) {

/* Evaluate in global scope scripts embedded inside the toolbar */
eval.call({}, ([].slice.call(el.querySelectorAll('script')).map(function (script) {
return script.firstChild.nodeValue;
}).join(';\n')));
var i, scripts = [].slice.call(el.querySelectorAll('script'));
for (i = 0; i < scripts.length; ++i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not define i inside the loop? for (var i..., that way you can be sure you won't get any scope bleeding.

Copy link
Member Author

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 using let, but not for var (which is scoped only by functions, not by loops)

Copy link
Contributor

Choose a reason for hiding this comment

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

I was confusing it with let indeed. In case of var this is indeed better 👍

eval.call({}, scripts[i].firstChild.nodeValue);
}

el.style.display = -1 !== xhr.responseText.indexOf('sf-toolbarreset') ? 'block' : 'none';

Expand All @@ -440,7 +441,7 @@
}

/* Handle toolbar-info position */
var i, toolbarBlocks = [].slice.call(el.querySelectorAll('.sf-toolbar-block'));
var toolbarBlocks = [].slice.call(el.querySelectorAll('.sf-toolbar-block'));
for (i = 0; i < toolbarBlocks.length; ++i) {
toolbarBlocks[i].onmouseover = function () {
var toolbarInfo = this.querySelectorAll('.sf-toolbar-info')[0];
Expand Down