From 4c6528774a445603746538eaa70cebafc93bb941 Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Fri, 21 Aug 2026 19:47:50 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20address=20review=20round=202=20=E2=80=94?= =?UTF-8?q?=20XSS,=20retargeting,=20abandonment,=20redirects?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Hub data-table names were inlined as raw HTML.** `skip()` built the confirmation body as an HTML string, and `createAsyncConfirmationModal` renders `children` through `createRawSnippet`. A `datatable_name` comes straight from the hub export, and a hub is not necessarily ours — `hub_base_url` is an instance setting — so one carrying an event-bearing element ran script in this authenticated origin. Escaped. Same class as the round-1 SVG finding, in a different sink. **The setup step read unretargeted resource paths.** `installProject` rewrites every resource into `f//`, but the step re-fetched the raw export and used its paths verbatim. Importing into a folder other than the slug made `getResource` throw for every stub, the catch skipped them, and the step reported "You're all set" over credentials nobody had filled. It now retargets the same way the import did, and filters to the import folder — the containment guard the installer applies, so a crafted export cannot name a path outside it and get offered for editing. **Abandoning only stopped between phases.** `installProject` takes a `stopped` callback now, checked before every write loop, so leaving mid-run stops the remaining items instead of just the remaining phases. **A failed setup migration reported success.** `runMigrationsFor` swallowed the error, so the wizard marked its "Run migrations" step done and closed over a failure — leaving the data table name taken and no way back to retry. Rethrown, which is what the wizard's checklist reads. **`onboardingDestination` used a weaker redirect check.** `/\evil.com` passes `startsWith('/') && !startsWith('//')` but WHATWG URL parsing resolves it to another origin. Replaced with `toSameOriginRelativePath`, which already rejects that, control characters and oversized values. **Two workspace ids reached step 3 that the backend refuses:** a blank one (the Continue gate never required `id.trim()`) and `global`, which `check_w_id_conflict` rejects outright while `existsWorkspace` reports it free. Also: `size="xs2"` → `unifiedSize="2xs"`, and two doc comments reattached to the functions they describe. Co-Authored-By: Claude Opus 5 (1M context) --- .../lib/components/ImportProjectStep.svelte | 21 +++++++- .../src/lib/components/ImportSetupStep.svelte | 48 +++++++++++++------ .../src/lib/components/datatableSchemaSql.ts | 10 ++-- .../workspaceSettings/projectInstall.ts | 30 +++++++++++- .../src/lib/importWizard/execution.svelte.ts | 5 +- frontend/src/lib/utils/workspaceId.test.ts | 17 +++++++ frontend/src/lib/utils/workspaceId.ts | 8 ++++ .../projects/import/+page@(root).svelte | 11 ++++- .../user/(user)/onboarding/+page.svelte | 9 ++-- 9 files changed, 130 insertions(+), 29 deletions(-) diff --git a/frontend/src/lib/components/ImportProjectStep.svelte b/frontend/src/lib/components/ImportProjectStep.svelte index 2aef08b6e7..5d50ceab20 100644 --- a/frontend/src/lib/components/ImportProjectStep.svelte +++ b/frontend/src/lib/components/ImportProjectStep.svelte @@ -14,6 +14,7 @@ import { ImportExecution, plannedTasks } from '$lib/importWizard/execution.svelte' import SetupChecklist, { type SetupStep } from '$lib/components/wizards/SetupChecklist.svelte' import { beforeNavigate, goto } from '$app/navigation' + import { untrack } from 'svelte' import { FOLDER_NAME_RE, planProblem, type ImportPlan } from '$lib/importWizard/plan' import type { ImportProjectSummary } from '$lib/components/ImportProjectCard.svelte' import { ArrowLeft, Download, Loader2 } from 'lucide-svelte' @@ -32,6 +33,13 @@ /** Hands the run to the page, which needs the export's data tables to know * whether a setup step follows this one. */ onExecution?: (execution: ImportExecution | undefined) => void + /** + * The run this step already made, handed back when it is remounted. Step 4 unmounts + * this component, so returning from it would otherwise arrive at a fresh step with no + * run — offering Import again over a bundle that is already in, and on a new + * workspace failing at create because the finished run cleared its parking. + */ + resume?: ImportExecution | undefined onBack: () => void } @@ -42,7 +50,8 @@ onFinish, onBack, setupPending = false, - onExecution + onExecution, + resume }: Props = $props() let folder = $state(plan.folder ?? plan.slug) @@ -137,7 +146,15 @@ // clearing it from an effect, so a previous run's outcome can never be shown // against another plan. The folder is deliberately not part of the tag: it is // pushed onto the existing run instead (see `start`). - let run = $state<{ key: string; execution: ImportExecution } | undefined>(undefined) + // Seeded from the handed-back run, under the same tag a fresh one would carry, so the + // `planKey` guard below still rejects it if the destination changed in between. + // `untrack`, because this is a mount-time snapshot on purpose: a later plan change must + // invalidate the run through that tag, not silently re-seed it. + let run = $state<{ key: string; execution: ImportExecution } | undefined>( + untrack(() => + resume ? { key: JSON.stringify(plan.destination) + plan.slug, execution: resume } : undefined + ) + ) const planKey = $derived(JSON.stringify(plan.destination) + plan.slug) const execution = $derived(run?.key === planKey ? run.execution : undefined) $effect(() => onExecution?.(execution)) diff --git a/frontend/src/lib/components/ImportSetupStep.svelte b/frontend/src/lib/components/ImportSetupStep.svelte index de31d2d6cb..548fad5875 100644 --- a/frontend/src/lib/components/ImportSetupStep.svelte +++ b/frontend/src/lib/components/ImportSetupStep.svelte @@ -18,8 +18,13 @@ import { registryCcCapableFor } from '$lib/components/oauthRegistry' import { resourceTypeDisplayName } from '$lib/components/resourceTypeDisplay' import { applyOneMigration } from '$lib/components/workspaceSettings/projectInstall' - import type { ProjectMigration } from '$lib/components/workspaceSettings/projectBundle' + import { + retargetProjectExport, + type ProjectExport, + type ProjectMigration + } from '$lib/components/workspaceSettings/projectBundle' import { sendUserToast } from '$lib/toast' + import { escapeHtml } from '$lib/utils' // The last step, and the only optional one: it exists when the project's data // tables are not configured in the destination. The import has already run — @@ -33,12 +38,16 @@ interface Props { workspace: string slug: string + /** The folder the import wrote into. The export names resources under the project's + * own slug and `installProject` retargets them, so reading the raw paths here would + * look for stubs that are not where they landed. */ + folder?: string onSkip: () => void onFinish: () => void onBack?: () => void } - let { workspace, slug, onSkip, onFinish, onBack }: Props = $props() + let { workspace, slug, folder, onSkip, onFinish, onBack }: Props = $props() type Row = { name: string @@ -133,10 +142,7 @@ `/api/w/${encodeURIComponent(workspace)}/hub/projects/${encodeURIComponent(slug)}/export` ) if (!res.ok) throw new Error(`the hub proxy answered ${res.status}`) - const exportData = (await res.json()) as { - migrations?: ProjectMigration[] - resources?: { path: string; resource_type: string }[] - } + const exportData = (await res.json()) as ProjectExport const enabled = (exportData.migrations ?? []).filter( (m) => m.enabled && (m.sql ?? '').trim() !== '' ) @@ -159,7 +165,17 @@ justSaved: false } }) - projectResources = exportData.resources ?? [] + // Retargeted the same way the import was, so these are where the stubs actually + // landed. `retargetProjectExport` is a no-op when the folder is the slug, which is + // every new-workspace import. + const target = folder?.trim() || slug + const retargeted = retargetProjectExport(exportData, exportData.project?.slug ?? slug, target) + // Contained for the same reason the import contains: a crafted export can name a + // path outside the folder, and offering that for editing would reach a resource + // this import was never allowed to create. + projectResources = (retargeted.resources ?? []) + .map((r) => ({ path: String(r.path), resource_type: String((r as any).resource_type) })) + .filter((r) => r.path.startsWith(`f/${target}/`)) await refreshBlanks() } catch (e: any) { loadError = e?.body ?? e?.message ?? String(e) @@ -247,12 +263,6 @@ void load() }) - /** - * Configure every named data table in one write, then run each one's migrations. - * The config is read back and merged rather than replaced: `editDataTableConfig` - * takes the whole settings object, so sending only ours would delete any the - * workspace already has. - */ /** * The data table now exists — run the migrations that were skipped for it during the * import, which is the whole reason this step waits for the configuration. @@ -277,6 +287,11 @@ row.status = 'failed' row.error = e?.body ?? e?.message ?? String(e) sendUserToast(`Could not run the migrations for ${name}: ${row.error}`, true) + // Rethrown, because this also runs as the wizard's last checklist step + // (`onFinishAlso`). Swallowing it there makes the wizard report a clean finish + // over a failed migration, and close — leaving the data table name taken and no + // way back to retry it. + throw e } finally { working = false } @@ -291,7 +306,12 @@ */ async function skip(): Promise { if (pendingTables.length > 0) { - const names = pendingTables.map((r) => r.name).join(', ') + // Escaped: `confirmationModal.ask` renders `children` through `createRawSnippet`, + // so this string is HTML, and the name is a `datatable_name` straight out of the + // hub export. A hub is not ours — `hub_base_url` is an instance setting and the + // wizard can be pointed at any of them — so a name carrying an event-bearing + // element would otherwise run script in this authenticated origin. + const names = pendingTables.map((r) => escapeHtml(r.name)).join(', ') const one = pendingTables.length === 1 const confirmed = await confirmationModal.ask({ title: 'The project will not run', diff --git a/frontend/src/lib/components/datatableSchemaSql.ts b/frontend/src/lib/components/datatableSchemaSql.ts index bcf868c04f..44ca2414cc 100644 --- a/frontend/src/lib/components/datatableSchemaSql.ts +++ b/frontend/src/lib/components/datatableSchemaSql.ts @@ -146,11 +146,6 @@ function resolveColumnType(c: TableEditorValuesColumn): { return { datatype: c.datatype, defaultValue: c.defaultValue } } -/** - * SQL for an added table, with the CREATE TABLE and the FK constraints split so - * callers creating several tables can emit every CREATE before any constraint — - * required for circular FKs, where no creation order satisfies inline FKs. - */ /** * The schema API reports a foreign key's target as a bare table name whenever it * lives in the same schema as the table declaring it. Emitting that verbatim @@ -172,6 +167,11 @@ function qualifyFkTarget( return targetTable } +/** + * SQL for an added table, with the CREATE TABLE and the FK constraints split so + * callers creating several tables can emit every CREATE before any constraint — + * required for circular FKs, where no creation order satisfies inline FKs. + */ export function generateAddedTableSql( change: TableDiff, sourceSchema: DatabaseSchema, diff --git a/frontend/src/lib/components/workspaceSettings/projectInstall.ts b/frontend/src/lib/components/workspaceSettings/projectInstall.ts index 6cae3d5e0e..994985947d 100644 --- a/frontend/src/lib/components/workspaceSettings/projectInstall.ts +++ b/frontend/src/lib/components/workspaceSettings/projectInstall.ts @@ -266,11 +266,26 @@ export async function installProject(args: { /** Called once, before the reviewed migrations are applied, when there are any. Lets a * caller show them as their own step rather than folding them into the item import. */ onMigrationsStart?: () => void + /** + * Asked before each write. Returning true stops the run where it is — the writes already + * made stay, the rest never start. Nothing here can cancel a request already in flight, + * so this is the granularity available without threading an `AbortSignal` through every + * service call: the import wizard uses it when the user confirms leaving mid-run. + */ + stopped?: () => boolean hasEeLicense: boolean onResult: (r: InstallResult) => void }): Promise { - const { workspace, exportData, folder, migrations, hasEeLicense, onResult, onMigrationsStart } = - args + const { + workspace, + exportData, + folder, + migrations, + hasEeLicense, + onResult, + onMigrationsStart, + stopped + } = args const record = (path: string, p: Promise): Promise => p.then( @@ -278,6 +293,9 @@ export async function installProject(args: { (e: any) => onResult({ path, ok: false, error: errorMessage(e) }) ) + /** Every write goes through here, so one check covers items, variables and migrations. */ + const halted = () => stopped?.() === true + try { await FolderService.createFolder({ workspace, requestBody: { name: folder } }) } catch {} @@ -317,6 +335,7 @@ export async function installProject(args: { } for (const s of proj.scripts) { + if (halted()) return // `$var:` is resolved in job args (flow inputs, schedule args, trigger config), // not in script source, so there is no variable arg to contain here. await checkedItem(s.path, extractScriptRefs(s.content ?? ''), undefined, () => @@ -324,19 +343,23 @@ export async function installProject(args: { ) } for (const f of proj.flows) { + if (halted()) return await checkedItem(f.path, extractFlowRefs(f.value), f.value, () => importFlow(workspace, f)) } for (const r of proj.resources) { + if (halted()) return await checked(r.path, () => importResourceStub(workspace, r)) } // Placeholders for the project's internal `$var:`/`$jsonvar:` refs (retargeted // into this folder). External refs are rejected per-item, so only stub in-folder // ones; guard again in case an out-of-folder ref slipped through retargeting. for (const p of collectExportVarPaths(proj)) { + if (halted()) return if (!p.startsWith(prefix)) continue await record(`variable: ${p}`, importVariablePlaceholder(workspace, p)) } for (const a of proj.apps) { + if (halted()) return const isRaw = a.app_type === 'raw' const refs = isRaw ? extractRawAppRefs(a.value?.raw ?? '') : extractAppRefs(a.value) // Raw apps hold their runnables in the `value.raw` JSON string; parse it so the @@ -376,6 +399,7 @@ export async function installProject(args: { return varContainmentViolation(cfg, folder) } for (const t of proj.triggers) { + if (halted()) return const violation = guard(t.path, t.runnable_path) ?? triggerConfigViolation(t) await record( String(t.path), @@ -397,8 +421,10 @@ export async function installProject(args: { } // Apply the reviewed data table migrations after items exist. + if (halted()) return if (migrations.length) onMigrationsStart?.() for (const m of migrations) { + if (halted()) return await record( `data table: ${m.datatable_name}`, applyOneMigration(workspace, exportData.project.slug, m) diff --git a/frontend/src/lib/importWizard/execution.svelte.ts b/frontend/src/lib/importWizard/execution.svelte.ts index 5c0cc2aea1..ecede98611 100644 --- a/frontend/src/lib/importWizard/execution.svelte.ts +++ b/frontend/src/lib/importWizard/execution.svelte.ts @@ -368,7 +368,10 @@ export class ImportExecution { migrations, hasEeLicense: this.#deps.hasEeLicense, onResult: (r) => (this.results = [...this.results, r]), - onMigrationsStart: () => this.#set('migrate', 'running') + onMigrationsStart: () => this.#set('migrate', 'running'), + // Checked before every write, so leaving mid-run stops the remaining items + // rather than only the phases. What already landed stays and is listed. + stopped: () => this.#abandoned }) } catch (e: any) { this.#set('import', 'failed', String(e)) diff --git a/frontend/src/lib/utils/workspaceId.test.ts b/frontend/src/lib/utils/workspaceId.test.ts index 3ed85b6b6f..bcbeb1b780 100644 --- a/frontend/src/lib/utils/workspaceId.test.ts +++ b/frontend/src/lib/utils/workspaceId.test.ts @@ -69,3 +69,20 @@ describe('toWorkspaceId', () => { expect(validateWorkspaceId(id)).toBeUndefined() }) }) + +describe('validateWorkspaceId — the reserved id', () => { + // `check_w_id_conflict` refuses it, and `existsWorkspace` reports it free, so without + // this the wizard walks the user to the last step before the create fails. + it('refuses `global`', () => { + expect(validateWorkspaceId('global')).toMatch(/not allowed/i) + }) + + it('refuses it as the effective id too', () => { + expect(validateWorkspaceId('wm-fork-x', 'global')).toMatch(/not allowed/i) + }) + + it('still accepts ids that merely contain it', () => { + expect(validateWorkspaceId('global-ops')).toBeUndefined() + expect(validateWorkspaceId('my-global')).toBeUndefined() + }) +}) diff --git a/frontend/src/lib/utils/workspaceId.ts b/frontend/src/lib/utils/workspaceId.ts index 1d7b989e13..b1bcbd9e1b 100644 --- a/frontend/src/lib/utils/workspaceId.ts +++ b/frontend/src/lib/utils/workspaceId.ts @@ -13,6 +13,9 @@ export const WORKSPACE_ID_MAX_LENGTH = 50 /** `validate_workspace_name` (windmill-common/src/workspaces.rs:246) refuses a longer name. */ export const WORKSPACE_NAME_MAX_LENGTH = 50 +/** `check_w_id_conflict` (windmill-api-workspaces/src/workspaces.rs:5111) rejects this id. */ +const RESERVED_WORKSPACE_ID = 'global' + /** * The reason `id` is not a usable workspace id, or undefined when it is. * @@ -24,6 +27,11 @@ export function validateWorkspaceId(id: string, effectiveId: string = id): strin if (!WORKSPACE_ID_RE.test(id)) { return 'ID can only contain letters, numbers and dashes and must not finish by a dash' } + // `check_w_id_conflict` refuses it outright, and `existsWorkspace` reports it free — + // so without this the wizard walks the user to the last step before the create fails. + if (id === RESERVED_WORKSPACE_ID || effectiveId === RESERVED_WORKSPACE_ID) { + return `'${RESERVED_WORKSPACE_ID}' is not allowed as a workspace ID` + } if (effectiveId.length > WORKSPACE_ID_MAX_LENGTH) { return `ID '${effectiveId}' is too long (${effectiveId.length} chars). Maximum is ${WORKSPACE_ID_MAX_LENGTH}.` } diff --git a/frontend/src/routes/(root)/(logged)/projects/import/+page@(root).svelte b/frontend/src/routes/(root)/(logged)/projects/import/+page@(root).svelte index 06cfd3c31b..99c0ab420d 100644 --- a/frontend/src/routes/(root)/(logged)/projects/import/+page@(root).svelte +++ b/frontend/src/routes/(root)/(logged)/projects/import/+page@(root).svelte @@ -446,7 +446,7 @@ onClick={() => expandCollapseAll?.()} title={allExpanded ? 'Collapse all' : 'Expand all'} startIcon={{ icon: allExpanded ? ChevronsDownUp : ChevronsUpDown }} - size="xs2" + unifiedSize="2xs" variant="default" > {allExpanded ? 'Collapse' : 'Expand'} @@ -482,7 +482,12 @@