Add built-in security and quality rules to Commit Coach

stephenway · stephenway · commit 9da2909ef1b1 · 2025-10-07T13:39:21.000-07:00
- Introduced new security rules to detect hardcoded secrets, SQL injection risks, and XSS vulnerabilities in commits.
- Added quality checks for debug code, large file additions, dependency updates, missing error handling in async functions, and TypeScript 'any' type usage.
- Enhanced documentation in README.md, EXAMPLES.md, and RULES.md to reflect the new rules and their configurations.
- Updated the InsightGenerator to generate insights based on the new security and quality checks.
diff --git a/README.md b/README.md
@@ -76,7 +76,24 @@ integrations:
 
 ## Built-in Rules
 
+### 🔒 Security Rules
+- **Hardcoded Secrets**: Detects API keys, tokens, and credentials
+- **SQL Injection**: Identifies potential SQL injection vulnerabilities
+- **XSS Prevention**: Warns about unsafe DOM manipulation
+
+### 🧪 Code Quality Rules
 - **Test Coverage**: Warns when source files lack tests
+- **Debug Code**: Detects console.log, debugger statements
+- **Error Handling**: Flags async functions without try/catch
+- **TypeScript Safety**: Warns about 'any' type usage
+
+### 📝 Workflow Rules
+- **Merge Conflicts**: Detects unresolved conflict markers
+- **Large Files**: Warns about large file additions
+- **Dependencies**: Alerts on package updates
+- **Configuration**: Flags config file changes
+
+### 🔄 API & Documentation Rules
 - **Public API Changes**: Detects API removals and new APIs
 - **Documentation**: Suggests updating docs for new features
 - **Commit Quality**: Flags large commits and short messages
diff --git a/commit-coach.yml.example b/commit-coach.yml.example
@@ -67,6 +67,75 @@ rules:
     conditions: ["hasTodos"]
     message: "TODO/FIXME comments found - track these items"
 
+  # Security rules
+  - id: hardcoded-secrets
+    enabled: true
+    severity: error
+    conditions: ["diff.match(/['\"](sk-|pk_|ghp_|gho_|ghu_|ghs_|ghr_|AKIA|ya29\.)/)"]
+    message: "Potential hardcoded secret detected - use environment variables instead"
+
+  - id: sql-injection-risk
+    enabled: true
+    severity: error
+    conditions: ["diff.match(/query.*\\$\\{.*\\}|query.*\\+.*\\+/)"]
+    message: "Potential SQL injection risk detected - use parameterized queries"
+
+  - id: xss-risk
+    enabled: true
+    severity: error
+    conditions: ["diff.includes('innerHTML') && !diff.includes('textContent')"]
+    message: "Potential XSS risk - use textContent instead of innerHTML"
+
+  # Code quality rules
+  - id: debug-code
+    enabled: true
+    severity: warning
+    conditions: ["diff.includes('console.log') || diff.includes('debugger') || diff.includes('alert(')"]
+    message: "Debug code detected - remove before merging"
+
+  - id: large-file-addition
+    enabled: true
+    severity: warning
+    conditions: ["files.some(f => f.status === 'added' && f.additions > 1000)"]
+    message: "Large file(s) added - consider if these should be in version control"
+
+  - id: dependency-update
+    enabled: true
+    severity: info
+    conditions: ["files.some(f => f.path.includes('package.json') || f.path.includes('yarn.lock') || f.path.includes('pnpm-lock.yaml'))"]
+    message: "Dependencies updated - run tests and check for breaking changes"
+
+  - id: missing-error-handling
+    enabled: true
+    severity: warning
+    conditions: ["diff.includes('async ') && !diff.includes('try') && !diff.includes('catch')"]
+    message: "Async function added without error handling - consider try/catch blocks"
+
+  - id: typescript-any-type
+    enabled: true
+    severity: warning
+    conditions: ["files.some(f => f.path.endsWith('.ts')) && diff.includes(': any')"]
+    message: "Avoid using 'any' type - use specific types for better type safety"
+
+  # Workflow rules
+  - id: merge-conflict-markers
+    enabled: true
+    severity: error
+    conditions: ["diff.includes('<<<<<<<') || diff.includes('>>>>>>>') || diff.includes('=======')"]
+    message: "Merge conflict markers detected - resolve conflicts before committing"
+
+  - id: binary-file-addition
+    enabled: true
+    severity: warning
+    conditions: ["files.some(f => f.status === 'added' && /\.(jpg|jpeg|png|gif|pdf|zip|exe|dll|bin|so|dylib)$/i.test(f.path))"]
+    message: "Binary file(s) added - ensure they're necessary and properly sized"
+
+  - id: config-file-changes
+    enabled: true
+    severity: info
+    conditions: ["files.some(f => f.path.includes('config') || f.path.includes('.env') || f.path.includes('settings'))"]
+    message: "Configuration files changed - verify all environments are updated"
+
 # Output configuration
 output:
   format: console  # console, comment, status-check, report
