Security audit: Fix 7 vulnerabilities and add security headers#21
Merged
Security audit: Fix 7 vulnerabilities and add security headers#21
Conversation
[Agent: Main Assistant] CRITICAL: - Path traversal in file upload: sanitize filenames, add resolve() check HIGH: - Command injection: replace shell=True with allowlist + list-form subprocess - SSRF in ingest scan: add hostname allowlist for base_url parameter MEDIUM: - Error disclosure: replace str(e) in HTTP responses with generic messages - Airtable formula injection: escape single quotes in media_id values - Gemini API key: move from URL query parameter to x-goog-api-key header - CORS/headers: tighten CORS methods/headers, add security response headers Also documents 4 advisory findings (no auth, no rate limiting, WS auth, sys.path manipulation) in SECURITY_AUDIT.md for future consideration. https://claude.ai/code/session_018s8XVCwoprXPYHe1PjNRq9
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
2 participants
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
This PR addresses a comprehensive security audit of the Editorial Assistant codebase. 7 vulnerabilities have been fixed and 4 advisory findings are documented in the new
SECURITY_AUDIT.mdfor future architectural decisions.Fixed Vulnerabilities
Critical
api/routers/upload.py): Addedsanitize_upload_filename()function that strips directory components, removes dangerous characters, and validates resolved paths to prevent writing files outside thetranscripts/directory.High
api/routers/system.py): Replacedsubprocess.Popen(..., shell=True)with an allowlist-based approach using list-form subprocess calls. Onlyrun_worker.pyandwatch_transcripts.pycan be started.api/routers/ingest.py): AddedALLOWED_INGEST_HOSTSallowlist to restrict ingest server URLs tommingest.pbswi.wisc.edu, preventing probing of internal network addresses.Medium
detail=str(e)patterns with generic error messages. Full exception details remain in server logs for debugging.api/services/airtable.py,mcp_server/server.py): Added_escape_airtable_formula_value()to escape single quotes and backslashes in media IDs before interpolating into Airtable filter formulas.api/services/llm.py): Moved API key from URL query parameter tox-goog-api-keyHTTP header to prevent leakage in logs and error messages.api/main.py): Tightened CORS to explicit method/header lists and added security headers middleware (X-Content-Type-Options,X-Frame-Options,X-XSS-Protection,Referrer-Policy,Permissions-Policy).Advisory Findings (Documented, Not Fixed)
Four high/medium-priority findings require architectural decisions:
slowapimiddleware, especially for/api/chat/messageand/api/upload/transcriptsFiles Modified
api/routers/upload.py- Path traversal protection with filename sanitizationapi/routers/system.py- Command injection fix with allowlistapi/routers/chat_prototype.py- Generic error messagesapi/routers/ingest.py- SSRF protection with host allowlistapi/services/airtable.py- Formula injection escapingapi/services/llm.py- API key moved to headerapi/main.py- Security headers and CORS tighteningmcp_server/server.py- Formula injection escapingtests/test_ingest_api.py- Updated test URLs to use allowlisted hostSECURITY_AUDIT.md- New comprehensive audit report (152 lines)Testing
Updated ingest API tests to use the allowlisted host (
mmingest.pbswi.wisc.edu) instead of example.com. All fixes maintain backward compatibility with existing functionality.https://claude.ai/code/session_018s8XVCwoprXPYHe1PjNRq9