Skip to content

Fix stored XSS vulnerabilities in HTML helper methods and views#2567

Draft
Copilot wants to merge 7 commits into
mainfrom
copilot/xss-security-review
Draft

Fix stored XSS vulnerabilities in HTML helper methods and views#2567
Copilot wants to merge 7 commits into
mainfrom
copilot/xss-security-review

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 6, 2026

Several helpers and views were constructing HTML via unsafe string concatenation — marking admin/project-member-controlled content (attribute labels, titles, descriptions, error messages) as html_safe without escaping, enabling stored XSS.

Changes

app/helpers/bootstrap_helper.rb

  • folding_panel: call h(title) before appending the caret <span>h() is a no-op for already-safe strings, escapes plain strings
  • panel_title: init title_html as ''.html_safe (SafeBuffer) so << auto-escapes any non-safe input; remove terminal .html_safe call on plain String

app/helpers/extended_metadata_helper.rb

  • extended_metadata_attribute_description: replace raw string concat with content_tag so description is escaped
  • extended_metadata_form_field_for_attribute / render_extended_metadata_value: wrap attribute.label in h() before passing to folding_panel

app/helpers/samples_helper.rb

  • sample_attribute_display_title: escape attribute.title with h(), use SafeBuffer << for unit appending, use content_tag for the pid <small> span
  • linked_extended_metadata_form_field: escape attr.label with h() in HTML string
  • linked_extended_metadata_attribute_display: escape attr.title with h(); pass h(attr.label) to folding_panel

Views

  • _sample_error_messages.html.erb / _spreadsheet_errors.html.erb: remove .html_safe from error.full_message — attribute names derived from sample type definitions (project-member-controlled) were rendered unescaped
  • _table_view.html.erb: escape both attribute.title and error message e with h() in popover content string

Example

Before:

# extended_metadata_helper.rb
def extended_metadata_attribute_description(description)
  html = '<p class="help-block"><small>' + description + '</small></p>'
  html.html_safe
end

# samples_helper.rb — panel_title
title_html = ''
title_html << title
title_html.html_safe  # no escaping performed

After:

def extended_metadata_attribute_description(description)
  content_tag(:p, class: 'help-block') { content_tag(:small, description) }
end

# panel_title
title_html = ''.html_safe  # SafeBuffer: << escapes non-safe strings automatically
title_html << title

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens several Rails helpers and ERB views against stored XSS by ensuring user-/project-member-controlled strings (attribute labels/titles/descriptions and validation messages) are escaped before being inserted into HTML.

Changes:

  • Updated folding_panel / panel_title to avoid unsafe string concatenation patterns and ensure non-safe input is escaped.
  • Escaped extended metadata labels/descriptions via h(...) and content_tag(...) instead of raw concatenation.
  • Removed unsafe .html_safe usage from validation error rendering and escaped popover content in the samples table.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
app/helpers/bootstrap_helper.rb Escapes panel titles and uses SafeBuffer concatenation to prevent title-based XSS.
app/helpers/extended_metadata_helper.rb Uses escaping/content_tag for linked metadata labels and descriptions.
app/helpers/samples_helper.rb Escapes attribute titles/labels and avoids unsafe HTML building in sample attribute display.
app/views/spreadsheets/_spreadsheet_errors.html.erb Stops rendering error.full_message as raw HTML.
app/views/samples/_sample_error_messages.html.erb Stops rendering error.full_message as raw HTML.
app/views/samples/_table_view.html.erb Escapes attribute titles and per-field error text used in popovers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +32 to 33
title = h(title) unless title.nil?
title += " <span class=\"#{collapsed ? 'caret' : 'caret-up'}\"></span>".html_safe
Comment on lines 106 to +114
def sample_attribute_display_title(attribute)
title = attribute.title
title = h(attribute.title)
if (unit = attribute.unit) && !unit.dimensionless?
title += " ( #{unit} )"
title << ' ( ' << h(unit) << ' )'
end
unless attribute.pid.blank?
title += content_tag(:small, 'data-tooltip'=>attribute.pid) do
" [ "+attribute.short_pid+ " ]"
end.html_safe
title << content_tag(:small, " [ #{attribute.short_pid} ]", 'data-tooltip' => attribute.pid)
end
title.html_safe
title
…ute_display_title to verify script tags and image tags with event handlers are escaped
…itles for folding_panel in sample panels so counter <span> elements render correctly while the helper escapes any malicious HTML

                                     -
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants