diff --git a/CLAUDE.md b/CLAUDE.md index 39f94220..de0b8d8b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -81,7 +81,8 @@ pos-cli/ │ ├── data/ # Import/export/clean │ ├── assets/ # Asset deployment │ ├── logsv2/ # OpenObserve logs integration -│ └── validators/ # Input validation +│ ├── validation/ # Ajv schema validation (shared by GUI + MCP) +│ └── validators/ # CLI argument validators (url, email, paths) ├── mcp-min/ # MCP server implementation │ ├── index.js # Starts stdio + HTTP/SSE transports │ ├── stdio-server.js # MCP over stdio (for editor integrations) @@ -397,6 +398,96 @@ Can run with sync: `pos-cli gui serve staging --sync --open` - **ora** - Loading spinners - **yeoman-generator** - Code generators +## Input Validation (Ajv) + +**Key files**: `lib/validation/index.js`, `lib/validation/schemas/gui.js`, +`mcp-min/validate-params.js`, `mcp-min/schemas/auth.js`, `mcp-min/schemas/default.js` + +Untrusted input is validated against JSON Schema with **Ajv** (draft-07) before it reaches +any handler. Ajv is used rather than a code-first library because the MCP protocol requires +JSON Schema on the wire: each tool's `inputSchema` is advertised verbatim in `tools/list`, +so the schema we publish and the schema we enforce are the same object and cannot drift. + +`lib/validation/index.js` exposes one function: + +```javascript +import { validate } from '#lib/validation/index.js'; + +const result = validate(schema, data); // { valid, errors, message, schemaError } +const coerced = validate(schema, req.query, { mode: 'coercing' }); +``` + +- **`strict` mode (default)** — for JSON bodies. Leaves the caller's data untouched. +- **`coercing` mode** — for query strings, where every value arrives as a string. Ajv + applies coercion and defaults **by mutating the object in place**. +- **`result.schemaError`** — the schema itself would not compile. That is our defect, not + the caller's, so report it as 500 / `-32603` — but still reject, because nothing was + actually checked. Reserved for genuine compile failures: an unknown `mode` throws a + `RangeError` (a caller bug), and boolean schemas — legal JSON Schema that cannot key the + compile cache — validate normally rather than surfacing as a phantom compile failure. + +`validate()` returns only `{ valid, errors, message, schemaError }`. It does not return the +data; in `coercing` mode the caller's own object is what gets mutated. + +Ajv runs in `strict: true` mode so a malformed schema fails loudly at compile time. +`allowUnionTypes` is the one rule relaxed, for fields that genuinely accept two types — +`logsSearchSchema.query`, which arrives as an object over POST and a string over GET, is +the only one. `ajv-formats` is loaded, and `format: 'uri'` / `format: 'email'` on the shared +auth properties are what use it. + +### Enforcement points + +| Where | What is validated | +|---|---| +| `mcp-min/http-server.js` — `POST /call`, `/call-stream` | tool params vs `inputSchema` → 400 | +| `mcp-min/http-server.js` — JSON-RPC `tools/call` | same → `-32602` | +| `mcp-min/stdio-server.js` — `tools/call` + legacy direct invocation | same → `-32602` | +| `mcp-min/tools.js` | `tools.config.json` vs `tools.config.schema.json`, plus tool names | +| `lib/server.js` | GUI requests for graph / liquid / logs / logsv2 / sync | + +The five MCP dispatch sites (the first three rows) route through `rejectionFor` in +`mcp-min/validate-params.js`, so the mapping from a rejection to a status code (400/500, +`-32602`/`-32603`) is made in one place for the transports. The GUI server keeps its own +`rejectInvalid` in `lib/server.js` because it answers with a different body shape; the two +apply the same 400/500 rule and have to be changed together. +A tool that declares no schema falls back to `OPEN_OBJECT_SCHEMA` in +`mcp-min/schemas/default.js` — the same constant both `tools/list` responses advertise, so +what is published and what is enforced cannot disagree. + +Adding a tool to `mcp-min/` needs no wiring: both transports validate against whatever +`inputSchema` the tool declares. A tool with no schema accepts any object. + +### Two rules to preserve + +**`env` must stay optional on tools that authenticate.** `resolveAuth` (`mcp-min/auth.js`) +resolves credentials in this order: + +1. explicit `url` + `email` + `token` params +2. the named `.pos` environment (`params.env`) +3. `MPKIT_URL` / `MPKIT_EMAIL` / `MPKIT_TOKEN` env vars +4. the first entry in `.pos` + +Marking `env` as `required` would reject three of those four supported call styles. Tools +closing their schema with `additionalProperties: false` must also spread in +`authProperties` from `mcp-min/schemas/auth.js`, or the explicit-credentials path becomes +unreachable. That rule is enforced by `mcp-min/__tests__/validate-params.test.js`, which +derives the tool list by scanning for `resolveAuth` rather than hard-coding names — a +hand-written list silently stops guarding tools added later. + +Because `env` is advertised as optional, an MCP client that omits it lands on step 4 — the +*first* `.pos` entry — including for mutating tools (`data-import`, `constants-set`, +`uploads-push`). Runtime behaviour is unchanged, since nothing enforced `required` before, +but the advertised contract now invites the omission. + +**The tools config fails closed.** A missing or unparseable config falls back to defaults; +one that parses but is invalid throws `ToolsConfigError`. That file decides which tools are +exposed, so ignoring a broken one would silently re-enable every tool the author meant to +switch off. Two checks, because the schema alone is not enough: it validates the shape, and +`loadToolsConfig` separately rejects entries naming a tool that does not exist — a typo +like `deploy-strt` matches nothing in `applyConfig` and would otherwise leave `deploy-start` +enabled while the config looks like it took effect. `bin/pos-cli-mcp.js` catches the error +and reports it through `logger`, so a config mistake never surfaces as a Node stack trace. + ### Testing Philosophy Integration tests against real platformOS instances for reliability. Tests cover: - Deploy (various strategies, error handling) diff --git a/backlog/completed/task-3 - add-ajv-input-validation-fix-GUI-logs-regression-and-close-the-gaps-in-the-new-validation-layer.md b/backlog/completed/task-3 - add-ajv-input-validation-fix-GUI-logs-regression-and-close-the-gaps-in-the-new-validation-layer.md new file mode 100644 index 00000000..37ffae20 --- /dev/null +++ b/backlog/completed/task-3 - add-ajv-input-validation-fix-GUI-logs-regression-and-close-the-gaps-in-the-new-validation-layer.md @@ -0,0 +1,210 @@ +--- +id: TASK-3 +title: >- + add-ajv-input-validation: fix GUI logs regression and close the gaps in the + new validation layer +status: Done +assignee: [] +created_date: '2026-09-02 10:21' +updated_date: '2026-09-02 14:17' +labels: + - bug + - validation + - mcp + - gui + - security +dependencies: [] +references: + - lib/validation/index.js + - lib/validation/schemas/gui.js + - lib/server.js + - mcp-min/validate-params.js + - mcp-min/schemas/auth.js + - mcp-min/tools.js + - mcp-min/http-server.js + - mcp-min/stdio-server.js + - gui/next/src/lib/api/logs.js + - gui/next/src/routes/logs/+page.svelte + - CLAUDE.md +priority: high +ordinal: 18000 +--- + +## Description + + +Branch `add-ajv-input-validation` (commits fe9f93c, 8dd89bd) adds Ajv JSON Schema validation on the MCP transports (stdio + HTTP), on `mcp-min/tools.config.json`, and on the GUI server routes, as input-validation evidence for the SOC2 audit. It also relaxes `required: ['env']` on tools that authenticate, because `resolveAuth` supports three other call styles. + +The design is right and should not change: validating each MCP tool's own `inputSchema` means the schema advertised in `tools/list` and the schema enforced are the same object, so there is no second definition to drift. `allowUnionTypes` is genuinely required (verified: `logsSearchSchema.query` is the only schema that needs it). The new tests do bite — six independent mutations of the production code each broke tests as intended. + +Everything below lives in the code and tests this branch introduces. It is one task because shipping any of it would either break a user-visible feature or leave the branch's own stated contract unenforced. + +## 1. Blocker: the admin GUI Logs page returns 400 on every load + +`lib/validation/schemas/gui.js` types the log cursor as `{ type: 'integer', minimum: 0 }`. The client that actually calls the route (`gui/next/src/lib/api/logs.js:18-20`) sends the literal string `null` when there is no cursor: + +```js +const last = args.last ?? null; +return fetch(`${url}?lastId=` + last) // -> ?lastId=null +``` + +`routes/logs/+page.svelte:41` seeds that value from `$state.logs.logs?.at(-1)?.id ?? null`, so the first poll always sends it. Ajv does not coerce `"null"` to an integer. Verified against a running `lib/server.js`: + +``` +GET /api/logs?lastId=null -> 400 {"error":"Invalid request: /lastId must be integer"} +``` + +`logs.get` swallows the rejection into `{ error }`, so the page renders empty with no indication why. `?lastId=` (empty) is rejected the same way. + +The schema comment in `gui.js:56` shows the root cause: the constraint was derived from `lib/test-runner/logStream.js` rather than from the client that calls the route. The preferred fix is client-side (`args.last ?? 0` — `0` is already the "from the beginning" sentinel that `logStream.js` uses) plus a `gui/next` rebuild, since `gui/next/build` is committed and shipped in the npm package. Normalising empty/`null`/`undefined` away inside the route is acceptable only as a compatibility shim for already-installed GUI builds, and must be commented as such. + +The same field is typed `integer` here but `string` in `mcp-min/logs/fetch.js:15` and `mcp-min/logs/stream.js`. Pick one, or document why they differ. + +## 2. The "one schema, no drift" claim is broken by a third default + +Three different objects now describe "this tool declares no schema": `{ type: 'object', properties: {} }` in the stdio tools/list (`stdio-server.js:52`), `{ type: 'object', additionalProperties: true }` in the HTTP tools/list (`http-server.js:175`), and `{ type: 'object' }` in the enforcement path (`validate-params.js:6`). The first two predate the branch; the branch added the third, which is the one actually enforced. Hoist a single exported constant and use it at all three sites. `envs-list` (`mcp-min/tools.js:100`) is the only tool without `additionalProperties: false` — now that schemas are enforced, close it like the others. + +## 3. `validate()` mislabels two non-schema failures as schema-compile failures + +`result.schemaError` maps to HTTP 500 / JSON-RPC `-32603` at four call sites. Two failures are wrongly routed there: + +``` +validate({type:'object'}, {}, {mode:'nope'}) -> schemaError: "Schema failed to compile: Unknown validation mode: nope" +validate(true, {x:1}) -> schemaError: "Schema failed to compile: Invalid value used as weak map key" +``` + +The first is a caller programming error, not a schema defect. The second is a *legal* boolean JSON Schema that dies in the WeakMap cache at `lib/validation/index.js:45` because primitives cannot be WeakMap keys. Neither is reachable from today's schemas; both are traps for the next person. Also, `validate()` returns a `data` field that no caller reads — drop it or use it. + +## 4. The `env`-optional rule is unenforced for 15 of 22 tools + +CLAUDE.md states that any tool closing its schema with `additionalProperties: false` must also declare `url`/`email`/`token`, or the explicit-credentials path in `resolveAuth` becomes unreachable. Every tool satisfies this today (checked all 22 that call `resolveAuth`), but the test that guards it (`mcp-min/__tests__/validate-params.test.js:64-72`) uses a hand-written list of 7 tool names. Proof the guard does not hold: stripping `url`/`email`/`token` from `mcp-min/migrations/list.js` — a tool that calls `resolveAuth` — leaves all 338 mcp-min tests passing. Replace the list with an invariant derived from the tool registry so tools added later are covered automatically. + +Related: `mcp-min/schemas/auth.js` is applied to only 6 tools (`constants/*`, `data/import*`, `uploads/push`); the other 16 duplicate the three properties inline and, unlike the shared fragment, ship no `description` for them, so `tools/list` documents the same parameters inconsistently. Apply the fragment everywhere or delete it. + +## 5. `ajv-formats` is a dead runtime dependency + +`addFormats(ajv)` is called but no schema in the repo uses the `format` keyword (`mcp-min/check/index.js:17` is a property *named* `format`, not the keyword). Either drop the dependency or use it — `format: 'email'` / `'uri'` on the shared auth properties is the obvious missing half, given `lib/validators/email.js` and `lib/validators/url.js` already enforce that for CLI arguments. + +## 6. "The tools config fails closed" is only half true + +A config naming a tool that does not exist is silently ignored, which is exactly the outcome the new throw was added to prevent. Verified: `MCP_TOOLS_CONFIG = {"tools":{"deploy-strt":{"enabled":false}}}` starts the server with `deploy-start` still enabled and all 35 tools exposed. The schema cannot catch this because it cannot enumerate tool names; `applyConfig` (`mcp-min/tools.js:183`) iterates the registry and never inspects config keys. Add a pass over `Object.keys(config.tools)` that rejects (or at minimum loudly warns about) names with no matching tool. + +Separately, the throw itself reaches the user as a raw Node stack trace, doubled with the `log.error` line above it (`[ERROR] invalid tools config at …` followed by `Error: Invalid tools config at …` and 6 module-loader frames). Exit code is 1, which is correct, but the output contradicts CLAUDE.md's own error-handling guidance ("user-friendly error messages… log to logger for consistent formatting"). Catch at the `bin/pos-cli-mcp.js` boundary and report through `logger.Error`. + +## 7. Documented `resolveAuth` precedence is wrong + +The new CLAUDE.md section states the order as "explicit params, then `MPKIT_*` env vars, then the named `.pos` environment, then the first `.pos` entry". `mcp-min/auth.js:33-56` actually resolves: explicit params -> **named `.pos` environment** -> `MPKIT_*` -> first `.pos` entry. The same wrong order appears in the comment at `mcp-min/__tests__/validate-params.test.js:61`. CLAUDE.md is the file future agents follow, so this needs to be right. (The stale JSDoc in `auth.js` it was copied from is tracked separately.) + +Also worth a line in the affected tool descriptions: because `env` is now advertised as optional, an MCP client that omits it lands on the *first* `.pos` entry for mutating tools (`data-import`, `constants-set`, `uploads-push`). Runtime behaviour is unchanged — nothing enforced `required` before — but the advertised contract now invites the omission. + +## 8. Coverage gaps in the new tests + +- The `schemaError` -> 500 / `-32603` branch is untested at all four call sites. +- `test/unit/validation.test.js:89` ("compiles each schema once and reuses it") validates twice and asserts both are valid. It asserts nothing about caching. +- In `mcp-min/__tests__/tools-config-validation.test.js`, the `test.each` case for a bare `null` config passes for the wrong reason: with the throw removed it still fails, because `applyConfig` then dereferences `null`. Only the `false`/`0`/`""` cases exercise the fail-closed check the comment describes. +- No coverage for: stdio legacy direct invocation (`stdio-server.js:171-181`, including its non-JSON-RPC `send({ id, error })` shape), `/call-stream` legacy streaming validation (`http-server.js:229-234`), `logsv2` `query` as an object (the actual `gui/next` payload) or as a string (the union that forced `allowUnionTypes`), and POST-body coercion on `/api/logsv2` despite the comment at `lib/server.js:127-128` claiming it. +- `transport-validation.test.js:15` hardcodes port 5931 while sibling suites use 5920/5930/5940. `startHttp` resolves with the server, so `port: 0` plus `server.address().port` removes the collision class. +- Schema assertions are split between `.toBeUndefined()` and `.not.toContain('env')` for the same intent; the `required` relaxations on `data-validate` and `unit-tests-run` have no assertion at all. + +## 9. Style + +Two lines added to `mcp-min/http-server.js` exceed the 120-char `.editorconfig` limit: the `respond({ error: { code, … } })` call in JSON-RPC `tools/call` (128) and the `/call-stream` rejection (129). + +## Suite status at time of review + +On the lockfile-pinned `ajv@8.20.0` / `ajv-formats@3.0.1`: the five new/changed test files 80/80 pass; `mcp-min/__tests__` 338/338 pass; `test/unit` 1153 pass with 1 failure (`modules.test.js > publishVersion() … single-dir workflow` times out, reproduced identically on a clean `master` worktree — tracked separately). + +Note: `ajv`/`ajv-formats` were absent from the project's `node_modules` during review — `npm install` had never been run after they were added, so everything resolved from an unrelated `~/node_modules/ajv@8.18.0`. Run `npm ci` first. `npm run pretest` also rewrites `test/fixtures/yeoman{,/custom}/package-lock.json`; revert those before committing. + + +## Acceptance Criteria + +- [x] #1 GET /api/logs accepts the cursor shape gui/next actually sends, and the admin Logs page populates on first load with no 400 response +- [x] #2 The log cursor is typed consistently across lib/validation/schemas/gui.js, mcp-min/logs/fetch.js and mcp-min/logs/stream.js, or the divergence is documented in the schema +- [x] #3 A single shared constant describes the no-schema default, used by both tools/list responses and the enforcement path in validate-params.js +- [x] #4 The envs-list tool schema is closed with additionalProperties: false like every other tool +- [x] #5 validate() distinguishes an unknown mode and a boolean schema from a schema compile failure, and neither is reported as 500 / -32603 +- [x] #6 validate() no longer returns a field that no caller reads +- [x] #7 Removing url, email or token from the schema of any tool that calls resolveAuth fails the test suite (today: stripping them from mcp-min/migrations/list.js leaves all 338 mcp-min tests passing) +- [x] #8 mcp-min/schemas/auth.js is spread into every tool with a closed schema that authenticates, or removed in favour of the inline declarations +- [x] #9 ajv-formats is used by at least one schema or removed from package.json dependencies +- [x] #10 A tools.config.json entry naming a tool that does not exist is rejected or loudly warned about, so the fail-closed guarantee in CLAUDE.md holds +- [x] #11 An invalid tools config reports through logger with a user-facing message and exits non-zero, with no raw Node stack trace +- [x] #12 CLAUDE.md and the comment in validate-params.test.js state resolveAuth's actual precedence: explicit params, named .pos environment, MPKIT_* env vars, first .pos entry +- [ ] #13 The schemaError branch is covered by a test at each of the four sites that maps it to 500 / -32603 +- [x] #14 No vacuous assertions remain in the new suites: the compile-once test either verifies that Ajv compiles a schema once, or is removed +- [x] #15 The bare-null tools-config case fails because validation rejected the config, not because applyConfig dereferenced null +- [x] #16 Validation is covered for the stdio legacy direct-invocation path, the /call-stream legacy streaming path, logsv2 query-as-object and query-as-string, and POST-body coercion on /api/logsv2 +- [x] #17 transport-validation.test.js binds an ephemeral port rather than a hardcoded one +- [x] #18 Schema assertions use one consistent style, and the required relaxations on data-validate and unit-tests-run are asserted +- [ ] #19 No line added by this branch exceeds the 120-character limit in .editorconfig + + +## Final Summary + + +Verified at commits dfdd3ab ("fix GUI logs regression and close gaps in the validation layer") and a16db70 ("fix cursor round-trip and prototype-chain hole in config validation"). 17 of 19 acceptance criteria met; 2 carried forward, detailed below. + +## Blocker fixed and verified end to end + +Fixed on both sides: `gui/next/src/lib/api/logs.js` now sends `args.last ?? 0`, and `lib/server.js` strips `'null'`/`'undefined'`/`''` from the cursor before validating, with a comment naming it a shim for GUI builds already installed and stating the condition for removing it. `logsRequestSchema` gained `default: 0` so an absent cursor no longer interpolates `"undefined"` upstream. + +Probed against a real `lib/server.js` listener (a fake upstream, so 502 = passed validation and attempted the request): + +``` +?lastId=null (what the SHIPPED gui/next/build still sends) 502 +?lastId=0 (what the fixed source sends) 502 +?lastId=42 (subsequent poll) 502 +(no param) 502 +?lastId=1&admin=true 400 /lastId must be integer +?lastId=newest 400 /lastId must be integer +?lastId=-1 400 /lastId must be >= 0 +``` + +Note `gui/next/build` was deliberately not rebuilt — the shipped bundle still contains `c.last??null`, confirmed by grep. The shim is therefore what actually fixes the page for users today, and the source fix is dormant until someone runs `npm run build` in `gui/next`. That is the right trade (a SvelteKit rebuild would churn every hashed asset filename), it is documented in the code, and both shapes are covered by tests. + +## Verified by mutation, not just by reading + +Each mutation applied to production code, suite run, mutation reverted: + +- strip `...authProperties` from `mcp-min/migrations/list.js` (a tool the old hand-written list never named) → 2 failures, one naming the file. The invariant now scans `mcp-min/**` for `resolveAuth(` with fast-glob, so a tool added later is covered the moment it authenticates. This was the escape proven in the review. +- flatten `rejectionFor`'s status mapping to a constant 400 / -32602 → 4 failures, one per MCP dispatch path. +- flatten `rejectInvalid`'s mapping in `lib/server.js` to a constant 400 → **0 failures**. See carried-forward item below. + +## Criterion-by-criterion + +1, 2 — cursor accepted in every shape the GUI sends; typed `integer` consistently in `gui.js`, `mcp-min/logs/fetch.js` and `mcp-min/logs/stream.js`. +3, 4 — `OPEN_OBJECT_SCHEMA` in the new `mcp-min/schemas/default.js`, imported by both `tools/list` responses and `validate-params.js`; `envs-list` closed with `additionalProperties: false`. +5, 6 — unknown mode now throws `RangeError`; boolean schemas skip the WeakMap and validate normally; `data` removed from the return. All three asserted. +7, 8 — every one of the 22 `resolveAuth` tools spreads `authProperties`; no inline `url`/`email`/`token` triple remains anywhere. +9 — `ajv-formats` is now earned: `format: 'uri'` and `format: 'email'` on the shared auth properties. This matches `lib/validators/url.js`, which already requires a parseable absolute URL, so the CLI and MCP surfaces now agree. +10, 11 — `loadToolsConfig` rejects config keys matching no registered tool, using `hasOwnProperty` rather than `in` (with the reasoning in a comment), and names every unknown key. New `ToolsConfigError` caught at the `bin/pos-cli-mcp.js` boundary and reported through `logger`; a test asserts exit 1, the message present, and no stack frames or `node:internal` in the output. +12 — CLAUDE.md now states the real precedence (params → named `.pos` → `MPKIT_*` → first entry) and adds the first-`.pos`-entry warning to the descriptions of `data-import`, `constants-set`, `constants-unset` and `uploads-push` in `tools.config.json`. +14 — the compile-once test now counts schema property reads through a Proxy and asserts the second call reads nothing. It actually measures caching. +15 — the bare-`null` config case now asserts `must be object` and `not.toContain('TypeError')`, so it can no longer pass on the incidental `applyConfig` crash. +16 — added: stdio legacy direct invocation in both its JSON-RPC and bare `{ id, error }` shapes, `/call-stream` legacy streaming (including a `content-type: application/json` assertion, proving rejection precedes the SSE handshake), `logsv2` query-as-object and query-as-string, scalar-into-string-union coercion, a union miss, POST-body coercion, and a `searchAround` payload. +17 — `startHttp({ port: 0 })` with `server.address().port`. +18 — `required` assertions are now uniformly `toEqual([...])` or `toBeUndefined()`, plus a dedicated "required relaxations" block covering `data-validate` and `unit-tests-run`. + +## Found and fixed beyond the task + +Tightening `logs-fetch.lastId` to `integer` (criterion 2) would have broken the tool's own paging: the handler returned the cursor as a string, which the schema it advertises would then reject with -32602. The fix returns `Number(latestId)` and adds a round-trip test that feeds the returned cursor back through validation. `latestId` is only ever assigned a numeric string, so there is no NaN path. + +Worth a release note: `logs-fetch.lastId` and `logs-stream.startLastId` changed type from `string` to `integer`, and `envs-list` now rejects unknown parameters. MCP clients read `tools/list` per session so they adapt automatically, but a hard-coded caller sending `lastId: "42"` will now get -32602. + +## Carried forward + +**Criterion 13 — partially met.** Three of the four `schemaError` sites are genuinely covered via a test-only tool with an uncompilable schema, mutation-verified. The GUI leg is not: the "uncompilable schema on a GUI route" block in `test/unit/server.validation.test.js` builds a throwaway express app and reimplements the status expression inline, so it never exercises `lib/server.js`. Flattening the real `rejectInvalid` mapping to 400 passes all 32 tests in that file. Tracked as TASK-3.1. Left as a follow-up rather than a blocker because no schema in the repo fails to compile, so the branch is unreachable in practice — it is regression protection for a rule CLAUDE.md itself notes is written in two places. + +**Criterion 19 — deviation accepted.** Both code lines originally flagged in `mcp-min/http-server.js` are now within 120 characters. Four new lines exceed it, all of them single-quoted `description:` string literals in tool definitions, in files where `master` already carries description lines of 150–320 characters (`liquid/exec.js:7` is 317). Wrapping them would need string concatenation that no other tool uses. Flagged rather than silently accepted; reopen if the repo ever adopts a linter that enforces `.editorconfig`. + +**One stale comment.** `lib/validation/schemas/gui.js` still reads "The same field is declared `integer` here but `string` on the MCP logs tools" — both are now `integer`, which the same sentence goes on to say. Reads as a description of the change rather than of the code. Cosmetic. + +**Docs/test tension.** CLAUDE.md says "a tool with no schema accepts any object", but `validate-params.test.js` asserts `inputSchema.type === 'object'` for every registered tool, so such a tool would fail the suite. Both are defensible; they just do not agree about whether a schema is optional. + +## Suite status + +`mcp-min/__tests__` + `test/unit`: **1605 passing, 6 skipped, 1 failing** — `modules.test.js > publishVersion() … single-dir workflow`, reproduced identically on a clean `master` worktree and tracked as TASK-9. Up from ~1491 tests before the fix. + +Scope was respected exactly: TASK-4 through TASK-12 are untouched. Confirmed by inspection — no error middleware in `lib/server.js`, all six `tools[name]` / `mcpHandlers[method]` dispatch lookups still unguarded, `bin/pos-cli-mcp-config.js` still does not validate, `mcp-min/auth.js` JSDoc still has the wrong order, and `gui/next/src/lib/api/logsv2.js` untouched. The `hasOwnProperty` fix in commit a16db70 hardens config-key lookup, which is a different site from TASK-5's dispatch lookups, so there is no overlap. + diff --git a/backlog/tasks/task-10 - mcp-min-auth.js-JSDoc-states-the-wrong-resolveAuth-precedence.md b/backlog/tasks/task-10 - mcp-min-auth.js-JSDoc-states-the-wrong-resolveAuth-precedence.md new file mode 100644 index 00000000..61c2dff5 --- /dev/null +++ b/backlog/tasks/task-10 - mcp-min-auth.js-JSDoc-states-the-wrong-resolveAuth-precedence.md @@ -0,0 +1,42 @@ +--- +id: TASK-10 +title: mcp-min/auth.js JSDoc states the wrong resolveAuth precedence +status: To Do +assignee: [] +created_date: '2026-09-02 10:24' +labels: + - docs + - mcp +dependencies: [] +references: + - mcp-min/auth.js +priority: low +ordinal: 25000 +--- + +## Description + + +Pre-existing on `master`. This is the source of the same error that reached CLAUDE.md via the Ajv validation branch (fixed there under TASK-3), so fixing it here stops it being copied again. + +`mcp-min/auth.js:16-20` documents the fallback order as: + +``` + * 1. Explicit params (url + email + token) + * 2. MPKIT_* environment variables + * 3. Named .pos environment (params.env) + * 4. First environment in .pos config +``` + +The function body does something different, and its own inline comments say so: `params.env` is checked at `auth.js:39` **before** `MPKIT_*` at `auth.js:46`, labelled "Priority 2: Named .pos environment" and "Priority 3: MPKIT_*". The distinction matters — the code deliberately does not fall back to `MPKIT_*` when an env name was given ("the caller is being explicit"), which is the opposite of what the JSDoc implies. + +Correct order: explicit `url`+`email`+`token` params -> named `.pos` environment -> `MPKIT_*` env vars -> first `.pos` entry. + +Fix the JSDoc block to match, and while there, check for other copies of the wrong order in tool descriptions and README before they multiply. + + +## Acceptance Criteria + +- [ ] #1 The JSDoc on resolveAuth lists the precedence the function actually implements +- [ ] #2 The repo is searched for other copies of the wrong order (tool descriptions, README, docs) and any found are corrected + diff --git a/backlog/tasks/task-11 - MCP-HTTP-request-logging-writes-every-request-header-including-Authorization.md b/backlog/tasks/task-11 - MCP-HTTP-request-logging-writes-every-request-header-including-Authorization.md new file mode 100644 index 00000000..fdd82c7c --- /dev/null +++ b/backlog/tasks/task-11 - MCP-HTTP-request-logging-writes-every-request-header-including-Authorization.md @@ -0,0 +1,51 @@ +--- +id: TASK-11 +title: 'MCP HTTP request logging writes every request header, including Authorization' +status: To Do +assignee: [] +created_date: '2026-09-02 10:24' +labels: + - security + - mcp + - logs +dependencies: [] +references: + - mcp-min/http-server.js + - mcp-min/stdio-server.js + - mcp-min/auth.js + - mcp-min/portal/env-add.js + - mcp-min/log.js +priority: low +ordinal: 26000 +--- + +## Description + + +Pre-existing on `master`, found while reviewing the Ajv validation branch. + +`mcp-min/http-server.js:27-38` logs the full header object on every request: + +```js +log.debug('HTTP request', { + method: req.method, + url: req.originalUrl || req.url, + remoteAddress: req.ip || req.connection?.remoteAddress, + headers: req.headers +}); +``` + +Any `Authorization`, `Cookie` or `Mcp-Session-Id` header a client sends is written verbatim wherever `log.debug` goes. This is behind `DEBUG`, so it is not on by default, but `DEBUG=1` is the first thing anyone does when an MCP client misbehaves — which is exactly the situation where credentials are in play. `mcp-min/auth.js` already has a `maskToken` helper for precisely this reason, used when logging resolved auth. + +Also worth checking the `/call` and `tools/call` debug lines in the same file (`http-server.js:100`, `:198`) and their stdio equivalents: they log `params` in full, and `data-import`, `env-add`, `constants-set` and the explicit-credentials path all carry secrets in params. `mcp-min/portal/env-add.js:80` logs the whole `params` object explicitly. + +Redact a denylist of sensitive header names, and mask `token`/`password`/`value` in logged params (reusing `maskToken` where it fits) rather than dropping the logging — the request/param detail is genuinely useful for debugging. + + +## Acceptance Criteria + +- [ ] #1 Authorization, Cookie and equivalent sensitive headers are redacted in the MCP HTTP request log +- [ ] #2 Params logged by tool invocation paths mask token, password and credential values on both transports +- [ ] #3 Non-sensitive request and param detail is still logged, so DEBUG remains useful +- [ ] #4 A test asserts that a request carrying an Authorization header does not write it to the log + diff --git a/backlog/tasks/task-12 - CLAUDE.md-testing-section-still-describes-Jest-the-repo-runs-vitest.md b/backlog/tasks/task-12 - CLAUDE.md-testing-section-still-describes-Jest-the-repo-runs-vitest.md new file mode 100644 index 00000000..e4185cf9 --- /dev/null +++ b/backlog/tasks/task-12 - CLAUDE.md-testing-section-still-describes-Jest-the-repo-runs-vitest.md @@ -0,0 +1,43 @@ +--- +id: TASK-12 +title: CLAUDE.md testing section still describes Jest; the repo runs vitest +status: To Do +assignee: [] +created_date: '2026-09-02 10:24' +labels: + - docs + - tests +dependencies: [] +references: + - CLAUDE.md + - package.json + - vitest.config.js + - test/global-setup.js +priority: low +ordinal: 27000 +--- + +## Description + + +Pre-existing on `master`. Low stakes but actively misleading, since CLAUDE.md is what agents working in this repo read first. + +The "Testing" and "Testing Philosophy" sections describe a Jest setup that no longer exists: + +- `npm run test-watch` — the script is `test:watch` (`package.json`) +- "Tests run with `--runInBand` to prevent race conditions" — vitest is configured with `pool: 'forks'` and `fileParallelism: true` (`vitest.config.js`) +- "`npm test` — Run all tests with Jest" — `npm test` is `vitest run` +- "Fixtures are in `/test/fixtures/`" is still true, but the section omits that `pretest` installs the yeoman fixtures and that skipping it makes `test/unit/generators.test.js` fail with a confusing interactive prompt + +Also unmentioned: the `test:unit` / `test:integration` / `test:mcp-min` split, and the 60% coverage thresholds in `vitest.config.js` (which cover `lib/**` and `bin/**` but not `mcp-min/**`). + +The claim that tests require live credentials is only half true now — `test/unit` and `mcp-min/__tests__` run without them (`test/global-setup.js` skips cleanup when no real credentials are present); only `test/integration` needs `MPKIT_*`. Worth saying, because it tells a new contributor what they can actually run. + + +## Acceptance Criteria + +- [ ] #1 The CLAUDE.md testing sections name vitest and the actual script names +- [ ] #2 The test:unit / test:integration / test:mcp-min split is documented, including which of them need MPKIT_* credentials +- [ ] #3 The pretest yeoman-fixture install is documented, with the failure it causes when skipped +- [ ] #4 No reference to Jest or --runInBand remains in CLAUDE.md + diff --git a/backlog/tasks/task-3.1 - Pin-the-GUI-servers-schemaError-status-mapping-with-a-test-that-exercises-lib-server.js.md b/backlog/tasks/task-3.1 - Pin-the-GUI-servers-schemaError-status-mapping-with-a-test-that-exercises-lib-server.js.md new file mode 100644 index 00000000..b58667bb --- /dev/null +++ b/backlog/tasks/task-3.1 - Pin-the-GUI-servers-schemaError-status-mapping-with-a-test-that-exercises-lib-server.js.md @@ -0,0 +1,52 @@ +--- +id: TASK-3.1 +title: >- + Pin the GUI server's schemaError status mapping with a test that exercises + lib/server.js +status: To Do +assignee: [] +created_date: '2026-09-02 14:16' +labels: + - tests + - validation + - gui +dependencies: [] +references: + - lib/server.js + - test/unit/server.validation.test.js + - mcp-min/__tests__/transport-validation.test.js + - mcp-min/validate-params.js +parent_task_id: TASK-3 +priority: low +ordinal: 28000 +--- + +## Description + + +Carried forward from TASK-3, acceptance criterion 13 ("the schemaError branch is covered by a test at each of the four sites"). Three of the four sites are genuinely covered; the GUI site is not. + +The three MCP dispatch paths route through `rejectionFor` in `mcp-min/validate-params.js`, and `mcp-min/__tests__/transport-validation.test.js` registers a test-only tool with an uncompilable schema (`{ type: 'not-a-real-type' }`) to reach them for real. Mutation-verified: flattening the mapping in `rejectionFor` to a constant 400 / -32602 fails 4 tests. + +The GUI server keeps its own copy of the mapping in the `rejectInvalid` closure inside `lib/server.js` (CLAUDE.md notes the two "apply the same 400/500 rule and have to be changed together"). The test that claims to cover it — `test/unit/server.validation.test.js`, describe block "uncompilable schema on a GUI route" — builds a throwaway express app and calls `validate()` plus its own inline status expression. It never touches `lib/server.js`. Mutation-verified: replacing + +```js +const status = result.schemaError ? 500 : 400; // lib/server.js +``` + +with `const status = 400;` leaves all 32 tests in that file passing. + +So the duplicated rule is pinned on one side only, and the side that is not pinned is the one that duplicates it. + +The straightforward approach is to make the real route see an uncompilable schema — `vi.mock('#lib/validation/schemas/gui.js')` in a dedicated test file, returning a schema Ajv cannot compile for one of the five routes, then assert the route answers 500 and does not forward to the mocked Gateway. A separate file is needed because the existing suite depends on the real schemas. Replacing the throwaway-app test with that closes the gap; the alternative — exporting `rejectInvalid` so it can be tested directly — would also work but widens the module's surface for a test. + +Not urgent: no schema in the repo fails to compile today, so the 500 branch is unreachable in practice. It matters as regression protection for a rule that is written down in two places. + + +## Acceptance Criteria + +- [ ] #1 A test drives a real lib/server.js route whose schema will not compile, and asserts the response is 500 +- [ ] #2 That test asserts the mocked Gateway was not called, so an uncompilable schema still rejects rather than forwards +- [ ] #3 Changing the status expression in lib/server.js rejectInvalid to a constant 400 fails at least one test +- [ ] #4 The throwaway-express-app test in test/unit/server.validation.test.js is replaced rather than left alongside, so there is one place asserting this rule per side + diff --git a/backlog/tasks/task-4 - GUI-server-multer-failures-return-an-HTML-500-with-a-full-stack-trace.md b/backlog/tasks/task-4 - GUI-server-multer-failures-return-an-HTML-500-with-a-full-stack-trace.md new file mode 100644 index 00000000..012d52a7 --- /dev/null +++ b/backlog/tasks/task-4 - GUI-server-multer-failures-return-an-HTML-500-with-a-full-stack-trace.md @@ -0,0 +1,50 @@ +--- +id: TASK-4 +title: 'GUI server: multer failures return an HTML 500 with a full stack trace' +status: To Do +assignee: [] +created_date: '2026-09-02 10:22' +labels: + - bug + - security + - gui +dependencies: [] +references: + - lib/server.js + - test/unit/server.validation.test.js +priority: high +ordinal: 19000 +--- + +## Description + + +Pre-existing on `master`, unrelated to the Ajv validation branch, but found while reviewing it. + +`lib/server.js` has no error-handling middleware. The only route with a body parser that can reject before the handler runs is the sync proxy at `PUT /api/app_builder/marketplace_releases/sync`, which is wrapped in `upload.fields([{ name: 'path' }, { name: 'marketplace_builder_file_body' }])`. A multipart request carrying any other file field never reaches the handler — multer calls `next(err)` and express's default error handler answers. Reproduced against a running server: + +``` +PUT /api/app_builder/marketplace_releases/sync (file part named "bogus") +-> 500 Content-Type: text/html + 
MulterError: Unexpected field

