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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Escape id parameter for queues view #1687
Conversation
544aefd
to
70f14b1
Compare
@@ -2,7 +2,7 @@ | |||
|
|||
<% if current_queue = params[:id] %> | |||
|
|||
<h1>Pending jobs on <span class='hl'><%= current_queue %></span></h1> | |||
<h1>Pending jobs on <span class='hl'><%= h current_queue %></span></h1> |
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.
h
is only available in rails and resque does not depend on rails. This should do the trick:
<h1>Pending jobs on <span class='hl'><%= h current_queue %></span></h1> | |
<h1>Pending jobs on <span class='hl'><%= ERB::Util.html_escape current_queue %></span></h1> |
@brianvans does this look good to you?
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.
h
is aliased as a helper over here:
I noticed it was used in a few other views, so it made sense to use it here as well.
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.
Ah, nice! Didn't realize that.
@brianvans what do you think of the suggested change? If you commit/accept, I can work on getting this merged. |
@chrisccerami Here's another one ready for merging! |
@brianvans can you rebase on master so we can get the tests to pass? |
70f14b1
to
efe7ba1
Compare
Done! |
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.
@chrisccerami could you merge this one in?
@brianvans @iloveitaly could we get a security advisory published for this? |
cc @corincerami just realised you're the one that did the merging |
Fixes #1679