From e98fca780e1039adf36ec32ffaebe59bd25b6106 Mon Sep 17 00:00:00 2001 From: Jonathan Miller Date: Sun, 5 Jul 2026 14:46:01 -0500 Subject: [PATCH] fix: address org-governance release review (#727, #733) + dev deploy targeting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code-review findings on the org-governance release: - Fail closed on org-rule lookup error: getFirstMatchProtectedBranchRule swallowed FindOrgBranchRuleForBranch errors (returned nil,nil), silently dropping the org floor and falling back to the repo rule on a transient DB error. Propagate the error so the org rule stays enforced. - Stop the org rule locking out deploy-key and Actions-bot pushes: OrgProtectedBranch is team-only, so mergeMostRestrictive was ANDing the repo's WhitelistDeployKeys / WhitelistActionsUser (and the force-push, delete and merge counterparts) against the org side's always-false zero value, blocking every deploy-key and Actions push in any org with a matching branch rule. Carry those org-unmanaged fields through from the repo rule unchanged. - Org push-policy max-file-size now inspects only the pushed delta (diff-tree + cat-file --batch-check) instead of the full tip tree via ls-tree, so a pre-existing oversized file can no longer permanently block unrelated pushes. New branches (no base commit) still scan the full tree. Dev deploy targeting: - deploy-dev.yml drove the dev container image via `sed` on the SHARED compose file, but the pattern matched the *prod* service line (container_name: mokogitea) — leaving the dev service pinned to a stale image (so every "green" deploy recreated old code) while corrupting the prod image pin. Drive the dev service image from ${MOKOGITEA_DEV_TAG} instead; the env-var only affects the dev service. Claude-Session: https://claude.ai/code/session_01Wsno14cxE49MstXFs9G5KT --- .mokogitea/workflows/custom/deploy-dev.yml | 17 ++-- models/git/protected_branch_list.go | 4 +- models/git/protected_branch_merge.go | 19 +++-- routers/private/hook_pre_receive.go | 92 +++++++++++++++++----- 4 files changed, 98 insertions(+), 34 deletions(-) diff --git a/.mokogitea/workflows/custom/deploy-dev.yml b/.mokogitea/workflows/custom/deploy-dev.yml index 609227ee44..6e4361c608 100644 --- a/.mokogitea/workflows/custom/deploy-dev.yml +++ b/.mokogitea/workflows/custom/deploy-dev.yml @@ -96,15 +96,16 @@ jobs: echo 'Restarting dev container...' cd /opt/gitea-dev - sed -i "s|${{ env.IMAGE }}:[^ ]*|${{ env.IMAGE }}:$TAG|" docker-compose.yml - # The dev service uses a fixed container_name (mokogitea-dev). If a - # container with that name lingers under a different/none compose - # project (the symlinked /opt/gitea-dev path makes the derived project - # name unstable), `compose up` fails with a name conflict instead of - # recreating. Remove any such container first so the name is free, pin - # the project name for determinism, then force a fresh recreate. + # The dev service in the SHARED compose file reads its image tag from + # ${MOKOGITEA_DEV_TAG}. Drive it from the freshly built tag instead of + # rewriting the file with sed: the old sed pattern matched the *prod* + # service line (container_name: mokogitea) and left the dev service pinned + # to a stale image, so every dev deploy recreated old code while silently + # corrupting the prod image pin. The env-var only affects the dev service. + # Remove any lingering fixed-name container first so the recreate can't hit + # a name conflict, pin the project name for determinism, then force-recreate. docker rm -f mokogitea-dev 2>/dev/null || true - docker compose -p gitea-dev up -d --force-recreate mokogitea-dev + MOKOGITEA_DEV_TAG="$TAG" docker compose -p gitea-dev up -d --force-recreate mokogitea-dev echo 'Health check...' for i in 1 2 3 4 5 6 7 8; do diff --git a/models/git/protected_branch_list.go b/models/git/protected_branch_list.go index fa3acf37d7..8528177335 100644 --- a/models/git/protected_branch_list.go +++ b/models/git/protected_branch_list.go @@ -133,8 +133,10 @@ func getFirstMatchOrgProtectedBranchRule(ctx context.Context, repoID int64, bran orgRule, err := FindOrgBranchRuleForBranch(ctx, owner.ID, branchName) if err != nil { + // Fail closed: propagate the error so callers keep the org floor in force + // rather than silently falling back to the repo rule on a transient error. log.Error("FindOrgBranchRuleForBranch: %v", err) - return nil, nil + return nil, err } if orgRule == nil { return nil, nil diff --git a/models/git/protected_branch_merge.go b/models/git/protected_branch_merge.go index d7a0465799..5f4f6adb0a 100644 --- a/models/git/protected_branch_merge.go +++ b/models/git/protected_branch_merge.go @@ -26,27 +26,32 @@ func mergeMostRestrictive(repoRule, orgRule *ProtectedBranch) *ProtectedBranch { eff.CanPush = repoRule.CanPush && orgRule.CanPush eff.EnableWhitelist, eff.WhitelistUserIDs = mergeAllowlist(repoRule.EnableWhitelist, repoRule.WhitelistUserIDs, orgRule.EnableWhitelist, orgRule.WhitelistUserIDs) _, eff.WhitelistTeamIDs = mergeAllowlist(repoRule.EnableWhitelist, repoRule.WhitelistTeamIDs, orgRule.EnableWhitelist, orgRule.WhitelistTeamIDs) - eff.WhitelistDeployKeys = repoRule.WhitelistDeployKeys && orgRule.WhitelistDeployKeys - eff.WhitelistActionsUser = repoRule.WhitelistActionsUser && orgRule.WhitelistActionsUser + // Deploy-key and Actions-user allowances are not expressible in an + // OrgProtectedBranch (it is team-only), so the org rule imposes no constraint on + // them; carry the repo values through unchanged. ANDing against the org side's + // always-false zero value would silently lock out every deploy-key and + // Actions-bot push in any org that has a matching branch rule (see #727 review). + eff.WhitelistDeployKeys = repoRule.WhitelistDeployKeys + eff.WhitelistActionsUser = repoRule.WhitelistActionsUser // Force push. eff.CanForcePush = repoRule.CanForcePush && orgRule.CanForcePush eff.EnableForcePushAllowlist, eff.ForcePushAllowlistUserIDs = mergeAllowlist(repoRule.EnableForcePushAllowlist, repoRule.ForcePushAllowlistUserIDs, orgRule.EnableForcePushAllowlist, orgRule.ForcePushAllowlistUserIDs) _, eff.ForcePushAllowlistTeamIDs = mergeAllowlist(repoRule.EnableForcePushAllowlist, repoRule.ForcePushAllowlistTeamIDs, orgRule.EnableForcePushAllowlist, orgRule.ForcePushAllowlistTeamIDs) - eff.ForcePushAllowlistDeployKeys = repoRule.ForcePushAllowlistDeployKeys && orgRule.ForcePushAllowlistDeployKeys - eff.ForcePushAllowlistActionsUser = repoRule.ForcePushAllowlistActionsUser && orgRule.ForcePushAllowlistActionsUser + eff.ForcePushAllowlistDeployKeys = repoRule.ForcePushAllowlistDeployKeys + eff.ForcePushAllowlistActionsUser = repoRule.ForcePushAllowlistActionsUser // Delete. eff.CanDelete = repoRule.CanDelete && orgRule.CanDelete eff.EnableDeleteAllowlist, eff.DeleteAllowlistUserIDs = mergeAllowlist(repoRule.EnableDeleteAllowlist, repoRule.DeleteAllowlistUserIDs, orgRule.EnableDeleteAllowlist, orgRule.DeleteAllowlistUserIDs) _, eff.DeleteAllowlistTeamIDs = mergeAllowlist(repoRule.EnableDeleteAllowlist, repoRule.DeleteAllowlistTeamIDs, orgRule.EnableDeleteAllowlist, orgRule.DeleteAllowlistTeamIDs) - eff.DeleteAllowlistDeployKeys = repoRule.DeleteAllowlistDeployKeys && orgRule.DeleteAllowlistDeployKeys - eff.DeleteAllowlistActionsUser = repoRule.DeleteAllowlistActionsUser && orgRule.DeleteAllowlistActionsUser + eff.DeleteAllowlistDeployKeys = repoRule.DeleteAllowlistDeployKeys + eff.DeleteAllowlistActionsUser = repoRule.DeleteAllowlistActionsUser // Merge whitelist. eff.EnableMergeWhitelist, eff.MergeWhitelistUserIDs = mergeAllowlist(repoRule.EnableMergeWhitelist, repoRule.MergeWhitelistUserIDs, orgRule.EnableMergeWhitelist, orgRule.MergeWhitelistUserIDs) _, eff.MergeWhitelistTeamIDs = mergeAllowlist(repoRule.EnableMergeWhitelist, repoRule.MergeWhitelistTeamIDs, orgRule.EnableMergeWhitelist, orgRule.MergeWhitelistTeamIDs) - eff.MergeWhitelistActionsUser = repoRule.MergeWhitelistActionsUser && orgRule.MergeWhitelistActionsUser + eff.MergeWhitelistActionsUser = repoRule.MergeWhitelistActionsUser // Status checks. eff.EnableStatusCheck = repoRule.EnableStatusCheck || orgRule.EnableStatusCheck diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index dc87554575..efbbbfcbe8 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -646,7 +646,7 @@ func (ctx *preReceiveContext) checkOrgPushPolicyBranch(oldCommitID, newCommitID, } if policy.MaxFileSize > 0 { - if path, size := ctx.largestBlobOverLimit(newCommitID, policy.MaxFileSize); path != "" { + if path, size := ctx.largestBlobOverLimit(oldCommitID, newCommitID, policy.MaxFileSize); path != "" { ctx.JSON(http.StatusForbidden, private.Response{ UserMsg: fmt.Sprintf("Push rejected by the organization push policy: %q is %d bytes, over the %d-byte limit", path, size, policy.MaxFileSize), }) @@ -673,34 +673,90 @@ func (ctx *preReceiveContext) checkOrgPushPolicyTag(tagName string) bool { return true } -// largestBlobOverLimit returns the first file (and its size) in the pushed tip tree -// that exceeds limit bytes, or ("", 0) if none — or on any error (fail open). -func (ctx *preReceiveContext) largestBlobOverLimit(commitID string, limit int64) (string, int64) { - output, _, err := gitrepo.RunCmdString(ctx, - ctx.Repo.Repository, - gitcmd.NewCommand("ls-tree", "-r", "--long"). - AddDynamicArguments(commitID). - WithEnv(ctx.env), - ) - if err != nil { - log.Error("org push policy ls-tree for %-v: %v", ctx.Repo.Repository, err) +// largestBlobOverLimit returns the first file (and its size) added or modified by the +// push (oldCommitID..newCommitID) whose blob exceeds limit bytes, or ("", 0) if none — +// or on any error (fail open). Only the pushed delta is inspected so a pre-existing +// oversized file committed before the policy existed cannot permanently block unrelated +// pushes. For a new branch (no old commit) the full new tree is scanned, since every +// blob is effectively introduced by the push. +func (ctx *preReceiveContext) largestBlobOverLimit(oldCommitID, newCommitID string, limit int64) (string, int64) { + emptyID := ctx.Repo.GetObjectFormat().EmptyObjectID().String() + + // New branch: no base commit to diff against, so scan the full tip tree. ls-tree + // --long carries the blob size inline, so no extra sizing pass is needed. + if oldCommitID == "" || oldCommitID == emptyID { + output, _, err := gitrepo.RunCmdString(ctx, ctx.Repo.Repository, + gitcmd.NewCommand("ls-tree", "-r", "--long").AddDynamicArguments(newCommitID).WithEnv(ctx.env)) + if err != nil { + log.Error("org push policy ls-tree for %-v: %v", ctx.Repo.Repository, err) + return "", 0 + } + for _, line := range strings.Split(output, "\n") { + tab := strings.IndexByte(line, '\t') + if tab < 0 { + continue + } + fields := strings.Fields(line[:tab]) // mode, type, hash, size + if len(fields) < 4 || fields[1] != "blob" { + continue + } + if size, perr := strconv.ParseInt(fields[3], 10, 64); perr == nil && size > limit { + return line[tab+1:], size + } + } return "", 0 } + + // Existing branch: inspect only blobs added or modified by this push. diff-tree + // gives the changed paths and their new object ids but not sizes, so collect the + // candidate blob ids and size them in a single cat-file --batch-check pass. + output, _, err := gitrepo.RunCmdString(ctx, ctx.Repo.Repository, + gitcmd.NewCommand("diff-tree", "--no-commit-id", "-r").AddDynamicArguments(oldCommitID, newCommitID).WithEnv(ctx.env)) + if err != nil { + log.Error("org push policy diff-tree for %-v: %v", ctx.Repo.Repository, err) + return "", 0 + } + shaToPath := make(map[string]string) + var order []string for _, line := range strings.Split(output, "\n") { tab := strings.IndexByte(line, '\t') if tab < 0 { continue } - fields := strings.Fields(line[:tab]) // mode, type, hash, size - if len(fields) < 4 || fields[1] != "blob" { + fields := strings.Fields(line[:tab]) // ":oldmode newmode oldsha newsha status" + if len(fields) < 5 { continue } - size, perr := strconv.ParseInt(fields[3], 10, 64) - if perr != nil { + newMode, newSha, status := fields[1], fields[3], fields[4] + // Only regular, executable, or symlink blobs; skip submodule gitlinks (160000). + if newMode != "100644" && newMode != "100755" && newMode != "120000" { continue } - if size > limit { - return line[tab+1:], size + if status == "D" || newSha == emptyID { + continue + } + if _, seen := shaToPath[newSha]; !seen { + order = append(order, newSha) + } + shaToPath[newSha] = line[tab+1:] + } + if len(order) == 0 { + return "", 0 + } + + output, _, err = gitrepo.RunCmdString(ctx, ctx.Repo.Repository, + gitcmd.NewCommand("cat-file", "--batch-check").WithStdinBytes([]byte(strings.Join(order, "\n")+"\n")).WithEnv(ctx.env)) + if err != nil { + log.Error("org push policy cat-file for %-v: %v", ctx.Repo.Repository, err) + return "", 0 + } + for _, line := range strings.Split(output, "\n") { + fields := strings.Fields(line) // " " or " missing" + if len(fields) != 3 || fields[1] != "blob" { + continue + } + if size, perr := strconv.ParseInt(fields[2], 10, 64); perr == nil && size > limit { + return shaToPath[fields[0]], size } } return "", 0 -- 2.52.0