From bc614f481d8decae63c4340710523fdf4b0ecf67 Mon Sep 17 00:00:00 2001
From: Guilhem Lemouel <guilhemlemouel@gmail.com>
Date: Fri, 21 Aug 2026 19:24:08 +0200
Subject: [PATCH] =?UTF-8?q?fix:=20address=20review=20=E2=80=94=20username,?=
 =?UTF-8?q?=20name=20length,=20leaving=20mid-run?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

**The new-workspace username was never validated.** Step 2 shows the field when
the instance does not derive one, but neither the Continue gate nor
`planProblem` looked at it. `create_workspace` does not close that hole:
`nw.username.ok_or(...)` accepts `Some("")` and never runs the `VALID_USERNAME`
check `join_workspace` does, so a cleared field created a workspace whose owner
has an empty username, and a digit-first one was stored verbatim. Both now
refuse, using the same `validateUsername` the sibling creator has always run.

**The name length was unchecked**, so a >50-char name walked through two more
steps and failed at create. `WORKSPACE_NAME_MAX_LENGTH` sits next to the id
limit and `planProblem` checks it.

**Leaving mid-run did not stop the run.** The dialog promised "The import stops
where it is. Coming back to this link picks it up again", but navigating away
only unmounted the UI: the executor kept going, reached `done`, and called
`clearParkedImport()` — so returning to the link tried to create the workspace
again and failed with "already exists". Worse, the review drawer's teardown
resolved the pending review to `false`, meaning "skip the migrations", and the
orphan imported every item without the tables they need.

Nothing can abort a request already in flight — `installProject` takes no
signal — so `abandon()` stops the run at the next phase boundary and leaves the
workspace parked, and the teardown now resolves `'abort'`, which stops the
import rather than silently dropping the migrations.

Also drops a stale JSDoc above `hubAppIcon` still describing the fetch-and-
sanitize implementation that `ea31f73ed3` replaced.

Adds the coverage the review asked for: the parking decision at the end of a
run, and the two validation gates.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 .../lib/components/ImportProjectStep.svelte   | 22 ++++++---
 frontend/src/lib/hubProject.ts                | 18 --------
 frontend/src/lib/importWizard/abandon.test.ts | 41 +++++++++++++++++
 .../src/lib/importWizard/execution.svelte.ts  | 27 +++++++++--
 frontend/src/lib/importWizard/plan.test.ts    | 46 +++++++++++++++++++
 frontend/src/lib/importWizard/plan.ts         | 17 ++++++-
 frontend/src/lib/utils/workspaceId.ts         |  3 ++
 .../projects/import/+page@(root).svelte       | 17 ++++++-
 8 files changed, 161 insertions(+), 30 deletions(-)
 create mode 100644 frontend/src/lib/importWizard/abandon.test.ts

diff --git a/frontend/src/lib/components/ImportProjectStep.svelte b/frontend/src/lib/components/ImportProjectStep.svelte
index 01b5b2931a..2aef08b6e7 100644
--- a/frontend/src/lib/components/ImportProjectStep.svelte
+++ b/frontend/src/lib/components/ImportProjectStep.svelte
@@ -71,9 +71,10 @@
 	>([])
 	// Bumped per review session so the Monaco editors re-mount with the new SQL.
 	let reviewGeneration = $state(0)
-	let reviewResolve: ((run: boolean) => void) | undefined
+	/** `abort` stops the whole import; `false` only skips the migrations. */
+	let reviewResolve: ((run: boolean | 'abort') => void) | undefined
 
-	function openMigrationReview(migs: ProjectMigration[]): Promise<boolean> {
+	function openMigrationReview(migs: ProjectMigration[]): Promise<boolean | 'abort'> {
 		reviewList = migs.map((m) => ({
 			datatable_name: m.datatable_name,
 			sql: m.sql,
@@ -82,7 +83,7 @@
 		}))
 		reviewGeneration++
 		reviewDrawer?.openDrawer()
-		return new Promise((resolve) => (reviewResolve = resolve))
+		return new Promise<boolean | 'abort'>((resolve) => (reviewResolve = resolve))
 	}
 	function closeMigrationReview(run: boolean) {
 		// Capture + clear first so the `on:close` fired by closeDrawer() (which would
@@ -117,6 +118,9 @@
 		const runnable = enabled.filter((m) => present.has(m.datatable_name))
 		if (runnable.length === 0) return []
 		const run = await openMigrationReview(runnable)
+		// `abort` is the teardown case: the step is gone, so stop rather than import the
+		// items without the tables the review was about.
+		if (run === 'abort') return null
 		if (!run) return []
 		return reviewList
 			.filter((r) => r.run && r.sql.trim() !== '')
@@ -244,15 +248,21 @@
 			// Deliberately not re-read against `running`: the answer was about leaving, and a
 			// run that finished in the meantime only makes leaving safer.
 			leaveApproved = true
+			// Stop the run before navigating. Nothing can abort a request already in flight,
+			// so this stops it at the next phase boundary and keeps the workspace parked, so
+			// the link the message promises actually resumes instead of failing on create.
+			execution?.abandon()
 			await goto(to)
 		} finally {
 			askingToLeave = false
 		}
 	}
 
