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.

Sign up for GitHub

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

Jump to bottom

Handle base64 decoding correctly to avoid panic #26483

Merged
merged 3 commits into from
Aug 14, 2023
Merged

Handle base64 decoding correctly to avoid panic #26483

merged 3 commits into from
Aug 14, 2023

Conversation

wxiaoguang
Copy link
Contributor

Fix the panic if the "base64 secret" is too long.

cmd/web.go:212:func1() [F] PANIC: runtime error: index out of range [2] with length 2
/usr/lib/go/src/runtime/panic.go:114 (0x56498b06155b)
        goPanicIndex: }
/usr/lib/go/src/encoding/base64/base64.go:378 (0x56498b21ee58)
        (*Encoding).decodeQuantum: case 3:
/usr/lib/go/src/encoding/base64/base64.go:546 (0x56498b21fd3e)
        (*Encoding).Decode: return n, err
/build/gitea/src/gitea/modules/setting/oauth2.go:129 (0x56498b689519)
/build/gitea/src/gitea/modules/setting/setting.go:123 (0x56498b6942f3)
/build/gitea/src/gitea/modules/setting/setting.go:108 (0x56498b694224)

@wxiaoguang wxiaoguang added this to the 1.21.0 milestone Aug 14, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 14, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 14, 2023
@techknowlogick
Copy link
Member

Could you add a test case?

@wxiaoguang
Copy link
Contributor Author

Isn't there already one?

image

@techknowlogick
Copy link
Member

@wxiaoguang ah yes, you are right. I tried to get one review in before sleep, but clearly I should've waited until tomorrow ;)

@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 Aug 14, 2023
@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 14, 2023
@wxiaoguang wxiaoguang enabled auto-merge (squash) August 14, 2023 09:50
@wxiaoguang wxiaoguang merged commit ed1be4c into go-gitea:main Aug 14, 2023
23 checks passed
@Zulgrib
Copy link

Zulgrib commented Aug 14, 2023

Thank you @wxiaoguang for your help yesterday, and for the fix!

@wxiaoguang wxiaoguang deleted the fix-base64-panic branch August 14, 2023 10:58
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 15, 2023
@zjjhot
Merge remote-tracking branch 'upstream/main'
05f701b 
* upstream/main:
  Use `object-fit: contain` for oauth2 custom icons (go-gitea#26493)
  add disable workflow feature (go-gitea#26413)
  Move dropzone progress bar to bottom to show filename when uploading (go-gitea#26492)
  Handle base64 decoding correctly to avoid panic (go-gitea#26483)
  Allow to archive labels (go-gitea#26478)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Nov 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@delvh delvh delvh approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/bug
Projects
None yet
Milestone
1.21.0
Development

Successfully merging this pull request may close these issues.
6 participants
@wxiaoguang @techknowlogick @Zulgrib @lunny @delvh @GiteaBot