From 7e6db8ee8a409a15f387e226534b40d90f919db2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Sat, 13 Dec 2025 15:15:56 +0100 Subject: [PATCH 01/37] editor: implement fork-on-edit workflow for article editing * set fork-related context data * show submit button based on permissions * add ForkAndEdit field to EditRepoFileForm * modify EditFilePost to handle fork creation and commit to fork * reuse existing fork if user already has one --- custom/templates/shared/repo/article.tmpl | 40 ++++++++++++----- custom/templates/shared/repo/edit.tmpl | 7 ++- routers/web/explore/repo.go | 39 +++++++++++++++++ routers/web/repo/editor.go | 52 ++++++++++++++++++++++- services/forms/repo_form_editor.go | 3 +- 5 files changed, 127 insertions(+), 14 deletions(-) diff --git a/custom/templates/shared/repo/article.tmpl b/custom/templates/shared/repo/article.tmpl index 510afe8bc2..da506bc150 100644 --- a/custom/templates/shared/repo/article.tmpl +++ b/custom/templates/shared/repo/article.tmpl @@ -66,22 +66,40 @@
{{if .IsSigned}} -
- + {{else if .HasExistingFork}} + {{/* User has an existing fork - submit to fork */}} + + + ({{.ExistingFork.OwnerName}}/{{.ExistingFork.Name}}) + + {{else if .NeedsFork}} + {{/* User needs to create a fork - show fork-and-submit button */}} + + {{else}} + {{/* Fallback - show disabled button */}} + -
+ {{end}} {{else}} - {{end}} -
{{end}} diff --git a/custom/templates/shared/repo/edit.tmpl b/custom/templates/shared/repo/edit.tmpl index 1332bff29b..22b341cea5 100644 --- a/custom/templates/shared/repo/edit.tmpl +++ b/custom/templates/shared/repo/edit.tmpl @@ -40,7 +40,10 @@ {{else}} {{/* Editor form */}} -
+ {{.CsrfTokenHtml}} @@ -49,6 +52,8 @@ {{/* Optional commit message fields - will use default if empty */}} + {{/* Fork-on-edit field - set by JavaScript based on user's choice */}} +
diff --git a/routers/web/explore/repo.go b/routers/web/explore/repo.go index e1c7bc1450..ab4306438a 100644 --- a/routers/web/explore/repo.go +++ b/routers/web/explore/repo.go @@ -626,6 +626,9 @@ func prepareArticleView(ctx *context.Context, gitRepo *git.Repository, entries [ } } ctx.Data["FileSize"] = fileSize + + // Set up fork-on-edit context data + prepareArticleForkOnEditData(ctx) case "history": // For history mode, get file commit history commitsCount, err := gitRepo.FileCommitsCount(defaultBranch, readmeTreePath) @@ -734,3 +737,39 @@ func getFileContributorCount(gitRepo *git.Repository, branch, filePath string) ( return int64(len(lines)), nil } + +// prepareArticleForkOnEditData sets up context data for fork-on-edit workflow +// This determines whether the user can edit directly, needs to fork, or already has a fork +func prepareArticleForkOnEditData(ctx *context.Context) { + // Default values + ctx.Data["NeedsFork"] = false + ctx.Data["HasExistingFork"] = false + ctx.Data["ExistingFork"] = nil + ctx.Data["IsRepoOwner"] = false + + // If user is not signed in, they can't edit at all + if ctx.Doer == nil { + return + } + + repo := ctx.Repo.Repository + + // Check if user owns the repository + isOwner := repo.OwnerID == ctx.Doer.ID + ctx.Data["IsRepoOwner"] = isOwner + + if isOwner { + // User can edit directly, no fork needed + return + } + + // User cannot write directly - check for existing fork + existingFork := repo_model.GetForkedRepo(ctx, ctx.Doer.ID, repo.ID) + if existingFork != nil { + ctx.Data["HasExistingFork"] = true + ctx.Data["ExistingFork"] = existingFork + } else { + // User needs to create a fork + ctx.Data["NeedsFork"] = true + } +} diff --git a/routers/web/repo/editor.go b/routers/web/repo/editor.go index b58b12300e..4769ce705e 100644 --- a/routers/web/repo/editor.go +++ b/routers/web/repo/editor.go @@ -13,6 +13,7 @@ import ( git_model "code.gitea.io/gitea/models/git" "code.gitea.io/gitea/models/issues" + repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/modules/charset" "code.gitea.io/gitea/modules/git" @@ -26,6 +27,7 @@ import ( "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/context/upload" "code.gitea.io/gitea/services/forms" + repo_service "code.gitea.io/gitea/services/repository" files_service "code.gitea.io/gitea/services/repository/files" ) @@ -362,7 +364,18 @@ func EditFilePost(ctx *context.Context) { return } - _, err := files_service.ChangeRepoFiles(ctx, ctx.Repo.Repository, ctx.Doer, &files_service.ChangeRepoFilesOptions{ + // Determine target repository - either the original or a fork + targetRepo := ctx.Repo.Repository + + // Handle fork-and-edit workflow + if parsed.form.ForkAndEdit { + targetRepo = handleForkAndEdit(ctx, parsed.form) + if ctx.Written() { + return + } + } + + _, err := files_service.ChangeRepoFiles(ctx, targetRepo, ctx.Doer, &files_service.ChangeRepoFilesOptions{ LastCommitID: parsed.form.LastCommit, OldBranch: parsed.OldBranchName, NewBranch: parsed.NewBranchName, @@ -384,9 +397,46 @@ func EditFilePost(ctx *context.Context) { return } + // If we committed to a fork, redirect to the fork's article page + if parsed.form.ForkAndEdit && targetRepo != nil { + ctx.JSONRedirect(targetRepo.Link() + "?mode=read") + return + } + redirectForCommitChoice(ctx, parsed, parsed.form.TreePath) } +// handleForkAndEdit handles the fork-and-edit workflow +// It returns the fork repository to commit to, or nil if an error occurred +func handleForkAndEdit(ctx *context.Context, form *forms.EditRepoFileForm) *repo_model.Repository { + originalRepo := ctx.Repo.Repository + + // Check if user already has a fork + existingFork := repo_model.GetForkedRepo(ctx, ctx.Doer.ID, originalRepo.ID) + if existingFork != nil { + return existingFork + } + + // Create a new fork + forkName := getUniqueRepositoryName(ctx, ctx.Doer.ID, originalRepo.Name) + if forkName == "" { + ctx.JSONError(ctx.Tr("repo.fork.failed")) + return nil + } + + fork := ForkRepoTo(ctx, ctx.Doer, repo_service.ForkRepoOptions{ + BaseRepo: originalRepo, + Name: forkName, + Description: originalRepo.Description, + SingleBranch: originalRepo.DefaultBranch, + }) + if ctx.Written() { + return nil + } + + return fork +} + // DeleteFile render delete file page func DeleteFile(ctx *context.Context) { prepareEditorCommitFormOptions(ctx, "_delete") diff --git a/services/forms/repo_form_editor.go b/services/forms/repo_form_editor.go index 3ad2eae75d..01159ff6c9 100644 --- a/services/forms/repo_form_editor.go +++ b/services/forms/repo_form_editor.go @@ -39,7 +39,8 @@ func (f *CommitCommonForm) GetCommitCommonForm() *CommitCommonForm { type EditRepoFileForm struct { CommitCommonForm - Content optional.Option[string] + Content optional.Option[string] + ForkAndEdit bool // If true, fork the repository first and commit to the fork } type DeleteRepoFileForm struct { From fa785a69ba251e8a62ccf65ff39cca1291efe0ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Sat, 13 Dec 2025 16:04:04 +0100 Subject: [PATCH 02/37] editor: prevent editing fork when user owns repo for same subject * query repos by owner and subject * check for existing user repo for subject * disable edit/fork buttons when user already has a repo for the subject * show message in edit view explaining why editing is disabled --- custom/templates/shared/repo/article.tmpl | 8 +++++++- custom/templates/shared/repo/edit.tmpl | 18 ++++++++++++++++++ models/repo/repo.go | 17 +++++++++++++++++ routers/web/explore/repo.go | 19 +++++++++++++++++++ 4 files changed, 61 insertions(+), 1 deletion(-) diff --git a/custom/templates/shared/repo/article.tmpl b/custom/templates/shared/repo/article.tmpl index da506bc150..f748f5bf6a 100644 --- a/custom/templates/shared/repo/article.tmpl +++ b/custom/templates/shared/repo/article.tmpl @@ -66,7 +66,13 @@
{{if .IsSigned}} - {{if .IsRepoOwner}} + {{if .HasOwnRepoForSubject}} + {{/* User already owns a different repository for this subject - disable editing */}} + + {{else if .IsRepoOwner}} {{/* User can edit directly - show simple submit button */}}
+ {{else if .HasOwnRepoForSubject}} + {{/* User already owns a different repository for this subject - show message instead of editor */}} +
+
+ {{svg "octicon-info" 16 "tw-mr-2"}} + You already have an article for this subject +
+

+ You cannot edit or fork this article because you already own a fork for the subject + "{{if .Repository.SubjectRelation}}{{.Repository.SubjectRelation.Name}}{{else}}{{.Repository.Name}}{{end}}". +

+

+ Your article: {{.OwnRepoForSubject.OwnerName}} / {{if .Repository.SubjectRelation}}{{.Repository.SubjectRelation.Name}}{{else}}{{.Repository.Name}}{{end}} +

+

+ In Forkana, each user can only have one article per subject. If you want to incorporate changes from this article, you can view it and manually update your own. +

