Skip to content

fix: Mask request URLs in provider error messages - #918

Open
zachsplat wants to merge 1 commit into
OpenZeppelin:mainfrom
zachsplat:fix/mask-request-urls
Open

zachsplat wants to merge 1 commit into
OpenZeppelin:mainfrom
zachsplat:fix/mask-request-urls

Conversation

@zachsplat

@zachsplat zachsplat commented Oct 9, 2026 •

Copy link
Copy Markdown

Summary

Follow-up to #916, from the review there.

  • categorize_reqwest_error built ProviderError::RequestError and ProviderError::Other from err.to_string(), and reqwest appends the full request URL to its messages (error sending request for url (...)). That text ends up in the error = %e fields on the retry warn lines and in error reasons, so a key in the path or query of an RPC URL could still be logged on connection failures and HTTP errors. The URL inside the text is now replaced with its mask_url form. I did it on the text rather than with err.without_url() because categorize_reqwest_error and the From<&reqwest::Error> and eyre downcast paths only hold a reference, and without_url takes the error by value. The host stays visible, which keeps the messages useful for telling providers apart.
  • mask_url now hides userinfo: https://user:pass@host/path becomes https://***@host/***, and https://user:pass@host becomes https://***@host. An @ later in the path is not treated as userinfo.

Testing Process

  • services::provider::tests::test_categorize_reqwest_error_masks_url_in_message: a connection error to http://127.0.0.1:9/v2/SECRET_KEY_PATH?apikey=SECRET_KEY_QUERY. reqwest's own message contains the key; the ProviderError text does not, and shows http://127.0.0.1:9/***.
  • services::provider::tests::test_categorize_reqwest_error_status_masks_url_in_message: the same for an HTTP 500 from mockito, which takes the RequestError path.
  • utils::url::tests: three new cases for userinfo with a path, without a path (with and without a port, and with a query), and one for an @ in the path.
  • With the fix reverted and the tests kept, both new categorize_reqwest_error tests fail.
  • RUST_TEST_THREADS=1 cargo test --lib utils::: 630 passed, 29 ignored. services::provider: 247 passed. models::relayer (its RPC config responses use mask_url): 216 passed.
  • cargo fmt --all -- --check and cargo clippy --all-features --workspace --lib --bins --no-deps -- -D warnings --allow deprecated are clean on 1.93.0.

Checklist

  • Add a reference to related issues in the PR description.
  • Add unit tests if applicable.

Summary by CodeRabbit

  • Bug Fixes
    • Error messages now redact request URLs, helping prevent sensitive URL details from appearing in connection and server error reports.
    • URL masking now hides credentials while preserving the host, and correctly distinguishes credentials from @ characters in the URL path.

@zachsplat
zachsplat requested a review from a team as a code owner October 9, 2026 14:01
@coderabbitai

coderabbitai Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 44048da4-8583-4402-be77-96cca694c101

📥 Commits

Reviewing files that changed from the base of the PR and between d7f151f and 34ecdc1.


📒 Files selected for processing (2)
  • src/services/provider/mod.rs
  • src/utils/url.rs

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.



Walkthrough

URL masking now hides userinfo within the authority. Provider error messages use masked request URLs when the error includes a URL. Tests cover userinfo masking and redaction in connection and HTTP 500 errors.

Changes

URL Redaction

Layer / File(s) Summary
Mask URL userinfo
src/utils/url.rs
mask_url masks userinfo within the authority and retains the existing path and query masking. Examples and tests cover userinfo in URLs and @ in a path.
Redact provider error URLs
src/services/provider/mod.rs
Provider error categorization uses request URLs masked by mask_url. Existing status codes and error categories remain unchanged. Tests cover connection failures and HTTP 500 errors.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: zeljkox

Merge Risk

Merge Risk: ⚪ Minimal · up to 34ecd

URL masking now covers authority credentials, and provider errors apply masked URLs when the request URL appears in their text. No concrete leak or error-category regression is established, so the change appears ready for normal merge checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 34ecd

The change reduces credential exposure without expanding access. A narrow coupling between error text and retry policy can also cause some failed requests to stop before retrying or falling back to another endpoint.

Retained concerns

  • Low · reliability · inferred: Redacting a URL can change failure containment because status-free Other errors use displayed text for retry classification. When timeout, connection, or reset occurs only in the masked path or query, configurations allowing multiple attempts can now return immediately without retrying, marking the endpoint failed, or attempting another provider. This is a conditional source-level behavior change, not a demonstrated production outage.

Security review details

Security Blast Radius

  • inferred — The affected confidentiality surface is request-URL content included in provider error strings and their downstream displays. The reviewed changes transform reporting text rather than request destinations, credentials used for requests, redirect authority, or caller privileges.

Security Findings and Attack Paths

  • observed — Retry logging also writes provider_url directly, including on failed attempts. Those statements are unchanged from the PR base. This is a separate pre-existing exposure path, not a credential disclosure introduced or worsened by this PR; effective log access and deployed URL contents were not established.

Trust Boundaries and Controls

  • observed — The new redaction control is conditional on an attached URL and an exact string match. Errors without an attached URL retain their text, and non-reqwest fallback conversions remain unchanged. Consequently, this is targeted redaction rather than an end-to-end secrecy guarantee for all provider errors or logs.

Hardening Proposals

  • proposed — Separate retry classification from redacted display text using structured transport properties or a stable classification captured before formatting. This would keep confidentiality improvements from changing failure-containment decisions.



🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: masking request URLs in provider error messages.
Description check Passed The description includes the required Summary, Testing Process, and Checklist sections. It references related issue #916, documents the implementation and tests, and marks applicable checklist items c…
Docstring Coverage Passed Docstring coverage is 90.91% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 2 files.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.


✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR


  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

A rabbit checks the URL trail,
And hides the secrets from the tale.
The host stays clear, the path is veiled,
Error text now guards what once prevailed.
Hop by hop, the tests set sail.

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant