paul/picpeak
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fix(workflows): close review blockers — prefetch-safe approvals + loud gate-edge failure

Browse Source 
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'.
This commit is contained in:
Luca
2026-06-23 23:35:45 +02:00
parent d14f1d850c
commit 98ab717043
3 changed files with 92 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/src/routes/publicWorkflowApprovals.js
+58 -6
View File
@@ -1,16 +1,19 @@
/** /**
* Public workflow-approval endpoint — the confirm/deny links emailed to the * 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 * 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 * Prefetch safety: GET is NEVER state-changing. Email clients + security
* single-use and the handler is idempotent (a second click shows "already * scanners (Outlook Safe Links, Gmail, Proofpoint, AV link-checkers) GET email
* recorded"), so prefetching can't double-act. * 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 express = require('express');
const router = express.Router(); const router = express.Router();
const { actByToken } = require('../services/workflows'); const { actByToken, peekApproval } = require('../services/workflows');
function page(title, body) { function page(title, body) {
return `<!doctype html><html><head><meta charset="utf-8">` return `<!doctype html><html><head><meta charset="utf-8">`
@@ -20,7 +23,56 @@ function page(title, body) {
+ `<h2 style="font-weight:600">${title}</h2><p style="color:#4b5563;line-height:1.6">${body}</p></body></html>`; + `<h2 style="font-weight:600">${title}</h2><p style="color:#4b5563;line-height:1.6">${body}</p></body></html>`;
} }
// Escape any prompt text we echo into the interstitial HTML.
function esc(s) {
return String(s == null ? '' : s).replace(/[&<>"']/g, (c) => (
{ '&': '&amp;', '<': '&lt;', '>': '&gt;', '"': '&quot;', "'": '&#39;' }[c]
));
}
function decisionPage(token, emphasis, prompt) {
const btn = (href, label, primary) => `<form method="POST" action="${href}" style="display:inline">`
+ `<button type="submit" style="cursor:pointer;margin:6px;padding:12px 20px;border-radius:8px;border:1px solid #d1d5db;`
+ `font-size:15px;font-weight:600;${primary
? 'background:#1d9e75;color:#fff;border-color:#1d9e75'
: 'background:#fff;color:#374151'}">${label}</button></form>`;
const body = (prompt ? `<span style="display:block;margin-bottom:16px">${esc(prompt)}</span>` : '')
+ `<div>`
+ btn(`confirm`, 'Confirm payment received', emphasis === 'confirm')
+ btn(`deny`, 'No payment received', emphasis === 'deny')
+ `</div>`
+ `<p style="color:#9ca3af;font-size:13px;margin-top:20px">Choosing is a single, final action.</p>`;
return page('Confirm your response', body);
}
// GET — render the interstitial. READ-ONLY: never mutates / resumes.
router.get('/:token/:action', async (req, res) => { 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; const { token, action } = req.params;
if (!['confirm', 'deny'].includes(action)) { if (!['confirm', 'deny'].includes(action)) {
return res.status(400).send(page('Invalid link', 'This confirmation link is not valid.')); 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.')); 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) { 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( return res.send(page(
'Thank you', 'Thank you',
backend/src/services/workflows/approvals.js
+15 -1
View File
@@ -102,6 +102,20 @@ async function actByToken(rawToken, decision) {
return finalizeApproval(approval, decision, { acted_via: 'email' }); 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. */ /** Act on an approval from the admin webview inbox. */
async function actById(id, decision, adminId) { async function actById(id, decision, adminId) {
const approval = await db('workflow_approvals').where({ id }).first(); const approval = await db('workflow_approvals').where({ id }).first();
@@ -123,4 +137,4 @@ async function listPending(limit = 100) {
.limit(limit); .limit(limit);
} }
module.exports = { hashToken, createApproval, actByToken, actById, listPending }; module.exports = { hashToken, createApproval, actByToken, actById, listPending, peekApproval };
backend/src/services/workflows/engine.js
+19 -25
View File
@@ -62,12 +62,14 @@ function matchFilter(filter, payload) {
if (!filter || typeof filter !== 'object') return true; if (!filter || typeof filter !== 'object') return true;
const { field, op = 'eq', value } = filter; const { field, op = 'eq', value } = filter;
const actual = payload ? payload[field] : undefined; 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) { switch (op) {
case 'neq': return actual != value; // eslint-disable-line eqeqeq case 'neq': return actual !== value;
case 'truthy': return Boolean(actual); case 'truthy': return Boolean(actual);
case 'falsy': return !actual; case 'falsy': return !actual;
case 'eq': 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(); const run = await db('workflow_runs').where({ id: runId }).first();
if (!run || run.status !== 'waiting') return; if (!run || run.status !== 'waiting') return;
const { edges } = await loadGraph(run.workflow_id, run.version); 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; 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() }); 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; } 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 * 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 * 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 = { module.exports = {
emitWorkflowEvent, emitWorkflowEvent,
isBuiltinFlowActive, isBuiltinFlowActive,
isBuiltinFlowPresent,
runDueWaits, runDueWaits,
emitDueEventReminders, emitDueEventReminders,
recoverStaleRuns, recoverStaleRuns,