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

Fix user queries when they contain " #3037

Merged
merged 1 commit into from Jun 6, 2020

Conversation

aledeg
Copy link
Member

@aledeg aledeg commented Jun 6, 2020

Changes proposed in this pull request:

  • Fix user query when they contain "

How to test the feature manually:

  1. search for a string containing a "
  2. create a user query from it
  3. save the queries created
  4. before the fix, the search pattern disappear from the configuration

Pull request checklist:

  • clear commit messages
  • code manually tested
  • unit tests written (optional if too hard)
  • documentation updated

Additional information can be found in the documentation.

@aledeg aledeg added UI 🎨 User Interfaces Search 🔍 labels Jun 6, 2020
@aledeg aledeg added this to the 1.17.0 milestone Jun 6, 2020
@Alkarex
Copy link
Member

Alkarex commented Jun 6, 2020

I believe we need something like htmlspecialchars.
Try with e.g. <"te&st'> and we get some unescaped chars in the DOM

@aledeg
Copy link
Member Author

aledeg commented Jun 6, 2020

ok, I'll try to fix that as well. I did not encounter that type of problem before. I'll keep you posted.

@aledeg
Copy link
Member Author

aledeg commented Jun 6, 2020

I cannot see the unescaped chars you mentioned. I've tried with the string you mentioned and everything is properly urlencoded.

@Alkarex
Copy link
Member

Alkarex commented Jun 6, 2020

The HTML behind
image
looks like that:
image

@Alkarex
Copy link
Member

Alkarex commented Jun 6, 2020

We can also produce an XSS (blocked thanks to our CSP rules) with <script>alert('XSS');</script>
image

@aledeg
Copy link
Member Author

aledeg commented Jun 6, 2020

I've checked and it did not trigger any XSS. But it was not displayed properly either. I fixed it anyway.

@@ -52,7 +52,7 @@

<ul>
<?php if ($query->hasSearch()) { ?>
<li class="item"><?= _t('conf.query.search', $query->getSearch()->getRawInput()) ?></li>
<li class="item"><?= _t('conf.query.search', htmlspecialchars($query->getSearch()->getRawInput())) ?></li>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<li class="item"><?= _t('conf.query.search', htmlspecialchars($query->getSearch()->getRawInput())) ?></li>
<li class="item"><?= _t('conf.query.search', htmlspecialchars($query->getSearch()->getRawInput(), ENT_NOQUOTES, 'UTF-8')) ?></li>

Explicit encoding

@Alkarex
Copy link
Member

Alkarex commented Jun 6, 2020

You need to disable our CSP protection to test XSS ;-)

Before, the user queries were working filter-wise but they failed at being displayed
properly in the configuration page. Thus they were stored without the search param.
Now, the search is URL encoded to avoid that kind of behavior and keep the search
param through out the user query's life.
@Alkarex Alkarex merged commit b2b249d into FreshRSS:master Jun 6, 2020
@aledeg aledeg deleted the hotfix/user-queries branch June 6, 2020 19:46
@Alkarex Alkarex modified the milestones: 1.17.0, 1.16.3 Aug 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants