From bab90bed5fbc1a68df73822de1fb3a072c522eb2 Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Fri, 14 Aug 2026 15:05:02 +0200 Subject: [PATCH] fix(frontend): read connection strings the way libpq does Co-Authored-By: Claude Opus 5 (1M context) --- backend/ee-repo-ref.txt | 2 +- .../components/wizards/SetupChecklist.svelte | 50 +++++++++---------- .../AddDataTableWizard.svelte | 8 ++- .../utils/postgresConnectionString.test.ts | 26 ++++------ .../src/lib/utils/postgresConnectionString.ts | 27 +++++++--- 5 files changed, 61 insertions(+), 52 deletions(-) diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index ac0d037e67..a7c13dfdbb 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -71ef2cf2a5badd53d16d5cc945ab4ada1a639314 +81a16d2aaab969e3e22b128e736e833120f70105 diff --git a/frontend/src/lib/components/wizards/SetupChecklist.svelte b/frontend/src/lib/components/wizards/SetupChecklist.svelte index 5c176a00a6..8036d25856 100644 --- a/frontend/src/lib/components/wizards/SetupChecklist.svelte +++ b/frontend/src/lib/components/wizards/SetupChecklist.svelte @@ -37,12 +37,20 @@ let openedDescriptions: Record = $state({}) + function toggleDescription(i: number) { + if (openedDescriptions[i]) delete openedDescriptions[i] + else openedDescriptions[i] = true + } + $effect(() => { for (let i = 0; i < steps.length; i++) { if (steps[i].status === 'failed') openedDescriptions[i] = true } }) + const titleRowClass = (status: SetupStepStatus) => + twMerge('text-xs font-medium flex justify-between items-center', titleClass[status]) + const titleClass: Record = { pending: 'text-hint/75', running: 'text-primary', @@ -55,17 +63,7 @@
{#each steps as step, i} {@const descriptionOpened = openedDescriptions[i] ?? false} - - -
{ - if (!step.description) return - if (descriptionOpened) delete openedDescriptions[i] - else openedDescriptions[i] = true - }} - > +
{#if step.status === 'running'} @@ -79,14 +77,15 @@ {/if}
- - {step.title} - {#if step.description} + + {#if step.description} + + {:else} + {step.title} + {/if} {#if descriptionOpened} -
e.stopPropagation()} - > +
{step.description}
{/if} @@ -109,7 +107,7 @@
{#if step.substeps?.length} -
e.stopPropagation()}> +
{/if} diff --git a/frontend/src/lib/components/workspaceSettings/AddDataTableWizard.svelte b/frontend/src/lib/components/workspaceSettings/AddDataTableWizard.svelte index 2724d0f0d2..3aa0e127af 100644 --- a/frontend/src/lib/components/workspaceSettings/AddDataTableWizard.svelte +++ b/frontend/src/lib/components/workspaceSettings/AddDataTableWizard.svelte @@ -415,6 +415,9 @@ // Read off one attempt against one project; the review step would otherwise warn about // a limitation that no longer applies while claiming session pooling right above it. poolerUnavailable = undefined + // Same for the failure carried back to the review step: it names inputs that have since + // been edited, so it would describe a run nobody can still act on. + lastFailure = '' if (maxStep > wiz.step) maxStep = wiz.step } @@ -650,7 +653,10 @@ * at all while it is going, and once it has a result there is nothing left to lose. */ function hasUnfinishedIntent(): boolean { - return wiz.provider !== undefined && !run.running && !run.result + // A failed run counts: its inputs are still editable and it is the case that can have + // left something behind, so leaving then is the discard most worth confirming. Only a + // run that succeeded has nothing left to lose. + return wiz.provider !== undefined && !run.running && !run.result?.ok } /** Backdrop, Escape and the close button all arrive here. */ diff --git a/frontend/src/lib/utils/postgresConnectionString.test.ts b/frontend/src/lib/utils/postgresConnectionString.test.ts index 1fa3455a16..88f41792b7 100644 --- a/frontend/src/lib/utils/postgresConnectionString.test.ts +++ b/frontend/src/lib/utils/postgresConnectionString.test.ts @@ -36,24 +36,18 @@ describe('parsePostgresConnectionString', () => { expect(parsePostgresConnectionString('')).toBeUndefined() }) - // Only the last `@` can be the one before the host, and passwords hold `@` often enough - // that splitting at the first would silently keep half of one and connect nowhere. - it('splits credentials at the last @', () => { - expect( - parsePostgresConnectionString('postgres://u:p@ss@db.example.com:5432/mydb') - ).toMatchObject({ password: 'p@ss', host: 'db.example.com', port: 5432 }) - }) - - // A password is free to contain a `%`, which is not the start of an escape, so nothing - // here is unescaped -- a credential means the characters it is written with. - it('keeps percent escapes as typed', () => { - expect(parsePostgresConnectionString('postgres://u:p%40ss@host/db')?.password).toBe('p%40ss') + // Verified against psql: `postgres://role:p%40ss@host/db` authenticates as `p@ss`, and an + // unencoded `@` puts the rest of the password in libpq's host too. Reading these any other + // way would make the same string mean something here that it means nowhere else. + it('decodes percent escapes in credentials, as libpq does', () => { + expect(parsePostgresConnectionString('postgres://u:p%40ss@host/db')?.password).toBe('p@ss') + expect(parsePostgresConnectionString('postgres://u%40corp:p@host/db')?.user).toBe('u@corp') }) }) // The wizard offers the same connection as a string or as fields and switches between them -// by composing and reparsing, so a password holding a reserved character has to survive the -// trip: it would otherwise come back wrong rather than failing to parse. +// by composing and reparsing. A password holding a character the URI reserves is the case +// that breaks silently: it comes back wrong rather than failing to parse. describe('composePostgresConnectionString', () => { it('leaves an explicit prefer out of the string, since libpq assumes it', () => { const parts = { user: 'u', host: 'h', port: undefined, dbname: 'db', sslmode: 'prefer' } @@ -66,8 +60,8 @@ describe('composePostgresConnectionString', () => { it('round-trips through parse', () => { const parts = { - user: 'u', - password: 'p@ss/w:rd%', + user: 'u@corp', + password: 'p@ss/w:rd', host: 'db.example.com', port: 6543, dbname: 'mydb', diff --git a/frontend/src/lib/utils/postgresConnectionString.ts b/frontend/src/lib/utils/postgresConnectionString.ts index a53490d6be..558134979c 100644 --- a/frontend/src/lib/utils/postgresConnectionString.ts +++ b/frontend/src/lib/utils/postgresConnectionString.ts @@ -9,14 +9,14 @@ * user switch, so parse and compose have to be inverses: whatever one produces, * the other must read back unchanged. * - * Credentials are split at the *last* `@`, since that is the only one that can precede a - * host: a password holding an unescaped `@` is common enough that reading the first one - * would quietly put half the password in the host. Everything else is taken literally -- - * a percent escape stays the characters that were typed. + * libpq is the arbiter of what a connection string means, so this follows it rather than + * RFC 3986 where they differ: credentials are split at the *first* `@` -- an unencoded one + * lands in the host for libpq too -- and percent escapes in them are decoded, so `p%40ss` + * authenticates as `p@ss`. */ const CONNECTION_STRING = - /postgres(?:ql)?:\/\/(?[^:@]+)(?::(?.+))?@(?[^:\/?@]+)(?::(?\d+))?\/(?[^\?]+)?(?:\?.*sslmode=(?[^&]+))?/ + /postgres(?:ql)?:\/\/(?[^:@]+)(?::(?[^@]+))?@(?[^:\/?]+)(?::(?\d+))?\/(?[^\?]+)?(?:\?.*sslmode=(?[^&]+))?/ /** * A database someone types into Windmill is almost never localhost, so callers ask for TLS @@ -33,6 +33,15 @@ export type PostgresConnectionParts = { sslmode?: string } +/** A lone `%` is not an escape, and a password is free to contain one. */ +function decode(value: string): string { + try { + return decodeURIComponent(value) + } catch { + return value + } +} + /** Undefined when the string is not a postgres URI. */ export function parsePostgresConnectionString( connectionString: string @@ -41,8 +50,8 @@ export function parsePostgresConnectionString( if (!match?.groups) return undefined const { user, password, host, port, dbname, sslmode } = match.groups return { - user, - password: password || undefined, + user: decode(user), + password: password ? decode(password) : undefined, host, port: port ? Number(port) : undefined, dbname: dbname || undefined, @@ -55,7 +64,9 @@ export function parsePostgresConnectionString( * string stays the short one people recognize when nothing was overridden. */ export function composePostgresConnectionString(parts: PostgresConnectionParts): string { - const credentials = parts.password ? `${parts.user}:${parts.password}` : parts.user + const credentials = parts.password + ? `${encodeURIComponent(parts.user)}:${encodeURIComponent(parts.password)}` + : encodeURIComponent(parts.user) const port = parts.port ? `:${parts.port}` : '' const query = parts.sslmode && parts.sslmode !== 'prefer' ? `?sslmode=${parts.sslmode}` : '' return `postgres://${credentials}@${parts.host}${port}/${parts.dbname ?? ''}${query}`