Skip to content
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 filename to avoid XSS from malicious input #117

Merged
merged 1 commit into from
Mar 23, 2020
Merged

Escape filename to avoid XSS from malicious input #117

merged 1 commit into from
Mar 23, 2020

Conversation

pbyrne
Copy link
Contributor

@pbyrne pbyrne commented Mar 3, 2020

Because:

  • If user input is provided for the file name (as in rendering an SVG based on a URL parameter), the blanket marking of the SVG output as HTML-safe exposes an app to an XSS attack in the comment listing the file that was not found.
  • This happened in our app, which we resolved by adding a whitelist to check if the SVG file existed before calling inline_svg, but it feels safer to have the gem handle this better from the get-go.

Solution:

  • HTML-escape the filename rendering the comment that it was not found.
  • I considered some alternate solutions like using content_tag to render the SVG rather than a raw string, but this felt like a smaller change.

Because:

* If user input is provided for the file name (as in rendering an SVG
  based on a URL parameter), the blanket marking of the SVG output as
  HTML-safe exposes an app to an XSS attack in the comment listing the
  file that was not found.

Solution:

* HTML-escape the filename rendering the comment that it was not found.
Copy link
Owner

@jamesmartin jamesmartin left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR @pbyrne. The change looks good to me. 👍

@pbyrne
Copy link
Contributor Author

pbyrne commented Mar 19, 2020

Thanks for the 👍, @jamesmartin! Wondering when this could be merged and a new release pushed.

@jamesmartin jamesmartin merged commit 7c303a0 into jamesmartin:master Mar 23, 2020
@jamesmartin
Copy link
Owner

jamesmartin commented Mar 23, 2020

Sorry it the delay, @pbyrne. I've merged your PR and will make a new release later this week.

@jamesmartin
Copy link
Owner

Actually, it looks like your change isn't backwards compatible with Rails 3:

ActionView::Template::Error: undefined method `html_escape_once' for ERB::Util:Module
89
  Did you mean?  html_escape

We'll probably need a conditional to check for that. The good news is that we'll be dropping Rails 3 support in Version 2 of this gem, but for now we've got to support it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants