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

Add option to disable ambiguous unicode characters detection #28454

Merged
merged 4 commits into from
Dec 17, 2023

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang added the type/enhancement An improvement of existing functionality label Dec 13, 2023
@wxiaoguang wxiaoguang added this to the 1.22.0 milestone Dec 13, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 13, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 13, 2023
@wxiaoguang wxiaoguang force-pushed the fix-unicode-warning branch 2 times, most recently from bb88607 to c405a81 Compare December 13, 2023 16:29
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 14, 2023
@silverwind
Copy link
Member

Maybe it should even be disabled by default because as I see it, the current detection is almost pure false-positives for the "warnings", only the "errors" are real problems.

@lunny
Copy link
Member

lunny commented Dec 14, 2023

Maybe it should even be disabled by default because as I see it, the current detection is almost pure false-positives for the "warnings", only the "errors" are real problems.

Then we need a break label.

@silverwind
Copy link
Member

Maybe it should even be disabled by default because as I see it, the current detection is almost pure false-positives for the "warnings", only the "errors" are real problems.

Then we need a break label.

It wouldn't break anyone's workflow I assume. Not a fan of unnecessary breaking labels.

@wxiaoguang
Copy link
Contributor Author

Maybe it should even be disabled by default because as I see it, the current detection is almost pure false-positives for the "warnings", only the "errors" are real problems.

I won't do so at the moment, because otherwise some extremists would catch this point and criticize that "Gitea doesn't take security seriously“

@silverwind
Copy link
Member

silverwind commented Dec 14, 2023

I think ideally we just remove the warning category (which warns on any irregular whitespace), and just keep the error category (which are the real bidi exploits). Doesn't have to be in this PR.

@wxiaoguang
Copy link
Contributor Author

I think ideally we just remove the warning category (which warns on any irregular whitespace), and just keep the error category (which are the real bidi exploits). Doesn't have to be in this PR.

That's why I used AMBIGUOUS_UNICODE_DETECTION = true, in the future it could be improved to : AMBIGUOUS_UNICODE_DETECTION = error or AMBIGUOUS_UNICODE_DETECTION = bidi, etc

@JakobDev
Copy link
Contributor

I think ideally we just remove the warning category

Sounds like a good idea. Currently this feature is kinda useless. If you have almost only false positives, Users do not care about the Warning and will ignore it, even when there's a real threat.

You should also add a User setting. Not everyone is Admin.

@wxiaoguang
Copy link
Contributor Author

I think ideally we just remove the warning category

Sounds like a good idea. Currently this feature is kinda useless. If you have almost only false positives, Users do not care about the Warning and will ignore it, even when there's a real threat.

You should also add a User setting. Not everyone is Admin.

It doesn't work that way at the moment, every detected problem is rendered as "warning".

Any improvement could be done later.

image

@silverwind
Copy link
Member

silverwind commented Dec 14, 2023

Does this hide the "Escape" button in file view and diffs? I think it should when the feature is disabled as the button is only useful for that feature.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Dec 14, 2023

Does this hide the "Escape" button in file view and diffs? I think it should when the feature is disabled as the button is only useful for that feature.

Yes. There won't be "escape" button if there is no detected ambiguous unicode character.


update: due to some old code problem, see #28454 (comment)

routers/web/repo/view.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 17, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Dec 17, 2023
@lunny lunny enabled auto-merge (squash) December 17, 2023 14:04
@wxiaoguang wxiaoguang added the backport/v1.21 This PR should be backported to Gitea 1.21 label Dec 17, 2023
@lunny lunny merged commit 20929ed into go-gitea:main Dec 17, 2023
25 checks passed
@GiteaBot
Copy link
Contributor

I was unable to create a backport for 1.21. @wxiaoguang, please send one manually. 🍵

go run ./contrib/backport 28454
...  // fix git conflicts if any
go run ./contrib/backport --continue

