diff --git a/SECURITY.md b/SECURITY.md index daf74d31..1c531edb 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -12,6 +12,7 @@ This is the canonical source of truth for SAPOT's known security-relevant config | Hardcoded JWT secret fallback | `server/app/db_operations/token.py` (`SECRET_KEY`) | The default value has been removed; `JWT_SECRET_KEY` is now required, and the app raises `RuntimeError` at import time if unset. **Rotate to a newly generated secret** (`openssl rand -hex 32`) — the old hardcoded value must be considered compromised since it was committed to source. | | CORS wildcard + credentials | `server/app/main.py` | `allow_origins=["*"]` replaced with an explicit allowlist read from `CORS_ALLOWED_ORIGINS` (comma-separated). The app raises `RuntimeError` at import time if unset. | | Testing router in production | `server/app/main.py`, `server/app/api/testing.py` | The router is imported and mounted only in `development` or `staging`. A router-wide dependency also returns 404 outside those environments if the router is mis-mounted. Every state-changing route requires the `X-QA-Token` shared secret. A production-process regression test exercises every testing path. | +| WebSocket JWTs exposed in request URLs (issue #225) | Server `/ws/` and `/gps/ws/*`; mobile and admin clients | WebSocket URLs no longer contain JWTs. Clients offer exactly `sapot.jwt` and the access token through `Sec-WebSocket-Protocol`; the server selects `sapot.jwt`. Query-token compatibility was removed, and non-development clients require `wss://`. | ## Required environment variables (new) diff --git a/admin-frontend/sapot-admin/lib/ws/Websocketmanager.ts b/admin-frontend/sapot-admin/lib/ws/Websocketmanager.ts index c0cc3de3..193a801c 100644 --- a/admin-frontend/sapot-admin/lib/ws/Websocketmanager.ts +++ b/admin-frontend/sapot-admin/lib/ws/Websocketmanager.ts @@ -81,6 +81,29 @@ export async function getToken(): Promise { return data.token; } +export function normalizeWebSocketDomain( + configuredDomain: string | undefined, + fallbackHost: string, + isDevelopment = process.env.NODE_ENV === "development", +): string { + const value = configuredDomain?.trim() || `wss://${fallbackHost}`; + const withProtocol = value.includes("://") ? value : `wss://${value}`; + const normalized = withProtocol + .replace(/^http:\/\//, "ws://") + .replace(/^https:\/\//, "wss://") + .replace(/\/+$/, ""); + + if (!isDevelopment && normalized.startsWith("ws://")) { + throw new Error("Plaintext WebSocket URLs are only allowed in development"); + } + + if (!normalized.startsWith("ws://") && !normalized.startsWith("wss://")) { + throw new Error("WebSocket domain must use ws:// or wss://"); + } + + return normalized; +} + /* ========================= SEND HELPERS ========================= */ @@ -221,14 +244,13 @@ export async function connectWebSocket(userId: string) { try { const token = await getToken(); - const raw = process.env.NEXT_PUBLIC_WEBSOCKET_DOMAIN; - const wsDomain = (raw || `wss://${window.location.host}`) - .replace(/^http:\/\//, "ws://") - .replace(/^https:\/\//, "wss://"); - - const url = `${wsDomain}/ws/?token=${token}`; + const wsDomain = normalizeWebSocketDomain( + process.env.NEXT_PUBLIC_WEBSOCKET_DOMAIN, + window.location.host, + ); + const url = `${wsDomain}/ws/`; - socket = new WebSocket(url); + socket = new WebSocket(url, ["sapot.jwt", token]); socket.onopen = () => { console.log("[WS] Connected"); @@ -262,7 +284,7 @@ export async function connectWebSocket(userId: string) { await handleSeen(parsed.data as SeenPayload); break; default: - console.warn("[WS] Unknown message type:", (parsed as any).type); + console.warn("[WS] Unknown message type"); } }; diff --git a/docs/api/conventions.md b/docs/api/conventions.md index 62bdedc8..8a8b5b95 100644 --- a/docs/api/conventions.md +++ b/docs/api/conventions.md @@ -170,10 +170,10 @@ Profile pictures are served at `/static/profile_pictures/` directly by ## WebSocket Authentication -WebSocket endpoints authenticate via a `token` query parameter (browsers cannot set custom headers on WS upgrades): +WebSocket endpoints authenticate through the standard subprotocol offer because browser clients cannot set an `Authorization` header on the upgrade request: -``` -wss:///ws/?token= +```javascript +const socket = new WebSocket("wss:///ws/", ["sapot.jwt", accessToken]); ``` -The server closes the connection with code 1008 (policy violation) if the token is invalid. +The offer must contain exactly `sapot.jwt` followed by the access token. The server selects `sapot.jwt` after validating the token. Missing, malformed, expired, or extra protocols close with code 1008 (policy violation). Any `token` query parameter is rejected, even when the subprotocol credentials are valid. diff --git a/docs/api/gps.md b/docs/api/gps.md index aa0d0891..39c8c0ad 100644 --- a/docs/api/gps.md +++ b/docs/api/gps.md @@ -2,14 +2,14 @@ Machine-readable spec: [`openapi/gps.yaml`](openapi/gps.yaml) (generated from the live FastAPI app — REST routes only; WebSocket routes are not representable in OpenAPI and are documented in prose below). Note: neither REST route declares a `response_model`, so the generated YAML's `200` response schema is empty (`{}`) — the JSON examples below are the only documented shape for these responses. -GPS endpoints stream and query user location data (router in `server/app/api/gps.py`, prefix `/gps`). All GPS endpoints require JWT Bearer auth (via `token` query param for WebSocket routes). +GPS endpoints stream and query user location data (router in `server/app/api/gps.py`, prefix `/gps`). REST endpoints use JWT Bearer auth. WebSocket routes use the `sapot.jwt` subprotocol contract. ## Endpoints at a glance | Method | Path | Auth | Summary | |---|---|---|---| -| WS | `/gps/ws/{user_id}` | `token` query param (JWT) | Stream live GPS coordinates from a user's device; server persists and fans out to monitoring rescuers. | -| WS | `/gps/ws/monitor/rescuers/{rescuer_id}` | `token` query param (JWT, rescuer role) | Live feed of every user's GPS updates, for rescuers. | +| WS | `/gps/ws/{user_id}` | `Sec-WebSocket-Protocol: sapot.jwt, ` | Stream live GPS coordinates from a user's device; server persists and fans out to monitoring rescuers. | +| WS | `/gps/ws/monitor/rescuers/{rescuer_id}` | `Sec-WebSocket-Protocol: sapot.jwt, `; rescuer role | Live feed of every user's GPS updates, for rescuers. | | GET | `/gps/latest` | JWT Bearer (rescuer role) | Most recent location for every user who has sent at least one ping. | | GET | `/gps/history/{user_id}` | JWT Bearer (rescuer role) | Location history for a specific user, most recent first. | @@ -19,10 +19,13 @@ GPS endpoints stream and query user location data (router in `server/app/api/gps Stream live GPS coordinates from a user's device to the server. The server persists each ping and fans out to all monitoring rescuers in real time. -**Auth:** `token` query parameter (JWT) +**Auth:** ordered WebSocket subprotocol offer `sapot.jwt`, then the access token -``` -wss:///gps/ws/?token= +```javascript +const socket = new WebSocket("wss:///gps/ws/", [ + "sapot.jwt", + accessToken, +]); ``` **Validation:** @@ -62,10 +65,13 @@ users based on this field. Open a live feed of all users' GPS updates. Rescuers only. -**Auth:** `token` query parameter (JWT) +**Auth:** ordered WebSocket subprotocol offer `sapot.jwt`, then the access token -``` -wss:///gps/ws/monitor/rescuers/?token= +```javascript +const socket = new WebSocket( + "wss:///gps/ws/monitor/rescuers/", + ["sapot.jwt", accessToken], +); ``` **Validation:** diff --git a/docs/api/messaging-and-websocket.md b/docs/api/messaging-and-websocket.md index 388956cd..4376bebb 100644 --- a/docs/api/messaging-and-websocket.md +++ b/docs/api/messaging-and-websocket.md @@ -6,7 +6,7 @@ Machine-readable spec: [`openapi/messaging-and-websocket.yaml`](openapi/messagin | Method | Path | Auth | Summary | |---|---|---|---| -| WS | `/ws/` | `token` query param (JWT); optional `target_id` | Real-time hub: chat relay, WebRTC signalling, presence, public chat. | +| WS | `/ws/` | `Sec-WebSocket-Protocol: sapot.jwt, `; optional `target_id` | Real-time hub: chat relay, WebRTC signalling, presence, public chat. | | GET | `/public-chat` | JWT Bearer | Paginated public chat history. Query params: `limit` (default 100), `before` (created_at cursor, epoch ms). | ## Overview @@ -23,15 +23,18 @@ The server does **not** read message content — it relays encrypted blobs. The ## WebSocket /ws/ -**Auth:** `token` query parameter (JWT) +**Auth:** ordered WebSocket subprotocol offer `sapot.jwt`, then the access token -``` -wss:///ws/?token= -wss:///ws/?token=&target_id= +```javascript +const socket = new WebSocket("wss:///ws/", ["sapot.jwt", accessToken]); +const targetedSocket = new WebSocket("wss:///ws/?target_id=", [ + "sapot.jwt", + accessToken, +]); ``` **On connect:** -1. Token is validated; connection is rejected (code 1008) if invalid. +1. The subprotocol offer and token are validated; the server selects `sapot.jwt`. Invalid or ambiguous offers close with code 1008. 2. Queued messages for this user are drained and delivered. 3. A `status-update` broadcast (`online`) is sent to all connected users. @@ -45,7 +48,7 @@ sequenceDiagram participant Server as /ws/ participant Others as Other connected clients - Client->>Server: connect wss://host/ws/?token= + Client->>Server: connect wss://host/ws/
protocols: sapot.jwt, JWT alt token invalid Server-->>Client: close (code 1008) else token valid diff --git a/docs/api/openapi/messaging-and-websocket.yaml b/docs/api/openapi/messaging-and-websocket.yaml index e439b8eb..c2ffa2ba 100644 --- a/docs/api/openapi/messaging-and-websocket.yaml +++ b/docs/api/openapi/messaging-and-websocket.yaml @@ -41,48 +41,6 @@ paths: application/json: schema: $ref: '#/components/schemas/HTTPValidationError' - /ws/: - get: - tags: - - websockets - - peer connection - summary: Testing Area - operationId: testing_area_ws__get - parameters: - - name: target_id - in: query - required: true - schema: - type: string - format: uuid - title: Target Id - - name: my_id - in: query - required: true - schema: - type: string - format: uuid - title: My Id - - name: token - in: query - required: true - schema: - type: string - title: Token - responses: - '200': - description: Successful Response - content: - application/json: - schema: {} - '404': - description: Not Found - '422': - description: Validation Error - content: - application/json: - schema: - $ref: '#/components/schemas/HTTPValidationError' components: schemas: HTTPValidationError: diff --git a/docs/architecture/networking-lan-model.md b/docs/architecture/networking-lan-model.md index 7fb088cf..02afef7e 100644 --- a/docs/architecture/networking-lan-model.md +++ b/docs/architecture/networking-lan-model.md @@ -74,7 +74,7 @@ sequenceDiagram ## Server connectivity -The SAPOT server is a fixed node on the LAN (static IP or DHCP reservation). Nginx listens on port 443 (TLS). All mobile REST and WebSocket connections go to this server via `https:///`. +The SAPOT server is a fixed node on the LAN (static IP or DHCP reservation). Nginx listens on port 443 (TLS). Mobile REST connections use `https:///`, and WebSocket connections use `wss:///`. --- diff --git a/docs/architecture/security-architecture.md b/docs/architecture/security-architecture.md index c4f8ea3d..1b41b364 100644 --- a/docs/architecture/security-architecture.md +++ b/docs/architecture/security-architecture.md @@ -13,6 +13,12 @@ The server uses a dual-token system (access + refresh): JWT signing uses `PyJWT`. Token expiry constants are in `db_operations/token.py`. +### WebSocket authentication + +WebSocket clients send no JWT in the request URL. They offer exactly two subprotocol values in order: `sapot.jwt` and the access token. The server validates the token, then accepts the connection with `sapot.jwt` selected. + +The server closes the handshake with code 1008 when the protocols are missing, malformed, expired, or ambiguous. It also rejects every `token` query parameter, including requests that also provide valid subprotocol credentials. Production and preview clients use `wss://`; plaintext `ws://` is limited to explicit local development. + --- ## Password hashing diff --git a/docs/deployment/environment-config.md b/docs/deployment/environment-config.md index ac58a7b2..2ed6ec58 100644 --- a/docs/deployment/environment-config.md +++ b/docs/deployment/environment-config.md @@ -85,7 +85,7 @@ Set in EAS project secrets or a local `.env` file (not committed). |---|---|---| | `APP_VARIANT` | Build variant: `development`, `preview`, or unset for production | EAS build | | `SERVER_CA` | Base64-encoded **private CA** PEM, materialized into `server_ca.pem` at prebuild by `app.config.ts`'s `withServerCa` | EAS cloud builds only | -| `EXPO_PUBLIC_DEV_HOST` | Dev-only server hostname/IP used by `config/runtime.ts` to build the API/WS/tile-server URLs (`https://`, `wss://`) | `__DEV__` builds only | +| `EXPO_PUBLIC_DEV_HOST` | Dev-only server hostname/IP used by `config/runtime.ts` to build the API/WS/tile-server URLs. WSS is the default; an explicit plaintext WebSocket origin is accepted only in `__DEV__`. | `__DEV__` builds only | | `EXPO_PUBLIC_SERVER_VERIFY_KEY` | Public key used to verify the server's identity/signature (`config/runtime.ts` → `getServerVerifyKey()`) | All builds, if server signing is enabled | | `EXPO_PUBLIC_ENABLED_LOG_MODULES` | Comma-separated log scopes; unset = all | Development only | | `EXPO_PUBLIC_LOG_TO_FILE` | Set to `1` to force file logging in dev (always on in non-dev builds) | Development only | @@ -115,7 +115,7 @@ Set in EAS project secrets or a local `.env` file (not committed). |---|---|---| | `API_DOMAIN` | SAPOT server base URL, read server-side only (`api/fetch.ts`, `api/login.ts`, `app/api/**/route.ts`, `actions/auth.ts`) — **not** prefixed `NEXT_PUBLIC_`, so it never reaches the client bundle | Yes | | `NEXT_PUBLIC_MAP_STYLE` | MapLibre tile style URL (`ui/components/MapLibre.tsx`) | Yes | -| `NEXT_PUBLIC_WEBSOCKET_DOMAIN` | WebSocket server domain (`lib/ws/Websocketmanager.ts`, non-null asserted — required) | Yes | +| `NEXT_PUBLIC_WEBSOCKET_DOMAIN` | Browser-facing WebSocket origin used by `lib/ws/Websocketmanager.ts`. Production must use `wss://`; local development may explicitly use `ws://` or `http://`. Keep this separate from internal `API_DOMAIN` Docker hostnames. | Yes | | `NODE_ENV` | Toggles the `secure` flag on auth cookies (production vs dev) | Set by the Node runtime; not usually hand-set | Set in `.env.local` (not committed) or the host service manager. `admin-frontend/sapot-admin/.env.example` diff --git a/docs/features/gps/design.md b/docs/features/gps/design.md index 276e89cd..f514a947 100644 --- a/docs/features/gps/design.md +++ b/docs/features/gps/design.md @@ -65,7 +65,7 @@ useEffect(() => { const { status } = await Location.requestForegroundPermissionsAsync() if (status !== 'granted') { setPermissionDenied(true); return } - ws = new WebSocket(`ws://${serverHost}/gps/ws/${userId}?token=${jwt}`) + ws = new WebSocket(`wss://${serverHost}/gps/ws/${userId}`, ['sapot.jwt', jwt]) ws.onopen = () => { locationSub = await Location.watchPositionAsync( { accuracy: Location.Accuracy.Balanced, timeInterval: 5000 }, @@ -112,8 +112,10 @@ monitorWs.onmessage = (e) => { | Path | Role | Direction | Description | |-----------------------------------|------------|----------------|---------------------------------------| -| `/gps/ws/{user_id}?token=JWT` | Any user | client → server| Receive location frames; save + relay | -| `/gps/ws/monitor/rescuers/{id}?token=JWT` | Rescuer | server → client | Push live location frames | +| `/gps/ws/{user_id}` | Any user | client → server| Receive location frames; save + relay | +| `/gps/ws/monitor/rescuers/{id}` | Rescuer | server → client | Push live location frames | + +Both endpoints require the ordered WebSocket subprotocols `sapot.jwt` and the access token. The server selects `sapot.jwt` after authentication. A query token or an ambiguous protocol offer closes with code 1008. ### Connection Manager @@ -188,6 +190,7 @@ sequenceDiagram The GPS WebSocket is managed entirely within `useGpsStreaming` and `gps.py`. It: - Uses a separate WebSocket URL path (`/gps/ws/*` vs `/ws`). +- Keeps credentials out of the URL by sending the access token as the second `Sec-WebSocket-Protocol` value. - Has its own reconnection logic independent of `ConnectionService`. - Does not share state with the messaging or signalling layers. - Can remain connected even if the messaging WebSocket is disconnected. diff --git a/docs/features/gps/requirements.md b/docs/features/gps/requirements.md index bc4f50a5..4436b3b7 100644 --- a/docs/features/gps/requirements.md +++ b/docs/features/gps/requirements.md @@ -23,7 +23,7 @@ GPS location sharing uses a dedicated, server-mediated WebSocket layer independe ### FR-GP-01 — Location Streaming (Any User) - Any authenticated user may stream their GPS location to the server. -- The mobile app opens a WebSocket connection to `WS /gps/ws/{user_id}?token=`. +- The mobile app opens `WS /gps/ws/{user_id}` and offers `sapot.jwt` followed by the access token as WebSocket subprotocols. - The app sends location frames at a configurable interval (default 5 seconds): ```json @@ -58,7 +58,7 @@ GPS location sharing uses a dedicated, server-mediated WebSocket layer independe ### FR-GP-05 — Rescuer Monitor WebSocket -- Rescuers open a separate WebSocket `WS /gps/ws/monitor/rescuers/{rescuer_id}?token=` to receive live location broadcasts. +- Rescuers open a separate `WS /gps/ws/monitor/rescuers/{rescuer_id}` connection with the same `sapot.jwt` subprotocol contract to receive live location broadcasts. - The server pushes each new `UserLocation` frame to all connected rescuer monitors immediately after saving. - Monitor connection is independent of the main messaging WebSocket. @@ -93,6 +93,7 @@ GPS location sharing uses a dedicated, server-mediated WebSocket layer independe | NFR-GP-02 | Server must handle 50 concurrent streaming users without degradation | | NFR-GP-03 | GPS WebSocket must reconnect automatically after network interruption | | NFR-GP-04 | Location data must not be transmitted over unencrypted connections | +| NFR-GP-05 | WebSocket request URLs must not contain access tokens | --- diff --git a/docs/features/gps/testing.md b/docs/features/gps/testing.md index 43998a2e..1be991b8 100644 --- a/docs/features/gps/testing.md +++ b/docs/features/gps/testing.md @@ -40,6 +40,8 @@ | Scenario | Expected result | |----------|-----------------| | User connects to `WS /gps/ws/{user_id}` with valid JWT | Connection accepted; user added to `streaming_connections` | +| User offers `sapot.jwt` followed by a valid JWT | Connection accepted with `sapot.jwt` selected; URL contains no credential | +| User sends a query token or malformed subprotocol list | Connection closes with code 1008 | | User sends `{ "lat": 14.5995, "lng": 120.9842 }` | Row inserted in `user_locations` with correct user_id and timestamp | | User sends malformed frame (missing `lng`) | Connection receives error frame; no DB write | | User disconnects | Removed from `streaming_connections`; no error | @@ -69,6 +71,7 @@ | Hook unmounts | WebSocket `close()` called; `locationSub.remove()` called | | Permission denied on mount | `permissionDenied` state set to `true`; WebSocket never opened | | WebSocket closes unexpectedly | Reconnect attempted after 3 s; exponential back-off applied | +| WebSocket reconnects | Constructor receives `["sapot.jwt", currentToken]`; URL still contains no token | | Component unmounts before reconnect fires | Reconnect timer cleared; no further connection attempts | | GPS update fires while WS is not yet open | Frame queued or skipped; no crash | diff --git a/mobile-app/sapot-mobile-app/config/__tests__/runtime.test.ts b/mobile-app/sapot-mobile-app/config/__tests__/runtime.test.ts index 955ef2fc..7d796412 100644 --- a/mobile-app/sapot-mobile-app/config/__tests__/runtime.test.ts +++ b/mobile-app/sapot-mobile-app/config/__tests__/runtime.test.ts @@ -90,6 +90,29 @@ describe("getWsUrl", () => { jest.resetModules(); }); + it("allows an explicit plaintext websocket URL in development", () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (globalThis as any).__DEV__ = true; + const { normalizeWebSocketUrl } = require("../runtime"); + + expect(normalizeWebSocketUrl("http://127.0.0.1:8000/")).toBe( + "ws://127.0.0.1:8000" + ); + }); + + it("rejects plaintext websocket URLs outside development", () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (globalThis as any).__DEV__ = false; + const { normalizeWebSocketUrl } = require("../runtime"); + + expect(() => normalizeWebSocketUrl("ws://server.sapot.lan")).toThrow( + "Plaintext WebSocket URLs are only allowed in development" + ); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (globalThis as any).__DEV__ = true; + }); + it("should return development websocket URL when in development mode", () => { const { getWsUrl } = require("../runtime"); const result = getWsUrl(); diff --git a/mobile-app/sapot-mobile-app/config/runtime.ts b/mobile-app/sapot-mobile-app/config/runtime.ts index 728ae989..1fc75f99 100644 --- a/mobile-app/sapot-mobile-app/config/runtime.ts +++ b/mobile-app/sapot-mobile-app/config/runtime.ts @@ -66,23 +66,44 @@ export function getServerVerifyKey(): string | undefined { return process.env.EXPO_PUBLIC_SERVER_VERIFY_KEY; } +export const normalizeWebSocketUrl = (baseUrl: string) => { + const trimmed = baseUrl.trim().replace(/\/+$/, ""); + let normalized: string; + + if (trimmed.startsWith("wss://") || trimmed.startsWith("ws://")) { + normalized = trimmed; + } else if (trimmed.startsWith("https://")) { + normalized = `wss://${trimmed.slice("https://".length)}`; + } else if (trimmed.startsWith("http://")) { + normalized = `ws://${trimmed.slice("http://".length)}`; + } else { + normalized = `wss://${trimmed}`; + } + + if (!__DEV__ && normalized.startsWith("ws://")) { + throw new Error("Plaintext WebSocket URLs are only allowed in development"); + } + + return normalized; +}; + export const getWsUrl = () => { - if (_hostOverride) return `wss://${_hostOverride}`; + if (_hostOverride) return normalizeWebSocketUrl(_hostOverride); if (__DEV__) { - return `wss://${DEV_HOST}`; + return normalizeWebSocketUrl(DEV_HOST ?? ""); } const channel = Updates.channel; switch (channel) { case "preview": - return `wss://${SERVER_NAME}`; + return normalizeWebSocketUrl(SERVER_NAME); case "production": - return `wss://${SERVER_NAME}`; + return normalizeWebSocketUrl(SERVER_NAME); default: - return `wss://${DEV_HOST}`; + return normalizeWebSocketUrl(DEV_HOST ?? ""); } }; diff --git a/mobile-app/sapot-mobile-app/docs/API.md b/mobile-app/sapot-mobile-app/docs/API.md index 670b0823..903d1338 100644 --- a/mobile-app/sapot-mobile-app/docs/API.md +++ b/mobile-app/sapot-mobile-app/docs/API.md @@ -792,7 +792,8 @@ the start of history has been reached. Location history and last-known positions for the map screen. Live streaming does **not** go through REST — it uses a dedicated WebSocket (`/gps/ws/`), independent of -`ConnectionService`. +`ConnectionService`. The WebSocket URL contains no token. The client offers `sapot.jwt` and the +access token as its two subprotocol values. ### `GET /gps/latest` — Latest Location Per User **Auth:** Required diff --git a/mobile-app/sapot-mobile-app/docs/ARCHITECTURE.md b/mobile-app/sapot-mobile-app/docs/ARCHITECTURE.md index fade9f60..f7db8953 100644 --- a/mobile-app/sapot-mobile-app/docs/ARCHITECTURE.md +++ b/mobile-app/sapot-mobile-app/docs/ARCHITECTURE.md @@ -265,7 +265,7 @@ Thin injectable wrappers around native modules, allowing them to be replaced wit |---|---| | `TcpServerAdapter` | `react-native-tcp-socket` (server) | | `TcpClientAdapter` | `react-native-tcp-socket` (client, one per peer) | -| `WsSignalingAdapter` | WebSocket with auto-reconnect + heartbeat — shared by `SignalingService`, `ConnectionService`, and `PublicChatService` | +| `WsSignalingAdapter` | WebSocket with auto-reconnect and heartbeat, shared by `SignalingService`, `ConnectionService`, and `PublicChatService`. Offers `sapot.jwt` and the access token as subprotocols so credentials never enter the URL. | | `WebrtcAdapter` | **Facade.** `react-native-webrtc` (RTCPeerConnection, one per peer). Liveness ping/pong probing now delegated to `LivenessMonitor`; ICE-restart backoff delegated to `IceRestartController`. Both sub-units are driven by the adapter via injected closures (no direct adapter reference). `// TODO(refactor): extract local-media-controls` — `initializeLocalStream*`, `toggleMic`, `toggleCamera`, `switchCamera`, `getLocalStream` share `peerConnection`/`localStream` with `createPeerConnection`/`cleanup`, so this split is deferred to avoid a PC-core split; reaching <800 lines is possible once that seam is clean. | | `LivenessMonitor` | Application-level data-channel ping/pong probe, extracted from `WebrtcAdapter`. Detects half-open links and triggers ICE restart via closures. | | `IceRestartController` | ICE-restart scheduling and exponential-backoff logic, extracted from `WebrtcAdapter`. Drives `createOffer({ iceRestart: true })` and emits `signal-offer`/`ice-restarting`/`connection-failed` via closures. | diff --git a/mobile-app/sapot-mobile-app/docs/ENV_CONFIG.md b/mobile-app/sapot-mobile-app/docs/ENV_CONFIG.md index ad6dfa62..bdbde52e 100644 --- a/mobile-app/sapot-mobile-app/docs/ENV_CONFIG.md +++ b/mobile-app/sapot-mobile-app/docs/ENV_CONFIG.md @@ -133,7 +133,7 @@ Logic in `config/runtime.ts`: | Condition | API Base URL | WS Base URL | Tile Server URL | |---|---|---|---| -| Host override set | `https://` | `wss://` | `https:///tiles` | +| Host override set | `https://` | normalized override, secure unless explicit local development | `https:///tiles` | | `__DEV__ === true` | `https://` | `wss://` | `https:///tiles` | | EAS channel `preview` | `https://server.sapot.lan` | `wss://server.sapot.lan` | `https://server.sapot.lan/tiles` | | EAS channel `production` | `https://server.sapot.lan` | `wss://server.sapot.lan` | `https://server.sapot.lan/tiles` | @@ -141,7 +141,9 @@ Logic in `config/runtime.ts`: `server.sapot.lan` is a stable, build-time-fixed hostname (`config/runtime.ts`'s `SERVER_NAME` constant), resolved via normal DNS/hosts on the network — it is not baked to a literal IP, so the server's IP can change without a mobile rebuild as long as `server.sapot.lan` still resolves to it (see the cert-rotation runbook's SAN, which includes both the DNS name and the LAN IP). To point to a different backend locally, update `DEV_HOST` in `config/runtime.ts` or set `EXPO_PUBLIC_DEV_HOST`, or use the dev/QA host override (`setRuntimeHostOverride`, persisted via `secure-config.ts`). -The app always speaks HTTPS/WSS, including in `__DEV__` — there is no plaintext HTTP fallback. Your local dev server must terminate TLS with a cert the dev build's network-security-config trusts (system/user CA store, or the bundled default CA); see `docs/getting-started/mobile-app-setup.md`'s "Configure TLS trust for local development" section. +Preview and production builds require WSS. `normalizeWebSocketUrl()` rejects `ws://` and `http://` outside `__DEV__`. Local development still defaults to WSS, but may use an explicit `ws://` or `http://` origin when a TLS terminator is unavailable. Production REST and tile URLs remain HTTPS-only. + +The mobile signaling and GPS clients both use this shared normalization. They connect to token-free paths and pass `["sapot.jwt", accessToken]` to the WebSocket constructor. Reconnects reuse the current token through the same contract. --- diff --git a/mobile-app/sapot-mobile-app/docs/audits/api-test-cases.md b/mobile-app/sapot-mobile-app/docs/audits/api-test-cases.md index 077a256d..72c62047 100644 --- a/mobile-app/sapot-mobile-app/docs/audits/api-test-cases.md +++ b/mobile-app/sapot-mobile-app/docs/audits/api-test-cases.md @@ -201,15 +201,16 @@ Admin tests seed the DB directly (do not rely on the unauthenticated `/testing/* | ID | Endpoint | Scenario | Request | Expected Response | Priority | Severity | Automate | |----|----------|----------|---------|-------------------|----------|----------|----------| -| API-150 | WS `/gps/ws/{user_id}` | Auth matches user_id | `?token=own_token` + own user_id | Connected; location saved on message | P0 | Critical | Pytest | -| API-151 | WS `/gps/ws/{user_id}` | Auth mismatch (spoofing) | `?token=user_a_token` + user_b_id | Close 1008 | P0 | Critical | Pytest | +| API-150 | WS `/gps/ws/{user_id}` | Auth matches user_id | Protocols `["sapot.jwt", own_token]` + own user_id | Connected with `sapot.jwt`; location saved on message | P0 | Critical | Pytest | +| API-151 | WS `/gps/ws/{user_id}` | Auth mismatch (spoofing) | Protocols `["sapot.jwt", user_a_token]` + user_b_id | Close 1008 | P0 | Critical | Pytest | | API-152 | WS `/gps/ws/{user_id}` | Stream valid coordinates | `{"lat": 14.5, "lng": 121.0}` | Saved to DB; broadcast to monitors | P0 | Critical | Pytest | | API-153 | WS `/gps/ws/{user_id}` | Stream invalid coords | `{"lat": "bad", "lng": null}` | Handled gracefully (not crash) | P1 | High | Pytest | | API-154 | GET `/gps/latest` | Rescuer gets latest | Bearer (rescuer) | 200 list of `{user_id, lat, lng, timestamp}` | P0 | Critical | Pytest | | API-155 | GET `/gps/latest` | Regular user blocked | Bearer (non-rescuer) | 403 | P0 | Critical | Pytest | | API-156 | GET `/gps/history/{user_id}` | Valid history | Bearer (rescuer) | 200 list of locations | P1 | High | Pytest | | API-157 | GET `/gps/history/{user_id}` | No history | Bearer (rescuer) + user with no GPS | 404 | P1 | High | Pytest | -| API-158 | WS `/gps/ws/monitor/rescuers/{id}` | No auth required (BUG) | No token | Currently connects — MUST be 1008/401 | P0 | Critical | Pytest | +| API-158 | WS `/gps/ws/monitor/rescuers/{id}` | Missing or non-rescuer auth | No protocols or non-rescuer JWT | Close 1008 | P0 | Critical | Pytest | +| API-159 | WS `/gps/ws/monitor/rescuers/{id}` | Valid rescuer auth | Protocols `["sapot.jwt", rescuer_token]` + matching ID | Connected with `sapot.jwt` selected | P0 | Critical | Pytest | --- @@ -217,9 +218,9 @@ Admin tests seed the DB directly (do not rely on the unauthenticated `/testing/* | ID | Endpoint | Scenario | Request | Expected Response | Priority | Severity | Automate | |----|----------|----------|---------|-------------------|----------|----------|----------| -| API-160 | WS `/ws/` | Connect with valid access token | `?token=valid` | Connected; `{type: "status-update", status: "online"}` broadcast | P0 | Critical | Pytest | -| API-161 | WS `/ws/` | Connect with expired token | `?token=expired` | Close 1008 | P0 | Critical | Pytest | -| API-162 | WS `/ws/` | Connect without token | No `?token` | Close 1008 | P0 | Critical | Pytest | +| API-160 | WS `/ws/` | Connect with valid access token | Protocols `["sapot.jwt", valid_token]`; token-free URL | Connected with `sapot.jwt`; online status broadcast | P0 | Critical | Pytest | +| API-161 | WS `/ws/` | Connect with expired token | Protocols `["sapot.jwt", expired_token]` | Close 1008 | P0 | Critical | Pytest | +| API-162 | WS `/ws/` | Connect without protocols | No subprotocol offer | Close 1008 | P0 | Critical | Pytest | | API-163 | WS `/ws/` | Ping → pong | `{type: "ping"}` | `{type: "pong"}` | P0 | Critical | Pytest | | API-164 | WS `/ws/` | Get active users | `{type: "get-active-users"}` | List of connected user UUIDs | P1 | High | Pytest | | API-165 | WS `/ws/` | Chat to online peer | `{type: "chat", data: {to: online_peer}}` | Peer receives; no server-ack | P0 | Critical | Pytest | @@ -233,6 +234,8 @@ Admin tests seed the DB directly (do not rely on the unauthenticated `/testing/* | API-173 | WS `/ws/` | Queued messages drained on connect | Messages in queue | All delivered immediately on connect | P0 | Critical | Pytest | | API-174 | WS `/ws/` | Stale ACK-type entries deleted on drain | ACK in queue | Deleted without delivery | P1 | High | Pytest | | API-175 | WS `/ws/` | Seen-type delivered then deleted | Seen in queue | Delivered then removed | P1 | High | Pytest | +| API-176 | WS `/ws/` | Ambiguous subprotocol offer | Missing marker/token, reversed order, or extra protocol | Close 1008 | P0 | Critical | Pytest | +| API-177 | WS `/ws/` | Query-token downgrade attempt | Query token alone or combined with valid subprotocols | Close 1008 | P0 | Critical | Pytest | --- diff --git a/mobile-app/sapot-mobile-app/docs/audits/regression-suite.md b/mobile-app/sapot-mobile-app/docs/audits/regression-suite.md index ce15aafc..78fd69f8 100644 --- a/mobile-app/sapot-mobile-app/docs/audits/regression-suite.md +++ b/mobile-app/sapot-mobile-app/docs/audits/regression-suite.md @@ -160,7 +160,7 @@ Before merging to `main` or tagging a release: 1. GET / → {"state": "running"} ✓ 2. POST /auth/ (register) → 200 + tokens ✓ 3. POST /auth/token (login) → 200 + tokens ✓ -4. WS /ws/?token= → connected + status-update:online ✓ +4. WS /ws/ with protocols [sapot.jwt, token] → connected + status-update:online ✓ 5. Send {type: "ping"} → {type: "pong"} ✓ 6. GET /sync/pull?last_pulled_at=0 → {changes, timestamp} ✓ 7. GET /user-utils/get-announcements → 200 ✓ diff --git a/mobile-app/sapot-mobile-app/docs/audits/test-cases.md b/mobile-app/sapot-mobile-app/docs/audits/test-cases.md index 6aa037de..7b702315 100644 --- a/mobile-app/sapot-mobile-app/docs/audits/test-cases.md +++ b/mobile-app/sapot-mobile-app/docs/audits/test-cases.md @@ -346,10 +346,11 @@ The table below is a quick-reference for precondition states. Full setup procedu | TC-244 | Key Recovery | PBKDF2 is deterministic | Known inputs | 1. Derive key twice | Same key both times | P0 | Critical | Jest | | TC-245 | Key Recovery | Wrong recovery key rejected | | 1. Provide wrong key | Recovery blocked | P0 | Critical | Jest | | TC-246 | Security | Testing endpoints require admin auth | Admin token required | 1. POST `/testing/test-make-admin` without token 2. POST with non-admin token 3. POST with admin token | Steps 1–2 return 401/403; step 3 returns 200 | P0 | Critical | Pytest | -| TC-247 | Security | GPS monitor WS requires valid rescuer token | | 1. Connect to `/gps/ws/monitor/rescuers/{id}` without token 2. Connect with non-rescuer token 3. Connect with mismatched rescuer ID 4. Connect with valid rescuer token and matching ID | Steps 1–3 close with code 1008; step 4 connects successfully | P0 | Critical | Pytest | +| TC-247 | Security | GPS monitor WS requires valid rescuer token | | 1. Connect without subprotocols 2. Connect with `["sapot.jwt", non_rescuer_token]` 3. Connect with a mismatched rescuer ID 4. Connect with `["sapot.jwt", rescuer_token]` and matching ID | Steps 1–3 close with code 1008; step 4 selects `sapot.jwt` and connects | P0 | Critical | Pytest | | TC-248 | Security | `/auth/exists` rate-limited | | 1. Send 100 req/min | Rate limit applied | P0 | Critical | Pytest | | TC-249 | Security | Server Host Override not in production build | Prod build | 1. Open drawer | URL override field absent | P0 | Critical | Maestro | | TC-250 | Security | JWT uses environment secret | Production | 1. Check env | `JWT_SECRET_KEY` set; no hardcoded fallback | P0 | Critical | Pytest | +| TC-251 | Security | WebSocket JWT never enters a URL (issue #225) | Updated server and clients | 1. Connect signaling and GPS 2. Trigger reconnect 3. Inspect request paths and constructor args 4. Attempt a query token | Paths contain no secret; constructor protocols are `["sapot.jwt", token]`; query attempt closes 1008 | P0 | Critical | Jest + Pytest | --- diff --git a/mobile-app/sapot-mobile-app/docs/audits/test-inventory.md b/mobile-app/sapot-mobile-app/docs/audits/test-inventory.md index 7c600940..0038c2e5 100644 --- a/mobile-app/sapot-mobile-app/docs/audits/test-inventory.md +++ b/mobile-app/sapot-mobile-app/docs/audits/test-inventory.md @@ -225,7 +225,7 @@ Scope: React Native / Expo frontend + FastAPI backend ## Critical Security Issues (Found During Inventory) 1. **`/testing/test-make-admin` and `/testing/test-make-rescuer`** — no authentication, included in production router. Any unauthenticated request can grant admin or rescuer privileges. -2. **`/gps/ws/monitor/rescuers/{rescuer_id}`** — GPS location monitor WebSocket has no authentication. Any client can receive all user GPS coordinates. +2. **Resolved (TC-247, issue #225):** `/gps/ws/monitor/rescuers/{rescuer_id}` now requires a matching rescuer JWT through the `sapot.jwt` subprotocol contract. Missing, malformed, or query-string credentials close with code 1008. 3. **`/auth/exists`** — no rate limiting; enables username and email enumeration at scale. 4. **JWT secret fallback** — hardcoded fallback value used when `JWT_SECRET_KEY` env var is not set in production. 5. **CORS fully open** — `allow_origins=["*"]` combined with `allow_credentials=True` is an invalid CORS configuration per spec; credentials are silently dropped by browsers. diff --git a/mobile-app/sapot-mobile-app/features/gps/services/__tests__/gps-location-service.test.ts b/mobile-app/sapot-mobile-app/features/gps/services/__tests__/gps-location-service.test.ts new file mode 100644 index 00000000..497d9be5 --- /dev/null +++ b/mobile-app/sapot-mobile-app/features/gps/services/__tests__/gps-location-service.test.ts @@ -0,0 +1,86 @@ +import * as Location from "expo-location"; + +import { GpsLocationService } from "../gps-location-service"; + +jest.mock("expo-location", () => ({ + Accuracy: { Balanced: 3 }, + watchPositionAsync: jest.fn(), +})); + +interface ConstructorCall { + url: string; + protocols?: string | string[]; +} + +class MockWebSocket { + static OPEN = 1; + static calls: ConstructorCall[] = []; + static instances: MockWebSocket[] = []; + + readyState = 0; + onopen: (() => void) | null = null; + onerror: ((event: unknown) => void) | null = null; + onclose: + | ((event: { code: number; reason: string; wasClean: boolean }) => void) + | null = null; + send = jest.fn(); + + constructor(url: string, protocols?: string | string[]) { + MockWebSocket.calls.push({ url, protocols }); + MockWebSocket.instances.push(this); + } + + close(code = 1000, reason = "") { + this.readyState = 3; + this.onclose?.({ code, reason, wasClean: code === 1000 }); + } +} + +describe("GpsLocationService WebSocket authentication", () => { + beforeEach(() => { + jest.useFakeTimers(); + MockWebSocket.calls = []; + MockWebSocket.instances = []; + globalThis.WebSocket = MockWebSocket as unknown as typeof WebSocket; + jest.mocked(Location.watchPositionAsync).mockResolvedValue({ + remove: jest.fn(), + }); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it("keeps the token out of the GPS URL and offers it as a subprotocol", async () => { + const service = new GpsLocationService(); + + await service.start("https://server.sapot.lan/", "user/id", "gps-token"); + + expect(MockWebSocket.calls[0]).toEqual({ + url: "wss://server.sapot.lan/gps/ws/user%2Fid", + protocols: ["sapot.jwt", "gps-token"], + }); + expect(MockWebSocket.calls[0].url).not.toContain("gps-token"); + + service.stop(); + }); + + it("reuses the current token as a subprotocol when reconnecting", async () => { + const service = new GpsLocationService(); + await service.start("wss://server.sapot.lan", "user-id", "current-token"); + + MockWebSocket.instances[0].onclose?.({ + code: 1006, + reason: "network_lost", + wasClean: false, + }); + jest.advanceTimersByTime(3_000); + + expect(MockWebSocket.calls[1]).toEqual({ + url: "wss://server.sapot.lan/gps/ws/user-id", + protocols: ["sapot.jwt", "current-token"], + }); + + service.stop(); + }); +}); diff --git a/mobile-app/sapot-mobile-app/features/gps/services/gps-location-service.ts b/mobile-app/sapot-mobile-app/features/gps/services/gps-location-service.ts index 91137556..6c6d4406 100644 --- a/mobile-app/sapot-mobile-app/features/gps/services/gps-location-service.ts +++ b/mobile-app/sapot-mobile-app/features/gps/services/gps-location-service.ts @@ -1,4 +1,5 @@ import * as Location from "expo-location"; +import { normalizeWebSocketUrl } from "@/config/runtime"; import { toAppError } from "@/features/shared/core/errors"; import { gpsLog } from "@/features/shared/core/utils/logger"; import { haversineMeters } from "../utils/haversine"; @@ -10,6 +11,7 @@ const MIN_SEND_INTERVAL_MS = 3_000; const SIGNIFICANT_MOVE_METERS = 10; const HEARTBEAT_INTERVAL_MS = 30_000; const RECONNECT_DELAY_MS = 3_000; +const WEBSOCKET_AUTH_PROTOCOL = "sapot.jwt"; export class GpsLocationService { private ws: WebSocket | null = null; @@ -143,10 +145,10 @@ export class GpsLocationService { private connectWs() { if (this.stopped) return; - const base = this.wsBaseUrl.replace(/\/+$/, ""); - const url = `${base}/gps/ws/${this.userId}?token=${encodeURIComponent(this.token)}`; + const base = normalizeWebSocketUrl(this.wsBaseUrl); + const url = `${base}/gps/ws/${encodeURIComponent(this.userId)}`; gpsLog.info("gps › ws connect", { url }); - const ws = new WebSocket(url); + const ws = new WebSocket(url, [WEBSOCKET_AUTH_PROTOCOL, this.token]); ws.onopen = () => { gpsLog.info("gps › ws open"); diff --git a/mobile-app/sapot-mobile-app/features/shared/connection/adapters/__tests__/ws-signaling-adapter-auth.test.ts b/mobile-app/sapot-mobile-app/features/shared/connection/adapters/__tests__/ws-signaling-adapter-auth.test.ts new file mode 100644 index 00000000..e33656e1 --- /dev/null +++ b/mobile-app/sapot-mobile-app/features/shared/connection/adapters/__tests__/ws-signaling-adapter-auth.test.ts @@ -0,0 +1,99 @@ +import { WsSignalingAdapter } from "../ws-signaling-adapter"; + +interface ConstructorCall { + url: string; + protocols?: string | string[]; +} + +class MockWebSocket { + static OPEN = 1; + static calls: ConstructorCall[] = []; + static instances: MockWebSocket[] = []; + + readyState = 0; + onopen: (() => void) | null = null; + onmessage: ((event: { data: unknown }) => void) | null = null; + onerror: ((event: unknown) => void) | null = null; + onclose: + | ((event: { code: number; reason: string; wasClean: boolean }) => void) + | null = null; + send = jest.fn(); + + constructor(url: string, protocols?: string | string[]) { + MockWebSocket.calls.push({ url, protocols }); + MockWebSocket.instances.push(this); + } + + open() { + this.readyState = MockWebSocket.OPEN; + this.onopen?.(); + } + + close(code = 1000, reason = "") { + this.readyState = 3; + this.onclose?.({ code, reason, wasClean: code === 1000 }); + } +} + +describe("WsSignalingAdapter authentication", () => { + beforeEach(() => { + jest.useFakeTimers(); + MockWebSocket.calls = []; + MockWebSocket.instances = []; + globalThis.WebSocket = MockWebSocket as unknown as typeof WebSocket; + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it("keeps the token out of the URL and offers it as a subprotocol", async () => { + const adapter = new WsSignalingAdapter(); + const connectPromise = adapter.connect({ + baseUrl: "https://server.sapot.lan/", + token: "secret-token", + extraQuery: { target_id: "peer-id" }, + }); + + MockWebSocket.instances[0].open(); + await connectPromise; + + expect(MockWebSocket.calls[0]).toEqual({ + url: "wss://server.sapot.lan/ws/?target_id=peer-id", + protocols: ["sapot.jwt", "secret-token"], + }); + expect(MockWebSocket.calls[0].url).not.toContain("secret-token"); + + adapter.disconnect(); + }); + + it("reuses the current token as a subprotocol when reconnecting", async () => { + jest.spyOn(Math, "random").mockReturnValue(0.5); + const adapter = new WsSignalingAdapter(); + const connectPromise = adapter.connect({ + baseUrl: "wss://server.sapot.lan", + token: "current-token", + reconnectBaseDelayMs: 100, + reconnectMaxDelayMs: 100, + }); + + MockWebSocket.instances[0].open(); + await connectPromise; + MockWebSocket.instances[0].onclose?.({ + code: 1006, + reason: "network_lost", + wasClean: false, + }); + + jest.advanceTimersByTime(100); + + expect(MockWebSocket.calls).toHaveLength(2); + expect(MockWebSocket.calls[1]).toEqual({ + url: "wss://server.sapot.lan/ws/", + protocols: ["sapot.jwt", "current-token"], + }); + + MockWebSocket.instances[1].open(); + adapter.disconnect(); + }); +}); diff --git a/mobile-app/sapot-mobile-app/features/shared/connection/adapters/ws-signaling-adapter.ts b/mobile-app/sapot-mobile-app/features/shared/connection/adapters/ws-signaling-adapter.ts index 4de133ff..a69d5ec8 100644 --- a/mobile-app/sapot-mobile-app/features/shared/connection/adapters/ws-signaling-adapter.ts +++ b/mobile-app/sapot-mobile-app/features/shared/connection/adapters/ws-signaling-adapter.ts @@ -1,4 +1,5 @@ import EventEmitter from "events"; +import { normalizeWebSocketUrl } from "@/config/runtime"; import { SendPublicChatPayload } from "@/features/shared/core/messaging-types"; import { AckMessage, @@ -41,7 +42,7 @@ interface WsLike { } interface WsConstructor { - new (url: string): WsLike; + new (url: string, protocols?: string | string[]): WsLike; OPEN: number; } @@ -55,7 +56,7 @@ interface ConnectOptions { reconnectMaxDelayMs?: number; heartbeatIntervalMs?: number; heartbeatTimeoutMs?: number; - extraQuery?: Record; + extraQuery?: { target_id?: string }; } type AdapterState = "idle" | "connecting" | "open" | "closing"; @@ -70,6 +71,7 @@ interface QueuedSignalingMessage { /** WebRTC negotiation traffic — meaningless once its session is gone. */ const NEGOTIATION_TYPES = new Set(["offer", "answer", "ice-candidate"]); +const WEBSOCKET_AUTH_PROTOCOL = "sapot.jwt"; /** * WsSignalingAdapter handles websocket signaling for WebRTC negotiation. @@ -144,7 +146,10 @@ export class WsSignalingAdapter extends EventEmitter { hasToken: Boolean(options.token), }); - const socket = new (this.getWebSocketCtor())(wsUrl); + const socket = new (this.getWebSocketCtor())(wsUrl, [ + WEBSOCKET_AUTH_PROTOCOL, + options.token, + ]); this.socket = socket; this.state = "connecting"; const socketEpoch = ++this.socketEpoch; @@ -749,10 +754,8 @@ export class WsSignalingAdapter extends EventEmitter { private buildWsUrl(options: ConnectOptions) { const path = options.path ?? "/ws/"; - const normalizedBase = this.normalizeBaseUrl(options.baseUrl); - const query: Record = { - token: options.token, - }; + const normalizedBase = normalizeWebSocketUrl(options.baseUrl); + const query = { target_id: options.extraQuery?.target_id }; const queryString = Object.entries(query) .filter(([, value]) => value !== undefined) @@ -766,24 +769,6 @@ export class WsSignalingAdapter extends EventEmitter { return `${normalizedBase}${path}${queryString ? `?${queryString}` : ""}`; } - private normalizeBaseUrl(baseUrl: string) { - const trimmedBase = baseUrl.replace(/\/+$/, ""); - - if (trimmedBase.startsWith("ws://") || trimmedBase.startsWith("wss://")) { - return trimmedBase; - } - - if (trimmedBase.startsWith("https://")) { - return `wss://${trimmedBase.slice("https://".length)}`; - } - - if (trimmedBase.startsWith("http://")) { - return `ws://${trimmedBase.slice("http://".length)}`; - } - - return `ws://${trimmedBase}`; - } - private getWebSocketCtor(): WsConstructor { const ctor = (globalThis as unknown as { WebSocket?: WsConstructor }) .WebSocket; diff --git a/postman/collections/messaging-and-websocket.postman_collection.json b/postman/collections/messaging-and-websocket.postman_collection.json index 14a37790..872ce91b 100644 --- a/postman/collections/messaging-and-websocket.postman_collection.json +++ b/postman/collections/messaging-and-websocket.postman_collection.json @@ -304,297 +304,6 @@ "protocolProfileBehavior": { "disableBodyPruning": true } - }, - { - "id": "5a4f0b23c28fc98164b5e018bf7595ce", - "name": "Testing Area", - "request": { - "name": "Testing Area", - "description": { - }, - "url": { - "path": [ - "ws", - "" - ], - "host": [ - "{{baseUrl}}" - ], - "query": [ - { - "disabled": false, - "description": { - "content": "(Required) ", - "type": "text/plain" - }, - "key": "target_id", - "value": "" - }, - { - "disabled": false, - "description": { - "content": "(Required) ", - "type": "text/plain" - }, - "key": "my_id", - "value": "" - }, - { - "disabled": false, - "description": { - "content": "(Required) ", - "type": "text/plain" - }, - "key": "token", - "value": "" - } - ], - "variable": [ - - ] - }, - "header": [ - { - "key": "Accept", - "value": "application/json" - } - ], - "method": "GET", - "body": { - } - }, - "response": [ - { - "id": "faef25f8636913b709009dc77c9c68db", - "name": "Successful Response", - "originalRequest": { - "url": { - "path": [ - "ws", - "" - ], - "host": [ - "{{baseUrl}}" - ], - "query": [ - { - "disabled": false, - "description": { - "content": "(Required) ", - "type": "text/plain" - }, - "key": "target_id", - "value": "" - }, - { - "disabled": false, - "description": { - "content": "(Required) ", - "type": "text/plain" - }, - "key": "my_id", - "value": "" - }, - { - "disabled": false, - "description": { - "content": "(Required) ", - "type": "text/plain" - }, - "key": "token", - "value": "" - } - ], - "variable": [ - - ] - }, - "header": [ - { - "key": "Accept", - "value": "application/json" - } - ], - "method": "GET", - "body": { - } - }, - "status": "OK", - "code": 200, - "header": [ - { - "key": "Content-Type", - "value": "application/json" - } - ], - "body": "", - "cookie": [ - - ], - "_postman_previewlanguage": "json" - }, - { - "id": "90f64c9b6e3f4e5697a29e09d666efbd", - "name": "Not Found", - "originalRequest": { - "url": { - "path": [ - "ws", - "" - ], - "host": [ - "{{baseUrl}}" - ], - "query": [ - { - "disabled": false, - "description": { - "content": "(Required) ", - "type": "text/plain" - }, - "key": "target_id", - "value": "" - }, - { - "disabled": false, - "description": { - "content": "(Required) ", - "type": "text/plain" - }, - "key": "my_id", - "value": "" - }, - { - "disabled": false, - "description": { - "content": "(Required) ", - "type": "text/plain" - }, - "key": "token", - "value": "" - } - ], - "variable": [ - - ] - }, - "method": "GET", - "body": { - } - }, - "status": "Not Found", - "code": 404, - "header": [ - - ], - "cookie": [ - - ], - "_postman_previewlanguage": "text" - }, - { - "id": "56cba9d2f0be725b8908ff21900cbc24", - "name": "Validation Error", - "originalRequest": { - "url": { - "path": [ - "ws", - "" - ], - "host": [ - "{{baseUrl}}" - ], - "query": [ - { - "disabled": false, - "description": { - "content": "(Required) ", - "type": "text/plain" - }, - "key": "target_id", - "value": "" - }, - { - "disabled": false, - "description": { - "content": "(Required) ", - "type": "text/plain" - }, - "key": "my_id", - "value": "" - }, - { - "disabled": false, - "description": { - "content": "(Required) ", - "type": "text/plain" - }, - "key": "token", - "value": "" - } - ], - "variable": [ - - ] - }, - "header": [ - { - "key": "Accept", - "value": "application/json" - } - ], - "method": "GET", - "body": { - } - }, - "status": "Unprocessable Entity (WebDAV) (RFC 4918)", - "code": 422, - "header": [ - { - "key": "Content-Type", - "value": "application/json" - } - ], - "body": "{\n \"detail\": [\n {\n \"loc\": [\n \"\",\n \"\"\n ],\n \"msg\": \"\",\n \"type\": \"\"\n },\n {\n \"loc\": [\n \"\",\n \"\"\n ],\n \"msg\": \"\",\n \"type\": \"\"\n }\n ]\n}", - "cookie": [ - - ], - "_postman_previewlanguage": "json" - } - ], - "event": [ - { - "listen": "test", - "script": { - "type": "text/javascript", - "exec": [ - "// Generated by scripts/generate_postman_collection.rb. Do not hand-edit --", - "// regenerating overwrites this file.", - "pm.test(\"does not return a server error\", function () {", - " pm.expect(pm.response.code).to.be.below(500);", - "});", - "", - "pm.test(\"responds within 5s\", function () {", - " pm.expect(pm.response.responseTime).to.be.below(5000);", - "});", - "", - "pm.test(\"a JSON content-type means a parseable JSON body\", function () {", - " var contentType = pm.response.headers.get(\"Content-Type\") || \"\";", - " if (contentType.indexOf(\"application/json\") === -1) {", - " return;", - " }", - " if (pm.response.text().length === 0) {", - " return;", - " }", - " pm.response.to.be.json;", - "});" - ] - } - } - ], - "protocolProfileBehavior": { - "disableBodyPruning": true - } } ] } diff --git a/server/app/api/gps.py b/server/app/api/gps.py index 9ece7fda..19e4a129 100644 --- a/server/app/api/gps.py +++ b/server/app/api/gps.py @@ -15,7 +15,11 @@ from app.db_operations.token import get_current_user, get_current_user_rescuer from app.db_operations.user_search import _resolve_role -from app.db_operations.websockets import authenticate_websocket, WebSocketAuthError +from app.db_operations.websocket_auth import ( + WEBSOCKET_AUTH_PROTOCOL, + WebSocketAuthError, + authenticate_websocket, +) from app.models.rescuer import Rescuer from app.models.users import User from app.models.location import UserLocation @@ -36,11 +40,10 @@ async def stream_gps_location( websocket: WebSocket, user_id: str, - token: str, session: SessionDep ): try: - authed_id = await authenticate_websocket(websocket, token) + authed_id = await authenticate_websocket(websocket) except WebSocketAuthError: logger.warning("WebSocket auth rejected: invalid or expired token client=%s", websocket.client) return @@ -48,7 +51,7 @@ async def stream_gps_location( if str(authed_id) != user_id: await websocket.close(code=1008) return - await websocket.accept() + await websocket.accept(subprotocol=WEBSOCKET_AUTH_PROTOCOL) try: user_uuid = uuid.UUID(user_id) @@ -176,11 +179,10 @@ def get_user_location_history( async def monitor_live_feed( websocket: WebSocket, rescuer_id: str, - token: str, session: SessionDep, ): try: - authed_id = await authenticate_websocket(websocket, token) + authed_id = await authenticate_websocket(websocket) except WebSocketAuthError: logger.warning("WebSocket auth rejected: invalid or expired token client=%s", websocket.client) return diff --git a/server/app/api/peer_connection.py b/server/app/api/peer_connection.py index b2036d9e..5158df55 100644 --- a/server/app/api/peer_connection.py +++ b/server/app/api/peer_connection.py @@ -1,25 +1,23 @@ import asyncio -import json import logging import re - import ast -import enum -from typing import Annotated -from uuid import UUID import json -import ast +from uuid import UUID -from sqlmodel import except_, select, Session +from sqlmodel import select, Session from app.db_operations.auth import SessionDep, engine -from fastapi import APIRouter, Depends, WebSocket -from fastapi.responses import HTMLResponse -from fastapi import APIRouter, WebSocket, WebSocketDisconnect, Query -from app.db_operations.token import verify_token +from fastapi import APIRouter, WebSocket, WebSocketDisconnect from app.models.queued import Queue from app.models.signalling import SignalMessage -from fastapi import Query, WebSocketDisconnect -from app.db_operations.websockets import authenticate_websocket, relay_message, relay_public_message, validate_message_sender, validate_sender, relay_signal, receive_signal_message, WebSocketAuthError +from app.db_operations.websocket_auth import WebSocketAuthError, authenticate_websocket +from app.db_operations.websockets import ( + receive_signal_message, + relay_message, + relay_public_message, + relay_signal, + validate_sender, +) from app.db_operations.connection_manager import manager from app.db_operations.activity import set_user_status from app.models.websocketComms import MessageData, PublicMessageData @@ -76,55 +74,6 @@ def _set_status_bg(user_id: UUID, status: str) -> None: # })); -html = """ - - - - Chat - - -

