From 5d02468d2a3abcc41adccb966f00fcd8f03cea05 Mon Sep 17 00:00:00 2001 From: Pooya Paridel Date: Tue, 27 Oct 2026 18:45:29 -0700 Subject: [PATCH 1/5] refactor(telemetry): replace the write-access check with a one-line notice Since the last redesign the write-access check never changed what the customer saw: every result led to the same warning and the same consent prompt. It was also the source of every review round (definer functions, the Redshift LIKE escape, sequences). So remove it. - core: delete checkWriteAccess, its catalog queries, WriteAccessResult, and the writeAccess field of DestinationTestReport. Keep the S3 result location check, now athenaResultLocation.ts: Test connection still skips its Athena SELECT 1 when results would land inside the table's data, because read-only credentials do not prevent that. - mcp: the gate asks once per process to confirm one line ("It is your responsibility to give this server read-only credentials."), with no check. Elicitation, the env variable, shared prompts and the cancellation check are unchanged. - VS Code and desktop: Settings shows the same line as a static warning. Remove WriteAccessWarning and the renderer test runner it needed. - READMEs: describe the notice instead of the check. --- .github/workflows/build.yml | 3 - .../src/athenaResultLocation.test.ts | 130 +++++++ .../src/athenaResultLocation.ts | 101 +++++ .../src/destinationTest.test.ts | 38 +- .../src/destinationTest.ts | 21 +- .../src/index.ts | 2 +- .../src/writeAccess.test.ts | 318 --------------- .../src/writeAccess.ts | 368 ------------------ .../README.md | 35 +- .../src/envKeys.ts | 7 +- .../src/queryChokePoint.test.ts | 6 - .../src/registerTools.test.ts | 48 +-- .../src/registerTools.ts | 3 - .../src/writeAccessGate.test.ts | 80 ++-- .../src/writeAccessGate.ts | 87 ++--- .../README.md | 14 +- .../webview-ui/package.json | 3 +- .../webview-ui/src/SettingsModal.tsx | 6 +- .../src/WriteAccessWarning.test.tsx | 41 -- .../webview-ui/src/WriteAccessWarning.tsx | 52 --- .../webview-ui/src/types.ts | 4 - .../webview-ui/test.mjs | 43 -- 22 files changed, 327 insertions(+), 1083 deletions(-) create mode 100644 packages/aws-durable-execution-sdk-js-telemetry-core/src/athenaResultLocation.test.ts create mode 100644 packages/aws-durable-execution-sdk-js-telemetry-core/src/athenaResultLocation.ts delete mode 100644 packages/aws-durable-execution-sdk-js-telemetry-core/src/writeAccess.test.ts delete mode 100644 packages/aws-durable-execution-sdk-js-telemetry-core/src/writeAccess.ts delete mode 100644 packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/src/WriteAccessWarning.test.tsx delete mode 100644 packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/src/WriteAccessWarning.tsx delete mode 100644 packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/test.mjs diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index b8b6096f2..32a15bf27 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -123,9 +123,6 @@ jobs: - name: Typecheck the renderer working-directory: packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui run: npx tsc --noEmit - - name: Test the renderer - working-directory: packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui - run: npm test - name: Bundle the VS Code extension (renderer + extension host) run: npm run build -w packages/aws-durable-execution-sdk-js-telemetry-vscode - name: Bundle the desktop app diff --git a/packages/aws-durable-execution-sdk-js-telemetry-core/src/athenaResultLocation.test.ts b/packages/aws-durable-execution-sdk-js-telemetry-core/src/athenaResultLocation.test.ts new file mode 100644 index 000000000..4270fede3 --- /dev/null +++ b/packages/aws-durable-execution-sdk-js-telemetry-core/src/athenaResultLocation.test.ts @@ -0,0 +1,130 @@ +jest.mock("./athena", () => ({ + getTableLocation: jest.fn(), + getWorkGroupOutput: jest.fn(), +})); + +import { getTableLocation, getWorkGroupOutput } from "./athena"; +import { checkAthenaResultLocation } from "./athenaResultLocation"; +import type { TelemetryConfig } from "./configCore"; + +function cfg(overrides: Partial): TelemetryConfig { + return { + region: "us-east-1", + awsProfile: undefined, + destinationType: "aurora", + auroraResourceArn: "arn:rds", + auroraSecretArn: "arn:secret", + auroraDatabase: "postgres", + redshiftDatabase: "dev", + redshiftWorkgroupName: "wg", + redshiftClusterIdentifier: "", + redshiftDbUser: "", + redshiftSecretArn: "", + athenaDatabase: "db", + athenaTable: "durable_execution_telemetry", + athenaWorkgroup: "", + athenaOutputLocation: "", + athenaS3Location: "", + ...overrides, + } as TelemetryConfig; +} + +beforeEach(() => jest.resetAllMocks()); + +describe("checkAthenaResultLocation", () => { + const location = getTableLocation as jest.Mock; + const workgroup = getWorkGroupOutput as jest.Mock; + const credentials = (() => Promise.resolve({})) as never; + beforeEach(() => workgroup.mockResolvedValue({ enforced: false })); + + it("is inside when the result location is inside the table's data", async () => { + location.mockResolvedValue("s3://bucket/telemetry"); + const r = await checkAthenaResultLocation( + cfg({ athenaOutputLocation: "s3://bucket/telemetry/athena-results/" }), + credentials, + ); + expect(r.kind).toBe("inside"); + }); + + it("is outside when only a name prefix matches", async () => { + // A plain prefix test would treat "telemetry-results" as inside "telemetry". + location.mockResolvedValue("s3://bucket/telemetry"); + const r = await checkAthenaResultLocation( + cfg({ athenaOutputLocation: "s3://bucket/telemetry-results/" }), + credentials, + ); + expect(r).toEqual({ kind: "outside" }); + }); + + it("reads the primary workgroup when none is configured", async () => { + await checkAthenaResultLocation(cfg({}), credentials); + expect(workgroup).toHaveBeenCalledWith( + expect.objectContaining({ workgroup: "primary" }), + ); + }); + + it("uses the workgroup's location when the settings set none", async () => { + workgroup.mockResolvedValue({ + enforced: false, + outputLocation: "s3://bucket/telemetry/r/", + }); + location.mockResolvedValue("s3://bucket/telemetry/"); + expect((await checkAthenaResultLocation(cfg({}), credentials)).kind).toBe( + "inside", + ); + }); + + it("uses an enforced workgroup location instead of the configured one", async () => { + workgroup.mockResolvedValue({ + enforced: true, + outputLocation: "s3://bucket/telemetry/r/", + }); + location.mockResolvedValue("s3://bucket/telemetry/"); + const r = await checkAthenaResultLocation( + cfg({ athenaWorkgroup: "team", athenaOutputLocation: "s3://elsewhere/" }), + credentials, + ); + expect(r.kind).toBe("inside"); + expect(workgroup).toHaveBeenCalledWith( + expect.objectContaining({ workgroup: "team" }), + ); + }); + + it("uses athenaS3Location when the table does not exist yet", async () => { + location.mockResolvedValue(undefined); + const r = await checkAthenaResultLocation( + cfg({ + athenaOutputLocation: "s3://b/data/results/", + athenaS3Location: "s3://b/data/", + }), + credentials, + ); + expect(r.kind).toBe("inside"); + }); + + it.each([ + ["no result location anywhere", {}, "s3://b/data/"], + [ + "no data location", + { athenaOutputLocation: "s3://b/results/" }, + undefined, + ], + ] as const)("is undetermined with %s", async (_name, overrides, table) => { + location.mockResolvedValue(table); + expect( + (await checkAthenaResultLocation(cfg(overrides), credentials)).kind, + ).toBe("undetermined"); + }); + + it("is undetermined, with the error, when the workgroup cannot be read", async () => { + workgroup.mockRejectedValue(new Error("AccessDenied: athena:GetWorkGroup")); + const r = await checkAthenaResultLocation( + cfg({ athenaOutputLocation: "s3://b/results/" }), + credentials, + ); + expect(r).toEqual({ + kind: "undetermined", + reason: expect.stringContaining("AccessDenied: athena:GetWorkGroup"), + }); + }); +}); diff --git a/packages/aws-durable-execution-sdk-js-telemetry-core/src/athenaResultLocation.ts b/packages/aws-durable-execution-sdk-js-telemetry-core/src/athenaResultLocation.ts new file mode 100644 index 000000000..6038d83da --- /dev/null +++ b/packages/aws-durable-execution-sdk-js-telemetry-core/src/athenaResultLocation.ts @@ -0,0 +1,101 @@ +/** + * Where Athena writes query results, relative to the table's data. + * + * Athena writes a result file for every query, including Test connection's + * `SELECT 1`. If the result location is inside the table's data location, the + * table reads those files as records, so every query adds rows. Read-only + * credentials do not prevent this, because Athena must be allowed to write its + * results. So Test connection checks the location before it runs its probe. + */ +import type { TelemetryConfig } from "./configCore"; +import type { resolveCredentials } from "./configCore"; +import { getTableLocation, getWorkGroupOutput } from "./athena"; + +/** + * Ends an S3 URI with `/`, so a prefix comparison cannot match `s3://b/data` + * against `s3://b/data-results`. + */ +function withTrailingSlash(uri: string): string { + const trimmed = uri.trim(); + return trimmed.endsWith("/") ? trimmed : `${trimmed}/`; +} + +/** + * Where Athena writes query results relative to the table's data. + * + * - `inside`: every query writes a result file the table then reads as rows. + * - `outside`: queries do not add rows. + * - `undetermined`: the location could not be established; `reason` says why. + * + * A workgroup with EnforceWorkGroupConfiguration replaces the location a query + * submits, so the workgroup is read (with `athena:GetWorkGroup`) even when + * `athenaOutputLocation` is set. Athena uses `primary` when no workgroup is + * configured. Test connection calls this before its Athena probe, because the + * probe itself writes a result file. + */ +export type AthenaResultLocation = + | { kind: "inside"; finding: string } + | { kind: "outside" } + | { kind: "undetermined"; reason: string }; + +export async function checkAthenaResultLocation( + cfg: TelemetryConfig, + credentials: ReturnType, +): Promise { + let outputLocation: string | undefined; + let dataLocation: string | undefined; + try { + const workgroup = await getWorkGroupOutput({ + region: cfg.region, + credentials, + workgroup: cfg.athenaWorkgroup || "primary", + }); + outputLocation = workgroup.enforced + ? workgroup.outputLocation + : cfg.athenaOutputLocation || workgroup.outputLocation; + const tableLocation = await getTableLocation({ + region: cfg.region, + credentials, + database: cfg.athenaDatabase, + table: cfg.athenaTable, + }); + dataLocation = tableLocation ?? (cfg.athenaS3Location || undefined); + } catch (err) { + return { + kind: "undetermined", + reason: `Could not read the Athena workgroup or Glue table: ${errorMessage(err)}`, + }; + } + if (!outputLocation) { + return { + kind: "undetermined", + reason: + "Neither the settings nor the Athena workgroup set a result location.", + }; + } + if (!dataLocation) { + return { + kind: "undetermined", + reason: "The table does not exist and no S3 data location is set.", + }; + } + if ( + withTrailingSlash(outputLocation).startsWith( + withTrailingSlash(dataLocation), + ) + ) { + return { + kind: "inside", + finding: + `Athena writes query results to ${outputLocation}, which is inside ` + + `the table's data location ${dataLocation}. The table reads those ` + + `result files as records, so every query adds rows. Move the result ` + + `location outside the data location.`, + }; + } + return { kind: "outside" }; +} + +function errorMessage(err: unknown): string { + return err instanceof Error ? err.message : String(err); +} diff --git a/packages/aws-durable-execution-sdk-js-telemetry-core/src/destinationTest.test.ts b/packages/aws-durable-execution-sdk-js-telemetry-core/src/destinationTest.test.ts index 063400bd2..25f8a85a7 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-core/src/destinationTest.test.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-core/src/destinationTest.test.ts @@ -33,18 +33,14 @@ jest.mock("./opensearch", () => ({ jest.mock("./configCore", () => ({ resolveCredentials: jest.fn(() => ({})), })); -jest.mock("./writeAccess", () => ({ - checkWriteAccess: jest.fn(async () => ({ - status: "unverified", - reason: "r", - })), +jest.mock("./athenaResultLocation", () => ({ // Default: results land outside the data, so the Athena probe may run. checkAthenaResultLocation: jest.fn(async () => ({ kind: "outside" })), })); import type { TelemetryConfig } from "./configCore"; import { testDestination } from "./destinationTest"; -import { checkAthenaResultLocation, checkWriteAccess } from "./writeAccess"; +import { checkAthenaResultLocation } from "./athenaResultLocation"; import { tableExists, runAthenaQuery } from "./athena"; import { runAuroraQuery } from "./aurora"; import { runRedshiftQuery } from "./redshift"; @@ -466,33 +462,3 @@ describe("testDestination — CloudWatch Logs", () => { expect(r.checks.at(-1)?.detail).toMatch(/first invocation/); }); }); - -describe("testDestination — write-access check", () => { - const aurora = baseCfg({ - destinationType: "aurora", - auroraResourceArn: "arn:aws:rds:us-east-1:1:cluster:c", - auroraSecretArn: "arn:aws:secretsmanager:us-east-1:1:secret:s", - }); - - it("attaches the write-access result when every check passed", async () => { - (runAuroraQuery as jest.Mock).mockResolvedValue({ rows: [] }); - (checkWriteAccess as jest.Mock).mockResolvedValueOnce({ - status: "can-write", - findings: ["The database user is a superuser."], - }); - const r = await testDestination(aurora); - expect(r.ok).toBe(true); - expect(r.writeAccess).toEqual({ - status: "can-write", - findings: ["The database user is a superuser."], - }); - }); - - it("skips the write-access check when a check failed", async () => { - (runAuroraQuery as jest.Mock).mockRejectedValue(new Error("unreachable")); - const r = await testDestination(aurora); - expect(r.ok).toBe(false); - expect(r.writeAccess).toBeUndefined(); - expect(checkWriteAccess).not.toHaveBeenCalled(); - }); -}); diff --git a/packages/aws-durable-execution-sdk-js-telemetry-core/src/destinationTest.ts b/packages/aws-durable-execution-sdk-js-telemetry-core/src/destinationTest.ts index 4768af2f9..a153c7450 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-core/src/destinationTest.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-core/src/destinationTest.ts @@ -10,11 +10,7 @@ import { tableExists, runAthenaQuery } from "./athena"; import { runAuroraQuery } from "./aurora"; import { runRedshiftQuery } from "./redshift"; import { pingOpenSearch, countOpenSearchDocs } from "./opensearch"; -import { - checkAthenaResultLocation, - checkWriteAccess, - type WriteAccessResult, -} from "./writeAccess"; +import { checkAthenaResultLocation } from "./athenaResultLocation"; /** * Result of a single check within a destination test (e.g. "Glue table exists", @@ -37,12 +33,6 @@ export interface DestinationTestReport { ok: boolean; summary: string; checks: DestinationCheck[]; - /** - * Whether queries could change data at this destination. Present only when - * every check passed, because the write check needs a reachable destination. - * Both results are warnings, not failed checks, so they do not change `ok`. - */ - writeAccess?: WriteAccessResult; } /** Awaits a connectivity probe and turns success/throw into a DestinationCheck. */ @@ -99,15 +89,6 @@ export interface DestinationTestOptions { export async function testDestination( cfg: TelemetryConfig, options: DestinationTestOptions = {}, -): Promise { - const result = await runChecks(cfg, options); - if (!result.ok) return result; - return { ...result, writeAccess: await checkWriteAccess(cfg) }; -} - -async function runChecks( - cfg: TelemetryConfig, - options: DestinationTestOptions, ): Promise { const credentials = resolveCredentials(cfg.awsProfile); diff --git a/packages/aws-durable-execution-sdk-js-telemetry-core/src/index.ts b/packages/aws-durable-execution-sdk-js-telemetry-core/src/index.ts index a0bd0a99e..26bf8ac38 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-core/src/index.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-core/src/index.ts @@ -7,7 +7,7 @@ export * from "./chartSpec"; export * from "./configCore"; export * from "./copilotBridge"; export * from "./destinationTest"; -export * from "./writeAccess"; +export * from "./athenaResultLocation"; export * from "./dynamodb"; export * from "./explorerSession"; export * from "./hostCapabilities"; diff --git a/packages/aws-durable-execution-sdk-js-telemetry-core/src/writeAccess.test.ts b/packages/aws-durable-execution-sdk-js-telemetry-core/src/writeAccess.test.ts deleted file mode 100644 index aeac14f75..000000000 --- a/packages/aws-durable-execution-sdk-js-telemetry-core/src/writeAccess.test.ts +++ /dev/null @@ -1,318 +0,0 @@ -jest.mock("./aurora", () => ({ runAuroraQuery: jest.fn() })); -jest.mock("./redshift", () => ({ runRedshiftQuery: jest.fn() })); -jest.mock("./athena", () => ({ - getTableLocation: jest.fn(), - getWorkGroupOutput: jest.fn(), -})); -jest.mock("./configCore", () => ({ resolveCredentials: jest.fn(() => ({})) })); - -import { getTableLocation, getWorkGroupOutput } from "./athena"; -import { runAuroraQuery } from "./aurora"; -import type { TelemetryConfig } from "./configCore"; -import { runRedshiftQuery } from "./redshift"; -import { checkAthenaResultLocation, checkWriteAccess } from "./writeAccess"; - -function cfg(overrides: Partial): TelemetryConfig { - return { - region: "us-east-1", - awsProfile: undefined, - destinationType: "aurora", - auroraResourceArn: "arn:rds", - auroraSecretArn: "arn:secret", - auroraDatabase: "postgres", - redshiftDatabase: "dev", - redshiftWorkgroupName: "wg", - redshiftClusterIdentifier: "", - redshiftDbUser: "", - redshiftSecretArn: "", - athenaDatabase: "db", - athenaTable: "durable_execution_telemetry", - athenaWorkgroup: "", - athenaOutputLocation: "", - athenaS3Location: "", - ...overrides, - } as TelemetryConfig; -} - -const DUTY = - "It is your responsibility to give these tools read-only credentials."; - -/** - * Queues the catalog query results in the order the engine runs them. - * Aurora: superuser, tables, sequences, functions. Redshift: superuser, - * tables, functions. - */ -function catalog( - destinationType: "aurora" | "redshift", - rows: { - superuser?: string; - tables?: string[]; - sequences?: string[]; - functions?: string[]; - } = {}, -): void { - const runner = ( - destinationType === "aurora" ? runAuroraQuery : runRedshiftQuery - ) as jest.Mock; - const toRows = (values: string[] = []) => ({ rows: values.map((v) => [v]) }); - runner.mockResolvedValueOnce({ rows: [[rows.superuser ?? "false"]] }); - runner.mockResolvedValueOnce(toRows(rows.tables)); - if (destinationType === "aurora") - runner.mockResolvedValueOnce(toRows(rows.sequences)); - runner.mockResolvedValueOnce(toRows(rows.functions)); -} - -beforeEach(() => jest.resetAllMocks()); - -describe.each(["aurora", "redshift"] as const)( - "checkWriteAccess — %s", - (destinationType) => { - const config = cfg({ destinationType }); - const runner = ( - destinationType === "aurora" ? runAuroraQuery : runRedshiftQuery - ) as jest.Mock; - - it("is unverified, never read-only, when nothing is found", async () => { - catalog(destinationType); - const r = await checkWriteAccess(config); - expect(r.status).toBe("unverified"); - expect(r.status === "unverified" && r.reason).toContain(DUTY); - }); - - it("reports a superuser", async () => { - catalog(destinationType, { superuser: "true" }); - expect(await checkWriteAccess(config)).toEqual({ - status: "can-write", - findings: ["The database user is a superuser."], - }); - }); - - it("names each writable table", async () => { - catalog(destinationType, { tables: ["public.t", "app.orders"] }); - expect(await checkWriteAccess(config)).toEqual({ - status: "can-write", - findings: [ - "The database user can write table public.t.", - "The database user can write table app.orders.", - ], - }); - }); - - it("names each function that runs with another role's privileges", async () => { - catalog(destinationType, { functions: ["app.purge(integer)"] }); - expect(await checkWriteAccess(config)).toEqual({ - status: "can-write", - findings: [ - "A query can call function app.purge(integer), which runs with " + - "another role's privileges and can write.", - ], - }); - }); - - it("is unverified, with the error and the customer's duty, when the database cannot be reached", async () => { - runner.mockRejectedValue(new Error("connection refused")); - const r = await checkWriteAccess(config); - expect(r.status).toBe("unverified"); - expect(r.status === "unverified" && r.reason).toContain( - "connection refused", - ); - expect(r.status === "unverified" && r.reason).toContain(DUTY); - }); - }, -); - -describe("checkWriteAccess — aurora sequences", () => { - it("names each sequence the user can advance or reset", async () => { - catalog("aurora", { sequences: ["public.order_ids"] }); - expect(await checkWriteAccess(cfg({ destinationType: "aurora" }))).toEqual({ - status: "can-write", - findings: [ - "The database user can advance or reset sequence public.order_ids (nextval/setval).", - ], - }); - }); - - it("asks about superuser status, tables, sequences and functions, in that order", async () => { - catalog("aurora"); - await checkWriteAccess(cfg({ destinationType: "aurora" })); - const sqls = (runAuroraQuery as jest.Mock).mock.calls.map( - ([o]) => o.sql as string, - ); - expect(sqls[0]).toMatch(/rolsuper/); - expect(sqls[1]).toMatch(/has_table_privilege/); - expect(sqls[2]).toMatch(/has_sequence_privilege\(c\.oid, 'USAGE'\)/); - expect(sqls[2]).toMatch(/has_sequence_privilege\(c\.oid, 'UPDATE'\)/); - expect(sqls[3]).toMatch(/prosecdef/); - }); -}); - -describe("checkWriteAccess — emitted catalog SQL", () => { - // Redshift reads a backslash in a string literal as an escape, so a LIKE - // pattern such as 'pg\_%' reaches LIKE as 'pg_%' and skips schemas such as - // pgdata. The schema filter must therefore not depend on a backslash. - it.each(["aurora", "redshift"] as const)( - "%s: no query contains a backslash or a LIKE", - async (destinationType) => { - catalog(destinationType); - await checkWriteAccess(cfg({ destinationType })); - const runner = ( - destinationType === "aurora" ? runAuroraQuery : runRedshiftQuery - ) as jest.Mock; - for (const [opts] of runner.mock.calls) { - expect(opts.sql).not.toMatch(/\\/); - expect(opts.sql).not.toMatch(/\bLIKE\b/i); - } - }, - ); -}); - -describe("checkAthenaResultLocation", () => { - const location = getTableLocation as jest.Mock; - const workgroup = getWorkGroupOutput as jest.Mock; - const credentials = (() => Promise.resolve({})) as never; - beforeEach(() => workgroup.mockResolvedValue({ enforced: false })); - - it("is inside when the result location is inside the table's data", async () => { - location.mockResolvedValue("s3://bucket/telemetry"); - const r = await checkAthenaResultLocation( - cfg({ athenaOutputLocation: "s3://bucket/telemetry/athena-results/" }), - credentials, - ); - expect(r.kind).toBe("inside"); - }); - - it("is outside when only a name prefix matches", async () => { - // A plain prefix test would treat "telemetry-results" as inside "telemetry". - location.mockResolvedValue("s3://bucket/telemetry"); - const r = await checkAthenaResultLocation( - cfg({ athenaOutputLocation: "s3://bucket/telemetry-results/" }), - credentials, - ); - expect(r).toEqual({ kind: "outside" }); - }); - - it("reads the primary workgroup when none is configured", async () => { - await checkAthenaResultLocation(cfg({}), credentials); - expect(workgroup).toHaveBeenCalledWith( - expect.objectContaining({ workgroup: "primary" }), - ); - }); - - it("uses the workgroup's location when the settings set none", async () => { - workgroup.mockResolvedValue({ - enforced: false, - outputLocation: "s3://bucket/telemetry/r/", - }); - location.mockResolvedValue("s3://bucket/telemetry/"); - expect((await checkAthenaResultLocation(cfg({}), credentials)).kind).toBe( - "inside", - ); - }); - - it("uses an enforced workgroup location instead of the configured one", async () => { - workgroup.mockResolvedValue({ - enforced: true, - outputLocation: "s3://bucket/telemetry/r/", - }); - location.mockResolvedValue("s3://bucket/telemetry/"); - const r = await checkAthenaResultLocation( - cfg({ athenaWorkgroup: "team", athenaOutputLocation: "s3://elsewhere/" }), - credentials, - ); - expect(r.kind).toBe("inside"); - expect(workgroup).toHaveBeenCalledWith( - expect.objectContaining({ workgroup: "team" }), - ); - }); - - it("uses athenaS3Location when the table does not exist yet", async () => { - location.mockResolvedValue(undefined); - const r = await checkAthenaResultLocation( - cfg({ - athenaOutputLocation: "s3://b/data/results/", - athenaS3Location: "s3://b/data/", - }), - credentials, - ); - expect(r.kind).toBe("inside"); - }); - - it.each([ - ["no result location anywhere", {}, "s3://b/data/"], - [ - "no data location", - { athenaOutputLocation: "s3://b/results/" }, - undefined, - ], - ] as const)("is undetermined with %s", async (_name, overrides, table) => { - location.mockResolvedValue(table); - expect( - (await checkAthenaResultLocation(cfg(overrides), credentials)).kind, - ).toBe("undetermined"); - }); - - it("is undetermined, with the error, when the workgroup cannot be read", async () => { - workgroup.mockRejectedValue(new Error("AccessDenied: athena:GetWorkGroup")); - const r = await checkAthenaResultLocation( - cfg({ athenaOutputLocation: "s3://b/results/" }), - credentials, - ); - expect(r).toEqual({ - kind: "undetermined", - reason: expect.stringContaining("AccessDenied: athena:GetWorkGroup"), - }); - }); -}); - -describe("checkWriteAccess — s3", () => { - const location = getTableLocation as jest.Mock; - const workgroup = getWorkGroupOutput as jest.Mock; - beforeEach(() => workgroup.mockResolvedValue({ enforced: false })); - - it("is can-write when results land inside the table's data", async () => { - location.mockResolvedValue("s3://bucket/telemetry/"); - const r = await checkWriteAccess( - cfg({ - destinationType: "s3", - athenaOutputLocation: "s3://bucket/telemetry/r/", - }), - ); - expect(r.status).toBe("can-write"); - }); - - it("is unverified, never read-only, when results land outside the data", async () => { - location.mockResolvedValue("s3://bucket/telemetry/"); - const r = await checkWriteAccess( - cfg({ - destinationType: "s3", - athenaOutputLocation: "s3://bucket/results/", - }), - ); - expect(r.status).toBe("unverified"); - expect(r.status === "unverified" && r.reason).toContain(DUTY); - }); - - it("is unverified with the reason when the location is undetermined", async () => { - workgroup.mockRejectedValue(new Error("AccessDenied")); - const r = await checkWriteAccess( - cfg({ - destinationType: "s3", - athenaOutputLocation: "s3://bucket/results/", - }), - ); - expect(r.status === "unverified" && r.reason).toContain("AccessDenied"); - }); -}); - -describe("checkWriteAccess — destinations no check covers", () => { - it.each(["dynamodb", "opensearch", "cloudwatch-logs-exporter"] as const)( - "%s is unverified, never read-only", - async (destinationType) => { - const r = await checkWriteAccess(cfg({ destinationType })); - expect(r.status).toBe("unverified"); - expect(r.status === "unverified" && r.reason).toContain(DUTY); - expect(runAuroraQuery).not.toHaveBeenCalled(); - expect(runRedshiftQuery).not.toHaveBeenCalled(); - }, - ); -}); diff --git a/packages/aws-durable-execution-sdk-js-telemetry-core/src/writeAccess.ts b/packages/aws-durable-execution-sdk-js-telemetry-core/src/writeAccess.ts deleted file mode 100644 index d1af085af..000000000 --- a/packages/aws-durable-execution-sdk-js-telemetry-core/src/writeAccess.ts +++ /dev/null @@ -1,368 +0,0 @@ -/** - * Looks for ways that read-only queries could still change data at a - * destination, so the hosts can warn the customer and ask for consent. - * - * WHY THERE IS NO "READ-ONLY" RESULT. - * - * 1. `assertReadOnly` refuses a second statement and every write keyword. So a - * write can only come from something legal inside one SELECT. - * 2. On PostgreSQL such a SELECT can write through tables, superuser status, - * SECURITY DEFINER functions, sequences, extensions such as dblink, and - * more. The list has no fixed end, so no set of catalog queries can show - * that a user cannot write. - * 3. So a finding gives `can-write`, and no finding gives `unverified`: the - * tools could not confirm the credentials are read-only, and checking them - * is the customer's responsibility. Both ask for the customer's consent. - * - * What is checked: - * - Aurora PostgreSQL: superuser status, writable tables, sequences the user - * can advance or reset, and SECURITY DEFINER functions of other roles. - * - Redshift: superuser status, writable tables, Lambda UDFs and other roles' - * SECURITY DEFINER functions. - * - S3 + Athena: whether Athena writes query results inside the table's data, - * where the table reads them as records. - * - Other destinations: nothing; the result is `unverified`. - * - * This module runs its own catalog queries. They are fixed SELECTs written - * here, not model output, so they do not go through `assertReadOnly`. - */ -import type { TelemetryConfig } from "./configCore"; -import { resolveCredentials } from "./configCore"; -import { runAuroraQuery } from "./aurora"; -import { runRedshiftQuery } from "./redshift"; -import { getTableLocation, getWorkGroupOutput } from "./athena"; - -export type WriteAccessResult = - /** The check found at least one way to change data. `findings` names each one. */ - | { status: "can-write"; findings: string[] } - /** - * The check found no way, could not look, or failed. `reason` says which. - * This is not a statement that the credentials are read-only. - */ - | { status: "unverified"; reason: string }; - -/** Ends every `unverified` reason. */ -const CUSTOMER_DUTY = - "It is your responsibility to give these tools read-only credentials."; - -/** Lists at most this many tables, and this many functions, so the warning stays short. */ -const MAX_LISTED_TABLES = 5; - -const POSTGRES_SUPERUSER_SQL = - "SELECT rolsuper FROM pg_roles WHERE rolname = current_user"; - -/** - * `has_table_privilege(oid, ...)` resolves role membership, so a privilege the - * user holds through a group role is found too. A table in a schema without - * USAGE cannot be written, so those schemas are skipped. - */ -const POSTGRES_WRITABLE_TABLES_SQL = `SELECT n.nspname || '.' || c.relname -FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace -WHERE c.relkind IN ('r', 'p') - AND n.nspname NOT IN ('pg_catalog', 'information_schema') - AND has_schema_privilege(n.oid, 'USAGE') - AND (has_table_privilege(c.oid, 'INSERT') OR has_table_privilege(c.oid, 'UPDATE') - OR has_table_privilege(c.oid, 'DELETE') OR has_table_privilege(c.oid, 'TRUNCATE')) -LIMIT ${MAX_LISTED_TABLES}`; - -/** - * `nextval` needs USAGE or UPDATE on a sequence, and `setval` needs UPDATE. - * Both change the sequence's state for good, even inside a SELECT. - */ -const POSTGRES_WRITABLE_SEQUENCES_SQL = `SELECT n.nspname || '.' || c.relname -FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace -WHERE c.relkind = 'S' - AND n.nspname NOT IN ('pg_catalog', 'information_schema') - AND has_schema_privilege(n.oid, 'USAGE') - AND (has_sequence_privilege(c.oid, 'USAGE') OR has_sequence_privilege(c.oid, 'UPDATE')) -LIMIT ${MAX_LISTED_TABLES}`; - -const REDSHIFT_SUPERUSER_SQL = - "SELECT usesuper FROM pg_user WHERE usename = current_user"; - -/** - * System schemas are skipped with `LEFT(schemaname, 3) <> 'pg_'`, not with - * `NOT LIKE 'pg\_%'`. Redshift reads a backslash in a string literal as an - * escape, so `\_` reaches LIKE as `_`, a wildcard, and a schema such as - * `pgdata` would be skipped. The LEFT comparison needs no escape on either - * engine. - * - * Unverified against a live Redshift cluster; tested on PostgreSQL 16, whose - * catalog functions Redshift inherits. Redshift's `has_table_privilege` takes a - * table name, and resolving a name in a schema without USAGE raises "permission - * denied for schema". SQL does not fix the order of AND operands, so the CASE - * checks USAGE first and only then resolves the name. - */ -const REDSHIFT_WRITABLE_TABLES_SQL = `SELECT schemaname || '.' || tablename -FROM pg_tables -WHERE schemaname NOT IN ('pg_catalog', 'information_schema') - AND LEFT(schemaname, 3) <> 'pg_' - AND CASE WHEN has_schema_privilege(schemaname, 'USAGE') - THEN has_table_privilege(quote_ident(schemaname) || '.' || quote_ident(tablename), 'INSERT') - OR has_table_privilege(quote_ident(schemaname) || '.' || quote_ident(tablename), 'UPDATE') - OR has_table_privilege(quote_ident(schemaname) || '.' || quote_ident(tablename), 'DELETE') - ELSE false END -LIMIT ${MAX_LISTED_TABLES}`; - -/** - * A SECURITY DEFINER function runs with its owner's privileges. So a user with - * no writable table can still write by calling one that another role owns and - * the user may execute. A function the user owns runs with the user's own - * privileges, which the table query already covers, so those are skipped. - */ -const POSTGRES_DEFINER_FUNCTIONS_SQL = `SELECT p.oid::regprocedure::text -FROM pg_proc p JOIN pg_namespace n ON n.oid = p.pronamespace -WHERE p.prosecdef - AND n.nspname NOT IN ('pg_catalog', 'information_schema') - AND p.proowner <> (SELECT oid FROM pg_roles WHERE rolname = current_user) - AND has_schema_privilege(n.oid, 'USAGE') - AND has_function_privilege(p.oid, 'EXECUTE') -LIMIT ${MAX_LISTED_TABLES}`; - -/** - * Redshift: a Lambda UDF (language `exfunc`) runs code with the Lambda - * function's role, and a SECURITY DEFINER function runs with its owner's - * privileges. Unverified against a live Redshift cluster. It does not check - * EXECUTE privilege, because Redshift's `has_function_privilege` takes a - * signature string this query cannot build reliably. So it can list a - * function the user cannot call; that only adds a warning. - */ -const REDSHIFT_RISKY_FUNCTIONS_SQL = `SELECT n.nspname || '.' || p.proname -FROM pg_proc p - JOIN pg_namespace n ON n.oid = p.pronamespace - JOIN pg_language l ON l.oid = p.prolang -WHERE n.nspname NOT IN ('pg_catalog', 'information_schema') - AND LEFT(n.nspname, 3) <> 'pg_' - AND (l.lanname = 'exfunc' - OR (p.prosecdef AND p.proowner <> (SELECT usesysid FROM pg_user WHERE usename = current_user))) -LIMIT ${MAX_LISTED_TABLES}`; - -/** Rows of each catalog query. The Data API returns booleans as "true"/"false". */ -interface CatalogRows { - superuser: string[][]; - tables: string[][]; - sequences: string[][]; - functions: string[][]; -} - -function databaseFindings(rows: CatalogRows): WriteAccessResult { - const findings: string[] = []; - if (rows.superuser[0]?.[0] === "true") { - findings.push("The database user is a superuser."); - } - for (const [table] of rows.tables) { - findings.push(`The database user can write table ${table}.`); - } - for (const [sequence] of rows.sequences) { - findings.push( - `The database user can advance or reset sequence ${sequence} ` + - `(nextval/setval).`, - ); - } - for (const [fn] of rows.functions) { - findings.push( - `A query can call function ${fn}, which runs with another role's ` + - `privileges and can write.`, - ); - } - if (findings.length > 0) return { status: "can-write", findings }; - return { - status: "unverified", - reason: - "No write privilege was found, but a SELECT on this engine can still " + - "change data through functions and extensions this check cannot see. " + - CUSTOMER_DUTY, - }; -} - -/** - * Ends an S3 URI with `/`, so a prefix comparison cannot match `s3://b/data` - * against `s3://b/data-results`. - */ -function withTrailingSlash(uri: string): string { - const trimmed = uri.trim(); - return trimmed.endsWith("/") ? trimmed : `${trimmed}/`; -} - -/** - * Where Athena writes query results relative to the table's data. - * - * - `inside`: every query writes a result file the table then reads as rows. - * - `outside`: queries do not add rows. - * - `undetermined`: the location could not be established; `reason` says why. - * - * A workgroup with EnforceWorkGroupConfiguration replaces the location a query - * submits, so the workgroup is read (with `athena:GetWorkGroup`) even when - * `athenaOutputLocation` is set. Athena uses `primary` when no workgroup is - * configured. Test connection calls this before its Athena probe, because the - * probe itself writes a result file. - */ -export type AthenaResultLocation = - | { kind: "inside"; finding: string } - | { kind: "outside" } - | { kind: "undetermined"; reason: string }; - -export async function checkAthenaResultLocation( - cfg: TelemetryConfig, - credentials: ReturnType, -): Promise { - let outputLocation: string | undefined; - let dataLocation: string | undefined; - try { - const workgroup = await getWorkGroupOutput({ - region: cfg.region, - credentials, - workgroup: cfg.athenaWorkgroup || "primary", - }); - outputLocation = workgroup.enforced - ? workgroup.outputLocation - : cfg.athenaOutputLocation || workgroup.outputLocation; - const tableLocation = await getTableLocation({ - region: cfg.region, - credentials, - database: cfg.athenaDatabase, - table: cfg.athenaTable, - }); - dataLocation = tableLocation ?? (cfg.athenaS3Location || undefined); - } catch (err) { - return { - kind: "undetermined", - reason: `Could not read the Athena workgroup or Glue table: ${errorMessage(err)}`, - }; - } - if (!outputLocation) { - return { - kind: "undetermined", - reason: - "Neither the settings nor the Athena workgroup set a result location.", - }; - } - if (!dataLocation) { - return { - kind: "undetermined", - reason: "The table does not exist and no S3 data location is set.", - }; - } - if ( - withTrailingSlash(outputLocation).startsWith( - withTrailingSlash(dataLocation), - ) - ) { - return { - kind: "inside", - finding: - `Athena writes query results to ${outputLocation}, which is inside ` + - `the table's data location ${dataLocation}. The table reads those ` + - `result files as records, so every query adds rows. Move the result ` + - `location outside the data location.`, - }; - } - return { kind: "outside" }; -} - -function errorMessage(err: unknown): string { - return err instanceof Error ? err.message : String(err); -} - -/** - * Looks for ways queries through these tools could change data at `cfg`'s - * destination. Never throws: a failure of the check itself is returned as - * `unverified` with the error as the reason. - */ -export async function checkWriteAccess( - cfg: TelemetryConfig, -): Promise { - const credentials = resolveCredentials(cfg.awsProfile); - try { - switch (cfg.destinationType) { - case "aurora": { - const conn = { - region: cfg.region, - credentials, - resourceArn: cfg.auroraResourceArn, - secretArn: cfg.auroraSecretArn, - database: cfg.auroraDatabase, - }; - const superuser = await runAuroraQuery({ - ...conn, - sql: POSTGRES_SUPERUSER_SQL, - }); - const tables = await runAuroraQuery({ - ...conn, - sql: POSTGRES_WRITABLE_TABLES_SQL, - }); - const sequences = await runAuroraQuery({ - ...conn, - sql: POSTGRES_WRITABLE_SEQUENCES_SQL, - }); - const functions = await runAuroraQuery({ - ...conn, - sql: POSTGRES_DEFINER_FUNCTIONS_SQL, - }); - return databaseFindings({ - superuser: superuser.rows, - tables: tables.rows, - sequences: sequences.rows, - functions: functions.rows, - }); - } - case "redshift": { - const conn = { - region: cfg.region, - credentials, - database: cfg.redshiftDatabase, - workgroupName: cfg.redshiftWorkgroupName || undefined, - clusterIdentifier: cfg.redshiftClusterIdentifier || undefined, - dbUser: cfg.redshiftDbUser || undefined, - secretArn: cfg.redshiftSecretArn || undefined, - }; - const superuser = await runRedshiftQuery({ - ...conn, - sql: REDSHIFT_SUPERUSER_SQL, - }); - const tables = await runRedshiftQuery({ - ...conn, - sql: REDSHIFT_WRITABLE_TABLES_SQL, - }); - const functions = await runRedshiftQuery({ - ...conn, - sql: REDSHIFT_RISKY_FUNCTIONS_SQL, - }); - // Redshift has no sequences (it uses IDENTITY columns). - return databaseFindings({ - superuser: superuser.rows, - tables: tables.rows, - sequences: [], - functions: functions.rows, - }); - } - case "s3": { - const location = await checkAthenaResultLocation(cfg, credentials); - if (location.kind === "inside") { - return { status: "can-write", findings: [location.finding] }; - } - return { - status: "unverified", - reason: - (location.kind === "undetermined" - ? `${location.reason} ` - : "Athena writes query results outside the table's data. ") + - "These tools cannot read the identity's S3 and Glue permissions. " + - CUSTOMER_DUTY, - }; - } - default: - return { - status: "unverified", - reason: - "These tools cannot read this destination's permissions. " + - CUSTOMER_DUTY, - }; - } - } catch (err) { - return { - status: "unverified", - reason: `The check could not run: ${errorMessage(err)} ${CUSTOMER_DUTY}`, - }; - } -} diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/README.md b/packages/aws-durable-execution-sdk-js-telemetry-mcp/README.md index a165a65cb..806bc85df 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/README.md +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/README.md @@ -127,34 +127,19 @@ needs read access to whichever destination you point at. > - **OpenSearch:** map the identity to a role with read-only index permissions > on the telemetry index. -### Credentials that can change data +### Confirming the read-only responsibility Before the first `query`, `get_execution` or `list_executions` call in a -session, the server looks for ways that queries could change data: - -- **Aurora:** whether the database user is a superuser, can write any table, - can advance or reset a sequence (`nextval`/`setval`), or can call a - `SECURITY DEFINER` function that another role owns. -- **Redshift:** whether the database user is a superuser, can write any table, - or can call a Lambda UDF or another role's `SECURITY DEFINER` function. -- **S3 + Athena:** whether Athena writes query results inside the table's data - location, where the table reads them as records. It reads the workgroup with - `athena:GetWorkGroup`, because a workgroup that enforces its configuration - replaces the configured result location. -- **Other destinations:** nothing is checked. - -The server never concludes that your credentials are read-only. A `SELECT` can -change data in ways no check can list, so finding nothing proves nothing. -Checking that the credentials can only read is your responsibility. - -So the server always asks you to accept before the first query. The prompt -names what it found, or says it could not confirm the credentials are -read-only. A client that supports MCP form elicitation shows the prompt; -accepting allows queries for the rest of the session. A client that does not -support it gets a tool error. To accept permanently, set +session, the server asks you to confirm one line: queries are written by the +agent, and it is your responsibility to give this server read-only +credentials. The server does not check the credentials, because no check can +show that a `SELECT` cannot change data. + +A client that supports MCP form elicitation shows the prompt; confirming allows +queries for the rest of the session. A client that does not support it gets a +tool error. To confirm permanently, set `DURABLE_TELEMETRY_ACCEPT_WRITE_ACCESS=true` in the server's `env` block. Only -you can accept: no tool accepts on the agent's behalf. `test_destination` -reports the same check in its `writeAccess` field. +you can confirm: no tool confirms on the agent's behalf. ### Destinations and their required variables diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/envKeys.ts b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/envKeys.ts index fe91fbe56..ab5ddd7be 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/envKeys.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/envKeys.ts @@ -60,9 +60,10 @@ export const MCP_SETTING_KEYS: readonly string[] = SETTING_KEYS.filter( ); /** - * Set to `true` in the server's `env` block to accept that the configured - * credentials can change data (see writeAccessGate.ts). It is not a setting - * key: the extension and desktop app only warn, so they have no such setting. + * Set to `true` in the server's `env` block to confirm, permanently, that + * giving this server read-only credentials is the user's responsibility (see + * writeAccessGate.ts). It is not a setting key: the extension and desktop app + * only show a warning, so they have no such setting. */ export const ACCEPT_WRITE_ACCESS_ENV_VAR = "DURABLE_TELEMETRY_ACCEPT_WRITE_ACCESS"; diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/queryChokePoint.test.ts b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/queryChokePoint.test.ts index 1c9f8df2d..95f92cc57 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/queryChokePoint.test.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/queryChokePoint.test.ts @@ -97,12 +97,6 @@ const PERMITTED_OUTSIDE_CHOKE_POINT: Record< "the test_destination tool. Core's own connectivity probe; it issues fixed " + "read-only statements that no caller can influence.", }, - checkWriteAccess: { - file: "registerTools.ts", - reason: - "the write-access gate before the first data query. It issues fixed " + - "catalog SELECTs and a Glue GetTable that no caller can influence.", - }, }; /** Every non-test `.ts` file under this package's src, recursively. */ diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.test.ts b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.test.ts index edfe738e0..b73cd1ac5 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.test.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.test.ts @@ -1,16 +1,14 @@ /** * Calls the registered tools through a real MCP client, connected in memory, - * with core's write check and this package's query paths replaced by spies. + * with this package's query paths replaced by spies. * - * The property under test: until the user accepts, no data tool reaches its - * query path, whatever the write check reports. Deleting any tool's - * `if (blocked) return blocked;` fails a test here. + * The property under test: until the user confirms, no data tool reaches its + * query path. Deleting any tool's `if (blocked) return blocked;` fails a test + * here. */ -const checkWriteAccess = jest.fn(); const testDestination = jest.fn(); jest.mock("@aws/durable-execution-sdk-js-telemetry-core", () => ({ ...jest.requireActual("@aws/durable-execution-sdk-js-telemetry-core"), - checkWriteAccess: (...args: unknown[]) => checkWriteAccess(...args), testDestination: (...args: unknown[]) => testDestination(...args), })); const runReadOnlyQuery = jest.fn(); @@ -41,11 +39,6 @@ const CONFIG = readConfigFromEnv({ DURABLE_TELEMETRY_REGION: "us-east-1", }).config; -const CAN_WRITE = { - status: "can-write", - findings: ["The database user can write table public.t."], -}; - /** The data tools, each with arguments and the spy for its query path. */ const DATA_TOOLS = [ ["query", { sql: "SELECT 1" }, runReadOnlyQuery], @@ -82,58 +75,41 @@ beforeEach(() => { }); describe.each(DATA_TOOLS)("%s", (tool, args, queryPath) => { - it("does not reach its query path when queries could change data", async () => { - checkWriteAccess.mockResolvedValue(CAN_WRITE); - const client = await connect(false); - const result = await client.callTool({ name: tool, arguments: args }); - expect(result.isError).toBe(true); - expect(payloadOf(result).writeAccess).toEqual(CAN_WRITE); - expect(queryPath).not.toHaveBeenCalled(); - await client.close(); - }); - - it("does not reach its query path when the check found nothing", async () => { - // Finding nothing is not proof of read-only, so the user must still accept. - checkWriteAccess.mockResolvedValue({ status: "unverified", reason: "r" }); + it("does not reach its query path until the user confirms", async () => { const client = await connect(false); const result = await client.callTool({ name: tool, arguments: args }); expect(result.isError).toBe(true); + expect(payloadOf(result).error).toContain( + "It is your responsibility to give this server read-only credentials.", + ); expect(queryPath).not.toHaveBeenCalled(); await client.close(); }); - it("reaches its query path without checking when the environment accepts", async () => { + it("reaches its query path when the environment confirms", async () => { const client = await connect(true); await client.callTool({ name: tool, arguments: args }); - expect(checkWriteAccess).not.toHaveBeenCalled(); expect(queryPath).toHaveBeenCalledTimes(1); await client.close(); }); }); describe("test_destination", () => { - it("reports the write check and is not blocked by it", async () => { - testDestination.mockResolvedValue({ - ok: true, - summary: "s", - checks: [], - writeAccess: CAN_WRITE, - }); + it("is not blocked by the gate", async () => { + testDestination.mockResolvedValue({ ok: true, summary: "s", checks: [] }); const client = await connect(false); const result = await client.callTool({ name: "test_destination", arguments: {}, }); expect(result.isError).toBeFalsy(); - expect(payloadOf(result).writeAccess).toEqual(CAN_WRITE); - expect(checkWriteAccess).not.toHaveBeenCalled(); + expect(testDestination).toHaveBeenCalledTimes(1); await client.close(); }); }); describe.each(DATA_TOOLS)("%s and cancellation", (tool, args, queryPath) => { it("does not query for a call cancelled while it waited for consent", async () => { - checkWriteAccess.mockResolvedValue(CAN_WRITE); const server = new McpServer({ name: "s", version: "0" }); registerTools(server, CONFIG, false); // A client that supports the prompt, and answers it only when told to. diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.ts b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.ts index 9ec059898..07757b04b 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.ts @@ -6,7 +6,6 @@ */ import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { - checkWriteAccess, testDestination, type TelemetryConfig, } from "@aws/durable-execution-sdk-js-telemetry-core"; @@ -68,7 +67,6 @@ export function registerTools( ): void { // The three data tools call this before any query; see writeAccessGate.ts. const writeAccessGate = createWriteAccessGate({ - check: () => checkWriteAccess(config), acceptedByEnv, ask: (message) => askUserToAccept(server.server, message), }); @@ -144,7 +142,6 @@ export function registerTools( ok: report.ok, summary: report.summary, checks: report.checks, - writeAccess: report.writeAccess, destinationType: config.destinationType, region: config.region, // Nothing was missing on this path; report it explicitly so the diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.test.ts b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.test.ts index c31b7f874..40f1a7556 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.test.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.test.ts @@ -1,112 +1,84 @@ -import type { WriteAccessResult } from "@aws/durable-execution-sdk-js-telemetry-core"; import { ACCEPT_WRITE_ACCESS_ENV_VAR, askUserToAccept, + CONSENT_MESSAGE, createWriteAccessGate, type ElicitingServer, type PromptAnswer, writeAccessAcceptedByEnv, } from "./writeAccessGate"; -const CAN_WRITE: WriteAccessResult = { - status: "can-write", - findings: ["The database user is a superuser."], -}; - function gate(opts: { - result: WriteAccessResult; answer?: PromptAnswer | Error; acceptedByEnv?: boolean; }) { - const check = jest.fn(async () => opts.result); const ask = jest.fn(async () => { if (opts.answer instanceof Error) throw opts.answer; return opts.answer; }); const run = createWriteAccessGate({ - check, ask, acceptedByEnv: opts.acceptedByEnv ?? false, }); - return { run, check, ask }; + return { run, ask }; } describe("createWriteAccessGate", () => { - it.each([ - CAN_WRITE, - { status: "unverified", reason: "Nothing found." }, - ] as WriteAccessResult[])( - "asks the user when the check reports $status", - async (result) => { - const { run, ask } = gate({ result, answer: "accept" }); - expect(await run()).toBeNull(); - expect(ask).toHaveBeenCalledTimes(1); - }, - ); - - it("blocks with the findings when the user declines", async () => { - const { run } = gate({ result: CAN_WRITE, answer: "decline" }); - const blocked = await run(); - expect(blocked?.writeAccess).toEqual(CAN_WRITE); - expect(blocked?.error).toContain("The database user is a superuser."); - expect(blocked?.error).toContain("The user did not accept the risk."); - expect(blocked?.howToAccept).toContain(ACCEPT_WRITE_ACCESS_ENV_VAR); + it("asks the user to confirm the read-only responsibility", async () => { + const { run, ask } = gate({ answer: "accept" }); + expect(await run()).toBeNull(); + expect(ask).toHaveBeenCalledWith(CONSENT_MESSAGE); + expect(CONSENT_MESSAGE).toContain( + "It is your responsibility to give this server read-only credentials.", + ); }); - it("tells an unverified user that the credentials are unconfirmed", async () => { - const { run, ask } = gate({ - result: { status: "unverified", reason: "Nothing found." }, - answer: "decline", - }); + it("blocks with the message when the user declines", async () => { + const { run } = gate({ answer: "decline" }); const blocked = await run(); - expect(ask.mock.calls[0]).toEqual([ - expect.stringContaining( - "could not confirm that its credentials are read-only. Nothing found.", - ), - ]); - expect(blocked?.error).toContain("Nothing found."); + expect(blocked?.error).toContain(CONSENT_MESSAGE); + expect(blocked?.error).toContain("The user did not confirm."); + expect(blocked?.howToAccept).toContain(ACCEPT_WRITE_ACCESS_ENV_VAR); }); it("blocks when the client cannot show a prompt", async () => { - const { run } = gate({ result: CAN_WRITE, answer: undefined }); + const { run } = gate({ answer: undefined }); expect((await run())?.error).toContain( - "The client cannot show an accept prompt.", + "The client cannot show a confirmation prompt.", ); }); it("blocks like a client without prompts when the prompt request fails", async () => { - const { run } = gate({ result: CAN_WRITE, answer: new Error("timeout") }); + const { run } = gate({ answer: new Error("timeout") }); expect((await run())?.error).toContain( - "The client cannot show an accept prompt.", + "The client cannot show a confirmation prompt.", ); }); - it("checks and asks only once after the user accepts", async () => { - const { run, check, ask } = gate({ result: CAN_WRITE, answer: "accept" }); + it("asks only once after the user confirms", async () => { + const { run, ask } = gate({ answer: "accept" }); await run(); expect(await run()).toBeNull(); - expect(check).toHaveBeenCalledTimes(1); expect(ask).toHaveBeenCalledTimes(1); }); it("asks again on the next call after a decline", async () => { - const { run, ask } = gate({ result: CAN_WRITE, answer: "decline" }); + const { run, ask } = gate({ answer: "decline" }); await run(); await run(); expect(ask).toHaveBeenCalledTimes(2); }); - it("shares one check and one prompt between concurrent calls", async () => { - const { run, check, ask } = gate({ result: CAN_WRITE, answer: "accept" }); + it("shares one prompt between concurrent calls", async () => { + const { run, ask } = gate({ answer: "accept" }); await Promise.all([run(), run(), run()]); - expect(check).toHaveBeenCalledTimes(1); expect(ask).toHaveBeenCalledTimes(1); }); - it("skips the check entirely when the environment accepts the risk", async () => { - const { run, check } = gate({ result: CAN_WRITE, acceptedByEnv: true }); + it("never asks when the environment confirms", async () => { + const { run, ask } = gate({ acceptedByEnv: true }); expect(await run()).toBeNull(); - expect(check).not.toHaveBeenCalled(); + expect(ask).not.toHaveBeenCalled(); }); }); diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.ts b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.ts index 94c777e33..1a0a5aea7 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.ts @@ -1,27 +1,26 @@ /** - * Stops the data tools until the user accepts the risk that queries could - * change data. + * Stops the data tools until the user confirms, once per process, that giving + * this server read-only credentials is their responsibility. * - * WHY THIS GATE ALWAYS ASKS. + * WHY THE SERVER ASKS INSTEAD OF CHECKING. * * 1. The agent writes the SQL this server runs, and the agent can be * prompt-injected. * 2. `assertReadOnly` reads query text. On Aurora and Redshift a SELECT can - * still write through a function, so it cannot block every write. - * 3. Core's `checkWriteAccess` reports either `can-write` (it found a way) or - * `unverified` (it found none, could not look, or failed). It never reports - * read-only, because no check can show that. - * 4. So the user accepts once per process before the first query, and the - * prompt says which of the two results the check gave. + * still change data through functions, sequences and extensions, so it + * cannot block every write. + * 3. No check of the credentials can show they are read-only, so the server + * does not try. The credentials' permissions are the only real protection, + * and only the user controls them. + * 4. So the user confirms that responsibility before the first query. * - * WHO CAN ACCEPT. + * WHO CAN CONFIRM. * - * Only the user. Acceptance comes from an elicitation prompt that the client + * Only the user. Confirmation comes from an elicitation prompt that the client * shows to the user, or from the {@link ACCEPT_WRITE_ACCESS_ENV_VAR} variable - * in the client's server config. There is deliberately no tool that accepts, + * in the client's server config. There is deliberately no tool that confirms, * because the agent could call it on the user's behalf. */ -import type { WriteAccessResult } from "@aws/durable-execution-sdk-js-telemetry-core"; import { ACCEPT_WRITE_ACCESS_ENV_VAR } from "./envKeys"; export { ACCEPT_WRITE_ACCESS_ENV_VAR }; @@ -37,9 +36,12 @@ export function writeAccessAcceptedByEnv(env: NodeJS.ProcessEnv): boolean { */ export type PromptAnswer = "accept" | "decline" | "cancel" | undefined; +/** The one line the user is asked to confirm. */ +export const CONSENT_MESSAGE = + "Queries are written by the agent and run with this server's credentials. " + + "It is your responsibility to give this server read-only credentials."; + export interface WriteAccessGateDeps { - /** Runs core's check against the configured destination. */ - check: () => Promise; /** Whether {@link ACCEPT_WRITE_ACCESS_ENV_VAR} accepts the risk. */ acceptedByEnv: boolean; /** Shows `message` to the user and returns the answer. */ @@ -66,8 +68,7 @@ export interface ElicitingServer { } /** - * Asks the user, through MCP form elicitation, to accept credentials that can - * change data. Returns `undefined` when the client cannot show a form. + * Asks the user, through MCP form elicitation, to confirm `message`. Returns `undefined` when the client cannot show a form. * * The form has one checkbox that defaults to unchecked. A user who submits the * form without checking it has not accepted, so that counts as a decline. @@ -84,7 +85,7 @@ export async function askUserToAccept( properties: { acceptRisk: { type: "boolean", - title: "I accept that queries in this session can change data", + title: "I understand, and I want to continue", default: false, }, }, @@ -99,7 +100,6 @@ export async function askUserToAccept( /** The tool-result payload returned instead of running the query. */ export interface WriteAccessBlocked { error: string; - writeAccess: WriteAccessResult; howToAccept: string; } @@ -107,22 +107,20 @@ export interface WriteAccessBlocked { * Returns a function that the data tools call before each query. It resolves * to `null` when the query may run, or to a {@link WriteAccessBlocked} payload. * - * Once the query may run, it may run for the rest of the process, so the - * check and the prompt happen at most once after acceptance. A blocked result - * is not remembered: the next tool call checks and asks again. + * Once the user confirms, queries may run for the rest of the process. A + * blocked result is not remembered: the next tool call asks again. */ export function createWriteAccessGate( deps: WriteAccessGateDeps, ): () => Promise { let allowed = deps.acceptedByEnv; - // Concurrent tool calls share one check and one prompt. + // Concurrent tool calls share one prompt. let pending: Promise | undefined; async function decide(): Promise { - const result = await deps.check(); let answer: PromptAnswer; try { - answer = await deps.ask(promptMessage(result)); + answer = await deps.ask(CONSENT_MESSAGE); } catch { // The client advertised prompts but the request failed. Treat it like a // client that cannot prompt, so the user still gets the env var route. @@ -132,7 +130,7 @@ export function createWriteAccessGate( allowed = true; return null; } - return blocked(result, answer); + return blocked(answer); } return async () => { @@ -144,39 +142,16 @@ export function createWriteAccessGate( }; } -function describe(result: WriteAccessResult): string { - if (result.status === "can-write") { - return `The credentials this server uses can change data. ${result.findings.join(" ")}`; - } - return ( - "The server could not confirm that its credentials are read-only. " + - result.reason - ); -} - -function promptMessage(result: WriteAccessResult): string { - return ( - `${describe(result)} ` + - "Queries are written by the agent, and the server's read-only check " + - "reads query text, so it cannot block every write. Accept to continue " + - "for this session." - ); -} - -function blocked( - result: WriteAccessResult, - answer: PromptAnswer, -): WriteAccessBlocked { +function blocked(answer: PromptAnswer): WriteAccessBlocked { const reason = answer === undefined - ? "The client cannot show an accept prompt." - : "The user did not accept the risk."; + ? "The client cannot show a confirmation prompt." + : "The user did not confirm."; return { - error: `Refused to run queries. ${describe(result)} ${reason}`, - writeAccess: result, + error: `Refused to run queries. ${CONSENT_MESSAGE} ${reason}`, howToAccept: - `Ask the user to switch to read-only credentials, or to set ` + - `${ACCEPT_WRITE_ACCESS_ENV_VAR}=true in this server's env block and ` + - `restart it. Only the user can accept; there is no tool for it.`, + `Ask the user to confirm, or to set ${ACCEPT_WRITE_ACCESS_ENV_VAR}=true ` + + `in this server's env block and restart it. Only the user can confirm; ` + + `there is no tool for it.`, }; } diff --git a/packages/aws-durable-execution-sdk-js-telemetry-vscode/README.md b/packages/aws-durable-execution-sdk-js-telemetry-vscode/README.md index b45a7b1ec..d28131b3b 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-vscode/README.md +++ b/packages/aws-durable-execution-sdk-js-telemetry-vscode/README.md @@ -420,16 +420,10 @@ Redshift needs one credential action, chosen by how you connect: > save and to write Athena results. Scope `s3:PutObject` to the results bucket. > If the Glue table already exists, omit the two Glue write actions. > -> **Test connection** in Settings looks for ways that queries could change -> data. On Aurora it checks superuser status, writable tables, sequences, and -> other roles' `SECURITY DEFINER` functions. On Redshift it checks superuser -> status, writable tables, Lambda UDFs and `SECURITY DEFINER` functions. On -> S3 + Athena it checks whether query results land inside the table's data, -> where every query would add rows; it skips its own test query in that case. -> A passing test always shows a warning. Either it lists what it found, or it -> says it could not confirm the credentials are read-only. Finding nothing -> proves nothing, so checking the credentials is your responsibility. The -> warning does not stop you from saving. +> The Settings page repeats this as a one-line warning. For S3 + Athena, +> **Test connection** also checks where Athena writes query results. If they +> would land inside the table's data, every query would add rows, so the test +> skips its own test query and reports the problem. ## How Queries Are Generated diff --git a/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/package.json b/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/package.json index 2d68d6868..ca1252994 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/package.json +++ b/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/package.json @@ -7,8 +7,7 @@ }, "scripts": { "build": "node build.mjs", - "watch": "node build.mjs --watch", - "test": "node test.mjs" + "watch": "node build.mjs --watch" }, "dependencies": { "@cloudscape-design/code-view": "^3.0.185", diff --git a/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/src/SettingsModal.tsx b/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/src/SettingsModal.tsx index 6ff026ae9..92018db42 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/src/SettingsModal.tsx +++ b/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/src/SettingsModal.tsx @@ -18,7 +18,6 @@ import type { } from "./types"; import { RECOMMENDED_BEDROCK_MODELS } from "./types"; import { postMessage } from "./hostBridge"; -import { WriteAccessWarning } from "./WriteAccessWarning"; interface Props { visible: boolean; @@ -523,6 +522,10 @@ export function SettingsModal({ the config is complete) without saving. For S3 + Athena it also verifies the Glue table and runs a test query. + + Queries can be written by a model. It is your responsibility + to use read-only credentials. + {testResult && ( )} - ), diff --git a/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/src/WriteAccessWarning.test.tsx b/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/src/WriteAccessWarning.test.tsx deleted file mode 100644 index 2bf2d707e..000000000 --- a/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/src/WriteAccessWarning.test.tsx +++ /dev/null @@ -1,41 +0,0 @@ -import assert from "node:assert/strict"; -import { describe, it } from "node:test"; -import { renderToStaticMarkup } from "react-dom/server"; -import { WriteAccessWarning } from "./WriteAccessWarning"; -import type { DestinationTestReport } from "./types"; - -function render(writeAccess: DestinationTestReport["writeAccess"]): string { - return renderToStaticMarkup(); -} - -describe("WriteAccessWarning", () => { - it("lists every finding when the credentials can change data", () => { - const html = render({ - status: "can-write", - findings: [ - "The database user can write table public.t.", - "The database user is a superuser.", - ], - }); - assert.match(html, /These credentials can change data/); - assert.match(html, /The database user can write table public\.t\./); - assert.match(html, /The database user is a superuser\./); - }); - - it("says the credentials are unconfirmed, with the reason, when nothing was found", () => { - const html = render({ - status: "unverified", - reason: - "It is your responsibility to give these tools read-only credentials.", - }); - assert.match( - html, - /Could not confirm that these credentials are read-only/, - ); - assert.match(html, /It is your responsibility/); - }); - - it("renders nothing when there is no result", () => { - assert.equal(render(undefined), ""); - }); -}); diff --git a/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/src/WriteAccessWarning.tsx b/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/src/WriteAccessWarning.tsx deleted file mode 100644 index d76c6d1ed..000000000 --- a/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/src/WriteAccessWarning.tsx +++ /dev/null @@ -1,52 +0,0 @@ -import Alert from "@cloudscape-design/components/alert"; -import Box from "@cloudscape-design/components/box"; -import SpaceBetween from "@cloudscape-design/components/space-between"; -import type { DestinationTestReport } from "./types"; - -/** - * The warning under "Test connection". Every passing test shows one. - * - * - `can-write`: names each way a query could change data. - * - `unverified`: the check found nothing, could not look, or failed. That - * does not mean the credentials are read-only, so the warning says checking - * them is the customer's responsibility and shows the reason. - * - No result (a check failed earlier): renders nothing. - * - * It never blocks saving; the MCP server is the host that asks for consent. - */ -export function WriteAccessWarning({ - writeAccess, -}: { - writeAccess: DestinationTestReport["writeAccess"]; -}) { - if (writeAccess?.status === "can-write") { - return ( - - - - Queries here are written by a model. The explorer refuses queries - that are not read-only, but that check reads query text and cannot - block every write. Use credentials that can only read. See "Required - IAM Permissions" in the README. - - {writeAccess.findings.map((f) => ( - - • {f} - - ))} - - - ); - } - if (writeAccess?.status === "unverified") { - return ( - - {writeAccess.reason} - - ); - } - return null; -} diff --git a/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/src/types.ts b/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/src/types.ts index fd165a44a..0bf16aff7 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/src/types.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/src/types.ts @@ -66,10 +66,6 @@ export interface DestinationTestReport { ok: boolean; summary: string; checks: DestinationCheck[]; - /** Mirrors core's WriteAccessResult; present only when every check passed. */ - writeAccess?: - | { status: "can-write"; findings: string[] } - | { status: "unverified"; reason: string }; } /** A single SQS message, normalized for display. */ diff --git a/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/test.mjs b/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/test.mjs deleted file mode 100644 index d3c1ab476..000000000 --- a/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/test.mjs +++ /dev/null @@ -1,43 +0,0 @@ -// Runs the renderer's tests (src/**/*.test.tsx) with Node's built-in test runner. -// -// The renderer has no test framework of its own, and the extension's Jest -// config ignores this folder. esbuild already builds the renderer, so it also -// bundles each test file for Node, with the same JSX settings and with CSS -// imports dropped. `node --test` then runs the bundles. -import { spawnSync } from "node:child_process"; -import { mkdtempSync, readdirSync, rmSync } from "node:fs"; -import { tmpdir } from "node:os"; -import { basename, join } from "node:path"; -import esbuild from "esbuild"; - -const tests = readdirSync("src", { recursive: true }) - .filter((f) => /\.test\.tsx?$/.test(f)) - .map((f) => join("src", f)); -if (tests.length === 0) { - console.error("No renderer tests found under src/."); - process.exit(1); -} - -const outdir = mkdtempSync(join(tmpdir(), "webview-ui-tests-")); -try { - await esbuild.build({ - entryPoints: tests, - bundle: true, - platform: "node", - format: "cjs", - outdir, - outExtension: { ".js": ".cjs" }, - jsx: "automatic", - loader: { ".css": "empty" }, - logLevel: "warning", - }); - const bundles = tests.map((t) => - join(outdir, basename(t).replace(/\.tsx?$/, ".cjs")), - ); - const run = spawnSync(process.execPath, ["--test", ...bundles], { - stdio: "inherit", - }); - process.exitCode = run.status ?? 1; -} finally { - rmSync(outdir, { recursive: true, force: true }); -} From fa0d22d4693b302a00fd2a917aad3808e6bc6463 Mon Sep 17 00:00:00 2001 From: Pooya Paridel Date: Tue, 27 Oct 2026 22:38:36 -0700 Subject: [PATCH 2/5] fix(telemetry): name an Athena result location inside the data; MCP 1.1.0 Review of the notice found that the MCP prompt no longer warned when Athena writes query results inside the table's data, where every query adds rows whatever the credentials. #1110's check had covered it. - mcp: the gate takes an optional configurationWarning. For s3 it runs checkAthenaResultLocation and, when results land inside the data, appends the finding to the prompt and to a blocked result. - mcp: the prompt's checkbox field is `confirmed` (was `acceptRisk`), and the gate docs say "confirm" instead of "accept the risk". - vscode/desktop: Settings uses the same wording as the MCP line. - mcp README: the S3 note, and why athena:GetWorkGroup is needed. - Bump the MCP package to 1.1.0. Release notes and a changelog entry record that test_destination and blocked results no longer carry writeAccess. --- CHANGELOG.md | 9 +++ package-lock.json | 2 +- .../README.md | 7 +++ .../package.json | 2 +- .../src/registerTools.test.ts | 60 ++++++++++++++++++- .../src/registerTools.ts | 12 ++++ .../src/writeAccessGate.test.ts | 22 +++++-- .../src/writeAccessGate.ts | 28 +++++---- .../webview-ui/src/SettingsModal.tsx | 5 +- release-notes/telemetry-mcp-1.1.0.md | 28 +++++++++ 10 files changed, 154 insertions(+), 21 deletions(-) create mode 100644 release-notes/telemetry-mcp-1.1.0.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a5fe4925..edc505285 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Telemetry MCP 1.1.0 + +- The server asks you to confirm, once per session, that giving it read-only + credentials is your responsibility. It no longer checks the credentials, so + `test_destination` and blocked data-tool results no longer return a + `writeAccess` field. On S3 + Athena the prompt also names a result location + inside the table's data. See the + [release notes](release-notes/telemetry-mcp-1.1.0.md). + ### Coordinated release summary Core 3.0.0, OTel 2.0.0 and Testing SDK 1.3.2 release together. The entries diff --git a/package-lock.json b/package-lock.json index ec94ce65e..36e93ea36 100644 --- a/package-lock.json +++ b/package-lock.json @@ -16489,7 +16489,7 @@ }, "packages/aws-durable-execution-sdk-js-telemetry-mcp": { "name": "@aws/durable-execution-sdk-js-telemetry-mcp", - "version": "1.0.0", + "version": "1.1.0", "license": "Apache-2.0", "dependencies": { "@modelcontextprotocol/sdk": "^1.32.1", diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/README.md b/packages/aws-durable-execution-sdk-js-telemetry-mcp/README.md index 806bc85df..a82ebe5e4 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/README.md +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/README.md @@ -135,6 +135,13 @@ agent, and it is your responsibility to give this server read-only credentials. The server does not check the credentials, because no check can show that a `SELECT` cannot change data. +On S3 + Athena the prompt also says when Athena writes query results inside +the table's data location. The table then reads each result file as records, +so every query adds rows, whatever the credentials. To find the result +location, the server reads the Athena workgroup, which needs +`athena:GetWorkGroup`. `test_destination` skips its own test query in that +case and reports the problem. + A client that supports MCP form elicitation shows the prompt; confirming allows queries for the rest of the session. A client that does not support it gets a tool error. To confirm permanently, set diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/package.json b/packages/aws-durable-execution-sdk-js-telemetry-mcp/package.json index a44fda3b5..4fbb013d7 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/package.json +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/package.json @@ -1,6 +1,6 @@ { "name": "@aws/durable-execution-sdk-js-telemetry-mcp", - "version": "1.0.0", + "version": "1.1.0", "license": "Apache-2.0", "private": false, "repository": { diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.test.ts b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.test.ts index b73cd1ac5..c2d0d8235 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.test.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.test.ts @@ -7,9 +7,12 @@ * here. */ const testDestination = jest.fn(); +const checkAthenaResultLocation = jest.fn(); jest.mock("@aws/durable-execution-sdk-js-telemetry-core", () => ({ ...jest.requireActual("@aws/durable-execution-sdk-js-telemetry-core"), testDestination: (...args: unknown[]) => testDestination(...args), + checkAthenaResultLocation: (...args: unknown[]) => + checkAthenaResultLocation(...args), })); const runReadOnlyQuery = jest.fn(); jest.mock("./readOnlyQuery", () => ({ @@ -46,9 +49,19 @@ const DATA_TOOLS = [ ["list_executions", {}, runListExecutions], ] as const; -async function connect(acceptedByEnv: boolean): Promise { +const S3_CONFIG = readConfigFromEnv({ + DURABLE_TELEMETRY_DESTINATION_TYPE: "s3", + DURABLE_TELEMETRY_ATHENA_DATABASE: "db", + DURABLE_TELEMETRY_ATHENA_OUTPUT_LOCATION: "s3://bucket/telemetry/results/", + DURABLE_TELEMETRY_REGION: "us-east-1", +}).config; + +async function connect( + acceptedByEnv: boolean, + config = CONFIG, +): Promise { const server = new McpServer({ name: "s", version: "0" }); - registerTools(server, CONFIG, acceptedByEnv); + registerTools(server, config, acceptedByEnv); // No elicitation capability, so the gate cannot ask and must block. const client = new Client({ name: "c", version: "0" }, { capabilities: {} }); const [a, b] = InMemoryTransport.createLinkedPair(); @@ -94,6 +107,47 @@ describe.each(DATA_TOOLS)("%s", (tool, args, queryPath) => { }); }); +describe("the S3 result-location warning", () => { + it("names Athena results inside the table's data before the first query", async () => { + checkAthenaResultLocation.mockResolvedValue({ + kind: "inside", + finding: "Athena writes query results inside the table's data.", + }); + const client = await connect(false, S3_CONFIG); + const result = await client.callTool({ + name: "query", + arguments: { sql: "SELECT 1" }, + }); + expect(payloadOf(result).error).toContain( + "Athena writes query results inside the table's data.", + ); + expect(runReadOnlyQuery).not.toHaveBeenCalled(); + await client.close(); + }); + + it.each([ + { kind: "outside" }, + { kind: "undetermined", reason: "No location." }, + ])("adds nothing when the location is $kind", async (location) => { + checkAthenaResultLocation.mockResolvedValue(location); + const client = await connect(false, S3_CONFIG); + const result = await client.callTool({ + name: "query", + arguments: { sql: "SELECT 1" }, + }); + expect(payloadOf(result).error).not.toMatch(/Athena|No location/); + expect(checkAthenaResultLocation).toHaveBeenCalledTimes(1); + await client.close(); + }); + + it("is not checked for other destinations", async () => { + const client = await connect(false); + await client.callTool({ name: "query", arguments: { sql: "SELECT 1" } }); + expect(checkAthenaResultLocation).not.toHaveBeenCalled(); + await client.close(); + }); +}); + describe("test_destination", () => { it("is not blocked by the gate", async () => { testDestination.mockResolvedValue({ ok: true, summary: "s", checks: [] }); @@ -123,7 +177,7 @@ describe.each(DATA_TOOLS)("%s and cancellation", (tool, args, queryPath) => { resolve(); return new Promise((reply) => { answer = () => - reply({ action: "accept", content: { acceptRisk: true } }); + reply({ action: "accept", content: { confirmed: true } }); }); }); }); diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.ts b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.ts index 07757b04b..8a60b34f2 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.ts @@ -6,6 +6,8 @@ */ import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { + checkAthenaResultLocation, + resolveCredentials, testDestination, type TelemetryConfig, } from "@aws/durable-execution-sdk-js-telemetry-core"; @@ -69,6 +71,16 @@ export function registerTools( const writeAccessGate = createWriteAccessGate({ acceptedByEnv, ask: (message) => askUserToAccept(server.server, message), + // On S3 + Athena, results written inside the table's data add rows on + // every query, whatever the credentials. So the prompt names that too. + configurationWarning: async () => { + if (config.destinationType !== "s3") return undefined; + const location = await checkAthenaResultLocation( + config, + resolveCredentials(config.awsProfile), + ); + return location.kind === "inside" ? location.finding : undefined; + }, }); // Returns a tool result when the call must not query, or null when it may. // The gate can wait a long time for the user's answer, and several calls can diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.test.ts b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.test.ts index 40f1a7556..96bc15556 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.test.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.test.ts @@ -11,6 +11,7 @@ import { function gate(opts: { answer?: PromptAnswer | Error; acceptedByEnv?: boolean; + warning?: string; }) { const ask = jest.fn(async () => { if (opts.answer instanceof Error) throw opts.answer; @@ -19,6 +20,7 @@ function gate(opts: { const run = createWriteAccessGate({ ask, acceptedByEnv: opts.acceptedByEnv ?? false, + configurationWarning: async () => opts.warning, }); return { run, ask }; } @@ -75,6 +77,18 @@ describe("createWriteAccessGate", () => { expect(ask).toHaveBeenCalledTimes(1); }); + it("adds a configuration warning to the prompt and to a blocked result", async () => { + const { run, ask } = gate({ + answer: "decline", + warning: "Results land in the data.", + }); + const blocked = await run(); + expect(ask).toHaveBeenCalledWith( + `${CONSENT_MESSAGE} Results land in the data.`, + ); + expect(blocked?.error).toContain("Results land in the data."); + }); + it("never asks when the environment confirms", async () => { const { run, ask } = gate({ acceptedByEnv: true }); expect(await run()).toBeNull(); @@ -120,7 +134,7 @@ describe("askUserToAccept", () => { it("accepts only when the user checks the box", async () => { const s = server( { elicitation: { form: {} } }, - { action: "accept", content: { acceptRisk: true } }, + { action: "accept", content: { confirmed: true } }, ); expect(await askUserToAccept(s, "m")).toBe("accept"); expect(s.elicitInput).toHaveBeenCalledWith( @@ -131,7 +145,7 @@ describe("askUserToAccept", () => { it("treats a submitted form with the box unchecked as a decline", async () => { const s = server( { elicitation: { form: {} } }, - { action: "accept", content: { acceptRisk: false } }, + { action: "accept", content: { confirmed: false } }, ); expect(await askUserToAccept(s, "m")).toBe("decline"); }); @@ -185,7 +199,7 @@ describe("askUserToAccept against the MCP SDK", () => { it("shows the prompt and accepts when the user checks the box", async () => { const { server, client, received } = await connect( { elicitation: {} }, - { action: "accept", content: { acceptRisk: true } }, + { action: "accept", content: { confirmed: true } }, ); expect(await askUserToAccept(server.server, "risk")).toBe("accept"); expect(received).toEqual([ @@ -203,7 +217,7 @@ describe("askUserToAccept against the MCP SDK", () => { it("rejects a reply that does not match the form schema", async () => { const { server, client } = await connect( { elicitation: {} }, - { action: "accept", content: { acceptRisk: "yes" } }, + { action: "accept", content: { confirmed: "yes" } }, ); await expect(askUserToAccept(server.server, "risk")).rejects.toThrow(); await client.close(); diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.ts b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.ts index 1a0a5aea7..870ea5a73 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.ts @@ -25,7 +25,7 @@ import { ACCEPT_WRITE_ACCESS_ENV_VAR } from "./envKeys"; export { ACCEPT_WRITE_ACCESS_ENV_VAR }; -/** True when the environment accepts the risk. Only `true` (any case) counts. */ +/** True when the environment confirms. Only `true` (any case) counts. */ export function writeAccessAcceptedByEnv(env: NodeJS.ProcessEnv): boolean { return /^true$/i.test((env[ACCEPT_WRITE_ACCESS_ENV_VAR] ?? "").trim()); } @@ -42,10 +42,16 @@ export const CONSENT_MESSAGE = "It is your responsibility to give this server read-only credentials."; export interface WriteAccessGateDeps { - /** Whether {@link ACCEPT_WRITE_ACCESS_ENV_VAR} accepts the risk. */ + /** Whether {@link ACCEPT_WRITE_ACCESS_ENV_VAR} confirms. */ acceptedByEnv: boolean; /** Shows `message` to the user and returns the answer. */ ask: (message: string) => Promise; + /** + * A problem with the destination's configuration that the user must also + * see before the first query, or `undefined` when there is none. It is + * added to the prompt and to a blocked result. + */ + configurationWarning?: () => Promise; } /** @@ -83,18 +89,18 @@ export async function askUserToAccept( requestedSchema: { type: "object", properties: { - acceptRisk: { + confirmed: { type: "boolean", title: "I understand, and I want to continue", default: false, }, }, - required: ["acceptRisk"], + required: ["confirmed"], }, }); if (answer.action !== "accept") return answer.action; - const content = answer.content as { acceptRisk?: unknown } | undefined; - return content?.acceptRisk === true ? "accept" : "decline"; + const content = answer.content as { confirmed?: unknown } | undefined; + return content?.confirmed === true ? "accept" : "decline"; } /** The tool-result payload returned instead of running the query. */ @@ -118,9 +124,11 @@ export function createWriteAccessGate( let pending: Promise | undefined; async function decide(): Promise { + const warning = await deps.configurationWarning?.(); + const message = warning ? `${CONSENT_MESSAGE} ${warning}` : CONSENT_MESSAGE; let answer: PromptAnswer; try { - answer = await deps.ask(CONSENT_MESSAGE); + answer = await deps.ask(message); } catch { // The client advertised prompts but the request failed. Treat it like a // client that cannot prompt, so the user still gets the env var route. @@ -130,7 +138,7 @@ export function createWriteAccessGate( allowed = true; return null; } - return blocked(answer); + return blocked(message, answer); } return async () => { @@ -142,13 +150,13 @@ export function createWriteAccessGate( }; } -function blocked(answer: PromptAnswer): WriteAccessBlocked { +function blocked(message: string, answer: PromptAnswer): WriteAccessBlocked { const reason = answer === undefined ? "The client cannot show a confirmation prompt." : "The user did not confirm."; return { - error: `Refused to run queries. ${CONSENT_MESSAGE} ${reason}`, + error: `Refused to run queries. ${message} ${reason}`, howToAccept: `Ask the user to confirm, or to set ${ACCEPT_WRITE_ACCESS_ENV_VAR}=true ` + `in this server's env block and restart it. Only the user can confirm; ` + diff --git a/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/src/SettingsModal.tsx b/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/src/SettingsModal.tsx index 92018db42..ee66a4aa9 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/src/SettingsModal.tsx +++ b/packages/aws-durable-execution-sdk-js-telemetry-vscode/webview-ui/src/SettingsModal.tsx @@ -523,8 +523,9 @@ export function SettingsModal({ also verifies the Glue table and runs a test query. - Queries can be written by a model. It is your responsibility - to use read-only credentials. + Queries are written by a model and run with these + credentials. It is your responsibility to use read-only + credentials. {testResult && ( Date: Tue, 27 Oct 2026 22:56:46 -0700 Subject: [PATCH 3/5] refactor(telemetry): make the gate's configurationWarning required Every caller passes it, so the optional call had a branch no test could reach (rule 4). --- .../src/writeAccessGate.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.ts b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.ts index 870ea5a73..97278d98d 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.ts @@ -51,7 +51,7 @@ export interface WriteAccessGateDeps { * see before the first query, or `undefined` when there is none. It is * added to the prompt and to a blocked result. */ - configurationWarning?: () => Promise; + configurationWarning: () => Promise; } /** @@ -124,7 +124,7 @@ export function createWriteAccessGate( let pending: Promise | undefined; async function decide(): Promise { - const warning = await deps.configurationWarning?.(); + const warning = await deps.configurationWarning(); const message = warning ? `${CONSENT_MESSAGE} ${warning}` : CONSENT_MESSAGE; let answer: PromptAnswer; try { From 38fbc2d58064ef0a92b5e6bf7631e2cbaf0cb360 Mon Sep 17 00:00:00 2001 From: Pooya Paridel Date: Tue, 27 Oct 2026 23:12:09 -0700 Subject: [PATCH 4/5] docs(telemetry): tidy comments and document when the S3 warning is skipped Reword comments that still named the removed write check. Document that the MCP prompt says nothing about the Athena result location when it cannot be read, or when DURABLE_TELEMETRY_ACCEPT_WRITE_ACCESS skips the prompt; test_destination reports it in both cases. Pin one configuration check per shared prompt in the gate test. --- .../src/athenaMetadata.test.ts | 2 +- .../src/athenaResultLocation.ts | 5 +++-- .../README.md | 6 ++++-- .../src/queryChokePoint.test.ts | 2 +- .../src/registerTools.ts | 2 +- .../src/writeAccessGate.test.ts | 10 ++++++---- .../src/writeAccessGate.ts | 3 ++- release-notes/telemetry-mcp-1.1.0.md | 4 +++- 8 files changed, 21 insertions(+), 13 deletions(-) diff --git a/packages/aws-durable-execution-sdk-js-telemetry-core/src/athenaMetadata.test.ts b/packages/aws-durable-execution-sdk-js-telemetry-core/src/athenaMetadata.test.ts index 6b5b18ff6..9045a7e67 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-core/src/athenaMetadata.test.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-core/src/athenaMetadata.test.ts @@ -51,7 +51,7 @@ describe("getTableLocation", () => { expect(await getTableLocation(table)).toBeUndefined(); }); - it("rethrows any other Glue error, so the write check reports unknown", async () => { + it("rethrows any other Glue error, so the location check reports undetermined", async () => { const err = Object.assign(new Error("denied"), { name: "AccessDeniedException", }); diff --git a/packages/aws-durable-execution-sdk-js-telemetry-core/src/athenaResultLocation.ts b/packages/aws-durable-execution-sdk-js-telemetry-core/src/athenaResultLocation.ts index 6038d83da..cea3fdebc 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-core/src/athenaResultLocation.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-core/src/athenaResultLocation.ts @@ -5,7 +5,8 @@ * `SELECT 1`. If the result location is inside the table's data location, the * table reads those files as records, so every query adds rows. Read-only * credentials do not prevent this, because Athena must be allowed to write its - * results. So Test connection checks the location before it runs its probe. + * results. So Test connection checks the location before it runs its probe, + * and the MCP server names it in the consent prompt before the first query. */ import type { TelemetryConfig } from "./configCore"; import type { resolveCredentials } from "./configCore"; @@ -31,7 +32,7 @@ function withTrailingSlash(uri: string): string { * submits, so the workgroup is read (with `athena:GetWorkGroup`) even when * `athenaOutputLocation` is set. Athena uses `primary` when no workgroup is * configured. Test connection calls this before its Athena probe, because the - * probe itself writes a result file. + * probe itself writes a result file. The MCP consent prompt calls it too. */ export type AthenaResultLocation = | { kind: "inside"; finding: string } diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/README.md b/packages/aws-durable-execution-sdk-js-telemetry-mcp/README.md index a82ebe5e4..b77a397fa 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/README.md +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/README.md @@ -139,8 +139,10 @@ On S3 + Athena the prompt also says when Athena writes query results inside the table's data location. The table then reads each result file as records, so every query adds rows, whatever the credentials. To find the result location, the server reads the Athena workgroup, which needs -`athena:GetWorkGroup`. `test_destination` skips its own test query in that -case and reports the problem. +`athena:GetWorkGroup`. If it cannot read the location, the prompt says nothing +about it. With `DURABLE_TELEMETRY_ACCEPT_WRITE_ACCESS=true` there is no prompt, +so the location is not checked either. In both cases, run `test_destination`: +it skips its own test query and reports the problem. A client that supports MCP form elicitation shows the prompt; confirming allows queries for the rest of the session. A client that does not support it gets a diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/queryChokePoint.test.ts b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/queryChokePoint.test.ts index 95f92cc57..23138c643 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/queryChokePoint.test.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/queryChokePoint.test.ts @@ -296,7 +296,7 @@ describe("query choke point", () => { // whole thing this file exists to prevent. "sendConverse", "listBedrockModels", - // Reads an Athena workgroup's result settings for the write check. + // Reads an Athena workgroup's result settings for the result-location check. "getWorkGroupOutput", "listenToQueue", "verifyResult", diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.ts b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.ts index 8a60b34f2..b7302d97a 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.ts @@ -256,7 +256,7 @@ export function registerTools( }; } - // 2. Stop until the user accepts credentials that can change data. + // 2. Stop until the user confirms the read-only responsibility. const blocked = await blockedByWriteAccess(extra.signal); if (blocked) return blocked; diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.test.ts b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.test.ts index 96bc15556..16a89367a 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.test.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.test.ts @@ -17,12 +17,13 @@ function gate(opts: { if (opts.answer instanceof Error) throw opts.answer; return opts.answer; }); + const configurationWarning = jest.fn(async () => opts.warning); const run = createWriteAccessGate({ ask, acceptedByEnv: opts.acceptedByEnv ?? false, - configurationWarning: async () => opts.warning, + configurationWarning, }); - return { run, ask }; + return { run, ask, configurationWarning }; } describe("createWriteAccessGate", () => { @@ -71,10 +72,11 @@ describe("createWriteAccessGate", () => { expect(ask).toHaveBeenCalledTimes(2); }); - it("shares one prompt between concurrent calls", async () => { - const { run, ask } = gate({ answer: "accept" }); + it("shares one prompt and one configuration check between concurrent calls", async () => { + const { run, ask, configurationWarning } = gate({ answer: "accept" }); await Promise.all([run(), run(), run()]); expect(ask).toHaveBeenCalledTimes(1); + expect(configurationWarning).toHaveBeenCalledTimes(1); }); it("adds a configuration warning to the prompt and to a blocked result", async () => { diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.ts b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.ts index 97278d98d..1970dc41f 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/writeAccessGate.ts @@ -74,7 +74,8 @@ export interface ElicitingServer { } /** - * Asks the user, through MCP form elicitation, to confirm `message`. Returns `undefined` when the client cannot show a form. + * Asks the user, through MCP form elicitation, to confirm `message`. Returns + * `undefined` when the client cannot show a form. * * The form has one checkbox that defaults to unchecked. A user who submits the * form without checking it has not accepted, so that counts as a decline. diff --git a/release-notes/telemetry-mcp-1.1.0.md b/release-notes/telemetry-mcp-1.1.0.md index 7a062960a..f5c21ddaa 100644 --- a/release-notes/telemetry-mcp-1.1.0.md +++ b/release-notes/telemetry-mcp-1.1.0.md @@ -9,7 +9,9 @@ written by the agent, and it is your responsibility to give the server read-only credentials. - On S3 + Athena the prompt also says when Athena writes query results inside - the table's data location, where every query adds rows. + the table's data location, where every query adds rows. When + `DURABLE_TELEMETRY_ACCEPT_WRITE_ACCESS=true` skips the prompt, run + `test_destination` to see this check. - The prompt's checkbox field is `confirmed` (was `acceptRisk`). ## Removed From d4ce9376bf5cacd3cd43dcb0ca46773d6293df5b Mon Sep 17 00:00:00 2001 From: Pooya Paridel Date: Wed, 28 Oct 2026 13:36:57 -0700 Subject: [PATCH 5/5] fix(telemetry): say when the Athena location check did not complete When checkAthenaResultLocation is undetermined (for example GetWorkGroup is denied), the MCP consent prompt now says the check did not complete and gives the reason, so the user knows before confirming. A test covers it. --- .../README.md | 4 +-- .../src/registerTools.test.ts | 27 ++++++++++++++----- .../src/registerTools.ts | 12 +++++++-- release-notes/telemetry-mcp-1.1.0.md | 3 ++- 4 files changed, 35 insertions(+), 11 deletions(-) diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/README.md b/packages/aws-durable-execution-sdk-js-telemetry-mcp/README.md index b77a397fa..db753c7d8 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/README.md +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/README.md @@ -139,8 +139,8 @@ On S3 + Athena the prompt also says when Athena writes query results inside the table's data location. The table then reads each result file as records, so every query adds rows, whatever the credentials. To find the result location, the server reads the Athena workgroup, which needs -`athena:GetWorkGroup`. If it cannot read the location, the prompt says nothing -about it. With `DURABLE_TELEMETRY_ACCEPT_WRITE_ACCESS=true` there is no prompt, +`athena:GetWorkGroup`. If it cannot read the location, the prompt says the +check did not complete, and why. With `DURABLE_TELEMETRY_ACCEPT_WRITE_ACCESS=true` there is no prompt, so the location is not checked either. In both cases, run `test_destination`: it skips its own test query and reports the problem. diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.test.ts b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.test.ts index c2d0d8235..23e025f4f 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.test.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.test.ts @@ -125,17 +125,32 @@ describe("the S3 result-location warning", () => { await client.close(); }); - it.each([ - { kind: "outside" }, - { kind: "undetermined", reason: "No location." }, - ])("adds nothing when the location is $kind", async (location) => { - checkAthenaResultLocation.mockResolvedValue(location); + it("says the check did not complete when the location is undetermined", async () => { + checkAthenaResultLocation.mockResolvedValue({ + kind: "undetermined", + reason: "AccessDenied: athena:GetWorkGroup", + }); + const client = await connect(false, S3_CONFIG); + const result = await client.callTool({ + name: "query", + arguments: { sql: "SELECT 1" }, + }); + expect(payloadOf(result).error).toContain( + "The server could not check whether Athena writes query results " + + "inside the table's data: AccessDenied: athena:GetWorkGroup", + ); + expect(runReadOnlyQuery).not.toHaveBeenCalled(); + await client.close(); + }); + + it("adds nothing when the location is outside the data", async () => { + checkAthenaResultLocation.mockResolvedValue({ kind: "outside" }); const client = await connect(false, S3_CONFIG); const result = await client.callTool({ name: "query", arguments: { sql: "SELECT 1" }, }); - expect(payloadOf(result).error).not.toMatch(/Athena|No location/); + expect(payloadOf(result).error).not.toMatch(/Athena/); expect(checkAthenaResultLocation).toHaveBeenCalledTimes(1); await client.close(); }); diff --git a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.ts b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.ts index b7302d97a..c0343c089 100644 --- a/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.ts +++ b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.ts @@ -72,14 +72,22 @@ export function registerTools( acceptedByEnv, ask: (message) => askUserToAccept(server.server, message), // On S3 + Athena, results written inside the table's data add rows on - // every query, whatever the credentials. So the prompt names that too. + // every query, whatever the credentials. So the prompt names that too, + // and says so when the location could not be determined. configurationWarning: async () => { if (config.destinationType !== "s3") return undefined; const location = await checkAthenaResultLocation( config, resolveCredentials(config.awsProfile), ); - return location.kind === "inside" ? location.finding : undefined; + if (location.kind === "inside") return location.finding; + if (location.kind === "undetermined") { + return ( + "The server could not check whether Athena writes query results " + + `inside the table's data: ${location.reason}` + ); + } + return undefined; }, }); // Returns a tool result when the call must not query, or null when it may. diff --git a/release-notes/telemetry-mcp-1.1.0.md b/release-notes/telemetry-mcp-1.1.0.md index f5c21ddaa..0b4bb17f0 100644 --- a/release-notes/telemetry-mcp-1.1.0.md +++ b/release-notes/telemetry-mcp-1.1.0.md @@ -9,7 +9,8 @@ written by the agent, and it is your responsibility to give the server read-only credentials. - On S3 + Athena the prompt also says when Athena writes query results inside - the table's data location, where every query adds rows. When + the table's data location, where every query adds rows, or that it could not + check the location, and why. When `DURABLE_TELEMETRY_ACCEPT_WRITE_ACCESS=true` skips the prompt, run `test_destination` to see this check. - The prompt's checkbox field is `confirmed` (was `acceptRisk`).