From da21598f104bd338917abcbbea1bbca4434a7854 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 3 Jul 2026 15:31:17 -0400 Subject: [PATCH] fix(server): bearer-gate checkItemVisible's admin bypass (BUG-1918) (#793) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bearer-authed platform admins who are restricted workspace members can no longer read, update, delete, or export a hidden collection's item by direct ref — checkItemVisible's unconditional admin bypass previously ignored isBearerAuth entirely, letting a bearer admin sidestep BUG-1917's list-level scoping for anyone who could guess a ref. Cookie session admins keep the existing unrestricted web-UI affordance. checkItemVisible gains an isBearer parameter (mirroring the existing authIsBearer idiom in resolverWorkspaceRole / guestResourceFilterCore) and gates its admin bypass on !isBearer. requireItemVisible's own signature is unchanged, so its ~20 call sites (comments, links, stars, versions, timeline, playbooks, backlinks, storage, artifact-export) inherit the fix for free; the three direct checkItemVisible callers (writeItemResolveError, handleBulkItems, resolverItemVisible) are updated explicitly. --- internal/server/handlers_items.go | 2 +- internal/server/handlers_items_bulk.go | 2 +- internal/server/handlers_items_bulk_test.go | 48 +++++ internal/server/handlers_items_test.go | 181 ++++++++++++++++++ internal/server/handlers_ref_resolver.go | 11 +- internal/server/handlers_ref_resolver_test.go | 112 ++++++++++- internal/server/server.go | 23 ++- 7 files changed, 365 insertions(+), 14 deletions(-) diff --git a/internal/server/handlers_items.go b/internal/server/handlers_items.go index b309f3b0..3e0e127a 100644 --- a/internal/server/handlers_items.go +++ b/internal/server/handlers_items.go @@ -727,7 +727,7 @@ func (s *Server) createItemChecked(r *http.Request, workspaceID string, coll *mo // archived item is never revealed to a caller who could not otherwise see it. func (s *Server) writeItemResolveError(w http.ResponseWriter, r *http.Request, workspaceID, ref string) { if archived, err := s.store.ResolveItemIncludeDeleted(workspaceID, ref); err == nil && archived != nil && archived.DeletedAt != nil { - if visible, verr := s.checkItemVisible(workspaceID, archived, currentUser(r), workspaceRole(r)); verr == nil && visible { + if visible, verr := s.checkItemVisible(workspaceID, archived, currentUser(r), workspaceRole(r), isBearerAuth(r)); verr == nil && visible { writeError(w, http.StatusConflict, "archived", fmt.Sprintf("%q is archived. Fetch it read-only with GET, or restore it before editing.", ref)) return diff --git a/internal/server/handlers_items_bulk.go b/internal/server/handlers_items_bulk.go index 9fcab996..b8162a94 100644 --- a/internal/server/handlers_items_bulk.go +++ b/internal/server/handlers_items_bulk.go @@ -207,7 +207,7 @@ func (s *Server) handleBulkItems(w http.ResponseWriter, r *http.Request) { // Per-item visibility gate. Report invisible items as // not-found so a restricted member can't probe existence by ref. - visible, verr := s.checkItemVisible(workspaceID, item, user, role) + visible, verr := s.checkItemVisible(workspaceID, item, user, role, isBearerAuth(r)) if verr != nil { resp.Failed = append(resp.Failed, bulkItemFailure{Ref: ref, Error: verr.Error()}) continue diff --git a/internal/server/handlers_items_bulk_test.go b/internal/server/handlers_items_bulk_test.go index 3dbb01c0..5fb24852 100644 --- a/internal/server/handlers_items_bulk_test.go +++ b/internal/server/handlers_items_bulk_test.go @@ -508,6 +508,54 @@ func TestBulkItems_CollectionMoveValidatesStatusOverride(t *testing.T) { } } +// TestBulkItems_AdminBearer_RestrictedMemberBlockedOnHiddenItem pins +// BUG-1918 for handleBulkItems' own direct checkItemVisible call site +// (the per-ref loop, distinct from the requireItemVisible shim the +// single-item handlers use). Not part of the dispatcher's explicit test +// sweep for this bug, but it's a call site the fix touches directly, so +// it gets its own coverage: a bearer-authed admin who is a restricted +// member (collection_access="specific") gets the hidden-collection item +// reported as a per-ref "not found" failure, same as any other +// restricted member — not silently archived via the old unconditional +// admin bypass — while the visible-collection item still succeeds. +func TestBulkItems_AdminBearer_RestrictedMemberBlockedOnHiddenItem(t *testing.T) { + f := newBearerGateItemFixture(t) + + rr := doRequestWithHeaders(f.srv, "POST", "/api/v1/workspaces/"+f.ws.Slug+"/items/bulk", + map[string]any{"ids": []string{f.hiddenItem.Ref, f.visibleItem.Ref}, "op": "archive"}, + f.bearerHeaders()) + if rr.Code != http.StatusOK { + t.Fatalf("bulk bearer admin: expected 200 envelope, got %d: %s", rr.Code, rr.Body.String()) + } + var resp bulkItemsResponse + parseJSON(t, rr, &resp) + if len(resp.Updated) != 1 || resp.Updated[0].Ref != f.visibleItem.Ref { + t.Fatalf("expected only the visible item archived, got %+v", resp.Updated) + } + if len(resp.Failed) != 1 || resp.Failed[0].Ref != f.hiddenItem.Ref { + t.Fatalf("expected the hidden item to fail as not-found, got %+v", resp.Failed) + } + + // Cookie admin — unrestricted, both succeed. Fresh items: the + // visible-collection item above was already archived by the bearer + // call, and ResolveItem (used for every op but "restore") hides + // archived items, so reusing it here would fail for an unrelated + // reason (already gone) rather than exercising the assertion. + hidden2 := f.newItem(t, f.hiddenCollID, "Hidden item 2") + visible2 := f.newItem(t, f.visibleCollID, "Visible item 2") + rr = doRequestWithCookie(f.srv, "POST", "/api/v1/workspaces/"+f.ws.Slug+"/items/bulk", + map[string]any{"ids": []string{hidden2.Ref, visible2.Ref}, "op": "archive"}, + f.sessionToken) + if rr.Code != http.StatusOK { + t.Fatalf("bulk cookie admin: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + var cookieResp bulkItemsResponse + parseJSON(t, rr, &cookieResp) + if len(cookieResp.Updated) != 2 { + t.Fatalf("expected both items archived for cookie admin, got %+v", cookieResp) + } +} + // TestBulkItems_RouteDoesNotShadowItemSlug guards the route-ordering // fix: /items/bulk is a static segment registered before the // /items/{itemSlug} param route, so it must not be treated as an item diff --git a/internal/server/handlers_items_test.go b/internal/server/handlers_items_test.go index 56bd2741..35210dc5 100644 --- a/internal/server/handlers_items_test.go +++ b/internal/server/handlers_items_test.go @@ -2479,3 +2479,184 @@ func TestCreateItem_AdminBearer_RestrictedMemberIsScoped(t *testing.T) { t.Fatalf("cookie admin create in hidden collection: expected 201, got %d: %s", rr.Code, rr.Body.String()) } } + +// bearerGateItemFixture seeds a platform admin who is also a restricted +// workspace member (collection_access="specific", scoped to "visible" +// only), one item in the "hidden" collection, and one in "visible". +// Shared setup for BUG-1918's single-item bearer-gate tests +// (GET/PATCH/DELETE/export), mirroring +// TestCreateItem_AdminBearer_RestrictedMemberIsScoped's fixture shape. +type bearerGateItemFixture struct { + srv *Server + ws *models.Workspace + admin *models.User + hiddenCollID string + visibleCollID string + hiddenItem *models.Item + visibleItem *models.Item + bearerToken string + sessionToken string +} + +// newItem creates a fresh item in the given (hidden/visible) collection. +// Used by tests that mutate the fixture's default hiddenItem/visibleItem +// (e.g. archiving them) and need an untouched pair for a follow-up +// assertion. +func (f *bearerGateItemFixture) newItem(t *testing.T, collID, title string) *models.Item { + t.Helper() + item, err := f.srv.store.CreateItem(f.ws.ID, collID, models.ItemCreate{ + Title: title, Fields: `{"status":"open"}`, + }) + if err != nil { + t.Fatalf("create item %q: %v", title, err) + } + return item +} + +func newBearerGateItemFixture(t *testing.T) *bearerGateItemFixture { + t.Helper() + srv := testServer(t) + + admin, err := srv.store.CreateUser(models.UserCreate{ + Email: "admin@example.com", Name: "Admin", Password: "correct-horse-battery-staple", Role: "admin", + }) + if err != nil { + t.Fatalf("create admin: %v", err) + } + ws, err := srv.store.CreateWorkspace(models.WorkspaceCreate{Name: "ItemGateBearer", OwnerID: admin.ID}) + if err != nil { + t.Fatalf("create workspace: %v", err) + } + if err := srv.store.AddWorkspaceMember(ws.ID, admin.ID, "editor"); err != nil { + t.Fatalf("add member: %v", err) + } + + schema := `{"fields":[{"key":"status","type":"select","options":["open","done"],"default":"open"}]}` + visible, err := srv.store.CreateCollection(ws.ID, models.CollectionCreate{ + Name: "Visible", Slug: "visible", Prefix: "VIS", Schema: schema, + }) + if err != nil { + t.Fatalf("create visible collection: %v", err) + } + hidden, err := srv.store.CreateCollection(ws.ID, models.CollectionCreate{ + Name: "Hidden", Slug: "hidden", Prefix: "HID", Schema: schema, + }) + if err != nil { + t.Fatalf("create hidden collection: %v", err) + } + visibleItem, err := srv.store.CreateItem(ws.ID, visible.ID, models.ItemCreate{ + Title: "Visible item", Fields: `{"status":"open"}`, + }) + if err != nil { + t.Fatalf("create visible item: %v", err) + } + hiddenItem, err := srv.store.CreateItem(ws.ID, hidden.ID, models.ItemCreate{ + Title: "Hidden item", Fields: `{"status":"open"}`, + }) + if err != nil { + t.Fatalf("create hidden item: %v", err) + } + if err := srv.store.SetMemberCollectionAccess(ws.ID, admin.ID, "specific", []string{visible.ID}); err != nil { + t.Fatalf("set member collection access: %v", err) + } + + tok, err := srv.store.CreateAPIToken(admin.ID, models.APITokenCreate{ + Name: "admin-pat", WorkspaceID: ws.ID, + }, 0, 0) + if err != nil { + t.Fatalf("CreateAPIToken: %v", err) + } + sessTok, err := srv.store.CreateSession(admin.ID, "web-test", "192.0.2.1", "", 24*time.Hour) + if err != nil { + t.Fatalf("CreateSession: %v", err) + } + + return &bearerGateItemFixture{ + srv: srv, ws: ws, admin: admin, + hiddenCollID: hidden.ID, visibleCollID: visible.ID, + hiddenItem: hiddenItem, visibleItem: visibleItem, + bearerToken: tok.Token, sessionToken: sessTok, + } +} + +func (f *bearerGateItemFixture) bearerHeaders() map[string]string { + return map[string]string{"Authorization": "Bearer " + f.bearerToken} +} + +// TestGetItem_AdminBearer_RestrictedMemberBlockedOnHiddenCollection pins +// the read-path side of BUG-1918: checkItemVisible's unconditional +// admin bypass previously let a bearer-authed admin fetch ANY item by +// ref, even one in a collection they can't see per their own +// member_collection_access — bypassing BUG-1917's list-level scoping +// entirely for anyone who can guess (or enumerate) a ref. +func TestGetItem_AdminBearer_RestrictedMemberBlockedOnHiddenCollection(t *testing.T) { + f := newBearerGateItemFixture(t) + + rr := doRequestWithHeaders(f.srv, "GET", "/api/v1/workspaces/"+f.ws.Slug+"/items/"+f.hiddenItem.Slug, nil, f.bearerHeaders()) + if rr.Code != http.StatusNotFound { + t.Fatalf("bearer admin GET hidden item: expected 404, got %d: %s", rr.Code, rr.Body.String()) + } + + rr = doRequestWithHeaders(f.srv, "GET", "/api/v1/workspaces/"+f.ws.Slug+"/items/"+f.visibleItem.Slug, nil, f.bearerHeaders()) + if rr.Code != http.StatusOK { + t.Fatalf("bearer admin GET visible item: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + + // Cookie admin — unrestricted, sees both (the pre-existing web UI + // admin affordance). + rr = doRequestWithCookie(f.srv, "GET", "/api/v1/workspaces/"+f.ws.Slug+"/items/"+f.hiddenItem.Slug, nil, f.sessionToken) + if rr.Code != http.StatusOK { + t.Fatalf("cookie admin GET hidden item: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + rr = doRequestWithCookie(f.srv, "GET", "/api/v1/workspaces/"+f.ws.Slug+"/items/"+f.visibleItem.Slug, nil, f.sessionToken) + if rr.Code != http.StatusOK { + t.Fatalf("cookie admin GET visible item: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } +} + +// TestUpdateItem_AdminBearer_RestrictedMemberBlockedOnHiddenCollection +// pins the write-path side of BUG-1918. PATCH must 404 at +// requireItemVisible before requireEditPermission's role/grant check +// ever runs — the hidden-collection item must be indistinguishable from +// a genuinely nonexistent one. +func TestUpdateItem_AdminBearer_RestrictedMemberBlockedOnHiddenCollection(t *testing.T) { + f := newBearerGateItemFixture(t) + + rr := doRequestWithHeaders(f.srv, "PATCH", "/api/v1/workspaces/"+f.ws.Slug+"/items/"+f.hiddenItem.Slug, + map[string]interface{}{"title": "Renamed"}, f.bearerHeaders()) + if rr.Code != http.StatusNotFound { + t.Fatalf("bearer admin PATCH hidden item: expected 404, got %d: %s", rr.Code, rr.Body.String()) + } + + // Visible-collection item: admin is an editor member there, so the + // edit succeeds. + rr = doRequestWithHeaders(f.srv, "PATCH", "/api/v1/workspaces/"+f.ws.Slug+"/items/"+f.visibleItem.Slug, + map[string]interface{}{"title": "Renamed"}, f.bearerHeaders()) + if rr.Code != http.StatusOK { + t.Fatalf("bearer admin PATCH visible item: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + + // Cookie admin — unrestricted, can PATCH the hidden item too. + rr = doRequestWithCookie(f.srv, "PATCH", "/api/v1/workspaces/"+f.ws.Slug+"/items/"+f.hiddenItem.Slug, + map[string]interface{}{"title": "Cookie renamed"}, f.sessionToken) + if rr.Code != http.StatusOK { + t.Fatalf("cookie admin PATCH hidden item: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } +} + +// TestDeleteItem_AdminBearer_RestrictedMemberBlockedOnHiddenCollection +// completes the write-path sweep for BUG-1918 (DELETE). +func TestDeleteItem_AdminBearer_RestrictedMemberBlockedOnHiddenCollection(t *testing.T) { + f := newBearerGateItemFixture(t) + + rr := doRequestWithHeaders(f.srv, "DELETE", "/api/v1/workspaces/"+f.ws.Slug+"/items/"+f.hiddenItem.Slug, nil, f.bearerHeaders()) + if rr.Code != http.StatusNotFound { + t.Fatalf("bearer admin DELETE hidden item: expected 404, got %d: %s", rr.Code, rr.Body.String()) + } + + // Cookie admin — unrestricted, can delete the hidden item. + rr = doRequestWithCookie(f.srv, "DELETE", "/api/v1/workspaces/"+f.ws.Slug+"/items/"+f.hiddenItem.Slug, nil, f.sessionToken) + if rr.Code != http.StatusNoContent { + t.Fatalf("cookie admin DELETE hidden item: expected 204, got %d: %s", rr.Code, rr.Body.String()) + } +} diff --git a/internal/server/handlers_ref_resolver.go b/internal/server/handlers_ref_resolver.go index fda33701..5904a1fb 100644 --- a/internal/server/handlers_ref_resolver.go +++ b/internal/server/handlers_ref_resolver.go @@ -170,12 +170,19 @@ func (s *Server) resolverItemVisible(r *http.Request, ws *models.Workspace, item // Derive the role the same way RequireWorkspaceAccess does. Pass the // bearer signal so a bearer-borne platform admin doesn't get a // cross-workspace owner bypass (BUG-1618). - role := s.resolverWorkspaceRole(ws, user, isBearerAuth(r)) + authIsBearer := isBearerAuth(r) + role := s.resolverWorkspaceRole(ws, user, authIsBearer) if role == "" { // Not a member, no grants, not admin/owner. Not visible. return false, nil } - return s.checkItemVisible(ws.ID, item, user, role) + // checkItemVisible's own admin bypass must also be bearer-gated + // (BUG-1918) — otherwise a bearer-admin member with a restricted + // role (correctly derived above) would still get the unconditional + // admin bypass inside checkItemVisible, reopening the exact + // cross-workspace leak resolverWorkspaceRole's authIsBearer param + // exists to close. + return s.checkItemVisible(ws.ID, item, user, role, authIsBearer) } // resolverWorkspaceRole reproduces RequireWorkspaceAccess's role lookup diff --git a/internal/server/handlers_ref_resolver_test.go b/internal/server/handlers_ref_resolver_test.go index caad3207..fd0d9981 100644 --- a/internal/server/handlers_ref_resolver_test.go +++ b/internal/server/handlers_ref_resolver_test.go @@ -301,7 +301,7 @@ func TestCheckItemVisible_TokenizedRoleAllowsNilUser(t *testing.T) { } for _, role := range []string{"owner", "editor"} { - ok, err := srv.checkItemVisible(ws.ID, item, nil, role) + ok, err := srv.checkItemVisible(ws.ID, item, nil, role, false) if err != nil { t.Errorf("role=%q: unexpected error: %v", role, err) } @@ -312,10 +312,10 @@ func TestCheckItemVisible_TokenizedRoleAllowsNilUser(t *testing.T) { // Sanity: nil user with no role still rejects (this is the path that // distinguishes legitimate legacy tokens from anonymous probes). - if ok, _ := srv.checkItemVisible(ws.ID, item, nil, ""); ok { + if ok, _ := srv.checkItemVisible(ws.ID, item, nil, "", false); ok { t.Error("nil user + empty role: expected visible=false") } - if ok, _ := srv.checkItemVisible(ws.ID, item, nil, "guest"); ok { + if ok, _ := srv.checkItemVisible(ws.ID, item, nil, "guest", false); ok { t.Error("nil user + guest role: expected visible=false (guest needs grants which require a user)") } } @@ -389,7 +389,7 @@ func TestCheckItemVisible_AuthenticatedEditorWithRestrictedAccess(t *testing.T) // return false. The round-2 bug returned true here, disabling the // per-collection gate for every read AND write path through // requireItemVisible / the resolver. - visible, err := srv.checkItemVisible(ws.ID, itemB, editor, "editor") + visible, err := srv.checkItemVisible(ws.ID, itemB, editor, "editor", false) if err != nil { t.Fatalf("checkItemVisible: unexpected error: %v", err) } @@ -406,7 +406,7 @@ func TestCheckItemVisible_AuthenticatedEditorWithRestrictedAccess(t *testing.T) if err != nil { t.Fatalf("CreateItem in alpha: %v", err) } - visible, err = srv.checkItemVisible(ws.ID, itemA, editor, "editor") + visible, err = srv.checkItemVisible(ws.ID, itemA, editor, "editor", false) if err != nil { t.Fatalf("checkItemVisible (allowed): %v", err) } @@ -415,6 +415,108 @@ func TestCheckItemVisible_AuthenticatedEditorWithRestrictedAccess(t *testing.T) } } +// TestCheckItemVisible_AdminBearerRestrictedRoleDeniedOnHiddenCollection +// pins BUG-1918: checkItemVisible's unconditional `user.Role == "admin"` +// bypass ignored isBearerAuth entirely, so a bearer-authed platform +// admin could see ANY item by ref regardless of their actual workspace +// role — bypassing BUG-1917's list-level scoping for anyone who could +// guess (or enumerate) a ref. +// +// This is a unit-level test of checkItemVisible directly rather than an +// HTTP-level one, because no HTTP-reachable admin-bearer request can +// demonstrate the bug for a REAL restricted member: RequireWorkspaceAccess +// already derives the member's actual (non-owner) role for a bearer-admin +// member (middleware_auth.go), so `role` here is exactly what a live +// request would carry — this test exercises checkItemVisible with that +// same (user, role, isBearer) shape directly, which is representative of +// every requireItemVisible-mediated HTTP handler (GET/PATCH/DELETE, and +// every comment/link/star/version/timeline/playbook/backlink/storage +// consumer that funnels through it). +// +// Note this shape does NOT cover the artifact-export consumer +// (handlers_artifact_export.go): exportable items live exclusively in +// system collections (playbooks/conventions), which +// store.VisibleCollectionIDs always includes for members regardless of +// collection_access="specific" — so a restricted-member admin sees them +// either way, fixed or not. And a bearer-admin who is NOT a member gets +// 403 at RequireWorkspaceAccess before ever reaching checkItemVisible, +// even with an unrelated grant (BUG-1616/1618's membership-only stance +// denies the grant-fallback for bearer admins specifically, unlike +// ordinary non-admin guests). So the export path has no HTTP-reachable +// scenario where this fix changes behavior — it's protected end-to-end +// by those two other invariants, and this test's coverage of the shared +// checkItemVisible helper is what backs it, same as every other +// requireItemVisible consumer. +func TestCheckItemVisible_AdminBearerRestrictedRoleDeniedOnHiddenCollection(t *testing.T) { + srv := testServer(t) + + owner, err := srv.store.CreateUser(models.UserCreate{ + Email: "owner2@example.com", Name: "Owner2", Username: "owner2", + Password: "pw-test-12345", + }) + if err != nil { + t.Fatalf("CreateUser owner: %v", err) + } + ws, err := srv.store.CreateWorkspace(models.WorkspaceCreate{ + Name: "WS2", OwnerID: owner.ID, + }) + if err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + collA, err := srv.store.CreateCollection(ws.ID, models.CollectionCreate{ + Name: "Alpha2", Slug: "alpha2", Prefix: "ALPHA2", + }) + if err != nil { + t.Fatalf("CreateCollection alpha2: %v", err) + } + collB, err := srv.store.CreateCollection(ws.ID, models.CollectionCreate{ + Name: "Beta2", Slug: "beta2", Prefix: "BETA2", + }) + if err != nil { + t.Fatalf("CreateCollection beta2: %v", err) + } + itemB, err := srv.store.CreateItem(ws.ID, collB.ID, models.ItemCreate{ + Title: "Forbidden", + }) + if err != nil { + t.Fatalf("CreateItem in beta2: %v", err) + } + + admin, err := srv.store.CreateUser(models.UserCreate{ + Email: "admin2@example.com", Name: "Admin2", Username: "admin2", + Password: "pw-test-12345", Role: "admin", + }) + if err != nil { + t.Fatalf("CreateUser admin: %v", err) + } + if err := srv.store.AddWorkspaceMember(ws.ID, admin.ID, "editor"); err != nil { + t.Fatalf("AddWorkspaceMember admin: %v", err) + } + if err := srv.store.SetMemberCollectionAccess(ws.ID, admin.ID, "specific", []string{collA.ID}); err != nil { + t.Fatalf("SetMemberCollectionAccess: %v", err) + } + + // Bearer admin: the gate applies — restricted "editor" role with + // collection_access="specific" (only collA) must not see collB's item. + visible, err := srv.checkItemVisible(ws.ID, itemB, admin, "editor", true) + if err != nil { + t.Fatalf("checkItemVisible: unexpected error: %v", err) + } + if visible { + t.Fatal("bearer-admin with restricted collection_access saw a hidden-collection item: BUG-1918 regression") + } + + // Cookie admin (isBearer=false): the pre-existing unconditional + // bypass still applies. + visible, err = srv.checkItemVisible(ws.ID, itemB, admin, "editor", false) + if err != nil { + t.Fatalf("checkItemVisible: unexpected error: %v", err) + } + if !visible { + t.Fatal("cookie-session admin should see any item regardless of collection_access") + } +} + // TestRefResolver_RestrictedMemberWithSystemCollection pins round-2 P1.2: // a restricted member with conventions/playbooks (system collections) // access plus an item grant in an UNRELATED collection must be able to diff --git a/internal/server/server.go b/internal/server/server.go index b715bd10..18de8ec6 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -1662,7 +1662,7 @@ func (s *Server) visibleCollectionIDs(r *http.Request, workspaceID string) ([]st // (e.g. handlers_ref_resolver.go) should use checkItemVisible directly with // a manually-derived role. func (s *Server) requireItemVisible(w http.ResponseWriter, r *http.Request, workspaceID string, item *models.Item) bool { - visible, err := s.checkItemVisible(workspaceID, item, currentUser(r), workspaceRole(r)) + visible, err := s.checkItemVisible(workspaceID, item, currentUser(r), workspaceRole(r), isBearerAuth(r)) if err != nil { writeInternalError(w, err) return false @@ -1687,6 +1687,15 @@ func (s *Server) requireItemVisible(w http.ResponseWriter, r *http.Request, work // - user — currentUser(r) at the call site; nil for unauthenticated. // - role — workspaceRole(r) at the call site, or the role derived // manually by callers operating outside RequireWorkspaceAccess. +// - isBearer — isBearerAuth(r) at the call site (BUG-1918). Narrows +// rule 3 below the same way BUG-1616/1617 narrowed the analogous +// bypasses in visibleCollectionIDs, resolverWorkspaceRole, and +// guestResourceFilterCore: a platform admin's global read access is +// a cookie-session / web-UI affordance only. A bearer-borne admin +// (CLI / PAT / MCP) who is a restricted workspace member must fall +// through to the same per-collection filter every other member +// faces — otherwise BUG-1917's list-level scoping is bypassable by +// guessing a ref and hitting the single-item endpoints directly. // // Rules (in order): // @@ -1704,7 +1713,8 @@ func (s *Server) requireItemVisible(w http.ResponseWriter, r *http.Request, work // 2. nil user past rule 1 → not visible. Anonymous viewers without a // tokenized role have no item-read access (share links own the // public-read surface via /s/{token}). -// 3. Admin user (user.Role == "admin") → always visible. +// 3. Admin user via cookie session (user.Role == "admin" && !isBearer) +// → always visible. Bearer-borne admins fall through to rule 4. // 4. Otherwise: replay the guestResourceFilterCore + member-collection- // access logic that requireItemVisible used to inline, with a system- // collections union added to the item-grants branch (Codex round-2 @@ -1712,7 +1722,7 @@ func (s *Server) requireItemVisible(w http.ResponseWriter, r *http.Request, work // an unrelated item grant previously 404'd on system-collection items // because the item-grants branch only checked direct grants + the // member's explicit collection-access list). -func (s *Server) checkItemVisible(workspaceID string, item *models.Item, user *models.User, role string) (bool, error) { +func (s *Server) checkItemVisible(workspaceID string, item *models.Item, user *models.User, role string, isBearer bool) (bool, error) { // Tokenized-nil-user bypass. RequireWorkspaceAccess synthesizes // "owner" on fresh installs (UserCount == 0, currentUser == nil) and // "editor" for legacy workspace-scoped API tokens (currentUser == @@ -1730,8 +1740,11 @@ func (s *Server) checkItemVisible(workspaceID string, item *models.Item, user *m if user == nil { return false, nil } - // Admin sees everything (matches visibleCollectionIDs's nil-filter shape). - if user.Role == "admin" { + // Admin sees everything, but only for cookie-session auth (matches + // visibleCollectionIDs's nil-filter shape). Bearer-borne admins + // (BUG-1918) fall through to the same per-collection filter every + // other member faces. + if user.Role == "admin" && !isBearer { return true, nil }