+
{{else}} {{/* Editor form */}} 0 { + ownRepo, err := repo_model.GetRepositoryByOwnerIDAndSubjectID(ctx, ctx.Doer.ID, repo.SubjectID) + if err != nil { + ctx.ServerError("GetRepositoryByOwnerIDAndSubjectID", err) + return + } + if ownRepo != nil && ownRepo.ID != repo.ID { + // User already owns a different repository for this subject + // Disable all editing options + ctx.Data["HasOwnRepoForSubject"] = true + ctx.Data["OwnRepoForSubject"] = ownRepo + return + } + } + // User cannot write directly - check for existing fork existingFork := repo_model.GetForkedRepo(ctx, ctx.Doer.ID, repo.ID) if existingFork != nil { From a64a811c265d2634ff3991131091928818bb79a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Mon, 15 Dec 2025 22:17:00 +0100 Subject: [PATCH 03/37] editor: consolidate fork-on-edit permission logic * fix POST handler to validate subject ownership --- routers/web/explore/repo.go | 49 ++++++---------------------- routers/web/repo/editor.go | 20 +++++++++--- services/repository/fork.go | 65 +++++++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 43 deletions(-) diff --git a/routers/web/explore/repo.go b/routers/web/explore/repo.go index a564aa6931..f173165c54 100644 --- a/routers/web/explore/repo.go +++ b/routers/web/explore/repo.go @@ -749,46 +749,17 @@ func prepareArticleForkOnEditData(ctx *context.Context) { ctx.Data["HasOwnRepoForSubject"] = false ctx.Data["OwnRepoForSubject"] = nil - // If user is not signed in, they can't edit at all - if ctx.Doer == nil { - return - } - - repo := ctx.Repo.Repository - - // Check if user owns the repository - isOwner := repo.OwnerID == ctx.Doer.ID - ctx.Data["IsRepoOwner"] = isOwner - - if isOwner { - // User can edit directly, no fork needed + perms, err := repo_service.CheckForkOnEditPermissions(ctx, ctx.Doer, ctx.Repo.Repository) + if err != nil { + ctx.ServerError("CheckForkOnEditPermissions", err) return } - // Check if user already owns a different repository for the same subject - // In Forkana, each user should only have one repository per subject - if repo.SubjectID > 0 { - ownRepo, err := repo_model.GetRepositoryByOwnerIDAndSubjectID(ctx, ctx.Doer.ID, repo.SubjectID) - if err != nil { - ctx.ServerError("GetRepositoryByOwnerIDAndSubjectID", err) - return - } - if ownRepo != nil && ownRepo.ID != repo.ID { - // User already owns a different repository for this subject - // Disable all editing options - ctx.Data["HasOwnRepoForSubject"] = true - ctx.Data["OwnRepoForSubject"] = ownRepo - return - } - } - - // User cannot write directly - check for existing fork - existingFork := repo_model.GetForkedRepo(ctx, ctx.Doer.ID, repo.ID) - if existingFork != nil { - ctx.Data["HasExistingFork"] = true - ctx.Data["ExistingFork"] = existingFork - } else { - // User needs to create a fork - ctx.Data["NeedsFork"] = true - } + // Map permissions to context data + ctx.Data["IsRepoOwner"] = perms.IsRepoOwner + ctx.Data["HasOwnRepoForSubject"] = perms.BlockedBySubject + ctx.Data["OwnRepoForSubject"] = perms.OwnRepoForSubject + ctx.Data["HasExistingFork"] = perms.HasExistingFork + ctx.Data["ExistingFork"] = perms.ExistingFork + ctx.Data["NeedsFork"] = perms.NeedsFork } diff --git a/routers/web/repo/editor.go b/routers/web/repo/editor.go index 4769ce705e..e58016a2c6 100644 --- a/routers/web/repo/editor.go +++ b/routers/web/repo/editor.go @@ -411,10 +411,22 @@ func EditFilePost(ctx *context.Context) { func handleForkAndEdit(ctx *context.Context, form *forms.EditRepoFileForm) *repo_model.Repository { originalRepo := ctx.Repo.Repository - // Check if user already has a fork - existingFork := repo_model.GetForkedRepo(ctx, ctx.Doer.ID, originalRepo.ID) - if existingFork != nil { - return existingFork + // Prevent bypassing UI restrictions + perms, err := repo_service.CheckForkOnEditPermissions(ctx, ctx.Doer, originalRepo) + if err != nil { + ctx.ServerError("CheckForkOnEditPermissions", err) + return nil + } + + // Block if user already owns a different repository for the same subject + if perms.BlockedBySubject { + ctx.JSONError(ctx.Tr("repo.fork.already_own_subject_repo")) + return nil + } + + // Return existing fork if user already has one + if perms.HasExistingFork && perms.ExistingFork != nil { + return perms.ExistingFork } // Create a new fork diff --git a/services/repository/fork.go b/services/repository/fork.go index 8ddf3f8dc9..19e8c8e6fd 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -27,6 +27,71 @@ import ( "xorm.io/builder" ) +// ForkOnEditPermissions contains the permission state for fork-on-edit workflow. +// This struct consolidates all permission checks needed to determine how a user +// can edit a repository they don't own. +type ForkOnEditPermissions struct { + // IsRepoOwner is true if the user owns the repository + IsRepoOwner bool + // CanEditDirectly is true if the user can commit directly to the repository + CanEditDirectly bool + // NeedsFork is true if the user needs to create a fork to edit + NeedsFork bool + // HasExistingFork is true if the user already has a fork of this repository + HasExistingFork bool + // ExistingFork is the user's existing fork (nil if none) + ExistingFork *repo_model.Repository + // BlockedBySubject is true if the user already owns a different repo for the same subject + BlockedBySubject bool + // OwnRepoForSubject is the user's existing repo for the subject (nil if none) + OwnRepoForSubject *repo_model.Repository +} + +// CheckForkOnEditPermissions determines the user's editing permissions for a repository. +// It checks ownership, subject ownership restrictions, and existing forks. +// Returns nil permissions if doer is nil (not signed in). +func CheckForkOnEditPermissions(ctx context.Context, doer *user_model.User, repo *repo_model.Repository) (*ForkOnEditPermissions, error) { + perms := &ForkOnEditPermissions{} + + // Not signed in - no permissions + if doer == nil { + return perms, nil + } + + // Check if user owns the repository + if repo.OwnerID == doer.ID { + perms.IsRepoOwner = true + perms.CanEditDirectly = true + return perms, nil + } + + // Check if user already owns a different repository for the same subject + // In Forkana, each user should only have one repository per subject + if repo.SubjectID > 0 { + ownRepo, err := repo_model.GetRepositoryByOwnerIDAndSubjectID(ctx, doer.ID, repo.SubjectID) + if err != nil { + return nil, err + } + if ownRepo != nil && ownRepo.ID != repo.ID { + // User already owns a different repository for this subject + perms.BlockedBySubject = true + perms.OwnRepoForSubject = ownRepo + return perms, nil + } + } + + // Check for existing fork + existingFork := repo_model.GetForkedRepo(ctx, doer.ID, repo.ID) + if existingFork != nil { + perms.HasExistingFork = true + perms.ExistingFork = existingFork + } else { + perms.NeedsFork = true + } + + return perms, nil +} + // ErrForkAlreadyExist represents a "ForkAlreadyExist" kind of error. type ErrForkAlreadyExist struct { Uname string From 420c61827b8640c4972e1a2cdce40e4c07fe96e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Mon, 15 Dec 2025 22:35:20 +0100 Subject: [PATCH 04/37] templates: update article editing logic --- custom/templates/shared/repo/article.tmpl | 29 +++++++++++++---------- 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/custom/templates/shared/repo/article.tmpl b/custom/templates/shared/repo/article.tmpl index f748f5bf6a..c57e5eb923 100644 --- a/custom/templates/shared/repo/article.tmpl +++ b/custom/templates/shared/repo/article.tmpl @@ -66,7 +66,16 @@
{{if .IsSigned}} - {{if .HasOwnRepoForSubject}} + {{if .HasExistingFork}} + {{/* User has an existing fork - redirect to fork's article page */}} + + {{svg "octicon-repo-forked" 16 "tw-mr-1"}} + Edit in Your Fork + + + ({{.ExistingFork.OwnerName}}/{{.ExistingFork.Name}}) + + {{else if .HasOwnRepoForSubject}} {{/* User already owns a different repository for this subject - disable editing */}} - {{else if .HasExistingFork}} - {{/* User has an existing fork - submit to fork */}} - - - ({{.ExistingFork.OwnerName}}/{{.ExistingFork.Name}}) - {{else if .NeedsFork}} {{/* User needs to create a fork - show fork-and-submit button */}} - + {{/* Submit Change Request - not implemented yet, disabled for now */}} + {{else}} {{/* Fallback - show disabled button */}} From 1f5ef1d93aabaae55246a223ba9cc883e12730c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Tue, 16 Dec 2025 10:01:50 +0100 Subject: [PATCH 05/37] migrations: add composite indexes for fork-on-edit optimization * create IDX_repository_owner_subject and IDX_repository_owner_fork * optimizes GetRepositoryByOwnerIDAndSubjectID() and GetForkedRepo() queries --- models/migrations/custom/v1_25_custom/v327.go | 43 +++++++++++++++++++ models/migrations/migrations.go | 1 + 2 files changed, 44 insertions(+) create mode 100644 models/migrations/custom/v1_25_custom/v327.go diff --git a/models/migrations/custom/v1_25_custom/v327.go b/models/migrations/custom/v1_25_custom/v327.go new file mode 100644 index 0000000000..abf1444e8d --- /dev/null +++ b/models/migrations/custom/v1_25_custom/v327.go @@ -0,0 +1,43 @@ +// Copyright 2025 okTurtles Foundation. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_25_custom + +import ( + "xorm.io/xorm" + "xorm.io/xorm/schemas" +) + +// repositoryForkOnEditIndexes is a temporary struct used only for this migration. +// It defines composite indexes to optimize fork-on-edit permission queries: +// - IDX_repository_owner_subject: for GetRepositoryByOwnerIDAndSubjectID() +// - IDX_repository_owner_fork: for GetForkedRepo() +type repositoryForkOnEditIndexes struct { + ID int64 `xorm:"pk autoincr"` + OwnerID int64 + SubjectID int64 + ForkID int64 +} + +func (*repositoryForkOnEditIndexes) TableName() string { + return "repository" +} + +func (*repositoryForkOnEditIndexes) TableIndices() []*schemas.Index { + // Composite index for GetRepositoryByOwnerIDAndSubjectID() + // Query: WHERE owner_id = ? AND subject_id = ? + ownerSubjectIndex := schemas.NewIndex("IDX_repository_owner_subject", schemas.IndexType) + ownerSubjectIndex.AddColumn("owner_id", "subject_id") + + // Composite index for GetForkedRepo() + // Query: WHERE owner_id = ? AND fork_id = ? + ownerForkIndex := schemas.NewIndex("IDX_repository_owner_fork", schemas.IndexType) + ownerForkIndex.AddColumn("owner_id", "fork_id") + + return []*schemas.Index{ownerSubjectIndex, ownerForkIndex} +} + +// AddCompositeIndexesForForkOnEdit adds composite indexes to optimize fork-on-edit permission queries. +func AddCompositeIndexesForForkOnEdit(x *xorm.Engine) error { + return x.Sync(new(repositoryForkOnEditIndexes)) +} diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 2d45e04a6a..be17f4e484 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -401,6 +401,7 @@ func prepareMigrationTasks() []*migration { newMigration(324, "Forkana: create subjects table and populate with existing data", v1_25_custom.CreateSubjectsTable), newMigration(325, "Forkana: add subject_id foreign key to repository table", v1_25_custom.AddSubjectForeignKeyToRepository), newMigration(326, "Forkana: add slug column to subjects table", v1_25_custom.AddSubjectSlugColumn), + newMigration(327, "Forkana: add composite indexes for fork-on-edit optimization", v1_25_custom.AddCompositeIndexesForForkOnEdit), } return preparedMigrations } From 49e1cf361d38b3d1da44c8e3c24d97ec98a9bf2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Tue, 16 Dec 2025 10:16:23 +0100 Subject: [PATCH 06/37] editor: remove unused form parameter from handleForkAndEdit --- routers/web/repo/editor.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/routers/web/repo/editor.go b/routers/web/repo/editor.go index e58016a2c6..14af52fea4 100644 --- a/routers/web/repo/editor.go +++ b/routers/web/repo/editor.go @@ -369,7 +369,7 @@ func EditFilePost(ctx *context.Context) { // Handle fork-and-edit workflow if parsed.form.ForkAndEdit { - targetRepo = handleForkAndEdit(ctx, parsed.form) + targetRepo = handleForkAndEdit(ctx) if ctx.Written() { return } @@ -408,7 +408,7 @@ func EditFilePost(ctx *context.Context) { // handleForkAndEdit handles the fork-and-edit workflow // It returns the fork repository to commit to, or nil if an error occurred -func handleForkAndEdit(ctx *context.Context, form *forms.EditRepoFileForm) *repo_model.Repository { +func handleForkAndEdit(ctx *context.Context) *repo_model.Repository { originalRepo := ctx.Repo.Repository // Prevent bypassing UI restrictions From bd3ef945f2bf186d275d5a05bf01b8551d91e83b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Tue, 16 Dec 2025 10:41:47 +0100 Subject: [PATCH 07/37] editor: fix getUniqueRepositoryName bugs and optimize * fix wrong error condition (returned on any error, not just 'not found') * fix double increment bug (i++ in loop body + loop increment) * optimise from O(n) queries to single query with hash set lookup O(1) * add proper error logging --- routers/web/repo/editor_util.go | 42 ++++++++++++++++++++++++++------- 1 file changed, 34 insertions(+), 8 deletions(-) diff --git a/routers/web/repo/editor_util.go b/routers/web/repo/editor_util.go index f910f0bd40..e61d962da6 100644 --- a/routers/web/repo/editor_util.go +++ b/routers/web/repo/editor_util.go @@ -9,6 +9,7 @@ import ( "path" "strings" + "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" @@ -86,18 +87,43 @@ func getParentTreeFields(treePath string) (treeNames, treePaths []string) { return treeNames, treePaths } -// getUniqueRepositoryName Gets a unique repository name for a user -// It will append a - postfix if the name is already taken +// getUniqueRepositoryName Gets a unique repository name for a user. +// It will append a - postfix if the name is already taken. +// Uses a single query to fetch all matching names, then finds the first available. func getUniqueRepositoryName(ctx context.Context, ownerID int64, name string) string { - uniqueName := name + // Single query to find all existing repository names for this owner + // that start with the base name (case-insensitive) + var existingNames []string + lowerName := strings.ToLower(name) + err := db.GetEngine(ctx).Table("repository"). + Where("owner_id = ?", ownerID). + And("lower_name LIKE ?", lowerName+"%"). + Cols("lower_name"). + Find(&existingNames) + if err != nil { + log.Error("getUniqueRepositoryName: failed to query existing names: %v", err) + return "" + } + + // Build a set for O(1) lookup + nameSet := make(map[string]bool, len(existingNames)) + for _, n := range existingNames { + nameSet[n] = true + } + + // Check if base name is available + if !nameSet[lowerName] { + return name + } + + // Find first available name with - suffix for i := 1; i < 1000; i++ { - _, err := repo_model.GetRepositoryByName(ctx, ownerID, uniqueName) - if err != nil || repo_model.IsErrRepoNotExist(err) { - return uniqueName + candidate := fmt.Sprintf("%s-%d", name, i) + if !nameSet[strings.ToLower(candidate)] { + return candidate } - uniqueName = fmt.Sprintf("%s-%d", name, i) - i++ } + return "" } From ba5bb2aacdc8098de74a502e557f856f4d905913 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Tue, 16 Dec 2025 11:29:36 +0100 Subject: [PATCH 08/37] fork: parallelize permission queries * run subject ownership check and fork detection concurrently --- services/repository/fork.go | 46 ++++++++++++++++++++++++++----------- 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/services/repository/fork.go b/services/repository/fork.go index 19e8c8e6fd..cbefa4b56c 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -24,6 +24,7 @@ import ( "code.gitea.io/gitea/modules/util" notify_service "code.gitea.io/gitea/services/notify" + "golang.org/x/sync/errgroup" "xorm.io/builder" ) @@ -65,23 +66,42 @@ func CheckForkOnEditPermissions(ctx context.Context, doer *user_model.User, repo return perms, nil } - // Check if user already owns a different repository for the same subject - // In Forkana, each user should only have one repository per subject + // Run subject ownership check and fork detection in parallel + // These queries are independent and can be executed concurrently + var ownRepo *repo_model.Repository + var existingFork *repo_model.Repository + + g, gCtx := errgroup.WithContext(ctx) + + // Check if user owns a different repository for the same subject if repo.SubjectID > 0 { - ownRepo, err := repo_model.GetRepositoryByOwnerIDAndSubjectID(ctx, doer.ID, repo.SubjectID) - if err != nil { - return nil, err - } - if ownRepo != nil && ownRepo.ID != repo.ID { - // User already owns a different repository for this subject - perms.BlockedBySubject = true - perms.OwnRepoForSubject = ownRepo - return perms, nil - } + g.Go(func() error { + var err error + ownRepo, err = repo_model.GetRepositoryByOwnerIDAndSubjectID(gCtx, doer.ID, repo.SubjectID) + return err + }) } // Check for existing fork - existingFork := repo_model.GetForkedRepo(ctx, doer.ID, repo.ID) + g.Go(func() error { + existingFork = repo_model.GetForkedRepo(gCtx, doer.ID, repo.ID) + return nil // GetForkedRepo doesn't return an error + }) + + // Wait for both queries to complete + if err := g.Wait(); err != nil { + return nil, err + } + + // Process subject ownership result + if ownRepo != nil && ownRepo.ID != repo.ID { + // User already owns a different repository for this subject + perms.BlockedBySubject = true + perms.OwnRepoForSubject = ownRepo + return perms, nil + } + + // Process fork detection result if existingFork != nil { perms.HasExistingFork = true perms.ExistingFork = existingFork From 8b5c32dee917b9ddef88aa52e8f0c9642f07ffc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Tue, 16 Dec 2025 12:18:54 +0100 Subject: [PATCH 09/37] fork: add ErrUserOwnsSubjectRepo error type * add error for subject ownership violations * validate subject ownership before forking --- routers/api/v1/repo/fork.go | 2 +- routers/web/repo/fork.go | 2 ++ services/repository/fork.go | 39 +++++++++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 1 deletion(-) diff --git a/routers/api/v1/repo/fork.go b/routers/api/v1/repo/fork.go index eab45ef392..787abeb663 100644 --- a/routers/api/v1/repo/fork.go +++ b/routers/api/v1/repo/fork.go @@ -160,7 +160,7 @@ func CreateFork(ctx *context.APIContext) { if err != nil { if errors.Is(err, util.ErrAlreadyExist) || repo_model.IsErrReachLimitOfRepo(err) { ctx.APIError(http.StatusConflict, err) - } else if errors.Is(err, user_model.ErrBlockedUser) || repo_model.IsErrForkTreeTooLarge(err) { + } else if errors.Is(err, user_model.ErrBlockedUser) || repo_model.IsErrForkTreeTooLarge(err) || repo_service.IsErrUserOwnsSubjectRepo(err) { ctx.APIError(http.StatusForbidden, err) } else { ctx.APIErrorInternal(err) diff --git a/routers/web/repo/fork.go b/routers/web/repo/fork.go index 293d966f72..438cc1ae6a 100644 --- a/routers/web/repo/fork.go +++ b/routers/web/repo/fork.go @@ -231,6 +231,8 @@ func ForkRepoTo(ctx *context.Context, owner *user_model.User, forkOpts repo_serv ctx.JSONError(ctx.Tr("repo.fork.blocked_user")) case repo_model.IsErrForkTreeTooLarge(err): ctx.JSONError(ctx.Tr("repo.fork.tree_size_limit_reached")) + case repo_service.IsErrUserOwnsSubjectRepo(err): + ctx.JSONError(ctx.Tr("repo.fork.already_own_subject_repo")) default: ctx.ServerError("ForkPost", err) } diff --git a/services/repository/fork.go b/services/repository/fork.go index cbefa4b56c..aa437df28e 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -133,6 +133,29 @@ func (err ErrForkAlreadyExist) Unwrap() error { return util.ErrAlreadyExist } +// ErrUserOwnsSubjectRepo represents an error when a user already owns a different +// repository for the same subject and cannot fork/edit another repository for that subject. +type ErrUserOwnsSubjectRepo struct { + UserID int64 + SubjectID int64 + ExistingRepoID int64 +} + +// IsErrUserOwnsSubjectRepo checks if an error is an ErrUserOwnsSubjectRepo. +func IsErrUserOwnsSubjectRepo(err error) bool { + _, ok := err.(ErrUserOwnsSubjectRepo) + return ok +} + +func (err ErrUserOwnsSubjectRepo) Error() string { + return fmt.Sprintf("user already owns repository for subject [user_id: %d, subject_id: %d, existing_repo_id: %d]", + err.UserID, err.SubjectID, err.ExistingRepoID) +} + +func (err ErrUserOwnsSubjectRepo) Unwrap() error { + return util.ErrAlreadyExist +} + // ForkRepoOptions contains the fork repository options type ForkRepoOptions struct { BaseRepo *repo_model.Repository @@ -203,6 +226,22 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork return nil, err } + // Check if user already owns a different repository for the same subject + // In Forkana, each user should only have one repository per subject + if opts.BaseRepo.SubjectID > 0 { + ownRepo, err := repo_model.GetRepositoryByOwnerIDAndSubjectID(ctx, owner.ID, opts.BaseRepo.SubjectID) + if err != nil { + return nil, err + } + if ownRepo != nil && ownRepo.ID != opts.BaseRepo.ID { + return nil, ErrUserOwnsSubjectRepo{ + UserID: owner.ID, + SubjectID: opts.BaseRepo.SubjectID, + ExistingRepoID: ownRepo.ID, + } + } + } + forkedRepo, err := repo_model.GetUserFork(ctx, opts.BaseRepo.ID, owner.ID) if err != nil { return nil, err From ecac0929555497393c2665098c96160f32fb3839 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Tue, 16 Dec 2025 16:13:33 +0100 Subject: [PATCH 10/37] editor: improve fork-on-edit error messages in templates * replace 'Cannot Edit' button with 'Edit Your Article' action link * add translation keys for fork-on-edit error messages * link users to their existing article when subject ownership blocks editing --- custom/options/locale/locale_en-US.ini | 4 ++++ custom/templates/shared/repo/article.tmpl | 10 +++++----- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/custom/options/locale/locale_en-US.ini b/custom/options/locale/locale_en-US.ini index dcba82221b..ecea4de9df 100644 --- a/custom/options/locale/locale_en-US.ini +++ b/custom/options/locale/locale_en-US.ini @@ -1087,6 +1087,10 @@ view_all_tags = View all tags fork_no_valid_owners = This repository cannot be forked because there are no valid owners. fork.blocked_user = Cannot fork the repository because you are blocked by the repository owner. fork.tree_size_limit_reached = Cannot fork the repository because the fork tree has reached its maximum size limit. +fork.already_own_subject_repo = You already have an article for this subject. You can only have one article per subject. +fork.failed = Failed to create fork. Please try again. +editor.edit_your_article = Edit Your Article +editor.already_have_article = You already have an article for this subject use_template = Use this template open_with_editor = Open with %s download_zip = Download ZIP diff --git a/custom/templates/shared/repo/article.tmpl b/custom/templates/shared/repo/article.tmpl index c57e5eb923..bc488d9b0c 100644 --- a/custom/templates/shared/repo/article.tmpl +++ b/custom/templates/shared/repo/article.tmpl @@ -76,11 +76,11 @@ ({{.ExistingFork.OwnerName}}/{{.ExistingFork.Name}}) {{else if .HasOwnRepoForSubject}} - {{/* User already owns a different repository for this subject - disable editing */}} - + {{/* User already owns a different repository for this subject - show link to their article */}} + + {{svg "octicon-pencil" 16 "tw-mr-1"}} + {{ctx.Locale.Tr "repo.editor.edit_your_article"}} + {{else if .IsRepoOwner}} {{/* User can edit directly - show simple submit button */}} {{else if .NeedsFork}} {{/* User needs to create a fork - show fork-and-submit button */}} {{/* Submit Change Request - not implemented yet, disabled for now */}} {{else}} {{/* Fallback - show disabled button */}} {{end}} {{else}} {{end}}
From 35c540aa5f7038c38c4b6bc579268672dac461b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Thu, 18 Dec 2025 15:52:27 +0100 Subject: [PATCH 12/37] templates: add translation keys and simplify logic * simplify fork_and_edit hidden field to only use .NeedsFork * extract repeated nil-guard pattern into $subjectName template variable --- custom/options/locale/locale_en-US.ini | 5 +++++ custom/templates/shared/repo/edit.tmpl | 18 +++++++++--------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/custom/options/locale/locale_en-US.ini b/custom/options/locale/locale_en-US.ini index 1095e568bc..a598be9c68 100644 --- a/custom/options/locale/locale_en-US.ini +++ b/custom/options/locale/locale_en-US.ini @@ -1101,6 +1101,11 @@ editor.edit_in_your_fork = Edit in Your Fork editor.submit_changes = Submit Changes editor.sign_in_to_edit = Sign in to Edit editor.already_have_article = You already have an article for this subject +editor.cannot_edit_own_subject = You cannot edit or fork this article because you already own a fork for the subject "%s". +editor.your_article = Your article: +editor.one_article_per_subject = In Forkana, each user can only have one article per subject. If you want to incorporate changes from this article, you can view it and manually update your own. +editor.forked_of = Forked of: +editor.copy_ref = Copy Ref use_template = Use this template open_with_editor = Open with %s download_zip = Download ZIP diff --git a/custom/templates/shared/repo/edit.tmpl b/custom/templates/shared/repo/edit.tmpl index 3e41b7120d..fcfaa351ca 100644 --- a/custom/templates/shared/repo/edit.tmpl +++ b/custom/templates/shared/repo/edit.tmpl @@ -15,7 +15,7 @@ {{if .Repository.IsFork}} {{svg "octicon-repo-forked" 14 "tw-mr-1"}} - Forked of: + {{ctx.Locale.Tr "repo.editor.forked_of"}} {{if .Repository.BaseRepo}} {{/* Go html/template automatically HTML-escapes these strings to prevent XSS */}} {{.Repository.BaseRepo.OwnerName}} / {{.Repository.BaseRepo.Name}} @@ -29,7 +29,7 @@
- {{svg "octicon-link" 14 "tw-mr-1"}} CopyRef + {{svg "octicon-link" 14 "tw-mr-1"}} {{ctx.Locale.Tr "repo.editor.copy_ref"}}
@@ -40,20 +40,20 @@
{{else if .HasOwnRepoForSubject}} {{/* User already owns a different repository for this subject - show message instead of editor */}} + {{$subjectName := Iif .Repository.SubjectRelation .Repository.SubjectRelation.Name .Repository.Name}}
{{svg "octicon-info" 16 "tw-mr-2"}} - You already have an article for this subject + {{ctx.Locale.Tr "repo.editor.already_have_article"}}

- You cannot edit or fork this article because you already own a fork for the subject - "{{if .Repository.SubjectRelation}}{{.Repository.SubjectRelation.Name}}{{else}}{{.Repository.Name}}{{end}}". + {{ctx.Locale.Tr "repo.editor.cannot_edit_own_subject" $subjectName}}

- Your article: {{.OwnRepoForSubject.OwnerName}} / {{if .Repository.SubjectRelation}}{{.Repository.SubjectRelation.Name}}{{else}}{{.Repository.Name}}{{end}} + {{ctx.Locale.Tr "repo.editor.your_article"}} {{.OwnRepoForSubject.OwnerName}} / {{$subjectName}}

- In Forkana, each user can only have one article per subject. If you want to incorporate changes from this article, you can view it and manually update your own. + {{ctx.Locale.Tr "repo.editor.one_article_per_subject"}}

{{else}} @@ -70,8 +70,8 @@ {{/* Optional commit message fields - will use default if empty */}} - {{/* Fork-on-edit field - set by JavaScript based on user's choice */}} - + {{/* Fork-on-edit field - only true when user needs to create a new fork (not when they have an existing fork, as that case shows a link instead of a form) */}} +
From b760e1a25c63a1572ea043b35baa85dad92c777e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Fri, 19 Dec 2025 16:40:10 +0100 Subject: [PATCH 13/37] article: add confirmation modal to fork article button --- custom/options/locale/locale_en-US.ini | 4 ++ custom/templates/shared/repo/article.tmpl | 8 +++- web_src/js/features/article-editor.ts | 57 +++++++++++++++++++++-- 3 files changed, 64 insertions(+), 5 deletions(-) diff --git a/custom/options/locale/locale_en-US.ini b/custom/options/locale/locale_en-US.ini index a598be9c68..2c886f296e 100644 --- a/custom/options/locale/locale_en-US.ini +++ b/custom/options/locale/locale_en-US.ini @@ -1096,6 +1096,10 @@ fork.tree_size_limit_reached = Cannot fork the repository because the fork tree fork.already_own_subject_repo = You already have an article for this subject. You can only have one article per subject. fork.failed = Failed to create fork. Please try again. fork_article = Fork +fork_article_confirm_title = Are you sure you want to Fork? +fork_article_confirm_body = Forking creates a separate version of the article under your control. Please do this only if you've made a genuine effort to collaborate with the owner and still strongly disagree with their decisions. If you'd like to collaborate on the article by making edits, return to the article, apply your changes, and submit. +fork_article_confirm_yes = Yes, Fork article +fork_article_confirm_cancel = Go back editor.edit_your_article = Edit Your Article editor.edit_in_your_fork = Edit in Your Fork editor.submit_changes = Submit Changes diff --git a/custom/templates/shared/repo/article.tmpl b/custom/templates/shared/repo/article.tmpl index 7034f325b6..f7b3de3c14 100644 --- a/custom/templates/shared/repo/article.tmpl +++ b/custom/templates/shared/repo/article.tmpl @@ -89,7 +89,13 @@ {{else if .NeedsFork}} {{/* User needs to create a fork - show fork-and-submit button */}} - diff --git a/web_src/js/features/article-editor.ts b/web_src/js/features/article-editor.ts index 72fb917146..e5ba16ab93 100644 --- a/web_src/js/features/article-editor.ts +++ b/web_src/js/features/article-editor.ts @@ -1,5 +1,40 @@ import {createToastEditor} from './toast-editor.ts'; import {submitFormFetchAction} from './common-fetch-action.ts'; +import {fomanticQuery} from '../modules/fomantic/base.ts'; +import {createElementFromHTML} from '../utils/dom.ts'; +import {svg} from '../svg.ts'; +import {html, htmlRaw} from '../utils/html.ts'; + +// Create a confirmation modal with custom button text +function createForkConfirmModal(title: string, body: string, confirmText: string, cancelText: string): HTMLElement { + return createElementFromHTML(html` +
+
${title}
+

${body}

+
+ + +
+
+ `.trim()); +} + +// Show fork confirmation modal and return a promise that resolves to true if confirmed +function showForkConfirmModal(title: string, body: string, confirmText: string, cancelText: string): Promise { + const modal = createForkConfirmModal(title, body, confirmText, cancelText); + return new Promise((resolve) => { + const $modal = fomanticQuery(modal); + $modal.modal({ + onApprove() { + resolve(true); + }, + onHidden() { + $modal.remove(); + resolve(false); + }, + }).modal('show'); + }); +} export function initArticleEditor() { const editForm = document.querySelector('#article-edit-form'); @@ -18,13 +53,27 @@ export function initArticleEditor() { hideModeSwitch: false, // Allow mode switching }); - // Fork button is now handled by a form in the template with proper navigation - // No JavaScript handler needed - the form submission navigates to the fork page - // Handle Submit Changes button - const submitButton = document.querySelector('#submit-changes-button'); + const submitButton = document.querySelector('#submit-changes-button'); if (submitButton && !submitButton.classList.contains('disabled')) { submitButton.addEventListener('click', async () => { + // Check if this is a fork-and-edit action that needs confirmation + const isForkAndEdit = submitButton.getAttribute('data-fork-and-edit') === 'true'; + + if (isForkAndEdit) { + // Get confirmation modal content from data attributes + const title = submitButton.getAttribute('data-fork-confirm-title') || 'Confirm Fork'; + const body = submitButton.getAttribute('data-fork-confirm-body') || 'Are you sure you want to fork this article?'; + const confirmText = submitButton.getAttribute('data-fork-confirm-yes') || 'Yes, Fork'; + const cancelText = submitButton.getAttribute('data-fork-confirm-cancel') || 'Cancel'; + + // Show confirmation modal + const confirmed = await showForkConfirmModal(title, body, confirmText, cancelText); + if (!confirmed) { + return; // User cancelled, do nothing + } + } + // Update textarea with editor content before submission textarea.value = editor.getMarkdown(); From 77adece172546765926c4aff7b53b46ccb3a0863 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Fri, 19 Dec 2025 16:44:01 +0100 Subject: [PATCH 14/37] article: add tooltip to Fork button --- custom/templates/shared/repo/article.tmpl | 1 + 1 file changed, 1 insertion(+) diff --git a/custom/templates/shared/repo/article.tmpl b/custom/templates/shared/repo/article.tmpl index f7b3de3c14..a98a98d580 100644 --- a/custom/templates/shared/repo/article.tmpl +++ b/custom/templates/shared/repo/article.tmpl @@ -92,6 +92,7 @@
{{else}} {{/* Editor form */}} - diff --git a/routers/web/repo/editor.go b/routers/web/repo/editor.go index 96b127d376..6f44e350ab 100644 --- a/routers/web/repo/editor.go +++ b/routers/web/repo/editor.go @@ -363,7 +363,9 @@ func EditFilePost(ctx *context.Context) { return } - if parsed.CommitFormOptions.NeedFork { + // Skip the NeedFork workflow if ForkAndEdit is true + // The ForkAndEdit workflow (handled later) will create the fork + if parsed.CommitFormOptions.NeedFork && !parsed.form.ForkAndEdit { baseRepo := ctx.Repo.Repository repoName := getUniqueRepositoryName(ctx, ctx.Doer.ID, baseRepo.Name) if repoName == "" { diff --git a/services/context/permission.go b/services/context/permission.go index c0a5a98724..3194561d3b 100644 --- a/services/context/permission.go +++ b/services/context/permission.go @@ -23,8 +23,15 @@ func RequireRepoAdmin() func(ctx *Context) { } // CanWriteToBranch checks if the user is allowed to write to the branch of the repo +// If the request has fork_and_edit=true in the form data, the check is skipped +// because the handler will create a fork and commit to that instead. func CanWriteToBranch() func(ctx *Context) { return func(ctx *Context) { + // Allow fork-and-edit workflow to bypass write permission check + // The handler will create a fork and commit to that instead + if ctx.Req.FormValue("fork_and_edit") == "true" { + return + } if !ctx.Repo.CanWriteToBranch(ctx, ctx.Doer, ctx.Repo.BranchName) { ctx.NotFound(nil) return From 28828f9ed4b58b3ecc34e28b7e07e6baa6c6ac03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Sun, 21 Dec 2025 12:55:48 +0100 Subject: [PATCH 19/37] tests: fork-and-edit workflow * CheckForkOnEditPermissions for owner/non-owner/unauthenticated * CanWriteToBranch middleware bypass with fork_and_edit=true * existing fork detection using fixture data (repo11/repo10) * subject ownership blocking prevents duplicate forks * form action URL uses RepoOperationsLink not RepoLink --- tests/integration/repo_fork_edit_test.go | 266 +++++++++++++++++++++++ 1 file changed, 266 insertions(+) create mode 100644 tests/integration/repo_fork_edit_test.go diff --git a/tests/integration/repo_fork_edit_test.go b/tests/integration/repo_fork_edit_test.go new file mode 100644 index 0000000000..6cc9824623 --- /dev/null +++ b/tests/integration/repo_fork_edit_test.go @@ -0,0 +1,266 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package integration + +import ( + "net/http" + "path" + "testing" + + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + repo_service "code.gitea.io/gitea/services/repository" + "code.gitea.io/gitea/tests" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestForkAndEditPermissions tests the CheckForkOnEditPermissions function +// which determines how a user can edit a repository they don't own. +func TestForkAndEditPermissions(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + // Test fixtures: + // - user2 owns repo1 (ID: 1) with subject_id: 1 + // - user4 is a regular user who doesn't own repo1 + owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + nonOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + + t.Run("OwnerCanEditDirectly", func(t *testing.T) { + perms, err := repo_service.CheckForkOnEditPermissions(t.Context(), owner, repo) + require.NoError(t, err) + assert.True(t, perms.IsRepoOwner) + assert.True(t, perms.CanEditDirectly) + assert.False(t, perms.NeedsFork) + assert.False(t, perms.HasExistingFork) + assert.False(t, perms.BlockedBySubject) + }) + + t.Run("NonOwnerNeedsFork", func(t *testing.T) { + perms, err := repo_service.CheckForkOnEditPermissions(t.Context(), nonOwner, repo) + require.NoError(t, err) + assert.False(t, perms.IsRepoOwner) + assert.False(t, perms.CanEditDirectly) + assert.True(t, perms.NeedsFork) + assert.False(t, perms.HasExistingFork) + assert.False(t, perms.BlockedBySubject) + }) + + t.Run("UnauthenticatedUserNoPermissions", func(t *testing.T) { + perms, err := repo_service.CheckForkOnEditPermissions(t.Context(), nil, repo) + require.NoError(t, err) + assert.False(t, perms.IsRepoOwner) + assert.False(t, perms.CanEditDirectly) + assert.False(t, perms.NeedsFork) + }) +} + +// TestForkAndEditMiddlewareBypass tests that the CanWriteToBranch middleware +// correctly bypasses permission checks when fork_and_edit=true is set. +func TestForkAndEditMiddlewareBypass(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + nonOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + + sessionNonOwner := loginUser(t, nonOwner.Name) + + t.Run("NonOwnerWithoutForkAndEditGetsDenied", func(t *testing.T) { + // Get the edit page to obtain CSRF token + editURL := path.Join(owner.Name, repo.Name, "_edit", repo.DefaultBranch, "README.md") + req := NewRequest(t, "GET", editURL) + resp := sessionNonOwner.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + // POST without fork_and_edit should be denied (404) + form := map[string]string{ + "_csrf": htmlDoc.GetCSRF(), + "last_commit": htmlDoc.GetInputValueByName("last_commit"), + "tree_path": "README.md", + "content": "Test content without fork_and_edit", + "commit_choice": "direct", + // Note: fork_and_edit is NOT set + } + + req = NewRequestWithValues(t, "POST", editURL, form) + sessionNonOwner.MakeRequest(t, req, http.StatusNotFound) + }) + + t.Run("NonOwnerWithForkAndEditPassesMiddleware", func(t *testing.T) { + // Get the edit page to obtain CSRF token + editURL := path.Join(owner.Name, repo.Name, "_edit", repo.DefaultBranch, "README.md") + req := NewRequest(t, "GET", editURL) + resp := sessionNonOwner.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + // POST with fork_and_edit=true should NOT return 404 + // (it may fail later due to git operations, but the middleware should pass) + form := map[string]string{ + "_csrf": htmlDoc.GetCSRF(), + "last_commit": htmlDoc.GetInputValueByName("last_commit"), + "tree_path": "README.md", + "content": "Test content with fork_and_edit", + "commit_choice": "direct", + "fork_and_edit": "true", + } + + req = NewRequestWithValues(t, "POST", editURL, form) + // We don't check the response code here because git operations may fail + // in the test environment. The key test is that we don't get 404 from + // the middleware - any other response means the middleware passed. + resp = sessionNonOwner.MakeRequest(t, req, NoExpectedStatus) + + // The response should NOT be 404 (middleware passed) + // It may be 200 (success) or 400 (git operation failed) but not 404 + assert.NotEqual(t, http.StatusNotFound, resp.Code, + "fork_and_edit=true should bypass CanWriteToBranch middleware") + }) +} + +// TestForkAndEditOwnerNoFork tests that repository owners don't get fork_and_edit set +func TestForkAndEditOwnerNoFork(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + + sessionOwner := loginUser(t, owner.Name) + + // Owner should be able to edit directly without fork_and_edit + editURL := path.Join(owner.Name, repo.Name, "_edit", repo.DefaultBranch, "README.md") + req := NewRequest(t, "GET", editURL) + resp := sessionOwner.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + // Check that the form does NOT have fork_and_edit set to true + forkAndEditInput := htmlDoc.doc.Find("input[name=fork_and_edit]") + if forkAndEditInput.Length() > 0 { + val, _ := forkAndEditInput.Attr("value") + assert.NotEqual(t, "true", val, "Owner should not have fork_and_edit=true") + } +} + +// TestForkAndEditExistingForkDetection tests that existing forks are detected +// by CheckForkOnEditPermissions using fixture data. +// Note: We use repo11 which is a fork of repo10 owned by user13 in the fixtures. +func TestForkAndEditExistingForkDetection(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + // repo11 (ID: 11) is a fork of repo10 (ID: 10) owned by user13 (ID: 13) + // This is set up in the fixtures + user13 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 13}) + repo10 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10}) + repo11 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 11}) + + // Verify fixture data is correct + require.True(t, repo11.IsFork, "repo11 should be a fork") + require.Equal(t, repo10.ID, repo11.ForkID, "repo11 should be a fork of repo10") + require.Equal(t, user13.ID, repo11.OwnerID, "repo11 should be owned by user13") + + t.Run("ExistingForkIsDetected", func(t *testing.T) { + perms, err := repo_service.CheckForkOnEditPermissions(t.Context(), user13, repo10) + require.NoError(t, err) + assert.True(t, perms.HasExistingFork, "Should detect existing fork") + require.NotNil(t, perms.ExistingFork, "ExistingFork should not be nil") + assert.Equal(t, repo11.ID, perms.ExistingFork.ID) + }) +} + +// TestForkAndEditUnauthenticated tests that unauthenticated users cannot use fork-and-edit +func TestForkAndEditUnauthenticated(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + + t.Run("UnauthenticatedCannotPost", func(t *testing.T) { + // Unauthenticated user should be redirected to login + editURL := path.Join(owner.Name, repo.Name, "_edit", repo.DefaultBranch, "README.md") + req := NewRequest(t, "GET", editURL) + MakeRequest(t, req, http.StatusSeeOther) // Redirect to login + }) +} + +// TestForkAndEditBlockedBySubject tests that users who own a different repo +// for the same subject are blocked from fork-and-edit. +// This test uses fixture data to avoid git operations. +func TestForkAndEditBlockedBySubject(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + // user2 owns repo1 with subject_id: 1 + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + + // user5 is a regular user + otherUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) + + // Get the subject from the repo + subject := unittest.AssertExistsAndLoadBean(t, &repo_model.Subject{ID: repo.SubjectID}) + + // Create a repository for user5 with the same subject + // Note: In Forkana, creating a repo with an existing subject creates a fork + ownRepo, err := repo_service.CreateRepositoryDirectly(t.Context(), otherUser, otherUser, repo_service.CreateRepoOptions{ + Name: "my-subject-repo", + Subject: subject.Name, + }, true) + require.NoError(t, err) + require.NotNil(t, ownRepo) + defer func() { + _ = repo_service.DeleteRepositoryDirectly(t.Context(), ownRepo.ID) + }() + + t.Run("BlockedBySubjectOwnership", func(t *testing.T) { + // The user now owns a repo for this subject + // CheckForkOnEditPermissions should detect this + perms, err := repo_service.CheckForkOnEditPermissions(t.Context(), otherUser, repo) + require.NoError(t, err) + + // Log what we got for debugging + t.Logf("ownRepo.IsFork=%v, ownRepo.ForkID=%d, repo.ID=%d", ownRepo.IsFork, ownRepo.ForkID, repo.ID) + t.Logf("perms.HasExistingFork=%v, perms.BlockedBySubject=%v", perms.HasExistingFork, perms.BlockedBySubject) + + // The user should either: + // 1. Have an existing fork detected (if ownRepo is a fork of repo), OR + // 2. Be blocked by subject ownership (if ownRepo is a different repo for the same subject) + // Either way, they should NOT be able to create a new fork + assert.True(t, perms.HasExistingFork || perms.BlockedBySubject, + "User should either have existing fork or be blocked by subject ownership") + }) +} + +// TestForkAndEditFormActionURL tests that the form action URL is correct +func TestForkAndEditFormActionURL(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + nonOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + + sessionNonOwner := loginUser(t, nonOwner.Name) + + t.Run("FormActionUsesRepoOperationsLink", func(t *testing.T) { + // The form action should use RepoOperationsLink (/{owner}/{repo}/...) + // not RepoLink (which could be /article/... for subject repos) + editURL := path.Join(owner.Name, repo.Name, "_edit", repo.DefaultBranch, "README.md") + req := NewRequest(t, "GET", editURL) + resp := sessionNonOwner.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + // Find the form action + form := htmlDoc.doc.Find("form.edit-form, form[action*='_edit']") + if form.Length() > 0 { + action, exists := form.Attr("action") + if exists { + // Action should contain the owner/repo path, not /article/ + assert.Contains(t, action, owner.Name) + assert.Contains(t, action, repo.Name) + assert.NotContains(t, action, "/article/") + } + } + }) +} + From f1162b0e6b9d5b68aeb323051747e909ee20cc30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Sun, 21 Dec 2025 13:34:47 +0100 Subject: [PATCH 20/37] tests: add database migration tests for v326 and v327 * slug generation migration covering basic slugs, mixed-case deduplication, special characters, unique constraint verification, and empty table handling * index creation, idempotency, and index type verification --- .../custom/v1_25_custom/v326_test.go | 221 ++++++++++++++++++ .../custom/v1_25_custom/v327_test.go | 143 ++++++++++++ 2 files changed, 364 insertions(+) create mode 100644 models/migrations/custom/v1_25_custom/v327_test.go diff --git a/models/migrations/custom/v1_25_custom/v326_test.go b/models/migrations/custom/v1_25_custom/v326_test.go index de9b86a815..e3abd73f35 100644 --- a/models/migrations/custom/v1_25_custom/v326_test.go +++ b/models/migrations/custom/v1_25_custom/v326_test.go @@ -4,11 +4,16 @@ package v1_25_custom import ( + "strings" "testing" + "code.gitea.io/gitea/models/migrations/base" repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/modules/timeutil" "github.com/stretchr/testify/assert" + "xorm.io/xorm" + "xorm.io/xorm/schemas" ) // TestMigrationUsesApplicationSlugFunction verifies that the migration v326 @@ -129,3 +134,219 @@ func TestMigrationSlugConsistency(t *testing.T) { }) } } + +// subjectPreMigration represents the subject table before v326 migration +type subjectPreMigration struct { + ID int64 `xorm:"pk autoincr"` + Name string `xorm:"VARCHAR(255) NOT NULL"` + CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` + UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` +} + +func (*subjectPreMigration) TableName() string { + return "subject" +} + +// subjectResult is used to query the subject table after migration +type subjectResult struct { + ID int64 `xorm:"pk autoincr"` + Name string `xorm:"VARCHAR(255) NOT NULL"` + Slug string `xorm:"VARCHAR(255)"` +} + +func (*subjectResult) TableName() string { + return "subject" +} + +// Test_AddSubjectSlugColumn tests the v326 migration that adds the slug column +// to the subject table and generates slugs for existing subjects. +func Test_AddSubjectSlugColumn(t *testing.T) { + // Helper function to set up a fresh subject table + setupSubjectTable := func(t *testing.T, x *xorm.Engine) { + t.Helper() + _, _ = x.Exec("DROP TABLE IF EXISTS subject") + err := x.Sync(new(subjectPreMigration)) + assert.NoError(t, err) + } + + // Prepare and load the testing database + x, deferable := base.PrepareTestEnv(t, 0) + defer deferable() + if x == nil || t.Failed() { + return + } + + // Test Case 1: Basic slug generation + t.Run("BasicSlugGeneration", func(t *testing.T) { + setupSubjectTable(t, x) + + // Insert test subjects using raw SQL to avoid struct mismatch + _, err := x.Exec("INSERT INTO subject (name) VALUES (?)", "The Moon") + assert.NoError(t, err) + _, err = x.Exec("INSERT INTO subject (name) VALUES (?)", "Computer Science") + assert.NoError(t, err) + _, err = x.Exec("INSERT INTO subject (name) VALUES (?)", "Café Français") + assert.NoError(t, err) + + // Run the migration + err = AddSubjectSlugColumn(x) + assert.NoError(t, err) + + // Verify slugs were generated correctly + var results []subjectResult + err = x.Table("subject").Find(&results) + assert.NoError(t, err) + assert.Len(t, results, 3) + + // Create a map for easier verification + slugMap := make(map[string]string) + for _, s := range results { + slugMap[s.Name] = s.Slug + } + + assert.Equal(t, "the-moon", slugMap["The Moon"]) + assert.Equal(t, "computer-science", slugMap["Computer Science"]) + assert.Equal(t, "cafe-francais", slugMap["Café Français"]) + }) + + // Test Case 2: Mixed-case subject names produce same slug (deduplication) + t.Run("MixedCaseDeduplication", func(t *testing.T) { + setupSubjectTable(t, x) + + // Insert subjects that differ only in case + // Note: These would normally be deduplicated at creation time, + // but we test the migration handles them with numeric suffixes + _, err := x.Exec("INSERT INTO subject (name) VALUES (?)", "the moon") + assert.NoError(t, err) + _, err = x.Exec("INSERT INTO subject (name) VALUES (?)", "The Moon") + assert.NoError(t, err) + _, err = x.Exec("INSERT INTO subject (name) VALUES (?)", "THE MOON") + assert.NoError(t, err) + + // Run the migration + err = AddSubjectSlugColumn(x) + assert.NoError(t, err) + + // Verify slugs were generated with deduplication + var results []subjectResult + err = x.Table("subject").OrderBy("id").Find(&results) + assert.NoError(t, err) + assert.Len(t, results, 3) + + // First one gets the base slug, others get numeric suffixes + assert.Equal(t, "the-moon", results[0].Slug) + assert.Equal(t, "the-moon-2", results[1].Slug) + assert.Equal(t, "the-moon-3", results[2].Slug) + }) + + // Test Case 3: Special characters are handled correctly + t.Run("SpecialCharacters", func(t *testing.T) { + setupSubjectTable(t, x) + + // Note: @ and # are removed entirely (not replaced with hyphens) + // Underscores are replaced with hyphens + // Accented characters are normalized + _, err := x.Exec("INSERT INTO subject (name) VALUES (?)", "Hello@World#2024!") + assert.NoError(t, err) + _, err = x.Exec("INSERT INTO subject (name) VALUES (?)", "hello_world_test") + assert.NoError(t, err) + _, err = x.Exec("INSERT INTO subject (name) VALUES (?)", "El Camiño?") + assert.NoError(t, err) + _, err = x.Exec("INSERT INTO subject (name) VALUES (?)", "Zürich") + assert.NoError(t, err) + + // Run the migration + err = AddSubjectSlugColumn(x) + assert.NoError(t, err) + + // Verify slugs + var results []subjectResult + err = x.Table("subject").Find(&results) + assert.NoError(t, err) + + slugMap := make(map[string]string) + for _, s := range results { + slugMap[s.Name] = s.Slug + } + + // @ and # are removed, not replaced with hyphens + assert.Equal(t, "helloworld2024", slugMap["Hello@World#2024!"]) + // Underscores are replaced with hyphens + assert.Equal(t, "hello-world-test", slugMap["hello_world_test"]) + // Accented characters are normalized + assert.Equal(t, "el-camino", slugMap["El Camiño?"]) + assert.Equal(t, "zurich", slugMap["Zürich"]) + }) + + // Test Case 4: Verify UNIQUE constraint on slug column + t.Run("UniqueConstraint", func(t *testing.T) { + setupSubjectTable(t, x) + + _, err := x.Exec("INSERT INTO subject (name) VALUES (?)", "Test Subject") + assert.NoError(t, err) + + // Run the migration + err = AddSubjectSlugColumn(x) + assert.NoError(t, err) + + // Verify the UNIQUE index exists on slug column + tables, err := x.DBMetas() + assert.NoError(t, err) + + var subjectTable *schemas.Table + for _, table := range tables { + if table.Name == "subject" { + subjectTable = table + break + } + } + assert.NotNil(t, subjectTable, "Subject table should exist") + + // Check for unique index on slug - look for any index that: + // 1. Contains the slug column (note: xorm may include quotes in column names) + // 2. Is either a UniqueType index OR has a name containing "UQE" (xorm convention) + foundUniqueSlugIndex := false + for _, index := range subjectTable.Indexes { + hasSlugCol := false + for _, col := range index.Cols { + // Strip quotes from column name (xorm may include them for SQLite) + cleanCol := strings.Trim(col, `"'`) + if cleanCol == "slug" { + hasSlugCol = true + break + } + } + if hasSlugCol { + // Check if it's a unique index (either by type or by naming convention) + if index.Type == schemas.UniqueType || strings.Contains(strings.ToUpper(index.Name), "UQE") { + foundUniqueSlugIndex = true + break + } + } + } + assert.True(t, foundUniqueSlugIndex, "UNIQUE index on slug column should exist") + }) + + // Test Case 5: Empty table (no subjects) + t.Run("EmptyTable", func(t *testing.T) { + setupSubjectTable(t, x) + + // Run the migration on empty table + err := AddSubjectSlugColumn(x) + assert.NoError(t, err) + + // Verify table structure is correct + tables, err := x.DBMetas() + assert.NoError(t, err) + + var subjectTable *schemas.Table + for _, table := range tables { + if table.Name == "subject" { + subjectTable = table + break + } + } + assert.NotNil(t, subjectTable, "Subject table should exist") + assert.NotNil(t, subjectTable.GetColumn("slug"), "Slug column should exist") + }) +} diff --git a/models/migrations/custom/v1_25_custom/v327_test.go b/models/migrations/custom/v1_25_custom/v327_test.go new file mode 100644 index 0000000000..1edc014799 --- /dev/null +++ b/models/migrations/custom/v1_25_custom/v327_test.go @@ -0,0 +1,143 @@ +// Copyright 2025 okTurtles Foundation. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_25_custom + +import ( + "testing" + + "code.gitea.io/gitea/models/migrations/base" + + "github.com/stretchr/testify/assert" + "xorm.io/xorm/schemas" +) + +// Test_AddCompositeIndexesForForkOnEdit tests the v327 migration that adds +// composite indexes to optimize fork-on-edit permission queries. +func Test_AddCompositeIndexesForForkOnEdit(t *testing.T) { + // Define the Repository table structure for testing + // Only include the columns relevant to the indexes being created + type Repository struct { + ID int64 `xorm:"pk autoincr"` + OwnerID int64 `xorm:"INDEX"` + SubjectID int64 `xorm:"INDEX"` + ForkID int64 `xorm:"INDEX"` + } + + // Prepare and load the testing database + x, deferable := base.PrepareTestEnv(t, 0, new(Repository)) + defer deferable() + if x == nil || t.Failed() { + return + } + + // Helper function to check if an index with specific columns exists + hasIndexWithColumns := func(table *schemas.Table, cols []string) bool { + for _, index := range table.Indexes { + if len(index.Cols) == len(cols) { + match := true + for _, col := range cols { + found := false + for _, indexCol := range index.Cols { + if indexCol == col { + found = true + break + } + } + if !found { + match = false + break + } + } + if match { + return true + } + } + } + return false + } + + // Test Case 1: Verify indexes are created correctly + t.Run("IndexesCreated", func(t *testing.T) { + // Run the migration + err := AddCompositeIndexesForForkOnEdit(x) + assert.NoError(t, err) + + // Get table metadata + tables, err := x.DBMetas() + assert.NoError(t, err) + + // Find the repository table + var repoTable *schemas.Table + for _, table := range tables { + if table.Name == "repository" { + repoTable = table + break + } + } + assert.NotNil(t, repoTable, "Repository table should exist") + + // Check for composite index on (owner_id, subject_id) + assert.True(t, hasIndexWithColumns(repoTable, []string{"owner_id", "subject_id"}), + "Composite index on (owner_id, subject_id) should exist") + + // Check for composite index on (owner_id, fork_id) + assert.True(t, hasIndexWithColumns(repoTable, []string{"owner_id", "fork_id"}), + "Composite index on (owner_id, fork_id) should exist") + }) + + // Test Case 2: Verify migration is idempotent (can run multiple times) + t.Run("Idempotent", func(t *testing.T) { + // Run the migration again (should not error) + err := AddCompositeIndexesForForkOnEdit(x) + assert.NoError(t, err, "Migration should be idempotent and not error on second run") + + // Verify indexes still exist + tables, err := x.DBMetas() + assert.NoError(t, err) + + var repoTable *schemas.Table + for _, table := range tables { + if table.Name == "repository" { + repoTable = table + break + } + } + assert.NotNil(t, repoTable) + + // Verify the composite indexes still exist after running twice + assert.True(t, hasIndexWithColumns(repoTable, []string{"owner_id", "subject_id"}), + "Composite index on (owner_id, subject_id) should still exist after second run") + assert.True(t, hasIndexWithColumns(repoTable, []string{"owner_id", "fork_id"}), + "Composite index on (owner_id, fork_id) should still exist after second run") + }) + + // Test Case 3: Verify indexes are regular (non-unique) indexes + t.Run("IndexType", func(t *testing.T) { + tables, err := x.DBMetas() + assert.NoError(t, err) + + var repoTable *schemas.Table + for _, table := range tables { + if table.Name == "repository" { + repoTable = table + break + } + } + assert.NotNil(t, repoTable) + + // Find the composite indexes and verify they are regular indexes + for _, index := range repoTable.Indexes { + if len(index.Cols) == 2 { + isOwnerSubject := hasIndexWithColumns(repoTable, []string{"owner_id", "subject_id"}) + isOwnerFork := hasIndexWithColumns(repoTable, []string{"owner_id", "fork_id"}) + if isOwnerSubject || isOwnerFork { + // These should be regular indexes, not unique indexes + assert.Equal(t, schemas.IndexType, index.Type, + "Composite index should be a regular index, not unique") + } + } + } + }) +} + From 4b1b055d7878aed11256b2156615d6fd8e86fbbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Sun, 21 Dec 2025 13:44:04 +0100 Subject: [PATCH 21/37] api: add tests for fork with subject conflict and fix error handling order * fork API behavior when user already owns a repository for the same subject * fork blocked by subject ownership (403), fork succeeds for user without subject repo (202), fork blocked even with custom name (403) * fix error handling order in CreateFork --- routers/api/v1/repo/fork.go | 8 ++- tests/integration/api_fork_test.go | 94 ++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 3 deletions(-) diff --git a/routers/api/v1/repo/fork.go b/routers/api/v1/repo/fork.go index 787abeb663..11579cf518 100644 --- a/routers/api/v1/repo/fork.go +++ b/routers/api/v1/repo/fork.go @@ -158,10 +158,12 @@ func CreateFork(ctx *context.APIContext) { Description: repo.Description, }) if err != nil { - if errors.Is(err, util.ErrAlreadyExist) || repo_model.IsErrReachLimitOfRepo(err) { - ctx.APIError(http.StatusConflict, err) - } else if errors.Is(err, user_model.ErrBlockedUser) || repo_model.IsErrForkTreeTooLarge(err) || repo_service.IsErrUserOwnsSubjectRepo(err) { + // Check IsErrUserOwnsSubjectRepo BEFORE errors.Is(err, util.ErrAlreadyExist) + // because ErrUserOwnsSubjectRepo.Unwrap() returns util.ErrAlreadyExist + if errors.Is(err, user_model.ErrBlockedUser) || repo_model.IsErrForkTreeTooLarge(err) || repo_service.IsErrUserOwnsSubjectRepo(err) { ctx.APIError(http.StatusForbidden, err) + } else if errors.Is(err, util.ErrAlreadyExist) || repo_model.IsErrReachLimitOfRepo(err) { + ctx.APIError(http.StatusConflict, err) } else { ctx.APIErrorInternal(err) } diff --git a/tests/integration/api_fork_test.go b/tests/integration/api_fork_test.go index bd2d38ef4f..b03ff8c67c 100644 --- a/tests/integration/api_fork_test.go +++ b/tests/integration/api_fork_test.go @@ -14,9 +14,11 @@ import ( user_model "code.gitea.io/gitea/models/user" api "code.gitea.io/gitea/modules/structs" org_service "code.gitea.io/gitea/services/org" + repo_service "code.gitea.io/gitea/services/repository" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestCreateForkNoLogin(t *testing.T) { @@ -124,3 +126,95 @@ func TestGetPrivateReposForks(t *testing.T) { assert.Equal(t, "forked-repo", forks[0].Name) assert.Equal(t, privateOrg.Name, forks[0].Owner.UserName) } + +// TestAPIForkWithSubjectConflict tests that forking a repository fails with HTTP 403 +// when the user already owns a different repository for the same subject. +// This is a Forkana-specific constraint: each user can only have one repository per subject. +func TestAPIForkWithSubjectConflict(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + // Setup: + // - user2 owns repo1 with subject_id: 1 (example-subject) + // - user5 will create their own repo for the same subject + // - user5 then tries to fork repo1 - should fail with 403 + + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + user5 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) + repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + + // Verify repo1 has a subject + require.Greater(t, repo1.SubjectID, int64(0), "repo1 should have a subject") + subject := unittest.AssertExistsAndLoadBean(t, &repo_model.Subject{ID: repo1.SubjectID}) + + // Create a repository for user5 with the same subject + // In Forkana, this creates a fork of the root repository + user5Repo, err := repo_service.CreateRepositoryDirectly(t.Context(), user5, user5, repo_service.CreateRepoOptions{ + Name: "my-subject-article", + Subject: subject.Name, + }, true) + require.NoError(t, err) + require.NotNil(t, user5Repo) + defer func() { + _ = repo_service.DeleteRepositoryDirectly(t.Context(), user5Repo.ID) + }() + + // Get API token for user5 + user5Sess := loginUser(t, user5.Name) + user5Token := getTokenForLoggedInUser(t, user5Sess, auth_model.AccessTokenScopeWriteRepository) + + t.Run("ForkBlockedBySubjectOwnership", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + // user5 tries to fork repo1 (owned by user2) for the same subject + // This should fail because user5 already owns a repo for this subject + req := NewRequestWithJSON(t, "POST", "/api/v1/repos/"+user2.Name+"/"+repo1.Name+"/forks", &api.CreateForkOption{}). + AddTokenAuth(user5Token) + resp := MakeRequest(t, req, http.StatusForbidden) + + // Verify the error message mentions subject ownership + var apiError api.APIError + DecodeJSON(t, resp, &apiError) + assert.Contains(t, apiError.Message, "subject", + "Error message should mention subject ownership conflict") + }) + + t.Run("ForkSucceedsForUserWithoutSubjectRepo", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + // user4 doesn't own a repo for the same subject as repo1 + // so they should be able to fork repo1 successfully + user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + user4Sess := loginUser(t, user4.Name) + user4Token := getTokenForLoggedInUser(t, user4Sess, auth_model.AccessTokenScopeWriteRepository) + + // user4 should be able to fork repo1 since they don't own a repo for that subject + forkName := "user4-fork-of-repo1" + req := NewRequestWithJSON(t, "POST", "/api/v1/repos/"+user2.Name+"/"+repo1.Name+"/forks", &api.CreateForkOption{ + Name: &forkName, + }).AddTokenAuth(user4Token) + resp := MakeRequest(t, req, http.StatusAccepted) + + var fork api.Repository + DecodeJSON(t, resp, &fork) + assert.Equal(t, forkName, fork.Name) + assert.Equal(t, user4.Name, fork.Owner.UserName) + + // Clean up the fork + defer func() { + forkRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{Name: forkName, OwnerID: user4.ID}) + _ = repo_service.DeleteRepositoryDirectly(t.Context(), forkRepo.ID) + }() + }) + + t.Run("ForkBlockedWithCustomName", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + // Even with a custom fork name, the fork should be blocked + // because user5 already owns a repo for the same subject + customName := "custom-fork-name" + req := NewRequestWithJSON(t, "POST", "/api/v1/repos/"+user2.Name+"/"+repo1.Name+"/forks", &api.CreateForkOption{ + Name: &customName, + }).AddTokenAuth(user5Token) + MakeRequest(t, req, http.StatusForbidden) + }) +} From 9f8de0de7c1dcc9dd646848b68968c4831508d89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Sun, 21 Dec 2025 14:14:59 +0100 Subject: [PATCH 22/37] permissions: restrict fork_and_edit bypass to _edit and _new actions only --- services/context/permission.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/services/context/permission.go b/services/context/permission.go index 3194561d3b..767179fcef 100644 --- a/services/context/permission.go +++ b/services/context/permission.go @@ -25,12 +25,22 @@ func RequireRepoAdmin() func(ctx *Context) { // CanWriteToBranch checks if the user is allowed to write to the branch of the repo // If the request has fork_and_edit=true in the form data, the check is skipped // because the handler will create a fork and commit to that instead. +// The fork_and_edit bypass is ONLY allowed for _edit and _new actions, +// which properly handle the fork-and-edit workflow via handleForkAndEdit(). +// Other actions (delete, upload, diffpatch, cherrypick) do NOT support fork_and_edit +// and must not allow this bypass. func CanWriteToBranch() func(ctx *Context) { return func(ctx *Context) { // Allow fork-and-edit workflow to bypass write permission check // The handler will create a fork and commit to that instead if ctx.Req.FormValue("fork_and_edit") == "true" { - return + // Only allow fork_and_edit bypass for _edit and _new actions + // These are the only handlers that properly implement the fork-and-edit workflow + editorAction := ctx.PathParam("editor_action") + if editorAction == "_edit" || editorAction == "_new" { + return + } + // For other actions, ignore fork_and_edit and fall through to permission check } if !ctx.Repo.CanWriteToBranch(ctx, ctx.Doer, ctx.Repo.BranchName) { ctx.NotFound(nil) From e699e2b9640a1941bc26533a650dde88946d1d0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Sun, 21 Dec 2025 14:15:42 +0100 Subject: [PATCH 23/37] tests: add fork-on-edit permission tests --- tests/integration/repo_fork_edit_test.go | 125 +++++++++++++++++++++++ 1 file changed, 125 insertions(+) diff --git a/tests/integration/repo_fork_edit_test.go b/tests/integration/repo_fork_edit_test.go index 6cc9824623..a2fcca0949 100644 --- a/tests/integration/repo_fork_edit_test.go +++ b/tests/integration/repo_fork_edit_test.go @@ -264,3 +264,128 @@ func TestForkAndEditFormActionURL(t *testing.T) { }) } +// TestForkAndEditSecurityBypass tests that fork_and_edit=true cannot be used +// to bypass permission checks on non-edit endpoints (delete, upload, diffpatch, cherrypick). +// This is a security test to ensure the CanWriteToBranch middleware only allows +// the bypass for _edit and _new actions which properly handle fork-and-edit. +func TestForkAndEditSecurityBypass(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + nonOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + + sessionNonOwner := loginUser(t, nonOwner.Name) + + // Get CSRF token from any page + editURL := path.Join(owner.Name, repo.Name, "_edit", repo.DefaultBranch, "README.md") + req := NewRequest(t, "GET", editURL) + resp := sessionNonOwner.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + csrf := htmlDoc.GetCSRF() + + t.Run("DeleteEndpointRejectsForkAndEdit", func(t *testing.T) { + // Attempt to delete a file with fork_and_edit=true + // This should be rejected with 404 (permission denied) + deleteURL := path.Join(owner.Name, repo.Name, "_delete", repo.DefaultBranch, "README.md") + form := map[string]string{ + "_csrf": csrf, + "tree_path": "README.md", // Attacker might try to set this to bypass checks + "commit_choice": "direct", + "fork_and_edit": "true", // Attempting to bypass permission check + } + + req := NewRequestWithValues(t, "POST", deleteURL, form) + resp := sessionNonOwner.MakeRequest(t, req, NoExpectedStatus) + + // Should get 404 (permission denied) because fork_and_edit bypass + // is not allowed for _delete action + assert.Equal(t, http.StatusNotFound, resp.Code, + "fork_and_edit=true should NOT bypass CanWriteToBranch for _delete action") + }) + + t.Run("UploadEndpointRejectsForkAndEdit", func(t *testing.T) { + // Attempt to upload a file with fork_and_edit=true + // This should be rejected with 404 (permission denied) + uploadURL := path.Join(owner.Name, repo.Name, "_upload", repo.DefaultBranch, "/") + form := map[string]string{ + "_csrf": csrf, + "tree_path": "", + "commit_choice": "direct", + "fork_and_edit": "true", // Attempting to bypass permission check + } + + req := NewRequestWithValues(t, "POST", uploadURL, form) + resp := sessionNonOwner.MakeRequest(t, req, NoExpectedStatus) + + // Should get 404 (permission denied) because fork_and_edit bypass + // is not allowed for _upload action + assert.Equal(t, http.StatusNotFound, resp.Code, + "fork_and_edit=true should NOT bypass CanWriteToBranch for _upload action") + }) + + t.Run("DiffPatchEndpointRejectsForkAndEdit", func(t *testing.T) { + // Attempt to apply a patch with fork_and_edit=true + // This should be rejected with 404 (permission denied) + diffpatchURL := path.Join(owner.Name, repo.Name, "_diffpatch", repo.DefaultBranch, "/") + form := map[string]string{ + "_csrf": csrf, + "tree_path": "", + "content": "fake patch content", + "commit_choice": "direct", + "fork_and_edit": "true", // Attempting to bypass permission check + } + + req := NewRequestWithValues(t, "POST", diffpatchURL, form) + resp := sessionNonOwner.MakeRequest(t, req, NoExpectedStatus) + + // Should get 404 (permission denied) because fork_and_edit bypass + // is not allowed for _diffpatch action + assert.Equal(t, http.StatusNotFound, resp.Code, + "fork_and_edit=true should NOT bypass CanWriteToBranch for _diffpatch action") + }) + + t.Run("EditEndpointAllowsForkAndEdit", func(t *testing.T) { + // Verify that _edit still allows fork_and_edit=true + // This is the legitimate use case + editURL := path.Join(owner.Name, repo.Name, "_edit", repo.DefaultBranch, "README.md") + form := map[string]string{ + "_csrf": csrf, + "last_commit": htmlDoc.GetInputValueByName("last_commit"), + "tree_path": "README.md", + "content": "Test content", + "commit_choice": "direct", + "fork_and_edit": "true", + } + + req := NewRequestWithValues(t, "POST", editURL, form) + resp := sessionNonOwner.MakeRequest(t, req, NoExpectedStatus) + + // Should NOT get 404 - the middleware should pass + // (may get other errors due to git operations, but not 404) + assert.NotEqual(t, http.StatusNotFound, resp.Code, + "fork_and_edit=true SHOULD bypass CanWriteToBranch for _edit action") + }) + + t.Run("NewEndpointAllowsForkAndEdit", func(t *testing.T) { + // Verify that _new still allows fork_and_edit=true + // This is the legitimate use case + newURL := path.Join(owner.Name, repo.Name, "_new", repo.DefaultBranch, "/") + form := map[string]string{ + "_csrf": csrf, + "tree_path": "README.md", + "content": "Test content", + "commit_choice": "direct", + "fork_and_edit": "true", + } + + req := NewRequestWithValues(t, "POST", newURL, form) + resp := sessionNonOwner.MakeRequest(t, req, NoExpectedStatus) + + // Should NOT get 404 - the middleware should pass + // (may get other errors due to git operations, but not 404) + assert.NotEqual(t, http.StatusNotFound, resp.Code, + "fork_and_edit=true SHOULD bypass CanWriteToBranch for _new action") + }) +} + From f0efd1458955b6d44899fbf748080eea5151ac03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Sun, 21 Dec 2025 14:28:09 +0100 Subject: [PATCH 24/37] fork: replace type assertion with wrapped error support --- services/repository/fork.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/services/repository/fork.go b/services/repository/fork.go index a06c697527..c8433dbf3f 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -5,6 +5,7 @@ package repository import ( "context" + "errors" "fmt" "strings" "time" @@ -143,8 +144,8 @@ type ErrUserOwnsSubjectRepo struct { // IsErrUserOwnsSubjectRepo checks if an error is an ErrUserOwnsSubjectRepo. func IsErrUserOwnsSubjectRepo(err error) bool { - _, ok := err.(ErrUserOwnsSubjectRepo) - return ok + var e ErrUserOwnsSubjectRepo + return errors.As(err, &e) } func (err ErrUserOwnsSubjectRepo) Error() string { From 90a12c68394025bc1a8a3753dc3c36690f8ae3b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Sun, 21 Dec 2025 14:30:08 +0100 Subject: [PATCH 25/37] templates: document fallback logic --- custom/templates/shared/repo/edit.tmpl | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/custom/templates/shared/repo/edit.tmpl b/custom/templates/shared/repo/edit.tmpl index 5c31803617..dd14fdb7f4 100644 --- a/custom/templates/shared/repo/edit.tmpl +++ b/custom/templates/shared/repo/edit.tmpl @@ -39,7 +39,9 @@

{{ctx.Locale.Tr "repo.editor.file_not_editable_hint"}}

{{else if .HasOwnRepoForSubject}} - {{/* User already owns a different repository for this subject - show message instead of editor */}} + {{/* User already owns a different repository for this subject - show message instead of editor. + SubjectRelation should always be loaded here (via LoadSubject in ArticleView), + but we fall back to Repository.Name as a defensive measure. */}} {{$subjectName := Iif .Repository.SubjectRelation .Repository.SubjectRelation.Name .Repository.Name}}
From fc3ee6c66f57abd7e84aad819cacc09662bd5691 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Sun, 21 Dec 2025 14:39:22 +0100 Subject: [PATCH 26/37] repo: return errors from GetForkedRepo * update GetForkedRepo signature to return (*Repository, error) * update all callers to handle the new error return value * fixes silent error swallowing --- models/repo/fork.go | 12 ++++++++---- routers/api/v1/repo/pull.go | 6 +++++- routers/web/repo/compare.go | 18 +++++++++++++++--- routers/web/repo/fork.go | 11 ++++++++--- services/context/repo.go | 8 ++++++-- services/repository/fork.go | 5 +++-- 6 files changed, 45 insertions(+), 15 deletions(-) diff --git a/models/repo/fork.go b/models/repo/fork.go index da232f6a85..0595f70296 100644 --- a/models/repo/fork.go +++ b/models/repo/fork.go @@ -24,15 +24,19 @@ func GetRepositoriesByForkID(ctx context.Context, forkID int64) ([]*Repository, } // GetForkedRepo checks if given user has already forked a repository with given ID. -func GetForkedRepo(ctx context.Context, ownerID, repoID int64) *Repository { +// Returns (nil, nil) if no fork exists, (repo, nil) if fork exists, or (nil, err) on database error. +func GetForkedRepo(ctx context.Context, ownerID, repoID int64) (*Repository, error) { repo := new(Repository) - has, _ := db.GetEngine(ctx). + has, err := db.GetEngine(ctx). Where("owner_id=? AND fork_id=?", ownerID, repoID). Get(repo) + if err != nil { + return nil, err + } if has { - return repo + return repo, nil } - return nil + return nil, nil } // HasForkedRepo checks if given user has already forked a repository with given ID. diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index ff6ddbce2d..b2ec4c133c 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -1125,7 +1125,11 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) isSameRepo := ctx.Repo.Owner.ID == headUser.ID // Check if current user has fork of repository or in the same repository. - headRepo := repo_model.GetForkedRepo(ctx, headUser.ID, baseRepo.ID) + headRepo, err := repo_model.GetForkedRepo(ctx, headUser.ID, baseRepo.ID) + if err != nil { + ctx.APIErrorInternal(err) + return nil, nil + } if headRepo == nil && !isSameRepo { err = baseRepo.GetBaseRepo(ctx) if err != nil { diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 2e56f6934a..d5c298b86c 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -360,7 +360,11 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { // "OwnForkRepo" var ownForkRepo *repo_model.Repository if ctx.Doer != nil && baseRepo.OwnerID != ctx.Doer.ID { - repo := repo_model.GetForkedRepo(ctx, ctx.Doer.ID, baseRepo.ID) + repo, err := repo_model.GetForkedRepo(ctx, ctx.Doer.ID, baseRepo.ID) + if err != nil { + ctx.ServerError("GetForkedRepo", err) + return nil + } if repo != nil { ownForkRepo = repo ctx.Data["OwnForkRepo"] = ownForkRepo @@ -384,13 +388,21 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { // 5. If the headOwner has a fork of the baseRepo - use that if !has { - ci.HeadRepo = repo_model.GetForkedRepo(ctx, ci.HeadUser.ID, baseRepo.ID) + ci.HeadRepo, err = repo_model.GetForkedRepo(ctx, ci.HeadUser.ID, baseRepo.ID) + if err != nil { + ctx.ServerError("GetForkedRepo", err) + return nil + } has = ci.HeadRepo != nil } // 6. If the baseRepo is a fork and the headUser has a fork of that use that if !has && baseRepo.IsFork { - ci.HeadRepo = repo_model.GetForkedRepo(ctx, ci.HeadUser.ID, baseRepo.ForkID) + ci.HeadRepo, err = repo_model.GetForkedRepo(ctx, ci.HeadUser.ID, baseRepo.ForkID) + if err != nil { + ctx.ServerError("GetForkedRepo", err) + return nil + } has = ci.HeadRepo != nil } diff --git a/routers/web/repo/fork.go b/routers/web/repo/fork.go index 438cc1ae6a..64be68064f 100644 --- a/routers/web/repo/fork.go +++ b/routers/web/repo/fork.go @@ -156,13 +156,18 @@ func ForkPost(ctx *context.Context) { } var err error + var repo *repo_model.Repository traverseParentRepo := forkRepo for { if !repository.CanUserForkBetweenOwners(ctxUser.ID, traverseParentRepo.OwnerID) { ctx.JSONError(ctx.Tr("repo.settings.new_owner_has_same_repo")) return } - repo := repo_model.GetForkedRepo(ctx, ctxUser.ID, traverseParentRepo.ID) + repo, err = repo_model.GetForkedRepo(ctx, ctxUser.ID, traverseParentRepo.ID) + if err != nil { + ctx.ServerError("GetForkedRepo", err) + return + } if repo != nil { ctx.JSONRedirect(ctxUser.HomeLink() + "/" + url.PathEscape(repo.Name)) return @@ -189,7 +194,7 @@ func ForkPost(ctx *context.Context) { } } - repo := ForkRepoTo(ctx, ctxUser, repo_service.ForkRepoOptions{ + forkedRepo := ForkRepoTo(ctx, ctxUser, repo_service.ForkRepoOptions{ BaseRepo: forkRepo, Name: form.RepoName, Description: form.Description, @@ -198,7 +203,7 @@ func ForkPost(ctx *context.Context) { if ctx.Written() { return } - ctx.JSONRedirect(ctxUser.HomeLink() + "/" + url.PathEscape(repo.Name)) + ctx.JSONRedirect(ctxUser.HomeLink() + "/" + url.PathEscape(forkedRepo.Name)) } func ForkRepoTo(ctx *context.Context, owner *user_model.User, forkOpts repo_service.ForkRepoOptions) *repo_model.Repository { diff --git a/services/context/repo.go b/services/context/repo.go index b7da0fcd79..ac1a1b60e9 100644 --- a/services/context/repo.go +++ b/services/context/repo.go @@ -123,10 +123,14 @@ func PrepareCommitFormOptions(ctx *Context, doer *user_model.User, targetRepo *r branchName := refName.ShortName() // TODO: CanMaintainerWriteToBranch is a bad name, but it really does what "CanWriteToBranch" does if !issues_model.CanMaintainerWriteToBranch(ctx, doerRepoPerm, branchName, doer) { - targetRepo = repo_model.GetForkedRepo(ctx, doer.ID, targetRepo.ID) - if targetRepo == nil { + forkedRepo, err := repo_model.GetForkedRepo(ctx, doer.ID, targetRepo.ID) + if err != nil { + return nil, err + } + if forkedRepo == nil { return &CommitFormOptions{NeedFork: true}, nil } + targetRepo = forkedRepo // now, we get our own forked repo; it must be writable by us. } submitToForkedRepo := targetRepo.ID != originRepo.ID diff --git a/services/repository/fork.go b/services/repository/fork.go index c8433dbf3f..83a0da4ca3 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -85,8 +85,9 @@ func CheckForkOnEditPermissions(ctx context.Context, doer *user_model.User, repo // Check for existing fork g.Go(func() error { - existingFork = repo_model.GetForkedRepo(gCtx, doer.ID, repo.ID) - return nil // GetForkedRepo doesn't return an error + var err error + existingFork, err = repo_model.GetForkedRepo(gCtx, doer.ID, repo.ID) + return err }) // Wait for both queries to complete From 2d929e2ccd810ec785716da7c56d3e85f477250d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Mon, 22 Dec 2025 12:50:48 +0100 Subject: [PATCH 27/37] linting: fix issues * fix modal promise resolution pattern * add PathEscapeSegments to article template URL * update test comment to list all removed special characters --- custom/templates/shared/repo/article.tmpl | 2 +- eslint.config.ts | 1 + models/db/error_nosqlite.go | 4 ++-- models/migrations/custom/v1_25_custom/v326_test.go | 2 +- models/migrations/custom/v1_25_custom/v327.go | 3 +++ models/migrations/custom/v1_25_custom/v327_test.go | 1 - services/repository/fork.go | 6 +++--- tests/e2e/fork-article-modal.test.e2e.ts | 12 ++++++------ tests/integration/api_fork_test.go | 2 +- tests/integration/repo_fork_edit_test.go | 1 - web_src/js/features/article-editor.ts | 5 +++-- 11 files changed, 21 insertions(+), 18 deletions(-) diff --git a/custom/templates/shared/repo/article.tmpl b/custom/templates/shared/repo/article.tmpl index a98a98d580..4cadefac3a 100644 --- a/custom/templates/shared/repo/article.tmpl +++ b/custom/templates/shared/repo/article.tmpl @@ -68,7 +68,7 @@ {{if .IsSigned}} {{if .HasExistingFork}} {{/* User has an existing fork - redirect to fork's article page */}} - + {{svg "octicon-repo-forked" 16 "tw-mr-1"}} {{ctx.Locale.Tr "repo.editor.edit_in_your_fork"}} diff --git a/eslint.config.ts b/eslint.config.ts index 678a49647c..0358f8d341 100644 --- a/eslint.config.ts +++ b/eslint.config.ts @@ -26,6 +26,7 @@ export default defineConfig([ 'web_src/js/vendor', 'web_src/fomantic', 'public/assets/js', + 'tests/e2e/reports', ]), { files: [`**/*.{${[...jsExts, ...tsExts].join(',')}}`], diff --git a/models/db/error_nosqlite.go b/models/db/error_nosqlite.go index 3a8568fcee..3d496345df 100644 --- a/models/db/error_nosqlite.go +++ b/models/db/error_nosqlite.go @@ -1,8 +1,8 @@ -//go:build !sqlite - // Copyright 2021 The Gitea Authors. All rights reserved. // SPDX-License-Identifier: MIT +//go:build !sqlite + package db // isErrDuplicateKeySQLite is a stub for non-SQLite builds. diff --git a/models/migrations/custom/v1_25_custom/v326_test.go b/models/migrations/custom/v1_25_custom/v326_test.go index e3abd73f35..2b1c0145b4 100644 --- a/models/migrations/custom/v1_25_custom/v326_test.go +++ b/models/migrations/custom/v1_25_custom/v326_test.go @@ -269,7 +269,7 @@ func Test_AddSubjectSlugColumn(t *testing.T) { slugMap[s.Name] = s.Slug } - // @ and # are removed, not replaced with hyphens + // Special characters (@, #, !) are removed, not replaced with hyphens assert.Equal(t, "helloworld2024", slugMap["Hello@World#2024!"]) // Underscores are replaced with hyphens assert.Equal(t, "hello-world-test", slugMap["hello_world_test"]) diff --git a/models/migrations/custom/v1_25_custom/v327.go b/models/migrations/custom/v1_25_custom/v327.go index abf1444e8d..acc3fe439e 100644 --- a/models/migrations/custom/v1_25_custom/v327.go +++ b/models/migrations/custom/v1_25_custom/v327.go @@ -38,6 +38,9 @@ func (*repositoryForkOnEditIndexes) TableIndices() []*schemas.Index { } // AddCompositeIndexesForForkOnEdit adds composite indexes to optimize fork-on-edit permission queries. +// These composite indexes optimize the CheckForkOnEditPermissions queries: +// - (owner_id, subject_id): Used by GetRepositoryByOwnerIDAndSubjectID to check if user owns a different repo for the same subject +// - (owner_id, fork_id): Used by GetForkedRepo and HasForkedRepo to detect existing forks func AddCompositeIndexesForForkOnEdit(x *xorm.Engine) error { return x.Sync(new(repositoryForkOnEditIndexes)) } diff --git a/models/migrations/custom/v1_25_custom/v327_test.go b/models/migrations/custom/v1_25_custom/v327_test.go index 1edc014799..921633f9ab 100644 --- a/models/migrations/custom/v1_25_custom/v327_test.go +++ b/models/migrations/custom/v1_25_custom/v327_test.go @@ -140,4 +140,3 @@ func Test_AddCompositeIndexesForForkOnEdit(t *testing.T) { } }) } - diff --git a/services/repository/fork.go b/services/repository/fork.go index 83a0da4ca3..fe9c811713 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -51,7 +51,7 @@ type ForkOnEditPermissions struct { // CheckForkOnEditPermissions determines the user's editing permissions for a repository. // It checks ownership, subject ownership restrictions, and existing forks. -// Returns nil permissions if doer is nil (not signed in). +// Returns an empty permissions struct if doer is nil (not signed in). func CheckForkOnEditPermissions(ctx context.Context, doer *user_model.User, repo *repo_model.Repository) (*ForkOnEditPermissions, error) { perms := &ForkOnEditPermissions{} @@ -67,8 +67,8 @@ func CheckForkOnEditPermissions(ctx context.Context, doer *user_model.User, repo return perms, nil } - // Run subject ownership check and fork detection in parallel - // These queries are independent and can be executed concurrently + // Run subject ownership check and fork detection in parallel. + // These queries are independent and can be executed concurrently. var ownRepo *repo_model.Repository var existingFork *repo_model.Repository diff --git a/tests/e2e/fork-article-modal.test.e2e.ts b/tests/e2e/fork-article-modal.test.e2e.ts index 6ee59cd8db..5a42985e43 100644 --- a/tests/e2e/fork-article-modal.test.e2e.ts +++ b/tests/e2e/fork-article-modal.test.e2e.ts @@ -436,13 +436,13 @@ test.describe('Accessibility Tests', () => { const confirmButton = modal.locator('.actions .ok.button'); // Buttons should have accessible text content - const cancelText = await cancelButton.textContent(); - const confirmText = await confirmButton.textContent(); + const cancelText = cancelButton; + const confirmText = confirmButton; - expect(cancelText).toBeTruthy(); - expect(cancelText!.trim().length).toBeGreaterThan(0); - expect(confirmText).toBeTruthy(); - expect(confirmText!.trim().length).toBeGreaterThan(0); + await expect(cancelText).toHaveText(); + expect(cancelText.trim().length).toBeGreaterThan(0); + await expect(confirmText).toHaveText(); + expect(confirmText.trim().length).toBeGreaterThan(0); // Verify specific accessible names expect(cancelText).toContain('Go back'); diff --git a/tests/integration/api_fork_test.go b/tests/integration/api_fork_test.go index b03ff8c67c..0b1096d351 100644 --- a/tests/integration/api_fork_test.go +++ b/tests/integration/api_fork_test.go @@ -143,7 +143,7 @@ func TestAPIForkWithSubjectConflict(t *testing.T) { repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) // Verify repo1 has a subject - require.Greater(t, repo1.SubjectID, int64(0), "repo1 should have a subject") + require.Positive(t, repo1.SubjectID, "repo1 should have a subject") subject := unittest.AssertExistsAndLoadBean(t, &repo_model.Subject{ID: repo1.SubjectID}) // Create a repository for user5 with the same subject diff --git a/tests/integration/repo_fork_edit_test.go b/tests/integration/repo_fork_edit_test.go index a2fcca0949..2e4cfc269f 100644 --- a/tests/integration/repo_fork_edit_test.go +++ b/tests/integration/repo_fork_edit_test.go @@ -388,4 +388,3 @@ func TestForkAndEditSecurityBypass(t *testing.T) { "fork_and_edit=true SHOULD bypass CanWriteToBranch for _new action") }) } - diff --git a/web_src/js/features/article-editor.ts b/web_src/js/features/article-editor.ts index e5ba16ab93..1c1d0f5cba 100644 --- a/web_src/js/features/article-editor.ts +++ b/web_src/js/features/article-editor.ts @@ -23,14 +23,15 @@ function createForkConfirmModal(title: string, body: string, confirmText: string function showForkConfirmModal(title: string, body: string, confirmText: string, cancelText: string): Promise { const modal = createForkConfirmModal(title, body, confirmText, cancelText); return new Promise((resolve) => { + let approved = false; const $modal = fomanticQuery(modal); $modal.modal({ onApprove() { - resolve(true); + approved = true; }, onHidden() { $modal.remove(); - resolve(false); + resolve(approved); }, }).modal('show'); }); From 3cc6c00db3e29382c0f9c6778c7efdc0ae7c558d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Mon, 22 Dec 2025 17:56:27 +0100 Subject: [PATCH 28/37] tests: use slices instead of forloop --- models/migrations/custom/v1_25_custom/v327_test.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/models/migrations/custom/v1_25_custom/v327_test.go b/models/migrations/custom/v1_25_custom/v327_test.go index 921633f9ab..8bc3b7b666 100644 --- a/models/migrations/custom/v1_25_custom/v327_test.go +++ b/models/migrations/custom/v1_25_custom/v327_test.go @@ -4,6 +4,7 @@ package v1_25_custom import ( + "slices" "testing" "code.gitea.io/gitea/models/migrations/base" @@ -37,13 +38,7 @@ func Test_AddCompositeIndexesForForkOnEdit(t *testing.T) { if len(index.Cols) == len(cols) { match := true for _, col := range cols { - found := false - for _, indexCol := range index.Cols { - if indexCol == col { - found = true - break - } - } + found := slices.Contains(index.Cols, col) if !found { match = false break From a6a3609fdd193b424e1710c99e42291a30811672 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Mon, 22 Dec 2025 18:11:29 +0100 Subject: [PATCH 29/37] tests: fix ESLint and TypeScript errors * replace page.waitForSelector() with expect(locator).toBeAttached() to satisfy playwright/no-wait-for-selector rule * fix accessibility test to use proper Playwright assertions instead of treating Locators as strings --- tests/e2e/fork-article-modal.test.e2e.ts | 34 +++++++++--------------- 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/tests/e2e/fork-article-modal.test.e2e.ts b/tests/e2e/fork-article-modal.test.e2e.ts index 5a42985e43..78270dc4eb 100644 --- a/tests/e2e/fork-article-modal.test.e2e.ts +++ b/tests/e2e/fork-article-modal.test.e2e.ts @@ -20,7 +20,7 @@ test.describe('Fork Article Confirmation Modal', () => { // Listen for console errors to debug JavaScript issues page.on('console', (msg) => { if (msg.type() === 'error') { - console.log(`Console error: ${msg.text()}`); + console.error(`Console error: ${msg.text()}`); } }); @@ -38,7 +38,7 @@ test.describe('Fork Article Confirmation Modal', () => { // Wait for the Toast UI Editor to be initialized // The editor creates a .toastui-editor element when initialized - await page.waitForSelector('.toastui-editor', {state: 'attached', timeout: 20000}); + await expect(page.locator('.toastui-editor')).toBeAttached({timeout: 20000}); await forkButton.click(); @@ -93,7 +93,7 @@ test.describe('Fork Article Confirmation Modal', () => { await expect(page.locator('#article-edit-form')).toBeVisible({timeout: 10000}); // Wait for the Toast UI Editor to be initialized - await page.waitForSelector('.toastui-editor', {state: 'attached', timeout: 20000}); + await expect(page.locator('.toastui-editor')).toBeAttached({timeout: 20000}); await forkButton.click(); @@ -131,7 +131,7 @@ test.describe('Fork Article Confirmation Modal', () => { await expect(page.locator('#article-edit-form')).toBeVisible({timeout: 10000}); // Wait for the Toast UI Editor to be initialized - await page.waitForSelector('.toastui-editor', {state: 'attached', timeout: 20000}); + await expect(page.locator('.toastui-editor')).toBeAttached({timeout: 20000}); const urlBefore = page.url(); @@ -166,7 +166,7 @@ test.describe('Fork Article Confirmation Modal', () => { await expect(page.locator('#article-edit-form')).toBeVisible({timeout: 10000}); // Wait for the Toast UI Editor to be initialized - await page.waitForSelector('.toastui-editor', {state: 'attached', timeout: 20000}); + await expect(page.locator('.toastui-editor')).toBeAttached({timeout: 20000}); const urlBefore = page.url(); @@ -254,7 +254,7 @@ test.describe('Fork-on-Edit Permission Tests', () => { await expect(page.locator('#article-edit-form')).toBeVisible({timeout: 10000}); // Wait for the Toast UI Editor to be initialized - await page.waitForSelector('.toastui-editor', {state: 'attached', timeout: 20000}); + await expect(page.locator('.toastui-editor')).toBeAttached({timeout: 20000}); await submitButton.click(); @@ -339,7 +339,7 @@ test.describe('Accessibility Tests', () => { await expect(forkButton).toBeVisible({timeout: 10000}); // Wait for the Toast UI Editor to be initialized - await page.waitForSelector('.toastui-editor', {state: 'attached', timeout: 20000}); + await expect(page.locator('.toastui-editor')).toBeAttached({timeout: 20000}); // Focus the Fork button and press Enter await forkButton.focus(); @@ -363,7 +363,7 @@ test.describe('Accessibility Tests', () => { await expect(forkButton).toBeVisible({timeout: 10000}); // Wait for the Toast UI Editor to be initialized - await page.waitForSelector('.toastui-editor', {state: 'attached', timeout: 20000}); + await expect(page.locator('.toastui-editor')).toBeAttached({timeout: 20000}); await forkButton.click(); @@ -395,7 +395,7 @@ test.describe('Accessibility Tests', () => { await expect(forkButton).toBeVisible({timeout: 10000}); // Wait for the Toast UI Editor to be initialized - await page.waitForSelector('.toastui-editor', {state: 'attached', timeout: 20000}); + await expect(page.locator('.toastui-editor')).toBeAttached({timeout: 20000}); await forkButton.click(); @@ -425,7 +425,7 @@ test.describe('Accessibility Tests', () => { await expect(forkButton).toBeVisible({timeout: 10000}); // Wait for the Toast UI Editor to be initialized - await page.waitForSelector('.toastui-editor', {state: 'attached', timeout: 20000}); + await expect(page.locator('.toastui-editor')).toBeAttached({timeout: 20000}); await forkButton.click(); @@ -436,17 +436,9 @@ test.describe('Accessibility Tests', () => { const confirmButton = modal.locator('.actions .ok.button'); // Buttons should have accessible text content - const cancelText = cancelButton; - const confirmText = confirmButton; - - await expect(cancelText).toHaveText(); - expect(cancelText.trim().length).toBeGreaterThan(0); - await expect(confirmText).toHaveText(); - expect(confirmText.trim().length).toBeGreaterThan(0); - - // Verify specific accessible names - expect(cancelText).toContain('Go back'); - expect(confirmText).toContain('Yes, Fork article'); + // Verify specific accessible names using Playwright's toContainText assertion + await expect(cancelButton).toContainText('Go back'); + await expect(confirmButton).toContainText('Yes, Fork article'); await context.close(); }); From 1bcad13e71b70d229bbb293bde601550b7ae25cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Mon, 22 Dec 2025 18:52:43 +0100 Subject: [PATCH 30/37] tests: fix toastui-editor selector * toast UI Editor creates two .toastui-editor elements (md-mode and ww-mode) * use .first() to satisfy Playwright's strict mode requirement --- tests/e2e/fork-article-modal.test.e2e.ts | 28 +++++++++++++++--------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/tests/e2e/fork-article-modal.test.e2e.ts b/tests/e2e/fork-article-modal.test.e2e.ts index 78270dc4eb..e9913d6a24 100644 --- a/tests/e2e/fork-article-modal.test.e2e.ts +++ b/tests/e2e/fork-article-modal.test.e2e.ts @@ -37,8 +37,8 @@ test.describe('Fork Article Confirmation Modal', () => { await expect(page.locator('#article-edit-form')).toBeVisible({timeout: 10000}); // Wait for the Toast UI Editor to be initialized - // The editor creates a .toastui-editor element when initialized - await expect(page.locator('.toastui-editor')).toBeAttached({timeout: 20000}); + // The editor creates two .toastui-editor elements (md-mode and ww-mode), so use .first() + await expect(page.locator('.toastui-editor').first()).toBeAttached({timeout: 20000}); await forkButton.click(); @@ -93,7 +93,8 @@ test.describe('Fork Article Confirmation Modal', () => { await expect(page.locator('#article-edit-form')).toBeVisible({timeout: 10000}); // Wait for the Toast UI Editor to be initialized - await expect(page.locator('.toastui-editor')).toBeAttached({timeout: 20000}); + // The editor creates two .toastui-editor elements (md-mode and ww-mode), so use .first() + await expect(page.locator('.toastui-editor').first()).toBeAttached({timeout: 20000}); await forkButton.click(); @@ -131,7 +132,8 @@ test.describe('Fork Article Confirmation Modal', () => { await expect(page.locator('#article-edit-form')).toBeVisible({timeout: 10000}); // Wait for the Toast UI Editor to be initialized - await expect(page.locator('.toastui-editor')).toBeAttached({timeout: 20000}); + // The editor creates two .toastui-editor elements (md-mode and ww-mode), so use .first() + await expect(page.locator('.toastui-editor').first()).toBeAttached({timeout: 20000}); const urlBefore = page.url(); @@ -166,7 +168,8 @@ test.describe('Fork Article Confirmation Modal', () => { await expect(page.locator('#article-edit-form')).toBeVisible({timeout: 10000}); // Wait for the Toast UI Editor to be initialized - await expect(page.locator('.toastui-editor')).toBeAttached({timeout: 20000}); + // The editor creates two .toastui-editor elements (md-mode and ww-mode), so use .first() + await expect(page.locator('.toastui-editor').first()).toBeAttached({timeout: 20000}); const urlBefore = page.url(); @@ -254,7 +257,8 @@ test.describe('Fork-on-Edit Permission Tests', () => { await expect(page.locator('#article-edit-form')).toBeVisible({timeout: 10000}); // Wait for the Toast UI Editor to be initialized - await expect(page.locator('.toastui-editor')).toBeAttached({timeout: 20000}); + // The editor creates two .toastui-editor elements (md-mode and ww-mode), so use .first() + await expect(page.locator('.toastui-editor').first()).toBeAttached({timeout: 20000}); await submitButton.click(); @@ -339,7 +343,8 @@ test.describe('Accessibility Tests', () => { await expect(forkButton).toBeVisible({timeout: 10000}); // Wait for the Toast UI Editor to be initialized - await expect(page.locator('.toastui-editor')).toBeAttached({timeout: 20000}); + // The editor creates two .toastui-editor elements (md-mode and ww-mode), so use .first() + await expect(page.locator('.toastui-editor').first()).toBeAttached({timeout: 20000}); // Focus the Fork button and press Enter await forkButton.focus(); @@ -363,7 +368,8 @@ test.describe('Accessibility Tests', () => { await expect(forkButton).toBeVisible({timeout: 10000}); // Wait for the Toast UI Editor to be initialized - await expect(page.locator('.toastui-editor')).toBeAttached({timeout: 20000}); + // The editor creates two .toastui-editor elements (md-mode and ww-mode), so use .first() + await expect(page.locator('.toastui-editor').first()).toBeAttached({timeout: 20000}); await forkButton.click(); @@ -395,7 +401,8 @@ test.describe('Accessibility Tests', () => { await expect(forkButton).toBeVisible({timeout: 10000}); // Wait for the Toast UI Editor to be initialized - await expect(page.locator('.toastui-editor')).toBeAttached({timeout: 20000}); + // The editor creates two .toastui-editor elements (md-mode and ww-mode), so use .first() + await expect(page.locator('.toastui-editor').first()).toBeAttached({timeout: 20000}); await forkButton.click(); @@ -425,7 +432,8 @@ test.describe('Accessibility Tests', () => { await expect(forkButton).toBeVisible({timeout: 10000}); // Wait for the Toast UI Editor to be initialized - await expect(page.locator('.toastui-editor')).toBeAttached({timeout: 20000}); + // The editor creates two .toastui-editor elements (md-mode and ww-mode), so use .first() + await expect(page.locator('.toastui-editor').first()).toBeAttached({timeout: 20000}); await forkButton.click(); From 482f29f89f40f2a260880a6ea339e1767ba2f324 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Gaud=C3=AAncio?= Date: Tue, 23 Dec 2025 00:27:07 +0100 Subject: [PATCH 31/37] fork-on-edit: address code review findings * add maxUniqueNameAttempts constant to replace magic number 1000 * use t.Cleanup with nil check instead of defer for safer test cleanup * remove unused data-existing-fork attribute from article template * add comment explaining that subject ownership check takes precedence --- custom/templates/shared/repo/article.tmpl | 1 - routers/web/repo/editor_util.go | 10 +++++++--- services/repository/fork.go | 8 +++++--- tests/integration/api_fork_test.go | 8 +++++--- 4 files changed, 17 insertions(+), 10 deletions(-) diff --git a/custom/templates/shared/repo/article.tmpl b/custom/templates/shared/repo/article.tmpl index 4cadefac3a..ab7e31fc52 100644 --- a/custom/templates/shared/repo/article.tmpl +++ b/custom/templates/shared/repo/article.tmpl @@ -91,7 +91,6 @@ {{/* User needs to create a fork - show fork-and-submit button */}}