Compare commits

..

3 Commits

Author SHA1 Message Date
agent_coder e609832ae4 fix(#348 review round-2 F5-F6): index page_access(workspace_id) + test the workspace-cache bust
Both are direct consequences of the round-1 F1 fix (uncaching
hasRestrictedPagesInWorkspace):

- F5: that EXISTS(SELECT 1 FROM page_access WHERE workspace_id=?) now runs
  per-request on every whole-workspace list endpoint (global search + suggest,
  favorites, notifications, recent, created-by), and page_access only had a
  space_id index → a seq scan in the common zero-restriction case. Added
  idx_page_access_workspace_id to the perf migration (up + down) so it's an
  index-only existence probe.
- F6: the DomainMiddleware workspace cache invalidation was untested — the
  int-spec passed `{}` for cacheManager, so bustWorkspaceCache's `del` threw into
  its own try/catch and never ran. Added a Map-backed cache double with a working
  del and two tests: updateSetting busts WORKSPACE_SELF_HOSTED; updateSharingSettings
  busts WORKSPACE_SELF_HOSTED + WORKSPACE_BY_HOST(hostname). A missed/mismatched
  bust key now fails the suite instead of letting a stale security-relevant
  workspace row (enforceSso/status) outlive the mutation.

Gate: server tsc 0; workspace-repo-update-setting + page-permission-workspace-filter
int-specs pass on real Postgres (the new index applies via global-setup).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-10 04:32:38 +03:00
agent_coder ee03da4018 fix(#348 review F1-F4): uncache the workspace-restriction gate + int-spec + docs
- F1 [medium — the substantive one]: hasRestrictedPagesInWorkspace is now UNCACHED
  (a plain EXISTS per call, like its sibling hasRestrictedPagesInSpace). Caching it
  (even 5s) reintroduced an access-control leak the space path never had: a
  concurrent whole-workspace read in the insert->commit window of the FIRST
  restricted page could re-populate `false` under withCache (read-then-set, no
  del-during-read guard) and override the insert-time bust, leaking that page to
  unauthorized users for up to the TTL. Uncaching removes both the DB/cache
  asymmetry and the TOCTOU race; the space path already accepts this per-call cost.
  Reverted the now-unnecessary insertPageAccess cache-bust and removed the dead
  HAS_RESTRICTED_PAGES_IN_WORKSPACE cache key.
- F2 [test]: page-permission-workspace-filter.int-spec.ts (real PG) — the
  short-circuit returns the full input set with zero restrictions AND filters out
  the page the user can't reach when a restriction is present (proving the authz
  behavior is unchanged), the 0->1 transition flips immediately, and the flag is
  per-workspace scoped.
- F3 [doc]: documented the deploy-time write-lock in the migration header — the
  non-CONCURRENT GIN trigram builds take a SHARE lock that blocks writes on
  pages/users/… for minutes on a large tenant; run in a maintenance window or
  build CONCURRENTLY out-of-band for big installs.
- F4 [doc]: corrected the jwt.strategy comment — the reused req.raw.workspace is
  the middleware's selectAll superset (not "the exact row this query returns"),
  harmless because AuthWorkspace already preferred that object.

Gate: server tsc 0; the new int-spec 3/3 on real Postgres.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-10 04:32:38 +03:00
agent_coder 28251b1e08 perf(server): low-hanging backend wins — indexes, auth dedup, embed coalescing, CTE short-circuit (#348)
One migration + targeted hot-path fixes. API behavior 1:1 (schema change = added
indexes + a byte-identical f_unaccent function-body swap, see below).

- Trigram + composite indexes (20260705T120000-perf-indexes.ts): GIN trigram on
  LOWER(f_unaccent(title/name)) for pages/users/groups (the /search/suggest
  leading-wildcard LIKE did a seq scan per keystroke — EXPLAIN now confirms
  Bitmap Index Scan on idx_pages_title_trgm), + page_history(page_id,id DESC),
  comments(page_id,id). DEVIATION (verified byte-identical): PG18 cannot inline
  the two-arg f_unaccent body during index creation, so up() swaps it to the
  schema-qualified single-arg `SELECT public.unaccent($1)` — same dictionary,
  identical output for all inputs, so the tsvector trigger + main @@ search stay
  consistent with NO reindex; down() restores the exact two-arg body.
- Auth path: jwt.strategy reuses req.raw.workspace when workspaceId matches (the
  middleware already validated it) instead of re-querying; domain.middleware
  caches the workspace lookup (withCache 15s, invalidated in all 8 WorkspaceRepo
  mutators, with a Date reviver for the JSON-serialized cache). USER + SESSION
  caching DEFERRED — the invalidation surface (role change doesn't revoke
  sessions; revocation includes background jobs) can't be safely covered, and a
  missed hook on a security path is worse than the win.
- AI re-embed coalescing: aiQueue.add gets {jobId: embed-<id>, delay: 30s} so
  active editing collapses to one job (worker reads current page state).
- filterAccessiblePageIds: hasRestrictedPagesInWorkspace short-circuit skips the
  recursive-ancestor CTE when a workspace has zero restricted pages (wired from
  search/favorites/notifications/recent/created-by). EXISTS on the same pageAccess
  table the CTE anti-joins → no false-positive / no access leak. Busts the cache
  on insertPageAccess so a 0->1 restricted transition takes effect immediately
  (review F1).
- Small: syncTransclusion guarded by a family-node probe (both old+new content, so
  the removal path is preserved); mention notifications enqueue only when the set
  gained a member; redis maintainLock clears a prior interval (leak fix).

Skipped as risky (flagged): global ValidationPipe transform change; a pool-wide
statement_timeout (would kill long CREATE INDEX migrations on the same pool).
NOTE: kept the trash query's `content` select — the trash UI reads page.content
for its preview modal (review F3, would have regressed).

Gate: server tsc 0; jest page-permission/auth/search/persistence 15 suites pass;
migration up+down+idempotency verified on real PG18 with EXPLAIN confirming index
use. No new deps.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-10 04:32:38 +03:00
25 changed files with 663 additions and 484 deletions
@@ -1,3 +1,10 @@
export const HISTORY_INTERVAL = 5 * 60 * 1000;
export const HISTORY_FAST_INTERVAL = 60 * 1000;
export const HISTORY_FAST_THRESHOLD = 5 * 60 * 1000;
// #348 — debounce window for the per-page RAG re-embed job. Repeated saves
// within this window collapse to a single delayed job (coalesced by a stable
// jobId), so active editing does not pile up expensive re-embeds (external API
// + page_embeddings rewrite, concurrency 1). The worker reads the CURRENT page
// state at run time, so the last content within the window wins.
export const EMBED_DEBOUNCE_MS = 30 * 1000;
@@ -431,7 +431,17 @@ describe('PersistenceExtension.onStoreDocument — Approach-A boundary snapshot'
it('uses the canonical page.id (not the slugId doc name) for post-store side effects (#260)', async () => {
const SLUG = 'slug-1'; // persistedHumanPage.slugId; findById resolves it
const document = ydocFor(doc('NEW AGENT CONTENT'));
pageRepo.findById.mockResolvedValue(persistedHumanPage('NEW AGENT CONTENT'));
// #348 — the transclusion sync now runs only when the new OR the previously
// persisted content carries a transclusion-family node. Give the persisted
// (old) content a pageEmbed so the sync path is exercised and the #260
// UUID-vs-slugId contract asserted below is still verified.
pageRepo.findById.mockResolvedValue({
...persistedHumanPage('NEW AGENT CONTENT'),
content: {
type: 'doc',
content: [{ type: 'pageEmbed', attrs: { sourcePageId: 'src-1' } }],
},
});
pageHistoryRepo.findPageLastHistory.mockResolvedValue(null);
// A `page.<slugId>` document name (the bug's smoking gun), agent store over
@@ -36,6 +36,7 @@ import {
import { Page } from '@docmost/db/types/entity.types';
import { CollabHistoryService } from '../services/collab-history.service';
import {
EMBED_DEBOUNCE_MS,
HISTORY_FAST_INTERVAL,
HISTORY_FAST_THRESHOLD,
HISTORY_INTERVAL,
@@ -45,6 +46,7 @@ import {
observeCollabLoad,
observeCollabStore,
} from '../../integrations/metrics/metrics.registry';
import { hasTransclusionFamilyNodes } from '../../core/page/transclusion/utils/transclusion-prosemirror.util';
/**
* #251 — wire format of the client→server stateless message that signals a
@@ -450,7 +452,18 @@ export class PersistenceExtension implements Extension {
// Use the canonical page UUID (page.id), not the doc-name id, which may be
// a slugId for a `page.<slugId>` doc (#260). The transclusion/reference
// syncs write uuid-typed columns, so a slugId here threw Postgres 22P02.
await this.syncTransclusion(page.id, page.workspaceId, tiptapJson);
//
// #348 — skip the three sync SELECTs when neither the new content nor the
// previously-persisted content has any transclusion/reference/pageEmbed
// node: nothing to insert, and (the DB mirrors the old content) nothing to
// delete. Whenever either side has one, run the idempotent sync exactly as
// before so removals are still reconciled.
if (
hasTransclusionFamilyNodes(tiptapJson) ||
hasTransclusionFamilyNodes(page.content)
) {
await this.syncTransclusion(page.id, page.workspaceId, tiptapJson);
}
}
if (page) {
@@ -466,7 +479,17 @@ export class PersistenceExtension implements Extension {
(m) => m.entityId,
);
if (userMentions.length > 0) {
// #348 — only enqueue when the mentioned-user set actually GAINED a member.
// The processor (processPageMention) already no-ops when every current
// mention was present before (newMentions.length === 0), so skipping the
// enqueue in that case is behavior-identical and avoids piling up no-op jobs
// on every save of a page that merely CONTAINS (unchanged) mentions.
const oldMentionedUserIdSet = new Set(oldMentionedUserIds);
const hasNewMentionedUser = userMentions.some(
(m) => !oldMentionedUserIdSet.has(m.entityId),
);
if (hasNewMentionedUser) {
await this.notificationQueue.add(QueueJob.PAGE_MENTION_NOTIFICATION, {
userMentions: userMentions.map((m) => ({
userId: m.entityId,
@@ -481,12 +504,23 @@ export class PersistenceExtension implements Extension {
} as IPageMentionNotificationJob);
}
await this.aiQueue.add(QueueJob.PAGE_CONTENT_UPDATED, {
// Canonical UUID: the embedding reindex resolves pages by uuid, so a
// slugId here threw Postgres 22P02 invalid-uuid (#260).
pageIds: [page.id],
workspaceId: page.workspaceId,
});
await this.aiQueue.add(
QueueJob.PAGE_CONTENT_UPDATED,
{
// Canonical UUID: the embedding reindex resolves pages by uuid, so a
// slugId here threw Postgres 22P02 invalid-uuid (#260).
pageIds: [page.id],
workspaceId: page.workspaceId,
},
// #348 — coalesce re-embeds during active editing. A stable per-page
// jobId + delay means repeated saves within EMBED_DEBOUNCE_MS collapse
// to one delayed job instead of one expensive re-embed per save. The
// worker reads the current page state at run time, so last content wins.
// BullMQ forbids ':' in custom job ids (Redis key separator), so '-' is
// used; page.id is a UUID, so the id is unique per page. removeOnComplete
// (queue.module) frees the id after each run so the next window re-arms.
{ jobId: `embed-${page.id}`, delay: EMBED_DEBOUNCE_MS },
);
await this.enqueuePageHistory(page, lastUpdatedSource);
}
@@ -220,6 +220,13 @@ export class RedisSyncExtension<TCE extends CustomEvents> implements Extension {
};
async maintainLock(documentName: string) {
// #348 — clear any existing timer for this document before installing a new
// one. Without this, a second maintainLock for the same document (a
// reload-without-unload) overwrites this.locks[documentName] and leaks the
// previous interval, which keeps firing SET forever with no way to clear it.
if (this.locks[documentName]) {
clearInterval(this.locks[documentName]);
}
this.locks[documentName] = setInterval(() => {
this.pub.set(
this.getKey(documentName),
@@ -4,8 +4,21 @@ export const CacheKey = {
`perm:space-roles:${userId}:${spaceId}`,
PAGE_CAN_EDIT: (userId: string, pageId: string) =>
`perm:can-edit:${userId}:${pageId}`,
// #348 — DomainMiddleware workspace resolution. Self-hosted resolves the single
// workspace (constant key); cloud resolves by the request subdomain (lowercased
// to match the case-insensitive `LOWER(hostname)` lookup). Every WorkspaceRepo
// mutator busts these, so staleness is bounded by both explicit invalidation and
// the short TTL below.
WORKSPACE_SELF_HOSTED: 'workspace:self-hosted',
WORKSPACE_BY_HOST: (subdomain: string) =>
`workspace:byhost:${subdomain.toLowerCase()}`,
};
// Permission caches dedupe repeated checks within and across short request bursts.
// 5s keeps staleness on revocations bounded.
export const PERMISSION_CACHE_TTL_MS = 5_000;
// #348 — workspace row changes rarely; a short TTL bounds staleness of
// security-relevant fields (enforceSso/enforceMfa/status) even if an explicit
// bust is ever missed, while still removing the per-request workspace query.
export const WORKSPACE_CACHE_TTL_MS = 15_000;
@@ -1,13 +1,42 @@
import { Injectable, NestMiddleware, NotFoundException } from '@nestjs/common';
import { Inject, Injectable, NestMiddleware } from '@nestjs/common';
import { FastifyRequest, FastifyReply } from 'fastify';
import { CACHE_MANAGER } from '@nestjs/cache-manager';
import { Cache } from 'cache-manager';
import { EnvironmentService } from '../../integrations/environment/environment.service';
import { WorkspaceRepo } from '@docmost/db/repos/workspace/workspace.repo';
import { Workspace } from '@docmost/db/types/entity.types';
import { withCache } from '../helpers/with-cache';
import { CacheKey, WORKSPACE_CACHE_TTL_MS } from '../helpers/cache-keys';
// #348 — timestamptz columns on the workspace row. The cache store (Keyv/Redis)
// JSON-serializes values, so a cached workspace comes back with these fields as
// ISO strings. Reviving them to Date keeps the cached path byte-identical to the
// direct DB path (postgres.js returns Date), so nothing downstream can observe a
// cache hit vs miss. Idempotent: `new Date(date)` on an already-Date value is a
// no-op-equivalent. Keep in sync with the workspace timestamptz columns.
const WORKSPACE_DATE_FIELDS: Array<keyof Workspace> = [
'createdAt',
'updatedAt',
'deletedAt',
'trialEndAt',
];
function reviveWorkspaceDates(workspace: Workspace): Workspace {
for (const field of WORKSPACE_DATE_FIELDS) {
const value = workspace[field];
if (value != null) {
(workspace as any)[field] = new Date(value as any);
}
}
return workspace;
}
@Injectable()
export class DomainMiddleware implements NestMiddleware {
constructor(
private workspaceRepo: WorkspaceRepo,
private environmentService: EnvironmentService,
@Inject(CACHE_MANAGER) private readonly cacheManager: Cache,
) {}
async use(
req: FastifyRequest['raw'],
@@ -15,13 +44,21 @@ export class DomainMiddleware implements NestMiddleware {
next: () => void,
) {
if (this.environmentService.isSelfHosted()) {
const workspace = await this.workspaceRepo.findFirst();
// #348 — cache the single-workspace lookup that runs on every request.
// Invalidated by every WorkspaceRepo mutator (see bustWorkspaceCache).
const workspace = await withCache(
this.cacheManager,
CacheKey.WORKSPACE_SELF_HOSTED,
WORKSPACE_CACHE_TTL_MS,
() => this.workspaceRepo.findFirst(),
);
if (!workspace) {
//throw new NotFoundException('Workspace not found');
(req as any).workspaceId = null;
return next();
}
reviveWorkspaceDates(workspace);
// TODO: unify
(req as any).workspaceId = workspace.id;
(req as any).workspace = workspace;
@@ -29,13 +66,21 @@ export class DomainMiddleware implements NestMiddleware {
const header = req.headers.host;
const subdomain = header.split('.')[0];
const workspace = await this.workspaceRepo.findByHostname(subdomain);
// #348 — cache per-subdomain workspace resolution. Keyed by subdomain (the
// hostname column); busted per hostname by every WorkspaceRepo mutator.
const workspace = await withCache(
this.cacheManager,
CacheKey.WORKSPACE_BY_HOST(subdomain),
WORKSPACE_CACHE_TTL_MS,
() => this.workspaceRepo.findByHostname(subdomain),
);
if (!workspace) {
(req as any).workspaceId = null;
return next();
}
reviveWorkspaceDates(workspace);
(req as any).workspaceId = workspace.id;
(req as any).workspace = workspace;
}
@@ -729,35 +729,6 @@ export class AiChatToolsService {
}),
),
// Schema + description live in @docmost/mcp's SHARED_TOOL_SPECS (#423).
// meta.hash in the result is the baseHash drawioUpdate requires.
drawioGet: sharedTool(
sharedToolSpecs.drawioGet,
async ({ pageId, node, format }) =>
await client.drawioGet(pageId, node, format ?? 'xml'),
),
// Schema + description live in @docmost/mcp's SHARED_TOOL_SPECS (#423).
// The flat schema fields are regrouped into the client's `where` object.
drawioCreate: sharedTool(
sharedToolSpecs.drawioCreate,
async ({ pageId, xml, position, anchorNodeId, anchorText, title }) =>
await client.drawioCreate(
pageId,
{ position, anchorNodeId, anchorText },
xml,
title,
),
),
// Schema + description live in @docmost/mcp's SHARED_TOOL_SPECS (#423).
// baseHash is the optimistic lock: mismatch => structured conflict error.
drawioUpdate: sharedTool(
sharedToolSpecs.drawioUpdate,
async ({ pageId, node, xml, baseHash }) =>
await client.drawioUpdate(pageId, node, xml, baseHash),
),
// Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294).
// The table reference parameter was unified to `table` (was `tableRef`).
tableInsertRow: sharedTool(
@@ -168,32 +168,6 @@ export interface DocmostClientLike {
url: string,
opts?: { align?: 'left' | 'center' | 'right'; alt?: string },
): Promise<Record<string, unknown>>;
// --- draw.io diagrams (#423, stage 1) ---
// Read a diagram as decoded mxGraph XML (default) or the raw .drawio.svg.
// meta.hash is the optimistic-lock key drawioUpdate expects as baseHash.
drawioGet(
pageId: string,
node: string,
format?: 'xml' | 'svg',
): Promise<Record<string, unknown>>;
// Lint mxGraph XML, build the .drawio.svg attachment and insert a drawio node.
drawioCreate(
pageId: string,
where: {
position: 'before' | 'after' | 'append';
anchorNodeId?: string;
anchorText?: string;
},
xml: string,
title?: string,
): Promise<Record<string, unknown>>;
// Optimistic-locked full replacement of a diagram (baseHash from drawioGet).
drawioUpdate(
pageId: string,
node: string,
xml: string,
baseHash: string,
): Promise<Record<string, unknown>>;
tableInsertRow(
pageId: string,
tableRef: string,
@@ -51,7 +51,21 @@ export class JwtStrategy extends PassportStrategy(Strategy, 'jwt') {
throw new UnauthorizedException();
}
const workspace = await this.workspaceRepo.findById(payload.workspaceId);
// #348 — reuse the workspace DomainMiddleware already loaded for this request
// instead of re-querying it. `validate()` above has confirmed
// `req.raw.workspaceId === payload.workspaceId` (or that it is unset), and the
// middleware sets `req.raw.workspace` alongside `req.raw.workspaceId` from the
// SAME workspace row, so when the ids match this is that row. NOTE it is the
// middleware's `selectAll` object (a superset of the fallback `findById` base
// fields — it also carries licenseKey/auditRetentionDays); that is harmless
// here because every consumer reads this workspace via the AuthWorkspace
// decorator, which already preferred `req.raw.workspace` (the selectAll object)
// over `req.user.workspace` before this change. Fall back to the query if the
// middleware did not populate it (a path that bypasses DomainMiddleware).
const workspace =
req.raw.workspace && req.raw.workspaceId === payload.workspaceId
? req.raw.workspace
: await this.workspaceRepo.findById(payload.workspaceId);
if (!workspace) {
throw new UnauthorizedException();
@@ -38,6 +38,8 @@ export class FavoriteService {
await this.pagePermissionRepo.filterAccessiblePageIds({
pageIds: result.items,
userId,
// #348 — favorites load at app-start; enable the workspace short-circuit.
workspaceId,
});
const accessibleSet = new Set(accessibleIds);
result.items = result.items.filter((id) => accessibleSet.has(id));
@@ -125,6 +127,8 @@ export class FavoriteService {
await this.pagePermissionRepo.filterAccessiblePageIds({
pageIds,
userId,
// #348 — workspace-level short-circuit for the favorites list.
workspaceId,
});
accessiblePageSet = new Set(accessibleIds);
}
@@ -23,7 +23,12 @@ export class NotificationController {
@Body() dto: ListNotificationsDto,
@AuthUser() user: User,
) {
return this.notificationService.findByUserId(user.id, dto, dto.type);
return this.notificationService.findByUserId(
user.id,
dto,
dto.type,
user.workspaceId,
);
}
@HttpCode(HttpStatus.OK)
@@ -45,6 +45,7 @@ export class NotificationService {
userId: string,
pagination: PaginationOptions,
type: NotificationTab = 'all',
workspaceId?: string | null,
) {
const result = await this.notificationRepo.findByUserId(
userId,
@@ -61,6 +62,8 @@ export class NotificationService {
await this.pagePermissionRepo.filterAccessiblePageIds({
pageIds,
userId,
// #348 — notifications list; enable the workspace short-circuit.
workspaceId,
});
const accessibleSet = new Set(accessiblePageIds);
+12 -2
View File
@@ -446,7 +446,11 @@ export class PageController {
);
}
return this.pageService.getRecentPages(user.id, pagination);
return this.pageService.getRecentPages(
user.id,
pagination,
user.workspaceId,
);
}
@HttpCode(HttpStatus.OK)
@@ -469,7 +473,13 @@ export class PageController {
}
}
return this.pageService.getCreatedByPages(targetUserId, user.id, pagination, dto.spaceId);
return this.pageService.getCreatedByPages(
targetUserId,
user.id,
pagination,
dto.spaceId,
user.workspaceId,
);
}
@HttpCode(HttpStatus.OK)
@@ -1165,6 +1165,7 @@ export class PageService {
async getRecentPages(
userId: string,
pagination: PaginationOptions,
workspaceId?: string | null,
): Promise<CursorPaginationResult<Page>> {
const result = await this.pageRepo.getRecentPages(userId, pagination);
@@ -1174,6 +1175,8 @@ export class PageService {
await this.pagePermissionRepo.filterAccessiblePageIds({
pageIds,
userId,
// #348 — cross-space "recent"; enable the workspace short-circuit.
workspaceId,
});
const accessibleSet = new Set(accessibleIds);
result.items = result.items.filter((p) => accessibleSet.has(p.id));
@@ -1187,6 +1190,7 @@ export class PageService {
requestingUserId: string,
pagination: PaginationOptions,
spaceId?: string,
workspaceId?: string | null,
): Promise<CursorPaginationResult<Page>> {
const result = await this.pageRepo.getCreatedByPages(
creatorId,
@@ -1201,6 +1205,9 @@ export class PageService {
await this.pagePermissionRepo.filterAccessiblePageIds({
pageIds,
userId: requestingUserId,
spaceId,
// #348 — enable the workspace short-circuit when not space-scoped.
workspaceId,
});
const accessibleSet = new Set(accessibleIds);
result.items = result.items.filter((p) => accessibleSet.has(p.id));
@@ -93,6 +93,41 @@ function collectNodes<T>(
return Array.from(byKey.values());
}
/**
* #348 cheap early-exit probe: does this doc contain ANY node the transclusion
* syncs care about (`transclusionSource` / `transclusionReference` / `pageEmbed`)?
* Lets the collab store skip the three sync SELECTs when neither the previous nor
* the new content has any such node there is nothing to insert, and (since the
* DB mirrors the previously-persisted content) nothing to delete. Walks once and
* short-circuits on the first match; uses the same depth ceiling as the
* collectors. Deliberately does NOT skip `transclusionSource` subtrees: it only
* answers "any node present?", so descending everywhere is strictly conservative
* (it can never wrongly report "none").
*/
export function hasTransclusionFamilyNodes(doc: unknown): boolean {
const visit = (node: any, depth: number): boolean => {
if (!node || typeof node !== 'object') return false;
if (depth > MAX_PM_WALK_DEPTH) return false;
if (
node.type === TRANSCLUSION_TYPE ||
node.type === REFERENCE_TYPE ||
node.type === PAGE_EMBED_TYPE
) {
return true;
}
if (Array.isArray(node.content)) {
for (const child of node.content) {
if (visit(child, depth + 1)) return true;
}
}
return false;
};
return visit(doc, 0);
}
/**
* Walks a ProseMirror JSON document and returns one snapshot per top-level
* `transclusion` node. Does not recurse into transclusions (schema disallows
@@ -155,6 +155,8 @@ export class SearchService {
pageIds,
userId: opts.userId,
spaceId: searchParams.spaceId,
// #348 — enables the workspace-level short-circuit when not space-scoped.
workspaceId: opts.workspaceId,
});
const accessibleSet = new Set(accessibleIds);
results = results.filter((r: any) => accessibleSet.has(r.id));
@@ -266,6 +268,8 @@ export class SearchService {
await this.pagePermissionRepo.filterAccessiblePageIds({
pageIds,
userId,
// #348 — workspace-level short-circuit for the suggest path.
workspaceId,
});
const accessibleSet = new Set(accessibleIds);
pages = pages.filter((p) => accessibleSet.has(p.id));
@@ -0,0 +1,118 @@
import { type Kysely, sql } from 'kysely';
/**
* #348 targeted hot-path indexes.
*
* 1. GIN trigram indexes for `/search/suggest`. That endpoint runs a
* leading-wildcard `LOWER(f_unaccent(col)) LIKE '%q%'` per keystroke, which
* is a sequential scan without a trigram index. The index EXPRESSIONS below
* are `LOWER(f_unaccent(title|name))`, matching the predicates in
* search.service.ts exactly so the planner uses them (verified with EXPLAIN:
* the suggest predicate resolves to a Bitmap Index Scan on these indexes).
*
* IMMUTABLE-wrapper fix (required for the index to build): `f_unaccent` was
* defined as `SELECT unaccent('unaccent', $1)` (the two-arg, dictionary-named
* unaccent). That body CANNOT be used in an index expression: when Postgres
* inlines the IMMUTABLE SQL wrapper while building the index it fails to
* resolve the two-arg call (`function unaccent(unknown, text) does not exist`,
* the `'unaccent'` literal loses its regdictionary coercion). The single-arg
* `unaccent($1)` is the same operation (the default text-search dictionary IS
* `unaccent`; verified byte-equal on accented samples), and crucially
* SCHEMA-QUALIFIED as `public.unaccent($1)` it inlines cleanly, so the index
* builds. We therefore `CREATE OR REPLACE` `f_unaccent` to the qualified
* single-arg body. This is output-identical for every existing caller (the
* tsvector trigger, the main `tsv @@` search, and the suggest LIKE), so no
* reindex/backfill is needed; `down()` restores the original two-arg body.
* (The `unaccent` extension is installed in `public` in this codebase, which
* is why `public.unaccent` is the correct qualification.)
*
* 2. Composite indexes for two ORDER-BY-only-on-id queries that currently sort
* on top of a created_at index:
* - page_history: `findPageHistoryByPageId` does WHERE page_id ORDER BY id
* DESC, but only `(page_id, created_at DESC)` exists extra sort.
* - comments: `findPageComments` does WHERE page_id ORDER BY id ASC, but only
* `(page_id)` exists extra sort.
*
* DEPLOY-TIME LOCK WARNING: these are plain (non-CONCURRENT) CREATE INDEX
* statements CONCURRENTLY is impossible because Kysely runs each migration in a
* transaction. They take a SHARE lock that BLOCKS writes (INSERT/UPDATE/DELETE) on
* pages/users/groups/comments/page_history for the duration of the build. The two
* GIN trigram builds on pages.title / users.name are the slow ones and can take
* minutes on a large tenant a write-outage window during the deploy migration.
* For large installations, run this migration in a maintenance window, or build
* the trigram indexes out-of-band with CREATE INDEX CONCURRENTLY before deploying
* (then this migration's `IF NOT EXISTS` is a no-op). Small/typical tenants are
* unaffected.
*/
export async function up(db: Kysely<any>): Promise<void> {
// Index-compatible, output-identical redefinition of f_unaccent (see header).
await sql`
CREATE OR REPLACE FUNCTION f_unaccent(text)
RETURNS text
LANGUAGE sql
IMMUTABLE PARALLEL SAFE STRICT
AS $func$
SELECT public.unaccent($1);
$func$
`.execute(db);
// Search-suggest trigram indexes. Expressions match search.service.ts.
await sql`
CREATE INDEX IF NOT EXISTS idx_pages_title_trgm
ON pages USING gin ((LOWER(f_unaccent(title))) gin_trgm_ops)
`.execute(db);
await sql`
CREATE INDEX IF NOT EXISTS idx_users_name_trgm
ON users USING gin ((LOWER(f_unaccent(name))) gin_trgm_ops)
`.execute(db);
await sql`
CREATE INDEX IF NOT EXISTS idx_groups_name_trgm
ON groups USING gin ((LOWER(f_unaccent(name))) gin_trgm_ops)
`.execute(db);
// page_history: WHERE page_id ORDER BY id DESC (findPageHistoryByPageId).
await sql`
CREATE INDEX IF NOT EXISTS idx_page_history_page_id
ON page_history (page_id, id DESC)
`.execute(db);
// comments: WHERE page_id ORDER BY id ASC (findPageComments).
await sql`
CREATE INDEX IF NOT EXISTS idx_comments_page_id_id
ON comments (page_id, id)
`.execute(db);
// page_access(workspace_id): #348 made hasRestrictedPagesInWorkspace uncached
// (F1 fix), so `EXISTS(SELECT 1 FROM page_access WHERE workspace_id=?)` now runs
// per-request on every whole-workspace list endpoint (global search + suggest,
// favorites, notifications, recent, created-by). page_access only had a
// space_id index → that EXISTS was a seq scan in the common zero-restriction
// case. This index makes it an index-only existence probe.
await sql`
CREATE INDEX IF NOT EXISTS idx_page_access_workspace_id
ON page_access (workspace_id)
`.execute(db);
}
export async function down(db: Kysely<any>): Promise<void> {
// Drop the expression indexes before restoring the function body.
await sql`DROP INDEX IF EXISTS idx_pages_title_trgm`.execute(db);
await sql`DROP INDEX IF EXISTS idx_users_name_trgm`.execute(db);
await sql`DROP INDEX IF EXISTS idx_groups_name_trgm`.execute(db);
await sql`DROP INDEX IF EXISTS idx_page_history_page_id`.execute(db);
await sql`DROP INDEX IF EXISTS idx_comments_page_id_id`.execute(db);
await sql`DROP INDEX IF EXISTS idx_page_access_workspace_id`.execute(db);
// Restore the original two-arg (dictionary-named) f_unaccent body.
await sql`
CREATE OR REPLACE FUNCTION f_unaccent(text)
RETURNS text
LANGUAGE sql
IMMUTABLE PARALLEL SAFE STRICT
AS $func$
SELECT unaccent('unaccent', $1);
$func$
`.execute(db);
}
@@ -657,8 +657,9 @@ export class PagePermissionRepo {
pageIds: string[];
userId: string;
spaceId?: string;
workspaceId?: string | null;
}): Promise<string[]> {
const { pageIds, userId, spaceId } = opts;
const { pageIds, userId, spaceId, workspaceId } = opts;
if (pageIds.length === 0) return [];
if (spaceId) {
@@ -666,6 +667,17 @@ export class PagePermissionRepo {
if (!hasRestrictions) {
return pageIds;
}
} else if (workspaceId) {
// #348 — whole-workspace callers (no spaceId: favorites, notifications,
// recent, created-by, global search) skip the recursive-ancestor CTE + anti
// -join entirely when the workspace has ZERO restricted pages. When any
// restriction DOES exist, fall through to the identical CTE below, so
// behavior is unchanged whenever restrictions are present.
const hasRestrictions =
await this.hasRestrictedPagesInWorkspace(workspaceId);
if (!hasRestrictions) {
return pageIds;
}
}
const results = await this.db
@@ -903,6 +915,39 @@ export class PagePermissionRepo {
return Boolean(result?.exists);
}
/**
* Workspace-level analogue of hasRestrictedPagesInSpace: does ANY page in the
* whole workspace carry a restriction? Lets whole-workspace access filters
* short-circuit the recursive-ancestor CTE when nothing is restricted at all.
*
* UNCACHED (like the sibling hasRestrictedPagesInSpace) a single cheap
* `EXISTS(pageAccess WHERE workspaceId=?)` per call. This is an ACCESS-CONTROL
* gate on whole-workspace list endpoints, so it must never go stale: caching it
* (even 5s) reintroduced a leak the space-path never had a concurrent
* whole-workspace read in the insert->commit window of the FIRST restricted page
* could re-populate `false` under withCache (read-then-set, no del-during-read
* guard) and override the insert bust, leaking that page to unauthorized users
* for up to the TTL (#348 review F1). An uncached EXISTS removes both the
* cache/DB asymmetry with hasRestrictedPagesInSpace and that race; the space
* path already accepts this exact per-call cost.
*/
async hasRestrictedPagesInWorkspace(workspaceId: string): Promise<boolean> {
const result = await this.db
.selectNoFrom((eb) =>
eb
.exists(
eb
.selectFrom('pageAccess')
.select(sql`1`.as('one'))
.where('pageAccess.workspaceId', '=', workspaceId),
)
.as('exists'),
)
.executeTakeFirst();
return Boolean(result?.exists);
}
/**
* Given a list of parent page IDs, return which ones have at least one accessible child.
* Efficient batch query for sidebar hasChildren calculation.
@@ -581,6 +581,9 @@ export class PageRepo {
const query = this.db
.selectFrom('pages')
.select(this.baseFields)
// NOTE: `content` IS needed here — the trash UI reads page.content to render
// the deleted-page preview modal (trash.tsx handlePageClick ->
// TrashPageContentModal pageContent). Do NOT drop it (see #348 review F3).
.select('content')
.select((eb) => this.withSpace(eb))
.select((eb) => this.withDeletedBy(eb))
@@ -1,4 +1,6 @@
import { Injectable } from '@nestjs/common';
import { Inject, Injectable } from '@nestjs/common';
import { CACHE_MANAGER } from '@nestjs/cache-manager';
import { Cache } from 'cache-manager';
import { InjectKysely } from 'nestjs-kysely';
import { KyselyDB, KyselyTransaction } from '../../types/kysely.types';
import { dbOrTx } from '../../utils';
@@ -9,6 +11,7 @@ import {
} from '@docmost/db/types/entity.types';
import { ExpressionBuilder, sql } from 'kysely';
import { DB, Workspaces } from '@docmost/db/types/db';
import { CacheKey } from '../../../common/helpers/cache-keys';
/**
* Writable `settings.ai.provider` keys, enforced at this generic SQL layer. This
@@ -61,7 +64,34 @@ export class WorkspaceRepo {
'temporaryNoteHours',
'isScimEnabled',
];
constructor(@InjectKysely() private readonly db: KyselyDB) {}
constructor(
@InjectKysely() private readonly db: KyselyDB,
@Inject(CACHE_MANAGER) private readonly cacheManager: Cache,
) {}
/**
* #348 bust the DomainMiddleware workspace caches after any workspace write.
* Deletes BOTH the self-hosted (constant) key and the cloud per-hostname key so
* a single implementation covers either deployment mode (the irrelevant key is a
* harmless no-op). Best-effort: a cache error must never fail the write, and a
* missed bust is bounded by WORKSPACE_CACHE_TTL_MS. Note: a hostname RENAME only
* busts the NEW hostname's key (the row returned here carries the new hostname);
* the old key expires via TTL.
*/
private async bustWorkspaceCache(
workspace?: Pick<Workspace, 'hostname'> | undefined,
): Promise<void> {
try {
await this.cacheManager.del(CacheKey.WORKSPACE_SELF_HOSTED);
if (workspace?.hostname) {
await this.cacheManager.del(
CacheKey.WORKSPACE_BY_HOST(workspace.hostname),
);
}
} catch {
// cache is best-effort; TTL is the backstop
}
}
async findById(
workspaceId: string,
@@ -144,12 +174,14 @@ export class WorkspaceRepo {
trx?: KyselyTransaction,
): Promise<Workspace> {
const db = dbOrTx(this.db, trx);
return db
const workspace = await db
.updateTable('workspaces')
.set({ ...updatableWorkspace, updatedAt: new Date() })
.where('id', '=', workspaceId)
.returning(this.baseFields)
.executeTakeFirst();
await this.bustWorkspaceCache(workspace);
return workspace;
}
async insertWorkspace(
@@ -157,11 +189,14 @@ export class WorkspaceRepo {
trx?: KyselyTransaction,
): Promise<Workspace> {
const db = dbOrTx(this.db, trx);
return db
const workspace = await db
.insertInto('workspaces')
.values(insertableWorkspace)
.returning(this.baseFields)
.executeTakeFirst();
// Bust the cached "not found" so a fresh install / new tenant is seen at once.
await this.bustWorkspaceCache(workspace);
return workspace;
}
async count(): Promise<number> {
@@ -203,7 +238,7 @@ export class WorkspaceRepo {
trx?: KyselyTransaction,
) {
const db = dbOrTx(this.db, trx);
return db
const workspace = await db
.updateTable('workspaces')
.set({
settings: sql`COALESCE(settings, '{}'::jsonb)
@@ -214,6 +249,8 @@ export class WorkspaceRepo {
.where('id', '=', workspaceId)
.returning(this.baseFields)
.executeTakeFirst();
await this.bustWorkspaceCache(workspace);
return workspace;
}
async updateAiSettings(
@@ -223,7 +260,7 @@ export class WorkspaceRepo {
trx?: KyselyTransaction,
) {
const db = dbOrTx(this.db, trx);
return db
const workspace = await db
.updateTable('workspaces')
.set({
settings: sql`COALESCE(settings, '{}'::jsonb)
@@ -234,6 +271,8 @@ export class WorkspaceRepo {
.where('id', '=', workspaceId)
.returning(this.baseFields)
.executeTakeFirst();
await this.bustWorkspaceCache(workspace);
return workspace;
}
/**
@@ -272,7 +311,7 @@ export class WorkspaceRepo {
entries.flatMap(([k, v]) => [sql.lit(k), sql`${v}::text`]),
)})`
: sql`'{}'::jsonb`;
return db
const workspace = await db
.updateTable('workspaces')
.set({
settings: sql`COALESCE(settings, '{}'::jsonb) || jsonb_build_object(
@@ -287,6 +326,8 @@ export class WorkspaceRepo {
.where('id', '=', workspaceId)
.returning(this.baseFields)
.executeTakeFirst();
await this.bustWorkspaceCache(workspace);
return workspace;
}
/**
@@ -303,7 +344,7 @@ export class WorkspaceRepo {
trx?: KyselyTransaction,
) {
const db = dbOrTx(this.db, trx);
return db
const workspace = await db
.updateTable('workspaces')
.set({
settings: sql`COALESCE(settings, '{}'::jsonb)
@@ -313,6 +354,8 @@ export class WorkspaceRepo {
.where('id', '=', workspaceId)
.returning(this.baseFields)
.executeTakeFirst();
await this.bustWorkspaceCache(workspace);
return workspace;
}
async updateSharingSettings(
@@ -322,7 +365,7 @@ export class WorkspaceRepo {
trx?: KyselyTransaction,
) {
const db = dbOrTx(this.db, trx);
return db
const workspace = await db
.updateTable('workspaces')
.set({
settings: sql`COALESCE(settings, '{}'::jsonb)
@@ -333,6 +376,8 @@ export class WorkspaceRepo {
.where('id', '=', workspaceId)
.returning(this.baseFields)
.executeTakeFirst();
await this.bustWorkspaceCache(workspace);
return workspace;
}
async updateTemplateSettings(
@@ -342,7 +387,7 @@ export class WorkspaceRepo {
trx?: KyselyTransaction,
) {
const db = dbOrTx(this.db, trx);
return db
const workspace = await db
.updateTable('workspaces')
.set({
settings: sql`COALESCE(settings, '{}'::jsonb)
@@ -353,6 +398,8 @@ export class WorkspaceRepo {
.where('id', '=', workspaceId)
.returning(this.baseFields)
.executeTakeFirst();
await this.bustWorkspaceCache(workspace);
return workspace;
}
}
@@ -0,0 +1,113 @@
import { Kysely } from 'kysely';
import { PagePermissionRepo } from '@docmost/db/repos/page/page-permission.repo';
import { GroupRepo } from '@docmost/db/repos/group/group.repo';
import {
getTestDb,
destroyTestDb,
createWorkspace,
createSpace,
createUser,
createPage,
} from './db';
/**
* #348 the whole-workspace access-filter short-circuit is an ACCESS-CONTROL
* path, so it must produce the SAME result as the full recursive-ancestor CTE.
*
* filterAccessiblePageIds({ workspaceId }) (no spaceId the favorites /
* notifications / recent / created-by / global-search callers) skips the CTE only
* when the workspace has ZERO restricted pages. A page is "restricted &
* inaccessible" when it (or an ancestor) has a `pageAccess` row and the user has
* no matching `pagePermissions`. Driven against real Postgres, asserts:
* 1. zero restrictions -> short-circuit returns the full input set;
* 2. a restriction present -> the CTE runs and drops the page the user can't
* reach while keeping the reachable ones (behavior unchanged);
* 3. inserting the FIRST pageAccess flips hasRestrictedPagesInWorkspace
* false -> true immediately (the 0->1 transition now uncached, no stale
* window, review F1); it is scoped per workspace.
*/
describe('#348 filterAccessiblePageIds workspace short-circuit (real PG)', () => {
let db: Kysely<any>;
let repo: PagePermissionRepo;
let workspaceId: string;
let otherWorkspaceId: string;
let userId: string;
let spaceId: string;
beforeAll(async () => {
db = getTestDb();
// hasRestrictedPagesInWorkspace is now uncached, and no other cached
// permission path is exercised here, so a no-op cache stub suffices.
const cacheStub = {
get: async () => undefined,
set: async () => undefined,
del: async () => undefined,
} as never;
repo = new PagePermissionRepo(db, new GroupRepo(db), cacheStub);
const ws = await createWorkspace(db);
workspaceId = ws.id;
const other = await createWorkspace(db);
otherWorkspaceId = other.id;
const user = await createUser(db, workspaceId);
userId = user.id;
const space = await createSpace(db, workspaceId);
spaceId = space.id;
});
afterAll(async () => {
await destroyTestDb();
});
it('zero restrictions: short-circuit returns the full input set', async () => {
const p1 = await createPage(db, { workspaceId, spaceId });
const p2 = await createPage(db, { workspaceId, spaceId });
expect(await repo.hasRestrictedPagesInWorkspace(workspaceId)).toBe(false);
const ids = [p1.id, p2.id];
const filtered = await repo.filterAccessiblePageIds({
pageIds: ids,
userId,
workspaceId,
});
expect(new Set(filtered)).toEqual(new Set(ids));
});
it('a restriction present: filters out the page the user cannot reach', async () => {
const openPage = await createPage(db, { workspaceId, spaceId });
const restrictedPage = await createPage(db, { workspaceId, spaceId });
// Add a pageAccess row on restrictedPage with NO matching pagePermissions for
// `userId` → the CTE anti-join marks it inaccessible for this user.
await db
.insertInto('pageAccess')
.values({
pageId: restrictedPage.id,
workspaceId,
spaceId,
accessLevel: 'read',
creatorId: userId,
})
.execute();
// 0->1 transition is reflected immediately (uncached).
expect(await repo.hasRestrictedPagesInWorkspace(workspaceId)).toBe(true);
const filtered = await repo.filterAccessiblePageIds({
pageIds: [openPage.id, restrictedPage.id],
userId,
workspaceId,
});
expect(filtered).toContain(openPage.id);
expect(filtered).not.toContain(restrictedPage.id);
});
it('hasRestrictedPagesInWorkspace is scoped per workspace', async () => {
// The other workspace has no pageAccess rows → still false, unaffected by the
// restriction added above in `workspaceId`.
expect(await repo.hasRestrictedPagesInWorkspace(otherWorkspaceId)).toBe(
false,
);
});
});
@@ -1,7 +1,25 @@
import { Kysely } from 'kysely';
import { WorkspaceRepo } from '@docmost/db/repos/workspace/workspace.repo';
import { CacheKey } from 'src/common/helpers/cache-keys';
import { getTestDb, destroyTestDb, createWorkspace } from './db';
// A minimal Map-backed cache double with a working `del` (the previous `{}` stub
// made bustWorkspaceCache's `del` throw into its own try/catch, so the #348
// invalidation was never actually exercised — review F6).
function makeCacheDouble() {
const store = new Map<string, unknown>();
return {
store,
get: async (k: string) => store.get(k),
set: async (k: string, v: unknown) => {
store.set(k, v);
},
del: async (k: string) => {
store.delete(k);
},
};
}
/**
* A WorkspaceRepo.updateSetting jsonb-MERGE (the html-embed kill-switch
* write-half). Setting a single top-level key must NOT clobber sibling
@@ -15,7 +33,9 @@ describe('WorkspaceRepo.updateSetting (jsonb merge) [integration]', () => {
beforeAll(() => {
db = getTestDb();
// Repos are plain classes taking @InjectKysely() db — instantiate directly.
repo = new WorkspaceRepo(db as any);
// 2nd arg is CACHE_MANAGER (used only to bust the #348 workspace cache); a
// stub is fine here since bustWorkspaceCache is best-effort (try/catch).
repo = new WorkspaceRepo(db as any, {} as any);
});
afterAll(async () => {
@@ -58,3 +78,62 @@ describe('WorkspaceRepo.updateSetting (jsonb merge) [integration]', () => {
expect(updated.settings).toEqual({ htmlEmbed: false });
});
});
/**
* #348 F6 the DomainMiddleware workspace cache (WORKSPACE_SELF_HOSTED /
* WORKSPACE_BY_HOST, 15s TTL) caches security-relevant fields (enforceSso/
* enforceMfa/status). Its correctness rests entirely on bustWorkspaceCache being
* called from every mutator. This exercises the real invalidation with a working
* cache double (not the {} stub, whose del throws-and-swallows): warm the cache
* like DomainMiddleware, mutate, and assert the busted key is gone so a stale
* workspace row can't outlive the mutation.
*/
describe('WorkspaceRepo bustWorkspaceCache invalidation [integration]', () => {
let db: Kysely<any>;
beforeAll(() => {
db = getTestDb();
});
afterAll(async () => {
await destroyTestDb();
});
it('updateSetting busts the self-hosted workspace cache key', async () => {
const cache = makeCacheDouble();
const repo = new WorkspaceRepo(db as any, cache as any);
const ws = await createWorkspace(db, { settings: {} });
// Warm the cache as DomainMiddleware would (self-hosted key).
cache.store.set(CacheKey.WORKSPACE_SELF_HOSTED, ws);
expect(cache.store.has(CacheKey.WORKSPACE_SELF_HOSTED)).toBe(true);
await repo.updateSetting(ws.id, 'htmlEmbed', true);
// The mutation must have invalidated the cached row.
expect(cache.store.has(CacheKey.WORKSPACE_SELF_HOSTED)).toBe(false);
});
it('updateSharingSettings busts the by-host workspace cache key too', async () => {
const cache = makeCacheDouble();
const repo = new WorkspaceRepo(db as any, cache as any);
const ws = await createWorkspace(db, { settings: {} });
// createWorkspace assigns a unique hostname; read it back for the by-host key.
const { hostname } = await db
.selectFrom('workspaces')
.select(['hostname'])
.where('id', '=', ws.id)
.executeTakeFirstOrThrow();
// Warm BOTH keys (self-hosted + by-host); the by-host bust needs the row's
// hostname, which the mutator returns from the DB.
cache.store.set(CacheKey.WORKSPACE_SELF_HOSTED, ws);
cache.store.set(CacheKey.WORKSPACE_BY_HOST(hostname as string), ws);
await repo.updateSharingSettings(ws.id, 'allowInvite', true);
expect(cache.store.has(CacheKey.WORKSPACE_SELF_HOSTED)).toBe(false);
expect(cache.store.has(CacheKey.WORKSPACE_BY_HOST(hostname as string))).toBe(
false,
);
});
});
+32 -115
View File
@@ -167,35 +167,6 @@ function isUuid(value: string): boolean {
return typeof value === "string" && UUID_RE.test(value);
}
/**
* Collab-token cache TTL in milliseconds (issue #435). Read fresh from the
* environment on every mint like collab-session.ts readConfig so tests and a
* live rollback can change it without reloading the module.
*
* Why a cache at all: the live CollabSession registry (#400/#431) keys sessions
* on (wsUrl, pageId, collabToken) for identity isolation (invariant 4). But BOTH
* collab-token sources mint a FRESH token per mutation the in-app provider
* re-signs a JWT whose iat/exp (seconds) changes every second, and the external
* MCP POSTs /auth/collab-token each call so the token in the key changed on
* every op and the session was almost never reused (connect-storms, 25s
* timeouts, zombie sessions). Caching the token per-client keeps the key stable
* across a burst of mutations so ONE session is reused.
*
* Default 5 min: well under the 24h collab-token lifetime AND <= the collab
* session max-age (10 min, MCP_COLLAB_SESSION_MAX_AGE_MS), so the
* permission-staleness window is not widened beyond what #431 already accepted.
* The rollback knob is an EXPLICIT 0 (or a negative number): that DISABLES the
* cache an exact fetch-per-call legacy path, mirroring how idleMs<=0 disables
* the session cache. Unset OR unparseable (e.g. a typo like "5min", "abc") falls
* back to the 5-min default with the cache ON parseInt yields NaN, which is
* treated as "not configured", not as "disabled". So to turn the cache off you
* must set the value to exactly 0, not to garbage.
*/
function readCollabTokenTtlMs(): number {
const raw = parseInt(process.env.MCP_COLLAB_TOKEN_TTL_MS ?? "", 10);
return Number.isFinite(raw) ? Math.max(0, raw) : 5 * 60 * 1000;
}
export class DocmostClient {
private client: AxiosInstance;
private token: string | null = null;
@@ -234,15 +205,6 @@ export class DocmostClient {
// resolvePageId), so only slugId->uuid entries are stored/read here.
private pageIdCache = new Map<string, string>();
// Collab-token cache (issue #435): the last minted collab token plus the
// wall-clock time it was minted, so a burst of content mutations reuses ONE
// token and therefore ONE live CollabSession (whose registry key includes the
// token — #400 invariant 4). Per-instance: a DocmostClient is built per
// user/per chat request, so a cached token can never leak across identities.
// Reset whenever the client's identity changes (login() / this.token cleared);
// bypassed on a forced refresh (the 401/403 reauth path). null = no token yet.
private collabTokenCache: { token: string; mintedAt: number } | null = null;
// Two construction forms:
// - new DocmostClient(config) // discriminated union (current)
// - new DocmostClient(baseURL, email, password) // legacy positional creds
@@ -311,11 +273,8 @@ export class DocmostClient {
if (config && isAuthError && !config._retry && !isLoginRequest) {
config._retry = true;
// Drop the stale token + Authorization header before re-login. Also
// clear the collab-token cache (#435): a new identity/login must not
// keep serving a collab token minted under the old one.
// Drop the stale token + Authorization header before re-login.
this.token = null;
this.collabTokenCache = null;
delete this.client.defaults.headers.common["Authorization"];
try {
await this.login();
@@ -364,9 +323,6 @@ export class DocmostClient {
throw new Error("getToken returned an empty token");
}
this.token = token;
// Identity (re)established: drop any collab token minted under a
// previous identity so the #435 cache can never outlive it.
this.collabTokenCache = null;
this.client.defaults.headers.common["Authorization"] =
`Bearer ${token}`;
})
@@ -389,34 +345,8 @@ export class DocmostClient {
* by this.client's response interceptor; this helper replicates that
* behaviour for collab-token requests: ensure a token, try once, and on an
* expired-token auth error perform a fresh login and retry exactly once.
*
* Collab-token cache (issue #435): both sources the getCollabToken provider
* (in-app agent) AND the REST /auth/collab-token endpoint (external MCP) mint
* a FRESH token per call, whose string therefore changes every op. Since the
* live CollabSession registry keys on the token string (#400/#431 invariant 4),
* that churned the key and defeated session reuse. So we cache the last minted
* token per-client for readCollabTokenTtlMs() and hand it back for a burst of
* mutations, keeping the session key stable. `forceRefresh` bypasses the cache
* (the 401/403 reauth retry uses it, so the retry cannot be handed the same
* stale token that just failed otherwise reauth would be a no-op). TTL 0
* disables the cache: exact fetch-per-call legacy behaviour.
*/
private async getCollabTokenWithReauth(
forceRefresh = false,
): Promise<string> {
const ttl = readCollabTokenTtlMs();
// Serve the cached collab token while it is still fresh (identity isolation
// is preserved: the cache is a per-instance field on a client built per
// user/per chat request, and it is cleared on every identity change).
if (
!forceRefresh &&
ttl > 0 &&
this.collabTokenCache &&
Date.now() - this.collabTokenCache.mintedAt < ttl
) {
return this.collabTokenCache.token;
}
private async getCollabTokenWithReauth(): Promise<string> {
// Collab-token PROVIDER path: when a getCollabToken provider was supplied
// (the internal agent's provenance collab token), use it instead of the
// REST /auth/collab-token endpoint. Re-invoke it once on a 401/403 (e.g. the
@@ -427,13 +357,23 @@ export class DocmostClient {
if (typeof token !== "string" || token.length === 0) {
throw new Error("getCollabToken returned an empty token");
}
return this.rememberCollabToken(token, ttl);
return token;
} catch (e) {
// On an auth error retry EXACTLY once, forcing a refresh so the retry
// re-invokes the provider (bypassing the cache) for a genuinely fresh
// token. `!forceRefresh` bounds it to a single retry (no loop).
if (this.isCollabAuthError(e) && !forceRefresh) {
return this.getCollabTokenWithReauth(true);
const axiosStatus = axios.isAxiosError(e)
? e.response?.status
: undefined;
const attachedStatus = (e as any)?.status;
const isAuthError =
axiosStatus === 401 ||
axiosStatus === 403 ||
attachedStatus === 401 ||
attachedStatus === 403;
if (isAuthError) {
const token = await this.getCollabTokenFn();
if (typeof token !== "string" || token.length === 0) {
throw new Error("getCollabToken returned an empty token");
}
return token;
}
throw e;
}
@@ -441,51 +381,28 @@ export class DocmostClient {
await this.ensureAuthenticated();
try {
const token = await getCollabToken(this.apiUrl, this.token!);
return this.rememberCollabToken(token, ttl);
return await getCollabToken(this.apiUrl, this.token!);
} catch (e) {
// getCollabToken wraps the AxiosError in a plain Error but attaches the
// HTTP status as `.status`, so isCollabAuthError detects an auth failure
// via either the raw AxiosError shape OR the attached status.
if (this.isCollabAuthError(e) && !forceRefresh) {
// Fresh login (which clears this.token AND the collab-token cache), then
// retry exactly once with the cache bypassed via forceRefresh.
// HTTP status as `.status`, so detect an auth failure via either the raw
// AxiosError shape OR the attached status.
const axiosStatus = axios.isAxiosError(e)
? e.response?.status
: undefined;
const attachedStatus = (e as any)?.status;
const isAuthError =
axiosStatus === 401 ||
axiosStatus === 403 ||
attachedStatus === 401 ||
attachedStatus === 403;
if (isAuthError) {
await this.login();
return this.getCollabTokenWithReauth(true);
return await getCollabToken(this.apiUrl, this.token!);
}
throw e;
}
}
/**
* Store a freshly minted collab token in the per-client cache (issue #435) and
* return it unchanged. No-op write when the cache is disabled (ttl<=0) or the
* token is empty, so a disabled cache is exact fetch-per-call legacy behaviour
* and a bad token is never cached.
*/
private rememberCollabToken(token: string, ttl: number): string {
if (ttl > 0 && typeof token === "string" && token.length > 0) {
this.collabTokenCache = { token, mintedAt: Date.now() };
}
return token;
}
/**
* True when an error carries a 401/403 either as a raw AxiosError
* (`error.response.status`) or as the plain-Error `.status` that
* lib/auth-utils.getCollabToken attaches after wrapping the AxiosError.
*/
private isCollabAuthError(e: unknown): boolean {
const axiosStatus = axios.isAxiosError(e) ? e.response?.status : undefined;
const attachedStatus = (e as any)?.status;
return (
axiosStatus === 401 ||
axiosStatus === 403 ||
attachedStatus === 401 ||
attachedStatus === 403
);
}
/**
* Connect to the collaboration websocket, read the live doc, apply
* `transform`, write the result, and wait for the server to persist it
@@ -1,282 +0,0 @@
// Unit tests for the collab-token cache (issue #435). The live CollabSession
// registry (#400/#431) keys sessions on (wsUrl, pageId, collabToken), so a token
// string that changes every op defeats reuse. This cache holds the last minted
// token per DocmostClient for MCP_COLLAB_TOKEN_TTL_MS so a burst of mutations
// reuses ONE token -> ONE session. These tests exercise both mint sources:
// - the getCollabToken PROVIDER path (in-app agent), via a counting provider fn;
// - the REST /auth/collab-token path (external MCP), via a mock http server.
// getCollabTokenWithReauth is private in TS but a plain method on the compiled
// build, so the tests call it directly (same convention as reauth.test.mjs).
import { test, afterEach, after } from "node:test";
import assert from "node:assert/strict";
import http from "node:http";
import { DocmostClient } from "../../build/client.js";
// Restore the env knob after each test so cases do not leak into one another.
const ENV_KEY = "MCP_COLLAB_TOKEN_TTL_MS";
afterEach(() => {
delete process.env[ENV_KEY];
});
// ---------------------------------------------------------------------------
// Small mock server for the REST /auth/collab-token path. Counts collab-token
// mints and can be told to 401 the first N of them (to drive the reauth retry).
// ---------------------------------------------------------------------------
function readBody(req) {
return new Promise((resolve) => {
let raw = "";
req.on("data", (c) => (raw += c));
req.on("end", () => resolve(raw));
});
}
function sendJson(res, status, obj, extra = {}) {
res.writeHead(status, { "Content-Type": "application/json", ...extra });
res.end(JSON.stringify(obj));
}
const openServers = [];
after(async () => {
await Promise.all(
openServers.map((s) => new Promise((r) => s.close(r))),
);
});
// state: { collabCalls, loginCalls, unauthorizedCollabHits }
function spawnCollabServer(state, { collabAuthFailsFor = 0 } = {}) {
return new Promise((resolve) => {
const server = http.createServer(async (req, res) => {
await readBody(req);
if (req.url === "/api/auth/login") {
state.loginCalls++;
// A fresh authToken per login so an identity change is observable.
sendJson(res, 200, { success: true }, {
"Set-Cookie": `authToken=login-${state.loginCalls}; Path=/; HttpOnly`,
});
return;
}
if (req.url === "/api/auth/collab-token") {
state.collabCalls++;
if (state.collabCalls <= collabAuthFailsFor) {
sendJson(res, 401, { message: "Unauthorized" });
return;
}
// Unique token per mint so a stale cached value is distinguishable.
sendJson(res, 200, { data: { token: `collab-${state.collabCalls}` } });
return;
}
sendJson(res, 404, { message: "not found" });
});
server.listen(0, "127.0.0.1", () => {
openServers.push(server);
resolve(`http://127.0.0.1:${server.address().port}/api`);
});
});
}
// ===========================================================================
// PROVIDER path (in-app agent getCollabToken fn)
// ===========================================================================
// A counting provider that returns a distinct token each call so a cached
// (reused) token is visibly the SAME string while a fresh mint is different.
function countingProvider() {
let n = 0;
const fn = async () => {
n++;
return `provider-token-${n}`;
};
return {
fn,
get calls() {
return n;
},
};
}
test("within TTL, repeated calls return the SAME token and mint ONCE (provider path)", async () => {
process.env[ENV_KEY] = "300000"; // 5 min
const p = countingProvider();
const client = new DocmostClient({
apiUrl: "http://127.0.0.1:1/api",
getToken: async () => "access",
getCollabToken: p.fn,
});
const a = await client.getCollabTokenWithReauth();
const b = await client.getCollabTokenWithReauth();
const c = await client.getCollabTokenWithReauth();
assert.equal(a, "provider-token-1");
assert.equal(b, a, "second call reuses the cached token");
assert.equal(c, a, "third call reuses the cached token");
assert.equal(p.calls, 1, "the provider is invoked exactly once within the TTL");
});
test("after TTL expiry a new token is minted (provider path)", async () => {
process.env[ENV_KEY] = "20"; // 20ms TTL
const p = countingProvider();
const client = new DocmostClient({
apiUrl: "http://127.0.0.1:1/api",
getToken: async () => "access",
getCollabToken: p.fn,
});
const a = await client.getCollabTokenWithReauth();
await new Promise((r) => setTimeout(r, 40)); // let the TTL lapse
const b = await client.getCollabTokenWithReauth();
assert.equal(a, "provider-token-1");
assert.equal(b, "provider-token-2", "a fresh token is minted after expiry");
assert.equal(p.calls, 2);
});
test("MCP_COLLAB_TOKEN_TTL_MS=0 disables the cache: mint on EVERY call (provider path)", async () => {
process.env[ENV_KEY] = "0";
const p = countingProvider();
const client = new DocmostClient({
apiUrl: "http://127.0.0.1:1/api",
getToken: async () => "access",
getCollabToken: p.fn,
});
await client.getCollabTokenWithReauth();
await client.getCollabTokenWithReauth();
await client.getCollabTokenWithReauth();
assert.equal(p.calls, 3, "cache disabled -> exact fetch-per-call legacy path");
});
test("a 401 triggers the internal reauth retry, which bypasses the cache and mints fresh (provider path)", async () => {
process.env[ENV_KEY] = "300000";
let n = 0;
const provider = async () => {
n++;
if (n === 1) {
// The FIRST mint fails with an auth error; the internal reauth retry must
// re-invoke the provider (bypassing the empty cache) for a fresh token.
const err = new Error("collab token expired");
err.status = 401;
throw err;
}
return `provider-token-${n}`;
};
const client = new DocmostClient({
apiUrl: "http://127.0.0.1:1/api",
getToken: async () => "access",
getCollabToken: provider,
});
// Cache is empty: mint #1 401s -> the reauth retry mints #2 and caches it.
const tok = await client.getCollabTokenWithReauth();
assert.equal(tok, "provider-token-2", "the post-401 retry token wins");
assert.equal(n, 2, "exactly one failed mint + one retry, no loop");
// The retried token is what got cached (no extra mint on a cache hit).
const cached = await client.getCollabTokenWithReauth();
assert.equal(cached, "provider-token-2");
assert.equal(n, 2, "served from cache, provider not re-invoked");
});
test("forceRefresh=true bypasses a warm cache and mints a fresh token (provider path)", async () => {
process.env[ENV_KEY] = "300000";
const p = countingProvider();
const client = new DocmostClient({
apiUrl: "http://127.0.0.1:1/api",
getToken: async () => "access",
getCollabToken: p.fn,
});
const first = await client.getCollabTokenWithReauth(); // caches token-1
assert.equal(first, "provider-token-1");
// A forced refresh (what the reauth path passes) must NOT return the cached
// token-1; it mints a fresh token-2 and replaces the cache.
const forced = await client.getCollabTokenWithReauth(true);
assert.equal(forced, "provider-token-2", "cache bypassed on forceRefresh");
assert.equal(p.calls, 2);
const cached = await client.getCollabTokenWithReauth();
assert.equal(cached, "provider-token-2", "the fresh token replaced the cache");
assert.equal(p.calls, 2);
});
test("two consecutive mutations keep the SAME token, so the session key is stable (provider path)", async () => {
// The whole point of #435: acquireCollabSession keys on the token, so two
// acquire calls in a burst must be handed the identical token string.
process.env[ENV_KEY] = "300000";
const p = countingProvider();
const client = new DocmostClient({
apiUrl: "http://127.0.0.1:1/api",
getToken: async () => "access",
getCollabToken: p.fn,
});
const t1 = await client.getCollabTokenWithReauth();
const t2 = await client.getCollabTokenWithReauth();
assert.equal(t1, t2, "identical token across two mutations -> one session key");
assert.equal(p.calls, 1);
});
// ===========================================================================
// REST /auth/collab-token path (external MCP)
// ===========================================================================
test("within TTL, the REST /auth/collab-token endpoint is hit ONCE", async () => {
process.env[ENV_KEY] = "300000";
const state = { collabCalls: 0, loginCalls: 0 };
const baseURL = await spawnCollabServer(state);
const client = new DocmostClient(baseURL, "user@example.com", "pw");
const a = await client.getCollabTokenWithReauth();
const b = await client.getCollabTokenWithReauth();
assert.equal(a, "collab-1");
assert.equal(b, a, "cached token reused");
assert.equal(state.collabCalls, 1, "POST /auth/collab-token called once");
});
test("TTL=0 hits the REST endpoint on every call", async () => {
process.env[ENV_KEY] = "0";
const state = { collabCalls: 0, loginCalls: 0 };
const baseURL = await spawnCollabServer(state);
const client = new DocmostClient(baseURL, "user@example.com", "pw");
await client.getCollabTokenWithReauth();
await client.getCollabTokenWithReauth();
assert.equal(state.collabCalls, 2, "cache disabled -> fetch each call");
});
test("401 on REST collab-token re-logs-in and refetches (cache bypassed)", async () => {
process.env[ENV_KEY] = "300000";
const state = { collabCalls: 0, loginCalls: 0 };
// The first collab-token mint 401s; the reauth path logs in and retries.
const baseURL = await spawnCollabServer(state, { collabAuthFailsFor: 1 });
const client = new DocmostClient(baseURL, "user@example.com", "pw");
// Pre-seed a token so the initial call does not perform an initial login.
client.token = "seed";
client.client.defaults.headers.common["Authorization"] = "Bearer seed";
const tok = await client.getCollabTokenWithReauth();
assert.equal(tok, "collab-2", "the post-reauth mint wins, not the failed one");
assert.equal(state.loginCalls, 1, "re-login happened exactly once");
assert.equal(state.collabCalls, 2, "one failed mint + one successful retry");
});
test("a fresh login clears the cache so a collab token cannot outlive the identity", async () => {
process.env[ENV_KEY] = "300000";
const state = { collabCalls: 0, loginCalls: 0 };
const baseURL = await spawnCollabServer(state);
const client = new DocmostClient(baseURL, "user@example.com", "pw");
const before = await client.getCollabTokenWithReauth();
assert.equal(before, "collab-1");
// Simulate an identity change (the 401 interceptor / re-login path calls
// login(), which must drop the cached collab token).
await client.login();
const after = await client.getCollabTokenWithReauth();
assert.equal(after, "collab-2", "cache was invalidated by login(); refetched");
assert.equal(state.collabCalls, 2);
});
@@ -81,10 +81,6 @@ const HOST_CONTRACT_METHODS = [
"insertImage",
"replaceImage",
"insertFootnote",
// draw.io diagrams (#423, stage 1) — read + create + optimistic-locked update
"drawioGet",
"drawioCreate",
"drawioUpdate",
// write (comment)
"createComment",
"resolveComment",