diff --git a/docs/EXAMPLES.md b/docs/EXAMPLES.md
@@ -452,11 +452,17 @@ integrations:
 ```yaml
 # .commit-coach.yml
 rules:
-  # Security patterns
+  # Built-in security rules (automatically enabled)
+  - id: hardcoded-secrets
+    enabled: true
+    severity: error
+    conditions: ["diff.match(/['\"](sk-|pk_|ghp_|gho_|ghu_|ghs_|ghr_|AKIA|ya29\.)/)"]
+    message: "Potential hardcoded secret detected - use environment variables"
+
   - id: sql-injection-risk
     enabled: true
     severity: error
-    conditions: ["diff.match(/query.*\\$\\{.*\\}/)"]
+    conditions: ["diff.match(/query.*\\$\\{.*\\}|query.*\\+.*\\+/)"]
     message: "Potential SQL injection risk - use parameterized queries"
 
   - id: xss-risk
@@ -465,18 +471,19 @@ rules:
     conditions: ["diff.includes('innerHTML') && !diff.includes('textContent')"]
     message: "Potential XSS risk - use textContent instead of innerHTML"
 
-  - id: hardcoded-secrets
-    enabled: true
-    severity: error
-    conditions: ["diff.match(/['\"](sk-|pk_|ghp_|gho_|ghu_|ghs_|ghr_)/)"]
-    message: "Potential hardcoded secret detected - use environment variables"
-
+  # Additional security rules
   - id: crypto-usage
     enabled: true
     severity: warning
     conditions: ["diff.includes('crypto') || diff.includes('hash')"]
     message: "Cryptographic code changed - review security implications"
 
+  - id: debug-code
+    enabled: true
+    severity: warning
+    conditions: ["diff.includes('console.log') || diff.includes('debugger')"]
+    message: "Debug code detected - remove before merging"
+
 output:
   format: status-check
   maxInsights: 5
diff --git a/docs/RULES.md b/docs/RULES.md
@@ -184,16 +184,90 @@ Commit Coach comes with several built-in rules that you can enable/disable:
   message: "Breaking changes detected - update version and changelog"
 ```
 
+### Security Rules
+
+```yaml
+- id: hardcoded-secrets
+  enabled: true
+  severity: error
+  conditions: ["diff.match(/['\"](sk-|pk_|ghp_|gho_|ghu_|ghs_|ghr_|AKIA|ya29\.)/)"]
+  message: "Potential hardcoded secret detected - use environment variables instead"
+
+- id: sql-injection-risk
+  enabled: true
+  severity: error
+  conditions: ["diff.match(/query.*\\$\\{.*\\}|query.*\\+.*\\+/)"]
+  message: "Potential SQL injection risk detected - use parameterized queries"
+
+- id: xss-risk
+  enabled: true
+  severity: error
+  conditions: ["diff.includes('innerHTML') && !diff.includes('textContent')"]
+  message: "Potential XSS risk - use textContent instead of innerHTML"
+```
+
 ### Code Quality Rules
 
 ```yaml
+- id: debug-code
+  enabled: true
+  severity: warning
+  conditions: ["diff.includes('console.log') || diff.includes('debugger') || diff.includes('alert(')"]
+  message: "Debug code detected - remove before merging"
+
+- id: large-file-addition
+  enabled: true
+  severity: warning
+  conditions: ["files.some(f => f.status === 'added' && f.additions > 1000)"]
+  message: "Large file(s) added - consider if these should be in version control"
+
+- id: dependency-update
+  enabled: true
+  severity: info
+  conditions: ["files.some(f => f.path.includes('package.json') || f.path.includes('yarn.lock') || f.path.includes('pnpm-lock.yaml'))"]
+  message: "Dependencies updated - run tests and check for breaking changes"
+
+- id: missing-error-handling
+  enabled: true
+  severity: warning
+  conditions: ["diff.includes('async ') && !diff.includes('try') && !diff.includes('catch')"]
+  message: "Async function added without error handling - consider try/catch blocks"
+
+- id: typescript-any-type
+  enabled: true
+  severity: warning
+  conditions: ["files.some(f => f.path.endsWith('.ts')) && diff.includes(': any')"]
+  message: "Avoid using 'any' type - use specific types for better type safety"
+
 - id: todo-comments
   enabled: true
   severity: info
   conditions: ["hasTodos"]
   message: "TODO/FIXME comments found - track these items"
 ```
 
+### Workflow Rules
+
+```yaml
+- id: merge-conflict-markers
+  enabled: true
+  severity: error
+  conditions: ["diff.includes('<<<<<<<') || diff.includes('>>>>>>>') || diff.includes('=======')"]
+  message: "Merge conflict markers detected - resolve conflicts before committing"
+
+- id: binary-file-addition
+  enabled: true
+  severity: warning
+  conditions: ["files.some(f => f.status === 'added' && /\.(jpg|jpeg|png|gif|pdf|zip|exe|dll|bin|so|dylib)$/i.test(f.path))"]
+  message: "Binary file(s) added - ensure they're necessary and properly sized"
+
+- id: config-file-changes
+  enabled: true
+  severity: info
+  conditions: ["files.some(f => f.path.includes('config') || f.path.includes('.env') || f.path.includes('settings'))"]
+  message: "Configuration files changed - verify all environments are updated"
+```
+
 ## Custom Rule Examples
 
 ### Security Rules
diff --git a/src/insights/generator.ts b/src/insights/generator.ts
