diff --git a/.agents/skills/add-block/SKILL.md b/.agents/skills/add-block/SKILL.md index 695975eba2b..02584416cef 100644 --- a/.agents/skills/add-block/SKILL.md +++ b/.agents/skills/add-block/SKILL.md @@ -919,6 +919,12 @@ Derive templates from the service's real use cases. Each prompt should name a co - **Ground every skill in operations the block actually exposes** — cross-check each skill's steps against `tools.access`. Never describe an action the integration cannot perform. - **Derive skills from real, popular use cases found online — never invent them.** Web-search the service's documented use cases (vendor use-case/solutions pages, official docs describing the workflow, reputable "top automations for X" articles) and only add a skill you can source as something people genuinely do with the service. Do not hallucinate skills. +## Generated tool metadata + +Adding a block on its own needs **no** regeneration — a block references existing tool IDs through `tools.access` and does not change any tool's shape. + +But if the same change also adds, edits **or removes** a tool, run `bun run tool-metadata:generate` and commit the result, or CI fails on stale artifacts. That matters here because a block's `outputs` are authored to match its tools' outputs, and the UI now reads those from the generated metadata rather than the executable registry — an unregenerated tool change makes the block's outputs disagree with what the panel renders. See `.agents/skills/tool-registry-boundary/SKILL.md`. + ## Checklist Before Finishing - [ ] `integrationType` is set to the correct `IntegrationType` enum value @@ -933,6 +939,7 @@ Derive templates from the service's real use cases. Each prompt should name a co - [ ] Tools.config.tool returns correct tool ID (snake_case) - [ ] Outputs match tool outputs - [ ] Block + meta registered in registry-maps.ts (`BLOCK_REGISTRY` / `BLOCK_META_REGISTRY`) +- [ ] If any tool was added, changed or removed alongside the block: ran `bun run tool-metadata:generate` and committed the artifacts - [ ] If icon missing: asked user to provide SVG - [ ] If triggers exist: `triggers` config set, trigger subBlocks spread - [ ] Optional/rarely-used fields set to `mode: 'advanced'` diff --git a/.agents/skills/add-integration/SKILL.md b/.agents/skills/add-integration/SKILL.md index 84689e6be03..da7eccf4cd5 100644 --- a/.agents/skills/add-integration/SKILL.md +++ b/.agents/skills/add-integration/SKILL.md @@ -415,6 +415,16 @@ export const tools: Record = { } ``` +Then regenerate the generated tool metadata and commit it: + +```bash +bun run tool-metadata:generate +``` + +Client code reads `params`/`outputs` from these artifacts rather than importing +the registry, so a tool you add, change or remove is invisible to the UI until they are regenerated, +and CI fails on stale ones. See `.agents/skills/tool-registry-boundary/SKILL.md`. + ### Block Registry (`apps/sim/blocks/registry-maps.ts`) The data maps (`BLOCK_REGISTRY` + `BLOCK_META_REGISTRY`) live in `registry-maps.ts`; `registry.ts` holds only the accessor functions. Add the import and an entry to each map alphabetically: @@ -490,6 +500,7 @@ If creating V2 versions (API-aligned outputs): - [ ] All optional outputs have `optional: true` - [ ] Created `index.ts` barrel export - [ ] Registered all tools in `tools/registry.ts` +- [ ] Ran `bun run tool-metadata:generate` and committed the regenerated artifacts ### Block - [ ] Created `blocks/blocks/{service}.ts` diff --git a/.agents/skills/add-tools/SKILL.md b/.agents/skills/add-tools/SKILL.md index e5fc364b598..1ed42f2e364 100644 --- a/.agents/skills/add-tools/SKILL.md +++ b/.agents/skills/add-tools/SKILL.md @@ -296,6 +296,17 @@ export const tools = { } ``` +3. Regenerate the tool metadata artifacts: + +```bash +bun run tool-metadata:generate +``` + +Client code reads a tool's `params`/`outputs` from generated metadata rather than +importing the registry, so a tool you add, change or remove is invisible to the UI until +these are regenerated — and CI fails on stale artifacts. Commit the result. See +`.agents/skills/tool-registry-boundary/SKILL.md`. + ## Wiring Tools into the Block (Required) After registering in `tools/registry.ts`, you MUST also update the block definition at `apps/sim/blocks/blocks/{service}.ts`. This is not optional — tools are only usable from the UI if they are wired into the block. @@ -443,6 +454,7 @@ All tool IDs MUST use `snake_case`: `{service}_{action}` (e.g., `x_create_tweet` - [ ] Types file has all interfaces - [ ] Index.ts exports all tools and re-exports types (`export * from './types'`) - [ ] Tools registered in `tools/registry.ts` +- [ ] `bun run tool-metadata:generate` run and the regenerated artifacts committed - [ ] Block wired: `tools.access`, dropdown options, subBlocks, `tools.config`, outputs, inputs ## Final Validation (Required) diff --git a/.agents/skills/tool-registry-boundary/SKILL.md b/.agents/skills/tool-registry-boundary/SKILL.md new file mode 100644 index 00000000000..a1463f1c8fa --- /dev/null +++ b/.agents/skills/tool-registry-boundary/SKILL.md @@ -0,0 +1,94 @@ +--- +name: tool-registry-boundary +description: Keep the executable tool registry out of client-reachable module graphs — when to read `@/tools/metadata` instead of `getTool`, how to measure whether an import edge pulls the registry, and how to regenerate the metadata artifacts. Use when touching `apps/sim/tools/registry.ts`, `tools/utils.ts`, `tools/params.ts`, or anything that calls `getTool`. +--- + +# Tool Registry Boundary Skill + +You keep the 4,300-tool executable registry out of module graphs that don't execute tools. + +## The rule + +> Client-reachable code reads tool **metadata**. Only code that actually executes a tool imports the **registry**. + +`@/tools/registry` is a ~9,000-line barrel importing every tool. Each `ToolConfig` mixes plain data (`params`, `outputs`, `name`) with closures — `request.url`, `request.headers`, `transformResponse`, `directExecution`, `postProcess`. Those closures reach the SDK clients, API helpers and parsers each integration needs, and that is what makes the barrel expensive: reaching it costs ~4,700 additional modules. + +`getTool()` returns the whole `ToolConfig`, so a single `getTool` import anywhere in a client-reachable file drags all of it in. + +## Which module to import + +| you need | import | notes | +| --- | --- | --- | +| whether a tool id exists | `hasToolId` from `@/tools/tool-ids` | ~110 KB — the cheapest module | +| to resolve an unversioned name | `resolveToolId` from `@/tools/tool-ids` | | +| every tool id | `getToolIds` from `@/tools/tool-ids` | | +| a tool's params | `getToolParams` / `getToolMetadata` from `@/tools/metadata` | ~4 MB | +| a tool's declared outputs | `getToolOutputsMetadata` from `@/tools/metadata-outputs` | ~4 MB, separate on purpose | +| to **execute** a tool | `getTool` from `@/tools/utils`, or `@/tools/utils.server` | server paths only | + +Three modules, cheapest first. Ids are their own artifact because resolution and existence checks need only the key set; outputs are their own because they are the larger half of the data with a single consumer. `@/tools/metadata` and `@/tools/metadata-outputs` both resolve ids through `@/tools/tool-ids`, which is what keeps them independent of each other — do not "helpfully" re-export one from another, or every caller pays for all three. + +All lookups guard with `Object.hasOwn`. `JSON.parse` yields an object with the normal prototype, so a bare bracket lookup returns inherited members: `getToolMetadata('constructor')` returned a *function* typed as tool metadata before that was fixed. + +## The generated artifacts + +`apps/sim/tools/generated/tool-ids.ts`, `tool-metadata.ts` and `tool-outputs.ts` are produced by `scripts/sync-tool-metadata.ts`: + +```bash +bun run tool-metadata:generate # after adding/changing a tool +bun run tool-metadata:check # what CI runs; fails if stale +``` + +Never hand-edit them. If you add a tool or change a tool's `params`/`outputs`, regenerate and commit the result, or CI fails. + +Three non-obvious properties, each of which was measured and is easy to undo by accident: + +- **The data is a JSON string parsed at runtime, not an imported `.json` and not an object literal.** With `resolveJsonModule` (which this repo enables), a `.json` import makes TypeScript infer a literal type for all 4,300+ entries and takes `tsc --noEmit` from **12.6s to 8m07s** — a 38x regression. An ambient `declare module` does *not* short-circuit it, and an object literal costs the same. A single string literal is one cheap token for both the compiler and the bundler, and `JSON.parse` beats evaluating the equivalent literal at runtime. Do not "clean this up" into a `.json` import. +- **The generator refuses to emit function values.** If you add a field to `METADATA_FIELDS` that contains a closure, generation fails loudly rather than shipping executable config to the client. `hosting` and `schemaEnrichment` are excluded for exactly this reason (`hosting.enabled`, `pricing`, and `enrichSchema` are functions) — they are server-only. +- **Empty param entries are stripped.** The registry contains one (`stt_deepgram_v2`), which crashes callers that read `param.type` while iterating. +- **Lookups resolve versions.** `getTool` maps an unversioned name onto the newest version, and 246 tools are versioned. A plain key lookup would silently report them missing — a quiet correctness bug, not a crash. `resolveToolId` reproduces that against the id set and is differentially tested against the original. + +## Testing code that reads tool metadata + +Mock the module the code under test actually reads. `vi.mock('@/tools/utils', () => toolsUtilsMock)` only controls `getTool`; code that reads `params`/`outputs`/`name` goes through `@/tools/metadata`, so mocking `tools/utils` there is a **no-op that still passes** — because the real generated artifacts happen to agree with the mock fixtures. The test looks green while controlling nothing. + +```ts +import { blocksMock, toolsMetadataMock, toolsUtilsMock } from '@sim/testing/mocks' + +vi.mock('@/tools/utils', () => toolsUtilsMock) // executable lookup +vi.mock('@/tools/metadata', () => toolsMetadataMock) // params / outputs / name +``` + +Both are backed by the same `mockToolConfigs`, so mocking both gives one consistent tool universe. If you are unsure whether a mock is load-bearing, change a fixture value to a sentinel and confirm the test fails. +## The guard + +`bun run check:tool-registry-boundary` (CI: "Tool registry client-boundary audit") walks the module graph from each workspace route and fails if `@/tools/registry` is reachable, printing the exact import chain that reintroduced it. + +If it fails, do not add the entry to an allowlist — there isn't one. Find the symbol the offending file actually needs and move it to a registry-free module, exactly as `mergeToolParameters` and `formatParameterLabel` were. + +Run it with `--verbose` to print per-route module counts, which is also the quickest way to see whether a change moved the graph. + +## How to verify an edge actually got cut + +Do not eyeball imports — the registry is reached through several redundant paths, so cutting one buys nothing while another survives. Walk the graph: + +1. From the entry you care about, follow `import` and `export … from` (skipping `import type`), resolving `@/` against `apps/sim`. +2. Check whether `apps/sim/tools/registry.ts` is in the reachable set, and print the parent chain if it is. +3. Compare the reachable module count before and after. + +Reference points measured on this repo: + +| entry | modules | +| --- | --- | +| `tools/registry.ts` reachable | ~4,900 | +| `tools/merge-params.ts` (leaf) | 2 | +| `providers/utils.ts` after cutting its `params` edge | 22 | +| `app/workspace/[workspaceId]/w/page.tsx` (canvas) | 6,592 before, 1,908 after | + +The canvas route reached the registry through **four** redundant edges — `providers/utils` (via `tools/params`), `lib/workflows/blocks/block-outputs`, `lib/workflows/sanitization/validation`, and `serializer/index`. Cutting any one alone moved the module count by ~1. They all had to go before anything improved; measure the route, not the file you edited. + +## When adding a new caller + +Ask what the caller does with the config. If it reads `params`, `outputs`, `name`, `description` or just checks existence, it belongs on `@/tools/metadata` — no exceptions, even on a path you believe is server-only today, because a future client import will silently re-attach the registry to the graph. + +If it genuinely executes — builds a request, transforms a response, runs `directExecution` — use `getTool`, and keep that file off client-reachable paths. diff --git a/.agents/skills/validate-integration/SKILL.md b/.agents/skills/validate-integration/SKILL.md index a6da2ef56a1..a0b6dc198c6 100644 --- a/.agents/skills/validate-integration/SKILL.md +++ b/.agents/skills/validate-integration/SKILL.md @@ -295,13 +295,31 @@ Group findings by severity: After reporting, fix every **critical** and **warning** issue. Apply **suggestions** where they don't add unnecessary complexity. +### Regenerate Derived Artifacts + +Several files are generated from tool and block definitions. Editing a tool or block WITHOUT regenerating them fails CI, so run these before pushing: + +```bash +bun run tool-metadata:generate # repo root — apps/sim/tools/generated/* +cd apps/sim && bun run generate-docs # docs .mdx + lib/integrations/integrations.json + docs icons +``` + +- **`tool-metadata:generate`** — required whenever a tool's `outputs`, `params`, or descriptions change. CI enforces this with `bun run tool-metadata:check`, which fails with *"Generated tool metadata is stale"*. This is the easiest gate to miss, because nothing in the tool file hints that a generated artifact mirrors it. +- **`generate-docs`** — required whenever block metadata changes (`bgColor`, `name`, `description`, operations, outputs). Regenerates the integration `.mdx`, `integrations.json`, and the docs copy of `components/icons.tsx`. + +**Always diff the regen output before committing.** These generators rewrite every file they own, so they will also sweep in unrelated drift that accumulated on the base branch — pages losing sections, unrelated icons appearing. Keep only the hunks belonging to the integration under validation and `git checkout --` the rest, otherwise an unrelated doc regression rides along in the PR. Verify no page was silently dropped by comparing the directory listing before and after. + +If an icon changed, `apps/sim/components/icons.tsx` is the source of truth and `apps/docs/components/icons.tsx` is its generated mirror — they must end up byte-identical for that component. + ### Validation Output After fixing, confirm: 1. `bun run lint` passes with no fixes needed -2. TypeScript compiles clean (no type errors) -3. Re-read all modified files to verify fixes are correct -4. Any remaining unknown response schemas were explicitly reported to the user instead of guessed +2. TypeScript compiles clean (no type errors) — check the error list is empty for the files you touched; pre-existing unrelated errors in a worktree usually mean workspace packages resolve to the main checkout +3. The integration's tests pass, and any test you added actually fails without its fix (revert it once and watch it go red) +4. Derived artifacts regenerated and their diffs reviewed (see above) +5. Re-read all modified files to verify fixes are correct +6. Any remaining unknown response schemas were explicitly reported to the user instead of guessed ## Checklist Summary @@ -322,5 +340,8 @@ After fixing, confirm: - [ ] Validated `{Service}BlockMeta` exported with at least 7 templates - [ ] Reported all issues grouped by severity - [ ] Fixed all critical and warning issues +- [ ] Ran `bun run tool-metadata:generate` if any tool outputs/params changed, and confirmed `bun run tool-metadata:check` passes +- [ ] Ran `bun run generate-docs` if any block metadata changed, and reverted unrelated drift the generator swept in - [ ] Ran `bun run lint` after fixes - [ ] Verified TypeScript compiles clean +- [ ] Verified added tests fail without their fix diff --git a/.claude/commands/add-block.md b/.claude/commands/add-block.md index 600184d1067..bcc57d1ef48 100644 --- a/.claude/commands/add-block.md +++ b/.claude/commands/add-block.md @@ -918,6 +918,12 @@ Derive templates from the service's real use cases. Each prompt should name a co - **Ground every skill in operations the block actually exposes** — cross-check each skill's steps against `tools.access`. Never describe an action the integration cannot perform. - **Derive skills from real, popular use cases found online — never invent them.** Web-search the service's documented use cases (vendor use-case/solutions pages, official docs describing the workflow, reputable "top automations for X" articles) and only add a skill you can source as something people genuinely do with the service. Do not hallucinate skills. +## Generated tool metadata + +Adding a block on its own needs **no** regeneration — a block references existing tool IDs through `tools.access` and does not change any tool's shape. + +But if the same change also adds, edits **or removes** a tool, run `bun run tool-metadata:generate` and commit the result, or CI fails on stale artifacts. That matters here because a block's `outputs` are authored to match its tools' outputs, and the UI now reads those from the generated metadata rather than the executable registry — an unregenerated tool change makes the block's outputs disagree with what the panel renders. See `.agents/skills/tool-registry-boundary/SKILL.md`. + ## Checklist Before Finishing - [ ] `integrationType` is set to the correct `IntegrationType` enum value @@ -932,6 +938,7 @@ Derive templates from the service's real use cases. Each prompt should name a co - [ ] Tools.config.tool returns correct tool ID (snake_case) - [ ] Outputs match tool outputs - [ ] Block + meta registered in registry-maps.ts (`BLOCK_REGISTRY` / `BLOCK_META_REGISTRY`) +- [ ] If any tool was added, changed or removed alongside the block: ran `bun run tool-metadata:generate` and committed the artifacts - [ ] If icon missing: asked user to provide SVG - [ ] If triggers exist: `triggers` config set, trigger subBlocks spread - [ ] Optional/rarely-used fields set to `mode: 'advanced'` diff --git a/.claude/commands/add-integration.md b/.claude/commands/add-integration.md index 91218c3b139..02aeb75c545 100644 --- a/.claude/commands/add-integration.md +++ b/.claude/commands/add-integration.md @@ -414,6 +414,16 @@ export const tools: Record = { } ``` +Then regenerate the generated tool metadata and commit it: + +```bash +bun run tool-metadata:generate +``` + +Client code reads `params`/`outputs` from these artifacts rather than importing +the registry, so a tool you add, change or remove is invisible to the UI until they are regenerated, +and CI fails on stale ones. See `.agents/skills/tool-registry-boundary/SKILL.md`. + ### Block Registry (`apps/sim/blocks/registry-maps.ts`) The data maps (`BLOCK_REGISTRY` + `BLOCK_META_REGISTRY`) live in `registry-maps.ts`; `registry.ts` holds only the accessor functions. Add the import and an entry to each map alphabetically: @@ -489,6 +499,7 @@ If creating V2 versions (API-aligned outputs): - [ ] All optional outputs have `optional: true` - [ ] Created `index.ts` barrel export - [ ] Registered all tools in `tools/registry.ts` +- [ ] Ran `bun run tool-metadata:generate` and committed the regenerated artifacts ### Block - [ ] Created `blocks/blocks/{service}.ts` diff --git a/.claude/commands/add-tools.md b/.claude/commands/add-tools.md index a09f2da5803..60ab5448d96 100644 --- a/.claude/commands/add-tools.md +++ b/.claude/commands/add-tools.md @@ -295,6 +295,17 @@ export const tools = { } ``` +3. Regenerate the tool metadata artifacts: + +```bash +bun run tool-metadata:generate +``` + +Client code reads a tool's `params`/`outputs` from generated metadata rather than +importing the registry, so a tool you add, change or remove is invisible to the UI until +these are regenerated — and CI fails on stale artifacts. Commit the result. See +`.agents/skills/tool-registry-boundary/SKILL.md`. + ## Wiring Tools into the Block (Required) After registering in `tools/registry.ts`, you MUST also update the block definition at `apps/sim/blocks/blocks/{service}.ts`. This is not optional — tools are only usable from the UI if they are wired into the block. @@ -442,6 +453,7 @@ All tool IDs MUST use `snake_case`: `{service}_{action}` (e.g., `x_create_tweet` - [ ] Types file has all interfaces - [ ] Index.ts exports all tools and re-exports types (`export * from './types'`) - [ ] Tools registered in `tools/registry.ts` +- [ ] `bun run tool-metadata:generate` run and the regenerated artifacts committed - [ ] Block wired: `tools.access`, dropdown options, subBlocks, `tools.config`, outputs, inputs ## Final Validation (Required) diff --git a/.claude/commands/tool-registry-boundary.md b/.claude/commands/tool-registry-boundary.md new file mode 100644 index 00000000000..676fa256cbc --- /dev/null +++ b/.claude/commands/tool-registry-boundary.md @@ -0,0 +1,93 @@ +--- +description: Keep the executable tool registry out of client-reachable module graphs — when to read `@/tools/metadata` instead of `getTool`, how to measure whether an import edge pulls the registry, and how to regenerate the metadata artifacts. Use when touching `apps/sim/tools/registry.ts`, `tools/utils.ts`, `tools/params.ts`, or anything that calls `getTool`. +--- + +# Tool Registry Boundary Skill + +You keep the 4,300-tool executable registry out of module graphs that don't execute tools. + +## The rule + +> Client-reachable code reads tool **metadata**. Only code that actually executes a tool imports the **registry**. + +`@/tools/registry` is a ~9,000-line barrel importing every tool. Each `ToolConfig` mixes plain data (`params`, `outputs`, `name`) with closures — `request.url`, `request.headers`, `transformResponse`, `directExecution`, `postProcess`. Those closures reach the SDK clients, API helpers and parsers each integration needs, and that is what makes the barrel expensive: reaching it costs ~4,700 additional modules. + +`getTool()` returns the whole `ToolConfig`, so a single `getTool` import anywhere in a client-reachable file drags all of it in. + +## Which module to import + +| you need | import | notes | +| --- | --- | --- | +| whether a tool id exists | `hasToolId` from `@/tools/tool-ids` | ~110 KB — the cheapest module | +| to resolve an unversioned name | `resolveToolId` from `@/tools/tool-ids` | | +| every tool id | `getToolIds` from `@/tools/tool-ids` | | +| a tool's params | `getToolParams` / `getToolMetadata` from `@/tools/metadata` | ~4 MB | +| a tool's declared outputs | `getToolOutputsMetadata` from `@/tools/metadata-outputs` | ~4 MB, separate on purpose | +| to **execute** a tool | `getTool` from `@/tools/utils`, or `@/tools/utils.server` | server paths only | + +Three modules, cheapest first. Ids are their own artifact because resolution and existence checks need only the key set; outputs are their own because they are the larger half of the data with a single consumer. `@/tools/metadata` and `@/tools/metadata-outputs` both resolve ids through `@/tools/tool-ids`, which is what keeps them independent of each other — do not "helpfully" re-export one from another, or every caller pays for all three. + +All lookups guard with `Object.hasOwn`. `JSON.parse` yields an object with the normal prototype, so a bare bracket lookup returns inherited members: `getToolMetadata('constructor')` returned a *function* typed as tool metadata before that was fixed. + +## The generated artifacts + +`apps/sim/tools/generated/tool-ids.ts`, `tool-metadata.ts` and `tool-outputs.ts` are produced by `scripts/sync-tool-metadata.ts`: + +```bash +bun run tool-metadata:generate # after adding/changing a tool +bun run tool-metadata:check # what CI runs; fails if stale +``` + +Never hand-edit them. If you add a tool or change a tool's `params`/`outputs`, regenerate and commit the result, or CI fails. + +Three non-obvious properties, each of which was measured and is easy to undo by accident: + +- **The data is a JSON string parsed at runtime, not an imported `.json` and not an object literal.** With `resolveJsonModule` (which this repo enables), a `.json` import makes TypeScript infer a literal type for all 4,300+ entries and takes `tsc --noEmit` from **12.6s to 8m07s** — a 38x regression. An ambient `declare module` does *not* short-circuit it, and an object literal costs the same. A single string literal is one cheap token for both the compiler and the bundler, and `JSON.parse` beats evaluating the equivalent literal at runtime. Do not "clean this up" into a `.json` import. +- **The generator refuses to emit function values.** If you add a field to `METADATA_FIELDS` that contains a closure, generation fails loudly rather than shipping executable config to the client. `hosting` and `schemaEnrichment` are excluded for exactly this reason (`hosting.enabled`, `pricing`, and `enrichSchema` are functions) — they are server-only. +- **Empty param entries are stripped.** The registry contains one (`stt_deepgram_v2`), which crashes callers that read `param.type` while iterating. +- **Lookups resolve versions.** `getTool` maps an unversioned name onto the newest version, and 246 tools are versioned. A plain key lookup would silently report them missing — a quiet correctness bug, not a crash. `resolveToolId` reproduces that against the id set and is differentially tested against the original. + +## Testing code that reads tool metadata + +Mock the module the code under test actually reads. `vi.mock('@/tools/utils', () => toolsUtilsMock)` only controls `getTool`; code that reads `params`/`outputs`/`name` goes through `@/tools/metadata`, so mocking `tools/utils` there is a **no-op that still passes** — because the real generated artifacts happen to agree with the mock fixtures. The test looks green while controlling nothing. + +```ts +import { blocksMock, toolsMetadataMock, toolsUtilsMock } from '@sim/testing/mocks' + +vi.mock('@/tools/utils', () => toolsUtilsMock) // executable lookup +vi.mock('@/tools/metadata', () => toolsMetadataMock) // params / outputs / name +``` + +Both are backed by the same `mockToolConfigs`, so mocking both gives one consistent tool universe. If you are unsure whether a mock is load-bearing, change a fixture value to a sentinel and confirm the test fails. +## The guard + +`bun run check:tool-registry-boundary` (CI: "Tool registry client-boundary audit") walks the module graph from each workspace route and fails if `@/tools/registry` is reachable, printing the exact import chain that reintroduced it. + +If it fails, do not add the entry to an allowlist — there isn't one. Find the symbol the offending file actually needs and move it to a registry-free module, exactly as `mergeToolParameters` and `formatParameterLabel` were. + +Run it with `--verbose` to print per-route module counts, which is also the quickest way to see whether a change moved the graph. + +## How to verify an edge actually got cut + +Do not eyeball imports — the registry is reached through several redundant paths, so cutting one buys nothing while another survives. Walk the graph: + +1. From the entry you care about, follow `import` and `export … from` (skipping `import type`), resolving `@/` against `apps/sim`. +2. Check whether `apps/sim/tools/registry.ts` is in the reachable set, and print the parent chain if it is. +3. Compare the reachable module count before and after. + +Reference points measured on this repo: + +| entry | modules | +| --- | --- | +| `tools/registry.ts` reachable | ~4,900 | +| `tools/merge-params.ts` (leaf) | 2 | +| `providers/utils.ts` after cutting its `params` edge | 22 | +| `app/workspace/[workspaceId]/w/page.tsx` (canvas) | 6,592 before, 1,908 after | + +The canvas route reached the registry through **four** redundant edges — `providers/utils` (via `tools/params`), `lib/workflows/blocks/block-outputs`, `lib/workflows/sanitization/validation`, and `serializer/index`. Cutting any one alone moved the module count by ~1. They all had to go before anything improved; measure the route, not the file you edited. + +## When adding a new caller + +Ask what the caller does with the config. If it reads `params`, `outputs`, `name`, `description` or just checks existence, it belongs on `@/tools/metadata` — no exceptions, even on a path you believe is server-only today, because a future client import will silently re-attach the registry to the graph. + +If it genuinely executes — builds a request, transforms a response, runs `directExecution` — use `getTool`, and keep that file off client-reachable paths. diff --git a/.claude/commands/validate-integration.md b/.claude/commands/validate-integration.md index 2b3358851e1..b243e5c1963 100644 --- a/.claude/commands/validate-integration.md +++ b/.claude/commands/validate-integration.md @@ -294,13 +294,31 @@ Group findings by severity: After reporting, fix every **critical** and **warning** issue. Apply **suggestions** where they don't add unnecessary complexity. +### Regenerate Derived Artifacts + +Several files are generated from tool and block definitions. Editing a tool or block WITHOUT regenerating them fails CI, so run these before pushing: + +```bash +bun run tool-metadata:generate # repo root — apps/sim/tools/generated/* +cd apps/sim && bun run generate-docs # docs .mdx + lib/integrations/integrations.json + docs icons +``` + +- **`tool-metadata:generate`** — required whenever a tool's `outputs`, `params`, or descriptions change. CI enforces this with `bun run tool-metadata:check`, which fails with *"Generated tool metadata is stale"*. This is the easiest gate to miss, because nothing in the tool file hints that a generated artifact mirrors it. +- **`generate-docs`** — required whenever block metadata changes (`bgColor`, `name`, `description`, operations, outputs). Regenerates the integration `.mdx`, `integrations.json`, and the docs copy of `components/icons.tsx`. + +**Always diff the regen output before committing.** These generators rewrite every file they own, so they will also sweep in unrelated drift that accumulated on the base branch — pages losing sections, unrelated icons appearing. Keep only the hunks belonging to the integration under validation and `git checkout --` the rest, otherwise an unrelated doc regression rides along in the PR. Verify no page was silently dropped by comparing the directory listing before and after. + +If an icon changed, `apps/sim/components/icons.tsx` is the source of truth and `apps/docs/components/icons.tsx` is its generated mirror — they must end up byte-identical for that component. + ### Validation Output After fixing, confirm: 1. `bun run lint` passes with no fixes needed -2. TypeScript compiles clean (no type errors) -3. Re-read all modified files to verify fixes are correct -4. Any remaining unknown response schemas were explicitly reported to the user instead of guessed +2. TypeScript compiles clean (no type errors) — check the error list is empty for the files you touched; pre-existing unrelated errors in a worktree usually mean workspace packages resolve to the main checkout +3. The integration's tests pass, and any test you added actually fails without its fix (revert it once and watch it go red) +4. Derived artifacts regenerated and their diffs reviewed (see above) +5. Re-read all modified files to verify fixes are correct +6. Any remaining unknown response schemas were explicitly reported to the user instead of guessed ## Checklist Summary @@ -321,5 +339,8 @@ After fixing, confirm: - [ ] Validated `{Service}BlockMeta` exported with at least 7 templates - [ ] Reported all issues grouped by severity - [ ] Fixed all critical and warning issues +- [ ] Ran `bun run tool-metadata:generate` if any tool outputs/params changed, and confirmed `bun run tool-metadata:check` passes +- [ ] Ran `bun run generate-docs` if any block metadata changed, and reverted unrelated drift the generator swept in - [ ] Ran `bun run lint` after fixes - [ ] Verified TypeScript compiles clean +- [ ] Verified added tests fail without their fix diff --git a/.claude/rules/sim-settings-pages.md b/.claude/rules/sim-settings-pages.md index 154b45a2599..8710553e800 100644 --- a/.claude/rules/sim-settings-pages.md +++ b/.claude/rules/sim-settings-pages.md @@ -167,11 +167,32 @@ Any settings surface with editable state uses **one** shared stack — never hand-roll a Save button, a Discard button, a `beforeunload`, or an "Unsaved changes" modal: -- **`saveDiscardActions(config)`** (`…/components/save-discard-actions/save-discard-actions`) - — returns the canonical dirty-gated **Discard + Save** `SettingsAction[]` (empty - when not dirty). Spread it into a `SettingsPanel` `actions` array, beside any +- **`saveDiscardActions(config)`** (`@/components/settings/save-discard-actions`) + — returns the canonical **Discard + Save** `SettingsAction[]`. **Save is always + rendered** (primary), disabled until there is something to save, so every + editable surface announces its primary action in the same place and a create + form is never a page with no visible way to commit it; **Discard appears only + when dirty**. Spread it into a `SettingsPanel` `actions` array, beside any sibling actions (a detail view's Delete / Remove override). Config: `dirty`, - `saving`, `onSave`, `onDiscard`, `saveDisabled?`, `saveLabel?`, `savingLabel?`. + `saving`, `onSave`, `onDiscard`, `saveDisabled?`, `saveTooltip?`, `creating?`, + `saveLabel?`, `savingLabel?`. Create flows pass `creating` — the + Create / Creating... labels come as a pair and can never drift apart. + `saveLabel`/`savingLabel` are only for genuinely bespoke wording (SSO's + `Update`); never hand-roll the pair to get a create label. +- **``** (same module) — the identical rule + rendered as chips, for surfaces whose header takes a `ReactNode` instead of + action data (`CredentialDetailLayout`: skills, secrets, connected credentials). + Both stacks derive from the one function; never hand-roll a Save chip. + +`CredentialDetailLayout` stays slot-driven for exactly two reasons: its back +control is a real `` (deep-linkable / middle-clickable, which +`SettingsBackAction`'s `onSelect` cannot express), and actions like +`SkillImportButton` own a hidden file input and their own pending state. +**Everything else in one of those headers should be `SettingsAction` data** +rendered through `` from +`@/components/settings/settings-header` — that is the shared chip path, and it +is what keeps tone/icon/variant/tooltip handling from drifting between the two +shells. Reach for it before hand-rolling a `Chip`. - **`useSettingsUnsavedGuard({ isDirty })`** (`…/settings/hooks/use-settings-unsaved-guard`) — syncs the page's local `isDirty` into the shared `useSettingsDirtyStore` (so the sidebar's **section-switch** confirm + the centralized `beforeunload` both diff --git a/.cursor/commands/add-block.md b/.cursor/commands/add-block.md index fbfe5b4c957..e4776c63147 100644 --- a/.cursor/commands/add-block.md +++ b/.cursor/commands/add-block.md @@ -913,6 +913,12 @@ Derive templates from the service's real use cases. Each prompt should name a co - **Ground every skill in operations the block actually exposes** — cross-check each skill's steps against `tools.access`. Never describe an action the integration cannot perform. - **Derive skills from real, popular use cases found online — never invent them.** Web-search the service's documented use cases (vendor use-case/solutions pages, official docs describing the workflow, reputable "top automations for X" articles) and only add a skill you can source as something people genuinely do with the service. Do not hallucinate skills. +## Generated tool metadata + +Adding a block on its own needs **no** regeneration — a block references existing tool IDs through `tools.access` and does not change any tool's shape. + +But if the same change also adds, edits **or removes** a tool, run `bun run tool-metadata:generate` and commit the result, or CI fails on stale artifacts. That matters here because a block's `outputs` are authored to match its tools' outputs, and the UI now reads those from the generated metadata rather than the executable registry — an unregenerated tool change makes the block's outputs disagree with what the panel renders. See `.agents/skills/tool-registry-boundary/SKILL.md`. + ## Checklist Before Finishing - [ ] `integrationType` is set to the correct `IntegrationType` enum value @@ -927,6 +933,7 @@ Derive templates from the service's real use cases. Each prompt should name a co - [ ] Tools.config.tool returns correct tool ID (snake_case) - [ ] Outputs match tool outputs - [ ] Block + meta registered in registry-maps.ts (`BLOCK_REGISTRY` / `BLOCK_META_REGISTRY`) +- [ ] If any tool was added, changed or removed alongside the block: ran `bun run tool-metadata:generate` and committed the artifacts - [ ] If icon missing: asked user to provide SVG - [ ] If triggers exist: `triggers` config set, trigger subBlocks spread - [ ] Optional/rarely-used fields set to `mode: 'advanced'` diff --git a/.cursor/commands/add-integration.md b/.cursor/commands/add-integration.md index 6267a08b17d..d81aa6e2fd0 100644 --- a/.cursor/commands/add-integration.md +++ b/.cursor/commands/add-integration.md @@ -409,6 +409,16 @@ export const tools: Record = { } ``` +Then regenerate the generated tool metadata and commit it: + +```bash +bun run tool-metadata:generate +``` + +Client code reads `params`/`outputs` from these artifacts rather than importing +the registry, so a tool you add, change or remove is invisible to the UI until they are regenerated, +and CI fails on stale ones. See `.agents/skills/tool-registry-boundary/SKILL.md`. + ### Block Registry (`apps/sim/blocks/registry-maps.ts`) The data maps (`BLOCK_REGISTRY` + `BLOCK_META_REGISTRY`) live in `registry-maps.ts`; `registry.ts` holds only the accessor functions. Add the import and an entry to each map alphabetically: @@ -484,6 +494,7 @@ If creating V2 versions (API-aligned outputs): - [ ] All optional outputs have `optional: true` - [ ] Created `index.ts` barrel export - [ ] Registered all tools in `tools/registry.ts` +- [ ] Ran `bun run tool-metadata:generate` and committed the regenerated artifacts ### Block - [ ] Created `blocks/blocks/{service}.ts` diff --git a/.cursor/commands/add-tools.md b/.cursor/commands/add-tools.md index f5bfeaeee94..8bb6f8398b5 100644 --- a/.cursor/commands/add-tools.md +++ b/.cursor/commands/add-tools.md @@ -290,6 +290,17 @@ export const tools = { } ``` +3. Regenerate the tool metadata artifacts: + +```bash +bun run tool-metadata:generate +``` + +Client code reads a tool's `params`/`outputs` from generated metadata rather than +importing the registry, so a tool you add, change or remove is invisible to the UI until +these are regenerated — and CI fails on stale artifacts. Commit the result. See +`.agents/skills/tool-registry-boundary/SKILL.md`. + ## Wiring Tools into the Block (Required) After registering in `tools/registry.ts`, you MUST also update the block definition at `apps/sim/blocks/blocks/{service}.ts`. This is not optional — tools are only usable from the UI if they are wired into the block. @@ -437,6 +448,7 @@ All tool IDs MUST use `snake_case`: `{service}_{action}` (e.g., `x_create_tweet` - [ ] Types file has all interfaces - [ ] Index.ts exports all tools and re-exports types (`export * from './types'`) - [ ] Tools registered in `tools/registry.ts` +- [ ] `bun run tool-metadata:generate` run and the regenerated artifacts committed - [ ] Block wired: `tools.access`, dropdown options, subBlocks, `tools.config`, outputs, inputs ## Final Validation (Required) diff --git a/.cursor/commands/tool-registry-boundary.md b/.cursor/commands/tool-registry-boundary.md new file mode 100644 index 00000000000..d560d4f40e5 --- /dev/null +++ b/.cursor/commands/tool-registry-boundary.md @@ -0,0 +1,89 @@ +# Tool Registry Boundary Skill + +You keep the 4,300-tool executable registry out of module graphs that don't execute tools. + +## The rule + +> Client-reachable code reads tool **metadata**. Only code that actually executes a tool imports the **registry**. + +`@/tools/registry` is a ~9,000-line barrel importing every tool. Each `ToolConfig` mixes plain data (`params`, `outputs`, `name`) with closures — `request.url`, `request.headers`, `transformResponse`, `directExecution`, `postProcess`. Those closures reach the SDK clients, API helpers and parsers each integration needs, and that is what makes the barrel expensive: reaching it costs ~4,700 additional modules. + +`getTool()` returns the whole `ToolConfig`, so a single `getTool` import anywhere in a client-reachable file drags all of it in. + +## Which module to import + +| you need | import | notes | +| --- | --- | --- | +| whether a tool id exists | `hasToolId` from `@/tools/tool-ids` | ~110 KB — the cheapest module | +| to resolve an unversioned name | `resolveToolId` from `@/tools/tool-ids` | | +| every tool id | `getToolIds` from `@/tools/tool-ids` | | +| a tool's params | `getToolParams` / `getToolMetadata` from `@/tools/metadata` | ~4 MB | +| a tool's declared outputs | `getToolOutputsMetadata` from `@/tools/metadata-outputs` | ~4 MB, separate on purpose | +| to **execute** a tool | `getTool` from `@/tools/utils`, or `@/tools/utils.server` | server paths only | + +Three modules, cheapest first. Ids are their own artifact because resolution and existence checks need only the key set; outputs are their own because they are the larger half of the data with a single consumer. `@/tools/metadata` and `@/tools/metadata-outputs` both resolve ids through `@/tools/tool-ids`, which is what keeps them independent of each other — do not "helpfully" re-export one from another, or every caller pays for all three. + +All lookups guard with `Object.hasOwn`. `JSON.parse` yields an object with the normal prototype, so a bare bracket lookup returns inherited members: `getToolMetadata('constructor')` returned a *function* typed as tool metadata before that was fixed. + +## The generated artifacts + +`apps/sim/tools/generated/tool-ids.ts`, `tool-metadata.ts` and `tool-outputs.ts` are produced by `scripts/sync-tool-metadata.ts`: + +```bash +bun run tool-metadata:generate # after adding/changing a tool +bun run tool-metadata:check # what CI runs; fails if stale +``` + +Never hand-edit them. If you add a tool or change a tool's `params`/`outputs`, regenerate and commit the result, or CI fails. + +Three non-obvious properties, each of which was measured and is easy to undo by accident: + +- **The data is a JSON string parsed at runtime, not an imported `.json` and not an object literal.** With `resolveJsonModule` (which this repo enables), a `.json` import makes TypeScript infer a literal type for all 4,300+ entries and takes `tsc --noEmit` from **12.6s to 8m07s** — a 38x regression. An ambient `declare module` does *not* short-circuit it, and an object literal costs the same. A single string literal is one cheap token for both the compiler and the bundler, and `JSON.parse` beats evaluating the equivalent literal at runtime. Do not "clean this up" into a `.json` import. +- **The generator refuses to emit function values.** If you add a field to `METADATA_FIELDS` that contains a closure, generation fails loudly rather than shipping executable config to the client. `hosting` and `schemaEnrichment` are excluded for exactly this reason (`hosting.enabled`, `pricing`, and `enrichSchema` are functions) — they are server-only. +- **Empty param entries are stripped.** The registry contains one (`stt_deepgram_v2`), which crashes callers that read `param.type` while iterating. +- **Lookups resolve versions.** `getTool` maps an unversioned name onto the newest version, and 246 tools are versioned. A plain key lookup would silently report them missing — a quiet correctness bug, not a crash. `resolveToolId` reproduces that against the id set and is differentially tested against the original. + +## Testing code that reads tool metadata + +Mock the module the code under test actually reads. `vi.mock('@/tools/utils', () => toolsUtilsMock)` only controls `getTool`; code that reads `params`/`outputs`/`name` goes through `@/tools/metadata`, so mocking `tools/utils` there is a **no-op that still passes** — because the real generated artifacts happen to agree with the mock fixtures. The test looks green while controlling nothing. + +```ts +import { blocksMock, toolsMetadataMock, toolsUtilsMock } from '@sim/testing/mocks' + +vi.mock('@/tools/utils', () => toolsUtilsMock) // executable lookup +vi.mock('@/tools/metadata', () => toolsMetadataMock) // params / outputs / name +``` + +Both are backed by the same `mockToolConfigs`, so mocking both gives one consistent tool universe. If you are unsure whether a mock is load-bearing, change a fixture value to a sentinel and confirm the test fails. +## The guard + +`bun run check:tool-registry-boundary` (CI: "Tool registry client-boundary audit") walks the module graph from each workspace route and fails if `@/tools/registry` is reachable, printing the exact import chain that reintroduced it. + +If it fails, do not add the entry to an allowlist — there isn't one. Find the symbol the offending file actually needs and move it to a registry-free module, exactly as `mergeToolParameters` and `formatParameterLabel` were. + +Run it with `--verbose` to print per-route module counts, which is also the quickest way to see whether a change moved the graph. + +## How to verify an edge actually got cut + +Do not eyeball imports — the registry is reached through several redundant paths, so cutting one buys nothing while another survives. Walk the graph: + +1. From the entry you care about, follow `import` and `export … from` (skipping `import type`), resolving `@/` against `apps/sim`. +2. Check whether `apps/sim/tools/registry.ts` is in the reachable set, and print the parent chain if it is. +3. Compare the reachable module count before and after. + +Reference points measured on this repo: + +| entry | modules | +| --- | --- | +| `tools/registry.ts` reachable | ~4,900 | +| `tools/merge-params.ts` (leaf) | 2 | +| `providers/utils.ts` after cutting its `params` edge | 22 | +| `app/workspace/[workspaceId]/w/page.tsx` (canvas) | 6,592 before, 1,908 after | + +The canvas route reached the registry through **four** redundant edges — `providers/utils` (via `tools/params`), `lib/workflows/blocks/block-outputs`, `lib/workflows/sanitization/validation`, and `serializer/index`. Cutting any one alone moved the module count by ~1. They all had to go before anything improved; measure the route, not the file you edited. + +## When adding a new caller + +Ask what the caller does with the config. If it reads `params`, `outputs`, `name`, `description` or just checks existence, it belongs on `@/tools/metadata` — no exceptions, even on a path you believe is server-only today, because a future client import will silently re-attach the registry to the graph. + +If it genuinely executes — builds a request, transforms a response, runs `directExecution` — use `getTool`, and keep that file off client-reachable paths. diff --git a/.cursor/commands/validate-integration.md b/.cursor/commands/validate-integration.md index 37543db5dee..45af009bc1d 100644 --- a/.cursor/commands/validate-integration.md +++ b/.cursor/commands/validate-integration.md @@ -289,13 +289,31 @@ Group findings by severity: After reporting, fix every **critical** and **warning** issue. Apply **suggestions** where they don't add unnecessary complexity. +### Regenerate Derived Artifacts + +Several files are generated from tool and block definitions. Editing a tool or block WITHOUT regenerating them fails CI, so run these before pushing: + +```bash +bun run tool-metadata:generate # repo root — apps/sim/tools/generated/* +cd apps/sim && bun run generate-docs # docs .mdx + lib/integrations/integrations.json + docs icons +``` + +- **`tool-metadata:generate`** — required whenever a tool's `outputs`, `params`, or descriptions change. CI enforces this with `bun run tool-metadata:check`, which fails with *"Generated tool metadata is stale"*. This is the easiest gate to miss, because nothing in the tool file hints that a generated artifact mirrors it. +- **`generate-docs`** — required whenever block metadata changes (`bgColor`, `name`, `description`, operations, outputs). Regenerates the integration `.mdx`, `integrations.json`, and the docs copy of `components/icons.tsx`. + +**Always diff the regen output before committing.** These generators rewrite every file they own, so they will also sweep in unrelated drift that accumulated on the base branch — pages losing sections, unrelated icons appearing. Keep only the hunks belonging to the integration under validation and `git checkout --` the rest, otherwise an unrelated doc regression rides along in the PR. Verify no page was silently dropped by comparing the directory listing before and after. + +If an icon changed, `apps/sim/components/icons.tsx` is the source of truth and `apps/docs/components/icons.tsx` is its generated mirror — they must end up byte-identical for that component. + ### Validation Output After fixing, confirm: 1. `bun run lint` passes with no fixes needed -2. TypeScript compiles clean (no type errors) -3. Re-read all modified files to verify fixes are correct -4. Any remaining unknown response schemas were explicitly reported to the user instead of guessed +2. TypeScript compiles clean (no type errors) — check the error list is empty for the files you touched; pre-existing unrelated errors in a worktree usually mean workspace packages resolve to the main checkout +3. The integration's tests pass, and any test you added actually fails without its fix (revert it once and watch it go red) +4. Derived artifacts regenerated and their diffs reviewed (see above) +5. Re-read all modified files to verify fixes are correct +6. Any remaining unknown response schemas were explicitly reported to the user instead of guessed ## Checklist Summary @@ -316,5 +334,8 @@ After fixing, confirm: - [ ] Validated `{Service}BlockMeta` exported with at least 7 templates - [ ] Reported all issues grouped by severity - [ ] Fixed all critical and warning issues +- [ ] Ran `bun run tool-metadata:generate` if any tool outputs/params changed, and confirmed `bun run tool-metadata:check` passes +- [ ] Ran `bun run generate-docs` if any block metadata changed, and reverted unrelated drift the generator swept in - [ ] Ran `bun run lint` after fixes - [ ] Verified TypeScript compiles clean +- [ ] Verified added tests fail without their fix diff --git a/.github/workflows/test-build.yml b/.github/workflows/test-build.yml index 0b3704a9736..c072d19f149 100644 --- a/.github/workflows/test-build.yml +++ b/.github/workflows/test-build.yml @@ -153,6 +153,12 @@ jobs: - name: Verify realtime prune graph run: bun run check:realtime-prune + - name: Tool registry client-boundary audit + run: bun run check:tool-registry-boundary + + - name: Verify generated tool metadata is in sync + run: bun run tool-metadata:check + - name: Verify skill projections are in sync run: bun run skills:check diff --git a/apps/docs/components/icons.tsx b/apps/docs/components/icons.tsx index efedba2000d..606e5b0faf6 100644 --- a/apps/docs/components/icons.tsx +++ b/apps/docs/components/icons.tsx @@ -2221,21 +2221,108 @@ export function EyeIcon(props: SVGProps) { ) } +/** + * Corporate Atlassian mark, used for family-wide Atlassian credentials — one + * API token authenticates Jira, Jira Service Management, and Confluence, so no + * single product mark represents it. Individual products keep their own icons. + */ +export function AtlassianIcon(props: SVGProps) { + const id = useId() + const gradientId = `atlassian_gradient_${id}` + + return ( + + ) +} + export function ConfluenceIcon(props: SVGProps) { + const id = useId() + const topGradientId = `confluence_top_${id}` + const bottomGradientId = `confluence_bottom_${id}` + return ( ) @@ -2768,19 +2855,58 @@ export function LinkupIcon(props: SVGProps) { } export function JiraIcon(props: SVGProps) { + const id = useId() + const middleGradientId = `jira_middle_${id}` + const bottomGradientId = `jira_bottom_${id}` + return ( ) @@ -7558,6 +7684,31 @@ export function SixtyfourIcon(props: SVGProps) { ) } +/** + * The "sim" brand wordmark (v1.0 brand guide simLogotype paths — the same mark + * the navbar/login header renders), inked with the theme-adaptive + * `--text-body`. Used as the icon for the Auto model option; the wide viewBox + * letterboxes itself inside square icon slots. + */ +export function SimAutoIcon(props: SVGProps) { + return ( + + ) +} + export function SimTriggerIcon(props: SVGProps) { return ( ) { return ( - + + + ) +} + +export function ZohoDeskIcon(props: SVGProps) { + return ( + ) } diff --git a/apps/docs/components/ui/icon-mapping.ts b/apps/docs/components/ui/icon-mapping.ts index 8070694868b..283ff057a5f 100644 --- a/apps/docs/components/ui/icon-mapping.ts +++ b/apps/docs/components/ui/icon-mapping.ts @@ -248,6 +248,7 @@ import { ZendeskIcon, ZepIcon, ZeroBounceIcon, + ZohoDeskIcon, ZoomIcon, ZoomInfoIcon, } from '@/components/icons' @@ -535,6 +536,7 @@ export const blockTypeToIconMap: Record = { zendesk: ZendeskIcon, zep: ZepIcon, zerobounce: ZeroBounceIcon, + zoho_desk: ZohoDeskIcon, zoom: ZoomIcon, zoominfo: ZoomInfoIcon, } diff --git a/apps/docs/content/docs/en/agents/custom-tools.mdx b/apps/docs/content/docs/en/agents/custom-tools.mdx index c551d942652..9648b00910e 100644 --- a/apps/docs/content/docs/en/agents/custom-tools.mdx +++ b/apps/docs/content/docs/en/agents/custom-tools.mdx @@ -156,7 +156,7 @@ From **Settings → Custom Tools** you can: \ No newline at end of file +]} /> diff --git a/apps/docs/content/docs/en/integrations/logfire.mdx b/apps/docs/content/docs/en/integrations/logfire.mdx index 1ab38b081aa..9304de6be54 100644 --- a/apps/docs/content/docs/en/integrations/logfire.mdx +++ b/apps/docs/content/docs/en/integrations/logfire.mdx @@ -7,7 +7,7 @@ import { BlockInfoCard } from "@/components/ui/block-info-card" {/* MANUAL-CONTENT-START:intro */} @@ -95,7 +95,7 @@ Search Logfire spans and logs using structured filters for message text, service | ↳ `level` | string | Severity name, such as info, warn, or error | | ↳ `message` | string | Human-readable message | | ↳ `spanName` | string | Template label for similar records | -| ↳ `kind` | string | Record kind: span, log, or span_event | +| ↳ `kind` | string | Record kind: span, log, span_event, or pending_span | | ↳ `serviceName` | string | Service that emitted the record | | ↳ `deploymentEnvironment` | string | Deployment environment of the record | | ↳ `traceId` | string | Trace this record belongs to | @@ -134,7 +134,7 @@ Fetch every span and log belonging to a Logfire trace, ordered from earliest to | ↳ `level` | string | Severity name, such as info, warn, or error | | ↳ `message` | string | Human-readable message | | ↳ `spanName` | string | Template label for similar records | -| ↳ `kind` | string | Record kind: span, log, or span_event | +| ↳ `kind` | string | Record kind: span, log, span_event, or pending_span | | ↳ `serviceName` | string | Service that emitted the record | | ↳ `deploymentEnvironment` | string | Deployment environment of the record | | ↳ `traceId` | string | Trace this record belongs to | @@ -164,5 +164,7 @@ Resolve which Logfire organization and project a read token belongs to. Useful f | --------- | ---- | ----------- | | `organizationName` | string | Logfire organization the read token belongs to | | `projectName` | string | Logfire project the read token belongs to | +| `expiresAt` | string | When the read token expires. Null when it never expires. | +| `spendingCapReachedAt` | string | When the organization's spending cap was reached, which stops queries. Null when it has not been reached. | diff --git a/apps/docs/content/docs/en/integrations/meta.json b/apps/docs/content/docs/en/integrations/meta.json index 56235d7d9c2..ef27475a3a9 100644 --- a/apps/docs/content/docs/en/integrations/meta.json +++ b/apps/docs/content/docs/en/integrations/meta.json @@ -261,6 +261,8 @@ "zendesk", "zep", "zerobounce", + "zoho-desk-service-account", + "zoho_desk", "zoom", "zoom-service-account", "zoominfo" diff --git a/apps/docs/content/docs/en/integrations/zoho-desk-service-account.mdx b/apps/docs/content/docs/en/integrations/zoho-desk-service-account.mdx new file mode 100644 index 00000000000..d1cee116931 --- /dev/null +++ b/apps/docs/content/docs/en/integrations/zoho-desk-service-account.mdx @@ -0,0 +1,162 @@ +--- +title: Zoho Desk Self Clients +description: Set up a Zoho Self Client so your workflows can call Zoho Desk without a personal OAuth login +--- + +import { Callout } from 'fumadocs-ui/components/callout' +import { Step, Steps } from 'fumadocs-ui/components/steps' +import { FAQ } from '@/components/ui/faq' + +A Zoho **Self Client** is an OAuth client that has no redirect URL and no end user. Instead of sending someone through a consent screen, your workflows authenticate to Zoho Desk with the client's own ID and secret, and Sim mints a short-lived access token on demand — no user consent to expire, and no personal login that breaks when someone leaves the team. + +This is the recommended way to use Zoho Desk in production workflows: the credential belongs to your organization rather than a person, the granted scopes are explicit, and tokens are minted fresh whenever a workflow runs. + +## Prerequisites + +You need a Zoho account with access to the [Zoho API Console](https://api-console.zoho.com) for the same organization your Zoho Desk portal belongs to, and the Zoho Desk **organization ID** for that portal. + + +A Self Client can authenticate against the **US, EU, IN, or AU** accounts server — pick your region with the **Data center** field when you add the credential, or leave it blank for US. Organizations in the JP, CA, SA, CN, or UK data centers are not supported yet. API calls are then routed to the Desk host for that same region, so data residency is honored end to end. + +The interactive **OAuth** connection is a separate path and remains **US-only** (`accounts.zoho.com`), so a non-US organization can connect only through a Self Client. + + + +Zoho Desk **webhooks are a Professional-edition and above feature**. The Zoho Desk trigger in Sim provisions a webhook subscription, so it does not work on Free or Standard plans. The trigger also requires a personal **OAuth** connection rather than a Self Client — see [Triggers](#triggers-still-need-oauth) below. + + +## Setting Up the Self Client + +### 1. Create the Self Client + + + + Sign in at [api-console.zoho.com](https://api-console.zoho.com) with the Zoho account that owns the Desk portal + + + Click **Add Client**, choose **Self Client**, and click **Create** — then **OK** on the confirmation + + {/* TODO(screenshot): Zoho API Console Add Client dialog with Self Client selected */} + + + Open the new client and switch to the **Client Secret** tab. Copy the **Client ID** and **Client Secret** — these are two of the three values you'll paste into Sim + + {/* TODO(screenshot): Self Client Client Secret tab showing Client ID and Client Secret */} + + + + +You do **not** need the **Generate Code** tab. That tab produces a one-time authorization code for the code-exchange flow; the client-credentials flow Sim uses needs only the client ID, the client secret, and your organization ID. + + +### 2. Find Your Organization ID + + + + Open Zoho Desk and go to **Setup** (the gear icon) → **Developer Space** → **API** + + + Copy the numeric **Organization ID** (also called the Org ID or portal ID) shown there — for example `600123456` + + {/* TODO(screenshot): Zoho Desk Setup > Developer Space > API showing the Organization ID */} + + + + +This must be the organization ID of the Desk portal you want the workflows to act on. If your Zoho account has more than one Desk portal, using the wrong ID makes Zoho either reject the token request or issue a token scoped to the wrong portal. + + +### 3. Know Your Data Center + +Zoho hosts each organization in one data center, and the accounts server that issues tokens is per region. Look at the URL you use to sign in to Zoho Desk and pick the matching code: + +| Data center | Sign-in domain | Code to enter | +| --- | --- | --- | +| United States | `zoho.com` | `us` (or leave blank) | +| Europe | `zoho.eu` | `eu` | +| India | `zoho.in` | `in` | +| Australia | `zoho.com.au` | `au` | + +Organizations in the JP, CA, SA, CN, and UK data centers cannot be connected yet. + +### 4. Scopes + +Sim requests exactly the scopes its Zoho Desk tools and trigger exercise: + +``` +Desk.tickets.READ +Desk.tickets.UPDATE +Desk.contacts.READ +Desk.agents.READ +Desk.basic.READ +Desk.webhooks.CREATE +Desk.webhooks.DELETE +aaaserver.profile.READ +``` + +Sim sends this list on every token request, so there is nothing to pre-configure on the Self Client itself. If Zoho rejects the request with an invalid-scope error, the Self Client's owner does not have access to one of the Desk modules above in that organization. + +A scope that is granted but insufficient surfaces at run time as a `4xx` from the Zoho Desk API naming the scope problem. + +### 5. Protect the Client Secret + +The client secret is bearer material for your Zoho Desk organization, limited only by the scopes above. Treat it like a password — do not commit it to source control or share it publicly. Sim encrypts it at rest. + + +Regenerating or revoking the Self Client in the Zoho API Console invalidates the stored pair immediately. If you rotate it, update the credential in Sim right away. + + +## Adding the Self Client to Sim + + + + Open **Integrations** from your workspace sidebar + + + Search for "Zoho Desk" and open it, then click **Add to Sim** and choose **Add Self Client** + + {/* TODO(screenshot): Zoho Desk integration page with the Add Self Client connect option */} + + + In the **Add Zoho Desk Self Client** dialog, paste the **Client ID**, the **Client secret**, and the numeric **Organization ID**. Set **Data center** to your region (`us`, `eu`, `in`, or `au`) — leave it blank for US. Optionally set a display name and description + + {/* TODO(screenshot): Add Zoho Desk Self Client dialog with all fields filled in */} + + + Click **Add Self Client**. Sim verifies the credentials by minting a real access token from Zoho — if it fails, the error tells you whether Zoho rejected the credentials or couldn't be reached. + + + +## Using the Self Client in Workflows + +Add a Zoho Desk block to your workflow. In the credential dropdown, your Self Client appears alongside any OAuth credentials. Select it and configure the block as you normally would. + +{/* TODO(screenshot): Zoho Desk block in a workflow with the Self Client selected as the credential */} + +The block calls the Zoho Desk REST API with a freshly minted access token — the same requests as the OAuth flow, so the Zoho Desk tools work the same way, subject to the scopes above. + +One exception is worth knowing: Zoho publishes no OAuth scope for the attachment download sub-path, so **Get Attachment** is the one operation whose scope requirement we could not confirm from Zoho's documentation. If it returns a scope error, the requested scope list needs widening. + +### Triggers still need OAuth + +The Zoho Desk **trigger** provisions and tears down its own webhook subscription against your Desk organization, and that provisioning path currently runs only against a personal OAuth connection. Connect a Zoho Desk account through OAuth for triggers, and use the Self Client for the blocks that read and update tickets. + +Because the OAuth flow is US-only, an organization outside the US data center can use Zoho Desk blocks through a Self Client but cannot use the Zoho Desk trigger. + +## Token Behavior + +Access tokens minted from a Self Client live for one hour and there is **no refresh token** — Sim mints a new token whenever one is needed, and caches the current one until it is close to expiry. Two events invalidate the stored credential: + +- **Revoking or deleting the Self Client** in the Zoho API Console — no new tokens can be minted +- **Regenerating the client secret** — the stored pair stops working; paste the new secret into the credential in Sim + + value instead." }, + { question: "Zoho rejects my credentials with invalid_client — why?", answer: "Either the client ID or secret was mistyped, or the client you created is not a Self Client. Only Self Clients support the client-credentials grant — in the Zoho API Console, Add Client → Self Client. Copy both values from the client's Client Secret tab." }, + { question: "Zoho returns missing_org_info or rejects the organization — why?", answer: "Zoho could not resolve a Desk organization from the ID you pasted. Re-copy the numeric Organization ID from Setup → Developer Space → API in the Desk portal you want to use. If your Zoho account has multiple Desk portals, make sure it is the ID of the right one." }, + { question: "Can I use a Self Client with a non-US Zoho account?", answer: "Yes, for the US, EU, IN, and AU data centers. Set the Data center field to us, eu, in, or au when you add the credential, and Sim mints tokens against that region's accounts server and calls the Desk host in the same region. Leaving it blank means US. The JP, CA, SA, CN, and UK data centers are not supported yet, and the interactive OAuth connection remains US-only." }, + { question: "I picked the wrong data center — what happens?", answer: "The region's accounts server does not know your organization, so Zoho rejects the token request and Sim reports that it could not authenticate. Edit the credential and set the Data center to the region whose domain you sign in to Zoho Desk with." }, + { question: "Why doesn't my Zoho Desk trigger work with the Self Client?", answer: "The trigger provisions a webhook subscription in your Desk organization, and that path runs against a personal OAuth connection only. Connect Zoho Desk through OAuth for triggers. Separately, Zoho Desk webhooks require a Professional-edition plan or above — they are unavailable on Free and Standard." }, + { question: "How do I rotate the credentials?", answer: "Regenerate the client secret on the Self Client's Client Secret tab in the Zoho API Console, then update the credential in Sim with the new secret. The old secret stops working as soon as it's regenerated, so update Sim promptly." }, +]} /> diff --git a/apps/docs/content/docs/en/integrations/zoho_desk.mdx b/apps/docs/content/docs/en/integrations/zoho_desk.mdx new file mode 100644 index 00000000000..2081c2a2a14 --- /dev/null +++ b/apps/docs/content/docs/en/integrations/zoho_desk.mdx @@ -0,0 +1,515 @@ +--- +title: Zoho Desk +description: Manage Zoho Desk tickets, comments, threads, and contacts +--- + +import { BlockInfoCard } from "@/components/ui/block-info-card" + + + +{/* MANUAL-CONTENT-START:intro */} +[Zoho Desk](https://www.zoho.com/desk/) is Zoho's customer support help desk. Support teams use it to receive tickets from email, web forms, chat, phone, and social channels, route them to the right department and agent, and track every customer conversation through to resolution. + +With the Sim Zoho Desk integration, you can: + +- **Read and filter tickets**: List tickets across an organization filtered by department, status, or priority, or fetch a single ticket by ID with its related contact, assignee, and department. +- **Update tickets**: Change subject, status, priority, assignee, department, category, due date, and custom fields — useful for AI triage that classifies an incoming ticket and writes the result back. +- **Work with conversations**: List and read ticket threads (the customer-facing email/chat exchange) and comments (internal agent notes), then add your own comment as public or private. +- **Look up contacts**: Retrieve the contact behind a ticket to enrich it with data from your CRM or knowledge base. +- **Download attachments**: Pull an attachment from a thread or comment into a Sim file you can pass to downstream blocks. +- **Trigger on events**: Start a workflow when a ticket, comment, thread, contact, agent, task, or article changes in Zoho Desk. + +**How it works in Sim:** +Add a Zoho Desk block to your workflow, connect your Zoho account, and pick the Organization (portal) to work in — Sim loads the list for you from the connected account. Choose an operation and fill in its parameters; the block calls the Zoho Desk API and returns structured data for downstream blocks. For comment and thread bodies, Sim adds a derived plain-text `contentText` field alongside Zoho's raw HTML `content`, so an AI agent can read the message without HTML markup. + +To trigger on Zoho Desk activity instead, use the block's trigger mode. Sim creates the webhook subscription in Zoho Desk for you and removes it automatically when the workflow is undeployed. + +**Requirements and limitations** + +> Zoho Desk webhooks require a Zoho Desk edition of **Professional or higher** — Free and Standard plans cannot create webhook subscriptions, so the trigger will fail to deploy on those plans. +> +> Connecting a Zoho account with **OAuth** — which the trigger requires — works only for the **US data center** (`accounts.zoho.com`). To use Zoho Desk blocks from the EU, India, or Australia data centers, connect a [Self Client](/integrations/zoho-desk-service-account) instead and set its data center. The Japan, Canada, Saudi Arabia, China, and UK data centers are not supported by either path. +{/* MANUAL-CONTENT-END */} + + +## Usage Instructions + +Read and update Zoho Desk tickets, manage comments and threads, look up contacts, and download attachments. Can also trigger workflows from Zoho Desk webhook events. + + + +## Actions + +### `zoho_desk_list_tickets` + +List tickets from a Zoho Desk organization with optional filters. Returns a list projection: description, resolution, statusType and classification are only available from Get Ticket. + +#### Input + +| Parameter | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `apiDomain` | string | No | Zoho Desk data-center REST base URL | +| `orgId` | string | Yes | Zoho Desk organization ID | +| `from` | number | No | Pagination start index \(0-based, max 4999\) | +| `limit` | number | No | Number of tickets to return \(1-100, default 10\) | +| `departmentIds` | string | No | Filter by department ID \(comma-separated for multiple\) | +| `status` | string | No | Filter by status, including custom statuses. Comma-separate to match multiple \(e.g. "Open,On Hold"\) | +| `priority` | string | No | Filter by priority. Comma-separate to match multiple \(e.g. "High,Urgent"\) | +| `sortBy` | string | No | Sort field: createdTime, customerResponseTime, or responseDueDate. Prefix with - for descending. | +| `include` | string | No | Comma-separated related data to embed. Allowed: contacts, products, departments, team, isRead, assignee | + +#### Output + +| Parameter | Type | Description | +| --------- | ---- | ----------- | +| `tickets` | array | List of tickets | +| ↳ `id` | string | Ticket ID | +| ↳ `ticketNumber` | string | Human-readable ticket number | +| ↳ `subject` | string | Ticket subject | +| ↳ `description` | string | Ticket description \(raw; may be HTML\) | +| ↳ `descriptionText` | string | Plain-text rendering of the description: HTML stripped when the body contains markup, otherwise the description verbatim | +| ↳ `status` | string | Ticket status | +| ↳ `statusType` | string | Status category \(Open/Closed/On Hold\) | +| ↳ `priority` | string | Ticket priority | +| ↳ `category` | string | Ticket category | +| ↳ `subCategory` | string | Ticket sub-category | +| ↳ `classification` | string | Ticket classification | +| ↳ `channel` | string | Origin channel | +| ↳ `departmentId` | string | Department ID | +| ↳ `contactId` | string | Contact ID | +| ↳ `accountId` | string | Account ID | +| ↳ `assigneeId` | string | Assignee ID | +| ↳ `email` | string | Contact email | +| ↳ `phone` | string | Contact phone | +| ↳ `dueDate` | string | Due date | +| ↳ `responseDueDate` | string | Response due date | +| ↳ `createdTime` | string | Created timestamp | +| ↳ `modifiedTime` | string | Last modified timestamp | +| ↳ `closedTime` | string | Closed timestamp | +| ↳ `resolution` | string | Resolution text | +| ↳ `threadCount` | string | Number of threads | +| ↳ `commentCount` | string | Number of comments | +| ↳ `webUrl` | string | Web URL to the ticket | +| ↳ `isEscalated` | boolean | Whether the ticket is escalated | +| ↳ `isOverDue` | boolean | Whether the ticket is overdue | +| ↳ `isSpam` | boolean | Whether the ticket is marked spam | +| ↳ `cf` | json | Custom field values, keyed by custom field API name | +| `count` | number | Number of tickets returned | + +### `zoho_desk_get_ticket` + +Retrieve a single Zoho Desk ticket by ID. + +#### Input + +| Parameter | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `apiDomain` | string | No | Zoho Desk data-center REST base URL | +| `orgId` | string | Yes | Zoho Desk organization ID | +| `ticketId` | string | Yes | Ticket ID to retrieve | +| `include` | string | No | Comma-separated related data to embed. Allowed: contacts, products, assignee, departments, contract, isRead, team, skills | + +#### Output + +| Parameter | Type | Description | +| --------- | ---- | ----------- | +| `ticket` | object | The ticket | +| ↳ `id` | string | Ticket ID | +| ↳ `ticketNumber` | string | Human-readable ticket number | +| ↳ `subject` | string | Ticket subject | +| ↳ `description` | string | Ticket description \(raw; may be HTML\) | +| ↳ `descriptionText` | string | Plain-text rendering of the description: HTML stripped when the body contains markup, otherwise the description verbatim | +| ↳ `status` | string | Ticket status | +| ↳ `statusType` | string | Status category \(Open/Closed/On Hold\) | +| ↳ `priority` | string | Ticket priority | +| ↳ `category` | string | Ticket category | +| ↳ `subCategory` | string | Ticket sub-category | +| ↳ `classification` | string | Ticket classification | +| ↳ `channel` | string | Origin channel | +| ↳ `departmentId` | string | Department ID | +| ↳ `contactId` | string | Contact ID | +| ↳ `accountId` | string | Account ID | +| ↳ `assigneeId` | string | Assignee ID | +| ↳ `email` | string | Contact email | +| ↳ `phone` | string | Contact phone | +| ↳ `dueDate` | string | Due date | +| ↳ `responseDueDate` | string | Response due date | +| ↳ `createdTime` | string | Created timestamp | +| ↳ `modifiedTime` | string | Last modified timestamp | +| ↳ `closedTime` | string | Closed timestamp | +| ↳ `resolution` | string | Resolution text | +| ↳ `threadCount` | string | Number of threads | +| ↳ `commentCount` | string | Number of comments | +| ↳ `webUrl` | string | Web URL to the ticket | +| ↳ `isEscalated` | boolean | Whether the ticket is escalated | +| ↳ `isOverDue` | boolean | Whether the ticket is overdue | +| ↳ `isSpam` | boolean | Whether the ticket is marked spam | +| ↳ `cf` | json | Custom field values, keyed by custom field API name | + +### `zoho_desk_update_ticket` + +Update fields on an existing Zoho Desk ticket. + +#### Input + +| Parameter | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `apiDomain` | string | No | Zoho Desk data-center REST base URL | +| `orgId` | string | Yes | Zoho Desk organization ID | +| `ticketId` | string | Yes | Ticket ID to update | +| `subject` | string | No | Ticket subject | +| `status` | string | No | Ticket status \(e.g. Open, Closed\) | +| `priority` | string | No | Ticket priority \(e.g. High\) | +| `assigneeId` | string | No | Assignee \(agent\) ID | +| `departmentId` | string | No | Department ID | +| `category` | string | No | Ticket category | +| `subCategory` | string | No | Ticket sub-category | +| `dueDate` | string | No | Due date \(ISO 8601\) | +| `description` | string | No | Ticket description | +| `resolution` | string | No | Resolution notes recorded on the ticket | +| `classification` | string | No | Ticket classification: Problem, Request, Question, or Others | +| `customFields` | json | No | Custom field values as a JSON object, keyed by custom field API name | + +#### Output + +| Parameter | Type | Description | +| --------- | ---- | ----------- | +| `ticket` | object | The updated ticket | +| ↳ `id` | string | Ticket ID | +| ↳ `ticketNumber` | string | Human-readable ticket number | +| ↳ `subject` | string | Ticket subject | +| ↳ `description` | string | Ticket description \(raw; may be HTML\) | +| ↳ `descriptionText` | string | Plain-text rendering of the description: HTML stripped when the body contains markup, otherwise the description verbatim | +| ↳ `status` | string | Ticket status | +| ↳ `statusType` | string | Status category \(Open/Closed/On Hold\) | +| ↳ `priority` | string | Ticket priority | +| ↳ `category` | string | Ticket category | +| ↳ `subCategory` | string | Ticket sub-category | +| ↳ `classification` | string | Ticket classification | +| ↳ `channel` | string | Origin channel | +| ↳ `departmentId` | string | Department ID | +| ↳ `contactId` | string | Contact ID | +| ↳ `accountId` | string | Account ID | +| ↳ `assigneeId` | string | Assignee ID | +| ↳ `email` | string | Contact email | +| ↳ `phone` | string | Contact phone | +| ↳ `dueDate` | string | Due date | +| ↳ `responseDueDate` | string | Response due date | +| ↳ `createdTime` | string | Created timestamp | +| ↳ `modifiedTime` | string | Last modified timestamp | +| ↳ `closedTime` | string | Closed timestamp | +| ↳ `resolution` | string | Resolution text | +| ↳ `threadCount` | string | Number of threads | +| ↳ `commentCount` | string | Number of comments | +| ↳ `webUrl` | string | Web URL to the ticket | +| ↳ `isEscalated` | boolean | Whether the ticket is escalated | +| ↳ `isOverDue` | boolean | Whether the ticket is overdue | +| ↳ `isSpam` | boolean | Whether the ticket is marked spam | +| ↳ `cf` | json | Custom field values, keyed by custom field API name | + +### `zoho_desk_list_comments` + +List comments on a Zoho Desk ticket. + +#### Input + +| Parameter | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `apiDomain` | string | No | Zoho Desk data-center REST base URL | +| `orgId` | string | Yes | Zoho Desk organization ID | +| `ticketId` | string | Yes | Ticket ID | +| `from` | number | No | Pagination start index \(0-based\) | +| `limit` | number | No | Number of comments to return \(1-100, default 50\) | + +#### Output + +| Parameter | Type | Description | +| --------- | ---- | ----------- | +| `comments` | array | List of comments | +| ↳ `id` | string | Comment ID | +| ↳ `content` | string | Comment content \(raw; may be HTML\) | +| ↳ `contentType` | string | Content type \(plainText/html\) | +| ↳ `contentText` | string | Plain-text rendering of content \(HTML stripped when contentType is html\) | +| ↳ `isPublic` | boolean | Whether the comment is public | +| ↳ `commenterId` | string | Commenter ID | +| ↳ `commenter` | object | Who wrote the comment | +| ↳ `name` | string | Display name | +| ↳ `firstName` | string | First name | +| ↳ `lastName` | string | Last name | +| ↳ `email` | string | Email address | +| ↳ `type` | string | Commenter type \(AGENT/END_USER\) | +| ↳ `roleName` | string | Role name | +| ↳ `photoURL` | string | Avatar URL | +| ↳ `commentedTime` | string | Commented timestamp | +| ↳ `modifiedTime` | string | Modified timestamp | +| ↳ `attachments` | array | Comment attachments | +| ↳ `id` | string | Attachment ID | +| ↳ `name` | string | File name | +| ↳ `size` | string | File size as reported by Zoho | +| ↳ `href` | string | Download href | +| `count` | number | Number of comments returned | + +### `zoho_desk_add_comment` + +Add a comment to a Zoho Desk ticket. + +#### Input + +| Parameter | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `apiDomain` | string | No | Zoho Desk data-center REST base URL | +| `orgId` | string | Yes | Zoho Desk organization ID | +| `ticketId` | string | Yes | Ticket ID | +| `content` | string | Yes | Comment content | +| `contentType` | string | No | Content type: plainText or html. Defaults to plainText so agent-written text posts literally; pass 'html' to send markup \(Zoho's own API default is html\). | +| `isPublic` | boolean | No | Whether the comment is public | + +#### Output + +| Parameter | Type | Description | +| --------- | ---- | ----------- | +| `comment` | object | The created comment | +| ↳ `id` | string | Comment ID | +| ↳ `content` | string | Comment content \(raw; may be HTML\) | +| ↳ `contentType` | string | Content type \(plainText/html\) | +| ↳ `contentText` | string | Plain-text rendering of content \(HTML stripped when contentType is html\) | +| ↳ `isPublic` | boolean | Whether the comment is public | +| ↳ `commenterId` | string | Commenter ID | +| ↳ `commenter` | object | Who wrote the comment | +| ↳ `name` | string | Display name | +| ↳ `firstName` | string | First name | +| ↳ `lastName` | string | Last name | +| ↳ `email` | string | Email address | +| ↳ `type` | string | Commenter type \(AGENT/END_USER\) | +| ↳ `roleName` | string | Role name | +| ↳ `photoURL` | string | Avatar URL | +| ↳ `commentedTime` | string | Commented timestamp | +| ↳ `modifiedTime` | string | Modified timestamp | +| ↳ `attachments` | array | Comment attachments | +| ↳ `id` | string | Attachment ID | +| ↳ `name` | string | File name | +| ↳ `size` | string | File size as reported by Zoho | +| ↳ `href` | string | Download href | + +### `zoho_desk_list_threads` + +List conversation threads on a Zoho Desk ticket, newest first (Zoho sorts by sendDateTime descending by default). Returns a list projection: message bodies (content, summary, to/cc/bcc) come back only from Get Thread. + +#### Input + +| Parameter | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `apiDomain` | string | No | Zoho Desk data-center REST base URL | +| `orgId` | string | Yes | Zoho Desk organization ID | +| `ticketId` | string | Yes | Ticket ID | +| `from` | number | No | Pagination start index \(0-based\) | +| `limit` | number | No | Number of threads to return \(1-200, default 100\) | + +#### Output + +| Parameter | Type | Description | +| --------- | ---- | ----------- | +| `threads` | array | List of threads | +| ↳ `id` | string | Thread ID | +| ↳ `channel` | string | Thread channel | +| ↳ `direction` | string | Direction \(in/out\) | +| ↳ `content` | string | Thread content \(raw; may be HTML\) | +| ↳ `contentType` | string | Content type | +| ↳ `contentText` | string | Plain-text rendering of content \(HTML stripped when contentType is html\) | +| ↳ `summary` | string | Thread summary | +| ↳ `responderId` | string | Responder ID | +| ↳ `createdTime` | string | Created timestamp | +| ↳ `hasAttach` | boolean | Whether the thread has attachments | +| ↳ `attachmentCount` | string | Number of attachments | +| ↳ `fromEmailAddress` | string | From email address | +| ↳ `to` | string | To email address | +| ↳ `cc` | string | CC email address | +| ↳ `bcc` | string | BCC email address | +| ↳ `replyTo` | string | Reply-to email address | +| ↳ `isForward` | boolean | Whether the thread is a forward | +| ↳ `isContentTruncated` | boolean | Whether Zoho truncated the thread content; fetch fullContentURL for the rest | +| ↳ `fullContentURL` | string | URL returning the untruncated thread content | +| ↳ `plainText` | string | Zoho's own plain-text rendering of the thread, when it supplies one | +| ↳ `status` | string | Delivery status of an outgoing thread \(SUCCESS/FAILED/DRAFT\) | +| ↳ `isDescriptionThread` | boolean | Whether this thread is the ticket's original description | +| ↳ `visibility` | string | Thread visibility \(e.g. public\) | +| ↳ `canReply` | boolean | Whether the thread can be replied to | +| ↳ `author` | object | Who sent the thread | +| ↳ `name` | string | Display name | +| ↳ `firstName` | string | First name | +| ↳ `lastName` | string | Last name | +| ↳ `email` | string | Email address | +| ↳ `type` | string | Author type \(AGENT/END_USER\) | +| ↳ `photoURL` | string | Avatar URL | +| ↳ `attachments` | array | Thread attachments | +| ↳ `id` | string | Attachment ID | +| ↳ `name` | string | File name | +| ↳ `size` | string | File size as reported by Zoho | +| ↳ `href` | string | Download href | +| `count` | number | Number of threads returned | + +### `zoho_desk_get_thread` + +Retrieve the full content of a single Zoho Desk ticket thread. + +#### Input + +| Parameter | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `apiDomain` | string | No | Zoho Desk data-center REST base URL | +| `orgId` | string | Yes | Zoho Desk organization ID | +| `ticketId` | string | Yes | Ticket ID | +| `threadId` | string | Yes | Thread ID | + +#### Output + +| Parameter | Type | Description | +| --------- | ---- | ----------- | +| `thread` | object | The thread | +| ↳ `id` | string | Thread ID | +| ↳ `channel` | string | Thread channel | +| ↳ `direction` | string | Direction \(in/out\) | +| ↳ `content` | string | Thread content \(raw; may be HTML\) | +| ↳ `contentType` | string | Content type | +| ↳ `contentText` | string | Plain-text rendering of content \(HTML stripped when contentType is html\) | +| ↳ `summary` | string | Thread summary | +| ↳ `responderId` | string | Responder ID | +| ↳ `createdTime` | string | Created timestamp | +| ↳ `hasAttach` | boolean | Whether the thread has attachments | +| ↳ `attachmentCount` | string | Number of attachments | +| ↳ `fromEmailAddress` | string | From email address | +| ↳ `to` | string | To email address | +| ↳ `cc` | string | CC email address | +| ↳ `bcc` | string | BCC email address | +| ↳ `replyTo` | string | Reply-to email address | +| ↳ `isForward` | boolean | Whether the thread is a forward | +| ↳ `isContentTruncated` | boolean | Whether Zoho truncated the thread content; fetch fullContentURL for the rest | +| ↳ `fullContentURL` | string | URL returning the untruncated thread content | +| ↳ `plainText` | string | Zoho's own plain-text rendering of the thread, when it supplies one | +| ↳ `status` | string | Delivery status of an outgoing thread \(SUCCESS/FAILED/DRAFT\) | +| ↳ `isDescriptionThread` | boolean | Whether this thread is the ticket's original description | +| ↳ `visibility` | string | Thread visibility \(e.g. public\) | +| ↳ `canReply` | boolean | Whether the thread can be replied to | +| ↳ `author` | object | Who sent the thread | +| ↳ `name` | string | Display name | +| ↳ `firstName` | string | First name | +| ↳ `lastName` | string | Last name | +| ↳ `email` | string | Email address | +| ↳ `type` | string | Author type \(AGENT/END_USER\) | +| ↳ `photoURL` | string | Avatar URL | +| ↳ `attachments` | array | Thread attachments | +| ↳ `id` | string | Attachment ID | +| ↳ `name` | string | File name | +| ↳ `size` | string | File size as reported by Zoho | +| ↳ `href` | string | Download href | + +### `zoho_desk_get_contact` + +Retrieve a Zoho Desk contact by ID. + +#### Input + +| Parameter | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `apiDomain` | string | No | Zoho Desk data-center REST base URL | +| `orgId` | string | Yes | Zoho Desk organization ID | +| `contactId` | string | Yes | Contact ID to retrieve | + +#### Output + +| Parameter | Type | Description | +| --------- | ---- | ----------- | +| `contact` | object | The contact | +| ↳ `id` | string | Contact ID | +| ↳ `firstName` | string | First name | +| ↳ `lastName` | string | Last name | +| ↳ `email` | string | Primary email | +| ↳ `secondaryEmail` | string | Secondary email | +| ↳ `phone` | string | Phone number | +| ↳ `mobile` | string | Mobile number | +| ↳ `accountId` | string | Associated account ID | +| ↳ `ownerId` | string | Owner ID | +| ↳ `type` | string | Contact type | +| ↳ `title` | string | Job title | +| ↳ `street` | string | Street | +| ↳ `city` | string | City | +| ↳ `state` | string | State | +| ↳ `country` | string | Country | +| ↳ `zip` | string | ZIP / postal code | +| ↳ `description` | string | Description | +| ↳ `cf` | json | Custom field values, keyed by custom field API name | + +### `zoho_desk_get_attachment` + +Download a Zoho Desk ticket attachment (from its href) as a file. + +#### Input + +| Parameter | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `apiDomain` | string | No | Zoho Desk data-center REST base URL | +| `orgId` | string | Yes | Zoho Desk organization ID | +| `href` | string | Yes | Attachment download href \(from a thread or comment attachment\) | +| `fileName` | string | No | Optional file name for the downloaded file | + +#### Output + +| Parameter | Type | Description | +| --------- | ---- | ----------- | +| `file` | file | The downloaded attachment file | + +### `zoho_desk_list_organizations` + +List the Zoho Desk organizations (portals) the connected account can access. + +#### Input + +| Parameter | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `apiDomain` | string | No | Zoho Desk data-center REST base URL | + +#### Output + +| Parameter | Type | Description | +| --------- | ---- | ----------- | +| `organizations` | array | Accessible organizations | +| ↳ `id` | string | Organization ID | +| ↳ `companyName` | string | Company name | +| ↳ `portalName` | string | Portal name | +| `count` | number | Number of organizations returned | + + + +## Triggers + +A **Trigger** is a block that starts a workflow when an event happens in this service. + +### Zoho Desk Event + +Trigger a workflow when a Zoho Desk event occurs (ticket, comment, thread, contact, agent, task, or article changes). + +#### Configuration + +| Parameter | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `triggerCredentials` | string | Yes | This trigger creates and manages a webhook subscription in your Zoho Desk account. | +| `orgId` | project-selector | Yes | The Zoho Desk organization \(portal\) to subscribe in. | +| `manualOrgId` | string | Yes | Type an organization ID instead of picking one from the list. | +| `eventType` | string | Yes | Event | +| `triggerDepartmentIds` | string | No | Restrict events to these departments. Leave empty for all departments. | +| `fields` | string | No | For Ticket Updated: only fire when one of these fields changes \(max 5\). Previous values are included in the payload. | +| `direction` | string | No | Thread Direction | + +#### Output + +| Parameter | Type | Description | +| --------- | ---- | ----------- | +| `eventType` | string | The Zoho Desk event type \(e.g. Ticket_Add\) | +| `eventTime` | string | Event time in milliseconds since epoch | +| `orgId` | string | Zoho Desk organization ID | +| `payload` | json | The full resource that changed \(ticket, comment, thread, etc.\). Comment and thread events gain a derived plain-text `contentText` alongside the raw `content` + `contentType`; ticket events gain `descriptionText` alongside `description`. | +| `prevState` | json | Previous state of the resource \(update events only\) | + diff --git a/apps/docs/content/docs/en/logs-debugging/index.mdx b/apps/docs/content/docs/en/logs-debugging/index.mdx index df544382d9a..fcf9b8c8f49 100644 --- a/apps/docs/content/docs/en/logs-debugging/index.mdx +++ b/apps/docs/content/docs/en/logs-debugging/index.mdx @@ -43,7 +43,7 @@ This is the level you debug at, because a run fails when one of its blocks fails ### Input and output -Each block in the sidebar has two tabs. The **Input** tab shows the resolved values the block actually ran with: the literal values you typed, and the earlier outputs it read by reference (with API keys redacted). The **Output** tab shows what the block produced, formatted as an object, with markdown rendered for agent-generated text. +Each block in the sidebar has two tabs. The **Input** tab shows the resolved values the block actually ran with: the literal values you typed, and the earlier outputs it read by reference. Exact values successfully substituted from Secrets through `{{KEY}}` are masked in this trace copy; see [Execution log protection](/platform/credentials#execution-log-protection). The **Output** tab shows what the block produced, formatted as an object, with markdown rendered for agent-generated text. The input tab is the important one. A block reads earlier outputs by name, written ``, and the input tab shows what those references resolved to at run time. If a reference pointed at a value that was not there, you see it here as missing or wrong, not as the tag you wrote. See [how blocks pass data](/workflows/data-flow) for how those references resolve. diff --git a/apps/docs/content/docs/en/logs-debugging/logging.mdx b/apps/docs/content/docs/en/logs-debugging/logging.mdx index 3c1b3970501..fdbcbdbd64f 100644 --- a/apps/docs/content/docs/en/logs-debugging/logging.mdx +++ b/apps/docs/content/docs/en/logs-debugging/logging.mdx @@ -57,7 +57,7 @@ Click any entry to open its sidebar: the run's timeline (start/end, total durati The block's result — JSON-formatted structured data, markdown rendering for AI content, and a copy button. - What the block received — resolved variable values, referenced outputs, and environment variables. API keys are automatically redacted. + What the block received — resolved variable values, referenced outputs, and environment variables. Exact secret values activated by a successful `{{KEY}}` substitution are masked in this trace view. See [Execution log protection](/platform/credentials#execution-log-protection). @@ -95,7 +95,7 @@ import { FAQ } from '@/components/ui/faq' +### Execution log protection + +When a saved secret is successfully substituted through a `{{KEY}}` reference, Sim masks exact, case-sensitive occurrences of its resolved value in log-facing content. This includes the editor's live block-log display, Logs Overview input and output, stored execution traces, log-read API responses, and the Logs block's **Get Run Details** output. Function and Agent span inputs, outputs, and errors, Agent thinking, and Agent tool-call arguments, results, and errors are protected. The replacement is normally shown as `{{KEY}}`. + +This is an observability projection only. Secret resolution and workflow behavior are unchanged: blocks, tools, models, and downstream steps receive the real runtime value. Stored functional execution data, workflow execution responses, streams, callbacks, block state, and snapshots are not rewritten. Log-facing views and read APIs receive a separate protected copy, so the Logs Overview **Workflow Input** and **Workflow Output** are masked without changing the underlying workflow result. + -Secret values are never exposed in the workflow editor or execution logs — they are only resolved during execution. +Masking is activated only when Sim successfully resolves a value from **Settings → Secrets** through `{{KEY}}`. A hardcoded literal, direct `environmentVariables['KEY']` read, or shell `$KEY` read does not activate it by itself. Once activated, every exact occurrence of that value in the run's log-facing content is masked. Encoded, hashed, or otherwise transformed versions are not matched. Do not deliberately return or print secrets. ## Secret Details @@ -115,7 +121,8 @@ When a workflow runs, secrets resolve in this order: - **Never hardcode secrets** in workflow input fields — always use `{{KEY}}` references The block always operates on the current workspace. Costs are denominated in credits, both for - the cost filter and the cost output. + the cost filter and the cost output. **Get Run Details** is a log-read path, so both `traceSpans` + and `finalOutput` come from the protected log-facing projection described under + [Execution log protection](/platform/credentials#execution-log-protection). The underlying runtime + result remains unchanged. ({ @@ -12,7 +15,7 @@ const { mockResolveRole } = vi.hoisted(() => ({ })) vi.mock('@/middleware/permissions', () => ({ - resolveCurrentWorkflowRole: mockResolveRole, + resolveCurrentRoomPermission: mockResolveRole, ROLE_REVALIDATION_TTL_MS: 30_000, })) @@ -20,6 +23,7 @@ import { ACCESS_REVALIDATION_SWEEP_INTERVAL_MS, startAccessRevalidationSweep, } from '@/access-revalidation' +import { registerRoomEvictionHandler } from '@/handlers/room-eviction' import type { IRoomManager, UserPresence } from '@/rooms' interface FakeSocket { @@ -30,9 +34,9 @@ interface FakeSocket { leave: ReturnType } -function makeSocket(id: string, userId: string | undefined, workflowId?: string): FakeSocket { +function makeSocket(id: string, userId: string | undefined, room?: string): FakeSocket { const rooms = new Set([id]) - if (workflowId) rooms.add(workflowId) + if (room) rooms.add(room) return { id, userId, @@ -131,7 +135,7 @@ describe('access-revalidation sweep', () => { expect(manager.removeUserFromRoom).not.toHaveBeenCalled() }) - it('resolves with the static safe fallback and no presence reads in the scan', async () => { + it('resolves with the room safe fallback and no presence reads in the scan', async () => { const socket = makeSocket('sock-1', 'user-1', 'wf-1') const manager = makeManager([socket], [{ socketId: 'sock-1', role: 'admin' }]) mockResolveRole.mockResolvedValue('admin') @@ -140,32 +144,123 @@ describe('access-revalidation sweep', () => { await sweep.runOnce() sweep.stop() - expect(mockResolveRole).toHaveBeenCalledWith('user-1', 'wf-1', 'read') + expect(mockResolveRole).toHaveBeenCalledWith('user-1', { type: 'workflow', id: 'wf-1' }, 'read') // The security scan must stay Redis-free — presence is never consulted. expect(manager.getRoomUsers).not.toHaveBeenCalled() }) - it('never evicts a socket joined only to a non-workflow room (files/tables/file-doc)', async () => { - // The sweep shares one io with the files/tables/file-doc handlers. Those rooms are - // namespaced (`workspace-files:ws-1`, `table:t-1`), so treating every socket.rooms - // entry as a workflow id would resolve a bogus permission → null → evict the socket - // from its files/table room every pass. Non-workflow rooms must be filtered out. + it('sweeps non-workflow rooms against their own resource, not a bogus workflow id', async () => { + // The sweep shares one io with the files/tables/file-doc handlers. Their rooms are + // namespaced (`workspace-files:ws-1`, `table:t-1`), so each name is decoded and + // authorized as its own room type — the whole point of covering them at all. const filesSocket = makeSocket('sock-1', 'user-1', 'workspace-files:ws-1') const tableSocket = makeSocket('sock-2', 'user-2', 'table:t-1') const manager = makeManager([filesSocket, tableSocket]) - // Even if the role resolver would say "no access", these must never be swept. - mockResolveRole.mockResolvedValue(null) + mockResolveRole.mockResolvedValue('write') const sweep = startAccessRevalidationSweep(manager) await sweep.runOnce() sweep.stop() - expect(mockResolveRole).not.toHaveBeenCalled() + expect(mockResolveRole).toHaveBeenCalledWith( + 'user-1', + { type: 'workspace-files', id: 'ws-1' }, + 'read' + ) + expect(mockResolveRole).toHaveBeenCalledWith('user-2', { type: 'table', id: 't-1' }, 'read') + // Still authorized: nobody is evicted. expect(filesSocket.leave).not.toHaveBeenCalled() - expect(filesSocket.emit).not.toHaveBeenCalled() expect(tableSocket.leave).not.toHaveBeenCalled() - expect(tableSocket.emit).not.toHaveBeenCalled() + }) + + it('evicts a revoked socket from a presence-free workspace-files room without touching presence', async () => { + const socket = makeSocket('sock-1', 'user-1', 'workspace-files:ws-1') + const manager = makeManager([socket]) + mockResolveRole.mockResolvedValue(null) + + const sweep = startAccessRevalidationSweep(manager) + await sweep.runOnce() + sweep.stop() + + expect(socket.emit).toHaveBeenCalledWith( + 'room-access-revoked', + expect.objectContaining({ room: { type: 'workspace-files', id: 'ws-1' } }) + ) + expect(socket.leave).toHaveBeenCalledWith('workspace-files:ws-1') + // These rooms hold no room-manager presence, so nothing is owed to the cleanup lane. expect(manager.removeUserFromRoom).not.toHaveBeenCalled() + expect(manager.broadcastPresenceUpdate).not.toHaveBeenCalled() + }) + + it('evicts a revoked socket from a table room and clears its presence', async () => { + const socket = makeSocket('sock-1', 'user-1', 'table:t-1') + const manager = makeManager([socket], [{ socketId: 'sock-1', role: 'read' }]) + mockResolveRole.mockResolvedValue(null) + + const sweep = startAccessRevalidationSweep(manager) + await sweep.runOnce() + sweep.stop() + + expect(socket.leave).toHaveBeenCalledWith('table:t-1') + expect(manager.removeUserFromRoom).toHaveBeenCalledWith({ type: 'table', id: 't-1' }, 'sock-1') + expect(manager.broadcastPresenceUpdate).toHaveBeenCalledWith({ type: 'table', id: 't-1' }) + }) + + it('evicts a file-doc socket downgraded to read, and keeps its table room', async () => { + // A file-doc room IS the editor and requires `write`; a table room requires only + // `read`. One downgraded user in both rooms must lose exactly the document. + const socket = makeSocket('sock-1', 'user-1', 'workspace-file-doc:file-1') + socket.rooms.add('table:t-1') + const manager = makeManager([socket]) + mockResolveRole.mockResolvedValue('read') + + const sweep = startAccessRevalidationSweep(manager) + await sweep.runOnce() + sweep.stop() + + expect(socket.leave).toHaveBeenCalledWith('workspace-file-doc:file-1') + expect(socket.leave).not.toHaveBeenCalledWith('table:t-1') + expect(socket.emit).toHaveBeenCalledWith( + 'room-access-revoked', + expect.objectContaining({ room: { type: 'workspace-file-doc', id: 'file-1' } }) + ) + }) + + it('falls back to the room type own membership level on a cold-cache failure', async () => { + // A static 'read' fallback would have evicted every file-doc socket (which needs + // `write`) the first time the DB blipped with a cold cache. + const socket = makeSocket('sock-1', 'user-1', 'workspace-file-doc:file-1') + const manager = makeManager([socket]) + mockResolveRole.mockResolvedValue('write') + + const sweep = startAccessRevalidationSweep(manager) + await sweep.runOnce() + sweep.stop() + + expect(mockResolveRole).toHaveBeenCalledWith( + 'user-1', + { type: 'workspace-file-doc', id: 'file-1' }, + 'write' + ) + expect(socket.leave).not.toHaveBeenCalled() + }) + + it('runs the room type registered eviction handler so handler-local state is dropped', async () => { + const evicted = vi.fn() + registerRoomEvictionHandler(ROOM_TYPES.WORKSPACE_FILE_DOC, evicted) + const socket = makeSocket('sock-1', 'user-1', 'workspace-file-doc:file-1') + const manager = makeManager([socket]) + mockResolveRole.mockResolvedValue(null) + + const sweep = startAccessRevalidationSweep(manager) + await sweep.runOnce() + sweep.stop() + + expect(evicted).toHaveBeenCalledWith( + 'sock-1', + { type: 'workspace-file-doc', id: 'file-1' }, + manager.io + ) }) it('evicts only the revoked socket, not co-members of the room', async () => { diff --git a/apps/realtime/src/access-revalidation.ts b/apps/realtime/src/access-revalidation.ts index 93edaae9a0b..8e0ca514812 100644 --- a/apps/realtime/src/access-revalidation.ts +++ b/apps/realtime/src/access-revalidation.ts @@ -1,10 +1,22 @@ import { createLogger } from '@sim/logger' +import { ROOM_MEMBERSHIP_ACTIONS, satisfiesRoomMembership } from '@sim/platform-authz/room-policy' import type { AccessRevokedBroadcast } from '@sim/realtime-protocol/events' -import { parseRoomName, ROOM_TYPES } from '@sim/realtime-protocol/rooms' +import { + parseRoomName, + ROOM_TYPES, + type RoomRef, + type RoomType, + roomName, +} from '@sim/realtime-protocol/rooms' import { sleep } from '@sim/utils/helpers' +import { + evictSocketFromRoom, + runRoomEvictionHandler, + setEvictionCleanupSink, +} from '@/handlers/room-eviction' import type { AuthenticatedSocket } from '@/middleware/auth' -import { ROLE_REVALIDATION_TTL_MS, resolveCurrentWorkflowRole } from '@/middleware/permissions' -import { type IRoomManager, workflowRoom as wf } from '@/rooms' +import { ROLE_REVALIDATION_TTL_MS, resolveCurrentRoomPermission } from '@/middleware/permissions' +import type { IRoomManager } from '@/rooms' const logger = createLogger('AccessRevalidation') @@ -21,14 +33,30 @@ const logger = createLogger('AccessRevalidation') export const ACCESS_REVALIDATION_SWEEP_INTERVAL_MS = ROLE_REVALIDATION_TTL_MS /** - * Non-null fallback consumed by {@link resolveCurrentWorkflowRole} only on a - * transient DB failure with a cold cache, where returning a non-null role - * (never eviction) is the safe outcome during an outage. The scan deliberately - * does not read presence for a per-socket join-time role: that would put a - * Redis dependency inside the security-critical scan lane, and the fallback's - * only job is to be non-null. + * Fallback consumed by {@link resolveCurrentRoomPermission} only on a transient DB + * failure with a cold cache, where resolving to a permission that KEEPS the socket + * (never eviction) is the safe outcome during an outage. It is the room's own + * membership level, so it satisfies that room's requirement exactly — a static + * `'read'` would have evicted every file-doc socket (which needs `write`) on a cold + * cache blip. The scan deliberately does not read presence for a per-socket + * join-time role: that would put a Redis dependency inside the security-critical + * scan lane, and the fallback's only job is to be permissive. */ -const FALLBACK_ROLE = 'read' +function fallbackRoleFor(type: RoomType): string { + return ROOM_MEMBERSHIP_ACTIONS[type] +} + +/** + * Room types whose membership is mirrored in the room manager's (Redis) presence + * state, and therefore need a presence removal + rebroadcast after an eviction. + * The workspace-files / workspace-tables invalidation rooms carry no presence at + * all, and a file-doc room's roster is pod-local in-memory state reconciled by its + * registered eviction handler — neither has anything for the cleanup lane to do. + */ +const PRESENCE_ROOM_TYPES: ReadonlySet = new Set([ + ROOM_TYPES.WORKFLOW, + ROOM_TYPES.TABLE, +]) /** * Upper bound on a single socket's authorization check inside the scan. A DB @@ -57,36 +85,39 @@ export interface AccessRevalidationSweep { } interface ScanTarget { - workflowId: string + room: RoomRef + /** The Socket.IO room name — the key the socket actually holds. */ + name: string socket: AuthenticatedSocket userId: string } /** - * Collects this pod's authenticated sockets with the workflow room each has + * Collects this pod's authenticated sockets paired with EVERY room each has * joined, in stable socket order. * * Rooms are derived from the socket's own `rooms` set (pod-local, no Redis * round-trips). A socket may occupy several rooms of different types at once - * (workflow canvas, workspace-files browser, table, file-doc), all on the same - * io — so each name is decoded with {@link parseRoomName} and only **workflow** - * rooms are swept here. Non-workflow room names are namespaced (`type:id`) and - * resolve to a non-workflow type; sweeping them as workflow ids would resolve a - * bogus permission, come back `null`, and spuriously evict the socket from its - * files/table room every pass. Only local sockets are evaluated — sockets are - * sticky to a pod, so every socket is swept by exactly one pod using that pod's - * warm role cache (mirroring the per-pod reasoning of the write-path cache). + * (workflow canvas, workspace-files browser, table, file-doc), all on the same io + * — so each name is decoded with {@link parseRoomName} and swept as its own type. + * Decoding (rather than assuming workflow) is what makes this safe: a namespaced + * `type:id` name resolves to the right resource, so it is authorized against that + * resource's workspace instead of resolving a bogus workflow permission. + * + * Only local sockets are evaluated — sockets are sticky to a pod, so every socket + * is swept by exactly one pod using that pod's warm role cache (mirroring the + * per-pod reasoning of the write-path cache). */ function collectScanTargets(io: IRoomManager['io']): ScanTarget[] { const targets: ScanTarget[] = [] for (const socket of io.sockets.sockets.values()) { const authed = socket as AuthenticatedSocket if (!authed.userId) continue - for (const room of socket.rooms) { - if (room === socket.id) continue - const ref = parseRoomName(room) - if (ref?.type !== ROOM_TYPES.WORKFLOW) continue - targets.push({ workflowId: ref.id, socket: authed, userId: authed.userId }) + for (const name of socket.rooms) { + if (name === socket.id) continue + const ref = parseRoomName(name) + if (!ref) continue + targets.push({ room: ref, name, socket: authed, userId: authed.userId }) } } return targets @@ -94,14 +125,19 @@ function collectScanTargets(io: IRoomManager['io']): ScanTarget[] { /** * Starts a per-pod loop that re-validates every connected socket's workspace - * role and evicts sockets whose access has been revoked, closing the read-side - * gap left by the join-only access check. + * permission — for EVERY room type it occupies — and evicts sockets whose access + * no longer satisfies the room, closing the gap left by the join-only access + * check. Without it, a member removed or downgraded mid-session keeps live + * collaborative access (including durable document writes) for the whole lifetime + * of an already-open socket. * - * Blip-safety: eviction fires *only* when {@link resolveCurrentWorkflowRole} - * returns `null`, which happens solely for a successful DB "no access" result or - * a previously-recorded revocation reused across a failure. A transient DB error - * against a still-authorized (or freshly-joined) user resolves to the last-known - * or fallback role, so a database blip never evicts anyone. + * Blip-safety: eviction fires *only* when {@link resolveCurrentRoomPermission} + * resolves to a permission that does not satisfy the room's membership level, + * which happens solely for a successful DB result (no access, or a level below + * the room's requirement) or a previously-recorded revocation reused across a + * failure. A transient DB error against a still-authorized (or freshly-joined) + * user resolves to the last-known or the room's own fallback level, so a database + * blip never evicts anyone. * * Liveness: the loop runs as two independently-guarded lanes. The security scan * (local sockets + DB role checks + emit/leave) touches no Redis at all; the @@ -118,7 +154,7 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR let scanRunning = false let cleanupRunning = false /** - * Round-robin cursor: the `${socketId}:${workflowId}` key of the last target + * Round-robin cursor: the `${socketId}:${roomName}` key of the last target * the previous pass processed. Each pass resumes after it, so a fixed prefix * of hanging authorization checks can never starve the sockets behind it — * every target is examined within a bounded number of passes. @@ -126,16 +162,17 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR let scanCursorKey: string | null = null /** - * Room-state cleanups owed for evicted sockets, keyed - * `${socketId}:${workflowId}`. Every eviction enqueues here (the evicted - * socket has already left the Socket.IO room, so membership scans will never - * see it again); the cleanup lane drains the queue until each removal is - * confirmed, so remaining collaborators do not keep a stale presence entry. + * Presence cleanups owed for evicted sockets, keyed `${socketId}:${roomName}`. + * Every eviction from a presence-backed room enqueues here (the evicted socket + * has already left the Socket.IO room, so membership scans will never see it + * again); the cleanup lane drains the queue until each removal is confirmed, so + * remaining collaborators do not keep a stale presence entry. */ - const pendingCleanups = new Map() + const pendingCleanups = new Map() - async function cleanupEvictedSocket(socketId: string, workflowId: string): Promise { - const key = `${socketId}:${workflowId}` + async function cleanupEvictedSocket(socketId: string, room: RoomRef): Promise { + const name = roomName(room) + const key = `${socketId}:${name}` try { // A fully-disconnected socket already had its presence removed by the // disconnect handler (removeSocketFromAllRooms), so there is nothing left to @@ -148,13 +185,15 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR } // Unlike removeUserFromRoom, this read does not swallow transport errors, - // so a Redis outage lands in the catch below and defers the cleanup. - const currentRoom = await roomManager.getRoomForSocket(socketId, ROOM_TYPES.WORKFLOW) - const currentWorkflowId = currentRoom?.id ?? null - if (currentWorkflowId !== null && currentWorkflowId !== workflowId) { - // The socket has since moved to a different workflow it can still - // access; that join's room switch already removed this room's presence - // entry, so there is nothing stale left to clean here. + // so a Redis outage lands in the catch below and defers the cleanup. The + // lookup is per room TYPE (a socket holds at most one room of each), so a + // socket that also sits in an unrelated room type is unaffected. + const currentRoom = await roomManager.getRoomForSocket(socketId, room.type) + const currentRoomId = currentRoom?.id ?? null + if (currentRoomId !== null && currentRoomId !== room.id) { + // The socket has since moved to a different room of this type that it can + // still access; that join's room switch already removed this room's + // presence entry, so there is nothing stale left to clean here. pendingCleanups.delete(key) return } @@ -162,7 +201,7 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR // Synchronous re-join guard with no awaits before the removal: if the // socket legitimately re-joined this room after the eviction (access // restored), that join re-added its presence — removal would erase it. - if (io.sockets.sockets.get(socketId)?.rooms.has(workflowId)) { + if (io.sockets.sockets.get(socketId)?.rooms.has(name)) { pendingCleanups.delete(key) return } @@ -170,7 +209,7 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR // A null mapping here is the normal case (the socket's mapping key may have // expired) and does NOT mean "skip" — the eviction still removes the presence // entry from the known target room via the explicit ref below. - const removed = await roomManager.removeUserFromRoom(wf(workflowId), socketId) + const removed = await roomManager.removeUserFromRoom(room, socketId) if (!removed) { // `false` conflates two outcomes: the entry was already gone (a no-op), or a // transport error the manager swallowed. Only retry when the socket is still mapped @@ -179,27 +218,27 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR // the presence entry is already gone, so the cleanup is complete: dropping it avoids // re-enqueuing a still-connected socket forever. (A real Redis outage throws at // getRoomForSocket and is deferred by the outer catch, never reaching here.) - if (currentWorkflowId === workflowId) { + if (currentRoomId === room.id) { throw new Error('room-state removal not confirmed') } pendingCleanups.delete(key) return } - await roomManager.broadcastPresenceUpdate(wf(workflowId)) + await roomManager.broadcastPresenceUpdate(room) pendingCleanups.delete(key) } catch (error) { - pendingCleanups.set(key, { socketId, workflowId }) + pendingCleanups.set(key, { socketId, room }) logger.warn( - `Room-state cleanup failed for evicted socket ${socketId} on ${workflowId}; will retry next sweep`, + `Room-state cleanup failed for evicted socket ${socketId} on ${name}; will retry next sweep`, error ) } } async function drainPendingCleanups(): Promise { - for (const [, { socketId, workflowId }] of pendingCleanups) { - await cleanupEvictedSocket(socketId, workflowId) + for (const [, { socketId, room }] of pendingCleanups) { + await cleanupEvictedSocket(socketId, room) } } @@ -220,23 +259,39 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR }) } - function revokeSocket(socket: AuthenticatedSocket, workflowId: string): void { + // Handler-initiated evictions (the per-frame gates) hand failed presence cleanups + // here: they have already left the Socket.IO room, so the scan can no longer + // rediscover them, and this lane is the only thing that retries. + setEvictionCleanupSink((socketId, room) => { + pendingCleanups.set(`${socketId}:${roomName(room)}`, { socketId, room }) + launchCleanups() + }) + + function revokeSocket(socket: AuthenticatedSocket, room: RoomRef, name: string): void { // Security-critical, pod-local, and synchronous: stop this socket receiving - // room broadcasts immediately. Room-state cleanup is only enqueued here — - // the cleanup lane performs the Redis work, so eviction never blocks on it. - const payload: AccessRevokedBroadcast = { - workflowId, - message: 'Your access to this workflow has been revoked', - timestamp: Date.now(), + // room broadcasts immediately, and drop the handler-local state that would + // otherwise still accept its frames (a file-doc socket's room binding is what + // gates its document writes). Redis presence cleanup is only ENQUEUED here — + // the cleanup lane performs that work, so eviction never blocks on it. + if (room.type === ROOM_TYPES.WORKFLOW) { + // Workflow keeps its historical wire event and payload shape, which existing + // clients key off `workflowId`; every other type shares the generic path. + const payload: AccessRevokedBroadcast = { + workflowId: room.id, + message: 'Your access to this workflow has been revoked', + timestamp: Date.now(), + } + socket.emit('access-revoked', payload) + socket.leave(name) + runRoomEvictionHandler(socket.id, room, io) + logger.info(`Revoked live access for user ${socket.userId} on ${name} (socket ${socket.id})`) + } else { + evictSocketFromRoom(socket, room, 'Your access to this resource has been revoked', io) } - socket.emit('access-revoked', payload) - socket.leave(workflowId) - logger.info( - `Revoked live access for user ${socket.userId} on workflow ${workflowId} (socket ${socket.id})` - ) - - pendingCleanups.set(`${socket.id}:${workflowId}`, { socketId: socket.id, workflowId }) + if (PRESENCE_ROOM_TYPES.has(room.type)) { + pendingCleanups.set(`${socket.id}:${name}`, { socketId: socket.id, room }) + } } async function scanMemberships(): Promise { @@ -246,7 +301,7 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR let startIndex = 0 if (scanCursorKey !== null) { const cursorIndex = targets.findIndex( - ({ socket, workflowId }) => `${socket.id}:${workflowId}` === scanCursorKey + ({ socket, name }) => `${socket.id}:${name}` === scanCursorKey ) if (cursorIndex !== -1) { startIndex = (cursorIndex + 1) % targets.length @@ -256,7 +311,7 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR const deadline = Date.now() + SCAN_PASS_BUDGET_MS for (let offset = 0; offset < targets.length; offset++) { - const { workflowId, socket, userId } = targets[(startIndex + offset) % targets.length] + const { room, name, socket, userId } = targets[(startIndex + offset) % targets.length] const remainingBudget = deadline - Date.now() if (remainingBudget <= 0) { @@ -272,25 +327,30 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR // resolution keeps running in the background and is re-raced when the // rotation returns to this socket, so it is acted on once it settles. const role = await Promise.race([ - resolveCurrentWorkflowRole(userId, workflowId, FALLBACK_ROLE), + resolveCurrentRoomPermission(userId, room, fallbackRoleFor(room.type)), sleep(Math.min(SCAN_SOCKET_TIMEOUT_MS, remainingBudget)).then(() => SCAN_TIMED_OUT), ]) - if (role === SCAN_TIMED_OUT) { + // {@link SCAN_TIMED_OUT} is the only symbol this race can yield; matching on + // the type narrows it out of the permission comparison below. + if (typeof role === 'symbol') { logger.warn( - `Authorization check timed out for user ${userId} on workflow ${workflowId}; skipping this pass` + `Authorization check timed out for user ${userId} on ${name}; skipping this pass` ) - } else if (role === null) { - revokeSocket(socket, workflowId) + } else if (!satisfiesRoomMembership(role, room.type)) { + // Covers both revocation (`null`) and downgrade below the level this room + // requires — a member dropped to `read` may keep a table room but not the + // collaborative document editor, exactly as at join time. + revokeSocket(socket, room, name) } } catch (error) { - // Never evict on an unexpected error — only a definitive `null` role + // Never evict on an unexpected error — only a definitive resolved permission // evicts, so a failure here leaves the socket's access intact. logger.warn( - `Access re-validation failed for user ${userId} on workflow ${workflowId}; leaving membership intact`, + `Access re-validation failed for user ${userId} on ${name}; leaving membership intact`, error ) } finally { - scanCursorKey = `${socket.id}:${workflowId}` + scanCursorKey = `${socket.id}:${name}` } } } @@ -332,7 +392,10 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR ) return { - stop: () => clearInterval(timer), + stop: () => { + clearInterval(timer) + setEvictionCleanupSink(null) + }, runOnce, } } diff --git a/apps/realtime/src/handlers/file-doc.test.ts b/apps/realtime/src/handlers/file-doc.test.ts index d56952bc619..938092d4484 100644 --- a/apps/realtime/src/handlers/file-doc.test.ts +++ b/apps/realtime/src/handlers/file-doc.test.ts @@ -6,6 +6,8 @@ import { FILE_DOC_MESSAGE_TYPE, FILE_DOC_SEED, } from '@sim/realtime-protocol/file-doc' +import { ROOM_TYPES } from '@sim/realtime-protocol/rooms' +import { sleep } from '@sim/utils/helpers' import * as decoding from 'lib0/decoding' import * as encoding from 'lib0/encoding' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' @@ -38,6 +40,7 @@ import { flushAllFileDocRooms, setupWorkspaceFileDocHandlers, } from '@/handlers/file-doc' +import { beginRoomPermissionRead, commitRoomPermission } from '@/middleware/permissions' type Handler = (payload?: unknown) => Promise | void @@ -244,6 +247,72 @@ describe('setupWorkspaceFileDocHandlers', () => { expect(mockAuthorizeRoom).not.toHaveBeenCalled() }) + it('does not re-enter the room when access was revoked while the join was in flight', async () => { + // The sweep records a revocation before it evicts, so a join whose authorize + // completed just before that must not put the socket back in the document. + const { io } = createIo() + const { socket, handlers } = setup('socket-race', io, { userId: 'user-race' }) + + mockAuthorizeRoom.mockImplementation(async () => { + // Simulate the revocation landing between this join's authorize and its commit, + // exactly as the sweep would record it. + commitRoomPermission( + 'user-race', + { type: ROOM_TYPES.WORKSPACE_FILE_DOC, id: 'file-1' }, + null, + beginRoomPermissionRead() + ) + return { allowed: true, status: 200, workspaceId: 'ws-1', workspacePermission: 'write' } + }) + + await handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-1', clientId: 1 }) + await flushMicrotasks() + + expect(socket.emit).toHaveBeenCalledWith( + FILE_DOC_EVENTS.JOIN_ERROR, + expect.objectContaining({ code: 'ACCESS_DENIED', retryable: false }) + ) + expect(socket.join).not.toHaveBeenCalled() + expect(joinSuccessFileId(socket)).toBeUndefined() + }) + + it('re-reads access when the cached decision expired mid-join, instead of failing open', async () => { + // A join stalled longer than the cache TTL: the sweep's denial is recorded with a + // later read ticket (so this join's own allow is correctly dropped) but has since + // expired. Peeking the cache would read that as "unknown" and let the socket back + // into the document, so the join must re-resolve against the database. + vi.useFakeTimers() + try { + const room = { type: ROOM_TYPES.WORKSPACE_FILE_DOC, id: 'file-stale' } + const { io } = createIo() + const { socket, handlers } = setup('socket-stale', io, { userId: 'user-stale' }) + + mockAuthorizeRoom.mockImplementation(async ({ action }: { action: string }) => { + // The authoritative current answer: access is gone. + if (action !== 'write') + return { allowed: false, status: 403, workspaceId: 'ws-1', workspacePermission: null } + // This join's own authorize saw the pre-revocation state, and the sweep records + // the revocation (later read ticket) while it is still in flight. + commitRoomPermission('user-stale', room, null, beginRoomPermissionRead()) + await sleep(31_000) + return { allowed: true, status: 200, workspaceId: 'ws-1', workspacePermission: 'write' } + }) + + const joining = handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-stale', clientId: 1 }) + await vi.advanceTimersByTimeAsync(31_000) + await joining + + expect(socket.emit).toHaveBeenCalledWith( + FILE_DOC_EVENTS.JOIN_ERROR, + expect.objectContaining({ code: 'ACCESS_DENIED', retryable: false }) + ) + expect(socket.join).not.toHaveBeenCalled() + expect(joinSuccessFileId(socket)).toBeUndefined() + } finally { + vi.useRealTimers() + } + }) + it('requires write permission and reports 404 as NOT_FOUND', async () => { mockAuthorizeRoom.mockResolvedValue({ allowed: false, status: 404, workspacePermission: null }) const { io } = createIo() @@ -299,6 +368,87 @@ describe('setupWorkspaceFileDocHandlers', () => { expect(mockFetchFileDocPersist).toHaveBeenCalled() }) + it('drops document frames and evicts once the editor loses write access mid-session', async () => { + // The join-time check is not a standing right: a collaborator downgraded to `read` + // (or removed) must stop landing durable edits on the socket they already hold. + // A distinct user/file so the recorded revocation — written under fake timers, so it + // outlives this test in real time — cannot leak into siblings through the + // module-global role cache. + vi.useFakeTimers() + try { + mockFetchFileDocSeed.mockResolvedValue(seedResult('# From server')) + const { io, sent } = createIo() + const { socket, handlers } = setup('socket-revoked', io, { userId: 'user-revoked' }) + await handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-revoked', clientId: 1 }) + await vi.advanceTimersByTimeAsync(0) + + // Access is downgraded to read-only, and the cached join-time decision expires. + mockAuthorizeRoom.mockResolvedValue({ + allowed: false, + status: 403, + workspaceId: 'ws-1', + workspacePermission: 'read', + }) + await vi.advanceTimersByTimeAsync(31_000) + + const edit = new Y.Doc() + edit.getText(FILE_DOC_FIELD).insert(0, 'edit after revocation') + const editFrame = () => + handlers[FILE_DOC_EVENTS.MESSAGE]( + frame(FILE_DOC_MESSAGE_TYPE.SYNC, (e) => + syncProtocol.writeUpdate(e, Y.encodeStateAsUpdate(edit)) + ) + ) + + // The first frame after expiry finds nothing cached, so it is accepted and kicks + // off the authoritative re-read (never a synchronous DB wait on the relay path). + editFrame() + await vi.advanceTimersByTimeAsync(0) + + // The next frame is gated on the now-authoritative denial: dropped, and the socket + // is evicted rather than left holding the room. + editFrame() + await vi.advanceTimersByTimeAsync(0) + + expect(socket.emit).toHaveBeenCalledWith( + 'room-access-revoked', + expect.objectContaining({ room: { type: 'workspace-file-doc', id: 'file-revoked' } }) + ) + expect(socket.leave).toHaveBeenCalledWith('workspace-file-doc:file-revoked') + + // The binding is gone, so every later frame is inert — nothing is applied and + // nothing reaches the room. + const sentAfterEviction = sent.length + const emitsAfterEviction = socket.emit.mock.calls.length + editFrame() + await vi.advanceTimersByTimeAsync(0) + expect(sent.length).toBe(sentAfterEviction) + expect(socket.emit.mock.calls.length).toBe(emitsAfterEviction) + } finally { + vi.useRealTimers() + } + }) + + it('keeps relaying document frames while the cached permission still allows writing', async () => { + const { io, sent } = createIo() + const { socket, handlers } = setup('socket-1', io) + await handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-1', clientId: 1 }) + await flushMicrotasks() + + const before = sent.length + const edit = new Y.Doc() + edit.getText(FILE_DOC_FIELD).insert(0, 'still allowed') + handlers[FILE_DOC_EVENTS.MESSAGE]( + frame(FILE_DOC_MESSAGE_TYPE.SYNC, (e) => + syncProtocol.writeUpdate(e, Y.encodeStateAsUpdate(edit)) + ) + ) + await flushMicrotasks() + + expect(sent.slice(before).length).toBeGreaterThan(0) + expect(socket.emit).not.toHaveBeenCalledWith('room-access-revoked', expect.anything()) + }) + it('applies + fans out an agent-streamed frame (SYNC_NO_PERSIST) but never persists it', async () => { mockFetchFileDocSeed.mockResolvedValue(seedResult('# From server')) const { io, sent } = createIo() diff --git a/apps/realtime/src/handlers/file-doc.ts b/apps/realtime/src/handlers/file-doc.ts index a3d761bb40f..a0152fd85f6 100644 --- a/apps/realtime/src/handlers/file-doc.ts +++ b/apps/realtime/src/handlers/file-doc.ts @@ -24,6 +24,7 @@ * @module */ import { createLogger } from '@sim/logger' +import { ROOM_MEMBERSHIP_ACTIONS, satisfiesRoomMembership } from '@sim/platform-authz/room-policy' import { FILE_DOC_EVENTS, FILE_DOC_MESSAGE_TYPE, @@ -51,8 +52,10 @@ import { REDIS_ORIGIN, REDIS_SNAPSHOT_ORIGIN, } from '@/handlers/file-doc-store' +import { evictSocketFromRoom, registerRoomEvictionHandler } from '@/handlers/room-eviction' import { resolveRoomJoinAuth } from '@/handlers/room-join-auth' import type { AuthenticatedSocket } from '@/middleware/auth' +import { peekRoomPermission, resolveCurrentRoomPermission } from '@/middleware/permissions' import type { IRoomManager } from '@/rooms' const logger = createLogger('FileDocHandlers') @@ -837,11 +840,73 @@ function emitJoinError( }) } -function handleMessage(socket: AuthenticatedSocket, data: unknown) { +/** + * The permission occupying a file-doc room requires — `write`, since the room IS + * the collaborative editor. Sourced from the shared map so the join check, the + * per-frame gate below, and the re-validation sweep can never drift apart. + */ +const FILE_DOC_ACTION = ROOM_MEMBERSHIP_ACTIONS[ROOM_TYPES.WORKSPACE_FILE_DOC] + +/** + * Per-frame authorization for a socket's inbound document/awareness frames. + * + * Room membership alone is NOT a standing right to write: a collaborator removed + * from the workspace — or downgraded to `read` — must stop landing durable edits on + * an already-open socket, without waiting for the next re-validation sweep. This is + * the synchronous half of that enforcement; the sweep is the asynchronous half. + * + * Reads the shared role cache without awaiting, because this sits on the Yjs relay + * hot path (one call per keystroke-sized frame). Three outcomes: + * - fresh cached permission that satisfies `write` → accept. + * - fresh cached permission that does NOT → drop the frame and evict immediately. + * - nothing fresh cached (TTL lapsed) → accept this frame and kick off a background + * refresh, so the very next frames are gated on an authoritative read. Accepting + * is correct rather than lax: the entry was authoritative when written and every + * join records one, so the exposure is bounded by the same TTL the workflow + * write-path has always had, and the sweep evicts independently. + */ +function isFileDocWriteAllowed(socket: AuthenticatedSocket, io: Server, name: string): boolean { + const userId = socket.userId + const fileId = fileDocRooms.get(name)?.fileId + if (!userId || !fileId) return false + + const room = fileDocRoom(fileId) + const cached = peekRoomPermission(userId, room) + if (cached === undefined) { + // Single-flighted, so a burst of frames triggers at most one query. + void resolveCurrentRoomPermission(userId, room, FILE_DOC_ACTION).catch(() => {}) + return true + } + if (satisfiesRoomMembership(cached, ROOM_TYPES.WORKSPACE_FILE_DOC)) return true + + logger.warn( + `Dropping file-doc frame from user ${userId} whose access to file ${fileId} no longer permits writing` + ) + // Evicting (not just dropping the frame) is what makes this stick: the registered + // eviction handler clears `socketToRoomName`, and every inbound frame is gated on + // that binding — so the socket cannot apply another document update even if it + // keeps sending them. + evictSocketFromRoom(socket, room, 'Your access to this document has been revoked', io) + return false +} + +/** + * Reconciles this handler's pod-local state when the access re-validation sweep + * evicts a socket from a file-doc room (the sweep has already emitted the + * revocation and left the Socket.IO room). Scoped to the evicted room so a socket + * that has since switched documents keeps the one it legitimately holds. + */ +registerRoomEvictionHandler(ROOM_TYPES.WORKSPACE_FILE_DOC, (socketId, room, io) => { + if (socketToRoomName.get(socketId) !== roomName(room)) return + cleanupFileDocForSocket(socketId, io) +}) + +function handleMessage(socket: AuthenticatedSocket, io: Server, data: unknown) { const name = socketToRoomName.get(socket.id) if (!name) return const room = fileDocRooms.get(name) if (!room) return + if (!isFileDocWriteAllowed(socket, io, name)) return const bytes = toFileDocBytes(data) if (!bytes) return @@ -1009,7 +1074,7 @@ export function setupWorkspaceFileDocHandlers( const authorized = await resolveRoomJoinAuth({ userId, room, - action: 'write', + action: FILE_DOC_ACTION, logger, logLabel: `file-doc room for ${userId}`, messages: { @@ -1026,9 +1091,25 @@ export function setupWorkspaceFileDocHandlers( // awareness). Resolved here so the generation guard below also covers this await. const avatarUrl = await resolveAvatarUrl(socket, userId) + // Re-check access immediately before registering, mirroring the workflow join: the + // access re-validation sweep records a revocation BEFORE it evicts, so a join that + // authorized just before the revocation must not complete afterwards and re-bind + // the socket to the document. This RE-RESOLVES rather than peeking the cache — a + // peek treats an expired entry as unknown and fails open, which a join stalled + // longer than the cache TTL would slip straight through. Normally a cache hit (this + // join's own authorize just warmed it), so it costs no extra query. + const currentPermission = await resolveCurrentRoomPermission(userId, room, FILE_DOC_ACTION) + if (!satisfiesRoomMembership(currentPermission, ROOM_TYPES.WORKSPACE_FILE_DOC)) { + logger.warn(`User ${userId} lost write access to file ${fileId} before the join completed`) + emitJoinError(socket, fileId, 'Access denied to file', 'ACCESS_DENIED', false) + return + } + // Abort a JOIN superseded during authorization/identity resolution: the socket // disconnected, or a newer JOIN (a document switch) bumped the generation. Registering // here would leak a dead socket's room or bind the socket to the wrong document. + // Last await before the commit, so nothing can interleave between the access + // re-check above and the registration below. if (socket.disconnected || joinGeneration.get(socket.id) !== generation) return const entry = getOrCreateRoom(io, room) @@ -1148,7 +1229,7 @@ export function setupWorkspaceFileDocHandlers( } }) - socket.on(FILE_DOC_EVENTS.MESSAGE, (data: unknown) => handleMessage(socket, data)) + socket.on(FILE_DOC_EVENTS.MESSAGE, (data: unknown) => handleMessage(socket, io, data)) socket.on(FILE_DOC_EVENTS.LEAVE, (payload?: LeaveFileDocPayload) => { try { diff --git a/apps/realtime/src/handlers/operations.test.ts b/apps/realtime/src/handlers/operations.test.ts new file mode 100644 index 00000000000..a386a81b677 --- /dev/null +++ b/apps/realtime/src/handlers/operations.test.ts @@ -0,0 +1,181 @@ +/** + * @vitest-environment node + * + * End-to-end guard for the socket operation ACL: the security boundary is not the + * role table on its own but whether a role reaches `persistWorkflowOperation`. + * These tests drive the real handler with the real permission middleware (only the + * database and the workspace authorizer are mocked) and assert on the persist call, + * because that is what durably rewrites `workflow_blocks`. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { IRoomManager } from '@/rooms' + +const { mockAuthorizeWorkflow, mockPersist, mockAssertMutable } = vi.hoisted(() => ({ + mockAuthorizeWorkflow: vi.fn(), + mockPersist: vi.fn(), + mockAssertMutable: vi.fn(), +})) + +vi.mock('@sim/platform-authz/workflow', () => ({ + authorizeWorkflowByWorkspacePermission: mockAuthorizeWorkflow, + assertWorkflowMutable: mockAssertMutable, + WorkflowLockedError: class WorkflowLockedError extends Error {}, +})) + +vi.mock('@/database/operations', () => ({ + persistWorkflowOperation: mockPersist, +})) + +import { setupOperationsHandlers } from '@/handlers/operations' + +const WORKFLOW_ID = 'wf-acl' +const BLOCK_ID = 'block-1' + +type Handler = (payload: unknown) => Promise | void + +function createSocket(id: string) { + const handlers: Record = {} + const toEmit = vi.fn() + const socket = { + id, + userId: `user-${id}`, + userName: 'Test User', + on: vi.fn((event: string, handler: Handler) => { + handlers[event] = handler + }), + emit: vi.fn(), + to: vi.fn().mockReturnValue({ emit: toEmit }), + } + return { socket, handlers, toEmit } +} + +function createRoomManager(socketId: string, role: string): IRoomManager { + return { + isReady: () => true, + getRoomForSocket: vi.fn().mockResolvedValue({ type: 'workflow', id: WORKFLOW_ID }), + getUserSession: vi + .fn() + .mockResolvedValue({ userId: `user-${socketId}`, userName: 'Test User' }), + hasRoom: vi.fn().mockResolvedValue(true), + getRoomUsers: vi.fn().mockResolvedValue([{ socketId, userId: `user-${socketId}`, role }]), + updateUserActivity: vi.fn().mockResolvedValue(undefined), + updateRoomLastModified: vi.fn().mockResolvedValue(undefined), + } as unknown as IRoomManager +} + +/** A committed single-block move — the form that writes `workflow_blocks`. */ +function committedPositionUpdate() { + return { + operationId: 'op-1', + operation: 'update-position', + target: 'block', + timestamp: Date.now(), + payload: { id: BLOCK_ID, position: { x: 999_999, y: 999_999 }, commit: true }, + } +} + +/** The batch form, which persists with no `commit` flag at all. */ +function batchPositionUpdate() { + return { + operationId: 'op-2', + operation: 'batch-update-positions', + target: 'blocks', + timestamp: Date.now(), + payload: { updates: [{ id: BLOCK_ID, position: { x: 1, y: 2 } }] }, + } +} + +function setup(id: string, role: string) { + const { socket, handlers, toEmit } = createSocket(id) + setupOperationsHandlers( + socket as unknown as Parameters[0], + createRoomManager(id, role) + ) + return { socket, handlers, toEmit } +} + +describe('workflow operation ACL', () => { + beforeEach(() => { + vi.clearAllMocks() + mockAssertMutable.mockResolvedValue(undefined) + mockPersist.mockResolvedValue(undefined) + }) + + describe('read-only member', () => { + beforeEach(() => { + mockAuthorizeWorkflow.mockResolvedValue({ allowed: true, workspacePermission: 'read' }) + }) + + it('cannot persist a committed block position', async () => { + // Unique socket id per test: the permission cache is module-global and keyed + // by (user, workflow), so sharing a user across roles would hit a warm entry. + const { socket, handlers } = setup('sock-read-1', 'read') + + await handlers['workflow-operation'](committedPositionUpdate()) + + expect(mockPersist).not.toHaveBeenCalled() + expect(socket.emit).toHaveBeenCalledWith( + 'operation-forbidden', + expect.objectContaining({ type: 'INSUFFICIENT_PERMISSIONS' }) + ) + }) + + it('cannot persist a batch position update', async () => { + const { socket, handlers } = setup('sock-read-2', 'read') + + await handlers['workflow-operation'](batchPositionUpdate()) + + expect(mockPersist).not.toHaveBeenCalled() + expect(socket.emit).toHaveBeenCalledWith( + 'operation-forbidden', + expect.objectContaining({ type: 'INSUFFICIENT_PERMISSIONS' }) + ) + }) + + it('still relays an UNCOMMITTED position update without persisting it', async () => { + // The smooth-drag broadcast is deliberately ungated (it never persists), and + // tightening the ACL must not turn it into an error. + const { socket, handlers, toEmit } = setup('sock-read-3', 'read') + + await handlers['workflow-operation']({ + ...committedPositionUpdate(), + payload: { id: BLOCK_ID, position: { x: 5, y: 6 }, commit: false }, + }) + + expect(mockPersist).not.toHaveBeenCalled() + expect(toEmit).toHaveBeenCalledWith('workflow-operation', expect.anything()) + expect(socket.emit).not.toHaveBeenCalledWith( + 'operation-forbidden', + expect.objectContaining({ type: 'INSUFFICIENT_PERMISSIONS' }) + ) + }) + }) + + describe('write member (positive control)', () => { + beforeEach(() => { + mockAuthorizeWorkflow.mockResolvedValue({ allowed: true, workspacePermission: 'write' }) + }) + + it('persists a committed block position', async () => { + const { handlers } = setup('sock-write-1', 'write') + + await handlers['workflow-operation'](committedPositionUpdate()) + + expect(mockPersist).toHaveBeenCalledWith( + WORKFLOW_ID, + expect.objectContaining({ operation: 'update-position' }) + ) + }) + + it('persists a batch position update', async () => { + const { handlers } = setup('sock-write-2', 'write') + + await handlers['workflow-operation'](batchPositionUpdate()) + + expect(mockPersist).toHaveBeenCalledWith( + WORKFLOW_ID, + expect.objectContaining({ operation: 'batch-update-positions' }) + ) + }) + }) +}) diff --git a/apps/realtime/src/handlers/room-eviction.ts b/apps/realtime/src/handlers/room-eviction.ts new file mode 100644 index 00000000000..2a417489b64 --- /dev/null +++ b/apps/realtime/src/handlers/room-eviction.ts @@ -0,0 +1,107 @@ +import { createLogger } from '@sim/logger' +import { + ROOM_ACCESS_REVOKED_EVENT, + type RoomAccessRevokedBroadcast, +} from '@sim/realtime-protocol/events' +import { type RoomRef, type RoomType, roomName } from '@sim/realtime-protocol/rooms' +import type { Server } from 'socket.io' +import type { AuthenticatedSocket } from '@/middleware/auth' + +const logger = createLogger('RoomEviction') + +/** + * Drops whatever pod-local state a handler keeps for a socket in one of its rooms. + * Called after the socket has already been removed from the Socket.IO room, so it + * only has to reconcile the handler's own bookkeeping. + */ +export type RoomEvictionHandler = (socketId: string, room: RoomRef, io: Server) => void + +const handlers = new Map() + +/** + * Registers a room type's local-state cleanup for involuntary eviction (the access + * re-validation sweep, or a per-frame gate that catches a revocation first). + * + * A registry rather than a direct import so the security-critical sweep stays + * domain-neutral: it never has to know that a file-doc room keeps an in-memory + * Y.Doc + ownership map while a table room keeps Redis presence. Handlers register + * at module load, which every handler module does at bootstrap. + */ +export function registerRoomEvictionHandler(type: RoomType, handler: RoomEvictionHandler): void { + handlers.set(type, handler) +} + +/** + * Runs the registered cleanup for an evicted socket, if the room type has one. + * Never throws — an eviction has already taken effect (the socket left the room) + * by the time this runs, so a failing cleanup must not unwind it. + */ +export function runRoomEvictionHandler(socketId: string, room: RoomRef, io: Server): void { + const handler = handlers.get(room.type) + if (!handler) return + try { + handler(socketId, room, io) + } catch (error) { + logger.warn(`Room eviction cleanup failed for socket ${socketId} on ${room.type}`, error) + } +} + +/** Enqueues a presence cleanup owed for an evicted socket. */ +type EvictionCleanupSink = (socketId: string, room: RoomRef) => void + +let evictionCleanupSink: EvictionCleanupSink | null = null + +/** + * Registers the retrying presence-cleanup lane (owned by the access-revalidation + * sweep) that handler-initiated evictions hand failed cleanups to. + * + * An eviction removes the socket from the Socket.IO room synchronously, which is + * also how the sweep discovers work — so once a handler evicts, the sweep can no + * longer find that (socket, room) pair to retry a Redis removal that failed. This + * sink is the handoff that keeps the sweep's retry semantics (re-join guard, + * moved-room guard, no infinite retry) as the single implementation instead of a + * second, subtly different retry loop per handler. Unset outside a running sweep, + * where {@link requestEvictionCleanup} is a no-op. + */ +export function setEvictionCleanupSink(sink: EvictionCleanupSink | null): void { + evictionCleanupSink = sink +} + +/** + * Asks the sweep's cleanup lane to (re)try presence removal for a socket already + * evicted from `room`. Call only when the immediate best-effort removal failed — + * the happy path stays immediate so peers see the departure without waiting for a + * sweep tick. + */ +export function requestEvictionCleanup(socketId: string, room: RoomRef): void { + evictionCleanupSink?.(socketId, room) +} + +/** + * Evicts one socket from one non-workflow room after a confirmed loss of access: + * tell the client, stop it receiving room broadcasts, and drop the handler-local + * state that would otherwise still accept its frames. + * + * The single eviction path shared by both enforcement points — the periodic + * re-validation sweep and the per-frame gates that catch a revocation first — so + * they cannot diverge on what "evicted" means. Everything here is synchronous and + * pod-local; any Redis presence removal is the caller's own follow-up (the sweep + * owns a retrying cleanup lane for it). + * + * Workflow rooms keep their own `access-revoked` wire event for client + * compatibility and are deliberately not routed through here. + */ +export function evictSocketFromRoom( + socket: AuthenticatedSocket, + room: RoomRef, + message: string, + io: Server +): void { + const payload: RoomAccessRevokedBroadcast = { room, message, timestamp: Date.now() } + socket.emit(ROOM_ACCESS_REVOKED_EVENT, payload) + socket.leave(roomName(room)) + runRoomEvictionHandler(socket.id, room, io) + logger.info( + `Revoked live access for user ${socket.userId} on ${roomName(room)} (socket ${socket.id})` + ) +} diff --git a/apps/realtime/src/handlers/room-join-auth.ts b/apps/realtime/src/handlers/room-join-auth.ts index a7c2a9aa24c..f54d6a81782 100644 --- a/apps/realtime/src/handlers/room-join-auth.ts +++ b/apps/realtime/src/handlers/room-join-auth.ts @@ -1,6 +1,7 @@ import type { createLogger } from '@sim/logger' import { authorizeRoom } from '@sim/platform-authz/rooms' import type { RoomRef } from '@sim/realtime-protocol/rooms' +import { beginRoomPermissionRead, commitRoomPermission } from '@/middleware/permissions' type Authorized = Awaited> @@ -34,6 +35,10 @@ export async function resolveRoomJoinAuth( const { userId, room, action, logger, logLabel, messages, emitError } = params let authorized: Authorized + // Taken before the query so this read is ordered against every other one: a + // decision from a later-started read (the access-revalidation sweep's denial) + // is never overwritten by this older result — see {@link commitRoomPermission}. + const readSeq = beginRoomPermissionRead() try { authorized = await authorizeRoom({ userId, room, action }) } catch (error) { @@ -42,6 +47,16 @@ export async function resolveRoomJoinAuth( return null } + // Feed the fresh authoritative read into the shared role cache that the access + // re-validation sweep and the per-frame write gates consult, so both start warm on + // the room this socket just joined — and so a re-granted user's join immediately + // supersedes a cached revocation instead of waiting out its TTL. A 400 (room type + // not authorizable here) resolved no permission at all and is deliberately not + // recorded. A 404 records `null`: the resource is genuinely gone. + if (authorized.status !== 400) { + commitRoomPermission(userId, room, authorized.workspacePermission, readSeq) + } + if (!authorized.allowed) { emitError({ error: authorized.status === 404 ? messages.notFound : messages.accessDenied, diff --git a/apps/realtime/src/handlers/tables.test.ts b/apps/realtime/src/handlers/tables.test.ts index 518c5ee376a..74310477d93 100644 --- a/apps/realtime/src/handlers/tables.test.ts +++ b/apps/realtime/src/handlers/tables.test.ts @@ -3,6 +3,7 @@ */ import { ROOM_TYPES } from '@sim/realtime-protocol/rooms' import { TABLE_PRESENCE_EVENTS } from '@sim/realtime-protocol/table-presence' +import { sleep } from '@sim/utils/helpers' import { beforeEach, describe, expect, it, vi } from 'vitest' import type { IRoomManager } from '@/rooms' @@ -19,7 +20,9 @@ vi.mock('@sim/platform-authz/rooms', () => ({ authorizeRoom: mockAuthorizeRoom, })) +import { setEvictionCleanupSink } from '@/handlers/room-eviction' import { setupTablesHandlers } from '@/handlers/tables' +import { beginRoomPermissionRead, commitRoomPermission } from '@/middleware/permissions' const TABLE_ROOM = { type: ROOM_TYPES.TABLE, id: 'table-1' } @@ -174,6 +177,202 @@ describe('setupTablesHandlers', () => { }) }) + it('does not join when access was revoked while the join was in flight', async () => { + // The sweep records a revocation before it evicts, so a join whose authorize + // completed just before that must not put the socket back in the room. + const { socket, handlers } = createSocket({ id: 'socket-race', userId: 'user-race' }) + const roomManager = createRoomManager() + setupTablesHandlers(socket as unknown as SetupArg, roomManager) + + mockAuthorizeRoom.mockImplementation(async () => { + commitRoomPermission( + 'user-race', + { type: ROOM_TYPES.TABLE, id: 'table-race' }, + null, + beginRoomPermissionRead() + ) + return { allowed: true, status: 200, workspaceId: 'ws-1', workspacePermission: 'admin' } + }) + + await handlers[TABLE_PRESENCE_EVENTS.JOIN]({ tableId: 'table-race' }) + + expect(socket.emit).toHaveBeenCalledWith( + TABLE_PRESENCE_EVENTS.JOIN_ERROR, + expect.objectContaining({ code: 'ACCESS_DENIED', retryable: false }) + ) + expect(socket.join).not.toHaveBeenCalled() + expect(roomManager.addUserToRoom).not.toHaveBeenCalled() + }) + + it('drops a cell selection and evicts once the viewer loses access mid-session', async () => { + // Distinct user/table so the recorded revocation cannot leak into sibling tests + // through the module-global role cache. + vi.useFakeTimers() + try { + const room = { type: ROOM_TYPES.TABLE, id: 'table-revoked' } + const { socket, handlers, toEmit } = createSocket({ id: 'socket-9', userId: 'user-9' }) + const roomManager = createRoomManager({ + getRoomForSocket: vi.fn().mockResolvedValue(room), + }) + setupTablesHandlers(socket as unknown as SetupArg, roomManager) + + await handlers[TABLE_PRESENCE_EVENTS.JOIN]({ tableId: 'table-revoked' }) + await vi.advanceTimersByTimeAsync(0) + + // Access removed, and the join-time decision expires. + mockAuthorizeRoom.mockResolvedValue({ + allowed: false, + status: 403, + workspaceId: 'ws-1', + workspacePermission: null, + }) + await vi.advanceTimersByTimeAsync(31_000) + + const cell = { + anchor: { rowId: 'row-1', columnId: 'col-a' }, + focus: { rowId: 'row-1', columnId: 'col-a' }, + } + // First selection after expiry finds nothing cached: accepted, and it kicks off the + // authoritative re-read rather than blocking the relay on a DB round-trip. + await handlers[TABLE_PRESENCE_EVENTS.CELL_SELECTION]({ cell }) + await vi.advanceTimersByTimeAsync(0) + + toEmit.mockClear() + await handlers[TABLE_PRESENCE_EVENTS.CELL_SELECTION]({ cell }) + await vi.advanceTimersByTimeAsync(0) + + expect(toEmit).not.toHaveBeenCalled() + expect(socket.emit).toHaveBeenCalledWith( + 'room-access-revoked', + expect.objectContaining({ room }) + ) + expect(socket.leave).toHaveBeenCalledWith('table:table-revoked') + expect(roomManager.removeUserFromRoom).toHaveBeenCalledWith(room, 'socket-9') + } finally { + vi.useRealTimers() + } + }) + + it('hands a failed presence removal to the sweep so the eviction stays retryable', async () => { + // The eviction already left the Socket.IO room, so the sweep's scan can no longer + // rediscover this socket — a failed removal here would strand a ghost collaborator + // until disconnect unless it is handed to the retrying cleanup lane. + vi.useFakeTimers() + const owed: Array<{ socketId: string; roomId: string }> = [] + setEvictionCleanupSink((socketId, room) => owed.push({ socketId, roomId: room.id })) + try { + const room = { type: ROOM_TYPES.TABLE, id: 'table-defer' } + const { socket, handlers } = createSocket({ id: 'socket-defer', userId: 'user-defer' }) + const roomManager = createRoomManager({ + getRoomForSocket: vi.fn().mockResolvedValue(room), + removeUserFromRoom: vi.fn().mockRejectedValue(new Error('redis down')), + }) + setupTablesHandlers(socket as unknown as SetupArg, roomManager) + + await handlers[TABLE_PRESENCE_EVENTS.JOIN]({ tableId: 'table-defer' }) + await vi.advanceTimersByTimeAsync(0) + + mockAuthorizeRoom.mockResolvedValue({ + allowed: false, + status: 403, + workspaceId: 'ws-1', + workspacePermission: null, + }) + await vi.advanceTimersByTimeAsync(31_000) + + const cell = { + anchor: { rowId: 'row-1', columnId: 'col-a' }, + focus: { rowId: 'row-1', columnId: 'col-a' }, + } + await handlers[TABLE_PRESENCE_EVENTS.CELL_SELECTION]({ cell }) + await vi.advanceTimersByTimeAsync(0) + await handlers[TABLE_PRESENCE_EVENTS.CELL_SELECTION]({ cell }) + await vi.advanceTimersByTimeAsync(0) + + expect(socket.leave).toHaveBeenCalledWith('table:table-defer') + expect(owed).toEqual([{ socketId: 'socket-defer', roomId: 'table-defer' }]) + } finally { + setEvictionCleanupSink(null) + vi.useRealTimers() + } + }) + + it('keeps the prior table room when a switch is denied at the access re-check', async () => { + // A denied switch must not silently drop the client from a table it may still be + // allowed to occupy, so the prior room is left only once the join is certain. + vi.useFakeTimers() + try { + const prior = { type: ROOM_TYPES.TABLE, id: 'table-prior' } + const { socket, handlers } = createSocket({ id: 'socket-switch', userId: 'user-switch' }) + const roomManager = createRoomManager({ + getRoomForSocket: vi.fn().mockResolvedValue(prior), + }) + setupTablesHandlers(socket as unknown as SetupArg, roomManager) + + let call = 0 + mockAuthorizeRoom.mockImplementation(async () => { + call += 1 + if (call === 1) { + // A later-started read drops this join's own decision, and the join stalls past + // the TTL so that decision is expired by re-check time — forcing the re-resolve + // down its database path below. + commitRoomPermission( + 'user-switch', + { type: ROOM_TYPES.TABLE, id: 'table-target' }, + 'admin', + beginRoomPermissionRead() + ) + await sleep(31_000) + return { allowed: true, status: 200, workspaceId: 'ws-1', workspacePermission: 'admin' } + } + // The authoritative current answer: access is gone. + return { allowed: false, status: 403, workspaceId: 'ws-1', workspacePermission: null } + }) + + const joining = handlers[TABLE_PRESENCE_EVENTS.JOIN]({ tableId: 'table-target' }) + await vi.advanceTimersByTimeAsync(31_000) + await joining + + expect(socket.emit).toHaveBeenCalledWith( + TABLE_PRESENCE_EVENTS.JOIN_ERROR, + expect.objectContaining({ code: 'ACCESS_DENIED', retryable: false }) + ) + // Neither joined the target nor abandoned the prior room. + expect(socket.join).not.toHaveBeenCalled() + expect(socket.leave).not.toHaveBeenCalled() + expect(roomManager.removeUserFromRoom).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) + + it('aborts the join when a revocation lands during the prior-room leave', async () => { + // The prior-room leave is the only await left between the authoritative access + // re-check and the commit, so a sweep revocation recorded in that window must still + // stop the join — `superseded()` alone only watches the join generation. + const prior = { type: ROOM_TYPES.TABLE, id: 'table-prior-2' } + const target = { type: ROOM_TYPES.TABLE, id: 'table-target-2' } + const { socket, handlers } = createSocket({ id: 'socket-window', userId: 'user-window' }) + const roomManager = createRoomManager({ + getRoomForSocket: vi.fn().mockResolvedValue(prior), + removeUserFromRoom: vi.fn().mockImplementation(async () => { + // The sweep records the revocation while the prior-room leave is in flight. + commitRoomPermission('user-window', target, null, beginRoomPermissionRead()) + return true + }), + }) + setupTablesHandlers(socket as unknown as SetupArg, roomManager) + + await handlers[TABLE_PRESENCE_EVENTS.JOIN]({ tableId: 'table-target-2' }) + + expect(socket.emit).toHaveBeenCalledWith( + TABLE_PRESENCE_EVENTS.JOIN_ERROR, + expect.objectContaining({ code: 'ACCESS_DENIED', retryable: false }) + ) + expect(socket.join).not.toHaveBeenCalled() + expect(roomManager.addUserToRoom).not.toHaveBeenCalled() + }) + it('drops a malformed cell selection without storing or relaying it', async () => { const { socket, handlers, toEmit } = createSocket() const roomManager = createRoomManager({ diff --git a/apps/realtime/src/handlers/tables.ts b/apps/realtime/src/handlers/tables.ts index 2474063c9a0..6fad57fb6d6 100644 --- a/apps/realtime/src/handlers/tables.ts +++ b/apps/realtime/src/handlers/tables.ts @@ -1,4 +1,5 @@ import { createLogger } from '@sim/logger' +import { ROOM_MEMBERSHIP_ACTIONS, satisfiesRoomMembership } from '@sim/platform-authz/room-policy' import { ROOM_TYPES, type RoomRef, roomName } from '@sim/realtime-protocol/rooms' import { type JoinTablePayload, @@ -7,8 +8,10 @@ import { type TableCellSelection, } from '@sim/realtime-protocol/table-presence' import { resolveAvatarUrl } from '@/handlers/avatar' +import { evictSocketFromRoom, requestEvictionCleanup } from '@/handlers/room-eviction' import { resolveRoomJoinAuth } from '@/handlers/room-join-auth' import type { AuthenticatedSocket } from '@/middleware/auth' +import { peekRoomPermission, resolveCurrentRoomPermission } from '@/middleware/permissions' import type { IRoomManager, UserPresence } from '@/rooms' import { filterVisiblePresence, sweepStalePresence } from '@/rooms/presence-visibility' @@ -20,6 +23,41 @@ const MAX_CELL_ID_LENGTH = 200 /** The table presence room ref for a table id. */ const tableRoom = (tableId: string): RoomRef => ({ type: ROOM_TYPES.TABLE, id: tableId }) +/** + * The permission occupying a table room requires. Sourced from the shared map so + * the join check, the per-operation gate below, and the access re-validation sweep + * can never drift apart. + */ +const TABLE_ACTION = ROOM_MEMBERSHIP_ACTIONS[ROOM_TYPES.TABLE] + +/** + * Per-operation authorization for an already-joined socket, mirroring the file-doc + * gate: room membership is not a standing right, so a collaborator whose workspace + * access was revoked stops publishing presence into the room without waiting for + * the next re-validation sweep. + * + * Reads the shared role cache without awaiting (`undefined` = nothing fresh cached, + * which means "unknown", not "denied") and kicks off a background refresh in that + * case, so the exposure window is the cache TTL rather than the socket's lifetime. + * On a confirmed loss of access it evicts, which also clears the room mapping this + * handler resolves every operation through. + */ +function isTableAccessAllowed( + socket: AuthenticatedSocket, + room: RoomRef +): { allowed: boolean; revoked: boolean } { + const userId = socket.userId + if (!userId) return { allowed: false, revoked: false } + + const cached = peekRoomPermission(userId, room) + if (cached === undefined) { + void resolveCurrentRoomPermission(userId, room, TABLE_ACTION).catch(() => {}) + return { allowed: true, revoked: false } + } + if (satisfiesRoomMembership(cached, ROOM_TYPES.TABLE)) return { allowed: true, revoked: false } + return { allowed: false, revoked: true } +} + function isCellRef(value: unknown): value is TableCellRef { if (typeof value !== 'object' || value === null) return false const ref = value as { rowId?: unknown; columnId?: unknown } @@ -50,6 +88,42 @@ function normalizeCellSelection(cell: unknown): TableCellSelection | undefined { } } +/** + * Evicts a socket from a table room after a confirmed loss of access, then drops + * its presence so peers stop seeing its selection. + * + * The eviction leaves the Socket.IO room immediately, which is also how the sweep + * discovers work — so a presence removal that fails here would be unretryable and + * strand a ghost collaborator until disconnect. The removal is therefore attempted + * inline (peers see the departure at once) and handed to the sweep's retrying + * cleanup lane if it fails or reports nothing removed, which is the same signal + * the sweep treats as a deferrable failure. + */ +async function evictFromTable( + socket: AuthenticatedSocket, + roomManager: IRoomManager, + room: RoomRef +): Promise { + evictSocketFromRoom(socket, room, 'Your access to this table has been revoked', roomManager.io) + try { + // `false` conflates "already gone" with a transport error the manager swallowed; + // deferring on it is harmless (the lane drops a cleanup it finds already clean) + // and is the only signal a swallowed failure gives us. + const removed = await roomManager.removeUserFromRoom(room, socket.id) + if (!removed) { + requestEvictionCleanup(socket.id, room) + return + } + await roomManager.broadcastPresenceUpdate(room, socket.id) + } catch (error) { + logger.warn( + `Presence cleanup failed for evicted table socket ${socket.id}; deferring to the sweep`, + error + ) + requestEvictionCleanup(socket.id, room) + } +} + /** * Live cell-selection presence for the table grid. Mirrors the workspace-files * join flow but is table-scoped (room id = tableId) with a bidirectional @@ -135,7 +209,7 @@ export function setupTablesHandlers(socket: AuthenticatedSocket, roomManager: IR const authorized = await resolveRoomJoinAuth({ userId, room, - action: 'read', + action: TABLE_ACTION, logger, logLabel: `table room for ${userId}`, messages: { @@ -151,16 +225,6 @@ export function setupTablesHandlers(socket: AuthenticatedSocket, roomManager: IR // Server-authenticated avatar for the presence roster. const avatarUrl = await resolveAvatarUrl(socket, userId) - // Leave a previously-joined table room if switching tables. No generation guard is needed - // around this: serialization guarantees no concurrent op committed to a different room - // during the lookup, so `currentRoom` is the socket's genuine prior room, safe to leave. - const currentRoom = await roomManager.getRoomForSocket(socket.id, ROOM_TYPES.TABLE) - if (currentRoom && currentRoom.id !== tableId) { - socket.leave(roomName(currentRoom)) - await roomManager.removeUserFromRoom(currentRoom, socket.id) - await roomManager.broadcastPresenceUpdate(currentRoom) - } - // Reclaim presence orphaned by an ungraceful disconnect (no `disconnecting` // event fires on a pod crash; the room hashes have no TTL). Returns the roster it // read so the same-tab dedup below reuses it instead of issuing a second read. @@ -182,10 +246,64 @@ export function setupTablesHandlers(socket: AuthenticatedSocket, roomManager: IR } } + // Re-check access too: the access re-validation sweep records a revocation BEFORE + // it evicts, so a join that authorized just before the revocation must not + // complete afterwards and put the socket back in the room. RE-RESOLVES rather + // than peeking — a peek treats an expired entry as unknown and fails open, which + // a join stalled longer than the cache TTL would slip through. Normally a cache + // hit (this join's own authorize just warmed it). + const currentPermission = await resolveCurrentRoomPermission(userId, room, TABLE_ACTION) + if (!satisfiesRoomMembership(currentPermission, ROOM_TYPES.TABLE)) { + socket.emit(TABLE_PRESENCE_EVENTS.JOIN_ERROR, { + tableId, + error: 'Access denied to table', + code: 'ACCESS_DENIED', + retryable: false, + }) + return + } + + // Only now that the join is certain to proceed, leave a previously-joined table room + // if switching. Deliberately AFTER the access re-check: a denial there aborts the + // join, and leaving first would silently drop the client from a prior table it may + // still be allowed to occupy. No generation guard is needed around this — + // serialization guarantees no concurrent op committed to a different room during the + // lookup, so `currentRoom` is the socket's genuine prior room, safe to leave. + const currentRoom = await roomManager.getRoomForSocket(socket.id, ROOM_TYPES.TABLE) + if (currentRoom && currentRoom.id !== tableId) { + socket.leave(roomName(currentRoom)) + await roomManager.removeUserFromRoom(currentRoom, socket.id) + await roomManager.broadcastPresenceUpdate(currentRoom) + } + // Final re-check before the membership commit: a LEAVE or a newer JOIN enqueued during the - // awaits above bumped the generation, or the socket disconnected. Abort before registering. + // awaits above — including the access re-resolve — bumped the generation, or the socket + // disconnected. if (superseded()) return + // The prior-room leave above is the one place this handler still awaits AFTER the + // authoritative access re-check (file-doc and the workspace-list rooms leave + // synchronously, so they have no such window). A sweep revocation landing in that + // window would otherwise let this join put a revoked socket back in the room, since + // `superseded()` only watches the join generation. A cache PEEK is the right + // instrument here and needs no await: the authoritative resolve moments ago wrote a + // fresh entry, so the only way this reads differently is a newer decision recorded + // since — exactly the revocation being guarded against. Synchronous, so nothing can + // interleave between it and the join below. + // `undefined` stays "unknown, not denied" here as everywhere else in this handler — + // only a definitively cached insufficient permission aborts a join the authoritative + // check just passed. + const finalCheck = peekRoomPermission(userId, room) + if (finalCheck !== undefined && !satisfiesRoomMembership(finalCheck, ROOM_TYPES.TABLE)) { + socket.emit(TABLE_PRESENCE_EVENTS.JOIN_ERROR, { + tableId, + error: 'Access denied to table', + code: 'ACCESS_DENIED', + retryable: false, + }) + return + } + socket.join(roomName(room)) const presence: UserPresence = { @@ -298,6 +416,14 @@ export function setupTablesHandlers(socket: AuthenticatedSocket, roomManager: IR const room = await roomManager.getRoomForSocket(socket.id, ROOM_TYPES.TABLE) if (!room) return + // Membership was authorized at join; re-check it here so a revoked viewer + // stops publishing presence into the room mid-session. + const access = isTableAccessAllowed(socket, room) + if (!access.allowed) { + if (access.revoked) await evictFromTable(socket, roomManager, room) + return + } + // Persist so a later joiner sees this viewer's current selection in the join ack. await roomManager.updateUserActivity(room, socket.id, { cell: selection }) diff --git a/apps/realtime/src/handlers/workspace-invalidation-room.test.ts b/apps/realtime/src/handlers/workspace-invalidation-room.test.ts index 643fca696f0..e487edd0a8e 100644 --- a/apps/realtime/src/handlers/workspace-invalidation-room.test.ts +++ b/apps/realtime/src/handlers/workspace-invalidation-room.test.ts @@ -2,6 +2,7 @@ * @vitest-environment node */ import { ROOM_TYPES } from '@sim/realtime-protocol/rooms' +import { sleep } from '@sim/utils/helpers' import { beforeEach, describe, expect, it, vi } from 'vitest' import type { IRoomManager } from '@/rooms' @@ -19,6 +20,7 @@ vi.mock('@sim/platform-authz/rooms', () => ({ })) import { setupWorkspaceInvalidationRoom } from '@/handlers/workspace-invalidation-room' +import { beginRoomPermissionRead, commitRoomPermission } from '@/middleware/permissions' type Payload = { workspaceId?: string } @@ -157,6 +159,82 @@ describe.each([ROOM_TYPES.WORKSPACE_FILES, ROOM_TYPES.WORKSPACE_TABLES] as const expect(roomManager.broadcastPresenceUpdate).not.toHaveBeenCalled() }) + it('aborts a join superseded during the access re-check await', async () => { + // The access re-resolve is an await like any other: a leave landing during it must + // still cancel this join, or the stale join would leave the room the client + // switched to and commit the abandoned one. Forced down the re-resolve's DB path + // by expiring the cached decision mid-join, so the interleaving is deterministic + // rather than dependent on microtask ordering. + vi.useFakeTimers() + try { + const { handlers, socket } = createSocket({ id: 'socket-sup', userId: 'user-sup' }) + setupWorkspaceInvalidationRoom( + socket as unknown as Parameters[0], + createRoomManager(), + roomType + ) + + let call = 0 + mockAuthorizeRoom.mockImplementation(async () => { + call += 1 + if (call === 1) { + // A later-started read commits, so this join's own decision is dropped; then + // the join stalls past the TTL so that decision is expired by re-check time. + commitRoomPermission( + 'user-sup', + { type: roomType, id: 'ws-sup' }, + 'admin', + beginRoomPermissionRead() + ) + await sleep(31_000) + } else { + // Second call is the re-check's re-resolve: the client leaves during it. + handlers[leaveEvent]({ workspaceId: 'ws-sup' }) + } + return { allowed: true, status: 200, workspaceId: 'ws-sup', workspacePermission: 'admin' } + }) + + const joining = handlers[joinEvent]({ workspaceId: 'ws-sup' }) + await vi.advanceTimersByTimeAsync(31_000) + await joining + + expect(call).toBe(2) + expect(socket.join).not.toHaveBeenCalled() + expect(socket.emit).not.toHaveBeenCalledWith(successEvent, expect.anything()) + } finally { + vi.useRealTimers() + } + }) + + it('does not join when access was revoked while the join was in flight', async () => { + // The sweep records a revocation before it evicts, so a join whose authorize + // completed just before that must not put the socket back in the room. + const { handlers, socket } = createSocket({ id: 'socket-race', userId: 'user-race' }) + setupWorkspaceInvalidationRoom( + socket as unknown as Parameters[0], + createRoomManager(), + roomType + ) + + mockAuthorizeRoom.mockImplementation(async () => { + commitRoomPermission( + 'user-race', + { type: roomType, id: 'ws-race' }, + null, + beginRoomPermissionRead() + ) + return { allowed: true, status: 200, workspaceId: 'ws-race', workspacePermission: 'admin' } + }) + + await handlers[joinEvent]({ workspaceId: 'ws-race' }) + + expect(socket.emit).toHaveBeenCalledWith( + errorEvent, + expect.objectContaining({ code: 'ACCESS_DENIED', retryable: false }) + ) + expect(socket.join).not.toHaveBeenCalled() + }) + it('leaves a previously-joined room when switching workspaces', async () => { const { socket, handlers, rooms } = createSocket() rooms.add(roomOf('ws-old')) diff --git a/apps/realtime/src/handlers/workspace-invalidation-room.ts b/apps/realtime/src/handlers/workspace-invalidation-room.ts index aacb141e9ae..362e37875f6 100644 --- a/apps/realtime/src/handlers/workspace-invalidation-room.ts +++ b/apps/realtime/src/handlers/workspace-invalidation-room.ts @@ -1,7 +1,9 @@ import { createLogger } from '@sim/logger' +import { ROOM_MEMBERSHIP_ACTIONS, satisfiesRoomMembership } from '@sim/platform-authz/room-policy' import { type RoomRef, type RoomType, roomName } from '@sim/realtime-protocol/rooms' import { resolveRoomJoinAuth } from '@/handlers/room-join-auth' import type { AuthenticatedSocket } from '@/middleware/auth' +import { resolveCurrentRoomPermission } from '@/middleware/permissions' import type { IRoomManager } from '@/rooms' const logger = createLogger('WorkspaceInvalidationRoom') @@ -89,7 +91,7 @@ export function setupWorkspaceInvalidationRoom( const authorized = await resolveRoomJoinAuth({ userId: socket.userId, room: ref, - action: 'read', + action: ROOM_MEMBERSHIP_ACTIONS[roomType], logger, logLabel: `${roomType} room for ${socket.userId}`, messages: { @@ -102,8 +104,31 @@ export function setupWorkspaceInvalidationRoom( }) if (!authorized) return - // A newer join started on this socket during authorize (or it dropped): abort so a - // stale join can't leave the room the client has since switched to. + // Re-check access before committing: the access re-validation sweep records a + // revocation BEFORE it evicts, so a join that authorized just before the + // revocation must not complete afterwards and put the socket back in the room. + // RE-RESOLVES rather than peeking — a peek treats an expired entry as unknown and + // fails open, which a join stalled longer than the cache TTL would slip through. + // Normally a cache hit (this join's own authorize just warmed it). Mirrors the + // file-doc and table joins. + const currentPermission = await resolveCurrentRoomPermission( + socket.userId, + ref, + ROOM_MEMBERSHIP_ACTIONS[roomType] + ) + if (!satisfiesRoomMembership(currentPermission, roomType)) { + socket.emit(errorEvent, { + workspaceId, + error: 'Access denied to workspace', + code: 'ACCESS_DENIED', + retryable: false, + }) + return + } + + // A newer join started on this socket during the awaits above — including the access + // re-resolve — or it dropped: abort so a stale join can't leave the room the client has + // since switched to. Last await before the commit, so nothing interleaves after it. if (joinGeneration !== joinAttempt || socket.disconnected) return // Leave any previously-joined room of this type (workspace switch), read straight from the diff --git a/apps/realtime/src/middleware/permissions.test.ts b/apps/realtime/src/middleware/permissions.test.ts index d9f0b4d15f0..c109259f390 100644 --- a/apps/realtime/src/middleware/permissions.test.ts +++ b/apps/realtime/src/middleware/permissions.test.ts @@ -7,6 +7,7 @@ * - Edge cases and invalid inputs */ +import { ALL_SOCKET_OPERATIONS } from '@sim/realtime-protocol/constants' import { expectPermissionAllowed, expectPermissionDenied, @@ -116,9 +117,10 @@ describe('checkRolePermission', () => { }) describe('read role', () => { - it('should only allow update-position for read role', () => { + it('should deny update-position for read role (it persists block coordinates)', () => { const result = checkRolePermission('read', 'update-position') - expectPermissionAllowed(result) + expectPermissionDenied(result, 'read') + expectPermissionDenied(result, 'update-position') }) it('should deny batch-add-blocks operation for read role', () => { @@ -137,9 +139,10 @@ describe('checkRolePermission', () => { expectPermissionDenied(result, 'read') }) - it('should allow batch-update-positions operation for read role', () => { + it('should deny batch-update-positions for read role (it persists block coordinates)', () => { const result = checkRolePermission('read', 'batch-update-positions') - expectPermissionAllowed(result) + expectPermissionDenied(result, 'read') + expectPermissionDenied(result, 'batch-update-positions') }) it('should deny replace-state operation for read role', () => { @@ -157,11 +160,11 @@ describe('checkRolePermission', () => { expectPermissionDenied(result, 'read') }) - it('should deny all write operations for read role', () => { - const readAllowedOps = ['update-position', 'batch-update-positions'] - const writeOperations = SOCKET_OPERATIONS.filter((op) => !readAllowedOps.includes(op)) - - for (const operation of writeOperations) { + it('grants the read role NO operation at all', () => { + // Every operation reaching this gate is persisted, so a read-only member must + // hold none of them — including the position updates that used to be granted + // here on the mistaken premise that they were ephemeral cursor sync. + for (const operation of ALL_SOCKET_OPERATIONS) { const result = checkRolePermission('read', operation) expect(result.allowed).toBe(false) expect(result.reason).toContain('read') @@ -209,28 +212,42 @@ describe('checkRolePermission', () => { }) describe('permission hierarchy verification', () => { - it('should verify admin has same permissions as write', () => { - const adminOps = ROLE_ALLOWED_OPERATIONS.admin - const writeOps = ROLE_ALLOWED_OPERATIONS.write - - // Admin and write should have same operations - expect(adminOps).toEqual(writeOps) - }) - - it('should verify read is a subset of write permissions', () => { - const readOps = ROLE_ALLOWED_OPERATIONS.read - const writeOps = ROLE_ALLOWED_OPERATIONS.write - - for (const op of readOps) { - expect(writeOps).toContain(op) + // These assert the PRODUCTION ACL over the protocol's complete operation list. + // They used to compare the shared test fixture against itself, which certified + // whatever the fixture said — including, for a while, the read-role grants that + // let a read-only member persist block positions. + + it('grants admin everything write has, plus the admin-only operations', () => { + for (const operation of ALL_SOCKET_OPERATIONS) { + if (checkRolePermission('write', operation).allowed) { + expect(checkRolePermission('admin', operation).allowed).toBe(true) + } + } + // Strictly greater: at least one operation admin holds and write does not. + const adminOnly = ALL_SOCKET_OPERATIONS.filter( + (operation) => + checkRolePermission('admin', operation).allowed && + !checkRolePermission('write', operation).allowed + ) + expect(adminOnly.length).toBeGreaterThan(0) + }) + + it('grants read nothing, so it is trivially a subset of write', () => { + const readAllowed = ALL_SOCKET_OPERATIONS.filter( + (operation) => checkRolePermission('read', operation).allowed + ) + expect(readAllowed).toEqual([]) + }) + + it('keeps the shared fixture in step with the production ACL', () => { + // The fixture is a convenience mirror; drift between it and the real table is + // what made the stale read grants look intentional. + for (const operation of ALL_SOCKET_OPERATIONS) { + const fixtureAllows = ROLE_ALLOWED_OPERATIONS.read.includes( + operation as (typeof ROLE_ALLOWED_OPERATIONS.read)[number] + ) + expect(fixtureAllows).toBe(checkRolePermission('read', operation).allowed) } - }) - - it('should verify read has minimal permissions', () => { - const readOps = ROLE_ALLOWED_OPERATIONS.read - expect(readOps).toHaveLength(2) - expect(readOps).toContain('update-position') - expect(readOps).toContain('batch-update-positions') }) }) @@ -244,7 +261,7 @@ describe('checkRolePermission', () => { readAllowed: false, }, { operation: 'update', adminAllowed: true, writeAllowed: true, readAllowed: false }, - { operation: 'update-position', adminAllowed: true, writeAllowed: true, readAllowed: true }, + { operation: 'update-position', adminAllowed: true, writeAllowed: true, readAllowed: false }, { operation: 'update-name', adminAllowed: true, writeAllowed: true, readAllowed: false }, { operation: 'toggle-enabled', adminAllowed: true, writeAllowed: true, readAllowed: false }, { operation: 'update-parent', adminAllowed: true, writeAllowed: true, readAllowed: false }, @@ -265,7 +282,7 @@ describe('checkRolePermission', () => { operation: 'batch-update-positions', adminAllowed: true, writeAllowed: true, - readAllowed: true, + readAllowed: false, }, { operation: 'replace-state', adminAllowed: true, writeAllowed: true, readAllowed: false }, ] @@ -337,21 +354,32 @@ describe('checkWorkflowOperationPermission', () => { expect(result.reason).toMatch(/revoked/i) }) - it('denies writes after a downgrade to read but still allows position updates', async () => { + it('denies every persisted operation after a downgrade to read, positions included', async () => { mockAuthorize.mockResolvedValue({ allowed: true, workspacePermission: 'read' }) const denied = await checkWorkflowOperationPermission(userId, workflowId, 'update', 'write') expect(denied.allowed).toBe(false) expect(denied.role).toBe('read') - const allowed = await checkWorkflowOperationPermission( + // A committed position update writes workflow_blocks, so a downgraded member + // loses it too — this used to be allowed and was the escalation path. + const position = await checkWorkflowOperationPermission( userId, workflowId, 'update-position', 'write' ) - expect(allowed.allowed).toBe(true) - expect(allowed.role).toBe('read') + expect(position.allowed).toBe(false) + expect(position.role).toBe('read') + + const batch = await checkWorkflowOperationPermission( + userId, + workflowId, + 'batch-update-positions', + 'write' + ) + expect(batch.allowed).toBe(false) + expect(batch.role).toBe('read') }) it('caches the role within the TTL to avoid a DB read on every operation', async () => { @@ -519,6 +547,43 @@ describe('verifyWorkflowAccess role-cache refresh', () => { expect(await resolveCurrentWorkflowRole(userId, workflowId, 'read')).toBe('write') }) + it('does not let a stale join-time allow bury a sweep denial that started later', async () => { + const userId = 'vw-user-3' + const workflowId = 'vw-wf-3' + + // Hand out a controllable promise per authorization call, so the two reads can be + // started in one order and settled in the other. + const settle: Array<(value: { allowed: boolean; workspacePermission: string | null }) => void> = + [] + mockAuthorize.mockImplementation( + () => + new Promise((resolve) => { + settle.push(resolve) + }) + ) + + // A join-style verify starts FIRST (against pre-revocation state) and stalls. + const staleJoin = verifyWorkflowAccess(userId, workflowId) + for (let i = 0; i < 10 && settle.length < 1; i++) await Promise.resolve() + expect(settle).toHaveLength(1) + + // The sweep's authorization starts AFTER it, and stalls too. + const sweep = resolveCurrentWorkflowRole(userId, workflowId, 'read') + for (let i = 0; i < 10 && settle.length < 2; i++) await Promise.resolve() + expect(settle).toHaveLength(2) + + // The sweep's denial lands first, then the older join's allow. Ordering by WRITE + // time would let the join bury the denial and hand the socket another full TTL of + // access; ordering by read start keeps the denial in force. + settle[1]({ allowed: false, workspacePermission: null }) + expect(await sweep).toBeNull() + settle[0]({ allowed: true, workspacePermission: 'write' }) + await staleJoin + + mockAuthorize.mockRejectedValue(new Error('must not re-query')) + expect(await resolveCurrentWorkflowRole(userId, workflowId, 'read')).toBeNull() + }) + it('does not let a stale in-flight resolution overwrite a fresher verify decision', async () => { const userId = 'vw-user-2' const workflowId = 'vw-wf-2' diff --git a/apps/realtime/src/middleware/permissions.ts b/apps/realtime/src/middleware/permissions.ts index 779814257bf..69892a590a6 100644 --- a/apps/realtime/src/middleware/permissions.ts +++ b/apps/realtime/src/middleware/permissions.ts @@ -1,6 +1,7 @@ import { db } from '@sim/db' import { workflow } from '@sim/db/schema' import { createLogger } from '@sim/logger' +import { authorizeRoom, type PermissionType } from '@sim/platform-authz/rooms' import { authorizeWorkflowByWorkspacePermission } from '@sim/platform-authz/workflow' import { BLOCK_OPERATIONS, @@ -12,6 +13,7 @@ import { VARIABLE_OPERATIONS, WORKFLOW_OPERATIONS, } from '@sim/realtime-protocol/constants' +import { ROOM_TYPES, type RoomRef, roomName } from '@sim/realtime-protocol/rooms' import { and, eq, isNull } from 'drizzle-orm' const logger = createLogger('SocketPermissions') @@ -54,17 +56,25 @@ const WRITE_OPERATIONS: string[] = [ WORKFLOW_OPERATIONS.REPLACE_STATE, ] -// Read role can only update positions (for cursor sync, etc.) -const READ_OPERATIONS: string[] = [ - BLOCK_OPERATIONS.UPDATE_POSITION, - BLOCKS_OPERATIONS.BATCH_UPDATE_POSITIONS, -] - -// Define operation permissions based on role +/** + * Operation permissions by role. + * + * `read` grants NOTHING. Every operation that reaches this gate is durably + * persisted — `handlers/operations.ts` follows an allowed check with + * `persistWorkflowOperation`, which writes `workflow_blocks` / `workflow` rows — + * so granting a read-only member any entry here is a write, not a read. + * + * This previously listed `updatePosition` + `batchUpdatePositions` as readable + * "for cursor sync", which they are not: live cursors ride their own + * `cursor-update` event (`handlers/presence.ts`), and the smooth-drag broadcast + * is the UNCOMMITTED position path, which returns before persisting and never + * consults this table at all. The only thing the grant actually enabled was a + * read-only collaborator permanently rewriting block coordinates. + */ const ROLE_PERMISSIONS: Record = { admin: [...ADMIN_ONLY_OPERATIONS, ...WRITE_OPERATIONS], write: WRITE_OPERATIONS, - read: READ_OPERATIONS, + read: [], } // Check if a role allows a specific operation (no DB query, pure logic) @@ -85,11 +95,12 @@ export function checkRolePermission( } /** - * TTL for the per-pod role cache backing live re-validation. It gates both the - * mutating-operation checks ({@link checkWorkflowOperationPermission}) and the - * periodic read-access sweep (`access-revalidation.ts`), so a revoked or - * downgraded collaborator loses write access — and live reads — on an - * already-connected socket within a bounded window rather than until disconnect. + * TTL for the per-pod role cache backing live re-validation. It gates the + * mutating-operation checks ({@link checkWorkflowOperationPermission}), the + * per-frame room write gates (file-doc / tables), and the periodic access sweep + * (`access-revalidation.ts`), so a revoked or downgraded collaborator loses write + * access — and live reads — on an already-connected socket within a bounded + * window rather than until disconnect. */ export const ROLE_REVALIDATION_TTL_MS = 30_000 @@ -99,11 +110,42 @@ const MAX_ROLE_CACHE_ENTRIES = 5_000 interface CachedRole { /** Authoritative workspace role, or `null` when the user has no access. */ role: string | null + /** + * The {@link beginRoomPermissionRead} ticket of the query this decision came + * from — i.e. when its DB read STARTED, not when it was written. + */ + readSeq: number expiresAt: number } /** - * Per-pod cache of authoritative workspace roles, keyed by `${userId}:${workflowId}`. + * Monotonic ticket counter establishing a total order over authorization READS. + * + * Ordering by write time is not sufficient: two readers can start their queries in + * one order and finish in the other, so the decision written last can be derived + * from the older read. A join that authorized before a revocation but returned + * after the sweep's denial would then win — leaving a revoked user with another + * full TTL of access. Every writer therefore takes a ticket before it queries and + * a decision only yields to one from a strictly later-started read. + */ +let roleReadSeqCounter = 0 + +/** + * Takes a read ticket. Call immediately BEFORE issuing an authorization query, and + * pass the result to {@link commitRoomPermission} once it returns. + */ +export function beginRoomPermissionRead(): number { + return ++roleReadSeqCounter +} + +/** + * Per-pod cache of authoritative workspace roles, keyed by + * `${userId}:${roomName(room)}`. + * + * The key uses the wire room name, so a workflow entry is `${userId}:${workflowId}` + * exactly as before (workflow rooms are unprefixed) while every other room type is + * namespaced by its own `type:id` — a file-doc and a table can never collide on an + * id, and one cache serves every room type. * * Socket connections are sticky to a single pod, so a socket's mutating operations are * always gated by the same pod's cache. We rely on TTL expiry (not cross-pod @@ -114,13 +156,17 @@ const roleCache = new Map() /** * In-flight resolutions keyed like {@link roleCache}. Concurrent callers for the same - * (user, workflow) share one authorization query instead of racing independent ones, so + * (user, room) share one authorization query instead of racing independent ones, so * cache writes per key are serialized — a slow, stale pre-revocation read can never * overwrite a newer recorded decision (e.g. the revocation the eviction sweep just * cached before kicking the socket). */ const inFlightRoleResolutions = new Map>() +function roleCacheKey(userId: string, room: RoomRef): string { + return `${userId}:${roomName(room)}` +} + function purgeExpiredRoles(now: number): void { for (const [key, entry] of roleCache) { if (entry.expiresAt <= now) { @@ -129,48 +175,81 @@ function purgeExpiredRoles(now: number): void { } } -/** - * Records a freshly-read authoritative decision into the role cache. Every - * successful DB read of a user's workspace role goes through this — including - * the join-time {@link verifyWorkflowAccess} — so a stale cached revocation - * never outlives a newer authoritative read (e.g. a re-granted user re-joining - * within the TTL of the sweep's recorded `null`). - */ -function recordRoleDecision(key: string, role: string | null): void { +function recordRoleDecision(key: string, role: string | null, readSeq: number): void { const now = Date.now() if (roleCache.size >= MAX_ROLE_CACHE_ENTRIES) { purgeExpiredRoles(now) } - roleCache.set(key, { role, expiresAt: now + ROLE_REVALIDATION_TTL_MS }) + roleCache.set(key, { role, readSeq, expiresAt: now + ROLE_REVALIDATION_TTL_MS }) +} + +/** + * Commits a freshly-read authoritative decision, unless a decision from a + * strictly later-started read is already cached — in which case that one stands + * and is returned instead. + * + * Every successful DB read of a user's workspace role goes through this: the + * cached resolver, the join-time {@link verifyWorkflowAccess}, and the room join + * authorizer. That is what keeps a stale cached revocation from outliving a newer + * authoritative read (a re-granted user re-joining inside the sweep's recorded + * `null` TTL) while equally keeping a stale ALLOW from burying the sweep's fresher + * denial. Returns the decision that ends up in force. + */ +function commitRoleDecision(key: string, role: string | null, readSeq: number): string | null { + const existing = roleCache.get(key) + if (existing !== undefined && existing.readSeq > readSeq) return existing.role + recordRoleDecision(key, role, readSeq) + return role +} + +/** + * Reads a user's authoritative effective workspace permission for a room, or + * `null` when they have none. Workflow rooms go through their own dedicated + * authorizer; every other room type resolves through the shared + * {@link authorizeRoom} (resource → owning workspace → effective permission). + * + * Always asks for the LOWEST level (`read`) so the resolved permission itself is + * returned rather than a boolean — callers compare it against whatever level the + * operation needs with `permissionSatisfies`. A room type that is not + * authorizable here (status 400) THROWS, so it lands in the caller's transient + * branch and can never be mistaken for a confirmed revocation. + */ +async function readAuthoritativeRoomPermission( + userId: string, + room: RoomRef +): Promise { + if (room.type === ROOM_TYPES.WORKFLOW) { + const authorization = await authorizeWorkflowByWorkspacePermission({ + workflowId: room.id, + userId, + action: 'read', + }) + return authorization.allowed ? (authorization.workspacePermission ?? null) : null + } + + const authorization = await authorizeRoom({ userId, room, action: 'read' }) + if (!authorization.allowed && authorization.status === 400) { + throw new Error(`Room type not authorizable: ${room.type}`) + } + return authorization.allowed ? (authorization.workspacePermission ?? null) : null } async function resolveRoleUncached( key: string, userId: string, - workflowId: string, + room: RoomRef, fallbackRole: string ): Promise { - const entryBeforeQuery = roleCache.get(key) + const readSeq = beginRoomPermissionRead() try { - const authorization = await authorizeWorkflowByWorkspacePermission({ - workflowId, - userId, - action: 'read', - }) - const role = authorization.allowed ? (authorization.workspacePermission ?? null) : null - // A fresh authoritative read (e.g. a join-time verifyWorkflowAccess) may - // have recorded a decision while this query was in flight. That write is - // newer than this query's read snapshot, so prefer it instead of - // overwriting it with a potentially stale result. - const entryAfterQuery = roleCache.get(key) - if (entryAfterQuery !== undefined && entryAfterQuery !== entryBeforeQuery) { - return entryAfterQuery.role - } - recordRoleDecision(key, role) - return role + const role = await readAuthoritativeRoomPermission(userId, room) + // Yields only to a decision from a later-STARTED read (see commitRoleDecision). + // Comparing write order instead would let a join whose authorize began before + // this one — but returned after it — bury this result. + return commitRoleDecision(key, role, readSeq) } catch (error) { logger.warn( - `Failed to re-validate role for user ${userId} on workflow ${workflowId}; using last known role`, + `Failed to re-validate role for user ${userId} on ${roomName(room)}; using last known role`, error ) // Prefer the last recorded decision — even if expired, and even if it is `null` for an @@ -183,23 +262,23 @@ async function resolveRoleUncached( } /** - * Resolves a user's current workspace role for a workflow, re-reading the `permissions` - * table at most once per {@link ROLE_REVALIDATION_TTL_MS} per pod. Concurrent calls for - * the same (user, workflow) coalesce onto a single in-flight query (single-flight), so - * out-of-order cache writes cannot resurrect revoked access. + * Resolves a user's current effective workspace permission for a room, re-reading the + * `permissions` table at most once per {@link ROLE_REVALIDATION_TTL_MS} per pod. + * Concurrent calls for the same (user, room) coalesce onto a single in-flight query + * (single-flight), so out-of-order cache writes cannot resurrect revoked access. * - * Returns `null` when the user genuinely has no access (removed/revoked). On a transient - * DB failure it reuses the last recorded decision for this (user, workflow) — including a - * previously recorded revocation (`null`) — and only falls back to `fallbackRole` when no - * decision has been recorded yet, so a blip neither blocks legitimate editors nor - * resurrects already-revoked access. + * Returns `null` when the user genuinely has no access (removed/revoked, or the room's + * resource is gone). On a transient DB failure it reuses the last recorded decision for + * this (user, room) — including a previously recorded revocation (`null`) — and only + * falls back to `fallbackRole` when no decision has been recorded yet, so a blip neither + * blocks legitimate editors nor resurrects already-revoked access. */ -export async function resolveCurrentWorkflowRole( +export async function resolveCurrentRoomPermission( userId: string, - workflowId: string, + room: RoomRef, fallbackRole: string ): Promise { - const key = `${userId}:${workflowId}` + const key = roleCacheKey(userId, room) const cached = roleCache.get(key) if (cached && cached.expiresAt > Date.now()) { return cached.role @@ -210,13 +289,65 @@ export async function resolveCurrentWorkflowRole( return inFlight } - const resolution = resolveRoleUncached(key, userId, workflowId, fallbackRole).finally(() => { + const resolution = resolveRoleUncached(key, userId, room, fallbackRole).finally(() => { inFlightRoleResolutions.delete(key) }) inFlightRoleResolutions.set(key, resolution) return resolution } +/** + * Workflow-room specialization of {@link resolveCurrentRoomPermission}, kept as the + * name the workflow operation/join paths already use. + */ +export function resolveCurrentWorkflowRole( + userId: string, + workflowId: string, + fallbackRole: string +): Promise { + return resolveCurrentRoomPermission( + userId, + { type: ROOM_TYPES.WORKFLOW, id: workflowId }, + fallbackRole + ) +} + +/** + * Non-blocking read of the cached permission for a (user, room). + * + * Returns `undefined` when nothing fresh is cached — the caller must NOT read that + * as a denial; it means "unknown, ask asynchronously". Exists for the per-frame + * gates on hot relay paths (Yjs document frames, table cell selections), which must + * stay synchronous: awaiting an authorization promise per keystroke would put a + * microtask between every frame and its apply. Freshness is bounded by the same + * {@link ROLE_REVALIDATION_TTL_MS}, and the periodic sweep independently evicts a + * revoked socket, so a gate built on this is never the only line of defense. + */ +export function peekRoomPermission(userId: string, room: RoomRef): string | null | undefined { + const cached = roleCache.get(roleCacheKey(userId, room)) + if (!cached || cached.expiresAt <= Date.now()) return undefined + return cached.role +} + +/** + * Commits an authoritative decision read outside this module (the room join + * authorizer) into the shared cache, so the sweep and the per-frame gates start + * warm on the room a socket just joined. + * + * `readSeq` must come from a {@link beginRoomPermissionRead} taken BEFORE the + * caller's query: the decision is discarded if a later-started read already + * committed one, so neither a stale ALLOW can bury the sweep's fresher denial nor + * a stale revocation outlive a re-grant. + */ +export function commitRoomPermission( + userId: string, + room: RoomRef, + permission: PermissionType | null, + readSeq: number +): void { + commitRoleDecision(roleCacheKey(userId, room), permission, readSeq) +} + /** * Live permission gate for mutating socket operations. Re-validates the user's workspace * role against the database (cached per pod for {@link ROLE_REVALIDATION_TTL_MS}) so that @@ -256,6 +387,9 @@ export async function verifyWorkflowAccess( userId: string, workflowId: string ): Promise<{ hasAccess: boolean; role?: string; workspaceId?: string }> { + // Taken before the reads below, so this decision yields to any authorization + // that started later (e.g. the sweep's denial) instead of by write order. + const readSeq = beginRoomPermissionRead() try { const workflowData = await db .select({ @@ -278,9 +412,10 @@ export async function verifyWorkflowAccess( action: 'read', }) - recordRoleDecision( - `${userId}:${workflowId}`, - authorization.allowed ? (authorization.workspacePermission ?? null) : null + commitRoleDecision( + roleCacheKey(userId, { type: ROOM_TYPES.WORKFLOW, id: workflowId }), + authorization.allowed ? (authorization.workspacePermission ?? null) : null, + readSeq ) if (!authorization.allowed || !authorization.workspacePermission) { diff --git a/apps/sim/app/(landing)/components/hero/components/hero-platform-loop/hero-platform-loop.tsx b/apps/sim/app/(landing)/components/hero/components/hero-platform-loop/hero-platform-loop.tsx index eb3d8b1c72a..9e8144d2c0d 100644 --- a/apps/sim/app/(landing)/components/hero/components/hero-platform-loop/hero-platform-loop.tsx +++ b/apps/sim/app/(landing)/components/hero/components/hero-platform-loop/hero-platform-loop.tsx @@ -163,7 +163,7 @@ export function HeroPlatformLoop() {
diff --git a/apps/sim/app/(landing)/components/hero/components/hero-platform-loop/hero-workflow-stage.tsx b/apps/sim/app/(landing)/components/hero/components/hero-platform-loop/hero-workflow-stage.tsx index 3164086c537..fc235d22e59 100644 --- a/apps/sim/app/(landing)/components/hero/components/hero-platform-loop/hero-workflow-stage.tsx +++ b/apps/sim/app/(landing)/components/hero/components/hero-platform-loop/hero-workflow-stage.tsx @@ -143,7 +143,7 @@ export function HeroWorkflowStage({ stroke='var(--workflow-edge)' strokeWidth={2} strokeLinecap='round' - className='transition-[stroke-dashoffset] duration-500 ease-[cubic-bezier(0.22,1,0.36,1)] [stroke-dasharray:1]' + className='transition-[stroke-dashoffset] duration-500 [stroke-dasharray:1] [transition-timing-function:cubic-bezier(0.22,1,0.36,1)]' style={{ strokeDashoffset: visible ? 0 : 1 } as CSSProperties} /> ) @@ -156,7 +156,7 @@ export function HeroWorkflowStage({
@@ -30,7 +30,7 @@ export function BlockHandles({ block, handlesVisible = true }: BlockHandlesProps diff --git a/apps/sim/app/(landing)/components/hero/components/hero-visual/hero-visual.tsx b/apps/sim/app/(landing)/components/hero/components/hero-visual/hero-visual.tsx index b163bf0672d..e3f46d6790a 100644 --- a/apps/sim/app/(landing)/components/hero/components/hero-visual/hero-visual.tsx +++ b/apps/sim/app/(landing)/components/hero/components/hero-visual/hero-visual.tsx @@ -962,7 +962,7 @@ export function HeroVisual() { GitHub block. FOCUS is the identity transform (block 1 centered); only the pull-out animates. */}
{showCard && (
@@ -1030,7 +1030,7 @@ export function HeroVisual() { {/* Edges (scene space, origin = panel center). Drawn as the camera follows the connection to each revealed block. */} shown ? 'translate-y-0 opacity-100 blur-0' : 'translate-y-1.5 opacity-0 blur-[3px]' @@ -220,7 +221,7 @@ export function StageHome({ morphs to a block. */}