fix: security hardening, connection pooling, and codebase cleanup#1
Open
BasitS-hash wants to merge 2 commits into
Open
fix: security hardening, connection pooling, and codebase cleanup#1BasitS-hash wants to merge 2 commits into
BasitS-hash wants to merge 2 commits into
Conversation
Security: - Remove JWT_SECRET weak fallback defaults; raise EnvironmentError at startup if unset - Untrack .env.production and add proper Python .gitignore (was Node.js template) - Use datetime.now(timezone.utc) in audit_log and health endpoint (fix naive timestamps) Code quality: - Delete dead config.py (Flask remnant, nothing imported it) - Add psycopg2 ThreadedConnectionPool (lazy init) — was opening a new connection per query - Fix logger.py to respect LOG_LEVEL env var and guard against duplicate handler registration - Validate IV (12 bytes) and tag (16 bytes) lengths in decode_entry_fields; catch invalid base64 - Return entry id from POST /entries/ so clients can reference the new resource Tests: - Add tests/conftest.py to set JWT_SECRET before app is imported - Assert 'id' in create_entry response Docker: - Run as non-root appuser; chown /app/logs before switching user
There was a problem hiding this comment.
Pull request overview
This PR hardens security configuration, improves operational behavior under load by introducing database connection pooling, and removes legacy/unsafe artifacts (tracked env file, dead config) across the FastAPI password manager API.
Changes:
- Enforce required secrets/env vars (notably
JWT_SECRET), remove weak fallbacks, and stop tracking production env files. - Add DB connection pooling and return newly-created entry IDs from
POST /entries/. - Normalize timestamps to UTC-aware datetimes, improve logging initialization, and run Docker container as a non-root user.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_api.py | Asserts POST /entries/ includes an id in the response. |
| tests/conftest.py | Ensures JWT_SECRET is set during tests to avoid startup failures. |
| src/routes/entries.py | Adds stricter decoding/validation for AES-GCM fields and returns inserted entry id. |
| src/routes/auth.py | Removes insecure JWT secret fallback and requires JWT_SECRET. |
| src/middleware/auth.py | Removes insecure JWT secret fallback and requires JWT_SECRET for token validation. |
| src/middleware/audit.py | Writes audit timestamps as UTC-aware datetimes. |
| src/logger.py | Respects LOG_LEVEL and avoids duplicate handler registration. |
| src/db.py | Introduces a ThreadedConnectionPool and switches query/execute helpers to pooled connections. |
| Dockerfile | Creates a non-root user and runs the app as that user. |
| config.py | Removes unused legacy Flask-era configuration module. |
| app.py | Updates /health timestamp to use UTC-aware datetime. |
| .gitignore | Replaces Node-centric ignore rules with Python-appropriate ignores and ignores .env.production. |
| .env.production | Removes tracked production env file from the repository. |
| .env.example | Updates example env file guidance and variables. |
Comments suppressed due to low confidence (1)
src/db.py:59
execute()has the same issue asquery():cur.close()is called unconditionally infinallyeven thoughcuris assigned inside thetry. If cursor creation fails, this will raiseUnboundLocalErrorand can prevent the pooled connection from being returned.
def execute(sql, params=None):
conn = get_connection()
try:
cur = conn.cursor()
cur.execute(sql, params or ())
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
32
to
36
| def query(sql, params=None): | ||
| """Execute a query and return results.""" | ||
| conn = get_connection() | ||
| try: | ||
| cur = conn.cursor(cursor_factory=psycopg2.extras.RealDictCursor) | ||
| cur.execute(sql, params or ()) |
Comment on lines
+41
to
+46
| try: | ||
| ciphertext = base64.b64decode(body.ciphertext) | ||
| iv = base64.b64decode(body.iv) | ||
| tag = base64.b64decode(body.tag) | ||
| except Exception: | ||
| raise HTTPException(status_code=400, detail="ciphertext, iv, and tag must be valid base64") |
Comment on lines
+46
to
+50
| raise HTTPException(status_code=400, detail="ciphertext, iv, and tag must be valid base64") | ||
| if len(iv) != 12: | ||
| raise HTTPException(status_code=400, detail="iv must be exactly 12 bytes for AES-GCM") | ||
| if len(tag) != 16: | ||
| raise HTTPException(status_code=400, detail="tag must be exactly 16 bytes for AES-GCM") |
HIGH findings: - Pin Docker GitHub Actions to full commit SHAs (supply chain hardening) - Add per-route rate limits: 5/min+20/hr on login, 3/min+10/hr on register - docker-compose JWT_SECRET now hard-fails with :? if unset (no silent fallback) - Constant-time login: always run argon2 verify even for unknown usernames to prevent timing-based username enumeration MEDIUM findings: - /auth/token returns 403 (not 404) for deleted user — closes user enumeration path - encryption_salt increased from 16 to 32 bytes (128 → 256 bits) - Refresh tokens switched from uuid.uuid4() to secrets.token_urlsafe(32) (256 bits) - Add .gstack/ to .gitignore so security reports never get committed Tests: - Disable rate limiting in TESTING=true mode (conftest.py sets it) so validation tests don't hit the per-IP limit
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
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.
Summary
JWT_SECRETweak fallback default — app now raisesEnvironmentErrorat startup if the variable is missing instead of silently using"your-secret-key-change-me".env.productionfrom git and replace the Node.js.gitignoretemplate with a proper Python oneaudit_logand/health— both now usedatetime.now(timezone.utc)(also fixes Python 3.12 deprecation ofutcnow())POST /entries/now returns{"ok": true, "id": "<uuid>"}— previously the client had no way to reference a newly created entry without re-listingdecode_entry_fieldsnow validates that IV is exactly 12 bytes and tag is exactly 16 bytes (AES-GCM requirements), and returns a clear 400 on invalid base64 instead of a generic 500psycopg2.connect()with aThreadedConnectionPool(2, 10)— the old approach would exhaust Postgres connections under any loadconfig.py— it was a Flask remnant, nothing in the codebase imported it, and it had hardcodedFLASK_ENVreferenceslogger.pyto respect theLOG_LEVELenv var and guard against duplicate handler registration on repeated importsappuserinstead of rootTest plan
pytest tests/withoutDATABASE_URL)docker build .succeeds and app starts as non-rootJWT_SECRETset🤖 Generated with Claude Code