From b69d017c5f67d2417a26c3353ecb71dc1e3563b4 Mon Sep 17 00:00:00 2001 From: Glenn Gore Date: Sun, 9 Aug 2026 08:28:39 +0200 Subject: [PATCH] fix(consent): read the executor's answer to a decision, don't drop it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sending a decision is not the end of the ceremony. The executor replies on the same DIDComm thread — a `decision/0.1#response` on success, a `trust-task-error/{0.1,0.2}` on refusal — and nothing here recognised either shape. Both fell through `onInboundMessage`'s final "anything else is ignored" branch: no log, no surface, nothing. So a refused approval was indistinguishable, from this side, from one that worked. The human was shown a change, agreed to it, the VTA said no, and the wallet discarded the reason — while the requester re-submitted forever and the operator had no way to tell which end was at fault. That is strictly worse than a lost prompt, because the person believes they have acted. `parseTaskConsentOutcome` reads the answer. `buildTaskConsentDecision` now returns the decision's document id alongside the packed message, so the caller can match the reply to the decision it sent and say *which* approval was refused rather than that one was. A refusal raises a notification, not just a console line: it contradicts something the user was shown seconds earlier and agreed to, so it has to reach them where they are. Only the authcrypt sender is trusted, and only to decide whether to believe the reply. An unenrolled sender is dropped — an unauthenticated party must not be able to tell this device an approval failed (an invitation to approve a second time) or that one succeeded. Nothing in the outcome grants anything; the executor's grant remains the authority. The awaiting-decision map is in-memory, bounded and best-effort by design. It explains an outcome, never decides one, so an MV3 teardown losing it costs a good log line and not correctness — persisting it would add a write to the consent hot path and buy nothing. `not-a-task-consent-request` remains the only reason a caller may ignore an inbound message, and the outcome check runs before the request parser so a reply cannot be mistaken for one. A consent request still falls through to the parser that prompts a human. Signed-off-by: Glenn Gore --- packages/core/src/inbound/task-consent.ts | 137 +++++++++++- .../tests/inbound.task-consent-outcome.mjs | 199 ++++++++++++++++++ packages/extension/src/offscreen.ts | 142 ++++++++++++- 3 files changed, 471 insertions(+), 7 deletions(-) create mode 100644 packages/core/tests/inbound.task-consent-outcome.mjs diff --git a/packages/core/src/inbound/task-consent.ts b/packages/core/src/inbound/task-consent.ts index 47cc8f0..e1124fa 100644 --- a/packages/core/src/inbound/task-consent.ts +++ b/packages/core/src/inbound/task-consent.ts @@ -39,7 +39,12 @@ import { packAuthcrypt, packAuthcryptJson, wrapForward, type Identity } from "../didcomm/index.js"; import type { RemoteDidcommEndpoint } from "../vta/didcomm.js"; -import { TRUST_TASK_ENVELOPE_TYPE, type TrustTask } from "../vta/protocol.js"; +import { + TRUST_TASK_ENVELOPE_TYPE, + isTrustTaskErrorType, + type TrustTask, + type TrustTaskErrorPayload, +} from "../vta/protocol.js"; import { signTrustTask } from "../trust-tasks/sign.js"; import { verifyTrustTaskProof } from "../trust-tasks/verify.js"; import type { SigningIdentity } from "../siop/self-issued.js"; @@ -51,6 +56,118 @@ export const TASK_CONSENT_DECISION_TYPE = /** VTA → requester: an approval landed and a grant is ready — re-submit now. */ export const TASK_CONSENT_GRANTED_TYPE = "https://trusttasks.org/spec/task-consent/granted/0.1"; +/** The executor's acknowledgement of a decision this device sent. */ +export const TASK_CONSENT_DECISION_RESPONSE_TYPE = `${TASK_CONSENT_DECISION_TYPE}#response`; + +/** + * What the executor did with a decision this device sent. + * + * `accepted: false` is the case that matters. A refusal means a human was + * shown a change, agreed to it, and the agreement did not take — which is + * strictly worse than a prompt that never arrived, because the person believes + * they have acted. It has to reach them. + */ +export type TaskConsentOutcome = + | { + accepted: true; + /** `granted` = threshold met, the requester can execute. `pending` = + * recorded, more approvals needed. `denied` = the request was aborted, + * which is a successful *outcome* of a `deny`, not a failure. */ + status: string; + approvals?: number; + needed?: number; + payloadDigest?: string; + /** The decision document id this answers, when the reply carried one. */ + thid?: string; + } + | { + accepted: false; + /** Framework status code — snake_case in error/0.1, lowerCamelCase in + * 0.2. Opaque: log it, don't branch on a casing. */ + code: string; + message?: string; + retryable: boolean; + details?: unknown; + thid?: string; + }; + +/** + * Parse the executor's reply to a `task-consent/decision` this device sent. + * + * Returns `null` for anything that is not such a reply — that is the only case + * a caller may ignore. + * + * ## Why this exists + * + * The executor answers a decision on the same DIDComm thread, as a Trust-Task + * envelope: a `decision/0.1#response` document on success, a + * `trust-task-error/{0.1,0.2}` on refusal. Nothing here recognised either, so + * both fell through the inbound handler's final "anything else is ignored" + * branch — no log, no surface, nothing. + * + * That is how an approval refused by the VTA looked identical, from this side, + * to one that was delivered and worked: the human approved, the wallet sent, + * the executor replied "no", and the wallet discarded the reply. The operator + * then watched the requester re-submit forever with no clue which end was at + * fault. Reading the answer is the difference between a two-minute diagnosis + * and an afternoon of packet-staring. + * + * ## What is trusted + * + * Only the authcrypt sender, and only to decide whether to *believe* the + * reply — it is diagnostic, and grants nothing. A reply whose sender is not an + * enrolled executor is dropped: an unauthenticated party must not be able to + * tell this device that its approval failed (a lie that invites the human to + * approve a second time), nor that it succeeded. + */ +export function parseTaskConsentOutcome( + message: Record, + opts: { enrolledExecutorDids: readonly string[] }, +): TaskConsentOutcome | null { + if (message.type !== TRUST_TASK_ENVELOPE_TYPE) return null; + + // A missing `from` means the transport could not authenticate the sender. + // Unlike the `granted` nudge — which is cross-checked against a digest the + // page already holds — nothing downstream re-verifies this, so an + // unattributable reply is dropped rather than believed. + const from = typeof message.from === "string" ? message.from : null; + if (!from || !opts.enrolledExecutorDids.includes(from)) return null; + + const doc = (message.body ?? {}) as Partial>>; + const thid = + (typeof message.thid === "string" ? message.thid : undefined) ?? + (typeof doc.threadId === "string" ? doc.threadId : undefined); + + if (isTrustTaskErrorType(doc.type)) { + const payload = (doc.payload ?? {}) as Partial; + return { + accepted: false, + code: typeof payload.code === "string" ? payload.code : "unknown", + ...(typeof payload.message === "string" ? { message: payload.message } : {}), + // The framework schema requires `retryable`; treat a missing one as + // "don't retry" rather than inventing optimism about a refusal. + retryable: payload.retryable === true, + ...(payload.details !== undefined ? { details: payload.details } : {}), + ...(thid ? { thid } : {}), + }; + } + + if (doc.type === TASK_CONSENT_DECISION_RESPONSE_TYPE) { + const payload = (doc.payload ?? {}) as Record; + return { + accepted: true, + status: typeof payload.status === "string" ? payload.status : "unknown", + ...(typeof payload.approvals === "number" ? { approvals: payload.approvals } : {}), + ...(typeof payload.needed === "number" ? { needed: payload.needed } : {}), + ...(typeof payload.payloadDigest === "string" + ? { payloadDigest: payload.payloadDigest } + : {}), + ...(thid ? { thid } : {}), + }; + } + + return null; +} /** * Parse a VTA→requester `task-consent/granted` notice. @@ -362,10 +479,23 @@ export async function buildTaskConsentDecisionDocument( return document; } +/** A `task-consent/decision` ready to send, and the id to recognise its + * answer by. */ +export interface BuiltTaskConsentDecision { + /** The packed, mediator-routed wire message. */ + packed: string; + /** The decision document's id. The executor answers on this thread + * (`thid`), so a caller that keeps it can match the reply to the decision + * it sent — and therefore tell the human *which* approval was refused. + * Returned rather than left inside the opaque packed blob because the + * alternative is not correlating at all, which is where this started. */ + id: string; +} + /** Build the authcrypted, mediator-routed `task-consent/decision` wire message. */ export async function buildTaskConsentDecision( args: BuildTaskConsentDecisionArgs, -): Promise { +): Promise { const document = await buildTaskConsentDecisionDocument(args); const message = { @@ -381,7 +511,8 @@ export async function buildTaskConsentDecision( { kid: args.vta.keyAgreementKid, jwk: args.vta.keyAgreementPublicJwk }, ]); const forwardJson = wrapForward(args.vta.did, args.holder.did, args.mediator.did, inner); - return packAuthcryptJson(forwardJson, args.holder, [ + const packed = await packAuthcryptJson(forwardJson, args.holder, [ { kid: args.mediator.keyAgreementKid, jwk: args.mediator.keyAgreementPublicJwk }, ]); + return { packed, id: document.id }; } diff --git a/packages/core/tests/inbound.task-consent-outcome.mjs b/packages/core/tests/inbound.task-consent-outcome.mjs new file mode 100644 index 0000000..5586c03 --- /dev/null +++ b/packages/core/tests/inbound.task-consent-outcome.mjs @@ -0,0 +1,199 @@ +// The executor's answer to a decision this device sent. +// +// A refusal is the worst inbound event in the ceremony: a human was shown a +// change, agreed to it, and the agreement did not take — so unlike a lost +// prompt, the person believes they have acted. The wallet used to drop that +// reply unread, because nothing recognised it and the inbound handler's final +// branch ignores what it cannot name. An approval the VTA rejected then looked, +// from this side, exactly like one that worked. +// +// These pin the two halves that matter: the answer is *read*, and it is read +// only when it comes from an executor this device is enrolled with. + +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { + parseTaskConsentOutcome, + TASK_CONSENT_DECISION_RESPONSE_TYPE, +} from "../dist/inbound/task-consent.js"; +import { + TRUST_TASK_ENVELOPE_TYPE, + TRUST_TASK_ERROR_TYPE, + TRUST_TASK_ERROR_TYPE_0_2, +} from "../dist/vta/protocol.js"; + +const VTA = "did:webvh:zScid:vta.example:glenn-vta"; +const OPTS = { enrolledExecutorDids: [VTA] }; +const THID = "urn:uuid:decision-1"; + +function envelope(body, overrides = {}) { + return { + id: "urn:uuid:reply-1", + type: TRUST_TASK_ENVELOPE_TYPE, + from: VTA, + to: ["did:key:zApprover"], + thid: THID, + body, + ...overrides, + }; +} + +function errorDoc(type, payload) { + return { id: "urn:uuid:err-1", type, threadId: THID, payload }; +} + +test("a permissionDenied refusal is read, not dropped", () => { + // Precisely the reply that went unread in the field: the transport gate + // refused the approver, and the wallet said nothing. + const outcome = parseTaskConsentOutcome( + envelope( + errorDoc(TRUST_TASK_ERROR_TYPE_0_2, { + code: "permissionDenied", + message: "DID not in ACL: did:key:zApprover", + retryable: false, + }), + ), + OPTS, + ); + assert.ok(outcome, "the refusal must be recognised"); + assert.equal(outcome.accepted, false); + assert.equal(outcome.code, "permissionDenied"); + assert.equal(outcome.retryable, false); + assert.match(outcome.message, /not in ACL/); + assert.equal(outcome.thid, THID, "correlates to the decision we sent"); +}); + +test("the 0.1 error type is read too, with its snake_case code left alone", () => { + // `code` is opaque: 0.1 says permission_denied, 0.2 says permissionDenied. + // Normalising here would invite a caller to branch on one casing. + const outcome = parseTaskConsentOutcome( + envelope( + errorDoc(TRUST_TASK_ERROR_TYPE, { code: "permission_denied", retryable: false }), + ), + OPTS, + ); + assert.equal(outcome.accepted, false); + assert.equal(outcome.code, "permission_denied"); +}); + +test("details ride through — that is where a task-specific reason lives", () => { + const outcome = parseTaskConsentOutcome( + envelope( + errorDoc(TRUST_TASK_ERROR_TYPE_0_2, { + code: "taskFailed", + retryable: false, + details: { payloadDigest: "abc123" }, + }), + ), + OPTS, + ); + assert.deepEqual(outcome.details, { payloadDigest: "abc123" }); +}); + +test("a missing retryable reads as not-retryable, never as optimism", () => { + const outcome = parseTaskConsentOutcome( + envelope(errorDoc(TRUST_TASK_ERROR_TYPE_0_2, { code: "internalError" })), + OPTS, + ); + assert.equal(outcome.retryable, false); +}); + +test("an accepted decision reports the status and the tally", () => { + const outcome = parseTaskConsentOutcome( + envelope({ + id: "urn:uuid:ok-1", + type: TASK_CONSENT_DECISION_RESPONSE_TYPE, + threadId: THID, + payload: { status: "granted", payloadDigest: "abc123", approvals: 1 }, + }), + OPTS, + ); + assert.equal(outcome.accepted, true); + assert.equal(outcome.status, "granted"); + assert.equal(outcome.approvals, 1); + assert.equal(outcome.payloadDigest, "abc123"); +}); + +test("a partial approval is accepted, and says how many more are needed", () => { + const outcome = parseTaskConsentOutcome( + envelope({ + id: "urn:uuid:ok-2", + type: TASK_CONSENT_DECISION_RESPONSE_TYPE, + threadId: THID, + payload: { status: "pending", payloadDigest: "abc123", approvals: 1, needed: 2 }, + }), + OPTS, + ); + assert.equal(outcome.accepted, true); + assert.equal(outcome.status, "pending"); + assert.equal(outcome.needed, 2); +}); + +test("a reply from anyone but an enrolled executor is not believed", () => { + // An unauthenticated party must not be able to tell this device its approval + // failed — that is an invitation to approve a second time — nor that one + // succeeded when it did not. + const outcome = parseTaskConsentOutcome( + envelope(errorDoc(TRUST_TASK_ERROR_TYPE_0_2, { code: "permissionDenied", retryable: false }), { + from: "did:key:zSomeoneElse", + }), + OPTS, + ); + assert.equal(outcome, null); +}); + +test("an unattributable reply is dropped", () => { + // No `from` means the transport could not authenticate the sender, and + // nothing downstream re-verifies this document. + const outcome = parseTaskConsentOutcome( + envelope(errorDoc(TRUST_TASK_ERROR_TYPE_0_2, { code: "permissionDenied", retryable: false }), { + from: undefined, + }), + OPTS, + ); + assert.equal(outcome, null); +}); + +test("anything that is not an answer returns null, so other handlers still see it", () => { + // The one case a caller may ignore. A consent *request* must fall through to + // the parser that prompts a human — returning an outcome here would swallow + // the prompt, which is the failure this whole module exists to prevent. + for (const body of [ + { id: "x", type: "https://trusttasks.org/spec/task-consent/request/0.1", payload: {} }, + { id: "x", type: "https://trusttasks.org/spec/vta/webvh/dids/update/1.0", payload: {} }, + {}, + ]) { + assert.equal(parseTaskConsentOutcome(envelope(body), OPTS), null); + } + assert.equal( + parseTaskConsentOutcome( + { type: "https://didcomm.org/messagepickup/3.0/status", from: VTA }, + OPTS, + ), + null, + ); +}); + +test("the thid falls back to the document threadId when the envelope omits it", () => { + const outcome = parseTaskConsentOutcome( + envelope(errorDoc(TRUST_TASK_ERROR_TYPE_0_2, { code: "permissionDenied", retryable: false }), { + thid: undefined, + }), + OPTS, + ); + assert.equal(outcome.thid, THID); +}); + +test("an answer with no correlation at all is still reported", () => { + // Losing the thread costs detail, never the report: a refusal the wallet + // cannot match to a specific decision is still a refusal the human needs. + const outcome = parseTaskConsentOutcome( + envelope({ id: "e", type: TRUST_TASK_ERROR_TYPE_0_2, payload: { code: "taskFailed", retryable: false } }, { + thid: undefined, + }), + OPTS, + ); + assert.equal(outcome.accepted, false); + assert.equal(outcome.thid, undefined); +}); diff --git a/packages/extension/src/offscreen.ts b/packages/extension/src/offscreen.ts index fdfb66c..1ab71db 100644 --- a/packages/extension/src/offscreen.ts +++ b/packages/extension/src/offscreen.ts @@ -29,6 +29,7 @@ import { parseTaskConsentGranted, requestTask, buildTaskConsentDecision, + parseTaskConsentOutcome, loadApproverIdentity, approverDid, TRUST_TASK_ENVELOPE_TYPE, @@ -602,6 +603,106 @@ async function doRequestTask(req: OffscreenRequestTaskRequest) { // not open two popups for the same change. const activeConsentDigests = new Set(); +// ── Decisions awaiting the executor's answer ───────────────────────────────── +// +// Keyed by the decision document's id, which is the `thid` the executor answers +// on. Sending a decision is not the end of the ceremony: the executor replies +// accepted-or-refused, and a refusal means a human agreed to a change that then +// did not happen. Without this the reply had nothing to match against and was +// dropped unread, so an approval the VTA rejected was indistinguishable here +// from one that worked. +// +// In-memory and best-effort by design. It exists to *explain* an outcome, never +// to decide one — the executor's grant is the authority, and nothing here is +// consulted for anything. So an MV3 teardown losing the map costs a good log +// line, not correctness; persisting it would buy nothing and add a write to the +// consent hot path. Bounded, because unbounded is how a long-lived offscreen +// document leaks. +const MAX_AWAITING_DECISIONS = 64; +interface AwaitingDecision { + payloadDigest: string; + decision: "approve" | "deny"; + taskType: string; + sentAt: number; +} +const awaitingDecisions = new Map(); + +function recordDecisionSent(id: string, entry: AwaitingDecision): void { + if (awaitingDecisions.size >= MAX_AWAITING_DECISIONS) { + // Oldest-first: `Map` preserves insertion order, and the executor answers + // in seconds, so anything at the head is long past being answered. + const oldest = awaitingDecisions.keys().next(); + if (!oldest.done) awaitingDecisions.delete(oldest.value); + } + awaitingDecisions.set(id, entry); +} + +/** Tell the human their approval did not take. A refusal is the one inbound + * event that contradicts something they were just shown and agreed to, so it + * gets a notification rather than a console line they will never read. */ +function notifyApprovalRefused(summary: string): void { + try { + chrome.notifications?.create({ + type: "basic", + iconUrl: chrome.runtime.getURL("icon-128.png"), + title: "Approval was not accepted", + message: summary, + priority: 2, + }); + } catch (e) { + // Notifications are a courtesy on top of the log, never the record of what + // happened — a browser that refuses one must not take the handler down. + console.warn("[pnm inbound] could not raise a refusal notification:", e); + } +} + +/** + * Handle the executor's answer to a decision this device sent. + * + * Returns `true` when the message was such an answer (and is now dealt with), + * so the caller stops treating it as anything else. + */ +async function handleTaskConsentOutcome( + vtaDid: string, + message: Record, +): Promise { + const outcome = parseTaskConsentOutcome(message, { + enrolledExecutorDids: await enrolledExecutorDids(vtaDid), + }); + if (!outcome) return false; + + // The decision this answers, when we still remember sending it. Absent after + // an MV3 teardown, or if the executor answered something we never sent — the + // outcome is still reported, just without the local detail. + const sent = outcome.thid ? awaitingDecisions.get(outcome.thid) : undefined; + if (outcome.thid) awaitingDecisions.delete(outcome.thid); + const what = sent + ? `${sent.decision} of ${sent.taskType} (digest ${sent.payloadDigest.slice(0, 12)}…)` + : `a decision this device sent (thid ${outcome.thid ?? "unknown"})`; + + if (outcome.accepted) { + console.info( + `[pnm inbound] task-consent decision accepted: ${outcome.status} — ${what}`, + outcome.approvals !== undefined + ? `approvals=${outcome.approvals}${outcome.needed !== undefined ? `/${outcome.needed}` : ""}` + : "", + ); + return true; + } + + console.error( + `[pnm inbound] task-consent decision REFUSED by the executor: ${what} — ` + + `code=${outcome.code} retryable=${outcome.retryable} ${outcome.message ?? ""}`, + outcome.details ?? "", + ); + notifyApprovalRefused( + sent + ? `The VTA refused your approval (${outcome.code}). The change has NOT been made.` + : `The VTA refused an approval from this device (${outcome.code}).`, + ); + return true; +} + // ─── Enrolled executors ─── // // Every approval request this wallet renders must be a Trust-Task document @@ -747,8 +848,18 @@ async function maybeRelayConsentLocally( payloadDigest: parsed.parsed.request.payloadDigest, thid: parsed.parsed.thid, }); - conn.send(outer); - console.info("[pnm consent relay] decision relayed over the worker session"); + conn.send(outer.packed); + recordDecisionSent(outer.id, { + payloadDigest: parsed.parsed.request.payloadDigest, + decision: "approve", + taskType: parsed.parsed.request.taskType, + sentAt: Date.now(), + }); + console.info( + "[pnm consent relay] decision relayed over the worker session; awaiting the", + "executor's answer on thid", + outer.id, + ); } finally { keepAlive.disconnect(); activeConsentDigests.delete(outcome.payloadDigest); @@ -1844,6 +1955,15 @@ async function dispatchInbound( return; } + // The executor's answer to a decision this device already sent — accepted, or + // refused with a reason. Checked before the request parser because it is a + // reply on the same envelope type, and `parseTaskConsentRequest` can only + // report it as `not-a-task-consent-request`, which is the one reason a caller + // is allowed to ignore. That is exactly how a refused approval used to vanish. + if (await handleTaskConsentOutcome(vtaDid, message)) { + return; + } + // Task-execution consent, first — it is the one inbound an *executor itself* // sends, and it is the one whose content a human will act on. // @@ -1963,8 +2083,22 @@ async function handleTaskConsent( payloadDigest: parsed.request.payloadDigest, thid: parsed.thid, }); - conn.send(outer); - console.info("[pnm inbound] task-consent decision sent:", decision); + conn.send(outer.packed); + // Remember what we sent, so the executor's answer can be matched to it. + // Sending is not the end of the ceremony — a refusal means the human agreed + // to a change that did not happen, and they have to be told which one. + recordDecisionSent(outer.id, { + payloadDigest: parsed.request.payloadDigest, + decision, + taskType: parsed.request.taskType, + sentAt: Date.now(), + }); + console.info( + "[pnm inbound] task-consent decision sent:", + decision, + "awaiting the executor's answer on thid", + outer.id, + ); } catch (e) { console.error("[pnm inbound] task-consent handling failed:", e); } finally {