From ad2d38a59b89424dca7a0d3c41c5f7d51defa31a Mon Sep 17 00:00:00 2001 From: Nicolas Date: Mon, 11 May 2026 18:00:25 +0200 Subject: [PATCH 1/6] fix: allow direct commits for unprotected files when branch-wide push is restricted --- services/context/repo.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/services/context/repo.go b/services/context/repo.go index 4c31b07b347f6..a137448d95f2b 100644 --- a/services/context/repo.go +++ b/services/context/repo.go @@ -173,6 +173,14 @@ func PrepareCommitFormOptions(ctx *Context, doer *user_model.User, targetRepo *r protectedBranch.Repo = targetRepo canPushWithProtection = protectedBranch.CanUserPush(ctx, doer) protectionRequireSigned = protectedBranch.RequireSignedCommits + // If the user can't push branch-wide but the specific file being edited + // matches an unprotected file pattern, direct commit is still allowed. + if !canPushWithProtection && ctx.Repo.TreePath != "" { + globUnprotected := protectedBranch.GetUnprotectedFilePatterns() + if protectedBranch.IsUnprotectedFile(globUnprotected, ctx.Repo.TreePath) { + canPushWithProtection = true + } + } } targetGitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, targetRepo) From a43d1f3829335f4e9a4b9a667268b7b086452a6d Mon Sep 17 00:00:00 2001 From: Nicolas Date: Mon, 11 May 2026 19:42:22 +0200 Subject: [PATCH 2/6] fixes + test --- routers/web/repo/editor.go | 4 ++-- services/context/repo.go | 6 +++--- tests/integration/editor_test.go | 21 +++++++++++++++++++++ 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/routers/web/repo/editor.go b/routers/web/repo/editor.go index 0aa32716836f7..4936508c96016 100644 --- a/routers/web/repo/editor.go +++ b/routers/web/repo/editor.go @@ -56,7 +56,7 @@ func prepareEditorPageFormOptions(ctx *context.Context, editorAction string) *co return nil } - commitFormOptions, err := context.PrepareCommitFormOptions(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.Permission, ctx.Repo.RefFullName) + commitFormOptions, err := context.PrepareCommitFormOptions(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.Permission, ctx.Repo.RefFullName, ctx.Repo.TreePath) if err != nil { ctx.ServerError("PrepareCommitFormOptions", err) return nil @@ -121,7 +121,7 @@ func prepareEditorCommitSubmittedForm[T forms.CommitCommonFormInterface](ctx *co commonForm := form.GetCommitCommonForm() commonForm.TreePath = files_service.CleanGitTreePath(commonForm.TreePath) - commitFormOptions, err := context.PrepareCommitFormOptions(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.Permission, ctx.Repo.RefFullName) + commitFormOptions, err := context.PrepareCommitFormOptions(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.Permission, ctx.Repo.RefFullName, commonForm.TreePath) if err != nil { ctx.ServerError("PrepareCommitFormOptions", err) return nil diff --git a/services/context/repo.go b/services/context/repo.go index a137448d95f2b..2769664ce1ce3 100644 --- a/services/context/repo.go +++ b/services/context/repo.go @@ -141,7 +141,7 @@ type CommitFormOptions struct { CanCreateBasePullRequest bool } -func PrepareCommitFormOptions(ctx *Context, doer *user_model.User, targetRepo *repo_model.Repository, doerRepoPerm access_model.Permission, refName git.RefName) (*CommitFormOptions, error) { +func PrepareCommitFormOptions(ctx *Context, doer *user_model.User, targetRepo *repo_model.Repository, doerRepoPerm access_model.Permission, refName git.RefName, treePath string) (*CommitFormOptions, error) { if !refName.IsBranch() { // it shouldn't happen because middleware already checks return nil, util.NewInvalidArgumentErrorf("ref %q is not a branch", refName) @@ -175,9 +175,9 @@ func PrepareCommitFormOptions(ctx *Context, doer *user_model.User, targetRepo *r protectionRequireSigned = protectedBranch.RequireSignedCommits // If the user can't push branch-wide but the specific file being edited // matches an unprotected file pattern, direct commit is still allowed. - if !canPushWithProtection && ctx.Repo.TreePath != "" { + if !canPushWithProtection && treePath != "" { globUnprotected := protectedBranch.GetUnprotectedFilePatterns() - if protectedBranch.IsUnprotectedFile(globUnprotected, ctx.Repo.TreePath) { + if protectedBranch.IsUnprotectedFile(globUnprotected, treePath) { canPushWithProtection = true } } diff --git a/tests/integration/editor_test.go b/tests/integration/editor_test.go index 4a06159690783..9073879df3de4 100644 --- a/tests/integration/editor_test.go +++ b/tests/integration/editor_test.go @@ -98,6 +98,27 @@ func testEditorProtectedBranch(t *testing.T) { resp := testEditorActionPostRequest(t, session, "/user2/repo1/_new/master/", map[string]string{"tree_path": "test-protected-branch.txt", "commit_choice": "direct"}) assert.Equal(t, http.StatusBadRequest, resp.Code) assert.Equal(t, `Cannot commit to protected branch "master".`, test.ParseJSONError(resp.Body.Bytes()).ErrorMessage) + + // Change "master" branch to mark files under "docs/" as unprotected + req = NewRequestWithValues(t, "POST", "/user2/repo1/settings/branches/edit", map[string]string{ + "rule_name": "master", + "protected_file_patterns": "", + "unprotected_file_patterns": "docs/*.md", + "enable_push": "true", + }) + session.MakeRequest(t, req, http.StatusSeeOther) + flashMsg = session.GetCookieFlashMessage() + assert.Equal(t, `Branch protection for rule "master" has been updated.`, flashMsg.SuccessMsg) + + // Try to commit a non-matching file to the "master" branch and it should fail + resp = testEditorActionPostRequest(t, session, "/user2/repo1/_new/master/", map[string]string{"tree_path": "test-protected-branch.txt", "commit_choice": "direct"}) + assert.Equal(t, http.StatusBadRequest, resp.Code) + assert.Equal(t, `Cannot commit to protected branch "master".`, test.ParseJSONError(resp.Body.Bytes()).ErrorMessage) + + // Try to commit a file matching the unprotected pattern and it should succeed + resp = testEditorActionPostRequest(t, session, "/user2/repo1/_new/master/", map[string]string{"tree_path": "docs/new.md", "commit_choice": "direct"}) + assert.Equal(t, http.StatusOK, resp.Code) + assert.Contains(t, resp.Body.String(), `"redirect":"/user2/repo1/src/branch/master/docs/new.md"`) } func testEditorActionPostRequest(t *testing.T, session *TestSession, requestPath string, params map[string]string) *httptest.ResponseRecorder { From 1de27bee5ce49260719d47b945ff702d5c49b716 Mon Sep 17 00:00:00 2001 From: Nicolas Date: Mon, 11 May 2026 19:50:31 +0200 Subject: [PATCH 3/6] use correct Treepath --- routers/web/repo/editor.go | 10 ++++++++-- services/context/repo.go | 6 +++--- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/routers/web/repo/editor.go b/routers/web/repo/editor.go index 4936508c96016..9400462c25506 100644 --- a/routers/web/repo/editor.go +++ b/routers/web/repo/editor.go @@ -56,7 +56,7 @@ func prepareEditorPageFormOptions(ctx *context.Context, editorAction string) *co return nil } - commitFormOptions, err := context.PrepareCommitFormOptions(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.Permission, ctx.Repo.RefFullName, ctx.Repo.TreePath) + commitFormOptions, err := context.PrepareCommitFormOptions(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.Permission, ctx.Repo.RefFullName) if err != nil { ctx.ServerError("PrepareCommitFormOptions", err) return nil @@ -120,8 +120,14 @@ func prepareEditorCommitSubmittedForm[T forms.CommitCommonFormInterface](ctx *co commonForm := form.GetCommitCommonForm() commonForm.TreePath = files_service.CleanGitTreePath(commonForm.TreePath) + // For "_new" the URL has no file segment, so ctx.Repo.TreePath is empty; + // use the submitted form path so PrepareCommitFormOptions can evaluate + // branch protection (e.g. unprotected file patterns) against the real file. + if ctx.Repo.TreePath == "" { + ctx.Repo.TreePath = commonForm.TreePath + } - commitFormOptions, err := context.PrepareCommitFormOptions(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.Permission, ctx.Repo.RefFullName, commonForm.TreePath) + commitFormOptions, err := context.PrepareCommitFormOptions(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.Permission, ctx.Repo.RefFullName) if err != nil { ctx.ServerError("PrepareCommitFormOptions", err) return nil diff --git a/services/context/repo.go b/services/context/repo.go index 2769664ce1ce3..a137448d95f2b 100644 --- a/services/context/repo.go +++ b/services/context/repo.go @@ -141,7 +141,7 @@ type CommitFormOptions struct { CanCreateBasePullRequest bool } -func PrepareCommitFormOptions(ctx *Context, doer *user_model.User, targetRepo *repo_model.Repository, doerRepoPerm access_model.Permission, refName git.RefName, treePath string) (*CommitFormOptions, error) { +func PrepareCommitFormOptions(ctx *Context, doer *user_model.User, targetRepo *repo_model.Repository, doerRepoPerm access_model.Permission, refName git.RefName) (*CommitFormOptions, error) { if !refName.IsBranch() { // it shouldn't happen because middleware already checks return nil, util.NewInvalidArgumentErrorf("ref %q is not a branch", refName) @@ -175,9 +175,9 @@ func PrepareCommitFormOptions(ctx *Context, doer *user_model.User, targetRepo *r protectionRequireSigned = protectedBranch.RequireSignedCommits // If the user can't push branch-wide but the specific file being edited // matches an unprotected file pattern, direct commit is still allowed. - if !canPushWithProtection && treePath != "" { + if !canPushWithProtection && ctx.Repo.TreePath != "" { globUnprotected := protectedBranch.GetUnprotectedFilePatterns() - if protectedBranch.IsUnprotectedFile(globUnprotected, treePath) { + if protectedBranch.IsUnprotectedFile(globUnprotected, ctx.Repo.TreePath) { canPushWithProtection = true } } From aadf6458c42cb5c5ebf9325649caf468c2d8cf60 Mon Sep 17 00:00:00 2001 From: Nicolas Date: Mon, 11 May 2026 20:30:14 +0200 Subject: [PATCH 4/6] enhance --- routers/web/repo/editor.go | 34 ++++++++++++++++++++++++++------ services/context/repo.go | 11 +++++++---- tests/integration/editor_test.go | 8 ++++++++ 3 files changed, 43 insertions(+), 10 deletions(-) diff --git a/routers/web/repo/editor.go b/routers/web/repo/editor.go index 9400462c25506..b46b5bcc0ea8b 100644 --- a/routers/web/repo/editor.go +++ b/routers/web/repo/editor.go @@ -111,6 +111,26 @@ func (f *preparedEditorCommitForm[T]) GetCommitMessage(defaultCommitMessage stri return commitMessage } +// destinationAllowsDirectCommit reports whether the form-submitted destination +// path qualifies for a direct commit via an unprotected file pattern, in cases +// where PrepareCommitFormOptions (which only saw the URL-derived path) said no. +func destinationAllowsDirectCommit(ctx *context.Context, opts *context.CommitFormOptions, destTreePath string) bool { + if opts.CanCommitToBranch { + return false // already allowed; nothing to refine + } + if destTreePath == "" || destTreePath == ctx.Repo.TreePath { + return false // no form destination, or same as URL path already evaluated + } + if opts.RequireSigned && !opts.WillSign { + return false // signing requirement still blocks + } + pb, _ := git_model.GetFirstMatchProtectedBranchRule(ctx, ctx.Repo.Repository.ID, ctx.Repo.BranchName) + if pb == nil { + return false + } + return pb.IsUnprotectedFile(pb.GetUnprotectedFilePatterns(), destTreePath) +} + func prepareEditorCommitSubmittedForm[T forms.CommitCommonFormInterface](ctx *context.Context) *preparedEditorCommitForm[T] { form := web.GetForm(ctx).(T) if ctx.HasError() { @@ -120,18 +140,20 @@ func prepareEditorCommitSubmittedForm[T forms.CommitCommonFormInterface](ctx *co commonForm := form.GetCommitCommonForm() commonForm.TreePath = files_service.CleanGitTreePath(commonForm.TreePath) - // For "_new" the URL has no file segment, so ctx.Repo.TreePath is empty; - // use the submitted form path so PrepareCommitFormOptions can evaluate - // branch protection (e.g. unprotected file patterns) against the real file. - if ctx.Repo.TreePath == "" { - ctx.Repo.TreePath = commonForm.TreePath - } commitFormOptions, err := context.PrepareCommitFormOptions(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.Permission, ctx.Repo.RefFullName) if err != nil { ctx.ServerError("PrepareCommitFormOptions", err) return nil } + // For "_edit"/"_new" POSTs the form submits the commit destination, which can + // differ from the URL-derived ctx.Repo.TreePath (rename on "_edit", or a path + // under "_new///"). PrepareCommitFormOptions only evaluated the + // URL path against unprotected file patterns; if the destination is the one + // that matches an unprotected pattern, allow direct commit. + if destinationAllowsDirectCommit(ctx, commitFormOptions, commonForm.TreePath) { + commitFormOptions.CanCommitToBranch = true + } if commitFormOptions.NeedFork { // It shouldn't happen, because we should have done the checks in the "GET" request. But just in case. ctx.JSONError(ctx.Locale.TrString("error.not_found")) diff --git a/services/context/repo.go b/services/context/repo.go index a137448d95f2b..580ffc3720a67 100644 --- a/services/context/repo.go +++ b/services/context/repo.go @@ -173,11 +173,14 @@ func PrepareCommitFormOptions(ctx *Context, doer *user_model.User, targetRepo *r protectedBranch.Repo = targetRepo canPushWithProtection = protectedBranch.CanUserPush(ctx, doer) protectionRequireSigned = protectedBranch.RequireSignedCommits - // If the user can't push branch-wide but the specific file being edited - // matches an unprotected file pattern, direct commit is still allowed. + // If the user can't push branch-wide but the file being edited matches + // an unprotected file pattern, direct commit is still allowed. This uses + // ctx.Repo.TreePath, which is the file being edited on GET ("_edit"), + // the directory on "_new///", or the file path on + // "_delete"/"_upload". The "_edit"/"_new" POST handlers refine this + // further using the form-submitted destination path. if !canPushWithProtection && ctx.Repo.TreePath != "" { - globUnprotected := protectedBranch.GetUnprotectedFilePatterns() - if protectedBranch.IsUnprotectedFile(globUnprotected, ctx.Repo.TreePath) { + if protectedBranch.IsUnprotectedFile(protectedBranch.GetUnprotectedFilePatterns(), ctx.Repo.TreePath) { canPushWithProtection = true } } diff --git a/tests/integration/editor_test.go b/tests/integration/editor_test.go index 9073879df3de4..7b4b848274c04 100644 --- a/tests/integration/editor_test.go +++ b/tests/integration/editor_test.go @@ -119,6 +119,14 @@ func testEditorProtectedBranch(t *testing.T) { resp = testEditorActionPostRequest(t, session, "/user2/repo1/_new/master/", map[string]string{"tree_path": "docs/new.md", "commit_choice": "direct"}) assert.Equal(t, http.StatusOK, resp.Code) assert.Contains(t, resp.Body.String(), `"redirect":"/user2/repo1/src/branch/master/docs/new.md"`) + + resp = testEditorActionPostRequest(t, session, "/user2/repo1/_edit/master/docs/new.md", map[string]string{ + "content": "renamed via editor", + "commit_choice": "direct", + "tree_path": "docs/renamed.md", + }) + assert.Equal(t, http.StatusOK, resp.Code) + assert.Contains(t, resp.Body.String(), `"redirect":"/user2/repo1/src/branch/master/docs/renamed.md"`) } func testEditorActionPostRequest(t *testing.T, session *TestSession, requestPath string, params map[string]string) *httptest.ResponseRecorder { From 1c0a1721618506561d4e80cf5b307e9a1e733f06 Mon Sep 17 00:00:00 2001 From: Nicolas Date: Tue, 12 May 2026 21:33:52 +0200 Subject: [PATCH 5/6] fix: refine commit permissions for protected branches and enhance related tests --- routers/web/repo/editor.go | 70 ++++++++++++++------------ routers/web/repo/editor_apply_patch.go | 5 ++ routers/web/repo/editor_cherry_pick.go | 5 ++ services/context/repo.go | 55 ++++++++++++++++---- tests/integration/editor_test.go | 30 +++++++++++ 5 files changed, 121 insertions(+), 44 deletions(-) diff --git a/routers/web/repo/editor.go b/routers/web/repo/editor.go index b46b5bcc0ea8b..8fef17404cbb3 100644 --- a/routers/web/repo/editor.go +++ b/routers/web/repo/editor.go @@ -61,6 +61,10 @@ func prepareEditorPageFormOptions(ctx *context.Context, editorAction string) *co ctx.ServerError("PrepareCommitFormOptions", err) return nil } + // Best-effort UI hint: refine CanCommitToBranch using the URL-derived path + // so the form renders the right default. The actual decision is re-run on + // POST against the operation's full affected path set. + context.RefineCanCommitToBranchByPaths(ctx, commitFormOptions, ctx.Repo.TreePath) if commitFormOptions.NeedFork { ForkToEdit(ctx) @@ -111,26 +115,6 @@ func (f *preparedEditorCommitForm[T]) GetCommitMessage(defaultCommitMessage stri return commitMessage } -// destinationAllowsDirectCommit reports whether the form-submitted destination -// path qualifies for a direct commit via an unprotected file pattern, in cases -// where PrepareCommitFormOptions (which only saw the URL-derived path) said no. -func destinationAllowsDirectCommit(ctx *context.Context, opts *context.CommitFormOptions, destTreePath string) bool { - if opts.CanCommitToBranch { - return false // already allowed; nothing to refine - } - if destTreePath == "" || destTreePath == ctx.Repo.TreePath { - return false // no form destination, or same as URL path already evaluated - } - if opts.RequireSigned && !opts.WillSign { - return false // signing requirement still blocks - } - pb, _ := git_model.GetFirstMatchProtectedBranchRule(ctx, ctx.Repo.Repository.ID, ctx.Repo.BranchName) - if pb == nil { - return false - } - return pb.IsUnprotectedFile(pb.GetUnprotectedFilePatterns(), destTreePath) -} - func prepareEditorCommitSubmittedForm[T forms.CommitCommonFormInterface](ctx *context.Context) *preparedEditorCommitForm[T] { form := web.GetForm(ctx).(T) if ctx.HasError() { @@ -146,28 +130,18 @@ func prepareEditorCommitSubmittedForm[T forms.CommitCommonFormInterface](ctx *co ctx.ServerError("PrepareCommitFormOptions", err) return nil } - // For "_edit"/"_new" POSTs the form submits the commit destination, which can - // differ from the URL-derived ctx.Repo.TreePath (rename on "_edit", or a path - // under "_new///"). PrepareCommitFormOptions only evaluated the - // URL path against unprotected file patterns; if the destination is the one - // that matches an unprotected pattern, allow direct commit. - if destinationAllowsDirectCommit(ctx, commitFormOptions, commonForm.TreePath) { - commitFormOptions.CanCommitToBranch = true - } if commitFormOptions.NeedFork { // It shouldn't happen, because we should have done the checks in the "GET" request. But just in case. ctx.JSONError(ctx.Locale.TrString("error.not_found")) return nil } - // check commit behavior + // The "cannot commit to protected branch" guard runs in each POST handler + // via editorCheckCanCommit, once it has refined CanCommitToBranch against + // the operation's real affected path set (rename touches two paths). fromBaseBranch := ctx.FormString("from_base_branch") commitToNewBranch := commonForm.CommitChoice == editorCommitChoiceNewBranch || fromBaseBranch != "" targetBranchName := util.Iif(commitToNewBranch, commonForm.NewBranchName, ctx.Repo.BranchName) - if targetBranchName == ctx.Repo.BranchName && !commitFormOptions.CanCommitToBranch { - ctx.JSONError(ctx.Tr("repo.editor.cannot_commit_to_protected_branch", targetBranchName)) - return nil - } if !issues.CanMaintainerWriteToBranch(ctx, ctx.Repo.Permission, targetBranchName, ctx.Doer) { ctx.NotFound(nil) @@ -219,6 +193,19 @@ func prepareEditorCommitSubmittedForm[T forms.CommitCommonFormInterface](ctx *co } } +// editorCheckCanCommit refines CanCommitToBranch against the operation's +// actual affected tree paths (rename touches both source and destination), +// then enforces the protected-branch guard when committing to the current +// branch. Returns false if the request has been answered with an error. +func editorCheckCanCommit[T any](ctx *context.Context, parsed *preparedEditorCommitForm[T], treePaths ...string) bool { + context.RefineCanCommitToBranchByPaths(ctx, parsed.CommitFormOptions, treePaths...) + if parsed.NewBranchName == ctx.Repo.BranchName && !parsed.CommitFormOptions.CanCommitToBranch { + ctx.JSONError(ctx.Tr("repo.editor.cannot_commit_to_protected_branch", parsed.NewBranchName)) + return false + } + return true +} + // redirectForCommitChoice redirects after committing the edit to a branch func redirectForCommitChoice[T any](ctx *context.Context, parsed *preparedEditorCommitForm[T], treePath string) { // when editing a file in a PR, it should return to the origin location @@ -364,6 +351,17 @@ func EditFilePost(ctx *context.Context) { return } + // Affected paths: form destination, plus the URL-derived source path on + // "_edit" so a rename's source path is also evaluated against unprotected + // patterns (matches services/pull/patch.go CheckUnprotectedFiles). + affectedPaths := []string{parsed.form.TreePath} + if !isNewFile && ctx.Repo.TreePath != "" && ctx.Repo.TreePath != parsed.form.TreePath { + affectedPaths = append(affectedPaths, ctx.Repo.TreePath) + } + if !editorCheckCanCommit(ctx, parsed, affectedPaths...) { + return + } + defaultCommitMessage := util.Iif(isNewFile, ctx.Locale.TrString("repo.editor.add", parsed.form.TreePath), ctx.Locale.TrString("repo.editor.update", parsed.form.TreePath)) var operation string @@ -429,6 +427,9 @@ func DeleteFilePost(ctx *context.Context) { ctx.JSONError("cannot delete root directory") // it should not happen unless someone is trying to be malicious return } + if !editorCheckCanCommit(ctx, parsed, treePath) { + return + } // Check if the path is a directory entry, err := ctx.Repo.Commit.GetTreeEntryByPath(treePath) @@ -491,6 +492,9 @@ func UploadFilePost(ctx *context.Context) { if ctx.Written() { return } + if !editorCheckCanCommit(ctx, parsed, parsed.form.TreePath) { + return + } defaultCommitMessage := ctx.Locale.TrString("repo.editor.upload_files_to_dir", util.IfZero(parsed.form.TreePath, "/")) err := files_service.UploadRepoFiles(ctx, ctx.Repo.Repository, ctx.Doer, &files_service.UploadRepoFileOptions{ diff --git a/routers/web/repo/editor_apply_patch.go b/routers/web/repo/editor_apply_patch.go index 111e0161f24d7..eb5eb0964cfdf 100644 --- a/routers/web/repo/editor_apply_patch.go +++ b/routers/web/repo/editor_apply_patch.go @@ -30,6 +30,11 @@ func NewDiffPatchPost(ctx *context.Context) { if ctx.Written() { return } + // Patch can touch arbitrary paths; pass the URL path as a best-effort hint. + // Real per-file enforcement lives in the service / pre-receive hook. + if !editorCheckCanCommit(ctx, parsed, ctx.Repo.TreePath) { + return + } defaultCommitMessage := ctx.Locale.TrString("repo.editor.patch") _, err := files.ApplyDiffPatch(ctx, ctx.Repo.Repository, ctx.Doer, &files.ApplyDiffPatchOptions{ diff --git a/routers/web/repo/editor_cherry_pick.go b/routers/web/repo/editor_cherry_pick.go index f3cbc098274b3..53ef853217402 100644 --- a/routers/web/repo/editor_cherry_pick.go +++ b/routers/web/repo/editor_cherry_pick.go @@ -47,6 +47,11 @@ func CherryPickPost(ctx *context.Context) { if ctx.Written() { return } + // Cherry-pick can touch arbitrary paths; pass the URL path as a best-effort + // hint. Real per-file enforcement lives in the service / pre-receive hook. + if !editorCheckCanCommit(ctx, parsed, ctx.Repo.TreePath) { + return + } defaultCommitMessage := util.Iif(parsed.form.Revert, ctx.Locale.TrString("repo.commit.revert-header", fromCommitID), ctx.Locale.TrString("repo.commit.cherry-pick-header", fromCommitID)) opts := &files.ApplyDiffPatchOptions{ diff --git a/services/context/repo.go b/services/context/repo.go index 580ffc3720a67..39da10871f790 100644 --- a/services/context/repo.go +++ b/services/context/repo.go @@ -139,6 +139,8 @@ type CommitFormOptions struct { WontSignReason string CanCreatePullRequest bool CanCreateBasePullRequest bool + + protectedBranch *git_model.ProtectedBranch // cached for RefineCanCommitToBranchByPaths } func PrepareCommitFormOptions(ctx *Context, doer *user_model.User, targetRepo *repo_model.Repository, doerRepoPerm access_model.Permission, refName git.RefName) (*CommitFormOptions, error) { @@ -173,18 +175,10 @@ func PrepareCommitFormOptions(ctx *Context, doer *user_model.User, targetRepo *r protectedBranch.Repo = targetRepo canPushWithProtection = protectedBranch.CanUserPush(ctx, doer) protectionRequireSigned = protectedBranch.RequireSignedCommits - // If the user can't push branch-wide but the file being edited matches - // an unprotected file pattern, direct commit is still allowed. This uses - // ctx.Repo.TreePath, which is the file being edited on GET ("_edit"), - // the directory on "_new///", or the file path on - // "_delete"/"_upload". The "_edit"/"_new" POST handlers refine this - // further using the form-submitted destination path. - if !canPushWithProtection && ctx.Repo.TreePath != "" { - if protectedBranch.IsUnprotectedFile(protectedBranch.GetUnprotectedFilePatterns(), ctx.Repo.TreePath) { - canPushWithProtection = true - } - } } + // The unprotected-file override is applied separately by callers via + // RefineCanCommitToBranchByPaths once they know which paths the commit will + // actually touch — for a rename that's both the source and the destination. targetGitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, targetRepo) if err != nil { @@ -220,6 +214,8 @@ func PrepareCommitFormOptions(ctx *Context, doer *user_model.User, targetRepo *r CanCreatePullRequest: canCreatePullRequest, CanCreateBasePullRequest: canCreateBasePullRequest, + + protectedBranch: protectedBranch, } editorAction := ctx.PathParam("editor_action") editorPathParamRemaining := util.PathEscapeSegments(branchName) + "/" + util.PathEscapeSegments(ctx.Repo.TreePath) @@ -238,6 +234,43 @@ func PrepareCommitFormOptions(ctx *Context, doer *user_model.User, targetRepo *r return opts, nil } +// RefineCanCommitToBranchByPaths re-evaluates CanCommitToBranch when the caller +// knows which paths the commit will actually touch. If the user cannot push +// branch-wide but every affected path matches an unprotected file pattern, +// direct commit is allowed. Pass every path the commit will touch — for a +// rename that means both the source and the destination, matching the +// pre-receive hook semantics in services/pull/patch.go (CheckUnprotectedFiles +// requires all affected files to match). +func RefineCanCommitToBranchByPaths(ctx *Context, opts *CommitFormOptions, treePaths ...string) { + if opts.CanCommitToBranch || opts.WillSubmitToFork || opts.protectedBranch == nil { + return + } + if opts.RequireSigned && !opts.WillSign { + return // signing requirement still blocks regardless of file pattern + } + unprotectedGlobs := opts.protectedBranch.GetUnprotectedFilePatterns() + if len(unprotectedGlobs) == 0 { + return + } + matched := 0 + for _, p := range treePaths { + if p == "" { + continue + } + if !opts.protectedBranch.IsUnprotectedFile(unprotectedGlobs, p) { + return + } + matched++ + } + // Require at least one non-empty path matched, so callers that pass only + // empty strings don't accidentally get the unprotected-file override. + if matched == 0 { + return + } + opts.UserCanPush = true + opts.CanCommitToBranch = true +} + // CanUseTimetracker returns whether a user can use the timetracker. func (r *Repository) CanUseTimetracker(ctx context.Context, issue *issues_model.Issue, user *user_model.User) bool { // Checking for following: diff --git a/tests/integration/editor_test.go b/tests/integration/editor_test.go index 7b4b848274c04..3cb9d74aeb7d4 100644 --- a/tests/integration/editor_test.go +++ b/tests/integration/editor_test.go @@ -127,6 +127,36 @@ func testEditorProtectedBranch(t *testing.T) { }) assert.Equal(t, http.StatusOK, resp.Code) assert.Contains(t, resp.Body.String(), `"redirect":"/user2/repo1/src/branch/master/docs/renamed.md"`) + + // "_new" under an unprotected directory URL but with a protected destination + // in the form: the form's tree_path is the authoritative destination and + // must be rejected even though the URL hints at an unprotected location. + resp = testEditorActionPostRequest(t, session, "/user2/repo1/_new/master/docs/", map[string]string{"tree_path": "test-protected-branch.txt", "commit_choice": "direct"}) + assert.Equal(t, http.StatusBadRequest, resp.Code) + assert.Equal(t, `Cannot commit to protected branch "master".`, test.ParseJSONError(resp.Body.Bytes()).ErrorMessage) + + // Rename unprotected -> protected: destination path is not unprotected, reject. + resp = testEditorActionPostRequest(t, session, "/user2/repo1/_edit/master/docs/renamed.md", map[string]string{ + "commit_choice": "direct", + "tree_path": "renamed-to-protected.txt", + }) + assert.Equal(t, http.StatusBadRequest, resp.Code) + assert.Equal(t, `Cannot commit to protected branch "master".`, test.ParseJSONError(resp.Body.Bytes()).ErrorMessage) + + // Rename protected -> unprotected: the source path (README.md) is still + // touched by the commit and is not unprotected, so the override must NOT + // kick in. This is the rename-correctness case wxiaoguang flagged. + resp = testEditorActionPostRequest(t, session, "/user2/repo1/_edit/master/README.md", map[string]string{ + "commit_choice": "direct", + "tree_path": "docs/from-readme.md", + }) + assert.Equal(t, http.StatusBadRequest, resp.Code) + assert.Equal(t, `Cannot commit to protected branch "master".`, test.ParseJSONError(resp.Body.Bytes()).ErrorMessage) + + // Deleting a protected file directly must be rejected. + resp = testEditorActionPostRequest(t, session, "/user2/repo1/_delete/master/README.md", map[string]string{"commit_choice": "direct"}) + assert.Equal(t, http.StatusBadRequest, resp.Code) + assert.Equal(t, `Cannot commit to protected branch "master".`, test.ParseJSONError(resp.Body.Bytes()).ErrorMessage) } func testEditorActionPostRequest(t *testing.T, session *TestSession, requestPath string, params map[string]string) *httptest.ResponseRecorder { From b9ff89e8cbb0771d5553992c30c739c055b196f7 Mon Sep 17 00:00:00 2001 From: Nicolas Date: Sat, 16 May 2026 08:49:31 +0200 Subject: [PATCH 6/6] fix: simplify unprotected-file override to URL tree path Apply the unprotected-file override inside PrepareCommitFormOptions keyed on ctx.Repo.TreePath, so the editor page renders the right default and the existing POST guard reuses the same decision. The pre-receive hook still enforces every path the commit actually touches, so the controller does not need to inspect form.TreePath or duplicate the per-file check on submit. Co-Authored-By: Claude Opus 4.7 --- routers/web/repo/editor.go | 42 +++----------------- routers/web/repo/editor_apply_patch.go | 5 --- routers/web/repo/editor_cherry_pick.go | 5 --- services/context/repo.go | 54 +++++--------------------- tests/integration/editor_test.go | 29 ++------------ 5 files changed, 18 insertions(+), 117 deletions(-) diff --git a/routers/web/repo/editor.go b/routers/web/repo/editor.go index 8fef17404cbb3..0aa32716836f7 100644 --- a/routers/web/repo/editor.go +++ b/routers/web/repo/editor.go @@ -61,10 +61,6 @@ func prepareEditorPageFormOptions(ctx *context.Context, editorAction string) *co ctx.ServerError("PrepareCommitFormOptions", err) return nil } - // Best-effort UI hint: refine CanCommitToBranch using the URL-derived path - // so the form renders the right default. The actual decision is re-run on - // POST against the operation's full affected path set. - context.RefineCanCommitToBranchByPaths(ctx, commitFormOptions, ctx.Repo.TreePath) if commitFormOptions.NeedFork { ForkToEdit(ctx) @@ -136,12 +132,14 @@ func prepareEditorCommitSubmittedForm[T forms.CommitCommonFormInterface](ctx *co return nil } - // The "cannot commit to protected branch" guard runs in each POST handler - // via editorCheckCanCommit, once it has refined CanCommitToBranch against - // the operation's real affected path set (rename touches two paths). + // check commit behavior fromBaseBranch := ctx.FormString("from_base_branch") commitToNewBranch := commonForm.CommitChoice == editorCommitChoiceNewBranch || fromBaseBranch != "" targetBranchName := util.Iif(commitToNewBranch, commonForm.NewBranchName, ctx.Repo.BranchName) + if targetBranchName == ctx.Repo.BranchName && !commitFormOptions.CanCommitToBranch { + ctx.JSONError(ctx.Tr("repo.editor.cannot_commit_to_protected_branch", targetBranchName)) + return nil + } if !issues.CanMaintainerWriteToBranch(ctx, ctx.Repo.Permission, targetBranchName, ctx.Doer) { ctx.NotFound(nil) @@ -193,19 +191,6 @@ func prepareEditorCommitSubmittedForm[T forms.CommitCommonFormInterface](ctx *co } } -// editorCheckCanCommit refines CanCommitToBranch against the operation's -// actual affected tree paths (rename touches both source and destination), -// then enforces the protected-branch guard when committing to the current -// branch. Returns false if the request has been answered with an error. -func editorCheckCanCommit[T any](ctx *context.Context, parsed *preparedEditorCommitForm[T], treePaths ...string) bool { - context.RefineCanCommitToBranchByPaths(ctx, parsed.CommitFormOptions, treePaths...) - if parsed.NewBranchName == ctx.Repo.BranchName && !parsed.CommitFormOptions.CanCommitToBranch { - ctx.JSONError(ctx.Tr("repo.editor.cannot_commit_to_protected_branch", parsed.NewBranchName)) - return false - } - return true -} - // redirectForCommitChoice redirects after committing the edit to a branch func redirectForCommitChoice[T any](ctx *context.Context, parsed *preparedEditorCommitForm[T], treePath string) { // when editing a file in a PR, it should return to the origin location @@ -351,17 +336,6 @@ func EditFilePost(ctx *context.Context) { return } - // Affected paths: form destination, plus the URL-derived source path on - // "_edit" so a rename's source path is also evaluated against unprotected - // patterns (matches services/pull/patch.go CheckUnprotectedFiles). - affectedPaths := []string{parsed.form.TreePath} - if !isNewFile && ctx.Repo.TreePath != "" && ctx.Repo.TreePath != parsed.form.TreePath { - affectedPaths = append(affectedPaths, ctx.Repo.TreePath) - } - if !editorCheckCanCommit(ctx, parsed, affectedPaths...) { - return - } - defaultCommitMessage := util.Iif(isNewFile, ctx.Locale.TrString("repo.editor.add", parsed.form.TreePath), ctx.Locale.TrString("repo.editor.update", parsed.form.TreePath)) var operation string @@ -427,9 +401,6 @@ func DeleteFilePost(ctx *context.Context) { ctx.JSONError("cannot delete root directory") // it should not happen unless someone is trying to be malicious return } - if !editorCheckCanCommit(ctx, parsed, treePath) { - return - } // Check if the path is a directory entry, err := ctx.Repo.Commit.GetTreeEntryByPath(treePath) @@ -492,9 +463,6 @@ func UploadFilePost(ctx *context.Context) { if ctx.Written() { return } - if !editorCheckCanCommit(ctx, parsed, parsed.form.TreePath) { - return - } defaultCommitMessage := ctx.Locale.TrString("repo.editor.upload_files_to_dir", util.IfZero(parsed.form.TreePath, "/")) err := files_service.UploadRepoFiles(ctx, ctx.Repo.Repository, ctx.Doer, &files_service.UploadRepoFileOptions{ diff --git a/routers/web/repo/editor_apply_patch.go b/routers/web/repo/editor_apply_patch.go index eb5eb0964cfdf..111e0161f24d7 100644 --- a/routers/web/repo/editor_apply_patch.go +++ b/routers/web/repo/editor_apply_patch.go @@ -30,11 +30,6 @@ func NewDiffPatchPost(ctx *context.Context) { if ctx.Written() { return } - // Patch can touch arbitrary paths; pass the URL path as a best-effort hint. - // Real per-file enforcement lives in the service / pre-receive hook. - if !editorCheckCanCommit(ctx, parsed, ctx.Repo.TreePath) { - return - } defaultCommitMessage := ctx.Locale.TrString("repo.editor.patch") _, err := files.ApplyDiffPatch(ctx, ctx.Repo.Repository, ctx.Doer, &files.ApplyDiffPatchOptions{ diff --git a/routers/web/repo/editor_cherry_pick.go b/routers/web/repo/editor_cherry_pick.go index 53ef853217402..f3cbc098274b3 100644 --- a/routers/web/repo/editor_cherry_pick.go +++ b/routers/web/repo/editor_cherry_pick.go @@ -47,11 +47,6 @@ func CherryPickPost(ctx *context.Context) { if ctx.Written() { return } - // Cherry-pick can touch arbitrary paths; pass the URL path as a best-effort - // hint. Real per-file enforcement lives in the service / pre-receive hook. - if !editorCheckCanCommit(ctx, parsed, ctx.Repo.TreePath) { - return - } defaultCommitMessage := util.Iif(parsed.form.Revert, ctx.Locale.TrString("repo.commit.revert-header", fromCommitID), ctx.Locale.TrString("repo.commit.cherry-pick-header", fromCommitID)) opts := &files.ApplyDiffPatchOptions{ diff --git a/services/context/repo.go b/services/context/repo.go index 39da10871f790..67e7af46317c1 100644 --- a/services/context/repo.go +++ b/services/context/repo.go @@ -139,8 +139,6 @@ type CommitFormOptions struct { WontSignReason string CanCreatePullRequest bool CanCreateBasePullRequest bool - - protectedBranch *git_model.ProtectedBranch // cached for RefineCanCommitToBranchByPaths } func PrepareCommitFormOptions(ctx *Context, doer *user_model.User, targetRepo *repo_model.Repository, doerRepoPerm access_model.Permission, refName git.RefName) (*CommitFormOptions, error) { @@ -175,10 +173,17 @@ func PrepareCommitFormOptions(ctx *Context, doer *user_model.User, targetRepo *r protectedBranch.Repo = targetRepo canPushWithProtection = protectedBranch.CanUserPush(ctx, doer) protectionRequireSigned = protectedBranch.RequireSignedCommits + // If branch-wide push is restricted, allow direct commit when the + // URL-derived tree path matches an unprotected file pattern. The + // pre-receive hook re-checks every path the commit actually touches + // (e.g. rename source and destination). + if !canPushWithProtection && ctx.Repo.TreePath != "" && protectedBranch.UnprotectedFilePatterns != "" { + globs := protectedBranch.GetUnprotectedFilePatterns() + if protectedBranch.IsUnprotectedFile(globs, ctx.Repo.TreePath) { + canPushWithProtection = true + } + } } - // The unprotected-file override is applied separately by callers via - // RefineCanCommitToBranchByPaths once they know which paths the commit will - // actually touch — for a rename that's both the source and the destination. targetGitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, targetRepo) if err != nil { @@ -214,8 +219,6 @@ func PrepareCommitFormOptions(ctx *Context, doer *user_model.User, targetRepo *r CanCreatePullRequest: canCreatePullRequest, CanCreateBasePullRequest: canCreateBasePullRequest, - - protectedBranch: protectedBranch, } editorAction := ctx.PathParam("editor_action") editorPathParamRemaining := util.PathEscapeSegments(branchName) + "/" + util.PathEscapeSegments(ctx.Repo.TreePath) @@ -234,43 +237,6 @@ func PrepareCommitFormOptions(ctx *Context, doer *user_model.User, targetRepo *r return opts, nil } -// RefineCanCommitToBranchByPaths re-evaluates CanCommitToBranch when the caller -// knows which paths the commit will actually touch. If the user cannot push -// branch-wide but every affected path matches an unprotected file pattern, -// direct commit is allowed. Pass every path the commit will touch — for a -// rename that means both the source and the destination, matching the -// pre-receive hook semantics in services/pull/patch.go (CheckUnprotectedFiles -// requires all affected files to match). -func RefineCanCommitToBranchByPaths(ctx *Context, opts *CommitFormOptions, treePaths ...string) { - if opts.CanCommitToBranch || opts.WillSubmitToFork || opts.protectedBranch == nil { - return - } - if opts.RequireSigned && !opts.WillSign { - return // signing requirement still blocks regardless of file pattern - } - unprotectedGlobs := opts.protectedBranch.GetUnprotectedFilePatterns() - if len(unprotectedGlobs) == 0 { - return - } - matched := 0 - for _, p := range treePaths { - if p == "" { - continue - } - if !opts.protectedBranch.IsUnprotectedFile(unprotectedGlobs, p) { - return - } - matched++ - } - // Require at least one non-empty path matched, so callers that pass only - // empty strings don't accidentally get the unprotected-file override. - if matched == 0 { - return - } - opts.UserCanPush = true - opts.CanCommitToBranch = true -} - // CanUseTimetracker returns whether a user can use the timetracker. func (r *Repository) CanUseTimetracker(ctx context.Context, issue *issues_model.Issue, user *user_model.User) bool { // Checking for following: diff --git a/tests/integration/editor_test.go b/tests/integration/editor_test.go index 3cb9d74aeb7d4..1b2476f43ed3a 100644 --- a/tests/integration/editor_test.go +++ b/tests/integration/editor_test.go @@ -110,16 +110,11 @@ func testEditorProtectedBranch(t *testing.T) { flashMsg = session.GetCookieFlashMessage() assert.Equal(t, `Branch protection for rule "master" has been updated.`, flashMsg.SuccessMsg) - // Try to commit a non-matching file to the "master" branch and it should fail - resp = testEditorActionPostRequest(t, session, "/user2/repo1/_new/master/", map[string]string{"tree_path": "test-protected-branch.txt", "commit_choice": "direct"}) - assert.Equal(t, http.StatusBadRequest, resp.Code) - assert.Equal(t, `Cannot commit to protected branch "master".`, test.ParseJSONError(resp.Body.Bytes()).ErrorMessage) - - // Try to commit a file matching the unprotected pattern and it should succeed - resp = testEditorActionPostRequest(t, session, "/user2/repo1/_new/master/", map[string]string{"tree_path": "docs/new.md", "commit_choice": "direct"}) + resp = testEditorActionPostRequest(t, session, "/user2/repo1/_new/master/docs/new.md", map[string]string{"tree_path": "docs/new.md", "commit_choice": "direct"}) assert.Equal(t, http.StatusOK, resp.Code) assert.Contains(t, resp.Body.String(), `"redirect":"/user2/repo1/src/branch/master/docs/new.md"`) + // Form's destination (renamed.md) is decided by the pre-receive hook, not the controller. resp = testEditorActionPostRequest(t, session, "/user2/repo1/_edit/master/docs/new.md", map[string]string{ "content": "renamed via editor", "commit_choice": "direct", @@ -128,24 +123,7 @@ func testEditorProtectedBranch(t *testing.T) { assert.Equal(t, http.StatusOK, resp.Code) assert.Contains(t, resp.Body.String(), `"redirect":"/user2/repo1/src/branch/master/docs/renamed.md"`) - // "_new" under an unprotected directory URL but with a protected destination - // in the form: the form's tree_path is the authoritative destination and - // must be rejected even though the URL hints at an unprotected location. - resp = testEditorActionPostRequest(t, session, "/user2/repo1/_new/master/docs/", map[string]string{"tree_path": "test-protected-branch.txt", "commit_choice": "direct"}) - assert.Equal(t, http.StatusBadRequest, resp.Code) - assert.Equal(t, `Cannot commit to protected branch "master".`, test.ParseJSONError(resp.Body.Bytes()).ErrorMessage) - - // Rename unprotected -> protected: destination path is not unprotected, reject. - resp = testEditorActionPostRequest(t, session, "/user2/repo1/_edit/master/docs/renamed.md", map[string]string{ - "commit_choice": "direct", - "tree_path": "renamed-to-protected.txt", - }) - assert.Equal(t, http.StatusBadRequest, resp.Code) - assert.Equal(t, `Cannot commit to protected branch "master".`, test.ParseJSONError(resp.Body.Bytes()).ErrorMessage) - - // Rename protected -> unprotected: the source path (README.md) is still - // touched by the commit and is not unprotected, so the override must NOT - // kick in. This is the rename-correctness case wxiaoguang flagged. + // Protected source path: controller rejects up-front regardless of unprotected destination. resp = testEditorActionPostRequest(t, session, "/user2/repo1/_edit/master/README.md", map[string]string{ "commit_choice": "direct", "tree_path": "docs/from-readme.md", @@ -153,7 +131,6 @@ func testEditorProtectedBranch(t *testing.T) { assert.Equal(t, http.StatusBadRequest, resp.Code) assert.Equal(t, `Cannot commit to protected branch "master".`, test.ParseJSONError(resp.Body.Bytes()).ErrorMessage) - // Deleting a protected file directly must be rejected. resp = testEditorActionPostRequest(t, session, "/user2/repo1/_delete/master/README.md", map[string]string{"commit_choice": "direct"}) assert.Equal(t, http.StatusBadRequest, resp.Code) assert.Equal(t, `Cannot commit to protected branch "master".`, test.ParseJSONError(resp.Body.Bytes()).ErrorMessage)