Create/import: logged-in users bounded to their own username or their orgs as owner (no arbitrary owner strings) #346
Labels
No labels
actions
bug
cli
duplicate
enhancement
fork
forum
git storage
help wanted
insights
invalid
issues
moderation
oidc
ownership transfer
packages
pr/merge protection rules
projects
pull requests
question
releases
sponsorships
tags
webhooks
wiki
wontfix
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
crueber/walhub#346
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
⚠️ 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:
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-widep.Writepasses before any owner/org consultation — with the identity surface present it callss.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 noaccess.json, soSynthesizeDefault(owner)runs, and the fallback at :238-244 grantsRoleWriteto any authenticated principal holding host-wide write, for ANY owner string. Nothing checksowner == p.NameorisOrgOwner(owner, p.Name).PUT …/apicreate →checkCreatefamily); no owner-vs-principal check anywhere ininternal/api/placeholder.goor the create gate (internal/identity/creategate.go).New.jsx,Import.jsx), andNew.jsxdefaults the owner from the current user but allows anything.B. SSH push gating is host-wide, not repo-scoped:
PrincipalForName(internal/server/auth.go:907-947) returns host-wideWrite/Adminflags (in token mode: the aggregate of the principal's static tokens; in oidc: the browser-login flags).internal/sshd/sshd.go:334-339) checks onlyp.Write— a host-wide boolean stamped from the key's permissions (sshd.go:275,perms.Extensions["write"]) — before dispatchinggit-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.smart.goreceive-pack) gates onrequireRead-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
CheckCreateAccess(ctx, owner, principal)— returning OK iff:owner == normPrincipal(principal.Name)(own username), orisOrgOwner(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), orCall it from
repoimport.checkCreateand the repo-create path before any namespace write (beforeallocNum/manifest create), failing 403 with a message naming the allowed owners ("owner must bealiceor one of alice's orgs").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-widep.Writeflag can remain as a coarse pre-check but must not be the final authority. NoteSSHReceivePack'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.AutoCreateOnPushcurrently 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).Acceptance criteria
<otheruser>/newrepofails;<self>/newrepoand<own-org>/newreposucceed (member-policy decision documented).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.
REVIEW PR #356 (fix/issue-346, head
154e8ddafter 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):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).
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).
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.#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.
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.
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).
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.
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).
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.
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.