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

Fix panic when an invalid oauth2 name is passed #20820

Merged
merged 2 commits into from
Aug 17, 2022

Conversation

balki
Copy link
Contributor

@balki balki commented Aug 17, 2022

When trying to access an invalid oauth2 link, we get an internal server error and can see a panic stack-trace in logs

Example:
Try to go to this url for a gitea installation
https://<gitea_url>/user/oauth2/DoesNotExist?redirect_to=

It causes an internal server error

Stack trace in log

2022/08/17 01:26:50 routers/web/base.go:134:1() [E] [62fc43da] PANIC: runtime error: invalid memory address or nil pointer dereference
        /usr/local/go/src/runtime/panic.go:220 (0x453095)
        /usr/local/go/src/runtime/signal_unix.go:818 (0x453065)
        /source/routers/web/auth/oauth.go:1100 (0x20f6ef7)
        /source/routers/web/auth/oauth.go:785 (0x20f4684)
        /source/modules/web/wrap_convert.go:47 (0x1f45196)
        /source/modules/web/wrap.go:41 (0x1f433c9)
        /usr/local/go/src/net/http/server.go:2084 (0x93cace)
       <clipped>

Root cause:

In this line here, err is nil. The caller assumes no error and tries to access a nil *Source

models/auth/oauth2.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 17, 2022
@balki balki requested a review from lunny August 17, 2022 02:13
@codecov-commenter
Copy link

Codecov Report

Merging #20820 (9c4da36) into main (efaa995) will decrease coverage by 0.01%.
The diff coverage is 17.64%.

@@            Coverage Diff             @@
##             main   #20820      +/-   ##
==========================================
- Coverage   47.06%   47.05%   -0.02%     
==========================================
  Files         987      988       +1     
  Lines      136398   136368      -30     
==========================================
- Hits        64201    64171      -30     
- Misses      64316    64317       +1     
+ Partials     7881     7880       -1     
Impacted Files Coverage Δ
models/auth/oauth2.go 59.56% <0.00%> (-0.56%) ⬇️
models/db/iterate.go 0.00% <0.00%> (ø)
models/git/lfs.go 25.90% <ø> (+2.53%) ⬆️
models/repo/attachment.go 53.38% <ø> (+6.72%) ⬆️
models/repo/repo_list.go 78.80% <ø> (+3.13%) ⬆️
models/user/search.go 87.83% <ø> (+16.40%) ⬆️
modules/hostmatcher/hostmatcher.go 85.22% <0.00%> (-1.99%) ⬇️
routers/private/mail.go 0.00% <0.00%> (ø)
routers/web/admin/admin.go 10.00% <0.00%> (-0.03%) ⬇️
services/auth/reverseproxy.go 0.00% <0.00%> (ø)
... and 14 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@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 17, 2022
@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 17, 2022
@techknowlogick techknowlogick merged commit c138e76 into go-gitea:main Aug 17, 2022
@balki balki deleted the patch-1 branch August 17, 2022 21:30
@zeripath
Copy link
Contributor

Please send backport

@lunny lunny added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Aug 18, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 18, 2022
* giteaofficial/main:
  Fix migration file name (go-gitea#20843)
  Check Mirror exists before linking its Repo (go-gitea#20840)
  [skip ci] Updated translations via Crowdin
  Add badge capabilities to users (go-gitea#20607)
  docs[zh-cn]: Managing Deployments With Environment Variables (go-gitea#20817)
  Correctly escape within tribute.js (go-gitea#20831)
  Fix panic when an invalid oauth2 name is passed (go-gitea#20820)
  Use the total issue count for UI (go-gitea#20785)
zeripath pushed a commit to zeripath/gitea that referenced this pull request Aug 21, 2022
Backport go-gitea#20820

When trying to access an invalid oauth2 link, we get an internal server error and can see a panic stack-trace in logs

Example:
Try to go to this url for a gitea installation
https://<gitea_url>/user/oauth2/DoesNotExist?redirect_to=

It causes an internal server error

Stack trace in log

```
2022/08/17 01:26:50 routers/web/base.go:134:1() [E] [62fc43da] PANIC: runtime error: invalid memory address or nil pointer dereference
        /usr/local/go/src/runtime/panic.go:220 (0x453095)
        /usr/local/go/src/runtime/signal_unix.go:818 (0x453065)
        /source/routers/web/auth/oauth.go:1100 (0x20f6ef7)
        /source/routers/web/auth/oauth.go:785 (0x20f4684)
        /source/modules/web/wrap_convert.go:47 (0x1f45196)
        /source/modules/web/wrap.go:41 (0x1f433c9)
        /usr/local/go/src/net/http/server.go:2084 (0x93cace)
       <clipped>
```

Root cause:

In this [line](https://github.com/go-gitea/gitea/blob/a4e91c4197483c94f13e623c962b6b011494e949/models/auth/oauth2.go#L516) here, err is nil. The caller assumes no error and tries to access a `nil *Source`
@zeripath zeripath added the backport/done All backports for this PR have been created label Aug 21, 2022
vsysoev pushed a commit to IntegraSDL/gitea that referenced this pull request Aug 28, 2022
@wxiaoguang wxiaoguang added this to the 1.18.0 milestone Oct 7, 2022
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
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 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants