feat(jans-cedarling): cap HTTP response body size on every remote fetch#14168
feat(jans-cedarling): cap HTTP response body size on every remote fetch#14168tareknaser wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughThis PR prevents memory exhaustion attacks by capping HTTP response body sizes across Cedarling's HTTP operations. A new configurable limit (default 10 MB, disabled at 0) is threaded through bootstrap configuration and enforced in the HTTP utilities layer, rejecting oversized responses before buffering into memory. ChangesHTTP Response Body Size Capping
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
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 `@jans-cedarling/http_utils/src/lib.rs`:
- Around line 362-365: The test assertion using matches! on the variable err
against HttpRequestReasonError::ResponseTooLarge lacks a descriptive failure
message; update the assertion to include a clear message (e.g., explaining we
expect a ResponseTooLarge with limit 1024) so failures report intent—locate the
assert!(matches!(err, HttpRequestReasonError::ResponseTooLarge { limit: 1024, ..
})) and add a descriptive string as the second argument to assert! per test
guidelines.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 2118d9ec-72b3-4a1d-8e98-7dd8dfc5b25a
⛔ Files ignored due to path filters (1)
jans-cedarling/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
docs/cedarling/reference/cedarling-properties.mdjans-cedarling/cedarling/src/bootstrap_config/decode.rsjans-cedarling/cedarling/src/bootstrap_config/raw_config/config.rsjans-cedarling/cedarling/src/bootstrap_config/raw_config/default_values.rsjans-cedarling/cedarling/src/http/mod.rsjans-cedarling/cedarling/src/init/policy_store.rsjans-cedarling/cedarling/src/jwt/http_utils.rsjans-cedarling/cedarling/src/jwt/key_service.rsjans-cedarling/cedarling/src/jwt/mod.rsjans-cedarling/cedarling/src/jwt/status_list/cache.rsjans-cedarling/cedarling/src/jwt/trusted_issuers_loader.rsjans-cedarling/http_utils/Cargo.tomljans-cedarling/http_utils/src/lib.rs
Signed-off-by: Tarek <tareknaser360@gmail.com>
…tConfig Signed-off-by: Tarek <tareknaser360@gmail.com>
… fetches Signed-off-by: Tarek <tareknaser360@gmail.com>
…strap property Signed-off-by: Tarek <tareknaser360@gmail.com>
03bc859 to
947f1a0
Compare
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 `@jans-cedarling/cedarling/src/jwt/http_utils.rs`:
- Around line 193-194: The enum in jwt/http_utils.rs contains dead variants
JsonDeserializeResponse and ReadTextResponse that are never constructed or
matched; remove those two variants from the error enum (and any associated
#[error(...)] attributes) and clean up any now-unused imports or uses in the
jwt::http_utils module (e.g., remove references to these variant names and
ensure the enum still compiles and is used where needed); run cargo build/tests
to validate there are no remaining references to JsonDeserializeResponse or
ReadTextResponse.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 34ddeb05-7cfc-49d6-b382-047ea72c961b
⛔ Files ignored due to path filters (1)
jans-cedarling/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
docs/cedarling/reference/cedarling-properties.mdjans-cedarling/cedarling/src/bootstrap_config/decode.rsjans-cedarling/cedarling/src/bootstrap_config/raw_config/config.rsjans-cedarling/cedarling/src/bootstrap_config/raw_config/default_values.rsjans-cedarling/cedarling/src/http/mod.rsjans-cedarling/cedarling/src/init/policy_store.rsjans-cedarling/cedarling/src/jwt/http_utils.rsjans-cedarling/cedarling/src/jwt/key_service.rsjans-cedarling/cedarling/src/jwt/mod.rsjans-cedarling/cedarling/src/jwt/status_list/cache.rsjans-cedarling/cedarling/src/jwt/trusted_issuers_loader.rsjans-cedarling/http_utils/Cargo.tomljans-cedarling/http_utils/src/lib.rs
Description
Every Cedarling HTTP fetch currently reads the full response body into memory with no size limit. A hostile or compromised remote can OOM the backend by serving a giant body.
This PR adds a configurable cap (default 10 MB) that is enforced before the body is fully buffered: if
Content-Lengthis above the cap, the fetch is rejected without reading the body; otherwise the body is streamed chunk by chunk and aborted as soon as the accumulated size would exceed the cap.Implementation Details
http_utils: add an optionalmax_response_sizefield onSenderand a publicread_response_cappedhelper.Sender::send,send_text,send_bytes, andsend_oncenow route through the helper.docs/cedarling/reference/cedarling-properties.mdunder the existing HTTP client section.http_utilsproving the cap rejects oversized responses.Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Closes #14167
Summary by CodeRabbit
New Features
Documentation