fix: warn on disabled TLS and cap response body size (#4 #7)#20
Conversation
- Log a warning when verify_ssl is disabled so the security implication of danger_accept_invalid_certs is visible - Add FetcherConfig.max_body_bytes (default 50 MiB) and stream the response body in chunks, aborting if Content-Length or the accumulated bytes exceed the cap, so a hostile server cannot exhaust memory Closes #4 Closes #7 https://claude.ai/code/session_012RmdaovmNWZVAim4XxCWwn
|
Warning Review limit reached
More reviews will be available in 12 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds HTTP response body size limiting to prevent excessive memory usage. A new ChangesHTTP response body size limiting
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 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.
Actionable comments posted: 1
🤖 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 `@src/fetchers/client.rs`:
- Around line 197-213: The chunk-reading loop must handle chunk read errors and
avoid unchecked size arithmetic: replace the while-let with a match on
resp.chunk().await so any Err(e) immediately returns
Err(FetcherError::RequestFailed(format!("chunk read error: {}", e))); when
checking Content-Length convert safely via
usize::try_from(resp.content_length().unwrap()) (or return an error if
conversion fails) instead of casting with as; and when accumulating use
checked_add (e.g. bytes.len().checked_add(chunk.len())) to detect overflow—if
checked_add yields None or the sum > max_body, stop reading and return
Err(FetcherError::RequestFailed(format!("response body too large: {} (max {})",
sum_or_estimate, max_body))); ensure you never push chunk into bytes after
detecting overflow or on chunk read error.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d5acaecc-23cc-4d82-9705-72e05ae42e1a
📒 Files selected for processing (3)
src/fetchers/client.rssrc/fetchers/config.rstests/fetchers_config.rs
Address CodeRabbit review on #20: - Replace `while let Ok(Some(chunk))` with a `match` that returns Err on chunk read errors, so a mid-body error no longer silently produces a truncated body - Use usize::try_from for the u64 Content-Length conversion so a value that does not fit a 32-bit usize is treated as too large rather than silently truncated - Use checked_add when accumulating chunk sizes to detect overflow https://claude.ai/code/session_012RmdaovmNWZVAim4XxCWwn
Summary
Two HTTP-client hardening fixes:
verify_ssl(false)passeddanger_accept_invalid_certs(true)to reqwest with no visible signal. Added alog::warn!onFetcher::newso disabling TLS verification surfaces in logs.resp.text().awaitpulled the entire body into memory with no cap, so a hostile or misconfigured server could trigger OOM. AddedFetcherConfig.max_body_bytes(default 50 MiB) plus a corresponding builder method. The client now (a) rejects when the advertisedContent-Lengthexceeds the cap, and (b) streams chunks with a running size check so servers that lie about or omitContent-Lengthstill cannot exhaust memory.Test plan
cargo test— full suite greencargo clippy --all-targets -- -D warningscleancargo fmt --checkcleanCloses #4
Closes #7
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests