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

Remove padding characters from random strings #15536

Closed
wants to merge 3 commits into from
Closed

Remove padding characters from random strings #15536

wants to merge 3 commits into from

Conversation

kuoruan
Copy link

@kuoruan kuoruan commented Apr 19, 2021

  • base64.RawURLEncoding added in go 1.15,
    can remove padding '=' characters from base64 strings

When I use gitea as a OIDC provider, I find the code generated by gitea is a base64 string contains =

P9BIOCLzf9pGeDgg6rc2HpLls5kehMVtWrxNfWUdho4=

Then the redirect query become

?code=P9BIOCLzf9pGeDgg6rc2HpLls5kehMVtWrxNfWUdho4%3D&state=MjVmMjhhODMtODA2Yy00

The = in code encoded to %3D and may cause some error. This commit removes the padding characters

kuoruan and others added 3 commits April 19, 2021 06:29
* base64.RawURLEncoding added in go 1.15,
  can remove padding '=' characters from base64 strings
@zeripath
Copy link
Contributor

Do we need to make changes to tolerate the raw url encoding when it comes back to us?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 19, 2021
@kuoruan
Copy link
Author

kuoruan commented Apr 20, 2021

Current project 's minimal support Golang version defined in go.mod is 1.14, so I updated the code to go 1.14 compatible.
Once the version update to 1.15, we can change to use raw url encoding.

@silverwind
Copy link
Member

The = in code encoded to %3D and may cause some error.

It is the correct encoding and I'd say this is a client-side error generally. Still probably fine to trim off the base64 padding as it is optional. A test would be nice.

@silverwind
Copy link
Member

silverwind commented May 4, 2021

On second thought, I think this function should not generate non-alphanumeric characters in first place as they all can cause issues in URLs. The RFC4648 base64 variant that golang uses can produce +, / and = characters.

I think we should just generate alphanumeric only, e.g. [a-zA-Z0-9] to avoid all such issues. See https://stackoverflow.com/questions/22892120 for some options on how to do it.

@lunny
Copy link
Member

lunny commented May 5, 2021

So how about to use base64.RawURLEncoding?

@zeripath
Copy link
Contributor

zeripath commented May 5, 2021

So how about to use base64.RawURLEncoding?

#15536 (comment)

Current project 's minimal support Golang version defined in go.mod is 1.14, so I updated the code to go 1.14 compatible.
Once the version update to 1.15, we can change to use raw url encoding.

@silverwind
Copy link
Member

Alternative, more extensive PR: #15741

silverwind added a commit to silverwind/gitea that referenced this pull request May 6, 2021
- Replace 3 functions that do the same with 1 shared one
- Use crypto/rand over math/rand for a stronger RNG
- Output only alphanumerical for URL compatibilty

Fixes: go-gitea#15536
zeripath pushed a commit that referenced this pull request May 10, 2021
* Use single shared random string generation function

- Replace 3 functions that do the same with 1 shared one
- Use crypto/rand over math/rand for a stronger RNG
- Output only alphanumerical for URL compatibilty

Fixes: #15536

* use const string method

* Update modules/avatar/avatar.go

Co-authored-by: a1012112796 <1012112796@qq.com>

Co-authored-by: a1012112796 <1012112796@qq.com>
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 10, 2021
* Use single shared random string generation function

- Replace 3 functions that do the same with 1 shared one
- Use crypto/rand over math/rand for a stronger RNG
- Output only alphanumerical for URL compatibilty

Fixes: go-gitea#15536

* use const string method

* Update modules/avatar/avatar.go

Co-authored-by: a1012112796 <1012112796@qq.com>

Co-authored-by: a1012112796 <1012112796@qq.com>
@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants