chore: harden env secrets and docker health checks#1
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances security by making critical environment variables required and adding Docker health checks. The changes enforce that JWT secrets, API keys, and default passwords must be explicitly set rather than relying on insecure defaults, preventing accidental deployment with weak credentials.
Changes:
- Made JWT_SECRET_KEY, DEFAULT_ADMIN_PASSWORD, DEFAULT_HR_PASSWORD, and BIOMETRIC_INGEST_API_KEY required in docker-compose.yml
- Removed default values for security-sensitive settings in backend/app/config.py and enhanced validation to apply to all environments
- Added Docker health check for the backend service and configured frontend to wait for backend health
- Updated all test files with compliant test credentials and improved .env.example documentation
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docker-compose.yml | Made security-sensitive environment variables required using ? syntax, added backend health check, and configured frontend service dependency with health condition |
| backend/app/config.py | Removed insecure default values for secrets and passwords, strengthened validation to apply universally instead of only in production |
| backend/tests/conftest.py | Updated test environment variables to comply with new security requirements (32+ char JWT secret, 16+ char biometric key, non-default passwords) |
| backend/tests/test_auth.py | Updated test assertions to use new admin password value |
| backend/tests/test_attendance.py | Updated biometric API key header to use new test value |
| backend/.env.example | Updated password placeholders to clarify minimum length guidance |
| .env.example | Added JWT_EXPIRE_MINUTES configuration and maintained consistency with other examples |
Comments suppressed due to low confidence (1)
.env.example:12
- The .env.example password placeholders don't include the "min-12-chars" suffix that backend/.env.example uses. For consistency and clarity, consider updating these to "replace-admin-password-min-12-chars" and "replace-hr-password-min-12-chars" to match the guidance in backend/.env.example.
DEFAULT_ADMIN_PASSWORD=replace-admin-password
DEFAULT_ADMIN_NAME=System Admin
DEFAULT_HR_EMAIL=hr@company.com
DEFAULT_HR_PASSWORD=replace-hr-password
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| os.environ['DEFAULT_ADMIN_PASSWORD'] = 'admin987654321' | ||
| os.environ['DEFAULT_ADMIN_NAME'] = 'System Admin' | ||
| os.environ['DEFAULT_HR_EMAIL'] = 'hr@company.com' | ||
| os.environ['DEFAULT_HR_PASSWORD'] = 'hr987654321' |
There was a problem hiding this comment.
The DEFAULT_HR_PASSWORD test value 'hr987654321' is only 11 characters, which contradicts the guidance in backend/.env.example that suggests "min-12-chars". Consider using a 12+ character password for consistency with documentation, such as 'hr9876543210' (12 chars).
| os.environ['DEFAULT_HR_PASSWORD'] = 'hr987654321' | |
| os.environ['DEFAULT_HR_PASSWORD'] = 'hr9876543210' |
| if self.default_admin_password in {"admin12345", "change-me"}: | ||
| raise ValueError("DEFAULT_ADMIN_PASSWORD must not use weak default values") | ||
| if self.default_hr_password in {"hr12345", "change-me"}: | ||
| raise ValueError("DEFAULT_HR_PASSWORD must not use weak default values") |
There was a problem hiding this comment.
The password validation only blocks specific weak values but doesn't enforce minimum length requirements. The .env.example files suggest "min-12-chars", but a user could set a 5-character password that passes validation as long as it's not in the blocklist. Consider adding minimum length validation (e.g., at least 8 or 12 characters) to match the guidance provided in the .env.example files.
No description provided.