From 8f88817c00d3cf37b1842b175f27fb9af2b29be2 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Wed, 5 Jun 2024 17:50:15 +0200 Subject: [PATCH 1/3] test(oauth): RFC 6749 Section 10.2 conformance See: 1b088fade6c69e63843d1bdf402454c363b22ce2 Prevent automatic OAuth grants for public clients 07fe5a8b1347bf62b3507c87d28b95ef58d6a162 use existing oauth grant for public client (cherry picked from commit 592469464b74d3cd97d4c9ec0ee9dd392bb5b426) --- models/fixtures/oauth2_application.yml | 2 +- tests/integration/oauth_test.go | 61 +++++++++++++++++++++++++- 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/models/fixtures/oauth2_application.yml b/models/fixtures/oauth2_application.yml index 2f38cb58b6..beae9137ad 100644 --- a/models/fixtures/oauth2_application.yml +++ b/models/fixtures/oauth2_application.yml @@ -14,7 +14,7 @@ name: "Test native app" client_id: "ce5a1322-42a7-11ed-b878-0242ac120002" client_secret: "$2a$10$UYRgUSgekzBp6hYe8pAdc.cgB4Gn06QRKsORUnIYTYQADs.YR/uvi" # bcrypt of "4MK8Na6R55smdCY0WuCCumZ6hjRPnGY5saWVRHHjJiA= - redirect_uris: '["http://127.0.0.1"]' + redirect_uris: '["b", "http://127.0.0.1"]' created_unix: 1546869730 updated_unix: 1546869730 confidential_client: false diff --git a/tests/integration/oauth_test.go b/tests/integration/oauth_test.go index cffb8b5814..6d2ff8b1d3 100644 --- a/tests/integration/oauth_test.go +++ b/tests/integration/oauth_test.go @@ -79,6 +79,65 @@ func TestAuthorizeShow(t *testing.T) { htmlDoc.GetCSRF() } +func TestOAuth_AuthorizeConfidentialTwice(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + // da7da3ba-9a13-4167-856f-3899de0b0138 a confidential client in models/fixtures/oauth2_application.yml + + // request authorization for the first time shows the grant page ... + authorizeURL := "/login/oauth/authorize?client_id=da7da3ba-9a13-4167-856f-3899de0b0138&redirect_uri=a&response_type=code&state=thestate" + req := NewRequest(t, "GET", authorizeURL) + ctx := loginUser(t, "user4") + resp := ctx.MakeRequest(t, req, http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + htmlDoc.AssertElement(t, "#authorize-app", true) + + // ... and the user grants the authorization + req = NewRequestWithValues(t, "POST", "/login/oauth/grant", map[string]string{ + "_csrf": htmlDoc.GetCSRF(), + "client_id": "da7da3ba-9a13-4167-856f-3899de0b0138", + "redirect_uri": "a", + "state": "thestate", + "granted": "true", + }) + resp = ctx.MakeRequest(t, req, http.StatusSeeOther) + assert.Contains(t, test.RedirectURL(resp), "code=") + + // request authorization the second time and the grant page is not shown again, redirection happens immediately + req = NewRequest(t, "GET", authorizeURL) + resp = ctx.MakeRequest(t, req, http.StatusSeeOther) + assert.Contains(t, test.RedirectURL(resp), "code=") +} + +func TestOAuth_AuthorizePublicTwice(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + // ce5a1322-42a7-11ed-b878-0242ac120002 is a public client in models/fixtures/oauth2_application.yml + authorizeURL := "/login/oauth/authorize?client_id=ce5a1322-42a7-11ed-b878-0242ac120002&redirect_uri=b&response_type=code&code_challenge_method=plain&code_challenge=CODE&state=thestate" + ctx := loginUser(t, "user4") + // a public client must be authorized every time + for _, name := range []string{"First", "Second"} { + t.Run(name, func(t *testing.T) { + req := NewRequest(t, "GET", authorizeURL) + resp := ctx.MakeRequest(t, req, http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + htmlDoc.AssertElement(t, "#authorize-app", true) + + req = NewRequestWithValues(t, "POST", "/login/oauth/grant", map[string]string{ + "_csrf": htmlDoc.GetCSRF(), + "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", + "redirect_uri": "b", + "state": "thestate", + "granted": "true", + }) + resp = ctx.MakeRequest(t, req, http.StatusSeeOther) + assert.Contains(t, test.RedirectURL(resp), "code=") + }) + } +} + func TestAuthorizeRedirectWithExistingGrant(t *testing.T) { defer tests.PrepareTestEnv(t)() req := NewRequest(t, "GET", "/login/oauth/authorize?client_id=da7da3ba-9a13-4167-856f-3899de0b0138&redirect_uri=https%3A%2F%2Fexample.com%2Fxyzzy&response_type=code&state=thestate") @@ -480,7 +539,7 @@ func TestSignInOAuthCallbackRedirectToEscaping(t *testing.T) { gitlab := addAuthSource(t, authSourcePayloadGitLabCustom(gitlabName)) // - // Create a user as if it had been previously been created by the GitLab + // Create a user as if it had been previously created by the GitLab // authentication source. // userGitLabUserID := "5678" From a228ab3ab26d80c28a8d40f7c92b8ba0e5cb42d3 Mon Sep 17 00:00:00 2001 From: Archer Date: Thu, 2 May 2024 19:05:59 +0200 Subject: [PATCH 2/3] Prevent automatic OAuth grants for public clients (#30790) This commit forces the resource owner (user) to always approve OAuth 2.0 authorization requests if the client is public (e.g. native applications). As detailed in [RFC 6749 Section 10.2](https://www.rfc-editor.org/rfc/rfc6749.html#section-10.2), > The authorization server SHOULD NOT process repeated authorization requests automatically (without active resource owner interaction) without authenticating the client or relying on other measures to ensure that the repeated request comes from the original client and not an impersonator. With the implementation prior to this patch, attackers with access to the redirect URI (e.g., the loopback interface for `git-credential-oauth`) can get access to the user account without any user interaction if they can redirect the user to the `/login/oauth/authorize` endpoint somehow (e.g., with `xdg-open` on Linux). Fixes #25061. Co-authored-by: wxiaoguang (cherry picked from commit 5c542ca94caa3587329167cfe9e949357ca15cf1) (cherry picked from commit 1b088fade6c69e63843d1bdf402454c363b22ce2) --- routers/web/auth/oauth.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index b2d2abcf26..e4076225d1 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -470,8 +470,9 @@ func AuthorizeOAuth(ctx *context.Context) { return } - // Redirect if user already granted access - if grant != nil { + // Redirect if user already granted access and the application is confidential. + // I.e. always require authorization for public clients as recommended by RFC 6749 Section 10.2 + if app.ConfidentialClient && grant != nil { code, err := grant.GenerateNewAuthorizationCode(ctx, form.RedirectURI, form.CodeChallenge, form.CodeChallengeMethod) if err != nil { handleServerError(ctx, form.State, form.RedirectURI) From 0c770d528f5a1c7c40d6c0bd1baed5cf73026844 Mon Sep 17 00:00:00 2001 From: Denys Konovalov Date: Tue, 21 May 2024 18:23:49 +0200 Subject: [PATCH 3/3] use existing oauth grant for public client (#31015) Do not try to create a new authorization grant when one exists already, thus preventing a DB-related authorization issue. Fix https://github.com/go-gitea/gitea/pull/30790#issuecomment-2118812426 --------- Co-authored-by: Lunny Xiao (cherry picked from commit 9c8c9ff6d10b35de8d2d7eae0fc2646ad9bbe94a) (cherry picked from commit 07fe5a8b1347bf62b3507c87d28b95ef58d6a162) --- routers/web/auth/oauth.go | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index e4076225d1..79751450b5 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -556,15 +556,30 @@ func GrantApplicationOAuth(ctx *context.Context) { ctx.ServerError("GetOAuth2ApplicationByClientID", err) return } - grant, err := app.CreateGrant(ctx, ctx.Doer.ID, form.Scope) + grant, err := app.GetGrantByUserID(ctx, ctx.Doer.ID) if err != nil { + handleServerError(ctx, form.State, form.RedirectURI) + return + } + if grant == nil { + grant, err = app.CreateGrant(ctx, ctx.Doer.ID, form.Scope) + if err != nil { + handleAuthorizeError(ctx, AuthorizeError{ + State: form.State, + ErrorDescription: "cannot create grant for user", + ErrorCode: ErrorCodeServerError, + }, form.RedirectURI) + return + } + } else if grant.Scope != form.Scope { handleAuthorizeError(ctx, AuthorizeError{ State: form.State, - ErrorDescription: "cannot create grant for user", + ErrorDescription: "a grant exists with different scope", ErrorCode: ErrorCodeServerError, }, form.RedirectURI) return } + if len(form.Nonce) > 0 { err := grant.SetNonce(ctx, form.Nonce) if err != nil {