diff --git a/.dockerignore b/.dockerignore index 4ab0f62..d9ee946 100644 --- a/.dockerignore +++ b/.dockerignore @@ -5,6 +5,24 @@ client/dist data uploads .git +.github .env +.env.* *.log *.md +!client/**/*.md +chart/ +docs/ +docker-compose.yml +unraid-template.xml +*.sqlite +*.sqlite-shm +*.sqlite-wal +*.db +*.db-shm +*.db-wal +coverage +.DS_Store +Thumbs.db +.vscode +.idea diff --git a/.gitignore b/.gitignore index 44d3160..595a6b5 100644 --- a/.gitignore +++ b/.gitignore @@ -11,6 +11,9 @@ client/public/icons/*.png *.db *.db-shm *.db-wal +*.sqlite +*.sqlite-shm +*.sqlite-wal # User data server/data/ diff --git a/AUDIT_FINDINGS.md b/AUDIT_FINDINGS.md new file mode 100644 index 0000000..65fff0f --- /dev/null +++ b/AUDIT_FINDINGS.md @@ -0,0 +1,281 @@ +# TREK Security & Code Quality Audit + +**Date:** 2026-03-30 +**Auditor:** Automated comprehensive audit +**Scope:** Full codebase — server, client, infrastructure, dependencies + +--- + +## Table of Contents + +1. [Security](#1-security) +2. [Code Quality](#2-code-quality) +3. [Best Practices](#3-best-practices) +4. [Dependency Hygiene](#4-dependency-hygiene) +5. [Documentation & DX](#5-documentation--dx) +6. [Testing](#6-testing) +7. [Remediation Summary](#7-remediation-summary) + +--- + +## 1. Security + +### 1.1 General + +| # | Severity | File | Line(s) | Description | Recommended Fix | Status | +|---|----------|------|---------|-------------|-----------------|--------| +| S-1 | **CRITICAL** | `server/src/middleware/auth.ts` | 17 | JWT `verify()` does not pin algorithm — accepts whatever algorithm is in the token header, potentially including `none`. | Pass `{ algorithms: ['HS256'] }` to all `jwt.verify()` calls. | FIXED | +| S-2 | **HIGH** | `server/src/websocket.ts` | 56 | Same JWT verify without algorithm pinning in WebSocket auth. | Pin algorithm to HS256. | FIXED | +| S-3 | **HIGH** | `server/src/middleware/mfaPolicy.ts` | 54 | Same JWT verify without algorithm pinning. | Pin algorithm to HS256. | FIXED | +| S-4 | **HIGH** | `server/src/routes/oidc.ts` | 84-88 | OIDC `generateToken()` includes excessive claims (username, email, role) in JWT payload. If the JWT is leaked, this exposes PII. | Only include `{ id: user.id }` in token, consistent with auth.ts. | FIXED | +| S-5 | **HIGH** | `client/src/api/websocket.ts` | 27 | Auth token passed in WebSocket URL query string (`?token=`). Tokens in URLs appear in server logs, proxy logs, and browser history. | Document as known limitation; WebSocket protocol doesn't easily support headers from browsers. Add `LOW` priority note to switch to message-based auth in the future. | DOCUMENTED | +| S-6 | **HIGH** | `client/vite.config.js` | 47-56 | Service worker caches ALL `/api/.*` responses with `NetworkFirst`, including auth tokens, user data, budget, reservations. Data persists after logout. | Exclude sensitive API paths from caching: `/api/auth/.*`, `/api/admin/.*`, `/api/backup/.*`. | FIXED | +| S-7 | **HIGH** | `client/vite.config.js` | 57-65 | User-uploaded files (possibly passport scans, booking confirmations) cached with `CacheFirst` for 30 days, persisting after logout. | Reduce cache lifetime; add note about clearing on logout. | FIXED | +| S-8 | **MEDIUM** | `server/src/index.ts` | 60 | CSP allows `'unsafe-inline'` for scripts, weakening XSS protection. | Remove `'unsafe-inline'` from `scriptSrc` if Vite build doesn't require it. If needed for development, only allow in non-production. | FIXED | +| S-9 | **MEDIUM** | `server/src/index.ts` | 64 | CSP `connectSrc` allows `http:` and `https:` broadly, permitting connections to any origin. | Restrict to known API domains (nominatim, overpass, Google APIs) or use `'self'` with specific external origins. | FIXED | +| S-10 | **MEDIUM** | `server/src/index.ts` | 62 | CSP `imgSrc` allows `http:` broadly. | Restrict to `https:` and `'self'` plus known image domains. | FIXED | +| S-11 | **MEDIUM** | `server/src/websocket.ts` | 84-90 | No message size limit on WebSocket messages. A malicious client could send very large messages to exhaust server memory. | Set `maxPayload` on WebSocketServer configuration. | FIXED | +| S-12 | **MEDIUM** | `server/src/websocket.ts` | 84 | No rate limiting on WebSocket messages. A client can flood the server with join/leave messages. | Add per-connection message rate limiting. | FIXED | +| S-13 | **MEDIUM** | `server/src/websocket.ts` | 29 | No origin validation on WebSocket connections. | Add origin checking against allowed origins. | FIXED | +| S-14 | **MEDIUM** | `server/src/routes/auth.ts` | 157-163 | JWT tokens have 24h expiry with no refresh token mechanism. Long-lived tokens increase window of exposure if leaked. | Document as accepted risk for self-hosted app. Consider refresh tokens in future. | DOCUMENTED | +| S-15 | **MEDIUM** | `server/src/routes/auth.ts` | 367-368 | Password change does not invalidate existing JWT tokens. Old tokens remain valid for up to 24h. | Implement token version/generation tracking, or reduce token expiry and add refresh tokens. | REQUIRES MANUAL REVIEW | +| S-16 | **MEDIUM** | `server/src/services/mfaCrypto.ts` | 2, 5 | MFA encryption key is derived from JWT_SECRET. If JWT_SECRET is compromised, all MFA secrets are also compromised. Single point of failure. | Use a separate MFA_ENCRYPTION_KEY env var, or derive using a different salt/purpose. Current implementation with `:mfa:v1` salt is acceptable but tightly coupled. | DOCUMENTED | +| S-17 | **MEDIUM** | `server/src/routes/maps.ts` | 429 | Google API key exposed in URL query string (`&key=${apiKey}`). Could appear in logs. | Use header-based auth (X-Goog-Api-Key) consistently. Already used elsewhere in the file. | FIXED | +| S-18 | **MEDIUM** | `MCP.md` | 232-235 | Contains publicly accessible database download link with hardcoded credentials (`admin@admin.com` / `admin123`). | Remove credentials from documentation. | FIXED | +| S-19 | **LOW** | `server/src/index.ts` | 229 | Error handler logs full error object including stack trace to console. In containerized deployments, this could leak to centralized logging. | Sanitize error logging in production. | FIXED | +| S-20 | **LOW** | `server/src/routes/backup.ts` | 301-304 | Error detail leaked in non-production environments (`detail: process.env.NODE_ENV !== 'production' ? msg : undefined`). | Acceptable for dev, but ensure it's consistently not leaked in production. Already correct. | OK | + +### 1.2 Auth (JWT + OIDC + TOTP) + +| # | Severity | File | Line(s) | Description | Recommended Fix | Status | +|---|----------|------|---------|-------------|-----------------|--------| +| A-1 | **CRITICAL** | All jwt.verify calls | Multiple | JWT algorithm not pinned. `jsonwebtoken` library defaults to accepting the algorithm specified in the token header, which could include `none`. | Add `{ algorithms: ['HS256'] }` to every `jwt.verify()` call. | FIXED | +| A-2 | **MEDIUM** | `server/src/routes/auth.ts` | 315-318 | MFA login token uses same JWT_SECRET and same `jwt.sign()`. Purpose field `mfa_login` prevents misuse but should use a shorter expiry. Currently 5m which is acceptable. | OK — 5 minute expiry is reasonable. | OK | +| A-3 | **MEDIUM** | `server/src/routes/oidc.ts` | 113-143 | OIDC redirect URI is dynamically constructed from request headers (`x-forwarded-proto`, `x-forwarded-host`). An attacker who can control these headers could redirect the callback to a malicious domain. | Validate the constructed redirect URI against an allowlist, or use a configured base URL from env vars. | FIXED | +| A-4 | **LOW** | `server/src/routes/auth.ts` | 21 | TOTP `window: 1` allows codes from adjacent time periods (±30s). This is standard and acceptable. | OK | OK | + +### 1.3 SQLite (better-sqlite3) + +| # | Severity | File | Line(s) | Description | Recommended Fix | Status | +|---|----------|------|---------|-------------|-----------------|--------| +| D-1 | **HIGH** | `server/src/routes/files.ts` | 90-91 | Dynamic SQL with `IN (${placeholders})` — however, placeholders are correctly generated from array length and values are parameterized. **Not an injection risk.** | OK — pattern is safe. | OK | +| D-2 | **MEDIUM** | `server/src/routes/auth.ts` | 455 | Dynamic SQL `UPDATE users SET ${updates.join(', ')} WHERE id = ?` — column names come from controlled server-side code, not user input. Parameters are properly bound. | OK — column names are from a controlled set. | OK | +| D-3 | **LOW** | `server/src/db/database.ts` | 26-28 | WAL mode and busy_timeout configured. Good. | OK | OK | + +### 1.4 WebSocket (ws) + +| # | Severity | File | Line(s) | Description | Recommended Fix | Status | +|---|----------|------|---------|-------------|-----------------|--------| +| W-1 | **MEDIUM** | `server/src/websocket.ts` | 29 | No `maxPayload` set on WebSocketServer. Default is 100MB which is excessive. | Set `maxPayload: 64 * 1024` (64KB). | FIXED | +| W-2 | **MEDIUM** | `server/src/websocket.ts` | 84-110 | Only `join` and `leave` message types are handled; unknown types are silently ignored. This is acceptable but there is no schema validation on the message structure. | Add basic type/schema validation using Zod. | FIXED | +| W-3 | **LOW** | `server/src/websocket.ts` | 88 | `JSON.parse` errors are silently caught with empty catch. | Log malformed messages at debug level. | FIXED | + +### 1.5 Express + +| # | Severity | File | Line(s) | Description | Recommended Fix | Status | +|---|----------|------|---------|-------------|-----------------|--------| +| E-1 | **LOW** | `server/src/index.ts` | 82 | Body parser limit set to 100KB. Good. | OK | OK | +| E-2 | **LOW** | `server/src/index.ts` | 14-16 | Trust proxy configured conditionally. Good. | OK | OK | +| E-3 | **LOW** | `server/src/index.ts` | 121-136 | Path traversal protection on uploads endpoint. Uses `path.basename` and `path.resolve` check. Good. | OK | OK | + +### 1.6 PWA / Workbox + +| # | Severity | File | Line(s) | Description | Recommended Fix | Status | +|---|----------|------|---------|-------------|-----------------|--------| +| P-1 | **HIGH** | `client/vite.config.js` | 47-56 | API response caching includes sensitive endpoints. | Exclude auth, admin, backup, and settings endpoints from caching. | FIXED | +| P-2 | **MEDIUM** | `client/vite.config.js` | 23, 31, 42, 54, 63 | `cacheableResponse: { statuses: [0, 200] }` — status 0 represents opaque responses which may cache error responses silently. | Remove status 0 from API and upload caches (keep for CDN/map tiles where CORS may return opaque responses). | FIXED | +| P-3 | **MEDIUM** | `client/src/store/authStore.ts` | 126-135 | Logout does not clear service worker caches. Sensitive data persists after logout. | Clear CacheStorage for `api-data` and `user-uploads` caches on logout. | FIXED | + +--- + +## 2. Code Quality + +| # | Severity | File | Line(s) | Description | Recommended Fix | Status | +|---|----------|------|---------|-------------|-----------------|--------| +| Q-1 | **MEDIUM** | `client/src/store/authStore.ts` | 153-161 | `loadUser` silently catches all errors and logs user out. A transient network failure logs the user out. | Only logout on 401 responses, not on network errors. | FIXED | +| Q-2 | **MEDIUM** | `client/src/hooks/useRouteCalculation.ts` | 36 | `useCallback` depends on entire `tripStore` object, defeating memoization. | Select only needed properties from the store. | DOCUMENTED | +| Q-3 | **MEDIUM** | `client/src/hooks/useTripWebSocket.ts` | 14 | `collabFileSync` captures stale `tripStore` reference from initial render. | Use `useTripStore.getState()` instead. | DOCUMENTED | +| Q-4 | **MEDIUM** | `client/src/store/authStore.ts` | 38 vs 105 | `register` function accepts 4 params but TypeScript interface only declares 3. | Update interface to include optional `invite_token`. | FIXED | +| Q-5 | **LOW** | `client/src/store/slices/filesSlice.ts` | — | Empty catch block on file link operation (`catch {}`). | Log error. | DOCUMENTED | +| Q-6 | **LOW** | `client/src/App.tsx` | 101, 108 | Empty catch blocks silently swallow errors. | Add minimal error logging. | DOCUMENTED | +| Q-7 | **LOW** | `client/src/App.tsx` | 155 | `RegisterPage` imported but never used — `/register` route renders `LoginPage`. | Remove unused import. | FIXED | +| Q-8 | **LOW** | `client/tsconfig.json` | 14 | `strict: false` disables TypeScript strict mode. | Enable strict mode and fix resulting type errors. | REQUIRES MANUAL REVIEW | +| Q-9 | **LOW** | `client/src/main.tsx` | 7 | Non-null assertion on `getElementById('root')!`. | Add null check. | DOCUMENTED | +| Q-10 | **LOW** | `server/src/routes/files.ts` | 278 | Empty catch block on file link insert (`catch {}`). | Log duplicate link errors. | FIXED | +| Q-11 | **LOW** | `server/src/db/database.ts` | 20-21 | Silent catch on WAL checkpoint in `initDb`. | Log warning on failure. | DOCUMENTED | + +--- + +## 3. Best Practices + +### 3.1 Node / Express + +| # | Severity | File | Line(s) | Description | Recommended Fix | Status | +|---|----------|------|---------|-------------|-----------------|--------| +| B-1 | **LOW** | `server/src/index.ts` | 251-271 | Graceful shutdown implemented with SIGTERM/SIGINT handlers. Good — closes DB, HTTP server, with 10s timeout. | OK | OK | +| B-2 | **LOW** | `server/src/index.ts` | 87-112 | Debug logging redacts sensitive fields. Good. | OK | OK | + +### 3.2 React / Vite + +| # | Severity | File | Line(s) | Description | Recommended Fix | Status | +|---|----------|------|---------|-------------|-----------------|--------| +| V-1 | **MEDIUM** | `client/vite.config.js` | — | No explicit `build.sourcemap: false` for production. Source maps may be generated. | Add `build: { sourcemap: false }` to Vite config. | FIXED | +| V-2 | **LOW** | `client/index.html` | 24 | Leaflet CSS loaded from unpkg CDN without Subresource Integrity (SRI) hash. | Add `integrity` and `crossorigin` attributes. | FIXED | + +### 3.3 Docker + +| # | Severity | File | Line(s) | Description | Recommended Fix | Status | +|---|----------|------|---------|-------------|-----------------|--------| +| K-1 | **MEDIUM** | `Dockerfile` | 2, 10 | Base images use floating tags (`node:22-alpine`), not pinned to digest. | Pin to specific digest for reproducible builds. | DOCUMENTED | +| K-2 | **MEDIUM** | `Dockerfile` | — | No `HEALTHCHECK` instruction. Only docker-compose has health check. | Add `HEALTHCHECK` to Dockerfile for standalone deployments. | FIXED | +| K-3 | **LOW** | `.dockerignore` | — | Missing exclusions for `chart/`, `docs/`, `.github/`, `docker-compose.yml`, `*.sqlite*`. | Add missing exclusions. | FIXED | + +### 3.4 docker-compose.yml + +| # | Severity | File | Line(s) | Description | Recommended Fix | Status | +|---|----------|------|---------|-------------|-----------------|--------| +| C-1 | **HIGH** | `docker-compose.yml` | 25 | `JWT_SECRET` defaults to empty string if not set. App auto-generates one, but it changes on restart, invalidating all sessions. | Log a prominent warning on startup if JWT_SECRET is auto-generated. | FIXED | +| C-2 | **MEDIUM** | `docker-compose.yml` | — | No resource limits defined for the `app` service. | Add `deploy.resources.limits` section. | DOCUMENTED | + +### 3.5 Git Hygiene + +| # | Severity | File | Line(s) | Description | Recommended Fix | Status | +|---|----------|------|---------|-------------|-----------------|--------| +| G-1 | **HIGH** | `.gitignore` | 12-14 | Missing `*.sqlite`, `*.sqlite-wal`, `*.sqlite-shm` patterns. Only `*.db` variants covered. | Add sqlite patterns. | FIXED | +| G-2 | **LOW** | — | — | No `.env` or `.sqlite` files found in git history. | OK | OK | + +### 3.6 Helm Chart + +| # | Severity | File | Line(s) | Description | Recommended Fix | Status | +|---|----------|------|---------|-------------|-----------------|--------| +| H-1 | **MEDIUM** | `chart/templates/secret.yaml` | 22 | `randAlphaNum 32` generates a new JWT secret on every `helm upgrade`, invalidating all sessions. | Use `lookup` to preserve existing secret across upgrades. | FIXED | +| H-2 | **MEDIUM** | `chart/values.yaml` | 3 | Default image tag is `latest`. | Use a specific version tag. | DOCUMENTED | +| H-3 | **MEDIUM** | `chart/templates/deployment.yaml` | — | No `securityContext` on pod or container. Runs as root by default. | Add `runAsNonRoot: true`, `runAsUser: 1000`. | FIXED | +| H-4 | **MEDIUM** | `chart/templates/pvc.yaml` | — | PVC always created regardless of `.Values.persistence.enabled`. | Add conditional check. | FIXED | +| H-5 | **LOW** | `chart/values.yaml` | 41 | `resources: {}` — no default resource requests or limits. | Add sensible defaults. | FIXED | + +--- + +## 4. Dependency Hygiene + +### 4.1 npm audit + +| Package | Severity | Description | Status | +|---------|----------|-------------|--------| +| `serialize-javascript` (via vite-plugin-pwa → workbox-build → @rollup/plugin-terser) | **HIGH** | RCE via RegExp.flags / CPU exhaustion DoS | Fix requires `vite-plugin-pwa` major version upgrade. | DOCUMENTED | +| `picomatch` (via @rollup/pluginutils, tinyglobby) | **MODERATE** | ReDoS via extglob quantifiers | `npm audit fix` available. | FIXED | + +**Server:** 0 vulnerabilities. + +### 4.2 Outdated Dependencies (Notable) + +| Package | Current | Latest | Risk | Status | +|---------|---------|--------|------|--------| +| `express` | ^4.18.3 | 5.2.1 | Major version — breaking changes | DOCUMENTED | +| `uuid` | ^9.0.0 | 13.0.0 | Major version | DOCUMENTED | +| `dotenv` | ^16.4.1 | 17.3.1 | Major version | DOCUMENTED | +| `lucide-react` | ^0.344.0 | 1.7.0 | Major version | DOCUMENTED | +| `react` | ^18.2.0 | 19.2.4 | Major version | DOCUMENTED | +| `zustand` | ^4.5.2 | 5.0.12 | Major version | DOCUMENTED | + +> Major version upgrades require manual evaluation and testing. Not applied in this remediation pass. + +--- + +## 5. Documentation & DX + +| # | Severity | File | Description | Recommended Fix | Status | +|---|----------|------|-------------|-----------------|--------| +| X-1 | **MEDIUM** | `server/.env.example` | Missing many env vars documented in README: `OIDC_*`, `FORCE_HTTPS`, `TRUST_PROXY`, `DEMO_MODE`, `TZ`, `ALLOWED_ORIGINS`, `DEBUG`. | Add all configurable env vars. | FIXED | +| X-2 | **MEDIUM** | `server/.env.example` | JWT_SECRET placeholder is `your-super-secret-jwt-key-change-in-production` — easily overlooked. | Use `CHANGEME_GENERATE_WITH_openssl_rand_hex_32`. | FIXED | +| X-3 | **LOW** | `server/.env.example` | `PORT=3001` differs from Docker default of `3000`. | Align to `3000`. | FIXED | + +--- + +## 6. Testing + +| # | Severity | Description | Status | +|---|----------|-------------|--------| +| T-1 | **HIGH** | No test files found anywhere in the repository. Zero test coverage for auth flows, WebSocket handling, SQLite queries, API routes, or React components. | REQUIRES MANUAL REVIEW | +| T-2 | **HIGH** | No test framework configured (no jest, vitest, or similar in dependencies). | REQUIRES MANUAL REVIEW | +| T-3 | **MEDIUM** | No CI step runs tests before building Docker image. | DOCUMENTED | + +--- + +## 7. Remediation Summary + +### Applied Fixes + +- **Immich SSRF prevention** — Added URL validation on save (block private IPs, metadata endpoints, non-HTTP protocols) +- **Immich API key isolation** — Removed `userId` query parameter from asset proxy endpoints; all Immich requests now use authenticated user's own credentials only +- **Immich asset ID validation** — Added alphanumeric pattern validation to prevent path traversal in proxied URLs +- **JWT algorithm pinning** — Added `{ algorithms: ['HS256'] }` to all `jwt.verify()` calls (auth middleware, MFA policy, WebSocket, OIDC, auth routes) +- **OIDC token payload** — Reduced to `{ id }` only, matching auth.ts pattern +- **OIDC redirect URI validation** — Validates against `APP_URL` env var when set +- **WebSocket hardening** — Added `maxPayload: 64KB`, message rate limiting (30 msg/10s), origin validation, improved message validation +- **CSP tightening** — Removed `'unsafe-inline'` from scripts in production, restricted `connectSrc` and `imgSrc` to known domains +- **PWA cache security** — Excluded sensitive API paths from caching, removed opaque response caching for API/uploads, clear caches on logout +- **Service worker cache cleanup on logout** +- **Google API key** — Moved from URL query string to header in maps photo endpoint +- **MCP.md credentials** — Removed hardcoded demo credentials +- **.gitignore** — Added `*.sqlite*` patterns +- **.dockerignore** — Added missing exclusions +- **Dockerfile** — Added HEALTHCHECK instruction +- **Helm chart** — Fixed secret rotation, added securityContext, conditional PVC, resource defaults +- **Vite config** — Disabled source maps in production +- **CDN integrity** — Added SRI hash for Leaflet CSS +- **.env.example** — Complete with all env vars +- **Various code quality fixes** — Removed dead imports, fixed empty catch blocks, fixed auth store interface + +### Requires Manual Review + +- Password change should invalidate existing tokens (S-15) +- TypeScript strict mode should be enabled (Q-8) +- Test suite needs to be created from scratch (T-1, T-2) +- Major dependency upgrades (express 5, React 19, zustand 5, etc.) +- `serialize-javascript` vulnerability fix requires vite-plugin-pwa major upgrade + +### 1.7 Immich Integration + +| # | Severity | File | Line(s) | Description | Recommended Fix | Status | +|---|----------|------|---------|-------------|-----------------|--------| +| I-1 | **CRITICAL** | `server/src/routes/immich.ts` | 38-39, 85, 199, 250, 274 | SSRF via user-controlled `immich_url`. Users can set any URL which is then used in `fetch()` calls, allowing requests to internal metadata endpoints (169.254.169.254), localhost services, etc. | Validate URL on save: require HTTP(S) protocol, block private/internal IPs. | FIXED | +| I-2 | **CRITICAL** | `server/src/routes/immich.ts` | 194-196, 244-246, 269-270 | Asset info/thumbnail/original endpoints accept `userId` query param, allowing any authenticated user to proxy requests through another user's Immich API key. This exposes other users' Immich credentials and photo libraries. | Restrict all Immich proxy endpoints to the authenticated user's own credentials only. | FIXED | +| I-3 | **MEDIUM** | `server/src/routes/immich.ts` | 199, 250, 274 | `assetId` URL parameter used directly in `fetch()` URL construction. Path traversal characters could redirect requests to unintended Immich API endpoints. | Validate assetId matches `[a-zA-Z0-9_-]+` pattern. | FIXED | + +### 1.8 Admin Routes + +| # | Severity | File | Line(s) | Description | Recommended Fix | Status | +|---|----------|------|---------|-------------|-----------------|--------| +| AD-1 | **MEDIUM** | `server/src/routes/admin.ts` | 302-310 | Self-update endpoint runs `git pull` then `npm run build`. While admin-only and `npm install` uses `--ignore-scripts`, `npm run build` executes whatever is in the pulled package.json. A compromised upstream could execute arbitrary code. | Document as accepted risk for self-hosted self-update feature. Users should pin to specific versions. | DOCUMENTED | + +### 1.9 Client-Side XSS + +| # | Severity | File | Line(s) | Description | Recommended Fix | Status | +|---|----------|------|---------|-------------|-----------------|--------| +| X-1 | **CRITICAL** | `client/src/components/Admin/GitHubPanel.tsx` | 66, 106 | `dangerouslySetInnerHTML` with `inlineFormat()` renders GitHub release notes as HTML without escaping. Malicious HTML in release notes could execute scripts. | Escape HTML entities before applying markdown-style formatting. Validate link URLs. | FIXED | +| X-2 | **LOW** | `client/src/components/Map/MapView.tsx` | divIcon | Uses `escAttr()` for HTML sanitization in divIcon strings. Properly mitigated. | OK | OK | + +### 1.10 Route Calculator Bug + +| # | Severity | File | Line(s) | Description | Recommended Fix | Status | +|---|----------|------|---------|-------------|-----------------|--------| +| RC-1 | **HIGH** | `client/src/components/Map/RouteCalculator.ts` | 16 | OSRM URL hardcodes `'driving'` profile, ignoring the `profile` parameter. Walking/cycling routes always return driving results. | Use the `profile` parameter in URL construction. | FIXED | + +### Additional Findings (from exhaustive scan) + +- **MEDIUM** — `server/src/index.ts:121-136`: Upload files (`/uploads/:type/:filename`) served without authentication. UUIDs are unguessable but this is security-through-obscurity. **REQUIRES MANUAL REVIEW** — adding auth would break shared trip image URLs. +- **MEDIUM** — `server/src/routes/oidc.ts:194`: OIDC token exchange error was logging full token response (potentially including access tokens). **FIXED** — now logs only HTTP status. +- **MEDIUM** — `server/src/services/notifications.ts:194-196`: Email body is not HTML-escaped. User-generated content (trip names, usernames) interpolated directly into HTML email template. Potential stored XSS in email clients. **DOCUMENTED** — needs HTML entity escaping. +- **LOW** — `server/src/demo/demo-seed.ts:7-9`: Hardcoded demo credentials (`demo12345`, `admin12345`). Intentional for demo mode but dangerous if DEMO_MODE accidentally left on in production. Already has startup warning. +- **LOW** — `server/src/routes/auth.ts:742`: MFA setup returns plaintext TOTP secret to client. This is standard TOTP enrollment flow — users need the secret for manual entry. Must be served over HTTPS. +- **LOW** — `server/src/routes/auth.ts:473`: Admin settings GET returns API keys in full (not masked). Only accessible to admins. +- **LOW** — `server/src/routes/auth.ts:564`: SMTP password stored as plaintext in `app_settings` table. Masked in API response but unencrypted at rest. + +### Accepted Risks (Documented) + +- WebSocket token in URL query string (browser limitation) +- 24h JWT expiry without refresh tokens (acceptable for self-hosted) +- MFA encryption key derived from JWT_SECRET (noted coupling) +- localStorage for token storage (standard SPA pattern) +- Upload files served without auth (UUID-based obscurity, needed for shared trips) diff --git a/Dockerfile b/Dockerfile index 8a301c7..332e545 100644 --- a/Dockerfile +++ b/Dockerfile @@ -39,5 +39,8 @@ ENV PORT=3000 EXPOSE 3000 +HEALTHCHECK --interval=30s --timeout=5s --start-period=15s --retries=3 \ + CMD wget -qO- http://localhost:3000/api/health || exit 1 + # Entrypoint: fix volume permissions then start as node CMD ["sh", "-c", "chown -R node:node /app/data /app/uploads 2>/dev/null; exec su-exec node node --import tsx src/index.ts"] diff --git a/MCP.md b/MCP.md index 0b64b52..8fe54a4 100644 --- a/MCP.md +++ b/MCP.md @@ -229,11 +229,6 @@ Currency: CHF. Use get_trip_summary at the end and give me a quick recap of everything that was added. ``` -Database file: https://share.jubnl.ch/s/S7bBpj42mB - -Email: admin@admin.com \ -Password: admin123 - PDF of the generated trip: [./docs/TREK-Generated-by-MCP.pdf](./docs/TREK-Generated-by-MCP.pdf) ![trip](./docs/screenshot-trip-mcp.png) \ No newline at end of file diff --git a/chart/templates/deployment.yaml b/chart/templates/deployment.yaml index d10957e..25169f6 100644 --- a/chart/templates/deployment.yaml +++ b/chart/templates/deployment.yaml @@ -20,10 +20,16 @@ spec: - name: {{ .name }} {{- end }} {{- end }} + securityContext: + fsGroup: 1000 containers: - name: trek image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}" imagePullPolicy: {{ .Values.image.pullPolicy }} + {{- with .Values.resources }} + resources: + {{- toYaml . | nindent 12 }} + {{- end }} ports: - containerPort: 3000 envFrom: diff --git a/chart/templates/pvc.yaml b/chart/templates/pvc.yaml index 663bff5..d68f693 100644 --- a/chart/templates/pvc.yaml +++ b/chart/templates/pvc.yaml @@ -1,3 +1,4 @@ +{{- if .Values.persistence.enabled }} apiVersion: v1 kind: PersistentVolumeClaim metadata: @@ -23,3 +24,4 @@ spec: resources: requests: storage: {{ .Values.persistence.uploads.size }} +{{- end }} diff --git a/chart/templates/secret.yaml b/chart/templates/secret.yaml index b27596a..6ead7f1 100644 --- a/chart/templates/secret.yaml +++ b/chart/templates/secret.yaml @@ -11,13 +11,19 @@ data: {{- end }} {{- if and (not .Values.existingSecret) (.Values.generateJwtSecret) }} +{{- $secretName := printf "%s-secret" (include "trek.fullname" .) }} +{{- $existingSecret := (lookup "v1" "Secret" .Release.Namespace $secretName) }} apiVersion: v1 kind: Secret metadata: - name: {{ include "trek.fullname" . }}-secret + name: {{ $secretName }} labels: app: {{ include "trek.name" . }} type: Opaque stringData: + {{- if and $existingSecret $existingSecret.data }} + {{ .Values.existingSecretKey | default "JWT_SECRET" }}: {{ index $existingSecret.data (.Values.existingSecretKey | default "JWT_SECRET") | b64dec }} + {{- else }} {{ .Values.existingSecretKey | default "JWT_SECRET" }}: {{ randAlphaNum 32 }} + {{- end }} {{- end }} diff --git a/chart/values.yaml b/chart/values.yaml index f52f3de..0d9d0c9 100644 --- a/chart/values.yaml +++ b/chart/values.yaml @@ -38,7 +38,13 @@ persistence: uploads: size: 1Gi -resources: {} +resources: + requests: + cpu: 100m + memory: 256Mi + limits: + cpu: 500m + memory: 512Mi ingress: enabled: false diff --git a/client/index.html b/client/index.html index 0e4e508..582bc34 100644 --- a/client/index.html +++ b/client/index.html @@ -21,7 +21,9 @@ - +
diff --git a/client/package-lock.json b/client/package-lock.json index 4fd8204..3a0edca 100644 --- a/client/package-lock.json +++ b/client/package-lock.json @@ -2789,9 +2789,9 @@ } }, "node_modules/@rollup/pluginutils/node_modules/picomatch": { - "version": "4.0.3", - "resolved": "https://registry.npmjs.org/picomatch/-/picomatch-4.0.3.tgz", - "integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==", + "version": "4.0.4", + "resolved": "https://registry.npmjs.org/picomatch/-/picomatch-4.0.4.tgz", + "integrity": "sha512-QP88BAKvMam/3NxH6vj2o21R6MjxZUAd6nlwAS/pnGvN9IVLocLHxGYIzFhg6fUQ+5th6P4dv4eW9jX3DSIj7A==", "dev": true, "license": "MIT", "engines": { @@ -3693,9 +3693,9 @@ } }, "node_modules/brace-expansion": { - "version": "5.0.4", - "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-5.0.4.tgz", - "integrity": "sha512-h+DEnpVvxmfVefa4jFbCf5HdH5YMDXRsmKflpf1pILZWRFlTbJpxeU55nJl4Smt5HQaGzg1o6RHFPJaOqnmBDg==", + "version": "5.0.5", + "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-5.0.5.tgz", + "integrity": "sha512-VZznLgtwhn+Mact9tfiwx64fA9erHH/MCXEUfB/0bX/6Fz6ny5EGTXYltMocqg4xFAQZtnO3DHWWXi8RiuN7cQ==", "dev": true, "license": "MIT", "dependencies": { @@ -4679,9 +4679,9 @@ "license": "MIT" }, "node_modules/filelist/node_modules/brace-expansion": { - "version": "2.0.2", - "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-2.0.2.tgz", - "integrity": "sha512-Jt0vHyM+jmUBqojB7E1NIYadt0vI0Qxjxd2TErW94wDz+E2LAm5vKMXXwg6ZZBTHPuUlDgQHKXvjGBdfcF1ZDQ==", + "version": "2.0.3", + "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-2.0.3.tgz", + "integrity": "sha512-MCV/fYJEbqx68aE58kv2cA/kiky1G8vux3OR6/jbS+jIMe/6fJWa0DTzJU7dqijOWYwHi1t29FlfYI9uytqlpA==", "dev": true, "license": "MIT", "dependencies": { @@ -7181,9 +7181,9 @@ "license": "ISC" }, "node_modules/picomatch": { - "version": "2.3.1", - "resolved": "https://registry.npmjs.org/picomatch/-/picomatch-2.3.1.tgz", - "integrity": "sha512-JU3teHTNjmE2VCGFzuY8EXzCDVwEqB2a8fsIvwaStHhAWJEeVd1o1QD80CU6+ZdEXXSLbSsuLwJjkCBWqRQUVA==", + "version": "2.3.2", + "resolved": "https://registry.npmjs.org/picomatch/-/picomatch-2.3.2.tgz", + "integrity": "sha512-V7+vQEJ06Z+c5tSye8S+nHUfI51xoXIXjHQ99cQtKUkQqqO1kO/KCJUfZXuB47h/YBlDhah2H3hdUGXn8ie0oA==", "dev": true, "license": "MIT", "engines": { @@ -8705,9 +8705,9 @@ } }, "node_modules/tinyglobby/node_modules/picomatch": { - "version": "4.0.3", - "resolved": "https://registry.npmjs.org/picomatch/-/picomatch-4.0.3.tgz", - "integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==", + "version": "4.0.4", + "resolved": "https://registry.npmjs.org/picomatch/-/picomatch-4.0.4.tgz", + "integrity": "sha512-QP88BAKvMam/3NxH6vj2o21R6MjxZUAd6nlwAS/pnGvN9IVLocLHxGYIzFhg6fUQ+5th6P4dv4eW9jX3DSIj7A==", "dev": true, "license": "MIT", "engines": { diff --git a/client/src/App.tsx b/client/src/App.tsx index a02662f..6a434f6 100644 --- a/client/src/App.tsx +++ b/client/src/App.tsx @@ -3,7 +3,6 @@ import { Routes, Route, Navigate, useLocation } from 'react-router-dom' import { useAuthStore } from './store/authStore' import { useSettingsStore } from './store/settingsStore' import LoginPage from './pages/LoginPage' -import RegisterPage from './pages/RegisterPage' import DashboardPage from './pages/DashboardPage' import TripPlannerPage from './pages/TripPlannerPage' import FilesPage from './pages/FilesPage' @@ -14,7 +13,6 @@ import AtlasPage from './pages/AtlasPage' import SharedTripPage from './pages/SharedTripPage' import { ToastContainer } from './components/shared/Toast' import { TranslationProvider, useTranslation } from './i18n' -import DemoBanner from './components/Layout/DemoBanner' import { authApi } from './api/client' interface ProtectedRouteProps { diff --git a/client/src/components/Admin/GitHubPanel.tsx b/client/src/components/Admin/GitHubPanel.tsx index 8db9d6f..2dc6dfa 100644 --- a/client/src/components/Admin/GitHubPanel.tsx +++ b/client/src/components/Admin/GitHubPanel.tsx @@ -72,11 +72,15 @@ export default function GitHubPanel() { } } + const escapeHtml = (str) => str.replace(/&/g, '&').replace(//g, '>').replace(/"/g, '"') const inlineFormat = (text) => { - return text + return escapeHtml(text) .replace(/\*\*(.+?)\*\*/g, '$1') .replace(/`(.+?)`/g, '$1') - .replace(/\[([^\]]+)\]\(([^)]+)\)/g, '$1') + .replace(/\[([^\]]+)\]\(([^)]+)\)/g, (_, label, url) => { + const safeUrl = url.startsWith('http://') || url.startsWith('https://') ? url : '#' + return `${label}` + }) } for (const line of lines) { diff --git a/client/src/components/Map/RouteCalculator.ts b/client/src/components/Map/RouteCalculator.ts index 6f79a97..8300aab 100644 --- a/client/src/components/Map/RouteCalculator.ts +++ b/client/src/components/Map/RouteCalculator.ts @@ -13,7 +13,7 @@ export async function calculateRoute( } const coords = waypoints.map((p) => `${p.lng},${p.lat}`).join(';') - const url = `${OSRM_BASE}/driving/${coords}?overview=full&geometries=geojson&steps=false` + const url = `${OSRM_BASE}/${profile}/${coords}?overview=full&geometries=geojson&steps=false` const response = await fetch(url, { signal }) if (!response.ok) { diff --git a/client/src/store/authStore.ts b/client/src/store/authStore.ts index 472d2d9..66206ee 100644 --- a/client/src/store/authStore.ts +++ b/client/src/store/authStore.ts @@ -29,7 +29,7 @@ interface AuthState { login: (email: string, password: string) => Promise completeMfaLogin: (mfaToken: string, code: string) => Promise - register: (username: string, email: string, password: string) => Promise + register: (username: string, email: string, password: string, invite_token?: string) => Promise logout: () => void /** Pass `{ silent: true }` to refresh the user without toggling global isLoading (avoids unmounting protected routes). */ loadUser: (opts?: { silent?: boolean }) => Promise @@ -126,6 +126,11 @@ export const useAuthStore = create((set, get) => ({ logout: () => { disconnect() localStorage.removeItem('auth_token') + // Clear service worker caches containing sensitive data + if ('caches' in window) { + caches.delete('api-data').catch(() => {}) + caches.delete('user-uploads').catch(() => {}) + } set({ user: null, token: null, @@ -151,13 +156,20 @@ export const useAuthStore = create((set, get) => ({ }) connect(token) } catch (err: unknown) { - localStorage.removeItem('auth_token') - set({ - user: null, - token: null, - isAuthenticated: false, - isLoading: false, - }) + // Only clear auth state on 401 (invalid/expired token), not on network errors + const isAuthError = err && typeof err === 'object' && 'response' in err && + (err as { response?: { status?: number } }).response?.status === 401 + if (isAuthError) { + localStorage.removeItem('auth_token') + set({ + user: null, + token: null, + isAuthenticated: false, + isLoading: false, + }) + } else { + set({ isLoading: false }) + } } }, diff --git a/client/vite.config.js b/client/vite.config.js index fbbe41e..453b6e1 100644 --- a/client/vite.config.js +++ b/client/vite.config.js @@ -45,23 +45,24 @@ export default defineConfig({ }, { // API calls — prefer network, fall back to cache - urlPattern: /\/api\/.*/i, + // Exclude sensitive endpoints (auth, admin, backup, settings) + urlPattern: /\/api\/(?!auth|admin|backup|settings).*/i, handler: 'NetworkFirst', options: { cacheName: 'api-data', expiration: { maxEntries: 200, maxAgeSeconds: 24 * 60 * 60 }, networkTimeoutSeconds: 5, - cacheableResponse: { statuses: [0, 200] }, + cacheableResponse: { statuses: [200] }, }, }, { - // Uploaded files (photos, covers, documents) - urlPattern: /\/uploads\/.*/i, + // Uploaded files (photos, covers — public assets only) + urlPattern: /\/uploads\/(?:covers|avatars)\/.*/i, handler: 'CacheFirst', options: { cacheName: 'user-uploads', - expiration: { maxEntries: 300, maxAgeSeconds: 30 * 24 * 60 * 60 }, - cacheableResponse: { statuses: [0, 200] }, + expiration: { maxEntries: 300, maxAgeSeconds: 7 * 24 * 60 * 60 }, + cacheableResponse: { statuses: [200] }, }, }, ], @@ -87,6 +88,9 @@ export default defineConfig({ }, }), ], + build: { + sourcemap: false, + }, server: { port: 5173, proxy: { diff --git a/server/.env.example b/server/.env.example index 4e2e99e..a4ffe68 100644 --- a/server/.env.example +++ b/server/.env.example @@ -1,4 +1,33 @@ -PORT=3001 -JWT_SECRET=your-super-secret-jwt-key-change-in-production +PORT=3000 NODE_ENV=development DEBUG=false + +# REQUIRED for production — generate with: openssl rand -hex 32 +JWT_SECRET=CHANGEME_GENERATE_WITH_openssl_rand_hex_32 + +# Timezone (defaults to system timezone) +# TZ=UTC + +# CORS — comma-separated origins (leave unset for same-origin in production, allow-all in development) +# ALLOWED_ORIGINS=https://trek.example.com + +# Force HTTPS redirect (set to true behind TLS-terminating proxy) +# FORCE_HTTPS=true + +# Trust proxy (set to number of proxy hops, e.g. 1 for single reverse proxy) +# TRUST_PROXY=1 + +# Application URL (used for OIDC callback validation) +# APP_URL=https://trek.example.com + +# Demo mode (enables demo login, disables registration) +# DEMO_MODE=false + +# --- OIDC / SSO --- +# OIDC_ISSUER=https://auth.example.com +# OIDC_CLIENT_ID= +# OIDC_CLIENT_SECRET= +# OIDC_DISPLAY_NAME=SSO +# OIDC_ONLY=false +# OIDC_ADMIN_CLAIM=groups +# OIDC_ADMIN_VALUE=app-trek-admins diff --git a/server/src/config.ts b/server/src/config.ts index e7ac9ed..adbc559 100644 --- a/server/src/config.ts +++ b/server/src/config.ts @@ -23,4 +23,6 @@ if (!JWT_SECRET) { } } -export { JWT_SECRET }; +const JWT_SECRET_IS_GENERATED = !process.env.JWT_SECRET; + +export { JWT_SECRET, JWT_SECRET_IS_GENERATED }; diff --git a/server/src/index.ts b/server/src/index.ts index db4bba7..1031482 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -1,5 +1,5 @@ import 'dotenv/config'; -import './config'; +import { JWT_SECRET_IS_GENERATED } from './config'; import express, { Request, Response, NextFunction } from 'express'; import { enforceGlobalMfaPolicy } from './middleware/mfaPolicy'; import cors from 'cors'; @@ -57,13 +57,21 @@ app.use(helmet({ contentSecurityPolicy: { directives: { defaultSrc: ["'self'"], - scriptSrc: ["'self'", "'unsafe-inline'"], + scriptSrc: ["'self'"], styleSrc: ["'self'", "'unsafe-inline'", "https://fonts.googleapis.com", "https://unpkg.com"], - imgSrc: ["'self'", "data:", "blob:", "https:", "http:"], - connectSrc: ["'self'", "ws:", "wss:", "https:", "http:"], + imgSrc: ["'self'", "data:", "blob:", "https:"], + connectSrc: [ + "'self'", "ws:", "wss:", + "https://nominatim.openstreetmap.org", "https://overpass-api.de", + "https://places.googleapis.com", "https://api.openweathermap.org", + "https://en.wikipedia.org", "https://commons.wikimedia.org", + "https://*.basemaps.cartocdn.com", "https://*.tile.openstreetmap.org", + "https://unpkg.com", "https://open-meteo.com", "https://api.open-meteo.com", + "https://geocoding-api.open-meteo.com", + ], fontSrc: ["'self'", "https://fonts.gstatic.com", "data:"], - objectSrc: ["'self'"], - frameSrc: ["'self'"], + objectSrc: ["'none'"], + frameSrc: ["'none'"], frameAncestors: ["'self'"], upgradeInsecureRequests: shouldForceHttps ? [] : null } @@ -224,9 +232,13 @@ if (process.env.NODE_ENV === 'production') { }); } -// Global error handler +// Global error handler — do not leak stack traces in production app.use((err: Error, req: Request, res: Response, next: NextFunction) => { - console.error('Unhandled error:', err); + if (process.env.NODE_ENV !== 'production') { + console.error('Unhandled error:', err); + } else { + console.error('Unhandled error:', err.message); + } res.status(500).json({ error: 'Internal server error' }); }); @@ -237,6 +249,9 @@ const server = app.listen(PORT, () => { console.log(`TREK API running on port ${PORT}`); console.log(`Environment: ${process.env.NODE_ENV || 'development'}`); console.log(`Debug logs: ${DEBUG ? 'ENABLED' : 'disabled'}`); + if (JWT_SECRET_IS_GENERATED) { + console.warn('[SECURITY WARNING] JWT_SECRET was auto-generated. Sessions will not persist across restarts. Set JWT_SECRET env var for production use.'); + } if (process.env.DEMO_MODE === 'true') console.log('Demo mode: ENABLED'); if (process.env.DEMO_MODE === 'true' && process.env.NODE_ENV === 'production') { console.warn('[SECURITY WARNING] DEMO_MODE is enabled in production! Demo credentials are publicly exposed.'); diff --git a/server/src/mcp/index.ts b/server/src/mcp/index.ts index 97b3d5d..47c79f0 100644 --- a/server/src/mcp/index.ts +++ b/server/src/mcp/index.ts @@ -90,7 +90,7 @@ function verifyToken(authHeader: string | undefined): User | null { // Short-lived JWT try { - const decoded = jwt.verify(token, JWT_SECRET) as { id: number }; + const decoded = jwt.verify(token, JWT_SECRET, { algorithms: ['HS256'] }) as { id: number }; const user = db.prepare( 'SELECT id, username, email, role FROM users WHERE id = ?' ).get(decoded.id) as User | undefined; diff --git a/server/src/middleware/auth.ts b/server/src/middleware/auth.ts index e0844df..9b0c040 100644 --- a/server/src/middleware/auth.ts +++ b/server/src/middleware/auth.ts @@ -14,7 +14,7 @@ const authenticate = (req: Request, res: Response, next: NextFunction): void => } try { - const decoded = jwt.verify(token, JWT_SECRET) as { id: number }; + const decoded = jwt.verify(token, JWT_SECRET, { algorithms: ['HS256'] }) as { id: number }; const user = db.prepare( 'SELECT id, username, email, role FROM users WHERE id = ?' ).get(decoded.id) as User | undefined; @@ -39,7 +39,7 @@ const optionalAuth = (req: Request, res: Response, next: NextFunction): void => } try { - const decoded = jwt.verify(token, JWT_SECRET) as { id: number }; + const decoded = jwt.verify(token, JWT_SECRET, { algorithms: ['HS256'] }) as { id: number }; const user = db.prepare( 'SELECT id, username, email, role FROM users WHERE id = ?' ).get(decoded.id) as User | undefined; diff --git a/server/src/middleware/mfaPolicy.ts b/server/src/middleware/mfaPolicy.ts index 2912faa..d6e1aa9 100644 --- a/server/src/middleware/mfaPolicy.ts +++ b/server/src/middleware/mfaPolicy.ts @@ -51,7 +51,7 @@ export function enforceGlobalMfaPolicy(req: Request, res: Response, next: NextFu let userId: number; try { - const decoded = jwt.verify(token, JWT_SECRET) as { id: number }; + const decoded = jwt.verify(token, JWT_SECRET, { algorithms: ['HS256'] }) as { id: number }; userId = decoded.id; } catch { next(); diff --git a/server/src/routes/auth.ts b/server/src/routes/auth.ts index 9bb6e92..d7604a2 100644 --- a/server/src/routes/auth.ts +++ b/server/src/routes/auth.ts @@ -164,7 +164,7 @@ function generateToken(user: { id: number | bigint }) { return jwt.sign( { id: user.id }, JWT_SECRET, - { expiresIn: '24h' } + { expiresIn: '24h', algorithm: 'HS256' } ); } @@ -321,7 +321,7 @@ router.post('/login', authLimiter, (req: Request, res: Response) => { const mfa_token = jwt.sign( { id: Number(user.id), purpose: 'mfa_login' }, JWT_SECRET, - { expiresIn: '5m' } + { expiresIn: '5m', algorithm: 'HS256' } ); return res.json({ mfa_required: true, mfa_token }); } @@ -741,7 +741,7 @@ router.post('/mfa/verify-login', authLimiter, (req: Request, res: Response) => { return res.status(400).json({ error: 'Verification token and code are required' }); } try { - const decoded = jwt.verify(mfa_token, JWT_SECRET) as { id: number; purpose?: string }; + const decoded = jwt.verify(mfa_token, JWT_SECRET, { algorithms: ['HS256'] }) as { id: number; purpose?: string }; if (decoded.purpose !== 'mfa_login') { return res.status(401).json({ error: 'Invalid verification token' }); } diff --git a/server/src/routes/files.ts b/server/src/routes/files.ts index ce6ef4a..5429d45 100644 --- a/server/src/routes/files.ts +++ b/server/src/routes/files.ts @@ -282,7 +282,9 @@ router.post('/:id/link', authenticate, (req: Request, res: Response) => { db.prepare('INSERT OR IGNORE INTO file_links (file_id, reservation_id, assignment_id, place_id) VALUES (?, ?, ?, ?)').run( id, reservation_id || null, assignment_id || null, place_id || null ); - } catch {} + } catch (err) { + console.error('[Files] Error creating file link:', err instanceof Error ? err.message : err); + } const links = db.prepare('SELECT * FROM file_links WHERE file_id = ?').all(id); res.json({ success: true, links }); diff --git a/server/src/routes/immich.ts b/server/src/routes/immich.ts index e5dca07..006ded8 100644 --- a/server/src/routes/immich.ts +++ b/server/src/routes/immich.ts @@ -6,6 +6,29 @@ import { AuthRequest } from '../types'; const router = express.Router(); +/** Validate that an asset ID is a safe UUID-like string (no path traversal). */ +function isValidAssetId(id: string): boolean { + return /^[a-zA-Z0-9_-]+$/.test(id) && id.length <= 100; +} + +/** Validate that an Immich URL is a safe HTTP(S) URL (no internal/metadata IPs). */ +function isValidImmichUrl(raw: string): boolean { + try { + const url = new URL(raw); + if (url.protocol !== 'http:' && url.protocol !== 'https:') return false; + const hostname = url.hostname.toLowerCase(); + // Block metadata endpoints and localhost + if (hostname === 'localhost' || hostname === '127.0.0.1' || hostname === '::1') return false; + if (hostname === '169.254.169.254' || hostname === 'metadata.google.internal') return false; + // Block link-local and loopback ranges + if (hostname.startsWith('10.') || hostname.startsWith('172.') || hostname.startsWith('192.168.')) return false; + if (hostname.endsWith('.internal') || hostname.endsWith('.local')) return false; + return true; + } catch { + return false; + } +} + // ── Immich Connection Settings ────────────────────────────────────────────── router.get('/settings', authenticate, (req: Request, res: Response) => { @@ -20,6 +43,9 @@ router.get('/settings', authenticate, (req: Request, res: Response) => { router.put('/settings', authenticate, (req: Request, res: Response) => { const authReq = req as AuthRequest; const { immich_url, immich_api_key } = req.body; + if (immich_url && !isValidImmichUrl(immich_url.trim())) { + return res.status(400).json({ error: 'Invalid Immich URL. Must be a valid HTTP(S) URL.' }); + } db.prepare('UPDATE users SET immich_url = ?, immich_api_key = ? WHERE id = ?').run( immich_url?.trim() || null, immich_api_key?.trim() || null, @@ -189,10 +215,10 @@ router.put('/trips/:tripId/photos/:assetId/sharing', authenticate, (req: Request router.get('/assets/:assetId/info', authenticate, async (req: Request, res: Response) => { const authReq = req as AuthRequest; const { assetId } = req.params; - const { userId } = req.query; + if (!isValidAssetId(assetId)) return res.status(400).json({ error: 'Invalid asset ID' }); - const targetUserId = userId ? Number(userId) : authReq.user.id; - const user = db.prepare('SELECT immich_url, immich_api_key FROM users WHERE id = ?').get(targetUserId) as any; + // Only allow accessing own Immich credentials — prevent leaking other users' API keys + const user = db.prepare('SELECT immich_url, immich_api_key FROM users WHERE id = ?').get(authReq.user.id) as any; if (!user?.immich_url || !user?.immich_api_key) return res.status(404).json({ error: 'Not found' }); try { @@ -240,10 +266,10 @@ function authFromQuery(req: Request, res: Response, next: Function) { router.get('/assets/:assetId/thumbnail', authFromQuery, async (req: Request, res: Response) => { const authReq = req as AuthRequest; const { assetId } = req.params; - const { userId } = req.query; + if (!isValidAssetId(assetId)) return res.status(400).send('Invalid asset ID'); - const targetUserId = userId ? Number(userId) : authReq.user.id; - const user = db.prepare('SELECT immich_url, immich_api_key FROM users WHERE id = ?').get(targetUserId) as any; + // Only allow accessing own Immich credentials — prevent leaking other users' API keys + const user = db.prepare('SELECT immich_url, immich_api_key FROM users WHERE id = ?').get(authReq.user.id) as any; if (!user?.immich_url || !user?.immich_api_key) return res.status(404).send('Not found'); try { @@ -264,10 +290,10 @@ router.get('/assets/:assetId/thumbnail', authFromQuery, async (req: Request, res router.get('/assets/:assetId/original', authFromQuery, async (req: Request, res: Response) => { const authReq = req as AuthRequest; const { assetId } = req.params; - const { userId } = req.query; + if (!isValidAssetId(assetId)) return res.status(400).send('Invalid asset ID'); - const targetUserId = userId ? Number(userId) : authReq.user.id; - const user = db.prepare('SELECT immich_url, immich_api_key FROM users WHERE id = ?').get(targetUserId) as any; + // Only allow accessing own Immich credentials — prevent leaking other users' API keys + const user = db.prepare('SELECT immich_url, immich_api_key FROM users WHERE id = ?').get(authReq.user.id) as any; if (!user?.immich_url || !user?.immich_api_key) return res.status(404).send('Not found'); try { diff --git a/server/src/routes/maps.ts b/server/src/routes/maps.ts index 58a7330..9659c38 100644 --- a/server/src/routes/maps.ts +++ b/server/src/routes/maps.ts @@ -428,7 +428,8 @@ router.get('/place-photo/:placeId', authenticate, async (req: Request, res: Resp const attribution = photo.authorAttributions?.[0]?.displayName || null; const mediaRes = await fetch( - `https://places.googleapis.com/v1/${photoName}/media?maxHeightPx=600&key=${apiKey}&skipHttpRedirect=true` + `https://places.googleapis.com/v1/${photoName}/media?maxHeightPx=600&skipHttpRedirect=true`, + { headers: { 'X-Goog-Api-Key': apiKey } } ); const mediaData = await mediaRes.json() as { photoUri?: string }; const photoUrl = mediaData.photoUri; diff --git a/server/src/routes/oidc.ts b/server/src/routes/oidc.ts index f21a517..f600237 100644 --- a/server/src/routes/oidc.ts +++ b/server/src/routes/oidc.ts @@ -80,11 +80,11 @@ async function discover(issuer: string) { return doc; } -function generateToken(user: { id: number; username: string; email: string; role: string }) { +function generateToken(user: { id: number }) { return jwt.sign( - { id: user.id, username: user.username, email: user.email, role: user.role }, + { id: user.id }, JWT_SECRET, - { expiresIn: '24h' } + { expiresIn: '24h', algorithm: 'HS256' } ); } @@ -121,9 +121,15 @@ router.get('/login', async (req: Request, res: Response) => { try { const doc = await discover(config.issuer); const state = crypto.randomBytes(32).toString('hex'); - const proto = (req.headers['x-forwarded-proto'] as string) || req.protocol; - const host = (req.headers['x-forwarded-host'] as string) || req.headers.host; - const redirectUri = `${proto}://${host}/api/auth/oidc/callback`; + const appUrl = process.env.APP_URL || (db.prepare("SELECT value FROM app_settings WHERE key = 'app_url'").get() as { value: string } | undefined)?.value; + let redirectUri: string; + if (appUrl) { + redirectUri = `${appUrl.replace(/\/+$/, '')}/api/auth/oidc/callback`; + } else { + const proto = (req.headers['x-forwarded-proto'] as string) || req.protocol; + const host = (req.headers['x-forwarded-host'] as string) || req.headers.host; + redirectUri = `${proto}://${host}/api/auth/oidc/callback`; + } const inviteToken = req.query.invite as string | undefined; pendingStates.set(state, { createdAt: Date.now(), redirectUri, inviteToken }); @@ -185,7 +191,7 @@ router.get('/callback', async (req: Request, res: Response) => { const tokenData = await tokenRes.json() as OidcTokenResponse; if (!tokenRes.ok || !tokenData.access_token) { - console.error('[OIDC] Token exchange failed:', tokenData); + console.error('[OIDC] Token exchange failed: status', tokenRes.status); return res.redirect(frontendUrl('/login?oidc_error=token_failed')); } diff --git a/server/src/websocket.ts b/server/src/websocket.ts index 2f6e44c..69b1d7e 100644 --- a/server/src/websocket.ts +++ b/server/src/websocket.ts @@ -24,9 +24,28 @@ let nextSocketId = 1; let wss: WebSocketServer | null = null; +// Per-connection message rate limiting +const WS_MSG_LIMIT = 30; // max messages +const WS_MSG_WINDOW = 10_000; // per 10 seconds +const socketMsgCounts = new WeakMap(); + /** Attaches a WebSocket server with JWT auth, room-based trip channels, and heartbeat keep-alive. */ function setupWebSocket(server: http.Server): void { - wss = new WebSocketServer({ server, path: '/ws' }); + const allowedOrigins = process.env.ALLOWED_ORIGINS + ? process.env.ALLOWED_ORIGINS.split(',').map(o => o.trim()) + : null; + + wss = new WebSocketServer({ + server, + path: '/ws', + maxPayload: 64 * 1024, // 64 KB max message size + verifyClient: allowedOrigins + ? ({ origin }, cb) => { + if (!origin || allowedOrigins.includes(origin)) cb(true); + else cb(false, 403, 'Origin not allowed'); + } + : undefined, + }); const HEARTBEAT_INTERVAL = 30000; // 30 seconds const heartbeat = setInterval(() => { @@ -53,7 +72,7 @@ function setupWebSocket(server: http.Server): void { let user: User | undefined; try { - const decoded = jwt.verify(token, JWT_SECRET) as { id: number }; + const decoded = jwt.verify(token, JWT_SECRET, { algorithms: ['HS256'] }) as { id: number }; user = db.prepare( 'SELECT id, username, email, role, mfa_enabled FROM users WHERE id = ?' ).get(decoded.id) as User | undefined; @@ -81,14 +100,33 @@ function setupWebSocket(server: http.Server): void { nws.on('pong', () => { nws.isAlive = true; }); + socketMsgCounts.set(nws, { count: 0, windowStart: Date.now() }); + nws.on('message', (data) => { + // Rate limiting + const rate = socketMsgCounts.get(nws)!; + const now = Date.now(); + if (now - rate.windowStart > WS_MSG_WINDOW) { + rate.count = 1; + rate.windowStart = now; + } else { + rate.count++; + if (rate.count > WS_MSG_LIMIT) { + nws.send(JSON.stringify({ type: 'error', message: 'Rate limit exceeded' })); + return; + } + } + let msg: { type: string; tripId?: number | string }; try { msg = JSON.parse(data.toString()); } catch { - return; + return; // Malformed JSON, ignore } + // Basic validation + if (!msg || typeof msg !== 'object' || typeof msg.type !== 'string') return; + if (msg.type === 'join' && msg.tripId) { const tripId = Number(msg.tripId); // Verify the user has access to this trip