Skip to content

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

@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