feat(security): Improve secrets handling#264
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR implements memory-resident secret hygiene for Guardian by introducing type-safe secret wrapper types and systematically migrating all long-lived secret storage to use them. It includes four wrapper types with zeroize-on-drop semantics, digest-keyed session storage, credential URL wrapping, and comprehensive feature documentation. ChangesSecret Wrapper Implementation and Migrations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This PR spans multiple functional layers with significant logic density: the new wrapper module requires understanding of trait blocking and constant-time equality; session storage restructuring involves state machine changes across two independent flows; URL wrapping affects config/registry parsing and test updates; keystore zeroization touches buffer lifecycle management; and comprehensive specification artifacts require cross-referencing to validate consistency. The heterogeneous nature of changes across dashboard, EVM, storage, and keystore contexts increases reasoning overhead. Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Wraps long-lived secret values held in the Guardian server process in typed wrappers that zero on drop, refuse Display/Serialize, redact Debug, and compare in constant time. Restructures operator/EVM session maps to be keyed by sha256(token) instead of the plaintext token, and wraps the keystore disk-read buffers via Zeroizing.
Changes:
- New
crates/server/src/secret/module withFixedKey,SecretBytes,SecretString,CredentialUrlwrappers (built onsecrecy::SecretBox) and act::eqhelper; compile-timeassert_not_impl_any!/assert_impl_all!invariants. - Migrated in-scope sites:
CursorSecret,EvmChainConfig.rpc_url,StorageMetadataBuilder.database_url, AWS Secrets Manager transient hex/bytes, and session-token map storage (digest-keyed) in dashboard and EVM modules. crates/miden-keystorewraps file-read buffers withzeroize::Zeroizing<Vec<u8>>; signer call sites cite upstreamZeroizeOnDropverification.
Reviewed changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
speckit/features/008-zeroize-secrets/* |
Feature spec/plan/research/data-model/contracts/quickstart/tasks/checklist. |
crates/server/src/secret/{mod,wrappers,ct,tests}.rs |
New wrapper types, constant-time eq helper, compile-time + runtime tests. |
crates/server/Cargo.toml, Cargo.lock |
Adds secrecy 0.10 (no serde feature), subtle 2.5, zeroize 1.7, static_assertions 1.1 (dev). |
crates/server/src/lib.rs |
Registers pub(crate) mod secret;. |
crates/server/src/dashboard/{cursor,config,state}.rs |
CursorSecret now wraps FixedKey<32>; cursor secret parsed at config load (FR-012); operator sessions digest-keyed. |
crates/server/src/evm/{config,contracts,session}.rs |
rpc_url becomes CredentialUrl (now pub(crate)); EVM sessions digest-keyed. |
crates/server/src/builder/storage.rs, audit/postgres.rs |
DATABASE_URL wrapped at read; database_url builder field becomes Option<CredentialUrl>. |
crates/server/src/ack/secrets_manager.rs |
AWS Secrets Manager hex/bytes wrapped in SecretString/SecretBytes for the call frame. |
crates/server/src/ack/miden_falcon_rpo/signer.rs, ack/miden_ecdsa/signer.rs |
Add FR-011 zeroize verification comments. |
crates/miden-keystore/src/{keystore,ecdsa_keystore}.rs, Cargo.toml |
Disk-read buffers wrapped in zeroize::Zeroizing. |
CONTRIBUTING.md, docs/CONFIGURATION.md |
Reviewer checklist and env-var scope docs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/server/src/evm/session.rs (1)
141-184: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd coverage for the digest-backed session path.
verify,authenticate, andlogoutnow depend on hashing the plaintext cookie token before every session-map operation, but this module's tests still only cover challenge bookkeeping and cookie formatting. Please add a local regression test that issues a session, extracts the cookie token, authenticates with it, then logs out and verifies the token is rejected.As per coding guidelines,
**/*.{rs,ts,tsx}: Update tests in the layer where behavior changes, plus at least one upstream consumer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/server/src/evm/session.rs` around lines 141 - 184, Add a regression test that covers the digest-backed session path by creating a session via the same flow that returns a VerifiedEvmSession (capture the plaintext cookie token from session_cookie_header), then call authenticate(&token, now) to ensure it succeeds, call logout(Some(&token), now) and finally call authenticate(&token, now) again to assert it fails; target the module tests that exercise VerifiedEvmSession creation and the methods authenticate and logout (these reference session_cookie_header, session_digest, authenticate, logout, and VerifiedEvmSession) and also update at least one upstream consumer test to mirror the end-to-end behavior change so the hashed-key lookup is exercised by the suite.crates/server/src/ack/secrets_manager.rs (1)
43-84: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftAdd coverage for the new Secrets Manager parsing paths.
This change adds a new failure branch when AWS returns no
secret_stringand moves key decoding throughSecretString/SecretBytes, but this module still only exercises env-var helpers. Please add tests for the missing-string case and at least one upstream consumer path so the wrapper-based parsing contract does not regress.As per coding guidelines,
**/*.{rs,ts,tsx}: Update tests in the layer where behavior changes, plus at least one upstream consumer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/server/src/ack/secrets_manager.rs` around lines 43 - 84, The tests are missing coverage for the new Secrets Manager parsing branches introduced in secret_string and parsed_secret_key; add unit tests in the same crate that (1) mock the AWS client to return a response with no secret_string and assert secret_string(secret_id) returns the ConfigurationError with the expected message, and (2) mock a secret_string containing invalid hex and assert parsed_secret_key::<T, _>(secret_id, parser) surfaces the ConfigurationError from hex::decode, plus add an integration-style test that exercises an upstream consumer using SecretString/SecretBytes (call the real parsed_secret_key path with a small parser closure) to ensure the wrapper parsing contract is honored. Ensure tests reference the secret_string and parsed_secret_key functions and validate error messages mention the secret_id so regressions in the new branches are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/server/src/builder/storage.rs`:
- Around line 80-85: The env parsing currently treats whitespace-only
DATABASE_URL as set; update the closure in the .database_url(...) chain to
reject strings that are empty after trimming and to pass a trimmed String into
CredentialUrl::new (e.g. replace filter(|s| !s.is_empty()) with a trim-based
check and map to CredentialUrl::new using s.trim().to_owned()); apply the same
change to the other occurrence referenced (the similar filter/map at the later
lines) so whitespace-only values are normalized out at parse time.
In `@crates/server/src/dashboard/config.rs`:
- Around line 45-58: The env var GUARDIAN_DASHBOARD_CURSOR_SECRET is read into a
plain String before parsing, leaving the hex secret on the heap; change the code
in the initializer that calls CursorSecret::from_hex(&hex) to wrap the
environment value immediately in a zeroizing wrapper (e.g.,
zeroize::Zeroizing<String> or secrecy::SecretString) and pass a borrowed
&*wrapped value into CursorSecret::from_hex so the raw hex is scrubbed when
dropped; update the analogous handling at the other occurrence (lines ~73-75)
the same way, and add unit tests for Config: one test for successful parse
(ensuring take_cursor_secret() returns the secret once), one test for invalid
hex error, and one test verifying that take_cursor_secret() is one-shot
(subsequent calls return None or equivalent) to cover the behavior change.
In `@crates/server/src/secret/wrappers.rs`:
- Around line 140-145: The current scheme_and_host() implementation (called by
CredentialUrl::Debug) can leak secrets for malformed authorities (e.g.,
"postgres://alice:secret/db"); update scheme_and_host (and any helper
scheme_and_host(self.expose_secret())) to parse the URL with a proper URL
parser, extract scheme and host (and optional port) only when parsing succeeds
and a host is present, and otherwise return the literal "<invalid-url>"; ensure
the function never derives host by string-splitting the authority (which caused
the alice:secret leak) and add tests covering "postgres://alice:secret/db" and
other malformed inputs to verify no secret is exposed.
In `@speckit/features/008-zeroize-secrets/contracts/secret-module.md`:
- Around line 19-24: The documented module surface is inaccurate: the real
module declares private modules `ct` and `wrappers` and only exposes wrapper
types, and `ct::eq` is test-only (`ct_eq` behind #[cfg(test)]). Update the
contract block to show `ct` and `wrappers` as private (remove or mark them as
not re-exported), export only the wrapper types `FixedKey`, `SecretBytes`,
`SecretString`, and `CredentialUrl` as the public API, and mark `ct_eq` as
test-only (or omit it from the public contract) so the contract matches the
actual visibility in mod.rs.
In `@speckit/features/008-zeroize-secrets/data-model.md`:
- Around line 23-24: The documented API signatures for the constructors and
accessors (e.g., fn new(bytes: [u8; N]) -> Self and fn expose_secret(&self) ->
&[u8; N]) currently say `pub` but their implementations are `pub(crate)` in the
secret wrappers; update the data model docs to use `pub(crate)` for those
methods (including the other occurrences noted at lines 39-40, 55-57, 71-73) so
the documentation matches the actual visibility in the wrappers implementation.
In `@speckit/features/008-zeroize-secrets/plan.md`:
- Line 97: Update the plan wording in the feature plan to match the implemented
secrets manager design: change the reference in the sentence mentioning
secrets_manager.rs so it no longer states "fetched bytes returned as
SecretBytes; cache field (if present) holds SecretBytes" and instead describes
that fetched bytes are transiently wrapped (not stored) and the public API still
returns the parsed key type (no cache field). Locate the mention of SecretBytes
and cache in the plan and replace it with language that reflects transient
wrapping and preservation of the parsed-key return type, and explicitly note "no
cache" to avoid drift.
In `@speckit/features/008-zeroize-secrets/research.md`:
- Around line 157-158: The table in Decision 7 currently lists operator and EVM
session tokens as `SecretString`, which contradicts Decision 6's chosen shape
(digest-keyed maps with plaintext tokens not retained); update the table entries
so sites that store session tokens (e.g., operator session token, EVM session
token referenced by `EvmChainConfig.rpc_url` and any usage in
`StorageMetadataBuilder.database_url`) are described as storing only a token
digest/lookup key (e.g., `TokenDigest` or `DigestKey`) rather than
`SecretString`, and add a short note that the plaintext token is ephemeral and
not persisted per Decision 6.
---
Outside diff comments:
In `@crates/server/src/ack/secrets_manager.rs`:
- Around line 43-84: The tests are missing coverage for the new Secrets Manager
parsing branches introduced in secret_string and parsed_secret_key; add unit
tests in the same crate that (1) mock the AWS client to return a response with
no secret_string and assert secret_string(secret_id) returns the
ConfigurationError with the expected message, and (2) mock a secret_string
containing invalid hex and assert parsed_secret_key::<T, _>(secret_id, parser)
surfaces the ConfigurationError from hex::decode, plus add an integration-style
test that exercises an upstream consumer using SecretString/SecretBytes (call
the real parsed_secret_key path with a small parser closure) to ensure the
wrapper parsing contract is honored. Ensure tests reference the secret_string
and parsed_secret_key functions and validate error messages mention the
secret_id so regressions in the new branches are caught.
In `@crates/server/src/evm/session.rs`:
- Around line 141-184: Add a regression test that covers the digest-backed
session path by creating a session via the same flow that returns a
VerifiedEvmSession (capture the plaintext cookie token from
session_cookie_header), then call authenticate(&token, now) to ensure it
succeeds, call logout(Some(&token), now) and finally call authenticate(&token,
now) again to assert it fails; target the module tests that exercise
VerifiedEvmSession creation and the methods authenticate and logout (these
reference session_cookie_header, session_digest, authenticate, logout, and
VerifiedEvmSession) and also update at least one upstream consumer test to
mirror the end-to-end behavior change so the hashed-key lookup is exercised by
the suite.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b967e83d-3d9b-4a1a-91cb-897ba8cadd7a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
CONTRIBUTING.mdcrates/miden-keystore/Cargo.tomlcrates/miden-keystore/src/ecdsa_keystore.rscrates/miden-keystore/src/keystore.rscrates/server/Cargo.tomlcrates/server/src/ack/miden_ecdsa/signer.rscrates/server/src/ack/miden_falcon_rpo/signer.rscrates/server/src/ack/secrets_manager.rscrates/server/src/audit/postgres.rscrates/server/src/builder/storage.rscrates/server/src/dashboard/config.rscrates/server/src/dashboard/cursor.rscrates/server/src/dashboard/state.rscrates/server/src/evm/config.rscrates/server/src/evm/contracts.rscrates/server/src/evm/session.rscrates/server/src/lib.rscrates/server/src/secret/ct.rscrates/server/src/secret/mod.rscrates/server/src/secret/tests.rscrates/server/src/secret/wrappers.rsdocs/CONFIGURATION.mdspeckit/features/008-zeroize-secrets/checklists/requirements.mdspeckit/features/008-zeroize-secrets/contracts/secret-module.mdspeckit/features/008-zeroize-secrets/data-model.mdspeckit/features/008-zeroize-secrets/plan.mdspeckit/features/008-zeroize-secrets/quickstart.mdspeckit/features/008-zeroize-secrets/research.mdspeckit/features/008-zeroize-secrets/spec.mdspeckit/features/008-zeroize-secrets/tasks.md
| .database_url( | ||
| std::env::var("DATABASE_URL") | ||
| .ok() | ||
| .filter(|s| !s.is_empty()) | ||
| .map(CredentialUrl::new), | ||
| ) |
There was a problem hiding this comment.
Normalize whitespace-only DATABASE_URL values.
filter(|s| !s.is_empty()) and the later expose_secret().is_empty() check both treat " " as configured, so a blank-but-whitespace DATABASE_URL now gets through env parsing and only fails later in migrations/pool setup with a lower-level Postgres error instead of the intended configuration error.
Suggested fix
.database_url(
std::env::var("DATABASE_URL")
.ok()
+ .map(|s| s.trim().to_owned())
.filter(|s| !s.is_empty())
.map(CredentialUrl::new),
)
@@
let database_url = self
.database_url
- .filter(|url| !url.expose_secret().is_empty())
+ .filter(|url| !url.expose_secret().trim().is_empty())
.ok_or_else(|| "DATABASE_URL environment variable is required".to_string())?;Also applies to: 102-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/server/src/builder/storage.rs` around lines 80 - 85, The env parsing
currently treats whitespace-only DATABASE_URL as set; update the closure in the
.database_url(...) chain to reject strings that are empty after trimming and to
pass a trimmed String into CredentialUrl::new (e.g. replace filter(|s|
!s.is_empty()) with a trim-based check and map to CredentialUrl::new using
s.trim().to_owned()); apply the same change to the other occurrence referenced
(the similar filter/map at the later lines) so whitespace-only values are
normalized out at parse time.
| let cursor_secret = std::env::var("GUARDIAN_DASHBOARD_CURSOR_SECRET") | ||
| .ok() | ||
| .map(|hex| CursorSecret::from_hex(&hex)) | ||
| .transpose() | ||
| .map_err(|e| { | ||
| format!( | ||
| "GUARDIAN_DASHBOARD_CURSOR_SECRET must be 32 hex-encoded bytes (64 chars): {e}" | ||
| ) | ||
| })?; | ||
| Ok(Self { | ||
| environment: environment_for_network(network_type).to_string(), | ||
| cursor_secret, | ||
| ..Self::default() | ||
| }; | ||
| if let Ok(secret_hex) = std::env::var("GUARDIAN_DASHBOARD_CURSOR_SECRET") { | ||
| config.cursor_secret_hex = Some(secret_hex); | ||
| } | ||
| Ok(config) | ||
| }) |
There was a problem hiding this comment.
Wrap the env secret immediately, and pin this path with a unit test.
Line 45 still pulls GUARDIAN_DASHBOARD_CURSOR_SECRET into a plain String, so the hex secret can sit in heap memory un-scrubbed even though CursorSecret::from_hex() zeroizes the decoded bytes. Please wrap the env value in Zeroizing or SecretString before borrowing it, and add a small test around successful parse, invalid hex, and the one-shot take_cursor_secret() behavior.
🔐 Minimal fix sketch
+use zeroize::Zeroizing;
+
- let cursor_secret = std::env::var("GUARDIAN_DASHBOARD_CURSOR_SECRET")
+ let cursor_secret = std::env::var("GUARDIAN_DASHBOARD_CURSOR_SECRET")
.ok()
- .map(|hex| CursorSecret::from_hex(&hex))
+ .map(Zeroizing::new)
+ .map(|hex| CursorSecret::from_hex(hex.as_str()))
.transpose()
.map_err(|e| {
format!(As per coding guidelines, "Update tests in the layer where behavior changes, plus at least one upstream consumer".
Also applies to: 73-75
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/server/src/dashboard/config.rs` around lines 45 - 58, The env var
GUARDIAN_DASHBOARD_CURSOR_SECRET is read into a plain String before parsing,
leaving the hex secret on the heap; change the code in the initializer that
calls CursorSecret::from_hex(&hex) to wrap the environment value immediately in
a zeroizing wrapper (e.g., zeroize::Zeroizing<String> or secrecy::SecretString)
and pass a borrowed &*wrapped value into CursorSecret::from_hex so the raw hex
is scrubbed when dropped; update the analogous handling at the other occurrence
(lines ~73-75) the same way, and add unit tests for Config: one test for
successful parse (ensuring take_cursor_secret() returns the secret once), one
test for invalid hex error, and one test verifying that take_cursor_secret() is
one-shot (subsequent calls return None or equivalent) to cover the behavior
change.
| /// Returns `scheme://host[:port]` with userinfo, path, and query stripped. | ||
| /// Safe to log. Returns `<invalid-url>` if parsing fails. | ||
| pub(crate) fn scheme_and_host(&self) -> String { | ||
| scheme_and_host(self.expose_secret()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the implementation and nearby context
sed -n '1,260p' crates/server/src/secret/wrappers.rs | nl -ba | sed -n '1,260p'
# Narrow searches for scheme_and_host and its callers
rg -n "scheme_and_host\\b" crates/server/src/secret/wrappers.rs crates/server/src -S || true
rg -n "CredentialUrl|Debug.*CredentialUrl|impl\\s+Debug\\s+for\\s+.*CredentialUrl" crates/server/src -S || true
# Also check Cargo.toml for url dependency (to see if url::Url is already available)
ls -la crates/server
rg -n "url\\s*=|name\\s*=\\s*\"url\"" -S crates/server/Cargo.toml Cargo.toml || trueRepository: OpenZeppelin/guardian
Length of output: 108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the implementation and nearby context (avoid nl)
sed -n '110,220p' crates/server/src/secret/wrappers.rs | cat -n
# Narrow searches for scheme_and_host and its callers
rg -n "scheme_and_host\\b" crates/server/src/secret/wrappers.rs crates/server/src -S || true
rg -n "CredentialUrl|Debug.*CredentialUrl|impl\\s+Debug\\s+for\\s+.*CredentialUrl" crates/server/src -S || true
# Check if `url` crate is already a dependency
rg -n '^[[:space:]]*url[[:space:]]*=' Cargo.toml crates/server/Cargo.toml || true
rg -n '"url"[[:space:]]*:' crates/server/Cargo.toml Cargo.toml || true
sed -n '1,200p' crates/server/Cargo.toml | cat -nRepository: OpenZeppelin/guardian
Length of output: 11129
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== wrappers.rs (line numbers) =="
python3 - <<'PY'
import pathlib
p = pathlib.Path("crates/server/src/secret/wrappers.rs")
lines = p.read_text().splitlines()
for i in range(120, 210):
if i < len(lines):
print(f"{i+1:4d}\t{lines[i]}")
PY
echo
echo "== secret/mod.rs (relevant comment) =="
python3 - <<'PY'
import pathlib
p = pathlib.Path("crates/server/src/secret/mod.rs")
lines = p.read_text().splitlines()
for i in range(1, 80):
if i < len(lines):
print(f"{i+1:4d}\t{lines[i]}")
PY
echo
echo "== secret/tests.rs (credential_url_debug_shows_only_scheme_and_host) =="
python3 - <<'PY'
import pathlib, re
p = pathlib.Path("crates/server/src/secret/tests.rs")
lines = p.read_text().splitlines()
start = 0
for i,l in enumerate(lines):
if "credential_url_debug_shows_only_scheme_and_host" in l:
start = max(0,i-5); break
end = min(len(lines), start+60)
for i in range(start, end):
print(f"{i+1:4d}\t{lines[i]}")
PYRepository: OpenZeppelin/guardian
Length of output: 5421
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "\\.scheme_and_host\\(" crates/server/src -S || true
rg -n "CredentialUrl\\(" crates/server/src -S || true
rg -n "CredentialUrl" crates/server/src | head -n 200 || trueRepository: OpenZeppelin/guardian
Length of output: 4535
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' crates/server/src/secret/tests.rs | cat -nRepository: OpenZeppelin/guardian
Length of output: 6444
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '230,320p' crates/server/src/audit/postgres.rs | cat -nRepository: OpenZeppelin/guardian
Length of output: 4473
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find if `url` crate is already used/declared in the repo/workspace
rg -n 'url::Url|Url::parse\\(' -S . || true
rg -n '["\x27]url["\x27]\\s*[:=]' -S **/Cargo.toml Cargo.toml || true
rg -n '^url\\s*=|\\[dependencies\\][^\\[]*\\nurl\\s*=' -S Cargo.toml crates/**/Cargo.toml || trueRepository: OpenZeppelin/guardian
Length of output: 355
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search code for url::Url / Url::parse usage
rg -n 'url::Url|Url::parse\\(' -S . || true
# Search Cargo.toml files for a dependency on the `url` crate
rg -n '"url"\\s*:|^url\\s*=|\\[dependencies\\][\\s\\S]*?\\nurl\\s*=' -S -g 'Cargo.toml' . || true
# Quick check: list Cargo.toml files under crates (to ensure -g works as expected)
ls -la Cargo.toml >/dev/null 2>&1 && echo "found root Cargo.toml" || echo "no root Cargo.toml"
find crates -maxdepth 3 -name Cargo.toml | headRepository: OpenZeppelin/guardian
Length of output: 411
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Code usage search (avoid regex metacharacters that caused earlier parse error)
rg -n 'url::Url|Url::parse' -S . || true
# Dependency search across all Cargo.toml files for the `url` crate
tmpfile=$(mktemp)
find . -name Cargo.toml -print0 > "$tmpfile"
python3 - <<'PY'
import subprocess, os, shlex, sys, pathlib, json
root="."
# find Cargo.toml paths
paths=[]
for p in pathlib.Path(root).rglob("Cargo.toml"):
paths.append(str(p))
# search each for `url` dependency key
matches=[]
for p in paths:
txt=p.read_text(errors="ignore")
# simple check for dependency line
if '"url"' in txt or "\nurl" in txt:
matches.append(p)
print("Cargo.toml files potentially mentioning url:", *matches, sep="\n- ")
PYRepository: OpenZeppelin/guardian
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== code usage of Url::parse / url::Url =="
rg -n 'url::Url|Url::parse' -S . || true
echo
echo "== Cargo.toml dependency key \"url\" =="
rg -n '"url"\s*:' -S -g 'Cargo.toml' . || true
echo
echo "== Cargo.toml lines mentioning \"url\" keyword =="
rg -n '\burl\b' -S -g 'Cargo.toml' . || trueRepository: OpenZeppelin/guardian
Length of output: 195
Fix CredentialUrl::scheme_and_host() to prevent secret leakage in CredentialUrl::Debug for malformed inputs.
CredentialUrl::Debug prints CredentialUrl({}) using scheme_and_host() (lines ~153-156), but the current implementation only strips userinfo when @ is present and otherwise treats the entire “authority” as host[:port] (lines ~170-188). For example, postgres://alice:secret/db becomes postgres://alice:secret, leaking the “secret” portion despite the “Safe to log” contract (lines ~140-145).
Switch to a real URL parser and return "<invalid-url>" if parsing/host/port extraction fails, and add coverage for the postgres://alice:secret/db-style case.
🔒 Minimal fix sketch
+use url::Url;
+
fn scheme_and_host(raw: &str) -> String {
- let (scheme, rest) = match raw.split_once("://") {
- Some(parts) => parts,
- None => return "<invalid-url>".to_owned(),
- };
- if scheme.is_empty() {
- return "<invalid-url>".to_owned();
- }
- let authority_end = rest.find(['/', '?', '#']).unwrap_or(rest.len());
- let authority = &rest[..authority_end];
- let host_port = match authority.rsplit_once('@') {
- Some((_, after)) => after,
- None => authority,
- };
- if host_port.is_empty() {
- return "<invalid-url>".to_owned();
- }
- format!("{scheme}://{host_port}")
+ let Ok(url) = Url::parse(raw) else {
+ return "<invalid-url>".to_owned();
+ };
+ let Some(host) = url.host_str() else {
+ return "<invalid-url>".to_owned();
+ };
+ match url.port() {
+ Some(port) => format!("{}://{host}:{port}", url.scheme()),
+ None => format!("{}://{host}", url.scheme()),
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/server/src/secret/wrappers.rs` around lines 140 - 145, The current
scheme_and_host() implementation (called by CredentialUrl::Debug) can leak
secrets for malformed authorities (e.g., "postgres://alice:secret/db"); update
scheme_and_host (and any helper scheme_and_host(self.expose_secret())) to parse
the URL with a proper URL parser, extract scheme and host (and optional port)
only when parsing succeeds and a host is present, and otherwise return the
literal "<invalid-url>"; ensure the function never derives host by
string-splitting the authority (which caused the alice:secret leak) and add
tests covering "postgres://alice:secret/db" and other malformed inputs to verify
no secret is exposed.
| pub(crate) mod ct; | ||
| pub(crate) mod wrappers; | ||
|
|
||
| pub(crate) use wrappers::{FixedKey, SecretBytes, SecretString, CredentialUrl}; | ||
| pub(crate) use ct::eq as ct_eq; | ||
| ``` |
There was a problem hiding this comment.
Align module-surface contract with actual mod.rs visibility and exports.
The documented surface here does not match crates/server/src/secret/mod.rs in the provided context: ct/wrappers are declared as private modules (mod ...), and ct_eq is test-only (#[cfg(test)]). Please update this contract block to reflect the real API boundary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@speckit/features/008-zeroize-secrets/contracts/secret-module.md` around lines
19 - 24, The documented module surface is inaccurate: the real module declares
private modules `ct` and `wrappers` and only exposes wrapper types, and `ct::eq`
is test-only (`ct_eq` behind #[cfg(test)]). Update the contract block to show
`ct` and `wrappers` as private (remove or mark them as not re-exported), export
only the wrapper types `FixedKey`, `SecretBytes`, `SecretString`, and
`CredentialUrl` as the public API, and mark `ct_eq` as test-only (or omit it
from the public contract) so the contract matches the actual visibility in
mod.rs.
| | Constructor | `pub fn new(bytes: [u8; N]) -> Self` | | ||
| | Exposure | `pub fn expose_secret(&self) -> &[u8; N]` | |
There was a problem hiding this comment.
Fix documented method visibility (pub vs pub(crate)).
These API signatures are documented as pub fn, but the implementation surface in crates/server/src/secret/wrappers.rs is pub(crate) fn. Please keep the data model consistent with crate-internal visibility guarantees.
Also applies to: 39-40, 55-57, 71-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@speckit/features/008-zeroize-secrets/data-model.md` around lines 23 - 24, The
documented API signatures for the constructors and accessors (e.g., fn
new(bytes: [u8; N]) -> Self and fn expose_secret(&self) -> &[u8; N]) currently
say `pub` but their implementations are `pub(crate)` in the secret wrappers;
update the data model docs to use `pub(crate)` for those methods (including the
other occurrences noted at lines 39-40, 55-57, 71-73) so the documentation
matches the actual visibility in the wrappers implementation.
| ├── ack/ | ||
| │ ├── miden_falcon_rpo/signer.rs # MODIFIED — keystore field unchanged; FR-011 verification recorded in research.md | ||
| │ ├── miden_ecdsa/signer.rs # MODIFIED — same | ||
| │ └── secrets_manager.rs # MODIFIED — fetched bytes returned as SecretBytes; cache field (if present) holds SecretBytes |
There was a problem hiding this comment.
Update stale Secrets Manager plan wording to final design.
This line still implies returning SecretBytes/cache-field behavior, which conflicts with the finalized migration notes in this PR set (transient wrapping + parsed-key return type preserved, no cache). Please sync this line with the implemented design to avoid drift.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@speckit/features/008-zeroize-secrets/plan.md` at line 97, Update the plan
wording in the feature plan to match the implemented secrets manager design:
change the reference in the sentence mentioning secrets_manager.rs so it no
longer states "fetched bytes returned as SecretBytes; cache field (if present)
holds SecretBytes" and instead describes that fetched bytes are transiently
wrapped (not stored) and the public API still returns the parsed key type (no
cache field). Locate the mention of SecretBytes and cache in the plan and
replace it with language that reflects transient wrapping and preservation of
the parsed-key return type, and explicitly note "no cache" to avoid drift.
| | `SecretString` | Variable-length secret string. Wraps `String`. | Operator session token; EVM session token. | | ||
| | `CredentialUrl` | A URL that may embed credentials in userinfo or query. Wraps `String`. | `EvmChainConfig.rpc_url`; `StorageMetadataBuilder.database_url`. | |
There was a problem hiding this comment.
Resolve session-token mapping contradiction in Decision 7.
This table maps operator/EVM session tokens to SecretString, but Decision 6 defines the accepted model as digest-keyed maps with plaintext token not retained long-term. Please revise these mapped sites so the taxonomy matches the chosen storage shape.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@speckit/features/008-zeroize-secrets/research.md` around lines 157 - 158, The
table in Decision 7 currently lists operator and EVM session tokens as
`SecretString`, which contradicts Decision 6's chosen shape (digest-keyed maps
with plaintext tokens not retained); update the table entries so sites that
store session tokens (e.g., operator session token, EVM session token referenced
by `EvmChainConfig.rpc_url` and any usage in
`StorageMetadataBuilder.database_url`) are described as storing only a token
digest/lookup key (e.g., `TokenDigest` or `DigestKey`) rather than
`SecretString`, and add a short note that the plaintext token is ephemeral and
not persisted per Decision 6.
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (88.77%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #264 +/- ##
==========================================
+ Coverage 76.40% 76.61% +0.21%
==========================================
Files 136 138 +2
Lines 24910 25123 +213
==========================================
+ Hits 19032 19249 +217
+ Misses 5878 5874 -4 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Summary by CodeRabbit
Release Notes