Skip to content

Escape attr.label and attr.value in AppHelper#recent_settings#5490

Open
ddri wants to merge 1 commit into
OSC:masterfrom
ddri:fix/app-helper-xss-escape
Open

Escape attr.label and attr.value in AppHelper#recent_settings#5490
ddri wants to merge 1 commit into
OSC:masterfrom
ddri:fix/app-helper-xss-escape

Conversation

@ddri

@ddri ddri commented May 27, 2026

Copy link
Copy Markdown
Contributor

The "Recently Used Apps" widget shows a popover with the settings from your last job submission. The label and value for each setting were inserted into the popover HTML without escaping:

# Before
"... <dt>#{attr.label}:</dt> <dd>#{attr.value}</dd> ..."

# After
"... <dt>#{h(attr.label)}:</dt> <dd>#{h(attr.value)}</dd> ..."

If a label or value contained HTML characters like <, >, or ", they would be rendered as markup instead of displayed as text. Wrapping both with h() escapes them before insertion.

Changes

  • app/helpers/app_helper.rb — escape attr.label and attr.value with h() before inserting into popover HTML
  • test/helpers/app_helper_test.rb — add tests for HTML characters in label and value, nil cases, and safe values (file was previously empty)

AppHelper#recent_settings builds an HTML string used as Bootstrap popover
content rendered with bs_html: 'true'. The label and value fields were
interpolated without escaping, allowing unescaped HTML to appear in the
popover. Use h() to escape both fields before insertion.
def recent_settings(app)
content = app.attributes.select(&:display?).map do |attr|
"<div class='row'> <dt>#{attr.label}:</dt> <dd>#{attr.value}</dd> </div>"
"<div class='row'> <dt>#{h(attr.label)}:</dt> <dd>#{h(attr.value)}</dd> </div>"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you use html_escape instead of h - I know it's an alias, but I just feel like being explicit is better here.

@Bubballoo3 Bubballoo3 moved this from Awaiting Review to Changes Requested in PR Review Pipeline May 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Changes Requested

Development

Successfully merging this pull request may close these issues.

4 participants