Run the baseline pre-sign policy before the pre-approval short-circuit - #944
Conversation
Walkthrough
ChangesRaw signature policy
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review found the case that made the first version of this incomplete, and it is worth stating plainly because the shape recurs: a denylist on one string leaves the label that is not on the list. An empty label is not "raw", so the comparison misses it. Worse, the approval surface drops a blank label entirely, and the qualifier marking a label unverified goes with it. So the request carrying no stated domain at all rendered with fewer warnings than one that admitted to being raw. Refusing the honest version while passing the anonymous one is the wrong way round, and closing only half of it would have read as a fix. The same divergence covers "raw" followed by a zero-width character. The upstream comparison trims Unicode whitespace, which does not include zero-width space, while the prompt strips it; the label evades the check and still renders as "raw". Both cases come from the same root: the thing that decides and the thing that displays disagree about what the label says. The second check therefore normalises exactly the way the prompt does and refuses anything whose displayed label is blank or raw, so the property is stated in terms of what the user will actually see. Two corrections to my own framing, both from the review. A raw request did not previously sign silently: without a pre-approval it reached the prompt showing "claimed: raw", so the genuine gap was the pre-approved path and this is defense in depth rather than a silent-signing fix. And the comment claiming nothing legitimately sends the raw label to a phone was wrong: the CLI's Also worth recording, since it validates a point the review made rather than mine: falsifying the new guard reproduced the stall it predicted. With the check removed, those two tests fall through to the prompt and wait out the full sixty-second response timeout instead of failing. That is why the raw-without-pre-approval case is in the suite: it is not coverage for its own sake, it is the difference between a regression that fails and one that hangs. Filed rather than fixed here: refusals are not audited anywhere, so a phone owner has no durable record that a peer probed with raw requests, and the same applies to the kill switch. A pre-approval can also still be burned without a signature through paths that return early after the hook runs. Both are pre-existing and neither is made worse by this change. |
1 participant
Summary
Installing hooks replaces the set rather than composing with it, so this crate's hooks are the entire pre-sign policy on mobile and the ones the crate provides never run here. Composing is one call, which is how the bundled combination in the same module is written. This invokes it, and then closes the gap that invoking it alone leaves.
Worth being accurate about what was broken, because the obvious reading overstates it. A raw request did not previously sign silently: absent a pre-approval it reached the human prompt and rendered as
claimed: raw. The real gap was the pre-approved path, and the value here is defense in depth plus parity with the crate policy rather than closing a silent-signing hole.Placement is the part worth reviewing. The policy goes above the pre-approval short-circuit, for the same reason the kill switch already sits there: an approval recorded earlier must not authorise a request the policy refuses now. Below the short-circuit, exactly the requests that skip the prompt would also skip the policy, which is the wrong half to exempt. That ordering also turns out to matter for a second reason: consuming a pre-approval removes it, so a policy running afterwards would let a peer burn a user's approval with a request that was never going to be signed.
A denylist on one string is not enough on its own, and review found the case that shows why. An empty label is not "raw", so the comparison misses it, and the approval surface drops a blank label along with the qualifier that marks a label unverified. The request carrying no stated domain at all therefore rendered with fewer warnings than one that admitted to being raw: the most dangerous request was the least alarming. The same divergence covers a label of
rawfollowed by a zero-width character, which the comparison misses and the prompt renders asraw. So the second check refuses anything whose displayed label is blank or raw, normalising exactly the way the prompt does before deciding.Requiring a structured body on every request is the stronger policy and is deliberately not applied. The payload is optional at the request site, so mandating it would refuse initiators that do not attach one, and a co-signer that cannot sign reads as broken rather than as protected.
One behavior change for release notes: the CLI's
frost network signdoes send the raw label, so a phone in that group now refuses it. That is not fatal. The coordinator excludes the index and fails over, erroring only when the phone's share is needed to reach threshold. Mobile enforcing this unconditionally whileserve --refuse-raw-signstays opt-in is a deliberate asymmetry: a phone holding key material is the right place to be stricter than a server default.Test plan
Five tests. The first stages a raw request that is already pre-approved, which is the case that distinguishes the two placements, since a pre-approval records that the user accepted one specific set of bytes rather than consent to sign an unstructured request. Two cover the displayed-label guard: a label that renders blank, across empty, whitespace and zero-width inputs, and
rawspelled with a zero-width character. One covers a raw request with no pre-approval. The last checks a pre-approved request with an ordinary label still passes, so that refusing everything cannot satisfy any of the others.Falsified rather than assumed, twice. Moving the policy below the short-circuit makes the first fail while the last still passes. Removing the displayed-label guard makes both of its tests fail while the other three pass. That second run also showed why the not-pre-approved case earns its place: without the guard those tests fall through to the prompt and wait out the full sixty-second response timeout instead of failing, and a regression that stalls the suite is harder to read than one that fails.
Formatter and clippy clean.