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

Pass options to the text method. #35

Merged
merged 1 commit into from Mar 4, 2015
Merged

Conversation

kaspth
Copy link
Contributor

@kaspth kaspth commented Mar 4, 2015

Fixes #31.

This lets us avoid encoding special chars twice in Rails as strip_tags could be written as:

def strip_tags(html)
  self.class.full_sanitizer.sanitize(html, encode_special_chars: false) # let Rails handle escaping.
end

I didn't document this option as it's really for our internal use. But there's a potential security vector we have to take into account here.

Alternatively, I guess we could just call html_safe in strip_tags, but that makes me feel uneasy too.

cc @rafaelfranca

This lets us avoid encoding special chars twice in Rails.
@rafaelfranca
Copy link
Member

:shipit:

@kaspth
Copy link
Contributor Author

kaspth commented Mar 4, 2015

Great, once the other issue is sorted we can bump the version number and ship a new release. Then I'll submit a PR to Rails to use this 😉

kaspth added a commit that referenced this pull request Mar 4, 2015
@kaspth kaspth merged commit 1d0ba0c into master Mar 4, 2015
@kaspth kaspth deleted the pass-options-to-sanitize branch March 4, 2015 22:05
@kaspth
Copy link
Contributor Author

kaspth commented Mar 4, 2015

It'll make it into 4.2.2, right?

@rafaelfranca
Copy link
Member

Right

@kaspth
Copy link
Contributor Author

kaspth commented Mar 7, 2015

Submitted rails/rails#19252 to add the binding on Rails' side.

@mtarnovan
Copy link

@rafaelfranca Looks like 4.2.2 was release without this fix.

@kaspth
Copy link
Contributor Author

kaspth commented Jun 19, 2015

@mtarnovan 4.2.2 was a security release. Those should have the fewest number of changes to make them as easy as possible to upgrade to.
This'll in the next bug fix release for 4.2.

@mtarnovan
Copy link

@kaspth I see, thanks.

@optimum-dulopin
Copy link

i use rails 4.2.5.1 and there is still this bug

@geoffevason
Copy link

geoffevason commented Jan 10, 2017

@optimum-dulopin I don't think this is considered a bug anymore. Although #35 originally solved the issue, it was undone here 49dfc15

Basically:

Loofah.fragment("&").text(encode_special_chars: false) => "&"  # Yay - this is  the way it used to be!
Loofah.fragment("&lt;script&gt;").text(encode_special_chars: false) => "<script>" # uh-oh - this would let anyone inject so we can't use encode_special_chars

Perhaps the bug is Loofah's use of encode_special_chars: false to actually unencoded encoded strings - I'm still trying to figure that out myself...

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.

sanitize without encode special chars
5 participants