From 05ed9b00bba0e594593fd311339a76b7d4eb2236 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Wed, 10 Jun 2026 21:50:58 +0200 Subject: [PATCH] fix(stage-6): dashboard hardening + audit Lows cherry-pick MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes 4 dashboard hardening findings and 5 of the Lows from the audit. Dashboard hardening: - Subtabs no longer re-fetch the app via api.apps.get on every page load. users/files/dead-letters drop the fetch outright (the variable was set but never read); queues + queues/[name] now consume the layout's AppContext via getContext for the page title. The layout's reloadApp() owns the historical-slug redirect — subtab-local redirect blocks are removed so there's no race. - The global :global(details > summary::before) chevron is now scoped to details.chevron. The script editor's "Advanced sandbox" details and the inbound-email-shape help-text both opt in; the script exec-list logs no longer inherit a spurious chevron. - deriveTab now matches the path segment by anchored ===, so a future /apps//queues-archived route wouldn't activate the queues tab. Lows cherry-pick: - ExecError gains Serialize/Deserialize derives + a snake_case tag so RemoteExecutorClient (cluster mode v1.3+) can round-trip the variant. - triggers_api rejects queue triggers whose visibility_timeout_secs is below the dispatcher's per-message executor budget; with no minimum the reclaim task races the handler and the queue silently double-delivers. Existing test using 5s updated to 30s. - New migration 0040: execution_logs.script_id cascade switched from ON DELETE CASCADE to ON DELETE SET NULL so deleting a script no longer wipes the forensic history that motivated the delete. - New migration 0041: dead_letters composite index on (app_id, created_at DESC) so the "list all" dashboard view stops falling back to seqscan + sort when unresolved=false. - Schema snapshot re-blessed. Deferred to v1.2: the ExecRequest principal serde(skip) marker (documented in-place; the cluster-mode PR will introduce the wire-safe snapshot at that point) and the `pic --help` mention of `picloud admin reset-password` (one-line follow-up). Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/executor-core/src/types.rs | 7 +++- .../0040_execution_logs_keep_history.sql | 18 ++++++++++ .../0041_dead_letters_composite_idx.sql | 12 +++++++ crates/manager-core/src/triggers_api.rs | 21 ++++++++++++ crates/manager-core/tests/expected_schema.txt | 7 ++-- crates/picloud/tests/queue_e2e.rs | 5 +-- dashboard/src/routes/+layout.svelte | 14 +++++--- .../src/routes/apps/[slug]/+layout.svelte | 23 ++++++++++--- dashboard/src/routes/apps/[slug]/+page.svelte | 2 +- .../apps/[slug]/dead-letters/+page.svelte | 14 +++----- .../src/routes/apps/[slug]/files/+page.svelte | 18 +--------- .../routes/apps/[slug]/queues/+page.svelte | 33 ++++++------------- .../apps/[slug]/queues/[name]/+page.svelte | 22 +++++-------- .../src/routes/apps/[slug]/users/+page.svelte | 18 +--------- .../src/routes/scripts/[id]/+page.svelte | 2 +- 15 files changed, 119 insertions(+), 97 deletions(-) create mode 100644 crates/manager-core/migrations/0040_execution_logs_keep_history.sql create mode 100644 crates/manager-core/migrations/0041_dead_letters_composite_idx.sql diff --git a/crates/executor-core/src/types.rs b/crates/executor-core/src/types.rs index 62f9cd4..a6b84e7 100644 --- a/crates/executor-core/src/types.rs +++ b/crates/executor-core/src/types.rs @@ -129,7 +129,12 @@ pub struct ExecStats { pub operations: u64, } -#[derive(Debug, Error)] +/// Serialize/Deserialize are derived for `RemoteExecutorClient` (cluster +/// mode v1.3+) — the executor returns this shape over the wire so the +/// orchestrator can reconstruct the variant rather than collapsing to a +/// generic string. Audit F-N-001 follow-up. +#[derive(Debug, Error, serde::Serialize, serde::Deserialize)] +#[serde(rename_all = "snake_case", tag = "kind")] pub enum ExecError { #[error("script failed to parse: {0}")] Parse(String), diff --git a/crates/manager-core/migrations/0040_execution_logs_keep_history.sql b/crates/manager-core/migrations/0040_execution_logs_keep_history.sql new file mode 100644 index 0000000..37e8ea8 --- /dev/null +++ b/crates/manager-core/migrations/0040_execution_logs_keep_history.sql @@ -0,0 +1,18 @@ +-- Audit Low finding: preserving forensic history. +-- +-- The `execution_logs.script_id` foreign key was created with ON DELETE +-- CASCADE in 0001_init. Deleting a script then wipes every log row that +-- ever referenced it — including the rows that captured the failure +-- that motivated the delete. Switch to ON DELETE SET NULL so the +-- forensic history survives and operators can still look up "what did +-- this dead script do before we removed it" by id. + +ALTER TABLE execution_logs + DROP CONSTRAINT IF EXISTS execution_logs_script_id_fkey; + +ALTER TABLE execution_logs + ALTER COLUMN script_id DROP NOT NULL; + +ALTER TABLE execution_logs + ADD CONSTRAINT execution_logs_script_id_fkey + FOREIGN KEY (script_id) REFERENCES scripts(id) ON DELETE SET NULL; diff --git a/crates/manager-core/migrations/0041_dead_letters_composite_idx.sql b/crates/manager-core/migrations/0041_dead_letters_composite_idx.sql new file mode 100644 index 0000000..7e875e7 --- /dev/null +++ b/crates/manager-core/migrations/0041_dead_letters_composite_idx.sql @@ -0,0 +1,12 @@ +-- Audit Low finding: speeding up the "list all DL for app" view. +-- +-- The dashboard's dead-letters tab issues +-- SELECT ... FROM dead_letters +-- WHERE app_id = $1 +-- ORDER BY created_at DESC LIMIT $2 +-- which falls back to a seq-scan + sort when the unresolved=false +-- filter is on (the partial idx_dead_letters_app_unresolved doesn't +-- cover resolved rows). This composite covers both modes. + +CREATE INDEX IF NOT EXISTS idx_dead_letters_app_created + ON dead_letters (app_id, created_at DESC); diff --git a/crates/manager-core/src/triggers_api.rs b/crates/manager-core/src/triggers_api.rs index cf07147..b2eaac2 100644 --- a/crates/manager-core/src/triggers_api.rs +++ b/crates/manager-core/src/triggers_api.rs @@ -34,6 +34,12 @@ use crate::trigger_repo::{ TriggerRepo, TriggerRepoError, }; +/// Minimum allowed queue visibility timeout. Anything shorter races the +/// dispatcher's per-message executor budget; reclaim would re-deliver +/// the message before the handler returned. Picked to outlast a slow +/// handler by a comfortable margin. +const MIN_QUEUE_VISIBILITY_TIMEOUT_SECS: u32 = 30; + #[derive(Clone)] pub struct TriggersState { pub triggers: Arc, @@ -567,6 +573,21 @@ async fn create_queue_trigger( "queue_name must not be empty".into(), )); } + // Reject obviously-too-short visibility timeouts. The dispatcher + // budgets up to ~5 minutes of executor wall-clock per message + // (DEFAULT_ASYNC_EXEC_TIMEOUT in dispatcher.rs); a 10s visibility + // timeout would race the executor and cause the reclaim task to + // re-deliver the message before the first handler returned, so the + // queue would silently double-deliver. The audit's Low finding. + if let Some(secs) = input.visibility_timeout_secs { + if secs < MIN_QUEUE_VISIBILITY_TIMEOUT_SECS { + return Err(TriggersApiError::Invalid(format!( + "visibility_timeout_secs must be >= {MIN_QUEUE_VISIBILITY_TIMEOUT_SECS} \ + (shorter than the dispatcher's per-message executor budget; \ + reclaim would race the handler)" + ))); + } + } validate_trigger_target(&*s.scripts, app_id, input.script_id).await?; let req = crate::trigger_repo::CreateQueueTrigger { diff --git a/crates/manager-core/tests/expected_schema.txt b/crates/manager-core/tests/expected_schema.txt index 68bc180..283d80d 100644 --- a/crates/manager-core/tests/expected_schema.txt +++ b/crates/manager-core/tests/expected_schema.txt @@ -181,7 +181,7 @@ table: email_trigger_details table: execution_logs id: uuid NOT NULL default=gen_random_uuid() - script_id: uuid NOT NULL + script_id: uuid NULL request_id: uuid NOT NULL request_path: text NULL request_headers: jsonb NOT NULL default='{}'::jsonb @@ -403,6 +403,7 @@ indexes on dead_letter_trigger_details: indexes on dead_letters: dead_letters_pkey: public.dead_letters USING btree (id) + idx_dead_letters_app_created: public.dead_letters USING btree (app_id, created_at DESC) idx_dead_letters_app_unresolved: public.dead_letters USING btree (app_id) WHERE (resolved_at IS NULL) idx_dead_letters_gc: public.dead_letters USING btree (created_at) @@ -589,7 +590,7 @@ constraints on email_trigger_details: constraints on execution_logs: [CHECK] execution_logs_status_check: CHECK ((status = ANY (ARRAY['success'::text, 'error'::text, 'timeout'::text, 'budget_exceeded'::text]))) [FOREIGN KEY] execution_logs_app_id_fk: FOREIGN KEY (app_id) REFERENCES apps(id) ON DELETE CASCADE - [FOREIGN KEY] execution_logs_script_id_fkey: FOREIGN KEY (script_id) REFERENCES scripts(id) ON DELETE CASCADE + [FOREIGN KEY] execution_logs_script_id_fkey: FOREIGN KEY (script_id) REFERENCES scripts(id) ON DELETE SET NULL [PRIMARY KEY] execution_logs_pkey: PRIMARY KEY (id) constraints on files: @@ -705,3 +706,5 @@ constraints on triggers: 0037: drop idx cron triggers due 0038: email realtime secret check 0039: app user invitations unique pending + 0040: execution logs keep history + 0041: dead letters composite idx diff --git a/crates/picloud/tests/queue_e2e.rs b/crates/picloud/tests/queue_e2e.rs index f57837b..90d9547 100644 --- a/crates/picloud/tests/queue_e2e.rs +++ b/crates/picloud/tests/queue_e2e.rs @@ -350,13 +350,14 @@ async fn queue_visibility_timeout_reclaim() { let (server, app_id) = server_for(pool.clone(), "vt").await; let script_id = create_script(&server, &app_id, "slow", ACK_HANDLER).await; - // Minimum visibility timeout per the repo's validation. + // Use the API's minimum visibility timeout (30s). The test below + // forces a stale claim that's 60s old, so it's reclaimable regardless. let resp = server .post(&format!("/api/v1/admin/apps/{app_id}/triggers/queue")) .json(&json!({ "script_id": script_id, "queue_name": "vt_queue", - "visibility_timeout_secs": 5 + "visibility_timeout_secs": 30 })) .await; resp.assert_status(axum::http::StatusCode::CREATED); diff --git a/dashboard/src/routes/+layout.svelte b/dashboard/src/routes/+layout.svelte index dcbd03a..7f1905f 100644 --- a/dashboard/src/routes/+layout.svelte +++ b/dashboard/src/routes/+layout.svelte @@ -231,22 +231,26 @@ cursor: pointer; } /*
default triangle replaced with a slate chevron so the - disclosure widget tracks the rest of the dark theme. */ - :global(details > summary) { + disclosure widget tracks the rest of the dark theme. Audit fix: + scoped to `details.chevron` so the chevron doesn't leak into + contexts where the summary is doing layout work of its own (e.g. + the script exec-list, whose summary has its own status badge, + time, and path columns — a chevron there reads as visual noise). */ + :global(details.chevron > summary) { list-style: none; cursor: pointer; } - :global(details > summary::-webkit-details-marker) { + :global(details.chevron > summary::-webkit-details-marker) { display: none; } - :global(details > summary::before) { + :global(details.chevron > summary::before) { content: '▸'; display: inline-block; margin-right: 0.4rem; color: var(--text-muted); transition: transform 120ms ease; } - :global(details[open] > summary::before) { + :global(details.chevron[open] > summary::before) { transform: rotate(90deg); } diff --git a/dashboard/src/routes/apps/[slug]/+layout.svelte b/dashboard/src/routes/apps/[slug]/+layout.svelte index a4ceb4f..a89fa41 100644 --- a/dashboard/src/routes/apps/[slug]/+layout.svelte +++ b/dashboard/src/routes/apps/[slug]/+layout.svelte @@ -57,11 +57,24 @@ if (pathname.endsWith(`/apps/${slug}`) || pathname.endsWith(`/apps/${slug}/`)) { return qpTab ?? 'scripts'; } - if (pathname.includes(`/apps/${slug}/users`)) return 'users'; - if (pathname.includes(`/apps/${slug}/files`)) return 'files'; - if (pathname.includes(`/apps/${slug}/queues`)) return 'queues'; - if (pathname.includes(`/apps/${slug}/dead-letters`)) return 'dead-letters'; - return 'scripts'; + // Anchored matches: only the named subtab *exactly* (followed by + // `/` or end-of-string), so a hypothetical `/apps//queues-archived` + // route doesn't activate the `queues` tab. Audit hardening note. + const root = `/apps/${slug}/`; + const after = pathname.startsWith(root) ? pathname.slice(root.length) : ''; + const head = after.split('/', 1)[0]; + switch (head) { + case 'users': + return 'users'; + case 'files': + return 'files'; + case 'queues': + return 'queues'; + case 'dead-letters': + return 'dead-letters'; + default: + return 'scripts'; + } } async function reloadApp() { diff --git a/dashboard/src/routes/apps/[slug]/+page.svelte b/dashboard/src/routes/apps/[slug]/+page.svelte index 31ca398..52fe1a8 100644 --- a/dashboard/src/routes/apps/[slug]/+page.svelte +++ b/dashboard/src/routes/apps/[slug]/+page.svelte @@ -1281,7 +1281,7 @@ X-Picloud-Signature HMAC-SHA256 (hex of the request body); leave it blank to accept unsigned POSTs (URL secrecy only).

-
+
Expected inbound JSON shape 
{`{
   "from": "sender@external.com",
diff --git a/dashboard/src/routes/apps/[slug]/dead-letters/+page.svelte b/dashboard/src/routes/apps/[slug]/dead-letters/+page.svelte
index a02b0d2..12f6e39 100644
--- a/dashboard/src/routes/apps/[slug]/dead-letters/+page.svelte
+++ b/dashboard/src/routes/apps/[slug]/dead-letters/+page.svelte
@@ -1,11 +1,8 @@
 
 
 
-	Queues — {app?.name ?? slug} — PiCloud
+	Queues — {appName} — PiCloud
 
 
 

diff --git a/dashboard/src/routes/apps/[slug]/queues/[name]/+page.svelte b/dashboard/src/routes/apps/[slug]/queues/[name]/+page.svelte
index 6ad0438..642f307 100644
--- a/dashboard/src/routes/apps/[slug]/queues/[name]/+page.svelte
+++ b/dashboard/src/routes/apps/[slug]/queues/[name]/+page.svelte
@@ -1,29 +1,25 @@
 
 
 
-	{name} — Queues — {app?.name ?? slug} — PiCloud
+	{name} — Queues — {appName} — PiCloud
 
 
 

diff --git a/dashboard/src/routes/apps/[slug]/users/+page.svelte b/dashboard/src/routes/apps/[slug]/users/+page.svelte
index c084d46..869ea3c 100644
--- a/dashboard/src/routes/apps/[slug]/users/+page.svelte
+++ b/dashboard/src/routes/apps/[slug]/users/+page.svelte
@@ -1,17 +1,10 @@