* chore: remove detect-secrets pre-commit hook (redundant with gitleaks) detect-secrets tracked false positives in .secrets.baseline using line-number-based offsets, causing constant merge conflicts and 173 KB of churn on every PR that touched flagged files. gitleaks provides equivalent pattern-based detection with path/regex allowlists that are stable across line changes. Additional CI coverage from Semgrep (p/secrets) and Bearer (secrets) remains in place. Changes: - Remove Yelp/detect-secrets hook from .pre-commit-config.yaml - Delete .secrets.baseline (5192 lines) - Remove .secrets.baseline references from .gitleaks.toml, .gitleaksignore, CODEOWNERS, danger-zone-alert.yml, SECURITY_REVIEW_PROCESS.md, .file-whitelist.txt - Remove dead SECRETS_MODIFIED logic from danger-zone-alert.yml - Add "do not re-add" notes in .gitleaks.toml and SECURITY.md - Add ADR-0001 documenting the decision * fix(ci): whitelist docs/decisions/ in suspicious filename check Replace the now-deleted .secrets.baseline entry with a pattern for docs/decisions/ ADR files, which may contain security-related keywords in their filenames (e.g., "remove-detect-secrets").
3.6 KiB
Security Review Process
Overview
This repository uses an automated alert system to highlight when PRs modify security-critical files. Instead of blocking development, we provide clear warnings and checklists to ensure dangerous changes get proper scrutiny.
How It Works
🚨 Automatic Detection
When a PR modifies any of these files:
- Database encryption (
encrypted_db.py,sqlcipher_*.py) - Authentication (
/auth/directory,password*.py) - Security utilities (
/security/directory) - Encryption/decryption code (
*encrypt*.py,*decrypt*.py,*crypto*.py)
The CI will:
- Post a prominent warning comment with security-specific review checklist
- Add labels to the PR (
security-review-needed,touches-encryption, etc.) - Create a status check showing "Security Review Required" (informational, not blocking)
📋 Review Checklists
The bot creates specific checklists based on what was modified:
For Encryption Changes:
- SQLCipher pragma order (key must come first)
- No hardcoded keys
- Backward compatibility
- Migration paths
For Authentication Changes:
- No auth bypasses
- Secure session handling
- Proper password hashing
- No privilege escalation
For All Security Changes:
- No exposed secrets
- No debug bypasses
- Safe error messages
- Secure logging
For Developers
When You Modify Security Files:
- Expect the warning - It's not a failure, just a heads-up
- Self-review first - Go through the checklist yourself
- Document your changes - Explain WHY the security code needed to change
- Test thoroughly - Especially with existing encrypted databases
- Be patient - Security reviews take time
Red Flags to Avoid:
❌ Changing pragma order in SQLCipher code ❌ Adding "temporary" auth bypasses ❌ Logging passwords or encryption keys ❌ Hardcoding credentials ❌ Disabling security checks "for testing"
For Reviewers
When You See the Warning:
- Take it seriously - These files can break security if done wrong
- Go through the checklist - Don't just check boxes, verify each item
- Test locally - Especially encryption changes with real databases
- Ask questions - If something seems off, dig deeper
- Get a second opinion - For CRITICAL changes, ask another reviewer
What Caused Our Previous Issue:
The SQLCipher pragma order bug that corrupted databases happened because:
- Pragmas were applied BEFORE setting the encryption key
- This wasn't caught in review despite being in "hotfix" branch
- The change seemed logical but was actually breaking
Lesson: Even "obvious" changes to encryption code need careful review!
The Philosophy
- Warnings, not blocks - We trust developers but want awareness
- Specific guidance - Checklists for what to look for
- Risk-based - More critical files get scarier warnings
- Educational - Help reviewers know what to check
Labels Added Automatically
security-review-needed- Always added for security filestouches-encryption- Database encryption modifiedtouches-authentication- Auth system modifiedcritical-changes- Multiple security systems affected
It's Not Perfect
This system won't catch everything:
- Logic bugs in security code
- Subtle vulnerabilities
- Dependencies with security issues
It's meant to make reviewers pause and think when touching dangerous code.
Questions?
If you're unsure about a security change:
- Ask in the PR for additional reviewers
- Test with production-like data
- Consider doing a security-focused code review session
- When in doubt, be more cautious