Fix WebSocket authentication to use configurable header #101
Conversation
WebSocket connections were falling back to test user instead of reading the authenticated user from the configured authentication header set by the reverse proxy. This caused file access checks to fail with 'Access denied: [email protected] attempted to access users/{actual_user}/...' when users tried to load their own files. Changes: - WebSocket endpoint now uses config.app_settings.auth_user_header - Consistent with HTTP endpoints via AuthMiddleware - Falls back to query parameter for dev/test compatibility - Added logging to identify authentication source Tests: - test_websocket_auth_header.py: Header-based auth, query param fallback - test_issue_access_denied_fix.py: Integration test for the bug scenario - test_security_header_injection.py: Header injection security tests Documentation: - docs/example/nginx.config: Production-ready nginx configuration - docs/example/reverse-proxy-examples.md: Nginx/Apache setup guide - Updated security architecture with header injection prevention Fixes #45
|
@ktpedre Just so you are tracking these changes. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if not user_email: | ||
| user_email = websocket.query_params.get('user') | ||
| if user_email: | ||
| logger.info("WebSocket authenticated via query parameter: %s", sanitize_for_logging(user_email)) |
Check failure
Code scanning / CodeQL
Log Injection High
Copilot Autofix
AI 7 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
🔒 Security Scan ResultsSecurity Scan SummaryScan ResultsPython SAST (Bandit)Recommendations
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a critical authentication bug where WebSocket connections were falling back to the test user instead of reading the authenticated user from the configurable authentication header set by the reverse proxy. This caused file access permission errors when users tried to access their own files.
The fix ensures WebSocket authentication is consistent with HTTP endpoint authentication by reading from config.app_settings.auth_user_header (configurable via AUTH_USER_HEADER environment variable, default: X-User-Email). The implementation maintains backward compatibility with query parameter fallback for development/testing and includes comprehensive security documentation about header injection prevention.
- WebSocket authentication now uses configurable header (consistent with AuthMiddleware)
- Comprehensive test coverage added for header authentication, fallback mechanisms, and security scenarios
- Production-ready reverse proxy configuration examples (nginx and Apache) with critical security requirements documented
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/main.py | Updated WebSocket endpoint to extract authenticated user from configurable auth header; added proper fallback chain (header → query param → test user) with logging for each authentication source |
| backend/tests/test_websocket_auth_header.py | Unit tests validating header-based authentication, query parameter fallback, test user fallback, and header precedence |
| backend/tests/test_issue_access_denied_fix.py | Integration test demonstrating the exact issue scenario and verifying the fix works correctly |
| backend/tests/test_security_header_injection.py | Security tests documenting header injection vulnerabilities and production verification procedures |
| docs/example/nginx.config | Production-ready nginx configuration with critical header stripping to prevent injection attacks |
| docs/example/reverse-proxy-examples.md | Comprehensive reverse proxy setup guide with nginx/Apache examples, security testing procedures, and deployment checklist |
| docs/archive/security_architecture.md | Updated WebSocket authentication flow documentation and enhanced security requirements with header injection prevention details |
| .github/copilot-instructions.md | Updated developer guide to reflect X-User-Email header with header stripping requirement |
| # Last Updated: 2025-11-10 | ||
| # | ||
| # SECURITY REQUIREMENTS: | ||
| # - This configuration includes header stripping to prevent header injection attacks |
There was a problem hiding this comment.
Double space found in comment. Should be "includes header" instead of "includes header".
| # - This configuration includes header stripping to prevent header injection attacks | |
| # - This configuration includes header stripping to prevent header injection attacks |
| # Fallback to test user or require auth | ||
| config_manager = app_factory.get_config_manager() | ||
| user_email = config_manager.app_settings.test_user or '[email protected]' | ||
| logger.info(f"WebSocket using fallback test user: {sanitize_for_logging(user_email)}") |
There was a problem hiding this comment.
[nitpick] Inconsistent logging style within the same code block. Lines 234 and 240 use logger format placeholders (logger.info("message: %s", value)), while this line uses an f-string. For consistency, consider changing to:
logger.info("WebSocket using fallback test user: %s", sanitize_for_logging(user_email))| logger.info(f"WebSocket using fallback test user: {sanitize_for_logging(user_email)}") | |
| logger.info("WebSocket using fallback test user: %s", sanitize_for_logging(user_email)) |
Add mandatory proxy secret check in websocket_endpoint before accepting WebSocket connections, ensuring consistency with HTTP AuthMiddleware security. Raise WebSocketException for invalid secrets to reject unauthorized access. Introduce WebSocketException import and sanitize logging for auth headers and user emails to prevent potential information leakage. This enhances overall system security in production environments.
| logger.info( | ||
| "WebSocket authenticated via %s header: %s", | ||
| sanitize_for_logging(auth_header_name), | ||
| sanitize_for_logging(user_email) |
Check failure
Code scanning / CodeQL
Log Injection High
Copilot Autofix
AI 7 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
🔒 Security Scan ResultsSecurity Scan SummaryScan ResultsPython SAST (Bandit)Recommendations
|
WebSocket connections were falling back to test user instead of reading the authenticated user from the configured authentication header set by the reverse proxy. This caused file access checks to fail with
Access denied: [email protected] attempted to access users/{actual_user}/...when users tried to load their own files.Changes
backend/main.py
config.app_settings.auth_user_headerto read the authentication header (configurable viaAUTH_USER_HEADERenvironment variable, default:X-User-Email)Tests
backend/tests/test_websocket_auth_header.py
backend/tests/test_issue_access_denied_fix.py
backend/tests/test_security_header_injection.py
Documentation
docs/example/nginx.config
docs/example/reverse-proxy-examples.md
docs/archive/security_architecture.md
Compatibility
This fix works seamlessly with the configurable
AUTH_USER_HEADERfeature, supporting custom header names for different reverse proxy setups (e.g.,X-User-Email,X-Remote-User,X-Authenticated-User) without code changes.Fixes #45