-
Notifications
You must be signed in to change notification settings - Fork 4
Fix WebSocket authentication to use configurable header #101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -9,7 +9,7 @@ | |||||
| from pathlib import Path | ||||||
| from uuid import uuid4 | ||||||
|
|
||||||
| from fastapi import FastAPI, WebSocket, WebSocketDisconnect | ||||||
| from fastapi import FastAPI, WebSocket, WebSocketDisconnect, WebSocketException | ||||||
| from fastapi.staticfiles import StaticFiles | ||||||
| from fastapi.responses import FileResponse | ||||||
| from dotenv import load_dotenv | ||||||
|
|
@@ -23,6 +23,7 @@ | |||||
| from core.security_headers_middleware import SecurityHeadersMiddleware | ||||||
| from core.otel_config import setup_opentelemetry | ||||||
| from core.utils import sanitize_for_logging | ||||||
| from core.auth import get_user_from_header | ||||||
|
|
||||||
| # Import from infrastructure | ||||||
| from infrastructure.app_factory import app_factory | ||||||
|
|
@@ -209,21 +210,59 @@ | |||||
| - Direct public access to this app bypasses authentication | ||||||
| - Use network isolation to prevent direct access | ||||||
| - The /login endpoint lives in the separate auth service | ||||||
| - Reverse proxy MUST strip client-provided X-User-Email headers before adding its own | ||||||
| (otherwise attackers can inject headers: X-User-Email: admin@company.com) | ||||||
| DEVELOPMENT vs PRODUCTION: | ||||||
| - Production: Extracts user from configured auth header (set by reverse proxy) | ||||||
| - Development: Falls back to 'user' query parameter (INSECURE, local only) | ||||||
| See docs/security_architecture.md for complete architecture details. | ||||||
| """ | ||||||
| # Extract user email using the same authentication flow as HTTP requests | ||||||
| # Priority: 1) configured auth header (production), 2) query param (dev), 3) test user (dev fallback) | ||||||
| config_manager = app_factory.get_config_manager() | ||||||
|
|
||||||
| # WebSocket connections must present the shared proxy secret (same as AuthMiddleware) | ||||||
| if ( | ||||||
| config_manager.app_settings.feature_proxy_secret_enabled | ||||||
| and config_manager.app_settings.proxy_secret | ||||||
| and not config_manager.app_settings.debug_mode | ||||||
| ): | ||||||
| proxy_secret_header = config_manager.app_settings.proxy_secret_header | ||||||
| proxy_secret_value = websocket.headers.get(proxy_secret_header) | ||||||
| if proxy_secret_value != config_manager.app_settings.proxy_secret: | ||||||
| logger.warning( | ||||||
| "WS proxy secret mismatch on %s", | ||||||
| sanitize_for_logging(websocket.client) | ||||||
| ) | ||||||
| raise WebSocketException(code=1008, reason="Invalid proxy secret") | ||||||
|
|
||||||
| await websocket.accept() | ||||||
|
|
||||||
| # Basic auth: derive user from query parameters or use test user | ||||||
| user_email = websocket.query_params.get('user') | ||||||
| user_email = None | ||||||
|
|
||||||
| # Check configured auth header first (consistent with AuthMiddleware) | ||||||
| auth_header_name = config_manager.app_settings.auth_user_header | ||||||
| x_email_header = websocket.headers.get(auth_header_name) | ||||||
| if x_email_header: | ||||||
| user_email = get_user_from_header(x_email_header) | ||||||
| logger.info( | ||||||
| "WebSocket authenticated via %s header: %s", | ||||||
| sanitize_for_logging(auth_header_name), | ||||||
| sanitize_for_logging(user_email) | ||||||
| ) | ||||||
|
|
||||||
| # Fallback to query parameter for backward compatibility (development/testing) | ||||||
| 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 failureCode scanning / CodeQL Log Injection High
This log entry depends on a
user-provided value Error loading related location Loading Copilot AutofixAI 10 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. |
||||||
|
|
||||||
| # Final fallback to test user (development mode only) | ||||||
| if not user_email: | ||||||
| # Fallback to test user or require auth | ||||||
| config_manager = app_factory.get_config_manager() | ||||||
| user_email = config_manager.app_settings.test_user or 'test@test.com' | ||||||
| logger.info(f"WebSocket using fallback test user: {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)) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| """ | ||
| Integration test demonstrating the fix for the access denied issue. | ||
|
|
||
| This test simulates the exact scenario from the issue: | ||
| - A file belongs to user 'agarlan@sandia.gov' | ||
| - WebSocket connection is authenticated as 'agarlan@sandia.gov' via X-User-Email header | ||
| - Attaching the file should succeed (not fail with "Access denied") | ||
| """ | ||
|
|
||
| import base64 | ||
| import pytest | ||
| from unittest.mock import MagicMock, AsyncMock, patch | ||
| from fastapi.testclient import TestClient | ||
|
|
||
| from main import app | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_components(): | ||
| """Mock all components needed for the test.""" | ||
| with patch('main.app_factory') as mock_factory: | ||
| # Mock config | ||
| mock_config = MagicMock() | ||
| mock_config.app_settings.test_user = 'test@test.com' | ||
| mock_config.app_settings.auth_user_header = 'X-User-Email' | ||
| mock_factory.get_config_manager.return_value = mock_config | ||
|
|
||
| # Mock file manager with S3 client | ||
| mock_file_manager = MagicMock() | ||
| mock_s3_client = MagicMock() | ||
|
|
||
| # Simulate a file that belongs to agarlan@sandia.gov | ||
| async def mock_get_file(user_email, s3_key): | ||
| """Mock S3 get_file that enforces user prefix check.""" | ||
| # This is the actual check from s3_client.py line 185 | ||
| if not s3_key.startswith(f"users/{user_email}/"): | ||
| raise Exception("Access denied to file") | ||
|
|
||
| # If user matches, return file metadata | ||
| return { | ||
| "key": s3_key, | ||
| "filename": "mypdf.pdf", | ||
| "content_base64": base64.b64encode(b"test content").decode(), | ||
| "content_type": "application/pdf", | ||
| "size": 100, | ||
| "etag": "test-etag" | ||
| } | ||
|
|
||
| mock_s3_client.get_file = AsyncMock(side_effect=mock_get_file) | ||
| mock_file_manager.s3_client = mock_s3_client | ||
|
|
||
| # Mock chat service | ||
| mock_chat_service = MagicMock() | ||
| mock_chat_service.handle_attach_file = AsyncMock(return_value={ | ||
| 'type': 'file_attach', | ||
| 'success': True, | ||
| 'filename': 'mypdf.pdf' | ||
| }) | ||
| mock_chat_service.end_session = MagicMock() | ||
| mock_factory.create_chat_service.return_value = mock_chat_service | ||
|
|
||
| yield { | ||
| 'factory': mock_factory, | ||
| 'config': mock_config, | ||
| 'file_manager': mock_file_manager, | ||
| 'chat_service': mock_chat_service | ||
| } | ||
|
|
||
|
|
||
| def test_issue_scenario_fixed_with_correct_user(mock_components): | ||
| """ | ||
| Test the exact scenario from the issue, demonstrating the fix. | ||
|
|
||
| Before fix: | ||
| - WebSocket would use test@test.com (from fallback) | ||
| - Attempting to access users/agarlan@sandia.gov/generated/file.pdf would fail | ||
| - Error: "Access denied: test@test.com attempted to access users/agarlan@sandia.gov/..." | ||
|
|
||
| After fix: | ||
| - WebSocket uses agarlan@sandia.gov (from X-User-Email header) | ||
| - Accessing users/agarlan@sandia.gov/generated/file.pdf succeeds | ||
| """ | ||
| client = TestClient(app) | ||
|
|
||
| # Simulate the production scenario: reverse proxy sets X-User-Email header | ||
| actual_user = "agarlan@sandia.gov" | ||
|
|
||
| # Connect with X-User-Email header (as set by reverse proxy) | ||
| with client.websocket_connect("/ws", headers={"X-User-Email": actual_user}): | ||
| # Verify the connection was created with the correct user | ||
| call_args = mock_components['factory'].create_chat_service.call_args | ||
| connection_adapter = call_args[0][0] | ||
|
|
||
| # This should be the actual user, not test@test.com | ||
| assert connection_adapter.user_email == actual_user, ( | ||
| f"Expected user to be {actual_user}, but got {connection_adapter.user_email}. " | ||
| "This would cause 'Access denied' errors when accessing user's files." | ||
| ) | ||
|
|
||
|
|
||
| def test_issue_scenario_would_fail_without_header(): | ||
| """ | ||
| Demonstrate that without the header, the old behavior (test user fallback) occurs. | ||
| This test shows why the issue existed in the first place. | ||
| """ | ||
| with patch('main.app_factory') as mock_factory: | ||
| # Mock config | ||
| mock_config = MagicMock() | ||
| mock_config.app_settings.test_user = 'test@test.com' | ||
| mock_config.app_settings.auth_user_header = 'X-User-Email' | ||
| mock_factory.get_config_manager.return_value = mock_config | ||
|
|
||
| # Mock chat service | ||
| mock_chat_service = MagicMock() | ||
| mock_chat_service.end_session = MagicMock() | ||
| mock_factory.create_chat_service.return_value = mock_chat_service | ||
|
|
||
| client = TestClient(app) | ||
|
|
||
| # Connect WITHOUT X-User-Email header (simulating old behavior or dev mode) | ||
| with client.websocket_connect("/ws"): | ||
| call_args = mock_factory.create_chat_service.call_args | ||
| connection_adapter = call_args[0][0] | ||
|
|
||
| # Without header, it falls back to test user | ||
| assert connection_adapter.user_email == 'test@test.com', ( | ||
| "Without X-User-Email header, should fall back to test user" | ||
| ) | ||
|
|
||
| # This would cause access denied when trying to access: | ||
| # users/agarlan@sandia.gov/generated/file.pdf | ||
| # because connection is authenticated as test@test.com | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| pytest.main([__file__, "-v"]) |
Check failure
Code scanning / CodeQL
Log Injection High
Copilot Autofix
AI 10 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.