Skip to content

Commit 66a2a32

Browse files
lklimekclaude
andcommitted
refactor(secret): back Secret with upstream SecretString
`model::secret::Secret` owned its own memsec-backed guarded buffer — a near-duplicate of `platform_wallet_storage::secrets::SecretString`, which gained `replace_range` upstream and can now back it directly. Collapse `Secret` into a newtype over `SecretString`. Deleted, not kept alongside: `GuardedBuf` and its two `unsafe impl Send`/`Sync`, its `Drop`, `alloc_failed`, `DEFAULT_CAPACITY`, the hand-rolled `reserve`/`splice`/`as_bytes`/`from_plaintext` helpers, `with_capacity` (no callers), the hand-written `Deserialize`, and every `unsafe` block in the file — eleven of them, now zero. The direct `memsec` and `region` dependencies go with them; `region` and its transitive `mach2` leave the graph entirely, and `memsec` remains only where it belongs, under `platform-wallet-storage`. Each `TextBuffer` operation reduces to `SecretString::replace_range`. Char-index to byte-offset translation stays here, via egui's `byte_index_from_char_index`, which always yields a character boundary at or before the end of the plaintext — so no edit can trip the bounds assertions that `replace_range` applies where the old `splice` silently clamped. Three behavioral consequences, all favorable: - Equality goes through `subtle::ConstantTimeEq` rather than a raw `memsec::memeq` call. `subtle` still short-circuits on a length mismatch, so the length remains observable through timing exactly as before; what changes is that the comparison is no longer `unsafe`. - Deserialization forwards to `SecretString`'s visitor, which copies a borrowed `&str` straight into guarded memory instead of routing through a transient `String`, and refuses a value past the vault's `MAX_PASSPHRASE_LEN` before allocating. The one caller, `ui/masternodes/testnet_fixture.rs`, supplies WIFs far below it. - An empty `Secret` now holds no allocation at all, where it previously locked a 4 KiB page. The UI holds many empty ones. `subtle` becomes unconditional: `PartialEq` is always compiled, so gating it on `mcp` no longer works. `secret-serde` is enabled unconditionally on `platform-wallet-storage` and `secret-schemars` under `mcp`/`cli`, which are the only features that generate tool schemas. The `JsonSchema` impl delegates to `SecretString`'s, keeping the schema name `Secret` — and so its `schema_id` — unchanged for existing MCP clients. Tests keep their intent: editing, multi-byte UTF-8, growth, take, trimmed, equality, clone, and drop behavior all still assert observable `Secret` behavior, and two new cases cover clone independence and serde. Those testing the deleted internals go with them — the full-capacity wipe and the no-shared-page guarantee are no longer reachable through `Secret`'s API and are proven in `platform-wallet-storage`'s own suite. CLAUDE.md's `#[ignore]` example named the deleted read-after-free test; it now names the surviving one in `wallet_backend/payments.rs`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 129f378 commit 66a2a32

4 files changed

Lines changed: 118 additions & 401 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ Two gaps where CI will **not** cover you:
6969
- **Draft PRs run no automatic CI.** Both workflows are gated on `github.event.pull_request.draft != true`, so a draft PR's `pull_request` runs are suppressed. Neither workflow declares a `workflow_dispatch` trigger, so there is no way to run them by hand against a draft branch — mark the PR ready for review (`ready_for_review` triggers the full run) to get CI coverage.
7070
- **Backend E2E tests are not in CI.** The step is commented out in `tests.yml`, and the tests are `#[ignore]`d. If a change touches backend behaviour that only `tests/backend-e2e/` covers, run those locally; CI will not.
7171

72-
A green CI run is only meaningful if it actually executed your tests. `cargo test <filter>` exits 0 and prints `test result: ok` even when the filter matches nothing — when checking a run, confirm your new test names appear in the log **with a pass status**, not merely present. A `#[ignore]`d test (e.g. `test_drop_zeroes_full_capacity` in `src/model/secret.rs`) can appear in the log as `ignored` without having actually run — the "full non-ignored-test gate" above intentionally excludes these; they stay a manual check.
72+
A green CI run is only meaningful if it actually executed your tests. `cargo test <filter>` exits 0 and prints `test result: ok` even when the filter matches nothing — when checking a run, confirm your new test names appear in the log **with a pass status**, not merely present. A `#[ignore]`d test (e.g. `core_max_send_with_single_utxo_builds_without_change` in `src/wallet_backend/payments.rs`) can appear in the log as `ignored` without having actually run — the "full non-ignored-test gate" above intentionally excludes these; they stay a manual check.
7373

