use rails-html-sanitizer >= 1.0.3

[marutosi]

CVE-2015-7579 says rails-html-sanitizer 1.0.2 has XSS vulnerability.

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @sgrif (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

[amatsuda]

Thanks!

[marutosi]

Could you cherry-pick to Rails 4.2?

[amatsuda]

Yep. Just cherry-picked to 5-0-stable and 4-2-stable.

[marutosi]

Thanks, I can see 5ca3796 on 4.2.

[rafaelfranca]

There is no reason to merge this pull request. We should not need to change the gemspec every time one dependency has a security release. This is what gemfiles are for. Could you revert?

[marutosi]

It is too ugly to keep 'gem "rails-html-sanitizer", ">= 1.0.3"' on Rails app for long time.

http://www.redmine.org/projects/redmine/repository/entry/tags/3.3.2/Gemfile#L23

# Request at least rails-html-sanitizer 1.0.3 because of security advisories 
gem "rails-html-sanitizer", ">= 1.0.3"

[rafaelfranca]

You don't need to keep it. Once you commit the lock just make sure you don't downgrade. If you want to enforce the right place is in the Gemfile. Definitely bumping dependencies in the gemspec every single time a security release happens is not the correct way to do this.

…

On Fri, Jan 27, 2017 at 8:36 AM Toshi MARUYAMA ***@***.***> wrote: It is too ugly to keep 'gem "rails-html-sanitizer", ">= 1.0.3"' on Rails app for long time. http://www.redmine.org/projects/redmine/repository/entry/tags/3.3.2/Gemfile#L23 # Request at least rails-html-sanitizer 1.0.3 because of security advisories gem "rails-html-sanitizer", ">= 1.0.3" — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#27814 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC66CmoVrXMYiYYWzU6BXDepBJPjwhuks5rWfL2gaJpZM4Lvbne> .

[marutosi]

Redmine does not bundle Gemfile.lock because plugins use own Gemfile and share Gemfile.lock.

[rafaelfranca]

Unfortunately there is nothing we can do. This is a concern of the application. Rails should not bump their dependencies when security releases are made.

…

On Fri, Jan 27, 2017 at 11:44 AM Toshi MARUYAMA ***@***.***> wrote: Redmine does not bundle Gemfile.lock because plugins use own Gemfile and share Gemfile.lock. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#27814 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC66PAJpAKHsjxVMI1bj9RJcwF_7WnLks5rWh78gaJpZM4Lvbne> .

[meineerde]

I'm on board with trying to reject the notion that any security fix in any of the dependencies of rails requires a bump of the gemspec. The expectation that this would always happen for any update of a dependency would probably put too much of a burden on Rails and the team and would probably just lead to a large number of dependency-only bumps, e.g. related to nokogiri with it's quite regular (security) updates.

However, rails-html-sanitizer is a strict dependency of Rails and it is maintained by the same team as Rails itself, following the same standards. From a user's perspective, rails-html-sanitizer is thus a part of Rails in exactly the same way ActionView itself is. The original issue which was the reason for the update from 1.0.2 to 1.0.3 warranted a post to rubyonrails-security and a coordinated release of the new gem version, further fostering this view.

Now, if there was an XSS issue in ActionView, its version (and by extension, Rails) would be bumped, no questions asked. Since rails-html-sanitizer is maintained independently from Rails, this lockstep-bump is not strictly necessary. However, given that

1. rails-html-sanitizer is part of Rails proper (although maintained in a different repo)
2. the changes between 1.0.2 and 1.0.3 are quite minimal, addressing only the security issue and some documentation

it should be handled the same way a security issue in this repository would be handled. In fact, it mostly was with the announcement to rubyonrails-security and coordinated release of version 1.0.3 of the gem by the Rails team. Now arguing to maintain lax dependencies on an insecure version of a core-gem feels rather weird.

Concluding, I'm strongly in favor of keeping the bump and releasing it as part of the next normal release. In the end, Rails is still omakase and it should try to provide secure defaults, the same way it tries to get out of developer's ways in all other areas.

[rafaelfranca]

The point is, you still get the newest rails-html-sanitizer when you install rails so it is still secure by default. The only point for bumping this dependency in the gemspec is to not allow applications to install old versions but it is the job of the Gemfile not the gemspec to upgrade application dependencies. I'm reverting this change.

[rafaelfranca]

Well, now that it is merged it is better to keep as it is unless it cause problems. But let's try to not merge this kind of change again.

[meineerde]

Thanks. I fully understand your point of view to not needlessly restrict dependencies. I still think it's a good idea to bump it in this case but I can see why you would generally try to avoid it if its not strictly necessary.

Thanks for your consideration and your continued work to keep Rails great. <3

[amatsuda]

@rafael I didn't think this commit is that bad.

As very well described by @meineerde, rails-html-sanitizer is not someone else's software but we are maintaining the gem. Bumping the version in AV gemspec would be a handy way to deliver our own message for the users to be sure to avoid bundling vulnerable gem.

I do agree that rails is depending on so many gems and we're not able to take care of every dependent gem's security problems in our side. But in case a fix like this with a certain evidence was suggested by someone, the only thing we need to do is to click the green button. It's mentally easier than rejecting the PR.

But now I know that you hate seeing a change in gemspecs, I'm not gonna merge a PR like this again.

[matthewd]

Concluding, I'm strongly in favor of keeping the bump and releasing it as part of the next normal release.

That's not "the same way a security issue in this repository would be handled", though: if we believe it's necessary to change actionview to force people to use the newer version of rails-html-sanitizer, because they're [for whatever reason] incapable of upgrading it directly in response to the security advisory, then AFAICS we are obliged to issue a full set of Rails security releases, which change nothing but that dependency.

[pixeltrix]

Argh - the changes to the Gemfile.lock weren't merged as well so now I'm not sure whether to commit them or revert this 😄