revoltchat / backend

Monorepo for Revolt backend services.
https://developers.revolt.chat/api/
Other
1.14k stars 125 forks source link

feat: auto-sanitise name when creating bot / user #257

Open insertish opened 1 year ago

insertish commented 1 year ago

Instead of restricting username value, just set a display name if username is non compliant and copy the sanitised name into username.

StaTicDKK17 commented 1 month ago

Hello. I would like to help with this. is there already somewhere in the backend where sanitization is happening or do I have to invent that from scratch?

insertish commented 1 month ago

Only validation, no sanitisation

https://github.com/revoltchat/backend/blob/main/crates/core/database/src/models/users/model.rs#L187 (create user) uses https://github.com/revoltchat/backend/blob/main/crates/core/database/src/models/users/model.rs#L277 (validate username) which throws an error if the username is invalid

I guess the best way to tackle it is to perform the same steps but instead stripping whatever the blocked elements are from the username, using that as the new username, and setting the display name field to the original username

NB. one needs to check the username is still of minimum length after it is sanitised, one could either replace blocked characters/strings with a placeholder such as _ or throw an error if it's too short, or a combination or both: strip until we need to append _ to the end

StaTicDKK17 commented 1 month ago

So, just to make sure I understand, we currently have some code for validating a username. Currently, if the username contains a specific kind of substring (which looks like protection against sqli), then we currently just throw an error that the username does not accept the criteria. But instead, I should just take the username, and remove the strings that violate the blocked parts. Ensure it is still valid in length, and return the result?

Do you want this action of sanitization to happen inside the validate_username function? Or should it be a different function? Because, then the name of the function doesn't really make great sense anymore since it actually tries to sanitize instead.

StaTicDKK17 commented 4 weeks ago

I don't think it makes any sense to try to sanitize for blocked names, since these are supposed to be on the whole username and not only on parts right? Like one could still create a user called something like "revoltFan" or similar, where revolt is a blocked username, but this is only part of the whole username, which contains more?

StaTicDKK17 commented 4 weeks ago

Alright, let's say I have created the code to make the feature. How do I contribute, this is my first time contributing to open source :)

Do I open a new branch on the repo and make a PR to main or I have to do something special? I have tried reading the contribution guidelines, but I am not entirely sure if I am right in creating a new branch and creating a PR or what.

IAmTomahawkx commented 4 weeks ago

Youll want to fork the repo, push your changes to your fork on main, and then open a pr from there (github should have a button on your fork)

StaTicDKK17 commented 4 weeks ago

Is there any minimum length of a username besides empty? I have searched around in the project for a while but couldn't find anything.

Also, are there tests such that I can test if the functionality works or do I just have to run the dev build and try visually to see if the solution works?

insertish commented 3 weeks ago

Off the top of my head, the request structs have validator properties on them which ensure min length of 2 or whatever it's set to. Maybe this should be put into revolt-config somewhere.

If there are no tests, feel free to write some if you're up for it, there are a few routes/database models with existing tests that you can look at to guide.

Otherwise, you'd need to just follow the development guide in README to test.