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: validate that token name doesn't contain only spaces #3437

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bassgeta
Copy link
Contributor

Description

Added a validation test to check if the string isn't just whitespaces. I however allow for leading and trailing whitespaces, but they get trimmed out when creating the token? 🤷

Testing

  1. Start up your dev env and run node ./scripts/create-colony-url.js
  2. Select "Create a new token"
    image
  3. Now try to break the token name validation, entering only spaces should fail
    image
    But spaces at the start/end should work if you have any content
    image

Diffs

Changes 🏗

  • createTokenValidationSchema now includes whitespace only validation for tokenName

Resolves #3298

@bassgeta bassgeta self-assigned this Oct 23, 2024
@bassgeta bassgeta requested review from a team as code owners October 23, 2024 13:11
Copy link
Contributor

@iamsamgibbs iamsamgibbs left a comment

Choose a reason for hiding this comment

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

Nice job this is working as expected!

Screenshot 2024-10-24 at 09 13 43

I guess we need a product decision on whether leading / trailing spaces?

Screenshot 2024-10-24 at 09 14 12

Either way it looks like we'll need to address how the colony name is displayed with spaces in elsewhere in the app as it doesn't deal well with multiple spaces:

Screenshot 2024-10-24 at 09 15 00 Screenshot 2024-10-24 at 09 14 18

It does retain the spaces in the db though, so this is purely a UI issue:

Screenshot 2024-10-24 at 09 15 20

None of that prevents this from getting merged though!

Copy link
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

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

All good here, it now validates against empty spaces

Screenshot from 2024-10-24 20-33-47
Screenshot from 2024-10-24 20-34-00
Screenshot from 2024-10-24 20-34-03

However I can still bypass it with Unicode spaces (in this case U+0020)

Screenshot from 2024-10-24 20-35-06
Screenshot from 2024-10-24 20-35-14
Screenshot from 2024-10-24 20-35-17

This should not make the focus of this PR, and arguably we shouldn't bother with Unicode validates as most nobody does (just test this on any major service's forms, and you won't get validated against -- this is actually my go to when I don't want to provide a name 😛).

However one could also argue that since this is a Token and it's going to be deployed "forever" with that name, it might become confusing, so we might want to prevent against that. (maybe run this validation through our "confusable characters" lib that we use for usenames?)

PS: for testing unicodes I use this website https://www.editpad.org/tool/invisible-character

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.

[Create Colony Flow]: It is possible to enter only spaces in the "Token name" field
3 participants