fix(consent): read the executor's answer to a decision, don't drop it - #112
Merged
Merged
Conversation
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 <glenn.g@affinidi.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The other half of OpenVTC/verifiable-trust-infrastructure#907 (merged). That fixed the VTA refusing an approver who held no ACL entry. This fixes the wallet throwing away the answer, which is why that took so long to find.
The bug
Sending a decision is not the end of the ceremony. The executor replies on the same DIDComm thread — a
task-consent/decision/0.1#responseon success, atrust-task-error/{0.1,0.2}on refusal — and nothing here recognised either shape. Both fell throughonInboundMessage's final "anything else is ignored" branch. No log, no surface, nothing.From the field, the entire wallet-side record of a refused approval was:
That second line is the refusal. It was logged as an arrival and then discarded unread.
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 dropped 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.
The fix
parseTaskConsentOutcome(core) reads the answer — both error versions and the success response, withstatus/approvals/needed/detailscarried through.codeis left opaque: 0.1 sayspermission_denied, 0.2 sayspermissionDenied, and normalising would invite a caller to branch on one casing.buildTaskConsentDecisionnow returns{ packed, id }. The id is thethidthe executor answers on, so the wallet can say which approval was refused rather than that one was. Both call sites (approver inbox, local relay) record it.notificationspermission was already declared and previously unused.What is trusted
Only the authcrypt sender, 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 that an approval failed — an invitation to approve a second time — nor that one succeeded. Nothing in the outcome grants anything; the executor's grant remains the authority, and this path is diagnostic.
Ordering
The outcome check runs before
parseTaskConsentRequest, because a reply rides the same envelope type and that parser can only classify it asnot-a-task-consent-request— the one reason a caller is allowed to ignore something, which is exactly how the reply vanished. A genuine consent request still falls through to the parser that prompts a human; there is a test for that.The awaiting-decision map
In-memory, bounded at 64, oldest-evicted. Deliberately not persisted: it explains an outcome, it never decides one, so an MV3 teardown losing it costs a good log line and not correctness. Persisting would add a write to the consent hot path and buy nothing. An answer that arrives with no local record is still reported, just without the "which decision" detail — there is a test for that too.
This is unrelated to the R1.6 persist-before-ack defect in CLAUDE.md, and does not touch it.
Tests
192 tests pass (11 new);
tsc -bclean across all four workspaces; build clean; the MV3 single-bundle / no-dynamic-import()invariant holds.New coverage: the permissionDenied refusal that actually occurred, the 0.1 error type,
detailspass-through, missingretryablereading as not-retryable, granted and partial-approval responses, sender not enrolled, sender unattributable, non-answers returningnullso other handlers still see them,thidfalling back to the documentthreadId, and an uncorrelated answer still being reported.