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

[feat request] validate username _before_ signup #259

Closed
ldpr opened this issue Sep 9, 2023 · 7 comments
Closed

[feat request] validate username _before_ signup #259

ldpr opened this issue Sep 9, 2023 · 7 comments
Assignees
Labels
Easy Good for Newcomers Enhancement / Feature Request Something New
Milestone

Comments

@ldpr
Copy link

ldpr commented Sep 9, 2023

ideally, it would be best for usernames to only be letters and numbers (maybe with hyphens and underscores?) I had a quick skim through our users table yesterday and even found some users with emojis as their username. it's humurous sure and works fine on sqlite, but will probably cause issues with mysql etc so it's probably best to nip this in the bud.

i'm unsure the best way of doing this and i'm a bit of a vue noob, it could be done via html with:

<input type="text" pattern="[a-z][a-z0-9-_]">

but maybe there's a better, cleaner way of doing it?

sorry also if this is something implemented in the newer crates, i had a quick skim through issues and didn't see it mentioned.

@da2ce7
Copy link
Contributor

da2ce7 commented Sep 10, 2023

I think that the best solution is that the backend validates the username. The backend should also provide the frontend with a regular expression that defines the rules for the username validation.

@josecelano josecelano added Enhancement / Feature Request Something New Easy Good for Newcomers labels Sep 11, 2023
@josecelano
Copy link
Member

Hi @ldpr, @da2ce7, I'm not sure if we should do it. As a native Spanish speaker, I know the pain of using a subset of ASCII/ISO 8859-1 (Latin-1) characters. I try to avoid it for domains when I write my name in signup forms, etcetera, because it's always a problem. But I do not know where we can put the limit. For example, I'm working on this issue:

torrust/torrust-index#288

to avoid empty names for categories, but we could also avoid emojis in category names. The question is, should we prohibit emojis in the database?

On the other hand, nowadays, databases should not have problems handling that kind of info. Besides, from the security point of view, it's probably better to use a username with emojis.

What I would recommend is not to allow it only when we are sure it is causing problems, and we should add tests to make sure it works.

Alternatively, we should not allow emojis in all fields except description fields. Why is it more critical to use emojis for usernames than categories? Maybe because, in the future, the username could be used in a URL? If that's the reason, I'm OK with the proposed solution, but not because it could be a problem for the database (unless we find a concrete problem).

@ldpr
Copy link
Author

ldpr commented Sep 15, 2023

Hey @josecelano!

What I would recommend is not to allow it only when we are sure it is causing problems, and we should add tests to make sure it works.

Are you sure this is the best way forward? I'm totally fine with whatever you decide as Torrust is still in development. I'm coming at it from a "we're using it in production already" angle, if that makes sense. For example, if emojis work fine now, great - but if they cause issues further down the line then we'd need to manually remove users/torrents (whatever is applicable) via the DB. Granted, this is pretty unlikely but emojis are just an example.

Just in case it was missed also, things like symbols (/#^* etc) are currently allowed and this has me a little concerned. The "standard" I guess for usernames is just text (emojis is fine), if that includes nuances with foreign languages etc then that's fine too. It shouldn't include things like commas, backslashes etc IMO.

The question is, should we prohibit emojis in the database?

Would a setting or flag be possible or would it be too much work? For example, by default Torrust could support using anything for usernames but if people were inclined, they could enable a flag to limit usernames, torrentnames & category names to normal text.

I feel like there should be some kind of standard or project that we could lean on as an example, but my google skills are lacking. If anyone knows more or has a few links, I'd love to read more!

@da2ce7
Copy link
Contributor

da2ce7 commented Sep 15, 2023

@josecelano

I think that we should at least enforce that the string is valid unicode; and have some sort of length restriction.

Alternatively, we should not allow emojis in all fields except description fields. Why is it more critical to use emojis for usernames than categories? Maybe because, in the future, the username could be used in a URL? If that's the reason, I'm OK with the proposed solution, but not because it could be a problem for the database (unless we find a concrete problem).

Then it is the responsibility of the fronted to sanitize the usernames of users that have placed 100 new lines and random unicode characters, and whatever that would disturb visual experience.

@josecelano
Copy link
Member

josecelano commented Sep 15, 2023

Hi @ldpr just to be clear, I'm not saying we should wait until we find a bug, or people start misusing this capacity, I meant we should make a decision based on concrete problems. I also have the feeling that emojis could be a pain, but I think we can not just avoid them for that reason. I really appreciate your feedback as a user who is using the Index in production, but there could be other users who like those features.

Anyway, since we do not have feedback from other current users, we could not allow the use of emojis and enable it in the future as a feature flag if some users demand it.

And regarding your comment @da2ce7 I totally agree. We should also make sure that the database encoding supports emojis even if we do not allow them now.

Finally, not allowing emojis except for description fields (maybe only for fields which contain markdown) is not an easy feature. Maybe we can start with the username because is pretty likely that, at some point is going to be used in a URL. We should address that concrete problem at the moment if you consider a percent-encoded emoji in the URL a bad thing, although I would not be surprised if they become popular:

@josecelano
Copy link
Member

I've opened the issue in the Index to change the API.

@josecelano
Copy link
Member

Hi @ldpr, implemented via torrust/torrust-index#504. I think I`m not going to add validation in the frontend for the time being because all the validation is done in the backend.

This is the error message:

image

It's not deployed yet to the live demo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Easy Good for Newcomers Enhancement / Feature Request Something New
Projects
Status: Done
Development

No branches or pull requests

4 participants