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 2: a comment is editable, by the person who wrote it
79c88d72d36d8974dca5859125f8f37f0b06c1f7 · Keenan Nemetz · 17d ago · parent 67dd8854
Phase 2: a comment is editable, by the person who wrote it Comment editing was the last open follow-up on Phase 2, and the workaround was "append another comment, or rewrite history and force-push" — a redaction tool used for a typo. An edit is now an ordinary commit that rewrites the comment's file in place: the path stays, so the id, the thread order and one `git log -p -- issues/<id>/comments/<cid>.md` that answers "what did it say" all survive. A delete removes the file and leaves the text in history. No `edited` field, for the same reason `updated` was dropped from issues: it would be a line rewritten on every edit, and the commit that changed the file already carries the time. Editing also does not re-surface anything — the feed question is a timestamp, not a field. Ownership is the interesting part. A comment's author is a name in a file, and a name is not authentication, so `pre-receive` compares the author on the OLD blob against the account the key resolved to. The old blob is the one that was validated when it landed; the new one is whatever the pusher typed, and reading it would let anyone adopt a comment by editing its author line in the same push. Names are never reissued, so a former name still resolves through the roster and an account keeps its own comments across a rename. An admin may override — a thread written by somebody who has left is otherwise unrepairable — and the override is written to data/audit.log, like a force-push or a release rewrite. Two things are deliberately outside the rule: - Adding a comment. A new file's author is a claim, exactly like an issue's `author`, and the boundary has never pretended otherwise. - The issue file itself. `state`, `labels` and `assignees` are shared fields, so closing or assigning somebody else's issue is triage; refusing it would make the fields useless, and it is the same write any writer could already do by hand. A description can therefore be corrected by any writer — unchanged behaviour, now stated. `vole issue edit` (title, body, labels, assignees) covers the issue file, and `issue comment` now prints the comment id it created while `issue show` and the web page print it too — otherwise `edit-comment` would take an id nobody could see. Two comments written in the same millisecond share their ULID timestamp prefix, so a short prefix is ambiguous there; the CLI names both matches instead of guessing. Also fixed on the way: `frontmatter.Value` reads one field without strict decoding. `AuthorOf` first used Decode, whose KnownFields is right for validation but wrong for "who wrote this" — the hook refused every comment edit with `field created not found`. Verified: TestCommentEditsBelongToTheirAuthor pushes as two accounts and drives all four refusals (rewrite, adopt-by-author-line, delete, and the untouched thread afterwards), plus an author editing his own comment, a writer closing someone else's issue, and an admin delete that lands only with its audit line. TestAuthorOfReadsAnyMetadataFile covers the strict-decoder bug directly; TestCommentPathMatchesTheReader pins the writer/validator path agreement; the CLI end-to-end test edits a comment, retitles an issue and deletes the comment through the binary against a real instance. scripts/ci.sh green (gofmt, vet, build, markdown links, go test -race ./...).
internal/hook/hook.go +116
| @@ -276,6 +276,12 @@ func PreReceive(cfg Config) int { | |||
| 276 | 276 | for _, prob := range approvalOwnership(g, c, actor, actorID, role, cfg.AuditPath, who) { | |
| 277 | 277 | problems = append(problems, prob) | |
| 278 | 278 | } | |
| 279 | // A comment is somebody's own words, so changing or deleting it is | ||
| 280 | // theirs too — checked against the author on the *old* blob, which is | ||
| 281 | // the one that was validated when it landed. | ||
| 282 | for _, prob := range commentOwnership(g, c, actor, actorID, role, cfg.AuditPath, cfg.UsersDB, cfg.Out) { | ||
| 283 | problems = append(problems, prob) | ||
| 284 | } | ||
| 279 | 285 | } | |
| 280 | 286 | } | |
| 281 | 287 | ||
| @@ -385,6 +391,116 @@ func approvalOwnership(g *gitx.Repo, c change, actor, actorID string, role acl.R | |||
| 385 | 391 | return problems | |
| 386 | 392 | } | |
| 387 | 393 | ||
| 394 | // commentOwnership enforces that a comment is changed or removed by the account | ||
| 395 | // whose words it is. | ||
| 396 | // | ||
| 397 | // The author of a comment is a name in a file, and a name is not authentication. | ||
| 398 | // What makes this check sound is *which* file it reads: the old blob was | ||
| 399 | // validated when it landed, whereas the new one is whatever the pusher wrote. | ||
| 400 | // Comparing the old author is also what closes the obvious bypass — editing the | ||
| 401 | // author field to your own name and the body in the same push. | ||
| 402 | // | ||
| 403 | // Three things are deliberately not enforced here: | ||
| 404 | // | ||
| 405 | // - Adding a comment is free. A new file's author is a claim, exactly like an | ||
| 406 | // issue's author field, and the push boundary has never pretended otherwise. | ||
| 407 | // - Issue files are not covered. They carry shared state — state, labels, | ||
| 408 | // assignees — so closing or labelling somebody else's issue is normal triage, | ||
| 409 | // and a description can be corrected by any writer, which it already could be. | ||
| 410 | // - A first push has nothing to compare against, so it is skipped. | ||
| 411 | func commentOwnership(g *gitx.Repo, c change, actor, actorID string, role acl.Role, auditPath, usersDB string, out io.Writer) []string { | ||
| 412 | if c.old == "" || c.old == gitx.ZeroSHA { | ||
| 413 | return nil | ||
| 414 | } | ||
| 415 | status, err := g.DiffStatus(c.old, c.new) | ||
| 416 | if err != nil { | ||
| 417 | return []string{fmt.Sprintf("issues: cannot verify comment authorship: %v", err)} | ||
| 418 | } | ||
| 419 | var problems []string | ||
| 420 | // Only open the roster when there is a comment in the push to check. | ||
| 421 | var roster *identity.Store | ||
| 422 | openRoster := func() *identity.Store { | ||
| 423 | if roster != nil || usersDB == "" { | ||
| 424 | return roster | ||
| 425 | } | ||
| 426 | store, err := identity.Open(usersDB) | ||
| 427 | if err != nil { | ||
| 428 | return nil | ||
| 429 | } | ||
| 430 | roster = store | ||
| 431 | return roster | ||
| 432 | } | ||
| 433 | defer func() { | ||
| 434 | if roster != nil { | ||
| 435 | roster.Close() | ||
| 436 | } | ||
| 437 | }() | ||
| 438 | |||
| 439 | for path, st := range status { | ||
| 440 | if !issues.IsCommentPath(path) { | ||
| 441 | continue | ||
| 442 | } | ||
| 443 | // A rename has no blob at the destination, so its author cannot be | ||
| 444 | // established; that is a refusal rather than a pass. | ||
| 445 | if st != "M" && st != "D" && st != "R" { | ||
| 446 | continue | ||
| 447 | } | ||
| 448 | raw, err := g.BlobAt(c.old, path) | ||
| 449 | if err != nil { | ||
| 450 | problems = append(problems, fmt.Sprintf( | ||
| 451 | "issues: %s: cannot establish whose comment this is, refusing the push: %v", path, err)) | ||
| 452 | continue | ||
| 453 | } | ||
| 454 | author, err := issues.AuthorOf(raw) | ||
| 455 | if err != nil { | ||
| 456 | problems = append(problems, fmt.Sprintf( | ||
| 457 | "issues: %s: cannot establish whose comment this is, refusing the push: %v", path, err)) | ||
| 458 | continue | ||
| 459 | } | ||
| 460 | if ownsAuthor(openRoster(), author, actor, actorID) { | ||
| 461 | continue | ||
| 462 | } | ||
| 463 | verb := "change" | ||
| 464 | if st == "D" { | ||
| 465 | verb = "delete" | ||
| 466 | } | ||
| 467 | // An admin may repair a thread — a comment by somebody who has left | ||
| 468 | // cannot be fixed by them — but never silently: the override is audited, | ||
| 469 | // like a force-push and a release rewrite. | ||
| 470 | if role.AtLeast(acl.Admin) { | ||
| 471 | auditLine(auditPath, fmt.Sprintf("comment-%s %s (author %s) by admin %s", st, path, author, actor)) | ||
| 472 | reject(out, "admin override, audited: %s %s %s's comment", actor, verb, author) | ||
| 473 | continue | ||
| 474 | } | ||
| 475 | problems = append(problems, fmt.Sprintf( | ||
| 476 | "issues: %s: only %s can %s this comment; %s is not that account", | ||
| 477 | path, author, verb, actor)) | ||
| 478 | } | ||
| 479 | return problems | ||
| 480 | } | ||
| 481 | |||
| 482 | // ownsAuthor reports whether the name on a file is the pushing account. Names are | ||
| 483 | // never reissued, so a former name still resolves to the same account. | ||
| 484 | // | ||
| 485 | // Without a roster the name the handshake supplied is all there is, and the | ||
| 486 | // comparison fails closed: after a rename that refuses the edit instead of | ||
| 487 | // opening it to anyone. | ||
| 488 | func ownsAuthor(roster *identity.Store, author, actor, actorID string) bool { | ||
| 489 | if roster != nil { | ||
| 490 | if id, ok := roster.ResolveID(author); ok { | ||
| 491 | return id == actorID | ||
| 492 | } | ||
| 493 | // ResolveID covers active names; Find also covers retired ones, so an | ||
| 494 | // account editing a comment it wrote before it renamed or retired still | ||
| 495 | // owns it. | ||
| 496 | if a, err := roster.Find(author); err == nil { | ||
| 497 | return a.ID == actorID | ||
| 498 | } | ||
| 499 | return false | ||
| 500 | } | ||
| 501 | return author == actor | ||
| 502 | } | ||
| 503 | |||
| 388 | 504 | // touchesWorkflows reports whether this push changes anything under the workflow | |
| 389 | 505 | // directory, so repositories without CI pay one diff and no more. | |
| 390 | 506 | func touchesWorkflows(g *gitx.Repo, c change) (bool, error) { | |