fix: address review round 2 — XSS, retargeting, abandonment, redirects

**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/<folder>/`, 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) <noreply@anthropic.com>

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))

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<void> {
 		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',

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,

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<void> {
-	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<unknown>): Promise<void> =>
 		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)

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))

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()
+	})
+})

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}.`
 	}

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 @@
 						<Button
 							unifiedSize="sm"
 							variant="accent"
-							disabled={!name.trim() || !!idProblem || !!usernameProblem || idTaken || checkingId}
+							disabled={!name.trim() ||
+								!id.trim() ||
+								!!idProblem ||
+								!!usernameProblem ||
+								idTaken ||
+								checkingId}
 							onClick={confirmNewWorkspace}
 						>
 							Continue →
@@ -499,11 +504,13 @@
 				onFinish={() => (setupNeeded ? go({}, 4) : finish())}
 				onBack={() => go({}, 2)}
 				onExecution={(e) => (execution = e)}
+				resume={execution}
 			/>
 		{:else}
 			<ImportSetupStep
 				workspace={planWorkspaceId(plan) ?? ''}
 				{slug}
+				folder={plan.folder}
 				onSkip={finish}
 				onFinish={finish}
 				onBack={() => go({}, 3)}

frontend/src/routes/(root)/(logged)/user/(user)/onboarding/+page.svelte

@@ -3,6 +3,7 @@
 	import { UserService } from '$lib/gen/services.gen'
 	import { goto } from '$lib/navigation'
 	import { page } from '$app/state'
+	import { toSameOriginRelativePath } from '$lib/logoutRedirect'
 	import CenteredModal from '$lib/components/CenteredModal.svelte'
 	import { Button } from '$lib/components/common'
 	import Popover from '$lib/components/meltComponents/Popover.svelte'
@@ -107,9 +108,11 @@
 	 * Same-origin relative paths only, so a crafted `?rd=` cannot bounce them off-site.
 	 */
 	function onboardingDestination(): string {
-		const rd = page.url.searchParams.get('rd')
-		if (rd && rd.startsWith('/') && !rd.startsWith('//')) return rd
-		return '/user/workspaces'
+		// `toSameOriginRelativePath` rather than a local check: it already rejects `//host`,
+		// `/\\host` (which WHATWG URL parsing resolves to a different origin), control
+		// characters and oversized values. A second, weaker copy of this is how one of those
+		// gets missed.
+		return toSameOriginRelativePath(page.url.searchParams.get('rd')) ?? '/user/workspaces'
 	}
 
 	async function skip() {