fix(jans-cedarling): add request timeouts to outbound HTTP clients#14003
Merged
Conversation
Signed-off-by: Tarek <tareknaser360@gmail.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors HTTP client surface to use a new HttpClientConfig (adds per-request timeout on native), exposes get_json/post_json helpers, threads a shared HttpClient through policy/JWT/lock code paths and tests, and replaces http_utils retry/error shapes with richer structured errors carrying status, retry count, and last-error context. ChangesHttpClient config, API, and call-site wiring
http_utils: richer HttpRequestError and retry context
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Member
✅ 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. |
mo-auto
added
comp-jans-cedarling
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jans-cedarling/cedarling/src/http/mod.rs`:
- Around line 273-281: Replace the assertion that checks the failure path using
assert!(response.is_err(), ...) with the test-guideline pattern using
expect_err(...) so the test fails with a clear message and prints the error;
specifically, change the check on the variable response (from the GET call
stored in response) to call response.expect_err("Expected timeout error") and
keep the elapsed timing assertion unchanged (the variables response, start,
elapsed, and request_timeout in mod.rs identify the location to update).
🪄 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: 2493fb17-4405-43a5-86e8-1b2eca40dd78
📒 Files selected for processing (3)
jans-cedarling/cedarling/src/http/mod.rsjans-cedarling/cedarling/src/init/policy_store.rsjans-cedarling/cedarling/src/jwt/http_utils.rs
Signed-off-by: Tarek <tareknaser360@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jans-cedarling/cedarling/src/http/mod.rs`:
- Around line 34-49: The HttpClient::new constructor currently only sets a total
request timeout and should accept and apply a per-connection timeout on non-WASM
targets to prevent TCP handshake stalls; add a new parameter (e.g.,
connect_timeout: Duration) to HttpClient::new, mirror the pattern used in
jwt/http_utils.rs (HTTP_CONNECT_TIMEOUT and chaining .connect_timeout(...) on
the Client::builder()), apply .connect_timeout(connect_timeout) for
#[cfg(not(target_arch = "wasm32"))], keep request_timeout consumed as a no-op on
WASM, and then update all call sites (such as the policy-store fetch in
init/policy_store.rs and any other invocations of HttpClient::new) to pass the
chosen connect timeout value.
🪄 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: dd37b0f7-dac4-4af0-b3f6-4ea240258c99
📒 Files selected for processing (2)
jans-cedarling/cedarling/src/http/mod.rsjans-cedarling/cedarling/src/jwt/http_utils.rs
Contributor
|
Thanks, @tareknaser, for the contribution. According to #14002 (comment) I will add some changes |
…uild Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…being used in HttpClient Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
dagregi
reviewed
May 4, 2026
…nd refactor bootstrap config This consolidates HTTP timeout configuration, refactors bootstrap config to support it, and updates all outbound HTTP callers (lock, policy store, JWK key service, trusted issuers, status list cache) along with examples and tests. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…roperties Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
jans-cedarling/cedarling/src/jwt/trusted_issuers_loader.rs (1)
1174-1178:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIncorrect assertion message — says "1 failed issuer" but expects
2.The expected value is
2but the message still says"loading state should have 1 failed issuer", which will mislead when the assertion fires.🐛 Proposed fix
assert_eq!( loader.loading_state.failed_issuers().len(), 2, - "loading state should have 1 failed issuer" + "loading state should have 2 failed issuers (failing_issuer and unreachable_issuer)" );🤖 Prompt for 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. In `@jans-cedarling/cedarling/src/jwt/trusted_issuers_loader.rs` around lines 1174 - 1178, The assertion uses assert_eq!(loader.loading_state.failed_issuers().len(), 2, "...") but the message incorrectly says "loading state should have 1 failed issuer"; update the failure message in the assert_eq call to accurately reflect the expected value (e.g., "loading state should have 2 failed issuers") so the assertion text matches the expected 2 and helps debugging; locate the assert_eq call referencing loader.loading_state.failed_issuers().len() and change only the message string.jans-cedarling/cedarling/examples/lock_integration.rs (1)
9-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun
cargo fmt—HttpClientConfigimport is not formatted consistently with the rest of the PR.The
HttpClientConfig,token is placed on the same line as the opening{, with the remaining items indented on subsequent lines. Other files changed in this PR (startup_benchmark.rs,authz_authorize_multi_issuer_benchmark.rs,context_data_store_benchmark.rs) all use the correctly formatted multi-line block.cargo fmt --checkwould flag this.🛠️ Proposed fix
-use cedarling::{HttpClientConfig, - AuthorizationConfig, BootstrapConfig, CedarEntityMapping, Cedarling, DataStoreConfig, - EntityData, JwtConfig, LockServiceConfig, LockTransport, LogConfig, LogLevel, LogTypeConfig, - PolicyStoreConfig, PolicyStoreSource, RequestUnsigned, -}; +use cedarling::{ + AuthorizationConfig, BootstrapConfig, CedarEntityMapping, Cedarling, DataStoreConfig, + EntityData, HttpClientConfig, JwtConfig, LockServiceConfig, LockTransport, LogConfig, + LogLevel, LogTypeConfig, PolicyStoreConfig, PolicyStoreSource, RequestUnsigned, +};As per coding guidelines: "Use rustfmt with project's rustfmt.toml settings for formatting."
🤖 Prompt for 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. In `@jans-cedarling/cedarling/examples/lock_integration.rs` around lines 9 - 13, The import list starting with HttpClientConfig is misformatted: move HttpClientConfig onto its own line to match the multi-line import block style used elsewhere (i.e., open the cedarling::{ line, then place HttpClientConfig, AuthorizationConfig, BootstrapConfig, CedarEntityMapping, Cedarling, DataStoreConfig, EntityData, JwtConfig, LockServiceConfig, LockTransport, LogConfig, LogLevel, LogTypeConfig, PolicyStoreConfig, PolicyStoreSource, RequestUnsigned each on their own indented line), then run cargo fmt (using the project's rustfmt.toml) to ensure formatting consistency.jans-cedarling/cedarling/src/lock/mod.rs (1)
620-626: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse
expect_err(...)and assert on the captured error once.The current pattern drops the first failure context with
assert!(result.is_err())and then unwraps the error separately. A singleexpect_err(...)plus a descriptivematches!assertion is clearer and matches the repo test rules.Suggested test cleanup
- let result = - LockService::new(pdp_id, &config, None, metrics, HttpClientConfig::default()).await; - assert!(result.is_err()); - assert!(matches!( - result.unwrap_err(), - InitLockServiceError::InvalidSsaJwt(_) - )); + let err = LockService::new(pdp_id, &config, None, metrics, HttpClientConfig::default()) + .await + .expect_err("lock service initialization should fail for a malformed SSA JWT"); + assert!( + matches!(err, InitLockServiceError::InvalidSsaJwt(_)), + "malformed SSA JWTs should map to InitLockServiceError::InvalidSsaJwt", + );As per coding guidelines, "For error checking in tests, use
result.expect_err("descriptive message")instead ofassert!(result.is_err())", "For pattern matching errors in tests, useassert!(matches!(...), "descriptive message")", and "All assertions must include a descriptive message explaining what is being tested."🤖 Prompt for 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. In `@jans-cedarling/cedarling/src/lock/mod.rs` around lines 620 - 626, Replace the two-step error check on the LockService::new call with a single expect_err and then assert the pattern on the captured error: call result.expect_err("failed to create LockService with invalid SSA JWT") to get the error, then use assert!(matches!(error, InitLockServiceError::InvalidSsaJwt(_)), "expected InvalidSsaJwt error when creating LockService"); this removes the initial assert!(result.is_err()) and unwrap_err() and provides descriptive messages for both the expect_err and the matches! assertion.jans-cedarling/cedarling/src/lock/register_client.rs (2)
145-154: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse
expect(...)here so failures keep the actual error.
assert!(result.is_ok())hides the registration error and gives you a much weaker failure signal than the repo standard requires.Suggested test cleanup
- let result = register_client( + register_client( pdp_id, &super::super::lock_config::Url(oidc_url), Some(&ssa_jwt.to_string()), false, crate::http::HttpClientConfig::default(), ) - .await; - - assert!(result.is_ok()); + .await + .expect("register_client should succeed when OIDC, DCR, and token endpoints are mocked"); oidc_endpoint.assert(); dcr_endpoint.assert(); token_endpoint.assert();- let result = register_client( + register_client( pdp_id, &super::super::lock_config::Url(oidc_url), None, false, crate::http::HttpClientConfig::default(), ) - .await; - - assert!(result.is_ok()); + .await + .expect("register_client should succeed without SSA when OIDC, DCR, and token endpoints are mocked"); oidc_endpoint.assert(); dcr_endpoint.assert(); token_endpoint.assert();As per coding guidelines, "For success checking in tests, use
result.expect("descriptive message")instead ofassert!(result.is_ok())" and "All assertions must include a descriptive message explaining what is being tested."Also applies to: 174-183
🤖 Prompt for 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. In `@jans-cedarling/cedarling/src/lock/register_client.rs` around lines 145 - 154, Replace the bare assertion that hides the error by calling expect on the Result returned from register_client so the test fails with the actual error message; specifically, change the assert!(result.is_ok()) after calling register_client(pdp_id, &super::super::lock_config::Url(oidc_url), Some(&ssa_jwt.to_string()), false, crate::http::HttpClientConfig::default()).await to result.expect("failed to register client for PDP id {pdp_id} with provided SSA"), and apply the same replacement for the other occurrence around the same register_client call (lines referencing result, ssa_jwt, and pdp_id) so all test success checks include a descriptive message and preserve the underlying error.
130-187: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd one failure-path test for the new HTTP client flow.
These tests only prove the happy path. Since this refactor replaced the old request wrapper with
HttpClientinitialization plusget_json/post_json, please add at least one negative case that asserts the mappedClientRegistrationErrorvariant for a failed OIDC fetch, DCR call, or token request.As per coding guidelines, "Include both positive and negative test cases for all tests."
🤖 Prompt for 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. In `@jans-cedarling/cedarling/src/lock/register_client.rs` around lines 130 - 187, Add a negative test for register_client that simulates a failing HTTP call (OIDC config fetch, DCR, or token) using the same mock Server used in test_register_client_with_ssa/test_register_client_without_ssa and assert the function returns the appropriate ClientRegistrationError variant; specifically, create a new tokio::test (e.g., test_register_client_oidc_fetch_failure) that sets up mock_oidc_endpoint to return an error (non-200 or malformed JSON) while keeping other endpoints as needed, call register_client (which uses HttpClient and its get_json/post_json), await the result, and assert it is Err(...) matching the mapped ClientRegistrationError variant for an OIDC fetch failure (or similarly for DCR/token if you choose that path).
🤖 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 `@docs/cedarling/reference/cedarling-properties.md`:
- Around line 80-81: The two retry-related properties are inconsistent: update
the documentation for CEDARLING_HTTP_REQUEST_RETRY_DELAY to match
CEDARLING_HTTP_REQUEST_MAX_RETRIES by explicitly marking both as "Only
applicable for native targets (not WASM)" and keep the same style/format and
default value presentation; specifically, edit the entries for
CEDARLING_HTTP_REQUEST_MAX_RETRIES and CEDARLING_HTTP_REQUEST_RETRY_DELAY so
both include the platform applicability note and consistent phrasing (e.g.,
"Only applicable for native targets (not WASM). Default is ...").
In `@jans-cedarling/cedarling/src/bootstrap_config/decode.rs`:
- Around line 137-141: from_raw_config constructs an HttpClientConfig using
raw.http_client_request_max_retries, raw.http_client_request_retry_delay, and
raw.http_client_request_timeout_millis unconditionally, but those raw fields are
cfg-gated away on wasm; wrap the HttpClientConfig construction in a
cfg(not(target_arch = "wasm32")) block (or provide a cfg(target_arch = "wasm32")
branch with wasm-safe defaults) so from_raw_config compiles for wasm targets;
reference the HttpClientConfig creation site and the raw.* fields to locate and
gate the code accordingly.
In `@jans-cedarling/cedarling/src/http/mod.rs`:
- Around line 161-168: post_json currently uses Sender::send which applies
automatic retries (via create_sender -> Sender::send) and can replay mutating
requests; change post_json to perform a single-shot send (no retries) by calling
a non-retrying API on Sender (e.g. send_once/send_no_retry) or add an explicit
parameter to post_json (e.g. allow_retries: bool) that only uses Sender::send
when true; update post_json signature and body to call the single-shot send
variant (or conditionalize on allow_retries) and preserve types T, F,
RequestBuilder, and HttpClientError so callers to post_json must opt into
retries for idempotent endpoints.
- Around line 70-77: The unconditional impl Default for HttpClientConfig
initializes request_timeout and uses DEFAULT_REQUEST_TIMEOUT which are missing
on wasm32; split the Default implementation by target: add one impl Default with
#[cfg(not(target_arch = "wasm32"))] that sets max_retries, retry_delay and
request_timeout using DEFAULT_REQUEST_TIMEOUT, and add a separate
#[cfg(target_arch = "wasm32")] impl Default that only sets max_retries and
retry_delay (omitting request_timeout), referencing the HttpClientConfig type,
Default impl, request_timeout field, and DEFAULT_REQUEST_TIMEOUT constant to
locate the code.
In `@jans-cedarling/cedarling/src/lock/mod.rs`:
- Around line 350-355: The call to init_http_client is currently mapped into
GetLockConfigError via .map_err(GetLockConfigError::from) which loses the
original InitializeHttpClientError; change the mapping so the init_http_client
error is preserved as the InitHttpClient (InitializeHttpClientError) variant
instead of converting to GetLockConfigError—i.e. replace the
.map_err(GetLockConfigError::from)? usage with a mapping that wraps or
propagates the InitializeHttpClientError (for example map_err(|e| /* preserve as
InitHttpClient variant */)?) so the error returned reflects
InitializeHttpClientError from init_http_client rather than GetLockConfigError.
In `@jans-cedarling/cedarling/src/lock/ssa_validation.rs`:
- Line 62: The doc comment in ssa_validation.rs concatenates two error bullets
into one line; split the merged entry so **`JwksFetchError`** and
**`MissingKeyId`** are each on their own bullet lines (e.g., end the line after
“Failed to fetch JWKS from IDP” and start a new line with “- **`MissingKeyId`**:
JWT header missing key ID”) so the rendered documentation shows two distinct
error entries.
- Around line 433-435: Remove the now-unreachable HttpClientError enum variant
and its #[from] conversion (which references InitializeHttpClientError) because
the HttpClient is injected and no code path in validate_ssa_jwt,
validate_ssa_jwt_with_config, or fetch_jwks can produce
InitializeHttpClientError; delete the HttpClientError variant from the error
enum, remove any uses of InitializeHttpClientError in that file (including the
import of InitializeHttpClientError), and run cargo check to ensure no other
code relied on that conversion.
In `@jans-cedarling/http_utils/src/lib.rs`:
- Around line 46-51: The HttpRequestError wrapper does not expose its inner
cause because the reason field lacks the error source attribute; add #[source]
to the reason field on the HttpRequestError struct so the inner
HttpRequestReasonError (and ultimately its reqwest::Error) appears in the error
chain, ensuring Error::source() and chain iteration surface the root cause
(verify the struct still derives/implements std::error::Error after the change).
- Around line 155-169: The retry failure currently drops HTTP status by
constructing HttpRequestError::new(HttpRequestReasonError::MaxRetriesExceeded,
None); capture the status from the response error (use err.status()) and pass it
into HttpRequestError::new (e.g., Some(status)) so the resulting error preserves
4xx/5xx metadata; keep the existing with_retry_count(attempt) and
with_last_error(err_msg) calls. Locate this in the Err(err) branch where
backoff.snooze().await.map_err(...) is used and update the constructor call to
include the extracted status.
---
Outside diff comments:
In `@jans-cedarling/cedarling/examples/lock_integration.rs`:
- Around line 9-13: The import list starting with HttpClientConfig is
misformatted: move HttpClientConfig onto its own line to match the multi-line
import block style used elsewhere (i.e., open the cedarling::{ line, then place
HttpClientConfig, AuthorizationConfig, BootstrapConfig, CedarEntityMapping,
Cedarling, DataStoreConfig, EntityData, JwtConfig, LockServiceConfig,
LockTransport, LogConfig, LogLevel, LogTypeConfig, PolicyStoreConfig,
PolicyStoreSource, RequestUnsigned each on their own indented line), then run
cargo fmt (using the project's rustfmt.toml) to ensure formatting consistency.
In `@jans-cedarling/cedarling/src/jwt/trusted_issuers_loader.rs`:
- Around line 1174-1178: The assertion uses
assert_eq!(loader.loading_state.failed_issuers().len(), 2, "...") but the
message incorrectly says "loading state should have 1 failed issuer"; update the
failure message in the assert_eq call to accurately reflect the expected value
(e.g., "loading state should have 2 failed issuers") so the assertion text
matches the expected 2 and helps debugging; locate the assert_eq call
referencing loader.loading_state.failed_issuers().len() and change only the
message string.
In `@jans-cedarling/cedarling/src/lock/mod.rs`:
- Around line 620-626: Replace the two-step error check on the LockService::new
call with a single expect_err and then assert the pattern on the captured error:
call result.expect_err("failed to create LockService with invalid SSA JWT") to
get the error, then use assert!(matches!(error,
InitLockServiceError::InvalidSsaJwt(_)), "expected InvalidSsaJwt error when
creating LockService"); this removes the initial assert!(result.is_err()) and
unwrap_err() and provides descriptive messages for both the expect_err and the
matches! assertion.
In `@jans-cedarling/cedarling/src/lock/register_client.rs`:
- Around line 145-154: Replace the bare assertion that hides the error by
calling expect on the Result returned from register_client so the test fails
with the actual error message; specifically, change the assert!(result.is_ok())
after calling register_client(pdp_id, &super::super::lock_config::Url(oidc_url),
Some(&ssa_jwt.to_string()), false,
crate::http::HttpClientConfig::default()).await to result.expect("failed to
register client for PDP id {pdp_id} with provided SSA"), and apply the same
replacement for the other occurrence around the same register_client call (lines
referencing result, ssa_jwt, and pdp_id) so all test success checks include a
descriptive message and preserve the underlying error.
- Around line 130-187: Add a negative test for register_client that simulates a
failing HTTP call (OIDC config fetch, DCR, or token) using the same mock Server
used in test_register_client_with_ssa/test_register_client_without_ssa and
assert the function returns the appropriate ClientRegistrationError variant;
specifically, create a new tokio::test (e.g.,
test_register_client_oidc_fetch_failure) that sets up mock_oidc_endpoint to
return an error (non-200 or malformed JSON) while keeping other endpoints as
needed, call register_client (which uses HttpClient and its get_json/post_json),
await the result, and assert it is Err(...) matching the mapped
ClientRegistrationError variant for an OIDC fetch failure (or similarly for
DCR/token if you choose that path).
🪄 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: 8287ed0c-99ce-4e7f-90c3-dd9bfa2c01b7
📒 Files selected for processing (36)
docs/cedarling/reference/cedarling-properties.mdjans-cedarling/cedarling/benches/authz_authorize_multi_issuer_benchmark.rsjans-cedarling/cedarling/benches/authz_authorize_unsigned_benchmark.rsjans-cedarling/cedarling/benches/context_data_store_benchmark.rsjans-cedarling/cedarling/benches/startup_benchmark.rsjans-cedarling/cedarling/examples/authorize_unsigned.rsjans-cedarling/cedarling/examples/bulk_authorization_benchmark.rsjans-cedarling/cedarling/examples/lock_integration.rsjans-cedarling/cedarling/examples/log_init.rsjans-cedarling/cedarling/examples/profiling_multi_issuer.rsjans-cedarling/cedarling/examples/profiling_unsigned.rsjans-cedarling/cedarling/src/bootstrap_config/decode.rsjans-cedarling/cedarling/src/bootstrap_config/mod.rsjans-cedarling/cedarling/src/bootstrap_config/raw_config/config.rsjans-cedarling/cedarling/src/bootstrap_config/raw_config/default_values.rsjans-cedarling/cedarling/src/common/policy_store.rsjans-cedarling/cedarling/src/http/mod.rsjans-cedarling/cedarling/src/init/policy_store.rsjans-cedarling/cedarling/src/init/service_config.rsjans-cedarling/cedarling/src/init/service_factory.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/cedarling/src/lib.rsjans-cedarling/cedarling/src/lock/lock_config.rsjans-cedarling/cedarling/src/lock/mod.rsjans-cedarling/cedarling/src/lock/register_client.rsjans-cedarling/cedarling/src/lock/ssa_validation.rsjans-cedarling/cedarling/src/lock/transport/rest.rsjans-cedarling/cedarling/src/log/mod.rsjans-cedarling/cedarling/src/tests/policy_store_loader.rsjans-cedarling/cedarling/src/tests/ssa_validation_integration.rsjans-cedarling/cedarling/src/tests/utils/cedarling_util.rsjans-cedarling/http_utils/src/lib.rs
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
mo-auto
added
area-documentation
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
jans-cedarling/cedarling/src/jwt/key_service.rs (1)
499-500:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUpdate remaining tests to pass the new
http_clientargument.Line 500, Line 546, Line 576, and Line 597 still call pre-refactor signatures, so test builds will fail after the method signature changes.
Suggested fix
+ let http_client = HttpClient::new(HttpClientConfig { + max_retries: 0, + retry_delay: Duration::from_millis(3), + request_timeout: Duration::from_millis(500), + }) + .expect("http client should be constructed"); let max_age = key_service - .refresh_keys_using_oidc(&openid_config, None) + .refresh_keys_using_oidc(&openid_config, None, &http_client) .await .expect("refresh with cache-control header should succeed");+ let http_client = HttpClient::new(HttpClientConfig { + max_retries: 0, + retry_delay: Duration::from_millis(3), + request_timeout: Duration::from_millis(500), + }) + .expect("http client should be constructed"); let max_age = key_service - .refresh_keys_using_oidc(&openid_config, None) + .refresh_keys_using_oidc(&openid_config, None, &http_client) .await .expect("refresh without cache-control header should succeed");+ let http_client = HttpClient::new(HttpClientConfig { + max_retries: 0, + retry_delay: Duration::from_millis(3), + request_timeout: Duration::from_millis(500), + }) + .expect("http client should be constructed"); key_service - .get_keys_using_oidc(&openid_config, None) + .get_keys_using_oidc(&openid_config, None, http_client.clone()) .await .expect("initial key fetch should succeed"); @@ key_service - .refresh_keys_using_oidc(&server.openid_config(), None) + .refresh_keys_using_oidc(&server.openid_config(), None, &http_client) .await .expect("refresh should succeed");Also applies to: 545-546, 575-576, 597-598
🤖 Prompt for 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. In `@jans-cedarling/cedarling/src/jwt/key_service.rs` around lines 499 - 500, Calls to key_service.refresh_keys_using_oidc still use the old two-arg signature; update each call to pass the new http_client parameter (i.e., call refresh_keys_using_oidc(&openid_config, None, &http_client) or &mut http_client as required by the new signature). Locate usages of refresh_keys_using_oidc in the tests (the calls inside the test functions that currently pass (&openid_config, None)) and either reuse the test's existing http client instance or construct one (e.g., reqwest::Client::new() or the test helper client) and pass it as the additional argument so the call matches the updated refresh_keys_using_oidc signature.jans-cedarling/cedarling/src/jwt/mod.rs (1)
911-923:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winTwo tests still call
JwtService::newwith the old argument list.Line 911 and Line 991 are missing the new
http_clientparameter, so these tests won’t compile with the updated constructor signature.Suggested fix
let jwt_service = JwtService::new( &JwtConfig { jwks: None, jwt_sig_validation: true, jwt_status_validation: false, signature_algorithms_supported: HashSet::from_iter([Algorithm::HS256]), jwks_refresh_min_interval: 0, ..Default::default() }, Some(HashMap::from([(server.issuer().to_string(), iss)])), None, Arc::new(MetricsCollector::new(0)), + HTTP_CLIENT.clone(), )let jwt_service = JwtService::new( &JwtConfig { jwks: None, jwt_sig_validation: true, jwt_status_validation: false, signature_algorithms_supported: HashSet::from_iter([Algorithm::HS256]), jwks_refresh_min_interval: 0, ..Default::default() }, Some(HashMap::from([(server.issuer().to_string(), iss)])), None, Arc::new(MetricsCollector::new(0)), + HTTP_CLIENT.clone(), )Also applies to: 991-1003
🤖 Prompt for 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. In `@jans-cedarling/cedarling/src/jwt/mod.rs` around lines 911 - 923, The JwtService::new calls are missing the new http_client argument; update both invocations (the one using JwtConfig {...} with Some(HashMap::from([(server.issuer().to_string(), iss)])) and the second similar call) to include a compatible http_client value as the additional parameter before the Arc::new(MetricsCollector::new(0)). Construct or pass the project's HTTP client type (for example an Arc-wrapped client instance such as Arc::new(reqwest::Client::new()) or the project's HttpClient) so the call signature matches JwtService::new(jwt_config, issuer_map, ???, http_client, Arc::new(MetricsCollector::new(0))). Ensure the same change is applied to both occurrences.
♻️ Duplicate comments (2)
docs/cedarling/reference/cedarling-properties.md (1)
79-81:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPlatform applicability is inconsistent between
CEDARLING_HTTP_REQUEST_MAX_RETRIESandCEDARLING_HTTP_REQUEST_RETRY_DELAY.Line 80 labels
CEDARLING_HTTP_REQUEST_MAX_RETRIESas native-only, while line 81 has no such restriction forCEDARLING_HTTP_REQUEST_RETRY_DELAY. Retry count and retry delay are both app-level settings that conceptually apply the same way on WASM. Once the unnecessary#[cfg(not(target_arch = "wasm32"))]gate is removed fromhttp_client_request_max_retriesinconfig.rs, the native-only qualifier should be dropped from line 80 as well.Suggested doc diff
-- **`CEDARLING_HTTP_REQUEST_MAX_RETRIES`** : Maximum number of retry attempts per request. Only applicable for native targets (not WASM). Default is `3`. +- **`CEDARLING_HTTP_REQUEST_MAX_RETRIES`** : Maximum number of retry attempts per request. Default is `3`.🤖 Prompt for 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. In `@docs/cedarling/reference/cedarling-properties.md` around lines 79 - 81, Update the docs so platform applicability is consistent: remove the "Only applicable for native targets (not WASM)" qualifier from the CEDARLING_HTTP_REQUEST_MAX_RETRIES entry so it matches CEDARLING_HTTP_REQUEST_RETRY_DELAY; this reflects the change you made in config.rs where you removed the #[cfg(not(target_arch = "wasm32"))] gate from http_client_request_max_retries, making retry count and retry delay both applicable on WASM and native targets.jans-cedarling/cedarling/src/bootstrap_config/decode.rs (1)
139-143:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winWASM compile failure:
from_raw_configaccesses cfg-gated fields without a cfg guard.Lines 140 and 142 reference
raw.http_client_request_max_retriesandraw.http_client_request_timeout_millis, both of which are#[cfg(not(target_arch = "wasm32"))]-gated inBootstrapConfigRaw. Line 142 also initialisesrequest_timeout, which is cfg-gated inHttpClientConfigitself.from_raw_confighas no cfg guard, so this fails to compile onwasm32.After removing the unnecessary
#[cfg]gate fromhttp_client_request_max_retriesinconfig.rs(the root cause for themax_retriespart), the remaining fix here is to guard therequest_timeoutfield:🔧 Proposed fix
let http_client_config = HttpClientConfig { max_retries: raw.http_client_request_max_retries, retry_delay: Duration::from_millis(raw.http_client_request_retry_delay), - request_timeout: Duration::from_millis(raw.http_client_request_timeout_millis), + #[cfg(not(target_arch = "wasm32"))] + request_timeout: Duration::from_millis(raw.http_client_request_timeout_millis), };🤖 Prompt for 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. In `@jans-cedarling/cedarling/src/bootstrap_config/decode.rs` around lines 139 - 143, from_raw_config constructs an HttpClientConfig using fields that are cfg-gated for non-wasm targets; specifically, request_timeout (and its source raw.http_client_request_timeout_millis from BootstrapConfigRaw) is only available off wasm, so wrap the code that reads raw.http_client_request_timeout_millis and sets HttpClientConfig.request_timeout in a cfg(not(target_arch = "wasm32")) guard (or use cfg_if) inside from_raw_config so the field is only accessed/initialized on non-wasm targets while keeping the rest of HttpClientConfig construction unchanged for wasm builds.
🤖 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/bootstrap_config/raw_config/config.rs`:
- Around line 344-370: Add unit tests in the existing config test module that
cover defaults and env-var overrides for the three new fields:
http_client_request_timeout_millis, http_client_request_max_retries, and
http_client_request_retry_delay; specifically, create tests that (1) load
RawConfig without those env vars and assert values equal the helper defaults
(default_http_client_request_timeout_millis, default_http_client_max_retries,
default_http_client_retry_delay), and (2) set each corresponding env var
(CEDARLING_HTTP_REQUEST_TIMEOUT_MILLIS, CEDARLING_HTTP_REQUEST_MAX_RETRIES,
CEDARLING_HTTP_REQUEST_RETRY_DELAY) to valid string and numeric values and
assert the deserialized values reflect the overrides (verifying
deserialize_or_parse_string_as_json behavior), plus at least one negative test
for invalid parsing of an env var to ensure deserialization fails as expected.
- Around line 355-362: The field http_client_request_max_retries is incorrectly
gated with #[cfg(not(target_arch = "wasm32"))]; remove that cfg so the field
(and its serde attributes: rename "CEDARLING_HTTP_REQUEST_MAX_RETRIES", default
= "default_http_client_max_retries", deserialize_with =
"deserialize_or_parse_string_as_json") exists on all targets, and relocate the
default_http_client_max_retries function out of any #[cfg(not(target_arch =
"wasm32"))] import block into the unrestricted imports so from_raw_config (in
decode.rs) can reference raw.http_client_request_max_retries on WASM without
compile errors.
---
Outside diff comments:
In `@jans-cedarling/cedarling/src/jwt/key_service.rs`:
- Around line 499-500: Calls to key_service.refresh_keys_using_oidc still use
the old two-arg signature; update each call to pass the new http_client
parameter (i.e., call refresh_keys_using_oidc(&openid_config, None,
&http_client) or &mut http_client as required by the new signature). Locate
usages of refresh_keys_using_oidc in the tests (the calls inside the test
functions that currently pass (&openid_config, None)) and either reuse the
test's existing http client instance or construct one (e.g.,
reqwest::Client::new() or the test helper client) and pass it as the additional
argument so the call matches the updated refresh_keys_using_oidc signature.
In `@jans-cedarling/cedarling/src/jwt/mod.rs`:
- Around line 911-923: The JwtService::new calls are missing the new http_client
argument; update both invocations (the one using JwtConfig {...} with
Some(HashMap::from([(server.issuer().to_string(), iss)])) and the second similar
call) to include a compatible http_client value as the additional parameter
before the Arc::new(MetricsCollector::new(0)). Construct or pass the project's
HTTP client type (for example an Arc-wrapped client instance such as
Arc::new(reqwest::Client::new()) or the project's HttpClient) so the call
signature matches JwtService::new(jwt_config, issuer_map, ???, http_client,
Arc::new(MetricsCollector::new(0))). Ensure the same change is applied to both
occurrences.
---
Duplicate comments:
In `@docs/cedarling/reference/cedarling-properties.md`:
- Around line 79-81: Update the docs so platform applicability is consistent:
remove the "Only applicable for native targets (not WASM)" qualifier from the
CEDARLING_HTTP_REQUEST_MAX_RETRIES entry so it matches
CEDARLING_HTTP_REQUEST_RETRY_DELAY; this reflects the change you made in
config.rs where you removed the #[cfg(not(target_arch = "wasm32"))] gate from
http_client_request_max_retries, making retry count and retry delay both
applicable on WASM and native targets.
In `@jans-cedarling/cedarling/src/bootstrap_config/decode.rs`:
- Around line 139-143: from_raw_config constructs an HttpClientConfig using
fields that are cfg-gated for non-wasm targets; specifically, request_timeout
(and its source raw.http_client_request_timeout_millis from BootstrapConfigRaw)
is only available off wasm, so wrap the code that reads
raw.http_client_request_timeout_millis and sets HttpClientConfig.request_timeout
in a cfg(not(target_arch = "wasm32")) guard (or use cfg_if) inside
from_raw_config so the field is only accessed/initialized on non-wasm targets
while keeping the rest of HttpClientConfig construction unchanged for wasm
builds.
🪄 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: 1ac2d812-90ca-4969-b2ad-c09dda6d92d8
📒 Files selected for processing (8)
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/jwt/http_utils.rsjans-cedarling/cedarling/src/jwt/key_service.rsjans-cedarling/cedarling/src/jwt/mod.rsjans-cedarling/cedarling/src/jwt/trusted_issuers_loader.rs
HttpClientConfig.request_timeout and BootstrapConfigRaw's
http_client_request_{max_retries,timeout_millis} are only available on
non-WASM targets, but decode.rs and the Default impl referenced them
unconditionally. Move default_http_client_retry_delay import outside
the wasm32 gate and add #[cfg] to all field accesses, fixing
wasm-pack test compilation.
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…r WASM compat Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…m error_for_status Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…rror Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…lidationError Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jans-cedarling/cedarling/src/jwt/http_utils.rs (1)
130-139:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize the media type before comparing with the status list content type.
The current strict literal comparison rejects valid HTTP responses that include media type parameters (e.g.,
application/statuslist+jwt; charset=utf-8) or differ in casing. Per RFC 7231, media types can include parameters and are case-insensitive. Extract the media type portion before the semicolon and compare case-insensitively to handle real-world server responses correctly.Proposed fix
if let Some(content_type) = response.headers().get("Content-Type") { let content_type = content_type .to_str() .map_err(|e| HttpError::InvalidHeader("Content-Type".to_string(), e))?; - if content_type != "application/statuslist+jwt" { + let media_type = content_type + .split(';') + .next() + .map(str::trim) + .unwrap_or_default(); + if !media_type.eq_ignore_ascii_case("application/statuslist+jwt") { return Err(HttpError::Unsupported(format!( "got unsupported status list type, '{0}', from '{1}'. Cedarling currently only supports 'application/statuslist+jwt'", content_type, url.as_str(), )));🤖 Prompt for 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. In `@jans-cedarling/cedarling/src/jwt/http_utils.rs` around lines 130 - 139, The code currently does a strict literal comparison of the Content-Type header which fails when parameters or casing differ; update the check in the block that reads response.headers().get("Content-Type") and sets the local content_type variable so that you parse out the media type (split on ';'), trim whitespace, convert to lowercase, then compare that value to "application/statuslist+jwt"; keep the existing InvalidHeader mapping and still return HttpError::Unsupported (including content_type and url.as_str()) when the normalized media type doesn't match.
♻️ Duplicate comments (3)
jans-cedarling/cedarling/src/http/mod.rs (2)
385-394:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
assert!(response.is_err(), …)still violates the test-style guideline.A previous review asked to replace this with
expect_err(...)and was marked "✅ Addressed", but the bareassert!is still here in the newget_times_out_when_server_never_respondstest. As per coding guidelines: "For error checking in tests, useresult.expect_err("descriptive message")instead ofassert!(result.is_err())".♻️ Proposed fix
- let start = std::time::Instant::now(); - let response = client.get(&format!("http://{addr}/")).await; - let elapsed = start.elapsed(); - - assert!(response.is_err(), "Expected timeout error: {response:?}"); + let start = std::time::Instant::now(); + client + .get(&format!("http://{addr}/")) + .await + .expect_err("Expected timeout error from non-responding server"); + let elapsed = start.elapsed();🤖 Prompt for 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. In `@jans-cedarling/cedarling/src/http/mod.rs` around lines 385 - 394, The test get_times_out_when_server_never_responds uses assert!(response.is_err(), ...) which violates the test-style guideline; replace that assertion with response.expect_err("descriptive message about expected timeout error") so the test fails with a clear error and shows the actual value; update the message to mention the expected timeout and keep the following elapsed check unchanged.
159-167:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReminder:
post_jsonstill issues retries implicitly.A prior review flagged that
post_jsongoes throughSender::send, which retries on transport errors and HTTP error responses, and that this is unsafe for non-idempotent endpoints (a timeout/reset after the server processed the write would be replayed and could double-apply side effects). That comment does not appear addressed in this revision — please confirm whether retries are intentional for current callers or whether this should be made single-shot / opt-in. Today the only caller chain that ends up here is DCR / token refresh on the lock side, which is non-idempotent.🤖 Prompt for 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. In `@jans-cedarling/cedarling/src/http/mod.rs` around lines 159 - 167, post_json currently calls Sender::send which performs automatic retries (transport and HTTP error retries), making non-idempotent calls unsafe; update the implementation so post_json does a single-shot send by either calling a non-retrying path (e.g. add/use Sender::send_once or a send(..., retry: false) variant) or add an explicit opt-in retry flag to post_json and its callers; adjust create_sender/Caller sites (notably the DCR/token refresh path) to use the new non-retrying call unless retries are intentionally required. Ensure function names referenced: post_json, create_sender, Sender::send (and add Sender::send_once or send with retry control) are updated consistently.jans-cedarling/cedarling/src/bootstrap_config/raw_config/config.rs (1)
356-363:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
http_client_request_max_retriescfg-gating remains inconsistent.
HttpClientConfig::max_retriesexists on all targets (retry logic is app-level, not browser-delegated), andhttp_client_request_retry_delaybelow is correctly ungated. Keepinghttp_client_request_max_retriesunder#[cfg(not(target_arch = "wasm32"))]while its sibling is unconditional means WASM consumers can configure retry delay but not retry count.decode.rsalready has acfg(target_arch = "wasm32")fallback toDEFAULT_MAX_RETRIES, so the wasm build compiles, but the bootstrap surface remains asymmetric.♻️ Proposed fix
- /// Maximum number of retry attempts per request. - #[cfg(not(target_arch = "wasm32"))] - #[serde( + /// Maximum number of retry attempts per request. + #[serde( rename = "CEDARLING_HTTP_REQUEST_MAX_RETRIES", default = "default_http_client_max_retries", deserialize_with = "deserialize_or_parse_string_as_json" )] pub http_client_request_max_retries: u32,Then move
default_http_client_max_retriesout of the cfg-gated import indefault_values.rs/config.rsand drop the#[cfg(target_arch = "wasm32")]fallback branch indecode.rs.🤖 Prompt for 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. In `@jans-cedarling/cedarling/src/bootstrap_config/raw_config/config.rs` around lines 356 - 363, The field http_client_request_max_retries is wrongly cfg-gated; make it unconditional like its sibling http_client_request_retry_delay so WASM consumers can set the retry count, move default_http_client_max_retries out of any #[cfg(not(target_arch = "wasm32"))] import so the default is available on all targets, and then remove the cfg(target_arch = "wasm32") fallback branch in decode.rs (which currently supplies DEFAULT_MAX_RETRIES) so decoding uses the unified default; reference HttpClientConfig::max_retries, http_client_request_max_retries, default_http_client_max_retries, and the fallback branch in decode.rs when making the changes.
🤖 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 123-126: The GET request built with
client.get_with_retry_with(...) is incorrectly setting a "Content-Type" header
(via the closure) but must use "Accept" to indicate the expected response media
type; change the header in the closure passed to get_with_retry_with to use
"Accept: application/statuslist+jwt" so the server can return the correct
representation for the subsequent exact-match validation in your code (the
closure passed to get_with_retry_with and the request URL variable named url are
the key symbols to update).
---
Outside diff comments:
In `@jans-cedarling/cedarling/src/jwt/http_utils.rs`:
- Around line 130-139: The code currently does a strict literal comparison of
the Content-Type header which fails when parameters or casing differ; update the
check in the block that reads response.headers().get("Content-Type") and sets
the local content_type variable so that you parse out the media type (split on
';'), trim whitespace, convert to lowercase, then compare that value to
"application/statuslist+jwt"; keep the existing InvalidHeader mapping and still
return HttpError::Unsupported (including content_type and url.as_str()) when the
normalized media type doesn't match.
---
Duplicate comments:
In `@jans-cedarling/cedarling/src/bootstrap_config/raw_config/config.rs`:
- Around line 356-363: The field http_client_request_max_retries is wrongly
cfg-gated; make it unconditional like its sibling
http_client_request_retry_delay so WASM consumers can set the retry count, move
default_http_client_max_retries out of any #[cfg(not(target_arch = "wasm32"))]
import so the default is available on all targets, and then remove the
cfg(target_arch = "wasm32") fallback branch in decode.rs (which currently
supplies DEFAULT_MAX_RETRIES) so decoding uses the unified default; reference
HttpClientConfig::max_retries, http_client_request_max_retries,
default_http_client_max_retries, and the fallback branch in decode.rs when
making the changes.
In `@jans-cedarling/cedarling/src/http/mod.rs`:
- Around line 385-394: The test get_times_out_when_server_never_responds uses
assert!(response.is_err(), ...) which violates the test-style guideline; replace
that assertion with response.expect_err("descriptive message about expected
timeout error") so the test fails with a clear error and shows the actual value;
update the message to mention the expected timeout and keep the following
elapsed check unchanged.
- Around line 159-167: post_json currently calls Sender::send which performs
automatic retries (transport and HTTP error retries), making non-idempotent
calls unsafe; update the implementation so post_json does a single-shot send by
either calling a non-retrying path (e.g. add/use Sender::send_once or a
send(..., retry: false) variant) or add an explicit opt-in retry flag to
post_json and its callers; adjust create_sender/Caller sites (notably the
DCR/token refresh path) to use the new non-retrying call unless retries are
intentionally required. Ensure function names referenced: post_json,
create_sender, Sender::send (and add Sender::send_once or send with retry
control) are updated consistently.
🪄 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: 2c7dfd83-6b73-4a40-ac3b-27f10412f48f
📒 Files selected for processing (10)
jans-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/jwt/http_utils.rsjans-cedarling/cedarling/src/jwt/key_service.rsjans-cedarling/cedarling/src/jwt/mod.rsjans-cedarling/cedarling/src/lock/mod.rsjans-cedarling/cedarling/src/lock/ssa_validation.rsjans-cedarling/http_utils/src/lib.rs
…equests Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…st GET request Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Group shared state into a `StatusListUpdateCtx` struct to fix clippy::too_many_arguments warning. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
dagregi
previously approved these changes
May 6, 2026
Signed-off-by: dagregi <dagmawi.m@proton.me>
olehbozhok
approved these changes
May 6, 2026
dagregi
approved these changes
May 6, 2026
6 tasks
Merged
6 tasks
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The two outbound HTTP clients in
jans-cedarlingare built fromreqwest::Clientdefaults, which set no timeouts at all. A slow or unresponsive issuer or policy-store host can stall a Cedarling task forever and exhaust the tokio runtime. This PR puts a bound on both of them.Implementation Details
LazyLock::new(Client::new)with a builder that sets a 10s request timeout and 5s connect timeout.get_times_out_when_server_never_responds) that accepts connections without ever responding and assertsHttpClient::getreturns an error within ~200msTest 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 #14002
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests