mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 15:05:40 +00:00
fix(server): bearer-gate checkItemVisible's admin bypass (BUG-1918) (#793)
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.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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())
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user