Redact archive URL password in snapshot download output

[fnando]

What

stellar snapshot create printed the user/environment-supplied archive URL (--archive-url / STELLAR_ARCHIVE_URL) verbatim to stderr while downloading. When that URL carried HTTP basic-auth credentials (https://user:password@host/), the password was written to stderr in cleartext at three sites in commands/snapshot/create.rs: "Downloading history", "Downloaded history", and "Downloading ledger headers". These now pass the URL through the existing redact_url helper, matching how network info, network ls, and doctor already handle credentialed URLs.

Why

stderr from a CLI is routinely captured in CI/CD logs, terminal recordings, and screen shares, so a private archive credential printed there is exposed to anyone who can read those. This is the same class of leak the redact_url work was written to prevent for RPC URLs; the snapshot command was the one display path that wasn't covered. Default flows (mainnet/testnet/futurenet) use hardcoded credential-free Stellar archives and are unaffected; this matters for operators pointing the command at a private, credentialed history archive.

Known limitations

N/A — a codebase-wide sweep of other print/log/error sites (RPC, signer, deploy links, container, error enums, Debug impls) confirmed every other credentialed-URL display path already redacts, so no further sites needed changing.

[claude]

Claude encountered an error after 4m 30s —— View job

---

Claude PR Review

Reviewing the PR…

Todos

• Load tools
• Read PR diff and changed files
• Check redact_url helper and other call sites for consistency
• Look for any other unredacted leak sites in create.rs
• Review test coverage in snapshot.rs
• Post review feedback
  • redact-snapshot-archive-url

[Copilot]

Pull request overview

This PR prevents accidental credential disclosure by ensuring stellar snapshot create no longer prints archive URLs containing HTTP basic-auth passwords verbatim to stderr during snapshot downloads. It reuses the existing redact_url helper already used in other commands to keep URL display consistent across the CLI.

Changes:

• Redact archive URL passwords in snapshot download progress output (“Downloading history”, “Downloaded history”, “Downloading ledger headers”).
• Add an integration test verifying the password is not present in snapshot create stderr output.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
cmd/soroban-cli/src/commands/snapshot/create.rs	Routes history and ledger-header download URL display through redact_url to avoid leaking basic-auth passwords.
cmd/crates/soroban-test/tests/it/integration/snapshot.rs	Adds an integration test ensuring a credentialed --archive-url does not leak the password to stderr and shows the redaction marker.

[claude]

Claude finished @fnando's task in 3m 34s —— View job

---

Claude PR Review

Todos

• Load tools
• Read PR diff and changed files
• Check redact_url helper and other call sites for consistency
• Look for any other unredacted leak sites in create.rs
• Review test coverage in snapshot.rs
• Post review feedback

---

Summary

Solid, well-scoped fix that closes a clear credential‑leak path and matches the redaction pattern already used by network info, network ls, doctor, and the actionlog. The diff is small (~36/-5), the helper reuse is appropriate, and the parse‑then‑redact ordering is the safer choice. A couple of points worth considering before merge.

Findings

1. Minor — inconsistent error handling on Url::from_str between the two callsites

cmd/soroban-cli/src/commands/snapshot/create.rs:563

let history_url = Url::from_str(&history_url).unwrap();

vs. cmd/soroban-cli/src/commands/snapshot/create.rs:620

let ledger_url = Url::from_str(&ledger_url).map_err(Error::ParsingBucketUrl)?;

The history-URL parse is a pre-existing unwrap() (not introduced by this PR), but it's the exact same shape of operation as the ledger-URL parse two functions down. For consistency — and to avoid a future panic if archive_url's trailing path or query ever interacts oddly with the appended segments — consider aligning on .map_err(Error::ParsingBucketUrl)? here too. Not blocking, but cheap to fix while you're in this code.

Fix this →

2. Test coverage gap — only one of three redacted print sites is exercised

cmd/crates/soroban-test/tests/it/integration/snapshot.rs:214-228

Because archive.invalid fails DNS on the very first request, only create.rs:565 ("Downloading history …") is actually reached. The two other sites this PR redacts — "Downloaded history" (line 598) and "Downloading ledger headers" (line 622) — are unreachable in this test, so a future regression that re-introduces an un-redacted history_url or ledger_url interpolation in either of those would not be caught. The redaction-marker assertion (.and(predicate::str::contains("redacted"))) is satisfied entirely by the first print site, which can mask this.

If you want defense-in-depth, the usual options are:

• Spin up a wiremock/local hyper server that returns a valid stellar-history.json and a ledger header so the test can walk the full success path, or
• Add a small unit test directly on the redacted format strings (cheap, no I/O).

Not a correctness bug in the PR, but the test is doing less work than its name suggests.

3. Worth verifying — reqwest::Error display paths

The three redacted print sites cover the direct URL interpolations, but the URL is also embedded in the URL passed to reqwest::Client::get(...) at create.rs:571, :628, and :738. If .send().await fails, the resulting reqwest::Error is wrapped in Error::DownloadingHistory / Error::GettingBucket and ultimately displayed:

• create.rs:217: print.warnln(format!("Failed to get ledger metadata from archive: {e}")); — directly to stderr.
• Errors propagated from get_history / cache_bucket via ? (lines 198, 234, 302) are surfaced by the outer error renderer.

reqwest 0.12.23 does redact credentials in some of its error paths, but the behavior isn't fully guaranteed across every error variant (TLS, redirect, connect, body read, etc.). The current test passes only the DNS‑resolution path of one site, so the other variants are unverified. Worth a manual check against a credentialed URL with at least one non-DNS failure mode (e.g., TLS error against a real host, or HTTP 500) to confirm supersecret doesn't appear in the propagated stderr. If it does, wrapping the reqwest error or .without_url()-style stripping at the boundary would be the fix.

4. Re: the earlier inline review comment on create.rs ledger_url

The earlier auto-review suggested moving the Url::from_str above the print for the ledger path "for consistency and safety." Looking at the actual diff, the PR already does this — let ledger_url = Url::from_str(...).map_err(...)?; at line 620 runs before the globeln call at line 622. The earlier comment looks like a misread of the unified diff; no action needed there.

5. Nits

• The new test name snapshot_create_redacts_archive_url_password matches the helper-test naming style elsewhere (leaves_url_without_password_unchanged, replaces_password_with_placeholder). Good.
• The redact_url import path (utils::url::redact_url) is consistent with how data.rs and network.rs pull it in.

Verdict

LGTM with non-blocking suggestions. The leak this PR fixes is real and the fix is correct; the items above are mostly about hardening the test and tying off a tangential reqwest::Error concern.
• redact-snapshot-archive-url

[claude]

Claude finished @fnando's task in 1m 22s —— View job

---

Claude PR Review

Todos

• Load tools
• Inspect latest commits on this branch
• Re-read the changed files at current HEAD
• Verify earlier findings still apply / check for new issues
• Post updated review

---

Re-review after sync

This synchronize event is the Merge branch 'main' into redact-snapshot-archive-url commit (9f3bc33); the code-bearing commit is still ce39041 Hide archive password in snapshot download output., unchanged from my earlier review. So the substance of that review still stands. Re-confirming below against current HEAD and folding in the author response.

Status of prior findings

1. unwrap() vs map_err(Error::ParsingBucketUrl)? at cmd/soroban-cli/src/commands/snapshot/create.rs:563 — still present, still not strictly required for safety (the Url being reconstructed at L563 starts from an already-parsed &Url arg plus hex segments, so it is not a realistic panic site). Leaving it as a pre-existing inconsistency is reasonable; out of scope for this PR.

2. Test coverage only exercises the first of three redacted print sites — author confirmed in this reply that this is intentional and that broader coverage is deferred until a mock-archive harness exists. The redaction logic itself is unit-tested in utils.rs::url::tests, so the regression risk for redact_url is contained. Reasonable trade-off — the test still meaningfully exercises the leak path that matters for the failure mode (DNS / connection error before request body).

3. reqwest::Error display paths — author confirmed reqwest strips userinfo from its own error URLs, which matches reqwest's behavior of internally wrapping URLs through without_url/with_url such that the password component is replaced before formatting. So there is no propagation leak from Error::DownloadingHistory(e) / Error::GettingBucket(e). I'm satisfied this is closed.

Spot-check on current HEAD

Confirmed at cmd/soroban-cli/src/commands/snapshot/create.rs:

• L565–568: Downloading history — redacted ✓
• L598–601: Downloaded history — redacted ✓
• L620–625: Url::from_str happens before the Downloading ledger headers print, and the print uses redact_url(ledger_url.as_str()) ✓ (the earlier claude inline suggestion to move the parse above the print was already applied — no action needed there)

And the redact helper at cmd/soroban-cli/src/utils.rs:275-283 is a Url::parse → set_password("redacted") → to_string() pipeline that correctly handles the userinfo-only case (test at L294 covers it) and falls through unchanged on parse failure (acceptable since the inputs at the call sites are already Url values being round-tripped via .as_str(), which cannot fail parsing).

Verdict

LGTM. The fix is correct, scoped, consistent with the established redact_url pattern, and the author has resolved the two open concerns from my prior pass (reqwest behavior + test-coverage scope as a deliberate follow-up). No blocking issues.
• redact-snapshot-archive-url