From d1c7b04d09f6a13896eaa1322ac690b2021539da Mon Sep 17 00:00:00 2001 From: Gusted Date: Mon, 2 Mar 2026 01:37:10 +0100 Subject: [PATCH 01/17] fix: verify code challenge of S256 We do not know for sure, but it is quite likely someone assumed implicit fallthrough. This meant that if someone used S256 for PKCE, it simply did not verify the code challenge and always accepted it. PKCE only started working recently as it was broken for a long time already, forgejo/forgejo!8678 --- routers/web/auth/oauth.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index 1fd4a0311b..8e8ede0008 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -480,8 +480,7 @@ func AuthorizeOAuth(ctx *context.Context) { // pkce support switch form.CodeChallengeMethod { - case "S256": - case "plain": + case "S256", "plain": if err := ctx.Session.Set("CodeChallengeMethod", form.CodeChallengeMethod); err != nil { handleAuthorizeError(ctx, AuthorizeError{ ErrorCode: ErrorCodeServerError, From faf448f35077957fa0b10470a4bb00e6de3daf47 Mon Sep 17 00:00:00 2001 From: Gusted Date: Mon, 2 Mar 2026 01:40:26 +0100 Subject: [PATCH 02/17] chore: add integration testing Build on the test of forgejo/forgejo!8678 to also verify PKCE works for S256. --- tests/integration/oauth_test.go | 153 +++++++++++++++++++++++--------- 1 file changed, 109 insertions(+), 44 deletions(-) diff --git a/tests/integration/oauth_test.go b/tests/integration/oauth_test.go index 9220f03ccd..21a1ff561f 100644 --- a/tests/integration/oauth_test.go +++ b/tests/integration/oauth_test.go @@ -1535,62 +1535,127 @@ func TestSignUpViaOAuth2FA(t *testing.T) { func TestAccessTokenWithPKCE(t *testing.T) { defer tests.PrepareTestEnv(t)() - var u *url.URL - t.Run("Grant", func(t *testing.T) { - session := loginUser(t, "user4") + session := loginUser(t, "user4") - session.MakeRequest(t, NewRequest(t, "GET", "/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"), http.StatusOK) - req := NewRequestWithValues(t, "POST", "/login/oauth/grant", map[string]string{ - "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", - "redirect_uri": "b", - "state": "thestate", - "granted": "true", + t.Run("Plain method", func(t *testing.T) { + defer unittest.AssertSuccessfulDelete(t, &auth_model.OAuth2Grant{UserID: 4, ApplicationID: 2}) + + var u *url.URL + t.Run("Grant", func(t *testing.T) { + session.MakeRequest(t, NewRequest(t, "GET", "/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"), http.StatusOK) + req := NewRequestWithValues(t, "POST", "/login/oauth/grant", map[string]string{ + "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", + "redirect_uri": "b", + "state": "thestate", + "granted": "true", + }) + resp := session.MakeRequest(t, req, http.StatusSeeOther) + + var err error + u, err = url.Parse(test.RedirectURL(resp)) + require.NoError(t, err) }) - resp := session.MakeRequest(t, req, http.StatusSeeOther) - var err error - u, err = url.Parse(test.RedirectURL(resp)) - require.NoError(t, err) + t.Run("Incorrect code verifier", func(t *testing.T) { + req := NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ + "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", + "code": u.Query().Get("code"), + "code_verifier": "just a guess", + "grant_type": "authorization_code", + "redirect_uri": "b", + }) + resp := MakeRequest(t, req, http.StatusBadRequest) + + var respBody map[string]any + DecodeJSON(t, resp, &respBody) + + if assert.Len(t, respBody, 2) { + assert.Equal(t, "unauthorized_client", respBody["error"]) + assert.Equal(t, "failed PKCE code challenge", respBody["error_description"]) + } + }) + + t.Run("Get access token", func(t *testing.T) { + req := NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ + "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", + "code": u.Query().Get("code"), + "code_verifier": "CODE", + "grant_type": "authorization_code", + "redirect_uri": "b", + }) + resp := MakeRequest(t, req, http.StatusOK) + + var respBody map[string]any + DecodeJSON(t, resp, &respBody) + + if assert.Len(t, respBody, 4) { + assert.NotEmpty(t, respBody["access_token"]) + assert.NotEmpty(t, respBody["token_type"]) + assert.NotEmpty(t, respBody["expires_in"]) + assert.NotEmpty(t, respBody["refresh_token"]) + } + }) }) - t.Run("Incorrect code verifier", func(t *testing.T) { - req := NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ - "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", - "code": u.Query().Get("code"), - "code_verifier": "just a guess", - "grant_type": "authorization_code", - "redirect_uri": "b", + t.Run("S256 method", func(t *testing.T) { + var u *url.URL + t.Run("Grant", func(t *testing.T) { + h := sha256.Sum256([]byte("CODE")) + hashedVerifier := base64.RawURLEncoding.EncodeToString(h[:]) + + session.MakeRequest(t, NewRequest(t, "GET", "/login/oauth/authorize?client_id=ce5a1322-42a7-11ed-b878-0242ac120002&redirect_uri=b&response_type=code&code_challenge_method=S256&code_challenge="+hashedVerifier+"&state=thestate"), http.StatusOK) + req := NewRequestWithValues(t, "POST", "/login/oauth/grant", map[string]string{ + "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", + "redirect_uri": "b", + "state": "thestate", + "granted": "true", + }) + resp := session.MakeRequest(t, req, http.StatusSeeOther) + + var err error + u, err = url.Parse(test.RedirectURL(resp)) + require.NoError(t, err) }) - resp := MakeRequest(t, req, http.StatusBadRequest) - var respBody map[string]any - DecodeJSON(t, resp, &respBody) + t.Run("Incorrect code verifier", func(t *testing.T) { + req := NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ + "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", + "code": u.Query().Get("code"), + "code_verifier": "just a guess", + "grant_type": "authorization_code", + "redirect_uri": "b", + }) + resp := MakeRequest(t, req, http.StatusBadRequest) - if assert.Len(t, respBody, 2) { - assert.Equal(t, "unauthorized_client", respBody["error"]) - assert.Equal(t, "failed PKCE code challenge", respBody["error_description"]) - } - }) + var respBody map[string]any + DecodeJSON(t, resp, &respBody) - t.Run("Get access token", func(t *testing.T) { - req := NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ - "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", - "code": u.Query().Get("code"), - "code_verifier": "CODE", - "grant_type": "authorization_code", - "redirect_uri": "b", + if assert.Len(t, respBody, 2) { + assert.Equal(t, "unauthorized_client", respBody["error"]) + assert.Equal(t, "failed PKCE code challenge", respBody["error_description"]) + } }) - resp := MakeRequest(t, req, http.StatusOK) - var respBody map[string]any - DecodeJSON(t, resp, &respBody) + t.Run("Get access token", func(t *testing.T) { + req := NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ + "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", + "code": u.Query().Get("code"), + "code_verifier": "CODE", + "grant_type": "authorization_code", + "redirect_uri": "b", + }) + resp := MakeRequest(t, req, http.StatusOK) - if assert.Len(t, respBody, 4) { - assert.NotEmpty(t, respBody["access_token"]) - assert.NotEmpty(t, respBody["token_type"]) - assert.NotEmpty(t, respBody["expires_in"]) - assert.NotEmpty(t, respBody["refresh_token"]) - } + var respBody map[string]any + DecodeJSON(t, resp, &respBody) + + if assert.Len(t, respBody, 4) { + assert.NotEmpty(t, respBody["access_token"]) + assert.NotEmpty(t, respBody["token_type"]) + assert.NotEmpty(t, respBody["expires_in"]) + assert.NotEmpty(t, respBody["refresh_token"]) + } + }) }) } From da766f1e199b8f19e71f8063c233d37d188fde12 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sun, 1 Mar 2026 23:32:33 +0100 Subject: [PATCH 03/17] fix: consider scopes for OAuth2 token via basic login There are two ways to use a OAuth2 token: Via the Authorization header as a Bearer token. Via the Authorization header as a Basic login. For the former the scope was correctly passed through, for the latter it was not and would mean no scope was checked if you used the token via this way. --- services/auth/basic.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/services/auth/basic.go b/services/auth/basic.go index 4125c914a9..502d806a12 100644 --- a/services/auth/basic.go +++ b/services/auth/basic.go @@ -73,7 +73,7 @@ func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore } // check oauth2 token - uid, _ := CheckOAuthAccessToken(req.Context(), authToken) + uid, grantScopes := CheckOAuthAccessToken(req.Context(), authToken) if uid != 0 { log.Trace("Basic Authorization: Valid OAuthAccessToken for user[%d]", uid) @@ -84,6 +84,11 @@ func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore } store.GetData()["IsApiToken"] = true + if grantScopes != "" { + store.GetData()["ApiTokenScope"] = auth_model.AccessTokenScope(grantScopes) + } else { + store.GetData()["ApiTokenScope"] = auth_model.AccessTokenScopeAll // fallback to all + } return u, nil } From 68b2930caad9ca7bb5993b9ebea140da75e4d5a2 Mon Sep 17 00:00:00 2001 From: Gusted Date: Mon, 2 Mar 2026 00:48:04 +0100 Subject: [PATCH 04/17] chore: add integration testing Modify the existing scopes tests for normal API tokens to OAuth2 tokens. --- tests/integration/api_token_test.go | 106 +++++++++++++++++++--------- 1 file changed, 72 insertions(+), 34 deletions(-) diff --git a/tests/integration/api_token_test.go b/tests/integration/api_token_test.go index 2beadfacc2..19946afce8 100644 --- a/tests/integration/api_token_test.go +++ b/tests/integration/api_token_test.go @@ -8,6 +8,7 @@ import ( "fmt" "net/http" "net/url" + "strings" "testing" auth_model "forgejo.org/models/auth" @@ -541,13 +542,29 @@ func runTestCase(t *testing.T, testCase *requiredScopeTestCase, user *user_model unauthorizedScopes = append(unauthorizedScopes, categoryUnauthorizedScopes...) } - accessToken := createAPIAccessTokenWithoutCleanUp(t, "test-token", user, unauthorizedScopes) - defer deleteAPIAccessToken(t, accessToken, user) - // Request the endpoint. Verify that permission is denied. - req := NewRequest(t, testCase.method, testCase.url). - AddTokenAuth(accessToken.Token) - MakeRequest(t, req, http.StatusForbidden) + + t.Run("Bearer", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + accessToken := createAPIAccessTokenWithoutCleanUp(t, "test-token", user, unauthorizedScopes) + defer deleteAPIAccessToken(t, accessToken, user) + + req := NewRequest(t, testCase.method, testCase.url). + AddTokenAuth(accessToken.Token) + MakeRequest(t, req, http.StatusForbidden) + }) + + t.Run("Basic", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + oauth2Token := createOAuth2Token(t, loginUser(t, user.Name), unauthorizedScopes) + defer unittest.AssertSuccessfulDelete(t, &auth_model.OAuth2Grant{ApplicationID: 2, UserID: user.ID}) + + req := NewRequest(t, testCase.method, testCase.url) + req.SetBasicAuth("x-oauth-basic", oauth2Token) + MakeRequest(t, req, http.StatusForbidden) + }) }) } @@ -585,6 +602,51 @@ func deleteAPIAccessToken(t *testing.T, accessToken api.AccessToken, user *user_ unittest.AssertNotExistsBean(t, &auth_model.AccessToken{ID: accessToken.ID}) } +func createOAuth2Token(t *testing.T, session *TestSession, scopes []auth_model.AccessTokenScope) string { + // Make a call to `/login/oauth/authorize` to get some session data. + session.MakeRequest(t, NewRequest(t, "GET", "/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"), http.StatusOK) + + var b strings.Builder + switch len(scopes) { + case 0: + break + case 1: + b.WriteString(string(scopes[0])) + default: + b.WriteString(string(scopes[0])) + for _, s := range scopes[1:] { + b.WriteString(" ") + b.WriteString(string(s)) + } + } + + req := NewRequestWithValues(t, "POST", "/login/oauth/grant", map[string]string{ + "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", + "redirect_uri": "b", + "state": "thestate", + "granted": "true", + "scope": b.String(), + }) + resp := session.MakeRequest(t, req, http.StatusSeeOther) + + u, err := url.Parse(test.RedirectURL(resp)) + require.NoError(t, err) + + req = NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ + "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", + "code": u.Query().Get("code"), + "code_verifier": "CODE", + "grant_type": "authorization_code", + "redirect_uri": "b", + }) + resp = MakeRequest(t, req, http.StatusOK) + + var respBody map[string]any + DecodeJSON(t, resp, &respBody) + + return respBody["access_token"].(string) +} + func TestAPITokenCreation(t *testing.T) { defer tests.PrepareTestEnv(t)() @@ -611,39 +673,15 @@ func TestAPITokenCreation(t *testing.T) { t.Run("Via OAuth2", func(t *testing.T) { defer tests.PrintCurrentTest(t)() - // Make a call to `/login/oauth/authorize` to get some session data. - session.MakeRequest(t, NewRequest(t, "GET", "/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"), http.StatusOK) + accessToken := createOAuth2Token(t, session, nil) - req := NewRequestWithValues(t, "POST", "/login/oauth/grant", map[string]string{ - "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", - "redirect_uri": "b", - "state": "thestate", - "granted": "true", - }) - resp := session.MakeRequest(t, req, http.StatusSeeOther) - - u, err := url.Parse(test.RedirectURL(resp)) - require.NoError(t, err) - - req = NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ - "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", - "code": u.Query().Get("code"), - "code_verifier": "CODE", - "grant_type": "authorization_code", - "redirect_uri": "b", - }) - resp = MakeRequest(t, req, http.StatusOK) - - var respBody map[string]any - DecodeJSON(t, resp, &respBody) - - req = NewRequestWithJSON(t, "POST", "/api/v1/users/user4/tokens", map[string]any{ + req := NewRequestWithJSON(t, "POST", "/api/v1/users/user4/tokens", map[string]any{ "name": "new-new-token", "scopes": []auth_model.AccessTokenScope{auth_model.AccessTokenScopeWriteUser}, }) - req.Request.Header.Set("Authorization", "basic "+base64.StdEncoding.EncodeToString([]byte("user4:"+respBody["access_token"].(string)))) + req.SetBasicAuth("user4", accessToken) - resp = MakeRequest(t, req, http.StatusUnauthorized) + resp := MakeRequest(t, req, http.StatusUnauthorized) respMsg := map[string]any{} DecodeJSON(t, resp, &respMsg) From ce0a3767230bf0ed2a16f69fd3eb0afe0dff6168 Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 11 Feb 2026 08:16:51 +0100 Subject: [PATCH 05/17] fix: check that attachments belong to correct resource It was possible to hijack attachments during update and create functions to another owner as permissions to check they weren't already attached to another resource and wasn't checked if it belonged to the repository that was being operated on. --- models/issues/comment.go | 29 ++++---------- models/issues/issue_update.go | 22 +++-------- models/repo/attachment.go | 64 +++++++++++++++++++++++++------ models/repo/release.go | 12 ++---- routers/web/repo/issue.go | 2 +- services/attachment/attachment.go | 6 +++ services/release/release.go | 37 ++++++++---------- 7 files changed, 93 insertions(+), 79 deletions(-) diff --git a/models/issues/comment.go b/models/issues/comment.go index c0a13ef03d..325fcbe30b 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -611,9 +611,13 @@ func (c *Comment) UpdateAttachments(ctx context.Context, uuids []string) error { } defer committer.Close() - attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, uuids) + if err := c.LoadIssue(ctx); err != nil { + return fmt.Errorf("LoadIssue: %w", err) + } + + attachments, err := repo_model.FindRepoAttachmentsByUUID(ctx, c.Issue.RepoID, uuids, repo_model.FindAttachmentOptions{}) if err != nil { - return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", uuids, err) + return fmt.Errorf("FindRepoAttachmentsByUUID[uuids=%q,repoID=%d]: %w", uuids, c.Issue.RepoID, err) } for i := 0; i < len(attachments); i++ { attachments[i].IssueID = c.IssueID @@ -889,7 +893,7 @@ func updateCommentInfos(ctx context.Context, opts *CreateCommentOptions, comment // Check comment type. switch opts.Type { case CommentTypeCode: - if err = updateAttachments(ctx, opts, comment); err != nil { + if err := comment.UpdateAttachments(ctx, opts.Attachments); err != nil { return err } if comment.ReviewID != 0 { @@ -909,7 +913,7 @@ func updateCommentInfos(ctx context.Context, opts *CreateCommentOptions, comment } fallthrough case CommentTypeReview: - if err = updateAttachments(ctx, opts, comment); err != nil { + if err := comment.UpdateAttachments(ctx, opts.Attachments); err != nil { return err } case CommentTypeReopen, CommentTypeClose: @@ -921,23 +925,6 @@ func updateCommentInfos(ctx context.Context, opts *CreateCommentOptions, comment return UpdateIssueCols(ctx, opts.Issue, "updated_unix") } -func updateAttachments(ctx context.Context, opts *CreateCommentOptions, comment *Comment) error { - attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, opts.Attachments) - if err != nil { - return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", opts.Attachments, err) - } - for i := range attachments { - attachments[i].IssueID = opts.Issue.ID - attachments[i].CommentID = comment.ID - // No assign value could be 0, so ignore AllCols(). - if _, err = db.GetEngine(ctx).ID(attachments[i].ID).Update(attachments[i]); err != nil { - return fmt.Errorf("update attachment [%d]: %w", attachments[i].ID, err) - } - } - comment.Attachments = attachments - return nil -} - func createDeadlineComment(ctx context.Context, doer *user_model.User, issue *Issue, newDeadlineUnix timeutil.TimeStamp) (*Comment, error) { var content string var commentType CommentType diff --git a/models/issues/issue_update.go b/models/issues/issue_update.go index 84a8820cd1..22e6fcb8d4 100644 --- a/models/issues/issue_update.go +++ b/models/issues/issue_update.go @@ -234,18 +234,18 @@ func AddDeletePRBranchComment(ctx context.Context, doer *user_model.User, repo * } // UpdateIssueAttachments update attachments by UUIDs for the issue -func UpdateIssueAttachments(ctx context.Context, issueID int64, uuids []string) (err error) { +func UpdateIssueAttachments(ctx context.Context, issue *Issue, uuids []string) (err error) { ctx, committer, err := db.TxContext(ctx) if err != nil { return err } defer committer.Close() - attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, uuids) + attachments, err := repo_model.FindRepoAttachmentsByUUID(ctx, issue.RepoID, uuids, repo_model.FindAttachmentOptions{}) if err != nil { - return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", uuids, err) + return fmt.Errorf("FindRepoAttachmentsByUUID[uuids=%q,repoID=%d]: %w", uuids, issue.RepoID, err) } for i := 0; i < len(attachments); i++ { - attachments[i].IssueID = issueID + attachments[i].IssueID = issue.ID if err := repo_model.UpdateAttachment(ctx, attachments[i]); err != nil { return fmt.Errorf("update attachment [id: %d]: %w", attachments[i].ID, err) } @@ -394,18 +394,8 @@ func NewIssueWithIndex(ctx context.Context, doer *user_model.User, opts NewIssue return err } - if len(opts.Attachments) > 0 { - attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, opts.Attachments) - if err != nil { - return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", opts.Attachments, err) - } - - for i := 0; i < len(attachments); i++ { - attachments[i].IssueID = opts.Issue.ID - if _, err = e.ID(attachments[i].ID).Update(attachments[i]); err != nil { - return fmt.Errorf("update attachment [id: %d]: %w", attachments[i].ID, err) - } - } + if err := UpdateIssueAttachments(ctx, opts.Issue, opts.Attachments); err != nil { + return fmt.Errorf("UpdateIssueAttachments: %w", err) } if err = opts.Issue.LoadAttributes(ctx); err != nil { return err diff --git a/models/repo/attachment.go b/models/repo/attachment.go index 6d903be5f8..1b6af572dd 100644 --- a/models/repo/attachment.go +++ b/models/repo/attachment.go @@ -1,4 +1,5 @@ // Copyright 2017 The Gitea Authors. All rights reserved. +// Copyright 2026 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package repo @@ -16,17 +17,30 @@ import ( "forgejo.org/modules/timeutil" "forgejo.org/modules/util" "forgejo.org/modules/validation" + + "xorm.io/builder" ) // Attachment represent a attachment of issue/comment/release. type Attachment struct { - ID int64 `xorm:"pk autoincr"` - UUID string `xorm:"uuid UNIQUE"` - RepoID int64 `xorm:"INDEX"` // this should not be zero - IssueID int64 `xorm:"INDEX"` // maybe zero when creating - ReleaseID int64 `xorm:"INDEX"` // maybe zero when creating - UploaderID int64 `xorm:"INDEX DEFAULT 0"` // Notice: will be zero before this column added - CommentID int64 `xorm:"INDEX"` + ID int64 `xorm:"pk autoincr"` + // UUID is the public identifier of the attachment, and is used during HTTP + // requests to refer to a specific attachment. + UUID string `xorm:"uuid UNIQUE"` + // UploaderID is always set and non-zero and refers to the user that has + // uploaded this attachment. + UploaderID int64 `xorm:"INDEX DEFAULT 0"` + // RepoID is always set and non-zero and refers to the repository where this + // attachment was uploaded to. + RepoID int64 `xorm:"INDEX"` + // IssueID, ReleaseID and CommentID have multiple possible states: + // - ReleaseID != 0 && IssueID == 0 && CommentID == 0: attached to release with id `ReleaseID`. + // - ReleaseID == 0 && IssueID != 0 && CommentID == 0: attached to the issue with id `IssueID`. + // - ReleaseID == 0 && IssueID != 0 && CommentID != 0: attached to comment with id `CommentID` that is in issue with id `IssueID`. + // All other states should be considered invalid. + IssueID int64 `xorm:"INDEX"` + ReleaseID int64 `xorm:"INDEX"` + CommentID int64 `xorm:"INDEX"` Name string DownloadCount int64 `xorm:"DEFAULT 0"` Size int64 `xorm:"DEFAULT 0"` @@ -73,6 +87,12 @@ func (a *Attachment) DownloadURL() string { return setting.AppURL + "attachments/" + url.PathEscape(a.UUID) } +// IsAttachedToResource returns true if this attachment is attached to a release, +// issue or comment. +func (a *Attachment) IsAttachedToResource() bool { + return a.ReleaseID != 0 || a.IssueID != 0 || a.CommentID != 0 +} + // ErrAttachmentNotExist represents a "AttachmentNotExist" kind of error. type ErrAttachmentNotExist struct { ID int64 @@ -133,15 +153,37 @@ func GetAttachmentByUUID(ctx context.Context, uuid string) (*Attachment, error) return attach, nil } -// GetAttachmentsByUUIDs returns attachment by given UUID list. -func GetAttachmentsByUUIDs(ctx context.Context, uuids []string) ([]*Attachment, error) { +type FindAttachmentOptions struct { + ReleaseID int64 + IssueID int64 + CommentID int64 +} + +func (opts FindAttachmentOptions) ToConds() builder.Cond { + return builder.Eq{"release_id": opts.ReleaseID, "issue_id": opts.IssueID, "comment_id": opts.CommentID} +} + +// FindRepoAttachmentsByUUID always returns attachment that has a UUID that is +// in the given `uuids` argument and is attached to the repository. +// +// The values in `opts` are always as a condition even if they are zero, this +// allows to search for attachments that are not yet attached to any resource by +// specifying a empty struct. +func FindRepoAttachmentsByUUID(ctx context.Context, repoID int64, uuids []string, opts FindAttachmentOptions) ([]*Attachment, error) { + // Nothing to match anyway. if len(uuids) == 0 { return []*Attachment{}, nil } - // Silently drop invalid uuids. + // At maximum nothing is filtered and we get all attachments via the UUID. attachments := make([]*Attachment, 0, len(uuids)) - return attachments, db.GetEngine(ctx).In("uuid", uuids).Find(&attachments) + + err := db.GetEngine(ctx). + Where("repo_id = ?", repoID). + In("uuid", uuids). + And(opts.ToConds()). + Find(&attachments) + return attachments, err } // ExistAttachmentsByUUID returns true if attachment exists with the given UUID diff --git a/models/repo/release.go b/models/repo/release.go index 4efbfb352d..edd628fa0f 100644 --- a/models/repo/release.go +++ b/models/repo/release.go @@ -224,18 +224,14 @@ func UpdateRelease(ctx context.Context, rel *Release) error { } // AddReleaseAttachments adds a release attachments -func AddReleaseAttachments(ctx context.Context, releaseID int64, attachmentUUIDs []string) (err error) { - // Check attachments - attachments, err := GetAttachmentsByUUIDs(ctx, attachmentUUIDs) +func AddReleaseAttachments(ctx context.Context, release *Release, attachmentUUIDs []string) (err error) { + attachments, err := FindRepoAttachmentsByUUID(ctx, release.RepoID, attachmentUUIDs, FindAttachmentOptions{}) if err != nil { - return fmt.Errorf("GetAttachmentsByUUIDs [uuids: %v]: %w", attachmentUUIDs, err) + return fmt.Errorf("FindRepoAttachmentsByUUID[uuids=%q,repoID=%d]: %w", attachmentUUIDs, release.RepoID, err) } for i := range attachments { - if attachments[i].ReleaseID != 0 { - return util.NewPermissionDeniedErrorf("release permission denied") - } - attachments[i].ReleaseID = releaseID + attachments[i].ReleaseID = release.ID // No assign value could be 0, so ignore AllCols(). if _, err = db.GetEngine(ctx).ID(attachments[i].ID).Update(attachments[i]); err != nil { return fmt.Errorf("update attachment [%d]: %w", attachments[i].ID, err) diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 10d3e6651b..cc09651e97 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -3682,7 +3682,7 @@ func updateAttachments(ctx *context.Context, item any, files []string) error { if len(files) > 0 { switch content := item.(type) { case *issues_model.Issue: - err = issues_model.UpdateIssueAttachments(ctx, content.ID, files) + err = issues_model.UpdateIssueAttachments(ctx, content, files) case *issues_model.Comment: err = content.UpdateAttachments(ctx, files) default: diff --git a/services/attachment/attachment.go b/services/attachment/attachment.go index b6f763842b..420e95d1eb 100644 --- a/services/attachment/attachment.go +++ b/services/attachment/attachment.go @@ -24,6 +24,9 @@ func NewAttachment(ctx context.Context, attach *repo_model.Attachment, file io.R if attach.RepoID == 0 { return nil, fmt.Errorf("attachment %s should belong to a repository", attach.Name) } + if attach.UploaderID == 0 { + return nil, fmt.Errorf("attachment %s should have a uploader", attach.Name) + } err := db.WithTx(ctx, func(ctx context.Context) error { attach.UUID = uuid.New().String() @@ -48,6 +51,9 @@ func NewExternalAttachment(ctx context.Context, attach *repo_model.Attachment) ( if attach.RepoID == 0 { return nil, fmt.Errorf("attachment %s should belong to a repository", attach.Name) } + if attach.UploaderID == 0 { + return nil, fmt.Errorf("attachment %s should have a uploader", attach.Name) + } if attach.ExternalURL == "" { return nil, fmt.Errorf("attachment %s should have a external url", attach.Name) } diff --git a/services/release/release.go b/services/release/release.go index 69889a1638..9536ebf6dd 100644 --- a/services/release/release.go +++ b/services/release/release.go @@ -192,7 +192,7 @@ func CreateRelease(gitRepo *git.Repository, rel *repo_model.Release, msg string, } } - if err = repo_model.AddReleaseAttachments(gitRepo.Ctx, rel.ID, addAttachmentUUIDs.Values()); err != nil { + if err = repo_model.AddReleaseAttachments(gitRepo.Ctx, rel, addAttachmentUUIDs.Values()); err != nil { return err } @@ -314,44 +314,37 @@ func UpdateRelease(ctx context.Context, doer *user_model.User, gitRepo *git.Repo } } - if err = repo_model.AddReleaseAttachments(ctx, rel.ID, addAttachmentUUIDs.Values()); err != nil { + if err = repo_model.AddReleaseAttachments(ctx, rel, addAttachmentUUIDs.Values()); err != nil { return fmt.Errorf("AddReleaseAttachments: %w", err) } deletedUUIDs := make(container.Set[string]) if len(delAttachmentUUIDs) > 0 { - // Check attachments - attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, delAttachmentUUIDs.Values()) + // Check delAttachments + delAttachments, err := repo_model.FindRepoAttachmentsByUUID(ctx, rel.RepoID, delAttachmentUUIDs.Values(), repo_model.FindAttachmentOptions{ReleaseID: rel.ID}) if err != nil { - return fmt.Errorf("GetAttachmentsByUUIDs [uuids: %v]: %w", delAttachmentUUIDs, err) + return fmt.Errorf("FindRepoAttachmentsByUUID[uuids=%q,repoID=%d,releaseID=%d]: %w", delAttachmentUUIDs.Values(), rel.RepoID, rel.ID, err) } - for _, attach := range attachments { - if attach.ReleaseID != rel.ID { - return util.SilentWrap{ - Message: "delete attachment of release permission denied", - Err: util.ErrPermissionDenied, - } - } + for _, attach := range delAttachments { deletedUUIDs.Add(attach.UUID) } - if _, err := repo_model.DeleteAttachments(ctx, attachments, true); err != nil { + if _, err := repo_model.DeleteAttachments(ctx, delAttachments, true); err != nil { return fmt.Errorf("DeleteAttachments [uuids: %v]: %w", delAttachmentUUIDs, err) } } if len(updateAttachmentUUIDs) > 0 { - // Check attachments - attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, updateAttachmentUUIDs.Values()) + // Check that attachments actually belong to repository and release. + attachments, err := repo_model.FindRepoAttachmentsByUUID(ctx, rel.RepoID, updateAttachmentUUIDs.Values(), repo_model.FindAttachmentOptions{ReleaseID: rel.ID}) if err != nil { - return fmt.Errorf("GetAttachmentsByUUIDs [uuids: %v]: %w", updateAttachmentUUIDs, err) + return fmt.Errorf("FindRepoAttachmentsByUUID[uuids=%q,repoID=%d,releaseID=%d]: %w", updateAttachmentUUIDs.Values(), rel.RepoID, rel.ID, err) } - for _, attach := range attachments { - if attach.ReleaseID != rel.ID { - return util.SilentWrap{ - Message: "update attachment of release permission denied", - Err: util.ErrPermissionDenied, - } + + if len(attachments) != len(updateAttachments) { + return util.SilentWrap{ + Message: "update attachment of release permission denied", + Err: util.ErrPermissionDenied, } } } From abdac1993ab38f12e939505681886684b4deb890 Mon Sep 17 00:00:00 2001 From: Gusted Date: Fri, 27 Feb 2026 00:03:02 +0100 Subject: [PATCH 06/17] chore: add unit test --- models/repo/attachment_test.go | 72 +++++++++-- .../attachment.yml | 116 ++++++++++++++++++ 2 files changed, 180 insertions(+), 8 deletions(-) create mode 100644 models/repo/fixtures/TestFindRepoAttachmentsByUUID/attachment.yml diff --git a/models/repo/attachment_test.go b/models/repo/attachment_test.go index 23f4b3799f..2ee4ee91ed 100644 --- a/models/repo/attachment_test.go +++ b/models/repo/attachment_test.go @@ -1,9 +1,12 @@ // Copyright 2017 The Gitea Authors. All rights reserved. +// Copyright 2026 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package repo_test import ( + "cmp" + "slices" "testing" "forgejo.org/models/db" @@ -88,14 +91,67 @@ func TestUpdateAttachment(t *testing.T) { unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{Name: "new_name"}) } -func TestGetAttachmentsByUUIDs(t *testing.T) { +func TestFindRepoAttachmentsByUUID(t *testing.T) { + defer unittest.OverrideFixtures("models/repo/fixtures/TestFindRepoAttachmentsByUUID")() require.NoError(t, unittest.PrepareTestDatabase()) - attachList, err := repo_model.GetAttachmentsByUUIDs(db.DefaultContext, []string{"a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a17", "not-existing-uuid"}) - require.NoError(t, err) - assert.Len(t, attachList, 2) - assert.Equal(t, "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11", attachList[0].UUID) - assert.Equal(t, "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a17", attachList[1].UUID) - assert.Equal(t, int64(1), attachList[0].IssueID) - assert.Equal(t, int64(5), attachList[1].IssueID) + sort := func(x []*repo_model.Attachment) { + slices.SortFunc(x, func(a, b *repo_model.Attachment) int { + return cmp.Compare(a.ID, b.ID) + }) + } + + t.Run("Empty UUIDs", func(t *testing.T) { + attachments, err := repo_model.FindRepoAttachmentsByUUID(t.Context(), 1001, []string{}, repo_model.FindAttachmentOptions{}) + require.NoError(t, err) + assert.Empty(t, attachments) + }) + + t.Run("Wrong repository", func(t *testing.T) { + attachments, err := repo_model.FindRepoAttachmentsByUUID(t.Context(), 1002, []string{"31b6f65e-2745-4e87-b02c-e6bb9890d399", "e19fd169-c2d1-4fd0-a6d5-9658fd4affed", "758e41f6-e3b7-4420-b34f-1920da0858aa"}, repo_model.FindAttachmentOptions{}) + require.NoError(t, err) + assert.Empty(t, attachments) + }) + + t.Run("Not attached", func(t *testing.T) { + attachments, err := repo_model.FindRepoAttachmentsByUUID(t.Context(), 1001, []string{"31b6f65e-2745-4e87-b02c-e6bb9890d399", "e19fd169-c2d1-4fd0-a6d5-9658fd4affed", "758e41f6-e3b7-4420-b34f-1920da0858aa"}, repo_model.FindAttachmentOptions{}) + require.NoError(t, err) + if assert.Len(t, attachments, 1) { + assert.Equal(t, "31b6f65e-2745-4e87-b02c-e6bb9890d399", attachments[0].UUID) + } + }) + + t.Run("Issue", func(t *testing.T) { + attachments, err := repo_model.FindRepoAttachmentsByUUID(t.Context(), 1001, []string{"17bcdb6b-dd84-4da1-b37a-671165402d8d", "e19fd169-c2d1-4fd0-a6d5-9658fd4affed", "774f276e-c85d-488e-b735-7bc07860c756"}, repo_model.FindAttachmentOptions{IssueID: 1001}) + require.NoError(t, err) + sort(attachments) + if assert.Len(t, attachments, 2) { + assert.Equal(t, "17bcdb6b-dd84-4da1-b37a-671165402d8d", attachments[0].UUID) + assert.Equal(t, "e19fd169-c2d1-4fd0-a6d5-9658fd4affed", attachments[1].UUID) + } + }) + + t.Run("Comment", func(t *testing.T) { + attachments, err := repo_model.FindRepoAttachmentsByUUID(t.Context(), 1001, []string{"edf0d986-8a12-447a-a4bb-e9aefead251b", "774f276e-c85d-488e-b735-7bc07860c756", "e19fd169-c2d1-4fd0-a6d5-9658fd4affed"}, repo_model.FindAttachmentOptions{IssueID: 1001, CommentID: 1001}) + require.NoError(t, err) + if assert.Len(t, attachments, 1) { + assert.Equal(t, "edf0d986-8a12-447a-a4bb-e9aefead251b", attachments[0].UUID) + } + + attachments, err = repo_model.FindRepoAttachmentsByUUID(t.Context(), 1001, []string{"edf0d986-8a12-447a-a4bb-e9aefead251b", "774f276e-c85d-488e-b735-7bc07860c756", "e19fd169-c2d1-4fd0-a6d5-9658fd4affed"}, repo_model.FindAttachmentOptions{IssueID: 1001, CommentID: 1002}) + require.NoError(t, err) + if assert.Len(t, attachments, 1) { + assert.Equal(t, "774f276e-c85d-488e-b735-7bc07860c756", attachments[0].UUID) + } + }) + + t.Run("Release", func(t *testing.T) { + attachments, err := repo_model.FindRepoAttachmentsByUUID(t.Context(), 1001, []string{"d2570bab-c843-486f-b7b7-23e011c42815", "758e41f6-e3b7-4420-b34f-1920da0858aa", "e19fd169-c2d1-4fd0-a6d5-9658fd4affed"}, repo_model.FindAttachmentOptions{ReleaseID: 1001}) + require.NoError(t, err) + if assert.Len(t, attachments, 2) { + sort(attachments) + assert.Equal(t, "758e41f6-e3b7-4420-b34f-1920da0858aa", attachments[0].UUID) + assert.Equal(t, "d2570bab-c843-486f-b7b7-23e011c42815", attachments[1].UUID) + } + }) } diff --git a/models/repo/fixtures/TestFindRepoAttachmentsByUUID/attachment.yml b/models/repo/fixtures/TestFindRepoAttachmentsByUUID/attachment.yml new file mode 100644 index 0000000000..934f92d264 --- /dev/null +++ b/models/repo/fixtures/TestFindRepoAttachmentsByUUID/attachment.yml @@ -0,0 +1,116 @@ +- + id: 1001 + uuid: 70d3e7b8-5e46-41eb-bd2d-afaba53056bd + repo_id: 0 + issue_id: 0 + release_id: 0 + uploader_id: 0 + comment_id: 0 + name: attach1 + download_count: 0 + size: 0 + created_unix: 1771300000 + +- + id: 1002 + uuid: 31b6f65e-2745-4e87-b02c-e6bb9890d399 + repo_id: 1001 + issue_id: 0 + release_id: 0 + uploader_id: 1001 + comment_id: 0 + name: attach1 + download_count: 0 + size: 0 + created_unix: 1771300001 + +- + id: 1003 + uuid: 03158f6c-487c-4bc5-b24b-10f13e21c2e7 + repo_id: 1001 + issue_id: 0 + release_id: 0 + uploader_id: 1002 + comment_id: 0 + name: attach1 + download_count: 0 + size: 0 + created_unix: 1771300002 + +- + id: 1004 + uuid: 17bcdb6b-dd84-4da1-b37a-671165402d8d + repo_id: 1001 + issue_id: 1001 + release_id: 0 + uploader_id: 1001 + comment_id: 0 + name: attach1 + download_count: 0 + size: 0 + created_unix: 1771300003 + +- + id: 1005 + uuid: e19fd169-c2d1-4fd0-a6d5-9658fd4affed + repo_id: 1001 + issue_id: 1001 + release_id: 0 + uploader_id: 1002 + comment_id: 0 + name: attach1 + download_count: 0 + size: 0 + created_unix: 1771300004 + +- + id: 1006 + uuid: 758e41f6-e3b7-4420-b34f-1920da0858aa + repo_id: 1001 + issue_id: 0 + release_id: 1001 + uploader_id: 1001 + comment_id: 0 + name: attach1 + download_count: 0 + size: 0 + created_unix: 1771300005 + +- + id: 1007 + uuid: d2570bab-c843-486f-b7b7-23e011c42815 + repo_id: 1001 + issue_id: 0 + release_id: 1001 + uploader_id: 1002 + comment_id: 0 + name: attach1 + download_count: 0 + size: 0 + created_unix: 1771300006 + +- + id: 1008 + uuid: edf0d986-8a12-447a-a4bb-e9aefead251b + repo_id: 1001 + issue_id: 1001 + release_id: 0 + uploader_id: 1001 + comment_id: 1001 + name: attach1 + download_count: 0 + size: 0 + created_unix: 1771300007 + +- + id: 1009 + uuid: 774f276e-c85d-488e-b735-7bc07860c756 + repo_id: 1001 + issue_id: 1001 + release_id: 0 + uploader_id: 1002 + comment_id: 1002 + name: attach1 + download_count: 0 + size: 0 + created_unix: 1771300008 From b55d819a914dd520e86137662c34f609d8e39163 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 28 Feb 2026 01:48:18 +0100 Subject: [PATCH 07/17] chore: add unit test --- services/release/release_test.go | 34 ++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/services/release/release_test.go b/services/release/release_test.go index 2a593862ce..e05efd9658 100644 --- a/services/release/release_test.go +++ b/services/release/release_test.go @@ -14,6 +14,7 @@ import ( "forgejo.org/modules/git" "forgejo.org/modules/gitrepo" "forgejo.org/modules/test" + "forgejo.org/modules/util" "forgejo.org/services/attachment" "github.com/stretchr/testify/assert" @@ -379,6 +380,39 @@ func TestRelease_Update(t *testing.T) { assert.Equal(t, release.ID, release.Attachments[0].ReleaseID) assert.Equal(t, "test2", release.Attachments[0].Name) assert.Equal(t, "https://about.gitea.com/", release.Attachments[0].ExternalURL) + + // delete the attachment + require.NoError(t, UpdateRelease(db.DefaultContext, user, gitRepo, release, false, []*AttachmentChange{ + { + Action: "delete", + UUID: externalAttachmentUUID, + }, + })) + release.Attachments = nil + require.NoError(t, repo_model.GetReleaseAttachments(db.DefaultContext, release)) + assert.Empty(t, release.Attachments) + + t.Run("Permission denied", func(t *testing.T) { + require.NoError(t, UpdateRelease(t.Context(), user, gitRepo, release, false, []*AttachmentChange{ + { + Action: "add", + Type: "attachment", + UUID: "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a13", + }, + })) + require.NoError(t, repo_model.GetReleaseAttachments(t.Context(), release)) + assert.Empty(t, release.Attachments) + + require.ErrorIs(t, UpdateRelease(t.Context(), user, gitRepo, release, false, []*AttachmentChange{ + { + Action: "update", + Name: "test2.txt", + UUID: "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a13", + }, + }), util.ErrPermissionDenied) + require.NoError(t, repo_model.GetReleaseAttachments(t.Context(), release)) + assert.Empty(t, release.Attachments) + }) } func TestRelease_createTag(t *testing.T) { From af0173894a6f93a991ae0d5baece6e2e6ec89d00 Mon Sep 17 00:00:00 2001 From: Gusted Date: Fri, 20 Feb 2026 18:10:49 +0100 Subject: [PATCH 08/17] fix: consider more risky redirects After evaluating `..`, it's possible that the resulting path is a risky redirect, as it might be a "browser" special redirect. Detect this case. --- modules/httplib/url.go | 19 ++++++++++++++++--- modules/httplib/url_test.go | 7 +++++-- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/modules/httplib/url.go b/modules/httplib/url.go index 32a02e3277..66ea77add7 100644 --- a/modules/httplib/url.go +++ b/modules/httplib/url.go @@ -1,20 +1,26 @@ // Copyright 2023 The Gitea Authors. All rights reserved. +// Copyright 2026 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package httplib import ( "net/url" + "path" "strings" "forgejo.org/modules/setting" ) +// Unfortunately browsers consider a redirect Location with preceding "//", "\\", "/\" and "\/" as meaning redirect to "http(s)://REST_OF_PATH" +// Therefore we should ignore these redirect locations to prevent open redirects. +func isBrowserRedirect(s string) bool { + return len(s) > 1 && (s[0] == '/' || s[0] == '\\') && (s[1] == '/' || s[1] == '\\') +} + // IsRiskyRedirectURL returns true if the URL is considered risky for redirects func IsRiskyRedirectURL(s string) bool { - // Unfortunately browsers consider a redirect Location with preceding "//", "\\", "/\" and "\/" as meaning redirect to "http(s)://REST_OF_PATH" - // Therefore we should ignore these redirect locations to prevent open redirects - if len(s) > 1 && (s[0] == '/' || s[0] == '\\') && (s[1] == '/' || s[1] == '\\') { + if isBrowserRedirect(s) { return true } @@ -23,5 +29,12 @@ func IsRiskyRedirectURL(s string) bool { return true } + // If the path contains `..` then it's still possible this is seen + // as a browser redirect, use `path.Clean` to eliminate each inner `..` + // and then check if that might be a browser redirect. + if strings.Contains(u.Path, "..") { + return isBrowserRedirect(path.Clean(u.Path)) + } + return false } diff --git a/modules/httplib/url_test.go b/modules/httplib/url_test.go index cd2ceac267..0fd59c13a6 100644 --- a/modules/httplib/url_test.go +++ b/modules/httplib/url_test.go @@ -29,7 +29,6 @@ func TestIsRiskyRedirectURL(t *testing.T) { {"/sub/foo", false}, {"http://localhost:3000/sub/foo", false}, {"http://localhost:3000/sub/test?param=false", false}, - // FIXME: should probably be true (would requires resolving references using setting.appURL.ResolveReference(u)) {"/sub/../", false}, {"http://localhost:3000/sub/../", false}, {"/sUb/", false}, @@ -58,10 +57,12 @@ func TestIsRiskyRedirectURL(t *testing.T) { {"://missing protocol scheme", true}, // FIXME: should probably be false {"//localhost:3000/sub/test?param=false", true}, + {"/a/../\\example.com", true}, + {"/a/%2e%2e/\\example.com", true}, } for _, tt := range tests { t.Run(tt.input, func(t *testing.T) { - assert.Equal(t, tt.want, IsRiskyRedirectURL(tt.input)) + assert.Equal(t, tt.want, IsRiskyRedirectURL(tt.input), tt.input) }) } } @@ -114,6 +115,8 @@ func TestIsRiskyRedirectURLWithoutSubURL(t *testing.T) { {"https://next.forgejo.org", true}, {"//next.forgejo.org/test?param=false", true}, {"//next.forgejo.org/sub/test?param=false", true}, + {"/a/../\\example.com", true}, + {"/a/%2e%2e/\\example.com", true}, } for _, tt := range tests { t.Run(tt.input, func(t *testing.T) { From f0e8763867338c4fd9d8a82be20b5f498e26d240 Mon Sep 17 00:00:00 2001 From: Gusted Date: Fri, 20 Feb 2026 20:42:56 +0100 Subject: [PATCH 09/17] fix: check the permission of canceling automerge The API already checked the permission sufficiently if auto merge could be cancelled by the doer. The web route did not. Consolidate this check in the function that lives in the services directory. --- options/locale_next/locale_en-US.json | 1 + routers/api/v1/repo/pull.go | 34 ++++++++------------------- routers/web/repo/pull.go | 17 +++++++++----- services/automerge/automerge.go | 21 ++++++++++++++++- 4 files changed, 42 insertions(+), 31 deletions(-) diff --git a/options/locale_next/locale_en-US.json b/options/locale_next/locale_en-US.json index 9047ca393c..f1849af859 100644 --- a/options/locale_next/locale_en-US.json +++ b/options/locale_next/locale_en-US.json @@ -61,6 +61,7 @@ "issues.updated": "updated %s", "issues.filters.labels.exclude": "Exclude label", "issues.filters.labels.unexclude": "Clear exclusion", + "repo.pulls.auto_merge.no_permission": "You do not have permission to cancel this pull request's auto merge.", "repo.pulls.poster_manage_approval": "Manage approval", "repo.pulls.poster_requires_approval": "Some workflows are waiting to be reviewed.", "repo.pulls.poster_requires_approval.tooltip": "The author of this pull request is not trusted to run workflows triggered by a pull request created from a forked repository or with AGit. The workflows triggered by a `pull_request` event will not run until they are approved.", diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 2e5babdf31..cae39a1466 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -14,6 +14,7 @@ import ( "forgejo.org/models" activities_model "forgejo.org/models/activities" + "forgejo.org/models/db" git_model "forgejo.org/models/git" issues_model "forgejo.org/models/issues" access_model "forgejo.org/models/perm/access" @@ -1373,33 +1374,18 @@ func CancelScheduledAutoMerge(ctx *context.APIContext) { return } - exist, autoMerge, err := pull_model.GetScheduledMergeByPullID(ctx, pull.ID) - if err != nil { - ctx.InternalServerError(err) - return - } - if !exist { - ctx.NotFound() - return - } - - if ctx.Doer.ID != autoMerge.DoerID { - allowed, err := access_model.IsUserRepoAdmin(ctx, ctx.Repo.Repository, ctx.Doer) - if err != nil { - ctx.InternalServerError(err) - return - } - if !allowed { + if err := automerge.RemoveScheduledAutoMerge(ctx, ctx.Doer, pull, ctx.Repo.Permission); err != nil { + switch { + case errors.Is(err, util.ErrPermissionDenied): ctx.Error(http.StatusForbidden, "No permission to cancel", "user has no permission to cancel the scheduled auto merge") - return + case db.IsErrNotExist(err): + ctx.NotFound() + default: + ctx.InternalServerError(err) } + return } - - if err := automerge.RemoveScheduledAutoMerge(ctx, ctx.Doer, pull); err != nil { - ctx.InternalServerError(err) - } else { - ctx.Status(http.StatusNoContent) - } + ctx.Status(http.StatusNoContent) } // GetPullRequestCommits gets all commits associated with a given PR diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 69710824a4..781a001fba 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1538,17 +1538,22 @@ func CancelAutoMergePullRequest(ctx *context.Context) { return } - if err := automerge.RemoveScheduledAutoMerge(ctx, ctx.Doer, issue.PullRequest); err != nil { - if db.IsErrNotExist(err) { + if err := automerge.RemoveScheduledAutoMerge(ctx, ctx.Doer, issue.PullRequest, ctx.Repo.Permission); err != nil { + switch { + case errors.Is(err, util.ErrPermissionDenied): + ctx.Flash.Error(ctx.Tr("repo.pulls.auto_merge.no_permission")) + ctx.Redirect(issue.HTMLURL()) + case db.IsErrNotExist(err): ctx.Flash.Error(ctx.Tr("repo.pulls.auto_merge_not_scheduled")) - ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, issue.Index)) - return + ctx.Redirect(issue.HTMLURL()) + default: + ctx.ServerError("RemoveScheduledAutoMerge", err) } - ctx.ServerError("RemoveScheduledAutoMerge", err) return } + ctx.Flash.Success(ctx.Tr("repo.pulls.auto_merge_canceled_schedule")) - ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, issue.Index)) + ctx.Redirect(issue.HTMLURL()) } func stopTimerIfAvailable(ctx *context.Context, user *user_model.User, issue *issues_model.Issue) error { diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index 0cdc113379..099d048927 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -20,6 +20,7 @@ import ( "forgejo.org/modules/log" "forgejo.org/modules/process" "forgejo.org/modules/queue" + "forgejo.org/modules/util" notify_service "forgejo.org/services/notify" pull_service "forgejo.org/services/pull" repo_service "forgejo.org/services/repository" @@ -67,7 +68,25 @@ func ScheduleAutoMerge(ctx context.Context, doer *user_model.User, pull *issues_ } // RemoveScheduledAutoMerge cancels a previously scheduled pull request -func RemoveScheduledAutoMerge(ctx context.Context, doer *user_model.User, pull *issues_model.PullRequest) error { +func RemoveScheduledAutoMerge(ctx context.Context, doer *user_model.User, pull *issues_model.PullRequest, repoPerms access_model.Permission) error { + exist, autoMerge, err := pull_model.GetScheduledMergeByPullID(ctx, pull.ID) + if err != nil { + return err + } + if !exist { + return db.ErrNotExist{Resource: "auto_merge", ID: pull.ID} + } + + if doer.ID != autoMerge.DoerID { + allowed, err := pull_service.IsUserAllowedToMerge(ctx, pull, repoPerms, doer) + if err != nil { + return err + } + if !allowed { + return util.ErrPermissionDenied + } + } + return db.WithTx(ctx, func(ctx context.Context) error { if err := pull_model.DeleteScheduledAutoMerge(ctx, pull.ID); err != nil { return err From a4fc54830e6c7b30e8261b6c6e942c6b9b2db9cf Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 24 Feb 2026 02:04:49 +0100 Subject: [PATCH 10/17] chore: unit testing --- services/automerge/automerge_test.go | 54 +++++++++++++++++++ .../pull_auto_merge.yml | 8 +++ services/automerge/main_test.go | 14 +++++ 3 files changed, 76 insertions(+) create mode 100644 services/automerge/automerge_test.go create mode 100644 services/automerge/fixtures/TestRemoveScheduledAutoMerge/pull_auto_merge.yml create mode 100644 services/automerge/main_test.go diff --git a/services/automerge/automerge_test.go b/services/automerge/automerge_test.go new file mode 100644 index 0000000000..4ad5b70cce --- /dev/null +++ b/services/automerge/automerge_test.go @@ -0,0 +1,54 @@ +// Copyright 2026 Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package automerge + +import ( + "testing" + + "forgejo.org/models/db" + issues_model "forgejo.org/models/issues" + "forgejo.org/models/perm" + access_model "forgejo.org/models/perm/access" + pull_model "forgejo.org/models/pull" + "forgejo.org/models/unit" + "forgejo.org/models/unittest" + user_model "forgejo.org/models/user" + "forgejo.org/modules/util" + "github.com/stretchr/testify/require" +) + +func TestRemoveScheduledAutoMerge(t *testing.T) { + defer unittest.OverrideFixtures("services/automerge/fixtures/TestRemoveScheduledAutoMerge")() + require.NoError(t, unittest.PrepareTestDatabase()) + + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + user5 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) + pull1 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1}) + pull2 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) + + t.Run("No automerge", func(t *testing.T) { + err := RemoveScheduledAutoMerge(t.Context(), user5, pull2, access_model.Permission{}) + require.ErrorIs(t, err, db.ErrNotExist{Resource: "auto_merge", ID: 2}) + }) + + t.Run("No permission", func(t *testing.T) { + err := RemoveScheduledAutoMerge(t.Context(), user5, pull1, access_model.Permission{}) + require.ErrorIs(t, err, util.ErrPermissionDenied) + + err = RemoveScheduledAutoMerge(t.Context(), user5, pull1, access_model.Permission{UnitsMode: map[unit.Type]perm.AccessMode{ + unit.TypePullRequests: perm.AccessModeRead, + }}) + require.ErrorIs(t, err, util.ErrPermissionDenied) + }) + + t.Run("Normal", func(t *testing.T) { + err := RemoveScheduledAutoMerge(t.Context(), user2, pull1, access_model.Permission{UnitsMode: map[unit.Type]perm.AccessMode{ + unit.TypePullRequests: perm.AccessModeWrite, + }}) + require.NoError(t, err) + + unittest.AssertExistsIf(t, false, &pull_model.AutoMerge{PullID: pull1.ID}) + unittest.AssertExistsIf(t, true, &issues_model.Comment{IssueID: pull1.IssueID, Type: issues_model.CommentTypePRUnScheduledToAutoMerge}) + }) +} diff --git a/services/automerge/fixtures/TestRemoveScheduledAutoMerge/pull_auto_merge.yml b/services/automerge/fixtures/TestRemoveScheduledAutoMerge/pull_auto_merge.yml new file mode 100644 index 0000000000..2cacb611c3 --- /dev/null +++ b/services/automerge/fixtures/TestRemoveScheduledAutoMerge/pull_auto_merge.yml @@ -0,0 +1,8 @@ +- + id: 1001 + pull_id: 1 + doer_id: 2 + merge_style: "merge" + message: "Automatically merged" + delete_branch_after_merge: false + created_unix: 1771800000 diff --git a/services/automerge/main_test.go b/services/automerge/main_test.go new file mode 100644 index 0000000000..8c0e558b17 --- /dev/null +++ b/services/automerge/main_test.go @@ -0,0 +1,14 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package automerge + +import ( + "testing" + + "forgejo.org/models/unittest" +) + +func TestMain(m *testing.M) { + unittest.MainTest(m) +} From 0da92e47ca1d58597fe4607c6ea38d08d1233615 Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 24 Feb 2026 03:15:02 +0100 Subject: [PATCH 11/17] chore: add integration testing --- services/automerge/automerge_test.go | 1 + tests/integration/pull_auto_merge_test.go | 106 ++++++++++++++++++++++ 2 files changed, 107 insertions(+) create mode 100644 tests/integration/pull_auto_merge_test.go diff --git a/services/automerge/automerge_test.go b/services/automerge/automerge_test.go index 4ad5b70cce..38ed4dcd9f 100644 --- a/services/automerge/automerge_test.go +++ b/services/automerge/automerge_test.go @@ -15,6 +15,7 @@ import ( "forgejo.org/models/unittest" user_model "forgejo.org/models/user" "forgejo.org/modules/util" + "github.com/stretchr/testify/require" ) diff --git a/tests/integration/pull_auto_merge_test.go b/tests/integration/pull_auto_merge_test.go new file mode 100644 index 0000000000..e9a428d4e7 --- /dev/null +++ b/tests/integration/pull_auto_merge_test.go @@ -0,0 +1,106 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package integration + +import ( + "fmt" + "net/http" + "net/url" + "os" + "path" + "strings" + "testing" + "time" + + unit_model "forgejo.org/models/unit" + "forgejo.org/models/unittest" + user_model "forgejo.org/models/user" + "forgejo.org/modules/git" + app_context "forgejo.org/services/context" + files_service "forgejo.org/services/repository/files" + "forgejo.org/tests" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPullRemoveAutomerge(t *testing.T) { + onApplicationRun(t, func(t *testing.T, u *url.URL) { + user5 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) + user5Session := loginUser(t, user5.Name) + user2Session := loginUser(t, "user2") + + repo, _, f := tests.CreateDeclarativeRepo(t, user5, "", + []unit_model.Type{unit_model.TypeCode, unit_model.TypePullRequests}, nil, + []*files_service.ChangeRepoFile{ + { + Operation: "create", + TreePath: "FUNFACT", + ContentReader: strings.NewReader( + "The Netherlands got its first openly gay prime minister today."), + }, + }, + ) + defer f() + + dstPath := t.TempDir() + cloneURL, _ := url.Parse(fmt.Sprintf("%suser5/%s.git", u.String(), repo.Name)) + cloneURL.User = url.UserPassword("user5", userPassword) + require.NoError(t, git.CloneWithArgs(t.Context(), nil, cloneURL.String(), dstPath, git.CloneRepoOptions{})) + doGitSetRemoteURL(dstPath, "origin", cloneURL)(t) + + require.NoError(t, git.NewCommand(t.Context(), "switch", "-c", "new-fun-fact").Run(&git.RunOpts{Dir: dstPath})) + + require.NoError(t, os.WriteFile(path.Join(dstPath, "README.md"), []byte("The house of representative already had that in 1937."), 0o600)) + require.NoError(t, git.AddChanges(dstPath, true)) + require.NoError(t, git.CommitChanges(dstPath, git.CommitChangesOptions{ + Committer: &git.Signature{ + Email: "user2@example.com", + Name: "user2", + When: time.Now(), + }, + Author: &git.Signature{ + Email: "user2@example.com", + Name: "user2", + When: time.Now(), + }, + Message: "Update funfact.", + })) + + require.NoError(t, git.NewCommand(t.Context(), "push", "origin", "HEAD:refs/for/main", "-o", "topic=new-fun-fact").Run(&git.RunOpts{Dir: dstPath})) + + // Create a protected branch rule for automerge. + user5Session.MakeRequest(t, NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/settings/branches/edit", repo.FullName()), map[string]string{ + "rule_name": "main", + "required_approvals": "1", + }), http.StatusSeeOther) + + // Start a automerge for new pull request. + user5Session.MakeRequest(t, NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/pulls/1/merge", repo.FullName()), map[string]string{ + "merge_message_field": "I love automation when it works", + "do": "merge", + "merge_when_checks_succeed": "true", + }), http.StatusOK) + + t.Run("No permission", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + user2Session.MakeRequest(t, NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/pulls/1/cancel_auto_merge", repo.FullName()), nil), http.StatusSeeOther) + + flashCookie := user2Session.GetCookie(app_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.Equal(t, "error%3DYou%2Bdo%2Bnot%2Bhave%2Bpermission%2Bto%2Bcancel%2Bthis%2Bpull%2Brequest%2527s%2Bauto%2Bmerge.", flashCookie.Value) + }) + + t.Run("Normal", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + user5Session.MakeRequest(t, NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/pulls/1/cancel_auto_merge", repo.FullName()), nil), http.StatusSeeOther) + + flashCookie := user5Session.GetCookie(app_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.Equal(t, "success%3DThe%2Bauto%2Bmerge%2Bwas%2Bcanceled%2Bfor%2Bthis%2Bpull%2Brequest.", flashCookie.Value) + }) + }) +} From 837557166b138304b800bdcd77141f15d80a6a43 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 21 Feb 2026 13:47:15 +0100 Subject: [PATCH 12/17] fix: check owner when changing state of project It was sufficiently checked for the repostiory case, but for user/org project it was not checked and you could change the state of any project by there mere knowledge of a ID. --- models/project/project.go | 62 ++++++++++++++++------------------ models/project/project_test.go | 37 -------------------- routers/web/org/projects.go | 9 +++-- routers/web/repo/projects.go | 9 +++-- 4 files changed, 43 insertions(+), 74 deletions(-) diff --git a/models/project/project.go b/models/project/project.go index c095087e51..3a36613b49 100644 --- a/models/project/project.go +++ b/models/project/project.go @@ -309,6 +309,18 @@ func GetProjectForRepoByID(ctx context.Context, repoID, id int64) (*Project, err return p, nil } +// GetProjectForUserByID returns the project by id that belongs to the specified user. +func GetProjectForUserByID(ctx context.Context, uid, id int64) (*Project, error) { + p := new(Project) + has, err := db.GetEngine(ctx).Where("id=? AND owner_id=?", id, uid).Get(p) + if err != nil { + return nil, err + } else if !has { + return nil, ErrProjectNotExist{ID: id} + } + return p, nil +} + // UpdateProject updates project properties func UpdateProject(ctx context.Context, p *Project) error { if !IsCardTypeValid(p.CardType) { @@ -346,42 +358,26 @@ func updateRepositoryProjectCount(ctx context.Context, repoID int64) error { return nil } -// ChangeProjectStatusByRepoIDAndID toggles a project between opened and closed -func ChangeProjectStatusByRepoIDAndID(ctx context.Context, repoID, projectID int64, isClosed bool) error { - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return err - } - defer committer.Close() - - p := new(Project) - - has, err := db.GetEngine(ctx).ID(projectID).Where("repo_id = ?", repoID).Get(p) - if err != nil { - return err - } else if !has { - return ErrProjectNotExist{ID: projectID, RepoID: repoID} - } - - if err := changeProjectStatus(ctx, p, isClosed); err != nil { - return err - } - - return committer.Commit() -} - -func changeProjectStatus(ctx context.Context, p *Project, isClosed bool) error { - p.IsClosed = isClosed - p.ClosedDateUnix = timeutil.TimeStampNow() - count, err := db.GetEngine(ctx).ID(p.ID).Where("repo_id = ? AND is_closed = ?", p.RepoID, !isClosed).Cols("is_closed", "closed_date_unix").Update(p) - if err != nil { - return err - } - if count < 1 { +// ChangeProjectStatus changes the status of the specified project to the state +// specified via the `isClosed` argument. +func ChangeProjectStatus(ctx context.Context, p *Project, isClosed bool) error { + if p.IsClosed == isClosed { return nil } - return updateRepositoryProjectCount(ctx, p.RepoID) + return db.WithTx(ctx, func(ctx context.Context) error { + p.IsClosed = isClosed + p.ClosedDateUnix = timeutil.TimeStampNow() + count, err := db.GetEngine(ctx).ID(p.ID).Cols("is_closed", "closed_date_unix").Update(p) + if err != nil { + return err + } + if count < 1 { + return nil + } + + return updateRepositoryProjectCount(ctx, p.RepoID) + }) } // DeleteProjectByID deletes a project from a repository. if it's not in a database diff --git a/models/project/project_test.go b/models/project/project_test.go index b6f7c7db27..73b8f01f00 100644 --- a/models/project/project_test.go +++ b/models/project/project_test.go @@ -8,7 +8,6 @@ import ( "forgejo.org/models/db" "forgejo.org/models/unittest" - "forgejo.org/modules/timeutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -48,42 +47,6 @@ func TestGetProjects(t *testing.T) { assert.Len(t, projects, 1) } -func TestProject(t *testing.T) { - require.NoError(t, unittest.PrepareTestDatabase()) - - project := &Project{ - Type: TypeRepository, - TemplateType: TemplateTypeBasicKanban, - CardType: CardTypeTextOnly, - Title: "New Project", - RepoID: 1, - CreatedUnix: timeutil.TimeStampNow(), - CreatorID: 2, - } - - require.NoError(t, NewProject(db.DefaultContext, project)) - - _, err := GetProjectByID(db.DefaultContext, project.ID) - require.NoError(t, err) - - // Update project - project.Title = "Updated title" - require.NoError(t, UpdateProject(db.DefaultContext, project)) - - projectFromDB, err := GetProjectByID(db.DefaultContext, project.ID) - require.NoError(t, err) - - assert.Equal(t, project.Title, projectFromDB.Title) - - require.NoError(t, ChangeProjectStatusByRepoIDAndID(db.DefaultContext, project.RepoID, project.ID, true)) - - // Retrieve from DB afresh to check if it is truly closed - projectFromDB, err = GetProjectByID(db.DefaultContext, project.ID) - require.NoError(t, err) - - assert.True(t, projectFromDB.IsClosed) -} - func TestProjectsSort(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) diff --git a/routers/web/org/projects.go b/routers/web/org/projects.go index 66c560f55e..a492d85d84 100644 --- a/routers/web/org/projects.go +++ b/routers/web/org/projects.go @@ -218,8 +218,13 @@ func ChangeProjectStatus(ctx *context.Context) { } id := ctx.ParamsInt64(":id") - if err := project_model.ChangeProjectStatusByRepoIDAndID(ctx, 0, id, toClose); err != nil { - ctx.NotFoundOrServerError("ChangeProjectStatusByRepoIDAndID", project_model.IsErrProjectNotExist, err) + project, err := project_model.GetProjectForUserByID(ctx, ctx.ContextUser.ID, id) + if err != nil { + ctx.NotFoundOrServerError("GetProjectForUserByID", project_model.IsErrProjectNotExist, err) + return + } + if err := project_model.ChangeProjectStatus(ctx, project, toClose); err != nil { + ctx.ServerError("ChangeProjectStatus", err) return } ctx.JSONRedirect(project_model.ProjectLinkForOrg(ctx.ContextUser, id)) diff --git a/routers/web/repo/projects.go b/routers/web/repo/projects.go index ecc1cc89d9..e3e9ce0eb7 100644 --- a/routers/web/repo/projects.go +++ b/routers/web/repo/projects.go @@ -192,8 +192,13 @@ func ChangeProjectStatus(ctx *context.Context) { } id := ctx.ParamsInt64(":id") - if err := project_model.ChangeProjectStatusByRepoIDAndID(ctx, ctx.Repo.Repository.ID, id, toClose); err != nil { - ctx.NotFoundOrServerError("ChangeProjectStatusByRepoIDAndID", project_model.IsErrProjectNotExist, err) + project, err := project_model.GetProjectForRepoByID(ctx, ctx.Repo.Repository.ID, id) + if err != nil { + ctx.NotFoundOrServerError("GetProjectForRepoByID", project_model.IsErrProjectNotExist, err) + return + } + if err := project_model.ChangeProjectStatus(ctx, project, toClose); err != nil { + ctx.ServerError("ChangeProjectStatus", err) return } ctx.JSONRedirect(project_model.ProjectLinkForRepo(ctx.Repo.Repository, id)) From 4f464db8a74bd22c2eb71a403e9130f9f585145f Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 21 Feb 2026 14:09:01 +0100 Subject: [PATCH 13/17] chore: add unit tests Unit tests for model functions --- models/project/project_test.go | 73 ++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/models/project/project_test.go b/models/project/project_test.go index 73b8f01f00..8a50d73ebc 100644 --- a/models/project/project_test.go +++ b/models/project/project_test.go @@ -1,4 +1,5 @@ // Copyright 2020 The Gitea Authors. All rights reserved. +// Copyright 2026 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package project @@ -7,6 +8,7 @@ import ( "testing" "forgejo.org/models/db" + repo_model "forgejo.org/models/repo" "forgejo.org/models/unittest" "github.com/stretchr/testify/assert" @@ -85,3 +87,74 @@ func TestProjectsSort(t *testing.T) { } } } + +func TestGetProjectForUserByID(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + found := func(t *testing.T, uid, id int64) { + t.Helper() + + p, err := GetProjectForUserByID(t.Context(), uid, id) + require.NoError(t, err) + if assert.NotNil(t, p) { + assert.Equal(t, id, p.ID) + } + } + + notFound := func(t *testing.T, uid, id int64) { + t.Helper() + + p, err := GetProjectForUserByID(t.Context(), uid, id) + require.ErrorIs(t, err, ErrProjectNotExist{ID: id}) + assert.Nil(t, p) + } + + found(t, 2, 4) + found(t, 2, 5) + found(t, 2, 6) + found(t, 3, 7) + notFound(t, 1, 4) + notFound(t, 1, 5) + notFound(t, 1, 6) + notFound(t, 1, 7) +} + +func TestChangeProjectStatus(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + t.Run("Unchanged", func(t *testing.T) { + project := unittest.AssertExistsAndLoadBean(t, &Project{ID: 1}) + + require.NoError(t, ChangeProjectStatus(t.Context(), project, project.IsClosed)) + + projectAfter := unittest.AssertExistsAndLoadBean(t, &Project{ID: 1}) + assert.Equal(t, project.IsClosed, projectAfter.IsClosed) + }) + + t.Run("Normal", func(t *testing.T) { + project := unittest.AssertExistsAndLoadBean(t, &Project{ID: 1}) + isClosed := !project.IsClosed + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: project.RepoID}) + + require.NoError(t, ChangeProjectStatus(t.Context(), project, isClosed)) + + projectAfter := unittest.AssertExistsAndLoadBean(t, &Project{ID: 1}) + repoAfter := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: project.RepoID}) + assert.Equal(t, isClosed, projectAfter.IsClosed) + assert.Equal(t, repo.NumProjects, repoAfter.NumProjects) + assert.Equal(t, repo.NumOpenProjects-1, repoAfter.NumOpenProjects) + assert.Equal(t, repo.NumClosedProjects+1, repoAfter.NumClosedProjects) + }) + + t.Run("Invalid ID", func(t *testing.T) { + project := &Project{ID: 1001, RepoID: 1} + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: project.RepoID}) + + require.NoError(t, ChangeProjectStatus(t.Context(), project, true)) + + repoAfter := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: project.RepoID}) + assert.Equal(t, repo.NumProjects, repoAfter.NumProjects) + assert.Equal(t, repo.NumOpenProjects, repoAfter.NumOpenProjects) + assert.Equal(t, repo.NumClosedProjects, repoAfter.NumClosedProjects) + }) +} From 58d844b32033701292121a9720135cafa83a5eb3 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 21 Feb 2026 14:32:45 +0100 Subject: [PATCH 14/17] chore: add integration testing --- tests/integration/project_test.go | 82 +++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/tests/integration/project_test.go b/tests/integration/project_test.go index b8fcafab7a..955caaf6f7 100644 --- a/tests/integration/project_test.go +++ b/tests/integration/project_test.go @@ -1,4 +1,5 @@ // Copyright 2023 The Gitea Authors. All rights reserved. +// Copyright 2026 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package integration @@ -78,3 +79,84 @@ func TestMoveRepoProjectColumns(t *testing.T) { require.NoError(t, project_model.DeleteProjectByID(db.DefaultContext, project1.ID)) } + +func TestChangeStatusProject(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + user5 := loginUser(t, "user5") + user2 := loginUser(t, "user2") + + t.Run("User", func(t *testing.T) { + project4CloseURL := "/user2/-/projects/4/close" + + t.Run("Doer is not context user", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + user5.MakeRequest(t, NewRequest(t, "POST", project4CloseURL), http.StatusNotFound) + unittest.AssertExistsIf(t, true, &project_model.Project{ID: 4}, "is_closed = false") + }) + + t.Run("Wrong ID", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + user5.MakeRequest(t, NewRequest(t, "POST", "/user5/-/projects/4/close"), http.StatusNotFound) + unittest.AssertExistsIf(t, true, &project_model.Project{ID: 4}, "is_closed = false") + + user5.MakeRequest(t, NewRequest(t, "POST", "/user5/-/projects/1/close"), http.StatusNotFound) + unittest.AssertExistsIf(t, true, &project_model.Project{ID: 1}, "is_closed = false") + + user5.MakeRequest(t, NewRequest(t, "POST", "/user5/-/projects/7/close"), http.StatusNotFound) + unittest.AssertExistsIf(t, true, &project_model.Project{ID: 7}, "is_closed = false") + }) + + t.Run("Normal", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + user2.MakeRequest(t, NewRequest(t, "POST", project4CloseURL), http.StatusOK) + unittest.AssertExistsIf(t, true, &project_model.Project{ID: 4}, "is_closed = true") + }) + }) + + t.Run("Organization", func(t *testing.T) { + project7CloseURL := "/org3/-/projects/7/close" + + t.Run("Doer does not have permission", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + user5.MakeRequest(t, NewRequest(t, "POST", project7CloseURL), http.StatusNotFound) + unittest.AssertExistsIf(t, true, &project_model.Project{ID: 7}, "is_closed = false") + }) + + t.Run("Normal", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + user2.MakeRequest(t, NewRequest(t, "POST", project7CloseURL), http.StatusOK) + unittest.AssertExistsIf(t, true, &project_model.Project{ID: 7}, "is_closed = true") + }) + }) + + t.Run("Repository", func(t *testing.T) { + project1CloseURL := "/user2/repo1/projects/1/close" + + t.Run("Doer does not have permission", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + user5.MakeRequest(t, NewRequest(t, "POST", project1CloseURL), http.StatusNotFound) + unittest.AssertExistsIf(t, true, &project_model.Project{ID: 1}, "is_closed = false") + }) + + t.Run("Wrong ID", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + user5.MakeRequest(t, NewRequest(t, "POST", "/user5/repo4/projects/1/close"), http.StatusNotFound) + unittest.AssertExistsIf(t, true, &project_model.Project{ID: 1}, "is_closed = false") + }) + + t.Run("Normal", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + user2.MakeRequest(t, NewRequest(t, "POST", project1CloseURL), http.StatusOK) + unittest.AssertExistsIf(t, true, &project_model.Project{ID: 1}, "is_closed = true") + }) + }) +} From 5e1a2f9cc493bfd78da821e5f75faafd9b6d0ff7 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 21 Feb 2026 14:47:07 +0100 Subject: [PATCH 15/17] fix: filter recipients for new release mails Remove recipients that are not active (e.g. done by moderation or organizational reasons) and those that have the permi ssion to read releases on that repository. --- services/mailer/mail_release.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/services/mailer/mail_release.go b/services/mailer/mail_release.go index 0f2ef33fe1..2111083bd4 100644 --- a/services/mailer/mail_release.go +++ b/services/mailer/mail_release.go @@ -6,8 +6,11 @@ package mailer import ( "bytes" "context" + "slices" + access_model "forgejo.org/models/perm/access" repo_model "forgejo.org/models/repo" + "forgejo.org/models/unit" user_model "forgejo.org/models/user" "forgejo.org/modules/base" "forgejo.org/modules/log" @@ -40,6 +43,12 @@ func MailNewRelease(ctx context.Context, rel *repo_model.Release) { return } + // Users are not eligible to receive this mail if they are not active or + // they don't have permissions to read releases. + recipients = slices.DeleteFunc(recipients, func(u *user_model.User) bool { + return !u.IsActive || !access_model.CheckRepoUnitUser(ctx, rel.Repo, u, unit.TypeReleases) + }) + langMap := make(map[string][]*user_model.User) for _, user := range recipients { if user.ID != rel.PublisherID { From 39078d478a43a164bb47712ced6115c6947e792e Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 21 Feb 2026 16:00:11 +0100 Subject: [PATCH 16/17] chore: add unit test --- .../fixtures/TestMailNewRelease/watch.yml | 5 + services/mailer/mail_release_test.go | 122 ++++++++++++++++++ 2 files changed, 127 insertions(+) create mode 100644 services/mailer/fixtures/TestMailNewRelease/watch.yml create mode 100644 services/mailer/mail_release_test.go diff --git a/services/mailer/fixtures/TestMailNewRelease/watch.yml b/services/mailer/fixtures/TestMailNewRelease/watch.yml new file mode 100644 index 0000000000..4c488a6f85 --- /dev/null +++ b/services/mailer/fixtures/TestMailNewRelease/watch.yml @@ -0,0 +1,5 @@ +- + id: 1001 + user_id: 11 + repo_id: 41 + mode: 1 diff --git a/services/mailer/mail_release_test.go b/services/mailer/mail_release_test.go new file mode 100644 index 0000000000..4b37afb8cd --- /dev/null +++ b/services/mailer/mail_release_test.go @@ -0,0 +1,122 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package mailer_test + +import ( + "testing" + + "forgejo.org/models/db" + repo_model "forgejo.org/models/repo" + "forgejo.org/models/unittest" + user_model "forgejo.org/models/user" + "forgejo.org/modules/gitrepo" + "forgejo.org/services/mailer" + release_service "forgejo.org/services/release" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMailNewRelease(t *testing.T) { + defer unittest.OverrideFixtures("services/mailer/fixtures/TestMailNewRelease")() + defer require.NoError(t, unittest.PrepareTestDatabase()) + + user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + user11 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 11}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + + gitRepo, err := gitrepo.OpenRepository(t.Context(), repo) + require.NoError(t, err) + defer gitRepo.Close() + + t.Run("Normal", func(t *testing.T) { + called := false + + defer mailer.MockMailSettings(func(msgs ...*mailer.Message) { + assert.Len(t, msgs, 2) + if user1.EmailTo() == msgs[0].To { + assert.Equal(t, user11.EmailTo(), msgs[1].To) + } else { + assert.Equal(t, user11.EmailTo(), msgs[0].To) + assert.Equal(t, user1.EmailTo(), msgs[1].To) + } + + assert.Equal(t, "v0.1 in user2/repo1 released", msgs[0].Subject) + called = true + })() + + require.NoError(t, release_service.CreateRelease(gitRepo, &repo_model.Release{ + RepoID: repo.ID, + Repo: repo, + PublisherID: user2.ID, + Publisher: user2, + TagName: "v0.1", + Target: "master", + Title: "v0.1 is released", + Note: "v0.1 is released", + }, "", []*release_service.AttachmentChange{})) + + assert.True(t, called) + }) + + t.Run("Non-active user", func(t *testing.T) { + _, err := db.GetEngine(db.DefaultContext).Exec("UPDATE user SET is_active=false WHERE id=11") + require.NoError(t, err) + + t.Cleanup(func() { + _, err := db.GetEngine(db.DefaultContext).Exec("UPDATE user SET is_active=true WHERE id=11") + require.NoError(t, err) + }) + called := false + + defer mailer.MockMailSettings(func(msgs ...*mailer.Message) { + assert.Len(t, msgs, 1) + assert.Equal(t, user1.EmailTo(), msgs[0].To) + + assert.Equal(t, "v0.2 in user2/repo1 released", msgs[0].Subject) + called = true + })() + + require.NoError(t, release_service.CreateRelease(gitRepo, &repo_model.Release{ + RepoID: repo.ID, + Repo: repo, + PublisherID: user2.ID, + Publisher: user2, + TagName: "v0.2", + Target: "master", + Title: "v0.2 is released", + Note: "v0.2 is released", + }, "", []*release_service.AttachmentChange{})) + + assert.True(t, called) + }) + + t.Run("No permissions for releases", func(t *testing.T) { + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 41}) + + gitRepo, err := gitrepo.OpenRepository(t.Context(), repo) + require.NoError(t, err) + defer gitRepo.Close() + + called := false + + defer mailer.MockMailSettings(func(msgs ...*mailer.Message) { + called = true + })() + + require.NoError(t, release_service.CreateRelease(gitRepo, &repo_model.Release{ + RepoID: repo.ID, + Repo: repo, + PublisherID: user2.ID, + Publisher: user2, + TagName: "v0.2", + Target: "master", + Title: "v0.2 is released", + Note: "v0.2 is released", + }, "", []*release_service.AttachmentChange{})) + + assert.False(t, called) + }) +} From c69ba8b1c1102fe1c6ff9af141be166f8b1a8ae5 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Fri, 6 Mar 2026 13:01:43 -0700 Subject: [PATCH 17/17] chore: release notes from #11514 & #11515 backports --- release-notes/11514.md | 7 +++++++ release-notes/11515.md | 7 +++++++ 2 files changed, 14 insertions(+) create mode 100644 release-notes/11514.md create mode 100644 release-notes/11515.md diff --git a/release-notes/11514.md b/release-notes/11514.md new file mode 100644 index 0000000000..078e479e6a --- /dev/null +++ b/release-notes/11514.md @@ -0,0 +1,7 @@ +- fix: PKCE challenges to Forgejo's OAuth identity provider were not validated when using the `S256` algorithm +- fix: Forgejo supports using an OAuth Bearer token with HTTP basic authentication, rather than Bearer token authentication, but did not properly apply the limited scopes of the OAuth grant +- fix: missing permission checks in attachment-related web endpoints allowed modifying attachments that a user did not own +- fix: email notifications for new releases could be sent to users that no longer access to the repository, or to inactive users +- fix: missing permission checks in user/org-owned projects would allow modifications of the open/closed state to be made to projects via insecure direct object references +- fix: missing permission checks in a web endpoint allowed cancellation of the automerge of a PR +- fix: prevent additional path-traversals in post-login redirect parameters that allowed for arbitrary redirects diff --git a/release-notes/11515.md b/release-notes/11515.md new file mode 100644 index 0000000000..078e479e6a --- /dev/null +++ b/release-notes/11515.md @@ -0,0 +1,7 @@ +- fix: PKCE challenges to Forgejo's OAuth identity provider were not validated when using the `S256` algorithm +- fix: Forgejo supports using an OAuth Bearer token with HTTP basic authentication, rather than Bearer token authentication, but did not properly apply the limited scopes of the OAuth grant +- fix: missing permission checks in attachment-related web endpoints allowed modifying attachments that a user did not own +- fix: email notifications for new releases could be sent to users that no longer access to the repository, or to inactive users +- fix: missing permission checks in user/org-owned projects would allow modifications of the open/closed state to be made to projects via insecure direct object references +- fix: missing permission checks in a web endpoint allowed cancellation of the automerge of a PR +- fix: prevent additional path-traversals in post-login redirect parameters that allowed for arbitrary redirects