diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index 326db6c..0a7c8f3 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -3,6 +3,9 @@ name: Build & Push Docker Image on: push: branches: [main] + paths-ignore: + - 'docs/**' + - '**/*.md' workflow_dispatch: permissions: diff --git a/AUDIT_FINDINGS.md b/AUDIT_FINDINGS.md deleted file mode 100644 index 65fff0f..0000000 --- a/AUDIT_FINDINGS.md +++ /dev/null @@ -1,281 +0,0 @@ -# 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/README.md b/README.md index 6f601af..e5d768a 100644 --- a/README.md +++ b/README.md @@ -155,7 +155,7 @@ services: # - OIDC_ONLY=false # Set to true to disable local password auth entirely (SSO only) # - OIDC_ADMIN_CLAIM=groups # OIDC claim used to identify admin users # - OIDC_ADMIN_VALUE=app-trek-admins # Value of the OIDC claim that grants admin role - # - OIDC_SCOPE=openid email profile groups # Space-separated OIDC scopes to request (must include scopes for any claim used by OIDC_ADMIN_CLAIM) + # - OIDC_SCOPE=openid email profile # Fully overrides the default. Add extra scopes as needed (e.g. add groups if using OIDC_ADMIN_CLAIM) # - OIDC_DISCOVERY_URL= # Override the OIDC discovery endpoint for providers with non-standard paths (e.g. Authentik) # - DEMO_MODE=false # Enable demo mode (resets data hourly) # - ADMIN_EMAIL=admin@trek.local # Initial admin e-mail — only used on first boot when no users exist @@ -295,7 +295,7 @@ trek.yourdomain.com { | `OIDC_ONLY` | Disable local password auth entirely (first SSO login becomes admin) | `false` | | `OIDC_ADMIN_CLAIM` | OIDC claim used to identify admin users | — | | `OIDC_ADMIN_VALUE` | Value of the OIDC claim that grants admin role | — | -| `OIDC_SCOPE` | Space-separated OIDC scopes to request. Must include scopes for any claim used by `OIDC_ADMIN_CLAIM` (e.g. add `groups` for group-based admin mapping) | `openid email profile groups` | +| `OIDC_SCOPE` | Space-separated OIDC scopes to request. **Fully replaces** the default — always include `openid email profile` plus any extra scopes you need (e.g. add `groups` when using `OIDC_ADMIN_CLAIM`) | `openid email profile` | | `OIDC_DISCOVERY_URL` | Override the auto-constructed OIDC discovery endpoint. Useful for providers that expose it at a non-standard path (e.g. Authentik: `https://auth.example.com/application/o/trek/.well-known/openid-configuration`) | — | | **Initial Setup** | | | | `ADMIN_EMAIL` | Email for the first admin account created on initial boot. Must be set together with `ADMIN_PASSWORD`. If either is omitted a random password is generated and printed to the server log. Has no effect once any user exists. | `admin@trek.local` | diff --git a/client/package-lock.json b/client/package-lock.json index 7c3d867..cc0e285 100644 --- a/client/package-lock.json +++ b/client/package-lock.json @@ -1,12 +1,12 @@ { "name": "trek-client", - "version": "2.7.2", + "version": "2.8.3", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "trek-client", - "version": "2.7.2", + "version": "2.8.3", "dependencies": { "@react-pdf/renderer": "^4.3.2", "axios": "^1.6.7", diff --git a/client/package.json b/client/package.json index e9799b5..c7e04b0 100644 --- a/client/package.json +++ b/client/package.json @@ -1,6 +1,6 @@ { "name": "trek-client", - "version": "2.7.2", + "version": "2.8.3", "private": true, "type": "module", "scripts": { diff --git a/docker-compose.yml b/docker-compose.yml index 2d141a9..d6af6b6 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -34,7 +34,7 @@ services: # - OIDC_ONLY=false # Set true to disable local password auth entirely (SSO only) # - OIDC_ADMIN_CLAIM=groups # OIDC claim used to identify admin users # - OIDC_ADMIN_VALUE=app-trek-admins # Value of the OIDC claim that grants admin role -# - OIDC_SCOPE=openid email profile groups # Space-separated OIDC scopes to request (must include scopes for any claim used by OIDC_ADMIN_CLAIM) +# - OIDC_SCOPE=openid email profile # Fully overrides the default. Add extra scopes as needed (e.g. add groups if using OIDC_ADMIN_CLAIM) # - OIDC_DISCOVERY_URL= # Override the OIDC discovery endpoint for providers with non-standard paths (e.g. Authentik) # - ADMIN_EMAIL=admin@trek.local # Initial admin e-mail — only used on first boot when no users exist # - ADMIN_PASSWORD=changeme # Initial admin password — only used on first boot when no users exist diff --git a/server/.env.example b/server/.env.example index 2c606c5..9c70b49 100644 --- a/server/.env.example +++ b/server/.env.example @@ -24,7 +24,7 @@ OIDC_ONLY=true # Disable local password auth entirely (SSO only) OIDC_ADMIN_CLAIM=groups # OIDC claim used to identify admin users OIDC_ADMIN_VALUE=app-trek-admins # Value of the OIDC claim that grants admin role OIDC_DISCOVERY_URL= # Override the auto-constructed OIDC discovery endpoint. Useful for providers (e.g. Authentik) that expose it at a non-standard path. Example: https://auth.example.com/application/o/trek/.well-known/openid-configuration -OIDC_SCOPE=openid email profile groups # Space-separated OIDC scopes to request (must include scopes for any claim used by OIDC_ADMIN_CLAIM) +OIDC_SCOPE=openid email profile # Fully overrides the default. Add extra scopes as needed (e.g. add groups if using OIDC_ADMIN_CLAIM) DEMO_MODE=false # Demo mode - resets data hourly diff --git a/server/package-lock.json b/server/package-lock.json index 5068225..b389e04 100644 --- a/server/package-lock.json +++ b/server/package-lock.json @@ -1,12 +1,12 @@ { "name": "trek-server", - "version": "2.7.2", + "version": "2.8.3", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "trek-server", - "version": "2.7.2", + "version": "2.8.3", "dependencies": { "@modelcontextprotocol/sdk": "^1.28.0", "archiver": "^6.0.1", diff --git a/server/package.json b/server/package.json index 2361002..d195604 100644 --- a/server/package.json +++ b/server/package.json @@ -1,6 +1,6 @@ { "name": "trek-server", - "version": "2.7.2", + "version": "2.8.3", "main": "src/index.ts", "scripts": { "start": "node --import tsx src/index.ts", diff --git a/unraid-template.xml b/unraid-template.xml index e239673..5d2759d 100644 --- a/unraid-template.xml +++ b/unraid-template.xml @@ -52,7 +52,7 @@ false - openid email profile groups + openid email profile