Skip to content

Enable gocritic equalFold and autofix issues #34678

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

silverwind
Copy link
Member

Enable https://go-critic.com/overview.html#equalfold, which is marked experimental, so was not enabled by default and ran make lint-go-fix. Reasons why strings.EqualFold is preferred here.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 10, 2025
@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/internal labels Jun 10, 2025
@wxiaoguang
Copy link
Contributor

I won't block it but IMO EqualFold is not a right approach if we only want to handle ASCII chars. Sometimes it is just abused.

func TestString(t *testing.T) {
	s1 := "K"
	s2 := "\u212A"
	assert.True(t, strings.EqualFold(s1, s2))
}

@silverwind
Copy link
Member Author

I see it as a benefit that it handles unicode as well. More often than not you want unicode comparison, especially when dealing with user-submitted data.

@wxiaoguang
Copy link
Contributor

Why it makes sense to use Unicode for HTTP headers or something else? Doesn't it break the standard if the headers are "folded" by unicode chars?

@wxiaoguang
Copy link
Contributor

In short:

  • If we'd like to check "if the string can pass", EqualFold relaxes the charsets, it might be abused or introduce security problems.
  • If we'd like to check "if the string should not pass", EqualFold make the result more strict, in most cases it should be fine.

@silverwind
Copy link
Member Author

silverwind commented Jun 10, 2025

Looks like there is a ASCII-only variant ascii.EqualFold:

https://pkg.go.dev/net/http/internal/ascii#EqualFold

Not sure if usable because its in internal namespace.

@wxiaoguang
Copy link
Contributor

Looks like there is a ASCII-only variant ascii.EqualFold:

https://pkg.go.dev/net/http/internal/ascii#EqualFold

Yup, we need to implement one by ourself, something like "strcasecmp/stricmp" in C, or we can just call it util.AsciiEqualFold or util.StrEqualNoCase

@silverwind
Copy link
Member Author

silverwind commented Jun 10, 2025

util.AsciiEqualFold sounds good, we can use it for all the header comparisons. Maybe even open a golang issue so that they make that function part of the public API.

@silverwind silverwind marked this pull request as draft June 10, 2025 10:03
@silverwind
Copy link
Member Author

Related golang issue: golang/go#68736

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/internal topic/code-linting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants