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

chore: regex sanitizing #22523

Closed
wants to merge 1 commit into from
Closed

chore: regex sanitizing #22523

wants to merge 1 commit into from

Conversation

GmBodhi
Copy link
Contributor

@GmBodhi GmBodhi commented Sep 10, 2021

Description
image

As you guys can see, the . matches everything and, I'm not sure if this was intended behaviour, if it isn't, then this PR resolves it :)

Verified

This commit was signed with the committer’s verified signature.
mrgrain Momo Kornher
@mrdoob
Copy link
Owner

mrdoob commented Sep 10, 2021

This seems simpler 😇

let value = filterInput.value;
if ( value === '.' ) value = '';

@CodyJasonBennett
Copy link
Contributor

CodyJasonBennett commented Sep 10, 2021

We'd need to escape/sanitize search inputs to not have regex injection into search. Otherwise, special characters can lead into unintended results -- . being the most common.

I did this here as well to have input treated as characters rather than an expression:

@octopoulos
Copy link
Contributor

I think it's a powerful feature to be able to use regular expressions in the search.
If '.' returns everything, then why is it bad? What do you want '.' to show?

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 11, 2021

The reported issue shows an unexpected behavior and should be fixed. The search should always perform a full-text search for the given input. If it shows a result that does not match to the search string, then it's a bug.

@octopoulos
Copy link
Contributor

octopoulos commented Sep 11, 2021

If you want to do full-text search, then you don't want to use a regexp at all... so why was a regexp used in the first place in the code (along with some hacks) when you could just do a normal text matching without any hack?

The suggest code fixes that I see above are just more hacks on that regexp ... adding a lot of lines for what could be done in just a few lines, and would run faster too.

I still like the ability to use regexp because it allows me to make an expression that returns my list of favorite examples and only them. Maybe both could be allowed somehow.

Edit: I found a solution to combine both, which I actually used in other projects, simply put: if the search string starts and ends with / like this: /cloth$|mesh/ then it works as a regexp, otherwise just as normal text. I'm going to make a PR to show this in action.

@GmBodhi
Copy link
Contributor Author

GmBodhi commented Sep 22, 2021

Should I close this?

@mrdoob
Copy link
Owner

mrdoob commented Sep 22, 2021

I do not think the search box should allow regex. Seems to be hard enough to search plain text already...

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 16, 2022

Closing in favor of #23729.

@Mugen87 Mugen87 closed this Mar 16, 2022
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.

None yet

5 participants