From 1c862c53412d66c6014c32ce4f9cfeef617e0f34 Mon Sep 17 00:00:00 2001 From: AlexeyShalaev <75322386+AlexeyShalaev@users.noreply.github.com> Date: Thu, 8 Oct 2026 12:48:21 +0300 Subject: [PATCH 1/2] feat(distribute): predict protected branches before any write on GitLab and Gitea A new optional platform.PushGuard tells which branches a protection rule keeps the write identity from pushing to at all. GitLab's writer reads the project's protected branches (one request; the project and its access level only when a rule matches the sync branch); Gitea's and Forgejo's read GET branches/{b} of each sync branch that exists (protected, user_can_push). When a decision pushes, distribute and distribute --dry-run ask it and block the target rules:protected-branch before any write, with a warning naming the rule; memory upkeep stays. Rules the writer cannot read still show at the push, refused before anything is written. plan, whose reader is no PushGuard, is unchanged. GitLab applies the most permissive of the protected branches that match a branch, to pushes and force pushes alike (docs.gitlab.com, protected branches and protection rules). doctor's rules check took the strictest instead and could fail a branch another rule lets the writer push to, or warn about force pushes another rule allows; both checks now use one verdict per branch. The fake platform models it: a Ruleset with NoPush refuses every push with GitLab's message, and WithPushGuard makes the writer report those branches. Tests: the drivers' NoPush and GitLab's verdict over several rules, a conformance case, TestRunProtectedBranch (dry run and distribute blocked, an open pull request kept, the push-time refusal without the guard or with hidden rules), and the live TestProtectedBranch on GitLab (a dry run, and two rules where the permissive one wins) and on Gitea and Forgejo. --- docs/concepts/overview.md | 4 +- docs/guide/troubleshooting.md | 5 + docs/project/e2e.md | 3 +- internal/decide/target.go | 4 + internal/distribute/inspect.go | 55 +++++++ internal/distribute/run.go | 6 +- internal/distribute/run_protected_test.go | 92 +++++++++++ internal/e2e/gitlab/protected_test.go | 55 +++++-- internal/e2e/protected_test.go | 79 +++++++++ internal/platform/conformance/commit.go | 17 ++ internal/platform/conformance/conformance.go | 1 + internal/platform/fake/fake.go | 1 + internal/platform/fake/gitrepo.go | 13 ++ internal/platform/fake/gitserve.go | 13 +- internal/platform/fake/reader.go | 1 + internal/platform/fake/rules.go | 81 +++++++++- internal/platform/fake/writer.go | 12 +- internal/platform/gitea/check.go | 36 +++++ internal/platform/gitea/pushguard_test.go | 58 +++++++ internal/platform/gitlab/check.go | 160 +++++++++++++++---- internal/platform/gitlab/pushguard_test.go | 119 ++++++++++++++ internal/platform/platform.go | 25 ++- 22 files changed, 782 insertions(+), 58 deletions(-) create mode 100644 internal/distribute/run_protected_test.go create mode 100644 internal/e2e/protected_test.go create mode 100644 internal/platform/gitea/pushguard_test.go create mode 100644 internal/platform/gitlab/pushguard_test.go diff --git a/docs/concepts/overview.md b/docs/concepts/overview.md index d32727f..9d68794 100644 --- a/docs/concepts/overview.md +++ b/docs/concepts/overview.md @@ -35,7 +35,9 @@ your-org/engineering-assets `plan` and `distribute` run the same pipeline; `plan` stops before writing, with the read account. `distribute --dry-run` runs it with the write account and also stops -before writing, and its report is the same as `plan`'s. +before writing. Its report is `plan`'s, except where the write account knows more: its +permissions, and on GitLab, Gitea and Forgejo the protected branches it may not push to, +which block a target before any write (`rules:protected-branch`). | Phase | What happens | |---|---| diff --git a/docs/guide/troubleshooting.md b/docs/guide/troubleshooting.md index 03a5c62..c6f1be9 100644 --- a/docs/guide/troubleshooting.md +++ b/docs/guide/troubleshooting.md @@ -47,6 +47,11 @@ A run whose hub commit is no longer the tip of the default branch ends `supersed `plan` runs with the reader, which cannot see what the writer may bypass: `plan` can predict a rule block that `distribute` will not hit. `doctor` checks with the writer. +`distribute --dry-run` and `distribute` read, with the writer, the protected branches +that keep it from pushing to the sync branch (GitLab's protected branches; on Gitea and +Forgejo, the protection of a sync branch that exists already) and block the target +before any write; other rules show at the first push, which they refuse before anything +is written. ## A target is deferred or failed diff --git a/docs/project/e2e.md b/docs/project/e2e.md index 0d0ebba..c4c1612 100644 --- a/docs/project/e2e.md +++ b/docs/project/e2e.md @@ -86,6 +86,7 @@ The script also sets `TOUCHMARK_E2E_REQUIRED=1`: the tests then fail instead of - Each step also checks which writes each target got, by the report's journal. - Before it checks that a second run writes nothing, the test waits for the server's own work after a push. The server writes the push event into the pull request's timeline from a queue, seconds after the push, and that changes the pull request's `updated_at`. For a person's pull request the comparison leaves `updated_at` out: touchmark never writes to one (its state, head, text, labels and comments tell), and the person's own push may land in its timeline late. - **`TestDoctor`**: `doctor` through `cli.Main` with the writer's token, over a fresh organisation: a writable opted-in repository (access ok; rules unknown while no sync branch exists, since the writer reads the protection of existing branches only), a read-only one (access fails), one not opted in (skipped), one whose sync branch a rule `touchmark/*` without pushes protects (rules fail), and one whose pull requests on the sync branch carry another hub's marker with the same id and a person's copy of this hub's (markers warn). The hub is a repository the writer reads (`hub-hidden` warns), then writes (fails). The token's scopes come from `GET /api/v1/token` where the forge has it; `--hub-token` with the admin's token reads the hub's Actions secrets, and a write key there fails. No token reaches any output. +- **`TestProtectedBranch`**: branch protection `touchmark/*` without pushes, which the writer reads for existing branches only. On a target whose pull request is open, a pack change is blocked `blocked:rules:protected-branch` before any write, in a dry run and in `distribute`, with a warning naming the rule, the pull request as it was. On a target protected before its sync branch exists, the first push meets the protection and is refused with the same reason, nothing written. - **`TestFacts`**: forge behavior touchmark relies on that the documentation leaves open, checked through the API and git alone, without the driver: - flavor detection; - the head filter of the pull request list (and whether it matches forks), `poster`, `pulls/{base}/{head}`; @@ -188,7 +189,7 @@ Secrets never appear on a command line: curl reads its `PRIVATE-TOKEN` header fr That group access token is revoked before `migrate` runs: the bot of a revoked token must still be found and its merge request adopted. - **`TestDoctor`**: `doctor` through `cli.Main` with the writer's token over a fresh subgroup: the token's expiry and scopes (`/personal_access_tokens/self`), a bot or service account (2fa ok); a Developer's access; protected branches over the sync branch, for Maintainers (rules fail) and for Developers without force pushes (warn); another hub's marker with the same id and a person's copy of this hub's (markers warn); a hub the writer does not see (`hub-hidden` ok), then one in its subgroup (fail). `--hub-token` with root's token reads the hub's variables and protected tags: a protected write key scoped to `touchmark-distribute` is ok, an unprotected one fails, a protected tag Developers may create fails. - **`TestSetup`**: `touchmark setup gitlab` through `cli.Main` as a maintainer runs it, with the person's token: an Owner of the seeded group, whose fresh subgroup holds the targets (one project opted in), and an Owner of a separate top-level group where the hub lives (GitLab shows a group's variables, which the final check reads, to its Owners only; a Maintainer gets that check as `unknown` and exit code 3). The hub starts as GitLab makes projects (the default branch protected for Maintainers) and keeps a stale write key, a personal access token of the person that root mints, in an unprotected variable of every environment. The first run must end with no failed, manual or unknown step, and root then reads: the reader and the writer (service accounts where the instance lets the person create them, else group access tokens; the `FINDING` line says which) with Reporter and Developer in the group and no access to the hub; `TOUCHMARK_READ_TOKEN` masked and not protected with scope `*`; `TOUCHMARK_WRITE_TOKEN` protected, masked and hidden with scope `touchmark-distribute`, and the stale copy gone, its token revoked (`GET /personal_access_tokens/self` with it answers 401); the environment; `main` with push No one, merge Maintainers and no force push; no pipeline variables; `protect_merge_request_pipelines` false; the two schedules. A second run and a dry run have only `ok` steps and rotate no token (the active tokens are the same before and after). `doctor --hub-token` with the person's token grades the hub with no `fail` or `warn`. With the runner, a merge request (merged by the person: push to `main` is No one now) names the writer in `hub.yml` and adds a pipeline whose jobs on `main` must succeed: one without an environment runs `probe` (the write key is not visible there) and `plan` (the reader's token reads the group), one with `environment: {name: touchmark-distribute, action: prepare}` runs `doctor` with the writer's token. No token appears in any output or job log. -- **`TestProtectedBranch`**: a wildcard protected branch. One target protects `touchmark/*` for Maintainers alone: the first push is refused and the target is `blocked:rules:protected-branch` with no write. Another lets Developers push but not force-push: the first run opens a merge request, and after a pack change the rebuilt branch needs a force push, which is refused, so the target is `blocked:rules:protected-branch` and its merge request stays as it was. +- **`TestProtectedBranch`**: wildcard protected branches, which the writer, a Developer, reads before it pushes. One target protects `touchmark/*` for Maintainers alone: a dry run and the first run block it `blocked:rules:protected-branch` before any write, with a warning naming the rule. Another lets Developers push but not force-push: the first run opens a merge request, and after a pack change the rebuilt branch needs a force push, which is refused, so the target is `blocked:rules:protected-branch` and its merge request stays as it was. A third has two rules on the sync branch, `touchmark/*` for Maintainers alone and the branch's own name for Developers with force pushes: GitLab applies the most permissive rule that matches a branch, so it is delivered and updated as if unprotected. - **`TestFacts`**: GitLab behavior touchmark relies on that the documentation leaves open, through the API and git alone: 409 on a duplicate merge request; draft title prefixes and a `Draft:` commit; labels created by name and `add_labels`; quick actions in a person's description on create and update, against touchmark-shaped text; `closed_by` for a person and for a bot, and `merged_by`; a deleted source branch closes the merge request, who GitLab names as the closer (`closed_by` and `resource_state_events`), and what a reopen without the branch does; a force push keeps it open; how soon after a git push the merge request API takes the new branch (`push-then-create`, see below); `repository_object_format` and sha256 projects; git over HTTP with Basic `oauth2:` and other user names; git with a group access token that has the scope `api` alone; descriptions of 200 000 bytes and around 1 MiB; the reader's `members/all` of the writer; rate-limit headers of a default instance; the bot of a revoked group access token, watched for 90 s (the user, and the author of its merge request); which users API view carries the `bot` field; what a fork of a project the reader cannot read shows; `GET /personal_access_tokens/self`; `push_rule` on CE; the pagination headers. `push-then-create` is why a merge request is opened again after a pause: GitLab checks a new merge request's source branch against the project's cached branch names, which the push's background job refreshes. On CE 19.4.1 under load a `POST /merge_requests` right after a push got `400 {"source_branch":["does not exist"]}` in 8 of 9 tries over three runs, and the merge request opened 1 to 10 s later; `HEAD /repository/branches/:branch` (the cached view) lagged `GET` of the same (Gitaly) by as much. When the branches API finds the branch, the driver's `CreatePR` returns `platform.ErrNotYet` (transient, with a `RetryAfter`), and the core sends the POST again after growing pauses, for at most 30 s. diff --git a/internal/decide/target.go b/internal/decide/target.go index 7332be0..4dd5042 100644 --- a/internal/decide/target.go +++ b/internal/decide/target.go @@ -34,6 +34,10 @@ const ( // because it would force the branch (GitHub's non_fast_forward): a // report reason "rules:". ReasonRulesNoForce = "rules:non-fast-forward" + // ReasonRulesProtected is the reason of a push to a branch a + // protection rule keeps the writer from pushing to at all (GitLab's + // protected branches, Gitea's branch protection, GitHub's). + ReasonRulesProtected = "rules:protected-branch" ) // CommentAutoDeclined is the Reason of the StepComment that follows the diff --git a/internal/distribute/inspect.go b/internal/distribute/inspect.go index 846a7b4..099afb5 100644 --- a/internal/distribute/inspect.go +++ b/internal/distribute/inspect.go @@ -594,6 +594,14 @@ func (r *run) inspectFull(ctx context.Context, t *target, optIn *config.OptIn, s if hasKept && (len(kept.Marker.Key) > 0 || touches(w.Decision, kept.PR.Number)) { w.Open = &kept } + if pushes(w.Decision) { + protected, err := r.readNoPush(ctx, w) + if err != nil { + r.fail(ctx, t, "read the protection of the target's branches", err, "internal") + return nil + } + r.blockProtected(w, protected) + } r.checkSigning(w) w.NeedPerms = needPerms(w.Decision.Steps) r.fillBody(w, sel, reads) @@ -769,6 +777,53 @@ func (r *run) readRules(ctx context.Context, w *Work) (platform.Rules, error) { return rules, nil } +// readNoPush reads which of the branches w's decision pushes to a +// protection rule keeps the writer from pushing to, when the provider's +// identity is a platform.PushGuard (GitLab's and Gitea's writers; a plan's +// reader never is one). Without one it returns none: such a push meets the +// rule at run time, refused before it writes. A transient failure is tried +// three times. +func (r *run) readNoPush(ctx context.Context, w *Work) ([]platform.Protected, error) { + g, ok := w.t.prov.reader.(platform.PushGuard) + if !ok { + return nil, nil + } + var branches []string + for _, s := range w.Decision.Steps { + if s.Kind == decide.StepPush || s.Kind == decide.StepRecreateBranch { + branches = uniqueNonEmpty(append(branches, s.Branch)) + } + } + var out []platform.Protected + err := r.retry(ctx, w.t.prov, func() error { + var err error + out, err = g.NoPush(ctx, w.t.repo, branches) + return err + }) + return out, err +} + +// blockProtected blocks w's decision as rules:protected-branch when a +// protection rule keeps the writer from a branch it pushes to, with a +// warning that names the rule, before any write: what a push would end in. +// Memory upkeep, which pushes nothing, stays. +func (r *run) blockProtected(w *Work, protected []platform.Protected) { + if len(protected) == 0 { + return + } + d := w.Decision + var upkeep []decide.Step + for _, s := range d.Steps { + if memoryUpkeep(s) { + upkeep = append(upkeep, s) + } + } + w.Decision = decide.TargetDecision{Outcome: decide.OutcomeBlocked, Reason: decide.ReasonRulesProtected, PR: d.PR, Branch: d.Branch, Steps: upkeep} + for _, p := range protected { + w.t.res.Warnings = append(w.t.res.Warnings, fmt.Sprintf("branch %s is protected (%s), and the writer may not push to it", p.Branch, p.Rule)) + } +} + // checkSigning decides how a push of w is signed. A signature is needed with // providers[].sign always, and under sign: auto when a rule of the default // or the sync branch requires signed commits (Rules.SignedCommits from the diff --git a/internal/distribute/run.go b/internal/distribute/run.go index f1e7a2f..5610015 100644 --- a/internal/distribute/run.go +++ b/internal/distribute/run.go @@ -165,7 +165,11 @@ type WriteDeps struct { // force pushes are decide's NoForcePush (blocked:rules:non-fast-forward // under our open pull request, a branch created afresh without one). // Rules the identity cannot read (Rules.Known false) change nothing. -// Then the signature (checkSigning): providers[].sign "always", or a +// A decision that still pushes asks the writer, when it is a +// platform.PushGuard (GitLab, Gitea and Forgejo), which branches it +// pushes to a protection rule keeps it from: one is blocked +// rules:protected-branch before any write (blockProtected), memory +// upkeep aside. Then the signature (checkSigning): providers[].sign "always", or a // rule requiring signed commits, needs one; the signing key makes it, // else the platform's API commit (Caps.Commit.API: phase F commits // through the stage ref), else, outside ModePlan, which never holds diff --git a/internal/distribute/run_protected_test.go b/internal/distribute/run_protected_test.go new file mode 100644 index 0000000..16c07d1 --- /dev/null +++ b/internal/distribute/run_protected_test.go @@ -0,0 +1,92 @@ +package distribute + +import ( + "slices" + "testing" + + "github.com/bedrock-python/touchmark/internal/platform/fake" + "github.com/bedrock-python/touchmark/internal/report" +) + +// protect makes branch patterns of r protected branches the writer may not +// push to. +func (g *gitWorld) protect(r string, patterns ...string) { + g.t.Helper() + g.p.AddRuleset(r, fake.Ruleset{Branches: patterns, NoPush: true}) + g.ok() +} + +// TestRunProtectedBranch: a protected branch the writer may not push to +// (GitLab's, Gitea's), which the writer as a platform.PushGuard reports, +// blocks the target rules:protected-branch before any write, in a dry run +// and in distribute, with a warning naming the rule; a plan, whose reader +// is no PushGuard, plans the pull request. An open pull request stays as +// it is. Without the guard, or with the rules hidden, the push meets the +// protection and is refused with the same reason, nothing written. +func TestRunProtectedBranch(t *testing.T) { + t.Parallel() + t.Run("guard", func(t *testing.T) { + t.Parallel() + w := newGitWorld(t, fake.WithPushGuard()) + x := w.optedIn("acme/x", nil) + w.protect(x.ID, "touchmark/*") + w.optedIn("acme/y", nil) + + plan := w.run(w.deps(ModePlan), ModePlan) + want(t, plan, "gh:acme/x", report.OutcomeOpened, "", 0) + for _, mode := range []Mode{ModeDryRun, ModeDistribute} { + rep := w.run(w.deps(mode), mode) + tg := want(t, rep, "gh:acme/x", report.OutcomeBlocked, "rules:protected-branch", 0) + if !hasWarningLike(tg, "branch "+branch+" is protected (touchmark/*)") { + t.Errorf("%v: warnings %q", mode, tg.Warnings) + } + if mode == ModeDistribute { + want(t, rep, "gh:acme/y", report.OutcomeOpened, "", 1) + } + } + if writes := w.writesOn("acme/x"); len(writes) > 0 || w.p.Branch(x.ID, branch) != "" { + t.Errorf("wrote to acme/x: %q, branch at %q", writes, w.p.Branch(x.ID, branch)) + } + }) + + t.Run("open pull request", func(t *testing.T) { + t.Parallel() + w := newGitWorld(t, fake.WithPushGuard()) + x := w.optedIn("acme/x", nil) + want(t, w.distribute(nil), "gh:acme/x", report.OutcomeOpened, "", 1) + head := w.p.Branch(x.ID, branch) + w.protect(x.ID, branch) + w.pack(slices.Concat(baseFiles, []string{"docs/new.md", version("docs/new.md", 1)})...) + + dry := w.run(w.deps(ModeDryRun), ModeDryRun) + want(t, dry, "gh:acme/x", report.OutcomeBlocked, "rules:protected-branch", 1) + rep := w.run(w.deps(ModeDistribute), ModeDistribute) + tg := want(t, rep, "gh:acme/x", report.OutcomeBlocked, "rules:protected-branch", 1) + if !hasWarningLike(tg, "branch "+branch+" is protected ("+branch+")") { + t.Errorf("warnings %q", tg.Warnings) + } + if writes := w.writesOn("acme/x"); len(writes) > 0 || w.p.Branch(x.ID, branch) != head { + t.Errorf("wrote to acme/x: %q, branch moved from %s to %s", writes, head, w.p.Branch(x.ID, branch)) + } + }) + + for name, opts := range map[string][]fake.Option{"no guard": nil, "hidden": {fake.WithPushGuard()}} { + t.Run(name, func(t *testing.T) { + t.Parallel() + w := newGitWorld(t, opts...) + x := w.optedIn("acme/x", nil) + w.protect(x.ID, "touchmark/*") + w.p.HideRules(x.ID, true) + w.ok() + want(t, w.both(nil), "gh:acme/x", report.OutcomeOpened, "", 0) + rep := w.run(w.deps(ModeDistribute), ModeDistribute) + tg := want(t, rep, "gh:acme/x", report.OutcomeBlocked, "rules:protected-branch", 0) + if tg.Writes != 0 || w.p.Branch(x.ID, branch) != "" { + t.Errorf("%d writes, branch at %q", tg.Writes, w.p.Branch(x.ID, branch)) + } + if !hasWarningLike(tg, "not allowed to push code to protected branches") { + t.Errorf("warnings %q", tg.Warnings) + } + }) + } +} diff --git a/internal/e2e/gitlab/protected_test.go b/internal/e2e/gitlab/protected_test.go index 9b28d4d..5428053 100644 --- a/internal/e2e/gitlab/protected_test.go +++ b/internal/e2e/gitlab/protected_test.go @@ -7,32 +7,59 @@ import ( "net/http" "strings" "testing" + + "github.com/bedrock-python/touchmark/internal/report" ) -// TestProtectedBranch: a target protects touchmark/* with a wildcard rule, -// which a writer with Developer cannot read beforehand (the protected -// branches API wants Maintainer), so the rule shows at push. +// TestProtectedBranch: targets protect touchmark/* with a wildcard rule, +// which the writer, a Developer, reads before it pushes (GET +// /projects/:id/protected_branches answers Developers), so a dry run +// predicts what a push meets. // -// - alpha lets Maintainers alone push to touchmark/*: the first push of -// the sync branch is refused, and the target is -// blocked:rules:protected-branch with no write at all. +// - alpha lets Maintainers alone push to touchmark/*: a dry run and the +// first run block it rules:protected-branch before any write, with a +// warning naming the rule. // - beta lets Developers push but not force-push: the first run opens a // merge request; after a pack change touchmark rebuilds the branch on -// the base, which needs a force push, and the target is +// the base, which needs a force push, and the push is refused: // blocked:rules:protected-branch, the merge request as it was. +// - delta has two rules on the sync branch: touchmark/* for Maintainers +// alone, and the branch's own name for Developers with force pushes. +// GitLab applies the most permissive rule that matches a branch, to +// pushes and force pushes, so delta is delivered as if unprotected. func TestProtectedBranch(t *testing.T) { e := needLive(t) s := newScenario(t, e) - for name, rule := range map[string]map[string]any{ - "alpha": {"name": "touchmark/*", "push_access_level": levelMaintainer, "merge_access_level": levelMaintainer}, - "beta": {"name": "touchmark/*", "push_access_level": levelDeveloper, "merge_access_level": levelDeveloper, "allow_force_push": false}, + for _, r := range []struct { + name string + rule map[string]any + }{ + {"alpha", map[string]any{"name": "touchmark/*", "push_access_level": levelMaintainer, "merge_access_level": levelMaintainer}}, + {"beta", map[string]any{"name": "touchmark/*", "push_access_level": levelDeveloper, "merge_access_level": levelDeveloper, "allow_force_push": false}}, + {"delta", map[string]any{"name": "touchmark/*", "push_access_level": levelMaintainer, "merge_access_level": levelMaintainer}}, + {"delta", map[string]any{"name": s.branch, "push_access_level": levelDeveloper, "merge_access_level": levelDeveloper, "allow_force_push": true}}, } { - e.api(e.Root).ok(t, http.MethodPost, fmt.Sprintf("/projects/%d/protected_branches", s.ids[name]), rule, nil) + e.api(e.Root).ok(t, http.MethodPost, fmt.Sprintf("/projects/%d/protected_branches", s.ids[r.name]), r.rule, nil) } // What the writer sees of the rule before it pushes. resp := e.api(e.Writer).do(t, http.MethodGet, fmt.Sprintf("/projects/%d/protected_branches", s.ids["alpha"]), nil) finding(t, "protected-branches-writer", "the writer's (Developer) GET /projects/:id/protected_branches: HTTP %d", resp.Status) + alphaWarned := func(what string, rep report.Delivery) { + t.Helper() + for _, tg := range rep.Targets { + if tg.Path == s.repos["alpha"] && !strings.Contains(strings.Join(tg.Warnings, " "), "is protected (touchmark/*)") { + t.Errorf("%s: alpha's warnings %q do not name the rule", what, tg.Warnings) + } + } + } + dry := s.decode(s.run(0, map[string]string{s.envVar("WRITE_TOKEN"): s.e.Writer.Token}, + "distribute", "--dry-run", "--hub", s.hub, "--hub-fp", s.fp, "--format", "json")) + s.want("a dry run", dry, s.expect(map[string]string{ + "alpha": "blocked:rules:protected-branch", "beta": "opened", "delta": "opened", + })) + alphaWarned("a dry run", dry) + rep := s.distribute(0) s.want("the first run", rep, s.expect(map[string]string{ "alpha": "blocked:rules:protected-branch", "beta": "opened: #1", "delta": "opened: #1", @@ -40,6 +67,7 @@ func TestProtectedBranch(t *testing.T) { s.wantOps("the first run", rep, map[string][]string{ "beta": {"push", "create-pr #1"}, "delta": {"push", "create-pr #1"}, }) + alphaWarned("the first run", rep) for _, tg := range rep.Targets { if tg.Path == s.repos["alpha"] { finding(t, "protected-branch-create", "alpha, touchmark/* pushed by Maintainers only: %s:%s %q", tg.Outcome, tg.Reason, tg.Warnings) @@ -58,11 +86,14 @@ func TestProtectedBranch(t *testing.T) { })) s.wantOps("the pack changed", rep, map[string][]string{"delta": {"push", "edit-pr #1"}}) for _, tg := range rep.Targets { - if tg.Path == s.repos["beta"] { + switch tg.Path { + case s.repos["beta"]: finding(t, "protected-branch-force", "beta, touchmark/* without force push, after a pack change: %s:%s %q", tg.Outcome, tg.Reason, tg.Warnings) if !strings.Contains(strings.Join(tg.Warnings, " ")+" "+tg.Reason, "protected") { t.Errorf("beta: %q %q does not name the protected branch", tg.Reason, tg.Warnings) } + case s.repos["delta"]: + finding(t, "protected-branch-permissive", "delta, touchmark/* for Maintainers and %s for Developers with force pushes: %s:%s", s.branch, tg.Outcome, tg.Reason) } } if after := s.mr("beta", 1); after.SHA != before.SHA || after.Description != before.Description || after.State != "opened" { diff --git a/internal/e2e/protected_test.go b/internal/e2e/protected_test.go new file mode 100644 index 0000000..415798b --- /dev/null +++ b/internal/e2e/protected_test.go @@ -0,0 +1,79 @@ +//go:build e2e + +package e2e + +import ( + "net/http" + "strings" + "testing" + + "github.com/bedrock-python/touchmark/internal/report" +) + +// TestProtectedBranch: branch protection on the forge, against pushes by +// anyone but administrators. The writer reads it only for branches that +// exist (GET branches/{b}: protected, user_can_push). +// +// - alpha protects touchmark/* after the first run opened its pull +// request: after a pack change, a dry run and distribute block it +// rules:protected-branch before any write, with a warning naming the +// rule, the pull request as it was. +// - beta protects touchmark/* before the first run: the sync branch does +// not exist, so the protection shows at its first push, refused with +// the same reason and nothing written. +func TestProtectedBranch(t *testing.T) { + e := needLive(t) + s := newScenario(t, e) + protect := func(name string) { + e.api(e.Admin).ok(t, http.MethodPost, "/repos/"+s.repos[name]+"/branch_protections", map[string]any{ + "rule_name": "touchmark/*", "enable_push": false, + }, nil) + } + // noWrites fails the test when rep has an op on target name. + noWrites := func(what string, rep report.Delivery, name string) { + t.Helper() + for _, op := range rep.Ops { + if op.Target == "forge:"+s.repos[name] { + t.Errorf("%s: %s got the write %s", what, name, op.Kind) + } + } + } + + protect("beta") + rep := s.distribute(0) + s.want("the first run", rep, map[string]string{"alpha": "opened: #1", "beta": "blocked:rules:protected-branch"}) + noWrites("the first run", rep, "beta") + if _, ok := s.branchHead("beta", s.branch); ok { + t.Errorf("beta has %s although no one may push to it", s.branch) + } + for _, tg := range rep.Targets { + if tg.Path == s.repos["beta"] { + finding(t, "protected-branch-new", "beta, touchmark/* protected before the sync branch exists: %s:%s %q", tg.Outcome, tg.Reason, tg.Warnings) + } + } + + protect("alpha") + before := s.pr("alpha", 1) + s.packs["packs/base/AGENTS.md"] = text("base AGENTS.md v2") + s.commitHub("base v2") + dry := s.decode(s.run(0, map[string]string{"TOUCHMARK_FORGE_WRITE_TOKEN": s.e.Writer.Token}, + "distribute", "--dry-run", "--hub", s.hub, "--hub-fp", s.fp, "--format", "json")) + rep = s.distribute(0) + for _, c := range []struct { + what string + rep report.Delivery + }{{"a dry run", dry}, {"the pack changed", rep}} { + s.want(c.what, c.rep, map[string]string{"alpha": "blocked:rules:protected-branch #1"}) + noWrites(c.what, c.rep, "alpha") + for _, tg := range c.rep.Targets { + if tg.Path == s.repos["alpha"] && !strings.Contains(strings.Join(tg.Warnings, " "), "is protected (touchmark/*)") { + t.Errorf("%s: alpha's warnings %q do not name the rule", c.what, tg.Warnings) + } + } + } + if after := s.pr("alpha", 1); after.Head.SHA != before.Head.SHA || after.Body != before.Body || after.State != "open" { + t.Errorf("alpha #1 changed although its branch is protected: %s at %s (was %s), body changed %v", + after.State, after.Head.SHA, before.Head.SHA, after.Body != before.Body) + } + s.scanPlatform() +} diff --git a/internal/platform/conformance/commit.go b/internal/platform/conformance/commit.go index b14f20f..4eb12c1 100644 --- a/internal/platform/conformance/commit.go +++ b/internal/platform/conformance/commit.go @@ -196,6 +196,23 @@ func testPreflight(t *testing.T, fx Fixture) { } } +// testPushGuard checks platform.PushGuard, for the drivers whose writer is +// one: on a repository without protection, neither the default branch nor +// a sync branch (one that does not exist yet, one that does) is listed, +// and the call writes nothing. +func testPushGuard(t *testing.T, fx Fixture) { + g, ok := fx.Writer().(platform.PushGuard) + if !ok { + t.Skip("the writer tells no protected branches upfront") + } + repo := fx.CreateRepo(t, RepoSpec{Name: "pushguard", Files: readme}) + fx.CreateBranch(t, repo, OtherBranch) + got, err := g.NoPush(t.Context(), repo, []string{repo.DefaultBranch, SyncBranch, OtherBranch, ""}) + if err != nil || len(got) > 0 { + t.Errorf("NoPush = %+v, %v on a repository without protection", got, err) + } +} + // ruleOfError returns the Rule of err's *platform.Error. func ruleOfError(err error) string { var pe *platform.Error diff --git a/internal/platform/conformance/conformance.go b/internal/platform/conformance/conformance.go index f84bc13..8be59ec 100644 --- a/internal/platform/conformance/conformance.go +++ b/internal/platform/conformance/conformance.go @@ -215,6 +215,7 @@ func Run(t *testing.T, newFixture func(t *testing.T) Fixture) { {"EnsureLabels", testEnsureLabels}, {"Commit", testCommit}, {"Preflight", testPreflight}, + {"PushGuard", testPushGuard}, {"Concurrent", testConcurrent}, } { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/platform/fake/fake.go b/internal/platform/fake/fake.go index a5a1caa..439342b 100644 --- a/internal/platform/fake/fake.go +++ b/internal/platform/fake/fake.go @@ -167,6 +167,7 @@ type Platform struct { // Rules and API commits (rules.go): preflight and apiCommits are set // by options only; workflowsHidden and apiUnsigned by their setters. preflight bool + pushGuard bool apiCommits bool workflowsHidden bool apiUnsigned bool diff --git a/internal/platform/fake/gitrepo.go b/internal/platform/fake/gitrepo.go index a45130e..68e5dd0 100644 --- a/internal/platform/fake/gitrepo.go +++ b/internal/platform/fake/gitrepo.go @@ -72,6 +72,8 @@ const ( hookSigned = "FAKE_SIGNED_BRANCHES" hookNoForce = "FAKE_NOFORCE_BRANCHES" hookNoDelete = "FAKE_NODELETE_BRANCHES" + // Branch patterns of the protected branches the pusher may not push to. + hookNoPush = "FAKE_NOPUSH_BRANCHES" ) // preReceiveHook is the pre-receive hook of every bare repository, with @@ -81,6 +83,9 @@ const ( // is judged by its diff, so moving a branch across others' workflow // changes counts, and a new ref by its diff from the default branch // (or from nothing in an empty repository); PushWorkflows; +// - a protected branch the pusher may not push to (a NoPush ruleset): +// any update, creation or deletion, with GitLab's message ("You are +// not allowed to push code to protected branches on this project."); // - the rulesets that hold the pusher (rules.go): the deletion of a // branch that refuses it ("Cannot delete this protected branch"), and, // for other updates, a force push to a branch that refuses them @@ -103,6 +108,10 @@ while read -r old new ref; do *[!0]*) ;; *) case $ref in refs/heads/*) ;; *) continue ;; esac + if [ -n "$FAKE_NOPUSH_BRANCHES" ] && matches "${ref#refs/heads/}" $FAKE_NOPUSH_BRANCHES; then + echo "GitLab: You are not allowed to push code to protected branches on this project." >&2 + exit 1 + fi if [ -n "$FAKE_NODELETE_BRANCHES" ] && matches "${ref#refs/heads/}" $FAKE_NODELETE_BRANCHES; then echo "error: GH013: Repository rule violations found for $ref." >&2 echo "" >&2 @@ -126,6 +135,10 @@ while read -r old new ref; do fi case $ref in refs/heads/*) ;; *) continue ;; esac branch=${ref#refs/heads/} + if [ -n "$FAKE_NOPUSH_BRANCHES" ] && matches "$branch" $FAKE_NOPUSH_BRANCHES; then + echo "GitLab: You are not allowed to push code to protected branches on this project." >&2 + exit 1 + fi forced= unsigned= if [ -n "$FAKE_NOFORCE_BRANCHES" ] && matches "$branch" $FAKE_NOFORCE_BRANCHES; then diff --git a/internal/platform/fake/gitserve.go b/internal/platform/fake/gitserve.go index 4679e62..175911c 100644 --- a/internal/platform/fake/gitserve.go +++ b/internal/platform/fake/gitserve.go @@ -109,10 +109,10 @@ type gitRequest struct { // the default branch, the base of a new branch). denyWorkflows bool branch string - // signed, noForce and noDelete are the branch patterns of the rulesets - // that hold the pusher (rules.go): signatures required, force pushes - // refused, deletions refused. - signed, noForce, noDelete []string + // signed, noForce, noDelete and noPush are the branch patterns of the + // rulesets that hold the pusher (rules.go): signatures required, force + // pushes refused, deletions refused, every push refused. + signed, noForce, noDelete, noPush []string } // credential is who a request's Basic credentials name: an account, with @@ -211,7 +211,7 @@ func (p *Platform) admit(r *http.Request, repoPath string, push bool) (gitReques } workflows := grant.Workflows && (c.target == nil || c.target.perms.Workflows) req.denyWorkflows = p.caps.WorkflowPerm && !workflows - req.signed, req.noForce, req.noDelete = p.hookRules(st, a) + req.signed, req.noForce, req.noDelete, req.noPush = p.hookRules(st, a) } return req, 0, "" } @@ -340,6 +340,9 @@ func (s *GitServer) backend(w http.ResponseWriter, r *http.Request, req gitReque if len(req.noDelete) > 0 { env = append(env, hookNoDelete+"="+strings.Join(req.noDelete, " ")) } + if len(req.noPush) > 0 { + env = append(env, hookNoPush+"="+strings.Join(req.noPush, " ")) + } h := &cgi.Handler{ Path: g.bin, Dir: g.dir, diff --git a/internal/platform/fake/reader.go b/internal/platform/fake/reader.go index 1844851..c1be7c3 100644 --- a/internal/platform/fake/reader.go +++ b/internal/platform/fake/reader.go @@ -31,6 +31,7 @@ var ops = map[string]string{ "Close": "close", "Snapshot": "snapshot", "Preflight": "preflight", + "NoPush": "read branch protection", "Commit": "commit", // The checks of doctor (checker.go). "Check": "check", diff --git a/internal/platform/fake/rules.go b/internal/platform/fake/rules.go index 24f9048..a6cfd77 100644 --- a/internal/platform/fake/rules.go +++ b/internal/platform/fake/rules.go @@ -33,6 +33,12 @@ import ( // flavors with Caps.WorkflowPerm. HideRules and HideWorkflows make those // unknown, as for an identity that cannot read them. // +// A ruleset with NoPush is a protected branch the accounts it holds may +// not push to at all, as GitLab's and Gitea's branch protection: the git +// server refuses their pushes to it, creations and deletions included, with +// GitLab's message. WithPushGuard makes Writer a platform.PushGuard that +// reports those branches (HideRules hides them). +// // WithAPICommits sets Caps.Commit and makes the per-target writers of // Target platform.Committers (git mode only): Commit makes the commit on // the platform, signed (a gpgsig header) unless SetAPISigning turned @@ -52,6 +58,9 @@ type Ruleset struct { NonFastForward bool // Deletion refuses deleting the branch. Deletion bool + // NoPush refuses every push to the branch, as a protected branch that + // does not let the account push (GitLab, Gitea). + NoPush bool // Bypass lists the accounts the ruleset does not hold (an App on the // bypass list). Bypass []platform.Account @@ -63,6 +72,11 @@ func WithPreflight() Option { return func(p *Platform) { p.preflight = true } } +// WithPushGuard makes Writer implement platform.PushGuard (see Ruleset). +func WithPushGuard() Option { + return func(p *Platform) { p.pushGuard = true } +} + // WithAPICommits makes the per-target writers implement // platform.Committer and sets Caps.Commit (API, SignedByPlatform, CAS), // after any WithFlavor. @@ -207,10 +221,10 @@ func (rs Ruleset) holds(account platform.Account) bool { } // hookRules returns the patterns of the branches on which the rulesets of s -// that hold account require signatures, forbid force pushes and forbid -// deletions, in the shell's case syntax of the pre-receive hook. Called -// with mu held. -func (p *Platform) hookRules(s *repoState, account platform.Account) (signed, noForce, noDelete []string) { +// that hold account require signatures, forbid force pushes, forbid +// deletions and forbid any push, in the shell's case syntax of the +// pre-receive hook. Called with mu held. +func (p *Platform) hookRules(s *repoState, account platform.Account) (signed, noForce, noDelete, noPush []string) { for _, rs := range s.rulesets { if !rs.holds(account) { continue @@ -235,8 +249,11 @@ func (p *Platform) hookRules(s *repoState, account platform.Account) (signed, no if rs.Deletion { noDelete = append(noDelete, pats...) } + if rs.NoPush { + noPush = append(noPush, pats...) + } } - return signed, noForce, noDelete + return signed, noForce, noDelete, noPush } // shellQuotePattern escapes the characters a branch name may hold that a @@ -279,6 +296,60 @@ func (p *Platform) rulesOf(ctx context.Context, as platform.Account, r platform. }) } +// noPushOf is NoPush of the PushGuard forms of Writer: the branches of r, +// the default branch aside, that a NoPush ruleset holding as covers, each +// with the ruleset's first pattern that matches it; none while the rules +// are hidden. +func (p *Platform) noPushOf(ctx context.Context, as platform.Account, r platform.Repo, branches []string) ([]platform.Protected, error) { + op := ops["NoPush"] + return do(ctx, p, &as, "NoPush", append([]string{r.Path}, branches...), func() ([]platform.Protected, error) { + s, err := p.repoOf(op, r) + if err != nil { + return nil, err + } + if s.rulesHidden { + return nil, nil + } + var out []platform.Protected + for _, b := range dedupe(branches) { + if b == "" || b == s.repo.DefaultBranch { + continue + } + for _, rs := range s.rulesets { + if !rs.NoPush || !rs.holds(as) || !rs.matches(b, s.repo.DefaultBranch) { + continue + } + rule := rs.Branches[0] + for _, pat := range rs.Branches { + if (Ruleset{Branches: []string{pat}}).matches(b, s.repo.DefaultBranch) { + rule = pat + break + } + } + out = append(out, platform.Protected{Branch: b, Rule: rule}) + break + } + } + return out, nil + }) +} + +// guardWriter is a writer that tells the branches it may not push to +// (WithPushGuard); preflightGuardWriter also reads the rules +// (WithPreflight). +type ( + guardWriter struct{ writer } + preflightGuardWriter struct{ preflightWriter } +) + +func (w *guardWriter) NoPush(ctx context.Context, repo platform.Repo, branches []string) ([]platform.Protected, error) { + return w.p.noPushOf(ctx, w.as, repo, branches) +} + +func (w *preflightGuardWriter) NoPush(ctx context.Context, repo platform.Repo, branches []string) ([]platform.Protected, error) { + return w.p.noPushOf(ctx, w.as, repo, branches) +} + // preflightReader is a reader that reads the rules (WithPreflight). type preflightReader struct{ reader } diff --git a/internal/platform/fake/writer.go b/internal/platform/fake/writer.go index 25b693b..c577694 100644 --- a/internal/platform/fake/writer.go +++ b/internal/platform/fake/writer.go @@ -18,10 +18,16 @@ import ( // Writer returns the platform.Writer of account as. It reads like Reader(as) // and writes where Grant or GrantWrite allowed it. func (p *Platform) Writer(as platform.Account) platform.Writer { - if p.preflight { - return &preflightWriter{writer{reader{p: p, as: as}}} + w := writer{reader{p: p, as: as}} + switch { + case p.preflight && p.pushGuard: + return &preflightGuardWriter{preflightWriter{w}} + case p.preflight: + return &preflightWriter{w} + case p.pushGuard: + return &guardWriter{w} } - return &writer{reader{p: p, as: as}} + return &w } type writer struct{ reader } diff --git a/internal/platform/gitea/check.go b/internal/platform/gitea/check.go index b245a30..df26ee2 100644 --- a/internal/platform/gitea/check.go +++ b/internal/platform/gitea/check.go @@ -1,6 +1,7 @@ package gitea import ( + "cmp" "context" "fmt" "slices" @@ -228,6 +229,40 @@ func (w *writer) branchRules(ctx context.Context, op, owner, name string, branch return platform.Finding{Check: "rules", Status: status, Detail: strings.Join(notes, "; ")}, nil } +// NoPush lists the branches among branches, the default branch aside, +// that branch protection keeps the writer from pushing to +// (platform.PushGuard): GET /repos/{owner}/{repo}/branches/{branch} of +// each, protected and not user_can_push, which the server works out for +// the caller. The API knows existing branches only, so a protection that +// covers a sync branch not created yet shows at its first push; so does +// anything it cannot read. +func (w *writer) NoPush(ctx context.Context, r platform.Repo, branches []string) ([]platform.Protected, error) { + const op = "read branch protection" + owner, name, err := repoPath(op, r) + if err != nil { + return nil, nil + } + var out []platform.Protected + for _, b := range uniqueBranches(branches) { + if b == r.DefaultBranch { + continue + } + var br apiRepoBranch + _, err := w.c.get(ctx, op, w.c.endpoint("repos", owner, name, "branches", b), nil, &br) + switch { + case err == nil: + case stops(err): + return nil, err + default: + continue + } + if br.Protected && !br.UserCanPush { + out = append(out, platform.Protected{Branch: b, Rule: cmp.Or(br.EffectiveRef, b)}) + } + } + return out, nil +} + // uniqueBranches returns the non-empty branches, each once, in order. func uniqueBranches(branches []string) []string { var out []string @@ -289,4 +324,5 @@ func keyBlob(line string) string { var ( _ platform.Checker = (*writer)(nil) _ platform.KeyChecker = (*writer)(nil) + _ platform.PushGuard = (*writer)(nil) ) diff --git a/internal/platform/gitea/pushguard_test.go b/internal/platform/gitea/pushguard_test.go new file mode 100644 index 0000000..242b335 --- /dev/null +++ b/internal/platform/gitea/pushguard_test.go @@ -0,0 +1,58 @@ +package gitea + +import ( + "net/http" + "slices" + "strings" + "testing" + + "github.com/bedrock-python/touchmark/internal/platform" +) + +func TestNoPush(t *testing.T) { + fx := newFixture(t, "gitea") + branch := func(name string, status int, protected, canPush bool) { + if status != http.StatusOK { + fx.json(http.MethodGet, "/repos/acme/api/branches/"+name, status, fx.apiMsg("no")) + return + } + fx.json(http.MethodGet, "/repos/acme/api/branches/"+name, http.StatusOK, map[string]any{ + "name": name, "commit": map[string]any{"id": strings.Repeat("a", 40)}, "protected": protected, + "required_approvals": 0, "enable_status_check": false, "status_check_contexts": []string{}, + "user_can_push": canPush, "user_can_merge": canPush, "effective_branch_protection_name": "touchmark/*", + }) + } + branch("touchmark/acme-eng", http.StatusOK, true, false) + branch("touchmark/pushable", http.StatusOK, true, true) + branch("touchmark/free", http.StatusOK, false, true) + branch("touchmark/new", http.StatusNotFound, false, false) + branch("touchmark/broken", http.StatusInternalServerError, false, false) + repo := platform.Repo{Host: fx.provider("gitea").Host, ID: "7", Path: "acme/api", DefaultBranch: "main"} + + got, err := fx.writer.NoPush(t.Context(), repo, []string{"main", "touchmark/acme-eng", "touchmark/pushable", "touchmark/free", + "touchmark/new", "touchmark/broken", "", "touchmark/acme-eng"}) + if err != nil { + t.Fatal(err) + } + want := []platform.Protected{{Branch: "touchmark/acme-eng", Rule: "touchmark/*"}} + if !slices.Equal(got, want) { + t.Errorf("NoPush = %+v, want %+v", got, want) + } + if n := len(fx.requests(http.MethodGet, "/repos/acme/api/branches/main")); n != 0 { + t.Errorf("NoPush read the default branch %d times", n) + } + if n := len(fx.requests(http.MethodGet, "/repos/acme/api/branches/touchmark/acme-eng")); n != 1 { + t.Errorf("NoPush read touchmark/acme-eng %d times, want 1", n) + } + + // A refused credential ends the call. + branch("touchmark/acme-eng", http.StatusUnauthorized, false, false) + if _, err := fx.writer.NoPush(t.Context(), repo, []string{"touchmark/acme-eng"}); platform.ClassOf(err) != platform.ClassAuth { + t.Errorf("NoPush with a refused credential: %v", err) + } + for _, c := range fx.requests("", "") { + if c.Method != http.MethodGet { + t.Errorf("NoPush wrote: %s %s", c.Method, c.Path) + } + } +} diff --git a/internal/platform/gitlab/check.go b/internal/platform/gitlab/check.go index c17ec0a..a70588e 100644 --- a/internal/platform/gitlab/check.go +++ b/internal/platform/gitlab/check.go @@ -84,11 +84,11 @@ type apiKey struct { // Developer is ok, Maintainer or Owner warn (a leaked key could read the // target's CI variables and change its protected branches), below // Developer fails. "rules" from the protected branches (Developer reads -// them): a protected branch whose name or wildcard covers a -// sync branch and does not let the writer push fails -// (blocked:rules:protected-branch), one that lets it push without force -// pushes warns (a rebuild is refused), a rule for a group the writer may -// belong to is unknown; push rules (EE) show only at push time. +// them), the most permissive of those whose name or wildcard covers a sync +// branch deciding, as GitLab does: when none lets the writer push it fails +// (blocked:rules:protected-branch), when none allows force pushes it warns +// (a rebuild is refused), and when only a group the writer may belong to +// could push it is unknown; push rules (EE) show only at push time. func (w *writer) Check(ctx context.Context, repos []platform.Repo, branches []string) ([]platform.Finding, error) { if len(repos) == 0 { return w.identityChecks(ctx) @@ -277,14 +277,64 @@ func (w *writer) accessLevel(ctx context.Context, op string, self platform.Accou return level, nil } -// protectedRules grades the protected branches that cover the sync -// branches, for the writer with level on the project. -func (w *writer) protectedRules(ctx context.Context, op, id string, self platform.Account, level int, branches []string) (platform.Finding, error) { - var rules []apiProtectedBranch - complete, err := listAll(ctx, w.c, op, w.c.projectURL(id, "protected_branches"), nil, maxProtectedPages, func(b apiProtectedBranch) error { +// listProtected lists the protected branches of project id; complete is +// false when it has more than touchmark reads. +func (w *writer) listProtected(ctx context.Context, op, id string) (rules []apiProtectedBranch, complete bool, err error) { + complete, err = listAll(ctx, w.c, op, w.c.projectURL(id, "protected_branches"), nil, maxProtectedPages, func(b apiProtectedBranch) error { rules = append(rules, b) return nil }) + return rules, complete, err +} + +// branchVerdict is what the protected branches that match one branch let +// the writer do there. GitLab applies the most permissive of the rules +// that match a branch, to pushes and to force pushes alike. +type branchVerdict struct { + // rules are the names of the protected branches that match it; none + // when it is not protected. + rules []string + // push is ok when one of them lets the writer push, unknown when none + // does but one lets a group push, whose members the writer cannot read, + // and fail otherwise. + push platform.FindingStatus + // force is set when one of them allows force pushes. + force bool +} + +// verdictOf grades branch under rules for the writer (selfID, with level on +// the project). +func verdictOf(rules []apiProtectedBranch, branch string, selfID int64, level int) branchVerdict { + v := branchVerdict{push: platform.FindingOK} + group := false + pushes := false + for _, rule := range rules { + if !protectedMatch(rule.Name, branch) { + continue + } + v.rules = append(v.rules, rule.Name) + v.force = v.force || rule.AllowForcePush + switch canPush(rule.Push, selfID, level) { + case platform.FindingOK: + pushes = true + case platform.FindingUnknown: + group = true + } + } + switch { + case len(v.rules) == 0, pushes: + case group: + v.push = platform.FindingUnknown + default: + v.push = platform.FindingFail + } + return v +} + +// protectedRules grades the protected branches that cover the sync +// branches, for the writer with level on the project. +func (w *writer) protectedRules(ctx context.Context, op, id string, self platform.Account, level int, branches []string) (platform.Finding, error) { + rules, complete, err := w.listProtected(ctx, op, id) if err != nil { return platform.Finding{}, err } @@ -301,23 +351,21 @@ func (w *writer) protectedRules(ctx context.Context, op, id string, self platfor if b == "" { continue } - for _, rule := range rules { - if !protectedMatch(rule.Name, b) { - continue - } - switch canPush(rule.Push, selfID, level) { - case platform.FindingFail: - worse(platform.FindingFail) - notes = append(notes, fmt.Sprintf("protected branch %s keeps the writer from pushing to %s: blocked:rules:protected-branch", rule.Name, b)) - case platform.FindingUnknown: - worse(platform.FindingUnknown) - notes = append(notes, fmt.Sprintf("protected branch %s covers %s and lets a group push, whose members the writer cannot read", rule.Name, b)) - default: - if !rule.AllowForcePush { - worse(platform.FindingWarn) - notes = append(notes, fmt.Sprintf("protected branch %s covers %s without force pushes: a rebuild of the sync branch is refused", rule.Name, b)) - } - } + v := verdictOf(rules, b, selfID, level) + if len(v.rules) == 0 { + continue + } + names := strings.Join(v.rules, ", ") + switch { + case v.push == platform.FindingFail: + worse(platform.FindingFail) + notes = append(notes, fmt.Sprintf("protected branch %s keeps the writer from pushing to %s: blocked:rules:protected-branch", names, b)) + case v.push == platform.FindingUnknown: + worse(platform.FindingUnknown) + notes = append(notes, fmt.Sprintf("protected branch %s covers %s and lets a group push, whose members the writer cannot read", names, b)) + case !v.force: + worse(platform.FindingWarn) + notes = append(notes, fmt.Sprintf("protected branch %s covers %s without force pushes: a rebuild of the sync branch is refused", names, b)) } } if !complete { @@ -331,6 +379,63 @@ func (w *writer) protectedRules(ctx context.Context, op, id string, self platfor return platform.Finding{Check: "rules", Status: status, Detail: strings.Join(notes, "; ")}, nil } +// NoPush lists the branches among branches, the default branch aside, +// that protected branches keep the writer from pushing to +// (platform.PushGuard): the rules that match the branch all deny the +// writer, and none lets a group push. One request lists the protected +// branches; the project and the writer's access level are read only when +// one matches. A listing touchmark cannot read whole, or anything else it +// cannot read, leaves the branches out: their pushes find the rules. Push +// rules (EE) show only at push time. +func (w *writer) NoPush(ctx context.Context, r platform.Repo, branches []string) ([]platform.Protected, error) { + const op = "read protected branches" + var want []string + for _, b := range branches { + if b != "" && b != r.DefaultBranch && !slices.Contains(want, b) { + want = append(want, b) + } + } + if len(want) == 0 { + return nil, nil + } + unread := func(err error) ([]platform.Protected, error) { + if stops(err) { + return nil, err + } + return nil, nil + } + id := projectID(r) + rules, complete, err := w.listProtected(ctx, op, id) + if err != nil { + return unread(err) + } + if !complete || !slices.ContainsFunc(want, func(b string) bool { + return slices.ContainsFunc(rules, func(rule apiProtectedBranch) bool { return protectedMatch(rule.Name, b) }) + }) { + return nil, nil + } + self, err := w.c.selfAccount(ctx) + if err != nil { + return unread(err) + } + p, err := w.c.getProject(ctx, op, id) + if err != nil { + return unread(err) + } + level, err := w.accessLevel(ctx, op, self, p) + if err != nil { + return unread(err) + } + selfID, _ := strconv.ParseInt(self.ID, 10, 64) + var out []platform.Protected + for _, b := range want { + if v := verdictOf(rules, b, selfID, level); v.push == platform.FindingFail { + out = append(out, platform.Protected{Branch: b, Rule: strings.Join(v.rules, ", ")}) + } + } + return out, nil +} + // protectedMatch reports whether a protected branch name, where '*' is a // wildcard for any run of characters, covers branch. func protectedMatch(pattern, branch string) bool { @@ -432,4 +537,5 @@ func keyBlob(line string) string { var ( _ platform.Checker = (*writer)(nil) _ platform.KeyChecker = (*writer)(nil) + _ platform.PushGuard = (*writer)(nil) ) diff --git a/internal/platform/gitlab/pushguard_test.go b/internal/platform/gitlab/pushguard_test.go new file mode 100644 index 0000000..74f2231 --- /dev/null +++ b/internal/platform/gitlab/pushguard_test.go @@ -0,0 +1,119 @@ +package gitlab + +import ( + "net/http" + "slices" + "testing" + + "github.com/bedrock-python/touchmark/internal/platform" +) + +// protectedLevel is a push access level of a protected branch: a role. +func protectedLevel(n int) map[string]any { + return map[string]any{"id": n, "access_level": n, "access_level_description": "x", "deploy_key_id": nil, "user_id": nil, "group_id": nil} +} + +// TestVerdictOf: GitLab applies the most permissive of the protected +// branches that match a branch, to pushes and to force pushes. +func TestVerdictOf(t *testing.T) { + const selfID, dev = 7, 30 + role := func(n int) apiProtectedLevel { return apiProtectedLevel{AccessLevel: n} } + userID, groupID := int64(selfID), int64(99) + exact := apiProtectedBranch{Name: "touchmark/acme-eng", Push: []apiProtectedLevel{role(30)}} + maintainers := apiProtectedBranch{Name: "touchmark/*", Push: []apiProtectedLevel{role(40)}, AllowForcePush: true} + noOne := apiProtectedBranch{Name: "*", Push: []apiProtectedLevel{role(0)}} + group := apiProtectedBranch{Name: "touchmark/*", Push: []apiProtectedLevel{{GroupID: &groupID}}} + user := apiProtectedBranch{Name: "touchmark/*", Push: []apiProtectedLevel{{UserID: &userID}}} + for _, tc := range []struct { + name string + rules []apiProtectedBranch + push platform.FindingStatus + force bool + n int + }{ + {"none", nil, platform.FindingOK, false, 0}, + {"another branch", []apiProtectedBranch{{Name: "main", Push: []apiProtectedLevel{role(0)}}}, platform.FindingOK, false, 0}, + {"Maintainers only", []apiProtectedBranch{maintainers}, platform.FindingFail, true, 1}, + {"Developers, no force", []apiProtectedBranch{exact}, platform.FindingOK, false, 1}, + {"exact allows, wildcard denies", []apiProtectedBranch{exact, maintainers}, platform.FindingOK, true, 2}, + {"wildcard allows, no one on another", []apiProtectedBranch{noOne, exact}, platform.FindingOK, false, 2}, + {"both deny", []apiProtectedBranch{noOne, maintainers}, platform.FindingFail, true, 2}, + {"a group", []apiProtectedBranch{group}, platform.FindingUnknown, false, 1}, + {"a group and a denial", []apiProtectedBranch{noOne, group}, platform.FindingUnknown, false, 2}, + {"the writer by id", []apiProtectedBranch{noOne, user}, platform.FindingOK, false, 2}, + } { + v := verdictOf(tc.rules, "touchmark/acme-eng", selfID, dev) + if v.push != tc.push || v.force != tc.force || len(v.rules) != tc.n { + t.Errorf("%s: %+v, want push %s, force %v, %d rules", tc.name, v, tc.push, tc.force, tc.n) + } + } +} + +func TestNoPush(t *testing.T) { + newRepo := func(fx *fixture) platform.Repo { + return platform.Repo{Host: fx.provider().Host, ID: "11", Path: "acme/api", DefaultBranch: "main"} + } + setup := func(t *testing.T, rules []any) *fixture { + fx := newFixture(t) + fx.json(http.MethodGet, "/user", http.StatusOK, self(7, "service_account_writer", true)) + fx.json(http.MethodGet, "/projects/11", http.StatusOK, project(11, "acme/api", + with("permissions", map[string]any{"project_access": nil, "group_access": map[string]any{"access_level": 30, "notification_level": 3}}))) + fx.pages("/projects/11/protected_branches", rules) + return fx + } + branches := []string{"main", "touchmark/acme-eng", "", "touchmark/old", "touchmark/acme-eng"} + + t.Run("blocked", func(t *testing.T) { + fx := setup(t, []any{ + map[string]any{"id": 1, "name": "main", "allow_force_push": false, "push_access_levels": []any{protectedLevel(0)}}, + map[string]any{"id": 2, "name": "touchmark/*", "allow_force_push": false, "push_access_levels": []any{protectedLevel(40)}}, + map[string]any{"id": 3, "name": "touchmark/old", "allow_force_push": false, "push_access_levels": []any{protectedLevel(30)}}, + }) + got, err := fx.writer.NoPush(t.Context(), newRepo(fx), branches) + if err != nil { + t.Fatal(err) + } + want := []platform.Protected{{Branch: "touchmark/acme-eng", Rule: "touchmark/*"}} + if !slices.Equal(got, want) { + t.Errorf("NoPush = %+v, want %+v", got, want) + } + if w := fx.writes(); len(w) > 0 { + t.Errorf("NoPush wrote: %+v", w) + } + }) + t.Run("no rule matches", func(t *testing.T) { + fx := setup(t, []any{ + map[string]any{"id": 1, "name": "main", "allow_force_push": false, "push_access_levels": []any{protectedLevel(0)}}, + }) + got, err := fx.writer.NoPush(t.Context(), newRepo(fx), branches) + if err != nil || len(got) > 0 { + t.Errorf("NoPush = %+v, %v", got, err) + } + // One request: the project and the account are not read. + if n := len(fx.requests("", "")); n != 1 { + t.Errorf("%d requests, want 1: %+v", n, fx.requests("", "")) + } + }) + t.Run("only the default branch", func(t *testing.T) { + fx := setup(t, nil) + got, err := fx.writer.NoPush(t.Context(), newRepo(fx), []string{"main", ""}) + if err != nil || len(got) > 0 || len(fx.requests("", "")) > 0 { + t.Errorf("NoPush = %+v, %v, requests %+v", got, err, fx.requests("", "")) + } + }) + t.Run("unreadable", func(t *testing.T) { + fx := setup(t, nil) + fx.json(http.MethodGet, "/projects/11/protected_branches", http.StatusForbidden, msg("403 Forbidden")) + got, err := fx.writer.NoPush(t.Context(), newRepo(fx), branches) + if err != nil || len(got) > 0 { + t.Errorf("a 403 listing: %+v, %v", got, err) + } + }) + t.Run("a refused credential", func(t *testing.T) { + fx := setup(t, nil) + fx.json(http.MethodGet, "/projects/11/protected_branches", http.StatusUnauthorized, msg("401 Unauthorized")) + if _, err := fx.writer.NoPush(t.Context(), newRepo(fx), branches); err == nil || platform.ClassOf(err) != platform.ClassAuth { + t.Errorf("a refused credential: %v", err) + } + }) +} diff --git a/internal/platform/platform.go b/internal/platform/platform.go index 2eacf4e..5599948 100644 --- a/internal/platform/platform.go +++ b/internal/platform/platform.go @@ -374,7 +374,7 @@ type Committer interface { Commit(ctx context.Context, r Repo, req CommitRequest) (Commit, error) } -// Checker runs write-identity checks for doctor and preflight. Optional. +// Checker runs write-identity checks for doctor. Optional. // // With repositories, Check reports the checks of each of them (Finding.Repo // its path): "access" (the identity may push and write pull requests), @@ -401,8 +401,9 @@ type KeyChecker interface { // Preflighter tells, before any write, which platform rules delivery to r // will meet (signatures, force pushes, deletions, the permission to change -// CI files). Optional: without it the core knows no rules upfront, as on -// GitLab, Gitea and Forgejo, where pushes find them at run time. +// CI files). Optional: without it the core knows none of these upfront, as +// on GitLab, Gitea and Forgejo, where pushes find them at run time (a +// PushGuard still tells the branches the writer may not push to at all). type Preflighter interface { // Preflight reads the rules on branches of r (the default branch and // the sync branch, which may not exist yet). @@ -430,6 +431,24 @@ type Rules struct { WorkflowsKnown, Workflows bool } +// PushGuard tells, before any write, which branches a protection rule +// keeps the write identity from pushing to at all (GitLab's protected +// branches, Gitea's and Forgejo's branch protection). Optional. +type PushGuard interface { + // NoPush returns the branches among branches, the default branch + // aside, that a rule the identity can read keeps it from pushing to, + // each with the rule. A rule it cannot read never lists a branch: + // the push finds it. Only a rate limit, a refused credential or the + // end of ctx fail the call; any other failure leaves branches out. + NoPush(ctx context.Context, r Repo, branches []string) ([]Protected, error) +} + +// Protected is a branch a protection rule keeps the identity from pushing +// to, and the rule (its name or pattern). +type Protected struct { + Branch, Rule string +} + // BatchReader reads one file from many repositories per request (GitHub // GraphQL aliases, GitLab GraphQL). Optional. type BatchReader interface { From 10891db95a7dfef89e15273046b90508f88789a9 Mon Sep 17 00:00:00 2001 From: AlexeyShalaev <75322386+AlexeyShalaev@users.noreply.github.com> Date: Thu, 8 Oct 2026 14:01:31 +0300 Subject: [PATCH 2/2] fix(gitea): no guessed rule name when the forge does not show it Gitea and Forgejo leave effective_branch_protection_name out for a writer without admin rights, so a protected branch's warning names the branch alone. The live tests now expect that, and a dry run on GitLab reports opened with an empty reason. --- docs/project/e2e.md | 2 +- internal/distribute/inspect.go | 6 +++++- internal/e2e/gitlab/protected_test.go | 2 +- internal/e2e/protected_test.go | 10 +++++++--- internal/platform/gitea/check.go | 6 +++--- internal/platform/platform.go | 3 ++- 6 files changed, 19 insertions(+), 10 deletions(-) diff --git a/docs/project/e2e.md b/docs/project/e2e.md index c4c1612..993761e 100644 --- a/docs/project/e2e.md +++ b/docs/project/e2e.md @@ -86,7 +86,7 @@ The script also sets `TOUCHMARK_E2E_REQUIRED=1`: the tests then fail instead of - Each step also checks which writes each target got, by the report's journal. - Before it checks that a second run writes nothing, the test waits for the server's own work after a push. The server writes the push event into the pull request's timeline from a queue, seconds after the push, and that changes the pull request's `updated_at`. For a person's pull request the comparison leaves `updated_at` out: touchmark never writes to one (its state, head, text, labels and comments tell), and the person's own push may land in its timeline late. - **`TestDoctor`**: `doctor` through `cli.Main` with the writer's token, over a fresh organisation: a writable opted-in repository (access ok; rules unknown while no sync branch exists, since the writer reads the protection of existing branches only), a read-only one (access fails), one not opted in (skipped), one whose sync branch a rule `touchmark/*` without pushes protects (rules fail), and one whose pull requests on the sync branch carry another hub's marker with the same id and a person's copy of this hub's (markers warn). The hub is a repository the writer reads (`hub-hidden` warns), then writes (fails). The token's scopes come from `GET /api/v1/token` where the forge has it; `--hub-token` with the admin's token reads the hub's Actions secrets, and a write key there fails. No token reaches any output. -- **`TestProtectedBranch`**: branch protection `touchmark/*` without pushes, which the writer reads for existing branches only. On a target whose pull request is open, a pack change is blocked `blocked:rules:protected-branch` before any write, in a dry run and in `distribute`, with a warning naming the rule, the pull request as it was. On a target protected before its sync branch exists, the first push meets the protection and is refused with the same reason, nothing written. +- **`TestProtectedBranch`**: branch protection `touchmark/*` without pushes, which the writer reads for existing branches only. On a target whose pull request is open, a pack change is blocked `blocked:rules:protected-branch` before any write, in a dry run and in `distribute`, with a warning naming the branch (the forge does not show the writer the rule's name), the pull request as it was. On a target protected before its sync branch exists, the first push meets the protection and is refused with the same reason, nothing written. - **`TestFacts`**: forge behavior touchmark relies on that the documentation leaves open, checked through the API and git alone, without the driver: - flavor detection; - the head filter of the pull request list (and whether it matches forks), `poster`, `pulls/{base}/{head}`; diff --git a/internal/distribute/inspect.go b/internal/distribute/inspect.go index 099afb5..b9711c2 100644 --- a/internal/distribute/inspect.go +++ b/internal/distribute/inspect.go @@ -820,7 +820,11 @@ func (r *run) blockProtected(w *Work, protected []platform.Protected) { } w.Decision = decide.TargetDecision{Outcome: decide.OutcomeBlocked, Reason: decide.ReasonRulesProtected, PR: d.PR, Branch: d.Branch, Steps: upkeep} for _, p := range protected { - w.t.res.Warnings = append(w.t.res.Warnings, fmt.Sprintf("branch %s is protected (%s), and the writer may not push to it", p.Branch, p.Rule)) + rule := "" + if p.Rule != "" { + rule = " (" + p.Rule + ")" + } + w.t.res.Warnings = append(w.t.res.Warnings, fmt.Sprintf("branch %s is protected%s, and the writer may not push to it", p.Branch, rule)) } } diff --git a/internal/e2e/gitlab/protected_test.go b/internal/e2e/gitlab/protected_test.go index 5428053..37715e4 100644 --- a/internal/e2e/gitlab/protected_test.go +++ b/internal/e2e/gitlab/protected_test.go @@ -56,7 +56,7 @@ func TestProtectedBranch(t *testing.T) { dry := s.decode(s.run(0, map[string]string{s.envVar("WRITE_TOKEN"): s.e.Writer.Token}, "distribute", "--dry-run", "--hub", s.hub, "--hub-fp", s.fp, "--format", "json")) s.want("a dry run", dry, s.expect(map[string]string{ - "alpha": "blocked:rules:protected-branch", "beta": "opened", "delta": "opened", + "alpha": "blocked:rules:protected-branch", "beta": "opened:", "delta": "opened:", })) alphaWarned("a dry run", dry) diff --git a/internal/e2e/protected_test.go b/internal/e2e/protected_test.go index 415798b..6d2f862 100644 --- a/internal/e2e/protected_test.go +++ b/internal/e2e/protected_test.go @@ -17,7 +17,8 @@ import ( // - alpha protects touchmark/* after the first run opened its pull // request: after a pack change, a dry run and distribute block it // rules:protected-branch before any write, with a warning naming the -// rule, the pull request as it was. +// branch (the forge does not show the writer the rule's name), the +// pull request as it was. // - beta protects touchmark/* before the first run: the sync branch does // not exist, so the protection shows at its first push, refused with // the same reason and nothing written. @@ -66,8 +67,11 @@ func TestProtectedBranch(t *testing.T) { s.want(c.what, c.rep, map[string]string{"alpha": "blocked:rules:protected-branch #1"}) noWrites(c.what, c.rep, "alpha") for _, tg := range c.rep.Targets { - if tg.Path == s.repos["alpha"] && !strings.Contains(strings.Join(tg.Warnings, " "), "is protected (touchmark/*)") { - t.Errorf("%s: alpha's warnings %q do not name the rule", c.what, tg.Warnings) + if tg.Path == s.repos["alpha"] && !strings.Contains(strings.Join(tg.Warnings, " "), "branch "+s.branch+" is protected") { + t.Errorf("%s: alpha's warnings %q do not name the protected branch", c.what, tg.Warnings) + } + if tg.Path == s.repos["alpha"] && c.what == "a dry run" { + finding(t, "protected-branch-predicted", "alpha, touchmark/* protected after its pull request opened, a dry run: %s:%s %q", tg.Outcome, tg.Reason, tg.Warnings) } } } diff --git a/internal/platform/gitea/check.go b/internal/platform/gitea/check.go index df26ee2..6bfc2bb 100644 --- a/internal/platform/gitea/check.go +++ b/internal/platform/gitea/check.go @@ -1,7 +1,6 @@ package gitea import ( - "cmp" "context" "fmt" "slices" @@ -233,7 +232,8 @@ func (w *writer) branchRules(ctx context.Context, op, owner, name string, branch // that branch protection keeps the writer from pushing to // (platform.PushGuard): GET /repos/{owner}/{repo}/branches/{branch} of // each, protected and not user_can_push, which the server works out for -// the caller. The API knows existing branches only, so a protection that +// the caller, with effective_branch_protection_name as the rule when the +// server shows it (it does not to a writer without admin rights). The API knows existing branches only, so a protection that // covers a sync branch not created yet shows at its first push; so does // anything it cannot read. func (w *writer) NoPush(ctx context.Context, r platform.Repo, branches []string) ([]platform.Protected, error) { @@ -257,7 +257,7 @@ func (w *writer) NoPush(ctx context.Context, r platform.Repo, branches []string) continue } if br.Protected && !br.UserCanPush { - out = append(out, platform.Protected{Branch: b, Rule: cmp.Or(br.EffectiveRef, b)}) + out = append(out, platform.Protected{Branch: b, Rule: br.EffectiveRef}) } } return out, nil diff --git a/internal/platform/platform.go b/internal/platform/platform.go index 5599948..b7c0b05 100644 --- a/internal/platform/platform.go +++ b/internal/platform/platform.go @@ -444,7 +444,8 @@ type PushGuard interface { } // Protected is a branch a protection rule keeps the identity from pushing -// to, and the rule (its name or pattern). +// to, and the rule (its name or pattern; empty when the platform does not +// show it). type Protected struct { Branch, Rule string }