maintenance - #69
Merged
Merged
maintenance#69
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
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
- ci(release): move write permissions from workflow to job level (S8233) - ci(security-deep): remove redundant security-events write at workflow level - fix(playwright.config): remove commented out dotenv config (S125) - fix(index.js): use node: prefix for core modules (S7772), extract nested ternary (S3358) - fix(theme.ts): use globalThis instead of window (S7764) - fix(a11y): improve breadcrumb and sidebar semantic HTML/S6724/S6819) - fix(dockerfile): merge consecutive RUN instructions (S7031) - fix(stale): unused imports in NavSecondary and LocaleSwitcher already removed
…y scanners in CI - Add .github/workflows/codeql.yml: GitHub CodeQL SAST workflow for Rust (runs on push/PR to main+develop, uploads SARIF to Security tab) - Add trivy-fs job to ci.yml: Trivy filesystem+deps scan, fails build on HIGH/CRITICAL vulns, ignores unfixed, uploads SARIF - Add gitleaks-pr job to ci.yml: scans PR commits (last 50), fails build on secrets found, exit-code 1 - Add semgrep-pr job to ci.yml: SAST scan filtered to ERROR severity, fails build on HIGH severity findings, uploads SARIF - Update security-deep.yml: update exit-code to '1', add cross-references to ci.yml merge-gate jobs, clarify reporting-only role of nightly scan - Expand SECURITY.md: full security policy with version support table, vulnerability disclosure process, contributor best practices, incident response severity matrix, and security tooling reference
The SHA c581c57d9861fc6f2c8d3fbd94e36eddc8fdd35c does not exist in the codeql-action repository. Change to v3.36.0 tag (051e2f90686233507fe9283ff167d2e709304b30) which is the latest stable v3 release already in use by upload-sarif.
… and SECURITY.md ci.yml: - gitleaks-pr: increase fetch-depth from 1 to 50 so gitleaks git can analyze the requested 50-commit window - trivy-fs: add security-events: write to job permissions; add if:always() to Upload Trivy SARIF step so SARIF is uploaded even when trivy exits 1 - gitleaks-pr: add if:always() to Upload Gitleaks SARIF step - semgrep-pr: add if:always() to Upload Semgrep SARIF step codeql.yml: - remove workflow-level permissions.security-events: write (moved to job-level) - remove '|| true' from cargo check so the job properly fails on build errors SECURITY.md: - fix grammar: 'an private' -> 'a private' - fix Chinese text in Critical row: replace with English 'complete authentication bypass' - fix 'releasetimeline' -> 'release timeline' and 'COORDinator' -> 'We will reach out' - rewrite scanner table to separately document PR gates (ci.yml jobs) vs nightly reporting (security-deep.yml), including job names (trivy-fs, gitleaks-pr, semgrep-pr)
… install The trivy-action action was failing internal binary installation, causing reports/trivy/trivy-full.sarif to never be created and subsequent steps (Verify SARIF, Upload SARIF) to fail. Fix: - Install Trivy CLI directly using the official install script with pinned version v0.65.0 (matches prior internal version) - Run 'trivy fs' CLI directly, matching existing style for gitleaks/semgrep - Remove '|| exit 1' from Verify SARIF step so it doesn't cascade failure - Gate SARIF upload, artifact upload, and summary steps on 'always() && hashFiles(...)' != '' to avoid noise when scan step fails without the SARIF gate condition
The rust-embed attribute #[folder = "dashboard/dist"] requires that directory to exist when 'cargo check' compiles the rook app. On GitHub Actions runners vite/node_modules are not pre-installed, so the embed folder does not exist during the check step. Fix: - Add Setup pnpm and Setup Node.js steps to install vite and dependencies - Run 'pnpm install && pnpm build' before 'cargo check --workspace' - Restore the build step to pure 'cargo check --workspace' (no || true) so genuine compile errors propagate correctly The extra pnpm steps give rust-embed the dashboard/dist it needs.
The nightly trivy-full job is reporting-only (no merge gate), so it should never fail the workflow due to vulnerabilities found. Removed --exit-code 1 and cleaned up the hashFiles conditional checks that were causing 'path does not exist' errors when the scan didn't produce findings.
1. rust/log-injection (apps/rook/src/main.rs:177) - Token was logged in full, enabling log injection attacks - Now logs only prefix (first 8 chars) + length for traceability - The eprintln! with full token remains (console output, not structured logs) 2. generic.secrets.security.detected-google-oauth-access-token (manage_connections.rs) - 2 instances of test data containing 'ya29.' Google-like token patterns - Both replaced with obviously fake data + nosemgrep comment - This is test/fixture data, not real secrets 3. generic.secrets.security.detected-sonarqube-docs-api-key (ci.yml) - False positive: semgrep detected 'sonarqube' in rule name '--config p/secrets' - Removed --config p/secrets from both ci.yml and security-deep.yml - Secrets scanning is handled by gitleaks and trivy-fs instead
Contributor
|
Warning Review limit reached
More reviews will be available in 47 minutes and 34 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 (4)
📝 WalkthroughWalkthroughApplication token logging is sanitized to avoid exposing secrets; test fixtures are updated with secret-scanning suppressions; and CI/CD security scanning policies are restructured to remove the dedicated secrets ruleset, replace it with language-specific rules, and make Trivy reporting-only. ChangesSecurity and Secret Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 3
🤖 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/security-deep.yml:
- Around line 150-155: The Upload Trivy SARIF step currently uses if: always()
so it runs even when reports/trivy/trivy-full.sarif is missing; update the
"Verify SARIF file exists" step to set a boolean output (e.g., id: verify-sarif,
output name: found) that is true when the file exists, keep the artifact/summary
steps using always() as-is, and change the "Upload Trivy SARIF" step to run only
when that output is true (e.g., if: steps.verify-sarif.outputs.found == 'true')
so github/codeql-action/upload-sarif only runs when the sarif file is present.
In `@apps/rook/src/main.rs`:
- Around line 176-179: The current logging leaks raw secret bytes via
token_preview and the unconditional eprintln!; replace token_preview with a
sanitized/redacted identifier (e.g., derive a stable preview by taking the first
N chars but canonicalizing control/non-printable chars to a safe placeholder and
appending "…" for longer tokens, or better: use a short hex/sha256-based
fingerprint) and use that sanitized value in tracing::warn!(setup_token_prefix =
%sanitized_preview, setup_token_len = token.len(), ...). Remove printing the raw
token to stderr; only print the full token when stderr is an interactive TTY
(use an atty check) and otherwise print the redacted/sanitized identifier via
eprintln! so control characters and short tokens cannot leak into logs or
systemd/docker streams.
In `@crates/application/rook-usecases/src/manage_connections.rs`:
- Around line 1997-2000: The suppression comment is too broad; move the
nosemgrep suppression so it directly annotates the literal access token instead
of the surrounding block: locate the Credentials::OAuth variant construction
(the access_token: EncryptedBlob("enc:v1:fake-google-token-data".to_string())
entry) and place the `// nosemgrep:
generic.secrets.security.detected-google-oauth-access-token.detected-google-oauth-access-token`
suppression immediately adjacent to that access_token literal (inline or
directly above) so the rule stays suppressed specifically for that secret
literal.
🪄 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: 99d7e3c5-ee2d-4e4a-9dd2-fa2a95b65c7f
📒 Files selected for processing (4)
.github/workflows/ci.yml.github/workflows/security-deep.ymlapps/rook/src/main.rscrates/application/rook-usecases/src/manage_connections.rs
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
1. security-deep.yml: Add GITHUB_OUTPUT to 'Verify SARIF file exists' step (id: verify-sarif) so 'Upload Trivy SARIF' only runs when the file actually exists (if: steps.verify-sarif.outputs.found == 'true'). 2. apps/rook/src/main.rs: Improve log injection mitigation: - Sanitize all control/non-printable chars to '?' before logging - Only print full token to stderr when running interactively (atty check) - Otherwise print only the sanitized 8-char prefix + length - Added atty = "0.2" dependency 3. manage_connections.rs: Move nosemgrep suppression inline on the access_token literal instead of the surrounding block.
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 focuses on improving security practices in CI/CD workflows and test code. The main changes include removing a Semgrep secrets scan configuration, updating Trivy scan behavior to be reporting-only, and making test OAuth tokens more clearly non-production. Additionally, logging of sensitive setup tokens has been improved to reduce the risk of log injection.
CI/CD Workflow Security Updates:
p/secretsSemgrep configuration from both.github/workflows/ci.ymland.github/workflows/security-deep.ymlto avoid false positives or unnecessary secret scanning in CI. [1] [2].github/workflows/security-deep.ymlto always exit with code 0 (reporting-only), ensuring that vulnerabilities do not fail the workflow, and simplified artifact upload conditions. [1] [2]Sensitive Data Handling Improvements:
apps/rook/src/main.rsto only log a prefix and length of the setup token, reducing the risk of log injection attacks.Test Code Improvements:
nosemgrepcomments to suppress secret detection warnings incrates/application/rook-usecases/src/manage_connections.rs. [1] [2]