diff --git a/.agents/skills/pr-backlog-reflection/SKILL.md b/.agents/skills/pr-backlog-reflection/SKILL.md index f6f98c4..76b7848 100644 --- a/.agents/skills/pr-backlog-reflection/SKILL.md +++ b/.agents/skills/pr-backlog-reflection/SKILL.md @@ -47,11 +47,11 @@ the work directly revealed a new, evidenced consequence. Classify each valid observation before presenting it: -| Classification | Use when | -| --- | --- | +| Classification | Use when | +| ------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Current-PR finding | It violates the intended contract, creates a credible regression, or is necessary for a supported surface. Keep it out of the backlog list and raise it in the active review/work. | -| Backlog candidate | It is valuable, concrete, and can be delivered independently after this PR. | -| Note only | It lacks enough evidence, duplicates existing work, or has no meaningful outcome. | +| Backlog candidate | It is valuable, concrete, and can be delivered independently after this PR. | +| Note only | It lacks enough evidence, duplicates existing work, or has no meaningful outcome. | ## Check that the work is not already tracked diff --git a/docs/specifications/internal-links.md b/docs/specifications/internal-links.md index 11e51e1..17dad2a 100644 --- a/docs/specifications/internal-links.md +++ b/docs/specifications/internal-links.md @@ -11,7 +11,7 @@ Compendium has exactly two ways to write an internal link, and both persist only - A **`page_link` block** stores its target on the block record's own `referencedRecordId` (Documents only, set today from the editor UI's document picker). - An **inline `[[wiki link]]`** is an ordinary rich-text `link` mark whose href uses a `record:` scheme (Documents or Collections; see `markdown-transcoding.md` for the Markdown ⇄ mark boundary). -Both resolve to a display title the same way, live, at read time — via `resolveInternalLinkTarget(doc, id)` in [`src/lib/data/links.ts`](../../src/lib/data/links.ts). That module is the one place "does this ID still name a Document or Collection, and what's it called now" is answered; `markdown-transcode.ts`, the `documents` service, and `BlockEditor.svelte` all call into it rather than each re-deriving the answer. This is what makes renaming or moving the target a non-event: nothing that points at it needs to change, because nothing that points at it ever stored the title. +Both resolve to a display title the same way, live, at read time — via `resolveInternalLinkTarget(doc, id)` in [`src/lib/data/links.ts`](../../src/lib/data/links.ts). That module is the one place "does this ID still name a Document or Collection, and what's it called now" is answered; `markdown-transcode.ts`, the `documents` service, and `BlockEditor.svelte` all call into it for display, and (since issue #62) `services/records.ts`'s `referencedRecordId` validation and `services/tokens.ts#createToken`'s Document/Collection grant-existence checks call into it for validation — every one of these rather than each re-deriving the answer. This is what makes renaming or moving the target a non-event: nothing that points at it needs to change, because nothing that points at it ever stored the title. `src/lib/data/links.ts` also exports `listOutgoingLinks(doc, documentId)`, which walks a Document's own records (`page_link` blocks and inline `record:` marks alike) and resolves each to the same `{ id, kind, title } | undefined` shape. `listIncomingLinks(doc, targetId)` builds its reverse index from that same function on first use, then incrementally refreshes only a changed link-bearing record or source Document's metadata. It returns the referring Document, the exact referring block, and its live text context. Outgoing and incoming views therefore derive from one ID-backed representation rather than parallel ad hoc scans. diff --git a/docs/specifications/service-layer.md b/docs/specifications/service-layer.md index 52bca07..817fbbd 100644 --- a/docs/specifications/service-layer.md +++ b/docs/specifications/service-layer.md @@ -71,7 +71,7 @@ src/lib/services/ revokeToken(actor, tokenHash) → void ``` -`tokens.ts` and `spaces.ts` are UI-only (`settings/tokens`, `/api/spaces`) — no MCP tool exposes minting or revoking a token or creating a Space. They are nevertheless registered in `services/manifest.ts` with `mcp: false, ui: true`, alongside token listing and audit-history listing, so adapter ownership is enforced without accidentally exposing them over MCP. `tokens.ts#createToken` still validates `allowedSpaceIds` against the workspace's real Spaces before persisting (#188) — a crafted request could otherwise grant a token access to a Space id that merely happens to exist somewhere else, since Space membership alone later authorizes access (`tokenAllowsParent`). +`tokens.ts` and `spaces.ts` are UI-only (`settings/tokens`, `/api/spaces`) — no MCP tool exposes minting or revoking a token or creating a Space. They are nevertheless registered in `services/manifest.ts` with `mcp: false, ui: true`, alongside token listing and audit-history listing, so adapter ownership is enforced without accidentally exposing them over MCP. `tokens.ts#createToken` validates every grant list — `allowedSpaceIds` against the workspace's real Spaces (#188), and (since #62) `allowedDocumentIds`/`allowedCollectionIds` against `resolveInternalLinkTarget` (`data/links.ts`) — before persisting, all three through one shared `validateEvery(ids, existsFn, ErrorClass)` helper. A crafted request could otherwise grant a token access to an id that merely happens to exist somewhere else (or doesn't exist at all), since existence alone later authorizes access (`tokenAllowsParent`) — no permission check is needed for a grant itself, since Phase 0 has no membership model gating who a caller may grant a _future_ token access to. Each function's first parameter is whatever identifies the caller for permission purposes — an `AccessToken` for MCP-originated calls, the fixed `CURRENT_USER` `ActorId` for Phase 0/1 UI calls (see `data-model.md` §1's `ActorId` union; this doesn't need to change). Where MCP and UI calls to the "same" use case need different permission rules (e.g. UI writes are currently unscoped, single-tenant; MCP writes are token-scoped), the service function is the one place that branches on that — not duplicated per adapter. diff --git a/src/lib/services/records.ts b/src/lib/services/records.ts index bc084d9..8c70996 100644 --- a/src/lib/services/records.ts +++ b/src/lib/services/records.ts @@ -5,7 +5,6 @@ import { clientIdForToken, isHeldByClient, releaseAgentHold } from '$lib/server/ import { createRecord as crdtCreateRecord, deleteRecord as crdtDeleteRecord, - getCollection as crdtGetCollection, getDocument as crdtGetDocument, getRecordYText, setRecordReferencedId as crdtSetRecordReferencedId, @@ -13,6 +12,7 @@ import { updateRecordContent, updateRecordProperties } from '$lib/data/records'; +import { resolveInternalLinkTarget } from '$lib/data/links'; import { logAudit } from '$lib/server/audit'; import { reserveRecordLocator, releaseRecordLocator } from '$lib/server/catalog'; import { markdownToRichText } from '$lib/mcp/markdown-transcode'; @@ -69,32 +69,57 @@ export class InvalidLinkTargetError extends Error { // all; it's optional there) is one too. const DOCUMENT_REFERENCE_BLOCK_TYPES: readonly BlockType[] = ['page_link', 'child_pages']; -function validateDocumentReferenceTarget(caller: CallerIdentity, targetId: string): void { - // The target is always a Document, which has its own real shard (#120) — - // resolveParentWorkspaceContext finds it via the catalog locator, falling - // back to the default doc for an untracked/legacy target. +const REFERENCE_TARGET_ERROR_KIND = { document: 'Document', collection: 'Collection' } as const; + +/** + * Validates a `referencedRecordId` against the target kind its block type + * requires — `document` for page_link/child_pages, `collection` for + * collection_view (mirrors the `linkedTarget?.kind === 'collection'` check + * the read side already applies, services/documents.ts's resolveRecordLink, + * closing the write-side gap tracked by issue #37). Built on + * `resolveInternalLinkTarget` (src/lib/data/links.ts) rather than a direct + * `crdtGetDocument`/`crdtGetCollection` call, so "does this ID exist, and as + * which kind" is answered in one shared place instead of reimplemented per + * kind (issue #62). + */ +function validateReferenceTarget( + caller: CallerIdentity, + targetId: string, + kind: 'document' | 'collection' +): void { + // The target has its own real shard (#120) — resolveParentWorkspaceContext + // finds it via the catalog locator, falling back to the default doc for an + // untracked/legacy target. const { doc, parentSpaceId } = resolveParentWorkspaceContext(targetId); - const target = crdtGetDocument(doc, targetId); - if (!target) throw new InvalidLinkTargetError(targetId); + const target = resolveInternalLinkTarget(doc, targetId); + const errorKind = REFERENCE_TARGET_ERROR_KIND[kind]; + if (target?.kind !== kind) throw new InvalidLinkTargetError(targetId, errorKind); if (isAccessToken(caller) && !tokenAllowsParent(caller, targetId, parentSpaceId)) { - throw new InvalidLinkTargetError(targetId); + throw new InvalidLinkTargetError(targetId, errorKind); } } -// A collection_view block's referencedRecordId must resolve to an existing, -// caller-accessible Collection (never a Document) — mirrors the -// `linkedTarget?.kind === 'collection'` check the read side already applies -// (services/documents.ts's resolveRecordLink), closing the write-side gap -// tracked by issue #37. -function validateCollectionReferenceTarget(caller: CallerIdentity, targetId: string): void { - // A Collection has its own real shard too (#120) — same locator-backed - // resolution validateDocumentReferenceTarget uses above. - const { doc, parentSpaceId } = resolveParentWorkspaceContext(targetId); - const target = crdtGetCollection(doc, targetId); - if (!target) throw new InvalidLinkTargetError(targetId, 'Collection'); - if (isAccessToken(caller) && !tokenAllowsParent(caller, targetId, parentSpaceId)) { - throw new InvalidLinkTargetError(targetId, 'Collection'); - } +/** + * The shared guard shape behind every page_link/child_pages/collection_view + * `referencedRecordId` check: the record's own parent must be a Document + * (both block types below only ever exist inside one), and the target must + * resolve to the given kind and be caller-accessible. `createRecord`'s and + * `writeRecord`'s validators (below) differ only in which block types they + * accept and their error wording — that difference is real (writeRecord + * doesn't support retargeting child_pages, whose target is UI-only post- + * creation) and stays in each of them; this is just their shared middle step + * (issue #62). + */ +function requireParentDocumentThenValidateTarget( + caller: CallerIdentity, + doc: Y.Doc, + parentId: string, + targetId: string, + kind: 'document' | 'collection', + parentErrorMessage: string +): void { + if (!crdtGetDocument(doc, parentId)) throw new Error(parentErrorMessage); + validateReferenceTarget(caller, targetId, kind); } const VIEW_TYPES: readonly ViewType[] = ['table', 'board', 'calendar']; @@ -133,17 +158,25 @@ function validateCreateReferencedRecordId( referencedRecordId: string ): void { if (blockType && DOCUMENT_REFERENCE_BLOCK_TYPES.includes(blockType)) { - if (!crdtGetDocument(doc, parentId)) { - throw new Error('page_link and child_pages blocks can only be created inside a Document.'); - } - validateDocumentReferenceTarget(caller, referencedRecordId); + requireParentDocumentThenValidateTarget( + caller, + doc, + parentId, + referencedRecordId, + 'document', + 'page_link and child_pages blocks can only be created inside a Document.' + ); return; } if (blockType === 'collection_view') { - if (!crdtGetDocument(doc, parentId)) { - throw new Error('collection_view blocks can only be created inside a Document.'); - } - validateCollectionReferenceTarget(caller, referencedRecordId); + requireParentDocumentThenValidateTarget( + caller, + doc, + parentId, + referencedRecordId, + 'collection', + 'collection_view blocks can only be created inside a Document.' + ); return; } throw new Error( @@ -268,17 +301,25 @@ function validateReferencedRecordIdWrite( referencedRecordId: string ): void { if (record.blockType === 'page_link') { - if (!crdtGetDocument(doc, record.parentId)) { - throw new Error('page_link blocks can only exist inside a Document.'); - } - validateDocumentReferenceTarget(caller, referencedRecordId); + requireParentDocumentThenValidateTarget( + caller, + doc, + record.parentId, + referencedRecordId, + 'document', + 'page_link blocks can only exist inside a Document.' + ); return; } if (record.blockType === 'collection_view') { - if (!crdtGetDocument(doc, record.parentId)) { - throw new Error('collection_view blocks can only exist inside a Document.'); - } - validateCollectionReferenceTarget(caller, referencedRecordId); + requireParentDocumentThenValidateTarget( + caller, + doc, + record.parentId, + referencedRecordId, + 'collection', + 'collection_view blocks can only exist inside a Document.' + ); return; } throw new Error( diff --git a/src/lib/services/tokens.test.ts b/src/lib/services/tokens.test.ts index 9a12885..ee02074 100644 --- a/src/lib/services/tokens.test.ts +++ b/src/lib/services/tokens.test.ts @@ -1,5 +1,12 @@ import { describe, expect, it } from 'vitest'; -import { createToken, revokeToken, UnknownSpaceError } from './tokens'; +import { + createToken, + revokeToken, + UnknownCollectionError, + UnknownDocumentError, + UnknownSpaceError +} from './tokens'; +import { createCollection, createDocument } from './index'; import { CURRENT_USER } from '$lib/server/current-user'; import { createToken as createRawToken, listTokens } from '$lib/mcp/tokens'; import { queryAuditLog } from '$lib/server/audit'; @@ -75,6 +82,74 @@ describe('service layer: createToken (#188)', () => { expect(listTokens()).toHaveLength(before); }); + + it('grants access to a real Document and a real Collection (issue #62)', () => { + const document = createDocument(CURRENT_USER, { title: 'Real Doc For Token Test' }); + const collection = createCollection(CURRENT_USER, { title: 'Real Collection For Token Test' }); + + const { record } = createToken(CURRENT_USER, { + clientLabel: 'Reference Grant Client', + allowedDocumentIds: [document.id], + allowedCollectionIds: [collection.id], + allowedSpaceIds: [] + }); + + expect(record.allowedDocumentIds).toEqual([document.id]); + expect(record.allowedCollectionIds).toEqual([collection.id]); + }); + + it('rejects a Document id that does not exist, without persisting a token (issue #62)', () => { + const before = listTokens().length; + + expect(() => + createToken(CURRENT_USER, { + clientLabel: 'Document Spoofer', + allowedDocumentIds: ['not-a-real-document-id'], + allowedCollectionIds: [], + allowedSpaceIds: [] + }) + ).toThrow(UnknownDocumentError); + + expect(listTokens()).toHaveLength(before); + }); + + it('rejects a Collection id that does not exist, without persisting a token (issue #62)', () => { + const before = listTokens().length; + + expect(() => + createToken(CURRENT_USER, { + clientLabel: 'Collection Spoofer', + allowedDocumentIds: [], + allowedCollectionIds: ['not-a-real-collection-id'], + allowedSpaceIds: [] + }) + ).toThrow(UnknownCollectionError); + + expect(listTokens()).toHaveLength(before); + }); + + it('rejects a Document id naming a real Collection, and vice versa (issue #62)', () => { + const document = createDocument(CURRENT_USER, { title: 'Doc Not A Collection' }); + const collection = createCollection(CURRENT_USER, { title: 'Collection Not A Doc' }); + + expect(() => + createToken(CURRENT_USER, { + clientLabel: 'Kind Mismatch A', + allowedDocumentIds: [collection.id], + allowedCollectionIds: [], + allowedSpaceIds: [] + }) + ).toThrow(UnknownDocumentError); + + expect(() => + createToken(CURRENT_USER, { + clientLabel: 'Kind Mismatch B', + allowedDocumentIds: [], + allowedCollectionIds: [document.id], + allowedSpaceIds: [] + }) + ).toThrow(UnknownCollectionError); + }); }); describe('service layer: revokeToken (#188)', () => { diff --git a/src/lib/services/tokens.ts b/src/lib/services/tokens.ts index 8b1d134..ac82684 100644 --- a/src/lib/services/tokens.ts +++ b/src/lib/services/tokens.ts @@ -7,7 +7,8 @@ import { type AccessToken } from '$lib/server/token-store'; import { logAudit } from '$lib/server/audit'; -import { actorForCaller, type CallerIdentity } from './permissions'; +import { resolveInternalLinkTarget } from '$lib/data/links'; +import { actorForCaller, resolveParentWorkspaceContext, type CallerIdentity } from './permissions'; /** Thrown when a token-creation request names a Space id that isn't a real Space in this workspace. */ export class UnknownSpaceError extends Error { @@ -17,6 +18,50 @@ export class UnknownSpaceError extends Error { } } +/** Thrown when a token-creation request names a Document id that isn't a real, existing Document. */ +export class UnknownDocumentError extends Error { + constructor(documentId: string) { + super(`${documentId} is not a Document in this workspace.`); + this.name = 'UnknownDocumentError'; + } +} + +/** Thrown when a token-creation request names a Collection id that isn't a real, existing Collection. */ +export class UnknownCollectionError extends Error { + constructor(collectionId: string) { + super(`${collectionId} is not a Collection in this workspace.`); + this.name = 'UnknownCollectionError'; + } +} + +/** + * Throws `new ErrorClass(id)` for the first `id` in `ids` that `existsFn` rejects — the shared + * shape behind every one of `createToken`'s grant-existence checks (Space/Document/Collection + * id lists), so "does every ID in this list exist" is answered once instead of reimplemented + * per list (issue #62). + */ +function validateEvery( + ids: string[], + existsFn: (id: string) => boolean, + ErrorClass: new (id: string) => Error +): void { + for (const id of ids) { + if (!existsFn(id)) throw new ErrorClass(id); + } +} + +/** A Document id naming a real, existing Document — a token grant needs no permission check of its own here (Phase 0 has no membership model gating who a caller may grant a *future* token access to), just existence and kind. */ +function documentExists(id: string): boolean { + const { doc } = resolveParentWorkspaceContext(id); + return resolveInternalLinkTarget(doc, id)?.kind === 'document'; +} + +/** A Collection id naming a real, existing Collection — see {@link documentExists}. */ +function collectionExists(id: string): boolean { + const { doc } = resolveParentWorkspaceContext(id); + return resolveInternalLinkTarget(doc, id)?.kind === 'collection'; +} + export interface CreateTokenInput { clientLabel: string; allowedDocumentIds: string[]; @@ -28,10 +73,11 @@ export interface CreateTokenInput { * Creates a new access token — the service-layer wrapper around `mcp/tokens.ts`'s * `createToken` (validate → mutate → audit, in one place, per `service-layer.md`). * Phase 0 permission: any caller may mint a token, matching `createSpace`/`createDocument`'s - * "single-tenant, no membership model yet" posture. `allowedSpaceIds` is validated against the - * workspace's real Spaces before persisting — a crafted request could otherwise grant a token - * access to a Space id that merely happens to exist somewhere else, since Space membership - * alone later authorizes access (`tokenAllowsParent`). + * "single-tenant, no membership model yet" posture. Every grant list — Spaces, Documents, + * Collections — is validated against what actually exists before persisting: a crafted + * request could otherwise grant a token access to an id that merely happens to exist + * somewhere else, or one that doesn't exist at all, since a dead grant only becomes + * apparent (never matching anything via `tokenAllowsParent`) rather than rejected up front. */ export function createToken( caller: CallerIdentity, @@ -41,9 +87,9 @@ export function createToken( const actor = actorForCaller(caller); const knownSpaceIds = new Set(listSpaces(workspaceId).map((space) => space.id)); - for (const spaceId of input.allowedSpaceIds) { - if (!knownSpaceIds.has(spaceId)) throw new UnknownSpaceError(spaceId); - } + validateEvery(input.allowedSpaceIds, (id) => knownSpaceIds.has(id), UnknownSpaceError); + validateEvery(input.allowedDocumentIds, documentExists, UnknownDocumentError); + validateEvery(input.allowedCollectionIds, collectionExists, UnknownCollectionError); const result = storeCreateToken(input); logAudit({ actor, action: 'create_token', targetRecordId: result.record.tokenHash }); diff --git a/src/routes/settings/tokens/+page.server.ts b/src/routes/settings/tokens/+page.server.ts index c0e529d..15cef99 100644 --- a/src/routes/settings/tokens/+page.server.ts +++ b/src/routes/settings/tokens/+page.server.ts @@ -1,6 +1,12 @@ import { fail } from '@sveltejs/kit'; import { listCollections, listDocuments, listSpaces, listTokens } from '$lib/services'; -import { createToken, revokeToken, UnknownSpaceError } from '$lib/services/tokens'; +import { + createToken, + revokeToken, + UnknownCollectionError, + UnknownDocumentError, + UnknownSpaceError +} from '$lib/services/tokens'; import { formString } from '$lib/server/form-data'; import type { Actions, PageServerLoad } from './$types'; @@ -45,6 +51,12 @@ export const actions: Actions = { if (err instanceof UnknownSpaceError) { return fail(400, { error: 'Invalid Space selection' }); } + if (err instanceof UnknownDocumentError) { + return fail(400, { error: 'Invalid Document selection' }); + } + if (err instanceof UnknownCollectionError) { + return fail(400, { error: 'Invalid Collection selection' }); + } throw err; } diff --git a/src/routes/settings/tokens/page.server.test.ts b/src/routes/settings/tokens/page.server.test.ts index ad0b4ea..130e771 100644 --- a/src/routes/settings/tokens/page.server.test.ts +++ b/src/routes/settings/tokens/page.server.test.ts @@ -94,6 +94,22 @@ describe('routes/settings/tokens/+page.server', () => { expect(record?.allowedSpaceIds).toEqual([space.id]); }); + it('create action rejects a documentId that does not exist (issue #62)', async () => { + const result = await actions.create( + formEvent({ clientLabel: 'Document Spoofer', documentIds: ['not-a-real-document-id'] }) + ); + expect(result).toEqual({ status: 400, data: { error: 'Invalid Document selection' } }); + expect(listTokens().some((t) => t.clientLabel === 'Document Spoofer')).toBe(false); + }); + + it('create action rejects a collectionId that does not exist (issue #62)', async () => { + const result = await actions.create( + formEvent({ clientLabel: 'Collection Spoofer', collectionIds: ['not-a-real-collection-id'] }) + ); + expect(result).toEqual({ status: 400, data: { error: 'Invalid Collection selection' } }); + expect(listTokens().some((t) => t.clientLabel === 'Collection Spoofer')).toBe(false); + }); + it('revoke action fails without a tokenHash', async () => { const result = await actions.revoke(formEvent({})); expect(result).toEqual({ status: 400, data: { error: 'Missing token' } });