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: encode search keyword #2903

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix: encode search keyword #2903

wants to merge 4 commits into from

Conversation

yajra
Copy link
Owner

@yajra yajra commented Nov 17, 2022

Fix #2901

@yajra yajra marked this pull request as draft November 17, 2022 07:22
@yajra yajra marked this pull request as ready for review November 18, 2022 07:21
@yajra
Copy link
Owner Author

yajra commented Nov 18, 2022

Note: need to review as encoding the keyword is a change in behavior and might cause a breaking change.

@sonarcloud
Copy link

sonarcloud bot commented Nov 22, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
48.1% 48.1% Duplication

Copy link
Contributor

@dyaskur dyaskur left a comment

Choose a reason for hiding this comment

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

This change will make we can not search non encoded special chars. an example we have data "Hand & Soulder"(no encode) and we search using "&", and the data will not shown.

My suggestion is using attribute like searchable in datatable options.

an example:

              {data: 'name', name: 'name',searchable: true, is_encoded:true},

and if is_encoded:true, we use e() function, otherwise return without encode.

@yajra
Copy link
Owner Author

yajra commented Dec 30, 2022

@dyaskur thanks for the review. However, adding is_encoded:true in the script will not affect the serverSide implementation as the js library will not send it back in the ajax request.

How about if we add another control on a column level that toggles encoded search? Maybe something like:

->encodedSearch(['col1', 'col2'])

@dyaskur
Copy link
Contributor

dyaskur commented Dec 30, 2022

I see, I thought datatable will send all column attribute to the ajax request. If can't send by client side, we need to set it on server side. And I think your idea looks good.

@aravael
Copy link

aravael commented Feb 16, 2023

@yajra , recently came up with a similar solution while debugging a weird behavior with the collection as a source. This change adds equality to a search input and modified collection at DataProcessor escapeRow which uses laravel's e() func.
Any estimates on when this will be merged?

@yajra
Copy link
Owner Author

yajra commented Feb 20, 2023

@aravael, would you be able to test this and see the impact on your existing project? Would you approve this PR?

@aravael
Copy link

aravael commented Feb 28, 2023

Hi @yajra . Recently managed to test it, can't approve this patch. Though it fixes the collection search, it breaks the builder source. Since we sanitize the input with e(), the database data are still not modified, therefore not found the result.

It works with a collection because both the collection and input sanitized. In case of builder source it doesn't work that way. Need to process that out.

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.

4 participants