Scan in commits check #213
Conversation
Actually, test were added and te check is pretty much a clone of `noSensitiveInfoRepositories` with custom validation checks added to support in-commits validations and checks
WalkthroughThis update introduces a new compliance check for scanning GitHub commits for sensitive information. It adds the check's implementation, validation logic, integration tests, and schema/migration changes to support the feature. The database configuration is updated, and foreign key constraints are enhanced to include cascading behavior for improved data integrity. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ComplianceCheck (scanCommitsForSensitiveInfo)
participant DataStore (DB)
participant Validator
User->>ComplianceCheck: Trigger check (with optional projects)
ComplianceCheck->>DataStore: Fetch check metadata and projects
ComplianceCheck->>DataStore: Fetch GitHub orgs and repos for projects
ComplianceCheck->>Validator: Validate org/repo secret scanning settings
Validator-->>ComplianceCheck: Return results, alerts, tasks
ComplianceCheck->>DataStore: Delete existing alerts/tasks for this check
ComplianceCheck->>DataStore: Upsert results, insert alerts/tasks
ComplianceCheck-->>User: Completion (results stored)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/database/migrations/1742403845916_update_check_scanCommitsForSensitiveInfo.js.js (1)
1-19: Well-structured migration with proper rollback support.The migration correctly updates the compliance check metadata and includes a proper down migration. The reference URL aligns with the PR objectives (issue #69).
Consider adding error handling for cases where the compliance check doesn't exist:
exports.up = async (knex) => { + const updated = await knex('compliance_checks') - await knex('compliance_checks') .where({ code_name: 'scanCommitsForSensitiveInfo' }) .update({ implementation_status: 'completed', implementation_type: 'computed', implementation_details_reference: 'https://github.com/OpenPathfinder/visionBoard/issues/69' }) + if (updated === 0) { + throw new Error('scanCommitsForSensitiveInfo compliance check not found') + } }src/checks/complianceChecks/scanCommitsForSensitiveInfo.js (2)
13-16: Consider adding validation for check existence.While the code handles the projects parameter well, consider adding validation to ensure the compliance check exists:
const check = await getCheckByCodeName('scanCommitsForSensitiveInfo') +if (!check) { + throw new Error('scanCommitsForSensitiveInfo compliance check not found') +}
26-31: Efficient parallel processing of database operations.Using
Promise.allfor bulk operations is a good performance optimization. Consider adding error handling to provide better debugging information if individual operations fail:-await Promise.all(analysis.results.map(result => upsertComplianceCheckResult(result))) +try { + await Promise.all(analysis.results.map(result => upsertComplianceCheckResult(result))) +} catch (error) { + debug('Error upserting results:', error) + throw error +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
__tests__/checks/scanCommitsForSensitiveInfo.test.js(1 hunks)__tests__/checks/validators/scanCommitsForSensitiveInfo.test.js(1 hunks)src/checks/complianceChecks/scanCommitsForSensitiveInfo.js(1 hunks)src/checks/validators/index.js(1 hunks)src/checks/validators/scanCommitsForSensitiveInfo.js(1 hunks)src/config/index.js(1 hunks)src/database/migrations/1742403845916_update_check_scanCommitsForSensitiveInfo.js.js(1 hunks)src/database/schema/schema.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
src/config/index.js (2)
Learnt from: UlisesGascon
PR: OpenPathfinder/visionBoard#233
File: playwright.config.js:59-59
Timestamp: 2025-05-03T07:11:12.719Z
Learning: In the visionBoard repository, the team has deliberately chosen to run Playwright tests without explicitly setting NODE_ENV=test in the webServer command, as part of a change that implements automatic database migrations on server start.
Learnt from: UlisesGascon
PR: OpenPathfinder/visionBoard#229
File: .github/workflows/e2e-tests.yml:16-29
Timestamp: 2025-05-03T05:39:42.677Z
Learning: For the VisionBoard project, database testing credentials (username: visionBoard, password: password) can be hardcoded in GitHub workflow files as they are the same default values used throughout the application for testing purposes.
src/database/migrations/1742403845916_update_check_scanCommitsForSensitiveInfo.js.js (1)
Learnt from: UlisesGascon
PR: OpenPathfinder/visionBoard#233
File: playwright.config.js:59-59
Timestamp: 2025-05-03T07:11:12.719Z
Learning: In the visionBoard repository, the team has deliberately chosen to run Playwright tests without explicitly setting NODE_ENV=test in the webServer command, as part of a change that implements automatic database migrations on server start.
__tests__/checks/validators/scanCommitsForSensitiveInfo.test.js (1)
Learnt from: UlisesGascon
PR: OpenPathfinder/visionBoard#229
File: e2e/website.spec.js:9-10
Timestamp: 2025-05-03T05:38:39.703Z
Learning: In the e2e tests for the VisionBoard repository, the testProjectId is currently hardcoded to match the test data created in the global-setup.js file. Even though there is a TODO comment about loading it from fixtures, the fixtures structure doesn't exist yet, so suggestions to implement this TODO should verify the fixtures structure first.
__tests__/checks/scanCommitsForSensitiveInfo.test.js (2)
Learnt from: UlisesGascon
PR: OpenPathfinder/visionBoard#229
File: e2e/website.spec.js:9-10
Timestamp: 2025-05-03T05:38:39.703Z
Learning: In the e2e tests for the VisionBoard repository, the testProjectId is currently hardcoded to match the test data created in the global-setup.js file. Even though there is a TODO comment about loading it from fixtures, the fixtures structure doesn't exist yet, so suggestions to implement this TODO should verify the fixtures structure first.
Learnt from: UlisesGascon
PR: OpenPathfinder/visionBoard#233
File: playwright.config.js:59-59
Timestamp: 2025-05-03T07:11:12.719Z
Learning: In the visionBoard repository, the team has deliberately chosen to run Playwright tests without explicitly setting NODE_ENV=test in the webServer command, as part of a change that implements automatic database migrations on server start.
🧬 Code Graph Analysis (4)
src/checks/validators/index.js (1)
src/checks/validators/scanCommitsForSensitiveInfo.js (1)
require(2-6)
__tests__/checks/validators/scanCommitsForSensitiveInfo.test.js (2)
src/checks/validators/scanCommitsForSensitiveInfo.js (1)
require(2-6)src/checks/validators/index.js (1)
scanCommitsForSensitiveInfo(7-7)
__tests__/checks/scanCommitsForSensitiveInfo.test.js (4)
src/checks/complianceChecks/scanCommitsForSensitiveInfo.js (3)
require(2-2)check(13-13)initializeStore(6-10)src/store/index.js (1)
getCheckByCodeName(52-55)__utils__/index.js (2)
resetDatabase(2-11)generateGithubRepoData(13-29)__fixtures__/index.js (1)
sampleGithubOrg(2-66)
src/checks/validators/scanCommitsForSensitiveInfo.js (1)
src/utils/index.js (3)
groupArrayItemsByCriteria(75-83)generatePercentage(85-89)getSeverityFromPriorityGroup(56-73)
🪛 GitHub Actions: CI
__tests__/checks/validators/scanCommitsForSensitiveInfo.test.js
[error] 1-1: ESLint: 'se' is assigned a value but never used. (no-unused-vars)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze
- GitHub Check: Playwright Tests
🔇 Additional comments (14)
src/config/index.js (1)
9-10: LGTM! Good addition for database configuration flexibility.The port configuration follows the established pattern and uses the standard PostgreSQL port as a sensible default.
src/database/schema/schema.sql (2)
1305-1305: Verify that CASCADE behavior aligns with business requirements.The addition of
ON UPDATE CASCADE ON DELETE CASCADEwill automatically propagate changes and deletions from thecompliance_checkstable toresources_for_compliance_checks. This is generally beneficial for maintaining referential integrity, but ensure this automatic cleanup behavior matches your business logic.
1313-1313: Consistent CASCADE behavior applied.Good consistency in applying the same CASCADE constraints to both foreign key relationships in the table.
src/checks/validators/index.js (1)
7-7: LGTM! Proper integration of the new validator.The new
scanCommitsForSensitiveInfovalidator is correctly imported and added to the exports, following the established pattern.Also applies to: 15-15
src/checks/complianceChecks/scanCommitsForSensitiveInfo.js (2)
5-10: Good initialization and dependency injection pattern.The function signature and store initialization follow established patterns in the codebase.
22-24: Excellent cleanup strategy to prevent orphaned records.The deletion of previous alerts and tasks before creating new ones is a best practice that prevents data inconsistency.
__tests__/checks/scanCommitsForSensitiveInfo.test.js (1)
1-259: Excellent comprehensive integration test coverage.The test suite provides thorough coverage of the
scanCommitsForSensitiveInfocompliance check with well-structured scenarios:
- Proper database setup/teardown using beforeAll/beforeEach/afterAll hooks
- Tests for passed checks with and without existing alerts/tasks cleanup
- Tests for various failure scenarios (org-level, repo-level, mixed configurations)
- Proper verification of database state changes (results, alerts, tasks)
- Clean separation of test data setup and assertions
The integration tests effectively validate the end-to-end behavior of the compliance check including database interactions.
__tests__/checks/validators/scanCommitsForSensitiveInfo.test.js (1)
4-403: Outstanding validator test coverage.The test suite provides comprehensive coverage of the
scanCommitsForSensitiveInfovalidator with 9 well-designed test cases covering:
- All possible combinations of organization and repository secret scanning states
- Proper handling of enabled, disabled, and unknown (null) values
- Mixed scenarios with organizations and repositories in different states
- Correct generation of alerts, results, and tasks for each scenario
- Appropriate status determination (passed, failed, unknown)
The test fixtures are well-structured and the assertions verify the complete output structure including rationale messages, percentages, and task descriptions.
src/checks/validators/scanCommitsForSensitiveInfo.js (6)
10-47: Well-designed utility functions for failure detection.The
getOrgFailuresandgetRepoFailuresfunctions provide clean separation of concerns for identifying organizations and repositories with disabled or unknown secret scanning configurations. The filtering logic correctly handles the different secret scanning attributes and null values.
49-69: Excellent message building functions.The
buildOrgMessageandbuildRepoMessagefunctions generate clear, descriptive messages for different failure scenarios. The percentage calculation for repositories provides valuable context about the scope of issues.
94-113: Smart early optimization for fully compliant projects.The early check for
allReposEnabled && allOrgDefaultsEnabledprovides an efficient path for projects that are fully compliant, avoiding unnecessary processing of failure scenarios. This optimization improves performance for the common success case.
127-150: Robust status determination logic.The status determination logic properly prioritizes failures over unknowns and handles complex rationale building for mixed scenarios. The conditional logic for combining organization and repository rationales is well-thought-out.
158-199: Comprehensive alert and task generation.The logic for generating alerts and tasks correctly handles different failure combinations (org-only, repo-only, mixed) with appropriate titles and descriptions. The task title generation provides actionable guidance with specific counts and percentages.
72-210: Excellent overall validator implementation.The main validator function demonstrates solid software engineering practices:
- Clear structure with logical flow from data processing to result generation
- Proper error handling and edge case coverage
- Effective use of debug logging for troubleshooting
- Clean separation between data analysis and output generation
- Consistent data structure for results, alerts, and tasks
The implementation correctly addresses the PR objective of extending sensitive information detection to include commit scanning capabilities.
__tests__/checks/validators/scanCommitsForSensitiveInfo.test.js
Outdated
Show resolved
Hide resolved
1 participant
Addresses #69 .
As commented there, overlaps
noSensitiveInfoInRepositoriesbut expands to do checks in commits as well.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores