-
Notifications
You must be signed in to change notification settings - Fork 83
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
Migrate to SafeListSanitizer #87
Conversation
cba4464
to
911899f
Compare
I merged your other PRs. Since this is a more significant change it would be good to rebase. @rafaelfranca do you have thoughts on this? |
911899f
to
a4c4318
Compare
@JuanitoFatas do you want to take a stab at the test failures in another PR? 🙏 |
A fix is in #90. |
SafeList is easier to understand
a4c4318
to
41b0b49
Compare
Forgot to get back to this one, thanks! |
Is this (removing a method) a breaking change to the public API? If so, SemVer requires a major-version bump. |
def white_list_sanitizer | ||
Html::WhiteListSanitizer | ||
ActiveSupport::Deprecation.warn "warning: white_list_sanitizer is" \ | ||
"deprecated, please use safe_list_sanitizer instead." |
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.
This is missing a space, shows up as "isdeprecated"
in deprecation messages.
Update ruby-rails-html-sanitizer to 1.3.0. ## 1.3.0 * Address deprecations in Loofah 2.3.0. *Josh Goodall* ## 1.2.0 * Remove needless `white_list_sanitizer` deprecation. By deprecating this, we were forcing Rails 5.2 to be updated or spew deprecations that users could do nothing about. That's pointless and I'm sorry for adding that! Now there's no deprecation warning and Rails 5.2 works out of the box, while Rails 6 can use the updated naming. *Kasper Timm Hansen* ## 1.1.0 * Add `safe_list_sanitizer` and deprecate `white_list_sanitizer` to be removed in 1.2.0. rails/rails-html-sanitizer#87 *Juanito Fatas* * Remove `href` from LinkScrubber's `tags` as it's not an element. rails/rails-html-sanitizer#92 *Juanito Fatas* * Explain that we don't need to bump Loofah here if there's CVEs. rails/rails-html-sanitizer@d4d823c *Kasper Timm Hansen*
Safelist is easier to understand and less offensive. Chose
SafeList
because a similar ongoing effort in loofah gem: flavorjones/loofah#158.View Updated README.md
How to run the test
Note
An attempt to fix the build in #90.
Original motivation rails/rails#33677, other community efforts examples