From 98ab717043e3fdefac0934bf8f4621d523b15e9a Mon Sep 17 00:00:00 2001 From: Luca <102960244+Luca-Timo@users.noreply.github.com> Date: Tue, 23 Jun 2026 23:35:45 +0200 Subject: [PATCH] =?UTF-8?q?fix(workflows):=20close=20review=20blockers=20?= =?UTF-8?q?=E2=80=94=20prefetch-safe=20approvals=20+=20loud=20gate-edge=20?= =?UTF-8?q?failure?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Blocker #1: GET /workflow-approvals/:token/:action no longer mutates. Email clients + security scanners (Outlook Safe Links, Gmail, Proofpoint, AV link-checkers) GET links before the human clicks, which previously advanced a payment-confirm gate silently. GET now renders a confirm/deny interstitial via a new read-only peekApproval(); only POST calls actByToken. Blocker #2: a gate decision with no matching edge now failRun()s instead of finishRun(). resumeRun matches the decision handle EXACTLY (no fall-back to outEdge's sole-edge heuristic), so a 'deny' with only a 'confirm' edge fails loudly in run history instead of taking the confirm path / a green 'done'. --- backend/src/routes/publicWorkflowApprovals.js | 64 +++++++++++++++++-- backend/src/services/workflows/approvals.js | 16 ++++- backend/src/services/workflows/engine.js | 44 ++++++------- 3 files changed, 92 insertions(+), 32 deletions(-) diff --git a/backend/src/routes/publicWorkflowApprovals.js b/backend/src/routes/publicWorkflowApprovals.js index f9bbfd00..559619de 100644 --- a/backend/src/routes/publicWorkflowApprovals.js +++ b/backend/src/routes/publicWorkflowApprovals.js @@ -1,16 +1,19 @@ /** * Public workflow-approval endpoint — the confirm/deny links emailed to the * admin when a workflow gate is reached. Token is the single-use raw value - * (hashed at rest); the action resumes the run down the matching edge. + * (hashed at rest); acting resumes the run down the matching edge. * - * GET is used so the link is clickable from an email client. The token is - * single-use and the handler is idempotent (a second click shows "already - * recorded"), so prefetching can't double-act. + * Prefetch safety: GET is NEVER state-changing. Email clients + security + * scanners (Outlook Safe Links, Gmail, Proofpoint, AV link-checkers) GET email + * URLs before the human clicks — a GET that acted would silently advance a + * payment-confirm gate. So GET renders an interstitial with buttons that POST + * the decision; only POST calls actByToken. The token still gates everything + * (256-bit, single-use), and prefetchers don't POST. */ const express = require('express'); const router = express.Router(); -const { actByToken } = require('../services/workflows'); +const { actByToken, peekApproval } = require('../services/workflows'); function page(title, body) { return `` @@ -20,7 +23,56 @@ function page(title, body) { + `

${title}

${body}

`; } +// Escape any prompt text we echo into the interstitial HTML. +function esc(s) { + return String(s == null ? '' : s).replace(/[&<>"']/g, (c) => ( + { '&': '&', '<': '<', '>': '>', '"': '"', "'": ''' }[c] + )); +} + +function decisionPage(token, emphasis, prompt) { + const btn = (href, label, primary) => `
` + + `
`; + const body = (prompt ? `${esc(prompt)}` : '') + + `
` + + btn(`confirm`, 'Confirm payment received', emphasis === 'confirm') + + btn(`deny`, 'No payment received', emphasis === 'deny') + + `
` + + `

Choosing is a single, final action.

