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/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-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.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..cea3fdebc --- /dev/null +++ b/packages/aws-durable-execution-sdk-js-telemetry-core/src/athenaResultLocation.ts @@ -0,0 +1,102 @@ +/** + * 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, + * and the MCP server names it in the consent prompt before the first query. + */ +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. The MCP consent prompt calls it too. + */ +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..db753c7d8 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,28 @@ 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. + +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 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. + +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/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/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..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 @@ -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. */ @@ -302,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.test.ts b/packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.test.ts index edfe738e0..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 @@ -1,17 +1,18 @@ /** * 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(); +const checkAthenaResultLocation = 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), + checkAthenaResultLocation: (...args: unknown[]) => + checkAthenaResultLocation(...args), })); const runReadOnlyQuery = jest.fn(); jest.mock("./readOnlyQuery", () => ({ @@ -41,11 +42,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], @@ -53,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(); @@ -82,58 +88,97 @@ 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, +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("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/); + 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: [] }); 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. @@ -147,7 +192,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 9ec059898..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 @@ -6,7 +6,8 @@ */ import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { - checkWriteAccess, + checkAthenaResultLocation, + resolveCredentials, testDestination, type TelemetryConfig, } from "@aws/durable-execution-sdk-js-telemetry-core"; @@ -68,9 +69,26 @@ 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), + // On S3 + Athena, results written inside the table's data add rows on + // 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), + ); + 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. // The gate can wait a long time for the user's answer, and several calls can @@ -144,7 +162,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 @@ -247,7 +264,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 c31b7f874..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 @@ -1,112 +1,100 @@ -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; + warning?: string; }) { - const check = jest.fn(async () => opts.result); const ask = jest.fn(async () => { if (opts.answer instanceof Error) throw opts.answer; return opts.answer; }); + const configurationWarning = jest.fn(async () => opts.warning); const run = createWriteAccessGate({ - check, ask, acceptedByEnv: opts.acceptedByEnv ?? false, + configurationWarning, }); - return { run, check, ask }; + return { run, ask, configurationWarning }; } 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 and one configuration check between concurrent calls", async () => { + const { run, ask, configurationWarning } = gate({ answer: "accept" }); await Promise.all([run(), run(), run()]); - expect(check).toHaveBeenCalledTimes(1); expect(ask).toHaveBeenCalledTimes(1); + expect(configurationWarning).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("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(); }); }); @@ -148,7 +136,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( @@ -159,7 +147,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"); }); @@ -213,7 +201,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([ @@ -231,7 +219,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 94c777e33..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 @@ -1,32 +1,31 @@ /** - * 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 }; -/** 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()); } @@ -37,13 +36,22 @@ 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. */ + /** 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; } /** @@ -66,8 +74,8 @@ 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. @@ -82,24 +90,23 @@ export async function askUserToAccept( requestedSchema: { type: "object", properties: { - acceptRisk: { + confirmed: { type: "boolean", - title: "I accept that queries in this session can change data", + 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. */ export interface WriteAccessBlocked { error: string; - writeAccess: WriteAccessResult; howToAccept: string; } @@ -107,22 +114,22 @@ 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(); + const warning = await deps.configurationWarning(); + const message = warning ? `${CONSENT_MESSAGE} ${warning}` : CONSENT_MESSAGE; let answer: PromptAnswer; try { - answer = await deps.ask(promptMessage(result)); + 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. @@ -132,7 +139,7 @@ export function createWriteAccessGate( allowed = true; return null; } - return blocked(result, answer); + return blocked(message, answer); } return async () => { @@ -144,39 +151,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(message: string, 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. ${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..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 @@ -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,11 @@ export function SettingsModal({ the config is complete) without saving. For S3 + Athena it also verifies the Glue table and runs a test query. + + Queries are written by a model and run with these + credentials. 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 }); -} diff --git a/release-notes/telemetry-mcp-1.1.0.md b/release-notes/telemetry-mcp-1.1.0.md new file mode 100644 index 000000000..0b4bb17f0 --- /dev/null +++ b/release-notes/telemetry-mcp-1.1.0.md @@ -0,0 +1,31 @@ +# Telemetry MCP 1.1.0 + +## Changed + +- The server no longer checks whether its credentials can change data. No + check could show that a `SELECT` cannot write, so every result led to the + same prompt. Before the first `query`, `get_execution` or `list_executions` + call in a session, the server now asks you to confirm one line: queries are + 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, 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`). + +## Removed + +- **Output change for tools that read results programmatically.** + `test_destination` no longer returns a `writeAccess` field. A blocked + `query`, `get_execution` or `list_executions` result no longer carries + `writeAccess`, and its `error` and `howToAccept` text changed. Agents read + these fields as text, so most setups need no change. + +## Unchanged + +- `DURABLE_TELEMETRY_ACCEPT_WRITE_ACCESS=true` still confirms permanently. +- A client without MCP form elicitation still gets a tool error that names the + variable. +- A call cancelled while it waits for confirmation runs no query.