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

Make confusable character warning less jarring #25069

Merged
merged 10 commits into from
Aug 3, 2023

Conversation

n0toose
Copy link
Contributor

@n0toose n0toose commented Jun 4, 2023

This commit assumes that the warning can be made more discreet
so as to make it less annoying for the people that do not actually
need the warning, without necessarily increasing the risk for those
that do need it.

This doesn't fix the underlying problem of the warning being shown
in certain cases that, say, a certain kind of whitespace character
like 0x1E could be absolutely justifiable from a technical
perspective.

This commit assumes that the warning can be made more discreet
so as to make it less annoying for the people that do not actually
need the warning, without necessarily increasing the risk for those
that do need it.

This doesn't fix the underlying problem of the warning being shown
in certain cases that, say, a certain kind of whitespace character
like 0x1E could be absolutely justifiable from a technical
perspective.
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 4, 2023
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 4, 2023
@n0toose
Copy link
Contributor Author

n0toose commented Jun 4, 2023

No warning's and error's are used anymore, as it is assumed that the large header is enough to attract the attention of the user.

I considered two cases:

  • use warning's in both cases
  • only use warning's in one of the two cases (but I wasn't too sure about that)

@delvh
Copy link
Member

delvh commented Jun 4, 2023

Any screenshot?

@n0toose
Copy link
Contributor Author

n0toose commented Jun 4, 2023

Any screenshot?

Sure, give me a second.

@n0toose
Copy link
Contributor Author

n0toose commented Jun 4, 2023

Full disclosure that I should've mentioned earlier: Considering that the change is minor and does not contain any logic changes or anything that could otherwise prevent the site from rendering correctly, I recreated the change again using "Inspect Element" (removed the tag, changed the text content) on Codeberg to save time.

I didn't see a reason to do so, but I can test this thoroughly if necessary.

image

image

@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 Jun 5, 2023
options/locale/locale_en-US.ini Outdated Show resolved Hide resolved
options/locale/locale_en-US.ini Outdated Show resolved Hide resolved
@n0toose n0toose marked this pull request as draft June 5, 2023 11:55
@n0toose n0toose marked this pull request as ready for review June 25, 2023 14:47
@techknowlogick
Copy link
Member

Thanks for this PR. I'm hesitant to approve with the warning removed, as this is in place to remedy a security concern and otherwise the security reporters had wanted to request a CVE without the warning.

@n0toose
Copy link
Contributor Author

n0toose commented Jun 26, 2023

Maybe we could use warning instead of error? I understand the reason why this is in place and would not like to compromise the readability of this warning (and therefore this isn't the kind of hill I'd be willing to die on), it's just that some users that have valid reasons to use e.g. form feeds get very annoyed by it as well.

Feel free to close this if you believe that this isn't the right way to do this.

@denyskon
Copy link
Member

denyskon commented Aug 1, 2023

I think using warning in both cases would be the best compromise.

@n0toose
Copy link
Contributor Author

n0toose commented Aug 2, 2023

I think using warning in both cases would be the best compromise.

I addressed this.

@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 Aug 2, 2023
@denyskon
Copy link
Member

denyskon commented Aug 2, 2023

@techknowlogick Do you want to block or is this compromise acceptable for you?

@denyskon denyskon added the type/enhancement An improvement of existing functionality label Aug 2, 2023
@denyskon denyskon added this to the 1.21.0 milestone Aug 2, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 3, 2023
@lunny lunny merged commit 0827fbd into go-gitea:main Aug 3, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 3, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 3, 2023
* upstream/main:
  Make confusable character warning less jarring (go-gitea#25069)
  Update Gmail example (go-gitea#26302)
  Fix the topic validation rule and suport dots (go-gitea#26286)
  Upgrade x/net to 0.13.0 (go-gitea#26297)
  add unit test for user renaming (go-gitea#26261)
  add some Wiki unit tests (go-gitea#26260)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Nov 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants