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.

Sign up for GitHub

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

Jump to bottom

[Validator] Add a NoSuspiciousCharacters constraint to validate a string is not suspicious #49300

Merged
merged 1 commit into from Feb 21, 2023
Merged

[Validator] Add a NoSuspiciousCharacters constraint to validate a string is not suspicious #49300

merged 1 commit into from Feb 21, 2023

Conversation

MatTheCat
Copy link
Contributor

@MatTheCat MatTheCat commented Feb 8, 2023 •
edited

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #49268
License MIT
Doc PR symfony/symfony-docs#17897

Leverage Spoofchecker::isSuspicious in a new constraint.

@carsonbot carsonbot added this to the 6.3 milestone Feb 8, 2023
@MatTheCat MatTheCat changed the title [Validator] Add a NotSuspicious constraint to validate a string is not a spoof attempt [Validator] Add a NotSuspicious constraint to validate a string is not suspicious Feb 8, 2023
@ro0NL
Copy link
Contributor

ro0NL commented Feb 8, 2023

what about a form option / serializer context entry to block such input by default

@MatTheCat
Copy link
Contributor Author

@ro0NL I’m not sure this is necessary: few inputs would be concerned by such validation 🤔

WebMamba
WebMamba reviewed Feb 8, 2023
View reviewed changes
Copy link
Contributor

@WebMamba WebMamba left a comment

Choose a reason for hiding this comment

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

Hey @MatTheCat Thanks for your work on this this! That looks cool, just left some minor thinking 😁

src/Symfony/Component/Validator/Constraints/NotSuspicious.php Outdated Show resolved Hide resolved
src/Symfony/Component/Validator/Constraints/NotSuspiciousValidator.php Outdated Show resolved Hide resolved
src/Symfony/Component/Validator/Constraints/NotSuspiciousValidator.php Outdated Show resolved Hide resolved
@alexislefebvre
Copy link
Contributor

How does it handle emojis? Could you please add a test case with a simple emoji like “:smiley:”?

@MatTheCat
Copy link
Contributor Author

@alexislefebvre emojis belong to the “Common” script and should never be considered suspicious (well unless you set the ASCII restriction level). Not sure this requires a dedicated test case?

@alexislefebvre
Copy link
Contributor

I didn't know that it was built in PHP: https://www.php.net/manual/en/spoofchecker.issuspicious.php The documentation is vague.

Sorry for the bad writing, I thought about testing a string that contain an emoji, not necessarily as a dedicated test case.

@MatTheCat MatTheCat force-pushed the ticket_49268 branch 4 times, most recently from 0d11716 to bad8dda Compare February 9, 2023 17:07
@MatTheCat MatTheCat changed the title [Validator] Add a NotSuspicious constraint to validate a string is not suspicious [Validator] Add a NoSuspiciousCharacters constraint to validate a string is not suspicious Feb 10, 2023
nicolas-grekas
nicolas-grekas approved these changes Feb 11, 2023
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Do you think we also need to provide a different message for each restriction level?

No need to, another PR can do it later if needed.

LGTM otherwise, thanks.

src/Symfony/Component/Validator/Constraints/NoSuspiciousCharacters.php Outdated Show resolved Hide resolved
src/Symfony/Component/Validator/Constraints/NoSuspiciousCharactersValidator.php Outdated Show resolved Hide resolved
src/Symfony/Component/Validator/Constraints/NoSuspiciousCharacters.php Outdated Show resolved Hide resolved
MatTheCat added a commit to MatTheCat/symfony that referenced this pull request Feb 11, 2023
@MatTheCat
Address symfony#49300 (review)
6b86297
@MatTheCat MatTheCat mentioned this pull request Feb 12, 2023
Merged
MatTheCat added a commit to MatTheCat/symfony that referenced this pull request Feb 13, 2023
@MatTheCat
Address symfony#49300 (review)
b923613
@MatTheCat MatTheCat force-pushed the ticket_49268 branch 3 times, most recently from 9c0be08 to 25bb90d Compare February 13, 2023 15:58
@MatTheCat MatTheCat force-pushed the ticket_49268 branch 3 times, most recently from f717ff3 to 6e2b092 Compare February 14, 2023 08:17
nicolas-grekas
nicolas-grekas approved these changes Feb 14, 2023
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

LGTM thanks!
please squash while fixing the last µ-comment

src/Symfony/Component/Validator/Tests/Constraints/NoSuspiciousCharactersValidatorTest.php Outdated Show resolved Hide resolved
@nicolas-grekas
Copy link
Member

Thank you @MatTheCat.

@nicolas-grekas nicolas-grekas merged commit 8fb797c into symfony:6.3 Feb 21, 2023
@MatTheCat MatTheCat deleted the ticket_49268 branch February 21, 2023 13:12
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Mar 22, 2023
@javiereguiluz
minor #17897 [Validator] Document the new NoSuspiciousCharacters co…
8fab448 
…nstraint (MatTheCat)

This PR was squashed before being merged into the 6.3 branch.

Discussion
----------

[Validator] Document the new `NoSuspiciousCharacters` constraint

Feature PR: symfony/symfony#49300

Commits
-------

e4d1b82 [Validator] Document the new `NoSuspiciousCharacters` constraint
@fabpot fabpot mentioned this pull request May 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@WebMamba WebMamba WebMamba left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Feature Status: Reviewed Validator
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.

Add validation rule based on Spoofchecker::isSuspicious()
7 participants
@MatTheCat @ro0NL @alexislefebvre @nicolas-grekas @stof @WebMamba @carsonbot