feat(custody): let an owner's own grant narrow their access level - #4417
Merged
Conversation
Until now the owner of a custody account always received WRITE, and the authorisation actually granted on that account was never consulted. That made one arrangement impossible to express: an authorisation in which the owner keeps inspection only and reserves acting for someone else. An active grant an owner holds on their own account now decides their level. Without such a grant nothing changes — the owner keeps full disposal, which is every account in production today. Managing grants stays tied to ownership rather than to the level, so an owner who narrows themselves can still hand the mandate back at any time and cannot lock themselves out. Reading is unaffected as well: a narrowed grant withdraws acting, not sight, and the holdings are the owner's either way. Adds the first test suite for CustodyAccountService, covering both paths and in particular that an inactive grant never narrows anything — deactivated history must not take part in authorisation.
Two review findings, both real. First, the narrowing was unreachable. Self-grants are refused and the owner's grant row could not be modified, so no API path led to the state the previous commit reacts to. An owner may now re-level their own grant — limiting themselves to inspection and taking the mandate back. Revoking that row stays refused: it would leave the account without an owner row and make the level unrecordable. Second, and worse: order creation never consulted the access level at all. It runs under the custody role and reaches the Safe on its own, so an owner limited to inspection could still trade. Hiding buttons in the frontend would have been decoration — the API accepted the order regardless. createOrder and confirmOrder now refuse when an own account is limited to inspection. Orders address a whole Safe rather than a single account, since balances and orders carry no account today. Any own account narrowed to READ therefore blocks acting: the order could touch exactly those holdings. Fail closed rather than guess which account an order belongs to. Verified against a running instance in both directions: with the narrowing in place the order is refused with 403, and with the mandate restored the very same request succeeds. Accounts without a narrowing grant — every account in production today — are unaffected.
…he order paths Two review findings. The acting check only looked at active accounts. Blocking or closing an account would therefore have lifted the restriction — exactly when caution matters most. Everywhere else a non-active account counts as absent and grants nothing; here absence would have granted something, namely the right to act. The status filter is gone and the test now insists a blocked account stays restricted. Not exploitable today, since no code path ever sets that status, but it would have been a trap for the first account-blocking feature. CustodyOrderService had no test suite either, so the two new call sites rested on manual verification alone. It now has one: both paths refuse when acting is narrowed, both pass the right identity rather than one of the two JWT ids, and a stranger is turned away on ownership before the narrowing check runs — so nobody can learn from the response whether an account is restricted. Also documents why the gap between check and write is left unlocked: only the owner manages grants and only the owner narrows themselves, so the sole party who could win that race is the one who may lift the restriction outright.
A narrowing blocks the owner's whole Safe and deliberately ignores account status. Grant management, however, went through requireOwner, which demanded an active account. Blocking a single account would therefore have stranded the grant on it: the owner could neither lift a narrowing they had placed there nor withdraw a stranger's access, and since one narrowing blocks every account they hold, a single block would have frozen the whole Safe with no way back. Grant management now depends on ownership alone. Blocking an account governs what may be done with it, not who decides that. Missing and foreign accounts still yield the same Forbidden, so existence stays unprobeable. Also corrects a comment that still described the status filter removed in the previous commit, and sorts an import.
It still claimed to be shared by checkAccess and requireOwner. Since the last commit requireOwner resolves the account itself, without the status filter, so that blocking an account cannot strand the grants on it. Only the data path goes through here now.
…tive Making grant management independent of account status went one step too far. It was meant to keep an owner from being stranded — able to lift a narrowing they placed on a blocked account, or withdraw a stranger's access. Issuing a new grant is neither. Widening the circle of authorised people during a hold is exactly what a hold is meant to prevent, and it is no way out of one. grantAccess now refuses on a non-active account, and does so before resolving the address, so the response cannot reveal whether a mail address is registered. Withdrawing and re-levelling stay open. grantAccess and getAccessList had no tests at all; both are covered now, including that an owner can still inspect grants on a blocked account. The blocked-account test for updateAccess used an unconditional stub that would have stayed green if a status filter crept back into requireOwner — it now evaluates the where clause, verified by reintroducing the filter and watching the test turn red.
…held Refusing new grants on a held account left the same door open one step over: raising an existing grant from inspection to acting widens someone's authority just as much, only through an existing row instead of a new one. It takes effect the moment the account is released, without anyone looking again. The rule is now uniform for a held account: rights may be taken away, not handed out. Lowering a stranger stays open, as does anything on the owner's own row, which is their way out of a narrowing. Uses BadRequestException rather than ConflictException, matching six existing places that refuse an action on an inactive or blocked resource; Conflict is reserved for duplicates.
❌ ESLint: 1 errors, 2 warnings |
Removes an import left unused when the exception type changed, uses the nullish operator the repo prefers, and records two assumptions that were only in my head: elevation is recognised by comparing the two levels that exist, so a third would have to turn it into an ordering comparison; and the status is read before the write, so whatever introduces a hold must deactivate that account's grants in the same change. Also covers granting write on a held account, not just read — the refusal must not depend on the level asked for.
Collaborator
Author
|
Seven review passes were needed to reach zero findings. Each fix exposed the next edge, so most of the passes found something the previous fix had opened:
Beyond CI, the behaviour was verified against a running instance in both directions — with the narrowing in place an order is refused, and with the mandate restored the identical request succeeds. Accounts without a narrowing grant, which is every account in production today, are unaffected. Neither |
TaprootFreak
marked this pull request as ready for review
July 28, 2026 02:30
This was referenced Jul 28, 2026
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.
What this changes
An access level on a custody account now decides two things it did not decide before.
The owner's own grant sets their level. The owner always received
WRITE, and the authorisation actually granted on the account was never consulted. An active grant an owner holds on their own account now decides it. Without such a grant nothing changes — the owner keeps full disposal, which is every account in production today.An owner may re-level their own grant. Previously no API path led to a narrowed state at all: self-grants are refused and the owner's grant row could not be modified. An owner can now limit themselves to inspection and take the mandate back. Revoking that row stays refused — it would leave the account without an owner row and make the level unrecordable.
Rights may be taken away while an account is held, not handed out. Grant management no longer depends on account status — otherwise blocking one account would strand the grants on it, and since a narrowing blocks the owner's whole Safe, a single block would freeze everything they hold with no way back. Issuing a new grant and raising a stranger from inspection to acting are refused on a held account, though: both widen someone's authority, which is what a hold is meant to prevent. Withdrawing and lowering stay open.
Order creation now consults the level. It never did. Orders run under the custody role and reach the Safe on their own, so an owner limited to inspection could still trade; hiding buttons in a frontend would have been decoration, because the API accepted the order regardless.
createOrderandconfirmOrdernow refuse when an own account is limited to inspection.Why
A signed authorisation can reserve acting for one person while the account holder only inspects. Today that arrangement cannot be expressed: whoever owns the account may act, whatever the mandate says.
Design notes
READtherefore blocks acting: the order could touch exactly those holdings. Fail closed rather than guess which account an order belongs to. This falls away once holdings carry an account.requireOwner), so a narrowed owner can hand the mandate back at any time.resolveOwnerAccountIdstill rightly skips the owner.active: truegrants count, unchanged.Tests
Neither service had a test suite. This adds both — 40 cases:
WRITE(the regression guard)READgrant is refusedWRITE, allowedREAD; withWRITEkeepsWRITEREADgrant on a foreign account does not block acting in one's own SafeupdateAccessandrevokeAccessVerification
Verified against a running instance in both directions, not only in CI:
The app was started with the change in place to confirm dependency injection resolves —
CustodyOrderServicegained a constructor dependency, and that layer has hidden a defect from static checks before.