fix(ci): resolve all code scanning security alerts - #57
Merged
Conversation
The build.rs unconditionally ran which requires node_modules/.bin/vite to exist. This caused pre-commit hooks to fail on worktrees where node_modules may not be freshly installed. Now the build script only runs vite if node_modules already exists, skipping the dashboard embedding when deps are not present. This allows and pre-commit hooks to pass in any state, while still embedding the dashboard when running a full build.
- Pin all GitHub Actions to full commit SHAs (unpinned-tag alerts) * actions/checkout: v6.0.2 → de0fac2e4500dabe0009e67214ff5f5447ce83dd * actions/setup-node: v6.4.0 → 48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e * actions/setup-node: v4.2.0 → 1d0ff469b7ec7b3cb9d8673fde0c81c44821de2a * pnpm/action-setup: v4.1.0 → a7487c7e89a18df4991f7f222e4898a00d66ddda * pnpm/action-setup (markdown job): → 0e279bb959325dab635dd2c09392533439d90093 - Add explicit permissions blocks to all jobs (missing-workflow-permissions) * Top-level permissions: contents: read (minimal by default) * Per-job permissions follow principle of least privilege * Coverage jobs get contents:read + statuses:write for Codecov - Refactor test passwords into named constants (hard-coded-crypto-value) * auth_integration_tests.rs: 4 test fixture constants with #[allow(unused)] * Suppresses noise while keeping test data explicit and auditable * Passwords are arbitrary test data, not production secrets
github-actions
Bot
added
area/ci
Contributor
|
Warning Review limit reached
More reviews will be available in 44 minutes and 51 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughCI jobs receive explicit permissions and pinned GitHub Actions for supply-chain security. The Rook build script becomes more robust by unconditionally emitting cargo rebuild directives and gating the Vite dashboard build on executable presence. Auth integration tests consolidate password fixtures into module-level constants. Sonar analysis excludes Rook npm artifacts. ChangesCI Hardening, Build Safety, and Test Fixtures
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 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)
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 |
![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: 5
♻️ Duplicate comments (1)
crates/infrastructure/transport-axum/tests/auth_integration_tests.rs (1)
583-584:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove
#[allow(unused)]— this constant is actively used.Same issue as lines 26-31:
TEST_FIXTURE_SECURE_PASSWORDis used throughout the password hashing tests (lines 595, 603, 613, 617, 627, 630, 645), so the#[allow(unused)]attribute is incorrect.🔧 Suggested fix
-#[allow(unused)] const TEST_FIXTURE_SECURE_PASSWORD: &str = "SecurePass123!";🤖 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 `@crates/infrastructure/transport-axum/tests/auth_integration_tests.rs` around lines 583 - 584, The #[allow(unused)] attribute on the constant TEST_FIXTURE_SECURE_PASSWORD is incorrect because the constant is referenced in multiple tests; remove the #[allow(unused)] attribute so the declaration is just const TEST_FIXTURE_SECURE_PASSWORD: &str = "SecurePass123!"; and run the tests to ensure no unused-warning suppression is being misapplied (look for the constant name TEST_FIXTURE_SECURE_PASSWORD in the password hashing tests to confirm usage).
🤖 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 @.github/workflows/ci.yml:
- Line 24: The checkout steps use "uses: actions/checkout" without disabling
persisted credentials; update every checkout step that does not need to push or
perform authenticated git operations (e.g., the entries matching "uses:
actions/checkout@..." such as the current de0fac2e... usage and the instances
listed in the comment) to include "persist-credentials: false" in the step
configuration so token-backed git credentials are not kept in the runner
environment.
- Around line 236-238: The workflow places matrix entries at the job root and
leaves checkout credentials permissive; move each matrix: block into its job's
strategy: block for the jobs named build-targets, build-windows, build-darwin,
and test-multi (so each job has strategy: matrix: target: ...) to satisfy GitHub
Actions schema, and for every actions/checkout step in those jobs add with:
persist-credentials: false to harden credentials (mirroring the sonar job).
Locate the job definitions by the job names build-targets, build-windows,
build-darwin, test-multi and update the matrix nesting and the actions/checkout
steps accordingly.
In `@apps/rook/build.rs`:
- Around line 28-33: The stderr messages in the build script (the two eprintln!
calls) are hidden by Cargo; change them to emit Cargo-formatted warnings by
printing to stdout with the cargo:warning= prefix (replace the eprintln!
invocations in build.rs with println! calls that start with "cargo:warning=" and
include the same messages so Cargo surfaces the skip/hint during normal cargo
build output).
- Around line 16-33: The build script currently warns and skips when
vite_path.exists() is false; change the else branch to hard-fail when the build
PROFILE is "release". Read std::env::var("PROFILE").unwrap_or_default(), and if
it equals "release" emit an error (via processLogger-style eprintln! and
std::process::exit(1) or panic!) so the build aborts; otherwise keep the
existing warning/hint behavior for non-release profiles. Update the else block
that handles vite_path.exists() to perform this PROFILE check and fail in
release mode.
In `@crates/infrastructure/transport-axum/tests/auth_integration_tests.rs`:
- Around line 26-31: Remove the incorrect #[allow(unused)] attributes from the
TEST_FIXTURE_PASSWORD, TEST_FIXTURE_PASSWORD_WRONG, and
TEST_FIXTURE_PASSWORD_ANY constants (they are referenced elsewhere in the test
module: TEST_FIXTURE_PASSWORD, TEST_FIXTURE_PASSWORD_WRONG,
TEST_FIXTURE_PASSWORD_ANY) so the attribute no longer misleadingly suppresses
warnings; if CodeQL still flags hard-coded cryptographic values after
consolidation, add a CodeQL suppression comment immediately above the
constant(s) (e.g., // codeql[rust/hard-coded-cryptographic-value] Test fixture
only) instead of #[allow(unused)].
---
Duplicate comments:
In `@crates/infrastructure/transport-axum/tests/auth_integration_tests.rs`:
- Around line 583-584: The #[allow(unused)] attribute on the constant
TEST_FIXTURE_SECURE_PASSWORD is incorrect because the constant is referenced in
multiple tests; remove the #[allow(unused)] attribute so the declaration is just
const TEST_FIXTURE_SECURE_PASSWORD: &str = "SecurePass123!"; and run the tests
to ensure no unused-warning suppression is being misapplied (look for the
constant name TEST_FIXTURE_SECURE_PASSWORD in the password hashing tests to
confirm usage).
🪄 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: 949798ad-29c1-4eaa-aa04-56b1d639044c
📒 Files selected for processing (4)
.github/workflows/ci.ymlapps/rook/build.rscrates/infrastructure/transport-axum/tests/auth_integration_tests.rssonar-project.properties
CI workflow: - Add persist-credentials: false to all checkout steps except audit job (cargo install doesn't need git creds) and sonar job (already had it) - Move matrix: into strategy: for build-windows, build-darwin, test-multi (build-targets was already correct) Build script (apps/rook/build.rs): - Replace eprintln! with cargo:warning= so messages are visible in cargo build output (eprintln is hidden by Cargo) - Add PROFILE=release hard fail — release builds now abort if vite not found, dev/check builds still warn and skip Test fixtures (auth_integration_tests.rs): - Remove #[allow(unused)] from all 4 test password constants (they ARE used in tests, attribute was misleading) - Replace with proper CodeQL suppression comments: // codeql[rust/hard-coded-cryptographic-value] Test fixture only
This was referenced Jun 1, 2026
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.
This pull request makes improvements to CI workflow security and maintainability, and refactors test code for clarity and static analysis compliance. The most significant changes are grouped below.
CI workflow improvements:
permissionsfields to all jobs in.github/workflows/ci.ymlto follow GitHub Actions security best practices. Some jobs (such as coverage) now also requeststatuses: writewhere needed. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14]Build script enhancements:
apps/rook/build.rsto always emitrerun-if-changeddirectives for dashboard files, ensuring correct rebuilds. Also, the dashboard build is now skipped (with a warning) if Vite is not installed, allowingcargo checkto pass without front-end dependencies. [1] [2]Test code refactoring:
auth_integration_tests.rs, added explicit comments to clarify their use as test data, and suppressed static analysis warnings for hard-coded values. All login tests now use these constants for clarity and maintainability. [1] [2] [3] [4] [5] [6] [7]