feat(wallet): seal-regression doctest, bounded token rendering, and a compiler-checked settled rule - #13
Merged
Merged
Conversation
…ttled rule compiler-checked
Three defense-in-depth findings from the 0.11.0 security gate, which returned
after that PR had already merged.
The seal on UnknownPhaseToken was real but the suite could not see it removed:
making the field pub again left all 101 tests green, because they reach
Unrecognized only through From<&str> and the seal is what defines that route's
reachable set. A compile_fail doctest is the instrument that works, since
doctests compile as a separate crate and see the type as a consumer does.
Verified: making the field pub fails it with "Test compiled successfully, but
it's marked compile_fail".
Display now ESCAPES. A node emitting "\u{1b}[2K\rsynced" turned a consumer's
"unknown phase: {token}" log line into one reading "synced" -- the erase-line
and carriage-return wipe the prefix. Making the ergonomic path raw and the safe
path opt-in had it backwards. as_str() stays raw for relaying, which is the only
use that needs the exact bytes, and display_bounded() adds a length cap for a
log line since nothing bounds the token on the wire.
may_render_as_settled() states once, as an exhaustive match, which phases are a
complete picture. The rule was prose only, and it is one mistaken || away in
every consumer that writes it out -- exactly the second-implementation drift
this crate exists to prevent.
BREAKING CHANGE: UnknownPhaseToken's Display output is now escaped rather than
raw. Introduced in 0.11.0 and changed here because the raw form is a log- and
label-spoofing vector; as_str() provides the unescaped bytes.
Zero and one-byte bounds, multi-byte characters that cannot be split, and control characters whose escapes expand about sixfold. The bound is computed on escaped bytes, so expansion is what a naive implementation overshoots on, and slicing the raw string would panic on a char boundary in the multi-byte rows. Proven once by hand against an external consumer; pinned here so it stays true.
SPEC.md carried a LITERAL 0x1B beside an already-escaped backslash-r, so the paragraph warning about terminal-control injection performed that injection on anyone reading the normative spec through a terminal -- cat, head and gh pr diff all had the preceding line erased. Now consistent with the code's own example at src/results.rs:1111. The file ships to crates.io (no include/exclude in Cargo.toml) and is include_str!'d at src/kats.rs:664, so the raw byte was published. Refs #2609.
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.
DO NOT MERGE — gate round in progress.
Follow-up to #12. Closes the three defense-in-depth findings the 0.11.0 security gate raised, which
arrived after #12 had already been merged and published. None was a live defect, so 0.11.0 stands;
these are the hardening, and all three are additive.
Refs DIG-Network/dig_ecosystem#2609.
1. The seal was real, but nothing could see it removed
UnknownPhaseToken's private field is what makesUnrecognized("synced")unconstructible. The gatemeasured the guard on that guard and found it absent: flipping the field back to
pub Stringleftall 101 tests green, while the forged value became constructible again —
The unit tests are structurally incapable of catching it: they reach
Unrecognizedonly throughFrom<&str>, and the seal is precisely what determines that route's reachable set. A test writteninside the crate cannot observe a visibility boundary.
A
compile_faildoctest can, because doctests compile as a separate crate and therefore see thetype exactly as a consumer does. Falsification-tested both ways: with the field private the suite is
green; with it public the doctest fails with "Test compiled successfully, but it's marked
compile_fail".2.
Displayemitted raw control characters — a real log-spoofing vectorA node returning
phase = "\u{1b}[2K\rsynced"made a consumer'sprint a terminal line reading
synced—ESC[2Kerases the line and\rreturns the cursor, sothe prefix saying it was unknown is gone. A U+202E override does the same to a UI label. Both are
node-supplied, and this contract's entire subject is a consumer not asserting something false about a
wallet.
Displaynow escapes, viachar::escape_debug— the standard library's own escaper, coveringC0/C1 controls,
DEL, and the format characters carrying bidi overrides. A hand-rolled table would bea second implementation of a security-relevant rule, and would drift.
The previous shape had the ergonomics backwards: the path a log line naturally reaches for was the
unsafe one, and safety was opt-in. Now the default is safe and
as_str()is the single, clearlydocumented raw escape hatch, kept raw because a relay must hand on the exact bytes.
display_bounded(max)adds a length cap, since nothing bounds the token on the wire — the contract istransport-agnostic, and rejecting an over-long token would reintroduce the fail-closed parse #12
removed. So the bound belongs at the point of display, not at decode.
3. The settled rule was prose only
Two phases mean the sync is idle and only one is good news.
may_render_as_settled()states it once,as an exhaustive match so a future phase must be classified deliberately rather than defaulting:
syncedno_wallet_enrollednot_started,syncing,wallet_not_unlockedWithout it, every consumer writes the rule out and
wallet_not_unlockedis one mistaken||fromwearing a green tick. That is the second-implementation drift this crate exists to prevent, and
shipping the helper before dig-app writes that code is the point.
Also folded in the gate's nit: the coercion assertion now checks against every known token rather
than just
synced/syncing, sono_wallet_enrolled— equally an all-clear — is covered too.SemVer — 0.11.0 → 0.12.0
Three added methods (
may_render_as_settled,unrecognized_token_value,display_bounded) areadditive.
Display's output changes from raw to escaped, which is a behaviour break; it is calledout in the commit's
BREAKING CHANGEfooter.Displayshipped in 0.11.0 hours ago with no adopters,and its old behaviour is the spoofing vector above, so changing it now is strictly better than leaving
it and documenting around it. Minor bump on a 0.x crate.
0.11.0 is not withdrawn. It is correct and consumers may adopt it; 0.12.0 is a superset.
How verified
cargo fmtclean,cargo clippy --all-targets -- -D warningsclean, coverage 98.43% lines via the CI gate's own command.the field is made public; the settled table is pinned against literals so widening the predicate to
"any idle phase" fails.
ESC,CRor bidi override survives either display path, whilethe raw accessor still returns the bytes verbatim and the escaped form stays legible rather than
redacted.
Blast radius
WalletSyncPhase/WalletSyncStatusResult/UnknownPhaseTokenare consumed bydig-node/crates/dig-node-serviceanddig-app, both of which are mid-adoption of 0.11.0. Nothinghere removes or renames an existing item, so an adopter on 0.11.0 moves to 0.12.0 by changing the pin
alone — unless it already calls
UnknownPhaseToken'sDisplay, which now escapes.