From c13197d0381c0fad314f452dc8c3ddbad06018ac Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Tue, 15 Sep 2026 20:26:24 +0100 Subject: [PATCH 1/5] feat: accept an org parameter on the execute tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The org came from KOSLI_ORG at startup, so reaching a second org meant restarting the server with different env vars. A per-call override already existed inside the free-form `params` record, but nothing in the tool schema told the model it was there, and passing an org to one of the four actions that are not org-scoped silently turned it into a junk query parameter. Promote `org` to a first-class optional input on execute_read_action and execute_write_action. It applies to that call only; omitting it keeps the KOSLI_ORG default, and `params.org` still works. Being a top-level input also means the target org is visible in the client's write-approval prompt instead of buried in a params blob. `normalizeOrg` and `orgError` are the one place a per-call org is worked out, and every way of naming one obeys the same rules: trimmed, `null` means not supplied (as `buildUrl` already treated it), and the value that was checked is the value that goes out. A blank org is rejected instead of producing a URL with the organization missing from it, and an org aimed at a non-org-scoped action is rejected instead of riding along as a stray query parameter. A call can name an org three ways — the tool input, `params.org`, and an `org` inside a write's request body — and two different names are now rejected rather than resolved, because the same code path performs writes. That needs `params.org` read before `unwrapBodyParam` and the body's org after it: the unwrap flattens a write's body over the top level, so reading once let the spread pick a winner and sent the write to an org nobody approved. Closes #8 Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 1 + README.md | 29 ++- src/index.ts | 19 +- src/tools/execute-action.ts | 68 ++++++- test/catalog.test.ts | 27 +++ test/fixtures/catalog-subset.json | 11 ++ test/tools/execute-action.test.ts | 284 ++++++++++++++++++++++++++++++ 7 files changed, 431 insertions(+), 8 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 6102f28..39724bb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -27,6 +27,7 @@ A Model Context Protocol (MCP) server that exposes the Kosli API to LLM clients - ESM only (`"type": "module"`, `NodeNext` resolution). Relative imports must use `.js` extensions even though the source is `.ts`. - TypeScript `strict: true`. Don't weaken it. - The `org` path parameter falls back to `config.org` (from `KOSLI_ORG`). Preserve this in `KosliClient.buildUrl`. +- A per-call org is interpreted in exactly one place: `normalizeOrg` + `orgError` in `src/tools/execute-action.ts`, which both execute tools go through (the `KOSLI_ORG` default is separate, and is not normalized — see `loadConfig`). The `org` tool input and `params.org` obey the same rules — trimmed, `null` means not supplied, blank rejected, an org on a non-org-scoped action rejected, and two different orgs in one call rejected rather than resolved, because the same path performs writes. Note the ordering: `params.org` is read *before* `unwrapBodyParam` and the body's org *after* it — the unwrap flattens a write's body over the top level, so reading only once lets the spread pick a winner and hides the disagreement. Keep that decision in the one place; don't re-implement it per tool in `index.ts`. - Errors from the Kosli API are returned as `{ error: true, status, statusText, message }` — not thrown. Tools stringify whatever they get. Keep this contract; the LLM handles the error object. - Responses are serialized with `JSON.stringify(result)` (compact, no pretty-printing) to minimize token usage. Don't revert to pretty-printing. - All API requests include `User-Agent: kosli-mcp-server/` for server-side tracking, with the version coming from `VERSION` in `src/version.ts`. Preserve the header and keep the version in it — the backend uses it to tell releases apart. diff --git a/README.md b/README.md index a449694..28c4f22 100644 --- a/README.md +++ b/README.md @@ -107,7 +107,34 @@ Typical LLM flow: > [!IMPORTANT] > `execute_write_action` creates, modifies, and deletes real resources in your Kosli organization. MCP clients gate these calls behind an approval prompt — read the action ID and parameters before approving. An LLM may select the wrong action, or the right action with the wrong parameters, and approval is the only checkpoint before the call is made. Treat deletions and anything touching service accounts or API keys with particular care. -The `org` path parameter defaults to `KOSLI_ORG` if not supplied. For `GET`/`DELETE`, non-path params become query parameters; for other methods they become the JSON body. +For `GET`/`DELETE`, non-path params become query parameters; for other methods they become the JSON body. + +### Working with more than one organization + +Both execute tools take an optional `org`, so a single running server can reach +any organization your token has access to — useful when you are moving between +orgs in one conversation rather than restarting the server for each: + +```json +{ "actionId": "list_envs", "org": "cyber-dojo", "fields": ["name"] } +``` + +- Omit it and the call goes to `KOSLI_ORG`. +- It applies to that one call. The next call goes back to `KOSLI_ORG`. +- Surrounding whitespace is trimmed, and a blank org is rejected rather than + quietly producing a URL with the organization missing from it. +- Four actions are not organization-scoped (`get_user_default_org`, + `list_system_attestation_types`, and the two `/schemas/...` actions). Passing + an org to one of those is rejected rather than ignored, whether it arrives as + `org` or inside `params`. +- There are three ways to name an org in one call: this input, `params.org`, and + an `org` inside a write's request body. They must agree; two different names + are rejected rather than resolved, because the same tools perform writes and + the target is never guessed. +- An org your token cannot reach comes back as the usual error object carrying + the API's status — `403` for both an org you are not a member of and one that + does not exist. Note that Kosli staff accounts can read any org, so a + successful read does not prove a write would be permitted. Both execute tools accept an optional `fields` array to request only specific top-level fields from each object in the response. This dramatically reduces response size and token usage: diff --git a/src/index.ts b/src/index.ts index 2f3a35b..4c721b6 100644 --- a/src/index.ts +++ b/src/index.ts @@ -13,6 +13,11 @@ import { VERSION } from "./version.js"; const entries = catalog as CatalogEntry[]; const config = loadConfig(); +// Shared by both execute tools so the two can't drift apart. +const ORG_INPUT = z.string().optional().describe( + `Kosli organization to run this call against, e.g. "cyber-dojo". Defaults to "${config.org}" (from KOSLI_ORG). It applies to this call only — the next call goes back to the default unless you set it again.`, +); + const server = new McpServer({ name: "kosli", version: VERSION, @@ -48,18 +53,19 @@ server.registerTool( "execute_read_action", { title: "Execute read action", - description: `Execute a read-only (GET) Kosli API action by its ID with the given parameters. Use search_actions first to find the action ID and required parameters. The 'org' parameter defaults to "${config.org}" — you do not need to supply it unless querying a different organization.`, + description: `Execute a read-only (GET) Kosli API action by its ID with the given parameters. Use search_actions first to find the action ID and required parameters. The call runs against the "${config.org}" organization unless you set 'org'.`, annotations: { readOnlyHint: true, }, inputSchema: { actionId: z.string().describe("The action ID from search_actions results"), + org: ORG_INPUT, params: z.record(z.string(), z.unknown()).optional().default({}).describe("Parameters for the action (path params or query params)"), fields: z.array(z.string()).optional().describe("Only include these fields in each object of the response. Dramatically reduces response size. Example: [\"name\",\"compliant\",\"fingerprint\",\"reasons_for_incompliance\"]"), }, }, - async ({ actionId, params, fields }) => { - const result = await executeAction(entries, config, actionId, params, fields, undefined, "GET"); + async ({ actionId, params, fields, org }) => { + const result = await executeAction(entries, config, actionId, params, fields, undefined, "GET", org); return { content: [ { @@ -75,19 +81,20 @@ server.registerTool( "execute_write_action", { title: "Execute write action", - description: `Execute a write (POST, PUT, PATCH, DELETE) Kosli API action by its ID with the given parameters. Use search_actions first to find the action ID and required parameters. The 'org' parameter defaults to "${config.org}" — you do not need to supply it unless querying a different organization.`, + description: `Execute a write (POST, PUT, PATCH, DELETE) Kosli API action by its ID with the given parameters. Use search_actions first to find the action ID and required parameters. The call runs against the "${config.org}" organization unless you set 'org' — check it before approving, the write lands in whichever org it names.`, annotations: { destructiveHint: true, readOnlyHint: false, }, inputSchema: { actionId: z.string().describe("The action ID from search_actions results"), + org: ORG_INPUT, params: z.record(z.string(), z.unknown()).optional().default({}).describe("Parameters for the action (path params, query params, or body)"), fields: z.array(z.string()).optional().describe("Only include these fields in each object of the response. Dramatically reduces response size."), }, }, - async ({ actionId, params, fields }) => { - const result = await executeAction(entries, config, actionId, params, fields, undefined, "WRITE"); + async ({ actionId, params, fields, org }) => { + const result = await executeAction(entries, config, actionId, params, fields, undefined, "WRITE", org); return { content: [ { diff --git a/src/tools/execute-action.ts b/src/tools/execute-action.ts index ce16dce..e6dba38 100644 --- a/src/tools/execute-action.ts +++ b/src/tools/execute-action.ts @@ -56,6 +56,49 @@ function unwrapBodyParam( return { ...rest, ...(body as Record) }; } +/** + * The org a call is aimed at, as it will actually be used: trimmed, and + * `undefined` when nothing was supplied. `null` counts as nothing, matching + * `KosliClient.buildUrl`, which falls back to `config.org` on a nullish + * `params.org`. + */ +function normalizeOrg(value: unknown): string | undefined { + if (value === undefined || value === null) return undefined; + return String(value).trim(); +} + +/** + * Check the orgs a call names, returning a message when the request cannot be + * honoured as written. This is the only place the target org is decided, so + * both execute tools behave identically and every way of naming one obeys the + * same rules. + * + * `named` holds the distinct orgs the call mentions, already normalized. There + * are three ways to name one and they must agree: the `org` argument, + * `params.org`, and — for a write — an `org` inside the request body, which + * `unwrapBodyParam` flattens over the top level. Two different names are + * rejected rather than resolved: the same code path performs writes, and + * guessing wrong would write to the wrong organization. + */ +function orgError(entry: CatalogEntry, named: string[]): string | undefined { + const takesOrg = entry.parameters.some((p) => p.name === "org" && p.in === "path"); + if (!takesOrg) { + if (named.length === 0) return undefined; + return `Action "${entry.id}" is not organization-scoped — it takes no org. Retry without the org parameter.`; + } + + if (named.length > 1) { + const quoted = named.map((o) => `"${o}"`).join(" and "); + return `Conflicting orgs in one call: ${quoted}. The org parameter, params.org, and an org in the request body must agree — supply just one.`; + } + + if (named[0] === "") { + return "An empty org was given. Name an organization, or omit the org parameter to use the configured default."; + } + + return undefined; +} + export async function executeAction( catalog: CatalogEntry[], config: Config, @@ -64,6 +107,7 @@ export async function executeAction( fields?: string[], fetchFn: FetchFn = globalThis.fetch, mode?: ToolMode, + org?: string, ): Promise { const entry = catalog.find((e) => e.id === actionId); if (!entry) { @@ -84,8 +128,30 @@ export async function executeAction( }; } + // Collect the org from every channel before deciding. `unwrapBodyParam` + // flattens a write's request body over the top level, so params.org has to be + // read before the unwrap and the body's org after it — otherwise the spread + // picks a winner and the disagreement is never seen. + const beforeUnwrap = normalizeOrg(params.org); + const unwrapped = unwrapBodyParam(entry, params); + const named = [...new Set( + [normalizeOrg(org), beforeUnwrap, normalizeOrg(unwrapped.org)] + .filter((o): o is string => o !== undefined), + )]; + + const message = orgError(entry, named); + if (message) return { error: true, message }; + + // The checked value is the one that goes out, so buildUrl cannot see a + // different org from the one that was approved. With nothing named, the key + // is dropped rather than left as a null that would ride along as a stray + // query parameter. + const withOrg = { ...unwrapped }; + if (named.length === 0) delete withOrg.org; + else withOrg.org = named[0]; + const client = new KosliClient(config, fetchFn); - const result = await client.execute(entry, unwrapBodyParam(entry, params)); + const result = await client.execute(entry, withOrg); // Never strip error shapes — pickFields would reduce them to {} // and hide the failure from the LLM. diff --git a/test/catalog.test.ts b/test/catalog.test.ts index 6342b46..87f72a3 100644 --- a/test/catalog.test.ts +++ b/test/catalog.test.ts @@ -17,4 +17,31 @@ describe("catalog.json", () => { const ids = catalog.map((entry) => entry.id); expect(new Set(ids).size).toBe(ids.length); }); + + // README names these four as the actions that reject an `org`. If the spec + // gains or loses one, update the README in the same PR as the regenerated + // catalog — otherwise the docs quietly start lying. + it("has exactly the four documented actions that take no org", () => { + const withoutOrg = catalog + .filter((entry) => !entry.parameters.some((p) => p.name === "org" && p.in === "path")) + .map((entry) => entry.id) + .sort(); + + expect(withoutOrg).toEqual([ + "environment_policy_schema_v1", + "flow_template_schema_v1", + "get_user_default_org", + "list_system_attestation_types", + ]); + }); + + // buildUrl only fills `{org}` from a path parameter; an org arriving any + // other way would bypass the checks in executeAction. + it("never declares org as a query or header parameter", () => { + const offenders = catalog + .filter((entry) => entry.parameters.some((p) => p.name === "org" && p.in !== "path")) + .map((entry) => entry.id); + + expect(offenders).toEqual([]); + }); }); diff --git a/test/fixtures/catalog-subset.json b/test/fixtures/catalog-subset.json index a546345..f861167 100644 --- a/test/fixtures/catalog-subset.json +++ b/test/fixtures/catalog-subset.json @@ -125,5 +125,16 @@ { "name": "body", "required": true, "description": "Request body (multipart/form-data)" } ], "searchText": "create or update policy create or update a policy in an organization. put policies org policies" + }, + { + "id": "get_user_default_org", + "method": "GET", + "path": "/user/default-org", + "summary": "Get default organization", + "description": "Get the default org for the current user.", + "tags": ["User"], + "parameters": [], + "requestBody": null, + "searchText": "get default organization get the default org for the current user. get user default-org user" } ] diff --git a/test/tools/execute-action.test.ts b/test/tools/execute-action.test.ts index e659921..ed0ae4a 100644 --- a/test/tools/execute-action.test.ts +++ b/test/tools/execute-action.test.ts @@ -292,3 +292,287 @@ describe("request body unwrapping", () => { ); }); }); + +describe("org selection", () => { + function mockFetchOk(body: unknown = { ok: true }) { + return vi.fn().mockResolvedValue({ + ok: true, + status: 200, + json: () => Promise.resolve(body), + }); + } + + it("targets the org given as the org parameter", async () => { + const mockFetch = mockFetchOk(); + + await executeAction(entries, config, "list_environments", {}, undefined, mockFetch, "GET", "cyber-dojo"); + + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/environments/cyber-dojo", + expect.anything(), + ); + }); + + it("falls back to the configured org when none is given", async () => { + const mockFetch = mockFetchOk(); + + await executeAction(entries, config, "list_environments", {}, undefined, mockFetch, "GET"); + + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/environments/test-org", + expect.anything(), + ); + }); + + it("still accepts an org supplied inside params", async () => { + const mockFetch = mockFetchOk(); + + await executeAction(entries, config, "list_environments", { org: "cyber-dojo" }, undefined, mockFetch, "GET"); + + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/environments/cyber-dojo", + expect.anything(), + ); + }); + + it("accepts the same org in both places", async () => { + const mockFetch = mockFetchOk(); + + await executeAction( + entries, config, "list_environments", { org: "cyber-dojo" }, + undefined, mockFetch, "GET", "cyber-dojo", + ); + + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/environments/cyber-dojo", + expect.anything(), + ); + }); + + it("rejects conflicting orgs without calling the API", async () => { + const mockFetch = vi.fn(); + + const result = await executeAction( + entries, config, "list_environments", { org: "kosli-public" }, + undefined, mockFetch, "GET", "cyber-dojo", + ); + + expect(result).toEqual({ + error: true, + message: + 'Conflicting orgs in one call: "cyber-dojo" and "kosli-public". The org parameter, params.org, and an org in the request body must agree — supply just one.', + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("rejects a blank org rather than building a URL with a missing segment", async () => { + const mockFetch = vi.fn(); + + const result = await executeAction( + entries, config, "list_environments", {}, + undefined, mockFetch, "GET", " ", + ); + + expect(result).toEqual({ + error: true, + message: + "An empty org was given. Name an organization, or omit the org parameter to use the configured default.", + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("rejects an org on an action that is not organization-scoped", async () => { + const mockFetch = vi.fn(); + + const result = await executeAction( + entries, config, "get_user_default_org", {}, + undefined, mockFetch, "GET", "cyber-dojo", + ); + + expect(result).toEqual({ + error: true, + message: + 'Action "get_user_default_org" is not organization-scoped — it takes no org. Retry without the org parameter.', + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("calls a non-org-scoped action when no org is given", async () => { + const mockFetch = mockFetchOk({ default_org_name: "test-org" }); + + const result = await executeAction(entries, config, "get_user_default_org", {}, undefined, mockFetch, "GET"); + + expect(result).toEqual({ default_org_name: "test-org" }); + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/user/default-org", + expect.anything(), + ); + }); + + it("trims a padded org rather than encoding the padding into the path", async () => { + const mockFetch = mockFetchOk(); + + await executeAction( + entries, config, "list_environments", {}, + undefined, mockFetch, "GET", " cyber-dojo ", + ); + + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/environments/cyber-dojo", + expect.anything(), + ); + }); + + it("applies the same trimming to an org supplied inside params", async () => { + const mockFetch = mockFetchOk(); + + await executeAction( + entries, config, "list_environments", { org: " cyber-dojo " }, + undefined, mockFetch, "GET", + ); + + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/environments/cyber-dojo", + expect.anything(), + ); + }); + + it("rejects a blank org supplied inside params, rather than dropping the path segment", async () => { + const mockFetch = vi.fn(); + + const result = await executeAction( + entries, config, "list_environments", { org: "" }, + undefined, mockFetch, "GET", + ); + + expect(result).toEqual({ + error: true, + message: + "An empty org was given. Name an organization, or omit the org parameter to use the configured default.", + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("treats a null org in params as not supplied, as buildUrl does", async () => { + const mockFetch = mockFetchOk(); + + await executeAction( + entries, config, "list_environments", { org: null }, + undefined, mockFetch, "GET", + ); + + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/environments/test-org", + expect.anything(), + ); + }); + + it("rejects an org nested in the request body that contradicts the org parameter", async () => { + const mockFetch = vi.fn(); + + const result = await executeAction( + entries, config, "post_control", + { body: { org: "other-org", identifier: "ctrl-1" } }, + undefined, mockFetch, "WRITE", "cyber-dojo", + ); + + expect(result).toEqual({ + error: true, + message: + 'Conflicting orgs in one call: "cyber-dojo" and "other-org". The org parameter, params.org, and an org in the request body must agree — supply just one.', + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("rejects an org supplied only inside params for a non-org-scoped action", async () => { + const mockFetch = vi.fn(); + + const result = await executeAction( + entries, config, "get_user_default_org", { org: "cyber-dojo" }, + undefined, mockFetch, "GET", + ); + + expect(result).toEqual({ + error: true, + message: + 'Action "get_user_default_org" is not organization-scoped — it takes no org. Retry without the org parameter.', + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("rejects a request body org that contradicts params.org, with no org parameter", async () => { + const mockFetch = vi.fn(); + + const result = await executeAction( + entries, config, "post_control", + { org: "approved-org", body: { org: "other-org", identifier: "ctrl-1" } }, + undefined, mockFetch, "WRITE", + ); + + expect(result).toEqual({ + error: true, + message: + 'Conflicting orgs in one call: "approved-org" and "other-org". The org parameter, params.org, and an org in the request body must agree — supply just one.', + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("rejects the same contradiction on the multipart write branch", async () => { + const mockFetch = vi.fn(); + + const result = await executeAction( + entries, config, "put_policy", + { org: "approved-org", body: { org: "other-org", name: "policy-1" } }, + undefined, mockFetch, "WRITE", + ); + + expect(result).toEqual({ + error: true, + message: + 'Conflicting orgs in one call: "approved-org" and "other-org". The org parameter, params.org, and an org in the request body must agree — supply just one.', + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("targets the org on a multipart write without adding it as a form field", async () => { + const mockFetch = mockFetchOk(); + + await executeAction( + entries, config, "put_policy", { body: { name: "policy-1" } }, + undefined, mockFetch, "WRITE", "cyber-dojo", + ); + + const [url, init] = mockFetch.mock.calls[0]; + expect(url).toBe("https://app.kosli.com/api/v2/policies/cyber-dojo"); + expect([...(init.body as FormData).keys()]).toEqual(["name"]); + }); + + it("drops a null org instead of forwarding it as a query parameter", async () => { + const mockFetch = mockFetchOk({ default_org_name: "test-org" }); + + await executeAction( + entries, config, "get_user_default_org", { org: null }, + undefined, mockFetch, "GET", + ); + + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/user/default-org", + expect.anything(), + ); + }); + + it("applies to writes, and the org does not leak into the request body", async () => { + const mockFetch = mockFetchOk({ created: true }); + const body = { identifier: "ctrl-1", name: "Control 1" }; + + await executeAction( + entries, config, "post_control", { body }, + undefined, mockFetch, "WRITE", "cyber-dojo", + ); + + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/controls/cyber-dojo", + expect.objectContaining({ body: JSON.stringify(body) }), + ); + }); +}); From d0130ca8f0e1895b591ba72a9fc3c359d81f787f Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Tue, 15 Sep 2026 21:08:52 +0100 Subject: [PATCH 2/5] fix: trim KOSLI_ORG, and pin the org assumptions the tool layer relies on Two gaps found reviewing #63. `loadConfig` rejected an empty `KOSLI_ORG` but never trimmed it. `" "` is truthy, so it passed the check and became the org path segment on every call. Trimming before the check makes the existing guard cover it. The message now says the value must not be blank, because the Desktop extension collects this in a free-text field where a stray space is easy to type. The catalog guard asserted `org` is never a query or header parameter, but not that it is never a request body field. `executeAction` reads an `org` in a write's body as the target org: `unwrapBodyParam` flattens the body over the top level, so the value moves into the path segment and leaves the payload. A future regenerated catalog with a body field genuinely called `org` would be hijacked that way with nothing failing. `bodyFields` decides what the guard can see. It follows schema composition rather than reading `properties` directly, because `resolve-refs.ts` inlines `$ref`s but copies `allOf` through untouched and two entries already carry one. It also collects the positions that name a field without declaring it: `required`, and both halves of the keywords that pull a schema or a name list in behind a trigger field. It does not walk `not`, where naming a field means requiring its absence, and it does not descend into a field's own properties: `unwrapBodyParam` flattens exactly one level, so a field an object deeper can never become the org. The walk does run on today's catalog, but every name it reaches is already in the root `properties`, so removing it would change no verdict and the suite would stay green. Direct tests pin each branch instead, so a later simplification cannot quietly revert the guard to reading `properties` alone. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 2 +- README.md | 2 +- src/config.ts | 6 ++- test/catalog.test.ts | 117 +++++++++++++++++++++++++++++++++++++++++++ test/config.test.ts | 14 ++++++ 5 files changed, 137 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 39724bb..518de35 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -27,7 +27,7 @@ A Model Context Protocol (MCP) server that exposes the Kosli API to LLM clients - ESM only (`"type": "module"`, `NodeNext` resolution). Relative imports must use `.js` extensions even though the source is `.ts`. - TypeScript `strict: true`. Don't weaken it. - The `org` path parameter falls back to `config.org` (from `KOSLI_ORG`). Preserve this in `KosliClient.buildUrl`. -- A per-call org is interpreted in exactly one place: `normalizeOrg` + `orgError` in `src/tools/execute-action.ts`, which both execute tools go through (the `KOSLI_ORG` default is separate, and is not normalized — see `loadConfig`). The `org` tool input and `params.org` obey the same rules — trimmed, `null` means not supplied, blank rejected, an org on a non-org-scoped action rejected, and two different orgs in one call rejected rather than resolved, because the same path performs writes. Note the ordering: `params.org` is read *before* `unwrapBodyParam` and the body's org *after* it — the unwrap flattens a write's body over the top level, so reading only once lets the spread pick a winner and hides the disagreement. Keep that decision in the one place; don't re-implement it per tool in `index.ts`. +- A per-call org is interpreted in exactly one place: `normalizeOrg` + `orgError` in `src/tools/execute-action.ts`, which both execute tools go through (the `KOSLI_ORG` default never passes through them: `loadConfig` trims it and rejects a blank one at startup, and `KosliClient.buildUrl` falls back to it when a call names no org). The `org` tool input and `params.org` obey the same rules — trimmed, `null` means not supplied, blank rejected, an org on a non-org-scoped action rejected, and two different orgs in one call rejected rather than resolved, because the same path performs writes. Note the ordering: `params.org` is read *before* `unwrapBodyParam` and the body's org *after* it — the unwrap flattens a write's body over the top level, so reading only once lets the spread pick a winner and hides the disagreement. Keep that decision in the one place; don't re-implement it per tool in `index.ts`. - Errors from the Kosli API are returned as `{ error: true, status, statusText, message }` — not thrown. Tools stringify whatever they get. Keep this contract; the LLM handles the error object. - Responses are serialized with `JSON.stringify(result)` (compact, no pretty-printing) to minimize token usage. Don't revert to pretty-printing. - All API requests include `User-Agent: kosli-mcp-server/` for server-side tracking, with the version coming from `VERSION` in `src/version.ts`. Preserve the header and keep the version in it — the backend uses it to tell releases apart. diff --git a/README.md b/README.md index 28c4f22..fa58f08 100644 --- a/README.md +++ b/README.md @@ -32,7 +32,7 @@ The server reads configuration from environment variables: | Variable | Required | Default | Notes | |----------|----------|---------|-------| | `KOSLI_API_TOKEN` | yes | — | Preferred. `KOSLI_API_KEY` is accepted as a fallback. | -| `KOSLI_ORG` | yes | — | Default org used when a path param `org` is not supplied. | +| `KOSLI_ORG` | yes | — | Default org used when a call does not supply one. Trimmed; whitespace-only is rejected at startup. | | `KOSLI_BASE_URL` | no | `https://app.kosli.com` | EU (default), US (`https://app.us.kosli.com`), or your single-tenant endpoint. | ## Wire up to an MCP client diff --git a/src/config.ts b/src/config.ts index 74f92a1..0447503 100644 --- a/src/config.ts +++ b/src/config.ts @@ -6,9 +6,11 @@ export function loadConfig(): Config { throw new Error("KOSLI_API_TOKEN (or KOSLI_API_KEY) environment variable is required"); } - const org = process.env.KOSLI_ORG; + // Trimmed so the existing check also catches a whitespace-only value, which + // is truthy and would otherwise become the org path segment on every call. + const org = process.env.KOSLI_ORG?.trim(); if (!org) { - throw new Error("KOSLI_ORG environment variable is required"); + throw new Error("KOSLI_ORG environment variable is required, and must not be blank"); } const baseUrl = process.env.KOSLI_BASE_URL || "https://app.kosli.com"; diff --git a/test/catalog.test.ts b/test/catalog.test.ts index 87f72a3..8a1a2fa 100644 --- a/test/catalog.test.ts +++ b/test/catalog.test.ts @@ -44,4 +44,121 @@ describe("catalog.json", () => { expect(offenders).toEqual([]); }); + + // executeAction reads an `org` inside a write's request body as the target + // org: unwrapBodyParam flattens the body over the top level, so the value + // moves into the path segment and leaves the payload. A body field genuinely + // called "org" and meaning something else would be hijacked that way. + it("never declares org as a request body property", () => { + const offenders = catalog + .filter((entry) => entry.requestBody?.some((body) => bodyFields(body.schema).includes("org"))) + .map((entry) => entry.id); + + expect(offenders).toEqual([]); + }); +}); + +// The field names a request body contributes at its top level, which are the +// ones unwrapBodyParam can turn into params — it flattens exactly one level, so +// a field nested inside another object is not one of them. A schema can name +// them directly, through `required`, or through composition, and the generator +// copies composition through untouched (see scripts/resolve-refs.ts, and the +// `allOf` already on create_artifact and post_override_attestation). +// +// `not` is deliberately not walked: a branch saying a field must be ABSENT does +// not name it. Known limit: a `patternProperties` at a body root whose pattern +// happens to admit "org" names the field by pattern rather than by name, so it +// is not collected. The catalog's 22 uses all sit inside nested property +// values, which are out of scope anyway. +function bodyFields(schema: unknown): string[] { + if (schema === null || typeof schema !== "object") return []; + const node = schema as Record; + + const declared = + node.properties !== null && typeof node.properties === "object" + ? Object.keys(node.properties as Record) + : []; + + const composed = ["allOf", "anyOf", "oneOf", "if", "then", "else"].flatMap((keyword) => { + const branch = node[keyword]; + return Array.isArray(branch) ? branch.flatMap(bodyFields) : bodyFields(branch); + }); + + // These map a trigger field to what it pulls in. Both halves name top-level + // fields: the key is the trigger, which is a field of the body by definition, + // and the value is either a schema or (draft-07 `dependencies`, and + // `dependentRequired`) a bare list of field names. + const dependent = ["dependentSchemas", "dependentRequired", "dependencies"].flatMap((keyword) => { + const branch = node[keyword]; + if (branch === null || typeof branch !== "object" || Array.isArray(branch)) return []; + const map = branch as Record; + return [...Object.keys(map), ...Object.values(map).flatMap(dependentFields)]; + }); + + return [...declared, ...names(node.required), ...composed, ...dependent]; +} + +function dependentFields(value: unknown): string[] { + return Array.isArray(value) ? names(value) : bodyFields(value); +} + +function names(value: unknown): string[] { + return Array.isArray(value) ? value.filter((v): v is string => typeof v === "string") : []; +} + +// bodyFields decides whether the guard above can see a field at all. The walk +// does run on today's catalog, but every name it reaches is already in the root +// `properties`, so removing it changes no verdict and the suite stays green. +// These tests pin each branch directly, so a later simplification cannot +// silently revert the guard to reading `properties` alone. +describe("bodyFields", () => { + it("collects properties declared directly", () => { + expect(bodyFields({ properties: { org: {}, flow_name: {} } })).toContain("org"); + }); + + it("collects a name that only appears in required", () => { + expect(bodyFields({ anyOf: [{ required: ["org"] }] })).toContain("org"); + }); + + it.each([ + ["allOf", { allOf: [{ properties: { org: {} } }] }], + ["anyOf", { anyOf: [{ properties: { org: {} } }] }], + ["oneOf", { oneOf: [{ properties: { org: {} } }] }], + ["if", { if: { properties: { org: {} } } }], + ["then", { if: {}, then: { properties: { org: {} } } }], + ["else", { if: {}, else: { properties: { org: {} } } }], + ["nested composition", { allOf: [{ anyOf: [{ properties: { org: {} } }] }] }], + ["dependentSchemas", { dependentSchemas: { fingerprint: { properties: { org: {} } } } }], + ["dependentRequired", { dependentRequired: { fingerprint: ["org"] } }], + ["dependencies as a schema", { dependencies: { fingerprint: { properties: { org: {} } } } }], + ["dependencies as a name list", { dependencies: { fingerprint: ["org"] } }], + ["a dependency trigger key", { dependentRequired: { org: ["scope"] } }], + ])("collects a name reached through %s", (_label, schema) => { + expect(bodyFields(schema)).toContain("org"); + }); + + it("does not collect a field nested inside another object", () => { + expect(bodyFields({ properties: { repo_info: { properties: { org: {} } } } })).not.toContain("org"); + }); + + it("does not collect a field a not branch forbids", () => { + expect(bodyFields({ not: { required: ["org"] } })).not.toContain("org"); + }); + + it("survives shapes the spec should never produce", () => { + const junk = { + properties: null, + required: [1, "org"], + anyOf: "not-an-array", + oneOf: [null, 5, []], + then: 7, + dependencies: ["not", "a", "map"], + dependentSchemas: { a: null, b: "text" }, + }; + + expect(() => bodyFields(junk)).not.toThrow(); + // "org" from required; "a" and "b" are dependency trigger keys, whose + // unusable values contribute nothing further. + expect(bodyFields(junk)).toEqual(["org", "a", "b"]); + }); }); diff --git a/test/config.test.ts b/test/config.test.ts index df040e2..c6b3d21 100644 --- a/test/config.test.ts +++ b/test/config.test.ts @@ -69,6 +69,20 @@ describe("loadConfig", () => { expect(() => loadConfig()).toThrow("KOSLI_ORG"); }); + it("throws when KOSLI_ORG is only whitespace", () => { + process.env.KOSLI_API_KEY = "test-key"; + process.env.KOSLI_ORG = " "; + + expect(() => loadConfig()).toThrow("KOSLI_ORG"); + }); + + it("trims KOSLI_ORG", () => { + process.env.KOSLI_API_KEY = "test-key"; + process.env.KOSLI_ORG = " test-org "; + + expect(loadConfig().org).toBe("test-org"); + }); + it("throws when KOSLI_BASE_URL is not https", () => { process.env.KOSLI_API_KEY = "test-key"; process.env.KOSLI_ORG = "test-org"; From e27597602a1ca8cfa976adb4c081b5702341fdc4 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Wed, 16 Sep 2026 06:38:31 +0100 Subject: [PATCH 3/5] fix: require the org to be a string, rather than coercing what arrives `params` is a record of `unknown`, so an org arriving that way can be any JSON the model emits. `normalizeOrg` ran `String()` over it, which turns a value that is not a name into one that looks like a name. `["cyber-dojo", "kosli-public"]` became the org `cyber-dojo,kosli-public`, and an org id became an org called `1234`. Each is a single name, so the check for two orgs named in one call saw nothing to disagree about and the request went out. Asking for two orgs at once is the obvious way to arrive at the first, and the answer was a 403 on a nonsense org rather than the message written for that mistake. The second is worse: the org path parameter has no pattern in the catalog, so nothing rules out an org genuinely named `1234`, and a write would land in a real organization nobody named. Only a string names an org now. Everything else takes the existing rejection path, whose message covers being absent, blank, and the wrong type at once. That message is also what a caller sees when an unusable value arrives next to a real org, since an unusable value is reported before any disagreement. Reported the other way round, the advice is to drop one of two orgs when one of them was never an org. Found by the review on #63. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 2 +- README.md | 6 +++- src/tools/execute-action.ts | 25 ++++++++++++----- test/tools/execute-action.test.ts | 46 ++++++++++++++++++++++++++++--- 4 files changed, 66 insertions(+), 13 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 518de35..0c28e7a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -27,7 +27,7 @@ A Model Context Protocol (MCP) server that exposes the Kosli API to LLM clients - ESM only (`"type": "module"`, `NodeNext` resolution). Relative imports must use `.js` extensions even though the source is `.ts`. - TypeScript `strict: true`. Don't weaken it. - The `org` path parameter falls back to `config.org` (from `KOSLI_ORG`). Preserve this in `KosliClient.buildUrl`. -- A per-call org is interpreted in exactly one place: `normalizeOrg` + `orgError` in `src/tools/execute-action.ts`, which both execute tools go through (the `KOSLI_ORG` default never passes through them: `loadConfig` trims it and rejects a blank one at startup, and `KosliClient.buildUrl` falls back to it when a call names no org). The `org` tool input and `params.org` obey the same rules — trimmed, `null` means not supplied, blank rejected, an org on a non-org-scoped action rejected, and two different orgs in one call rejected rather than resolved, because the same path performs writes. Note the ordering: `params.org` is read *before* `unwrapBodyParam` and the body's org *after* it — the unwrap flattens a write's body over the top level, so reading only once lets the spread pick a winner and hides the disagreement. Keep that decision in the one place; don't re-implement it per tool in `index.ts`. +- A per-call org is interpreted in exactly one place: `normalizeOrg` + `orgError` in `src/tools/execute-action.ts`, which both execute tools go through (the `KOSLI_ORG` default never passes through them: `loadConfig` trims it and rejects a blank one at startup, and `KosliClient.buildUrl` falls back to it when a call names no org). The `org` tool input and `params.org` obey the same rules — trimmed, only a string names one (coercing would turn `["a", "b"]` into the org `a,b`), `null` means not supplied, blank rejected, an org on a non-org-scoped action rejected, and two different orgs in one call rejected rather than resolved, because the same path performs writes. Note the ordering: `params.org` is read *before* `unwrapBodyParam` and the body's org *after* it — the unwrap flattens a write's body over the top level, so reading only once lets the spread pick a winner and hides the disagreement. Keep that decision in the one place; don't re-implement it per tool in `index.ts`. - Errors from the Kosli API are returned as `{ error: true, status, statusText, message }` — not thrown. Tools stringify whatever they get. Keep this contract; the LLM handles the error object. - Responses are serialized with `JSON.stringify(result)` (compact, no pretty-printing) to minimize token usage. Don't revert to pretty-printing. - All API requests include `User-Agent: kosli-mcp-server/` for server-side tracking, with the version coming from `VERSION` in `src/version.ts`. Preserve the header and keep the version in it — the backend uses it to tell releases apart. diff --git a/README.md b/README.md index fa58f08..7f2aaee 100644 --- a/README.md +++ b/README.md @@ -121,8 +121,12 @@ orgs in one conversation rather than restarting the server for each: - Omit it and the call goes to `KOSLI_ORG`. - It applies to that one call. The next call goes back to `KOSLI_ORG`. -- Surrounding whitespace is trimmed, and a blank org is rejected rather than +- Surrounding whitespace is trimmed. A blank org is rejected rather than quietly producing a URL with the organization missing from it. +- One org, named as a string. Anything else is rejected rather than coerced, + because coercion turns a value that is not a name into one that looks like a + name: `["cyber-dojo", "kosli-public"]` would request the org + `cyber-dojo,kosli-public`, and an org id would request an org called `1234`. - Four actions are not organization-scoped (`get_user_default_org`, `list_system_attestation_types`, and the two `/schemas/...` actions). Passing an org to one of those is rejected rather than ignored, whether it arrives as diff --git a/src/tools/execute-action.ts b/src/tools/execute-action.ts index e6dba38..1ee0e45 100644 --- a/src/tools/execute-action.ts +++ b/src/tools/execute-action.ts @@ -60,11 +60,19 @@ function unwrapBodyParam( * The org a call is aimed at, as it will actually be used: trimmed, and * `undefined` when nothing was supplied. `null` counts as nothing, matching * `KosliClient.buildUrl`, which falls back to `config.org` on a nullish - * `params.org`. + * `params.org`. An empty string means "supplied, but unusable", which + * `orgError` turns into a rejection. */ function normalizeOrg(value: unknown): string | undefined { if (value === undefined || value === null) return undefined; - return String(value).trim(); + // Only a string can name an org. `params` is a record of `unknown`, so the + // model can put anything here, and coercing it would turn a value that is not + // a name into one that looks like a name: `["a", "b"]` reads as the org "a,b" + // and an org id reads as an org called "1234". Both would then pass the + // agreement check below as a single name. Hand back the unusable marker + // instead and let orgError say so. + if (typeof value !== "string") return ""; + return value.trim(); } /** @@ -84,7 +92,14 @@ function orgError(entry: CatalogEntry, named: string[]): string | undefined { const takesOrg = entry.parameters.some((p) => p.name === "org" && p.in === "path"); if (!takesOrg) { if (named.length === 0) return undefined; - return `Action "${entry.id}" is not organization-scoped — it takes no org. Retry without the org parameter.`; + return `Action "${entry.id}" is not organization-scoped — it takes no org. Retry with no org in the org parameter, in params.org, or in the request body.`; + } + + // Before any disagreement: an unusable value is the thing to report, and + // reporting it as a nameless org disagreeing with a real one would send the + // caller to drop one of the two rather than to fix the bad value. + if (named.includes("")) { + return "The org must be a single non-empty organization name. Check the org parameter, params.org, and any org in the request body, or omit all of them to use the configured default."; } if (named.length > 1) { @@ -92,10 +107,6 @@ function orgError(entry: CatalogEntry, named: string[]): string | undefined { return `Conflicting orgs in one call: ${quoted}. The org parameter, params.org, and an org in the request body must agree — supply just one.`; } - if (named[0] === "") { - return "An empty org was given. Name an organization, or omit the org parameter to use the configured default."; - } - return undefined; } diff --git a/test/tools/execute-action.test.ts b/test/tools/execute-action.test.ts index ed0ae4a..99d276c 100644 --- a/test/tools/execute-action.test.ts +++ b/test/tools/execute-action.test.ts @@ -376,7 +376,7 @@ describe("org selection", () => { expect(result).toEqual({ error: true, message: - "An empty org was given. Name an organization, or omit the org parameter to use the configured default.", + "The org must be a single non-empty organization name. Check the org parameter, params.org, and any org in the request body, or omit all of them to use the configured default.", }); expect(mockFetch).not.toHaveBeenCalled(); }); @@ -392,7 +392,7 @@ describe("org selection", () => { expect(result).toEqual({ error: true, message: - 'Action "get_user_default_org" is not organization-scoped — it takes no org. Retry without the org parameter.', + 'Action "get_user_default_org" is not organization-scoped — it takes no org. Retry with no org in the org parameter, in params.org, or in the request body.', }); expect(mockFetch).not.toHaveBeenCalled(); }); @@ -448,7 +448,7 @@ describe("org selection", () => { expect(result).toEqual({ error: true, message: - "An empty org was given. Name an organization, or omit the org parameter to use the configured default.", + "The org must be a single non-empty organization name. Check the org parameter, params.org, and any org in the request body, or omit all of them to use the configured default.", }); expect(mockFetch).not.toHaveBeenCalled(); }); @@ -467,6 +467,44 @@ describe("org selection", () => { ); }); + it.each([ + ["a list of orgs", ["cyber-dojo", "kosli-public"]], + ["a single-element list", ["cyber-dojo"]], + ["an object", {}], + ["a number, which could name a real org", 1234], + ["a boolean", true], + ])("rejects %s in params rather than coercing it into the path", async (_label, org) => { + const mockFetch = vi.fn(); + + const result = await executeAction( + entries, config, "list_environments", { org }, + undefined, mockFetch, "GET", + ); + + expect(result).toEqual({ + error: true, + message: + "The org must be a single non-empty organization name. Check the org parameter, params.org, and any org in the request body, or omit all of them to use the configured default.", + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("reports the unusable value, not a disagreement, when a real org is named too", async () => { + const mockFetch = vi.fn(); + + const result = await executeAction( + entries, config, "list_environments", { org: [] }, + undefined, mockFetch, "GET", "cyber-dojo", + ); + + expect(result).toEqual({ + error: true, + message: + "The org must be a single non-empty organization name. Check the org parameter, params.org, and any org in the request body, or omit all of them to use the configured default.", + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + it("rejects an org nested in the request body that contradicts the org parameter", async () => { const mockFetch = vi.fn(); @@ -495,7 +533,7 @@ describe("org selection", () => { expect(result).toEqual({ error: true, message: - 'Action "get_user_default_org" is not organization-scoped — it takes no org. Retry without the org parameter.', + 'Action "get_user_default_org" is not organization-scoped — it takes no org. Retry with no org in the org parameter, in params.org, or in the request body.', }); expect(mockFetch).not.toHaveBeenCalled(); }); From 282abc3fda7e484b5ebb376bd2661e99fe607e56 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Wed, 16 Sep 2026 08:27:41 +0100 Subject: [PATCH 4/5] fix: say what a wrong-type org is, and pin the body-only channel The rejection message told `org: 1234` that the org "must be a single non-empty organization name". It is not empty. The mistake is that an org id is not a name, and the sentence did not say so. It now asks for a single organization name, given as a non-empty string, which covers the blank case too. Nothing pinned that an org named only inside a write's request body is honoured as the target. `unwrapBodyParam` flattens the body over the top level, so a body key is `params.org` and naming the target that way is supported. The other body-org tests pin that the value is read, since removing the post-unwrap read turns three of them red. Continuing to read it for the cross-check while no longer using it as the target passed the whole suite. One positive test pins that, including that the org leaves the payload on its way to the path segment. Found by the review on #63. Co-Authored-By: Claude Opus 5 --- src/tools/execute-action.ts | 2 +- test/tools/execute-action.test.ts | 29 +++++++++++++++++++++++++---- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/src/tools/execute-action.ts b/src/tools/execute-action.ts index 1ee0e45..3da4be5 100644 --- a/src/tools/execute-action.ts +++ b/src/tools/execute-action.ts @@ -99,7 +99,7 @@ function orgError(entry: CatalogEntry, named: string[]): string | undefined { // reporting it as a nameless org disagreeing with a real one would send the // caller to drop one of the two rather than to fix the bad value. if (named.includes("")) { - return "The org must be a single non-empty organization name. Check the org parameter, params.org, and any org in the request body, or omit all of them to use the configured default."; + return "The org must be a single organization name, given as a non-empty string. Check the org parameter, params.org, and any org in the request body, or omit all of them to use the configured default."; } if (named.length > 1) { diff --git a/test/tools/execute-action.test.ts b/test/tools/execute-action.test.ts index 99d276c..b73d012 100644 --- a/test/tools/execute-action.test.ts +++ b/test/tools/execute-action.test.ts @@ -376,7 +376,7 @@ describe("org selection", () => { expect(result).toEqual({ error: true, message: - "The org must be a single non-empty organization name. Check the org parameter, params.org, and any org in the request body, or omit all of them to use the configured default.", + "The org must be a single organization name, given as a non-empty string. Check the org parameter, params.org, and any org in the request body, or omit all of them to use the configured default.", }); expect(mockFetch).not.toHaveBeenCalled(); }); @@ -448,7 +448,7 @@ describe("org selection", () => { expect(result).toEqual({ error: true, message: - "The org must be a single non-empty organization name. Check the org parameter, params.org, and any org in the request body, or omit all of them to use the configured default.", + "The org must be a single organization name, given as a non-empty string. Check the org parameter, params.org, and any org in the request body, or omit all of them to use the configured default.", }); expect(mockFetch).not.toHaveBeenCalled(); }); @@ -484,7 +484,7 @@ describe("org selection", () => { expect(result).toEqual({ error: true, message: - "The org must be a single non-empty organization name. Check the org parameter, params.org, and any org in the request body, or omit all of them to use the configured default.", + "The org must be a single organization name, given as a non-empty string. Check the org parameter, params.org, and any org in the request body, or omit all of them to use the configured default.", }); expect(mockFetch).not.toHaveBeenCalled(); }); @@ -500,11 +500,32 @@ describe("org selection", () => { expect(result).toEqual({ error: true, message: - "The org must be a single non-empty organization name. Check the org parameter, params.org, and any org in the request body, or omit all of them to use the configured default.", + "The org must be a single organization name, given as a non-empty string. Check the org parameter, params.org, and any org in the request body, or omit all of them to use the configured default.", }); expect(mockFetch).not.toHaveBeenCalled(); }); + // A body-nested org is params.org after the unwrap, so it is a supported way + // to name the target rather than only something to cross-check. The other + // body-org tests pin that the value is READ: removing the post-unwrap read + // turns three of them red. None pinned that it is HONOURED. Keeping the + // cross-check while no longer using a body-only org as the target passed the + // whole suite before this test existed. + it("targets an org named only inside a write's request body", async () => { + const mockFetch = mockFetchOk({ created: true }); + + await executeAction( + entries, config, "post_control", + { body: { org: "cyber-dojo", identifier: "ctrl-1" } }, + undefined, mockFetch, "WRITE", + ); + + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/controls/cyber-dojo", + expect.objectContaining({ body: JSON.stringify({ identifier: "ctrl-1" }) }), + ); + }); + it("rejects an org nested in the request body that contradicts the org parameter", async () => { const mockFetch = vi.fn(); From 30dba3cf3f78b7dbf2d80287a324121d5e76c0c2 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Wed, 16 Sep 2026 08:50:31 +0100 Subject: [PATCH 5/5] test: pin that an org is encoded into one path segment `org` is now a declared top-level input on a tool that performs writes, which makes it the obvious field for a model to fill from free text. It ends up as a path segment. `buildUrl` percent-encodes it, so a value carrying slashes stays one segment instead of re-pointing the request at another endpoint. Nothing tied that to the org. Removing the encoding from the path interpolation passed the whole suite. `normalizeOrg` deliberately does not reject a slash, because the encoding is the defence, so the defence is worth a test next to the input that reaches it. Also records why the body channel is not always cross-checked. When `unwrapBodyParam` declines to flatten, the body's org is never read, and that is correct: an unflattened body cannot reach the path, so there is nothing to compare. Found by the review on #63. Co-Authored-By: Claude Opus 5 --- src/tools/execute-action.ts | 4 +++- test/tools/execute-action.test.ts | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/tools/execute-action.ts b/src/tools/execute-action.ts index 3da4be5..5008a24 100644 --- a/src/tools/execute-action.ts +++ b/src/tools/execute-action.ts @@ -142,7 +142,9 @@ export async function executeAction( // Collect the org from every channel before deciding. `unwrapBodyParam` // flattens a write's request body over the top level, so params.org has to be // read before the unwrap and the body's org after it — otherwise the spread - // picks a winner and the disagreement is never seen. + // picks a winner and the disagreement is never seen. When the unwrap declines, + // `unwrapped` is `params` and the body's org is never read. That is correct: + // an unflattened body cannot reach the path, so there is nothing to check. const beforeUnwrap = normalizeOrg(params.org); const unwrapped = unwrapBodyParam(entry, params); const named = [...new Set( diff --git a/test/tools/execute-action.test.ts b/test/tools/execute-action.test.ts index b73d012..a2639ac 100644 --- a/test/tools/execute-action.test.ts +++ b/test/tools/execute-action.test.ts @@ -620,6 +620,25 @@ describe("org selection", () => { ); }); + // The org is a top-level input on a tool that performs writes, and it lands in + // a path segment. buildUrl percent-encodes it, so a slash-bearing value stays + // one segment instead of re-pointing the request at another endpoint. + // normalizeOrg deliberately does not reject this: the encoding is the defence, + // so pin it here rather than leaving it implicit two files away. + it("encodes an org rather than letting it rewrite the path", async () => { + const mockFetch = mockFetchOk(); + + await executeAction( + entries, config, "list_environments", {}, + undefined, mockFetch, "GET", "../../user/default-org", + ); + + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/environments/..%2F..%2Fuser%2Fdefault-org", + expect.anything(), + ); + }); + it("applies to writes, and the org does not leak into the request body", async () => { const mockFetch = mockFetchOk({ created: true }); const body = { identifier: "ctrl-1", name: "Control 1" };