Serving a page rewrites the hosted repo: anonymous HTTP triggers ref migration #80

New issue

Closed

opened 2026-08-13 06:37:29 +00:00 by a73x · 0 comments

a73x commented

2026-08-13 06:37:29 +00:00

(Migrated from git.a73x.sh)

state::list_patches calls migrate_patch_layout(repo) (state.rs:1722), and the server calls list_patches on every repo page — repo/mod.rs:59, overview.rs:65, patches.rs:190. So rendering a page writes to the repository being served.

Demonstrated against a live server with one repo holding a single old-layout patch ref, and one unauthenticated request:

GET /old/patches?filter=all        -> 200

before = 205 refs                  after = 207 refs
- refs/collab/patches/46368211…            (bare, old layout)
+ refs/collab/patches/46368211…/events
+ refs/collab/patches/46368211…/rev/04387def…
+ refs/collab/patches/46368211…/rev/4a62be96…

The data survives. The events tip is byte-identical afterwards and the patch reads correctly from a fresh clone, so this is not corruption. The migration is careful and resumable, which is why it has gone unnoticed.

What is wrong is that it happens at all, on a read, from an unauthenticated request, with no lock:

Any visitor mutates the repository. On a public instance that is the internet.
migrate_patch_layout takes no lock. Two concurrent requests can migrate the same repo at once, and a request can migrate while receive-pack is writing.
It contradicts the rule enforced everywhere else in this codebase. Rendering must not write; that constraint has blocked patches all cycle, and the one place it is violated is the highest-stakes one.
On a read-only mount it fails per-patch and is skipped silently, so the server serves a repo it cannot migrate with no diagnostic.

Pre-existing, from 20afe11 — the migration was written for the client, where the caller owns the repo, and the server inherited it by calling the same function.

Fix: the server must not migrate. Options: a read path that tolerates both layouts without writing; or migration behind an explicit git-collab-server migrate an operator runs; or a flag the server sets to make reads non-migrating. Whichever, it needs to hold until e5096ffc strips legacy support, after which the server would simply fail to read an unmigrated repo — which is an argument for the explicit command, so an operator can migrate before upgrading rather than discovering it afterwards.

Found while checking whether upgrading a deployed server would damage existing repos.

Migrated from git-collab issue 5174338fb305c0c0d4b9f7eb5a2ec207c57d00a7.

Original author: a73x dev@a73x.sh.

Close reason: merged in patch 46c3cdc5

`state::list_patches` calls `migrate_patch_layout(repo)` (state.rs:1722), and the server calls `list_patches` on every repo page — `repo/mod.rs:59`, `overview.rs:65`, `patches.rs:190`. So rendering a page **writes to the repository being served**. Demonstrated against a live server with one repo holding a single old-layout patch ref, and one unauthenticated request: ``` GET /old/patches?filter=all -> 200 before = 205 refs after = 207 refs - refs/collab/patches/46368211… (bare, old layout) + refs/collab/patches/46368211…/events + refs/collab/patches/46368211…/rev/04387def… + refs/collab/patches/46368211…/rev/4a62be96… ``` **The data survives.** The events tip is byte-identical afterwards and the patch reads correctly from a fresh clone, so this is not corruption. The migration is careful and resumable, which is why it has gone unnoticed. What is wrong is that it happens **at all, on a read, from an unauthenticated request, with no lock**: - Any visitor mutates the repository. On a public instance that is the internet. - `migrate_patch_layout` takes no lock. Two concurrent requests can migrate the same repo at once, and a request can migrate while `receive-pack` is writing. - It contradicts the rule enforced everywhere else in this codebase. Rendering must not write; that constraint has blocked patches all cycle, and the one place it is violated is the highest-stakes one. - On a read-only mount it fails per-patch and is skipped silently, so the server serves a repo it cannot migrate with no diagnostic. Pre-existing, from 20afe11 — the migration was written for the client, where the caller owns the repo, and the server inherited it by calling the same function. Fix: the server must not migrate. Options: a read path that tolerates both layouts without writing; or migration behind an explicit `git-collab-server migrate` an operator runs; or a flag the server sets to make reads non-migrating. Whichever, it needs to hold until e5096ffc strips legacy support, after which the server would simply fail to read an unmigrated repo — which is an argument for the explicit command, so an operator can migrate before upgrading rather than discovering it afterwards. Found while checking whether upgrading a deployed server would damage existing repos. --- Migrated from git-collab issue `5174338fb305c0c0d4b9f7eb5a2ec207c57d00a7`. Original author: a73x <dev@a73x.sh>. Close reason: merged in patch 46c3cdc5