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

railties: configure sanitizer vendor in 7.1 defaults more robustly #51267

Merged

Conversation

flavorjones
Copy link
Member

@flavorjones flavorjones commented Mar 6, 2024

Motivation / Background

rails-html-santizer is a dependency of Action View and a transitive dependency of Action Text (via Action Pack), but may not be loaded until after railties sets configuration defaults, meaning that the sanitizer vendor may remain Rails::HTML4 and not be set to Rails::HTML5 as we intend in Rails 7.1.

This change requires rails-html-sanitizer immediately before it's needed, and avoids the possibly-incorrect assumption that Rails::HTML::Sanitizer is already defined.

Closes #51246

If merged, this should be backported to 7-1-stable.

Additional information

Because the buggy behavior is dependent on the order in which libraries are loaded, it's difficult to add to the test suite.

I tested manually using the repro from #51246:

#! /usr/bin/env ruby

require "bundler/inline"

gemfile do
  source "https://rubygems.org"
  gem "rails", path: ".."
end

require "action_controller/railtie"
require "action_view/railtie"
require "minitest/autorun"

class TestApp < Rails::Application
  config.load_defaults 7.1
  config.eager_load = true
end

TestApp.initialize!

class BugTest < Minitest::Test
  def test_sanitizer_vendor
    assert_equal ActionView::Helpers::SanitizeHelper.sanitizer_vendor, Rails::HTML5::Sanitizer
  end
end

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the railties label Mar 6, 2024
rails-html-santizer is a dependency of Action View and a transitive
dependency of Action Text (via Action Pack), but may not be loaded
until after railties sets configuration defaults.

This change `require`s rails-html-sanitizer immediately before it's
needed, and avoids the possibly-incorrect assumption that
Rails::HTML::Sanitizer is already defined.

Closes rails#51246

Co-authored-by: Rafael Mendonça França <rafael@rubyonrails.org>
@flavorjones
Copy link
Member Author

CI failure seems to be a flake and not related to this PR.

@flavorjones flavorjones changed the title Fix sanitizer vendor config with 7.1 defaults railties: configure sanitizer vendor in 7.1 defaults more robustly Mar 6, 2024
@skipkayhil
Copy link
Member

CI failure seems to be a flake and not related to this PR.

I went ahead and retried it, all green now ✅

@eileencodes eileencodes merged commit 029d31c into rails:main Mar 8, 2024
4 checks passed
eileencodes added a commit that referenced this pull request Mar 8, 2024
Note: this is a manual backport of #51267 as it did not apply cleanly.

rails-html-santizer is a dependency of Action View and a transitive
dependency of Action Text (via Action Pack), but may not be loaded
until after railties sets configuration defaults.

This change `require`s rails-html-sanitizer immediately before it's
needed, and avoids the possibly-incorrect assumption that
Rails::HTML::Sanitizer is already defined.

Closes #51246

Co-authored-by: Rafael Mendonça França <rafael@rubyonrails.org>
Co-authored-by: Mike Dalessio <mike.dalessio@gmail.com>
@eileencodes
Copy link
Member

I manually backported this in 5305d2d because it didn't apply cleanly.

@flavorjones flavorjones deleted the flavorjones-51246-rails-html-sanitizer branch March 8, 2024 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants