diff --git a/CLAUDE.md b/CLAUDE.md index 826f9a05..69208dac 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -157,7 +157,7 @@ Component build/test/qa commands are listed in the nested CLAUDE.md files. ## Cross-component contracts - **Front door**: Caddy on host `:8080`. `/core/*` is path-stripped and proxied to core (`:8080`); `/auth-realtime/*` is path-stripped and proxied to auth-realtime (`:8081`, so `/auth-realtime/api/...` and `/auth-realtime/hocuspocus` publicly); everything else goes to the web SSR container. The frontend reaches the core API via `NUXT_PUBLIC_CORE_API_BASE_HTTP_URL` (`…:8080/core`) and auth/realtime via `NUXT_PUBLIC_AUTH_REALTIME_API_BASE_*_URL` (`…:8080/auth-realtime`). -- **Trust boundary**: core has four surfaces. `/api/...` requires a session. `/api/x/...` (core) and `/api/internal/...` (auth-realtime) have no auth and must never be exposed by the reverse proxy (the Caddyfile blocks both at the front door) — they exist for service-to-service calls (auth-realtime ↔ core). `/api/apps/...` is public and sessionless: GitHub and Slack deliver webhooks and OAuth callbacks there, and every request proves itself with the provider's signature or an encrypted state parameter. `/api/mcp` is the MCP surface: OAuth 2.1 bearer tokens issued by auth-realtime (which is the authorization server via `@better-auth/mcp`), validated per request against auth-realtime's internal MCP session endpoint; the Caddyfile also routes the front-door `/.well-known/oauth-*` discovery documents and `/api/auth/*` OAuth endpoints to auth-realtime. +- **Trust boundary**: core has four surfaces. `/api/...` requires a session. `/api/x/...` (core) and `/api/internal/...` (auth-realtime) have no auth and must never be exposed by the reverse proxy (the Caddyfile blocks both at the front door) — they exist for service-to-service calls (auth-realtime ↔ core). Three of core's `/api/x` routes are internal *and* session-authed: fork (`POST .../branches`), branch update (`PUT .../branches/{branchId}`) and merge (`PUT .../merge`) act for a person but are reachable only through auth-realtime, which first flushes the branch's pending Hocuspocus store so core reads what the editors show, forwards the caller's session for core to authorize, and resets the branch's connections after a protection change. `/api/apps/...` is public and sessionless: GitHub and Slack deliver webhooks and OAuth callbacks there, and every request proves itself with the provider's signature or an encrypted state parameter. `/api/mcp` is the MCP surface: OAuth 2.1 bearer tokens issued by auth-realtime (which is the authorization server via `@better-auth/mcp`), validated per request against auth-realtime's internal MCP session endpoint; the Caddyfile also routes the front-door `/.well-known/oauth-*` discovery documents and `/api/auth/*` OAuth endpoints to auth-realtime. - **Session validation**: core's auth middleware validates sessions by calling auth-realtime's `/api/auth/get-session`; auth-realtime owns the Better Auth schema. - **Yjs invariant**: the Hocuspocus `documentName` is `"-"`, split on the first `-`. Never seed or merge one Y.Doc from another with `Y.applyUpdate` — use `replaceYdocContent` (`server/auth-realtime/src/ydocument.ts`). Read the document-storage section of [server/CLAUDE.md](server/CLAUDE.md) before touching branch content anywhere. - **System writes**: a protected branch refuses every persist but core's own. Core marks the batches it originates with `edit.Client.Apply`'s `system` argument, which only a caller acting for core itself may set, auth-realtime opens the direct connection under that origin, and `onStoreDocument` marks the persist system only while every change in the window came from core. Protection itself is enforced where a write enters the document — `onAuthenticate` makes a connection to a protected branch read-only — because the persist carries whole-document state and could otherwise smuggle an edit it never authorised. diff --git a/e2e/helpers/review.ts b/e2e/helpers/review.ts index 3f60ea48..56c69acd 100644 --- a/e2e/helpers/review.ts +++ b/e2e/helpers/review.ts @@ -25,9 +25,8 @@ export function branchSwitcher(page: Page): Locator { // makeReviewable turns the open page into a reviewable one through the // navbar options menu, which creates the draft branch and protects main. -// The page's current content must already be persisted if the draft is -// expected to start from it: the draft is seeded from the stored -// content, not the live document. +// makeReviewable enables the review workflow from the navbar menu and +// waits for the branch switcher that appears once the draft exists. export async function makeReviewable(page: Page): Promise { await page .getByRole("button", { name: t("editor.navbar.open-document-options") }) diff --git a/e2e/tests/prod-trust-boundary.test.ts b/e2e/tests/prod-trust-boundary.test.ts index 259eef5a..aa96af47 100644 --- a/e2e/tests/prod-trust-boundary.test.ts +++ b/e2e/tests/prod-trust-boundary.test.ts @@ -68,6 +68,24 @@ const CORE_INTERNAL_ENDPOINTS = [ path: "/core/api/x/documents/doc-e2e/branch/branch-e2e/", }, { name: "outgoing email", method: "POST", path: "/core/api/x/email" }, + // session-authed, but internal all the same: auth-realtime stores the + // editors' pending changes before core reads the branch, and a client + // reaching these directly would skip that. + { + name: "a branch merge", + method: "PUT", + path: "/core/api/x/documents/doc-e2e/merge", + }, + { + name: "a branch fork", + method: "POST", + path: "/core/api/x/documents/doc-e2e/branches", + }, + { + name: "a branch protection change", + method: "PUT", + path: "/core/api/x/documents/doc-e2e/branches/branch-e2e", + }, ] as const const AUTH_REALTIME_INTERNAL_ENDPOINTS = [ diff --git a/e2e/tests/review.test.ts b/e2e/tests/review.test.ts index d681c5c5..7252448a 100644 --- a/e2e/tests/review.test.ts +++ b/e2e/tests/review.test.ts @@ -228,6 +228,59 @@ test.describe("review workflow", () => { .toBe("Base text with the draft merged in") }) + // the store behind the editor is debounced, and enabling the workflow + // forks the page from what is stored: text typed just before has to be + // stored first or the draft starts without it and main's own store of + // it is refused once main is protected. No documentPersisted() here on + // purpose — the toggle lands inside the debounce window. + test("keeps text typed just before enabling review on both branches", async ({ + page, + request, + }) => { + await signUpWithWorkspace(page, request) + await createDocument(page) + const url = page.url() + await contentEditor(page).click() + await page.keyboard.type("Typed right before review") + await makeReviewable(page) + + await visit(page, url) + + await waitForEditor(page) + await expect + .poll(() => editorText(contentEditor(page))) + .toBe("Typed right before review") + await switchToBranch(page, "draft") + await expect + .poll(() => editorText(contentEditor(page))) + .toBe("Typed right before review") + }) + + // the merge reads the draft as stored, so text typed just before it + // has to be stored first. No documentPersisted() before the merge on + // purpose. + test("keeps text typed just before the merge", async ({ page, request }) => { + await signUpWithWorkspace(page, request) + await createDocument(page) + const url = page.url() + await contentEditor(page).click() + await page.keyboard.type("Base text") + await makeReviewable(page) + await switchToBranch(page, "draft") + await expect.poll(() => editorText(contentEditor(page))).toBe("Base text") + await contentEditor(page).click() + await page.keyboard.press("End") + await page.keyboard.type(" merged right away") + await mergeDraftIntoMain(page) + + await visit(page, url) + + await waitForEditor(page) + await expect + .poll(() => editorText(contentEditor(page))) + .toBe("Base text merged right away") + }) + test("keeps the merged content after a reload", async ({ page, request }) => { await signUpWithWorkspace(page, request) await createDocument(page) diff --git a/server/CLAUDE.md b/server/CLAUDE.md index 32c932c0..9216c7b8 100644 --- a/server/CLAUDE.md +++ b/server/CLAUDE.md @@ -89,7 +89,7 @@ Every organization is seeded at initialization with a Prometheus data source nam - `onLoadDocument` / `onStoreDocument` round-trip through core's internal `/api/x/documents/{id}/branch/{branchId}` endpoints. **Do not** use `Y.applyUpdate` to seed a freshly-created doc with content from another doc — the differing `clientID`s CRDT-merge and silently duplicate content. The codebase's pattern is `replaceYdocContent` in `server/auth-realtime/src/ydocument.ts`, which deep-clones `Y.XmlElement`s (preserving non-string attrs that `XmlElement.clone()` drops). Read the comments on `onLoadDocument` in `server/auth-realtime/src/hocuspocus.ts` and on `applyMergeToOpenDocument` in `server/auth-realtime/src/routes.ts` before touching this area. - **The main branch is identified by its `"default"` flag, not by its name.** The tree join and `FetchMainBranchContent` key on the flag; `UpdateDocumentBranch` refuses to rename the default branch so the two never disagree. Anything that creates a document's first branch — `NewDocument`, `Duplicate` — must set `Default: true`, and forks must not. - **Maintainers accumulate; nothing removes them.** The `maintainers` field on a branch update is *who edited in that persist*, not the document's maintainer set — auth-realtime's store hook sends the current session's editors and system writes send none. `UpsertDocumentMaintainers` only ever adds, so a diff against the stored set would drop everyone not editing at that moment. Removing a maintainer needs a path of its own; there is none today. -- Branch merging: `PUT /documents/:documentId/merge` is handled by auth-realtime, which proxies to core, then **directly mutates the in-memory target-branch Y.Doc** via `replaceYdocContent` (not `applyUpdate`) and immediately persists `rawContent` back through `/api/x/.../branch/:branchId` so a server restart can't reset the clientID and duplicate content on reconnect. +- **Fork, branch update and merge go through auth-realtime.** The web client calls `POST /documents/:documentId/branches`, `PUT /documents/:documentId/branches/:branchId` and `PUT /documents/:documentId/merge` on auth-realtime, which flushes the affected branch's pending Hocuspocus store (both sides for a merge; the `-default` alias too for the default branch) and refuses the operation if that store fails, then proxies to core's session-authed group under `/api/x` with the caller's headers. Core reads the branch's stored row, so without the flush a fork copies stale content, a merge reads it, and a protect toggle makes the pending user store be refused. After core confirms a protection change, auth-realtime drops the socket of every client on the branch's document (`resetConnections` in `hocuspocus.ts`): read-only is decided per connection in `onAuthenticate`, so each client reconnects with the permission the branch now has. Hocuspocus's own `closeConnections` is not that: it sends a close *message* on the still-open multiplexed socket, and the provider leaves the document closed until the page reloads. A merge then **directly mutates the in-memory target-branch Y.Doc** via `replaceYdocContent` (not `applyUpdate`) and immediately persists `rawContent` back through `/api/x/.../branch/:branchId` so a server restart can't reset the clientID and duplicate content on reconnect. ## External resource lifecycle (files, hooks) diff --git a/server/auth-realtime/CLAUDE.md b/server/auth-realtime/CLAUDE.md index d20f6646..5bc53412 100644 --- a/server/auth-realtime/CLAUDE.md +++ b/server/auth-realtime/CLAUDE.md @@ -61,7 +61,9 @@ src/ everything the service writes goes through headers.ts normalizing whatever shape a transport hands over auth.ts createAuth(deps) + the callbacks better-auth invokes - hocuspocus.ts createHocuspocus(deps) / createDocumentHooks(deps) + hocuspocus.ts createHocuspocus(deps) / createDocumentHooks(deps); the + flushDocument the branch routes run before core reads a + branch's stored row routes.ts createRoutes(deps) -> a Hono app operations.ts pure: applying the assistant's edit ops to a Y.Doc ydocument.ts pure: replacing a Y.Doc's content without CRDT merging diff --git a/server/auth-realtime/src/core.test.ts b/server/auth-realtime/src/core.test.ts index 6c452f49..fc7f432a 100644 --- a/server/auth-realtime/src/core.test.ts +++ b/server/auth-realtime/src/core.test.ts @@ -230,10 +230,10 @@ describe("createCoreClient", () => { }) describe("mergeBranches", () => { - // the merge is the one call that goes to core's session-authed - // surface rather than /api/x, because core decides from the - // forwarded headers whether the caller may merge - it("puts to core's public merge route with the caller's headers", async ({ + // the route is internal so nothing but this service reaches it, + // yet core still decides from the forwarded headers whether the + // caller may merge + it("puts to core's internal merge route with the caller's headers", async ({ expect, }) => { const http = stubHttp({ @@ -256,7 +256,7 @@ describe("createCoreClient", () => { expect(http.put).toHaveBeenCalledTimes(1) expect(http.put).toHaveBeenCalledWith( - "http://core:8080/api/documents/doc-1/merge", + "http://core:8080/api/x/documents/doc-1/merge", { fromBranchId: "branch-2", toBranchId: "branch-1", @@ -280,4 +280,67 @@ describe("createCoreClient", () => { expect(result.status).toBe(409) }) }) + + describe("createBranch", () => { + it("posts the body untouched to core's internal branches route with the caller's headers", async ({ + expect, + }) => { + const http = stubHttp({ + status: 200, + data: { branchId: "branch-2" }, + }) + const headers = new AxiosHeaders() + headers.set("cookie", "auth.session=abc") + const body = { + branch: "draft", + sourceBranchId: "branch-1", + } + + const result = await createCoreClient( + BASE_URL, + http, + ).createBranch("doc-1", { body }, { headers }) + + expect(http.post).toHaveBeenCalledTimes(1) + expect(http.post).toHaveBeenCalledWith( + "http://core:8080/api/x/documents/doc-1/branches", + body, + { headers }, + ) + expect(result.status).toBe(200) + expect(result.data).toEqual({ branchId: "branch-2" }) + }) + }) + + describe("updateBranch", () => { + it("puts the body untouched to core's internal branch route with the caller's headers", async ({ + expect, + }) => { + const http = stubHttp({ + status: 200, + data: { branchId: "branch-1" }, + }) + const headers = new AxiosHeaders() + headers.set("cookie", "auth.session=abc") + const body = { protected: true } + + const result = await createCoreClient( + BASE_URL, + http, + ).updateBranch( + "doc-1", + "branch-1", + { body }, + { headers }, + ) + + expect(http.put).toHaveBeenCalledTimes(1) + expect(http.put).toHaveBeenCalledWith( + "http://core:8080/api/x/documents/doc-1/branches/branch-1", + body, + { headers }, + ) + expect(result.status).toBe(200) + }) + }) }) diff --git a/server/auth-realtime/src/core.ts b/server/auth-realtime/src/core.ts index 97c50fa9..c6c78ae8 100644 --- a/server/auth-realtime/src/core.ts +++ b/server/auth-realtime/src/core.ts @@ -45,6 +45,12 @@ export interface MergedDocument { icon: string } +// the branch requests this service forwards for the web client. Their +// bodies are core's to validate, so they travel through untyped. +interface BranchRequest { + body: unknown +} + export interface RequestOptions { headers?: AxiosHeaders } @@ -105,6 +111,17 @@ export interface CoreClient { toBranchId: string, options: RequestOptions, ): Promise<{ status: number; data: MergedDocument }> + createBranch( + documentId: string, + request: BranchRequest, + options: RequestOptions, + ): Promise + updateBranch( + documentId: string, + branchId: string, + request: BranchRequest, + options: RequestOptions, + ): Promise } export function createCoreClient( @@ -167,9 +184,9 @@ export function createCoreClient( ) }, - // the merge itself goes through core's session-authed surface, - // not /api/x: the caller's own headers are forwarded so core - // decides whether they may merge. + // the branch operations live on /api/x so that only this service + // can reach them, but core still authorizes them from the + // caller's own headers, which is why those are forwarded. async mergeBranches( documentId, fromBranchId, @@ -177,7 +194,7 @@ export function createCoreClient( options, ) { const response = await http.put( - `${baseUrl}/api/documents/${documentId}/merge`, + `${internal}/documents/${documentId}/merge`, { fromBranchId, toBranchId }, options, ) @@ -187,5 +204,21 @@ export function createCoreClient( data: response.data as MergedDocument, } }, + + async createBranch(documentId, request, options) { + return http.post( + `${internal}/documents/${documentId}/branches`, + request.body, + options, + ) + }, + + async updateBranch(documentId, branchId, request, options) { + return http.put( + `${internal}/documents/${documentId}/branches/${branchId}`, + request.body, + options, + ) + }, } } diff --git a/server/auth-realtime/src/hocuspocus.test.ts b/server/auth-realtime/src/hocuspocus.test.ts index 2e16b3b1..45daa1d2 100644 --- a/server/auth-realtime/src/hocuspocus.test.ts +++ b/server/auth-realtime/src/hocuspocus.test.ts @@ -4,6 +4,7 @@ import { createDocumentHooks, createHocuspocus, parseDocumentName, + resetConnections, resolveBranchId, type AuthSession, } from "./hocuspocus.js" @@ -874,6 +875,246 @@ describe("createDocumentHooks", () => { expect(core.storeBranchContent).toHaveBeenCalledTimes(0) }) }) + + describe("flushDocument", () => { + // a server holding one open document whose store may be pending + // or running. executeNow stands in for hocuspocus running the + // debounced store: it calls the hook the way the server would. + function stubServer( + hooks: ReturnType, + document: ConnectedDocument | null, + state: "pending" | "running" | "idle", + ) { + const documents = new Map() + if (document) { + documents.set("doc1-branch1", document) + } + + const executeNow = vi.fn(() => + document + ? hooks.onStoreDocument({ + documentName: + "doc1-branch1", + document, + }) + : undefined, + ) + + return { + server: { + documents, + debouncer: { + isDebounced: () => + state === "pending", + isCurrentlyExecuting: () => + state === "running", + executeNow, + }, + }, + executeNow, + } + } + + it("runs the pending store now and resolves once core has it", async ({ + expect, + }) => { + const core = stubCore() + const hooks = hooksWith(core) + const { server, executeNow } = stubServer( + hooks, + seededDocument("Runbook"), + "pending", + ) + + await hooks.flushDocument(server, "doc1-branch1") + + expect(executeNow).toHaveBeenCalledWith( + "onStoreDocument-doc1-branch1", + ) + expect(core.storeBranchContent).toHaveBeenCalledTimes(1) + const [, , update] = + core.storeBranchContent.mock.calls[0] ?? [] + expect(update?.name).toBe("Runbook") + }) + + it("leaves a document that is not open alone", async ({ + expect, + }) => { + const core = stubCore() + const hooks = hooksWith(core) + const { server, executeNow } = stubServer( + hooks, + null, + "pending", + ) + + await hooks.flushDocument(server, "doc1-branch1") + + expect(executeNow).toHaveBeenCalledTimes(0) + expect(core.storeBranchContent).toHaveBeenCalledTimes(0) + }) + + it("leaves a document with nothing pending alone", async ({ + expect, + }) => { + const core = stubCore() + const hooks = hooksWith(core) + const { server, executeNow } = stubServer( + hooks, + seededDocument(), + "idle", + ) + + await hooks.flushDocument(server, "doc1-branch1") + + expect(executeNow).toHaveBeenCalledTimes(0) + expect(core.storeBranchContent).toHaveBeenCalledTimes(0) + }) + + // the store hook swallows the failure to warn the editors; the + // flush is the one caller that needs to see it. + it("rejects with the store's failure and still warns the editors", async ({ + expect, + }) => { + const core = stubCore() + core.storeBranchContent.mockRejectedValue( + new Error("core unreachable"), + ) + const hooks = hooksWith(core) + const document = seededDocument() + const { server } = stubServer( + hooks, + document, + "pending", + ) + + await expect( + hooks.flushDocument(server, "doc1-branch1"), + ).rejects.toThrow("core unreachable") + expect( + document.broadcastStateless, + ).toHaveBeenCalledWith( + JSON.stringify({ + type: "error", + code: "hocuspocus.store_failed", + }), + ) + }) + + it("waits for a store already in flight", async ({ + expect, + }) => { + const core = stubCore() + let finish: () => void = () => undefined + core.storeBranchContent.mockReturnValue( + new Promise((resolve) => { + finish = resolve + }), + ) + const hooks = hooksWith(core) + const document = seededDocument() + const { server, executeNow } = stubServer( + hooks, + document, + "running", + ) + const inFlight = hooks.onStoreDocument({ + documentName: "doc1-branch1", + document, + }) + + let flushed = false + const flush = hooks + .flushDocument(server, "doc1-branch1") + .then(() => { + flushed = true + }) + await Promise.resolve() + + expect(flushed).toBe(false) + finish() + await Promise.all([inFlight, flush]) + expect(flushed).toBe(true) + expect(executeNow).toHaveBeenCalledTimes(0) + expect(core.storeBranchContent).toHaveBeenCalledTimes(1) + }) + + // a persist that failed before the flush was asked for is not + // the flush's answer: nothing is pending, so nothing is awaited. + it("does not report a failure that predates it", async ({ + expect, + }) => { + const core = stubCore() + core.storeBranchContent.mockRejectedValueOnce( + new Error("core unreachable"), + ) + const hooks = hooksWith(core) + const document = seededDocument() + await hooks.onStoreDocument({ + documentName: "doc1-branch1", + document, + }) + const { server } = stubServer(hooks, document, "idle") + + await expect( + hooks.flushDocument(server, "doc1-branch1"), + ).resolves.toBeUndefined() + }) + }) +}) + +describe("resetConnections", () => { + function socket() { + return { webSocket: { close: vi.fn() } } + } + + // the provider reopens a document after losing its socket, which is + // what makes it authenticate again and pick up the branch's new + // permission + it("closes every client's socket with the reset code", ({ expect }) => { + const first = socket() + const second = socket() + const server = { + documents: new Map([ + [ + "doc1-branch1", + { + connections: new Map([ + [first, {}], + [second, {}], + ]), + }, + ], + ]), + } + + resetConnections(server, "doc1-branch1") + + expect(first.webSocket.close).toHaveBeenCalledWith( + 4205, + "Reset Connection", + ) + expect(second.webSocket.close).toHaveBeenCalledWith( + 4205, + "Reset Connection", + ) + }) + + it("leaves other documents' sockets alone", ({ expect }) => { + const other = socket() + const server = { + documents: new Map([ + [ + "doc1-branch2", + { connections: new Map([[other, {}]]) }, + ], + ]), + } + + resetConnections(server, "doc1-branch1") + + expect(other.webSocket.close).toHaveBeenCalledTimes(0) + }) }) describe("createHocuspocus", () => { @@ -881,14 +1122,14 @@ describe("createHocuspocus", () => { expect, }) => { const log = stubLog() - const server = createHocuspocus({ + const { instance } = createHocuspocus({ auth: stubAuth(), core: stubCore(), log, }) // the extension keeps its sink in its own configuration; the // server exposes no other way to reach it. - const logger = server.configuration.extensions[0] as { + const logger = instance.configuration.extensions[0] as { configuration: { log: (...args: unknown[]) => void } } @@ -901,20 +1142,46 @@ describe("createHocuspocus", () => { }) it("wires the document hooks onto the server", ({ expect }) => { - const server = createHocuspocus({ + const { instance } = createHocuspocus({ auth: stubAuth(), core: stubCore(), log: stubLog(), }) - expect(typeof server.configuration.onAuthenticate).toBe( + expect(typeof instance.configuration.onAuthenticate).toBe( "function", ) - expect(typeof server.configuration.onLoadDocument).toBe( + expect(typeof instance.configuration.onLoadDocument).toBe( "function", ) - expect(typeof server.configuration.onStoreDocument).toBe( + expect(typeof instance.configuration.onStoreDocument).toBe( "function", ) }) + + // the flush is bound to the server it came with: a document the + // server does not hold cannot have a pending store to run. + it("flushes against its own server", async ({ expect }) => { + const core = stubCore() + const { flushDocument } = createHocuspocus({ + auth: stubAuth(), + core, + log: stubLog(), + }) + + await flushDocument("doc1-branch1") + + expect(core.storeBranchContent).toHaveBeenCalledTimes(0) + }) + it("resets against its own server", ({ expect }) => { + const { resetConnections } = createHocuspocus({ + auth: stubAuth(), + core: stubCore(), + log: stubLog(), + }) + + expect(() => { + resetConnections("doc1-branch1") + }).not.toThrow() + }) }) diff --git a/server/auth-realtime/src/hocuspocus.ts b/server/auth-realtime/src/hocuspocus.ts index 78f33781..142b3834 100644 --- a/server/auth-realtime/src/hocuspocus.ts +++ b/server/auth-realtime/src/hocuspocus.ts @@ -36,6 +36,64 @@ interface ConnectedDocument extends Y.Doc { broadcastStateless(payload: string): void } +// the part of the hocuspocus server a flush reaches into: which documents +// are open, and the debouncer that holds their pending stores. +interface FlushableServer { + documents: { get(documentName: string): unknown } + debouncer: { + isDebounced(id: string): boolean + isCurrentlyExecuting(id: string): boolean + executeNow(id: string): unknown + } +} + +// the part a reset reaches into: the socket behind each of a document's +// client connections. +interface ResettableServer { + documents: { + get(documentName: string): + | { + connections: Map< + { + webSocket: { + close( + code?: number, + reason?: string, + ): void + } + }, + unknown + > + } + | undefined + } +} + +// the close code hocuspocus itself uses when it resets a connection. +const resetConnection = { code: 4205, reason: "Reset Connection" } + +// resetConnections drops the socket of every client on a document, so each +// reconnects and authenticates again. Hocuspocus's own closeConnections +// only tells the provider the document is closed, and the provider leaves +// it closed: it reopens a document after losing its socket, not after that +// message. Direct connections have no socket and are unaffected. +export function resetConnections( + server: ResettableServer, + documentName: string, +): void { + const document = server.documents.get(documentName) + if (!document) { + return + } + + for (const [connection] of document.connections) { + connection.webSocket.close( + resetConnection.code, + resetConnection.reason, + ) + } +} + // parseDocumentName splits a document name of the form // "documentId-branchIdentifier" into its component parts. The split is on // the first dash: xids carry none, but a branch identifier may. @@ -161,6 +219,12 @@ export function createDocumentHooks({ auth, core }: DocumentHookDeps) { // only ever lent to changes seen arriving under it. const documentSystemOnly = new Map() + // the most recent persist of each document, settled or not. A flush + // awaits the one it triggered or found in flight, which is how a + // failure onStoreDocument swallows for the editors' sake still + // reaches the caller that needed the content stored. + const lastPersist = new Map>() + return { async onAuthenticate({ connectionConfig, @@ -346,7 +410,7 @@ export function createDocumentHooks({ auth, core }: DocumentHookDeps) { documentSystemOnly.delete(data.documentName) - try { + const persist = (async () => { const { documentId, branchIdentifier } = parseDocumentName(data.documentName) const branchId = await resolveBranchId( @@ -376,6 +440,12 @@ export function createDocumentHooks({ auth, core }: DocumentHookDeps) { system, }, ) + })() + + lastPersist.set(data.documentName, persist) + + try { + await persist } catch (err) { Sentry.captureException(err) @@ -387,14 +457,48 @@ export function createDocumentHooks({ auth, core }: DocumentHookDeps) { ) } }, + + // flushDocument runs the document's pending store now and resolves + // once core has it, or rejects with what the store failed on. A + // document that is not open, or has nothing pending, is left + // alone: only a persist this call triggered or found in flight is + // awaited, so an old failure cannot block a later caller. + flushDocument: async ( + server: FlushableServer, + documentName: string, + ): Promise => { + if (!server.documents.get(documentName)) { + return + } + + const id = `onStoreDocument-${documentName}` + + if (server.debouncer.isDebounced(id)) { + // runs onStoreDocument under the document's save + // mutex, after any persist already in flight. + await server.debouncer.executeNow(id) + } else if (!server.debouncer.isCurrentlyExecuting(id)) { + return + } + + await lastPersist.get(documentName) + }, } } +interface HocuspocusServer { + instance: Hocuspocus + flushDocument: (documentName: string) => Promise + resetConnections: (documentName: string) => void +} + export function createHocuspocus({ log, - ...hooks -}: DocumentHookDeps & { log: ServiceLogger }): Hocuspocus { - return new Hocuspocus({ + ...deps +}: DocumentHookDeps & { log: ServiceLogger }): HocuspocusServer { + const { flushDocument, ...hooks } = createDocumentHooks(deps) + + const instance = new Hocuspocus({ // a line per connection, load, change and persist: a traffic // trace, so DEBUG. extensions: [ @@ -410,6 +514,15 @@ export function createHocuspocus({ }, }), ], - ...createDocumentHooks(hooks), + ...hooks, }) + + return { + instance, + flushDocument: (documentName) => + flushDocument(instance, documentName), + resetConnections: (documentName) => { + resetConnections(instance, documentName) + }, + } } diff --git a/server/auth-realtime/src/index.ts b/server/auth-realtime/src/index.ts index aa98c0ca..8c13bd2e 100644 --- a/server/auth-realtime/src/index.ts +++ b/server/auth-realtime/src/index.ts @@ -39,7 +39,11 @@ if (env.valkeyDsn) { const auth = createAuth({ env, store, dialect, redis, core, log }) -const hocuspocus = createHocuspocus({ +const { + instance: hocuspocus, + flushDocument, + resetConnections, +} = createHocuspocus({ auth: { getSession: (input) => auth.api.getSession(input) }, core, log, @@ -50,7 +54,18 @@ const app = new Hono() // the adapter's own state. const nodeWs = createNodeWebSocket({ app }) -app.route("/api", createRoutes({ env, auth, store, hocuspocus, core })) +app.route( + "/api", + createRoutes({ + env, + auth, + store, + hocuspocus, + flushDocument, + resetConnections, + core, + }), +) // the RFC 9728 protected-resource metadata for the MCP surface is served // by the mcp plugin's request hook, which matches root-anchored diff --git a/server/auth-realtime/src/routes.test.ts b/server/auth-realtime/src/routes.test.ts index 46fb6e75..73575009 100644 --- a/server/auth-realtime/src/routes.test.ts +++ b/server/auth-realtime/src/routes.test.ts @@ -1,4 +1,4 @@ -import { describe, it, vi } from "vitest" +import { describe, it, vi, type Mock } from "vitest" import * as Y from "yjs" import { SignJWT, exportJWK, generateKeyPair, type JWTPayload } from "jose" import { createRoutes, type DocumentRegistry } from "./routes.js" @@ -64,21 +64,58 @@ function build( auth?: ReturnType store?: StubStore hocuspocus?: DocumentRegistry + flushDocument?: Mock<(documentName: string) => Promise> core?: StubCore } = {}, ) { const auth = overrides.auth ?? stubAuth() const core = overrides.core ?? stubCore() const store = overrides.store ?? stubStore() + const flushDocument: Mock<(documentName: string) => Promise> = + overrides.flushDocument ?? vi.fn().mockResolvedValue(undefined) + const resetConnections = vi.fn<(documentName: string) => void>() const app = createRoutes({ env: overrides.env ?? ENV, auth, store, hocuspocus: overrides.hocuspocus ?? stubRegistry().registry, + flushDocument, + resetConnections, core, }) - return { app, auth, core, store } + return { app, auth, core, store, flushDocument, resetConnections } +} + +// the order in which the flushes and the core call happened, which is +// what the branch routes are about: the flush has to come first. +function callOrder(flushDocument: Mock, coreCall: Mock): string[] { + return [ + ...flushDocument.mock.invocationCallOrder.map( + (order, i) => + `${order}:flush ${String(flushDocument.mock.calls[i]?.[0])}`, + ), + ...coreCall.mock.invocationCallOrder.map( + (order) => `${order}:core`, + ), + ] + .sort( + (a, b) => + Number(a.split(":")[0]) - + Number(b.split(":")[0]), + ) + .map((entry) => entry.slice(entry.indexOf(":") + 1)) +} + +function json(body: unknown, cookie?: string): RequestInit { + return { + method: "PUT", + body: JSON.stringify(body), + headers: { + "Content-Type": "application/json", + ...(cookie ? { Cookie: cookie } : {}), + }, + } } function seededDocument(text: string): Y.Doc { @@ -216,7 +253,352 @@ describe("createRoutes", () => { }) }) + describe("POST /documents/:documentId/branches", () => { + it.for([ + { name: "a body that is not JSON", input: "not json" }, + { + name: "a body missing sourceBranchId", + input: '{"branch":"draft"}', + }, + { + name: "a sourceBranchId that is not a string", + input: '{"branch":"draft","sourceBranchId":1}', + }, + ])("rejects $name", async ({ input }, { expect }) => { + const { app, core, flushDocument } = build() + + const res = await app.request( + "/documents/doc1/branches", + { + ...json(null), + method: "POST", + body: input, + }, + ) + + expect(res.status).toBe(400) + expect(flushDocument).toHaveBeenCalledTimes(0) + expect(core.createBranch).toHaveBeenCalledTimes(0) + }) + + it("flushes the source branch before core copies it", async ({ + expect, + }) => { + const { app, core, flushDocument } = build() + + const res = await app.request( + "/documents/doc1/branches", + { + ...json( + { + branch: "draft", + sourceBranchId: "b1", + }, + "auth.session=abc", + ), + method: "POST", + }, + ) + + expect(res.status).toBe(200) + expect(await res.json()).toEqual({ + branchId: "branch-2", + }) + expect( + callOrder(flushDocument, core.createBranch), + ).toEqual(["flush doc1-b1", "core"]) + const [documentId, request, options] = + core.createBranch.mock.calls[0] ?? [] + expect(documentId).toBe("doc1") + expect(request?.body).toEqual({ + branch: "draft", + sourceBranchId: "b1", + }) + expect(options?.headers?.get("cookie")).toBe( + "auth.session=abc", + ) + }) + + // the default branch can be open under its id and under the + // "default" alias; a store pending under either is the same edits + it("also flushes the default alias when forking the default branch", async ({ + expect, + }) => { + const core = stubCore() + core.fetchBranches.mockResolvedValue([ + { + branchId: "b1", + default: true, + protected: false, + }, + ]) + const { app, flushDocument } = build({ core }) + + await app.request("/documents/doc1/branches", { + ...json({ + branch: "draft", + sourceBranchId: "b1", + }), + method: "POST", + }) + + expect( + flushDocument.mock.calls.map((call) => call[0]), + ).toEqual(["doc1-b1", "doc1-default"]) + }) + + it("refuses the fork when the flush fails", async ({ + expect, + }) => { + const flushDocument = vi + .fn() + .mockRejectedValue( + new Error("core unreachable"), + ) + const { app, core } = build({ flushDocument }) + + const res = await app.request( + "/documents/doc1/branches", + { + ...json({ + branch: "draft", + sourceBranchId: "b1", + }), + method: "POST", + }, + ) + + expect(res.status).toBe(500) + expect(await res.json()).toEqual({ + error: "pending changes could not be stored", + }) + expect(core.createBranch).toHaveBeenCalledTimes(0) + }) + + it("answers with core's refusal", async ({ expect }) => { + const core = stubCore() + core.createBranch.mockRejectedValue({ + response: { + status: 409, + data: { error: "branch exists" }, + }, + }) + const { app } = build({ core }) + + const res = await app.request( + "/documents/doc1/branches", + { + ...json({ + branch: "draft", + sourceBranchId: "b1", + }), + method: "POST", + }, + ) + + expect(res.status).toBe(409) + expect(await res.json()).toEqual({ + error: "branch exists", + }) + }) + }) + + describe("PUT /documents/:documentId/branches/:branchId", () => { + it("rejects a body that is not JSON", async ({ expect }) => { + const { app, core, flushDocument } = build() + + const res = await app.request( + "/documents/doc1/branches/b1", + { + ...json(null), + body: "not json", + }, + ) + + expect(res.status).toBe(400) + expect(flushDocument).toHaveBeenCalledTimes(0) + expect(core.updateBranch).toHaveBeenCalledTimes(0) + }) + + // the store the editors have pending is a user write, and core + // refuses those once the branch is protected: it has to land + // before the flag is set + it("flushes the branch before core changes it", async ({ + expect, + }) => { + const { app, core, flushDocument } = build() + + const res = await app.request( + "/documents/doc1/branches/b1", + json({ protected: true }, "auth.session=abc"), + ) + + expect(res.status).toBe(200) + expect( + callOrder(flushDocument, core.updateBranch), + ).toEqual(["flush doc1-b1", "core"]) + const [documentId, branchId, request, options] = + core.updateBranch.mock.calls[0] ?? [] + expect(documentId).toBe("doc1") + expect(branchId).toBe("b1") + expect(request?.body).toEqual({ protected: true }) + expect(options?.headers?.get("cookie")).toBe( + "auth.session=abc", + ) + }) + + it.for([ + { + name: "protecting", + before: false, + body: { protected: true }, + }, + { + name: "unprotecting", + before: true, + body: { protected: false }, + }, + ])( + "resets the branch's connections when $name it", + async ({ before, body }, { expect }) => { + const core = stubCore() + core.fetchBranches.mockResolvedValue([ + { + branchId: "b1", + default: true, + protected: before, + }, + ]) + const { app, resetConnections } = build({ + core, + }) + + await app.request( + "/documents/doc1/branches/b1", + json(body), + ) + + expect( + resetConnections.mock.calls.map( + (call) => call[0], + ), + ).toEqual(["doc1-b1", "doc1-default"]) + }, + ) + + it.for([ + { + name: "a rename", + body: { name: "release" }, + }, + { + name: "a protection the branch already has", + body: { protected: false }, + }, + ])( + "keeps the connections on $name", + async ({ body }, { expect }) => { + const core = stubCore() + core.fetchBranches.mockResolvedValue([ + { + branchId: "b1", + default: false, + protected: false, + }, + ]) + const { app, resetConnections } = build({ + core, + }) + + const res = await app.request( + "/documents/doc1/branches/b1", + json(body), + ) + + expect(res.status).toBe(200) + expect(resetConnections).toHaveBeenCalledTimes( + 0, + ) + }, + ) + + it("keeps the connections when core refuses the change", async ({ + expect, + }) => { + const core = stubCore() + core.updateBranch.mockResolvedValue({ + status: 403, + data: { error: "not permitted" }, + }) + const { app, resetConnections } = build({ core }) + + const res = await app.request( + "/documents/doc1/branches/b1", + json({ protected: true }), + ) + + expect(res.status).toBe(403) + expect(resetConnections).toHaveBeenCalledTimes(0) + }) + + it("refuses the change when the flush fails", async ({ + expect, + }) => { + const flushDocument = vi + .fn() + .mockRejectedValue( + new Error("core unreachable"), + ) + const { app, core, resetConnections } = build({ + flushDocument, + }) + + const res = await app.request( + "/documents/doc1/branches/b1", + json({ protected: true }), + ) + + expect(res.status).toBe(500) + expect(core.updateBranch).toHaveBeenCalledTimes(0) + expect(resetConnections).toHaveBeenCalledTimes(0) + }) + }) + describe("PUT /documents/:documentId/merge", () => { + it("flushes the source and then the target before core merges", async ({ + expect, + }) => { + const { app, core, flushDocument } = build() + + await app.request( + "/documents/doc1/merge", + json({ fromBranchId: "b2", toBranchId: "b1" }), + ) + + expect( + callOrder(flushDocument, core.mergeBranches), + ).toEqual(["flush doc1-b2", "flush doc1-b1", "core"]) + }) + + it("refuses the merge when a flush fails", async ({ + expect, + }) => { + const flushDocument = vi + .fn() + .mockRejectedValue( + new Error("core unreachable"), + ) + const { app, core } = build({ flushDocument }) + + const res = await app.request( + "/documents/doc1/merge", + json({ fromBranchId: "b2", toBranchId: "b1" }), + ) + + expect(res.status).toBe(500) + expect(core.mergeBranches).toHaveBeenCalledTimes(0) + }) + it.for([ { name: "a body that is not JSON", input: "not json" }, { name: "a body that is not an object", input: "null" }, diff --git a/server/auth-realtime/src/routes.ts b/server/auth-realtime/src/routes.ts index c9c50e60..bcdb2569 100644 --- a/server/auth-realtime/src/routes.ts +++ b/server/auth-realtime/src/routes.ts @@ -1,11 +1,16 @@ import * as Sentry from "@sentry/node" -import { Hono } from "hono" +import { Hono, type Context } from "hono" import type { ContentfulStatusCode } from "hono/utils/http-status" import { cors } from "hono/cors" import { createLocalJWKSet, jwtVerify } from "jose" import * as Y from "yjs" import type { Store } from "./db.js" -import type { CoreClient, MergedDocument } from "./core.js" +import type { + BranchSummary, + CoreClient, + HttpResponse, + MergedDocument, +} from "./core.js" import type { Env } from "./env.js" import { authMethods } from "./auth.js" import { toAxiosHeaders } from "./headers.js" @@ -44,6 +49,11 @@ export interface RouteDeps { auth: AuthHandler store: Store hocuspocus: DocumentRegistry + // runs a document's pending store now; resolves once core has it. + flushDocument: (documentName: string) => Promise + // drops every client's socket to a document so each reconnects and + // authenticates again. + resetConnections: (documentName: string) => void core: CoreClient } @@ -60,6 +70,16 @@ function readString(source: unknown, key: string): string | undefined { return typeof value === "string" ? value : undefined } +function readBoolean(source: unknown, key: string): boolean | undefined { + if (!source || typeof source !== "object") { + return undefined + } + + const value = (source as Record)[key] + + return typeof value === "boolean" ? value : undefined +} + // an axios rejection carrying core's own response, which the merge route // forwards verbatim so the caller sees core's status and error body rather // than a generic 500. @@ -91,10 +111,92 @@ export function createRoutes({ auth, store, hocuspocus, + flushDocument, + resetConnections, core, }: RouteDeps): Hono { const app = new Hono() + // the names a branch's document can be open under: its id, and for + // the default branch the alias a client may have connected with. + function branchDocumentNames( + documentId: string, + branchId: string, + branch: BranchSummary | undefined, + ): string[] { + const names = [`${documentId}-${branchId}`] + + if (branch?.default) { + names.push(`${documentId}-default`) + } + + return names + } + + // flushBranch stores what the editors hold for a branch before core + // reads its row. Returns the branch as core lists it, so a caller can + // tell what a change it is about to make actually changed. + async function flushBranch( + documentId: string, + branchId: string, + ): Promise { + const branches = await core.fetchBranches(documentId) + const branch = branches.find((b) => b.branchId === branchId) + + for (const name of branchDocumentNames( + documentId, + branchId, + branch, + )) { + await flushDocument(name) + } + + return branch + } + + // flushed runs the flushes an operation needs before it may go to + // core. A failure means core would read a row the editors are ahead + // of, so the operation is refused instead. + async function flushed( + c: Context, + run: () => Promise, + ): Promise { + try { + return await run() + } catch (error) { + Sentry.captureException(error) + + return c.json( + { + error: "pending changes could not be stored", + }, + 500, + ) + } + } + + // calledCore answers with whatever core answered, including a refusal + // carried on a rejection, so the caller sees core's verdict rather + // than a generic failure. + async function calledCore( + c: Context, + fallback: string, + call: () => Promise, + ): Promise { + try { + return await call() + } catch (error) { + Sentry.captureException(error) + + const upstream = upstreamResponse(error) + if (upstream) { + return c.json(upstream.data, upstream.status) + } + + return c.json({ error: fallback }, 500) + } + } + app.use( "*", cors({ @@ -133,11 +235,132 @@ export function createRoutes({ return c.json({ availableSlots: env.maxOrganizations - count }) }) - // auth is delegated: the caller's headers (including the session - // cookie) are forwarded to core, whose middleware validates the - // session and authorizes the merge. The in-memory Y.Doc mutation - // below only runs when core responds 200, so an unauthenticated - // caller changes nothing. + // the branch operations below are forwarded to core rather than + // handled here, and auth is delegated with them: the caller's headers + // (including the session cookie) go along, and core's middleware + // validates the session and authorizes the change. What this service + // adds is the flush: core reads the branch's stored row, and the + // editors are up to ten seconds ahead of it until the debounced store + // runs. + + // a fork copies the source branch's row. + app.post("/documents/:documentId/branches", async (c) => { + const documentId = c.req.param("documentId") + + let body: unknown + try { + body = await c.req.json() + } catch { + return c.json({ error: "invalid request body" }, 400) + } + + const sourceBranchId = readString(body, "sourceBranchId") + if (!sourceBranchId) { + return c.json( + { error: "sourceBranchId is required" }, + 400, + ) + } + + const source = await flushed(c, () => + flushBranch(documentId, sourceBranchId), + ) + if (source instanceof Response) { + return source + } + + const response = await calledCore( + c, + "failed to create branch", + () => + core.createBranch( + documentId, + { body }, + { + headers: toAxiosHeaders( + c.req.raw.headers, + ), + }, + ), + ) + if (response instanceof Response) { + return response + } + + return c.json( + response.data, + response.status as ContentfulStatusCode, + ) + }) + + // protecting a branch makes core refuse every store but its own, so + // the editors' pending one has to land first. Read-only is decided + // per connection when it authenticates, so a change either way + // resets the branch's connections: each client reconnects and comes + // back with the permission the branch now has. + app.put("/documents/:documentId/branches/:branchId", async (c) => { + const documentId = c.req.param("documentId") + const branchId = c.req.param("branchId") + + let body: unknown + try { + body = await c.req.json() + } catch { + return c.json({ error: "invalid request body" }, 400) + } + + const before = await flushed(c, () => + flushBranch(documentId, branchId), + ) + if (before instanceof Response) { + return before + } + + const response = await calledCore( + c, + "failed to update branch", + () => + core.updateBranch( + documentId, + branchId, + { body }, + { + headers: toAxiosHeaders( + c.req.raw.headers, + ), + }, + ), + ) + if (response instanceof Response) { + return response + } + + const protectedFlag = readBoolean(body, "protected") + + if ( + response.status === 200 && + protectedFlag !== undefined && + protectedFlag !== before?.protected + ) { + for (const name of branchDocumentNames( + documentId, + branchId, + before, + )) { + resetConnections(name) + } + } + + return c.json( + response.data, + response.status as ContentfulStatusCode, + ) + }) + + // a merge reads the source branch's row and rewrites the target's; + // both are flushed so neither side is behind its editors. The + // in-memory Y.Doc mutation below only runs when core responds 200, + // so an unauthenticated caller changes nothing. app.put("/documents/:documentId/merge", async (c) => { const documentId = c.req.param("documentId") @@ -160,36 +383,42 @@ export function createRoutes({ ) } - try { - const response = await core.mergeBranches( + const sides = await flushed(c, async () => { + await flushBranch(documentId, fromBranchId) + await flushBranch(documentId, toBranchId) + }) + if (sides instanceof Response) { + return sides + } + + const response = await calledCore(c, "failed to merge", () => + core.mergeBranches( documentId, fromBranchId, toBranchId, - { headers: toAxiosHeaders(c.req.raw.headers) }, - ) - - if (response.status === 200) { - await applyMergeToOpenDocument( - documentId, - toBranchId, - response.data, - ) - } - - return c.json( - response.data, - response.status as ContentfulStatusCode, - ) - } catch (error) { - Sentry.captureException(error) - - const upstream = upstreamResponse(error) - if (upstream) { - return c.json(upstream.data, upstream.status) - } - - return c.json({ error: "failed to merge" }, 500) + { + headers: toAxiosHeaders( + c.req.raw.headers, + ), + }, + ), + ) + if (response instanceof Response) { + return response } + + if (response.status === 200) { + await applyMergeToOpenDocument( + documentId, + toBranchId, + response.data as MergedDocument, + ) + } + + return c.json( + response.data, + response.status as ContentfulStatusCode, + ) }) // mirrors the merge into the target branch's live Y.Doc, if one is diff --git a/server/auth-realtime/src/test-helpers.ts b/server/auth-realtime/src/test-helpers.ts index 38d47118..9ff96493 100644 --- a/server/auth-realtime/src/test-helpers.ts +++ b/server/auth-realtime/src/test-helpers.ts @@ -56,6 +56,14 @@ export function stubCore(): StubCore { icon: "lucide:file", }, }), + createBranch: vi.fn().mockResolvedValue({ + status: 200, + data: { branchId: "branch-2" }, + }), + updateBranch: vi.fn().mockResolvedValue({ + status: 200, + data: { branchId: "branch-1" }, + }), } } diff --git a/server/core/internal/server/router.go b/server/core/internal/server/router.go index 36631326..aeb36243 100644 --- a/server/core/internal/server/router.go +++ b/server/core/internal/server/router.go @@ -111,7 +111,8 @@ func (s *Server) httpRouter() chi.Router { // Note that private routes are not protected by any // authentication middlewares (a reverse proxy like Caddy - // should handle that). + // should handle that), except the branch operations + // auth-realtime forwards on a person's behalf. sr.Mount("/x", s.internalRouter()) }) @@ -137,6 +138,19 @@ func (s *Server) internalRouter() chi.Router { ssr.Get("/", s.handlers.document.FetchDocumentBranchByIDUnsafe) ssr.Put("/", s.handlers.document.UpdateDocumentBranchByIDUnsafe) }) + + // the branch operations that read or rewrite a branch's stored + // content. They are internal so that only auth-realtime, which + // first stores what the editors still hold, can start one — yet + // they act for a person, so the session it forwards is required + // and authorizes the change exactly as on the public surface. + sr.Group(func(ssr chi.Router) { + ssr.Use(auth.Middleware(s.log, s.opts.Auth, s.client)) + ssr.Use(s.handlers.document.RequireDocumentAccess) + ssr.Put("/merge", s.handlers.document.MergeBranches) + ssr.Post("/branches", s.handlers.document.CreateDocumentBranch) + ssr.Put("/branches/{branchId}", s.handlers.document.UpdateDocumentBranch) + }) }) r.Post("/email", s.handlers.email.SendEmail) @@ -276,12 +290,9 @@ func (s *Server) router() chi.Router { ssr.Get("/maintainers", s.handlers.document.FetchDocumentMaintainers) ssr.Delete("/", s.handlers.document.DeleteDocument) ssr.Post("/duplicate", s.handlers.document.DuplicateDocument) - ssr.Put("/merge", s.handlers.document.MergeBranches) ssr.Route("/branches", func(sssr chi.Router) { sssr.Get("/", s.handlers.document.FetchDocumentBranches) - sssr.Post("/", s.handlers.document.CreateDocumentBranch) sssr.Route("/{branchId}", func(ssssr chi.Router) { - ssssr.Put("/", s.handlers.document.UpdateDocumentBranch) ssssr.Delete("/", s.handlers.document.DeleteDocumentBranch) ssssr.Put("/review-approve", s.handlers.document.UpdateBranchReviewApproval) ssssr.Post("/blocks/{blockUid}/run", s.handlers.block.RunBlock) diff --git a/server/core/internal/server/router_test.go b/server/core/internal/server/router_test.go index b1287555..90a28160 100644 --- a/server/core/internal/server/router_test.go +++ b/server/core/internal/server/router_test.go @@ -82,4 +82,22 @@ func Test_Server_httpRouter(t *testing.T) { assert.True(t, routes["POST /api/x/organizations/{organizationId}/teardown"]) assert.True(t, routes["PUT /api/x/documents/{documentId}/branch/{branchId}/"]) assert.True(t, routes["POST /api/x/email"]) + + // the branch operations are reachable only through auth-realtime, + // which stores the editors' pending changes before core reads the + // branch: a public mount would let a client skip that. + internalOnly := map[string]string{ + "PUT /api/x/documents/{documentId}/merge": "PUT /api/*/documents/{documentId}/merge", + "POST /api/x/documents/{documentId}/branches": "POST /api/*/documents/{documentId}/branches/", + "PUT /api/x/documents/{documentId}/branches/{branchId}": "PUT /api/*/documents/{documentId}/branches/{branchId}/", + } + + for internal, public := range internalOnly { + assert.True(t, routes[internal], "%s must be mounted internally", internal) + assert.False(t, routes[public], "%s must not be publicly routable", public) + } + + // the branch reads and the delete stay public alongside them. + assert.True(t, routes["GET /api/*/documents/{documentId}/branches/"]) + assert.True(t, routes["DELETE /api/*/documents/{documentId}/branches/{branchId}/"]) } diff --git a/web/app/composables/api/useDocumentAPI.nuxt.test.ts b/web/app/composables/api/useDocumentAPI.nuxt.test.ts index 5c4bd74b..ae9e13a1 100644 --- a/web/app/composables/api/useDocumentAPI.nuxt.test.ts +++ b/web/app/composables/api/useDocumentAPI.nuxt.test.ts @@ -807,7 +807,7 @@ describe("useDocumentAPI", { concurrent: false }, () => { ]) const putCalls = mockEndpoint( "PUT", - `/api/documents/${SHORT_ID}/branches/${BRANCH_ID}`, + `http://test.local/auth-realtime/api/documents/${SHORT_ID}/branches/${BRANCH_ID}`, () => null, ) seedQueryData(TREE_KEY, [makeElem(SHORT_ID, "Doc A")]) @@ -832,7 +832,7 @@ describe("useDocumentAPI", { concurrent: false }, () => { ]) const putCalls = mockEndpoint( "PUT", - `/api/documents/${DOC_ID}/branches/${SHORT_ID}`, + `http://test.local/auth-realtime/api/documents/${DOC_ID}/branches/${SHORT_ID}`, () => null, ) seedQueryData(TREE_KEY, [makeElem(DOC_ID, "Doc A")]) @@ -855,7 +855,7 @@ describe("useDocumentAPI", { concurrent: false }, () => { const treeCalls = mockEndpoint("GET", TREE_URL, () => []) const put = mockDeferredEndpoint( "PUT", - `/api/documents/${DOC_ID}/branches/${BRANCH_ID}`, + `http://test.local/auth-realtime/api/documents/${DOC_ID}/branches/${BRANCH_ID}`, ) seedQueryData(TREE_KEY, [makeElem(DOC_ID, "Doc A")]) const api = makeDocumentAPI() @@ -883,7 +883,7 @@ describe("useDocumentAPI", { concurrent: false }, () => { mockEndpoint("GET", TREE_URL, () => []) mockEndpoint( "PUT", - `/api/documents/${DOC_ID}/branches/${BRANCH_ID}`, + `http://test.local/auth-realtime/api/documents/${DOC_ID}/branches/${BRANCH_ID}`, () => { throw createError({ statusCode: 500 }) }, @@ -908,7 +908,7 @@ describe("useDocumentAPI", { concurrent: false }, () => { mockEndpoint("GET", TREE_URL, () => []) const put = mockDeferredEndpoint( "PUT", - `/api/documents/${DOC_ID}/branches/${BRANCH_ID}`, + `http://test.local/auth-realtime/api/documents/${DOC_ID}/branches/${BRANCH_ID}`, ) seedQueryData(TREE_KEY, [makeElem(DOC_ID, "Doc A")]) const api = makeDocumentAPI() @@ -932,7 +932,7 @@ describe("useDocumentAPI", { concurrent: false }, () => { it("bails out for a non-xid source branch", async ({ expect }) => { const postCalls = mockEndpoint( "POST", - `/api/documents/${DOC_ID}/branches`, + `http://test.local/auth-realtime/api/documents/${DOC_ID}/branches`, () => ({}), ) seedQueryData(BRANCHES_KEY, [makeBranch(BRANCH_ID, "main")]) @@ -954,7 +954,7 @@ describe("useDocumentAPI", { concurrent: false }, () => { }) => { const postCalls = mockEndpoint( "POST", - `/api/documents/${DOC_ID}/branches`, + `http://test.local/auth-realtime/api/documents/${DOC_ID}/branches`, () => ({ id: DOC_ID }), ) seedQueryData(BRANCHES_KEY, [makeBranch(BRANCH_ID, "main")]) @@ -990,9 +990,13 @@ describe("useDocumentAPI", { concurrent: false }, () => { it("falls back to empty metadata when the source branch is not cached", async ({ expect, }) => { - mockEndpoint("POST", `/api/documents/${DOC_ID}/branches`, () => ({ - id: DOC_ID, - })) + mockEndpoint( + "POST", + `http://test.local/auth-realtime/api/documents/${DOC_ID}/branches`, + () => ({ + id: DOC_ID, + }), + ) seedQueryData(BRANCHES_KEY, []) const api = makeDocumentAPI() @@ -1011,9 +1015,13 @@ describe("useDocumentAPI", { concurrent: false }, () => { }) it("rolls back the branches when the request fails", async ({ expect }) => { - mockEndpoint("POST", `/api/documents/${DOC_ID}/branches`, () => { - throw createError({ statusCode: 500 }) - }) + mockEndpoint( + "POST", + `http://test.local/auth-realtime/api/documents/${DOC_ID}/branches`, + () => { + throw createError({ statusCode: 500 }) + }, + ) seedQueryData(BRANCHES_KEY, [makeBranch(BRANCH_ID, "main")]) const api = makeDocumentAPI() @@ -1034,7 +1042,7 @@ describe("useDocumentAPI", { concurrent: false }, () => { }) => { const post = mockDeferredEndpoint( "POST", - `/api/documents/${DOC_ID}/branches`, + `http://test.local/auth-realtime/api/documents/${DOC_ID}/branches`, ) seedQueryData(BRANCHES_KEY, [makeBranch(BRANCH_ID, "main")]) const api = makeDocumentAPI() diff --git a/web/app/composables/api/useDocumentAPI.ts b/web/app/composables/api/useDocumentAPI.ts index a8f2c334..1e46a5c4 100644 --- a/web/app/composables/api/useDocumentAPI.ts +++ b/web/app/composables/api/useDocumentAPI.ts @@ -615,12 +615,17 @@ export default function () { return } - await $coreAPIClient(`/api/documents/${id}/branches/${branchId}`, { - method: "PUT", - body: { - protected: protectedMode, + // auth-realtime stores what the editor still holds before core + // changes the branch, and resets its connections afterwards. + await $authRealtimeAPIClient( + `/api/documents/${id}/branches/${branchId}`, + { + method: "PUT", + body: { + protected: protectedMode, + }, }, - }) + ) }, async onSuccess(_data, { id, branchId }) { if (!isXid(id) || !isXid(branchId)) { @@ -693,7 +698,9 @@ export default function () { return } - return await $coreAPIClient( + // auth-realtime stores what the editor still holds before core + // copies the source branch. + return await $authRealtimeAPIClient( `/api/documents/${docId}/branches`, { method: "POST",