diff --git a/docs/application-profiles.md b/docs/application-profiles.md index 931f526..f54277f 100644 --- a/docs/application-profiles.md +++ b/docs/application-profiles.md @@ -133,12 +133,28 @@ Profile `region` must be within `AWS_ALLOWED_REGIONS`. Per-resource `auth` overr | `AWS_MCP_APP_CONFIG` missing | `disabled`, empty list | Validation error: not configured | | KV read failure | `unavailable`, empty list | Validation error: unavailable | | Index missing | `available`, empty list | Validation error if profile requested | -| Index invalid | `available`, empty list (logged) | — | +| Index valid and empty | `available`, empty list | Validation error if profile requested | +| Index invalid (malformed JSON or schema) | `invalid`, empty list, safe `error` message | Validation error: index invalid | | Profile missing | — | Validation error: not found | | Profile invalid | — | Validation error (fail closed) | Invalid profiles do not break generic MCP tools. +## Diagnosing an invalid profile index + +When `list_application_profiles` returns `storeStatus: "invalid"`: + +1. The KV binding exists but `app-profiles/index.json` (or your custom `AWS_MCP_APP_PROFILE_INDEX_KEY`) contains malformed JSON or fails schema validation. +2. Re-validate and republish the index using the profile CLI: + +```bash +pnpm run app-profile:validate -- --file examples/app-profiles/example-prod.profile.json +pnpm run app-profile:put -- --file examples/app-profiles/example-prod.profile.json --remote +``` + +3. Confirm `list_application_profiles` returns `storeStatus: "available"` with the expected profile entries. +4. Never store secrets in profile KV — only resource names, prefixes, and role ARNs. + ## Secret boundaries Profiles **may** contain: diff --git a/docs/mcp-tools.md b/docs/mcp-tools.md index 8f28614..a2ee50a 100644 --- a/docs/mcp-tools.md +++ b/docs/mcp-tools.md @@ -2055,7 +2055,7 @@ These tools require the `application-ops` pack and optional KV-backed profiles ( ### 28. `list_application_profiles` -Returns safe profile metadata only: `id`, `displayName`, `environment`, `region`, `enabled`, `aliases`, `capabilities`, and `profileConfigAvailable`. Does not call AWS. Missing KV binding returns `storeStatus: "disabled"` with an empty list. +Returns safe profile metadata only: `id`, `displayName`, `environment`, `region`, `enabled`, `aliases`, `capabilities`, and `profileConfigAvailable`. Does not call AWS. Missing KV binding returns `storeStatus: "disabled"` with an empty list. A malformed or schema-invalid index returns `storeStatus: "invalid"` with an empty list and optional `error` message (no raw KV content). ### 29. `get_application_environment_overview` diff --git a/src/mcp/tools/definitions/list-application-profiles.test.ts b/src/mcp/tools/definitions/list-application-profiles.test.ts index 875cbe8..8c325f0 100644 --- a/src/mcp/tools/definitions/list-application-profiles.test.ts +++ b/src/mcp/tools/definitions/list-application-profiles.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it, vi } from "vitest"; import type { KVNamespace } from "@cloudflare/workers-types"; import { createTestGatewayContext } from "../../../test/gateway-context-fixture.js"; +import { INVALID_PROFILE_INDEX_ERROR } from "../../../profiles/loader.js"; import { createListApplicationProfilesToolManifest } from "./list-application-profiles.js"; const validIndex = { @@ -34,11 +35,15 @@ const validProfile = { function createMockKv(store: Record): KVNamespace { return { - get: vi.fn(async (key: string) => { + get: vi.fn(async (key: string, type?: "text" | "json") => { if (!(key in store)) { return null; } - return store[key]; + const value = store[key]; + if (type === "text") { + return typeof value === "string" ? value : JSON.stringify(value); + } + return value; }), put: vi.fn(), delete: vi.fn(), @@ -72,6 +77,23 @@ describe("list_application_profiles tool", () => { }); }); + it("returns invalid store state for malformed index", async () => { + const ctx = createTestGatewayContext({ + appConfig: createMockKv({ + "app-profiles/index.json": { version: 2, profiles: [] }, + }), + }); + const manifest = createListApplicationProfilesToolManifest(ctx); + const result = await manifest.handler({}); + + expect(result.structuredContent).toEqual({ + storeStatus: "invalid", + profiles: [], + error: INVALID_PROFILE_INDEX_ERROR, + }); + expect(JSON.stringify(result.structuredContent)).not.toContain("version"); + }); + it("returns profile metadata with profileConfigAvailable", async () => { const ctx = createTestGatewayContext({ appConfig: createMockKv({ diff --git a/src/mcp/tools/definitions/list-application-profiles.ts b/src/mcp/tools/definitions/list-application-profiles.ts index 118afcf..2db4fd2 100644 --- a/src/mcp/tools/definitions/list-application-profiles.ts +++ b/src/mcp/tools/definitions/list-application-profiles.ts @@ -60,15 +60,24 @@ export function createListApplicationProfilesToolManifest( })), ); - const structuredContent = { + const structuredContent: { + storeStatus: typeof listResult.status; + profiles: typeof profiles; + error?: string; + } = { storeStatus: listResult.status, profiles, }; + if (listResult.status === "invalid" && listResult.error) { + structuredContent.error = listResult.error; + } const text = listResult.status === "disabled" ? "Application profiles are not configured." - : `Found ${profiles.length} application profile(s) (store: ${listResult.status}).`; + : listResult.status === "invalid" + ? listResult.error ?? "Application profile index is invalid." + : `Found ${profiles.length} application profile(s) (store: ${listResult.status}).`; return { content: [{ type: "text" as const, text }], diff --git a/src/mcp/tools/descriptor.ts b/src/mcp/tools/descriptor.ts index 8f5745f..9ec8e85 100644 --- a/src/mcp/tools/descriptor.ts +++ b/src/mcp/tools/descriptor.ts @@ -701,8 +701,9 @@ const applicationProfileListEntrySchema = z.object({ }); export const listApplicationProfilesOutputSchema = withOptionalExecutionMetadata({ - storeStatus: z.enum(["disabled", "available", "unavailable"]), + storeStatus: z.enum(["disabled", "available", "unavailable", "invalid"]), profiles: z.array(applicationProfileListEntrySchema), + error: z.string().optional(), }); const applicationSectionSchema = z.object({ diff --git a/src/mcp/tools/list-ec2-instances.test.ts b/src/mcp/tools/list-ec2-instances.test.ts index f0d2d47..484704b 100644 --- a/src/mcp/tools/list-ec2-instances.test.ts +++ b/src/mcp/tools/list-ec2-instances.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; +import type { KVNamespace } from "@cloudflare/workers-types"; import { createTestGatewayContext } from "../../test/gateway-context-fixture.js"; import type { GatewayContext } from "../../config/context.js"; import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; @@ -126,6 +127,45 @@ describe("registerListEc2InstancesTool", () => { }); }); + it("succeeds when application profile index is invalid", async () => { + mockFetch.mockImplementation(() => + Promise.resolve( + ec2XmlResponse( + describeInstancesXml([ + instanceXml({ instanceId: "i-11111111" }), + ]), + ), + ), + ); + + const invalidIndexKv: KVNamespace = { + get: vi.fn(async (key: string, type?: "text" | "json") => { + if (key === "app-profiles/index.json" && type === "text") { + return "{not-valid-json"; + } + return null; + }), + put: vi.fn(), + delete: vi.fn(), + list: vi.fn(), + getWithMetadata: vi.fn(), + } as unknown as KVNamespace; + + const ctx = createTestGatewayContext({ + allowedRegions: ["us-east-1"], + appConfig: invalidIndexKv, + }); + const mock = makeMockServer(); + registerMcpToolForTest(mock.server, ctx, "list_ec2_instances"); + const tool = mock.getTool("list_ec2_instances")!; + const result = await tool.handler({}) as Record; + + expect(result).toHaveProperty("structuredContent"); + expect(result).not.toHaveProperty("isError", true); + const structured = result.structuredContent as { count: number }; + expect(structured.count).toBeGreaterThan(0); + }); + it("returns instances from multiple regions", async () => { mockFetch.mockImplementation((url: string) => { if (url.includes("us-east-1")) { diff --git a/src/profiles/access.test.ts b/src/profiles/access.test.ts index e65469a..167cc30 100644 --- a/src/profiles/access.test.ts +++ b/src/profiles/access.test.ts @@ -49,11 +49,15 @@ const validProfile = { function createMockKv(store: Record): KVNamespace { return { - get: vi.fn(async (key: string) => { + get: vi.fn(async (key: string, type?: "text" | "json") => { if (!(key in store)) { return null; } - return store[key]; + const value = store[key]; + if (type === "text") { + return typeof value === "string" ? value : JSON.stringify(value); + } + return value; }), put: vi.fn(), delete: vi.fn(), @@ -178,6 +182,18 @@ describe("resolveApplicationProfileForTool", () => { ); }); + it("rejects when profile index is invalid", async () => { + const ctx = createTestGatewayContext({ + appConfig: createMockKv({ + "app-profiles/index.json": { version: 2, profiles: [] }, + "app-profiles/profiles/example-prod.json": validProfile, + }), + }); + await expect(resolveApplicationProfileForTool(ctx, "example-prod")).rejects.toThrow( + /index is invalid/i, + ); + }); + it("rejects missing profile ids", async () => { const ctx = createTestGatewayContext({ appConfig: createMockKv({ diff --git a/src/profiles/access.ts b/src/profiles/access.ts index 1f256c0..fa14c66 100644 --- a/src/profiles/access.ts +++ b/src/profiles/access.ts @@ -3,7 +3,11 @@ import { isValidRoleArn } from "../aws/credentials/helpers.js"; import type { AwsCredentials } from "../aws/types.js"; import { ValidationError } from "../security/errors.js"; import { buildProfileKey } from "./keys.js"; -import { listApplicationProfiles, loadApplicationProfile } from "./loader.js"; +import { + INVALID_PROFILE_INDEX_ERROR, + listApplicationProfiles, + loadApplicationProfile, +} from "./loader.js"; import type { ProfileAuthConfig, ValidatedAppProfile } from "./types.js"; import { resolveProfileAuth, validateProfileId } from "./validation.js"; @@ -74,6 +78,9 @@ export async function resolveApplicationProfileForTool( "Application profiles are temporarily unavailable.", ); } + if (listResult.status === "invalid") { + throw new ValidationError("validation_error", INVALID_PROFILE_INDEX_ERROR); + } const safeProfileId = validateProfileId(profileId); const entry = listResult.profiles.find((profile) => profile.id === safeProfileId); diff --git a/src/profiles/loader.test.ts b/src/profiles/loader.test.ts index 4dd41d2..625d7f1 100644 --- a/src/profiles/loader.test.ts +++ b/src/profiles/loader.test.ts @@ -2,7 +2,11 @@ import { describe, expect, it, vi } from "vitest"; import type { KVNamespace } from "@cloudflare/workers-types"; import { ValidationError } from "../security/errors.js"; import { createTestGatewayContext } from "../test/gateway-context-fixture.js"; -import { listApplicationProfiles, loadApplicationProfile } from "./loader.js"; +import { + INVALID_PROFILE_INDEX_ERROR, + listApplicationProfiles, + loadApplicationProfile, +} from "./loader.js"; const validIndex = { version: 1, @@ -33,13 +37,23 @@ const validProfile = { }, }; +const invalidIndexResult = { + status: "invalid" as const, + profiles: [], + error: INVALID_PROFILE_INDEX_ERROR, +}; + function createMockKv(store: Record): KVNamespace { return { - get: vi.fn(async (key: string) => { + get: vi.fn(async (key: string, type?: "text" | "json") => { if (!(key in store)) { return null; } - return store[key]; + const value = store[key]; + if (type === "text") { + return typeof value === "string" ? value : JSON.stringify(value); + } + return value; }), put: vi.fn(), delete: vi.fn(), @@ -60,6 +74,13 @@ function createFailingKv(): KVNamespace { } as unknown as KVNamespace; } +function expectSafeInvalidResult(result: Awaited>) { + expect(result).toEqual(invalidIndexResult); + expect(JSON.stringify(result)).not.toContain("AKIA"); + expect(JSON.stringify(result)).not.toContain("password"); + expect(JSON.stringify(result)).not.toContain("secret"); +} + describe("listApplicationProfiles", () => { it("returns disabled state when appConfig binding is missing", async () => { const ctx = createTestGatewayContext(); @@ -85,14 +106,69 @@ describe("listApplicationProfiles", () => { expect(result).toEqual({ status: "available", profiles: [] }); }); - it("returns empty list for invalid index schema", async () => { + it("returns invalid state for invalid index schema", async () => { const ctx = createTestGatewayContext({ appConfig: createMockKv({ "app-profiles/index.json": { version: 2, profiles: [] }, }), }); const result = await listApplicationProfiles(ctx); - expect(result).toEqual({ status: "available", profiles: [] }); + expectSafeInvalidResult(result); + }); + + it("returns invalid state for malformed JSON index", async () => { + const ctx = createTestGatewayContext({ + appConfig: createMockKv({ + "app-profiles/index.json": "{not-valid-json", + }), + }); + const result = await listApplicationProfiles(ctx); + expectSafeInvalidResult(result); + }); + + it("returns invalid state for duplicate profile ids", async () => { + const ctx = createTestGatewayContext({ + appConfig: createMockKv({ + "app-profiles/index.json": { + version: 1, + profiles: [validIndex.profiles[0], validIndex.profiles[0]], + }, + }), + }); + const result = await listApplicationProfiles(ctx); + expectSafeInvalidResult(result); + }); + + it("returns invalid state for disallowed region in index", async () => { + const ctx = createTestGatewayContext({ + appConfig: createMockKv({ + "app-profiles/index.json": { + version: 1, + profiles: [{ ...validIndex.profiles[0], region: "eu-west-1" }], + }, + }), + }); + const result = await listApplicationProfiles(ctx); + expectSafeInvalidResult(result); + }); + + it("does not leak secret-like index content in invalid result", async () => { + const ctx = createTestGatewayContext({ + appConfig: createMockKv({ + "app-profiles/index.json": { + version: 1, + profiles: [ + { + ...validIndex.profiles[0], + displayName: "password=supersecret", + }, + ], + }, + }), + }); + const result = await listApplicationProfiles(ctx); + expectSafeInvalidResult(result); + expect(JSON.stringify(result)).not.toContain("supersecret"); }); it("returns unavailable state when KV read fails", async () => { @@ -145,6 +221,18 @@ describe("loadApplicationProfile", () => { ); }); + it("fails closed when index is invalid", async () => { + const ctx = createTestGatewayContext({ + appConfig: createMockKv({ + "app-profiles/index.json": { version: 2, profiles: [] }, + "app-profiles/profiles/example-prod.json": validProfile, + }), + }); + await expect(loadApplicationProfile(ctx, "example-prod")).rejects.toThrow( + /index is invalid/i, + ); + }); + it("returns validation error when profile is missing", async () => { const ctx = createTestGatewayContext({ appConfig: createMockKv({ diff --git a/src/profiles/loader.ts b/src/profiles/loader.ts index 427cd21..0f7b111 100644 --- a/src/profiles/loader.ts +++ b/src/profiles/loader.ts @@ -14,11 +14,19 @@ import { validateProfileIndexDocument, } from "./validation.js"; +export const INVALID_PROFILE_INDEX_ERROR = "Application profile index is invalid."; + type KvReadResult = { value: unknown; status: ProfileStoreStatus; }; +type KvIndexReadResult = { + value: unknown; + status: ProfileStoreStatus; + error?: string; +}; + async function readKvJson( kv: KVNamespace | undefined, key: string, @@ -39,6 +47,39 @@ async function readKvJson( } } +async function readKvIndex( + kv: KVNamespace | undefined, + key: string, +): Promise { + if (!kv) { + return { value: null, status: "disabled" }; + } + + try { + const raw = await kv.get(key, "text"); + if (raw === null) { + return { value: null, status: "available" }; + } + + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + logWarn({ phase: "app_profile_index_invalid" }); + return { + value: null, + status: "invalid", + error: INVALID_PROFILE_INDEX_ERROR, + }; + } + + return { value: parsed, status: "available" }; + } catch { + logWarn({ phase: "app_profile_kv_read_failed", operation: "get" }); + return { value: null, status: "unavailable" }; + } +} + function assertProfilesConfigured(status: ProfileStoreStatus): void { if (status === "disabled") { throw new ValidationError( @@ -52,13 +93,24 @@ function assertProfilesConfigured(status: ProfileStoreStatus): void { "Application profiles are temporarily unavailable.", ); } + if (status === "invalid") { + throw new ValidationError("validation_error", INVALID_PROFILE_INDEX_ERROR); + } +} + +function invalidIndexResult(): ListApplicationProfilesResult { + return { + status: "invalid", + profiles: [], + error: INVALID_PROFILE_INDEX_ERROR, + }; } export async function listApplicationProfiles( ctx: GatewayContext, ): Promise { const indexKey = resolveIndexKey(ctx.appProfileIndexKey); - const result = await readKvJson(ctx.appConfig, indexKey); + const result = await readKvIndex(ctx.appConfig, indexKey); if (result.status === "disabled") { return { status: "disabled", profiles: [] }; @@ -66,6 +118,9 @@ export async function listApplicationProfiles( if (result.status === "unavailable") { return { status: "unavailable", profiles: [] }; } + if (result.status === "invalid") { + return invalidIndexResult(); + } if (result.value === null) { return { status: "available", profiles: [] }; } @@ -73,9 +128,9 @@ export async function listApplicationProfiles( try { const index = validateProfileIndexDocument(result.value, ctx.allowedRegions); return { status: "available", profiles: index.profiles }; - } catch (error) { + } catch { logWarn({ phase: "app_profile_index_invalid" }); - return { status: "available", profiles: [] }; + return invalidIndexResult(); } } @@ -85,9 +140,17 @@ export async function loadApplicationProfile( ): Promise { const safeProfileId = validateProfileId(profileId); const indexKey = resolveIndexKey(ctx.appProfileIndexKey); - const indexResult = await readKvJson(ctx.appConfig, indexKey); + const indexResult = await readKvIndex(ctx.appConfig, indexKey); assertProfilesConfigured(indexResult.status); + if (indexResult.value !== null) { + try { + validateProfileIndexDocument(indexResult.value, ctx.allowedRegions); + } catch { + throw new ValidationError("validation_error", INVALID_PROFILE_INDEX_ERROR); + } + } + const profileKey = buildProfileKey(safeProfileId); const profileResult = await readKvJson(ctx.appConfig, profileKey); assertProfilesConfigured(profileResult.status); diff --git a/src/profiles/types.ts b/src/profiles/types.ts index 3b58587..4c19a44 100644 --- a/src/profiles/types.ts +++ b/src/profiles/types.ts @@ -83,9 +83,10 @@ export type ValidatedAppProfile = { resources: ProfileResources; }; -export type ProfileStoreStatus = "disabled" | "available" | "unavailable"; +export type ProfileStoreStatus = "disabled" | "available" | "unavailable" | "invalid"; export type ListApplicationProfilesResult = { status: ProfileStoreStatus; profiles: SafeProfileIndexEntry[]; + error?: string; };