WebSocket Chat

-

Your ID:

-
- - -
-
    -
- - - -""" - - -@router.get("/") -async def testing_area(target_id: UUID, my_id: UUID, token: str): - return HTMLResponse(html) - - def get_queued_messages(user_id: UUID, session: SessionDep, limit: int = 100): try: statement = select(Queue).where(Queue.to == user_id).limit(limit) @@ -167,7 +116,7 @@ def deep_parse_dict(data): @router.websocket("/") -async def main_web_socket(token: str, websocket: WebSocket, target_id: UUID|None = None): +async def main_web_socket(websocket: WebSocket, target_id: UUID|None = None): """ will relay the sdp between different users can handle @@ -177,13 +126,13 @@ async def main_web_socket(token: str, websocket: WebSocket, target_id: UUID|None handshakes """ try: - user_id = await authenticate_websocket(websocket, token) + user_id = await authenticate_websocket(websocket) except WebSocketAuthError: logger.warning("WebSocket auth rejected: invalid or expired token client=%s", websocket.client) return - await manager.connect(UUID(user_id), websocket) - asyncio.get_event_loop().run_in_executor(None, _set_status_bg, UUID(user_id), "Active") + await manager.connect(user_id, websocket) + asyncio.get_event_loop().run_in_executor(None, _set_status_bg, user_id, "Active") try: await manager.broadcast({"type": "status-update", "user_id": user_id, 'status': "online"}) except: @@ -191,7 +140,7 @@ async def main_web_socket(token: str, websocket: WebSocket, target_id: UUID|None try: with Session(engine) as session: - messages = get_queued_messages(UUID(user_id), session) + messages = get_queued_messages(user_id, session) if messages: for message in messages: try: @@ -238,10 +187,10 @@ async def main_web_socket(token: str, websocket: WebSocket, target_id: UUID|None except Exception: payload = raw_payload if isinstance(payload, dict) and payload.get("type") == "ping": - await manager.send_personal_message(UUID(user_id), {"type": "pong"}) + await manager.send_personal_message(user_id, {"type": "pong"}) # get online users elif isinstance(payload, dict) and payload.get("type") == "get-active-users": - await manager.send_personal_message(UUID(user_id), await manager.get_active_connections()) + await manager.send_personal_message(user_id, await manager.get_active_connections()) # relay public chat data elif isinstance(payload, PublicMessageData): with Session(engine) as session: @@ -263,5 +212,5 @@ async def main_web_socket(token: str, websocket: WebSocket, target_id: UUID|None await manager.broadcast({"type": "status-update","user_id": user_id, 'status': "offline"}) except: pass - asyncio.get_event_loop().run_in_executor(None, _set_status_bg, UUID(user_id), "Inactive") - await manager.disconnect(UUID(user_id)) + asyncio.get_event_loop().run_in_executor(None, _set_status_bg, user_id, "Inactive") + await manager.disconnect(user_id) diff --git a/server/app/db_operations/GPS_manager.py b/server/app/db_operations/GPS_manager.py index 59ef327e..2f26f526 100644 --- a/server/app/db_operations/GPS_manager.py +++ b/server/app/db_operations/GPS_manager.py @@ -1,6 +1,7 @@ -import json from fastapi import WebSocket from typing import Dict +from app.db_operations.websocket_auth import WEBSOCKET_AUTH_PROTOCOL + class GPSManager: def __init__(self): @@ -8,7 +9,7 @@ def __init__(self): self.active_monitors: Dict[str, WebSocket] = {} async def connect_monitor(self, user_id: str, websocket: WebSocket): - await websocket.accept() + await websocket.accept(subprotocol=WEBSOCKET_AUTH_PROTOCOL) self.active_monitors[user_id] = websocket def disconnect_monitor(self, user_id: str): diff --git a/server/app/db_operations/connection_manager.py b/server/app/db_operations/connection_manager.py index 55802969..0b5a55bb 100644 --- a/server/app/db_operations/connection_manager.py +++ b/server/app/db_operations/connection_manager.py @@ -7,6 +7,7 @@ from typing import Dict, Optional import redis.asyncio as aioredis +from app.db_operations.websocket_auth import WEBSOCKET_AUTH_PROTOCOL logger = logging.getLogger(__name__) @@ -61,7 +62,7 @@ async def shutdown(self) -> None: # ------------------------------------------------------------------ async def connect(self, user_id: UUID, websocket: WebSocket) -> None: - await websocket.accept() + await websocket.accept(subprotocol=WEBSOCKET_AUTH_PROTOCOL) self._local[user_id] = websocket await self._set_presence(user_id) pubsub = self._redis.pubsub() diff --git a/server/app/db_operations/websocket_auth.py b/server/app/db_operations/websocket_auth.py new file mode 100644 index 00000000..c250d236 --- /dev/null +++ b/server/app/db_operations/websocket_auth.py @@ -0,0 +1,38 @@ +from uuid import UUID + +from fastapi import WebSocket + +from app.db_operations.token import verify_token + +WEBSOCKET_AUTH_PROTOCOL = "sapot.jwt" + + +class WebSocketAuthError(Exception): + pass + + +async def authenticate_websocket(websocket: WebSocket) -> UUID: + protocols = websocket.scope.get("subprotocols", []) + + if ( + "token" in websocket.query_params + or len(protocols) != 2 + or protocols[0] != WEBSOCKET_AUTH_PROTOCOL + or not protocols[1] + ): + await websocket.close(code=1008) + raise WebSocketAuthError("Unauthorized") + + try: + user_id = verify_token(protocols[1]) + except KeyError: + user_id = None + if not user_id: + await websocket.close(code=1008) + raise WebSocketAuthError("Unauthorized") + + try: + return UUID(user_id) + except (TypeError, ValueError): + await websocket.close(code=1008) + raise WebSocketAuthError("Unauthorized") from None diff --git a/server/app/db_operations/websockets.py b/server/app/db_operations/websockets.py index 52aed6da..6f644bc2 100644 --- a/server/app/db_operations/websockets.py +++ b/server/app/db_operations/websockets.py @@ -8,25 +8,10 @@ from app.models.message import Message from app.models.queued import Queue from app.models.signalling import SignalMessage -from app.db_operations.token import verify_token from app.db_operations.connection_manager import manager from app.models.users import User from app.models.websocketComms import MessageData, PublicMessageData - -class WebSocketAuthError(Exception): - pass - - -async def authenticate_websocket(websocket: WebSocket, token: str) -> UUID: - user_id = verify_token(token) - - if not user_id: - await websocket.close(code=1008) - raise WebSocketAuthError("Unauthorized") - - return user_id - def validate_sender(payload: SignalMessage, user_id: UUID) -> bool: data = payload.data.model_dump() id = data.get('sender'), UUID diff --git a/server/app/tests/test_gps.py b/server/app/tests/test_gps.py index 3288afd8..b34cfefd 100644 --- a/server/app/tests/test_gps.py +++ b/server/app/tests/test_gps.py @@ -10,18 +10,29 @@ import json from app.tests.test_db_utils import get_auth_headers +from app.db_operations.token import create_access_token +from app.db_operations.websocket_auth import WEBSOCKET_AUTH_PROTOCOL -def test_stream_gps_location_success(client: TestClient): +def _protocols(user_id: uuid.UUID) -> list[str]: + token = create_access_token({"sub": str(user_id)}) + return [WEBSOCKET_AUTH_PROTOCOL, token] + + +def test_stream_gps_location_success(client: TestClient, test_user_instance): """ Test that a user can connect to the GPS WebSocket and send coordinates. """ - test_user_id = str(uuid.uuid4()) + test_user_id = str(test_user_instance.id) payload = {"lat": 14.4589, "lng": 120.9486} # 1. Open the WebSocket connection # Note: the path must match your router's path - with client.websocket_connect(f"/gps/ws/{test_user_id}") as websocket: + with client.websocket_connect( + f"/gps/ws/{test_user_id}", + subprotocols=_protocols(test_user_instance.id), + ) as websocket: + assert websocket.accepted_subprotocol == WEBSOCKET_AUTH_PROTOCOL # 2. Send the JSON data (Simulating the React Native 'send') websocket.send_json(payload) @@ -34,13 +45,16 @@ def test_stream_gps_location_success(client: TestClient): assert websocket.scope["path"] == f"/gps/ws/{test_user_id}" -def test_stream_gps_invalid_data(client: TestClient): +def test_stream_gps_invalid_data(client: TestClient, test_user_instance): """ Test how the WebSocket handles garbage data. """ - test_user_id = str(uuid.uuid4()) + test_user_id = str(test_user_instance.id) - with client.websocket_connect(f"/gps/ws/{test_user_id}") as websocket: + with client.websocket_connect( + f"/gps/ws/{test_user_id}", + subprotocols=_protocols(test_user_instance.id), + ) as websocket: # Sending a string instead of the expected JSON object websocket.send_text("not-a-json") @@ -166,15 +180,21 @@ def test_get_history_not_found(client: TestClient, test_user_instance, test_resc def test_gps_broadcast_to_rescuer(client, session, test_user_instance, test_rescuer): user_id_str = str(test_user_instance.id) - rescuer_id_str = str(test_rescuer.id) + rescuer_id_str = user_id_str payload = {"lat": 14.5, "lng": 121.0} + protocols = _protocols(test_user_instance.id) # 1. Start the Rescuer monitor - with client.websocket_connect(f"/gps/ws/monitor/rescuers/{rescuer_id_str}") as rescuer_ws: + with client.websocket_connect( + f"/gps/ws/monitor/rescuers/{rescuer_id_str}", subprotocols=protocols + ) as rescuer_ws: + assert rescuer_ws.accepted_subprotocol == WEBSOCKET_AUTH_PROTOCOL # 2. Open AND CLOSE the user connection # This triggers the broadcast while the rescuer is still 'alive' inside the 'with' block - with client.websocket_connect(f"/gps/ws/{user_id_str}") as user_ws: + with client.websocket_connect( + f"/gps/ws/{user_id_str}", subprotocols=protocols + ) as user_ws: user_ws.send_json(payload) # 3. Verify the broadcasted data was received @@ -184,4 +204,3 @@ def test_gps_broadcast_to_rescuer(client, session, test_user_instance, test_resc # After exiting the block, the rescuer is disconnected # and the 'raise' inside your endpoint is handled by the TestClient - diff --git a/server/app/tests/test_security_regression.py b/server/app/tests/test_security_regression.py index 08547236..ab87a37f 100644 --- a/server/app/tests/test_security_regression.py +++ b/server/app/tests/test_security_regression.py @@ -15,12 +15,14 @@ import pytest from fastapi import HTTPException from fastapi.testclient import TestClient +from starlette.websockets import WebSocketDisconnect from sqlmodel import Session from app.models.users import User from app.models.rescuer import Rescuer from app.db_operations.auth import get_password_hash -from app.tests.test_db_utils import get_auth_headers +from app.db_operations.token import create_access_token +from app.db_operations.websocket_auth import WEBSOCKET_AUTH_PROTOCOL def test_gsm_secret_is_required_at_import_time(): @@ -86,9 +88,9 @@ def rescuer_user(session: Session) -> User: return user -def _token(client: TestClient, username: str, password: str) -> str: - headers = get_auth_headers(client, username, password) - return headers["Authorization"].removeprefix("Bearer ") +def _protocols(user: User) -> list[str]: + token = create_access_token({"sub": str(user.id)}) + return [WEBSOCKET_AUTH_PROTOCOL, token] # --------------------------------------------------------------------------- @@ -99,47 +101,65 @@ class TestTC247MonitorWebSocketRequiresRescuerAuth: def test_rejects_missing_token(self, client: TestClient, rescuer_user: User): rescuer_id = str(rescuer_user.id) - with pytest.raises(Exception): + with pytest.raises(WebSocketDisconnect) as exc_info: with client.websocket_connect( - f"/gps/ws/monitor/rescuers/{rescuer_id}" # no ?token= + f"/gps/ws/monitor/rescuers/{rescuer_id}" ) as ws: ws.receive_text() + assert exc_info.value.code == 1008 def test_rejects_invalid_token(self, client: TestClient, rescuer_user: User): rescuer_id = str(rescuer_user.id) - with pytest.raises(Exception): + with pytest.raises(WebSocketDisconnect) as exc_info: with client.websocket_connect( - f"/gps/ws/monitor/rescuers/{rescuer_id}?token=not-a-real-token" + f"/gps/ws/monitor/rescuers/{rescuer_id}", + subprotocols=[WEBSOCKET_AUTH_PROTOCOL, "not-a-real-token"], ) as ws: ws.receive_text() + assert exc_info.value.code == 1008 def test_rejects_non_rescuer_token(self, client: TestClient, plain_user: User): - token = _token(client, plain_user.username, "PlainPass1") # Use the plain user's own ID so the ID check would pass — rescuer check must still reject - with pytest.raises(Exception): + with pytest.raises(WebSocketDisconnect) as exc_info: with client.websocket_connect( - f"/gps/ws/monitor/rescuers/{plain_user.id}?token={token}" + f"/gps/ws/monitor/rescuers/{plain_user.id}", + subprotocols=_protocols(plain_user), ) as ws: ws.receive_text() + assert exc_info.value.code == 1008 def test_rejects_rescuer_token_with_mismatched_id( self, client: TestClient, rescuer_user: User ): - token = _token(client, rescuer_user.username, "RescuerPass1") wrong_id = str(uuid.uuid4()) - with pytest.raises(Exception): + with pytest.raises(WebSocketDisconnect) as exc_info: with client.websocket_connect( - f"/gps/ws/monitor/rescuers/{wrong_id}?token={token}" + f"/gps/ws/monitor/rescuers/{wrong_id}", + subprotocols=_protocols(rescuer_user), ) as ws: ws.receive_text() + assert exc_info.value.code == 1008 def test_accepts_valid_rescuer_token(self, client: TestClient, rescuer_user: User): - token = _token(client, rescuer_user.username, "RescuerPass1") rescuer_id = str(rescuer_user.id) with client.websocket_connect( - f"/gps/ws/monitor/rescuers/{rescuer_id}?token={token}" + f"/gps/ws/monitor/rescuers/{rescuer_id}", + subprotocols=_protocols(rescuer_user), ) as ws: assert ws.scope["path"] == f"/gps/ws/monitor/rescuers/{rescuer_id}" + assert ws.accepted_subprotocol == WEBSOCKET_AUTH_PROTOCOL + + +def test_gps_stream_rejects_token_for_a_different_user( + client: TestClient, plain_user: User +): + with pytest.raises(WebSocketDisconnect) as exc_info: + with client.websocket_connect( + f"/gps/ws/{uuid.uuid4()}", subprotocols=_protocols(plain_user) + ): + pass + + assert exc_info.value.code == 1008 # --------------------------------------------------------------------------- diff --git a/server/app/tests/test_websocket_auth_rejection.py b/server/app/tests/test_websocket_auth_rejection.py index e3396dec..570525bd 100644 --- a/server/app/tests/test_websocket_auth_rejection.py +++ b/server/app/tests/test_websocket_auth_rejection.py @@ -19,17 +19,37 @@ """ import logging import uuid +from datetime import timedelta import pytest from fastapi.testclient import TestClient from starlette.websockets import WebSocketDisconnect +import app.api.peer_connection as peer_connection +from app.db_operations.connection_manager import manager +from app.db_operations.token import create_access_token +from app.db_operations.websocket_auth import WEBSOCKET_AUTH_PROTOCOL + + +def _protocols(token: str) -> list[str]: + return [WEBSOCKET_AUTH_PROTOCOL, token] + + +@pytest.fixture +def signaling_handshake_only(monkeypatch): + async def connect(user_id, websocket): + await websocket.accept(subprotocol=WEBSOCKET_AUTH_PROTOCOL) + + monkeypatch.setattr(manager, "connect", connect) + monkeypatch.setattr(peer_connection, "get_queued_messages", lambda *args: []) + monkeypatch.setattr(peer_connection, "_set_status_bg", lambda *args: None) + def test_invalid_token_rejects_cleanly_without_unhandled_exception(client: TestClient): # Arrange / Act / Assert: an invalid token must close the socket (1008) # without the raw auth exception escaping the ASGI app unhandled. with pytest.raises(WebSocketDisconnect) as exc_info: - with client.websocket_connect("/ws/?token=not-a-real-token"): + with client.websocket_connect("/ws/", subprotocols=_protocols("not-a-real-token")): pass assert exc_info.value.code == 1008 @@ -41,7 +61,7 @@ def test_invalid_token_logs_concise_warning_not_error(client: TestClient, caplog # Act with pytest.raises(WebSocketDisconnect): - with client.websocket_connect("/ws/?token=not-a-real-token"): + with client.websocket_connect("/ws/", subprotocols=_protocols("not-a-real-token")): pass # Assert: a concise warning was logged, and nothing at ERROR level @@ -60,7 +80,9 @@ def test_gps_stream_invalid_token_rejects_cleanly_without_unhandled_exception(cl # Act / Assert: an invalid token on the GPS stream socket must close # cleanly (1008) rather than letting WebSocketAuthError escape unhandled. with pytest.raises(WebSocketDisconnect) as exc_info: - with client.websocket_connect(f"/gps/ws/{user_id}?token=not-a-real-token"): + with client.websocket_connect( + f"/gps/ws/{user_id}", subprotocols=_protocols("not-a-real-token") + ): pass assert exc_info.value.code == 1008 @@ -73,7 +95,9 @@ def test_gps_stream_invalid_token_logs_concise_warning_not_error(client: TestCli # Act with pytest.raises(WebSocketDisconnect): - with client.websocket_connect(f"/gps/ws/{user_id}?token=not-a-real-token"): + with client.websocket_connect( + f"/gps/ws/{user_id}", subprotocols=_protocols("not-a-real-token") + ): pass # Assert @@ -90,7 +114,10 @@ def test_gps_monitor_invalid_token_rejects_cleanly_without_unhandled_exception(c # Act / Assert with pytest.raises(WebSocketDisconnect) as exc_info: - with client.websocket_connect(f"/gps/ws/monitor/rescuers/{rescuer_id}?token=not-a-real-token"): + with client.websocket_connect( + f"/gps/ws/monitor/rescuers/{rescuer_id}", + subprotocols=_protocols("not-a-real-token"), + ): pass assert exc_info.value.code == 1008 @@ -103,7 +130,10 @@ def test_gps_monitor_invalid_token_logs_concise_warning_not_error(client: TestCl # Act with pytest.raises(WebSocketDisconnect): - with client.websocket_connect(f"/gps/ws/monitor/rescuers/{rescuer_id}?token=not-a-real-token"): + with client.websocket_connect( + f"/gps/ws/monitor/rescuers/{rescuer_id}", + subprotocols=_protocols("not-a-real-token"), + ): pass # Assert @@ -125,7 +155,7 @@ def _make_ws_403_record() -> logging.LogRecord: pathname=__file__, lineno=1, msg='%s - "WebSocket %s" 403', - args=(("127.0.0.1", 54321), "/ws/?token=not-a-real-token"), + args=(("127.0.0.1", 54321), "/ws/"), exc_info=None, ) @@ -163,3 +193,88 @@ def test_uvicorn_ws_403_filter_leaves_other_uvicorn_error_records_alone(): ) assert filt.filter(record) is True + + +@pytest.mark.parametrize( + "protocols", + [ + [], + [WEBSOCKET_AUTH_PROTOCOL], + ["not-sapot.jwt", "token"], + [WEBSOCKET_AUTH_PROTOCOL, "token", "extra"], + ["token", WEBSOCKET_AUTH_PROTOCOL], + ], +) +def test_missing_or_ambiguous_protocols_close_with_1008( + client: TestClient, protocols: list[str] +): + with pytest.raises(WebSocketDisconnect) as exc_info: + with client.websocket_connect("/ws/", subprotocols=protocols): + pass + + assert exc_info.value.code == 1008 + + +def test_expired_subprotocol_token_closes_with_1008(client: TestClient): + token = create_access_token( + {"sub": str(uuid.uuid4())}, expires_delta=timedelta(seconds=-1) + ) + + with pytest.raises(WebSocketDisconnect) as exc_info: + with client.websocket_connect("/ws/", subprotocols=_protocols(token)): + pass + + assert exc_info.value.code == 1008 + + +def test_signed_token_without_subject_closes_with_1008(client: TestClient): + token = create_access_token({}) + + with pytest.raises(WebSocketDisconnect) as exc_info: + with client.websocket_connect("/ws/", subprotocols=_protocols(token)): + pass + + assert exc_info.value.code == 1008 + + +def test_query_only_token_closes_with_1008(client: TestClient): + token = create_access_token({"sub": str(uuid.uuid4())}) + + with pytest.raises(WebSocketDisconnect) as exc_info: + with client.websocket_connect(f"/ws/?token={token}"): + pass + + assert exc_info.value.code == 1008 + + +def test_query_token_with_valid_subprotocol_auth_closes_with_1008(client: TestClient): + token = create_access_token({"sub": str(uuid.uuid4())}) + + with pytest.raises(WebSocketDisconnect) as exc_info: + with client.websocket_connect( + f"/ws/?token={token}", subprotocols=_protocols(token) + ): + pass + + assert exc_info.value.code == 1008 + + +def test_valid_subprotocol_is_selected_and_target_id_remains_supported( + client: TestClient, signaling_handshake_only +): + token = create_access_token({"sub": str(uuid.uuid4())}) + target_id = uuid.uuid4() + + with client.websocket_connect( + f"/ws/?target_id={target_id}", subprotocols=_protocols(token) + ) as websocket: + assert websocket.accepted_subprotocol == WEBSOCKET_AUTH_PROTOCOL + assert websocket.scope["query_string"] == f"target_id={target_id}".encode() + + +def test_obsolete_websocket_html_test_route_is_not_reachable(client: TestClient): + response = client.get( + f"/ws/?target_id={uuid.uuid4()}&my_id={uuid.uuid4()}" + ) + + assert response.status_code == 404 diff --git a/server/app/tests/test_websocket_pool.py b/server/app/tests/test_websocket_pool.py index 39489a14..b7856fae 100644 --- a/server/app/tests/test_websocket_pool.py +++ b/server/app/tests/test_websocket_pool.py @@ -20,7 +20,9 @@ import app.api.peer_connection as peer_connection from app.main import app from app.db_operations.auth import get_session +from app.db_operations.connection_manager import manager from app.db_operations.token import create_access_token +from app.db_operations.websocket_auth import WEBSOCKET_AUTH_PROTOCOL @pytest.fixture(name="pool_engine") @@ -47,6 +49,12 @@ def override_get_session(): # _set_status_bg and the connect-time drain reach the module-level engine. monkeypatch.setattr(peer_connection, "engine", engine) + async def connect_without_redis(user_id, websocket): + await websocket.accept(subprotocol=WEBSOCKET_AUTH_PROTOCOL) + manager._local[user_id] = websocket + + monkeypatch.setattr(manager, "connect", connect_without_redis) + yield engine app.dependency_overrides.clear() @@ -71,12 +79,10 @@ def test_idle_websocket_holds_no_pool_connection(pool_engine): # Act: connect and reach the idle receive loop (ping/pong proves the # connect-time drain has completed and the handler is waiting). - with client.websocket_connect(f"/ws/?token={token}") as ws: - # The handler broadcasts an "online" status-update to all peers - # (including the connecting one) on connect — drain it first. - first = ws.receive_json() - assert first["type"] == "status-update" - + with client.websocket_connect( + "/ws/", subprotocols=[WEBSOCKET_AUTH_PROTOCOL, token] + ) as ws: + assert ws.accepted_subprotocol == WEBSOCKET_AUTH_PROTOCOL ws.send_json({"type": "ping"}) assert ws.receive_json() == {"type": "pong"} diff --git a/server/documentation.org b/server/documentation.org index 829d5c20..9488b29f 100644 --- a/server/documentation.org +++ b/server/documentation.org @@ -326,10 +326,10 @@ requires token url parameter sent to a user email. Responds with an html page requires user to be authenticated ** Websocket/signalling -*** SDP relay example ~/ws GET~ +*** SDP relay example ~/ws WebSocket~ - access like - ~https://ylp-software.onrender.com/ws/?target_id=8380dbb9-482e-42fa-a1fd-fafedd9f918d&token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIwOWM4ZDRlYS00OGYwLTRiN2UtYWY1Mi0yZDNmY2FkMGU2OGQiLCJleHAiOjE3NzI0NTU2MDh9.aD-vGqlRHdx_-zHm9Oev12VKCvOoU9MhLm-7rTLnNok&my_id=09c8d4ea-48f0-4b7e-af52-2d3fcad0e68d~. - Notice the url parameters of my_id, target_id, and token (access_token). + ~wss://server.sapot.lan/ws/?target_id=8380dbb9-482e-42fa-a1fd-fafedd9f918d~. + Offer ~sapot.jwt~ followed by the access token as WebSocket subprotocols. *** SDP relay ~ws://localhost:8000/ws/~ (not visible on the docs) - this is an endpoint that will be used for relaying SDP offers, answers, and @@ -338,7 +338,7 @@ requires user to be authenticated Anything else will be rejected. **** Sample Connection to websocket #+BEGIN_SRC json - ws://localhost:8000/ws/?target_id=${client_id}&token=${token} + ws://localhost:8000/ws/?target_id=${client_id} #+END_SRC Put this link url (change domain as needed but it needs ws:// or wss:// at the protocol) inside a WebSocket object, then send a sdp message like @@ -351,7 +351,7 @@ requires user to be authenticated } #+END_SRC - *Note that the ~from~ and ~to~ should be the same as the connection parameters* + *Note that the ~from~ and ~to~ should match the authenticated user and target.* ***** TODO ****** ADD TYPES @@ -1175,12 +1175,12 @@ use the web socket like: const WebSocket = require('ws'); // 1. Configuration - const TOKEN = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJlNTA1OGIwZS0xOTYxLTRiOWItODVmZC02NmU4Yjk3MzMwYjMiLCJqdGkiOiJlYjcwNjI4Ny1hNDA2LTQzMTYtOGQwNi0wMmI4YTFkZWJlODAiLCJ0eXBlIjoiYWNjZXNzIiwiZXhwIjoxNzc2OTkyNzA5fQ.TTOr7rSLK9rSYuDB2iefyTw0Z11WARzm9Y7enfI_EIk"; - const WS_URL = `ws://localhost:8000/ws/?token=${TOKEN}`; + const TOKEN = ""; + const WS_URL = "ws://localhost:8000/ws/"; const MY_USER_ID = "e5058b0e-1961-4b9b-85fd-66e8b97330b3"; - const socket = new WebSocket(WS_URL); + const socket = new WebSocket(WS_URL, ["sapot.jwt", TOKEN]); socket.on('open', () => { console.log("✅ Connected to Public Chat Server"); @@ -1221,5 +1221,3 @@ use the web socket like: #+END_SRC This public chat can be retrieved through ~/sync/pull~ endpoint. Public chat records must have conversation_id as ~null~. - -