Create/import: logged-in users bounded to their own username or their orgs as owner (no arbitrary owner strings) #346

Closed
opened 2026-09-11 18:47:31 +00:00 by crueber · 3 comments
Owner

⚠️ Sequencing: #348 must land first

This ticket depends on #348 (organizations: create-org flow, public org profile, management). The "member org" half of the owner-binding rule below has no meaning until organizations can actually be created and joined through the UI — today an org can only come into existence via the API (POST /api/v1/orgs), there is no create-org UI, no org profile, and no member-management discoverability (#348 covers all of that). Working this ticket before #348 would mean implementing and testing org-owner resolution against orgs that users cannot create or manage.

Implementation order: #348 → #346 → #347 (the push guardrail in #347 reuses this ticket's admission helper, so #346 lands before #347).


What's requested

A logged-in user creating or importing a repo must be bounded to:

  • their own username as the owner, or
  • an organization they are a member of (org-owner enough for v1).

No arbitrary owner strings.

Current state (code evidence) — the guardrail is currently MISSING

No arbitrary owner strings.

B. Push ownership guardrail. A push over SSH (and the HTTP equivalent) must only succeed when the SSH key belongs to a user who is the repo's owner, or a user attached to the owning org (member/owner per bindings), or has an explicit role binding on that repo. Host-wide "write" must not be the gate.

Current state (code evidence) — both guardrails are currently MISSING

A. Creation/import accepts any owner the caller names:

  • repoimport.checkCreate (internal/repoimport/service.go:496-515): authenticated + host-wide p.Write passes before any owner/org consultation — with the identity surface present it calls s.roles.CheckRole(ctx, owner, repo, p, RoleWrite), but…
  • CheckRole → Resolve (internal/identity/access.go:215-250) — for a repo that doesn't exist yet there is no access.json, so SynthesizeDefault(owner) runs, and the fallback at :238-244 grants RoleWrite to any authenticated principal holding host-wide write, for ANY owner string. Nothing checks owner == p.Name or isOrgOwner(owner, p.Name).
  • Same shape on the explicit-create path (PUT …/api create → checkCreate family); no owner-vs-principal check anywhere in internal/api/placeholder.go or the create gate (internal/identity/creategate.go).
  • The UI makes this trivially reachable: the new-repo and import forms are free-text owner fields (New.jsx, Import.jsx), and New.jsx defaults the owner from the current user but allows anything.
  • Net effect: any user with host-wide write can create/import repos under someone else's username or any org string, squatting the namespace.

