Closed ldpr closed 7 months ago
da2ce7
commented
1 year ago 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
commented
1 year ago 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:
https://github.com/torrust/torrust-index-backend/issues/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
commented
1 year ago 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
commented
1 year ago @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
commented
1 year ago 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
commented
7 months ago I've opened the issue in the Index to change the API.
josecelano
commented
7 months ago Hi @ldpr, implemented via https://github.com/torrust/torrust-index/pull/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:
It's not deployed yet to the live demo.
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.