bootstrap-ruby / bootstrap_form

Official repository of the bootstrap_form gem, a Rails form builder that makes it super easy to create beautiful-looking forms using Bootstrap 5.
MIT License
1.64k stars 351 forks source link

`join` May Cause Unwanted HTML Escapes #661

Closed lcreid closed 3 weeks ago

lcreid commented 1 year ago

Array#join always produces a String, so some uses of join in our code may be causing HTML-safe strings (e.g. error messages) to become "unsafe" and then they'll get escaped.

One place to investigate: https://github.com/bootstrap-ruby/bootstrap_form/blob/57a5be70f553d9ebf084fa39fa718e5dafeb38d3/lib/bootstrap_form/components/validation.rb#L74

May be related to #653?

donv commented 9 months ago

@lcreid Any more concrete example for this?

lcreid commented 9 months ago

If I search for join in the code excluding test/, and eliminating the instances that are joining file paths, I find these:

lib/bootstrap_form/form_builder.rb:
  69          ([*options[:html][:class]&.split(/\s+/)] + %w[row row-cols-auto g-3 align-items-center])
  70:         .compact.uniq.join(" ")
  71      end

lib/bootstrap_form/form_group_builder.rb:
  93        control_classes = css_options.delete(:control_class) { control_class }
  94:       css_options[:class] = [control_classes, css_options[:class]].compact.join(" ")
  95        css_options[:class] << " is-invalid" if error?(method)

lib/bootstrap_form/components/validation.rb:
  83  
  84:         object.errors[name].join(", ")
  85        end

lib/bootstrap_form/helpers/bootstrap.rb:
  34            if hide_attribute_name
  35:             object.errors[name].join(", ")
  36            else
  37:             object.errors.full_messages_for(name).join(", ")
  38            end

  95          end
  96:         ActiveSupport::SafeBuffer.new(tags.join)
  97        end

lib/bootstrap_form/inputs/rich_text_area.rb:
  12              prepend_and_append_input(name, options) do
  13:               options[:class] = ["trix-content", options[:class]].compact.join(" ")
  14                rich_text_area_without_bootstrap(name, options)

The code has been like this for a long time and we have no complaints. And if it's "broken", I think it's broken in the safe direction, meaning it will err on the side of escaping HTML. It's just something that I've always wanted to look at. Maybe when I retire, if I can ever afford to. Ha ha.

donv commented 9 months ago

I propose we either define what needs to change or close this issue and #653 .

lcreid commented 9 months ago

I took a run at this last week when my COVID wasn't so bad and ran into one case where the right solution isn't obvious. Still working on it.

lcreid commented 3 weeks ago

Fixed by #704