-	// If this step is torn down while the review drawer is open, resolve the promise
-	// the executor is waiting on rather than leaving it pending forever.
-	$effect(() => () => reviewResolve?.(false))
+	// Torn down with the review drawer open, the executor is still awaiting an answer.
+	// Abort rather than resolve: resolving to `false` means "skip the migrations", which
+	// would let the orphaned run import every item *without* the tables they need — the
+	// opposite of leaving it where it was.
+	$effect(() => () => reviewResolve?.('abort'))
 
 	const deleteModal = createAsyncConfirmationModal()
 	async function deleteWorkspace() {
diff --git a/frontend/src/lib/hubProject.ts b/frontend/src/lib/hubProject.ts
index 10294369a0..b58b338f7d 100644
--- a/frontend/src/lib/hubProject.ts
+++ b/frontend/src/lib/hubProject.ts
@@ -68,24 +68,6 @@ export async function fetchHubProject(slug: string): Promise<ImportProjectSummar
 	}
 }
 
-/**
- * One integration's icon, as SVG markup rather than an <img>: the hub's icons are
- * `fill="currentColor"`, so inlining them lets the icon follow the page's theme.
- * Returns undefined when the hub ships no icon for that slug (it 404s), which is
- * the caller's cue to fall back to a placeholder.
- *
- * Sanitized before it is returned, because the markup is inlined into this
- * authenticated origin and the hub is not necessarily ours: `hub_base_url` is an
- * instance setting, and the import wizard can be pointed at any hub by URL. A
- * hostile or compromised one answering with `<svg onload=…>` would otherwise run
- * script here. SVG profile only — no HTML, and `svg` plus `svgFilters` namespaces.
- *
- * `style` and `image` are forbidden on top of that profile, because the profile
- * allows both and neither is something an icon needs. An inline `<svg><style>` is
- * document-scoped, not shadowed — it would let the answering hub restyle this page,
- * including moving or hiding the wizard's own Import and Delete controls — and
- * `<image href="https://…">` is a page-view beacon pointed at whoever it likes.
- */
 /**
  * The icon for a hub integration slug, resolved from the icons Windmill already bundles.
  *
diff --git a/frontend/src/lib/importWizard/abandon.test.ts b/frontend/src/lib/importWizard/abandon.test.ts
new file mode 100644
index 0000000000..dd0ee0c82e
--- /dev/null
+++ b/frontend/src/lib/importWizard/abandon.test.ts
@@ -0,0 +1,41 @@
+import { beforeEach, describe, expect, it } from 'vitest'
+
+import { clearParkedImport, parkImport, resumableImport } from './parking'
+
+/**
+ * Leaving mid-run is the one case where finishing and *stopping* disagree about the parked
+ * workspace. A run that clears parking on its way out makes the link the user was told to
+ * come back to create the workspace a second time — and fail, because it already exists.
+ *
+ * `ImportExecution` needs a live API to construct, so this covers the decision itself rather
+ * than the class: what `#import`'s tail does with parking, given whether the user left.
+ */
+function finish(opts: { abandoned: boolean }): void {
+	if (!opts.abandoned) clearParkedImport()
+}
+
+describe('parking across the end of a run', () => {
+	beforeEach(() => clearParkedImport())
+
+	it('clears the parked workspace when the run finishes normally', () => {
+		parkImport({ slug: 'calendly', workspaceId: 'calendly-7' })
+		finish({ abandoned: false })
+		// A later import of the same project must reach its own create rather than adopt
+		// the workspace this run made.
+		expect(resumableImport('calendly', 'calendly-7')).toBe(false)
+	})
+
+	it('keeps it when the user left mid-run, so the link still resumes', () => {
+		parkImport({ slug: 'calendly', workspaceId: 'calendly-7' })
+		finish({ abandoned: true })
+		expect(resumableImport('calendly', 'calendly-7')).toBe(true)
+	})
+
+	it('still scopes the resume to the project that parked it', () => {
+		parkImport({ slug: 'calendly', workspaceId: 'calendly-7' })
+		finish({ abandoned: true })
+		// Abandoning must not turn the parked entry into a workspace any project can adopt.
+		expect(resumableImport('bitly', 'calendly-7')).toBe(false)
+		expect(resumableImport('calendly', 'bitly-1')).toBe(false)
+	})
+})
diff --git a/frontend/src/lib/importWizard/execution.svelte.ts b/frontend/src/lib/importWizard/execution.svelte.ts
index 9c9c48edce..5c0cc2aea1 100644
--- a/frontend/src/lib/importWizard/execution.svelte.ts
+++ b/frontend/src/lib/importWizard/execution.svelte.ts
@@ -208,16 +208,33 @@ export class ImportExecution {
 	 * `installProject` over the whole bundle, which is idempotent per item but does
 	 * not skip the ones that already landed.
 	 */
+	/**
+	 * Set when the user confirms leaving mid-run. Nothing here can abort a request already
+	 * in flight — `installProject` takes no signal — so this stops the run at the next phase
+	 * boundary instead, which is as far as "stops where it is" can honestly go.
+	 *
+	 * Its other job is to keep the parked workspace: a run that clears parking on its way out
+	 * would make the link the user was told to come back to create the workspace a second
+	 * time and fail with "already exists".
+	 */
+	#abandoned = false
+
+	/** The user has left. Stop at the next phase boundary and leave the run resumable. */
+	abandon() {
+		this.#abandoned = true
+	}
+
 	async run(): Promise<void> {
 		if (this.running) return
+		this.#abandoned = false
 		this.running = true
 		runState.active = true
 		this.error = undefined
 		try {
 			const workspace = await this.#ensureWorkspace()
-			if (!workspace) return
+			if (!workspace || this.#abandoned) return
 			const exportData = await this.#ensureExport(workspace)
-			if (!exportData) return
+			if (!exportData || this.#abandoned) return
 			await this.#import(workspace, exportData)
 		} finally {
 			this.running = false
@@ -380,8 +397,10 @@ export class ImportExecution {
 		// and the failures are listed. Only a hard stop leaves `done` false.
 		this.done = true
 		// Nothing left to resume. A later import of the same project must reach its
-		// create rather than adopt this one.
-		clearParkedImport()
+		// create rather than adopt this one — unless the user left mid-run, in which case
+		// the workspace this created is exactly what the link they were told to return to
+		// has to find.
+		if (!this.#abandoned) clearParkedImport()
 		if (failed > 0) this.error = `${failed} item${failed === 1 ? '' : 's'} failed to import.`
 	}
 
diff --git a/frontend/src/lib/importWizard/plan.test.ts b/frontend/src/lib/importWizard/plan.test.ts
index 0d7ec8bf19..c81ed700bc 100644
--- a/frontend/src/lib/importWizard/plan.test.ts
+++ b/frontend/src/lib/importWizard/plan.test.ts
@@ -84,6 +84,52 @@ describe('readPlan', () => {
 	})
 })
 
+/**
+ * `create_workspace` takes the username it is given and writes it to `usr.username` without
+ * checking it — `Some("")` passes its only guard — so a blank or malformed one becomes a
+ * workspace whose owner has no usable name. The wizard is the last thing that can refuse it.
+ */
+describe('planProblem — the new-workspace username', () => {
+	const dest = (username?: string) => ({
+		slug: 'calendly',
+		destination: { kind: 'new' as const, id: 'calendly', name: 'Calendly', username }
+	})
+
+	it('asks for nothing when the instance derives the username', () => {
+		expect(planProblem(dest(undefined))).toBeUndefined()
+	})
+
+	it('refuses a blank or whitespace-only username', () => {
+		expect(planProblem(dest(''))).toMatch(/needs a username/i)
+		expect(planProblem(dest('   '))).toMatch(/needs a username/i)
+	})
+
+	it('refuses one the backend would store verbatim but never accept elsewhere', () => {
+		expect(planProblem(dest('1bad'))).toMatch(/letters and numbers/i)
+		expect(planProblem(dest('a b'))).toMatch(/letters and numbers/i)
+	})
+
+	it('accepts a valid one', () => {
+		expect(planProblem(dest('guilhem'))).toBeUndefined()
+	})
+})
+
+/** `validate_workspace_name` refuses > 50 chars, and only at creation — two steps later. */
+describe('planProblem — the new-workspace name length', () => {
+	const named = (name: string) => ({
+		slug: 'calendly',
+		destination: { kind: 'new' as const, id: 'calendly', name }
+	})
+
+	it('accepts the longest name the backend takes', () => {
+		expect(planProblem(named('x'.repeat(50)))).toBeUndefined()
+	})
+
+	it('refuses one character more, rather than failing at create', () => {
+		expect(planProblem(named('x'.repeat(51)))).toMatch(/too long/i)
+	})
+})
+
 describe('planProblem', () => {
 	it('names what is missing, in the order the wizard asks for it', () => {
 		expect(planProblem({ slug: '' })).toMatch(/No project/)
diff --git a/frontend/src/lib/importWizard/plan.ts b/frontend/src/lib/importWizard/plan.ts
index 59e2217550..f209041787 100644
--- a/frontend/src/lib/importWizard/plan.ts
+++ b/frontend/src/lib/importWizard/plan.ts
@@ -11,7 +11,8 @@
  * user asks it to on the last step.
  */
 
-import { validateWorkspaceId } from '$lib/utils/workspaceId'
+import { validateUsername } from '$lib/utils'
+import { validateWorkspaceId, WORKSPACE_NAME_MAX_LENGTH } from '$lib/utils/workspaceId'
 
 /**
  * `existing` carries no workspace until one is picked: step 1 answers *which kind*
@@ -92,8 +93,22 @@ export function planProblem(plan: ImportPlan): string | undefined {
 	if (!d) return 'Pick a destination first'
 	if (d.kind === 'new') {
 		if (!d.name.trim()) return 'The new workspace needs a name'
+		// The backend refuses a longer one (`validate_workspace_name`), and only at
+		// creation — by then the wizard has already walked the user through two more steps.
+		if (d.name.trim().length > WORKSPACE_NAME_MAX_LENGTH) {
+			return `The name is too long (${d.name.trim().length} chars). Maximum is ${WORKSPACE_NAME_MAX_LENGTH}.`
+		}
 		const idProblem = validateWorkspaceId(d.id)
 		if (idProblem) return idProblem
+		// Only asked for when the instance does not derive it. `create_workspace` takes
+		// whatever it is given here — `Some("")` passes its only check — so a blank or
+		// malformed username is written to `usr.username` verbatim rather than refused.
+		// The sibling creator validates it; this is the same check.
+		if (d.username !== undefined) {
+			if (!d.username.trim()) return 'The new workspace needs a username'
+			const bad = validateUsername(d.username.trim())
+			if (bad) return bad
+		}
 	} else if (!d.workspaceId) {
 		return 'Pick the workspace to import into'
 	} else if (validateWorkspaceId(d.workspaceId)) {
diff --git a/frontend/src/lib/utils/workspaceId.ts b/frontend/src/lib/utils/workspaceId.ts
index 18bb8e45c5..1d7b989e13 100644
--- a/frontend/src/lib/utils/workspaceId.ts
+++ b/frontend/src/lib/utils/workspaceId.ts
@@ -10,6 +10,9 @@ export const WORKSPACE_ID_RE = /^\w+(-\w+)*$/
 /** The DB column and the git branch name derived from it both stop here. */
 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
+
 /**
  * The reason `id` is not a usable workspace id, or undefined when it is.
  *
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 7e51a6d3fd..06cfd3c31b 100644
--- a/frontend/src/routes/(root)/(logged)/projects/import/+page@(root).svelte
+++ b/frontend/src/routes/(root)/(logged)/projects/import/+page@(root).svelte
@@ -22,6 +22,7 @@
 	import { WorkspaceService, type UserWorkspaceList } from '$lib/gen'
 	import { canCreateWorkspace, loadUsernamePolicy } from '$lib/workspaceCreation'
 	import { toWorkspaceId, validateWorkspaceId } from '$lib/utils/workspaceId'
+	import { validateUsername } from '$lib/utils'
 	import {
 		readPlan,
 		planToSearch,
@@ -144,6 +145,17 @@
 	}
 
 	const idProblem = $derived(id.trim() ? validateWorkspaceId(id.trim()) : undefined)
+	// Only when the instance does not derive it, which is the only case the field is shown.
+	// `create_workspace` accepts whatever it is sent — `Some("")` passes its one check and is
+	// written to `usr.username` verbatim — so this is the only thing standing between a
+	// cleared field and a workspace whose owner has no username.
+	const usernameProblem = $derived(
+		automateUsername
+			? undefined
+			: !username.trim()
+				? 'A username is required'
+				: validateUsername(username.trim()) || undefined
+	)
 	// Step 2 shows the workspace list when step 1 chose "one I already have".
 	const choiceIsExisting = $derived(plan.destination?.kind === 'existing')
 
@@ -390,6 +402,9 @@
 						<label class="flex max-w-[50%] flex-col gap-1">
 							<span class="text-xs font-normal text-secondary">Your username in it</span>
 							<TextInput size="sm" bind:value={username} />
+							{#if usernameProblem && username.trim()}
+								<span class="text-2xs font-normal text-red-500">{usernameProblem}</span>
+							{/if}
 						</label>
 					{/if}
 				{:else}
@@ -467,7 +482,7 @@
 						<Button
 							unifiedSize="sm"
 							variant="accent"
-							disabled={!name.trim() || !!idProblem || idTaken || checkingId}
+							disabled={!name.trim() || !!idProblem || !!usernameProblem || idTaken || checkingId}
 							onClick={confirmNewWorkspace}
 						>
 							Continue →