From 41b7d221e4578287f86d845284af9eb27c9a57e7 Mon Sep 17 00:00:00 2001 From: Viktor Barzin Date: Fri, 13 Feb 2026 19:36:43 +0000 Subject: [PATCH] Fix 7 bugs: security, memory leak, stale state, error handling - WebSocket: verify task ownership before allowing subscribe (security) - POI routes: replace assert with HTTPException for production safety - cancel_task: return HTTP 404 instead of 200 for missing tasks - routing_config: add descriptive ValueError for invalid env vars - POIManager: show error feedback instead of silently swallowing failures - VisualizationCard: reset POI/travel mode state on metric switch - Map: clean up heatmap layers/sources on unmount to prevent memory leak - Update test to expect 404 from cancel_task ownership check --- api/app.py | 2 +- api/poi_routes.py | 3 ++- api/ws_routes.py | 4 ++++ config/routing_config.py | 13 +++++++++++-- frontend/src/components/Map.tsx | 13 +++++++++++++ frontend/src/components/POIManager.tsx | 13 +++++++++++-- frontend/src/components/VisualizationCard.tsx | 2 ++ tests/integration/test_api_workflow.py | 4 +--- 8 files changed, 45 insertions(+), 9 deletions(-) diff --git a/api/app.py b/api/app.py index 9bbcbc0..b76d4e5 100644 --- a/api/app.py +++ b/api/app.py @@ -415,7 +415,7 @@ async def cancel_task( # Verify user owns this task user_tasks = task_service.get_user_tasks(user.email) if task_id not in user_tasks: - return {"success": False, "message": "Task not found or not owned by user"} + raise HTTPException(status_code=404, detail="Task not found or not owned by user") try: task_service.cancel_task(task_id, user_email=user.email) diff --git a/api/poi_routes.py b/api/poi_routes.py index b701980..eb74841 100644 --- a/api/poi_routes.py +++ b/api/poi_routes.py @@ -60,7 +60,8 @@ def _get_user_id(user: User) -> int: if db_user is None: # Auto-create user on first POI interaction db_user = user_repo.create_user(user.email) - assert db_user.id is not None + if db_user.id is None: + raise HTTPException(status_code=500, detail="Failed to create user") return db_user.id diff --git a/api/ws_routes.py b/api/ws_routes.py index 2b2d44d..1bed49f 100644 --- a/api/ws_routes.py +++ b/api/ws_routes.py @@ -137,6 +137,10 @@ async def ws_task_progress(websocket: WebSocket) -> None: if msg_type == "subscribe": new_task_id = msg.get("task_id") if new_task_id: + # Verify task belongs to the authenticated user + user_tasks = task_service.get_user_tasks(user.email) + if new_task_id not in user_tasks: + continue channel = f"task_progress:{new_task_id}" if channel not in subscribed_channels: await pubsub.subscribe(channel) diff --git a/config/routing_config.py b/config/routing_config.py index 79a00e6..7080e34 100644 --- a/config/routing_config.py +++ b/config/routing_config.py @@ -6,6 +6,15 @@ from dataclasses import dataclass from typing import Self +def _int_env(name: str, default: str) -> int: + """Parse an integer environment variable with a descriptive error.""" + raw = os.environ.get(name, default) + try: + return int(raw) + except ValueError: + raise ValueError(f"Environment variable {name}={raw!r} must be an integer") + + @dataclass(frozen=True) class RoutingConfig: """Configuration for self-hosted routing engines (OSRM + OTP). @@ -39,8 +48,8 @@ class RoutingConfig: osrm_foot_url=os.environ.get("OSRM_FOOT_URL", "http://osrm-foot:5000"), osrm_bicycle_url=os.environ.get("OSRM_BICYCLE_URL", "http://osrm-bicycle:5000"), otp_url=os.environ.get("OTP_URL", "http://otp:8080"), - osrm_batch_size=int(os.environ.get("OSRM_BATCH_SIZE", "50")), - otp_max_concurrent=int(os.environ.get("OTP_MAX_CONCURRENT", "10")), + osrm_batch_size=_int_env("OSRM_BATCH_SIZE", "50"), + otp_max_concurrent=_int_env("OTP_MAX_CONCURRENT", "10"), ) def get_osrm_url(self, profile: str) -> str: diff --git a/frontend/src/components/Map.tsx b/frontend/src/components/Map.tsx index ff6b1bc..bc10edd 100644 --- a/frontend/src/components/Map.tsx +++ b/frontend/src/components/Map.tsx @@ -194,6 +194,19 @@ export function Map(props: MapProps) { if (updateTimeoutRef.current) { clearTimeout(updateTimeoutRef.current); } + // Remove heatmap layers and sources before destroying the map + if (heatmapRef.current && mapRef.current) { + for (const layerId of ['hexgrid-heatmap', 'hexgrid-heatmap-back']) { + if (mapRef.current.getLayer(layerId)) { + mapRef.current.removeLayer(layerId); + } + } + for (const sourceId of ['hexgrid-heatmap', 'hexgrid-heatmap-back']) { + if (mapRef.current.getSource(sourceId)) { + mapRef.current.removeSource(sourceId); + } + } + } heatmapRef.current = null; isMapLoadedRef.current = false; mapRef.current?.remove(); diff --git a/frontend/src/components/POIManager.tsx b/frontend/src/components/POIManager.tsx index bfeae46..bb4c503 100644 --- a/frontend/src/components/POIManager.tsx +++ b/frontend/src/components/POIManager.tsx @@ -23,6 +23,7 @@ export function POIManager({ user, listingType, onTaskCreated, pickedLocation, o const [lng, setLng] = useState(''); const [calculating, setCalculating] = useState(null); const [selectedModes, setSelectedModes] = useState(['WALK', 'BICYCLE', 'TRANSIT']); + const [error, setError] = useState(null); useEffect(() => { fetchUserPOIs(user).then(setPois).catch(() => {}); @@ -38,6 +39,7 @@ export function POIManager({ user, listingType, onTaskCreated, pickedLocation, o const handleCreate = async () => { if (!name || !lat || !lng) return; + setError(null); try { const poi = await createPOI(user, { name, @@ -62,16 +64,17 @@ export function POIManager({ user, listingType, onTaskCreated, pickedLocation, o // Non-fatal: POI created successfully, calculation can be retried manually. } } catch { - // silently fail + setError('Failed to create POI. Please try again.'); } }; const handleDelete = async (poiId: number) => { + setError(null); try { await deletePOI(user, poiId); setPois(prev => prev.filter(p => p.id !== poiId)); } catch { - // silently fail + setError('Failed to delete POI. Please try again.'); } }; @@ -95,6 +98,12 @@ export function POIManager({ user, listingType, onTaskCreated, pickedLocation, o return (
+ {error && ( +
+ {error} + +
+ )} {/* POI List */} {pois.map(poi => (
diff --git a/frontend/src/components/VisualizationCard.tsx b/frontend/src/components/VisualizationCard.tsx index d2a0229..67fb04a 100644 --- a/frontend/src/components/VisualizationCard.tsx +++ b/frontend/src/components/VisualizationCard.tsx @@ -24,6 +24,8 @@ export function VisualizationCard({ metric, onMetricChange, userPOIs, onPoiMetri onValueChange={(value) => { onMetricChange(value as Metric); if (value !== Metric.poi_travel) { + setSelectedPoiId(''); + setSelectedTravelMode(''); onPoiMetricChange?.(null); } }} diff --git a/tests/integration/test_api_workflow.py b/tests/integration/test_api_workflow.py index 132fc4f..4e32ccd 100644 --- a/tests/integration/test_api_workflow.py +++ b/tests/integration/test_api_workflow.py @@ -296,9 +296,7 @@ async def test_cancel_task_not_owned( monkeypatch.setattr("services.task_service.get_user_tasks", lambda email: []) resp = await async_client.post("/api/cancel_task?task_id=not-mine") - assert resp.status_code == 200 - data = resp.json() - assert data["success"] is False + assert resp.status_code == 404 @pytest.mark.asyncio