feat: keep a stored provider credential across an unrelated settings save - #1847
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (24)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe change adds a three-state provider bearer model. Settings saves can preserve a stored bearer without exposing it to JavaScript. The client validates provider identity, the WASM boundary carries keep intent, and the engine resolves or rejects that intent. ChangesStored bearer preservation
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant SettingsForm
participant ClientWorker
participant WasmByoIpfsConfig
participant Engine
participant StoredProvider
SettingsForm->>ClientWorker: save with KEEP_STORED_BEARER
ClientWorker->>WasmByoIpfsConfig: keepAccessToken = true
WasmByoIpfsConfig->>Engine: ByoBearer::Keep
Engine->>StoredProvider: resolve matching held bearer
StoredProvider-->>Engine: ByoBearer::Set
Engine-->>SettingsForm: publish result
Merge Risk: ⚪ Minimal · up to This change adds a way to preserve a stored provider credential during unrelated settings saves without ever exposing it to the browser. The credential-preservation logic, its rejection of contradictory or repointed keep requests, and its client/engine test coverage all appear sound based on the available evidence, and the one flagged documentation concern does not hold up under inspection. The change appears ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 55 functions across 23 files. (1 skipped: 1 too large.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
…save The vault settings record round-tripped every field but the provider bearer. The wasm boundary is write-only for that credential, so a read reports only that one is stored. A save publishes the record whole, which made a blank credential field mean no bearer and turned every unrelated settings change into a credential loss. The bearer is now three-state at every layer. ByoBearer::Keep is a save intent alone: it names no bytes, validate_byo_config refuses it on the encode path and on every request path, and Engine::save_vault_settings resolves it from the provider config the session placement already holds. A kept bearer is bound to the provider it was stored for. The save must name the same endpoint and the same kind, or it is refused. The keep intent crosses an untrusted realm boundary, so without that binding a host could carry a credential it cannot read on to an endpoint it chose and have the next placement present the member's bearer there. The wasm constructor refuses a keep intent that also carries bytes, and zeroizes those bytes before it refuses. The web settings form sends the keep intent when the credential field is untouched and the provider is unchanged, which replaces the refusal that asked the member to retype the credential for every save.
95dcb66 to
93d66d6
Compare
1 participant
The vault settings record round-tripped every field but the provider bearer. The wasm boundary is write-only for that credential, so a read reports only that one is stored, never its value. A save publishes the record whole, which made a blank credential field mean "no bearer" and turned every unrelated settings change into a credential loss. The fail-closed refusal that shipped with #1503 stopped the loss, at the cost of asking the member to retype the one field that is hardest to retype.
Closes #1535.
What changed
The bearer is three-state at every layer.
ByoBearerreplacesOption<Zeroizing<String>>onByoIpfsConfig:Nonestores no credential,Setcarries one, andKeepis a save intent that names no bytes.Engine::save_vault_settingsresolves aKeepfrom the provider config the session placement already holds, so the host still never sees the bearer.A kept bearer is bound to the provider it was stored for. The save must name the same endpoint and the same provider kind, or it is refused. This is the load-bearing part. The keep intent crosses an untrusted realm boundary, so without the binding a host could name an endpoint it controls, ask to keep a credential it cannot read, and have the next placement present the member's bearer to that endpoint. The web form applies the same rule before the member ever sees an error.
Fail-closed, release-active.
validate_byo_configis the one gate, and it refuses an unresolvedKeepon the seal path and on every request path. The decode path has no spelling for a keep, so the encode path returnsErrrather than asserting. Three verdicts reach the host asMalformedInputchecks:byo-credential-not-stored,byo-credential-repointed,byo-credential-unresolved.The wasm boundary refuses the contradiction.
ByoIpfsConfig::newtakes the keep flag beside the optional bytes and refuses both together, zeroizing the bytes before the refusal returns.The client and the form.
ByoIpfsConfigDescriptor.accessTokenbecomesArrayBuffer | 'keep' | null;commandTransferstill lists only a buffer, so the keep intent transfers nothing.settingsSaveVerdictnow answers with the keep decision instead of refusing a blank field, and the form copy says the credential is kept rather than asking for it again.Tests
crates/engine/tests/vault_settings.rs: a keeping save republishes the stored bearer and takes the new retention, read back off the sealed record from a second device; a keep with nothing held, a keep across a repointed endpoint, and a keep across a changed kind are each refused with nothing published.crates/engine/src/settings.rs: the seal path refuses an unresolved keep; the resolve takes, binds and refuses as stated; a supplied bearer is left alone.crates/wasm/tests/boundary.rs: a keep intent carrying its own bearer builds no config.packages/client: the codec spells a keep as the flag with no bearer bytes; the verdict keeps, clears, refuses a repointed endpoint and refuses a repointed kind.apps/web: the form keeps a stored credential through an unrelated edit and refuses to keep one on to a repointed provider.Suites run:
cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace,cargo check -p cipherbox-wasm --target wasm32-unknown-unknown,pnpm -r typecheck,pnpm lint,pnpm lint:tracker-refs, thepackages/clientsuite and theapps/websuite.Review gates
/simplify,/security-reviewand/crypto-privacy-reviewwere run inline againstgit diff main...HEAD. The crypto pass is the one this change was filed for: it re-opens the credential-redaction boundary two gates certified on #1503. It found no primitive, KDF edge or wire-format change, confirmed thatByoBearerprints only its variant name, thatVaultSettingsSummarystill drops the bearer at construction, and that no new path reads a stored bearer into JavaScript. The security pass produced the same-provider binding described above, which the issue body had placed in the form alone.Body checks / follow-ups filed
Body check 2026-09-14section records that the same-provider binding is an engine rule rather than only a form rule, that aPinMode::Hostedsession holds no bearer to keep and so refuses a keep fail-closed, and the three refusal check names.Summary by CodeRabbit
New Features
Bug Fixes
Note
Add
ByoBearerkeep-intent to preserve stored provider credential across unrelated vault settings savesByoIpfsConfigwith a three-stateByoBearerenum (None,Set,Keep), so a settings save can explicitly request retaining the session-held credential instead of re-sending itsettingsSaveVerdictin quota.ts now emits akeepStoredCredentialflag; it is true only when the provider endpoint and kind are unchanged and the form field is blank — repointing the provider or entering a new credential refuses the keepresolve_kept_bearerin settings.rs resolves aKeepintent against the held provider, requiring matching endpoint and kind and a real stored token; mismatches return dedicatedProviderErrorvariants (unresolved_credential,missing_stored_credential,repointed_credential)KEEP_STORED_BEARERsentinel through encoding, validation, and buffer scrubbing, keeping keep intents out of transferable-buffer validation and zeroization pathsByoIpfsConfig.access_tokenchanges fromOption<Zeroizing<String>>toByoBearer; any out-of-tree caller constructing or reading that field directly must migrate to the new enum and itstoken()accessorMacroscope summarized 95dcb66.