+     at wrappedFileFilter (/home/…/node_modules/multer/index.js:41:19)
+     at Multipart.<anonymous> (/home/…/node_modules/multer/lib/make-middleware.js:284:7)
+     … 8 more frames
+``` + +Two problems. The response leaks absolute filesystem paths and a dependency stack trace, and pos-cli never sets `NODE_ENV=production`, so express always includes the stack. And the GUI server sets `Access-Control-Allow-Origin: *` (`lib/server.js:97-101`), so any page open in the developer's browser can trigger it and read the result. + +It also contradicts the intent already documented in the file. `sendError` (`lib/server.js:38-41`) exists specifically so that "no error text is ever handed back for a browser to parse as HTML" — that guarantee holds for gateway rejections but not for parser rejections. + +Fix by registering a JSON error-handling middleware after the routes (or by wrapping the `upload.fields` call so a `MulterError` becomes a 400 JSON body). A `MulterError` is caller error, so 400 is right; anything else should be a 502/500 with the message only, never the stack. + +Note: an in-flight task (TASK-3) fixes a *different* unhandled-500 in the same handler — the missing-file case. That fix is already on the `add-ajv-input-validation` branch and does not cover this path, so the two do not conflict, but whoever picks this up should rebase on that branch if it has landed. + + +## Acceptance Criteria + +- [ ] #1 A multipart request to the sync proxy carrying an unexpected file field receives a JSON response, not HTML +- [ ] #2 No response from lib/server.js contains a stack trace or an absolute filesystem path, regardless of NODE_ENV +- [ ] #3 A multer parser rejection is reported as 400 (caller error); other unhandled errors keep their existing status semantics +- [ ] #4 A test in test/unit covers the unexpected-file-field case and asserts the response content type is JSON + diff --git a/backlog/tasks/task-5 - MCP-prototype-chain-tool-names-bypass-the-not-found-check-on-4-of-5-dispatch-paths.md b/backlog/tasks/task-5 - MCP-prototype-chain-tool-names-bypass-the-not-found-check-on-4-of-5-dispatch-paths.md new file mode 100644 index 00000000..78a3915d --- /dev/null +++ b/backlog/tasks/task-5 - MCP-prototype-chain-tool-names-bypass-the-not-found-check-on-4-of-5-dispatch-paths.md @@ -0,0 +1,54 @@ +--- +id: TASK-5 +title: >- + MCP: prototype-chain tool names bypass the not-found check on 4 of 5 dispatch + paths +status: To Do +assignee: [] +created_date: '2026-09-02 10:22' +labels: + - bug + - security + - mcp +dependencies: [] +references: + - mcp-min/http-server.js + - mcp-min/stdio-server.js + - mcp-min/tools.js +priority: medium +ordinal: 20000 +--- + +## Description + + +Pre-existing on `master`, unrelated to the Ajv validation branch, but found while reviewing it. + +The tool registry (`mcp-min/tools.js`) is a plain object literal, and four of the five dispatch sites look a tool up with a bare `tools[name]` and test only truthiness. Names inherited from `Object.prototype` therefore resolve to something truthy that is not a tool. Reproduced against a running `startHttp`: + +``` +POST /call {"tool":"constructor"} -> 500 {"error":"TypeError: entry.handler is not a function"} +POST /call {"tool":"toString"} -> 500 {"error":"TypeError: entry.handler is not a function"} +``` + +`constructor`, `toString`, `valueOf`, `hasOwnProperty` and `__proto__` all behave this way. The affected sites: + +- `mcp-min/http-server.js:93` — `POST /call` +- `mcp-min/http-server.js:226` — `POST /call-stream`, legacy branch +- `mcp-min/stdio-server.js:82` — MCP `tools/call` +- `mcp-min/stdio-server.js:171` — legacy direct invocation + +The JSON-RPC branch in the same file already gets it right (`mcp-min/http-server.js:189`: `if (!entry || typeof entry.handler !== 'function')`) and correctly answers `-32601 Tool not found`, which is the behaviour the other four should match. + +There is a worse variant in the stdio protocol dispatcher. `mcp-min/stdio-server.js:164` does `const mcpHandler = mcpHandlers[method]`, and `mcpHandlers` is also a plain object literal. A request with `method: "toString"` resolves to `Object.prototype.toString`, which is invoked, returns a string, and **sends no response at all** — the client waits for its `id` until it times out. + +Impact is bounded (the MCP server is a local developer tool, and no state is reachable this way) but it is a trivially reachable internal error and a hang from untrusted client input, on the surface this codebase is currently hardening for audit. Fix with `Object.hasOwn` or the `typeof handler === 'function'` guard, applied consistently at all five sites, and make the stdio protocol dispatcher fall through to its existing `-32601 Method not found` path. + + +## Acceptance Criteria + +- [ ] #1 A tools/call or /call request naming constructor, toString, valueOf, hasOwnProperty or __proto__ is answered as tool-not-found (404 / -32601), not as a 500 or an internal TypeError +- [ ] #2 A stdio request whose method is an Object.prototype name is answered as method-not-found rather than receiving no response +- [ ] #3 All five dispatch sites use the same lookup guard +- [ ] #4 Tests cover at least one prototype-chain name on each transport, including the stdio protocol dispatcher hang + diff --git a/backlog/tasks/task-6 - Extract-a-shared-tools.config-loader-so-pos-cli-mcp-config-and-the-MCP-server-agree.md b/backlog/tasks/task-6 - Extract-a-shared-tools.config-loader-so-pos-cli-mcp-config-and-the-MCP-server-agree.md new file mode 100644 index 00000000..1f0621c1 --- /dev/null +++ b/backlog/tasks/task-6 - Extract-a-shared-tools.config-loader-so-pos-cli-mcp-config-and-the-MCP-server-agree.md @@ -0,0 +1,45 @@ +--- +id: TASK-6 +title: >- + Extract a shared tools.config loader so pos-cli mcp config and the MCP server + agree +status: To Do +assignee: [] +created_date: '2026-09-02 10:23' +labels: + - refactor + - mcp +dependencies: [] +references: + - mcp-min/tools.js + - bin/pos-cli-mcp-config.js + - mcp-min/tools.config.schema.json +priority: medium +ordinal: 21000 +--- + +## Description + + +Pre-existing duplication on `master`, surfaced while reviewing the Ajv validation branch. + +`mcp-min/tools.config.json` (overridable with `MCP_TOOLS_CONFIG`) is read in two independent places that each reimplement the same logic: + +- `mcp-min/tools.js` — reads and parses it, then `applyConfig` skips tools with `enabled === false` and overrides descriptions +- `bin/pos-cli-mcp-config.js:20` — reads and parses it again, then re-derives the enabled/disabled split with its own `cfg.enabled === false` loop and its own `config.tools || {}` fallback + +They have already diverged. The `add-ajv-input-validation` branch adds schema validation to `mcp-min/tools.js` only, so `pos-cli mcp config` will print a configuration that the MCP server then refuses to start with. Neither reader tells the user that a configured tool name does not match any registered tool. + +Extract one module (e.g. `mcp-min/config-loader.js`) that reads, validates against `mcp-min/tools.config.schema.json`, reports unknown tool names, and returns the resolved config plus its source. Both callers use it. This also removes the second copy of the enabled/disabled rule, so `pos-cli mcp config` can never disagree with what the server actually exposes. + +Depends on TASK-3 only in the sense that the validation and unknown-name checks it adds should live in the shared loader rather than be re-added here — coordinate ordering, or do this after TASK-3 lands and move its logic in. + + +## Acceptance Criteria + +- [ ] #1 One module owns reading, validating and resolving mcp-min/tools.config.json, and both mcp-min/tools.js and bin/pos-cli-mcp-config.js use it +- [ ] #2 pos-cli mcp config rejects a config that the MCP server would reject, with the same message +- [ ] #3 The enabled/disabled rule exists in exactly one place +- [ ] #4 pos-cli mcp config output still distinguishes the bundled default from an MCP_TOOLS_CONFIG override +- [ ] #5 Tests cover both callers against a valid config, an invalid config, and a missing file + diff --git a/backlog/tasks/task-7 - gui-next-log-search-builds-SQL-by-interpolating-the-users-filter-string.md b/backlog/tasks/task-7 - gui-next-log-search-builds-SQL-by-interpolating-the-users-filter-string.md new file mode 100644 index 00000000..54a967ab --- /dev/null +++ b/backlog/tasks/task-7 - gui-next-log-search-builds-SQL-by-interpolating-the-users-filter-string.md @@ -0,0 +1,56 @@ +--- +id: TASK-7 +title: gui/next log search builds SQL by interpolating the user's filter string +status: To Do +assignee: [] +created_date: '2026-09-02 10:23' +labels: + - security + - gui + - logs +dependencies: [] +references: + - gui/next/src/lib/api/logsv2.js + - gui/next/src/lib/api/network.js + - lib/server.js + - lib/validation/schemas/gui.js + - lib/proxy.js +priority: medium +ordinal: 22000 +--- + +## Description + + +Pre-existing on `master`, found while reviewing the Ajv validation branch. + +`gui/next/src/lib/api/logsv2.js:33` builds an OpenObserve SQL statement by string-interpolating the free-text search box straight into three `ILIKE` predicates: + +```js +if(filters.message){ + filters.sql = `SELECT * FROM logs where message ILIKE '%${filters.message}%' OR type ILIKE '%${filters.message}%' OR options_data_url ILIKE '%${filters.message}%'`; +} +``` + +Nothing escapes the value. A single quote in the search term terminates the literal, and everything after it is parsed as SQL. The statement is then POSTed to `/api/logsv2`, which `lib/server.js` forwards to `Gateway.logsv2` -> `this.client.searchSQL(params)`, so the crafted statement reaches OpenObserve with the developer's credentials attached. + +Two things make this worth fixing rather than shrugging at: + +- The GUI server sets `Access-Control-Allow-Origin: *` (`lib/server.js:97-101`), so `POST /api/logsv2` is reachable from any page open in the developer's browser while `pos-cli gui serve` is running, with an arbitrary `query.sql` — the client-side interpolation is the visible symptom, but the route accepts any statement regardless. +- The Ajv validation branch validates only the *top level* of the logsv2 payload (`query` must be an object or string). The real payload nests `sql`, `from`, `size`, `start_time`, `end_time` inside `query`, none of which are described, so the one place in this codebase with an actual injection surface is the one place validation does not reach. + +Decide the model first, because it changes the fix. If arbitrary SQL from the browser is intended (it is a local developer tool pointed at the developer's own instance, and the Liquid evaluator already carries a "code executed here runs on the connected instance" warning), then say so explicitly in a comment on the route and at least escape the quote in the filter so the UI's own search box cannot produce a malformed or surprising query. If it is not intended, the filter has to be bound rather than interpolated, and `logsSearchSchema` needs to describe the nested `query` object so `sql` is constrained. + +The same pattern appears in `gui/next/src/lib/api/network.js` for the network-log view. + +Note `gui/next/build` is committed and shipped in the npm package, so any client-side change needs `npm run build` in `gui/next` and the built assets committed. + + +## Acceptance Criteria + +- [ ] #1 A search term containing a single quote produces a well-formed query and cannot terminate the SQL string literal +- [ ] #2 The intended trust model for /api/logsv2 is recorded in a comment on the route in lib/server.js +- [ ] #3 If arbitrary SQL is not intended, logsSearchSchema describes the nested query object and constrains sql +- [ ] #4 The same treatment is applied to the network-log search in gui/next/src/lib/api/network.js +- [ ] #5 gui/next is rebuilt and the built assets committed if the client changed + diff --git a/backlog/tasks/task-8 - gui-next-log-date-filter-mixes-milliseconds-and-microseconds-so-the-range-is-wrong.md b/backlog/tasks/task-8 - gui-next-log-date-filter-mixes-milliseconds-and-microseconds-so-the-range-is-wrong.md new file mode 100644 index 00000000..e2e9b82a --- /dev/null +++ b/backlog/tasks/task-8 - gui-next-log-date-filter-mixes-milliseconds-and-microseconds-so-the-range-is-wrong.md @@ -0,0 +1,50 @@ +--- +id: TASK-8 +title: >- + gui/next log date filter mixes milliseconds and microseconds, so the range is + wrong +status: To Do +assignee: [] +created_date: '2026-09-02 10:23' +labels: + - bug + - gui + - logs +dependencies: [] +references: + - gui/next/src/lib/api/logsv2.js + - gui/next/src/lib/api/network.js +priority: low +ordinal: 23000 +--- + +## Description + + +Pre-existing on `master`, found while reviewing the Ajv validation branch. + +`gui/next/src/lib/api/logsv2.js:22-31` and the identical block in `gui/next/src/lib/api/network.js:23-30` compute the search window from the date picker: + +```js +let date = new Date(filters.start_time); +date.setHours(23, 59, 59); + +filters.end_time = Math.floor(date.getTime() * 1000); // microseconds +filters.start_time = Math.floor(date.getTime() - 24 * 60 * 60 * 1000 * 3); // milliseconds +``` + +`end_time` is converted to microseconds (which is what OpenObserve expects); `start_time` is left in milliseconds. It is therefore roughly a thousand times too small — around 1970 in microsecond terms — so the intended three-day window silently becomes "everything up to end_time". Picking a date does not narrow the result set the way the UI implies. + +Two smaller things in the same block, worth settling while it is open: `date` is mutated by `setHours` before `start_time` is derived from it, so "3 days back" is measured from end-of-day rather than start-of-day; and the fixed 3-day lookback is not exposed anywhere in the UI, so a user selecting one date gets four days of logs with no indication. + +`gui/next/build` is committed and shipped in the npm package, so this needs `npm run build` in `gui/next` and the built assets committed. + + +## Acceptance Criteria + +- [ ] #1 start_time and end_time are sent in the same unit that the logs backend expects +- [ ] #2 Selecting a date narrows the returned logs to the intended window, verified against a real instance or a recorded response +- [ ] #3 The window the date picker actually applies is either visible in the UI or documented in a comment +- [ ] #4 The identical block in gui/next/src/lib/api/network.js is fixed the same way +- [ ] #5 gui/next is rebuilt and the built assets committed + diff --git a/backlog/tasks/task-9 - modules.test.js-publishVersion-single-dir-workflow-test-times-out.md b/backlog/tasks/task-9 - modules.test.js-publishVersion-single-dir-workflow-test-times-out.md new file mode 100644 index 00000000..25cfbbc6 --- /dev/null +++ b/backlog/tasks/task-9 - modules.test.js-publishVersion-single-dir-workflow-test-times-out.md @@ -0,0 +1,47 @@ +--- +id: TASK-9 +title: 'modules.test.js: publishVersion single-dir workflow test times out' +status: To Do +assignee: [] +created_date: '2026-09-02 10:24' +labels: + - bug + - tests + - modules +dependencies: [] +references: + - test/unit/modules.test.js + - lib/modules.js + - .github/workflows/tests.yaml +priority: medium +ordinal: 24000 +--- + +## Description + + +Pre-existing failure on `master`. Confirmed not caused by the `add-ajv-input-validation` branch: reproduced identically in a clean `master` worktree with the same `node_modules`. + +``` +FAIL test/unit/modules.test.js > publishVersion() — pre-flight validation + > does not error about directory when modules/ does not exist (single-dir workflow) +Error: Test timed out in 10000ms. + at test/unit/modules.test.js:379 +``` + +Every other test in the file passes (27/28). The rest of `test/unit` is green (1153 passing) apart from this one, so it is the only thing standing between the repo and a clean `npm test`. + +The test writes a manifest with `machine_name` and `version` and no `modules/` directory at all, then expects `publishVersion` to get past the pre-flight directory check and reach archiving. The 10s timeout suggests the call is waiting on something rather than throwing — a prompt with no TTY, or a network call to the Partner Portal, are the obvious candidates given the test environment has no real credentials. + +Note this failure is invisible unless you run the suite: CI runs `npm test`, which runs the whole thing, so it should be failing there too — worth checking whether CI is currently red on `master` or whether something about the CI environment makes this test pass. Also note `npm run pretest` must have run (it installs the yeoman fixtures) or `test/unit/generators.test.js` fails separately for an unrelated reason, which is easy to confuse with this. + +Either fix the hang or, if the scenario is genuinely unsupported, change the test to assert what `publishVersion` should do instead. + + +## Acceptance Criteria + +- [ ] #1 npm run test:unit passes with no failures +- [ ] #2 The cause of the hang is identified and stated in the fix (prompt without TTY, unmocked network call, or similar) +- [ ] #3 If publishVersion cannot support the single-dir workflow, the test asserts the intended behaviour rather than being deleted or skipped +- [ ] #4 It is confirmed whether CI on master is currently failing on this test, and recorded in the task notes + diff --git a/bin/pos-cli-mcp.js b/bin/pos-cli-mcp.js index 2cba2566..cab79090 100755 --- a/bin/pos-cli-mcp.js +++ b/bin/pos-cli-mcp.js @@ -1,3 +1,17 @@ #!/usr/bin/env node -import '../mcp-min/index.js'; +import logger from '../lib/logger.js'; + +// The server is loaded dynamically so a configuration problem raised while its module +// graph evaluates can be reported as a message rather than escaping as a raw Node stack +// trace, per the error-handling guidance in CLAUDE.md. A static import would evaluate +// before any statement here could guard it. +try { + await import('../mcp-min/index.js'); +} catch (error) { + if (error?.name === 'ToolsConfigError') { + await logger.Error(error.message, { exit: false, notify: false, hideTimestamp: true }); + process.exit(1); + } + throw error; +} diff --git a/gui/next/src/lib/api/logs.js b/gui/next/src/lib/api/logs.js index d80ff267..f36af560 100644 --- a/gui/next/src/lib/api/logs.js +++ b/gui/next/src/lib/api/logs.js @@ -15,7 +15,9 @@ const logs = { // the URL to use to connect to the API, in development or preview mode we are using the default pos-cli gui serve port const url = (typeof window !== 'undefined' && window.location.port !== '4173' && window.location.port !== '5173') ? `http://localhost:${parseInt(window.location.port)}/api/logs` : 'http://localhost:3333/api/logs'; - const last = args.last ?? null; + // 0 is the "from the beginning" sentinel the API expects. Sending `null` here put the + // literal string "null" in the query, which the server rejects as a non-integer. + const last = args.last ?? 0; return fetch(`${url}?lastId=` + last) .then(response => { diff --git a/lib/proxy.js b/lib/proxy.js index 1f5002d8..187ba584 100644 --- a/lib/proxy.js +++ b/lib/proxy.js @@ -93,7 +93,17 @@ class Gateway { } logs(json, { signal } = {}) { - return apiRequest({ uri: `${this.api_url}/logs?last_id=${json.lastId}`, json: true, forever: true, headers: this.defaultHeaders, signal }); + // Encoded rather than interpolated raw: the cursor reaches this method from a GUI + // query string as well as from internal pollers, and an unencoded value could append + // its own parameters to the request. + const lastId = encodeURIComponent(json.lastId); + return apiRequest({ + uri: `${this.api_url}/logs?last_id=${lastId}`, + json: true, + forever: true, + headers: this.defaultHeaders, + signal + }); } logsv2(params) { diff --git a/lib/server.js b/lib/server.js index 755f25fa..e0f911c7 100644 --- a/lib/server.js +++ b/lib/server.js @@ -15,6 +15,14 @@ const upload = multer(); import Gateway from '../lib/proxy.js'; import logger from '../lib/logger.js'; +import { validate } from '../lib/validation/index.js'; +import { + graphqlRequestSchema, + liquidRequestSchema, + logsRequestSchema, + logsSearchSchema, + syncRequestSchema +} from '../lib/validation/schemas/gui.js'; const start = (env, client) => { const port = env.PORT || 3333; @@ -39,16 +47,38 @@ const start = (env, client) => { return res.status(status).json({ error: (typeof body === 'string' && body) || error?.message || 'Request failed' }); }; + // Rejects the request when `payload` does not match `schema`, and reports which field + // was at fault so the GUI shows something more useful than an upstream 500. + const rejectInvalid = (res, schema, payload, { mode = 'strict' } = {}) => { + const result = validate(schema, payload, { mode }); + if (result.valid) return false; + + // A schema that will not compile is our defect, not the caller's — report it as a + // server error, but still reject, since nothing was actually checked. + const status = result.schemaError ? 500 : 400; + + logger.Debug(`Rejected invalid request: ${result.message}`); + res.status(status).json({ error: `Invalid request: ${result.message}`, details: result.errors }); + return true; + }; + const graphqlRouting = (req, res) => { + if (rejectInvalid(res, graphqlRequestSchema, req.body)) return; + gateway .graph(req.body) .then(body => res.send(body)) .catch(error => sendError(res, error)); }; + // POST carries the template as a JSON body; the form in gui/liquid falls back to a GET + // with `?content=...`, so the payload has to be read from whichever the request used. const liquidRouting = (req, res) => { + const payload = req.method === 'GET' ? req.query : req.body; + if (rejectInvalid(res, liquidRequestSchema, payload)) return; + gateway - .liquid(req.body) + .liquid(payload) .then(body => res.send(body)) .catch(error => sendError(res, error)); }; @@ -85,15 +115,31 @@ const start = (env, client) => { app.get('/api/liquid', liquidRouting); app.get('/api/logs', (req, res) => { + const params = { ...req.query }; + + // Compatibility shim for GUI builds already installed in the wild. Versions of + // gui/next up to this change send `?lastId=null` on the first poll (it built the + // query with `args.last ?? null`), and Ajv will not coerce "null" to an integer. + // The source has been fixed to send 0, so this can go once shipping a rebuilt + // gui/next/build is acceptable — it exists only so an older GUI does not 400. + if (['null', 'undefined', ''].includes(params.lastId)) delete params.lastId; + + if (rejectInvalid(res, logsRequestSchema, params, { mode: 'coercing' })) return; + gateway - .logs({ lastId: req.query.lastId }) + .logs({ lastId: params.lastId }) .then(body => res.send(body)) .catch(error => sendError(res, error)); }); + // Coercing mode on both verbs: the GET route can only deliver strings, and a POST body + // that spells a numeric field as a string is normalised rather than rejected. app.get('/api/logsv2', (req, res) => { + const params = { ...req.query }; + if (rejectInvalid(res, logsSearchSchema, params, { mode: 'coercing' })) return; + gateway - .logsv2({ ...req.query }) + .logsv2(params) .then(body => { res.send(body); }) @@ -103,6 +149,8 @@ const start = (env, client) => { }); app.post('/api/logsv2', (req, res) => { + if (rejectInvalid(res, logsSearchSchema, req.body, { mode: 'coercing' })) return; + gateway .logsv2(req.body) .then(body => { @@ -121,9 +169,19 @@ const start = (env, client) => { '/api/app_builder/marketplace_releases/sync', upload.fields([{ name: 'path' }, { name: 'marketplace_builder_file_body' }]), (req, res) => { + if (rejectInvalid(res, syncRequestSchema, req.body)) return; + + // The upload arrives through multer rather than the JSON body, so it is guarded + // here instead of in the schema. Without this the missing-field case dereferenced + // undefined and surfaced as an unhandled 500. + const uploaded = req.files?.marketplace_builder_file_body?.[0]; + if (!uploaded) { + return res.status(400).json({ error: 'Invalid request: missing file field marketplace_builder_file_body' }); + } + const formData = { path: req.body.path, - marketplace_builder_file_body: req.files.marketplace_builder_file_body[0].buffer + marketplace_builder_file_body: uploaded.buffer }; gateway @@ -133,7 +191,8 @@ const start = (env, client) => { } ); - app + // Returned so callers (and tests) can shut the listener down. + return app .listen(port, host, function() { logger.Debug(`Server is listening on ${port}`); logger.Success(`Connected to ${env.MARKETPLACE_URL}`); diff --git a/lib/validation/index.js b/lib/validation/index.js new file mode 100644 index 00000000..188fbc2c --- /dev/null +++ b/lib/validation/index.js @@ -0,0 +1,117 @@ +import Ajv from 'ajv'; +import addFormats from 'ajv-formats'; + +/** + * Runtime input validation, backed by Ajv (JSON Schema draft-07). + * + * Every MCP tool in mcp-min/ already declares its `inputSchema` as JSON Schema, and + * that same object is advertised to clients verbatim in `tools/list`. Validating + * against it means the contract we publish and the contract we enforce are one + * object — there is no second definition that can drift out of sync. + */ + +// Two Ajv instances, because the two input shapes need different treatment: +// +// strict - JSON request bodies. Values arrive with real types, so nothing is +// rewritten and the caller's data is left untouched. +// coercing - query strings. Every value arrives as a string, so `?size=100` has to +// become a number before `{ type: 'integer' }` can accept it. Ajv applies +// coercion and defaults by mutating the validated object in place, which +// is what callers reading `req.query` afterwards want. +// strict mode stays on so a malformed schema fails loudly at compile time rather than +// silently validating nothing. `allowUnionTypes` is the one rule relaxed: fields that +// legitimately accept more than one type exist (a search payload that arrives as an +// object over POST and as a string over GET), and Ajv otherwise rejects the schema. +const buildInstance = options => { + const ajv = new Ajv({ allErrors: true, strict: true, allowUnionTypes: true, ...options }); + addFormats(ajv); + return { ajv, compiled: new WeakMap() }; +}; + +const instances = { + strict: buildInstance({}), + coercing: buildInstance({ coerceTypes: true, useDefaults: true }) +}; + +// Schemas are stable module-level objects, so a WeakMap keyed on the schema keeps one +// compiled validator per schema for the process lifetime without pinning it in memory. +// +// `true` and `false` are legal JSON Schema (accept everything / reject everything) but +// cannot key a WeakMap, so they compile fresh each call. Nothing in this repo uses a +// boolean schema; the branch exists so one does not surface as a phantom compile failure. +const validatorFor = (schema, instance) => { + if (schema === null || typeof schema !== 'object') return instance.ajv.compile(schema); + + let validator = instance.compiled.get(schema); + if (!validator) { + validator = instance.ajv.compile(schema); + instance.compiled.set(schema, validator); + } + return validator; +}; + +// Ajv's raw messages drop the detail that matters most for the two failures callers hit +// constantly: which property is missing, and which unknown one was rejected. +const describeError = error => { + const at = error.instancePath || '(root)'; + + switch (error.keyword) { + case 'required': + return `${at} is missing required property '${error.params.missingProperty}'`; + case 'additionalProperties': + return `${at} has unknown property '${error.params.additionalProperty}'`; + default: + return `${at} ${error.message}`; + } +}; + +const MESSAGE_ERROR_LIMIT = 5; + +const summarize = errors => { + const shown = errors.slice(0, MESSAGE_ERROR_LIMIT).map(error => error.message); + const hidden = errors.length - shown.length; + return hidden > 0 ? `${shown.join('; ')} (+${hidden} more)` : shown.join('; '); +}; + +/** + * Validate `data` against `schema`. + * + * @param {object|boolean} schema - JSON Schema (draft-07) + * @param {*} data - value to validate; with mode 'coercing' it is mutated in place + * @param {object} [options] + * @param {'strict'|'coercing'} [options.mode] - see the instances above + * @returns {{valid: boolean, errors?: Array<{path: string, message: string}>, + * message?: string, schemaError?: boolean}} + * `schemaError` marks a schema that failed to compile. That is a defect in our own + * schema rather than bad input, so callers should report it as a server-side error — + * but still reject the call, since an uncompilable schema means nothing was checked. + * @throws {RangeError} when `mode` names an instance that does not exist. That is a + * caller bug rather than a schema defect, so it must not be reported as `schemaError` + * and routed to a 500 that blames the schema. + */ +const validate = (schema, data, { mode = 'strict' } = {}) => { + const instance = instances[mode]; + if (!instance) { + throw new RangeError(`Unknown validation mode: ${mode} (expected ${Object.keys(instances).join(' or ')})`); + } + + let validator; + try { + validator = validatorFor(schema, instance); + } catch (err) { + const message = `Schema failed to compile: ${err.message}`; + return { valid: false, schemaError: true, message, errors: [{ path: '(schema)', message }] }; + } + + if (validator(data)) return { valid: true }; + + const errors = (validator.errors || []).map(error => ({ + path: error.instancePath || '(root)', + message: describeError(error) + })); + + return { valid: false, errors, message: summarize(errors) }; +}; + +export { validate, describeError }; +export default validate; diff --git a/lib/validation/schemas/gui.js b/lib/validation/schemas/gui.js new file mode 100644 index 00000000..0e59933f --- /dev/null +++ b/lib/validation/schemas/gui.js @@ -0,0 +1,85 @@ +/** + * Request schemas for the local GUI server (lib/server.js). + * + * These endpoints proxy straight through to the connected platformOS instance, and + * lib/server.js sets `Access-Control-Allow-Origin: *`, so any page open in the + * developer's browser can reach them while the GUI is running. Validating here rejects + * malformed input before it is forwarded with the user's API token attached. + * + * The schemas stay open (`additionalProperties: true`) on purpose. The pages that call + * these endpoints are pre-built bundles (GraphiQL in particular sends whatever its + * fetcher assembles), so pinning an exact property set would risk rejecting a legitimate + * field we cannot see from here. What is checked is what the handlers actually depend + * on: that the payload is an object and that its known fields have workable types. + */ + +const graphqlRequestSchema = { + type: 'object', + additionalProperties: true, + required: ['query'], + properties: { + query: { type: 'string', minLength: 1 }, + variables: { type: ['object', 'null'] }, + operationName: { type: ['string', 'null'] } + } +}; + +const liquidRequestSchema = { + type: 'object', + additionalProperties: true, + required: ['content'], + properties: { + content: { type: 'string' } + } +}; + +// Mirrors what Gateway.logsv2 branches on (`query`, then `key`, then a plain SQL search) +// and what swagger-client's buildQuery/searchAround read off the params. +// +// `query` accepts a string as well as an object because that branch forwards the whole +// params object as the request body, and the GET route can only ever deliver a string. +const logsSearchSchema = { + type: 'object', + additionalProperties: true, + properties: { + query: { type: ['object', 'string'] }, + key: { type: 'string' }, + stream_name: { type: 'string' }, + sql: { type: 'string' }, + size: { type: 'integer', minimum: 0 }, + from: { type: 'integer', minimum: 0 }, + start_time: { type: 'integer' }, + end_time: { type: 'integer' } + } +}; + +// `lastId` is a log row id (`row.id`, seeded from 0 — see lib/test-runner/logStream.js) +// and Gateway.logs interpolates it straight into the request URL, so constraining it to an +// integer is what stops a caller from appending their own query parameters. +// +// `default: 0` matters: 0 is the "from the beginning" sentinel logStream.js already uses, +// so a first poll that sends no cursor resolves to it instead of interpolating the string +// "undefined" into the upstream URL, which is what happened before this schema existed. +// +// The same field is declared `integer` here but `string` on the MCP logs tools, which pass +// it through without coercion; both are normalised to integer so the cursor has one type +// across the codebase. +const logsRequestSchema = { + type: 'object', + additionalProperties: true, + properties: { + lastId: { type: 'integer', minimum: 0, default: 0 } + } +}; + +// The file itself arrives via multer, not JSON, so only `path` is schema-checkable here. +const syncRequestSchema = { + type: 'object', + additionalProperties: true, + required: ['path'], + properties: { + path: { type: 'string', minLength: 1 } + } +}; + +export { graphqlRequestSchema, liquidRequestSchema, logsRequestSchema, logsSearchSchema, syncRequestSchema }; diff --git a/mcp-min/__tests__/constants.test.js b/mcp-min/__tests__/constants.test.js index 129ea549..4857e1d0 100644 --- a/mcp-min/__tests__/constants.test.js +++ b/mcp-min/__tests__/constants.test.js @@ -88,8 +88,12 @@ describe('constants-list', () => { expect(res.error.message).toBe('Unauthorized'); }); + // `env` is optional across these tools: resolveAuth also accepts explicit + // url/email/token, MPKIT_* env vars, or the first .pos entry. test('has correct schema', () => { - expect(constantsListTool.inputSchema.required).toContain('env'); + expect(constantsListTool.inputSchema.required).toBeUndefined(); + expect(constantsListTool.inputSchema.properties).toHaveProperty('env'); + expect(constantsListTool.inputSchema.properties).toHaveProperty('token'); }); }); @@ -183,9 +187,8 @@ describe('constants-set', () => { }); test('has correct schema', () => { - expect(constantsSetTool.inputSchema.required).toContain('env'); - expect(constantsSetTool.inputSchema.required).toContain('name'); - expect(constantsSetTool.inputSchema.required).toContain('value'); + expect(constantsSetTool.inputSchema.required).toEqual(['name', 'value']); + expect(constantsSetTool.inputSchema.properties).toHaveProperty('env'); }); }); @@ -259,7 +262,7 @@ describe('constants-unset', () => { }); test('has correct schema', () => { - expect(constantsUnsetTool.inputSchema.required).toContain('env'); - expect(constantsUnsetTool.inputSchema.required).toContain('name'); + expect(constantsUnsetTool.inputSchema.required).toEqual(['name']); + expect(constantsUnsetTool.inputSchema.properties).toHaveProperty('env'); }); }); diff --git a/mcp-min/__tests__/data.import.test.js b/mcp-min/__tests__/data.import.test.js index 0c867bc3..77404cb0 100644 --- a/mcp-min/__tests__/data.import.test.js +++ b/mcp-min/__tests__/data.import.test.js @@ -23,7 +23,9 @@ describe('data-import tool', () => { expect(dataImportTool.inputSchema.properties).toHaveProperty('filePath'); expect(dataImportTool.inputSchema.properties).toHaveProperty('jsonData'); expect(dataImportTool.inputSchema.properties).toHaveProperty('zipFileUrl'); - expect(dataImportTool.inputSchema.required).toContain('env'); + // `env` is optional: resolveAuth also accepts url/email/token or MPKIT_* env vars. + expect(dataImportTool.inputSchema.required).toBeUndefined(); + expect(dataImportTool.inputSchema.properties).toHaveProperty('token'); }); test('returns error when env not found', async () => { @@ -326,7 +328,7 @@ describe('data-import tool', () => { test('has correct description and inputSchema', () => { expect(dataImportStatusTool.description).toContain('status'); expect(dataImportStatusTool.inputSchema.properties).toHaveProperty('jobId'); - expect(dataImportStatusTool.inputSchema.required).toContain('jobId'); + expect(dataImportStatusTool.inputSchema.required).toEqual(['jobId']); }); test('returns validation error when jobId not provided', async () => { diff --git a/mcp-min/__tests__/tools-config-validation.test.js b/mcp-min/__tests__/tools-config-validation.test.js new file mode 100644 index 00000000..ae6b4eab --- /dev/null +++ b/mcp-min/__tests__/tools-config-validation.test.js @@ -0,0 +1,174 @@ +/** + * tools.config.json decides which tools the MCP server exposes, so a config that does not + * match tools.config.schema.json must stop the server rather than be ignored — ignoring it + * would silently re-enable every tool the author meant to switch off. + * + * Each case runs in its own process because tools.js reads the config once, at import. + */ +import { spawnSync } from 'child_process'; +import fs from 'fs'; +import os from 'os'; +import path from 'path'; +import { fileURLToPath } from 'url'; +import { describe, test, expect, beforeAll, afterAll } from 'vitest'; + +const here = path.dirname(fileURLToPath(import.meta.url)); +const repoRoot = path.resolve(here, '..', '..'); + +let tmpDir; + +const write = (name, contents) => { + const file = path.join(tmpDir, name); + fs.writeFileSync(file, contents); + return file; +}; + +// Reports what `import('./mcp-min/tools.js')` did under the given config. +const loadWithConfig = configPath => { + const script = + "import('./mcp-min/tools.js')" + + ".then(m => console.log('OK:' + Object.keys(m.default).join(',')))" + + ".catch(e => { console.log('REJECTED:' + e.message); process.exitCode = 1; })"; + + const result = spawnSync(process.execPath, ['--input-type=module', '-e', script], { + cwd: process.cwd(), + env: { ...process.env, MCP_TOOLS_CONFIG: configPath }, + encoding: 'utf8' + }); + + return { stdout: result.stdout || '', status: result.status }; +}; + +beforeAll(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'pos-cli-tools-config-')); +}); + +afterAll(() => { + if (tmpDir) fs.rmSync(tmpDir, { recursive: true, force: true }); +}); + +describe('tools config validation', () => { + test('refuses to start when a config field has the wrong type', () => { + const configPath = write('wrong-type.json', JSON.stringify({ tools: { 'envs-list': { enabled: 'yes' } } })); + const { stdout, status } = loadWithConfig(configPath); + + expect(stdout).toContain('REJECTED:'); + expect(stdout).toContain('/tools/envs-list/enabled must be boolean'); + expect(status).not.toBe(0); + }); + + test('refuses to start when the config is not an object', () => { + const configPath = write('array.json', JSON.stringify(['envs-list'])); + const { stdout } = loadWithConfig(configPath); + + expect(stdout).toContain('REJECTED:'); + }); + + // These parse as valid JSON but are not configs. Testing the parsed value for + // truthiness would skip validation and fall through to defaults with everything on. + // + // The assertion names the schema violation deliberately: a bare `null` also crashes + // applyConfig with a TypeError, so asserting only that startup failed would pass even + // with the fail-closed check removed. + test.each(['null', 'false', '0', '""'])('refuses to start on a bare %s config', literal => { + const configPath = write(`falsy-${literal.replace(/\W/g, '_')}.json`, literal); + const { stdout, status } = loadWithConfig(configPath); + + expect(stdout).toContain('REJECTED:'); + expect(stdout).toContain('Invalid tools config'); + expect(stdout).toContain('must be object'); + expect(stdout).not.toContain('TypeError'); + expect(status).not.toBe(0); + }); + + // The schema constrains each entry's shape but cannot enumerate tool names, so this + // is the case it structurally cannot catch: a typo leaves the real tool enabled while + // the config looks like it took effect. + test('refuses to start when a config names a tool that does not exist', () => { + const configPath = write('typo.json', JSON.stringify({ tools: { 'deploy-strt': { enabled: false } } })); + const { stdout, status } = loadWithConfig(configPath); + + expect(stdout).toContain('no such tool: deploy-strt'); + expect(status).not.toBe(0); + }); + + test('names every unknown tool, not just the first', () => { + const configPath = write('typos.json', JSON.stringify({ + tools: { 'deploy-strt': { enabled: false }, 'constants-lst': { enabled: false } } + })); + const { stdout } = loadWithConfig(configPath); + + expect(stdout).toContain('deploy-strt'); + expect(stdout).toContain('constants-lst'); + }); + + // `name in registry` would accept these, because `in` walks the prototype chain — they + // would then match nothing in applyConfig and be silently ignored. + test.each(['toString', 'constructor', 'hasOwnProperty', 'valueOf'])( + 'refuses a config keyed by the inherited property %s', + key => { + const configPath = write(`proto-${key}.json`, JSON.stringify({ tools: { [key]: { enabled: false } } })); + const { stdout, status } = loadWithConfig(configPath); + + expect(stdout).toContain(`no such tool: ${key}`); + expect(status).not.toBe(0); + } + ); + + test('accepts a config naming only real tools', () => { + const configPath = write('real.json', JSON.stringify({ + tools: { 'deploy-start': { description: 'Ship it' } } + })); + const { stdout, status } = loadWithConfig(configPath); + + expect(status).toBe(0); + expect(stdout).toContain('deploy-start'); + }); + + test('applies a valid config and disables the named tool', () => { + const configPath = write('valid.json', JSON.stringify({ tools: { 'envs-list': { enabled: false } } })); + const { stdout, status } = loadWithConfig(configPath); + + expect(status).toBe(0); + expect(stdout).toContain('OK:'); + expect(stdout).not.toContain('envs-list'); + }); + + test('falls back to defaults when the config file does not exist', () => { + const { stdout, status } = loadWithConfig(path.join(tmpDir, 'absent.json')); + + expect(status).toBe(0); + expect(stdout).toContain('envs-list'); + }); + + // CLAUDE.md requires user-facing errors to go through the logger. A module-loader + // stack trace reaching the terminal is the failure mode this guards. + test('reports an invalid config as a message, not a Node stack trace', () => { + const configPath = write('for-cli.json', JSON.stringify({ tools: { 'envs-list': { enabled: 'yes' } } })); + + const result = spawnSync(process.execPath, [path.join(here, '..', '..', 'bin', 'pos-cli-mcp.js')], { + cwd: repoRoot, + env: { ...process.env, MCP_TOOLS_CONFIG: configPath }, + encoding: 'utf8', + timeout: 30000 + }); + + const output = `${result.stdout || ''}${result.stderr || ''}`; + + expect(result.status).toBe(1); + expect(output).toContain('Invalid tools config'); + expect(output).toContain('must be boolean'); + expect(output).not.toMatch(/^\s+at .+:\d+:\d+\)?$/m); // no stack frames + expect(output).not.toContain('node:internal'); + }); + + test('the config shipped in the package satisfies its own schema', () => { + const schema = JSON.parse(fs.readFileSync(path.join(here, '..', 'tools.config.schema.json'), 'utf8')); + const config = JSON.parse(fs.readFileSync(path.join(here, '..', 'tools.config.json'), 'utf8')); + + // Imported here rather than at the top so this file stays runnable in isolation. + return import('../../lib/validation/index.js').then(({ validate }) => { + expect(validate(schema, config).errors ?? []).toEqual([]); + }); + }); +}); diff --git a/mcp-min/__tests__/transport-validation.test.js b/mcp-min/__tests__/transport-validation.test.js new file mode 100644 index 00000000..f294e36b --- /dev/null +++ b/mcp-min/__tests__/transport-validation.test.js @@ -0,0 +1,291 @@ +/** + * Both MCP transports must reject params that do not match the schema they advertise in + * tools/list, before the params ever reach a handler. + */ +import http from 'http'; +import { spawn } from 'child_process'; +import { resolve, dirname } from 'path'; +import { fileURLToPath, pathToFileURL } from 'url'; +import { describe, test, expect, beforeAll, afterAll } from 'vitest'; +import startHttp from '../http-server.js'; +import tools from '../tools.js'; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const repoRoot = resolve(__dirname, '..', '..'); +const stdioScript = resolve(__dirname, '..', 'stdio-server.js'); + +// A schema Ajv cannot compile, registered as a tool so the schemaError branch — which +// maps to 500 / -32603 rather than the caller-blaming 400 / -32602 — is reachable. +const BROKEN_SCHEMA_TOOL = { + description: 'Test-only tool whose schema does not compile', + inputSchema: { type: 'not-a-real-type' }, + handler: async () => ({ ok: true, data: { reached: 'handler' } }) +}; + +let server; +let PORT; + +const post = (path, body) => + new Promise((resolvePromise, reject) => { + const req = http.request( + { hostname: '127.0.0.1', port: PORT, path, method: 'POST', headers: { 'Content-Type': 'application/json' } }, + res => { + let data = ''; + res.on('data', chunk => (data += chunk)); + res.on('end', () => + resolvePromise({ status: res.statusCode, headers: res.headers, body: data ? JSON.parse(data) : null }) + ); + } + ); + req.on('error', reject); + req.write(JSON.stringify(body)); + req.end(); + }); + +// No .pos fixture on purpose: every assertion here is about params being rejected before +// a handler runs, and writing .pos into the working directory races other suites. +beforeAll(async () => { + // The registry is a live object shared with http-server, so adding an entry here is + // visible to the running server. Port 0 lets the OS assign a free one, which removes + // the collision this suite previously risked with a hardcoded 5931. + tools['broken-schema'] = BROKEN_SCHEMA_TOOL; + server = await startHttp({ port: 0 }); + PORT = server.address().port; +}); + +afterAll(() => { + delete tools['broken-schema']; + if (server) server.close(); +}); + + +// Drives one request through a freshly spawned stdio server and resolves with the +// response carrying the same id. `injectBrokenTool` starts the server from an inline +// module that adds the uncompilable-schema tool to the live registry first — the server +// reads tools[name] per request, so a mutation before startStdio() is visible. +const runStdio = (message, { injectBrokenTool = false } = {}) => new Promise((done, reject) => { + const inline = [ + `import tools from ${JSON.stringify(pathToFileURL(resolve(__dirname, '..', 'tools.js')).href)};`, + `import startStdio from ${JSON.stringify(pathToFileURL(stdioScript).href)};`, + `tools['broken-schema'] = { inputSchema: { type: 'not-a-real-type' }, handler: async () => ({ ok: true }) };`, + 'startStdio();' + ].join('\n'); + + const child = injectBrokenTool + ? spawn(process.execPath, ['--input-type=module', '-e', inline], { cwd: repoRoot, stdio: 'pipe' }) + : spawn(process.execPath, [stdioScript], { cwd: repoRoot, stdio: 'pipe' }); + + let buffered = ''; + let sent = false; + let settled = false; + + const finish = (fn, value) => { + if (settled) return; + settled = true; + child.kill(); + fn(value); + }; + + child.stdout.on('data', chunk => { + buffered += chunk.toString(); + + if (!sent && buffered.includes('protocolVersion')) { + sent = true; + child.stdin.write(`${JSON.stringify(message)}\n`); + return; + } + + for (const line of buffered.split('\n')) { + if (!line.trim()) continue; + let parsed; + try { + parsed = JSON.parse(line); + } catch { + continue; + } + if (parsed.id === message.id) return finish(done, parsed); + } + }); + + child.on('error', err => finish(reject, err)); + + child.stdin.write(`${JSON.stringify({ + jsonrpc: '2.0', + id: 'init', + method: 'initialize', + params: { protocolVersion: '2024-11-05', capabilities: {} } + })}\n`); +}); + +describe('HTTP POST /call', () => { + test('rejects a missing required param with 400', async () => { + const res = await post('/call', { tool: 'constants-set', params: { env: 'staging' } }); + + expect(res.status).toBe(400); + expect(res.body.error).toContain("missing required property 'name'"); + expect(res.body.details).toBeInstanceOf(Array); + }); + + test('rejects an unknown param with 400', async () => { + const res = await post('/call', { tool: 'envs-list', params: {} }); + expect(res.status).toBe(200); + + const rejected = await post('/call', { tool: 'constants-list', params: { env: 'staging', wat: 1 } }); + expect(rejected.status).toBe(400); + expect(rejected.body.error).toContain("unknown property 'wat'"); + }); + + test('rejects a param of the wrong type with 400', async () => { + const res = await post('/call', { tool: 'logs-fetch', params: { limit: 'all' } }); + + expect(res.status).toBe(400); + expect(res.body.error).toContain('/limit'); + }); + + test('still accepts params that match the schema', async () => { + const res = await post('/call', { tool: 'envs-list', params: {} }); + + expect(res.status).toBe(200); + expect(res.body.result.ok).toBe(true); + }); +}); + +describe('HTTP JSON-RPC tools/call', () => { + test('rejects invalid params with -32602', async () => { + const res = await post('/call-stream', { + jsonrpc: '2.0', + id: 7, + method: 'tools/call', + params: { name: 'constants-set', arguments: { env: 'staging' } } + }); + + expect(res.status).toBe(200); + expect(res.body.error.code).toBe(-32602); + expect(res.body.error.message).toContain("missing required property 'name'"); + }); + + test('accepts valid params', async () => { + const res = await post('/call-stream', { + jsonrpc: '2.0', + id: 8, + method: 'tools/call', + params: { name: 'envs-list', arguments: {} } + }); + + expect(res.body.error).toBeUndefined(); + expect(res.body.result.content).toBeDefined(); + }); +}); + +describe('stdio tools/call', () => { + test('rejects invalid params with -32602', async () => { + const response = await runStdio({ + jsonrpc: '2.0', + id: 2, + method: 'tools/call', + params: { name: 'constants-set', arguments: { env: 'staging' } } + }); + + expect(response.error.code).toBe(-32602); + expect(response.error.message).toContain("missing required property 'name'"); + }, 20000); + + test('accepts valid params', async () => { + const response = await runStdio({ + jsonrpc: '2.0', + id: 6, + method: 'tools/call', + params: { name: 'envs-list', arguments: {} } + }); + + expect(response.error).toBeUndefined(); + expect(response.result.content).toBeDefined(); + }, 20000); +}); + +// A schema that will not compile is our defect, not the caller's, so it must not be +// reported as 400 / -32602. The mapping lives in one place (rejectionFor); these cover +// each transport path that consumes it. +describe('uncompilable schema is reported as a server error', () => { + test('HTTP POST /call answers 500, not 400', async () => { + const res = await post('/call', { tool: 'broken-schema', params: {} }); + + expect(res.status).toBe(500); + expect(res.body.error).toContain('Schema failed to compile'); + expect(JSON.stringify(res.body)).not.toContain('reached'); // handler never ran + }); + + test('HTTP JSON-RPC tools/call answers -32603, not -32602', async () => { + const res = await post('/call-stream', { + jsonrpc: '2.0', + id: 21, + method: 'tools/call', + params: { name: 'broken-schema', arguments: {} } + }); + + expect(res.body.error.code).toBe(-32603); + expect(res.body.error.message).toContain('Schema failed to compile'); + }); + + test('HTTP /call-stream legacy path answers 500 before opening the stream', async () => { + const res = await post('/call-stream', { tool: 'broken-schema', params: {} }); + + expect(res.status).toBe(500); + expect(res.body.error).toContain('Schema failed to compile'); + }); + + test('stdio tools/call answers -32603', async () => { + const response = await runStdio( + { jsonrpc: '2.0', id: 2, method: 'tools/call', params: { name: 'broken-schema', arguments: {} } }, + { injectBrokenTool: true } + ); + + expect(response.error.code).toBe(-32603); + expect(response.error.message).toContain('Schema failed to compile'); + }); +}); + +// The legacy path invokes a tool by naming it as the JSON-RPC method directly, and +// answers non-JSON-RPC callers with a bare { id, error } instead of an error object. +describe('stdio legacy direct invocation', () => { + test('rejects invalid params with -32602 for a JSON-RPC caller', async () => { + const response = await runStdio({ jsonrpc: '2.0', id: 3, method: 'constants-set', params: { env: 'staging' } }); + + expect(response.error.code).toBe(-32602); + expect(response.error.message).toContain("missing required property 'name'"); + }); + + test('rejects invalid params with a bare error string for a non-JSON-RPC caller', async () => { + const response = await runStdio({ id: 4, method: 'constants-set', params: { env: 'staging' } }); + + expect(typeof response.error).toBe('string'); + expect(response.error).toContain('Invalid params'); + expect(response.result).toBeUndefined(); + }); + + test('accepts valid params on the legacy path', async () => { + const response = await runStdio({ jsonrpc: '2.0', id: 5, method: 'envs-list', params: {} }); + + expect(response.error).toBeUndefined(); + expect(response.result.ok).toBe(true); + }); +}); + +// HTTP /call-stream legacy streaming path: validation runs before the SSE handshake, so +// a rejection is still an ordinary JSON response with a status code. +describe('HTTP /call-stream legacy streaming', () => { + test('rejects invalid params with 400 before the stream opens', async () => { + const res = await post('/call-stream', { tool: 'constants-set', params: { env: 'staging' } }); + + expect(res.status).toBe(400); + expect(res.body.error).toContain("missing required property 'name'"); + expect(res.headers['content-type']).toMatch(/application\/json/); + }); + + test('rejects an unknown param with 400', async () => { + const res = await post('/call-stream', { tool: 'constants-list', params: { env: 'staging', nope: 1 } }); + + expect(res.status).toBe(400); + expect(res.body.error).toContain("unknown property 'nope'"); + }); +}); diff --git a/mcp-min/__tests__/uploads.push.test.js b/mcp-min/__tests__/uploads.push.test.js index 66f19cf0..5fa216cd 100644 --- a/mcp-min/__tests__/uploads.push.test.js +++ b/mcp-min/__tests__/uploads.push.test.js @@ -93,8 +93,8 @@ describe('uploads-push', () => { test('has correct description and schema with required fields', () => { expect(uploadsTool.description).toContain('ZIP'); - expect(uploadsTool.inputSchema.required).toContain('env'); - expect(uploadsTool.inputSchema.required).toContain('filePath'); + // `env` is optional: resolveAuth also accepts url/email/token or MPKIT_* env vars. + expect(uploadsTool.inputSchema.required).toEqual(['filePath']); expect(uploadsTool.inputSchema.properties.env).toBeDefined(); expect(uploadsTool.inputSchema.properties.filePath).toBeDefined(); }); diff --git a/mcp-min/__tests__/validate-params.test.js b/mcp-min/__tests__/validate-params.test.js new file mode 100644 index 00000000..3f832db1 --- /dev/null +++ b/mcp-min/__tests__/validate-params.test.js @@ -0,0 +1,210 @@ +import { describe, test, expect } from 'vitest'; +import fs from 'fs'; +import path from 'path'; +import { fileURLToPath, pathToFileURL } from 'url'; +import fg from 'fast-glob'; +import tools from '../tools.js'; +import { validateToolParams } from '../validate-params.js'; + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..'); + +const authFileList = fg + .sync('mcp-min/**/*.js', { cwd: repoRoot, absolute: true, ignore: ['**/__tests__/**', '**/node_modules/**'] }) + .filter(file => !file.endsWith(`${path.sep}auth.js`)) + .filter(file => /\bresolveAuth\(/.test(fs.readFileSync(file, 'utf8'))); + +// Imported once at module scope so test.each can be built from real tool objects. +const authTools = await Promise.all( + authFileList.map(file => import(pathToFileURL(file).href).then(mod => mod.default)) +); + +const check = (name, params) => validateToolParams(name, tools[name], params); + +describe('tool input schemas', () => { + test('every registered tool has a schema Ajv can compile', () => { + const uncompilable = Object.entries(tools) + .filter(([name, tool]) => validateToolParams(name, tool, {}).schemaError) + .map(([name]) => name); + + expect(uncompilable).toEqual([]); + }); + + test('every registered tool declares an object schema', () => { + for (const [name, tool] of Object.entries(tools)) { + expect(tool.inputSchema?.type, `${name} inputSchema.type`).toBe('object'); + } + }); +}); + +describe('validateToolParams', () => { + test('accepts params that match the schema', () => { + expect(check('constants-set', { env: 'staging', name: 'API_KEY', value: 'x' }).valid).toBe(true); + }); + + test('rejects missing required params', () => { + const result = check('constants-set', { env: 'staging' }); + expect(result.valid).toBe(false); + expect(result.message).toContain("missing required property 'name'"); + }); + + test('rejects unknown params on a closed schema', () => { + const result = check('constants-list', { env: 'staging', dropTable: true }); + expect(result.valid).toBe(false); + expect(result.message).toContain("unknown property 'dropTable'"); + }); + + test('rejects a param of the wrong type', () => { + const result = check('logs-fetch', { limit: 'all' }); + expect(result.valid).toBe(false); + expect(result.errors[0].path).toBe('/limit'); + }); + + test('rejects a param outside its declared range', () => { + expect(check('logs-fetch', { limit: 999999 }).valid).toBe(false); + }); + + test('rejects params that are not an object at all', () => { + expect(check('constants-list', 'staging').valid).toBe(false); + expect(check('constants-list', ['staging']).valid).toBe(false); + }); + + test('treats absent params as an empty object', () => { + expect(check('envs-list', undefined).valid).toBe(true); + expect(check('constants-set', undefined).valid).toBe(false); + }); +}); + +// resolveAuth (mcp-min/auth.js) resolves credentials in this order: explicit +// url+email+token params, then the named `.pos` environment, then MPKIT_* env vars, then +// the first `.pos` entry. A schema that made `env` mandatory would reject three of those +// four supported call styles. +describe('authentication params stay accepted', () => { + // Derived from the source rather than hand-listed: a tool added later is covered the + // moment it calls resolveAuth. A hand-written list silently stopped guarding tools it + // did not happen to name. + const authenticatingFiles = authFileList; + + test('the scan finds the authenticating tools', () => { + expect(authenticatingFiles.length).toBeGreaterThanOrEqual(20); + }); + + test.each(authenticatingFiles.map((file, i) => [path.relative(repoRoot, file), i]))( + '%s declares url, email and token on a closed schema', + (_label, index) => { + const schema = authTools[index]?.inputSchema; + + // Only closed schemas can reject unknown properties, so only they can make the + // explicit-credentials path unreachable by omitting these three. + if (!schema || schema.additionalProperties !== false) return; + + for (const property of ['url', 'email', 'token']) { + expect(Object.keys(schema.properties || {}), `${_label} inputSchema.properties`) + .toContain(property); + } + } + ); + + const requiredExtras = { + 'constants-set': { name: 'A', value: '1' }, + 'constants-unset': { name: 'A' }, + 'data-import-status': { jobId: '1' }, + 'uploads-push': { filePath: 'uploads.zip' }, + 'unit-tests-run': { name: 'example_test' }, + 'deploy-status': { id: '1' }, + 'deploy-wait': { id: '1' }, + 'data-export-status': { jobId: '1' }, + 'data-clean-status': { jobId: '1' }, + 'data-clean': { confirmation: 'yes' }, + 'tests-run-async-result': { id: '1' }, + 'migrations-generate': { name: 'add_thing' }, + 'liquid-exec': { template: '{{ 1 }}' }, + 'graphql-exec': { query: '{ a }' }, + 'sync-file': { filePath: 'app/views/a.liquid' } + }; + + // Registry entries whose schema is the one exported by an authenticating file. Matched + // on the inputSchema object rather than the tool object, because applyConfig copies the + // tool when a config overrides its description but keeps the same schema reference. + // A name-based heuristic would wrongly sweep in portal tools like env-add, whose + // `token` parameter is data it sends rather than credentials it authenticates with. + const authSchemas = new Set(authTools.map(tool => tool?.inputSchema).filter(Boolean)); + const registeredAuthTools = Object.keys(tools).filter(name => authSchemas.has(tools[name].inputSchema)); + + test.each(registeredAuthTools)('%s accepts explicit url/email/token without env', name => { + const params = { url: 'https://example.com', email: 'a@b.c', token: 'tok', ...requiredExtras[name] }; + const result = check(name, params); + + expect(result.errors ?? []).toEqual([]); + expect(result.valid).toBe(true); + }); + + test.each(registeredAuthTools)('%s accepts env alone', name => { + expect(check(name, { env: 'staging', ...requiredExtras[name] }).valid).toBe(true); + }); + + test.each(registeredAuthTools)('%s accepts no auth params (MPKIT_* / default .pos entry)', name => { + expect(check(name, { ...requiredExtras[name] }).valid).toBe(true); + }); +}); + +// The branch relaxed `required` on these two so the schema matches what the handler +// actually needs; without an assertion the relaxation could be reverted unnoticed. +describe('required relaxations', () => { + test('data-validate requires nothing: validation runs locally and env is context only', () => { + expect(tools['data-validate'].inputSchema.required).toBeUndefined(); + }); + + test('unit-tests-run requires only name', () => { + expect(tools['unit-tests-run'].inputSchema.required).toEqual(['name']); + }); + + test.each([ + ['constants-list', undefined], + ['constants-set', ['name', 'value']], + ['constants-unset', ['name']], + ['data-import', undefined], + ['data-import-status', ['jobId']], + ['uploads-push', ['filePath']] + ])('%s no longer requires env', (name, expected) => { + expect(tools[name].inputSchema.required).toEqual(expected); + }); +}); + +// logs-fetch documents `lastId` as the cursor to hand back on the next call, so what it +// returns has to satisfy the schema it accepts. It previously returned a string while the +// schema demanded an integer, which broke paging with -32602. +describe('logs-fetch cursor round-trips', () => { + test('the returned cursor is accepted as the next request cursor', async () => { + const rows = [{ id: 41, message: 'a' }, { id: 42, message: 'b' }]; + let call = 0; + class MockGateway { + async logs() { + call += 1; + return { logs: call === 1 ? rows : [] }; + } + } + + const result = await tools['logs-fetch'].handler( + { url: 'https://example.com', email: 'a@b.c', token: 'tok' }, + { Gateway: MockGateway } + ); + + expect(result.ok).toBe(true); + expect(result.lastId).toBe(42); + expect(check('logs-fetch', { lastId: result.lastId }).valid).toBe(true); + }); + + test('the default cursor is also a valid next cursor', async () => { + class MockGateway { + async logs() { return { logs: [] }; } + } + + const result = await tools['logs-fetch'].handler( + { url: 'https://example.com', email: 'a@b.c', token: 'tok' }, + { Gateway: MockGateway } + ); + + expect(result.lastId).toBe(0); + expect(check('logs-fetch', { lastId: result.lastId }).valid).toBe(true); + }); +}); diff --git a/mcp-min/constants/list.js b/mcp-min/constants/list.js index a561a88b..a146af84 100644 --- a/mcp-min/constants/list.js +++ b/mcp-min/constants/list.js @@ -3,15 +3,16 @@ import { resolveAuth } from '../auth.js'; import Gateway from '../../lib/proxy.js'; import { getConstants } from '../../lib/graph/queries.js'; import { graphQLErrorMessage } from '../../lib/graph/response.js'; +import { authProperties } from '../schemas/auth.js'; const constantsListTool = { description: 'List all constants configured on a platformOS instance.', inputSchema: { type: 'object', additionalProperties: false, - required: ['env'], properties: { - env: { type: 'string', description: 'Environment name from .pos config' } + env: { type: 'string', description: 'Environment name from .pos config' }, + ...authProperties } }, handler: async (params, ctx = {}) => { diff --git a/mcp-min/constants/set.js b/mcp-min/constants/set.js index 874d13fa..6b0d2779 100644 --- a/mcp-min/constants/set.js +++ b/mcp-min/constants/set.js @@ -3,15 +3,17 @@ import { resolveAuth } from '../auth.js'; import Gateway from '../../lib/proxy.js'; import { setConstant } from '../../lib/graph/queries.js'; import { graphQLErrorMessage } from '../../lib/graph/response.js'; +import { authProperties } from '../schemas/auth.js'; const constantsSetTool = { - description: 'Set a constant on a platformOS instance. Creates or updates the constant.', + description: 'Set a constant on a platformOS instance. Creates or updates the constant. Omitting env (and url/email/token) targets the first environment in .pos, so name the environment explicitly.', inputSchema: { type: 'object', additionalProperties: false, - required: ['env', 'name', 'value'], + required: ['name', 'value'], properties: { env: { type: 'string', description: 'Environment name from .pos config' }, + ...authProperties, name: { type: 'string', description: 'Name of the constant (e.g., API_KEY)' }, value: { type: 'string', description: 'Value of the constant' } } diff --git a/mcp-min/constants/unset.js b/mcp-min/constants/unset.js index 5e9963bb..3ec7b1f4 100644 --- a/mcp-min/constants/unset.js +++ b/mcp-min/constants/unset.js @@ -3,15 +3,17 @@ import { resolveAuth } from '../auth.js'; import Gateway from '../../lib/proxy.js'; import { unsetConstant } from '../../lib/graph/queries.js'; import { graphQLErrorMessage } from '../../lib/graph/response.js'; +import { authProperties } from '../schemas/auth.js'; const constantsUnsetTool = { - description: 'Delete a constant from a platformOS instance.', + description: 'Delete a constant from a platformOS instance. Omitting env (and url/email/token) targets the first environment in .pos, so name the environment explicitly.', inputSchema: { type: 'object', additionalProperties: false, - required: ['env', 'name'], + required: ['name'], properties: { env: { type: 'string', description: 'Environment name from .pos config' }, + ...authProperties, name: { type: 'string', description: 'Name of the constant to delete' } } }, diff --git a/mcp-min/data/clean-status.js b/mcp-min/data/clean-status.js index a7ae7b04..8a806e75 100644 --- a/mcp-min/data/clean-status.js +++ b/mcp-min/data/clean-status.js @@ -2,6 +2,7 @@ import log from '../log.js'; import { resolveAuth, maskToken } from '../auth.js'; import Gateway from '../../lib/proxy.js'; +import { authProperties } from '../schemas/auth.js'; const dataCleanStatusTool = { description: 'Check the status of a data clean job. Poll until status is "done" or "failed".', @@ -10,9 +11,7 @@ const dataCleanStatusTool = { additionalProperties: false, properties: { env: { type: 'string', description: 'Environment name from .pos config' }, - url: { type: 'string', description: 'Instance URL (alternative to env)' }, - email: { type: 'string', description: 'Account email (alternative to env)' }, - token: { type: 'string', description: 'API token (alternative to env)' }, + ...authProperties, jobId: { type: 'string', description: 'Clean job ID returned from data-clean' } }, required: ['jobId'] diff --git a/mcp-min/data/clean.js b/mcp-min/data/clean.js index 111ff100..5e607525 100644 --- a/mcp-min/data/clean.js +++ b/mcp-min/data/clean.js @@ -2,6 +2,7 @@ import log from '../log.js'; import { resolveAuth, maskToken } from '../auth.js'; import Gateway from '../../lib/proxy.js'; +import { authProperties } from '../schemas/auth.js'; const CONFIRMATION_TEXT = 'CLEAN DATA'; @@ -12,9 +13,7 @@ const dataCleanTool = { additionalProperties: false, properties: { env: { type: 'string', description: 'Environment name from .pos config' }, - url: { type: 'string', description: 'Instance URL (alternative to env)' }, - email: { type: 'string', description: 'Account email (alternative to env)' }, - token: { type: 'string', description: 'API token (alternative to env)' }, + ...authProperties, confirmation: { type: 'string', description: `Confirmation text - must be exactly "${CONFIRMATION_TEXT}" to proceed` diff --git a/mcp-min/data/export-status.js b/mcp-min/data/export-status.js index 812841ae..173250ef 100644 --- a/mcp-min/data/export-status.js +++ b/mcp-min/data/export-status.js @@ -2,6 +2,7 @@ import log from '../log.js'; import { resolveAuth, maskToken } from '../auth.js'; import Gateway from '../../lib/proxy.js'; +import { authProperties } from '../schemas/auth.js'; const dataExportStatusTool = { description: 'Check the status of a data export job. When done, returns data (JSON) or zip_file_url (ZIP).', @@ -10,9 +11,7 @@ const dataExportStatusTool = { additionalProperties: false, properties: { env: { type: 'string', description: 'Environment name from .pos config' }, - url: { type: 'string', description: 'Instance URL (alternative to env)' }, - email: { type: 'string', description: 'Account email (alternative to env)' }, - token: { type: 'string', description: 'API token (alternative to env)' }, + ...authProperties, jobId: { type: 'string', description: 'Export job ID returned from data-export' }, isZip: { type: 'boolean', description: 'Whether the export is a ZIP file', default: false } }, diff --git a/mcp-min/data/export.js b/mcp-min/data/export.js index e80fbbd9..d64350bb 100644 --- a/mcp-min/data/export.js +++ b/mcp-min/data/export.js @@ -2,6 +2,7 @@ import log from '../log.js'; import { resolveAuth, maskToken } from '../auth.js'; import Gateway from '../../lib/proxy.js'; +import { authProperties } from '../schemas/auth.js'; const dataExportTool = { description: 'Start data export from platformOS instance. Returns job ID for status polling. When complete, status will include data or zip_file_url.', @@ -10,9 +11,7 @@ const dataExportTool = { additionalProperties: false, properties: { env: { type: 'string', description: 'Environment name from .pos config' }, - url: { type: 'string', description: 'Instance URL (alternative to env)' }, - email: { type: 'string', description: 'Account email (alternative to env)' }, - token: { type: 'string', description: 'API token (alternative to env)' }, + ...authProperties, exportInternalIds: { type: 'boolean', description: 'Use internal object IDs instead of external_id in exported data', diff --git a/mcp-min/data/import-status.js b/mcp-min/data/import-status.js index 9c64e920..e8b024c1 100644 --- a/mcp-min/data/import-status.js +++ b/mcp-min/data/import-status.js @@ -2,15 +2,17 @@ import log from '../log.js'; import { resolveAuth } from '../auth.js'; import Gateway from '../../lib/proxy.js'; +import { authProperties } from '../schemas/auth.js'; const dataImportStatusTool = { description: 'Check the status of a data import job. Poll until status is "done" or "failed".', inputSchema: { type: 'object', additionalProperties: false, - required: ['env', 'jobId'], + required: ['jobId'], properties: { env: { type: 'string', description: 'Environment name from .pos config' }, + ...authProperties, jobId: { type: 'string', description: 'Import job ID returned from data-import' } } }, diff --git a/mcp-min/data/import.js b/mcp-min/data/import.js index d9ae1df9..e363b1df 100644 --- a/mcp-min/data/import.js +++ b/mcp-min/data/import.js @@ -6,6 +6,7 @@ import path from 'path'; import os from 'os'; import { jsonToZipBuffer } from './json-to-csv.js'; import { validateRecords, validateJsonStructure } from './validate.js'; +import { authProperties } from '../schemas/auth.js'; import log from '../log.js'; import { resolveAuth, runWithAuth } from '../auth.js'; import Gateway from '../../lib/proxy.js'; @@ -33,13 +34,13 @@ async function uploadZipBuffer(buffer, gateway, presignUrlFn, uploadFileFn) { } const dataImportTool = { - description: 'Import data to platformOS instance. Accepts JSON (converted to CSV internally) or ZIP file with CSV files.', + description: 'Import data to platformOS instance. Accepts JSON (converted to CSV internally) or ZIP file with CSV files. Omitting env (and url/email/token) targets the first environment in .pos, so name the environment explicitly.', inputSchema: { type: 'object', additionalProperties: false, - required: ['env'], properties: { env: { type: 'string', description: 'Environment name from .pos config' }, + ...authProperties, filePath: { type: 'string', description: 'Path to JSON or ZIP file to import' }, jsonData: { type: 'object', description: 'JSON data object to import (records, users)' }, zipFileUrl: { type: 'string', description: 'Remote URL of ZIP archive to import' }, diff --git a/mcp-min/data/validate-tool.js b/mcp-min/data/validate-tool.js index 69d34408..1a9f6afe 100644 --- a/mcp-min/data/validate-tool.js +++ b/mcp-min/data/validate-tool.js @@ -9,7 +9,7 @@ const dataValidateTool = { inputSchema: { type: 'object', additionalProperties: false, - required: ['env'], + // Nothing is required: validation runs entirely locally, and `env` is context only. properties: { env: { type: 'string', diff --git a/mcp-min/deploy/start.js b/mcp-min/deploy/start.js index 59bfdc45..cf70f8a1 100644 --- a/mcp-min/deploy/start.js +++ b/mcp-min/deploy/start.js @@ -11,6 +11,7 @@ import { deployAssets } from '../../lib/assets.js'; const archive = { makeArchive }; const assets = { deployAssets }; import dir from '../../lib/directories.js'; +import { authProperties } from '../schemas/auth.js'; const startDeployTool = { description: 'Deploy to platformOS instance. Creates archive from app/ and modules/ directories, uploads it, and deploys assets directly to S3.', @@ -19,9 +20,7 @@ const startDeployTool = { additionalProperties: false, properties: { env: { type: 'string', description: 'Environment name from .pos config' }, - url: { type: 'string', description: 'Instance URL (alternative to env)' }, - email: { type: 'string', description: 'Account email (alternative to env)' }, - token: { type: 'string', description: 'API token (alternative to env)' }, + ...authProperties, partial: { type: 'boolean', description: 'Partial deploy - does not remove files missing from build', default: false } } }, diff --git a/mcp-min/deploy/status.js b/mcp-min/deploy/status.js index a8ea4237..7c161744 100644 --- a/mcp-min/deploy/status.js +++ b/mcp-min/deploy/status.js @@ -1,6 +1,7 @@ // platformos.deploy.status - check deployment status via Gateway.getStatus import { resolveAuth } from '../auth.js'; import Gateway from '../../lib/proxy.js'; +import { authProperties } from '../schemas/auth.js'; const statusDeployTool = { description: 'Get current deployment status using Gateway.getStatus(id).', @@ -9,9 +10,7 @@ const statusDeployTool = { additionalProperties: false, properties: { env: { type: 'string' }, - url: { type: 'string' }, - email: { type: 'string' }, - token: { type: 'string' }, + ...authProperties, endpoint: { type: 'string' }, id: { type: 'string', description: 'Deployment ID returned from start' } }, diff --git a/mcp-min/deploy/wait.js b/mcp-min/deploy/wait.js index ae8e2a02..ab3f3c5d 100644 --- a/mcp-min/deploy/wait.js +++ b/mcp-min/deploy/wait.js @@ -1,6 +1,7 @@ // platformos.deploy.wait - poll deployment status until completion import { resolveAuth } from '../auth.js'; import Gateway from '../../lib/proxy.js'; +import { authProperties } from '../schemas/auth.js'; function delay(ms) { return new Promise(r => setTimeout(r, ms)); } @@ -11,9 +12,7 @@ const waitDeployTool = { additionalProperties: false, properties: { env: { type: 'string' }, - url: { type: 'string' }, - email: { type: 'string' }, - token: { type: 'string' }, + ...authProperties, endpoint: { type: 'string' }, id: { type: 'string', description: 'Deployment ID' }, intervalMs: { type: 'integer', minimum: 200, default: 1000 }, diff --git a/mcp-min/graphql/exec.js b/mcp-min/graphql/exec.js index 6bedfe3c..02911950 100644 --- a/mcp-min/graphql/exec.js +++ b/mcp-min/graphql/exec.js @@ -2,6 +2,7 @@ import { resolveAuth, maskToken } from '../auth.js'; import Gateway from '../../lib/proxy.js'; import { graphQLErrors, formatGraphQLErrors } from '../../lib/graph/response.js'; +import { authProperties } from '../schemas/auth.js'; const execGraphqlTool = { description: 'Execute a GraphQL query or mutation on a platformOS instance via /api/graph. Returns JSON data and errors from the instance. Auth resolved from: explicit params > MPKIT_* env vars > .pos config. Use variables to pass dynamic values safely instead of string interpolation.', @@ -10,9 +11,7 @@ const execGraphqlTool = { additionalProperties: false, properties: { env: { type: 'string', description: 'Environment name from .pos config (e.g., staging, production). Used to resolve auth when url/email/token are not provided.' }, - url: { type: 'string', description: 'Instance URL (e.g., https://my-app.staging.oregon.platform-os.com). Requires email and token.' }, - email: { type: 'string', description: 'Email for instance authentication. Required with url and token.' }, - token: { type: 'string', description: 'API token for instance authentication. Required with url and email.' }, + ...authProperties, endpoint: { type: 'string', description: 'Override the base URL for the GraphQL endpoint. Defaults to the resolved instance URL.' }, query: { type: 'string', description: 'GraphQL query or mutation string (e.g., "{ users { results { id email } } }").' }, variables: { type: 'object', additionalProperties: true, description: 'Variables to pass to the GraphQL query/mutation. Preferred over string interpolation for dynamic values.' } diff --git a/mcp-min/http-server.js b/mcp-min/http-server.js index 86e430b2..52306762 100644 --- a/mcp-min/http-server.js +++ b/mcp-min/http-server.js @@ -2,6 +2,8 @@ import express from 'express'; import bodyParser from 'body-parser'; import { randomUUID } from 'crypto'; import tools from './tools.js'; +import { rejectionFor } from './validate-params.js'; +import { OPEN_OBJECT_SCHEMA } from './schemas/default.js'; import { sseHandler, writeSSE } from './sse.js'; import { DEBUG } from './config.js'; import log from './log.js'; @@ -92,6 +94,13 @@ export default async function startHttp({ port = 5910 } = {}) { const entry = tools[tool]; if (!entry) return res.status(404).json({ error: `tool not found: ${tool}` }); + const rejection = rejectionFor(tool, entry, params); + if (rejection) { + return res + .status(rejection.httpStatus) + .json({ error: `invalid params: ${rejection.message}`, details: rejection.errors }); + } + try { log.debug('HTTP /call', { tool, params, rawBodyKeys: Object.keys(body) }); const result = await entry.handler(params || {}, { transport: 'http', debug: DEBUG }); @@ -163,7 +172,7 @@ export default async function startHttp({ port = 5910 } = {}) { const list = Object.keys(tools).map((name) => ({ name, description: tools[name].description || '', - inputSchema: tools[name].inputSchema || { type: 'object', additionalProperties: true } + inputSchema: tools[name].inputSchema || OPEN_OBJECT_SCHEMA })); respond({ result: { tools: list } }); return; @@ -182,6 +191,17 @@ export default async function startHttp({ port = 5910 } = {}) { respond({ error: { code: -32601, message: `Tool not found: ${name}` } }); return; } + const rejection = rejectionFor(name, entry, args); + if (rejection) { + respond({ + error: { + code: rejection.jsonRpcCode, + message: `Invalid params: ${rejection.message}`, + data: { errors: rejection.errors } + } + }); + return; + } const result = await entry.handler(args, { transport: 'jsonrpc', debug: DEBUG }); // Wrap result as text content for broad client compatibility const text = (() => { try { return JSON.stringify(result); } catch { return String(result); } })(); @@ -211,6 +231,15 @@ export default async function startHttp({ port = 5910 } = {}) { const entry = tools[tool]; if (!entry) return res.status(404).json({ error: `tool not found: ${tool}` }); + // Validate before the SSE handshake: once the stream is open the status code is + // already sent, so a rejection could only be reported as an in-band error event. + const streamRejection = rejectionFor(tool, entry, params); + if (streamRejection) { + return res + .status(streamRejection.httpStatus) + .json({ error: `invalid params: ${streamRejection.message}`, details: streamRejection.errors }); + } + // Prepare SSE response sseHandler(req, res); diff --git a/mcp-min/liquid/exec.js b/mcp-min/liquid/exec.js index 31ab50ac..7d0a441c 100644 --- a/mcp-min/liquid/exec.js +++ b/mcp-min/liquid/exec.js @@ -1,6 +1,7 @@ // platformos.liquid.exec tool - execute Liquid on remote instance via Gateway.liquid import { resolveAuth, maskToken } from '../auth.js'; import Gateway from '../../lib/proxy.js'; +import { authProperties } from '../schemas/auth.js'; const execLiquidTool = { description: 'Render a Liquid template on a platformOS instance server-side via /api/app_builder/liquid_exec. Returns the rendered output. Useful for testing Liquid code, running one-off queries via {% graphql %}, or inspecting instance state. Auth resolved from: explicit params > MPKIT_* env vars > .pos config.', @@ -9,9 +10,7 @@ const execLiquidTool = { additionalProperties: false, properties: { env: { type: 'string', description: 'Environment name from .pos config (e.g., staging, production). Used to resolve auth when url/email/token are not provided.' }, - url: { type: 'string', description: 'Instance URL (e.g., https://my-app.staging.oregon.platform-os.com). Requires email and token.' }, - email: { type: 'string', description: 'Email for instance authentication. Required with url and token.' }, - token: { type: 'string', description: 'API token for instance authentication. Required with url and email.' }, + ...authProperties, endpoint: { type: 'string', description: 'Override the base URL for the Liquid exec endpoint. Defaults to the resolved instance URL.' }, template: { type: 'string', description: 'Liquid template string to render server-side (e.g., "Hello {{ name }}", "{% graphql g = \'users/search\' %}").' }, locals: { type: 'object', additionalProperties: true, description: 'Variables available inside the template as top-level Liquid variables (e.g., { "name": "World" } makes {{ name }} render "World").' } diff --git a/mcp-min/logs/fetch.js b/mcp-min/logs/fetch.js index 301fdc4c..6b0a8b64 100644 --- a/mcp-min/logs/fetch.js +++ b/mcp-min/logs/fetch.js @@ -1,6 +1,7 @@ // platformos.logs.fetch tool - batch fetch logs based on pos-cli fetch-logs import { resolveAuth, maskToken } from '../auth.js'; import Gateway from '../../lib/proxy.js'; +import { authProperties } from '../schemas/auth.js'; const fetchLogsTool = { description: 'Fetch recent logs in batches (NDJSON semantics, returns JSON array here). Mirrors pos-cli fetch-logs.', @@ -9,10 +10,8 @@ const fetchLogsTool = { additionalProperties: false, properties: { env: { type: 'string' }, - url: { type: 'string' }, - email: { type: 'string' }, - token: { type: 'string' }, - lastId: { type: 'string' }, + ...authProperties, + lastId: { type: 'integer', minimum: 0, description: 'Log row id to resume from (default 0)' }, endpoint: { type: 'string', description: 'Override API base url' }, limit: { type: 'integer', minimum: 1, maximum: 10000 } } @@ -64,7 +63,10 @@ const fetchLogsTool = { return { ok: true, logs: out, - lastId: latestId, + // Number, not the string the paging loop carries: `lastId` is declared as an + // integer on the way in, and the documented use of this field is to hand it + // straight back as the next call's cursor. + lastId: Number(latestId), meta: { startedAt, finishedAt: new Date().toISOString(), diff --git a/mcp-min/logs/stream.js b/mcp-min/logs/stream.js index e16c9d0e..af2cebb2 100644 --- a/mcp-min/logs/stream.js +++ b/mcp-min/logs/stream.js @@ -2,6 +2,7 @@ import log from '../log.js'; import { resolveAuth } from '../auth.js'; import Gateway from '../../lib/proxy.js'; +import { authProperties } from '../schemas/auth.js'; function matchesFilter(row, filter) { if (!filter) return true; @@ -16,13 +17,11 @@ const streamTool = { additionalProperties: false, properties: { env: { type: 'string' }, - url: { type: 'string' }, - email: { type: 'string' }, - token: { type: 'string' }, + ...authProperties, endpoint: { type: 'string' }, interval: { type: 'integer', minimum: 250 }, filter: { type: 'string' }, - startLastId: { type: 'string', description: 'Starting last id (default 0)' }, + startLastId: { type: 'integer', minimum: 0, description: 'Starting log row id (default 0)' }, maxDuration: { type: 'integer', description: 'Optional max duration ms' } } }, diff --git a/mcp-min/migrations/generate.js b/mcp-min/migrations/generate.js index fe4b2a33..015ea464 100644 --- a/mcp-min/migrations/generate.js +++ b/mcp-min/migrations/generate.js @@ -4,6 +4,7 @@ import path from 'path'; import { resolveAuth } from '../auth.js'; import Gateway from '../../lib/proxy.js'; import dir from '../../lib/directories.js'; +import { authProperties } from '../schemas/auth.js'; function ensureMigrationsDir() { const appDirectory = fs.existsSync(dir.APP) ? dir.APP : dir.LEGACY_APP; @@ -18,9 +19,7 @@ const generateMigrationTool = { additionalProperties: false, properties: { env: { type: 'string' }, - url: { type: 'string' }, - email: { type: 'string' }, - token: { type: 'string' }, + ...authProperties, name: { type: 'string', description: 'Base name of the migration, without timestamp' }, skipWrite: { type: 'boolean', description: 'When true, do not create local file', default: false }, endpoint: { type: 'string', description: 'Override API base URL' } diff --git a/mcp-min/migrations/list.js b/mcp-min/migrations/list.js index 5c5c0df5..a505b86f 100644 --- a/mcp-min/migrations/list.js +++ b/mcp-min/migrations/list.js @@ -1,6 +1,7 @@ // platformos.migrations.list - list migrations and their statuses via Gateway import { resolveAuth } from '../auth.js'; import Gateway from '../../lib/proxy.js'; +import { authProperties } from '../schemas/auth.js'; const listMigrationsTool = { description: 'List migrations deployed to the server with their current status.', @@ -9,9 +10,7 @@ const listMigrationsTool = { additionalProperties: false, properties: { env: { type: 'string' }, - url: { type: 'string' }, - email: { type: 'string' }, - token: { type: 'string' }, + ...authProperties, endpoint: { type: 'string', description: 'Override API base URL' } } }, diff --git a/mcp-min/migrations/run.js b/mcp-min/migrations/run.js index 4273cae7..cf75cef2 100644 --- a/mcp-min/migrations/run.js +++ b/mcp-min/migrations/run.js @@ -1,6 +1,7 @@ // platformos.migrations.run - run a specific migration by name or timestamp via Gateway import { resolveAuth } from '../auth.js'; import Gateway from '../../lib/proxy.js'; +import { authProperties } from '../schemas/auth.js'; function buildFormData({ timestamp, name }) { if (!timestamp && !name) throw new Error('INVALID_INPUT: Provide timestamp or name'); @@ -15,9 +16,7 @@ const runMigrationTool = { additionalProperties: false, properties: { env: { type: 'string' }, - url: { type: 'string' }, - email: { type: 'string' }, - token: { type: 'string' }, + ...authProperties, timestamp: { type: 'string', description: 'Numeric timestamp' }, name: { type: 'string', description: 'Alias for timestamp; full migration name without .liquid' }, endpoint: { type: 'string', description: 'Override API base URL' } diff --git a/mcp-min/schemas/auth.js b/mcp-min/schemas/auth.js new file mode 100644 index 00000000..9ab21096 --- /dev/null +++ b/mcp-min/schemas/auth.js @@ -0,0 +1,35 @@ +/** + * Shared input-schema fragment for the explicit-credentials path. + * + * `resolveAuth` (mcp-min/auth.js) accepts `url` + `email` + `token` on the params of + * every tool that authenticates, ahead of the `.pos` environment lookup. Tools that + * close their schema with `additionalProperties: false` must therefore declare these + * three, or validation would reject the very callers that path exists to serve. + * + * Spread into every authenticating tool rather than restated inline, so `tools/list` + * documents the same three parameters identically everywhere. The invariant is enforced + * by mcp-min/__tests__/validate-params.test.js, which derives the tool list from the + * registry rather than a hand-written list. + * + * `env` is deliberately not included: it is required on some tools and optional on + * others, and its description varies, so it stays declared per tool. + */ +const authProperties = { + url: { + type: 'string', + format: 'uri', + description: 'Instance URL (with email and token, bypasses .pos)' + }, + email: { + type: 'string', + format: 'email', + description: 'Account email (with url and token, bypasses .pos)' + }, + token: { + type: 'string', + description: 'API token (with url and email, bypasses .pos)' + } +}; + +export { authProperties }; +export default authProperties; diff --git a/mcp-min/schemas/default.js b/mcp-min/schemas/default.js new file mode 100644 index 00000000..4c4a3b2e --- /dev/null +++ b/mcp-min/schemas/default.js @@ -0,0 +1,18 @@ +/** + * The schema used for a tool that declares no `inputSchema` of its own. + * + * There is exactly one of these because the same object has to serve both roles: what + * the transports advertise in `tools/list` and what the enforcement path in + * validate-params.js actually checks against. Three separate literals previously + * disagreed about whether the default was open or closed, which is precisely the drift + * that validating each tool's own advertised schema is meant to rule out. + * + * It stays open (no `additionalProperties: false`): a tool that declared no schema never + * promised to reject unknown parameters, and enforcement must not invent a contract that + * `tools/list` did not publish. Every registered tool declares its own closed schema, so + * this is a fallback rather than a live policy. + */ +const OPEN_OBJECT_SCHEMA = { type: 'object', properties: {} }; + +export { OPEN_OBJECT_SCHEMA }; +export default OPEN_OBJECT_SCHEMA; diff --git a/mcp-min/stdio-server.js b/mcp-min/stdio-server.js index 5951fec4..b93d399d 100644 --- a/mcp-min/stdio-server.js +++ b/mcp-min/stdio-server.js @@ -2,6 +2,8 @@ import { createInterface } from 'readline'; import { fileURLToPath } from 'url'; import path from 'path'; import tools from './tools.js'; +import { rejectionFor } from './validate-params.js'; +import { OPEN_OBJECT_SCHEMA } from './schemas/default.js'; import { DEBUG } from './config.js'; import log from './log.js'; @@ -48,7 +50,7 @@ function getToolsList() { return Object.entries(tools).map(([name, tool]) => ({ name, description: tool.description || '', - inputSchema: tool.inputSchema || { type: 'object', properties: {} } + inputSchema: tool.inputSchema || OPEN_OBJECT_SCHEMA })); } @@ -84,6 +86,12 @@ const mcpHandlers = { return; } + const rejection = rejectionFor(name, tool, args); + if (rejection) { + sendError(id, rejection.jsonRpcCode, `Invalid params: ${rejection.message}`, { errors: rejection.errors }); + return; + } + // Send progress notification (keeps connection alive, prevents client timeout) let progressCounter = 0; function sendProgress(current, total, message) { @@ -160,6 +168,18 @@ export default function startStdio() { // Fallback: direct tool invocation (legacy/custom protocol) const tool = tools[method]; if (tool) { + const rejection = rejectionFor(method, tool, params); + if (rejection) { + const message = `Invalid params: ${rejection.message}`; + if (jsonrpc === '2.0') { + sendError(id, rejection.jsonRpcCode, message, { errors: rejection.errors }); + } else { + send({ id, error: message }); + } + log.debug('STDIO invalid params', { id, method, message }); + return; + } + try { const result = await tool.handler(params || {}, { transport: 'stdio', debug: DEBUG, log: log.info.bind(log) }); if (jsonrpc === '2.0') { diff --git a/mcp-min/sync/single-file.js b/mcp-min/sync/single-file.js index a3c11de8..44f29a6b 100644 --- a/mcp-min/sync/single-file.js +++ b/mcp-min/sync/single-file.js @@ -14,6 +14,7 @@ import { fillInTemplateValues } from '../../lib/templates.js'; import dir from '../../lib/directories.js'; import log from '../log.js'; import { resolveAuth, maskToken, runWithAuth } from '../auth.js'; +import { authProperties } from '../schemas/auth.js'; // Alias for backwards compatibility const templates = { fillInTemplateValues }; @@ -97,9 +98,7 @@ const singleFileTool = { properties: { filePath: { type: 'string', description: 'Absolute or relative path to the file to sync. Must be inside app/, marketplace_builder/, or modules/.' }, env: { type: 'string', description: 'Environment name from .pos config (e.g., staging, production). Used to resolve auth when url/email/token are not provided.' }, - url: { type: 'string', description: 'Instance URL (e.g., https://my-app.staging.oregon.platform-os.com). Requires email and token.' }, - email: { type: 'string', description: 'Email for instance authentication. Required with url and token.' }, - token: { type: 'string', description: 'API token for instance authentication. Required with url and email.' }, + ...authProperties, op: { type: 'string', enum: ['upload', 'delete'], description: 'Operation: "upload" to push file, "delete" to remove from instance. Auto-detected from file existence if omitted.' }, dryRun: { type: 'boolean', description: 'Validate file path, auth, and sync rules without actually uploading. Default: false.' }, confirmDelete: { type: 'boolean', description: 'Safety flag -- must be true to execute delete operations. Default: false.' } diff --git a/mcp-min/tests/run-async-result.js b/mcp-min/tests/run-async-result.js index ab52a323..64e70c4e 100644 --- a/mcp-min/tests/run-async-result.js +++ b/mcp-min/tests/run-async-result.js @@ -1,6 +1,7 @@ // platformos.tests.run-async-result - check result of an async test run via /_tests/results/:id import log from '../log.js'; import { resolveAuth, maskToken } from '../auth.js'; +import { authProperties } from '../schemas/auth.js'; async function makeRequest(options) { const { uri, method = 'GET', headers = {} } = options; @@ -34,9 +35,7 @@ const testsRunAsyncResultTool = { additionalProperties: false, properties: { env: { type: 'string', description: 'Environment name from .pos config' }, - url: { type: 'string', description: 'Instance URL (alternative to env)' }, - email: { type: 'string', description: 'Account email (alternative to env)' }, - token: { type: 'string', description: 'API token (alternative to env)' }, + ...authProperties, id: { type: 'string', description: 'Test run ID returned by tests-run-async' } }, required: ['id'] diff --git a/mcp-min/tests/run-async.js b/mcp-min/tests/run-async.js index df8939a6..efb9c144 100644 --- a/mcp-min/tests/run-async.js +++ b/mcp-min/tests/run-async.js @@ -1,6 +1,7 @@ // platformos.tests.run-async - trigger tests via /_tests/run_async (returns immediately) import log from '../log.js'; import { resolveAuth, maskToken } from '../auth.js'; +import { authProperties } from '../schemas/auth.js'; async function makeRequest(options) { const { uri, method = 'GET', headers = {} } = options; @@ -16,9 +17,7 @@ const testsRunAsyncTool = { additionalProperties: false, properties: { env: { type: 'string', description: 'Environment name from .pos config' }, - url: { type: 'string', description: 'Instance URL (alternative to env)' }, - email: { type: 'string', description: 'Account email (alternative to env)' }, - token: { type: 'string', description: 'API token (alternative to env)' } + ...authProperties, } }, handler: async (params, ctx = {}) => { diff --git a/mcp-min/tests/run.js b/mcp-min/tests/run.js index 39bf54a3..a473b8fb 100644 --- a/mcp-min/tests/run.js +++ b/mcp-min/tests/run.js @@ -1,6 +1,7 @@ // platformos.tests.run - execute tests via /_tests/run?formatter=text import log from '../log.js'; import { resolveAuth, maskToken } from '../auth.js'; +import { authProperties } from '../schemas/auth.js'; // Helper to make HTTP requests (replaces request-promise) async function makeRequest(options) { @@ -251,13 +252,13 @@ const testsRunTool = { additionalProperties: false, properties: { env: { type: 'string', description: 'Environment name from .pos config' }, - url: { type: 'string', description: 'Instance URL (alternative to env)' }, - email: { type: 'string', description: 'Account email (alternative to env)' }, - token: { type: 'string', description: 'API token (alternative to env)' }, + ...authProperties, path: { type: 'string', description: 'Optional test path filter (e.g., "tests/users")' }, name: { type: 'string', description: 'Test name filter (e.g., "create_user_test"). Required to avoid running all tests which causes timeouts.' } }, - required: ['env', 'name'] + // `env` is not required: resolveAuth also accepts url+email+token, MPKIT_* env + // vars, or falls back to the first .pos environment. + required: ['name'] }, handler: async (params, ctx = {}) => { const startedAt = new Date().toISOString(); diff --git a/mcp-min/tools-config-error.js b/mcp-min/tools-config-error.js new file mode 100644 index 00000000..e9cc0069 --- /dev/null +++ b/mcp-min/tools-config-error.js @@ -0,0 +1,16 @@ +/** + * Raised when mcp-min/tools.config.json (or MCP_TOOLS_CONFIG) is present but invalid. + * + * A distinct type so the CLI boundary in bin/pos-cli-mcp.js can tell a user-fixable + * configuration problem from an internal crash, and report it through the logger instead + * of letting a module-loader stack trace reach the terminal. + */ +class ToolsConfigError extends Error { + constructor(message) { + super(message); + this.name = 'ToolsConfigError'; + } +} + +export { ToolsConfigError }; +export default ToolsConfigError; diff --git a/mcp-min/tools.config.json b/mcp-min/tools.config.json index 6de244a3..f0eac729 100644 --- a/mcp-min/tools.config.json +++ b/mcp-min/tools.config.json @@ -56,7 +56,7 @@ }, "data-import": { "enabled": true, - "description": "Import data to platformOS instance. Accepts JSON (converted to CSV internally) or ZIP file with CSV files." + "description": "Import data to platformOS instance. Accepts JSON (converted to CSV internally) or ZIP file with CSV files. Omitting env (and url/email/token) targets the first environment in .pos, so name the environment explicitly." }, "data-import-status": { "enabled": true, @@ -108,7 +108,7 @@ }, "uploads-push": { "enabled": true, - "description": "Upload a ZIP file containing property uploads to platformOS instance. The ZIP should contain files referenced by upload-type properties." + "description": "Upload a ZIP file containing property uploads to platformOS instance. The ZIP should contain files referenced by upload-type properties. Omitting env (and url/email/token) targets the first environment in .pos, so name the environment explicitly." }, "constants-list": { "enabled": true, @@ -116,11 +116,11 @@ }, "constants-set": { "enabled": true, - "description": "Set a constant on a platformOS instance. Creates or updates the constant." + "description": "Set a constant on a platformOS instance. Creates or updates the constant. Omitting env (and url/email/token) targets the first environment in .pos, so name the environment explicitly." }, "constants-unset": { "enabled": true, - "description": "Delete a constant from a platformOS instance." + "description": "Delete a constant from a platformOS instance. Omitting env (and url/email/token) targets the first environment in .pos, so name the environment explicitly." }, "instance-create": { "enabled": true, diff --git a/mcp-min/tools.js b/mcp-min/tools.js index 0e1f8bb3..e5e6638d 100644 --- a/mcp-min/tools.js +++ b/mcp-min/tools.js @@ -4,18 +4,14 @@ import { readFileSync } from 'fs'; import { fileURLToPath } from 'url'; import { dirname, join } from 'path'; import files from '../lib/files.js'; +import { validate } from '../lib/validation/index.js'; +import { ToolsConfigError } from './tools-config-error.js'; -// Load tool configuration (descriptions and enabled/disabled state) -// MCP_TOOLS_CONFIG env var overrides the bundled config +// MCP_TOOLS_CONFIG env var overrides the bundled config. The config itself is loaded +// further down, once the tool registry exists to validate its keys against. const __dirname = dirname(fileURLToPath(import.meta.url)); -let toolsConfig = { tools: {} }; const configPath = process.env.MCP_TOOLS_CONFIG || join(__dirname, 'tools.config.json'); -try { - toolsConfig = JSON.parse(readFileSync(configPath, 'utf-8')); - log.debug('tools config loaded', { path: configPath, tools: Object.keys(toolsConfig.tools || {}).length }); -} catch (err) { - log.debug('tools config not found or invalid, using defaults', { path: configPath, error: String(err) }); -} +const configSchema = JSON.parse(readFileSync(join(__dirname, 'tools.config.schema.json'), 'utf-8')); // Keep tools.js lean by extracting complex tools into modules import singleFileTool from './sync/single-file.js'; @@ -77,6 +73,7 @@ const tools = { description: 'List configured environments from .pos (name and url)', inputSchema: { type: 'object', + additionalProperties: false, properties: {} }, handler: async (_params, ctx) => { @@ -157,6 +154,49 @@ const tools = { 'env-add': envAddTool }; +/** + * Read and validate the tools config. + * + * Fails closed on anything it can detect: this file decides which tools are exposed, so + * a config that is present but wrong must stop the server rather than be ignored, which + * would silently re-enable every tool the author meant to switch off. A missing or + * unparseable file is the one benign case — there is nothing to apply, so defaults win. + * + * @param {object} registry - the tool registry, used to reject names that match no tool + * @throws {ToolsConfigError} when the file is present but does not describe a valid config + */ +function loadToolsConfig(registry) { + let raw; + try { + raw = JSON.parse(readFileSync(configPath, 'utf-8')); + } catch (err) { + log.debug('tools config not found or unparseable, using defaults', { path: configPath, error: String(err) }); + return { tools: {} }; + } + + const result = validate(configSchema, raw); + if (!result.valid) { + throw new ToolsConfigError(`Invalid tools config at ${configPath}: ${result.message}`); + } + + // The schema constrains the shape of each entry but cannot enumerate tool names, so a + // typo like "deploy-strt" would otherwise be accepted, match nothing in applyConfig, + // and leave "deploy-start" enabled — the exact fail-open the schema check exists to + // prevent, and the harder one to notice because the config looks like it took effect. + // hasOwnProperty, not `in`: `in` walks the prototype chain, so an entry keyed + // `toString` or `constructor` would pass as a known tool and then be ignored. + const unknown = Object.keys(raw.tools || {}) + .filter(name => !Object.prototype.hasOwnProperty.call(registry, name)); + if (unknown.length > 0) { + throw new ToolsConfigError( + `Invalid tools config at ${configPath}: no such tool: ${unknown.join(', ')}` + ); + } + + log.debug('tools config loaded', { path: configPath, tools: Object.keys(raw.tools || {}).length }); + return raw; +} + // Apply configuration: override descriptions and filter disabled tools function applyConfig(allTools, config) { const result = {}; @@ -179,6 +219,6 @@ function applyConfig(allTools, config) { return result; } -const configuredTools = applyConfig(tools, toolsConfig); +const configuredTools = applyConfig(tools, loadToolsConfig(tools)); export default configuredTools; diff --git a/mcp-min/uploads/push.js b/mcp-min/uploads/push.js index 77662a7f..acbee5a5 100644 --- a/mcp-min/uploads/push.js +++ b/mcp-min/uploads/push.js @@ -7,15 +7,17 @@ import Gateway from '../../lib/proxy.js'; import { presignUrl } from '../../lib/presignUrl.js'; import { uploadFile } from '../../lib/s3UploadFile.js'; import { resolveAuth, runWithAuth } from '../auth.js'; +import { authProperties } from '../schemas/auth.js'; const uploadsPushTool = { - description: 'Upload a ZIP file containing property uploads to platformOS instance. The ZIP should contain files referenced by upload-type properties.', + description: 'Upload a ZIP file containing property uploads to platformOS instance. The ZIP should contain files referenced by upload-type properties. Omitting env (and url/email/token) targets the first environment in .pos, so name the environment explicitly.', inputSchema: { type: 'object', additionalProperties: false, - required: ['env', 'filePath'], + required: ['filePath'], properties: { env: { type: 'string', description: 'Environment name' }, + ...authProperties, filePath: { type: 'string', description: 'Path to ZIP file with uploads' } } }, diff --git a/mcp-min/validate-params.js b/mcp-min/validate-params.js new file mode 100644 index 00000000..50aa3777 --- /dev/null +++ b/mcp-min/validate-params.js @@ -0,0 +1,59 @@ +import { validate } from '../lib/validation/index.js'; +import { OPEN_OBJECT_SCHEMA } from './schemas/default.js'; +import log from './log.js'; + +/** + * Validate tool params against the tool's advertised `inputSchema`. + * + * Both transports call this before handing params to a handler, so the schema shown to + * clients in tools/list is the schema those params are actually checked against. + * + * @param {string} name - tool name, for logging + * @param {object} tool - tool entry from tools.js + * @param {*} params - untrusted params from the client + * @returns {{valid: boolean, errors?: Array, message?: string, schemaError?: boolean}} + */ +const validateToolParams = (name, tool, params) => { + const result = validate(tool.inputSchema || OPEN_OBJECT_SCHEMA, params ?? {}); + + if (!result.valid) { + log.debug('tool params rejected', { + tool: name, + message: result.message, + schemaError: Boolean(result.schemaError) + }); + } + + return result; +}; + +/** + * Validate tool params and, if they are rejected, describe the rejection in the terms + * both transports need. + * + * The status mapping lives here rather than at each call site so there is exactly one + * decision about how a rejection is reported. A schema that will not compile is our own + * defect, so it is a server error (500 / -32603) rather than the caller's fault + * (400 / -32602) — but either way the call is rejected, because a schema that did not + * compile checked nothing. + * + * @param {string} name - tool name, for logging + * @param {object} tool - tool entry from tools.js + * @param {*} params - untrusted params from the client + * @returns {null|{httpStatus: number, jsonRpcCode: number, message: string, errors: Array}} + * null when the params are valid. + */ +const rejectionFor = (name, tool, params) => { + const result = validateToolParams(name, tool, params); + if (result.valid) return null; + + return { + httpStatus: result.schemaError ? 500 : 400, + jsonRpcCode: result.schemaError ? -32603 : -32602, + message: result.message, + errors: result.errors + }; +}; + +export { validateToolParams, rejectionFor }; +export default validateToolParams; diff --git a/package-lock.json b/package-lock.json index d59b6876..b42d38d9 100644 --- a/package-lock.json +++ b/package-lock.json @@ -20,6 +20,8 @@ "@platformos/platformos-common": "^0.1.0", "@platformos/platformos-language-server-node": "^0.1.0", "@platformos/platformos-mcp-supervisor": "^0.1.0", + "ajv": "^8.20.0", + "ajv-formats": "^3.0.1", "async": "^3.2.6", "body-parser": "^2.2.2", "chalk": "^6.0.0", diff --git a/package.json b/package.json index 037d948b..9e21d90a 100644 --- a/package.json +++ b/package.json @@ -45,6 +45,8 @@ "@platformos/platformos-common": "^0.1.0", "@platformos/platformos-language-server-node": "^0.1.0", "@platformos/platformos-mcp-supervisor": "^0.1.0", + "ajv": "^8.20.0", + "ajv-formats": "^3.0.1", "async": "^3.2.6", "body-parser": "^2.2.2", "chalk": "^6.0.0", diff --git a/test/unit/server.validation.test.js b/test/unit/server.validation.test.js new file mode 100644 index 00000000..1df17c0c --- /dev/null +++ b/test/unit/server.validation.test.js @@ -0,0 +1,307 @@ +/** + * Request validation on the local GUI server. + * + * These routes proxy to the connected instance with the user's API token attached, and + * lib/server.js allows any origin, so malformed input must be rejected before it is + * forwarded upstream. + */ +import { describe, test, expect, vi, beforeAll, afterAll } from 'vitest'; +import request from 'supertest'; + +const forwarded = { graph: [], liquid: [], logs: [], logsv2: [], sync: [] }; + +vi.mock('#lib/proxy.js', () => ({ + default: class Gateway { + graph(body) { + forwarded.graph.push(body); + return Promise.resolve({ data: {} }); + } + liquid(body) { + forwarded.liquid.push(body); + return Promise.resolve({ result: 'ok' }); + } + logs(params) { + forwarded.logs.push(params); + return Promise.resolve({ logs: [] }); + } + logsv2(params) { + forwarded.logsv2.push(params); + return Promise.resolve({ logs: [] }); + } + sync(formData) { + forwarded.sync.push(formData); + return Promise.resolve({ status: 'ok' }); + } + } +})); + +vi.mock('#lib/logger.js', () => ({ + default: { Debug: vi.fn(), Success: vi.fn(), Error: vi.fn(), Print: vi.fn(), Warn: vi.fn() } +})); + +let server; +let agent; + +beforeAll(async () => { + const { start } = await import('#lib/server.js'); + // Port 0 lets the OS pick a free port, so the suite never collides with a real GUI. + server = start({ PORT: 0, HOST: '127.0.0.1', MARKETPLACE_URL: 'https://example.com' }); + agent = request(server); +}); + +afterAll(() => { + if (server) server.close(); +}); + +describe('POST /api/graph', () => { + test('forwards a well-formed query', async () => { + const res = await agent.post('/api/graph').send({ query: '{ records { id } }' }); + + expect(res.status).toBe(200); + expect(forwarded.graph.at(-1).query).toBe('{ records { id } }'); + }); + + test('rejects a missing query without calling upstream', async () => { + const before = forwarded.graph.length; + const res = await agent.post('/api/graph').send({ variables: {} }); + + expect(res.status).toBe(400); + expect(res.body.error).toContain("missing required property 'query'"); + expect(forwarded.graph).toHaveLength(before); + }); + + test('rejects an empty query', async () => { + const res = await agent.post('/api/graph').send({ query: '' }); + expect(res.status).toBe(400); + }); + + test('rejects a query of the wrong type', async () => { + const res = await agent.post('/api/graph').send({ query: { evil: true } }); + expect(res.status).toBe(400); + }); + + test('rejects a non-object body', async () => { + const res = await agent.post('/api/graph').set('Content-Type', 'application/json').send('"just a string"'); + expect(res.status).toBe(400); + }); + + test('keeps accepting extra fields the pre-built GraphiQL bundle may send', async () => { + const res = await agent + .post('/api/graph') + .send({ query: '{ a }', operationName: 'A', variables: { x: 1 }, extensions: {} }); + + expect(res.status).toBe(200); + }); +}); + +describe('/api/liquid', () => { + test('forwards a well-formed POST body', async () => { + const res = await agent.post('/api/liquid').send({ content: '{{ 1 | plus: 1 }}' }); + + expect(res.status).toBe(200); + expect(forwarded.liquid.at(-1)).toEqual({ content: '{{ 1 | plus: 1 }}' }); + }); + + test('rejects a POST with no content', async () => { + const res = await agent.post('/api/liquid').send({}); + expect(res.status).toBe(400); + expect(res.body.error).toContain("missing required property 'content'"); + }); + + test('reads content from the query string on GET, matching the GUI form', async () => { + const res = await agent.get('/api/liquid').query({ content: '{{ 2 }}' }); + + expect(res.status).toBe(200); + expect(forwarded.liquid.at(-1).content).toBe('{{ 2 }}'); + }); + + test('rejects a GET with no content', async () => { + const res = await agent.get('/api/liquid'); + expect(res.status).toBe(400); + }); +}); + +describe('GET /api/logs', () => { + test('coerces the cursor to a number', async () => { + const res = await agent.get('/api/logs').query({ lastId: '42' }); + + expect(res.status).toBe(200); + expect(forwarded.logs.at(-1).lastId).toBe(42); + }); + + // 0 is the "from the beginning" sentinel. Before the schema supplied it as a default, + // an absent cursor put the string "undefined" into the upstream URL. + test('defaults an absent cursor to 0', async () => { + const res = await agent.get('/api/logs'); + + expect(res.status).toBe(200); + expect(forwarded.logs.at(-1).lastId).toBe(0); + }); + + // gui/next up to this change built the query with `args.last ?? null`, so every first + // poll sent the literal string "null". Ajv will not coerce that to an integer, which + // made the admin Logs page 400 on load. Installed GUI builds still send it. + test.each(['null', '', 'undefined'])('accepts the cursor an older gui/next sends: %p', async value => { + const res = await agent.get('/api/logs').query({ lastId: value }); + + expect(res.status).toBe(200); + expect(forwarded.logs.at(-1).lastId).toBe(0); + }); + + // Gateway.logs interpolates the cursor into the request URL, so a value carrying its + // own query parameters must not reach it. + test('rejects a cursor that smuggles extra query parameters', async () => { + const before = forwarded.logs.length; + const res = await agent.get('/api/logs').query({ lastId: '1&admin=true' }); + + expect(res.status).toBe(400); + expect(res.body.error).toContain('/lastId'); + expect(forwarded.logs).toHaveLength(before); + }); + + test('rejects a non-numeric cursor', async () => { + expect((await agent.get('/api/logs').query({ lastId: 'newest' })).status).toBe(400); + }); + + test('rejects a negative cursor', async () => { + expect((await agent.get('/api/logs').query({ lastId: '-1' })).status).toBe(400); + }); +}); + +describe('/api/logsv2', () => { + test('coerces numeric query-string params on GET', async () => { + const res = await agent.get('/api/logsv2').query({ sql: 'select 1', size: '25', from: '0' }); + + expect(res.status).toBe(200); + expect(forwarded.logsv2.at(-1).size).toBe(25); + expect(forwarded.logsv2.at(-1).from).toBe(0); + }); + + test('rejects a numeric param that is not a number', async () => { + const res = await agent.get('/api/logsv2').query({ sql: 'select 1', size: 'lots' }); + + expect(res.status).toBe(400); + expect(res.body.error).toContain('/size'); + }); + + test('rejects a negative size', async () => { + const res = await agent.post('/api/logsv2').send({ sql: 'select 1', size: -5 }); + expect(res.status).toBe(400); + }); + + test('accepts a POST search body', async () => { + const res = await agent.post('/api/logsv2').send({ sql: 'select 1', size: 10 }); + expect(res.status).toBe(200); + }); +}); + +describe('/api/logsv2 payload shapes', () => { + // gui/next posts the search as an object; the GET route can only ever deliver a string. + // Both branches are live in Gateway.logsv2, and the union is why the Ajv instance runs + // with allowUnionTypes. + test('accepts query as an object on POST', async () => { + const res = await agent.post('/api/logsv2').send({ query: { sql: 'select 1', from: 0, size: 10 } }); + + expect(res.status).toBe(200); + expect(forwarded.logsv2.at(-1).query).toEqual({ sql: 'select 1', from: 0, size: 10 }); + }); + + test('accepts query as a string on GET', async () => { + const res = await agent.get('/api/logsv2').query({ query: 'select 1' }); + + expect(res.status).toBe(200); + expect(forwarded.logsv2.at(-1).query).toBe('select 1'); + }); + + // Coercing mode narrows a scalar into the string half of the union rather than + // rejecting it; the route is a pass-through, so normalising is the useful behaviour. + test('coerces a scalar query into the string branch of the union', async () => { + const res = await agent.post('/api/logsv2').send({ query: 42 }); + + expect(res.status).toBe(200); + expect(forwarded.logsv2.at(-1).query).toBe('42'); + }); + + test('rejects a query that matches neither branch of the union', async () => { + const res = await agent.post('/api/logsv2').send({ query: ['select 1'] }); + + expect(res.status).toBe(400); + expect(res.body.error).toContain('/query'); + }); + + // The route runs POST bodies through coercing mode too, so a client that spells a + // numeric field as a string is normalised rather than rejected. + test('coerces numeric fields in a POST body', async () => { + const res = await agent.post('/api/logsv2').send({ sql: 'select 1', size: '25', from: '5' }); + + expect(res.status).toBe(200); + expect(forwarded.logsv2.at(-1).size).toBe(25); + expect(forwarded.logsv2.at(-1).from).toBe(5); + }); + + test('accepts a searchAround payload', async () => { + const res = await agent.post('/api/logsv2').send({ key: 'abc', stream_name: 'logs', size: 10 }); + + expect(res.status).toBe(200); + expect(forwarded.logsv2.at(-1).key).toBe('abc'); + }); +}); + +describe('PUT /api/app_builder/marketplace_releases/sync', () => { + const url = '/api/app_builder/marketplace_releases/sync'; + + test('forwards a complete upload', async () => { + const res = await agent + .put(url) + .field('path', 'views/pages/index.liquid') + .attach('marketplace_builder_file_body', Buffer.from('hello'), 'index.liquid'); + + expect(res.status).toBe(200); + expect(forwarded.sync.at(-1).path).toBe('views/pages/index.liquid'); + }); + + test('rejects a missing path', async () => { + const res = await agent + .put(url) + .attach('marketplace_builder_file_body', Buffer.from('hello'), 'index.liquid'); + + expect(res.status).toBe(400); + expect(res.body.error).toContain("missing required property 'path'"); + }); + + // Previously this dereferenced undefined and surfaced as an unhandled 500. + test('rejects a missing file with 400 rather than crashing', async () => { + const res = await agent.put(url).field('path', 'views/pages/index.liquid'); + + expect(res.status).toBe(400); + expect(res.body.error).toContain('marketplace_builder_file_body'); + }); +}); + +// A schema that will not compile is our defect, not the caller's, so the GUI routes must +// answer 500 rather than 400 — while still refusing to forward the request upstream. +describe('uncompilable schema on a GUI route', () => { + test('answers 500 and does not call the gateway', async () => { + const express = (await import('express')).default; + const bodyParser = (await import('body-parser')).default; + const { validate } = await import('#lib/validation/index.js'); + + let forwardedCalls = 0; + const app = express(); + app.use(bodyParser.json()); + app.post('/broken', (req, res) => { + const result = validate({ type: 'not-a-real-type' }, req.body); + if (!result.valid) { + return res.status(result.schemaError ? 500 : 400).json({ error: `Invalid request: ${result.message}` }); + } + forwardedCalls += 1; + return res.json({ ok: true }); + }); + + const res = await request(app).post('/broken').send({ anything: true }); + + expect(res.status).toBe(500); + expect(res.body.error).toContain('Schema failed to compile'); + expect(forwardedCalls).toBe(0); + }); +}); diff --git a/test/unit/validation.test.js b/test/unit/validation.test.js new file mode 100644 index 00000000..a1152439 --- /dev/null +++ b/test/unit/validation.test.js @@ -0,0 +1,127 @@ +import { describe, test, expect } from 'vitest'; +import { validate } from '#lib/validation/index.js'; + +const schema = { + type: 'object', + additionalProperties: false, + required: ['name'], + properties: { + name: { type: 'string', minLength: 1 }, + count: { type: 'integer', minimum: 0 }, + enabled: { type: 'boolean' } + } +}; + +describe('validate', () => { + test('accepts a conforming object', () => { + const result = validate(schema, { name: 'core', count: 2 }); + expect(result.valid).toBe(true); + expect(result.errors).toBeUndefined(); + }); + + test('reports a missing required property by name', () => { + const result = validate(schema, { count: 1 }); + expect(result.valid).toBe(false); + expect(result.message).toContain("missing required property 'name'"); + }); + + test('reports an unknown property by name', () => { + const result = validate(schema, { name: 'core', bogus: true }); + expect(result.valid).toBe(false); + expect(result.message).toContain("unknown property 'bogus'"); + }); + + test('rejects a wrong type and points at the property', () => { + const result = validate(schema, { name: 'core', count: 'many' }); + expect(result.valid).toBe(false); + expect(result.errors[0].path).toBe('/count'); + }); + + test('rejects a non-object payload', () => { + expect(validate(schema, 'core').valid).toBe(false); + expect(validate(schema, ['core']).valid).toBe(false); + expect(validate(schema, null).valid).toBe(false); + }); + + test('collects every error, not just the first', () => { + const result = validate(schema, { count: -1, enabled: 'yes' }); + expect(result.errors.length).toBeGreaterThanOrEqual(3); + }); + + test('strict mode leaves the caller data untouched', () => { + const data = { name: 'core', count: 2 }; + validate(schema, data); + expect(data).toEqual({ name: 'core', count: 2 }); + }); + + test('coercing mode converts query-string values in place', () => { + const data = { name: 'core', count: '42', enabled: 'true' }; + const result = validate(schema, data, { mode: 'coercing' }); + + expect(result.valid).toBe(true); + expect(data.count).toBe(42); + expect(data.enabled).toBe(true); + }); + + test('coercing mode still rejects values that cannot be coerced', () => { + const result = validate(schema, { name: 'core', count: 'many' }, { mode: 'coercing' }); + expect(result.valid).toBe(false); + }); + + test('flags an uncompilable schema as our defect rather than bad input', () => { + const result = validate({ type: 'not-a-real-type' }, {}); + expect(result.valid).toBe(false); + expect(result.schemaError).toBe(true); + }); + + // schemaError routes to 500 / -32603, which blames our schema. An unknown mode is a + // caller bug and a boolean schema is perfectly legal, so neither belongs on that path. + test('throws on an unknown mode rather than blaming the schema', () => { + expect(() => validate(schema, { name: 'core' }, { mode: 'nope' })).toThrow(RangeError); + expect(() => validate(schema, { name: 'core' }, { mode: 'nope' })).toThrow(/Unknown validation mode/); + }); + + test('accepts boolean schemas, which cannot key the compile cache', () => { + expect(validate(true, { anything: 1 })).toEqual({ valid: true }); + + const rejected = validate(false, { anything: 1 }); + expect(rejected.valid).toBe(false); + expect(rejected.schemaError).toBeUndefined(); + }); + + test('does not return a data field', () => { + expect(validate(schema, { name: 'core' })).not.toHaveProperty('data'); + expect(validate(schema, {})).not.toHaveProperty('data'); + }); + + test('caps the summary message but keeps every error', () => { + const wide = { + type: 'object', + properties: Object.fromEntries('abcdefgh'.split('').map(k => [k, { type: 'string' }])) + }; + const data = Object.fromEntries('abcdefgh'.split('').map(k => [k, 1])); + const result = validate(wide, data); + + expect(result.errors).toHaveLength(8); + expect(result.message).toContain('(+3 more)'); + }); + + test('compiles each schema once and reuses it', () => { + // Counting compiles directly: a schema object validated twice must reach Ajv once. + const counted = { type: 'object', properties: { name: { type: 'string' } } }; + const seen = []; + const proxied = new Proxy(counted, { + get(target, prop, receiver) { + seen.push(prop); + return Reflect.get(target, prop, receiver); + } + }); + + validate(proxied, { name: 'a' }); + const afterFirst = seen.length; + validate(proxied, { name: 'b' }); + + expect(afterFirst).toBeGreaterThan(0); // first call compiled, so it read the schema + expect(seen.length).toBe(afterFirst); // second call read nothing: cache hit + }); +});