fix(security): resolve open code scanning and Dependabot alerts - #77
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR updates static analysis tooling configuration to exclude test files from gitleaks and Semgrep scanning, modernizes TTY detection to use Rust's standard library instead of the atty crate, and adds a Docker health check to the development container. ChangesTooling configuration and code modernization
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 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 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 @.gitleaks.toml:
- Around line 38-39: Remove the broad allowlist entries that silence gitleaks
for entire source modules and instead target only the test fixtures or literals;
specifically, delete the patterns that reference
crates/application/rook-usecases/src/manage_connections\.rs and
crates/application/rook-usecases/src/auth/set_admin_password\.rs from
.gitleaks.toml and add narrow exceptions for the exact fake-token strings or
move those fixtures into dedicated test-only files or modules (e.g., a tests/ or
#[cfg(test)]-only file) so that production code in manage_connections.rs and
set_admin_password.rs remains scanned by gitleaks.
In `@apps/rook/Cargo.toml`:
- Line 46: Update the pinned is-terminal dependency declaration to a current
release (change the is-terminal = "0.2" entry to is-terminal = "0.4.17") so the
project uses the latest crate; alternatively, if the project's MSRV is Rust 1.70
or newer, remove the is-terminal crate usage and switch call sites referencing
the is-terminal crate to the std::io::IsTerminal trait (replace uses of the
external crate's API with std::io::IsTerminal on Read/Write types) and remove
the is-terminal entry from Cargo.toml.
In `@apps/rook/src/config.rs`:
- Around line 112-113: The nosemgrep suppression is placed inside the reusable
RookConfig::load function, which hides tainted-path findings for all callers;
remove the inline suppression from RookConfig::load (the line above let content
= std::fs::read_to_string(path)? ) so the loader remains a general-purpose API,
and instead add the nosemgrep suppression at the specific CLI-controlled call
site that constructs/passes the trusted config path (the function that parses
CLI args or the entrypoint that calls RookConfig::load with the CLI-derived
path) so only that boundary is exempted.
In `@apps/rook/src/main.rs`:
- Line 206: Import the IsTerminal trait (use std::io::IsTerminal;) near the top
of the file, then replace the fully-qualified call
IsTerminal::is_terminal(&std::io::stderr()) with the idiomatic method call
std::io::stderr().is_terminal() so the is_terminal method is invoked directly on
the stderr stream (referencing the IsTerminal trait and the stderr() call).
In `@Dockerfile.dev`:
- Around line 57-58: The HEALTHCHECK uses curl but the runtime stage based on
debian:bookworm-slim doesn't include it, so update the runtime stage in the
Dockerfile to install curl (e.g., add apt-get update && apt-get install -y
--no-install-recommends curl and cleanup /var/lib/apt/lists) so the HEALTHCHECK
CMD curl -f http://localhost:8080/health || exit 1 can run; modify the runtime
stage block that sets FROM debian:bookworm-slim and the HEALTHCHECK line to
include the curl install step and keep layers minimal.
🪄 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: c309b63c-0f0c-45cf-8782-5805a8c585b9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.gitleaks.toml.semgrepignoreDockerfile.devapps/rook/Cargo.tomlapps/rook/src/config.rsapps/rook/src/main.rscrates/application/rook-usecases/src/manage_connections.rs
Comment on lines
+38
to
+39
| '''crates/application/rook-usecases/src/auth/set_admin_password\.rs''', | ||
| '''crates/application/rook-usecases/src/manage_connections\.rs''', |
Contributor
There was a problem hiding this comment.
Don't allowlist entire source modules under src/.
These entries disable gitleaks for every line in those files, not just the fake test tokens. crates/application/rook-usecases/src/manage_connections.rs already mixes production code with #[cfg(test)] blocks, so future real secrets in the non-test path would be silently ignored by CI. Prefer a narrower suppression on the specific fixture literals or move those fixtures into dedicated test-only files.
🤖 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 @.gitleaks.toml around lines 38 - 39, Remove the broad allowlist entries that
silence gitleaks for entire source modules and instead target only the test
fixtures or literals; specifically, delete the patterns that reference
crates/application/rook-usecases/src/manage_connections\.rs and
crates/application/rook-usecases/src/auth/set_admin_password\.rs from
.gitleaks.toml and add narrow exceptions for the exact fake-token strings or
move those fixtures into dedicated test-only files or modules (e.g., a tests/ or
#[cfg(test)]-only file) so that production code in manage_connections.rs and
set_admin_password.rs remains scanned by gitleaks.
Comment on lines
112
to
113
| // nosemgrep: rust.actix.path-traversal.tainted-path.tainted-path | ||
| let content = std::fs::read_to_string(path)?; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Keep the suppression at the trust boundary, not inside RookConfig::load.
RookConfig::load is a reusable file-reading API. Putting nosemgrep here suppresses tainted-path findings for every current and future caller, not just the CLI-controlled one that motivated this change. Prefer suppressing at the specific call site that passes the trusted config path.
🤖 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 `@apps/rook/src/config.rs` around lines 112 - 113, The nosemgrep suppression is
placed inside the reusable RookConfig::load function, which hides tainted-path
findings for all callers; remove the inline suppression from RookConfig::load
(the line above let content = std::fs::read_to_string(path)? ) so the loader
remains a general-purpose API, and instead add the nosemgrep suppression at the
specific CLI-controlled call site that constructs/passes the trusted config path
(the function that parses CLI args or the entrypoint that calls RookConfig::load
with the CLI-derived path) so only that boundary is exempted.
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
1 participant
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.
What
Security hardening pass — resolves open GitHub Security alerts (Code Scanning + Dependabot).
Changes
Dockerfile.devHEALTHCHECK+curlto runtime stageapps/rook/src/main.rsnosemgrepfromRookConfig::loadto the two CLI-level call sites (load_config,load_config_with_path).gitleaks.tomlmanage_connections.rsandset_admin_password.rs(production code now fully scanned)manage_connections.rsnosemgrepattyunmaintained (GHSA-g98v-hv3f-hcfr)Cargo.toml,main.rsatty = "0.2"withis-terminal = "0.4", idiomaticstderr().is_terminal()withuse std::io::IsTerminalAlready resolved (no action needed)
bootstrap_status.rsc06d517— file now has 214 lines and no credentialstemp_dirshared_test_db(), not security-sensitiveWhy
attyis unmaintained; replaced with stdlibIsTerminal(Rust 1.70+, project uses 1.89)nosemgrepmoved to call sites soRookConfig::loadremains a clean general-purpose APImanage_connections.rsandset_admin_password.rsis fully scannedTesting
cargo check --workspace— pre-existingdashboard/distmissing error (unrelated to these changes)semgrep scan --config=auto .— 0 findingsgit diff --statshows only targeted changesRelated Issues
Fixes GitHub Security alerts:
atty)