Sign in

Search issue titles and bodies, change requests and comments. Private repositories you cannot read are not included.

vole
Clone

Read only, no credentials:

https://rodent.software/vole.git

Write over SSH with a registered key:

git clone ssh://git@rodent.software:2222/vole.git

A free and open-source forge that lives in your Git repo.

Phase 3: change requests, with approvals bound to the commit they reviewed

1eeca4e67a65d71217d51b1c41632330225b351a · Keenan Nemetz · 18d ago · parent 92b37587

Phase 3: change requests, with approvals bound to the commit they reviewed

Review is now a git artifact. A request names a base branch and one head commit;
an approval names that same commit, so a branch that moves after approval is not
approved any more. Because `git push` is the merge, the gate lives in pre-receive
and there is no authenticated write surface to build.

  requests/<ulid>/meta.json          title, author, base, head, state
  requests/<ulid>/description.md
  requests/<ulid>/approvals/<user>.json   base + head + created

Two push shapes are accepted for a protected branch: the new tip is the approved
head, or the new tip is a merge whose parents are the branch tip and the approved
head. "The approved head is an ancestor of the new tip" is refused on purpose —
accepting it would let anyone append unapproved commits and land them together
with an approved one. Admins keep their bypass; `vole acl approvals <repo> <n>`
sets the count, and 0 restores admin-only.

Three integrity holes found while building it, each now covered by a test:

- An approval file names a principal, but nothing tied that name to the pusher, so
  any writer could commit approvals/<someone-else>.json. The hook now requires a
  newly added approval to be named after the SSH identity. That check initially
  skipped the push that *creates* the branch, where there is no old commit to diff
  against — which let whoever created the branch seed any approval they liked.
  Fixed to inspect the whole tree on a first push.
- Approvals were bound to the head but not the branch, so rewriting a request's
  base repointed an approval at somewhere the approver never looked.
- Self-approval was refused at the CLI but not in validation, so a hand-written
  tree could carry one.

Also relaxes the CLI to accept options after arguments, as git does. Refusing
`vole request approve <id> --as keenan` is a local dialect, not git's behaviour,
and `git commit -m msg path` is the shape people already type. Exit 129, no
bundling and `--`/`--end-of-options` are unchanged.

Verified end to end on a real instance with two keys: a writer's direct push to a
protected main is refused with a message naming the exact commit; the same push
still fails with a request open but unapproved; self-approval fails; an admin
approval lets it through; main advances; the web shows the approval as current and
the request as already merged; the audit log records the merge with request id and
approver.

Known rough edge: a merged request stays open until someone closes it. Marking it
merged automatically needs the server to write metadata during a push, which the
design deliberately does not do. Phase 4 CI is the natural place to close it.

internal/hook/hook.go +85−5

