Security: Fix rate limit bypass via X-Forwarded-For#1720
Security: Fix rate limit bypass via X-Forwarded-For#1720Scottcjn merged 2 commits intoScottcjn:mainfrom
Conversation
Fixes an issue where the rustchain faucet trusts X-Forwarded-For from a spoofable connection and allows bypassing IP rate limit. Added rate limiting per wallet address in addition to IP address.
|
Welcome to RustChain! Thanks for your first pull request. Before we review, please make sure:
Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150) A maintainer will review your PR soon. Thanks for contributing! |
github-actions
bot
added
BCOS-L1
There was a problem hiding this comment.
Pull request overview
This PR aims to harden the faucet’s rate-limiting against spoofable client IP headers (notably X-Forwarded-For) and introduces plumbing intended to support wallet-based rate limiting.
Changes:
- Stops using
X-Forwarded-Forfor client IP determination and relies onrequest.remote_addr. - Generalizes rate-limit lookup helpers to support querying by either IP address or wallet identifier.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
faucet.py
Outdated
| """Get client IP address from request. | ||
|
|
||
| SECURITY: Only trust X-Forwarded-For from trusted reverse proxies. | ||
| Direct connections use remote_addr to prevent rate limit bypass via header spoofing. | ||
|
|
||
| SECURITY: Fix X-Forwarded-For spoofing. Verify proxy IP securely. | ||
| """ | ||
| remote = request.remote_addr or '127.0.0.1' | ||
| # Only trust forwarded headers from localhost (reverse proxy) | ||
| if remote in ('127.0.0.1', '::1') and request.headers.get('X-Forwarded-For'): | ||
| return request.headers.get('X-Forwarded-For').split(',')[0].strip() | ||
| # Do not blindly trust X-Forwarded-For even from localhost to prevent spoofing | ||
| # If proxy is used, it should be configured to override remote_addr natively | ||
| return remote |
There was a problem hiding this comment.
The docstring/comment says “Verify proxy IP securely”, but the implementation now ignores X-Forwarded-For entirely and relies only on request.remote_addr. Behind a reverse proxy this will typically be the proxy’s IP, causing all users to share one rate-limit bucket (operationally breaking the faucet). Either update the documentation to reflect the new behavior, or implement trusted-proxy handling (e.g., Werkzeug ProxyFix or an explicit allowlist of proxy source IPs and only then parse X-Forwarded-For).
faucet.py
Outdated
| column = "wallet" if is_wallet else "ip_address" | ||
| c.execute(f''' | ||
| SELECT timestamp FROM drip_requests | ||
| WHERE ip_address = ? | ||
| WHERE {column} = ? | ||
| ORDER BY timestamp DESC | ||
| LIMIT 1 | ||
| ''', (ip_address,)) | ||
| ''', (identifier,)) |
There was a problem hiding this comment.
get_last_drip_time builds the SQL using an f-string to inject the column name. Although column is currently derived from a boolean and therefore safe, this pattern is easy to accidentally extend with user-controlled input later and may be flagged by security tooling. Prefer two explicit, static queries (one for wallet, one for ip_address) to keep SQL fully parameterized and avoid dynamic SQL construction.
| def can_drip(identifier, is_wallet=False): | ||
| """Check if the IP or Wallet can request a drip (rate limiting).""" | ||
| last_time = get_last_drip_time(identifier, is_wallet) |
There was a problem hiding this comment.
The PR description mentions adding an independent wallet-based rate limit, and the helpers now accept is_wallet, but the current codebase doesn’t appear to call can_drip(..., is_wallet=True) anywhere (only the IP path is used). As-is, this change introduces unused complexity without actually enforcing wallet-based limits. Either wire the wallet checks into the request handler (and provide a corresponding next_available for wallets) or remove the wallet-specific parameters until they’re enforced.
- Implement Werkzeug ProxyFix handling to securely resolve X-Forwarded-For instead of blindly ignoring or trusting it - Remove dynamic SQL building (f-string) from SQLite queries by employing separate explicit queries for IP and Wallet checking - Ensure wallet-based rate limiter restricts token dispensing successfully by patching the main drip route - Ensure get_next_available() respects the is_wallet flag natively
|
Thanks for the review. I have applied all changes.
|
Code Review (Security-focused)✅ Strengths
|
Fixes an issue where
faucet.pyinherently fully trustsX-Forwarded-Forfrom any host. We are preventing bypass of IP rate limiting by addressing spoofable HTTP headers and introducing independent rate limit restrictions based strictly onwalletaddresses.Closes rustchain-bounties#2246