From f466b2a15edd3f6623c07a07cdd43a55f87b30ae Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Wed, 10 Jun 2026 21:09:53 +0200 Subject: [PATCH] fix(stage-2): audit dashboard TS alignment + login UX Closes 4 audit findings: - Route TS interface now carries dispatch_mode (sync|async). Backend's shared::route::Route has had this since 0012_routes_dispatch_mode but the TS client silently always created sync routes and the list display dropped the field. Add a Dispatch select to the new-route form and an "ASYNC" badge in the route list. - api.files.downloadUrl pointed to a never-registered backend endpoint. The dashboard's live Download button was hitting 405. Add the GET handler (AppFilesRead + FilesRepo::head + FilesRepo::get, Content- Disposition: inline) at the same path that delete already used. - F-T-003: adminRequest's 401 handler called goto(login) without a recursion cap. If the login endpoint itself returned 401, the wrapper looped until browser nav limits. Track consecutive 401s within a 10s window and hard-reload to /login?reason=auth-loop after the third, showing the user an explanatory banner. Any 2xx resets the counter. - Login flow now honors a ?returnTo= query parameter. adminRequest and the root layout both append the current location when redirecting to /login; the login page validates the value is a same-origin admin path (no open-redirect) and goto's there after successful sign-in. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/manager-core/src/files_api.rs | 48 ++++++++++++++++- dashboard/src/lib/api.ts | 51 +++++++++++++++++-- dashboard/src/routes/+layout.svelte | 10 +++- dashboard/src/routes/login/+page.svelte | 40 ++++++++++++++- .../src/routes/scripts/[id]/+page.svelte | 27 +++++++++- 5 files changed, 166 insertions(+), 10 deletions(-) diff --git a/crates/manager-core/src/files_api.rs b/crates/manager-core/src/files_api.rs index ff96c2b..8de6c73 100644 --- a/crates/manager-core/src/files_api.rs +++ b/crates/manager-core/src/files_api.rs @@ -15,10 +15,12 @@ use std::sync::Arc; +use axum::body::Body; use axum::extract::{Path, Query, State}; +use axum::http::header::{CONTENT_DISPOSITION, CONTENT_LENGTH, CONTENT_TYPE}; use axum::http::StatusCode; use axum::response::{IntoResponse, Json, Response}; -use axum::routing::{delete, get}; +use axum::routing::get; use axum::{Extension, Router}; use picloud_shared::{AppId, Principal}; use serde::{Deserialize, Serialize}; @@ -41,7 +43,7 @@ pub fn files_admin_router(state: FilesAdminState) -> Router { .route("/apps/{app_id}/files", get(list_files)) .route( "/apps/{app_id}/files/{collection}/{file_id}", - delete(delete_file), + get(get_file).delete(delete_file), ) .with_state(state) } @@ -120,6 +122,48 @@ async fn list_files( })) } +/// `GET /apps/{id}/files/{collection}/{file_id}` — stream the file's +/// bytes inline. The dashboard's Download button hits this; the audit +/// flagged the missing handler (callers were 405'ing). Metadata gives +/// the Content-Type + Content-Disposition; bytes come from +/// `FilesRepo::get` (which checksum-verifies on read). +async fn get_file( + State(s): State, + Extension(principal): Extension, + Path((id_or_slug, collection, file_id)): Path<(String, String, String)>, +) -> Result { + let app_id = resolve_app(&*s.apps, &id_or_slug).await?; + require( + s.authz.as_ref(), + &principal, + Capability::AppFilesRead(app_id), + ) + .await?; + let id = Uuid::parse_str(&file_id).map_err(|_| FilesApiError::NotFound)?; + let meta = s + .files + .head(app_id, &collection, id) + .await? + .ok_or(FilesApiError::NotFound)?; + let bytes = s + .files + .get(app_id, &collection, id) + .await? + .ok_or(FilesApiError::NotFound)?; + let disposition = format!( + "inline; filename=\"{}\"", + meta.name.replace('"', "").replace(['\r', '\n'], "") + ); + let len = bytes.len(); + Ok(Response::builder() + .status(StatusCode::OK) + .header(CONTENT_TYPE, meta.content_type) + .header(CONTENT_DISPOSITION, disposition) + .header(CONTENT_LENGTH, len) + .body(Body::from(bytes)) + .expect("build files download response")) +} + async fn delete_file( State(s): State, Extension(principal): Extension, diff --git a/dashboard/src/lib/api.ts b/dashboard/src/lib/api.ts index 0be7376..a8b111a 100644 --- a/dashboard/src/lib/api.ts +++ b/dashboard/src/lib/api.ts @@ -100,6 +100,7 @@ export interface PatchAppInput { export type HostKind = 'any' | 'strict' | 'wildcard'; export type PathKind = 'exact' | 'prefix' | 'param'; +export type DispatchMode = 'sync' | 'async'; export interface Route { id: string; @@ -111,6 +112,7 @@ export interface Route { path_kind: PathKind; path: string; method: string | null; + dispatch_mode: DispatchMode; created_at: string; } @@ -121,6 +123,7 @@ export interface RouteInput { path_kind: PathKind; path: string; method?: string | null; + dispatch_mode?: DispatchMode; } export interface CheckRouteResponse { @@ -461,6 +464,27 @@ export class ApiError extends Error { } } +// F-T-003: if the server keeps returning 401 (e.g. the bounce to +// /login itself triggered another 401-emitting request), the previous +// implementation would just keep calling `goto(login)` — a tight loop. +// Track consecutive 401s; after the threshold within the window, hard- +// reload to a static auth-loop page so the user sees something other +// than a spinning dashboard. Any 2xx response resets the count. +let consecutive401Count = 0; +let first401At = 0; +const AUTH_LOOP_THRESHOLD = 3; +const AUTH_LOOP_WINDOW_MS = 10_000; + +function loginUrlWithReturnTo(): string { + if (!browser) return `${base}/login`; + const here = `${window.location.pathname}${window.location.search}`; + // Don't loop returnTo back to /login itself. + if (here === `${base}/login` || here.startsWith(`${base}/login?`)) { + return `${base}/login`; + } + return `${base}/login?returnTo=${encodeURIComponent(here)}`; +} + async function adminRequest(path: string, init?: RequestInit): Promise { const headers: Record = { 'content-type': 'application/json', @@ -478,9 +502,27 @@ async function adminRequest(path: string, init?: RequestInit): Promise { // and bounce to login — unless we're already on it, in which // case throw and let the login form render the error. clearSession(); - if (browser && !window.location.pathname.endsWith('/login')) { - void goto(`${base}/login`); + const now = Date.now(); + if (now - first401At > AUTH_LOOP_WINDOW_MS) { + first401At = now; + consecutive401Count = 1; + } else { + consecutive401Count += 1; } + if (browser && !window.location.pathname.endsWith('/login')) { + if (consecutive401Count >= AUTH_LOOP_THRESHOLD) { + // Hard-reload via location.assign — Svelte's goto keeps + // the SPA state which is what's keeping the loop alive. + window.location.assign(`${base}/login?reason=auth-loop`); + } else { + void goto(loginUrlWithReturnTo()); + } + } + } else if (res.ok) { + // Any successful response resets the loop counter — the user has + // a valid session again. + consecutive401Count = 0; + first401At = 0; } if (!res.ok) { const message = @@ -908,8 +950,9 @@ export const api = { `/api/v1/admin/apps/${encodeURIComponent(idOrSlug)}/files/${encodeURIComponent(collection)}/${fileId}`, { method: 'DELETE' } ), - /// F-U-009: build the admin download URL. GET returns the file - /// bytes inline; the browser handles content-disposition. + // Build the admin download URL. GET returns the file bytes + // inline with Content-Disposition set from the stored filename; + // the browser handles the download. downloadUrl: (idOrSlug: string, collection: string, fileId: string): string => `/api/v1/admin/apps/${encodeURIComponent(idOrSlug)}/files/${encodeURIComponent(collection)}/${fileId}` }, diff --git a/dashboard/src/routes/+layout.svelte b/dashboard/src/routes/+layout.svelte index fb4f028..dcbd03a 100644 --- a/dashboard/src/routes/+layout.svelte +++ b/dashboard/src/routes/+layout.svelte @@ -14,6 +14,14 @@ const isLoginRoute = $derived(page.url.pathname.endsWith('/login')); + function loginRedirectTarget(): string { + const here = `${page.url.pathname}${page.url.search}`; + if (here === `${base}/login` || here.startsWith(`${base}/login?`)) { + return `${base}/login`; + } + return `${base}/login?returnTo=${encodeURIComponent(here)}`; + } + onMount(async () => { // Hydrate the session: if there's a token, ask the server who we // are. On 401 the fetch wrapper already redirects to /login and @@ -21,7 +29,7 @@ const tok = getToken(); if (!tok) { if (!isLoginRoute) { - await goto(`${base}/login`); + await goto(loginRedirectTarget()); } booting = false; return; diff --git a/dashboard/src/routes/login/+page.svelte b/dashboard/src/routes/login/+page.svelte index eefc0dc..0bc36a6 100644 --- a/dashboard/src/routes/login/+page.svelte +++ b/dashboard/src/routes/login/+page.svelte @@ -9,13 +9,37 @@ let password = $state(''); let pending = $state(false); let error = $state(null); + let notice = $state(null); + + // Honor returnTo from the auth bounce so deep links survive a + // re-login. Validate it's a same-origin admin path (no open + // redirect): must start with the admin base prefix, no scheme, no + // host. Falls back to the dashboard root. + function safeReturnTo(): string { + if (typeof window === 'undefined') return `${base}/`; + const params = new URLSearchParams(window.location.search); + const raw = params.get('returnTo'); + if (!raw) return `${base}/`; + if (raw.includes('://') || raw.startsWith('//')) return `${base}/`; + if (!raw.startsWith(`${base}/`) && !raw.startsWith('/')) return `${base}/`; + // Don't bounce back into /login. + if (raw === `${base}/login` || raw.startsWith(`${base}/login?`)) return `${base}/`; + return raw; + } onMount(async () => { + if (typeof window !== 'undefined') { + const reason = new URLSearchParams(window.location.search).get('reason'); + if (reason === 'auth-loop') { + notice = + 'Your session keeps being rejected. Sign in again — if this keeps happening, the admin token issuer may be misconfigured.'; + } + } // Already signed in? Skip the form. if (!getToken()) return; try { await api.auth.me(); - await goto(`${base}/`); + await goto(safeReturnTo()); } catch { // stale token; let the form render } @@ -27,7 +51,7 @@ pending = true; try { await api.auth.login(username, password); - await goto(`${base}/`); + await goto(safeReturnTo()); } catch (e) { error = e instanceof ApiError ? e.message : 'Login failed'; } finally { @@ -65,6 +89,9 @@ + {#if notice} +
{notice}
+ {/if} {#if error}
{error}
{/if} @@ -156,6 +183,15 @@ font-size: 0.85rem; } + .notice { + background: #422006; + border: 1px solid #b45309; + color: #fde68a; + padding: 0.5rem 0.75rem; + border-radius: 0.375rem; + font-size: 0.85rem; + } + .hint { margin: 0; font-size: 0.75rem; diff --git a/dashboard/src/routes/scripts/[id]/+page.svelte b/dashboard/src/routes/scripts/[id]/+page.svelte index 3b3a5f7..3f79a05 100644 --- a/dashboard/src/routes/scripts/[id]/+page.svelte +++ b/dashboard/src/routes/scripts/[id]/+page.svelte @@ -209,6 +209,7 @@ // canonical display form for an unrestricted host. let newRouteHost = $state('*'); let newRouteMethod = $state(''); + let newRouteDispatchMode = $state<'sync' | 'async'>('sync'); let pathKindAutoUpdate = $state(true); let creatingRoute = $state(false); let createRouteError = $state(null); @@ -255,13 +256,15 @@ host: parsedHost.host, path_kind: newRoutePathKind, path: newRoutePath.trim(), - method: newRouteMethod.trim() || null + method: newRouteMethod.trim() || null, + dispatch_mode: newRouteDispatchMode }; await api.routes.create(id, input); showAddRoute = false; newRoutePath = '/'; newRouteHost = '*'; newRouteMethod = ''; + newRouteDispatchMode = 'sync'; pathKindAutoUpdate = true; await loadRoutes(); } catch (e) { @@ -619,6 +622,13 @@ +