Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't blindly mark flash strings HTML safe #3229

Merged
merged 2 commits into from
Aug 31, 2018
Merged

Don't blindly mark flash strings HTML safe #3229

merged 2 commits into from
Aug 31, 2018

Conversation

no-reply
Copy link
Contributor

@no-reply no-reply commented Aug 30, 2018

This view partial had originally marked all of the strings it received as #html_safe, effectively trusting all content. Using sanitize essentially does the opposite: distrusting all flash message content. Any tags or attributes not in the whitelist (https://github.com/flavorjones/loofah/blob/master/lib/loofah/html5/whitelist.rb) will be removed, regardless of the source.

This seems like the best solution, since flash messages are serialized as strings, so the providers cannot give us carefully constructed #html_safe strings. In general, flash messages shouldn't require markup more complex than basic formatting and links.

Fixes #3228
Connected to #3187

Changes proposed in this pull request:

  • Remove map(&:html_safe)!!!
  • Call sanitize on flash messages

Guidance for testing, such as acceptance criteria or new user interface behaviors:

@samvera/hyrax-code-reviewers

This is in partial fulfillment of #3228.

Removing the `#html_safe` calls from this line avoids marking arbitrary strings
as safe when they are very likely not.

Providers of flash messages need to be refactored to manage the safety of the
buffers they provide.
@jcoyne
Copy link
Member

jcoyne commented Aug 31, 2018

If I recall correctly, there is no way for flash messages, which get serialized into the session to be marked as HTML safe. I believe there are one or two places where we've inserted html link tags into the flash, so this change may break those links.

@jcoyne
Copy link
Member

jcoyne commented Aug 31, 2018

How about calling sanitize() on the flash? See: https://api.rubyonrails.org/classes/ActionView/Helpers/SanitizeHelper.html#method-i-sanitize

@no-reply
Copy link
Contributor Author

@jcoyne I think I follow. Since the flash messages are serialized into the session using the JSON serializer, they come out as strings, even if there were originally safe buffers?

Yeah, it seems like sanitize and a conservative whitelisted scrubber are the way to go here.

Do we have any example flash messages with HTML formatting?

@jcoyne
Copy link
Member

jcoyne commented Aug 31, 2018

@no-reply that's what I'm thinking. Sorry, I don't know where we have html in the flash though. Maybe these?

$ git grep flash app/controllers/|grep html
app/controllers/concerns/hyrax/works_controller_behavior.rb:            flash[:notice] = view_context.t('hyrax.works.create.after_create_html', application_name: view_context.application_name)
app/controllers/hyrax/batch_uploads_controller.rb:        flash[:notice] = view_context.t('hyrax.works.create.after_create_html', application_name: view_context.application_name)

jcoyne
jcoyne previously approved these changes Aug 31, 2018
no-reply pushed a commit that referenced this pull request Aug 31, 2018
Marking unknown strings as `#html_safe` in a view is not safe. This fixes many
instances of this, in favor of using the `sanitize` helper.

Related to #3187; #3229.
Using `sanitize` cleans up flash messages with
`Rails::Html::WhiteListSanitizer`.

This view partial had originally marked all of the strings it received as
`#html_safe`, effectively trusting all content. Using sanitize essentially does
the opposite: distrusting all flash message content. Any tags or attributes not
in the
whitelist (https://github.com/flavorjones/loofah/blob/master/lib/loofah/html5/whitelist.rb)
will be removed, regardless of the source.

This seems like the best solution, since flash messages are serialized as
strings, so the providers cannot give us carefully constructed `#html_safe`
strings. In general, flash messages shouldn't require markup more complex than
basic formatting and links.

Fixes #3228
Connected to #3187
no-reply pushed a commit that referenced this pull request Aug 31, 2018
Marking unknown strings as `#html_safe` in a view is not safe. This fixes many
instances of this, in favor of using the `sanitize` helper and `json_escape`.

Related to #3187; #3229.
@no-reply no-reply merged commit db15e71 into master Aug 31, 2018
@no-reply no-reply deleted the flash-xss branch August 31, 2018 22:31
@no-reply no-reply restored the flash-xss branch August 31, 2018 22:31
@no-reply no-reply deleted the flash-xss branch August 31, 2018 22:31
no-reply pushed a commit that referenced this pull request Sep 10, 2018
Marking unknown strings as `#html_safe` in a view is not safe. This fixes many
instances of this, in favor of using the `sanitize` helper and `json_escape`.

Related to #3187; #3229.
no-reply pushed a commit that referenced this pull request Sep 10, 2018
Marking unknown strings as `#html_safe` in a view is not safe. This fixes many
instances of this, in favor of using the `sanitize` helper and `json_escape`.

Related to #3187; #3229.
no-reply pushed a commit that referenced this pull request Sep 10, 2018
Marking unknown strings as `#html_safe` in a view is not safe. This fixes many
instances of this, in favor of using the `sanitize` helper and `json_escape`.

Related to #3187; #3229.
no-reply pushed a commit that referenced this pull request Sep 11, 2018
Marking unknown strings as `#html_safe` in a view is not safe. This fixes many
instances of this, in favor of using the `sanitize` helper and `json_escape`.

Related to #3187; #3229.
no-reply pushed a commit that referenced this pull request Sep 12, 2018
Marking unknown strings as `#html_safe` in a view is not safe. This fixes many
instances of this, in favor of using the `sanitize` helper and `json_escape`.

Related to #3187; #3229.
no-reply pushed a commit that referenced this pull request Sep 13, 2018
Marking unknown strings as `#html_safe` in a view is not safe. This fixes many
instances of this, in favor of using the `sanitize` helper and `json_escape`.

Related to #3187; #3229.
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.

Refactor HTML Safety of flash messages
2 participants