Refactor runtime option resolution into focused modules#245
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 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 |
|
Droid finished @EffortlessSteven's task —— View job Review complete for PR #245 (c2917c6) The refactoring correctly preserves runtime-option resolution semantics while improving code organization. One pre-existing SSRF risk was identified in registry fallback URL construction that should be addressed before merging. Validation: 1 comment posted (approved). 0 comments rejected. |
|
|
||
| fn default_registry_for_name(name: &str) -> Registry { | ||
| if name == "crates-io" { | ||
| Registry::crates_io() |
There was a problem hiding this comment.
[P1] SSRF risk in default_registry_for_name from unvalidated CLI input
When --registries is passed with a registry name not found in config, default_registry_for_name constructs format!("https://{name}.crates.io") by directly interpolating the user-supplied name argument. An attacker controlling the CLI --registries flag could probe internal infrastructure by specifying names like 169-254-169-254.crates.io or other internal hostname patterns as a crates.io subdomain. This is a pre-existing risk inherited from the original inline code, but the extraction into a dedicated function makes the attack surface more visible.
fn default_registry_for_name(name: &str) -> Registry {
if name == "crates-io" {
Registry::crates_io()
} else {
Registry {
name: name.to_string(),
api_base: format!("https://{name}.crates.io"),
index_base: None,
}
}
}Consider validating that name contains only permitted characters (e.g., alphanumeric, dash) before URL construction, or restrict to an allowlist of known registry identifiers.
|
Droid finished @EffortlessSteven's task —— View job Validation result: approved No candidate comments to validate — review_candidates.json contains an empty comments array. The prior SSRF finding posted as PR comment 3233464394 has already been addressed by this PR via the new Validation: 0 comments posted (approved). 0 comments rejected. |
Motivation
ShipperConfig::build_runtime_optionsimplementation had grown into a large, complex function that mixed many responsibilities and made reasoning and testing harder.Description
ShipperConfig::build_runtime_optionswith a small delegation point that callsruntime_options::buildand preserved the public API and precedence semantics.crates/shipper-config/src/runtime_options/mod.rswhich assembles top-levelRuntimeOptionsand factors out helpers for readiness, parallel, state-dir, and rehearsal resolution.retry.rs,registry.rs, andsecrets.rsundercrates/shipper-config/src/runtime_options/to encapsulate retry-policy resolution, multi-registry selection/fallback, and webhook/encryption override logic respectively.RetryPolicy::Customretains its semantics when resolving retry fields.Testing
cargo fmt --all -- --checkwhich succeeded.cargo test -p shipper-configand all tests passed (crate unit and integration tests completed successfully).cargo clippy -p shipper-config --all-targets --all-features -- -D warningswhich completed without warnings.Codex Task