diff --git a/src/mgmt/tools/createApiKey.ts b/src/mgmt/tools/createApiKey.ts index 74bc3c9..f59d47c 100644 --- a/src/mgmt/tools/createApiKey.ts +++ b/src/mgmt/tools/createApiKey.ts @@ -36,7 +36,11 @@ import { MGMT_ADDITIVE } from "./annotations.js"; // point when the account has one) is rendered in ONE place, shared with // mgmt_reveal_api_key, so a field cannot be surfaced on one path and dropped on // the other. That is precisely how enterpriseApiKeys went missing. -import { describeEndpointToken } from "./endpointToken.js"; +import { + describeEndpointToken, + ENDPOINT_TOKEN_DISCLOSURE, + ENDPOINT_TOKEN_DISCLOSURE_LINES, +} from "./endpointToken.js"; /** * SHARK-3513 — the human-facing description of a key creation. @@ -82,8 +86,14 @@ export function registerCreateApiKey({ "Create or get a dedicated per-project API key (JWT) for this " + "account, optionally restricted to a set of blockchains. " + "STATE-CHANGING. Idempotent by index: an existing index returns the " + - "existing key. The secret key material is never returned in the tool " + - "output." + + "existing key. " + + // SHARK-3620: this slot used to read "The secret key material is never + // returned in the tool output", which the reply falsified on every + // successful call — it carries the endpoint token, and a ready URL with + // the token in it. The approval page has always been accurate, so the + // description now IS the approval page's sentence rather than a second + // wording of it. + ENDPOINT_TOKEN_DISCLOSURE + TOTP_DESCRIPTION_SUFFIX + HITL_DESCRIPTION_SUFFIX, inputSchema: z @@ -167,10 +177,14 @@ export function registerCreateApiKey({ // credential-bearing reply has to be told that is what they are // approving, so the two are named separately: the signed material // stays hidden, the usable credential does not. - "The key's signed material (jwt_data) is never shown to the " + - "assistant. The reply DOES carry the key's endpoint token, and " + - "the account's enterprise API keys where it has any, which are " + - "live credentials that land in the conversation transcript.", + // + // SHARK-3620: and the tool DESCRIPTION now renders this same + // constant, because it used to assert the opposite. The wording is + // unchanged here; what changed is that there is only one of it, and + // that it arrives as TWO effect lines — as one 201-character line it + // was clipped by the page's 200-char per-effect bound, exactly at + // the clause naming the transcript. + ...ENDPOINT_TOKEN_DISCLOSURE_LINES, ], account: await accountAddressForDisplay(gateway), }), @@ -262,6 +276,11 @@ export function registerCreateApiKey({ const resolved = await describeEndpointToken({ key: created, worker: deps.worker, + // SHARK-3619: this is the create path, so the key may be seconds old + // and invisible to the RPC proxy for about a minute. Reveal renders + // the same surface without this caveat, because its key already + // exists. + justCreated: true, }); return { content: [ diff --git a/src/mgmt/tools/endpointToken.ts b/src/mgmt/tools/endpointToken.ts index 08ab363..090fb05 100644 --- a/src/mgmt/tools/endpointToken.ts +++ b/src/mgmt/tools/endpointToken.ts @@ -119,8 +119,16 @@ function enterpriseSurface(resolved: WorkerTokenResult): string { * create and reveal must not be able to disagree about it. */ const DATA_CALL_HANDOFF = - "\n\nTO MAKE DATA CALLS WITH IT. The URL above works immediately from any " + - "HTTP client, and that is the shortest path to a first call. The Ankr data " + + "\n\nTO MAKE DATA CALLS WITH IT. The URL above needs no session setup at " + + // SHARK-3619: this used to read "works immediately", which was a claim about + // TIME and was false for a key that had just been minted — the proxy answers + // -32050 for about a minute afterwards. What the sentence is actually for is + // the claim about SETUP: no session, no header, no client. That half is true + // on both paths and is what makes this the shortest route to a first call, so + // it is what survives. The timing caveat belongs to the create path alone and + // lives in NEW_KEY_PROPAGATION_NOTE. + "all: any HTTP client can call it, and that is the shortest path to a " + + "first call. The Ankr data " + "MCP server is different: it binds ONE API key per session, at connect time, " + "so a session that is already open keeps the key it was opened with and " + "cannot be repointed at this one. To reach this key from the data tools, set " + @@ -128,6 +136,85 @@ const DATA_CALL_HANDOFF = "NEW session, which in most clients means reconnecting that server. This key " + "stays valid meanwhile, so nothing has to be created again."; +/** + * SHARK-3620 — ONE sentence about what a key-bearing reply discloses, shared by + * the tool DESCRIPTION and the human approval PAGE. + * + * THE DEFECT IT CLOSES. mgmt_create_api_key's description said "The secret key + * material is never returned in the tool output" while the reply carried the + * endpoint token in full, plus a ready-to-call URL with the token embedded. The + * approval page for the same call was already accurate, and that is the point: + * the two disagreed about whether a live credential lands in the model + * transcript, which is precisely the property a reader checks before deciding + * whether a tool is safe to call in a shared or logged session. + * + * WHY A CONSTANT RATHER THAN TWO CAREFUL WORDINGS (see the LINES form below for + * why there are two of them). Two wordings that agree today + * are two wordings that can drift, and the drift is invisible because each side + * reads fine on its own. Sharing the sentence makes agreement structural: there + * is nothing to keep in sync. The pairing is asserted by identity in + * test/mgmt-key-lifecycle-truthfulness.ts, not by a pair of regexes. + * + * The DISTINCTION is the load-bearing part. There are two secrets here and only + * one of them is withheld, so a summary like "this returns credentials" would be + * true and useless. jwt_data is the input to the exchange and never leaves the + * server; the endpoint token is the result, and it is live. + */ +/** + * TWO LINES, AND THE SPLIT IS LOad-BEARING — found while wiring the pairing + * test. The approval page stores each effect through `clip(e, 200)`, and this + * sentence was 201 characters, so the page a human reads to decide whether a + * credential lands in their transcript was cut at "…which are live credentials…" + * and never reached the words that say where they land. It is the only truncated + * effect on the whole gated surface (test/mgmt-gated-display.test.ts now pins + * that for every call site), and it was the one that mattered most. + * + * Splitting rather than shortening keeps the distinction intact: line one is the + * secret that stays hidden, line two is the credential that does not. + */ +export const ENDPOINT_TOKEN_DISCLOSURE_LINES = [ + "The key's signed material (jwt_data) is never shown to the assistant.", + "The reply DOES carry the key's endpoint token, and the account's " + + "enterprise API keys where it has any, which are live credentials that " + + "land in the conversation transcript.", +]; + +/** The same disclosure as one sentence, for the tool description. */ +export const ENDPOINT_TOKEN_DISCLOSURE = + ENDPOINT_TOKEN_DISCLOSURE_LINES.join(" "); + +/** + * SHARK-3619 — the create path's timing caveat, in the reply that creates the + * expectation. + * + * MEASURED, NOT ESTIMATED. On 2026-08-07 a key created through this server + * answered -32050 on rpc.ankr.com and became callable between 60 and 90 seconds + * later; the transition itself fell inside a 10-second poll window. The number + * carries its date so that a future edit has to change both, the same discipline + * the allowlist writes' 45-100 second window follows. + * + * WHY THE VERDICT IS SPELLED OUT rather than left to the reader. The failure + * this ticket recorded was not that the delay was unmentioned, it was that every + * plausible reading of "API key not found" is wrong: create the key again (which + * costs another human approval and mints nothing), escalate, or report MCP key + * creation as broken. So the note names the code, the exact words the proxy + * uses, and what to do about them. + * + * WHY IT IS NOT IN THE SHARED HANDOFF STRING. mgmt_reveal_api_key renders the + * same endpoint surface for a key that already exists and is therefore already + * known to the proxy. Warning about a wait there would be false in the other + * direction. + */ +export const NEW_KEY_PROPAGATION_NOTE = + "\n\nIF THIS KEY WAS JUST MINTED, THE PROXY NEEDS A MOMENT. The control " + + "plane creates a key at once; the RPC proxy learns it afterwards, measured " + + "at roughly 60 to 90 seconds on 2026-08-07. Until then rpc.ankr.com answers " + + "HTTP 401 with `API key not found` (json-rpc code -32050) for this token. " + + "That is the key not being visible YET, not a failed creation: do not create " + + "it again, and do not report it as broken. This call is idempotent by slot, " + + "so a key that already existed is already known to the proxy and is callable " + + "now."; + /** * Turn a key into something the caller can call, or say plainly why not. * @@ -146,9 +233,17 @@ const DATA_CALL_HANDOFF = export async function describeEndpointToken({ key, worker, + justCreated = false, }: { key: { jwt_data?: string; is_encrypted: boolean; config?: string }; worker?: WorkerClient; + /** + * SHARK-3619 — true only on the create path, where the key may be seconds old + * and the proxy may not know it yet. The caveat rides on the ONE branch that + * hands over a usable URL: the encrypted / no-material / exchange-failed + * branches promise no immediacy to correct. + */ + justCreated?: boolean; }): Promise<{ ok: boolean; text: string }> { if (key.is_encrypted) { return { @@ -181,6 +276,7 @@ export async function describeEndpointToken({ "on the chains the key is scoped to. The same value is what the " + "allowlist, freeze and status tools take as `token`." + DATA_CALL_HANDOFF + + (justCreated ? NEW_KEY_PROPAGATION_NOTE : "") + enterpriseSurface(resolved), }; } catch (e) { diff --git a/src/mgmt/tools/freezeApiKey.ts b/src/mgmt/tools/freezeApiKey.ts index 7c8f97f..51233cc 100644 --- a/src/mgmt/tools/freezeApiKey.ts +++ b/src/mgmt/tools/freezeApiKey.ts @@ -23,7 +23,11 @@ // x-ankr-totp-token). The totp is never logged or echoed. import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { z } from "zod"; -import { type GatewayClient, GatewayError } from "../gateway/client.js"; +import { + type CounterStatus, + type GatewayClient, + GatewayError, +} from "../gateway/client.js"; import { totpSchema, TOTP_DESCRIPTION_SUFFIX, @@ -40,9 +44,88 @@ import { resolveKeyTarget, } from "./keyAddressing.js"; import { accountAddressForDisplay } from "./whoami.js"; -import { unobservedMeta } from "./writeOutcome.js"; +import { observedMeta, unobservedMeta } from "./writeOutcome.js"; import { MGMT_DESTRUCTIVE } from "./annotations.js"; +// --------------------------------------------------------------------------- +// SHARK-3622 — the two things the allowlist write on the same key already did, +// and this one did not: read the result back, and say the data plane lags. +// --------------------------------------------------------------------------- + +/** + * The measured lag, per DIRECTION, and never averaged into one number. + * + * On 2026-08-07 a freeze took 10 to 21 seconds to stop traffic on rpc.ankr.com + * after the control plane accepted it — faster than the blockchain allowlist's + * 45-100 second window, which is worth knowing before anyone writes a wait into + * a runbook. The UNFREEZE direction was not measured on that run, and printing + * the freeze number for it would be exactly the invented precision this ticket + * is about, so it says what it knows and no more. + * + * WHY THIS MATTERS MORE ON FREEZE THAN ANYWHERE ELSE. Freeze is the tool someone + * reaches for during an incident, when "is it actually stopped yet" is the only + * question. A reply that reads as a completed stop, while the proxy serves for + * another 20 seconds, is the difference between waiting and escalating. + */ +const FREEZE_PROPAGATION_NOTE = + " Enforcement at the RPC proxy follows the control plane rather than " + + "coinciding with it: a freeze was measured at 10 to 21 seconds to take " + + "effect on rpc.ankr.com (2026-08-07), so requests can still be served " + + "briefly after this reply."; + +const UNFREEZE_PROPAGATION_NOTE = + " Enforcement at the RPC proxy follows the control plane rather than " + + "coinciding with it, so traffic takes a moment to resume. Only the freeze " + + "direction has been measured (10 to 21 seconds, 2026-08-07); the unfreeze " + + "direction has not been measured."; + +const propagationNote = (freeze: boolean): string => + freeze ? FREEZE_PROPAGATION_NOTE : UNFREEZE_PROPAGATION_NOTE; + +/** What a status read settled, or why it settled nothing. */ +type StatusReadBack = + { ok: true; status: CounterStatus } | { ok: false; why: string }; + +/** + * Read the key's status straight back from the gateway. + * + * WHY THIS IS NOT THE READ-BACK THE ALLOWLIST WRITES REFUSE TO DO. Their comment + * warns against a check that would "race the 45-100s proxy propagation and + * manufacture false failures", and it is right — about the DATA plane. This read + * asks the CONTROL plane what it now holds, which is the same store the write + * just went to and is authoritative immediately. The two are different + * questions, and conflating them is how the caller ends up doing this read by + * hand anyway. + * + * A THROWN read is not a failed write. The write was already accepted; only the + * observation failed, so the reason travels back as text and the caller keeps a + * non-error result. + */ +async function readBackStatus( + gateway: GatewayClient, + token: string +): Promise { + try { + const status = await gateway.getJwtStatus(token); + // Same defensive shape as createApiKey's `index` guard: request() hands back + // `undefined as unknown as T` for an empty body, and CounterStatus types + // `frozen` as a required boolean, so a `typeof` test is the one TypeScript + // accepts as meaningful against a reply that may not honour the type. + if (!status || typeof status.frozen !== "boolean") { + return { + ok: false, + why: "the status route returned no state in its body", + }; + } + return { ok: true, status }; + } catch (e) { + return { ok: false, why: e instanceof Error ? e.message : String(e) }; + } +} + +const statusLine = (s: CounterStatus): string => + `frozen: ${s.frozen} (suspended: ${s.suspended}, freemium: ${s.freemium})`; + export function registerFreezeApiKey({ server, gateway, @@ -152,29 +235,110 @@ export function registerFreezeApiKey({ // SHARK-3522: say what a bodiless 200 actually proves — that the request // was ACCEPTED — not that the key IS frozen. // - // Deliberately NOT a request-vs-reply comparison like the allowlist and - // notif-config writes: there is nothing to compare. In the gateway source + // There is still nothing to compare IN THE REPLY. In the gateway source // (src/controllers/jwtcontroller.go) UpdateProjectFreezeState is // documented `@Success 200 {string} string ""` and the only Respond* // calls in the handler are error responders, so a success carries an empty // body. freezeJwt is typed Promise for that reason. // + // SHARK-3622: which is why the comparison now comes from a SECOND call. + // The old reply ended by telling the caller to run mgmt_get_api_key_status + // themselves — one gateway read, named in the sentence, that every caller + // had to write and that an agent which did not know to write reported as + // "frozen" while traffic was still being served. Doing it here removes a + // whole class of false "done" for the cost of the read the reply was + // already prescribing. + // // Freezing takes a customer's production traffic down, so overstating it // is operationally expensive in both directions: a human who believes an // unfreeze already took effect stops looking at an outage. const verb = freeze ? "FREEZE" : "UNFREEZE"; + const accepted = + `The gateway ACCEPTED the request to ${verb} API key ${target.label} ` + + `(HTTP 2xx)`; + const note = propagationNote(freeze); + + // SHARK-3622: the freeze route itself still says nothing, so the status + // route is asked. Three outcomes, and each states exactly what it knows. + const readBack = await readBackStatus(gateway, target.token); + if (!readBack.ok) { + return { + content: [ + { + type: "text", + text: + `${accepted}. This route returns no state in its body, so the ` + + `key's resulting status was NOT observed and is not confirmed ` + + `here. A follow-up status read did not settle it either: ` + + `${readBack.why}. Verify with mgmt_get_api_key_status before ` + + `relying on it.${note}`, + }, + ], + _meta: unobservedMeta("mgmt_get_api_key_status"), + }; + } + + const status = readBack.status; + if (status.frozen !== freeze) { + // The gateway's OWN read disagrees with the write it just accepted. + // + // WHY THIS IS NOT isError, when the allowlist writes DO raise it on a + // mismatch. Theirs compares the request against the state carried in + // the SAME reply, where a disagreement can only mean the write did not + // apply. This one compares against a SEPARATE read on a different + // route, which can legitimately trail the write by a moment — so the + // shim cannot tell "did not apply" from "read too early", and + // writeOutcome.ts's rule applies: asserting a failure the code has no + // evidence for is the same defect as asserting a success, pointed the + // other way. `isError` also means "retry me" to most agents, and a + // retry here spends a second human approval on a change that may + // already be in place — which is what the text tells them not to do. + // The uncertainty travels in `_meta.matchesRequest` instead, where a + // client can branch on it without parsing English. + return { + content: [ + { + type: "text", + text: + `${accepted}, but the status read back immediately ` + + `afterwards reports ${statusLine(status)}, which is NOT what ` + + `was requested. Two things produce this: the read raced the ` + + `write, or the write did not apply. Re-read with ` + + `mgmt_get_api_key_status before acting on it; do NOT ` + + `re-issue the ${verb}, which would spend another human ` + + `approval on a change that may already be in place.`, + }, + ], + _meta: { + ...observedMeta(), + frozen: status.frozen, + requested: freeze, + matchesRequest: false, + }, + }; + } + return { content: [ { type: "text", text: - `The gateway ACCEPTED the request to ${verb} API key ${target.label} ` + - `(HTTP 2xx). This route returns no state in its body, so the ` + - `key's resulting status was NOT observed and is not confirmed ` + - `here. Verify with mgmt_get_api_key_status before relying on it.`, + `${accepted}, and the status read back CONFIRMS it: ` + + `${statusLine(status)}.${note}`, }, ], - _meta: unobservedMeta("mgmt_get_api_key_status"), + _meta: { + ...observedMeta(), + frozen: status.frozen, + suspended: status.suspended, + freemium: status.freemium, + // Stated on BOTH observed paths, not just the disagreeing one: a + // client that branches on `matchesRequest` must not have to read + // `undefined` as agreement. That is the same trap writeOutcome.ts + // closed for `observed`. + requested: freeze, + matchesRequest: true, + }, }; } catch (e) { const authHint = diff --git a/src/mgmt/tools/platformApiKeys.ts b/src/mgmt/tools/platformApiKeys.ts index 631b074..38ffbaf 100644 --- a/src/mgmt/tools/platformApiKeys.ts +++ b/src/mgmt/tools/platformApiKeys.ts @@ -225,9 +225,17 @@ function mintEffects(ttlLabel: string): string[] { "The new key is a BEARER token for the whole Ankr management API, not an " + "RPC endpoint token: it does not fetch chain data, it administers this " + "account.", + // SHARK-3620: TWO lines, because the page clips each effect at 200 + // characters and this one was 202 — cut at "without asking a human to + // approve anyth…", losing the second factor, and losing the sentence that + // tells the human what they are approving. Of every line on the gated + // surface this is the one that must arrive whole: it is the difference + // between minting an admin credential and minting one that also removes the + // gate the human is currently standing at. "Anyone who holds it can do everything this assistant can do here — list, " + "create, edit, freeze and delete API keys, read usage and billing, and " + - "start a payment — without asking a human to approve anything and " + + "start a payment.", + "It does all of that without asking a human to approve anything and " + "without a second factor. Approving this is approving that.", `It works for ${ttlLabel} from now, or until it is deleted, whichever ` + `comes first. It cannot be limited to one chain, one project or one ` + diff --git a/src/mgmt/tools/sessions.ts b/src/mgmt/tools/sessions.ts index e98d47a..3ae9430 100644 --- a/src/mgmt/tools/sessions.ts +++ b/src/mgmt/tools/sessions.ts @@ -747,17 +747,23 @@ export function logoutOthersEffects(input: { }): string[] { const { others, unreadable } = input; const effects = [ - `${others.length} session(s) end immediately. Everything signed in on ` + - `this Ankr login stops working at once: other browsers, other ` + - `machines, other agents, CI jobs and scripts included, whether or not ` + - `anyone remembers they exist.`, + // SHARK-3620: split at the sentence boundary because the page clips every + // effect at 200 characters, and this one crossed the line as soon as the + // count reached one digit — losing "whether or not anyone remembers they + // exist", which is the clause that explains why the blast radius is larger + // than the list a human is looking at. A bound that bites depending on how + // many sessions the account happens to have is the worst kind. + `${others.length} session(s) end immediately.`, + `Everything signed in on this Ankr login stops working at once: other ` + + `browsers, other machines, other agents, CI jobs and scripts included, ` + + `whether or not anyone remembers they exist.`, "Each of them: " + others.map((s) => describeDevice(s.creation_details)).join("; "), "THIS session is NOT ended. This assistant keeps working.", - "No API key, allowlist, payment or account setting is touched. A Platform " + - "API key is a different credential and is NOT a session: it keeps " + - "working, so revoke one with mgmt_delete_platform_api_key if it may " + - "also have leaked.", + "No API key, allowlist, payment or account setting is touched.", + "A Platform API key is a different credential and is NOT a session: it " + + "keeps working, so revoke one with mgmt_delete_platform_api_key if it " + + "may also have leaked.", ]; if (unreadable > 0) { effects.push( diff --git a/test/mgmt-gated-display.test.ts b/test/mgmt-gated-display.test.ts index e0dfea5..e957a38 100644 --- a/test/mgmt-gated-display.test.ts +++ b/test/mgmt-gated-display.test.ts @@ -290,6 +290,21 @@ test("SHARK-3513: EVERY gated call site mints a self-describing display payload" (d.effects ?? []).length > 0, `${entry.tool}: the consequences must be listed` ); + // SHARK-3620: and none of them may be CUT OFF. The store clips every effect + // at 200 characters, silently, and exactly one line on the whole gated + // surface was over it — create's credential disclosure, truncated at + // "…which are live credentials…", losing the clause that says they land in + // the conversation transcript. A consequence a human cannot finish reading + // is not a consequence they were told, and the failure mode is invisible + // from the call site, so it is pinned here for every page rather than for + // the one that happened to be caught. + for (const effect of d.effects ?? []) { + assert.doesNotMatch( + effect, + /…$/, + `${entry.tool}: an effect was truncated by the display bound: ${effect}` + ); + } // SHARK-3577: the two SESSION writes are the only gated pages that carry NO // account, and it is not an omission. `account` is filled from // `GET /auth/users/profile`, which IS account-scoped, so under a selected diff --git a/test/mgmt-key-lifecycle-truthfulness.test.ts b/test/mgmt-key-lifecycle-truthfulness.test.ts new file mode 100644 index 0000000..a8f2aaf --- /dev/null +++ b/test/mgmt-key-lifecycle-truthfulness.test.ts @@ -0,0 +1,586 @@ +// SHARK-3619 / SHARK-3620 / SHARK-3622 — a key-lifecycle write must not +// describe a world different from the one it produces. +// +// All three were found by walking create -> call -> restrict -> freeze against +// prod on 2026-08-07, and they are the same defect pointed in three directions: +// the reply and the tool description each assert something the data plane does +// not do. +// +// - SHARK-3619. The create reply says the ready URL "works immediately". It +// does not: the proxy learns a NEW key roughly 60 to 90 seconds later, and +// until then rpc.ankr.com answers -32050 "API key not found" under HTTP 401. +// Every plausible reaction to that error is wrong (create it again, escalate, +// report MCP key creation as broken), and the reply is what caused it. +// - SHARK-3620. The tool description says "The secret key material is never +// returned in the tool output" while the reply carries the endpoint token in +// full. The approval page for the SAME call is accurate and separates the two +// secrets. A description that denies the disclosure is what makes the +// disclosure invisible. +// - SHARK-3622. freeze neither reads the resulting state back nor mentions the +// data-plane lag, while the blockchain-allowlist write on the same key, one +// minute earlier, does both. +// +// WHY THE PAIRING TESTS ARE STRUCTURAL RATHER THAN TEXTUAL. Asserting that two +// wordings "agree" by matching two regexes is the failure mode that produced +// SHARK-3620 in the first place: both sides pass their own assertion and drift +// apart anyway. So the disclosure is ONE exported constant and the tests assert +// IDENTITY - the description contains it, and the approval page's effects list +// contains it as an element. Changing one and not the other cannot be done. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { Client } from "@modelcontextprotocol/sdk/client/index.js"; +import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; +import { createMgmtServer } from "../src/mgmt/server.js"; +import type { + CounterStatus, + GatewayClient, +} from "../src/mgmt/gateway/client.js"; +import type { + WorkerClient, + WorkerTokenResult, +} from "../src/mgmt/gateway/worker.js"; +import { + type MgmtDeps, + createConfirmationStore, +} from "../src/mgmt/tools/confirmation.js"; +import { + ENDPOINT_TOKEN_DISCLOSURE, + ENDPOINT_TOKEN_DISCLOSURE_LINES, + NEW_KEY_PROPAGATION_NOTE, +} from "../src/mgmt/tools/endpointToken.js"; + +const JWT_DATA = "HEADER.PAYLOAD.SIGNATURE"; +const ENDPOINT_TOKEN = "b3d9f1a6c07e4b1e9f2a5c8d7e6b4a3f"; +const ADDRESS = "0xabc0000000000000000000000000000000000001"; +const TEST_SUB = "test-subject"; +const SLOT = 2; + +const keyAt = (index: number) => ({ + index, + jwt_data: JWT_DATA, + is_encrypted: false, + name: "acceptance-run", + description: "acceptance key", + config: '{"blockchains":["eth"]}', +}); + +/** + * A gateway whose freeze route answers as the real one does (bodiless 2xx), and + * whose STATUS route is supplied per test - that read is the subject of + * SHARK-3622, so every test states what it returns rather than inheriting it. + */ +function gatewayWith(status: (() => Promise) | undefined): { + gateway: GatewayClient; + calls: string[]; +} { + const calls: string[] = []; + return { + calls, + gateway: { + listJwtTokens: () => { + calls.push("listJwtTokens"); + return Promise.resolve([keyAt(SLOT)]); + }, + getUserProfile: () => Promise.resolve({ address: ADDRESS }), + createAdditionalJwt: () => { + calls.push("createAdditionalJwt"); + return Promise.resolve(keyAt(SLOT)); + }, + freezeJwt: () => { + calls.push("freezeJwt"); + return Promise.resolve(undefined); + }, + getJwtStatus: () => { + calls.push("getJwtStatus"); + // The harness default mirrors the real bodiless-200 shape: request() + // hands back undefined for an empty body. + return ( + status?.() ?? Promise.resolve(undefined as unknown as CounterStatus) + ); + }, + } as unknown as GatewayClient, + }; +} + +const statusOf = (frozen: boolean) => (): Promise => + Promise.resolve({ frozen, suspended: false, freemium: false }); + +/** A worker that resolves the endpoint token, as the live one does. */ +const workerOk = (): WorkerClient => ({ + importJwtToken: () => + Promise.resolve({ + token: ENDPOINT_TOKEN, + tier: "premium", + } as WorkerTokenResult), +}); + +function depsWith(worker?: WorkerClient): { + deps: MgmtDeps; + store: ReturnType; +} { + const confirmations = createConfirmationStore("http://localhost:3100"); + return { + deps: { + confirmations, + sub: TEST_SUB, + issuerUrl: "http://localhost:3100", + mfaEnforced: true, + worker, + }, + store: confirmations, + }; +} + +async function connect( + gateway: GatewayClient, + deps?: MgmtDeps +): Promise { + const server = createMgmtServer(gateway, deps); + const [clientT, serverT] = InMemoryTransport.createLinkedPair(); + const client = new Client({ name: "test", version: "0" }); + await server.connect(serverT); + await client.connect(clientT); + return client; +} + +const textOf = (r: unknown): string => + ((r as { content?: { text?: string }[] }).content ?? []) + .map((c) => c.text ?? "") + .join("\n"); + +const metaOf = (r: unknown): Record => + ((r as { _meta?: Record })._meta ?? {}) as Record< + string, + unknown + >; + +const isError = (r: unknown): boolean => + (r as { isError?: boolean }).isError === true; + +const mintedToken = (text: string): string | undefined => + /confirmToken: ([0-9a-f-]{36})/.exec(text)?.[1]; + +/** Drive a gated tool the way a human does: request, approve, repeat. */ +async function runApproved( + client: Client, + store: ReturnType, + name: string, + args: Record +): Promise { + const first = await client.callTool({ name, arguments: args }); + const token = mintedToken(textOf(first)); + assert.ok(token, `${name} must mint a confirmToken: ${textOf(first)}`); + assert.ok(store.approve(token, TEST_SUB), "approval must succeed"); + return client.callTool({ name, arguments: { ...args, confirmToken: token } }); +} + +/** The display payload a gated call parked for the approval page. */ +async function displayFor( + client: Client, + store: ReturnType, + name: string, + args: Record +): Promise<{ summary: string; effects?: string[] }> { + const first = await client.callTool({ name, arguments: args }); + const token = mintedToken(textOf(first)); + assert.ok(token, `${name} must mint a confirmToken`); + const pending = store.peek(token); + assert.ok(pending?.display, `${name} must park a display payload`); + return pending.display as { summary: string; effects?: string[] }; +} + +const descriptionOf = async (client: Client, name: string): Promise => { + const { tools } = await client.listTools(); + const tool = tools.find((t) => t.name === name); + assert.ok(tool, `${name} must be registered`); + return tool.description ?? ""; +}; + +// --------------------------------------------------------------------------- +// SHARK-3620 — the description and the approval page must say the same thing +// --------------------------------------------------------------------------- + +test("SHARK-3620: create's description does not claim the output withholds the secret", async () => { + const { gateway } = gatewayWith(undefined); + const client = await connect(gateway, depsWith(workerOk()).deps); + try { + const description = await descriptionOf(client, "mgmt_create_api_key"); + + // The exact claim the acceptance run falsified. It is not enough to add a + // truthful sentence beside it; the false one has to be gone. + assert.doesNotMatch( + description, + /secret key material is never returned/i, + "the description must stop denying a disclosure the reply performs" + ); + assert.match( + description, + /endpoint token/i, + "and must name the credential the reply actually carries" + ); + } finally { + await client.close(); + } +}); + +test("SHARK-3620: the description and the approval page carry the SAME disclosure sentence", async () => { + const { gateway } = gatewayWith(undefined); + const { deps, store } = depsWith(workerOk()); + const client = await connect(gateway, deps); + try { + const description = await descriptionOf(client, "mgmt_create_api_key"); + const display = await displayFor(client, store, "mgmt_create_api_key", { + index: SLOT, + name: "acceptance-run", + }); + + assert.ok( + description.includes(ENDPOINT_TOKEN_DISCLOSURE), + "the tool description must carry the shared disclosure verbatim" + ); + for (const line of ENDPOINT_TOKEN_DISCLOSURE_LINES) { + assert.ok( + (display.effects ?? []).includes(line), + `the approval page must carry this line verbatim: ${line}` + ); + } + // The page STORES each effect through clip(e, 200). Asserting the lines are + // present in the payload we handed over would pass even if the page then cut + // them, which is what was happening: the 201-character single sentence was + // truncated at "…which are live credentials…" and the human never saw where + // those credentials land. So the assertion is on what the STORE holds. + for (const effect of display.effects ?? []) { + assert.doesNotMatch( + effect, + /…$/, + `an effect a human is meant to read was truncated: ${effect}` + ); + } + } finally { + await client.close(); + } +}); + +test("SHARK-3620: the disclosure names BOTH secrets, and which one is withheld", async () => { + // The point of the sentence is the distinction. A version that said only + // "credentials are returned" would pass an includes() check while losing the + // thing that makes it actionable: jwt_data stays hidden, the endpoint token + // does not. + assert.match(ENDPOINT_TOKEN_DISCLOSURE, /jwt_data/); + assert.match(ENDPOINT_TOKEN_DISCLOSURE, /never shown/i); + assert.match(ENDPOINT_TOKEN_DISCLOSURE, /DOES carry/); + assert.match(ENDPOINT_TOKEN_DISCLOSURE, /endpoint token/i); +}); + +// --------------------------------------------------------------------------- +// SHARK-3619 — the create reply must not promise immediate usability +// --------------------------------------------------------------------------- + +test("SHARK-3619: the create reply does not claim the URL works immediately", async () => { + const { gateway } = gatewayWith(undefined); + const { deps, store } = depsWith(workerOk()); + const client = await connect(gateway, deps); + try { + const res = await runApproved(client, store, "mgmt_create_api_key", { + index: SLOT, + name: "acceptance-run", + }); + const text = textOf(res); + + assert.equal(isError(res), false); + assert.doesNotMatch( + text, + /works immediately/i, + "the reply must not assert immediacy the proxy contradicts for a minute" + ); + // The useful half of that sentence must survive: the URL still needs no + // session setup, which is the whole reason it is the shortest path. + assert.match(text, /HTTP client/i); + } finally { + await client.close(); + } +}); + +test("SHARK-3619: the create reply names the wait, the error, and that it is not a failure", async () => { + const { gateway } = gatewayWith(undefined); + const { deps, store } = depsWith(workerOk()); + const client = await connect(gateway, deps); + try { + const text = textOf( + await runApproved(client, store, "mgmt_create_api_key", { + index: SLOT, + name: "acceptance-run", + }) + ); + + assert.ok( + text.includes(NEW_KEY_PROPAGATION_NOTE), + "the create reply must carry the propagation note" + ); + // A caller who hits the window must be able to recognise what they are + // looking at from the reply alone: the code, the words the proxy uses, and + // the verdict that it is not a failed creation. + assert.match(text, /-32050/); + assert.match(text, /API key not found/i); + assert.match( + text, + /not a failed creation|not.{0,40}creation.{0,20}fail/i, + "the reply must say the error means not-ready-yet" + ); + assert.match( + text, + /do not create it again|do NOT create it again/i, + "and must head off the reaction that actually happened" + ); + // The number is a MEASUREMENT, so it is stated with its date rather than as + // folklore. Whoever changes it should have to change the date too. + assert.match(text, /60 to 90 seconds/); + assert.match(text, /2026-08-07/); + } finally { + await client.close(); + } +}); + +test("SHARK-3619: reveal does NOT carry the new-key note, because its key already exists", async () => { + // The renderer is shared with create on purpose (SHARK-3543), so the note has + // to be parameterised rather than bolted onto the shared string. Pinning the + // reveal path is what stops the next edit from putting a create-only warning + // on every key the account already has. + const { gateway } = gatewayWith(undefined); + const { deps, store } = depsWith(workerOk()); + const client = await connect(gateway, deps); + try { + const text = textOf( + await runApproved(client, store, "mgmt_reveal_api_key", { index: SLOT }) + ); + + assert.match( + text, + new RegExp(ENDPOINT_TOKEN), + "reveal still hands over the key" + ); + assert.doesNotMatch( + text, + /-32050/, + "an existing key is already known to the proxy; do not warn about a wait" + ); + } finally { + await client.close(); + } +}); + +// --------------------------------------------------------------------------- +// SHARK-3622 — freeze must read the state back, and state the lag +// --------------------------------------------------------------------------- + +test("SHARK-3622: an accepted freeze reports the status it read back", async () => { + const { gateway, calls } = gatewayWith(statusOf(true)); + const { deps, store } = depsWith(workerOk()); + const client = await connect(gateway, deps); + try { + const res = await runApproved(client, store, "mgmt_freeze_api_key", { + index: SLOT, + freeze: true, + }); + const text = textOf(res); + + assert.equal(isError(res), false); + assert.ok( + calls.includes("getJwtStatus"), + "the tool must perform the read it used to delegate to the caller" + ); + assert.match(text, /frozen: true/); + assert.doesNotMatch( + text, + /NOT observed/, + "a state that WAS read back must not be reported as unobserved" + ); + assert.equal(metaOf(res).observed, true); + assert.equal(metaOf(res).frozen, true); + // Both observed paths state it, so `undefined` never has to be read as + // agreement by a client that branches on this field. + assert.equal(metaOf(res).matchesRequest, true); + assert.equal(metaOf(res).requested, true); + } finally { + await client.close(); + } +}); + +test("SHARK-3622: the freeze reply states the measured data-plane lag", async () => { + const { gateway } = gatewayWith(statusOf(true)); + const { deps, store } = depsWith(workerOk()); + const client = await connect(gateway, deps); + try { + const text = textOf( + await runApproved(client, store, "mgmt_freeze_api_key", { + index: SLOT, + freeze: true, + }) + ); + + assert.match(text, /10 to 21 seconds/, "the measured window, not a guess"); + assert.match(text, /2026-08-07/, "with the date the measurement was taken"); + assert.match( + text, + /still be served|briefly|resume/i, + "and what that means for traffic in the meantime" + ); + } finally { + await client.close(); + } +}); + +test("SHARK-3622: unfreeze does not borrow the freeze direction's measurement", async () => { + // Only the freeze direction was measured. Printing the same number for the + // reverse would be exactly the kind of invented precision these three tickets + // are about. + const { gateway } = gatewayWith(statusOf(false)); + const { deps, store } = depsWith(workerOk()); + const client = await connect(gateway, deps); + try { + const text = textOf( + await runApproved(client, store, "mgmt_freeze_api_key", { + index: SLOT, + freeze: false, + }) + ); + + assert.match(text, /frozen: false/); + assert.match( + text, + /has not been measured|not measured/i, + "the unmeasured direction must say so" + ); + } finally { + await client.close(); + } +}); + +test("SHARK-3622: a status read that carries no state keeps today's unobserved wording", async () => { + // The freeze route answers bodiless, and so may the status route. When the + // read-back settles nothing, the tool must fall back to the claim it can + // support rather than inventing a confirmation. + const { gateway } = gatewayWith(undefined); + const { deps, store } = depsWith(workerOk()); + const client = await connect(gateway, deps); + try { + const res = await runApproved(client, store, "mgmt_freeze_api_key", { + index: SLOT, + freeze: true, + }); + const text = textOf(res); + + assert.equal(isError(res), false, "an accepted write is not a failure"); + assert.match(text, /ACCEPTED the request to FREEZE/); + assert.match(text, /NOT observed/); + assert.match(text, /mgmt_get_api_key_status/); + assert.equal(metaOf(res).observed, false); + assert.equal(metaOf(res).verifyWith, "mgmt_get_api_key_status"); + // The REASON, not just the shape. Without this the guard could degrade into + // a TypeError caught one frame later and still produce an unobserved result + // that passes every assertion above (mutation testing found exactly that: + // flipping `||` to `&&` in the guard survived). + assert.match(text, /the status route returned no state in its body/); + } finally { + await client.close(); + } +}); + +test("SHARK-3622: a status reply whose `frozen` is not a boolean settles nothing", async () => { + // The other half of the guard. A body that exists but does not honour the + // type is not "no body", and it is the shape that would otherwise print + // `frozen: undefined` as though it had been observed — the same defect + // createApiKey's `index` guard exists for, one layer in. + const { gateway } = gatewayWith(() => + Promise.resolve({ suspended: false, freemium: false } as CounterStatus) + ); + const { deps, store } = depsWith(workerOk()); + const client = await connect(gateway, deps); + try { + const res = await runApproved(client, store, "mgmt_freeze_api_key", { + index: SLOT, + freeze: true, + }); + const text = textOf(res); + + assert.equal(isError(res), false); + assert.match(text, /NOT observed/); + assert.match(text, /the status route returned no state in its body/); + assert.doesNotMatch( + text, + /frozen: undefined/, + "a missing flag must never be rendered as an observed value" + ); + assert.equal(metaOf(res).observed, false); + } finally { + await client.close(); + } +}); + +test("SHARK-3622: a read-back that contradicts the request is reported, not glossed", async () => { + const { gateway } = gatewayWith(statusOf(false)); + const { deps, store } = depsWith(workerOk()); + const client = await connect(gateway, deps); + try { + const res = await runApproved(client, store, "mgmt_freeze_api_key", { + index: SLOT, + freeze: true, + }); + const text = textOf(res); + + // NOT isError, and the distinction is the point. The write was accepted; a + // status read on a different route may simply have trailed it, so the shim + // cannot tell "did not apply" from "read too early". isError additionally + // reads as "retry me", and a retried freeze costs a second human approval + // for a change that may already be in place. The disagreement is carried in + // the text and in _meta.matchesRequest, which is checkable without parsing + // English. + assert.equal(isError(res), false, "an accepted write is not a failed one"); + assert.equal(metaOf(res).observed, true); + assert.equal(metaOf(res).matchesRequest, false); + assert.equal(metaOf(res).requested, true); + assert.match(text, /frozen: false/, "it must print what was actually read"); + assert.match(text, /NOT what was requested/, "and flag the disagreement"); + assert.match(text, /mgmt_get_api_key_status/, "and name the settling read"); + // Re-issuing the write costs another human approval for a change that may + // already be in place, so the reply must not send the caller there. + assert.match( + text, + /do NOT re-issue|do not re-issue/i, + "it must steer to a re-read rather than a second approval" + ); + } finally { + await client.close(); + } +}); + +test("SHARK-3622: a status read that throws leaves the write accepted and unobserved", async () => { + const { gateway } = gatewayWith(() => + Promise.reject(new Error("status route unavailable")) + ); + const { deps, store } = depsWith(workerOk()); + const client = await connect(gateway, deps); + try { + const res = await runApproved(client, store, "mgmt_freeze_api_key", { + index: SLOT, + freeze: true, + }); + const text = textOf(res); + + assert.equal( + isError(res), + false, + "a failed VERIFICATION must not be reported as a failed write" + ); + assert.match(text, /ACCEPTED the request to FREEZE/); + assert.match(text, /NOT observed/); + assert.match( + text, + /status route unavailable/, + "the reason the read-back settled nothing belongs in the reply" + ); + assert.equal(metaOf(res).observed, false); + } finally { + await client.close(); + } +}); diff --git a/test/mgmt-sessions.test.ts b/test/mgmt-sessions.test.ts index 33710e9..f2319c7 100644 --- a/test/mgmt-sessions.test.ts +++ b/test/mgmt-sessions.test.ts @@ -1785,23 +1785,28 @@ test("SHARK-3577: the bulk-logout consent page reads exactly this", async () => }); test("SHARK-3577: the bulk-logout effects are exactly these, with and without unaddressable entries", () => { - // Pinned from the source rather than the stored page, because the store - // CLIPS a long effect and a clipped assertion would only cover its prefix. + // Pinned from the source. It used to say the store "CLIPS a long effect and + // a clipped assertion would only cover its prefix" — which was true, and was + // the workaround rather than the fix: two of these lines were being cut off on + // the page a human approves, one of them depending on how many sessions the + // account happened to have. SHARK-3620 split them under the bound, and + // test/mgmt-gated-display.test.ts now fails any effect that arrives truncated, + // so source and page carry the same words again. assert.deepEqual( logoutOthersEffects({ others: [LAPTOP_SESSION, CI_SESSION], unreadable: 0, }), [ - "2 session(s) end immediately. Everything signed in on this " + - "Ankr login stops working at once: other browsers, other " + - "machines, other agents, CI jobs and scripts included, whether " + - "or not anyone remembers they exist.", + "2 session(s) end immediately.", + "Everything signed in on this Ankr login stops working at once: " + + "other browsers, other machines, other agents, CI jobs and " + + "scripts included, whether or not anyone remembers they exist.", "Each of them: Firefox 122 on Ubuntu 22.04 (desktop); linux " + "(server)", "THIS session is NOT ended. This assistant keeps working.", - "No API key, allowlist, payment or account setting is touched. " + - "A Platform API key is a different credential and is NOT a " + + "No API key, allowlist, payment or account setting is touched.", + "A Platform API key is a different credential and is NOT a " + "session: it keeps working, so revoke one with " + "mgmt_delete_platform_api_key if it may also have leaked.", ] @@ -1809,14 +1814,14 @@ test("SHARK-3577: the bulk-logout effects are exactly these, with and without un assert.deepEqual( logoutOthersEffects({ others: [LAPTOP_SESSION], unreadable: 1 }), [ - "1 session(s) end immediately. Everything signed in on this " + - "Ankr login stops working at once: other browsers, other " + - "machines, other agents, CI jobs and scripts included, whether " + - "or not anyone remembers they exist.", + "1 session(s) end immediately.", + "Everything signed in on this Ankr login stops working at once: " + + "other browsers, other machines, other agents, CI jobs and " + + "scripts included, whether or not anyone remembers they exist.", "Each of them: Firefox 122 on Ubuntu 22.04 (desktop)", "THIS session is NOT ended. This assistant keeps working.", - "No API key, allowlist, payment or account setting is touched. " + - "A Platform API key is a different credential and is NOT a " + + "No API key, allowlist, payment or account setting is touched.", + "A Platform API key is a different credential and is NOT a " + "session: it keeps working, so revoke one with " + "mgmt_delete_platform_api_key if it may also have leaked.", "NOT EVERYTHING: the gateway also returned 1 session entry " + diff --git a/test/mgmt-tools.test.ts b/test/mgmt-tools.test.ts index 7c0c059..bf85a16 100644 --- a/test/mgmt-tools.test.ts +++ b/test/mgmt-tools.test.ts @@ -400,8 +400,15 @@ test("SHARK-3381: gated write reaches the gateway only with totp + approved conf // is asserted by the call below. assert.match(text, /ACCEPTED/); assert.match(text, /FREEZE/); - assert.equal(calls.length, 1); - assert.equal(calls[0].method, "freezeJwt"); + // SHARK-3622: TWO calls now, and which two is the assertion. The write is + // still the only thing this test is about (a gated write reached the gateway), + // but the tool no longer stops there: it reads the resulting status back + // instead of telling the caller to. Pinning the pair by name is what keeps + // that from drifting into an extra write. + assert.deepEqual( + calls.map((c) => c.method), + ["freezeJwt", "getJwtStatus"] + ); // The totp is a shim-side gate for freeze (non-MFA route) — never forwarded // and never echoed. assert.doesNotMatch(text, /123456/); @@ -1416,9 +1423,17 @@ test("SHARK-3522 pass3: freeze reports the request as ACCEPTED, not as an observ "must not assert the key IS frozen from a bodiless 200" ); assert.match(t, /ACCEPTED/); - // It must say WHY it cannot confirm, and name the read-back. - assert.match(t, /no .*body|returns no state|empty body/i); - assert.match(t, /mgmt_get_api_key_status|mgmt_list_api_keys/); + // SHARK-3622 reopened this one. The claim being pinned is unchanged — the + // reply may not assert a state it did not see — but the tool now GOES AND + // SEES: it reads the status back rather than handing the caller the read. So + // a confirmation is allowed here, on the condition that it is sourced from + // that read and says so. What must never come back is a confirmation + // attributed to the bodiless 200, which is what the old wording guarded. + assert.match(t, /read back CONFIRMS it: frozen: true/); + // The unobserved wording still exists and is still correct; it now belongs to + // the branch where the status read settles nothing, and it is asserted there + // (test/mgmt-key-lifecycle-truthfulness.test.ts). + assert.match(t, /mgmt_get_api_key_status|read back/); await client.close(); });