Skip to content

Make page_title HTML-escape only once#92

Merged
neilvcarvalho merged 2 commits into
thoughtbot:mainfrom
elias19r:html-escape-only-once
Jun 10, 2025
Merged

Make page_title HTML-escape only once#92
neilvcarvalho merged 2 commits into
thoughtbot:mainfrom
elias19r:html-escape-only-once

Conversation

@elias19r
Copy link
Copy Markdown
Contributor

@elias19r elias19r commented Jun 9, 2025

This wraps the string returned by page_title with escape_once from
ActionView::Helpers::TagHelper to make sure it HTML-escapes only once.

Without it, rendering <%= page_title %> in an ERB template causes it
to be HTML-escaped twice.

https://api.rubyonrails.org/v6.1.0/classes/ActionView/Helpers/TagHelper.html#method-i-escape_once

Issue #73

This wraps the string returned by page_title with escape_once from
ActionView::Helpers::TagHelper to make sure it HTML-escapes only once.

Without it, rendering <%= page_title %> in an ERB template causes it
to be HTML-escaped twice.
content_for(page_title.page_title_symbol),
page_title.separator,
options[:reverse],
).to_s,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@neilvcarvalho

there shouldn't be any backwards compatibility issue in making flutie return HTML-safe strings

My first solution was calling .html_safe here and it worked, but only for the page title content. To make sure custom separator and custom app_name are also html-escaped, and once, I think we need to go with escape_once

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oooh, that's a great point. I hadn't thought of separators and app names needing to be escaped, as in "Johnson & Johnson > São Paulo Headquarters". TIL escape_once.

Comment thread lib/flutie/page_title_helper.rb Outdated
escape_once returns an html-safe string but only in Rails >= 7.1.
So for compatibility with older versions, we need to explicitly
apply html_safe here.

Co-authored-by: Neil Carvalho <me@neil.pro>
page_title.separator,
options[:reverse],
).to_s,
).html_safe
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Rails/OutputSafety: Tagging a string as html safe may be a security risk.

@neilvcarvalho neilvcarvalho merged commit 2842757 into thoughtbot:main Jun 10, 2025
26 checks passed
@neilvcarvalho
Copy link
Copy Markdown
Member

Thanks for your contribution!

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.

2 participants