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

false positive AND negative for XSS warning when using sanitize #1187

Closed
michaelglass opened this issue Apr 26, 2018 · 4 comments · Fixed by #1192
Closed

false positive AND negative for XSS warning when using sanitize #1187

michaelglass opened this issue Apr 26, 2018 · 4 comments · Fixed by #1192

Comments

@michaelglass
Copy link

michaelglass commented Apr 26, 2018

Background

Brakeman version: 4.2.1
Rails version: 4.2.10
Ruby version: 2.3.4

Issue

correctly using sanitize produces a warning

= sanitize link_to(image_tag("icons/twitter-gray.svg"), team_member.twitter_url, target: "_blank")

Full Brakeman Warning:

Confidence: Weak
Category: Cross-Site Scripting
Check: LinkToHref
Message: Potentially unsafe model attribute in link_to href
Code: link_to(image_tag("icons/twitter-gray.svg"), TeamMember.find(params[:id]).twitter_url, :target => "_blank")
File: app/views/home/_team_member.html.haml
Line: 13

but incorrectly using it does not, despite the fact that this is unsafe

= link_to(image_tag("icons/twitter-gray.svg"), sanitize(team_member.twitter_url), target: "_blank")
@presidentbeef
Copy link
Owner

The second issue is resolved with #1192.

The first issue is a bit more difficult, still thinking about how to solve it. At first I was going to ignore it because it seemed nonsensical but after testing I see it does make the link safe...

@michaelglass
Copy link
Author

@presidentbeef so actually, sanitize is only partially safe... it protects against javascript: but not data: (at least as of rails 4.2)

@michaelglass
Copy link
Author

(also: thank you for your wonderful gem!)

@presidentbeef
Copy link
Owner

Confirmed in Rails 5.2 that you can still get a data: XSS payload in there.

Repository owner locked and limited conversation to collaborators Jul 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants