k1nq/TREK
1
0
Fork 0
mirror of https://github.com/mauriceboe/TREK.git synced 2026-10-18 21:16:47 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(mcp): the six write tools that never had a demo check stay open to demo accounts under the central gate

Browse Source 
sync_trip_documents and the five manual school-holiday tools carried no demo check of their own before the gate moved into the registry, so a demo account could call them. The gate now lets exactly those through, and a test pins the list. upload_trip_file keeps the gate's order: a demo account is refused before its trip access is checked, where the tool used to answer a trip it cannot see with the no-access result first.
This commit is contained in:
Maurice
2026-10-10 19:39:09 +02:00
parent e7e26bf2e6
commit bb6eb1ab89
4 changed files with 66 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
server/CLAUDE.md
+1 -1
View File
@@ -71,7 +71,7 @@ Nest owns everything. Every domain is a DI module under `src/nest/<domain>/` (`c
- **`src/bootstrap.ts` — `buildApp()`** is the single builder for production and the test harness. Everything on the raw Express instance (global middleware, uploads, discovery metadata, statics, the named `jsonParser`/`urlencodedParser` wrappers) is registered **before `app.init()`**, because Nest throws `NotFoundException` on unmatched routes and nothing after init is reachable. The wrappers exempt `/mcp` so the MCP SDK reads the raw stream — **never `@Body()` a `/mcp` route**. Don't add routes to that pre-init shell; it bypasses every global guard, interceptor and filter.
- **Realtime**: `/ws` is a Nest gateway in `src/nest/realtime/`. Inject `RealtimeService`; tests hand in a `FakeRealtimeService` (`tests/helpers/fake-realtime.ts`) or spy on the container's instance instead of mocking the transport module. Trip mutations forward `X-Socket-Id` so the originating client doesn't echo its own change.
- **MCP**: `src/nest-mcp/` is the decorator/registry layer (`@McpController`/`@Tool`/`@Resource`/`@Prompt`; see its `README.md`); tools live in `<domain>.mcp.ts` files beside each module; the HTTP transport is `src/nest/mcp-transport/`; `src/mcp/` holds process-wide state only (scopes, session manager, boot policy). Every tool invocation writes an audit row through the registry's `onInvoke` seam. The demo-mode write block is the registry's `toolGate` (`trekDemoToolGate`): every tool not annotated read-only refuses a demo account there, so a tool never checks demo mode itself (ESLint refuses it in `*.mcp.ts`). MCP tools are adapters over the same services and permission checks as REST — a change on one side lands on the other in the same change.
- **MCP**: `src/nest-mcp/` is the decorator/registry layer (`@McpController`/`@Tool`/`@Resource`/`@Prompt`; see its `README.md`); tools live in `<domain>.mcp.ts` files beside each module; the HTTP transport is `src/nest/mcp-transport/`; `src/mcp/` holds process-wide state only (scopes, session manager, boot policy). Every tool invocation writes an audit row through the registry's `onInvoke` seam. The demo-mode write block is the registry's `toolGate` (`trekDemoToolGate`): every tool not annotated read-only refuses a demo account there (except the write tools in `DEMO_UNGATED_WRITE_TOOLS`, which never had a demo check), so a tool never checks demo mode itself (ESLint refuses it in `*.mcp.ts`). MCP tools are adapters over the same services and permission checks as REST — a change on one side lands on the other in the same change.
- **Scheduling**: `src/nest/scheduling/` (`CronRegistrarService`) is the one way to schedule a cron. Jobs are `*.job.ts` providers in their owning domain. The registrar refuses to schedule under `NODE_ENV=test` (an integration test pins it). **Never use a `@Cron` decorator**: it bypasses that gate. Every tick first takes the job's row in `scheduler_leases` (`SchedulerLeasesRepository.acquire`, one conditional upsert that decides the race), renews it while it runs and keeps it for a short settle window afterwards, so two processes on one database never run the same tick; a job needs nothing of its own for this. Claims on individual rows (the reminder jobs' `claimReminder`) stay where they are: the lease decides who runs a tick, the claim what a tick may send.
- **Plugins** (`src/nest/plugins/`): the child-process runtime (`supervisor/`, `runtime/plugin-host-entry.ts`, `protocol/envelope.ts`) is **intentionally not Nest**; it is the security boundary. `host/rpc-host.ts` registers an RPC method at spawn only if the plugin holds the unlocking permission (registration = authorization). Install-time gates live in `install/`; the manifest's `trek` version range fails closed, and `TREK_PLUGINS_IGNORE_TREK_RANGE` downgrades that to a warning. The protocol tables in `protocol/envelope.ts` (+ `src/plugin-event-sink.ts`) are **generated** into `plugin-sdk/` and `shared/`: run `gen:plugin-facts` after touching them and commit the output. What a plugin gets back is `protocol/output-contract.ts`: every wire method has an entry, and an entity result (`trips.getById`, `todos.list`, `vacay.mine`, ...) is cut down to its published fields in `rpc-host.ts` on the way out, together with every child row it carries whose field is listed in `PLUGIN_ENTITY_NESTED` (a day's `notes_items`, a reservation's `endpoints`). A new column on any whole-row table behind those entities, child tables included, fails `tests/unit/plugins/output-contract.schema.test.ts` until it is listed as published or withheld (the SDK's `PLUGIN_ENTITY_FIELDS` and `PLUGIN_ENTITY_NESTED` are generated from it); a method left as `readModel` must return a value built field by field from named columns. A plugin's own SQL (`host/plugin-data.service.ts`) is held to a row cap, and `query`, `tx` and an `exec` script to a time budget checked between rows and statements (`migrate` is deliberately not timed). `runtime/egress-policy.ts` is a hand-kept mirror guarded by a parity test in `plugin-sdk/`; change both sides together.
- **Storage**: `src/nest/storage/` is the upload driver abstraction (local / S3 / mirror behind one interface, with an admin surface and a usage job). Domain services never touch the filesystem or the S3 SDK directly. Where the data lives (`data/`, `uploads/`, backups, logs, key files, the DB file) is resolved in one place, `src/app-config/data-paths.ts`; a provider that reads one injects `DataPathsService`, plain helper modules shared with code outside the container call `resolveDataPaths()` (the list is in `src/app-config/README.md`), and nothing anchors a data path on its own `__dirname` or on `process.cwd()`.
server/src/mcp/nest-mcp-policy.ts
+23 -6
View File
@@ -57,16 +57,33 @@ const VALID_GROUP_MODES: ReadonlySet<string> = new Set(ALL_SCOPES);
export const trekMcpValidateAccess: McpAccessValidator = ({ group, mode }) =>
VALID_GROUP_MODES.has(`${group}:${mode}`) ? null : `no '${group}:${mode}' scope in SCOPES`;
/**
* Write tools a demo account has always been able to call: none of them ever
* carried a demo check of its own, so the central gate lets them through to
* keep that. Taking one off this list blocks it for demo accounts, which is a
* behaviour change of its own. tests/unit/mcp/demo-tool-gate.test.ts pins the
* list.
*/
export const DEMO_UNGATED_WRITE_TOOLS: ReadonlySet<string> = new Set([
'sync_trip_documents',
'create_manual_school_holiday_country',
'create_manual_school_holiday_region',
'update_manual_school_holiday_region',
'delete_manual_school_holiday_region',
'delete_manual_school_holiday_country',
]);
/**
* True for a tool the demo gate holds back: every tool that does not declare
* itself read-only. The annotation, not `access.mode`, is the signal, because
* the share/content scopes cover reads too (get_share_link,
* get_trip_calendar_feed, read_trip_file) and the predicate-gated tools carry
* no mode at all. tests/unit/mcp/demo-tool-gate.test.ts holds the two in step:
* every `mode: 'read'` tool is read-only and every `mode: 'write'` one is not.
* itself read-only, except the ones in DEMO_UNGATED_WRITE_TOOLS. The
* annotation, not `access.mode`, is the signal, because the share/content
* scopes cover reads too (get_share_link, get_trip_calendar_feed,
* read_trip_file) and the predicate-gated tools carry no mode at all.
* tests/unit/mcp/demo-tool-gate.test.ts holds the two in step: every
* `mode: 'read'` tool is read-only and every `mode: 'write'` one is not.
*/
export function isDemoGatedTool(tool: ToolOptions): boolean {
return tool.annotations?.readOnlyHint !== true;
return tool.annotations?.readOnlyHint !== true && !DEMO_UNGATED_WRITE_TOOLS.has(tool.name);
}
/**
server/src/nest-mcp/README.md
+2 -1
View File
@@ -83,7 +83,8 @@ createTestRegistry(instances, { accessPolicy, toolGate: (tool, ctx) => undefined
The gate is built from the container (`imports`/`inject`/`useFactory`), because
a check that reads the database cannot live in static `forRoot` options. TREK's
is `trekDemoToolGate` in `src/mcp/nest-mcp-policy.ts`: a demo account calling a
tool not annotated `readOnlyHint: true` gets `demoDenied()`. Tools therefore
tool not annotated `readOnlyHint: true` gets `demoDenied()`, except the few
write tools in `DEMO_UNGATED_WRITE_TOOLS` that never had a demo check. Tools therefore
never check demo mode themselves; ESLint refuses `isDemoUser`/`isDemoUserId`/
`demoDenied()` in `*.mcp.ts`.
server/tests/unit/mcp/demo-tool-gate.test.ts
+40 -2
View File
@@ -12,7 +12,7 @@
*/
import { ADDON_IDS } from '../../../src/addons';
import { db as testDb } from '../../../src/db/database';
import { isDemoGatedTool, trekDemoToolGate } from '../../../src/mcp/nest-mcp-policy';
import { DEMO_UNGATED_WRITE_TOOLS, isDemoGatedTool, trekDemoToolGate } from '../../../src/mcp/nest-mcp-policy';
import type { McpContext, McpRegistry, McpRegistryListing } from '../../../src/nest-mcp';
import { withRequestContext } from '../../../src/nest/database/request-context';
import { createUser } from '../../helpers/factories';
@@ -93,12 +93,42 @@ describe('demo tool gate over the whole registry', () => {
const { user: demo } = createUser(testDb, { email: 'demo@trek.app' });
const tools = await captureTools(registry, { userId: demo.id, scopes: null, isStaticToken: false });
const writes = [...tools].filter(([, t]) => t.annotations?.readOnlyHint !== true);
const writes = [...tools].filter(
([name, t]) => t.annotations?.readOnlyHint !== true && !DEMO_UNGATED_WRITE_TOOLS.has(name),
);
expect(writes.length).toBeGreaterThan(150);
const answers = await Promise.all(writes.map(async ([name, tool]) => ({ name, result: await tool.call() })));
expect(answers).toEqual(writes.map(([name]) => ({ name, result: DEMO_REFUSAL })));
});
it('DEMOGATE-007: the write tools that never had a demo check stay open to the demo account', async () => {
process.env.DEMO_MODE = 'true';
const { user: demo } = createUser(testDb, { email: 'demo@trek.app' });
const ctx: McpContext = { userId: demo.id, scopes: null, isStaticToken: false };
const tools = await captureTools(registry, ctx);
const gate = trekDemoToolGate(async () => true);
const open = [...DEMO_UNGATED_WRITE_TOOLS].sort();
expect(open).toEqual([
'create_manual_school_holiday_country',
'create_manual_school_holiday_region',
'delete_manual_school_holiday_country',
'delete_manual_school_holiday_region',
'sync_trip_documents',
'update_manual_school_holiday_region',
]);
for (const name of open) {
expect(tools.get(name), name).toBeDefined();
expect(tools.get(name)?.annotations?.readOnlyHint, name).not.toBe(true);
}
const refusals = await Promise.all(
open.map(async (name) => ({
name,
refusal: await gate({ name, annotations: tools.get(name)?.annotations }, ctx),
})),
);
expect(refusals).toEqual(open.map((name) => ({ name, refusal: undefined })));
});
it('DEMOGATE-002: no read-only tool is held back for the demo account', async () => {
process.env.DEMO_MODE = 'true';
const { user: demo } = createUser(testDb, { email: 'demo@trek.app' });
@@ -183,6 +213,14 @@ describe('trekDemoToolGate', () => {
expect(isDemo).not.toHaveBeenCalled();
});
it('DEMOGATE-014: a listed write tool passes the gate without asking about the caller', async () => {
const isDemo = vi.fn(async () => true);
const gate = trekDemoToolGate(isDemo);
expect(await gate({ name: 'sync_trip_documents', annotations: { readOnlyHint: false } }, ctx)).toBeUndefined();
expect(isDemo).not.toHaveBeenCalled();
expect(isDemoGatedTool({ name: 'delete_manual_school_holiday_country' })).toBe(false);
});
it('DEMOGATE-013: a tool without annotations counts as a write', () => {
expect(isDemoGatedTool({ name: 'x' })).toBe(true);
expect(isDemoGatedTool({ name: 'x', annotations: { readOnlyHint: true } })).toBe(false);