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

Make CORS work for oauth2 handlers #28184

Merged
merged 1 commit into from Nov 23, 2023
Merged

Conversation

wxiaoguang
Copy link
Contributor

Fix #25473

Although there was m.Post("/login/oauth/access_token", CorsHandler() ... ,it never really worked, because it still lacks the "OPTIONS" handler.

After the fix:

$ curl -v -X OPTIONS --header "Access-Control-Request-Method: POST" http://localhost:3000/login/oauth/access_token
*   Trying 127.0.0.1:3000...
> OPTIONS /login/oauth/access_token HTTP/1.1
> Accept: */*
> Access-Control-Request-Method: POST
>
< HTTP/1.1 200 OK
< Vary: Origin
< Vary: Access-Control-Request-Method
< Vary: Access-Control-Request-Headers
< X-Frame-Options: SAMEORIGIN
< X-Gitea-Debug: RUN_MODE=dev
< Date: Thu, 23 Nov 2023 11:45:36 GMT
< Content-Length: 0



$ curl -v -X OPTIONS http://localhost:3000/login/oauth/access_token
> OPTIONS /login/oauth/access_token HTTP/1.1
> Accept: */*
>
< HTTP/1.1 400 Bad Request
< Vary: Origin
< X-Frame-Options: SAMEORIGIN
< X-Gitea-Debug: RUN_MODE=dev
< Date: Thu, 23 Nov 2023 11:45:32 GMT
< Content-Length: 0

ps: I haven't fully tested the "oauth2 cors client", we can make further fixes if there are still problems.

@wxiaoguang wxiaoguang added type/bug backport/v1.21 This PR should be backported to Gitea 1.21 labels Nov 23, 2023
@wxiaoguang wxiaoguang added this to the 1.22.0 milestone Nov 23, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 23, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 23, 2023
@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 23, 2023
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand, why do we need these routes?
Are they required by OAuth?

@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 Nov 23, 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 Nov 23, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Nov 23, 2023
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Nov 23, 2023

I don't quite understand, why do we need these routes?

Because the "OPTIONS" is a separate request before "POST" (for CORS)

@wxiaoguang wxiaoguang merged commit 7c0ab8b into go-gitea:main Nov 23, 2023
25 checks passed
@wxiaoguang wxiaoguang deleted the fix-oauth-cors branch November 23, 2023 13:19
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Nov 23, 2023
Fix go-gitea#25473

Although there was `m.Post("/login/oauth/access_token", CorsHandler()...`,
it never really worked, because it still lacks the "OPTIONS" handler.
@GiteaBot GiteaBot added backport/done All backports for this PR have been created and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Nov 23, 2023
wxiaoguang added a commit that referenced this pull request Nov 23, 2023
Backport #28184

Fix #25473

Although there was `m.Post("/login/oauth/access_token", CorsHandler()...`,
it never really worked, because it still lacks the "OPTIONS" handler.

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Nov 24, 2023
* giteaofficial/main:
  Use db.Find instead of writing methods for every object (go-gitea#28084)
  Edit Discord Badge (go-gitea#28188)
  Use restricted sanitizer for repository description (go-gitea#28141)
  Use full width for PR comparison (go-gitea#28182)
  Make CORS work for oauth2 handlers (go-gitea#28184)
  Fix missing buttons (go-gitea#28179)
@morphelinho
Copy link
Contributor

Still observing a similar problem:

Request URL: https://gitea-instance/login/oauth/userinfo
Request Method: OPTIONS
Status Code: 405 Method Not Allowed

Server side log:
2023/12/18 20:52:13 cmd/web.go:111:showWebStartupMessage() [I] Gitea version: 1.21.2 built with GNU Make 4.4.1, go1.21.5 : bindata, timetzdata, sqlite, sqlite_unlock_notify
...
2023/12/18 21:11:36 ...eb/routing/logger.go:102:func1() [I] router: completed OPTIONS /login/oauth/userinfo for XXX.XXX.XXX.XXX, 405 Method Not Allowed in 0.2ms @ web/goget.go:20(web.goGet)

Any help would be appreciated.

@wxiaoguang
Copy link
Contributor Author

Because /login/oauth/userinfo was never covered by CORS.

This PR only fixes the access_token handler.

It could simply add a new line m.Options("/login/oauth/userinfo", CorsHandler(), misc.DummyBadRequest) to make userinfo work, you can try to build your own binary to see whether it works well (I don't use it so my test doesn't mean it really works as your expectation)

morphelinho added a commit to morphelinho/gitea that referenced this pull request Dec 22, 2023
wxiaoguang pushed a commit that referenced this pull request Dec 22, 2023
Follow #28184
Follow #28515

Fix problem with 405 method not allowed for CORS wrt OIDC
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Dec 22, 2023
Follow go-gitea#28184
Follow go-gitea#28515

Fix problem with 405 method not allowed for CORS wrt OIDC
lunny pushed a commit that referenced this pull request Dec 22, 2023
Backport #28583 by @morphelinho

Follow #28184
Follow #28515

Fix problem with 405 method not allowed for CORS wrt OIDC

Co-authored-by: morphelinho <morphelinho@users.noreply.github.com>
techknowlogick pushed a commit to techknowlogick/gitea that referenced this pull request Dec 23, 2023
Follow go-gitea#28184
Follow go-gitea#28515

Fix problem with 405 method not allowed for CORS wrt OIDC
fuxiaohei pushed a commit to fuxiaohei/gitea that referenced this pull request Jan 17, 2024
Fix go-gitea#25473

Although there was `m.Post("/login/oauth/access_token", CorsHandler()...`,
it never really worked, because it still lacks the "OPTIONS" handler.
fuxiaohei pushed a commit to fuxiaohei/gitea that referenced this pull request Jan 17, 2024
Follow go-gitea#28184
Follow go-gitea#28515

Fix problem with 405 method not allowed for CORS wrt OIDC
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
Fix go-gitea#25473

Although there was `m.Post("/login/oauth/access_token", CorsHandler()...`,
it never really worked, because it still lacks the "OPTIONS" handler.
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
Follow go-gitea#28184
Follow go-gitea#28515

Fix problem with 405 method not allowed for CORS wrt OIDC
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Feb 21, 2024
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 backport/v1.21 This PR should be backported to Gitea 1.21 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OAuth2 OPTIONS /login/oauth/userinfo 405
5 participants