`; + return page('Confirm your response', body); +} + +// GET — render the interstitial. READ-ONLY: never mutates / resumes. router.get('/:token/:action', async (req, res) => { + const { token, action } = req.params; + if (!['confirm', 'deny'].includes(action)) { + return res.status(400).send(page('Invalid link', 'This confirmation link is not valid.')); + } + try { + const info = await peekApproval(token); + if (!info.found) { + return res.status(404).send(page('Link not found', 'This confirmation link is invalid or has been revoked.')); + } + if (info.status !== 'pending') { + return res.send(page('Already recorded', `This request was already ${esc(info.status)}.`)); + } + if (info.expired) { + return res.status(410).send(page('Link expired', 'This confirmation link has expired. Use the workflow inbox in the admin panel instead.')); + } + // Relative form actions resolve against the current path's directory; the + // emphasis just highlights the button matching the link they clicked. + return res.send(decisionPage(token, action, info.prompt)); + } catch (e) { + return res.status(500).send(page('Something went wrong', 'Please try again or use the admin panel.')); + } +}); + +// POST — the actual decision. Only a human (or an explicit form submit) reaches +// here; prefetchers issue GET, not POST. +router.post('/:token/:action', async (req, res) => { const { token, action } = req.params; if (!['confirm', 'deny'].includes(action)) { return res.status(400).send(page('Invalid link', 'This confirmation link is not valid.')); @@ -34,7 +86,7 @@ router.get('/:token/:action', async (req, res) => { return res.status(410).send(page('Link expired', 'This confirmation link has expired. Use the workflow inbox in the admin panel instead.')); } if (result.already) { - return res.send(page('Already recorded', `This request was already ${result.status}.`)); + return res.send(page('Already recorded', `This request was already ${esc(result.status)}.`)); } return res.send(page( 'Thank you', diff --git a/backend/src/services/workflows/approvals.js b/backend/src/services/workflows/approvals.js index 667478f9..aee175d7 100644 --- a/backend/src/services/workflows/approvals.js +++ b/backend/src/services/workflows/approvals.js @@ -102,6 +102,20 @@ async function actByToken(rawToken, decision) { return finalizeApproval(approval, decision, { acted_via: 'email' }); } +/** + * Read-only lookup for the emailed token — used to render the confirm/deny + * interstitial WITHOUT mutating state (so email-client prefetchers can't + * advance the gate). Never resumes the run. + */ +async function peekApproval(rawToken) { + const a = await db('workflow_approvals').where({ token_hash: hashToken(rawToken) }).first(); + if (!a) return { found: false }; + let prompt = null; + try { prompt = (JSON.parse(a.payload || '{}') || {}).prompt || null; } catch (_) { /* ignore */ } + const expired = !!(a.expires_at && new Date(a.expires_at).getTime() < Date.now()); + return { found: true, status: a.status, prompt, expired }; +} + /** Act on an approval from the admin webview inbox. */ async function actById(id, decision, adminId) { const approval = await db('workflow_approvals').where({ id }).first(); @@ -123,4 +137,4 @@ async function listPending(limit = 100) { .limit(limit); } -module.exports = { hashToken, createApproval, actByToken, actById, listPending }; +module.exports = { hashToken, createApproval, actByToken, actById, listPending, peekApproval }; diff --git a/backend/src/services/workflows/engine.js b/backend/src/services/workflows/engine.js index 0453c248..5f64939b 100644 --- a/backend/src/services/workflows/engine.js +++ b/backend/src/services/workflows/engine.js @@ -62,12 +62,14 @@ function matchFilter(filter, payload) { if (!filter || typeof filter !== 'object') return true; const { field, op = 'eq', value } = filter; const actual = payload ? payload[field] : undefined; + // Strict equality: a filter {value: 0} must NOT match false/''/null (loose == + // conflated them). Numeric payloads vs string config are normalised below. switch (op) { - case 'neq': return actual != value; // eslint-disable-line eqeqeq + case 'neq': return actual !== value; case 'truthy': return Boolean(actual); case 'falsy': return !actual; case 'eq': - default: return actual == value; // eslint-disable-line eqeqeq + default: return actual === value; } } @@ -228,7 +230,21 @@ async function resumeRun(runId, { decisionHandle = null } = {}) { const run = await db('workflow_runs').where({ id: runId }).first(); if (!run || run.status !== 'waiting') return; const { edges } = await loadGraph(run.workflow_id, run.version); - const e = outEdge(edges, run.current_node, decisionHandle); + // For a gate decision, the edge MUST match the handle exactly — we cannot fall + // back to outEdge's "sole edge" heuristic, or a 'deny' with only a 'confirm' + // edge would silently take the confirm path. A missing handle edge is a broken + // graph → fail loudly (same posture as unknown nodes) so the lost decision is + // visible in run history instead of masquerading as a green 'done'. + let e; + if (decisionHandle != null) { + e = edges.find((x) => x.from_node === run.current_node && (x.from_handle || null) === decisionHandle); + if (!e) { + await failRun(runId, `gate decision '${decisionHandle}' has no matching edge from node '${run.current_node}'`); + return; + } + } else { + e = outEdge(edges, run.current_node, null); + } const nextKey = e ? e.to_node : null; await db('workflow_runs').where({ id: runId }).update({ status: 'running', current_node: nextKey, wake_at: null, updated_at: db.fn.now() }); if (!nextKey) { await finishRun(runId); return; } @@ -405,27 +421,6 @@ async function isBuiltinFlowActive(builtinKey) { } } -/** - * True when the workflows flag is on AND a built-in flow with this key EXISTS - * (enabled or not). The legacy automations use this to decide whether the engine - * OWNS the automation — once the built-in is seeded, the engine is the single - * switch: the legacy path stands down whether the flow is enabled (the flow - * sends) or disabled (the admin turned it off → nothing sends). Distinct from - * isBuiltinFlowActive, which asks whether the flow is currently firing. Fails - * CLOSED (false) so the legacy path keeps running if the subsystem is down. - */ -async function isBuiltinFlowPresent(builtinKey) { - try { - const { isFeatureEnabled } = require('../../middleware/requireFeatureFlag'); - if (!(await isFeatureEnabled('workflows'))) return false; - if (!(await db.schema.hasTable('workflows'))) return false; - const wf = await db('workflows').where({ builtin_key: builtinKey }).first(); - return !!wf; - } catch (e) { - return false; - } -} - /** * Emit `event.date_approaching` for events entering an enabled flow's lead * window. This is the trigger source for the pre-event reminder built-in, so it @@ -550,7 +545,6 @@ async function testRun(workflowId, { entityType = null, entityId = null, payload module.exports = { emitWorkflowEvent, isBuiltinFlowActive, - isBuiltinFlowPresent, runDueWaits, emitDueEventReminders, recoverStaleRuns,