uclibs / ucrate

Scholar@UC: University of Cincinnati's self-submission institutional repository
https://scholar.uc.edu
Other
5 stars 3 forks source link

Mysterious popup on index sort #950

Closed crowesn closed 2 years ago

crowesn commented 2 years ago

Descriptive summary

On Scholar production, catalog index sort options result in mysterious popup.

Screen Shot 2021-11-29 at 11 23 04 AM

Expected behavior

Selecting sort option results in index refresh with corresponding sorted results.

Actual behavior

The results appear to be sorted by select option, however, there is a mysterious pop-up dialog with "1" message.

Steps to reproduce the behavior

  1. Navigate to scholar catalog index
  2. Select "Sort by date uploaded" from sort dropdown/select
hortongn commented 2 years ago

Reopening this issue. We noticed after deploying to production that this fix broke our search term highlighting. Instead of getting yellow highlighted search terms on our search results we got things like This is the <mark>work</mark> title. It's listing the HTML tags instead of interpreting them. I think that's what we intended, but we didn't realize it would break search term highlighting.

crowesn commented 2 years ago

I'm not encountering this on production - can you share an example? https://scholar.uc.edu/catalog?utf8=%E2%9C%93&locale=en&search_field=all_fields&q=Paris

Screen Shot 2022-04-07 at 1 33 53 PM

hortongn commented 2 years ago

Sorry, we applied at hot fix on production to temporarily fix it. We changed https://github.com/uclibs/ucrate/blob/develop/app/helpers/mark_helper.rb#L37 from value back to value.html_safe. However, that undid the fix you had put in place for this issue.

Here's an example on Scholar-qa when someone searches for "test": https://scholar-qa.uc.edu/catalog?utf8=%E2%9C%93&locale=en&search_field=all_fields&q=test

So we ideally want to fix the XSS injection vulnerability while preserving the search highlighting.

crowesn commented 2 years ago

Instead of risking html_safe on those strings, I think we can sanitize the string and whitelist <mark> tags. I'll get in a PR with improved spec shortly.