@@ -15,6 +15,7 @@ import (
15 15 "github.com/rodent-software/vole/internal/gitx"
16 16 "github.com/rodent-software/vole/internal/issues"
17 17 "github.com/rodent-software/vole/internal/repo"
18 "github.com/rodent-software/vole/internal/requests"
18 19 )
19 20
20 21 type Config struct {
@@ -111,11 +112,44 @@ func PreReceive(cfg Config) int {
111 112 }
112 113 }
113 114
114 if a.IsProtected(c.ref) && !role.AtLeast(acl.Admin) {
115 problems = append(problems, fmt.Sprintf(
116 "%s is protected: %s may not push it directly (role: %s). Open a change request instead.",
117 c.ref, actor, role))
118 continue
115 if a.IsProtected(c.ref) {
116 // Admins keep the bypass they always had. Everyone else needs an
117 // approved request that names the exact commit they are landing.
118 if role.AtLeast(acl.Admin) {
119 continue
120 }
121 if a.RequiredApprovals() <= 0 {
122 problems = append(problems, fmt.Sprintf(
123 "%s is protected and this repository requires approvals_required=0, so only admins may push it (role: %s)",
124 c.ref, role))
125 continue
126 }
127 reqs, err := requests.Load(g, issues.Ref)
128 if err != nil {
129 problems = append(problems, fmt.Sprintf(
130 "%s: cannot read change requests, refusing the push: %v", c.ref, err))
131 continue
132 }
133 branch := strings.TrimPrefix(c.ref, "refs/heads/")
134 m, err := requests.FindApprovedMatch(g, reqs, requests.ApprovalRequirement{
135 Branch: branch,
136 NewTip: c.new,
137 OldTip: c.old,
138 Required: a.RequiredApprovals(),
139 })
140 if err != nil {
141 problems = append(problems, fmt.Sprintf("%s: cannot evaluate approvals: %v", c.ref, err))
142 continue
143 }
144 if m == nil {
145 problems = append(problems, fmt.Sprintf(
146 "%s is protected: %s needs %d approval(s) on an open change request whose head is exactly %s. "+
147 "Add requests/<ulid>/meta.json with base=%q and head=%s, then have an approver commit approvals/<their-name>.json naming that same head.",
148 c.ref, actor, a.RequiredApprovals(), short(c.new), branch, short(c.new)))
149 continue
150 }
151 auditLine(cfg.AuditPath, fmt.Sprintf("merge %s via request %s (head %s) approved by %s",
152 c.ref, m.Request.ID, short(m.Request.Head), strings.Join(m.Approvers, ",")))
119 153 }
120 154 // The issues branch is user-authored content, so it is validated at the
121 155 // boundary: bad data never reaches the tree the web reads.
@@ -123,6 +157,15 @@ func PreReceive(cfg Config) int {
123 157 for _, prob := range issues.Validate(g, c.new) {
124 158 problems = append(problems, "issues: "+prob)
125 159 }
160 for _, prob := range requests.Validate(g, c.new) {
161 problems = append(problems, "requests: "+prob)
162 }
163 // An approval is a statement about the actor's own review, so the file
164 // name must be the actor's name. Without this, anyone with write access
165 // could commit approvals/<someone-else>.json and approve on their behalf.
166 for _, prob := range impostorApprovals(g, c, actor) {
167 problems = append(problems, prob)
168 }
126 169 }
127 170 if cfg.MaxBlobBytes > 0 && c.new != gitx.ZeroSHA {
128 171 overs, err := oversizedBlobs(g, c, cfg.MaxBlobBytes)
@@ -182,6 +225,43 @@ func readChanges(r io.Reader) ([]change, error) {
182 225 return out, sc.Err()
183 226 }
184 227
228 // impostorApprovals reports approvals added in this push that name someone other
229 // than the pusher.
230 func impostorApprovals(g *gitx.Repo, c change, actor string) []string {
231 // On the first push the branch has no old commit, so every approval in the
232 // tree is new and must be checked. Skipping that would let whoever creates the
233 // branch seed approvals in other people's names.
234 added := []string{}
235 if c.old == "" || c.old == gitx.ZeroSHA {
236 blobs, err := g.LsTreeR(c.new)
237 if err != nil {
238 return []string{fmt.Sprintf("requests: cannot verify approvals in this push: %v", err)}
239 }
240 for _, b := range blobs {
241 add := requests.ApprovalPath(b.Name)
242 if add != "" {
243 added = append(added, b.Name)
244 }
245 }
246 } else {
247 var err error
248 added, err = g.AddedPaths(c.old, c.new)
249 if err != nil {
250 return []string{fmt.Sprintf("requests: cannot verify approvals in this push: %v", err)}
251 }
252 }
253 var problems []string
254 for _, p := range added {
255 m := requests.ApprovalPath(p)
256 if m == "" || m == actor {
257 continue
258 }
259 problems = append(problems, fmt.Sprintf(
260 "requests: %s: %s may not add an approval in %s's name; approvals must be committed by the approver", p, actor, m))
261 }
262 return problems
263 }
264
185 265 // auditLine is best-effort: failing to write the log must not change policy.
186 266 func auditLine(path, line string) {
187 267 if path == "" {