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

When redirecting URLs in Oauth contain query parameters, there is an issue with correctly detecting whether the URL is legal or not. #30336

Closed
wants to merge 1 commit into from

Conversation

mei-rune
Copy link
Contributor

@mei-rune mei-rune commented Apr 8, 2024

fix #26897

修复 Oauth 中重定向 url 中包含 query 参数时,不能正确地检测该 url 是否法的问题
When redirecting URLs in Oauth contain query parameters, there is an issue with correctly detecting whether the URL is legal or not.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 8, 2024
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 8, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Apr 8, 2024
@mei-rune mei-rune changed the title fix https://github.com/go-gitea/gitea/issues/26897 fix #26897 Apr 8, 2024
@silverwind
Copy link
Member

Please add a descriptive PR title.

@mei-rune mei-rune changed the title fix #26897 When redirecting URLs in Oauth contain query parameters, there is an issue with correctly detecting whether the URL is legal or not. Apr 8, 2024
lunny
lunny previously approved these changes Apr 10, 2024
@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 Apr 10, 2024
@lunny
Copy link
Member

lunny commented Apr 10, 2024

It's better to have a test for the method.

@lunny lunny dismissed their stale review April 13, 2024 06:20

replaced by #30457

@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 13, 2024
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 14, 2024

Thank you for your PR, but we shouldn't violate the standards.

	// OAuth2 requires the redirect URI to be an exact match, no dynamic parts are allowed.
	// https://stackoverflow.com/questions/55524480/should-dynamic-query-parameters-be-present-in-the-redirection-uri-for-an-oauth2
	// https://www.rfc-editor.org/rfc/rfc6819#section-5.2.3.3
	// https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest
	// https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics-12#section-3.1

@wxiaoguang wxiaoguang closed this Apr 14, 2024
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jul 14, 2024
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. modifies/go Pull requests that update Go code size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OAuth2 Application redirect URL with https, but it becomes contains some 'redirect' query params in url.
5 participants