B. SSH push gating is host-wide, not repo-scoped:

  • SSH auth resolves a key → principal → flags: PrincipalForName (internal/server/auth.go:907-947) returns host-wide Write/Admin flags (in token mode: the aggregate of the principal's static tokens; in oidc: the browser-login flags).
  • The exec gate (internal/sshd/sshd.go:334-339) checks only p.Write — a host-wide boolean stamped from the key's permissions (sshd.go:275, perms.Extensions["write"]) — before dispatching git-receive-pack.
  • SSHReceivePack (internal/server/bind_ssh.go:91-103) then deliberately leaves Write/Admin unset on the principal (documented at :97-102) and never re-checks repo-scoped access — so any key whose principal has host-wide write can push to ANY repo, including repos owned by other users/orgs.
  • HTTP push (smart.go receive-pack) gates on requireRead-class checks + AnonymousRead — verify per #345's changes whether an authenticated non-collaborator can push there too; the fix should cover both transports with one repo-scoped check.

Proposed design

  1. A single ownership/admission rule, applied at both gates. Define one helper on the identity surface — e.g. CheckCreateAccess(ctx, owner, principal) — returning OK iff:
    • owner == normPrincipal(principal.Name) (own username), or
    • isOrgOwner(owner, principal.Name) or the principal is an org member (v1: member may create under the org; tighten to owner-only if preferred — decision point), or
    • host admin.
      Call it from repoimport.checkCreate and the repo-create path before any namespace write (before allocNum/manifest create), failing 403 with a message naming the allowed owners ("owner must be alice or one of alice's orgs").
  2. Push guardrail: at receive-pack dispatch (SSH exec gate and the HTTP receive-pack handler), resolve the principal's role on the target repo via the existing Resolve/CheckRole — with the #345 visibility semantics and the create-admission rule for auto-create-on-push (a push creating a new repo must satisfy guardrail A for the repo's owner path). The host-wide p.Write flag can remain as a coarse pre-check but must not be the final authority. Note SSHReceivePack's documented constraint (:97-102: Write/Admin deliberately unset) — the repo-scoped check must therefore run before that principal is constructed, using the resolved principal name.
  3. Auto-create-on-push interaction: AutoCreateOnPush currently lets any authorized push create a repo under any owner prefix — the same ownership rule must gate the owner segment of the pushed path (the pusher can only auto-create under their own name or their orgs).
  4. UI honesty: the new-repo and import forms should offer an owner dropdown (own username + member orgs) instead of free text — mirrors the #328 dropdown direction and prevents the 403 as a surprise. Minimal version: validate client-side against the same rule and show the allowed owners.
  5. Existing-repos note: this rule applies at creation/push; already-existing repos under mismatched owners keep their access.json bindings (no migration), but the audit (#331) should note any current namespace squatting.

Acceptance criteria

  • A logged-in user creating/importing a repo with an owner that is neither their username nor an org they belong to → 403 naming the allowed owners; the repo is not created (no counter allocation, no manifest, no partial state).
  • Same rule enforced on auto-create-on-push: a push to <otheruser>/newrepo fails; <self>/newrepo and <own-org>/newrepo succeed (member-policy decision documented).
  • SSH push to an existing repo requires the pushing principal to hold write on THAT repo (binding, org, or owner) — a host-wide-write-only principal without a repo relationship is rejected on other users' repos.
  • HTTP receive-pack enforces the same repo-scoped check (both transports share the rule).
  • Host admins bypass (documented).
  • UI: owner fields on new-repo/import become dropdowns (username + member orgs) or validated free text with the same rule client-side.
  • Tests: creation matrix (self / member-org / non-member-org / other-user / anonymous × import, explicit create, auto-create-push) and push matrix (SSH + HTTP, owner / org member / host-write-only outsider / anonymous).
  • The spec docs (01_identity_permissions.md §matrix, 10_git_import.md auth section, 17_ssh.md §3) are amended in the same change — this closes a documented-gap class, so the docs must not keep describing the looser behavior.
## ⚠️ Sequencing: #348 must land first **This ticket depends on #348 (organizations: create-org flow, public org profile, management).** The "member org" half of the owner-binding rule below has no meaning until organizations can actually be **created and joined through the UI** — today an org can only come into existence via the API (`POST /api/v1/orgs`), there is no create-org UI, no org profile, and no member-management discoverability (#348 covers all of that). Working this ticket before #348 would mean implementing and testing org-owner resolution against orgs that users cannot create or manage. Implementation order: **#348 → #346 → #347** (the push guardrail in #347 reuses this ticket's admission helper, so #346 lands before #347). --- ## What's requested A logged-in user creating or importing a repo must be bounded to: - their **own username** as the owner, or - an **organization they are a member of** (org-owner enough for v1). No arbitrary owner strings. ## Current state (code evidence) — the guardrail is currently MISSING No arbitrary owner strings. **B. Push ownership guardrail.** A push over SSH (and the HTTP equivalent) must only succeed when the SSH key belongs to a user who is the repo's owner, or a user attached to the owning org (member/owner per bindings), or has an explicit role binding on that repo. Host-wide "write" must not be the gate. ## Current state (code evidence) — both guardrails are currently MISSING **A. Creation/import accepts any owner the caller names:** - `repoimport.checkCreate` (`internal/repoimport/service.go:496-515`): authenticated + host-wide `p.Write` passes **before any owner/org consultation** — with the identity surface present it calls `s.roles.CheckRole(ctx, owner, repo, p, RoleWrite)`, but… - `CheckRole` → `Resolve` (`internal/identity/access.go:215-250`) — for a repo that doesn't exist yet there is no `access.json`, so `SynthesizeDefault(owner)` runs, and the fallback at :238-244 grants `RoleWrite` to **any authenticated principal holding host-wide write, for ANY owner string**. Nothing checks `owner == p.Name` or `isOrgOwner(owner, p.Name)`. - Same shape on the explicit-create path (`PUT …/api` create → `checkCreate` family); no owner-vs-principal check anywhere in `internal/api/placeholder.go` or the create gate (`internal/identity/creategate.go`). - The UI makes this trivially reachable: the new-repo and import forms are free-text owner fields (`New.jsx`, `Import.jsx`), and `New.jsx` defaults the owner from the current user but allows anything. - Net effect: any user with host-wide write can create/import repos under **someone else's username or any org string**, squatting the namespace. **B. SSH push gating is host-wide, not repo-scoped:** - SSH auth resolves a key → principal → flags: `PrincipalForName` (`internal/server/auth.go:907-947`) returns host-wide `Write`/`Admin` flags (in token mode: the aggregate of the principal's static tokens; in oidc: the browser-login flags). - The exec gate (`internal/sshd/sshd.go:334-339`) checks **only `p.Write`** — a host-wide boolean stamped from the key's permissions (`sshd.go:275`, `perms.Extensions["write"]`) — before dispatching `git-receive-pack`. - `SSHReceivePack` (`internal/server/bind_ssh.go:91-103`) then deliberately leaves Write/Admin unset on the principal (documented at :97-102) and never re-checks repo-scoped access — so **any key whose principal has host-wide write can push to ANY repo**, including repos owned by other users/orgs. - HTTP push (`smart.go` receive-pack) gates on `requireRead`-class checks + `AnonymousRead` — verify per #345's changes whether an authenticated non-collaborator can push there too; the fix should cover both transports with one repo-scoped check. ## Proposed design 1. **A single ownership/admission rule, applied at both gates.** Define one helper on the identity surface — e.g. `CheckCreateAccess(ctx, owner, principal)` — returning OK iff: - `owner == normPrincipal(principal.Name)` (own username), or - `isOrgOwner(owner, principal.Name)` or the principal is an org **member** (v1: member may create under the org; tighten to owner-only if preferred — decision point), or - host admin. Call it from `repoimport.checkCreate` and the repo-create path **before** any namespace write (before `allocNum`/manifest create), failing 403 with a message naming the allowed owners ("owner must be `alice` or one of alice's orgs"). 2. **Push guardrail:** at receive-pack dispatch (SSH exec gate and the HTTP receive-pack handler), resolve the principal's role **on the target repo** via the existing `Resolve`/`CheckRole` — with the #345 visibility semantics and the create-admission rule for auto-create-on-push (a push creating a new repo must satisfy guardrail A for the repo's owner path). The host-wide `p.Write` flag can remain as a coarse pre-check but must not be the final authority. Note `SSHReceivePack`'s documented constraint (:97-102: Write/Admin deliberately unset) — the repo-scoped check must therefore run **before** that principal is constructed, using the resolved principal name. 3. **Auto-create-on-push interaction:** `AutoCreateOnPush` currently lets any authorized push create a repo under any owner prefix — the same ownership rule must gate the owner segment of the pushed path (the pusher can only auto-create under their own name or their orgs). 4. **UI honesty:** the new-repo and import forms should offer an owner dropdown (own username + member orgs) instead of free text — mirrors the #328 dropdown direction and prevents the 403 as a surprise. Minimal version: validate client-side against the same rule and show the allowed owners. 5. **Existing-repos note:** this rule applies at creation/push; already-existing repos under mismatched owners keep their access.json bindings (no migration), but the audit (#331) should note any current namespace squatting. ## Acceptance criteria - [ ] A logged-in user creating/importing a repo with an owner that is neither their username nor an org they belong to → 403 naming the allowed owners; the repo is not created (no counter allocation, no manifest, no partial state). - [ ] Same rule enforced on auto-create-on-push: a push to `<otheruser>/newrepo` fails; `<self>/newrepo` and `<own-org>/newrepo` succeed (member-policy decision documented). - [ ] SSH push to an existing repo requires the pushing principal to hold write on THAT repo (binding, org, or owner) — a host-wide-write-only principal without a repo relationship is rejected on other users' repos. - [ ] HTTP receive-pack enforces the same repo-scoped check (both transports share the rule). - [ ] Host admins bypass (documented). - [ ] UI: owner fields on new-repo/import become dropdowns (username + member orgs) or validated free text with the same rule client-side. - [ ] Tests: creation matrix (self / member-org / non-member-org / other-user / anonymous × import, explicit create, auto-create-push) and push matrix (SSH + HTTP, owner / org member / host-write-only outsider / anonymous). - [ ] The spec docs (01_identity_permissions.md §matrix, 10_git_import.md auth section, 17_ssh.md §3) are amended in the same change — this closes a documented-gap class, so the docs must not keep describing the looser behavior.
crueber added this to the v1 milestone 2026-09-11 18:47:31 +00:00
Author
Owner

Fix ready for review: #356 (branch fix/issue-346 onto main, no conflicts). Scope is creation/import admission only — the shared CheckCreateOwner helper carries a documented reuse contract for #347 (push guardrails + auto-create-on-push reuse it verbatim); 17_ssh.md deliberately untouched. Admin decision: global host admins bypass (documented in 01 §5.2 + helper comment). Do NOT merge per workspace instructions.

Fix ready for review: #356 (branch fix/issue-346 onto main, no conflicts). Scope is creation/import admission only — the shared CheckCreateOwner helper carries a documented reuse contract for #347 (push guardrails + auto-create-on-push reuse it verbatim); 17_ssh.md deliberately untouched. Admin decision: global host admins bypass (documented in 01 §5.2 + helper comment). Do NOT merge per workspace instructions.
Author
Owner

REVIEW PR #356 (fix/issue-346, head 154e8dd after 1 review fix) — security-gate review vs #346.

VERDICT: ready to merge (after the 1 fix I pushed directly; re-verified below).

WHAT I CHECKED (scratch worktree /tmp/pr356, removed afterward; main left untouched, still clean on 222e3f7; no docker/browser/live-instance per instructions — no browser drive: all gates are server-side + headless-tested, UI is cosmetic-on-top):

  1. DENY-BEFORE-WRITE — PASS on all 4 doors:
  • PUT twin internal/api/summary.go:repoPut calls checkCreateOwner BEFORE Repos.Create (no counter/manifest on deny).
  • POST twin internal/api/placeholder.go:post same ordering.
  • Import internal/repoimport/service.go:checkCreate calls roles.CheckCreateOwner BEFORE join-or-start; TestBeginOwnerDenyNoPartialState pins running/streams empty + 403 naming allowed owners.
  • Mirror twin internal/mirror/http.go:createFromURL checks hook BEFORE CreateRepo; ownerbind_test pins reg.Open fails after deny. Minor: admission runs before ParseRepoId validation (foreign+malformed owner yields 403 not 400) — acceptable, deny-first is the safe order.
  1. RULE — PASS: CheckCreateOwner (internal/identity/creategate.go) = anonymous 401 / admin bypass / self (normPrincipal case-fold, identity.go:133) / member-any-role, else 403 naming allowed owners. Admin bypass justified (operator repair; auth-none anon carries Admin so zero-config preserved; pinned by test incl. auth.None()). No enumeration leak: the 403 names the CALLER's own username+orgs (MemberOrgs over caller), never the target org's roster — nothing about target existence leaks. Anonymous 401 is a real 401 (law 9).

  2. PROBE ERRORS 503, NO FAIL-OPEN — PASS: getMembers error -> ErrUnavailable -> 503 on all doors (identity/repoimport/api/mirror tests each pin it). ownerDenyMessage MemberOrgs failure degrades the MESSAGE to generic shape, deny stands — correct (deny never depends on enumeration).

  3. SEAM REPLACEMENT — PASS w/ 1 fix: OrgGate/IsOrgMember types fully deleted, zero non-test callers (grep). Pre-1.0 no-alias rule holds: no shims, composition rewired (cmd/walhub/collab.go: CreateOwnerGate + mirror hook). FIX I PUSHED (154e8dd): docs/go/14_extensibility.md mirror amendment still said 'ReadGate/OrgGate shape' -> now 'ReadGate/CreateOwnerGate'. Remaining OrgGate/IsOrgMember mentions are intentional history (01 §decisions records the supersede) + two test func names (TestPutPlaceholderOrgGate/TestPostReposOrgGate, placeholder_test.go:204,410) — names only, suggest rename on touch, not a blocker.

  4. #347 REUSE CONTRACT — PASS: documented on CheckCreateOwner (creategate.go) — push auto-create reuses verbatim, no fork; 17_ssh.md deliberately untouched. #346 acceptance items 2-4 (auto-create-push, SSH/HTTP repo-scoped push) are NOT met here — correctly deferred to #347, PR states it. Merge of THIS pr must not be read as closing the push half.

  5. UI DROPDOWNS — PASS (server is real gate): New.jsx/Import.jsx select over allowedOwners (orgs.js: [self,...memberOrgs] sorted, fail->self-only); ?owner= preset clamps to self; curl with arbitrary owner still 403 (pinned api-side, not client-side). Admin-foreign-namespace via API documented.

  6. RE-SEEDED TESTS — NOT WEAKENED: TestIsOrgMember deletion is the intended behavior change (unclaimed-prefix legacy-open -> 403, docs mark #210 gate SUPERSEDED). FakeRoles Orgs==nil legacy-allow is test-fake-only, production has no such path. All matrices assert the NEW deny (incl. 'unclaimed foreign prefix' 403).

  7. AUTO-CREATE-ON-PUSH — explicitly #347 scope; this PR leaves push exactly as wide as before (no wider, no narrower). Tracked, not a hole introduced here.

  8. TESTS/COVERAGE — PASS, all -race green in scratch worktree: identity 97.2%, repoimport 95.9%, api 95.3%, mirror 96.8% (all >=95%); go build ./... ok; gofmt/vet clean. Node: orgs.test.js 7/7 (incl. 2 new allowedOwners); full web suite 692/695 with the SAME 3 dist-dependent failures as origin/main baseline (690/693 there — delta is exactly the +2 new passes; PR text says '13 pre-existing' but measured 3 in this env — count discrepancy only, baseline-identical holds).

  9. DOCS — PASS: 01 §5.2 rewritten w/ rule+cost+supersede note, 10/11/07_api/12_web_ui/14_extensibility amended in-change (law 12); decisions appended, not silently overridden.

No browser drive (workspace rule cited in PR; module scripts reuse in-file Show/For/select patterns; no new deps).

MERGE RECOMMENDATION: ready to merge.

REVIEW PR #356 (fix/issue-346, head 154e8dd after 1 review fix) — security-gate review vs #346. VERDICT: ready to merge (after the 1 fix I pushed directly; re-verified below). WHAT I CHECKED (scratch worktree /tmp/pr356, removed afterward; main left untouched, still clean on 222e3f7; no docker/browser/live-instance per instructions — no browser drive: all gates are server-side + headless-tested, UI is cosmetic-on-top): 1. DENY-BEFORE-WRITE — PASS on all 4 doors: - PUT twin internal/api/summary.go:repoPut calls checkCreateOwner BEFORE Repos.Create (no counter/manifest on deny). - POST twin internal/api/placeholder.go:post same ordering. - Import internal/repoimport/service.go:checkCreate calls roles.CheckCreateOwner BEFORE join-or-start; TestBeginOwnerDenyNoPartialState pins running/streams empty + 403 naming allowed owners. - Mirror twin internal/mirror/http.go:createFromURL checks hook BEFORE CreateRepo; ownerbind_test pins reg.Open fails after deny. Minor: admission runs before ParseRepoId validation (foreign+malformed owner yields 403 not 400) — acceptable, deny-first is the safe order. 2. RULE — PASS: CheckCreateOwner (internal/identity/creategate.go) = anonymous 401 / admin bypass / self (normPrincipal case-fold, identity.go:133) / member-any-role, else 403 naming allowed owners. Admin bypass justified (operator repair; auth-none anon carries Admin so zero-config preserved; pinned by test incl. auth.None()). No enumeration leak: the 403 names the CALLER's own username+orgs (MemberOrgs over caller), never the target org's roster — nothing about target existence leaks. Anonymous 401 is a real 401 (law 9). 3. PROBE ERRORS 503, NO FAIL-OPEN — PASS: getMembers error -> ErrUnavailable -> 503 on all doors (identity/repoimport/api/mirror tests each pin it). ownerDenyMessage MemberOrgs failure degrades the MESSAGE to generic shape, deny stands — correct (deny never depends on enumeration). 4. SEAM REPLACEMENT — PASS w/ 1 fix: OrgGate/IsOrgMember types fully deleted, zero non-test callers (grep). Pre-1.0 no-alias rule holds: no shims, composition rewired (cmd/walhub/collab.go: CreateOwnerGate + mirror hook). FIX I PUSHED (154e8dd): docs/go/14_extensibility.md mirror amendment still said 'ReadGate/OrgGate shape' -> now 'ReadGate/CreateOwnerGate'. Remaining OrgGate/IsOrgMember mentions are intentional history (01 §decisions records the supersede) + two test func names (TestPutPlaceholderOrgGate/TestPostReposOrgGate, placeholder_test.go:204,410) — names only, suggest rename on touch, not a blocker. 5. #347 REUSE CONTRACT — PASS: documented on CheckCreateOwner (creategate.go) — push auto-create reuses verbatim, no fork; 17_ssh.md deliberately untouched. #346 acceptance items 2-4 (auto-create-push, SSH/HTTP repo-scoped push) are NOT met here — correctly deferred to #347, PR states it. Merge of THIS pr must not be read as closing the push half. 6. UI DROPDOWNS — PASS (server is real gate): New.jsx/Import.jsx select over allowedOwners (orgs.js: [self,...memberOrgs] sorted, fail->self-only); ?owner= preset clamps to self; curl with arbitrary owner still 403 (pinned api-side, not client-side). Admin-foreign-namespace via API documented. 7. RE-SEEDED TESTS — NOT WEAKENED: TestIsOrgMember deletion is the intended behavior change (unclaimed-prefix legacy-open -> 403, docs mark #210 gate SUPERSEDED). FakeRoles Orgs==nil legacy-allow is test-fake-only, production has no such path. All matrices assert the NEW deny (incl. 'unclaimed foreign prefix' 403). 8. AUTO-CREATE-ON-PUSH — explicitly #347 scope; this PR leaves push exactly as wide as before (no wider, no narrower). Tracked, not a hole introduced here. 9. TESTS/COVERAGE — PASS, all -race green in scratch worktree: identity 97.2%, repoimport 95.9%, api 95.3%, mirror 96.8% (all >=95%); go build ./... ok; gofmt/vet clean. Node: orgs.test.js 7/7 (incl. 2 new allowedOwners); full web suite 692/695 with the SAME 3 dist-dependent failures as origin/main baseline (690/693 there — delta is exactly the +2 new passes; PR text says '13 pre-existing' but measured 3 in this env — count discrepancy only, baseline-identical holds). 10. DOCS — PASS: 01 §5.2 rewritten w/ rule+cost+supersede note, 10/11/07_api/12_web_ui/14_extensibility amended in-change (law 12); decisions appended, not silently overridden. No browser drive (workspace rule cited in PR; module scripts reuse in-file Show/For/select patterns; no new deps). MERGE RECOMMENDATION: ready to merge.
Author
Owner

Fixed by PR #356 (review clean + one stale doc reference fix by reviewer; deny-before-write on all 4 doors, no leaks, seam replacement clean, #347 contract ready), merged. Closing.

Fixed by PR #356 (review clean + one stale doc reference fix by reviewer; deny-before-write on all 4 doors, no leaks, seam replacement clean, #347 contract ready), merged. Closing.
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
crueber/walhub#346
No description provided.