From 388436d5005cca1b969379aab16bb68d86c24c61 Mon Sep 17 00:00:00 2001 From: Gusted Date: Mon, 2 Mar 2026 01:37:10 +0100 Subject: [PATCH 01/18] 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 f44a102a49..9099aedb44 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -479,8 +479,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 2b0ec87644321cbcf4c55f7705c6bfb1d39afc3c Mon Sep 17 00:00:00 2001 From: Gusted Date: Mon, 2 Mar 2026 01:40:26 +0100 Subject: [PATCH 02/18] 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 d1d5dffad5..16c8656bb8 100644 --- a/tests/integration/oauth_test.go +++ b/tests/integration/oauth_test.go @@ -1533,61 +1533,126 @@ 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 verfifier", 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 155acecb4b39ef6722ca5d1fe9d7aba7a511f9f6 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sun, 1 Mar 2026 23:32:33 +0100 Subject: [PATCH 03/18] 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 f117494762..2167055384 100644 --- a/services/auth/basic.go +++ b/services/auth/basic.go @@ -72,7 +72,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) @@ -83,6 +83,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 816a37f576d1cec3f4c7fce65aaac6d62c53e103 Mon Sep 17 00:00:00 2001 From: Gusted Date: Mon, 2 Mar 2026 00:48:04 +0100 Subject: [PATCH 04/18] 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 206887ba08..cbe73eaa37 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, cateogoryUnauthorizedScopes...) } - 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 fe55c0e76c3236b40809dc32b81a4f74964fb6d9 Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 11 Feb 2026 08:16:51 +0100 Subject: [PATCH 05/18] 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 06d35bb2d4..769feba54a 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 2310de7cb9..a421930d61 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 d46ea7f133..44e8e88596 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -3670,7 +3670,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 fa3073044a21705bb72558f768c8ae1fc799ef42 Mon Sep 17 00:00:00 2001 From: Gusted Date: Fri, 27 Feb 2026 00:03:02 +0100 Subject: [PATCH 06/18] 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 e5a4f83bea33431a5fb437df502cf3a27e86ba1a Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 28 Feb 2026 01:48:18 +0100 Subject: [PATCH 07/18] 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 912ffa2dbd01ea9376d651a79a1eb68fd266f737 Mon Sep 17 00:00:00 2001 From: Gusted Date: Fri, 20 Feb 2026 18:10:49 +0100 Subject: [PATCH 08/18] 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 f4581e0f23d7cceef93a50806b4eaa1f563bafc0 Mon Sep 17 00:00:00 2001 From: Gusted Date: Fri, 20 Feb 2026 20:42:56 +0100 Subject: [PATCH 09/18] 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 90133f36ff..0641a32e68 100644 --- a/options/locale_next/locale_en-US.json +++ b/options/locale_next/locale_en-US.json @@ -63,6 +63,7 @@ "repo.issues.filter_modified.hint": "Filter by last modified date", "repo.issues.filter_sort.hint": "Sort by: created/comments/updated/deadline", "issues.updated": "updated %s", + "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 778a1c17b1..5ae86c5925 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 fc57758c878053c2955dcf7bcf09040294ad24c0 Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 24 Feb 2026 02:04:49 +0100 Subject: [PATCH 10/18] 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 d1d90aa9c1b308566c6fc13197d11bf49a0420b9 Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 24 Feb 2026 03:15:02 +0100 Subject: [PATCH 11/18] 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 e7a5e0a82b17e14a5367fcc68b2218cdc4d741f2 Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Mon, 16 Feb 2026 05:52:47 +0100 Subject: [PATCH 12/18] test: backport from #9906 test data Required for backport of v15 security fixes w/ test automation, this is a partial cherry-pick of 5589182c548240aab4fb1f2dc23c91e267fb2ea8. Signed-off-by: Nils Philippsen Co-authored-by: Gusted Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9906 Reviewed-by: Gusted Reviewed-by: Robert Wolff Co-authored-by: Nils Philippsen Co-committed-by: Nils Philippsen --- models/fixtures/project.yml | 18 +++++++++++++++--- models/project/project_test.go | 12 ++++++------ 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/models/fixtures/project.yml b/models/fixtures/project.yml index 44d87bce04..54a3061859 100644 --- a/models/fixtures/project.yml +++ b/models/fixtures/project.yml @@ -42,7 +42,7 @@ is_closed: false creator_id: 2 board_type: 1 - type: 2 + type: 1 created_unix: 1688973000 updated_unix: 1688973000 @@ -54,7 +54,7 @@ is_closed: false creator_id: 2 board_type: 1 - type: 2 + type: 1 created_unix: 1688973000 updated_unix: 1688973000 @@ -66,6 +66,18 @@ is_closed: false creator_id: 2 board_type: 1 - type: 2 + type: 1 + created_unix: 1688973000 + updated_unix: 1688973000 + +- + id: 7 + title: project on org3 + owner_id: 3 + repo_id: 0 + is_closed: false + creator_id: 2 + board_type: 1 + type: 1 created_unix: 1688973000 updated_unix: 1688973000 diff --git a/models/project/project_test.go b/models/project/project_test.go index ab23bab0bf..b6f7c7db27 100644 --- a/models/project/project_test.go +++ b/models/project/project_test.go @@ -93,19 +93,19 @@ func TestProjectsSort(t *testing.T) { }{ { sortType: "default", - wants: []int64{1, 3, 2, 6, 5, 4}, + wants: []int64{1, 3, 2, 7, 6, 5, 4}, }, { sortType: "oldest", - wants: []int64{4, 5, 6, 2, 3, 1}, + wants: []int64{4, 5, 6, 7, 2, 3, 1}, }, { sortType: "recentupdate", - wants: []int64{1, 3, 2, 6, 5, 4}, + wants: []int64{1, 3, 2, 7, 6, 5, 4}, }, { sortType: "leastupdate", - wants: []int64{4, 5, 6, 2, 3, 1}, + wants: []int64{4, 5, 6, 7, 2, 3, 1}, }, } @@ -114,8 +114,8 @@ func TestProjectsSort(t *testing.T) { OrderBy: GetSearchOrderByBySortType(tt.sortType), }) require.NoError(t, err) - assert.Equal(t, int64(6), count) - if assert.Len(t, projects, 6) { + assert.Equal(t, int64(7), count) + if assert.Len(t, projects, 7) { for i := range projects { assert.Equal(t, tt.wants[i], projects[i].ID) } From d8cba03e1663ef35a15ce7241dba8322f3780c12 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 21 Feb 2026 13:47:15 +0100 Subject: [PATCH 13/18] 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 18c647c8ac..7d507b358d 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 c502e2b1e3d64ebfcd94506bd963312fa3abada0 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 21 Feb 2026 14:09:01 +0100 Subject: [PATCH 14/18] 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 14957b42bc5d2a0018b3c60d316be87bc81849cd Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 21 Feb 2026 14:32:45 +0100 Subject: [PATCH 15/18] 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 b566753e4744184abe874ea29c404569290cb8cd Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 21 Feb 2026 14:47:07 +0100 Subject: [PATCH 16/18] 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 0f2e6034cea1a50ef2b455a1c2e166c27378ff97 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 21 Feb 2026 16:00:11 +0100 Subject: [PATCH 17/18] 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 37cbf7f51998d78f94d4b8c48788a41eb97cff2d Mon Sep 17 00:00:00 2001 From: 0ko <0ko@noreply.codeberg.org> Date: Mon, 9 Mar 2026 09:56:33 +0500 Subject: [PATCH 18/18] [v14.0/forgejo] fix(e2e): use empty user for overflow menu test (from #11337) Potentially fixes one of the E2E failures that started to occur lately where the last tab (Starred repos) can overflow on user2's profile on CI. Replaced the user by the one that has no counters on the tabs. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11337 Reviewed-by: Gusted Reviewed-by: Otto Co-authored-by: 0ko <0ko@noreply.codeberg.org> Co-committed-by: 0ko <0ko@noreply.codeberg.org> (cherry picked from commit 32c6f64f395fc53c781e10a68ad4c0f813130240) --- tests/e2e/right-settings-button.test.e2e.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/e2e/right-settings-button.test.e2e.ts b/tests/e2e/right-settings-button.test.e2e.ts index 8daeddacf2..1e19fdae8c 100644 --- a/tests/e2e/right-settings-button.test.e2e.ts +++ b/tests/e2e/right-settings-button.test.e2e.ts @@ -50,7 +50,7 @@ test.describe('desktop viewport, unauthenticated', () => { test.use({viewport: {width: 1920, height: 300}}); test('User overview overflow menu should not be influenced', async ({page}) => { - await page.goto('/user2'); + await page.goto('/user8'); await expect(page.locator('.overflow-menu-items>#settings-btn')).toHaveCount(0);