diff --git a/backend/src/auth/url-policy.ts b/backend/src/auth/url-policy.ts index 7143e996..a4ea0390 100644 --- a/backend/src/auth/url-policy.ts +++ b/backend/src/auth/url-policy.ts @@ -3,6 +3,18 @@ export interface ConfiguredTransportUrlOptions { originOnly?: boolean; } +export function parseCredentialFreeHttpUrl(value: string): URL | undefined { + let url: URL; + try { + url = new URL(value); + } catch { + return undefined; + } + if (!["http:", "https:"].includes(url.protocol) + || url.username || url.password || url.search || url.hash) return undefined; + return url; +} + function canonicalLoopbackAuthority(value: string): boolean { const match = /^http:\/\/([^/?#]+)(?:[/?#]|$)/.exec(value); if (!match) return false; @@ -27,15 +39,8 @@ export function parseConfiguredTransportUrl( value: string, options: ConfiguredTransportUrlOptions, ): URL | undefined { - let url: URL; - try { - url = new URL(value); - } catch { - return undefined; - } - if (url.username || url.password || url.search || url.hash || (options.originOnly && url.pathname !== "/")) { - return undefined; - } + const url = parseCredentialFreeHttpUrl(value); + if (!url || (options.originOnly && url.pathname !== "/")) return undefined; if (url.protocol === "https:") return url; if (options.allowLoopbackHttp && url.protocol === "http:" && canonicalLoopbackAuthority(value)) return url; return undefined; diff --git a/backend/src/catalog/service.ts b/backend/src/catalog/service.ts index 454d3a0f..723fadb7 100644 --- a/backend/src/catalog/service.ts +++ b/backend/src/catalog/service.ts @@ -159,7 +159,7 @@ export class CatalogService { for (const id of Object.values(CATALOG_SECRET_IDS)) this.secretStore.forget(workspaceId, id); } - async test(database: WorkspaceDatabase): Promise { + async test(database: WorkspaceDatabase): Promise { return await this.operations.run(database.id, async () => { const testedAt = new Date().toISOString(); const controller = new AbortController(); @@ -168,6 +168,7 @@ export class CatalogService { ? database.binding.restAuth === "none" ? [] : [CATALOG_SECRET_IDS.apiKey] : []; const materialized = this.secretStore.materialize(database.workspaceId, required); + let result: DatabaseTestResult; try { if (database.binding.transport !== "rest_api") { const client = await this.postgres.connect(database, controller.signal); @@ -205,13 +206,13 @@ export class CatalogService { }, }); } - return { + result = { connectionStatus: "reachable", testedVersion: database.version, lastTestedAt: testedAt, }; } catch { - return { + result = { connectionStatus: "failed", testedVersion: database.version, lastTestedAt: testedAt, @@ -223,6 +224,7 @@ export class CatalogService { controller.abort(); materialized.release(); } + return await this.repository.recordTest(database.id, database.version, result); }); } } diff --git a/backend/src/routes/catalog-databases.ts b/backend/src/routes/catalog-databases.ts index c0c8a443..b00002a6 100644 --- a/backend/src/routes/catalog-databases.ts +++ b/backend/src/routes/catalog-databases.ts @@ -1,6 +1,7 @@ import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; import { z } from "zod"; import { isPrincipalContext, requirePermission } from "../auth/authorization.js"; +import { parseCredentialFreeHttpUrl } from "../auth/url-policy.js"; import { CatalogService, type CatalogSecretName } from "../catalog/service.js"; import { WorkspaceRegistryError } from "../workspaces/git-repository.js"; import { @@ -26,7 +27,9 @@ const bindingSchema = z.object({ host: optionalText, port: port.optional(), username: optionalText, - baseUrl: z.url().max(2048).optional(), + baseUrl: z.string().max(2048) + .refine((value) => parseCredentialFreeHttpUrl(value) !== undefined) + .optional(), restPath: z.string().regex(/^\/(?!\/)[^?#\\\u0000-\u001f]*$/).max(512).optional(), restAuth: z.enum(["none", "bearer", "x-api-key"]).optional(), tlsServername: optionalText, @@ -209,7 +212,7 @@ export function catalogDatabaseRoutes( const database = await deps.repository.get(id); if (!database) return reply.code(404).send({ code: "database_not_found", message: "Database configuration was not found." }); if (database.version !== version) return reply.code(409).send({ code: "database_stale", message: "Database configuration changed. Reload and try again." }); - const tested = await deps.repository.recordTest(id, version, await deps.service.test(database)); + const tested = await deps.service.test(database); if (!tested) return reply.code(409).send({ code: "database_stale", message: "Database configuration changed. Reload and try again." }); return { ...tested, configured: true, secrets: deps.service.configuredSecrets(tested.workspaceId) }; } catch (error) { return safeError(reply, error); } diff --git a/backend/test/catalog-databases-routes.test.ts b/backend/test/catalog-databases-routes.test.ts index 051c511f..fca44702 100644 --- a/backend/test/catalog-databases-routes.test.ts +++ b/backend/test/catalog-databases-routes.test.ts @@ -5,6 +5,8 @@ import { afterEach, expect, test, vi } from "vitest"; import { buildApp } from "../src/app.js"; import { loadConfig } from "../src/config.js"; import { MemoryCatalogRepository } from "../src/catalog/memory-repository.js"; +import { CatalogOperationCoordinator } from "../src/catalog/operation-coordinator.js"; +import type { CatalogPostgresAccess } from "../src/catalog/postgres-access.js"; import type { ObservedSchemaSnapshot } from "../src/catalog/types.js"; import { WorkspaceSecretStore } from "../src/workspaces/secret-store.js"; import type { WorkspaceRegistry, WorkspaceRevision } from "../src/workspaces/registry.js"; @@ -28,7 +30,13 @@ const workspace: WorkspaceDescriptor = { }; const revision: WorkspaceRevision = { id: "psd-clinical", commit: "a".repeat(40), blob: "b".repeat(40), snapshotPath: "/tmp/psd.yaml" }; -function setup(environment: Record = {}) { +function setup( + environment: Record = {}, + catalogDependencies: { + catalogOperationCoordinator?: CatalogOperationCoordinator; + catalogPostgresAccess?: CatalogPostgresAccess; + } = {}, +) { const secretRoot = mkdtempSync(join(tmpdir(), "catalog-secret-")); const runtimeRoot = mkdtempSync(join(tmpdir(), "catalog-secret-runtime-")); roots.push(secretRoot, runtimeRoot); @@ -49,6 +57,7 @@ function setup(environment: Record = {}) { workspaceSecretStore: secretStore, catalogRepository: repository, workspaceDiagnoser: vi.fn(), + ...catalogDependencies, }); return { app, secretStore, repository }; } @@ -130,6 +139,29 @@ test("lists orphaned records and takes the REST diagnostic path from workspace Y ])); }); +test.each([ + "https://reader:secret@psd.example/api", + "https://psd.example/api?token=secret", + "https://psd.example/api#secret", + "ftp://psd.example/api", +])("rejects unsafe REST base URL %s before persistence", async (baseUrl) => { + const { app, repository } = setup(); + const response = await app.inject({ + method: "POST", + url: "/catalog/databases", + payload: { + ...direct, + binding: { transport: "rest_api", baseUrl, restPath: "/health", restAuth: "bearer" }, + }, + }); + expect(response.statusCode).toBe(400); + expect(response.json()).toEqual({ + code: "database_invalid", + message: "Database configuration is invalid.", + }); + expect(await repository.getByWorkspace("psd-clinical")).toBeUndefined(); +}); + test("uses optimistic versions, keeps secrets write-only, and hard-deletes only local configuration", async () => { const { app, secretStore } = setup(); const created = (await app.inject({ method: "POST", url: "/catalog/databases", payload: direct })).json(); @@ -151,6 +183,44 @@ test("uses optimistic versions, keeps secrets write-only, and hard-deletes only expect((await app.inject({ method: "GET", url: "/catalog/databases" })).json()).toMatchObject([{ configured: false }]); }); +test("rejects a connection test while another catalog operation owns the database", async () => { + const coordinator = new CatalogOperationCoordinator(); + const postgres: CatalogPostgresAccess = { + connect: vi.fn(async () => { throw new Error("connection must not start"); }), + }; + const { app } = setup({}, { + catalogOperationCoordinator: coordinator, + catalogPostgresAccess: postgres, + }); + const created = (await app.inject({ + method: "POST", + url: "/catalog/databases", + payload: direct, + })).json(); + const release = coordinator.reserve(created.id); + + try { + const response = await app.inject({ + method: "POST", + url: `/catalog/databases/${created.id}/test`, + payload: { version: created.version }, + }); + + expect(response.statusCode).toBe(409); + expect(response.json()).toEqual({ + code: "database_operation_in_progress", + message: "A database operation is already in progress.", + }); + expect(postgres.connect).not.toHaveBeenCalled(); + expect((await app.inject({ + method: "GET", + url: `/catalog/databases/${created.id}`, + })).json()).toMatchObject({ connectionStatus: "untested" }); + } finally { + release(); + } +}); + test("returns exact global and per-database fleet metrics", async () => { const { app, repository } = setup(); const database = await repository.create(direct); diff --git a/backend/test/catalog-service.test.ts b/backend/test/catalog-service.test.ts new file mode 100644 index 00000000..98bce384 --- /dev/null +++ b/backend/test/catalog-service.test.ts @@ -0,0 +1,111 @@ +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { expect, test } from "vitest"; +import { MemoryCatalogRepository } from "../src/catalog/memory-repository.js"; +import { CatalogOperationCoordinator } from "../src/catalog/operation-coordinator.js"; +import type { CatalogPostgresAccess } from "../src/catalog/postgres-access.js"; +import { CatalogService } from "../src/catalog/service.js"; +import type { DatabaseTestResult, WorkspaceDatabase } from "../src/catalog/types.js"; +import type { WorkspaceRegistry } from "../src/workspaces/registry.js"; +import { WorkspaceSecretStore } from "../src/workspaces/secret-store.js"; + +interface Deferred { + promise: Promise; + resolve: () => void; +} + +function deferred(): Deferred { + let resolve!: () => void; + const promise = new Promise((done) => { resolve = done; }); + return { promise, resolve }; +} + +class PausingRecordRepository extends MemoryCatalogRepository { + constructor( + private readonly recordStarted: Deferred, + private readonly continueRecord: Deferred, + ) { + super(); + } + + override async recordTest( + id: string, + expectedVersion: number, + result: DatabaseTestResult, + ): Promise { + this.recordStarted.resolve(); + await this.continueRecord.promise; + return await super.recordTest(id, expectedVersion, result); + } +} + +test("holds the database reservation until the connection result is recorded", async () => { + const secretRoot = mkdtempSync(join(tmpdir(), "catalog-service-secret-")); + const runtimeRoot = mkdtempSync(join(tmpdir(), "catalog-service-runtime-")); + const recordStarted = deferred(); + const continueRecord = deferred(); + const repository = new PausingRecordRepository(recordStarted, continueRecord); + const coordinator = new CatalogOperationCoordinator(); + const secretStore = new WorkspaceSecretStore({ + root: secretRoot, + runtimeRoot, + installationId: "test", + }); + const postgres: CatalogPostgresAccess = { + connect: async () => ({ + query: async () => ({ rows: [{ database: "warehouse", schema: "datawarehouse" }] }), + end: async () => undefined, + }), + }; + + try { + const database = await repository.create({ + workspaceId: "psd-clinical", + engine: "postgres", + databaseName: "warehouse", + schema: "datawarehouse", + binding: { + transport: "postgres_direct", + host: "db.internal", + port: 5432, + username: "reader", + }, + }); + const service = new CatalogService( + repository, + {} as WorkspaceRegistry, + secretStore, + [], + 1_000, + postgres, + coordinator, + ); + + const connectionTest = service.test(database); + const firstCompletedPhase = await Promise.race([ + recordStarted.promise.then(() => "recording" as const), + connectionTest.then(() => "returned" as const), + ]); + expect(firstCompletedPhase).toBe("recording"); + + try { + await expect(coordinator.run(database.id, async () => "overlapped")) + .rejects.toThrow("A database operation is already in progress"); + } finally { + continueRecord.resolve(); + } + + expect(await connectionTest).toMatchObject({ + id: database.id, + version: 1, + connectionStatus: "reachable", + testedVersion: 1, + }); + expect(await coordinator.run(database.id, async () => "released")).toBe("released"); + } finally { + continueRecord.resolve(); + rmSync(secretRoot, { recursive: true, force: true }); + rmSync(runtimeRoot, { recursive: true, force: true }); + } +}); diff --git a/frontend/src/shell/DatabaseManagementPage.test.tsx b/frontend/src/shell/DatabaseManagementPage.test.tsx index d960f415..952761ee 100644 --- a/frontend/src/shell/DatabaseManagementPage.test.tsx +++ b/frontend/src/shell/DatabaseManagementPage.test.tsx @@ -496,7 +496,7 @@ test.each(synchronizationScopes)( }, ); -test("starts Generate Missing for one configured database without confirmation", async () => { +test("discloses source sampling before starting Generate Missing", async () => { const user = userEvent.setup(); const queuedRun = makeDescriptionGenerationRun({ scope: "missing", total: 3 }); let startBody: unknown; @@ -519,6 +519,15 @@ test("starts Generate Missing for one configured database without confirmation", await user.click(screen.getByRole("button", { name: "Actions" })); await user.click(await screen.findByRole("menuitem", { name: "Generate Missing" })); + expect(screen.getByText("Generate missing descriptions for Policlinico San Donato?")).toBeVisible(); + expect(screen.getByRole("note", { + name: "Metadata generation source data disclosure", + })).toHaveTextContent( + "When available, up to five real source rows and five representative values from columns not marked sensitive are sent to the selected provider.", + ); + expect(startBody).toBeUndefined(); + + await user.click(screen.getByRole("button", { name: "Generate Missing" })); await waitFor(() => expect(startBody).toEqual({ modelId: "local-qwen", scope: "missing", @@ -527,6 +536,44 @@ test("starts Generate Missing for one configured database without confirmation", expect(await screen.findByRole("dialog", { name: "Description generation" })).toBeVisible(); }); +test("keeps Fleet Generate Missing behind the source-data disclosure", async () => { + const user = userEvent.setup(); + let generationStarts = 0; + server.use( + http.get("/api/catalog/metadata-generation/models", () => HttpResponse.json({ + models: [{ id: "local-qwen", label: "Local Qwen" }], + default: "local-qwen", + })), + http.post("/api/catalog/databases/:databaseId/description-generation-runs", () => { + generationStarts += 1; + return HttpResponse.json(makeDescriptionGenerationRun({ scope: "missing" }), { status: 202 }); + }), + ); + renderPage({ rows: [makeDatabase()], presentation: "fleet" }); + + const databaseRow = await screen.findByRole("row", { name: /Policlinico San Donato/ }); + await user.click(within(databaseRow).getByRole("checkbox", { name: /toggle row selection/i })); + await user.selectOptions( + screen.getByRole("combobox", { name: "Batch action" }), + "generate-missing", + ); + await waitFor(() => expect(screen.getByRole("combobox", { + name: "Metadata description model", + })).toHaveValue("local-qwen")); + await user.click(screen.getByRole("button", { name: "Run action" })); + + expect(screen.getByRole("note", { + name: "Metadata generation source data disclosure", + })).toBeVisible(); + expect(generationStarts).toBe(0); + + await user.click(screen.getByRole("button", { name: "Cancel" })); + expect(screen.queryByRole("note", { + name: "Metadata generation source data disclosure", + })).not.toBeInTheDocument(); + expect(generationStarts).toBe(0); +}); + test("confirms Generate All replacement, supports cancel, and sends the database-wide scope", async () => { const user = userEvent.setup(); const queuedRun = makeDescriptionGenerationRun({ scope: "all", total: 6 }); @@ -552,6 +599,9 @@ test("confirms Generate All replacement, supports cancel, and sends the database expect(screen.getByText("Replace generated descriptions for Policlinico San Donato?")).toBeVisible(); expect(screen.getByText(/existing generated descriptions for eligible tables and columns will be replaced/i)).toBeVisible(); + expect(screen.getByRole("note", { + name: "Metadata generation source data disclosure", + })).toHaveTextContent(/real source rows.*selected provider/i); expect(startBody).toBeUndefined(); await user.click(screen.getByRole("button", { name: "Cancel" })); @@ -587,6 +637,7 @@ test("keeps the database selected and safely explains when no descriptions are e await user.click(within(databaseRow).getByRole("checkbox", { name: /toggle row selection/i })); await user.click(screen.getByRole("button", { name: "Actions" })); await user.click(await screen.findByRole("menuitem", { name: "Generate Missing" })); + await user.click(screen.getByRole("button", { name: "Generate Missing" })); expect(await screen.findByText( "No eligible catalog tables or columns need description generation.", @@ -648,7 +699,9 @@ test.each([ await user.click(await screen.findByRole("menuitem", { name: scope === "all" ? "Generate All" : "Generate Missing", })); - if (scope === "all") await user.click(screen.getByRole("button", { name: "Generate All" })); + await user.click(screen.getByRole("button", { + name: scope === "all" ? "Generate All" : "Generate Missing", + })); expect(await screen.findByRole("heading", { name: status === "completed" ? "Completed" : "Completed with errors", @@ -752,6 +805,7 @@ test("disables database-wide generation while a description generation is active await user.click(within(databaseRow).getByRole("checkbox", { name: /toggle row selection/i })); await user.click(screen.getByRole("button", { name: "Actions" })); await user.click(await screen.findByRole("menuitem", { name: "Generate Missing" })); + await user.click(screen.getByRole("button", { name: "Generate Missing" })); expect(await screen.findByRole("dialog", { name: "Description generation" })).toBeVisible(); databaseRow = await screen.findByRole("row", { name: /Policlinico San Donato/ }); diff --git a/frontend/src/shell/database-management/DatabaseGrid.tsx b/frontend/src/shell/database-management/DatabaseGrid.tsx index 79d35ffd..128c9f96 100644 --- a/frontend/src/shell/database-management/DatabaseGrid.tsx +++ b/frontend/src/shell/database-management/DatabaseGrid.tsx @@ -214,7 +214,9 @@ export function DatabaseGrid({ const [selectedRows, setSelectedRows] = useState([]); const [action, setAction] = useState<"test" | "sync" | "generate" | "suggest" | "delete" | null>(null); const [pendingDelete, setPendingDelete] = useState(null); - const [pendingGenerateAll, setPendingGenerateAll] = useState(false); + const [pendingGenerationScope, setPendingGenerationScope] = useState< + Extract | null + >(null); const context = useMemo( () => ({ canManage, presentation, onView, onOpenTables, onEdit, onDelete, onOpenSync }), [canManage, presentation, onView, onOpenTables, onEdit, onDelete, onOpenSync], @@ -376,10 +378,10 @@ export function DatabaseGrid({ return () => window.clearTimeout(timer); }, [pendingDelete]); useEffect(() => { - if (!pendingGenerateAll) return; - const timer = window.setTimeout(() => document.getElementById("database-generate-all-confirm-button")?.focus(), 0); + if (!pendingGenerationScope) return; + const timer = window.setTimeout(() => document.getElementById("database-generation-confirm-button")?.focus(), 0); return () => window.clearTimeout(timer); - }, [pendingGenerateAll]); + }, [pendingGenerationScope]); const closeDeleteConfirmation = () => { setPendingDelete(null); window.setTimeout(() => { @@ -387,8 +389,8 @@ export function DatabaseGrid({ else actionsTriggerRef.current?.focus(); }, 0); }; - const closeGenerateAllConfirmation = () => { - setPendingGenerateAll(false); + const closeGenerationConfirmation = () => { + setPendingGenerationScope(null); window.setTimeout(() => { if (presentation === "fleet") actionSelectRef.current?.focus(); else actionsTriggerRef.current?.focus(); @@ -407,16 +409,17 @@ export function DatabaseGrid({ setSelectedRows([]); } setPendingDelete(null); - setPendingGenerateAll(false); + setPendingGenerationScope(null); if (kind === "delete") window.setTimeout(() => searchInputRef.current?.focus(), 0); } catch { // The page-level operation owns safe error feedback. Preserve the selection for retry. + if (kind === "generate") setPendingGenerationScope(null); } finally { setAction(null); } }; const runFleetAction = async (selectedAction: DatabaseFleetAction) => { - if (selectedAction === "generate-all") { - setPendingGenerateAll(true); + if (selectedAction === "generate-all" || selectedAction === "generate-missing") { + setPendingGenerationScope(selectedAction === "generate-all" ? "all" : "missing"); return; } if (selectedAction === "clear-tables" || selectedAction === "clear-relationships") { @@ -432,10 +435,6 @@ export function DatabaseGrid({ await perform("sync", () => onSyncSelected(selectedRows, scope)); return; } - if (selectedAction === "generate-missing") { - await perform("generate", () => onGenerateDescriptions(selectedRows, "missing")); - return; - } await perform("suggest", () => onSuggestSensitive(selectedRows), false); }; @@ -452,35 +451,47 @@ export function DatabaseGrid({ : "flex min-h-12 flex-wrap items-center gap-3 border-b border-border px-3 py-2"} > {selectedRows.length > 0 ? ( - pendingGenerateAll ? ( + pendingGenerationScope ? (
{ - if (event.key === "Escape" && action === null) closeGenerateAllConfirmation(); + if (event.key === "Escape" && action === null) closeGenerationConfirmation(); }} >
-

- Replace generated descriptions for {selectedRows[0]?.workspaceName}? +

+ {pendingGenerationScope === "all" + ? `Replace generated descriptions for ${selectedRows[0]?.workspaceName}?` + : `Generate missing descriptions for ${selectedRows[0]?.workspaceName}?`}

-

- Existing generated descriptions for eligible tables and columns will be replaced. + {pendingGenerationScope === "all" ? ( +

+ Existing generated descriptions for eligible tables and columns will be replaced. +

+ ) : null} +

+ When available, up to five real source rows and five representative values from + columns not marked sensitive are sent to the selected provider.

- +
) : pendingDelete ? ( @@ -564,17 +575,14 @@ export function DatabaseGrid({ setPendingGenerateAll(true)} + onClick={() => setPendingGenerationScope("all")} > Generate All void perform( - "generate", - () => onGenerateDescriptions(selectedRows, "missing"), - )} + onClick={() => setPendingGenerationScope("missing")} > Generate Missing @@ -649,7 +657,7 @@ export function DatabaseGrid({ selectionColumnDef={{ width: 44, maxWidth: 44, pinned: "left" }} onSelectionChanged={({ api }) => { if (pendingDelete && action === null) setPendingDelete(null); - if (pendingGenerateAll && action === null) setPendingGenerateAll(false); + if (pendingGenerationScope && action === null) setPendingGenerationScope(null); setSelectedRows(api.getSelectedRows()); }} rowHeight={44}