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

🐛 Fixed escaping search terms that contain special characters #18151

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

markstos
Copy link
Contributor

  • There's a clear use-case for this code change, explained below
  • Commit message has a short title & references relevant issues
  • The build will pass (run yarn test:all and yarn lint)

Before, Sodo Search was not escaping search input before using the search terms in a regular expression, so using special characters could result in an invalid regular expression which would crash JavaScript.

As regular expressions date back to Perl, so does a standard solution for this, which called quotemeta in Perl. It doesn't exist 1:1 in JavaScript, but StackOverflow had the answer:

https://stackoverflow.com/questions/6318710/javascript-equivalent-of-perls-q-e-or-quotemeta

... where it appears I answered this question in 2014. Ha.

So a line of code is added to escape the special characters in the regex for passing them through. This is the same code that the quotemeta module on NPM would use.

Fixes: #18133

QA Log

Unfortunately, my local Ghost development environment is not set up perfectly to test this. I tried using ghost dev --search and that did link a local blog to a local version of Sodo search, but a new JavaScript error appears which I think is unrelated:

search-index.js:12 Uncaught TypeError: b.Document is not a constructor
    at new cm (search-index.js:12:27)

So I'd love if someone with a working dev environment could quickly repro the issue with a "+C" and then apply my match and test again.

Thanks!

@JaydenForYou
Copy link

I have already verified it.bug fixed.

@markstos markstos marked this pull request as ready for review September 15, 2023 12:45
@daniellockyer
Copy link
Member

daniellockyer commented Sep 15, 2023

@markstos Seems like flexsearch has broken during the 0.7.21 -> 0.7.31 upgrade, and we haven't published a release of the package since. I'll revert that package now 🙂

daniellockyer added a commit that referenced this pull request Sep 15, 2023
refs #18151
refs 1cbbe91

- something has broken within 0.7.31 and it's causing an error on the
  frontend
- this commit reverts the dependency back to the previous version
- also cleans up the dependency from announcement-bar, where it is not
  used
daniellockyer added a commit that referenced this pull request Sep 15, 2023
refs #18151
refs 1cbbe91

- something has broken within 0.7.31 and it's causing an error on the
  frontend
- this commit reverts the dependency back to the previous version
- also cleans up the dependency from announcement-bar, where it is not
  used
@markstos
Copy link
Contributor Author

After rebasing this branch off the lastest main with flexsearch revert, I'm able to confirm that a search for "+C" no longer fails and indeeds succeeds.

@daniellockyer daniellockyer changed the title 🐛 Fix escaping search terms that contain special characters (#18133) - Mark Stosberg 🐛 Fixed escaping search terms that contain special characters Sep 20, 2023
@daniellockyer daniellockyer merged commit 7fa083d into TryGhost:main Sep 20, 2023
14 checks passed
@daniellockyer
Copy link
Member

Thanks @markstos! 🙂

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.

the page crased while searching "c++" keyword
3 participants