7474
### User stories catalog
7575

Cargo.lock

Lines changed: 1 addition & 23 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,13 @@ platform-wallet = { git = "https://github.com/dashpay/platform", rev = "67d4ef3f
3535
"serde",
3636
"shielded",
3737
] }
38+
# `secret-serde` backs `model::secret::Secret`'s `Deserialize`: the upstream
39+
# visitor copies a borrowed `&str` straight into guarded memory instead of
40+
# routing through a transient `String`. `secret-schemars` is enabled by the
41+
# `mcp`/`cli` features, which are the only ones that generate tool schemas.
3842
platform-wallet-storage = { git = "https://github.com/dashpay/platform", rev = "67d4ef3f6340a1e983229b6870ef60cf7573602a", features = [
3943
"shielded",
44+
"secret-serde",
4045
] }
4146
zip32 = "0.2.0"
4247
grovestark = { git = "https://www.github.com/dashpay/grovestark", rev = "5b9e289cca54c79b1305d5f4f40bf1148f1eb0e3" }
@@ -100,14 +105,11 @@ tz-rs = { version = "0.7.0" }
100105
tempfile = "3.20.0"
101106
arc-swap = "1"
102107
urlencoding = "2"
103-
# INTENTIONAL(memsec-exact-pin): guarded allocator for `model::secret::Secret`; it
104-
# holds passphrases and private keys, so a version change deserves a human read of
105-
# the diff rather than an automatic pickup. Pure Rust (libc / windows-sys), no C
106-
# toolchain or libsodium.
107-
memsec = "=0.7.0"
108108
rmcp = { version = "1.2", optional = true, default-features = false }
109109
axum = { version = "0.8", default-features = false, features = ["http1", "tokio", "json"], optional = true }
110-
subtle = { version = "2.6", optional = true }
110+
# Constant-time equality for `model::secret::Secret` (unconditional) and the
111+
# MCP API-key check. Not optional: `PartialEq for Secret` is always compiled.
112+
subtle = "2.6"
111113
clap = { version = "4", features = ["derive"], optional = true }
112114
clap_complete = { version = "4", optional = true }
113115

@@ -122,14 +124,11 @@ raw-cpuid = "11.5.0"
122124
default = []
123125
testing = []
124126
bench = []
125-
mcp = ["dep:rmcp", "rmcp/server", "rmcp/macros", "rmcp/transport-streamable-http-server", "dep:axum", "dep:subtle"]
126-
cli = ["dep:rmcp", "rmcp/server", "rmcp/macros", "rmcp/client", "rmcp/transport-io", "rmcp/transport-streamable-http-client-reqwest", "dep:clap", "dep:clap_complete"]
127+
mcp = ["dep:rmcp", "rmcp/server", "rmcp/macros", "rmcp/transport-streamable-http-server", "dep:axum", "platform-wallet-storage/secret-schemars"]
128+
cli = ["dep:rmcp", "rmcp/server", "rmcp/macros", "rmcp/client", "rmcp/transport-io", "rmcp/transport-streamable-http-client-reqwest", "dep:clap", "dep:clap_complete", "platform-wallet-storage/secret-schemars"]
127129
headless = ["cli", "mcp"]
128130

129131
[dev-dependencies]
130-
# Page-size query for the `Secret` page-isolation test only; `memsec` owns the
131-
# locking itself, so nothing in the shipped binary needs this.
132-
region = "3.0.2"
133132
egui_kittest = { version = "0.35.0", features = ["eframe"] }
134133
tokio-shared-rt = "=0.1.0"
135134
criterion = "0.5.1"

0 commit comments

Comments
 (0)