From 714c40973d8dc5bc9a3eeb1e9bf7627a3d73b657 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 14:17:54 +0000 Subject: [PATCH 1/4] fix(core): scoped commitMigration() and correct id-validation error class MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two conformance gaps between core and @haverstack/conformance-fixtures, found while implementing haverstack/server#32: - ScopedStack (and Stack) had no permission-checked path for a per-record migration, so a server backing POST /records/:id/migrate had to bypass ScopedStack entirely or hand-duplicate its permission logic. Add Stack.commitMigration()/ScopedStack.commitMigration(), mirroring how update()/restoreVersion() wrap the adapter: write access and ownership via requireUpdatable() (refusing a non-owner write to a _grant record), create authority on toTypeId (closing the same family-crossing escalation create() is already closed against — otherwise a write-holder could migrate any record into _app/_config/_grant without ever holding a create grant there), did/appId and owner-did protection, file-ref gating, and content validated against toTypeId's schema. - Record id-format/reserved-prefix validation (Stack.create()/ ScopedStack.create()) threw StackValidationError (422) instead of StackQueryError (400), disagreeing with conformance-fixtures' pinned 400 expectation for structurally malformed ids — the same reasoning that already makes a malformed pagination cursor a StackQueryError. Message text is unchanged; only the error class/code moves. Updates access-control.md, data-model.md, and wire-format.md to describe the new method and its create-grant requirement. --- docs/spec/access-control.md | 3 +- docs/spec/data-model.md | 5 +- docs/spec/wire-format.md | 2 +- packages/core/src/stack.ts | 115 +++++++++++++++-- packages/core/tests/scoped-stack.test.ts | 150 ++++++++++++++++++++++- packages/core/tests/stack.test.ts | 109 +++++++++++++++- 6 files changed, 366 insertions(+), 18 deletions(-) diff --git a/docs/spec/access-control.md b/docs/spec/access-control.md index 537b784..80b6e98 100644 --- a/docs/spec/access-control.md +++ b/docs/spec/access-control.md @@ -97,7 +97,8 @@ A revocation is a soft delete like any other mutation — the owner can `undelet - **No wildcard `typeId`**: there is no `*` or catch-all. Every grant is opt-in per type. Adding a new type never implicitly inherits existing grants — it starts default-deny. - **Some system types can't be granted at all**: `grant()` refuses `_grant`, `_config`, and `_app`. Each would hand the grantee the machinery the model rests on — minting their own grants, rewriting stack ownership, or registering an app card claiming a DID that isn't theirs (see [App](./identity.md#app)). Other reserved types (`_attachment`, `_entity`, `_group`) stay grantable. The refusal is enforced **again at evaluation**: a `_grant` Record naming one of these families confers nothing, however it came to exist. `grant()` is not the only way a Record gets written — an unscoped `Stack`, a JSON import, or a server mapping a request body onto `Stack` can all mint one — so a rule enforced only at the writing helper would hold only for records that went through it. -- **A `_grant` Record is only writable by the owner acting alone.** Refusing grants _on_ `_grant` closes one route to authority; record-level `write` on a grant Record is another, reaching the same escalation by editing what an existing grant confers — its `actions`, `typeId`, or `granteeEntityId` — rather than by minting a fresh one. So `ScopedStack` refuses `update()`, `associate()`, `dissociate()`, `delete()`, `undelete()`, `restoreVersion()` and `setPermissions()` on any `_grant` Record with `StackPermissionError`, whatever the Record's own `permissions` say, and delegation does not carry it (see [Delegation](#delegation-principal-and-subject)). Nothing legitimate is lost: `grant()` and `revoke()` live on `Stack`, never on `StackClient`, so a scoped caller has no business writing one. +- **A `_grant` Record is only writable by the owner acting alone.** Refusing grants _on_ `_grant` closes one route to authority; record-level `write` on a grant Record is another, reaching the same escalation by editing what an existing grant confers — its `actions`, `typeId`, or `granteeEntityId` — rather than by minting a fresh one. So `ScopedStack` refuses `update()`, `associate()`, `dissociate()`, `delete()`, `undelete()`, `restoreVersion()`, `commitMigration()` and `setPermissions()` on any `_grant` Record with `StackPermissionError`, whatever the Record's own `permissions` say, and delegation does not carry it (see [Delegation](#delegation-principal-and-subject)). Nothing legitimate is lost: `grant()` and `revoke()` live on `Stack`, never on `StackClient`, so a scoped caller has no business writing one. +- **`commitMigration()` additionally requires create authority on `toTypeId`.** Update authority on a Record's current type is not enough by itself to move it into a different family — `ScopedStack.commitMigration()` also demands the same create grant `create()` would require to mint a fresh Record at `toTypeId`. Without this, a write-holder on any ordinary type could migrate a Record into `_app`, `_config`, or `_grant` — families otherwise reachable only through `defineType()`'s system bootstrap — forging system-record membership without ever holding a create grant there. Since those three are ungrantable (above), this closes the family-crossing route the same way it's closed for `create()`. **The fence is on writes only.** `get()`, `query()`, `getVersions()` and `getVersion()` on a `_grant` Record stay on their ordinary gates. Reading how a Record you can already reach came to be is not the escalation the fence exists to stop, and taking history away would leave a write-holder unable to audit the grant they hold — [history](./versioning.md#history-access) is the recovery surface, so losing it costs more than it protects. Snapshot `permissions` are stripped there as everywhere, so a grant's history discloses no more of the sharing graph than its current state does. `restoreVersion()` is a write and stays refused, even though reading the snapshot it would restore does not. `_config` is already unreachable through `Stack.get()`; `_app` keeps record-level `write` for its display fields, fenced only on the bindings a trust decision reads (see [DID bindings](./identity.md#did-bindings)). diff --git a/docs/spec/data-model.md b/docs/spec/data-model.md index 0b70b7f..9f4533e 100644 --- a/docs/spec/data-model.md +++ b/docs/spec/data-model.md @@ -152,7 +152,7 @@ Reserved, library-defined types: `_config@1` ([Stack initialization](../spec.md# ### Type migrations -A Type's defining app is the only serious writer of its own types, so migration is **explicit and owner-driven**, not a side effect of a read or an unrelated write. Disk state changes version only via a deliberate `migrateAll()` pass — the invariant is that all Records of a family sit at version N until the owning app moves them, so `query({ filter: { typeId } })` and grants targeting a type never silently miss not-yet-migrated records. +A Type's defining app is the only serious writer of its own types, so migration is **explicit and owner-driven**, not a side effect of a read or an unrelated write. Disk state changes version only via a deliberate `migrateAll()` pass or a per-record `commitMigration()` call — the invariant is that a Record's `typeId` never moves except through one of these two, so `query({ filter: { typeId } })` and grants targeting a type never silently miss not-yet-migrated records. Apps register migration functions between adjacent Type versions at startup. The library composes them into a full migration graph, so an app that only knows about v3 doesn't need to know that v1 ever existed. @@ -178,7 +178,8 @@ The migration registry is **per-stack-instance** — different stacks can be at - **`presentAt: 'latest'`** — an explicit opt-in on both `get()` and `query()` that applies the registered migration chain in memory before returning. Nothing is written to disk; this is a read-time convenience, never a persistence mechanism. Throws `StackMigrationError` when a matched Record's version can't be reconciled with what this app instance has registered (see stale-writer behavior below). - **`update()` never migrates.** It validates the merge-patched content against the Record's _own current_ stored Type — never the latest — and writes back at the same `typeId`. An unrelated content edit can never fold an invisible schema rewrite into the same version-history entry. - **Path composition** — migrations between adjacent versions are automatically chained (v1→v2→v3), so apps only ever register one step at a time. -- **`migrateAll("com.example.myapp/note")`** is the _only_ way disk state changes version. It eagerly commits all pending migrations for a type family in one deliberate pass — call it at app startup after registering migrations, or after a schema change. It sweeps soft-deleted Records unconditionally (`includeDeleted` is not a caller option in either direction — see [Deletion](./versioning.md#deletion)), validates each migrated result against the target Type's schema before writing, and aborts immediately on the first validation failure (a buggy migration function is a bug to surface, not to paper over by skipping the offending records) — anything already committed earlier in the pass stays committed. Previous content is snapshotted to version history before each write. +- **`migrateAll("com.example.myapp/note")`** eagerly commits all pending migrations for a type family in one deliberate pass — call it at app startup after registering migrations, or after a schema change. It sweeps soft-deleted Records unconditionally (`includeDeleted` is not a caller option in either direction — see [Deletion](./versioning.md#deletion)), validates each migrated result against the target Type's schema before writing, and aborts immediately on the first validation failure (a buggy migration function is a bug to surface, not to paper over by skipping the offending records) — anything already committed earlier in the pass stays committed. Previous content is snapshotted to version history before each write. +- **`commitMigration(id, toTypeId, content)`** is the single-record counterpart, changing one Record's `typeId` and `content` together in one step. Unlike `migrateAll()`, `content` here is supplied by the caller rather than produced by a registered `Migration` function — the client-side app that owns `toTypeId` computes it, and the library validates it against `toTypeId`'s schema exactly as `create()`/`update()` validate against a schema. This is what backs the wire's `POST /records/:id/migrate` (see [Wire format](./wire-format.md#records)); under `ScopedStack` it requires both update authority on the Record as it stands today and create authority on `toTypeId`, the latter closing the same family-crossing escalation `create()` is already closed against (see [Access control](./access-control.md#type-level-grants)). Previous content and `typeId` are snapshotted to version history first, same as `migrateAll()`. **Stale-writer behavior.** A Record whose version this app instance can't reconcile — older than what it's registered _and_ not bridged by a migration path, or newer than anything it has ever `defineType()`'d — is an explicit error (`StackMigrationError`) under `presentAt: 'latest'`, not a silent pass-through. This covers both directions of "the same app at two versions" meeting via a shared stack. Reading the Record as stored (the default, no `presentAt`) always succeeds regardless — the stale-writer signal only fires when the app explicitly asks for the migrated view and the library can't honestly provide one. diff --git a/docs/spec/wire-format.md b/docs/spec/wire-format.md index 88166a4..627d4a2 100644 --- a/docs/spec/wire-format.md +++ b/docs/spec/wire-format.md @@ -256,7 +256,7 @@ When present, the server applies the mutation only if the record's current versi **`entityId` and `principalId` are assigned by the server from the authenticated session, and MUST be ignored if a request body carries them.** They are the two fields that answer "who did this", so a server that echoes back what it was handed makes both self-reported — and `principalId` exists precisely to be the field that isn't (see [Identity § Attribution and what can be trusted](./identity.md#attribution-and-what-can-be-trusted)). A client naming its own `principalId` could dress any write up as a verified app action, defeating the `_app` cross-check that reads it. `ScopedStack` already overrides both regardless of what a caller passes, so a server built on it inherits this; one that maps a request body onto `Stack` directly has to drop them itself. The same applies to `version`, `createdAt`, and `updatedAt`, which the server assigns as it does on any write. `appId` is the deliberate exception — self-reported by design, and never a permission input. For `typeId: "_attachment@1"`, a non-owner requester gets `403` regardless of grants — see [Attachments](./attachments.md#creating-_attachment1-records-directly) for the refusal, its carve-out, and `POST /attachments` as the non-owner-safe combined path. -`POST /records/:id/migrate` is the only way a record's `typeId` changes after creation. Body: `{ "toTypeId": "...", "content": {...} }` — the full post-migration content, computed client-side by the type's owning app (migration functions are app code, not server code) and validated by the server against `toTypeId`'s schema before writing. This is what `stack.update()` uses to commit a pending lazy migration alongside a content patch (a content-only `PATCH` can't carry a `typeId` change), and what `stack.migrateAll()` uses for each record in a batch pass. +`POST /records/:id/migrate` is the only way a record's `typeId` changes after creation. Body: `{ "toTypeId": "...", "content": {...} }` — the full post-migration content, computed client-side by the type's owning app (migration functions are app code, not server code) and validated by the server against `toTypeId`'s schema before writing. This is what `stack.update()` uses to commit a pending lazy migration alongside a content patch (a content-only `PATCH` can't carry a `typeId` change), and what `stack.migrateAll()` uses for each record in a batch pass. `Stack.commitMigration()`/`ScopedStack.commitMigration()` is the client-side entry point that backs this endpoint for a single record — see [Type migrations](./data-model.md#type-migrations). ### Response envelope diff --git a/packages/core/src/stack.ts b/packages/core/src/stack.ts index cb03c68..70e7d22 100644 --- a/packages/core/src/stack.ts +++ b/packages/core/src/stack.ts @@ -481,20 +481,21 @@ const DEFAULT_GC_GRACE_MS = 24 * 60 * 60 * 1000; * Checked before the format check: the Crockford charset already excludes * "_", so a reserved-looking id (e.g. "_config") would otherwise just fail * as a generic format error instead of a specific, actionable one. + * + * Throws StackQueryError, not StackValidationError: a malformed id is + * structurally bad input the request never gets past — it doesn't reach + * type-schema validation — the same reasoning that makes an undecodable + * pagination cursor a StackQueryError rather than a content-validation + * failure. See StackQueryError's doc comment. */ function validateRecordId(id: string): void { if (id.startsWith(RESERVED_ID_PREFIX)) { - throw new StackValidationError([ - { path: 'id', message: `ID "${id}" uses the reserved "${RESERVED_ID_PREFIX}" prefix.` }, - ]); + throw new StackQueryError(`ID "${id}" uses the reserved "${RESERVED_ID_PREFIX}" prefix.`); } if (!isValidIdFormat(id)) { - throw new StackValidationError([ - { - path: 'id', - message: `Invalid ID "${id}": expected 12 lowercase Crockford base-32 characters.`, - }, - ]); + throw new StackQueryError( + `Invalid ID "${id}": expected 12 lowercase Crockford base-32 characters.`, + ); } } @@ -547,6 +548,19 @@ export interface StackClient { getVersions(id: string): Promise; getVersion(id: string, version: number): Promise; restoreVersion(id: string, version: number, opts?: IfVersionOptions): Promise; + /** + * Commit a per-record migration: change `typeId` and `content` together, + * validated against `toTypeId`'s schema. The only way a record's typeId + * changes after creation — see docs/spec/wire-format.md § Migration + * commit. No `ifVersion` precondition: `POST /records/:id/migrate` does + * not accept `If-Match` on the wire (see docs/spec/wire-format.md § + * Optimistic concurrency). + */ + commitMigration( + id: string, + toTypeId: TypeId, + content: Record, + ): Promise; getAttachment(fileId: string): Promise; putAttachment( data: Uint8Array, @@ -1398,6 +1412,53 @@ export class Stack implements StackClient { }); } + /** + * Commit a per-record migration: replace `content` and `typeId` together + * in one step, validated against `toTypeId`'s schema exactly as + * create()/update() validate against a type's schema. The single-record + * counterpart to migrateAll() — content here is supplied by the caller + * (computed client-side by the type's owning app, per + * docs/spec/wire-format.md § Migration commit) rather than a registered + * Migration function. Snapshots the prior state to version history, same + * as update()/restoreVersion(). No `ifVersion` precondition — the wire + * endpoint this backs doesn't accept `If-Match` (see + * docs/spec/wire-format.md § Optimistic concurrency). + */ + async commitMigration( + id: string, + toTypeId: TypeId, + content: Record, + ): Promise { + this.assertOpen(); + const existing = await this.adapter.getRecord(id); + if (!existing) { + throw new StackNotFoundError(`Record not found: "${id}"`); + } + + const type = await this.getTypeCached(toTypeId); + if (!type) { + throw new Error(`Unknown type: "${toTypeId}". Call defineType() first.`); + } + + const errors = [...validateReservedKeys(content), ...validateContent(content, type.schema)]; + if (errors.length > 0) { + throw new StackValidationError(errors); + } + + assertContentSize(content, this.features.maxContentBytes, 'Content'); + + if (id === SYSTEM_TYPES.CONFIG) { + this.checkConfigEntityIdUnchanged( + (existing.content as ConfigContent).entityId, + (content as ConfigContent).entityId, + ); + } + + return this.adapter.commitMigration(id, toTypeId, content, { + snapshot: this.buildVersionSnapshot(existing), + }); + } + /** Uniqueness for every unique binding field a newly created card claims. */ private async checkBindingsOnCreate( typeId: TypeId, @@ -2841,6 +2902,42 @@ export class ScopedStack implements StackClient { return this.stack.restoreVersion(id, version, opts); } + /** + * Commit a per-record migration on behalf of the subject: update + * authority on the record as it stands today — `requireUpdatable()`, the + * same gate `update()` and `restoreVersion()` use, refusing a non-owner + * write to a `_grant` Record — *and* create authority on `toTypeId`, the + * same authority `create()` would demand to mint a fresh record there. + * Without the latter, a requester holding only ordinary write access to + * some Record could migrate it into `_app`/`_config`/`_grant` — families + * otherwise reachable only through `defineType()`'s system bootstrap — + * forging system-record membership without ever holding a create grant + * on it. did/appId (on `_app`) and the owner's own did (on `_entity`) + * are protected the same way update() protects them, checked against + * both the Record's current family and `toTypeId` since migrate replaces + * `content` wholesale rather than patching it. See + * docs/spec/access-control.md § A `_grant` Record is only writable by + * the owner acting alone. + */ + async commitMigration( + id: string, + toTypeId: TypeId, + content: Record, + ): Promise { + const record = await this.requireUpdatable(id); + if (!(await this.checkCreateGrant(toTypeId))) { + throw new StackPermissionError(`No create grant for type "${toTypeId}"`); + } + const existingContent = record.content as Record; + const touchesIdentity = (field: 'did' | 'appId') => content[field] !== existingContent[field]; + this.requireOwnerForAppIdentity(record.typeId, touchesIdentity); + this.requireOwnerForAppIdentity(toTypeId, touchesIdentity); + this.requireOwnerForOwnerDid(record.typeId, content.did); + this.requireOwnerForOwnerDid(toTypeId, content.did); + await this.requireFileRefAccess(toTypeId, content); + return this.stack.commitMigration(id, toTypeId, content); + } + /** * Store bytes and create an _attachment@1 metadata record (create grant * on `_attachment@1` required; anonymous denied), returning that record. diff --git a/packages/core/tests/scoped-stack.test.ts b/packages/core/tests/scoped-stack.test.ts index 2b05193..cc30263 100644 --- a/packages/core/tests/scoped-stack.test.ts +++ b/packages/core/tests/scoped-stack.test.ts @@ -6,6 +6,7 @@ import { StackValidationError, StackConflictError, StackPayloadTooLargeError, + StackQueryError, } from '../src/stack.js'; import { generateId, crockford32Encode } from '../src/id.js'; import { MemoryAdapter, IncapableMemoryAdapter } from '../src/testing.js'; @@ -686,7 +687,7 @@ describe('ScopedStack.create — client-supplied id', () => { test('rejects a malformed id from a grantee', async () => { await expect( stack.asEntity(MEMBER).create(COMMENT, { text: 'hello' }, { id: 'too-short' }), - ).rejects.toThrow(StackValidationError); + ).rejects.toThrow(StackQueryError); }); test('rejects a reserved-prefix id from a grantee', async () => { @@ -694,7 +695,7 @@ describe('ScopedStack.create — client-supplied id', () => { stack .asEntity(MEMBER) .create(COMMENT, { text: 'hello' }, { id: '_' + generateId().slice(1) }), - ).rejects.toThrow(StackValidationError); + ).rejects.toThrow(StackQueryError); }); test('rejects an id whose timestamp is far outside the clock-skew tolerance', async () => { @@ -919,6 +920,151 @@ describe('ScopedStack — grant-based update/delete', () => { }); }); +// ------------------------------------------------------- +// ScopedStack.commitMigration +// ------------------------------------------------------- + +describe('ScopedStack.commitMigration', () => { + const COMMENT_V2 = 'com.example.test/comment@2'; + + beforeEach(async () => { + await stack.defineType(COMMENT, 'Comment', { text: { kind: 'text', required: true } }); + await stack.defineType( + COMMENT_V2, + 'Comment', + { text: { kind: 'text', required: true }, title: { kind: 'string' } }, + { migratesFrom: COMMENT }, + ); + }); + + test('anonymous requester cannot migrate a record', async () => { + const record = await stack.create(COMMENT, { text: 'hello' }); + await expect( + stack.asEntity(null).commitMigration(record.id, COMMENT_V2, { text: 'hello', title: '' }), + ).rejects.toThrow(StackPermissionError); + }); + + test('throws StackNotFoundError for a missing record', async () => { + await expect( + stack.asEntity(OWNER).commitMigration(generateId(), COMMENT_V2, { text: 'hello' }), + ).rejects.toThrow(StackNotFoundError); + }); + + test('an update grant on the record’s current family does not by itself authorize migrating it into a different family', async () => { + await stack.grant(MEMBER, [{ actions: ['update-any'], typeId: COMMENT }]); + const record = await stack.create(COMMENT, { text: 'hello' }, { entityId: STRANGER }); + + await expect( + stack.asEntity(MEMBER).commitMigration(record.id, NOTE, { text: 'hello' }), + ).rejects.toThrow(StackPermissionError); + }); + + test('an update grant on the source family plus a create grant on the destination family together authorize a cross-family migration', async () => { + await stack.grant(MEMBER, [ + { actions: ['update-any'], typeId: COMMENT }, + { actions: ['create'], typeId: NOTE }, + ]); + const record = await stack.create(COMMENT, { text: 'hello' }, { entityId: STRANGER }); + + const migrated = await stack + .asEntity(MEMBER) + .commitMigration(record.id, NOTE, { text: 'hello' }); + expect(migrated.typeId).toBe(NOTE); + }); + + test('a single grant naming both actions on one family covers an ordinary in-family migration', async () => { + await stack.grant(MEMBER, [{ actions: ['update-any', 'create'], typeId: COMMENT }]); + const record = await stack.create(COMMENT, { text: 'hello' }, { entityId: STRANGER }); + + const migrated = await stack + .asEntity(MEMBER) + .commitMigration(record.id, COMMENT_V2, { text: 'hello', title: '' }); + expect(migrated.typeId).toBe(COMMENT_V2); + expect(migrated.content).toEqual({ text: 'hello', title: '' }); + }); + + // _app, _config and _grant are ungrantable (checkCreateGrant() returns + // false for them regardless of what grants exist on the source family), + // so a write-holder can never migrate an ordinary record into one — + // otherwise migrate would be a second way to forge system-record + // membership without ever holding a create grant on it. + test('a write-holder cannot migrate an ordinary record into _app', async () => { + await stack.grant(MEMBER, [{ actions: ['update-any'], typeId: COMMENT }]); + const record = await stack.create(COMMENT, { text: 'hello' }, { entityId: STRANGER }); + + await expect( + stack.asEntity(MEMBER).commitMigration(record.id, '_app@1', { + appId: 'com.example.impostor', + name: 'Impostor', + }), + ).rejects.toThrow(StackPermissionError); + }); + + test('a write-holder cannot migrate an ordinary record into _grant', async () => { + await stack.grant(MEMBER, [{ actions: ['update-any'], typeId: COMMENT }]); + const record = await stack.create(COMMENT, { text: 'hello' }, { entityId: STRANGER }); + + await expect( + stack.asEntity(MEMBER).commitMigration(record.id, '_grant@1', { + typeId: COMMENT, + actions: ['create'], + }), + ).rejects.toThrow(StackPermissionError); + }); + + test('the owner acting alone can migrate a record into a system family', async () => { + const record = await stack.create(COMMENT, { text: 'hello' }); + + const migrated = await stack.asEntity(OWNER).commitMigration(record.id, '_app@1', { + appId: 'com.example.owner-tool', + name: 'Owner Tool', + }); + expect(migrated.typeId).toBe('_app@1'); + }); + + test('a write-holder cannot migrate a _grant record, even to a type they could otherwise create', async () => { + const [grantRecord] = await stack.grant(MEMBER, [{ typeId: NOTE, actions: ['read-own'] }]); + await stack.setPermissions(grantRecord.id, [ + { access: 'entity', entityId: MEMBER, read: true, write: true }, + ]); + await stack.grant(MEMBER, [{ actions: ['create'], typeId: COMMENT }]); + + await expect( + stack.asEntity(MEMBER).commitMigration(grantRecord.id, COMMENT, { text: 'x' }), + ).rejects.toThrow(StackPermissionError); + }); + + test('a write-holder with record-level write on an _app card cannot migrate it out of _app and shed its did', async () => { + const shared = await stack.create('_app@1', { + appId: 'com.example.notes', + name: 'My Notes App', + did: 'did:key:z6MkNotesApp', + }); + await stack.setPermissions(shared.id, [ + { access: 'entity', entityId: MEMBER, read: true, write: true }, + ]); + await stack.grant(MEMBER, [{ actions: ['create'], typeId: COMMENT }]); + + await expect( + stack.asEntity(MEMBER).commitMigration(shared.id, COMMENT, { text: 'hijacked' }), + ).rejects.toThrow(StackPermissionError); + }); + + test('a non-owner cannot claim the owner’s own did while migrating a record into _entity', async () => { + await stack.grant(MEMBER, [ + { actions: ['update-any'], typeId: COMMENT }, + { actions: ['create'], typeId: '_entity@1' }, + ]); + const record = await stack.create(COMMENT, { text: 'hello' }, { entityId: STRANGER }); + + await expect( + stack + .asEntity(MEMBER) + .commitMigration(record.id, '_entity@1', { did: OWNER, name: 'Impostor' }), + ).rejects.toThrow(StackPermissionError); + }); +}); + // ------------------------------------------------------- // ScopedStack.putAttachment — grant-based upload // ------------------------------------------------------- diff --git a/packages/core/tests/stack.test.ts b/packages/core/tests/stack.test.ts index 6c34e52..282d490 100644 --- a/packages/core/tests/stack.test.ts +++ b/packages/core/tests/stack.test.ts @@ -459,20 +459,20 @@ describe('create — client-supplied id', () => { test('rejects an id with the wrong length', async () => { await expect(stack.create(NOTE_V1, { text: 'hello' }, { id: 'too-short' })).rejects.toThrow( - StackValidationError, + StackQueryError, ); }); test('rejects an id with characters outside the Crockford charset', async () => { await expect(stack.create(NOTE_V1, { text: 'hello' }, { id: 'UPPERCASE123' })).rejects.toThrow( - StackValidationError, + StackQueryError, ); }); test('rejects an id using the reserved "_" prefix', async () => { await expect( stack.create(NOTE_V1, { text: 'hello' }, { id: '_' + generateId().slice(1) }), - ).rejects.toThrow(StackValidationError); + ).rejects.toThrow(StackQueryError); }); test('rejects a duplicate id with StackConflictError', async () => { @@ -1031,6 +1031,71 @@ describe('migrateAll', () => { }); }); +// ------------------------------------------------------- +// Stack.commitMigration +// ------------------------------------------------------- + +describe('Stack.commitMigration', () => { + beforeEach(async () => { + await stack.defineType( + NOTE_V2, + 'Note', + { + text: { kind: 'text', required: true }, + title: { kind: 'string' }, + }, + { migratesFrom: NOTE_V1 }, + ); + }); + + test('changes typeId and content together, bumping version', async () => { + const record = await stack.create(NOTE_V1, { text: 'hello' }); + + const migrated = await stack.commitMigration(record.id, NOTE_V2, { + text: 'hello', + title: 'pinned', + }); + + expect(migrated.typeId).toBe(NOTE_V2); + expect(migrated.content).toEqual({ text: 'hello', title: 'pinned' }); + expect(migrated.version).toBe(2); + expect((await adapter.getRecord(record.id))?.typeId).toBe(NOTE_V2); + }); + + test('snapshots the pre-migration typeId and content to version history', async () => { + const record = await stack.create(NOTE_V1, { text: 'original' }); + await stack.commitMigration(record.id, NOTE_V2, { text: 'original', title: '' }); + + const versions = await stack.getVersions(record.id); + expect(versions.length).toBe(1); + expect(versions[0].typeId).toBe(NOTE_V1); + expect(versions[0].content).toEqual({ text: 'original' }); + }); + + test('validates content against toTypeId’s schema', async () => { + const record = await stack.create(NOTE_V1, { text: 'hello' }); + + await expect( + stack.commitMigration(record.id, NOTE_V2, { title: 'missing text' }), + ).rejects.toThrow(StackValidationError); + expect((await adapter.getRecord(record.id))?.typeId).toBe(NOTE_V1); // never committed + }); + + test('throws for an unregistered toTypeId', async () => { + const record = await stack.create(NOTE_V1, { text: 'hello' }); + + await expect( + stack.commitMigration(record.id, 'com.example.test/note@99', { text: 'hello' }), + ).rejects.toThrow('Unknown type'); + }); + + test('throws StackNotFoundError for a missing record', async () => { + await expect( + stack.commitMigration(generateId(), NOTE_V2, { text: 'hello', title: '' }), + ).rejects.toThrow(StackNotFoundError); + }); +}); + // ------------------------------------------------------- // restoreVersion — typeId and validation // ------------------------------------------------------- @@ -1535,6 +1600,33 @@ describe('_config protections', () => { await seedConfig('owner-123'); await expect(stack.asEntity('owner-123').delete(CONFIG_ID)).rejects.toThrow(StackConflictError); }); + + test('commitMigration() rejects a change to entityId', async () => { + await seedConfig('owner-123'); + await stack.defineType('_config@2', 'Config', { + entityId: { kind: 'string', required: true }, + timezone: { kind: 'string' }, + }); + + await expect( + stack.commitMigration(CONFIG_ID, '_config@2', { entityId: 'someone-else' }), + ).rejects.toThrow(StackConflictError); + expect((await adapter.getRecord(CONFIG_ID))?.typeId).toBe(CONFIG_TYPE); // never committed + }); + + test('commitMigration() allows the same entityId', async () => { + await seedConfig('owner-123'); + await stack.defineType('_config@2', 'Config', { + entityId: { kind: 'string', required: true }, + timezone: { kind: 'string' }, + }); + + const migrated = await stack.commitMigration(CONFIG_ID, '_config@2', { + entityId: 'owner-123', + timezone: 'America/New_York', + }); + expect(migrated.typeId).toBe('_config@2'); + }); }); // ------------------------------------------------------- @@ -2160,6 +2252,17 @@ describe('reserved content keys', () => { }, ); + test.each(['__proto__', 'constructor', 'prototype'])( + 'commitMigration() rejects a %s content key', + async (key) => { + const record = await stack.create(NOTE_V1, { text: 'hi' }); + + await expect(stack.commitMigration(record.id, NOTE_V1, withKey(key, 'x'))).rejects.toThrow( + StackValidationError, + ); + }, + ); + test('ordinary undeclared fields still pass — permitted by design', async () => { const record = await stack.create(NOTE_V1, { text: 'hi', extra: 'kept' }); From 1a67c8f9442b3e676ac8f34f0aa1fc0de46d3825 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 18:54:11 +0000 Subject: [PATCH 2/4] fix(core)!: make commitMigration() owner-only and add its integrity checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the review of #171. commitMigration() writes a full content replacement under a new typeId, so it is create-shaped at the destination and update-shaped over the record as it stands — but it inherited only create()'s schema validation, reserved-key check and the _config guard. Every other gate the sibling write paths apply was absent, making migrate a second, unguarded route to state create()/update() refuse to reach. ScopedStack.commitMigration() is now owner-acting-alone, replacing the update-grant + create-grant model. This matches the bulk path: migrateAll() lives on Stack and is deliberately absent from StackClient, for the same reason grant()/revoke() are, so the per-record verb now carries the restriction the family-wide one already had. The grant-based version was reachable as a privilege escalation. Holding a create grant on _attachment@1 (a grantable type) plus write access to any record they authored, a requester could migrate that record into the family naming any fileId, then read the bytes through canAccessFile()'s uploader clause — the escalation create()'s non-owner _attachment@1 carve-out exists to refuse, reached by a path that did not apply it. Stack.commitMigration() gains the integrity checks it owed regardless of caller, since Stack is also reachable directly: - DID binding immutability across the union of the source and destination families' binding fields, so a card can neither shed its did by migrating out of _entity/_app nor pick one up on the way in. Previously a migration could move an _entity card onto another DID, which update() refuses via checkBindingImmutable(). - DID binding uniqueness in the destination family, excluding the record itself. Previously two cards could end up claiming one did, which create()/update() refuse with StackConflictError. - _attachment@1 fileId/mimeType/size immutability, asked value-wise rather than presence-wise since a full replacement necessarily re-sends every required field. Repointing fileId was the sharpest of these. - The mimeType-establishment check for a record arriving from outside the _attachment family, matching create(). - Migrating into _group is refused: a group's admin roster entry is stamped at creation and the adapter's commitMigration() writes typeId and content alone, so it would produce a group nobody but the owner can manage. Version-to-version migration within _group stays open and carries the existing roster. The id-validation change from the previous commit is unaffected. Specs updated to match: access-control.md replaces the create-grant bullet with the owner-only rule and its rationale, data-model.md documents the integrity checks, wire-format.md states that a server serves POST /records/:id/migrate to the owner and answers 403 otherwise, and notes the endpoint's absence from the If-Match list. Claude-Session: https://claude.ai/code/session_01JpQomi5W2T9zzS29wAaJFq --- docs/spec/access-control.md | 6 +- docs/spec/data-model.md | 4 +- docs/spec/wire-format.md | 2 +- packages/core/src/stack.ts | 164 +++++++++++++++++++---- packages/core/tests/scoped-stack.test.ts | 115 +++++++++++++--- packages/core/tests/stack.test.ts | 142 ++++++++++++++++++++ 6 files changed, 386 insertions(+), 47 deletions(-) diff --git a/docs/spec/access-control.md b/docs/spec/access-control.md index 80b6e98..939fc74 100644 --- a/docs/spec/access-control.md +++ b/docs/spec/access-control.md @@ -97,8 +97,10 @@ A revocation is a soft delete like any other mutation — the owner can `undelet - **No wildcard `typeId`**: there is no `*` or catch-all. Every grant is opt-in per type. Adding a new type never implicitly inherits existing grants — it starts default-deny. - **Some system types can't be granted at all**: `grant()` refuses `_grant`, `_config`, and `_app`. Each would hand the grantee the machinery the model rests on — minting their own grants, rewriting stack ownership, or registering an app card claiming a DID that isn't theirs (see [App](./identity.md#app)). Other reserved types (`_attachment`, `_entity`, `_group`) stay grantable. The refusal is enforced **again at evaluation**: a `_grant` Record naming one of these families confers nothing, however it came to exist. `grant()` is not the only way a Record gets written — an unscoped `Stack`, a JSON import, or a server mapping a request body onto `Stack` can all mint one — so a rule enforced only at the writing helper would hold only for records that went through it. -- **A `_grant` Record is only writable by the owner acting alone.** Refusing grants _on_ `_grant` closes one route to authority; record-level `write` on a grant Record is another, reaching the same escalation by editing what an existing grant confers — its `actions`, `typeId`, or `granteeEntityId` — rather than by minting a fresh one. So `ScopedStack` refuses `update()`, `associate()`, `dissociate()`, `delete()`, `undelete()`, `restoreVersion()`, `commitMigration()` and `setPermissions()` on any `_grant` Record with `StackPermissionError`, whatever the Record's own `permissions` say, and delegation does not carry it (see [Delegation](#delegation-principal-and-subject)). Nothing legitimate is lost: `grant()` and `revoke()` live on `Stack`, never on `StackClient`, so a scoped caller has no business writing one. -- **`commitMigration()` additionally requires create authority on `toTypeId`.** Update authority on a Record's current type is not enough by itself to move it into a different family — `ScopedStack.commitMigration()` also demands the same create grant `create()` would require to mint a fresh Record at `toTypeId`. Without this, a write-holder on any ordinary type could migrate a Record into `_app`, `_config`, or `_grant` — families otherwise reachable only through `defineType()`'s system bootstrap — forging system-record membership without ever holding a create grant there. Since those three are ungrantable (above), this closes the family-crossing route the same way it's closed for `create()`. +- **A `_grant` Record is only writable by the owner acting alone.** Refusing grants _on_ `_grant` closes one route to authority; record-level `write` on a grant Record is another, reaching the same escalation by editing what an existing grant confers — its `actions`, `typeId`, or `granteeEntityId` — rather than by minting a fresh one. So `ScopedStack` refuses `update()`, `associate()`, `dissociate()`, `delete()`, `undelete()`, `restoreVersion()` and `setPermissions()` on any `_grant` Record with `StackPermissionError`, whatever the Record's own `permissions` say, and delegation does not carry it (see [Delegation](#delegation-principal-and-subject)). Nothing legitimate is lost: `grant()` and `revoke()` live on `Stack`, never on `StackClient`, so a scoped caller has no business writing one. +- **`commitMigration()` is owner-acting-alone, and no grant substitutes for it.** Moving a Record between type families is not something record-level `write` or an `update` grant confers, in any combination: `ScopedStack.commitMigration()` refuses every requester but the owner acting alone, delegation included. This mirrors the bulk path — `migrateAll()` lives on `Stack` and is absent from `StackClient` for the same reason `grant()`/`revoke()` are — so the per-record verb carries the restriction the family-wide one already had, instead of introducing a grant model beside it. + + The restriction is what makes the verb safe to expose. `commitMigration()` replaces `content` and `typeId` wholesale, so it is create-shaped at the destination and update-shaped over the Record as it stands: a grant-based version would have to re-derive every gate `create()` applies _and_ every gate `update()` applies, and would reopen each one it missed. The sharpest is the non-owner `_attachment@1` refusal (see [Attachments](./attachments.md#creating-_attachment1-records-directly)) — a requester holding a create grant on `_attachment@1` and write access to any Record they authored could otherwise migrate that Record into the family naming any `fileId`, then read the bytes through the uploader clause. Ordinary write access to a Record is not consent to move it between families. **The fence is on writes only.** `get()`, `query()`, `getVersions()` and `getVersion()` on a `_grant` Record stay on their ordinary gates. Reading how a Record you can already reach came to be is not the escalation the fence exists to stop, and taking history away would leave a write-holder unable to audit the grant they hold — [history](./versioning.md#history-access) is the recovery surface, so losing it costs more than it protects. Snapshot `permissions` are stripped there as everywhere, so a grant's history discloses no more of the sharing graph than its current state does. `restoreVersion()` is a write and stays refused, even though reading the snapshot it would restore does not. `_config` is already unreachable through `Stack.get()`; `_app` keeps record-level `write` for its display fields, fenced only on the bindings a trust decision reads (see [DID bindings](./identity.md#did-bindings)). diff --git a/docs/spec/data-model.md b/docs/spec/data-model.md index 9f4533e..acd226c 100644 --- a/docs/spec/data-model.md +++ b/docs/spec/data-model.md @@ -179,7 +179,9 @@ The migration registry is **per-stack-instance** — different stacks can be at - **`update()` never migrates.** It validates the merge-patched content against the Record's _own current_ stored Type — never the latest — and writes back at the same `typeId`. An unrelated content edit can never fold an invisible schema rewrite into the same version-history entry. - **Path composition** — migrations between adjacent versions are automatically chained (v1→v2→v3), so apps only ever register one step at a time. - **`migrateAll("com.example.myapp/note")`** eagerly commits all pending migrations for a type family in one deliberate pass — call it at app startup after registering migrations, or after a schema change. It sweeps soft-deleted Records unconditionally (`includeDeleted` is not a caller option in either direction — see [Deletion](./versioning.md#deletion)), validates each migrated result against the target Type's schema before writing, and aborts immediately on the first validation failure (a buggy migration function is a bug to surface, not to paper over by skipping the offending records) — anything already committed earlier in the pass stays committed. Previous content is snapshotted to version history before each write. -- **`commitMigration(id, toTypeId, content)`** is the single-record counterpart, changing one Record's `typeId` and `content` together in one step. Unlike `migrateAll()`, `content` here is supplied by the caller rather than produced by a registered `Migration` function — the client-side app that owns `toTypeId` computes it, and the library validates it against `toTypeId`'s schema exactly as `create()`/`update()` validate against a schema. This is what backs the wire's `POST /records/:id/migrate` (see [Wire format](./wire-format.md#records)); under `ScopedStack` it requires both update authority on the Record as it stands today and create authority on `toTypeId`, the latter closing the same family-crossing escalation `create()` is already closed against (see [Access control](./access-control.md#type-level-grants)). Previous content and `typeId` are snapshotted to version history first, same as `migrateAll()`. +- **`commitMigration(id, toTypeId, content)`** is the single-record counterpart, changing one Record's `typeId` and `content` together in one step. Unlike `migrateAll()`, `content` here is supplied by the caller rather than produced by a registered `Migration` function — the client-side app that owns `toTypeId` computes it, and the library validates it against `toTypeId`'s schema exactly as `create()`/`update()` validate against a schema. This is what backs the wire's `POST /records/:id/migrate` (see [Wire format](./wire-format.md#records)). Under `ScopedStack` it is **owner-acting-alone**, matching `migrateAll()`'s own absence from `StackClient` — no grant or record-level `write` substitutes for it (see [Access control](./access-control.md#type-level-grants)). Previous content and `typeId` are snapshotted to version history first, same as `migrateAll()`. + + Because `content` is a full replacement written under a new `typeId`, `commitMigration()` is create-shaped at the destination and update-shaped over the Record as it stands, and owes both sets of integrity checks. DID bindings are held to immutability across the union of the two families' binding fields — a card can neither shed its `did` by migrating out of `_entity`/`_app` nor pick one up on the way in — and to uniqueness in the destination family (see [Identity § DID bindings](./identity.md#did-bindings)). An `_attachment@1` Record's `fileId`, `mimeType` and `size` stay immutable, and a Record arriving from outside that family is held to the same mimeType-establishment check `create()` applies. Migrating _into_ `_group` is refused outright: a group's `admin` roster entry is stamped at creation and a migration cannot stamp one, so it would produce a group nobody but the owner can manage — version-to-version migration within `_group` stays open and carries the existing roster with it. **Stale-writer behavior.** A Record whose version this app instance can't reconcile — older than what it's registered _and_ not bridged by a migration path, or newer than anything it has ever `defineType()`'d — is an explicit error (`StackMigrationError`) under `presentAt: 'latest'`, not a silent pass-through. This covers both directions of "the same app at two versions" meeting via a shared stack. Reading the Record as stored (the default, no `presentAt`) always succeeds regardless — the stale-writer signal only fires when the app explicitly asks for the migrated view and the library can't honestly provide one. diff --git a/docs/spec/wire-format.md b/docs/spec/wire-format.md index 627d4a2..7af3c6b 100644 --- a/docs/spec/wire-format.md +++ b/docs/spec/wire-format.md @@ -256,7 +256,7 @@ When present, the server applies the mutation only if the record's current versi **`entityId` and `principalId` are assigned by the server from the authenticated session, and MUST be ignored if a request body carries them.** They are the two fields that answer "who did this", so a server that echoes back what it was handed makes both self-reported — and `principalId` exists precisely to be the field that isn't (see [Identity § Attribution and what can be trusted](./identity.md#attribution-and-what-can-be-trusted)). A client naming its own `principalId` could dress any write up as a verified app action, defeating the `_app` cross-check that reads it. `ScopedStack` already overrides both regardless of what a caller passes, so a server built on it inherits this; one that maps a request body onto `Stack` directly has to drop them itself. The same applies to `version`, `createdAt`, and `updatedAt`, which the server assigns as it does on any write. `appId` is the deliberate exception — self-reported by design, and never a permission input. For `typeId: "_attachment@1"`, a non-owner requester gets `403` regardless of grants — see [Attachments](./attachments.md#creating-_attachment1-records-directly) for the refusal, its carve-out, and `POST /attachments` as the non-owner-safe combined path. -`POST /records/:id/migrate` is the only way a record's `typeId` changes after creation. Body: `{ "toTypeId": "...", "content": {...} }` — the full post-migration content, computed client-side by the type's owning app (migration functions are app code, not server code) and validated by the server against `toTypeId`'s schema before writing. This is what `stack.update()` uses to commit a pending lazy migration alongside a content patch (a content-only `PATCH` can't carry a `typeId` change), and what `stack.migrateAll()` uses for each record in a batch pass. `Stack.commitMigration()`/`ScopedStack.commitMigration()` is the client-side entry point that backs this endpoint for a single record — see [Type migrations](./data-model.md#type-migrations). +`POST /records/:id/migrate` is the only way a record's `typeId` changes after creation. Body: `{ "toTypeId": "...", "content": {...} }` — the full post-migration content, computed client-side by the type's owning app (migration functions are app code, not server code) and validated by the server against `toTypeId`'s schema before writing. This is what `stack.update()` uses to commit a pending lazy migration alongside a content patch (a content-only `PATCH` can't carry a `typeId` change), and what `stack.migrateAll()` uses for each record in a batch pass. `Stack.commitMigration()`/`ScopedStack.commitMigration()` is the client-side entry point that backs this endpoint for a single record — see [Type migrations](./data-model.md#type-migrations). A server built on `ScopedStack` serves this endpoint to the **stack owner** and answers `403` otherwise: migration is owner-driven, and no grant confers it (see [Access control](./access-control.md#type-level-grants)). This endpoint is absent from the `If-Match` list above, so a migration commit is unconditional last-writer-wins. ### Response envelope diff --git a/packages/core/src/stack.ts b/packages/core/src/stack.ts index 70e7d22..58e48fc 100644 --- a/packages/core/src/stack.ts +++ b/packages/core/src/stack.ts @@ -1423,6 +1423,13 @@ export class Stack implements StackClient { * as update()/restoreVersion(). No `ifVersion` precondition — the wire * endpoint this backs doesn't accept `If-Match` (see * docs/spec/wire-format.md § Optimistic concurrency). + * + * Because `content` is a full replacement written under a new `typeId`, + * this is create-shaped at the destination *and* update-shaped over the + * record as it stands, so it owes both sets of integrity checks — the + * binding rules, the attachment rules, and `_config`'s. Missing either + * half would make migrate a second, unguarded write path to the same + * state create()/update() refuse to reach. */ async commitMigration( id: string, @@ -1447,6 +1454,37 @@ export class Stack implements StackClient { assertContentSize(content, this.features.maxContentBytes, 'Content'); + const fromFamily = baseIdOf(existing.typeId); + const toFamily = baseIdOf(toTypeId); + const existingContent = existing.content as Record; + + // A `_group` record's `admin` roster entry is stamped by create(), the + // single site that does it — migrate cannot, since the adapter's + // commitMigration() writes `typeId` and `content` alone and leaves + // associations untouched. Minting one here would produce a group with + // an empty roster, manageable by nobody but the owner, so migrating + // *into* the family is refused. Version-to-version stays open and + // carries the existing roster with it. + if (toFamily === SYSTEM_TYPES.GROUP && fromFamily !== SYSTEM_TYPES.GROUP) { + throw new StackConflictError( + 'Cannot migrate a record into _group: a group’s admin roster is stamped at creation. ' + + 'Create the group instead.', + ); + } + + if (fromFamily === SYSTEM_TYPES.ATTACHMENT) { + this.checkAttachmentImmutableOnMigrate( + existingContent as unknown as AttachmentContent, + content as unknown as AttachmentContent, + ); + } else if (toFamily === SYSTEM_TYPES.ATTACHMENT) { + // A record arriving from outside the family stakes a fresh claim on + // its fileId, exactly as create() does — so it owes create()'s check. + await this.checkAttachmentMimeTypeOnCreate(content as unknown as AttachmentContent); + } + + await this.checkBindingsOnMigrate(existing.typeId, toTypeId, id, existingContent, content); + if (id === SYSTEM_TYPES.CONFIG) { this.checkConfigEntityIdUnchanged( (existing.content as ConfigContent).entityId, @@ -1494,6 +1532,44 @@ export class Stack implements StackClient { } } + /** + * Bindings across a migration. `content` is a full replacement rather + * than a patch, so there is no "field absent from the patch" case to + * exempt: every binding field either keeps its value, moves to a new + * one, or is shed by omission — and immutability refuses the last two + * whichever family they happen in. Asked across the union of the two + * families' binding fields, so a card can neither shed its DID by + * migrating out of `_entity`/`_app` nor pick one up on the way in. + * + * Uniqueness is asked only of the destination family, which is where the + * record's claim lives once the write lands, and excludes the record + * itself — re-sending the value it already holds claims nothing. + * See docs/spec/identity.md § DID bindings. + */ + private async checkBindingsOnMigrate( + fromTypeId: TypeId, + toTypeId: TypeId, + id: RecordId, + existing: Record, + content: Record, + ): Promise { + const fromFamily = baseIdOf(fromTypeId); + const toFamily = baseIdOf(toTypeId); + + const checked = new Set(); + for (const family of fromFamily === toFamily ? [fromFamily] : [fromFamily, toFamily]) { + for (const field of bindingFieldsOf(family)) { + if (checked.has(field)) continue; + checked.add(field); + this.checkBindingImmutable(family, field, existing[field], content[field]); + } + } + + for (const field of uniqueBindingFieldsOf(toFamily)) { + await this.checkBindingUnique(toFamily, field, content[field], id); + } + } + /** * A unique binding field is what a lookup resolves *by* — a record's * `principalId` by `_app.did`, its `entityId` by `_entity.did`. Two cards @@ -1645,6 +1721,40 @@ export class Stack implements StackClient { } } + /** + * The same immutability checkAttachmentImmutableFields() enforces, asked + * value-wise instead of presence-wise: a migration replaces content + * wholesale, so it necessarily re-sends `mimeType`, `fileId` and `size` + * (all required) and a presence check would refuse every migration. Only + * an actual change is a violation. + * + * Repointing `fileId` is the one that matters most: an `_attachment@1` + * record naming a fileId is what canAccessFile()'s uploader clause reads, + * so moving an existing record onto another file's hash is a route to + * bytes the record's author never uploaded. + */ + private checkAttachmentImmutableOnMigrate( + existing: AttachmentContent, + next: AttachmentContent, + ): void { + const errors: ValidationError[] = []; + if (next.mimeType !== existing.mimeType) { + errors.push({ + path: 'mimeType', + message: 'mimeType is immutable after creation; delete and re-upload to change it', + }); + } + if (next.fileId !== existing.fileId) { + errors.push({ path: 'fileId', message: 'fileId is immutable' }); + } + if (next.size !== existing.size) { + errors.push({ path: 'size', message: 'size is immutable' }); + } + if (errors.length > 0) { + throw new StackValidationError(errors); + } + } + /** * `_config.entityId` defines stack ownership; neither update() nor * restoreVersion() may change it. A conflict with stack integrity, not a @@ -2903,38 +3013,40 @@ export class ScopedStack implements StackClient { } /** - * Commit a per-record migration on behalf of the subject: update - * authority on the record as it stands today — `requireUpdatable()`, the - * same gate `update()` and `restoreVersion()` use, refusing a non-owner - * write to a `_grant` Record — *and* create authority on `toTypeId`, the - * same authority `create()` would demand to mint a fresh record there. - * Without the latter, a requester holding only ordinary write access to - * some Record could migrate it into `_app`/`_config`/`_grant` — families - * otherwise reachable only through `defineType()`'s system bootstrap — - * forging system-record membership without ever holding a create grant - * on it. did/appId (on `_app`) and the owner's own did (on `_entity`) - * are protected the same way update() protects them, checked against - * both the Record's current family and `toTypeId` since migrate replaces - * `content` wholesale rather than patching it. See - * docs/spec/access-control.md § A `_grant` Record is only writable by - * the owner acting alone. + * Commit a per-record migration — **the owner acting alone, only**. + * + * Migration is owner-driven by design: `migrateAll()`, the bulk path, + * lives on `Stack` and is deliberately absent from `StackClient`, the + * same way `grant()`/`revoke()` are. This is its per-record counterpart + * and carries the same restriction, rather than inventing a grant model + * that the bulk path deliberately doesn't have. + * + * The restriction is what makes the verb safe to expose at all. Migrate + * replaces `content` and `typeId` wholesale, so a grant-based version + * would have to re-derive every gate `create()` applies at the + * destination *and* every gate `update()` applies over the existing + * content, and would reopen each one it missed. The sharpest is the + * non-owner `_attachment@1` refusal create() carries: without it, a + * requester holding a create grant on `_attachment@1` and write access + * to any record they authored could migrate that record into the family + * naming any `fileId`, then read the bytes through canAccessFile()'s + * uploader clause — the exact escalation that carve-out exists to refuse + * (see docs/spec/attachments.md § Creating `_attachment@1` records + * directly). Ordinary write access to a record is not consent to move it + * between families. + * + * A server implementing `POST /records/:id/migrate` therefore serves it + * to the stack owner and answers 403 otherwise. See + * docs/spec/data-model.md § Type migrations. */ async commitMigration( id: string, toTypeId: TypeId, content: Record, ): Promise { - const record = await this.requireUpdatable(id); - if (!(await this.checkCreateGrant(toTypeId))) { - throw new StackPermissionError(`No create grant for type "${toTypeId}"`); - } - const existingContent = record.content as Record; - const touchesIdentity = (field: 'did' | 'appId') => content[field] !== existingContent[field]; - this.requireOwnerForAppIdentity(record.typeId, touchesIdentity); - this.requireOwnerForAppIdentity(toTypeId, touchesIdentity); - this.requireOwnerForOwnerDid(record.typeId, content.did); - this.requireOwnerForOwnerDid(toTypeId, content.did); - await this.requireFileRefAccess(toTypeId, content); + if (!this.ownerActingAlone) { + throw new StackPermissionError('Only the stack owner may commit a migration'); + } return this.stack.commitMigration(id, toTypeId, content); } diff --git a/packages/core/tests/scoped-stack.test.ts b/packages/core/tests/scoped-stack.test.ts index cc30263..bea86b6 100644 --- a/packages/core/tests/scoped-stack.test.ts +++ b/packages/core/tests/scoped-stack.test.ts @@ -950,7 +950,21 @@ describe('ScopedStack.commitMigration', () => { ).rejects.toThrow(StackNotFoundError); }); - test('an update grant on the record’s current family does not by itself authorize migrating it into a different family', async () => { + test('the owner acting alone can migrate a record', async () => { + const record = await stack.create(COMMENT, { text: 'hello' }); + + const migrated = await stack + .asEntity(OWNER) + .commitMigration(record.id, COMMENT_V2, { text: 'hello', title: '' }); + expect(migrated.typeId).toBe(COMMENT_V2); + expect(migrated.content).toEqual({ text: 'hello', title: '' }); + }); + + // Migration is owner-driven: migrateAll() is Stack-only and absent from + // StackClient, and the per-record path carries the same restriction. No + // combination of grants substitutes for it — ordinary write access to a + // record is not consent to move it between families. + test('an update grant on the record’s current family does not authorize migrating it', async () => { await stack.grant(MEMBER, [{ actions: ['update-any'], typeId: COMMENT }]); const record = await stack.create(COMMENT, { text: 'hello' }, { entityId: STRANGER }); @@ -959,35 +973,39 @@ describe('ScopedStack.commitMigration', () => { ).rejects.toThrow(StackPermissionError); }); - test('an update grant on the source family plus a create grant on the destination family together authorize a cross-family migration', async () => { + test('update and create grants together still do not authorize a migration', async () => { await stack.grant(MEMBER, [ { actions: ['update-any'], typeId: COMMENT }, { actions: ['create'], typeId: NOTE }, ]); const record = await stack.create(COMMENT, { text: 'hello' }, { entityId: STRANGER }); - const migrated = await stack - .asEntity(MEMBER) - .commitMigration(record.id, NOTE, { text: 'hello' }); - expect(migrated.typeId).toBe(NOTE); + await expect( + stack.asEntity(MEMBER).commitMigration(record.id, NOTE, { text: 'hello' }), + ).rejects.toThrow(StackPermissionError); }); - test('a single grant naming both actions on one family covers an ordinary in-family migration', async () => { + test('a grant naming both actions on one family does not cover an in-family migration', async () => { await stack.grant(MEMBER, [{ actions: ['update-any', 'create'], typeId: COMMENT }]); const record = await stack.create(COMMENT, { text: 'hello' }, { entityId: STRANGER }); - const migrated = await stack - .asEntity(MEMBER) - .commitMigration(record.id, COMMENT_V2, { text: 'hello', title: '' }); - expect(migrated.typeId).toBe(COMMENT_V2); - expect(migrated.content).toEqual({ text: 'hello', title: '' }); + await expect( + stack.asEntity(MEMBER).commitMigration(record.id, COMMENT_V2, { text: 'hello', title: '' }), + ).rejects.toThrow(StackPermissionError); + }); + + test('record-level write on the record does not authorize migrating it', async () => { + const record = await stack.create(COMMENT, { text: 'hello' }); + await stack.setPermissions(record.id, [ + { access: 'entity', entityId: MEMBER, read: true, write: true }, + ]); + await stack.grant(MEMBER, [{ actions: ['create'], typeId: COMMENT }]); + + await expect( + stack.asEntity(MEMBER).commitMigration(record.id, COMMENT_V2, { text: 'hello', title: '' }), + ).rejects.toThrow(StackPermissionError); }); - // _app, _config and _grant are ungrantable (checkCreateGrant() returns - // false for them regardless of what grants exist on the source family), - // so a write-holder can never migrate an ordinary record into one — - // otherwise migrate would be a second way to forge system-record - // membership without ever holding a create grant on it. test('a write-holder cannot migrate an ordinary record into _app', async () => { await stack.grant(MEMBER, [{ actions: ['update-any'], typeId: COMMENT }]); const record = await stack.create(COMMENT, { text: 'hello' }, { entityId: STRANGER }); @@ -1022,6 +1040,19 @@ describe('ScopedStack.commitMigration', () => { expect(migrated.typeId).toBe('_app@1'); }); + // The owner's authority here is its own, so delegation never carries it: + // an owner principal acting for a subject is not the owner acting alone, + // the same rule deleteAttachment() and setPermissions() apply. + test('an owner principal acting on behalf of a subject cannot migrate', async () => { + const record = await stack.create(COMMENT, { text: 'hello' }); + + await expect( + stack + .asEntity(OWNER, { onBehalfOf: MEMBER }) + .commitMigration(record.id, COMMENT_V2, { text: 'hello', title: '' }), + ).rejects.toThrow(StackPermissionError); + }); + test('a write-holder cannot migrate a _grant record, even to a type they could otherwise create', async () => { const [grantRecord] = await stack.grant(MEMBER, [{ typeId: NOTE, actions: ['read-own'] }]); await stack.setPermissions(grantRecord.id, [ @@ -1063,6 +1094,56 @@ describe('ScopedStack.commitMigration', () => { .commitMigration(record.id, '_entity@1', { did: OWNER, name: 'Impostor' }), ).rejects.toThrow(StackPermissionError); }); + + // Regression: create() refuses a non-owner _attachment@1 record naming a + // fileId they can't already reach, because an _attachment@1 record is + // what canAccessFile()'s uploader clause reads. Migrate must not be a + // second way in. See docs/spec/attachments.md § Creating `_attachment@1` + // records directly. + test('a grantee cannot reach attachment bytes by migrating a record into _attachment', async () => { + const secret = await stack.putAttachment(new Uint8Array([1, 2, 3, 4]), 'text/plain', 's.txt'); + const fileId = secret.content.fileId; + + await stack.grant(MEMBER, [ + { actions: ['create', 'update-own', 'read-own'], typeId: COMMENT }, + { actions: ['create', 'read-own'], typeId: '_attachment@1' }, + ]); + const view = stack.asEntity(MEMBER); + + // The direct route is already refused, and the bytes are out of reach. + await expect( + view.create('_attachment@1', { fileId, mimeType: 'text/plain', size: 4 }), + ).rejects.toThrow(StackPermissionError); + await expect(view.getAttachment(fileId)).rejects.toThrow(StackPermissionError); + + const decoy = await view.create(COMMENT, { text: 'decoy' }); + await expect( + view.commitMigration(decoy.id, '_attachment@1', { + fileId, + mimeType: 'text/plain', + size: 4, + }), + ).rejects.toThrow(StackPermissionError); + await expect(view.getAttachment(fileId)).rejects.toThrow(StackPermissionError); + }); + + // Regression: _entity is grantable and requireOwnerForOwnerDid() guards + // only the owner's own did, so binding immutability is what stops a + // grantee moving a contact card onto another DID. + test('a grantee cannot move an _entity card onto another did by migrating', async () => { + const card = await stack.create('_entity@1', { did: 'did:key:zAlice', name: 'Alice' }); + await stack.grant(MEMBER, [{ actions: ['update-any', 'create'], typeId: '_entity@1' }]); + + await expect( + stack + .asEntity(MEMBER) + .commitMigration(card.id, '_entity@1', { did: 'did:key:zBob', name: 'Alice' }), + ).rejects.toThrow(StackPermissionError); + expect((await adapter.getRecord(card.id))?.content).toEqual({ + did: 'did:key:zAlice', + name: 'Alice', + }); + }); }); // ------------------------------------------------------- diff --git a/packages/core/tests/stack.test.ts b/packages/core/tests/stack.test.ts index 282d490..01be6c5 100644 --- a/packages/core/tests/stack.test.ts +++ b/packages/core/tests/stack.test.ts @@ -1096,6 +1096,148 @@ describe('Stack.commitMigration', () => { }); }); +// ------------------------------------------------------- +// Stack.commitMigration — integrity checks +// +// Migrate writes a full content replacement under a new typeId, so it is +// create-shaped at the destination and update-shaped over the record as it +// stands. These cover the checks it owes on both counts — without them, +// migrate is a second write path to state create()/update() refuse. +// ------------------------------------------------------- + +describe('Stack.commitMigration — binding fields', () => { + test('refuses moving an _entity card onto another did', async () => { + const card = await stack.create('_entity@1', { did: 'did:key:zAlice', name: 'Alice' }); + + await expect( + stack.commitMigration(card.id, '_entity@1', { did: 'did:key:zBob', name: 'Alice' }), + ).rejects.toThrow(StackValidationError); + expect((await adapter.getRecord(card.id))?.content).toEqual({ + did: 'did:key:zAlice', + name: 'Alice', + }); + }); + + test('refuses shedding a did by migrating out of the family', async () => { + const card = await stack.create('_entity@1', { did: 'did:key:zAlice', name: 'Alice' }); + + await expect(stack.commitMigration(card.id, NOTE_V1, { text: 'shed' })).rejects.toThrow( + StackValidationError, + ); + }); + + test('refuses a did another _entity card already claims', async () => { + await stack.create('_entity@1', { did: 'did:key:zAlice', name: 'Alice' }); + const bare = await stack.create('_entity@1', { did: '', name: 'Unbound' }); + + await expect( + stack.commitMigration(bare.id, '_entity@1', { did: 'did:key:zAlice', name: 'Unbound' }), + ).rejects.toThrow(StackConflictError); + }); + + test('allows a migration that carries the same did through', async () => { + await stack.defineType('_entity@2', 'Entity', { + did: { kind: 'string', required: true }, + name: { kind: 'string', required: true }, + pronouns: { kind: 'string' }, + }); + const card = await stack.create('_entity@1', { did: 'did:key:zAlice', name: 'Alice' }); + + const migrated = await stack.commitMigration(card.id, '_entity@2', { + did: 'did:key:zAlice', + name: 'Alice', + pronouns: 'they/them', + }); + expect(migrated.typeId).toBe('_entity@2'); + }); +}); + +describe('Stack.commitMigration — _attachment protections', () => { + test('refuses repointing fileId', async () => { + const a = await stack.putAttachment(new Uint8Array([9]), 'text/plain', 'a.txt'); + + await expect( + stack.commitMigration(a.id, '_attachment@1', { + fileId: 'other-hash', + mimeType: 'text/plain', + size: 1, + }), + ).rejects.toThrow(StackValidationError); + expect((await adapter.getRecord(a.id))?.content).toEqual(a.content); + }); + + test('refuses rewriting mimeType and size', async () => { + const a = await stack.putAttachment(new Uint8Array([9]), 'text/plain', 'a.txt'); + + await expect( + stack.commitMigration(a.id, '_attachment@1', { + fileId: a.content.fileId, + mimeType: 'image/png', + size: 999, + }), + ).rejects.toThrow(StackValidationError); + }); + + test('allows a migration that carries the immutable fields through', async () => { + await stack.defineType('_attachment@2', 'Attachment', { + fileId: { kind: 'string', required: true }, + mimeType: { kind: 'string', required: true }, + size: { kind: 'number', required: true }, + filename: { kind: 'string' }, + caption: { kind: 'string' }, + }); + const a = await stack.putAttachment(new Uint8Array([9]), 'text/plain', 'a.txt'); + + const migrated = await stack.commitMigration(a.id, '_attachment@2', { + fileId: a.content.fileId, + mimeType: 'text/plain', + size: 1, + caption: 'hi', + }); + expect(migrated.typeId).toBe('_attachment@2'); + }); + + test('applies the mimeType-establishment check when arriving from outside the family', async () => { + const a = await stack.putAttachment(new Uint8Array([9]), 'text/plain', 'a.txt'); + const note = await stack.create(NOTE_V1, { text: 'decoy' }); + + await expect( + stack.commitMigration(note.id, '_attachment@1', { + fileId: a.content.fileId, + mimeType: 'image/png', + size: 1, + }), + ).rejects.toThrow(StackValidationError); + }); +}); + +describe('Stack.commitMigration — _group', () => { + test('refuses migrating a record into _group, whose admin roster is stamped at creation', async () => { + const note = await stack.create(NOTE_V1, { text: 'x' }); + + await expect( + stack.commitMigration(note.id, '_group@1', { name: 'Ghost Group' }), + ).rejects.toThrow(StackConflictError); + }); + + test('allows a _group record to migrate between versions, keeping its roster', async () => { + await stack.defineType('_group@2', 'Group', { + name: { kind: 'string', required: true }, + handle: { kind: 'string' }, + stackUrl: { kind: 'string' }, + topic: { kind: 'string' }, + }); + const group = await stack.create('_group@1', { name: 'Real Group' }); + + const migrated = await stack.commitMigration(group.id, '_group@2', { + name: 'Real Group', + topic: 'books', + }); + expect(migrated.typeId).toBe('_group@2'); + expect(migrated.associations).toEqual(group.associations); + }); +}); + // ------------------------------------------------------- // restoreVersion — typeId and validation // ------------------------------------------------------- From 67fe4b2a1b8934f1d5ecffac20437f3de1d8cce6 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 20:40:33 +0000 Subject: [PATCH 3/4] feat(core)!: accept ifVersion on commitMigration(), closing an inconsistency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ExpectedVersionOptions describes itself as "accepted by every mutation that bumps a record's version", and versioning.md says ifVersion "covers every mutation path per the one-rule versioning model". Both were false in exactly one place: commitMigration() bumps version and was the only mutating adapter method whose opts were SnapshotOptions alone. Migrate is also the write that most needs the fence — it replaces content wholesale rather than merge-patching it, so two racing writers lose strictly more than they do on update(). Treating it as the one exception left the API's only full-replacement write with no concurrency control. Threaded the way every sibling verb already threads it: - types.ts: adapter commitMigration() opts -> ExpectedVersionOptions & SnapshotOptions. - stack.ts: Stack/ScopedStack commitMigration() take IfVersionOptions; checkIfVersion() up front, expectedVersion passed to the adapter so the real check stays atomic inside the write. Added to the StackClient interface signature. - sqlite-shared: checkExpectedVersion() before BEGIN, matching patchContent()/restoreVersion() — fts5 removal has to precede the content update, so the precondition can't fold into the UPDATE's WHERE clause. Also gives commitMigration() a proper not-found error instead of failing after the write with "Record not found after commitMigration". - adapter-api: passes ifMatch through the existing request() helper. - record-adapter-sqlite, adapter-local, MemoryAdapter: opts widened to match the adapter contract. migrateAll() sends no ifVersion — a batch pass doesn't know each record's version going in, so bulk migration stays last-writer-wins. Wire compatibility: additive. The commit-migration conformance fixture sends no If-Match and does not pin its absence, so it is unaffected; by wire-format.md's own negotiation rule an optional new request header is a minor change, never a major one. Specs: wire-format.md adds POST .../migrate to the If-Match list and drops the sentence declaring it unconditional; versioning.md adds commitMigration to the enumeration of methods accepting ifVersion. Claude-Session: https://claude.ai/code/session_01JpQomi5W2T9zzS29wAaJFq --- docs/spec/versioning.md | 2 +- docs/spec/wire-format.md | 4 +-- packages/adapter-api/src/index.ts | 11 ++++--- packages/adapter-api/tests/api.test.ts | 16 ++++++++++ packages/adapter-local/src/index.ts | 2 +- packages/core/src/stack.ts | 20 ++++++++---- packages/core/src/testing.ts | 3 +- packages/core/src/types.ts | 8 ++--- packages/core/tests/stack.test.ts | 29 +++++++++++++++++ packages/record-adapter-sqlite/src/index.ts | 2 +- .../tests/record.test.ts | 32 +++++++++++++++++++ packages/sqlite-shared/src/record-logic.ts | 10 +++++- 12 files changed, 117 insertions(+), 22 deletions(-) diff --git a/docs/spec/versioning.md b/docs/spec/versioning.md index 03ad30c..c77359d 100644 --- a/docs/spec/versioning.md +++ b/docs/spec/versioning.md @@ -60,7 +60,7 @@ Restoring an `_app` or `_entity` Record is fenced for the same reason, by the ru ## Optimistic concurrency (`ifVersion`) -`version` is not conflict detection by itself — it exists to power rollback and soft-delete recovery. Nothing reads or writes it unless a caller opts in. Every mutating method (`update`, `delete`, `undelete`, `associate`, `dissociate`, `setPermissions`, `restoreVersion`) accepts an optional `ifVersion`: +`version` is not conflict detection by itself — it exists to power rollback and soft-delete recovery. Nothing reads or writes it unless a caller opts in. Every mutating method (`update`, `delete`, `undelete`, `associate`, `dissociate`, `setPermissions`, `restoreVersion`, `commitMigration`) accepts an optional `ifVersion`: ```ts await stack.update(id, { title: 'New' }, { ifVersion: 5 }); diff --git a/docs/spec/wire-format.md b/docs/spec/wire-format.md index 7af3c6b..95cda0f 100644 --- a/docs/spec/wire-format.md +++ b/docs/spec/wire-format.md @@ -243,7 +243,7 @@ POST /records/:id/migrate — commit a migration (change typeId + content tog `PATCH /records/:id` accepts a partial content object. Omitted fields retain their current values. A field set to `null` is removed (RFC 7396 / JSON Merge Patch). Associations and permissions are managed via their own endpoints. -**Optimistic concurrency:** `PATCH`, `DELETE`, `POST .../undelete`, `POST .../restore/:version`, and the association/permission endpoints below all accept an optional `If-Match` header: +**Optimistic concurrency:** `PATCH`, `DELETE`, `POST .../undelete`, `POST .../restore/:version`, `POST .../migrate`, and the association/permission endpoints below all accept an optional `If-Match` header: ``` PATCH /records/abc123 @@ -256,7 +256,7 @@ When present, the server applies the mutation only if the record's current versi **`entityId` and `principalId` are assigned by the server from the authenticated session, and MUST be ignored if a request body carries them.** They are the two fields that answer "who did this", so a server that echoes back what it was handed makes both self-reported — and `principalId` exists precisely to be the field that isn't (see [Identity § Attribution and what can be trusted](./identity.md#attribution-and-what-can-be-trusted)). A client naming its own `principalId` could dress any write up as a verified app action, defeating the `_app` cross-check that reads it. `ScopedStack` already overrides both regardless of what a caller passes, so a server built on it inherits this; one that maps a request body onto `Stack` directly has to drop them itself. The same applies to `version`, `createdAt`, and `updatedAt`, which the server assigns as it does on any write. `appId` is the deliberate exception — self-reported by design, and never a permission input. For `typeId: "_attachment@1"`, a non-owner requester gets `403` regardless of grants — see [Attachments](./attachments.md#creating-_attachment1-records-directly) for the refusal, its carve-out, and `POST /attachments` as the non-owner-safe combined path. -`POST /records/:id/migrate` is the only way a record's `typeId` changes after creation. Body: `{ "toTypeId": "...", "content": {...} }` — the full post-migration content, computed client-side by the type's owning app (migration functions are app code, not server code) and validated by the server against `toTypeId`'s schema before writing. This is what `stack.update()` uses to commit a pending lazy migration alongside a content patch (a content-only `PATCH` can't carry a `typeId` change), and what `stack.migrateAll()` uses for each record in a batch pass. `Stack.commitMigration()`/`ScopedStack.commitMigration()` is the client-side entry point that backs this endpoint for a single record — see [Type migrations](./data-model.md#type-migrations). A server built on `ScopedStack` serves this endpoint to the **stack owner** and answers `403` otherwise: migration is owner-driven, and no grant confers it (see [Access control](./access-control.md#type-level-grants)). This endpoint is absent from the `If-Match` list above, so a migration commit is unconditional last-writer-wins. +`POST /records/:id/migrate` is the only way a record's `typeId` changes after creation. Body: `{ "toTypeId": "...", "content": {...} }` — the full post-migration content, computed client-side by the type's owning app (migration functions are app code, not server code) and validated by the server against `toTypeId`'s schema before writing. This is what `stack.update()` uses to commit a pending lazy migration alongside a content patch (a content-only `PATCH` can't carry a `typeId` change), and what `stack.migrateAll()` uses for each record in a batch pass. `Stack.commitMigration()`/`ScopedStack.commitMigration()` is the client-side entry point that backs this endpoint for a single record — see [Type migrations](./data-model.md#type-migrations). A server built on `ScopedStack` serves this endpoint to the **stack owner** and answers `403` otherwise: migration is owner-driven, and no grant confers it (see [Access control](./access-control.md#type-level-grants)). Like every other endpoint that bumps a record's version, it accepts `If-Match` — a migration commit replaces content wholesale, so it is precisely the write a caller most needs to be able to fence. `stack.migrateAll()` sends none, since a batch pass doesn't know each record's version going in; a single `commitMigration()` passes whatever `ifVersion` its caller supplied. ### Response envelope diff --git a/packages/adapter-api/src/index.ts b/packages/adapter-api/src/index.ts index dbd24d3..fe4bdec 100644 --- a/packages/adapter-api/src/index.ts +++ b/packages/adapter-api/src/index.ts @@ -665,11 +665,14 @@ export class APIAdapter implements StackAdapter { id: RecordId, toTypeId: TypeId, content: Record, + opts: { expectedVersion?: number } = {}, ): Promise { - const raw = await this.request('POST', `/records/${id}/migrate`, { - toTypeId, - content, - }); + const raw = await this.request( + 'POST', + `/records/${id}/migrate`, + { toTypeId, content }, + { ifMatch: opts.expectedVersion }, + ); return parseRecord(raw); } diff --git a/packages/adapter-api/tests/api.test.ts b/packages/adapter-api/tests/api.test.ts index 6235ff1..f780b99 100644 --- a/packages/adapter-api/tests/api.test.ts +++ b/packages/adapter-api/tests/api.test.ts @@ -774,6 +774,22 @@ describe('commitMigration', () => { const result = await adapter.commitMigration('rec-abc123', 'com.example/note@2', {}); expect(result.typeId).toBe('com.example/note@2'); }); + + test('sends If-Match when expectedVersion is given', async () => { + const adapter = await openAdapter(); + mockFetch.mockResolvedValueOnce(jsonResponse(RECORD_RAW)); + await adapter.commitMigration('rec-abc123', 'com.example/note@2', {}, { expectedVersion: 5 }); + const [, init] = mockFetch.mock.lastCall as [string, RequestInit]; + expect((init.headers as Record)['If-Match']).toBe('"5"'); + }); + + test('omits If-Match when expectedVersion is not given', async () => { + const adapter = await openAdapter(); + mockFetch.mockResolvedValueOnce(jsonResponse(RECORD_RAW)); + await adapter.commitMigration('rec-abc123', 'com.example/note@2', {}); + const [, init] = mockFetch.mock.lastCall as [string, RequestInit]; + expect((init.headers as Record)['If-Match']).toBeUndefined(); + }); }); // ------------------------------------------------------- diff --git a/packages/adapter-local/src/index.ts b/packages/adapter-local/src/index.ts index 44b3f25..5d5f606 100644 --- a/packages/adapter-local/src/index.ts +++ b/packages/adapter-local/src/index.ts @@ -290,7 +290,7 @@ export class LocalAdapter implements StackAdapter { id: RecordId, toTypeId: TypeId, content: Record, - opts?: { snapshot?: RecordVersion }, + opts?: { expectedVersion?: number; snapshot?: RecordVersion }, ): Promise { return this.record.commitMigration(id, toTypeId, content, opts); } diff --git a/packages/core/src/stack.ts b/packages/core/src/stack.ts index 58e48fc..b03f86d 100644 --- a/packages/core/src/stack.ts +++ b/packages/core/src/stack.ts @@ -552,14 +552,15 @@ export interface StackClient { * Commit a per-record migration: change `typeId` and `content` together, * validated against `toTypeId`'s schema. The only way a record's typeId * changes after creation — see docs/spec/wire-format.md § Migration - * commit. No `ifVersion` precondition: `POST /records/:id/migrate` does - * not accept `If-Match` on the wire (see docs/spec/wire-format.md § - * Optimistic concurrency). + * commit. Takes `ifVersion` like every other mutation that bumps a + * record's version (see docs/spec/versioning.md § Optimistic + * concurrency); over the wire that is `If-Match`. */ commitMigration( id: string, toTypeId: TypeId, content: Record, + opts?: IfVersionOptions, ): Promise; getAttachment(fileId: string): Promise; putAttachment( @@ -1420,9 +1421,10 @@ export class Stack implements StackClient { * (computed client-side by the type's owning app, per * docs/spec/wire-format.md § Migration commit) rather than a registered * Migration function. Snapshots the prior state to version history, same - * as update()/restoreVersion(). No `ifVersion` precondition — the wire - * endpoint this backs doesn't accept `If-Match` (see - * docs/spec/wire-format.md § Optimistic concurrency). + * as update()/restoreVersion(), and takes the same optional `ifVersion` + * precondition every version-bumping mutation takes — checked atomically + * at the adapter, not here (see docs/spec/versioning.md § Optimistic + * concurrency). * * Because `content` is a full replacement written under a new `typeId`, * this is create-shaped at the destination *and* update-shaped over the @@ -1435,12 +1437,14 @@ export class Stack implements StackClient { id: string, toTypeId: TypeId, content: Record, + opts: IfVersionOptions = {}, ): Promise { this.assertOpen(); const existing = await this.adapter.getRecord(id); if (!existing) { throw new StackNotFoundError(`Record not found: "${id}"`); } + this.checkIfVersion(existing, opts.ifVersion); const type = await this.getTypeCached(toTypeId); if (!type) { @@ -1493,6 +1497,7 @@ export class Stack implements StackClient { } return this.adapter.commitMigration(id, toTypeId, content, { + expectedVersion: opts.ifVersion, snapshot: this.buildVersionSnapshot(existing), }); } @@ -3043,11 +3048,12 @@ export class ScopedStack implements StackClient { id: string, toTypeId: TypeId, content: Record, + opts: IfVersionOptions = {}, ): Promise { if (!this.ownerActingAlone) { throw new StackPermissionError('Only the stack owner may commit a migration'); } - return this.stack.commitMigration(id, toTypeId, content); + return this.stack.commitMigration(id, toTypeId, content, opts); } /** diff --git a/packages/core/src/testing.ts b/packages/core/src/testing.ts index 9cca21f..cf2210d 100644 --- a/packages/core/src/testing.ts +++ b/packages/core/src/testing.ts @@ -346,10 +346,11 @@ export class MemoryAdapter implements StackAdapter { id: string, toTypeId: TypeId, content: Record, - opts: { snapshot?: RecordVersion } = {}, + opts: { expectedVersion?: number; snapshot?: RecordVersion } = {}, ) { const record = this.records.get(id); if (!record) throw new Error(`Not found: ${id}`); + this.checkExpectedVersion(record, opts.expectedVersion); if (opts.snapshot) this.snapshotBeforeMutation(id, opts.snapshot); const updated = this.bump({ ...record, typeId: toTypeId, content }); this.records.set(id, updated); diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index f9b9012..5f1c118 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -557,15 +557,15 @@ export interface StackRecordAdapter { /** * Commit a migration: write new content under a new typeId in one step. - * This is the only way a record's typeId changes after creation — used - * exclusively by Stack.migrateAll(); Stack.update() never changes typeId - * as a side effect. Bumps version internally. + * This is the only way a record's typeId changes after creation — used by + * Stack.commitMigration() and Stack.migrateAll(); Stack.update() never + * changes typeId as a side effect. Bumps version internally. */ commitMigration( id: RecordId, toTypeId: TypeId, content: Record, - opts?: SnapshotOptions, + opts?: ExpectedVersionOptions & SnapshotOptions, ): Promise; // Types diff --git a/packages/core/tests/stack.test.ts b/packages/core/tests/stack.test.ts index 01be6c5..8c4a77f 100644 --- a/packages/core/tests/stack.test.ts +++ b/packages/core/tests/stack.test.ts @@ -1510,10 +1510,39 @@ describe('ifVersion', () => { expect(restored.content.text).toBe('hello'); }); + test('commitMigration() enforces ifVersion', async () => { + await stack.defineType( + NOTE_V2, + 'Note', + { text: { kind: 'text', required: true }, title: { kind: 'string' } }, + { migratesFrom: NOTE_V1 }, + ); + const record = await stack.create(NOTE_V1, { text: 'hello' }); // v1 + await stack.update(record.id, { text: 'v2' }); // v2 + + await expect( + stack.commitMigration(record.id, NOTE_V2, { text: 'v2', title: '' }, { ifVersion: 1 }), + ).rejects.toThrow(StackVersionConflictError); + // A rejected migration must leave the record at its current type. + expect((await adapter.getRecord(record.id))?.typeId).toBe(NOTE_V1); + + const migrated = await stack.commitMigration( + record.id, + NOTE_V2, + { text: 'v2', title: 'ok' }, + { ifVersion: 2 }, + ); // v3 + expect(migrated.version).toBe(3); + expect(migrated.typeId).toBe(NOTE_V2); + }); + test('ifVersion on a nonexistent record throws StackNotFoundError, not StackVersionConflictError', async () => { await expect(stack.update('nonexistent', { text: 'x' }, { ifVersion: 1 })).rejects.toThrow( StackNotFoundError, ); + await expect( + stack.commitMigration('nonexistent', NOTE_V1, { text: 'x' }, { ifVersion: 1 }), + ).rejects.toThrow(StackNotFoundError); }); }); diff --git a/packages/record-adapter-sqlite/src/index.ts b/packages/record-adapter-sqlite/src/index.ts index 1f9ccad..54b740b 100644 --- a/packages/record-adapter-sqlite/src/index.ts +++ b/packages/record-adapter-sqlite/src/index.ts @@ -206,7 +206,7 @@ export class NativeSQLiteRecordAdapter implements StackRecordAdapter { id: string, toTypeId: TypeId, content: Record, - opts?: { snapshot?: RecordVersion }, + opts?: { expectedVersion?: number; snapshot?: RecordVersion }, ): Promise { return this.record.commitMigration(id, toTypeId, content, opts); } diff --git a/packages/record-adapter-sqlite/tests/record.test.ts b/packages/record-adapter-sqlite/tests/record.test.ts index 41582c3..69f75dd 100644 --- a/packages/record-adapter-sqlite/tests/record.test.ts +++ b/packages/record-adapter-sqlite/tests/record.test.ts @@ -429,6 +429,38 @@ describe('expectedVersion', () => { expect(undeleted.version).toBe(4); }); + test('commitMigration enforces expectedVersion and leaves typeId untouched on mismatch', async () => { + const adapter = await initAdapter(); + const record = await adapter.createRecord(makeRecord({ content: { text: 'original' } })); + await adapter.patchContent(record.id, { text: 'v2' }); // -> v2 + + await expect( + adapter.commitMigration( + record.id, + 'com.example/note@2', + { text: 'migrated' }, + { + expectedVersion: 1, + }, + ), + ).rejects.toBeInstanceOf(StackVersionConflictError); + const untouched = await adapter.getRecord(record.id); + expect(untouched?.typeId).toBe(record.typeId); + expect(untouched?.version).toBe(2); + // The rejected write must not have disturbed the FTS index either. + const stillFindsV2 = await adapter.queryRecords({ filter: { search: 'v2' } }); + expect(stillFindsV2.records.map((r) => r.id)).toEqual([record.id]); + + const migrated = await adapter.commitMigration( + record.id, + 'com.example/note@2', + { text: 'migrated' }, + { expectedVersion: 2 }, + ); // -> v3 + expect(migrated.typeId).toBe('com.example/note@2'); + expect(migrated.version).toBe(3); + }); + test('hard deleteRecord enforces expectedVersion and leaves the record untouched on mismatch', async () => { const adapter = await initAdapter(); const record = await adapter.createRecord(makeRecord()); diff --git a/packages/sqlite-shared/src/record-logic.ts b/packages/sqlite-shared/src/record-logic.ts index 8905cc8..18de159 100644 --- a/packages/sqlite-shared/src/record-logic.ts +++ b/packages/sqlite-shared/src/record-logic.ts @@ -329,8 +329,16 @@ export class SharedSqlRecordLogic { id: string, toTypeId: TypeId, content: Record, - opts: { snapshot?: RecordVersion } = {}, + opts: { expectedVersion?: number; snapshot?: RecordVersion } = {}, ): Promise { + // Checked here rather than folded into the UPDATE's WHERE clause: + // fts5Strategy.remove() has to run before the content changes, so the + // precondition has to settle first — same shape as patchContent() and + // restoreVersion(). + const existing = await this.getRecord(id); + if (!existing) throw new Error(`Record not found: "${id}"`); + this.checkExpectedVersion(existing, opts.expectedVersion); + this.exec.exec('BEGIN'); try { if (opts.snapshot) this.snapshotBeforeMutation(id, opts.snapshot); From a316e65dbb7a8c19b7c0fbfed4b533b300519d13 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 20:43:35 +0000 Subject: [PATCH 4/4] refactor(core): route migrateAll() through commitMigration()'s checked path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit migrateAll() wrote straight to adapter.commitMigration() and ran only validateContent(), so it skipped every integrity check the previous commit added to Stack.commitMigration() — reserved keys, content size, DID binding immutability and uniqueness, the _attachment immutability and mimeType-establishment checks, the _group refusal, and the _config guard. That a Migration function is app code is not a trust boundary here: the app calling commitMigration() is the same app that registered the function, and neither is entitled to move a DID binding or repoint an attachment. registerMigration() also places no constraint on `from` and `to` sharing a baseId, so a registered path can itself cross type families — which is what these checks are about. migrateAll() was therefore an unguarded family-crossing write path, not merely a narrower one. Extracted commitMigrationChecked(existing, toTypeId, content, ifVersion?), taking the record already in hand rather than an id, so the batch pass does not pay a re-fetch per record. commitMigration() is now getRecord + not-found + checkIfVersion + the shared path; migrateAll()'s loop calls the shared path directly, passing no ifVersion. Behavior changes for migrateAll(): - A migration function that would move an _entity/_app DID binding, produce a duplicate binding, repoint an _attachment, or emit a reserved content key now aborts the pass. This is the one change that can break an existing app's migration function, and it is the intended outcome — those are the writes create()/update() already refuse. - Abort-on-first-failure and "anything already committed earlier in the pass stays committed" are unchanged; there are simply more conditions that can abort a pass. - The pre-loop "target type is not defined" check still runs first, so an undefined target still surfaces as StackMigrationError rather than the shared path's generic unknown-type error. Cost is negligible for ordinary app types: uniqueBindingFieldsOf() is empty outside _entity/_app, so most families add only a reserved-key scan and a size check per record. Spec: data-model.md § Type migrations now states that migrateAll() applies the same checks on the same shared path, and why app code is not a trust boundary for them. Claude-Session: https://claude.ai/code/session_01JpQomi5W2T9zzS29wAaJFq --- docs/spec/data-model.md | 4 +- packages/core/src/stack.ts | 41 ++++++++++++++----- packages/core/tests/stack.test.ts | 68 +++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+), 11 deletions(-) diff --git a/docs/spec/data-model.md b/docs/spec/data-model.md index acd226c..f1e0d44 100644 --- a/docs/spec/data-model.md +++ b/docs/spec/data-model.md @@ -181,7 +181,9 @@ The migration registry is **per-stack-instance** — different stacks can be at - **`migrateAll("com.example.myapp/note")`** eagerly commits all pending migrations for a type family in one deliberate pass — call it at app startup after registering migrations, or after a schema change. It sweeps soft-deleted Records unconditionally (`includeDeleted` is not a caller option in either direction — see [Deletion](./versioning.md#deletion)), validates each migrated result against the target Type's schema before writing, and aborts immediately on the first validation failure (a buggy migration function is a bug to surface, not to paper over by skipping the offending records) — anything already committed earlier in the pass stays committed. Previous content is snapshotted to version history before each write. - **`commitMigration(id, toTypeId, content)`** is the single-record counterpart, changing one Record's `typeId` and `content` together in one step. Unlike `migrateAll()`, `content` here is supplied by the caller rather than produced by a registered `Migration` function — the client-side app that owns `toTypeId` computes it, and the library validates it against `toTypeId`'s schema exactly as `create()`/`update()` validate against a schema. This is what backs the wire's `POST /records/:id/migrate` (see [Wire format](./wire-format.md#records)). Under `ScopedStack` it is **owner-acting-alone**, matching `migrateAll()`'s own absence from `StackClient` — no grant or record-level `write` substitutes for it (see [Access control](./access-control.md#type-level-grants)). Previous content and `typeId` are snapshotted to version history first, same as `migrateAll()`. - Because `content` is a full replacement written under a new `typeId`, `commitMigration()` is create-shaped at the destination and update-shaped over the Record as it stands, and owes both sets of integrity checks. DID bindings are held to immutability across the union of the two families' binding fields — a card can neither shed its `did` by migrating out of `_entity`/`_app` nor pick one up on the way in — and to uniqueness in the destination family (see [Identity § DID bindings](./identity.md#did-bindings)). An `_attachment@1` Record's `fileId`, `mimeType` and `size` stay immutable, and a Record arriving from outside that family is held to the same mimeType-establishment check `create()` applies. Migrating _into_ `_group` is refused outright: a group's `admin` roster entry is stamped at creation and a migration cannot stamp one, so it would produce a group nobody but the owner can manage — version-to-version migration within `_group` stays open and carries the existing roster with it. + Because `content` is a full replacement written under a new `typeId`, a migration commit is create-shaped at the destination and update-shaped over the Record as it stands, and owes both sets of integrity checks. DID bindings are held to immutability across the union of the two families' binding fields — a card can neither shed its `did` by migrating out of `_entity`/`_app` nor pick one up on the way in — and to uniqueness in the destination family (see [Identity § DID bindings](./identity.md#did-bindings)). An `_attachment@1` Record's `fileId`, `mimeType` and `size` stay immutable, and a Record arriving from outside that family is held to the same mimeType-establishment check `create()` applies. Migrating _into_ `_group` is refused outright: a group's `admin` roster entry is stamped at creation and a migration cannot stamp one, so it would produce a group nobody but the owner can manage — version-to-version migration within `_group` stays open and carries the existing roster with it. + + **`migrateAll()` applies these same checks**, on the same shared write path. That a `Migration` function is app code rather than a request body is not a trust boundary here: the app calling `commitMigration()` is the same app that registered the function, and neither is entitled to move a DID binding or repoint an attachment. `registerMigration()` also places no constraint on `from` and `to` sharing a `baseId`, so a registered path can itself cross type families — which is precisely what these checks are about. A migration function that would violate one aborts the pass like any other validation failure. **Stale-writer behavior.** A Record whose version this app instance can't reconcile — older than what it's registered _and_ not bridged by a migration path, or newer than anything it has ever `defineType()`'d — is an explicit error (`StackMigrationError`) under `presentAt: 'latest'`, not a silent pass-through. This covers both directions of "the same app at two versions" meeting via a shared stack. Reading the Record as stored (the default, no `presentAt`) always succeeds regardless — the stale-writer signal only fires when the app explicitly asks for the migrated view and the library can't honestly provide one. diff --git a/packages/core/src/stack.ts b/packages/core/src/stack.ts index b03f86d..9dbebe5 100644 --- a/packages/core/src/stack.ts +++ b/packages/core/src/stack.ts @@ -961,15 +961,11 @@ export class Stack implements StackClient { }); for (const record of result.records) { - const migratedContent = migrateFn(record.content); - const errors = validateContent(migratedContent, latestType.schema); - if (errors.length > 0) { - throw new StackValidationError(errors); - } - - await this.adapter.commitMigration(record.id, latestId, migratedContent, { - snapshot: this.buildVersionSnapshot(record), - }); + // Same checked path commitMigration() takes — a migration + // function is no more entitled to move a DID binding or repoint + // an attachment than a request body is. No ifVersion: a batch + // pass doesn't know each record's version going in. + await this.commitMigrationChecked(record, latestId, migrateFn(record.content)); migrated++; } @@ -1445,6 +1441,31 @@ export class Stack implements StackClient { throw new StackNotFoundError(`Record not found: "${id}"`); } this.checkIfVersion(existing, opts.ifVersion); + return this.commitMigrationChecked(existing, toTypeId, content, opts.ifVersion); + } + + /** + * The checked migration write, shared by commitMigration() and + * migrateAll(). Takes the record already in hand rather than an id: a + * batch pass holds each record from its own query page, and re-fetching + * per record would cost a read apiece for nothing. + * + * Both callers owe the same checks. migrateAll()'s content comes from a + * registered Migration function rather than a request body, but "app + * code" is not a trust boundary here — the app calling commitMigration() + * is the same app that registered the function, and neither may move a + * DID binding or repoint an attachment. registerMigration() also places + * no constraint on `from` and `to` sharing a baseId, so a migration path + * can cross type families; family-crossing is exactly what the checks + * below care about. + */ + private async commitMigrationChecked( + existing: StackRecord, + toTypeId: TypeId, + content: Record, + ifVersion?: number, + ): Promise { + const id = existing.id; const type = await this.getTypeCached(toTypeId); if (!type) { @@ -1497,7 +1518,7 @@ export class Stack implements StackClient { } return this.adapter.commitMigration(id, toTypeId, content, { - expectedVersion: opts.ifVersion, + expectedVersion: ifVersion, snapshot: this.buildVersionSnapshot(existing), }); } diff --git a/packages/core/tests/stack.test.ts b/packages/core/tests/stack.test.ts index 8c4a77f..1d61722 100644 --- a/packages/core/tests/stack.test.ts +++ b/packages/core/tests/stack.test.ts @@ -1029,6 +1029,74 @@ describe('migrateAll', () => { expect(result.migrated).toBe(1); expect((await adapter.getRecord(record.id))?.typeId).toBe(NOTE_V2); }); + + // migrateAll() and commitMigration() share one checked write path: a + // migration function is app code, but so is the app calling + // commitMigration(), and neither is entitled to move a DID binding or + // slip a reserved key past validation. + test('aborts when a migration function would move a DID binding', async () => { + await stack.defineType( + '_entity@2', + 'Entity', + { + did: { kind: 'string', required: true }, + name: { kind: 'string', required: true }, + }, + { migratesFrom: '_entity@1' }, + ); + stack.registerMigration({ + from: '_entity@1', + to: '_entity@2', + migrate: (content) => ({ ...content, did: 'did:key:zHijacked' }), + }); + const card = await stack.create('_entity@1', { did: 'did:key:zAlice', name: 'Alice' }); + + await expect(stack.migrateAll('_entity')).rejects.toThrow(StackValidationError); + expect((await adapter.getRecord(card.id))?.content).toEqual({ + did: 'did:key:zAlice', + name: 'Alice', + }); + }); + + test('aborts when a migration function emits a reserved content key', async () => { + await stack.defineType( + NOTE_V3, + 'Note', + { text: { kind: 'text', required: true }, title: { kind: 'string' } }, + { migratesFrom: NOTE_V2 }, + ); + stack.registerMigration({ + from: NOTE_V2, + to: NOTE_V3, + migrate: (content) => ({ ...content, ['__proto__']: 'polluted' }), + }); + await stack.create(NOTE_V2, { text: 'hi', title: '' }); + + await expect(stack.migrateAll('com.example.test/note')).rejects.toThrow(StackValidationError); + }); + + test('still carries an unchanged DID binding through a migration', async () => { + await stack.defineType( + '_entity@2', + 'Entity', + { + did: { kind: 'string', required: true }, + name: { kind: 'string', required: true }, + pronouns: { kind: 'string' }, + }, + { migratesFrom: '_entity@1' }, + ); + stack.registerMigration({ + from: '_entity@1', + to: '_entity@2', + migrate: (content) => ({ ...content, pronouns: 'they/them' }), + }); + const card = await stack.create('_entity@1', { did: 'did:key:zAlice', name: 'Alice' }); + + const result = await stack.migrateAll('_entity'); + expect(result.migrated).toBe(1); + expect((await adapter.getRecord(card.id))?.typeId).toBe('_entity@2'); + }); }); // -------------------------------------------------------