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

WIP: Add Ambiguous Character Detection to Description, FullName and others #20795

Closed
wants to merge 1 commit into from

Conversation

zeripath
Copy link
Contributor

Users can change their description, full-names and others to include
ambiguous and invisible characters which may lead to misleading
information.

This PR adds ambiguous/invisible character detection to these fields.

Signed-off-by: Andrew Thornton [email protected]

@zeripath zeripath added topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! topic/ui Change the appearance of the Gitea UI pr/wip This PR is not ready for review labels Aug 14, 2022
@zeripath zeripath force-pushed the ambiguous-names-email branch 4 times, most recently from 8a4353f to 18611f6 Compare August 17, 2022 19:56
Users can change their description, full-names and others to include
ambiguous and invisible characters which may lead to misleading
information.

This PR adds ambiguous/invisible character detection to these fields.

Signed-off-by: Andrew Thornton <[email protected]>
@silverwind
Copy link
Member

Does seem a bit over-the-top to me as in theory, any user-provided content can include those characters. In code it does somewhat makes sense to detect possible trickery, but I wonder if we really need to expand this mechanism outside code.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 23, 2022
@zeripath
Copy link
Contributor Author

Does seem a bit over-the-top to me as in theory, any user-provided content can include those characters. In code it does somewhat makes sense to detect possible trickery, but I wonder if we really need to expand this mechanism outside code.

It's really not over the top.

The user's full name is displayed in several places. Look at the following:

Αոԁгеԝ Тһοгոtοո

Only one of those letters is in the ASCII range.

The user's location and website are all displayed directly to the user.

The email address is displayed on the admin pages and this could be very confusing indeed.

Whether we should be confusable checking commit messages I don't know but the others seem serious enough to me.

@wxiaoguang
Copy link
Contributor

I do not think it's right to detect Ambiguous Character everywhere.

It already causes a lot of complains for the wiki pages, but nobody ever complains "there is no Ambiguous Character detection".

So I would suggest to close this PR.

@wxiaoguang wxiaoguang added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label May 2, 2023
@silverwind
Copy link
Member

I agree. It's a nice idea in theory but I think no one wants to see a warning just because the put a non-standard quote somewhere. It only is relevant to code, and even there, the current detection is too aggressive.

So I think we should not extend this beyond code and trim down to the detection to only warn on really nefarious stuff like unicode writing mode change characters and such.

See:

#23682
#23149
#24483

@silverwind silverwind closed this May 2, 2023
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. pr/wip This PR is not ready for review topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants