fix: harden the hub proxy, the workspace picker's gating and the rd hand-off

Findings from four local review passes over the branch:

- `list_projects` refuses when `disable_hub` is set, and `is_public_hub` now
  compares the parsed host, so no spelling of the public hub (mixed-case scheme
  or host, port, trailing dot, userinfo) forwards a member's bearer token there.
  Covered by a unit test table.
- The workspace picker waits on `usersWorkspaceStore` as well as `workspaces`,
  which derives to `[]` while the store is unloaded; with the create-form latch,
  one such frame swapped a member's picker for the create form until reload.
- `refreshSuperadmin` takes `force`, and the picker uses it: a `false` left over
  from a logged-out load decides whether the page is a picker or a create form.
  A cancelled call no longer publishes `false` over the live request's answer,
  and only its own request's handle is cleared.
- `rd` is sanitized once where it is derived rather than at each of the four
  hand-offs, so an absolute target keeps the OAuth callback's allowance and
  `https://evil.example/` is dropped.
- The archived-items probe answers "unknown" on failure, which keeps the
  ordinary caption and leaves the toolbar reachable.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012fRjnaHLwjpHN84gNNxah9
This commit is contained in:
Guilhem Lemouel
2026-09-08 10:39:02 +02:00
co-authored by Claude Opus 5
parent 0aefea7ce3
commit 38762db6cb
12 changed files with 231 additions and 74 deletions
+99 -9
View File
@@ -6,13 +6,14 @@ use axum::{
http::{request::Parts, StatusCode},
response::{IntoResponse, Response},
routing::{get, post},
Router,
Extension, Router,
};
use serde::{Deserialize, Deserializer, Serialize};
use windmill_common::{
error::{to_anyhow, Error},
global_settings::{load_value_from_global_settings, DISABLE_HUB_SETTING},
utils::require_admin,
HUB_BASE_URL,
DB, DEFAULT_HUB_BASE_URL, HUB_BASE_URL,
};
pub fn workspaced_service() -> Router {
@@ -553,17 +554,67 @@ async fn get_project_by_source(ctx: HubPublishCtx) -> Result<impl IntoResponse,
// `HubPublishCtx`, which requires an admin: nothing here is workspace-scoped or
// publishing-related. It exists at all because the hub's listing endpoint sends no
// CORS header, so the browser cannot read it directly the way it reads a single
// project. The caller's token rides along only so a private hub can authenticate the
// reader; `accept: application/json` is what makes the hub answer with JSON.
// project. `accept: application/json` is what makes the hub answer with JSON.
//
// The caller's token is sent only to a hub this instance was pointed at deliberately.
// Every other route here is admin-only; this one is not, so forwarding a member's
// bearer token to `hub.windmill.dev` would put a credential replayable against this
// instance on a host outside it — for a listing that needs no credential at all.
/// Whether this instance points at the public hub. Compared by parsed host rather than by the
/// string: `hub_base_url` is stored as the operator typed it, so `http://`, a port, a trailing
/// slash, a mixed-case scheme or host, userinfo and a trailing dot all name the same public
/// host — and each spelling that failed to match would send a member's token there. Parsing is
/// what `reqwest` does with the same string a line later, so this reads the host the request
/// will actually go to.
///
/// A value that does not parse answers "not the public hub", so the caller attaches the token —
/// harmless, because `reqwest` cannot build a request from that same value: it is rejected
/// before a connection is opened, and the token never reaches a socket.
fn is_public_hub(hub: &str) -> bool {
fn host_of(url: &str) -> Option<String> {
let parsed = url::Url::parse(url.trim()).ok()?;
if !matches!(parsed.scheme(), "http" | "https") {
return None;
}
Some(
parsed
.host_str()?
.trim_end_matches('.')
.to_ascii_lowercase(),
)
}
match (host_of(hub), host_of(DEFAULT_HUB_BASE_URL)) {
(Some(host), Some(default_host)) => host == default_host,
_ => false,
}
}
async fn list_projects(
_authed: ApiAuthed,
Extension(db): Extension<DB>,
Tokened { token }: Tokened,
) -> Result<impl IntoResponse, Error> {
let url = format!("{}/projects", **HUB_BASE_URL.load());
let res = HTTP_CLIENT
.get(&url)
.header("accept", "application/json")
.bearer_auth(&token)
// `disable_hub` turns the hub off for a closed instance, and this handler makes an
// outbound request. The frontend hides its entry points on the same setting, but that
// is presentation: an authenticated member can call this route directly, so the refusal
// has to live here.
let disabled = load_value_from_global_settings(&db, DISABLE_HUB_SETTING)
.await?
.and_then(|v| v.as_bool())
.unwrap_or(false);
if disabled {
return Err(Error::BadRequest(
"The hub is disabled on this instance".to_string(),
));
}
let hub = (**HUB_BASE_URL.load()).clone();
let url = format!("{}/projects", hub);
let mut req = HTTP_CLIENT.get(&url).header("accept", "application/json");
if !is_public_hub(&hub) {
req = req.bearer_auth(&token);
}
let res = req
.send()
.await
.map_err(|e| Error::InternalErr(format!("hub request failed: {e}")))?;
@@ -674,3 +725,42 @@ async fn forward_to_hub<T: Serialize>(
Ok((status, text))
}
#[cfg(test)]
mod tests {
use super::is_public_hub;
#[test]
fn public_hub_recognized_in_every_spelling() {
// The predicate decides whether a workspace member's bearer token leaves the
// instance, so both directions matter: a miss on the public hub sends the token
// to windmill.dev, and a false match withholds it from a private hub that needs it.
// Every spelling here is one `hub_base_url` can hold and `reqwest` will still send.
for hub in [
"https://hub.windmill.dev",
"http://hub.windmill.dev/",
"HTTPS://hub.windmill.dev",
"https://HUB.WINDMILL.DEV",
"https://hub.windmill.dev:443",
"https://hub.windmill.dev.",
"https://hub.windmill.dev/some/path",
" https://hub.windmill.dev ",
] {
assert!(is_public_hub(hub), "{hub} should be the public hub");
}
for hub in [
"https://hub.internal.example",
"https://hub.windmill.dev.evil.example",
"https://windmill.dev",
// The host is what the request goes to, whatever precedes the `@`.
"https://hub.windmill.dev@hub.internal.example",
// Unparseable, or not a scheme a request can be built from. Grouped with the
// private hubs because the caller then attaches the token, which is harmless here:
// `reqwest` rejects the same value before opening a connection.
"hub.windmill.dev",
"ftp://hub.windmill.dev",
] {
assert!(!is_public_hub(hub), "{hub} should not be the public hub");
}
}
}
@@ -64,15 +64,7 @@
import { setNoteEditorContext } from './graph/noteEditor.svelte'
import { GroupEditor, setGroupEditorContext } from './graph/groupEditor.svelte'
import { cleanFlow } from './flows/utils.svelte'
import {
DiffIcon,
HistoryIcon,
FileJson,
Settings,
Undo,
Redo,
Disc
} from 'lucide-svelte'
import { DiffIcon, HistoryIcon, FileJson, Settings, Undo, Redo, Disc } from 'lucide-svelte'
import Awareness from './Awareness.svelte'
import { getAllModules } from './flows/flowExplorer'
import { type FlowCopilotContext } from './copilot/flow'
@@ -1061,8 +1061,7 @@
<li>worker usage (worker, worker instance, vCPUs, memory)</li>
<li
>user usage (author count, operator count, the distinct guests of the last 30 days,
the seats they add past the free allowance, and the workspaces that allow
guests)</li
the seats they add past the free allowance, and the workspaces that allow guests)</li
>
<li>superadmin email addresses</li>
<li>development instance status</li>
@@ -1078,8 +1077,8 @@
loaded, whether SSO logins evaluate an IdP groups claim (SAML or OIDC) and change a
membership, the plan tier and quota shown when the execution meter is opened, whether
app sandbox isolation is turned on, whether a step's workspace script is edited from
the flow editor, how data tables and their migrations are set up and used, how often an
empty workspace home is seen, how often the home pages create menu and hub-project
the flow editor, how data tables and their migrations are set up and used, how often
an empty workspace home is seen, how often the home pages create menu and hub-project
picker are opened and from which entry point, and the name of any public hub project
imported from the home page and how far that import got, last 30 days)</li
>
@@ -1131,8 +1130,7 @@
<li>worker usage (worker, worker instance, vCPUs, memory)</li>
<li
>user usage (author count, operator count, the distinct guests of the last 30 days,
the seats they add past the free allowance, and the workspaces that allow
guests)</li
the seats they add past the free allowance, and the workspaces that allow guests)</li
>
<li>development instance status</li>
<li
@@ -1142,8 +1140,8 @@
loaded, whether SSO logins evaluate an IdP groups claim (SAML or OIDC) and change a
membership, the plan tier and quota shown when the execution meter is opened, whether
app sandbox isolation is turned on, whether a step's workspace script is edited from
the flow editor, how data tables and their migrations are set up and used, how often an
empty workspace home is seen, how often the home pages create menu and hub-project
the flow editor, how data tables and their migrations are set up and used, how often
an empty workspace home is seen, how often the home pages create menu and hub-project
picker are opened and from which entry point, and the name of any public hub project
imported from the home page and how far that import got, last 30 days)</li
>
@@ -7,12 +7,7 @@
import { redo, undo } from '$lib/history.svelte'
import { discardDraftAfterDeploy } from '$lib/userDraftToast'
import { UserDraftDbSyncer } from '$lib/userDraftDbSyncer.svelte'
import {
enterpriseLicense,
userStore,
userWorkspaces,
workspaceStore
} from '$lib/stores'
import { enterpriseLicense, userStore, userWorkspaces, workspaceStore } from '$lib/stores'
import { isMac, type Item, userPathPrefix } from '$lib/utils'
import { random_adj } from '$lib/components/random_positive_adjetive'
import {
@@ -121,10 +121,7 @@
</DrawerContent>
</Drawer>
<div
class="flex flex-col px-4 gap-2 text-sm"
id="app-editor-empty-runnable"
>
<div class="flex flex-col px-4 gap-2 text-sm" id="app-editor-empty-runnable">
<div class="mt-2 flex justify-between gap-4" id="app-editor-runnable-header">
<div class="font-bold items-baseline truncate">Choose a language</div>
<div class="flex gap-2">
@@ -18,6 +18,13 @@
import { logFeatureUsage } from '$lib/utils/featureUsage'
import { sendUserToast } from '$lib/toast'
/**
* Writes a hub project's items into the active workspace. It checks no permission itself,
* so a caller must not offer it to an operator or in a workspace whose direct-deploy
* protection has cleared `showEditButtons` — `ItemsList` gates both of its entry points on
* exactly that, and the import would otherwise fail item by item against the server's own
* checks, after the dialog had promised to run.
*/
interface Props {
/** The project the picker chose. Setting it opens the dialog. */
pick: HubProjectPick | undefined
@@ -133,9 +140,18 @@
// the import step show them, so the detail is fetched for the one project chosen. The
// card renders from the pick until it lands — counts are the only thing missing, and
// zero counts render as no badges rather than as zeroes.
//
// The answer is checked against the slug that asked for it: `resource()` aborts the
// previous controller but `fetchHubProject` takes no signal, and nothing orders the
// responses — so picking A, dismissing, then picking B can land A's name, author and
// counts over an import that writes B.
const detail = resource(
() => slug,
async (s) => (s ? await fetchHubProject(s) : undefined)
async (s) => {
if (!s) return undefined
const fetched = await fetchHubProject(s)
return s === slug ? fetched : detail.current
}
)
let project = $derived<ImportProjectSummary | undefined>(
detail.current ??
@@ -178,13 +194,7 @@
// The counters' key vocabularies, enumerated here so the whole set is reviewable at once.
type AbandonStage = 'running' | 'setup' | 'done' | 'idle'
type SetupOutcome = 'filled' | 'skipped' | 'none' | 'unchecked'
type SetupBucket =
| 'filled'
| 'none'
| 'unchecked'
| 'skipped_1'
| 'skipped_2_5'
| 'skipped_6plus'
type SetupBucket = 'filled' | 'none' | 'unchecked' | 'skipped_1' | 'skipped_2_5' | 'skipped_6plus'
// Set for the closing that Finish itself asks for, since that closing reaches `dismiss()`
// by the same falling edge as the X.
@@ -1016,10 +1016,12 @@
* Whether a workspace the default listing found empty is empty at all, or just has nothing
* unarchived — two different states that want two different things said about them. Asked
* only in that case, and once per workspace: one request for one row, never on a workspace
* with something in it. A failure answers "no archived items", which shows the ordinary
* placeholder rather than promising items that may not be there.
* with something in it. `hasArchived` is undefined when the request failed — see the catch
* for what that leaves standing.
*/
let archivedProbe = $state<{ workspace: string; hasArchived: boolean } | undefined>(undefined)
let archivedProbe = $state<{ workspace: string; hasArchived: boolean | undefined } | undefined>(
undefined
)
$effect(() => {
const ws = $workspaceStore
if (!ws || !workspaceEmpty || archivedProbe?.workspace === ws) return
@@ -1039,11 +1041,20 @@
})
archivedProbe = { workspace, hasArchived: (res.items?.length ?? 0) > 0 }
} catch (error) {
// Undefined, not false: false would say the workspace is empty and — since the
// toolbar is inert on the strength of the placeholder carrying the way to archived
// items — leave no way to them at all. Unknown keeps the ordinary caption, which
// promises nothing, and leaves the searchbar live as the fallback it used to be.
console.error('Could not check for archived items:', error)
archivedProbe = { workspace, hasArchived: false }
archivedProbe = { workspace, hasArchived: undefined }
}
}
let emptyStateAnswered = $derived(archivedProbe?.workspace === $workspaceStore)
/**
* The probe could not tell. The toolbar stays usable in that case: `inert` is only right
* while the placeholder is the way to archived items, and here it cannot be.
*/
let archivedUnknown = $derived(emptyStateAnswered && archivedProbe?.hasArchived === undefined)
/**
* Whether this user may be offered the create actions. The empty state's template import
* and create menu do no permission check of their own, so an operator — or a workspace
@@ -1066,6 +1077,12 @@
visiblePipelineFolders.size === 0 &&
!hasMoreServer
)
/**
* The toolbar is dimmed either way; `inert` also takes it off the pointer, which is only
* right while the placeholder carries the way to archived items. A probe that could not
* tell leaves it live as the fallback.
*/
let toolbarInert = $derived(workspaceEmpty && !archivedUnknown)
// Owners the counts found the user has something in, split by kind. They cover
// what the folder/username lists miss: an item shared individually out of a
@@ -1736,7 +1753,7 @@
lands; `inert` takes it out of the tab order and off the pointer meanwhile. A
workspace with nothing but archived items reaches them from its own placeholder,
so these controls are not the way there. -->
<div class="flex justify-start" class:opacity-40={workspaceEmpty} inert={workspaceEmpty}>
<div class="flex justify-start" class:opacity-40={workspaceEmpty} inert={toolbarInert}>
<ToggleButtonGroup
selected={itemKind}
onSelected={(v) => {
@@ -1838,7 +1855,7 @@
<div
class="relative text-primary w-full min-w-[200px] max-w-[26rem]"
class:opacity-40={workspaceEmpty}
inert={workspaceEmpty}
inert={toolbarInert}
>
<FilterSearchbar
schema={searchbarSchema}
@@ -1854,7 +1871,7 @@
<!-- Same gate the old create actions used: hidden from operators and in workspaces
whose direct-deploy protection cleared showEditButtons (NoDirectDeployAlert), since
the menu itself does no permission check. -->
{#if !$userStore?.operator && showEditButtons}
{#if canCreateHere}
<!-- No hub entry where the instance has the hub turned off: the same setting the
script and flow hub pickers observe. -->
<CreateActionsMenu
@@ -19,6 +19,15 @@
WORKSPACE_HANDOVER_MS
} from '$lib/workspaceCreation'
/**
* Creates a workspace. It enforces no permission of its own — `create_workspace` is the
* gate, and it refuses when `CREATE_WORKSPACE_REQUIRE_SUPERADMIN` is on and the caller is
* not one — so a surface that offers this form to someone who may not create is offering
* an action that ends in a 401. A caller must establish that first. The workspace picker
* asks `canCreateWorkspace()`; the onboarding step does not, because it is reached only
* from the cloud sign-in path, where the setting is off by definition — a surface with
* any other way in owes the check.
*/
interface Props {
/** Where to go once the workspace exists. It is already the active one by then. */
onCreated: () => void
-1
View File
@@ -185,7 +185,6 @@ export function hubProjectCatalogue(workspace: string): Promise<HubProjectPick[]
return catalogue.projects
}
async function loadCatalogue(workspace: string): Promise<HubProjectPick[]> {
const raw = await HubPublishService.listHubProjects({ workspace })
const rows = ((typeof raw === 'string' ? JSON.parse(raw) : raw)?.projects ??
+25 -9
View File
@@ -1,23 +1,39 @@
import { get } from 'svelte/store'
import { CancelablePromise, UserService, type GlobalUserInfo } from '$lib/gen'
import { CancelablePromise, CancelError, UserService, type GlobalUserInfo } from '$lib/gen'
import { superadmin, devopsRole } from './stores.js'
let promise: CancelablePromise<GlobalUserInfo> | null = null
async function _refreshSuperadmin(): Promise<void> {
let shouldFetch = get(superadmin) == undefined || get(devopsRole) == undefined
/**
* `force` asks the server even when the stores already hold an answer. Worth it where a wrong
* answer changes what the page offers rather than how it looks: a logged-out load sets both
* stores to `false` — the request 401s — and without `force` nothing asks again for the rest
* of the session, so the user who signs in next reads as neither superadmin nor devops.
*/
async function _refreshSuperadmin(opts?: { force?: boolean }): Promise<void> {
let shouldFetch = opts?.force || get(superadmin) == undefined || get(devopsRole) == undefined
if (!shouldFetch) return undefined
promise?.cancel()
promise = UserService.globalWhoami()
// Held locally so the check at the end can tell this request from a later caller's, which
// by then owns `promise`.
const mine = UserService.globalWhoami()
promise = mine
try {
const me = await promise
const me = await mine
superadmin.set(me.super_admin ? me.email : false)
devopsRole.set(me.devops || me.super_admin ? me.email : false)
} catch (error) {
superadmin.set(false)
devopsRole.set(false)
console.error('error refreshing superadmin/devops role', error)
// A cancellation says nothing about this user, so it must not be written down as an
// answer: `clearStores` cancels on logout, and a second caller cancels the first — and
// `false` here is precisely the stale state `force` exists to get out of.
if (!(error instanceof CancelError)) {
superadmin.set(false)
devopsRole.set(false)
console.error('error refreshing superadmin/devops role', error)
}
}
promise = null
// Only if nobody has started another: clearing a live request's handle would put it beyond
// the reach of `cancel()`, and it would then land on a session that had been cleared.
if (promise === mine) promise = null
}
export const refreshSuperadmin = Object.assign(_refreshSuperadmin, {
@@ -34,6 +34,7 @@
LogOut
} from 'lucide-svelte'
import { isCloudHosted } from '$lib/cloud'
import { isValidLogoutRedirect, toSameOriginRelativePath } from '$lib/logoutRedirect'
import { canCreateWorkspace } from '$lib/workspaceCreation'
import SimpleCreateWorkspace from '$lib/components/workspaceSettings/SimpleCreateWorkspace.svelte'
import AnimatedButton from '$lib/components/common/button/AnimatedButton.svelte'
@@ -59,7 +60,20 @@
let userSettings: UserSettings | undefined = $state()
let superadminSettings: SuperadminSettings | undefined = $state()
let rd = $derived($page.url.searchParams.get('rd'))
// Sanitized here rather than at each hand-off below: all four send an absolute `rd` to
// `window.location.href`, which unlike `goto` leaves the origin, and a fourth weaker copy
// of the check is how one of them gets missed. Absolute targets keep the allowance the
// OAuth callback uses (`isValidLogoutRedirect`: same origin, `*.windmill.dev`, the hub),
// since honouring one is why those branches exist. Anything else falls back to '/'.
let rd = $derived.by(() => {
const raw = $page.url.searchParams.get('rd')
if (!raw) return null
// Truthy for a safe relative path and for a same-origin URL; null for `//host`,
// `/\host` and control characters, which read as relative but are not.
if (toSameOriginRelativePath(raw)) return raw
if (!raw.startsWith('http')) return null
return isValidLogoutRedirect(raw) ? raw : null
})
run(() => {
if (userSettings && $page.url.hash.startsWith(USER_SETTINGS_HASH)) {
@@ -132,7 +146,11 @@
getCreateWorkspaceRequireSuperadmin()
}
refreshSuperadmin()
// Forced: this page hands the superadmin their instance settings and the list-all toggle,
// and stands the picker down entirely for a user who has nothing to pick — so a `false`
// left over from a logged-out load in this session (see `refreshSuperadmin`) does not just
// hide a button, it decides what the page is.
refreshSuperadmin({ force: true })
loadInvites()
loadWorkspaces()
@@ -142,22 +160,37 @@
// the empty list, so it *is* that action. Shown as the creation form rather than a page
// asking you to choose between one thing. Held back until the invites have loaded, or a
// user with an invite waiting would see a form for a workspace they do not need.
// `$derived.by` for the loaded test: a `$derived` reading `workspaces` directly narrows it
// to `never` here, the same reason `allWorkspaces` is written that way above.
let workspacesLoaded = $derived.by(() => workspaces !== undefined)
// Both halves, the way the markup below tests it: `workspaces` is assigned from
// `$userWorkspaces` by the legacy pre-effect, and that derives to `[]` while
// `usersWorkspaceStore` is still undefined — so `workspaces !== undefined` alone is true
// from the first flush of a hard load, with an empty list behind it. Since `showCreate`
// latches, one such frame would swap a member's picker for the create form until reload.
// `$derived.by` because a plain `$derived` reading `workspaces` narrows it to `never`
// here, the same reason `allWorkspaces` is written that way above.
let workspacesLoaded = $derived.by(
() => workspaces !== undefined && $usersWorkspaceStore !== undefined
)
// Not for a superadmin: this page is also where they reach the instance settings and the
// list-all toggle, and standing the picker down takes both away — a superadmin with no
// membership of their own has business here besides creating a workspace. Waiting for the
// store to answer rather than reading `!$superadmin`, which is true while `globalWhoami`
// is still in flight.
let nothingToChoose = $derived(
workspacesLoaded &&
invitesLoaded &&
createWorkspace &&
$superadmin !== undefined &&
!$superadmin &&
!list_all_as_super_admin &&
allWorkspaces.length === 0 &&
invites.length === 0
)
/**
* Where to go once a workspace exists. `rd` can be absolute — the CLI login flow sends one —
* and `goto` refuses those, which would strand the caller on its "Creating …" screen with
* the workspace already made. Same hand-off every other `rd` path on this page makes.
* Where to go once a workspace exists. `rd` can be absolute — a login flow persists the page
* URL it interrupted — and `goto` refuses those, which would strand the caller on its
* "Creating …" screen with the workspace already made. Same hand-off every other `rd` path
* on this page makes, over the value sanitized where `rd` is derived.
*/
function leaveForWorkspace() {
if (rd?.startsWith('http')) {
@@ -174,6 +207,11 @@
let showCreate = $state(false)
$effect(() => {
if (nothingToChoose) showCreate = true
// Except for a superadmin, whom `nothingToChoose` excludes — so this only ever undoes a
// latch that should not have happened: one taken on a stale `false` before the forced
// `refreshSuperadmin` above answered. Without it that superadmin would be stuck on the
// create form, instance settings and the list-all toggle gone with it, until a reload.
else if ($superadmin) showCreate = false
})
async function speakFriendAndEnterWorkspace(workspaceId: string) {
+1 -5
View File
@@ -307,11 +307,7 @@
<Splitpanes horizontal class="max-h-screen grow min-h-0">
<Pane size={33}>
{#if flowStore.val?.value?.modules}
<FlowModuleSchemaMap
disableAi
smallErrorHandler={true}
disableStaticInputs
/>
<FlowModuleSchemaMap disableAi smallErrorHandler={true} disableStaticInputs />
{:else}
<div class="text-red-400 mt-20">Missing flow modules</div>
{/if}