@GiteaBot GiteaBot added backport/manual No power to the bots! Create your backport yourself! and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Dec 17, 2023
@wxiaoguang wxiaoguang deleted the fix-unicode-warning branch December 17, 2023 14:55
wxiaoguang added a commit to wxiaoguang/gitea that referenced this pull request Dec 17, 2023
@wxiaoguang
Copy link
Contributor Author

Does this hide the "Escape" button in file view and diffs? I think it should when the feature is disabled as the button is only useful for that feature.

Yes. There won't be "escape" button if there is no detected ambiguous unicode character.

Just realized that some "escape" buttons are hidden correctly, while some are not, because some of the old code doesn't respect EscapeStatus.Escaped .... so some buttons are always there.

wolfogre pushed a commit that referenced this pull request Dec 18, 2023
…#28499)

Backport #28454 (the only conflict is caused by some comments)

* Close #24483
* Close #28123
* Close #23682
* Close #23149
zjjhot added a commit to zjjhot/gitea that referenced this pull request Dec 19, 2023
* giteaofficial/main:
  [skip ci] Updated translations via Crowdin
  Add option to disable ambiguous unicode characters detection (go-gitea#28454)
  Adjust object format interface (go-gitea#28469)
  Remove duplicate option in admin screen and now-unused translation keys (go-gitea#28492)
  [skip ci] Updated translations via Crowdin
  Initalize stroage for orphaned repository doctor (go-gitea#28487)
lunny pushed a commit that referenced this pull request Dec 21, 2023
Regression of #28454 . Now the string is escaped HTML, so it doesn't
need `| Safe`.

Fix #28575
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Dec 21, 2023
Regression of go-gitea#28454 . Now the string is escaped HTML, so it doesn't
need `| Safe`.

Fix go-gitea#28575
lafriks pushed a commit that referenced this pull request Dec 22, 2023
Backport #28576 by wxiaoguang

Regression of #28454 . Now the string is escaped HTML, so it doesn't
need `| Safe`.

Fix #28575

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
techknowlogick pushed a commit to techknowlogick/gitea that referenced this pull request Dec 23, 2023
Regression of go-gitea#28454 . Now the string is escaped HTML, so it doesn't
need `| Safe`.

Fix go-gitea#28575
uli-heller pushed a commit to uli-heller/gitea that referenced this pull request Jan 16, 2024
Backport go-gitea#28576 by wxiaoguang

Regression of go-gitea#28454 . Now the string is escaped HTML, so it doesn't
need `| Safe`.

Fix go-gitea#28575

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
fuxiaohei pushed a commit to fuxiaohei/gitea that referenced this pull request Jan 17, 2024
fuxiaohei pushed a commit to fuxiaohei/gitea that referenced this pull request Jan 17, 2024
Regression of go-gitea#28454 . Now the string is escaped HTML, so it doesn't
need `| Safe`.

Fix go-gitea#28575
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
Regression of go-gitea#28454 . Now the string is escaped HTML, so it doesn't
need `| Safe`.

Fix go-gitea#28575
@wxiaoguang wxiaoguang added the backport/done All backports for this PR have been created label Feb 21, 2024
@silverwind
Copy link
Member

Does this hide the "Escape" button in file view and diffs? I think it should when the feature is disabled as the button is only useful for that feature.

Yes. There won't be "escape" button if there is no detected ambiguous unicode character.

Just realized that some "escape" buttons are hidden correctly, while some are not, because some of the old code doesn't respect EscapeStatus.Escaped .... so some buttons are always there.

We should get rid of all those unneccesary and confusing escape buttons, e.g. only show if there is actually escapable content.

Copy link

github-actions bot commented Mar 1, 2024

Automatically locked because of our CONTRIBUTING guidelines

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created backport/manual No power to the bots! Create your backport yourself! backport/v1.21 This PR should be backported to Gitea 1.21 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/docs size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
6 participants