gabriel / musehub public
Open #131 Bug
filed by gabriel human · 55 days ago

Wire-protocol push has no repo-specific write-authorization check -- plus a general sweep

0 Anchors
Blast radius
Churn 30d
0 Proposals

Wire-protocol push has no repo-specific write-authorization check — plus a general sweep for the same class of bug

Background

While verifying aaronrene's collaborator access (write permission just granted on muse and musehub), traced the actual wire-protocol push handler end to end and found it has no repo-specific authorization check at all — a materially different, more serious problem than anything being checked at the time.

Confirmed, traced in code — not assumed

musehub/api/routes/wire.py::push_unpack_mpack (the real push handler) depends only on:

claims: TokenClaims = Depends(require_valid_token),

require_valid_token (= require_signed_request, musehub/auth/request_signing.py:289) verifies only that the request carries a valid MSign signature from some registered identity — it has no concept of which repo is being written to.

pusher_id/claims.handle is passed into the service function (wire_push_unpack_mpack, musehub/services/musehub_wire_push.py:539) but is never referenced again anywhere else in that 1707-line file — accepted as a parameter, never checked against anything.

There is no _assert_writable-style helper anywhere in wire.py — only _assert_readable (line 90), used for fetch/read paths. _guard_write_access — the exact per-repo collaborator/owner check musehub/api/routes/musehub/ proposals.py:116 and issues.py correctly call before allowing a write — appears in zero wire-protocol routes (confirmed via grep -rln _guard_write_access musehub/).

In plain terms: any authenticated MuseHub identity can currently push to any repo, public or private, regardless of collaborator status. This is unrelated to the collaborator-invite work that surfaced it — collaborator invites are still correct and necessary for MP creation and other REST-gated actions (which do call _guard_write_access correctly); push simply doesn't do what anyone would reasonably assume.

Why a sweep, not just a point fix

The REST entity routes (issues.py, proposals.py, labels.py, releases.py, repos.py) went through an explicit auth-hardening pass (Phase 3 of the "Quality Roadmap," 2026-04-07 — see ~/ecosystem/ QUALITY_ROADMAP.md) that added require_scope/_guard_write_access enforcement everywhere. wire.py is a separate, older, custom binary protocol router — it's plausible it was simply never included in that sweep, which raises the real question: are there other routers/endpoints with the same gap? This ticket exists to answer that definitively, not just patch the one confirmed hole.

Goal

  1. Fix the confirmed gap: push_unpack_mpack (and any sibling wire-protocol write endpoint — repair-object/snapshot/commit, force-push, tag push) enforces the same per-repo collaborator/owner check as the REST routes.
  2. Sweep every write-capable route in the codebase (grep for @router.post/ put/patch/delete across all of musehub/api/routes/) and confirm each one either calls _guard_write_access/require_scope appropriately, or is deliberately public-write by design (rare — document any such case explicitly, don't just assume).
  3. A regression test proves the wire push gap specifically: a collaborator with no write access (or no collaborator record at all) attempting to push must be rejected — today it would succeed.

Out of Scope

  • Redesigning the wire protocol's binary format or transport — this is an authorization gap, not a protocol design issue.
  • The org/quorum audit (musehub issue #130) — related in spirit (both are "is our permission model actually complete") but that's the CLI/ functionality-parity angle; this ticket is specifically about finding and closing authorization holes.

Phases

Phase 1 — Red: prove the gap with a real test

  • AUTHZ_01 — A registered identity with no collaborator record on a private repo successfully pushes via push_unpack_mpack today. Must pass (i.e., prove the vulnerability) against current code, then be inverted to a rejection assertion once fixed.
  • AUTHZ_02 — Same setup for a public repo — confirm whether public repos are even more exposed (anyone can push, not just anyone with an account) or whether some other check applies there.

Deliverable: the gap is proven with a real test, not just static analysis, before any fix lands.

Phase 2 — Fix the confirmed wire-protocol gap

  • Add a per-repo write-authorization check to push_unpack_mpack and every other wire-protocol write endpoint (repair-object/snapshot/ commit already partially check this per the earlier grep — verify those are actually correct and consistent, not just present).
  • AUTHZ_01/02 now correctly reject unauthorized pushes.

Deliverable: wire-protocol push is provably no longer bypassable.

Phase 3 — Full sweep

  • Enumerate every write route across musehub/api/routes/ (REST + wire + coord + mcp write-tools) and record, per route: does it check collaborator/owner access, does it use require_scope, or is it deliberately open? Produce this as a table in this issue, not just a pass/fail — a route relying on require_scope alone (recall: require_scope passes humans unconditionally, it's an agent-capability gate, not a repo-ownership gate) needs a second look, not just a checkbox.
  • Any additional gaps found get their own fix + test, following the same Phase 1→Phase 2 pattern used here.

Deliverable: a complete, documented map of every write route's actual authorization behavior — no more "plausible, unverified" routes.


Acceptance Criteria

  • push_unpack_mpack (and siblings) reject a push from a non-collaborator.
  • Every write route in musehub/api/routes/ has a documented, verified authorization story — not assumed correct because it "probably" calls the right dependency.
  • Full regression suite green; no privilege-escalation regressions introduced by the fix itself (e.g. don't accidentally lock out legitimate owners).

Implementation Order

Phase 1 → Phase 2 → Phase 3. Fix the confirmed, live gap first (it's on staging right now); the broader sweep comes after the acute issue is closed.

Activity
gabriel opened this issue 55 days ago
No activity yet. Use the CLI to comment.