From 5a4db980af036822e61660d7cd71311bcd2436fb Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sat, 9 May 2026 09:00:31 -0400 Subject: [PATCH] Fix authorization gaps for restricted documents - Tighten Document.findByIds so isPrivate filtering fails closed when the attribute is not loaded, and include isPrivate in the projection used by Relationship.findSourceDocumentIdsForUser so backlinks from restricted docs are no longer leaked to collection-only members. - Add !isPrivate gate to the unpublish policy so collection writers without direct membership cannot unpublish restricted documents. Co-Authored-By: Claude Opus 4.7 --- server/models/Document.ts | 18 ++-- server/models/Relationship.ts | 2 +- server/policies/document.test.ts | 73 ++++++++++++++++ server/policies/document.ts | 43 +++++----- server/routes/api/documents/documents.test.ts | 83 +++++++++++++++++++ 5 files changed, 187 insertions(+), 32 deletions(-) diff --git a/server/models/Document.ts b/server/models/Document.ts index 9d239bb702..4a4a3ad0e4 100644 --- a/server/models/Document.ts +++ b/server/models/Document.ts @@ -900,19 +900,17 @@ class Document extends ArchivableModel< return true; } - // Check isPrivate safely (may not be loaded when attributes are restricted) - let isPrivate = false; - try { - isPrivate = doc.isPrivate; - } catch { - // attribute not loaded, treat as unrestricted + // Fail closed if isPrivate cannot be determined — callers that restrict + // attributes must explicitly include isPrivate to opt into collection + // inheritance. + if (doc.isPrivate !== false) { + return false; } return ( - !isPrivate && - ((!doc.collection?.isPrivate && !user?.isGuest) || - (doc.collection?.memberships.length || 0) > 0 || - (doc.collection?.groupMemberships.length || 0) > 0) + (!doc.collection?.isPrivate && !user?.isGuest) || + (doc.collection?.memberships.length || 0) > 0 || + (doc.collection?.groupMemberships.length || 0) > 0 ); }); } diff --git a/server/models/Relationship.ts b/server/models/Relationship.ts index f7de4ddae4..b5d9f9e523 100644 --- a/server/models/Relationship.ts +++ b/server/models/Relationship.ts @@ -72,7 +72,7 @@ class Relationship extends IdModel< const documents = await Document.findByIds( relationships.map((relationship) => relationship.reverseDocumentId), { - attributes: ["id"], + attributes: ["id", "isPrivate", "collectionId"], userId: user.id, includeState: false, includeViews: false, diff --git a/server/policies/document.test.ts b/server/policies/document.test.ts index c7af6df971..c36c0c92ed 100644 --- a/server/policies/document.test.ts +++ b/server/policies/document.test.ts @@ -589,4 +589,77 @@ describe("restricted document", () => { const abilities = serialize(user, document); expect(abilities.manageUsers).toEqual(false); }); + + it("should not allow unpublish for collection writer without direct membership", async () => { + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id }); + const collection = await buildCollection({ + teamId: team.id, + permission: CollectionPermission.ReadWrite, + }); + const doc = await buildDocument({ + teamId: team.id, + collectionId: collection.id, + isPrivate: true, + }); + const document = await Document.findByPk(doc.id, { userId: user.id }); + const abilities = serialize(user, document); + expect(abilities.unpublish).toEqual(false); + }); + + it("should allow unpublish for direct ReadWrite member", async () => { + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id }); + const collection = await buildCollection({ + teamId: team.id, + permission: CollectionPermission.ReadWrite, + }); + const doc = await buildDocument({ + teamId: team.id, + collectionId: collection.id, + isPrivate: true, + }); + await UserMembership.create({ + documentId: doc.id, + userId: user.id, + permission: DocumentPermission.ReadWrite, + createdById: user.id, + }); + const document = await Document.findByPk(doc.id, { userId: user.id }); + const abilities = serialize(user, document); + expect(abilities.unpublish).toBeTruthy(); + }); + + it("should allow unpublish for team admin", async () => { + const team = await buildTeam(); + const admin = await buildAdmin({ teamId: team.id }); + const collection = await buildCollection({ + teamId: team.id, + permission: CollectionPermission.ReadWrite, + }); + const doc = await buildDocument({ + teamId: team.id, + collectionId: collection.id, + isPrivate: true, + }); + const document = await Document.findByPk(doc.id, { userId: admin.id }); + const abilities = serialize(admin, document); + expect(abilities.unpublish).toBeTruthy(); + }); + + it("should allow unpublish for collection writer on non-private document", async () => { + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id }); + const collection = await buildCollection({ + teamId: team.id, + permission: CollectionPermission.ReadWrite, + }); + const doc = await buildDocument({ + teamId: team.id, + collectionId: collection.id, + }); + const document = await Document.findByPk(doc.id, { userId: user.id }); + const abilities = serialize(user, document); + expect(abilities.unpublish).toBeTruthy(); + }); }); diff --git a/server/policies/document.ts b/server/policies/document.ts index 974c6942cc..9d18477683 100644 --- a/server/policies/document.ts +++ b/server/policies/document.ts @@ -1,7 +1,7 @@ import invariant from "invariant"; import { DocumentPermission, TeamPreference } from "@shared/types"; import { Document, Revision, User, Team } from "@server/models"; -import { allow, cannot, can } from "./cancan"; +import { allow, can } from "./cancan"; import { and, isTeamAdmin, isTeamModel, isTeamMutable, or } from "./utils"; allow(User, "createDocument", Team, (actor, document) => @@ -270,26 +270,27 @@ allow( (document, revision) => document.id === revision?.documentId ); -allow(User, "unpublish", Document, (user, document) => { - if ( - !document || - user.isGuest || - user.isViewer || - !document.isActive || - document.isDraft - ) { - return false; - } - - invariant( - document.collection, - "collection is missing, did you forget to include in the query scope?" - ); - if (cannot(user, "updateDocument", document.collection)) { - return false; - } - return user.teamId === document.teamId; -}); +allow(User, "unpublish", Document, (user, document) => + and( + !!document, + !user.isGuest, + !user.isViewer, + !!document?.isActive, + !document?.isDraft, + user.teamId === document?.teamId, + or( + includesMembership(document, [ + DocumentPermission.ReadWrite, + DocumentPermission.Admin, + ]), + and(isTeamAdmin(user, document), can(user, "read", document)), + and( + !document?.isPrivate, + can(user, "updateDocument", document?.collection) + ) + ) + ) +); function includesMembership( document: Document | null, diff --git a/server/routes/api/documents/documents.test.ts b/server/routes/api/documents/documents.test.ts index 9efbb68ed5..8464aeba25 100644 --- a/server/routes/api/documents/documents.test.ts +++ b/server/routes/api/documents/documents.test.ts @@ -1079,6 +1079,89 @@ describe("#documents.list", () => { expect(body.data[0].id).toEqual(anotherDoc.id); }); + it("should not return restricted backlinks for non-members", async () => { + const team = await buildTeam(); + const owner = await buildUser({ teamId: team.id }); + const user = await buildUser({ teamId: team.id }); + const collection = await buildCollection({ + teamId: team.id, + permission: CollectionPermission.ReadWrite, + }); + const document = await buildDocument({ + userId: owner.id, + teamId: team.id, + collectionId: collection.id, + }); + const restrictedDoc = await buildDocument({ + title: "restricted backlink", + text: "secret", + userId: owner.id, + teamId: team.id, + collectionId: collection.id, + isPrivate: true, + }); + await Relationship.create({ + reverseDocumentId: restrictedDoc.id, + type: RelationshipType.Backlink, + documentId: document.id, + userId: owner.id, + }); + const res = await server.post("/api/documents.list", { + body: { + token: user.getJwtToken(), + backlinkDocumentId: document.id, + }, + }); + const body = await res.json(); + expect(res.status).toEqual(200); + expect(body.data.length).toEqual(0); + }); + + it("should return restricted backlinks for direct members", async () => { + const team = await buildTeam(); + const owner = await buildUser({ teamId: team.id }); + const user = await buildUser({ teamId: team.id }); + const collection = await buildCollection({ + teamId: team.id, + permission: CollectionPermission.ReadWrite, + }); + const document = await buildDocument({ + userId: owner.id, + teamId: team.id, + collectionId: collection.id, + }); + const restrictedDoc = await buildDocument({ + title: "restricted backlink", + text: "secret", + userId: owner.id, + teamId: team.id, + collectionId: collection.id, + isPrivate: true, + }); + await UserMembership.create({ + documentId: restrictedDoc.id, + userId: user.id, + permission: DocumentPermission.Read, + createdById: owner.id, + }); + await Relationship.create({ + reverseDocumentId: restrictedDoc.id, + type: RelationshipType.Backlink, + documentId: document.id, + userId: owner.id, + }); + const res = await server.post("/api/documents.list", { + body: { + token: user.getJwtToken(), + backlinkDocumentId: document.id, + }, + }); + const body = await res.json(); + expect(res.status).toEqual(200); + expect(body.data.length).toEqual(1); + expect(body.data[0].id).toEqual(restrictedDoc.id); + }); + it("should require authentication", async () => { const res = await server.post("/api/documents.list"); const body = await res.json();