bootstrap_form icon indicating copy to clipboard operation
bootstrap_form copied to clipboard

`join` May Cause Unwanted HTML Escapes

Open lcreid opened this issue 3 years ago • 4 comments
trafficstars

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?

lcreid avatar Oct 01 '22 21:10 lcreid

@lcreid Any more concrete example for this?

donv avatar Sep 12 '23 09:09 donv

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.

lcreid avatar Sep 14 '23 02:09 lcreid

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

donv avatar Sep 14 '23 05:09 donv

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 avatar Sep 22 '23 00:09 lcreid

Fixed by #704

lcreid avatar Jun 09 '24 21:06 lcreid