From 9e5fd5bcaecdfd34df00e54d6e6a185b96cd9fd1 Mon Sep 17 00:00:00 2001 From: Daniel Pupius Date: Sun, 15 Mar 2026 15:41:24 +0000 Subject: [PATCH] Add code review findings as todos Security and architecture review of current codebase. 11 findings: - 3 P1 (XSS, hardcoded creds, unbounded memory growth) - 4 P2 (SSE protocol, broker deadlock, NetworkMap architecture, CORS) - 4 P3 (security headers, error leakage, dead code, binary payload) Co-Authored-By: Claude Sonnet 4.6 --- compound-engineering.local.md | 22 +++++++ ...01-pending-p1-xss-infowindow-node-names.md | 66 +++++++++++++++++++ ...2-pending-p1-hardcoded-mqtt-credentials.md | 54 +++++++++++++++ ...ng-p1-processedpackets-unbounded-growth.md | 64 ++++++++++++++++++ ...pending-p2-sse-http-error-after-headers.md | 51 ++++++++++++++ ...005-pending-p2-broker-shutdown-deadlock.md | 58 ++++++++++++++++ ...ding-p2-networkmap-functions-redeclared.md | 60 +++++++++++++++++ todos/007-pending-p2-wildcard-cors-no-auth.md | 56 ++++++++++++++++ ...008-pending-p2-unbounded-binary-payload.md | 51 ++++++++++++++ ...009-pending-p3-no-http-security-headers.md | 46 +++++++++++++ ...pending-p3-decode-error-leaks-internals.md | 45 +++++++++++++ ...-pending-p3-dead-code-mustparseduration.md | 34 ++++++++++ 12 files changed, 607 insertions(+) create mode 100644 compound-engineering.local.md create mode 100644 todos/001-pending-p1-xss-infowindow-node-names.md create mode 100644 todos/002-pending-p1-hardcoded-mqtt-credentials.md create mode 100644 todos/003-pending-p1-processedpackets-unbounded-growth.md create mode 100644 todos/004-pending-p2-sse-http-error-after-headers.md create mode 100644 todos/005-pending-p2-broker-shutdown-deadlock.md create mode 100644 todos/006-pending-p2-networkmap-functions-redeclared.md create mode 100644 todos/007-pending-p2-wildcard-cors-no-auth.md create mode 100644 todos/008-pending-p2-unbounded-binary-payload.md create mode 100644 todos/009-pending-p3-no-http-security-headers.md create mode 100644 todos/010-pending-p3-decode-error-leaks-internals.md create mode 100644 todos/011-pending-p3-dead-code-mustparseduration.md diff --git a/compound-engineering.local.md b/compound-engineering.local.md new file mode 100644 index 0000000..39f49c6 --- /dev/null +++ b/compound-engineering.local.md @@ -0,0 +1,22 @@ +--- +review_agents: + - compound-engineering:review:performance-oracle + - compound-engineering:review:architecture-strategist + - compound-engineering:review:security-sentinel + - compound-engineering:review:code-simplicity-reviewer +--- + +# Meshstream Review Context + +This is a Go + React/TypeScript application that: +- Subscribes to Meshtastic MQTT topics and decodes protobuf packets +- Streams decoded packets to browser clients via SSE +- Frontend uses React 19, Redux Toolkit, TanStack Router, and Google Maps API +- All state is in-memory (no database) +- Backend: Go with paho MQTT client and prefab web framework + +Key architectural patterns: +- Backend circular buffer (200 packets default) for new client catchup +- Frontend Redux aggregator slice processes all packet types +- Google Maps AdvancedMarkerElement for node visualization +- Protobuf definitions in proto/ generate Go and TypeScript types via make gen-proto diff --git a/todos/001-pending-p1-xss-infowindow-node-names.md b/todos/001-pending-p1-xss-infowindow-node-names.md new file mode 100644 index 0000000..94ad658 --- /dev/null +++ b/todos/001-pending-p1-xss-infowindow-node-names.md @@ -0,0 +1,66 @@ +--- +status: pending +priority: p1 +issue_id: "001" +tags: [code-review, security, xss, frontend] +dependencies: [] +--- + +# XSS via Node Names in Google Maps InfoWindow + +## Problem Statement + +`showInfoWindow` in `NetworkMap.tsx` builds an HTML string using `nodeName` (sourced from `node.longName || node.shortName`, which comes from MQTT packets) and passes it to `infoWindowRef.current.setContent(infoContent)`. Google Maps `InfoWindow.setContent` renders its argument as live HTML. A Meshtastic device with a `longName` of `` will execute arbitrary JavaScript in every viewer's browser. + +Attack path: malicious device broadcasts NodeInfo on public MQTT → Go decoder stores it faithfully → SSE streams to browser → Redux stores verbatim → map renders with no sanitization. + +## Findings + +- **File:** `web/src/components/dashboard/NetworkMap.tsx`, lines 431-451 +- `nodeName` is interpolated raw into an HTML template literal +- `infoWindowRef.current.setContent(infoContent)` renders the HTML directly +- All SVG marker templates on lines 344 and 390 use the same unsafe pattern (lower risk currently since values are internally computed, but should be normalized) +- No sanitization at any layer (decoder, SSE, Redux, component) + +## Proposed Solutions + +### Option A: DOM construction with .textContent (Recommended) +Replace the HTML template literal with `document.createElement` tree. Use `.textContent` for all data-derived values. +- **Pros:** Completely eliminates XSS, no extra dependency, idiomatic +- **Cons:** More verbose than template literal +- **Effort:** Small +- **Risk:** Low + +### Option B: Pass Element directly to setContent +Build a `div` element using DOM APIs and pass the element object to `setContent` (which accepts `Element | string`). +- **Pros:** Clean, no string HTML at all +- **Cons:** Slightly more DOM manipulation code +- **Effort:** Small +- **Risk:** Low + +### Option C: Add DOMPurify sanitization +Run `nodeName` through `DOMPurify.sanitize()` before interpolation. +- **Pros:** Minimal code change +- **Cons:** Adds a dependency; still uses HTML string pattern; sanitizers can be bypassed +- **Effort:** Small +- **Risk:** Medium (sanitizer bypass potential) + +## Recommended Action + +_Use Option A — DOM construction. It eliminates the vulnerability class entirely rather than mitigating it._ + +## Technical Details + +- **Affected files:** `web/src/components/dashboard/NetworkMap.tsx` +- **Function:** `showInfoWindow` (line ~412) +- **Data origin:** `data.nodeInfo.longName` / `data.nodeInfo.shortName` from MQTT protobuf + +## Acceptance Criteria + +- [ ] `showInfoWindow` constructs InfoWindow content using DOM APIs, not HTML strings +- [ ] No user-supplied string data is interpolated into HTML template literals in NetworkMap.tsx +- [ ] A node with `longName = ""` renders safely as literal text in the info window + +## Work Log + +- 2026-03-15: Identified by security-sentinel and architecture-strategist review agents diff --git a/todos/002-pending-p1-hardcoded-mqtt-credentials.md b/todos/002-pending-p1-hardcoded-mqtt-credentials.md new file mode 100644 index 0000000..6f7c8f8 --- /dev/null +++ b/todos/002-pending-p1-hardcoded-mqtt-credentials.md @@ -0,0 +1,54 @@ +--- +status: pending +priority: p1 +issue_id: "002" +tags: [code-review, security, credentials, configuration] +dependencies: [] +--- + +# Hardcoded MQTT Credentials in Source Code + +## Problem Statement + +`meshdev` / `large4cats` are committed as default flag values in `main.go`. Anyone reading the source or forking the repo has these credentials. Operators who deploy without setting environment variables silently use these known-public credentials, believing they have a private configuration. + +## Findings + +- **File:** `main.go`, lines 73-74 +- `getEnv("MQTT_USERNAME", "meshdev")` and `getEnv("MQTT_PASSWORD", "large4cats")` expose credentials in source +- Credentials appear in `--help` output and any build artifact +- While these are community broker credentials (not a private service), they are real auth credentials enabling broker access + +## Proposed Solutions + +### Option A: Empty defaults + startup validation (Recommended) +Use `""` as the default for both fields. Add a startup check that logs a warning (or hard-fails if a `--require-auth` flag is set) when credentials are empty. +- **Pros:** Forces operators to be explicit; no credentials in source +- **Effort:** Small +- **Risk:** Low (may break zero-config dev setups) + +### Option B: Document-only approach +Keep defaults but add a prominent comment and README warning. +- **Pros:** Zero code change +- **Cons:** Credentials still in source; doesn't protect production deployments +- **Effort:** Trivial +- **Risk:** Medium + +## Recommended Action + +_Option A. At minimum remove the password default; a username default of "meshdev" is less sensitive but should also go._ + +## Technical Details + +- **Affected file:** `main.go`, lines 73-74 +- **Related:** `MQTT_USERNAME` / `MQTT_PASSWORD` env vars already supported — just remove the hardcoded fallback values + +## Acceptance Criteria + +- [ ] No credential values appear in source code as string literals +- [ ] Server starts cleanly with credentials provided via env vars +- [ ] Server logs a clear message when credentials are not configured + +## Work Log + +- 2026-03-15: Identified by security-sentinel review agent diff --git a/todos/003-pending-p1-processedpackets-unbounded-growth.md b/todos/003-pending-p1-processedpackets-unbounded-growth.md new file mode 100644 index 0000000..449b6ff --- /dev/null +++ b/todos/003-pending-p1-processedpackets-unbounded-growth.md @@ -0,0 +1,64 @@ +--- +status: pending +priority: p1 +issue_id: "003" +tags: [code-review, performance, memory, frontend, redux] +dependencies: [] +--- + +# Unbounded Memory Growth in processedPackets and seenPackets + +## Problem Statement + +`aggregatorSlice.processedPackets` and `packetSlice.seenPackets` are `Record` maps that grow forever. Every processed packet adds an entry and nothing ever removes them. A session receiving modest Meshtastic traffic (~1 packet/sec) accumulates tens of thousands of keys over hours. Both maps serialize on every Redux state snapshot, are included in DevTools, and will be included in any future persistence layer. + +`processedPackets` is P1 because it's in the hot aggregator path queried on every packet; `seenPackets` (in packetSlice) has the same issue at P2 because it's less frequently queried. + +## Findings + +- **File:** `web/src/store/slices/aggregatorSlice.ts`, lines 76, 115 +- `processedPackets[packetKey] = true` written on every packet, never pruned +- No eviction path; `clearAggregatedData` resets it but requires explicit user action +- **File:** `web/src/store/slices/packetSlice.ts`, lines ~13, ~78-79 +- `seenPackets` same pattern; `packets` array is trimmed to 5000 but map is not + +## Proposed Solutions + +### Option A: Time-based eviction with timestamp map (Recommended) +Change `processedPackets: Record` to `processedPackets: Record` (value = timestamp). On each `processNewPacket` call, prune entries older than a TTL (e.g., 24h). Apply the same pattern to `seenPackets`. +- **Pros:** Consistent with topologySlice TTL pattern already in the spec; self-healing +- **Effort:** Small +- **Risk:** Low + +### Option B: Fixed-size LRU cap +Keep only the most recent N packet keys (e.g., 10000). When the cap is exceeded, drop the oldest. Use an insertion-ordered structure or a separate ordered array. +- **Pros:** Hard memory bound +- **Cons:** More complex; need to track insertion order +- **Effort:** Medium +- **Risk:** Low + +### Option C: Rely on clearAggregatedData +Document that operators should reload periodically; no code change. +- **Pros:** Zero effort +- **Cons:** Doesn't fix the problem; long-running sessions still leak +- **Risk:** High (ongoing) + +## Recommended Action + +_Option A — add timestamps to both maps and prune during packet processing. The topologySlice will use the same pattern, so this creates consistency._ + +## Technical Details + +- **Affected files:** `web/src/store/slices/aggregatorSlice.ts`, `web/src/store/slices/packetSlice.ts` +- Pattern: `Record` where value is unix timestamp; prune `now - value > TTL` at start of each dispatch + +## Acceptance Criteria + +- [ ] `processedPackets` entries older than 24h are pruned during packet processing +- [ ] `seenPackets` entries are similarly bounded +- [ ] Deduplication still works correctly after pruning (no duplicate packets in ~24h window) +- [ ] Memory usage stabilizes after hours of packet ingestion + +## Work Log + +- 2026-03-15: Identified by architecture-strategist review agent diff --git a/todos/004-pending-p2-sse-http-error-after-headers.md b/todos/004-pending-p2-sse-http-error-after-headers.md new file mode 100644 index 0000000..5c9113e --- /dev/null +++ b/todos/004-pending-p2-sse-http-error-after-headers.md @@ -0,0 +1,51 @@ +--- +status: pending +priority: p2 +issue_id: "004" +tags: [code-review, backend, sse, go, protocol] +dependencies: [] +--- + +# http.Error Called After SSE Headers Already Sent + +## Problem Statement + +In `server.go`, `http.Error()` is called in the client-disconnect, shutdown, and channel-closed cases after `w.WriteHeader(http.StatusOK)` and SSE headers have already been written. This produces `superfluous response.WriteHeader call` log noise and, more critically, writes HTTP error text into the SSE event stream body, corrupting the stream framing for any still-connected client. + +## Findings + +- **File:** `server/server.go`, lines 236-240, 245-249, 262-264 +- `w.WriteHeader(http.StatusOK)` + SSE headers written at line ~203 +- `http.Error()` calls in `<-notify`, `<-s.shutdown`, and `!ok` cases write to an already-open response body +- In the client-disconnect case this is harmless (connection is dead) +- In the `!ok` channel case the client may still be alive and receives garbled data + +## Proposed Solutions + +### Option A: Replace http.Error with plain return (Recommended) +In all three cases, simply `return` after logging. The SSE client handles reconnection automatically; no error response is needed on the already-open stream. +- **Effort:** Small +- **Risk:** Low + +### Option B: Flush a `data: error\n\n` SSE event before closing +Send a proper SSE event indicating shutdown, then return without calling `http.Error`. +- **Effort:** Small +- **Risk:** Low (slightly more client-visible information) + +## Recommended Action + +_Option A — just return. SSE clients reconnect automatically._ + +## Technical Details + +- **Affected file:** `server/server.go` + +## Acceptance Criteria + +- [ ] No `http.Error` calls after SSE headers have been written +- [ ] No `superfluous response.WriteHeader call` log warnings during normal client disconnect +- [ ] SSE stream body is never corrupted with HTTP error text + +## Work Log + +- 2026-03-15: Identified by architecture-strategist review agent diff --git a/todos/005-pending-p2-broker-shutdown-deadlock.md b/todos/005-pending-p2-broker-shutdown-deadlock.md new file mode 100644 index 0000000..66ebde4 --- /dev/null +++ b/todos/005-pending-p2-broker-shutdown-deadlock.md @@ -0,0 +1,58 @@ +--- +status: pending +priority: p2 +issue_id: "005" +tags: [code-review, backend, concurrency, go, mqtt] +dependencies: [] +--- + +# Broker dispatchLoop Self-Deadlock on Source Channel Close + +## Problem Statement + +When the MQTT source channel closes, `dispatchLoop` calls `b.Close()`. `Close` calls `b.wg.Wait()`. But `dispatchLoop` is running in the goroutine counted by `b.wg`, so `wg.Wait()` will never return — deadlock. The current shutdown order in `main.go` avoids this path, but the ordering dependency is implicit and undocumented. A future refactor inverting shutdown order would deadlock silently. + +## Findings + +- **File:** `mqtt/broker.go`, lines 193-199 +- `dispatchLoop` calls `b.Close()` when source channel closes +- `Close` calls `b.wg.Wait()`, waiting for `dispatchLoop` to finish +- `dispatchLoop` is blocked inside `Close` → deadlock +- Current `main.go` shutdown order (`broker.Close()` before `mqttClient.Disconnect()`) avoids triggering this, but the safety is fragile + +## Proposed Solutions + +### Option A: Send signal to Close rather than calling it directly (Recommended) +Have `dispatchLoop` close an internal `loopDone` channel rather than calling `b.Close()`. Have `b.Close()` check if `loopDone` is already closed to avoid double-work. +- **Effort:** Small-Medium +- **Risk:** Low + +### Option B: Use context cancellation +Pass a `context.Context` to `dispatchLoop`. When source closes, cancel the context. `Close` also cancels the context and waits for the loop to exit via `wg.Wait()`. +- **Effort:** Medium +- **Risk:** Low + +### Option C: Add comment documenting the ordering constraint +Document that `broker.Close()` must be called before `mqttClient.Disconnect()`. +- **Effort:** Trivial +- **Cons:** Fragile; future refactors will miss it +- **Risk:** Medium + +## Recommended Action + +_Option A or B. At minimum add a comment (Option C) as immediate mitigation, then refactor to remove the ordering dependency._ + +## Technical Details + +- **Affected file:** `mqtt/broker.go` +- **Related:** `main.go` shutdown sequence + +## Acceptance Criteria + +- [ ] Broker shuts down cleanly regardless of whether `broker.Close()` or MQTT disconnect is called first +- [ ] No goroutine leaks on shutdown +- [ ] Shutdown order is explicit and enforced, not accidental + +## Work Log + +- 2026-03-15: Identified by architecture-strategist review agent diff --git a/todos/006-pending-p2-networkmap-functions-redeclared.md b/todos/006-pending-p2-networkmap-functions-redeclared.md new file mode 100644 index 0000000..05ffa80 --- /dev/null +++ b/todos/006-pending-p2-networkmap-functions-redeclared.md @@ -0,0 +1,60 @@ +--- +status: pending +priority: p2 +issue_id: "006" +tags: [code-review, frontend, react, architecture, quality] +dependencies: [] +--- + +# NetworkMap.tsx Functions Redeclared in Component Body Every Render + +## Problem Statement + +`initializeMap`, `updateNodeMarkers`, `createMarker`, `updateMarker`, and `showInfoWindow` are declared inside the component function body without `useCallback`. They are redeclared on every render, cannot be properly listed in dependency arrays, and are the direct cause of the `/* eslint-disable react-hooks/exhaustive-deps */` at the top of the file. The planned topology polyline feature will make this worse if it follows the same pattern. + +## Findings + +- **File:** `web/src/components/dashboard/NetworkMap.tsx`, lines 261-455, line 1 (`eslint-disable`) +- `tryInitializeMap` useCallback (line 133) lists `updateNodeMarkers` and `initializeMap` in deps — but these change every render, defeating memoization +- The `eslint-disable` suppression acknowledges the problem but doesn't fix it +- Adding polyline management code to this pattern makes the component increasingly difficult to reason about + +## Proposed Solutions + +### Option A: Extract to module-level pure functions (Recommended) +Move functions that don't access component state/refs directly (e.g., `getMarkerIcon`, marker content builders) to module-level. Pass refs/state as parameters. +- **Pros:** Cleanest; enables proper useCallback dependency arrays; allows tree-shaking +- **Effort:** Medium +- **Risk:** Low + +### Option B: Wrap all in useCallback with correct deps +Convert all functions to useCallback with proper dependency lists. Remove the eslint-disable. +- **Pros:** Fixes the hook rules without restructuring +- **Cons:** Still keeps large functions in the component body +- **Effort:** Medium +- **Risk:** Low + +### Option C: Leave as-is, document for topology addition +Accept the pattern and apply it consistently for topology polylines. Remove the eslint-disable only when refactoring later. +- **Effort:** None now +- **Cons:** Technical debt grows; makes the topology feature harder to test +- **Risk:** Medium (ongoing) + +## Recommended Action + +_At minimum, do Option A for module-level pure helpers before adding topology polyline code. The topology feature addition is a natural trigger for this cleanup._ + +## Technical Details + +- **Affected file:** `web/src/components/dashboard/NetworkMap.tsx` +- This should be addressed before or alongside the topology polyline rendering implementation + +## Acceptance Criteria + +- [ ] The `/* eslint-disable react-hooks/exhaustive-deps */` comment is removed +- [ ] All `useCallback` hooks have accurate, complete dependency arrays +- [ ] Adding polyline management code does not require more eslint suppression + +## Work Log + +- 2026-03-15: Identified by architecture-strategist review agent diff --git a/todos/007-pending-p2-wildcard-cors-no-auth.md b/todos/007-pending-p2-wildcard-cors-no-auth.md new file mode 100644 index 0000000..abad95e --- /dev/null +++ b/todos/007-pending-p2-wildcard-cors-no-auth.md @@ -0,0 +1,56 @@ +--- +status: pending +priority: p2 +issue_id: "007" +tags: [code-review, security, cors, auth, backend] +dependencies: [] +--- + +# Wildcard CORS + No Authentication on SSE Endpoint + +## Problem Statement + +`/api/stream` sets `Access-Control-Allow-Origin: *`, meaning any JavaScript on any origin can subscribe to the full decoded packet feed. There is no authentication at any layer. The `/api/status` endpoint also exposes the MQTT server hostname and subscribed topic to any caller. + +## Findings + +- **File:** `server/server.go`, line ~182 +- `w.Header().Set("Access-Control-Allow-Origin", "*")` on the SSE endpoint +- No API key, session token, or any auth mechanism +- `/api/status` returns `mqttServer` and `mqttTopic` to unauthenticated callers + +## Proposed Solutions + +### Option A: Restrict CORS to same-origin or configured origin +Replace `*` with a configurable `MESHSTREAM_ALLOWED_ORIGIN` env var. Default to `""` (same-origin only). +- **Effort:** Small +- **Risk:** Low (may break cross-origin dev setups — document the env var) + +### Option B: Add optional static token auth +Add an optional `MESHSTREAM_API_TOKEN` env var. If set, require `Authorization: Bearer ` or `?token=` on all API requests. +- **Effort:** Small-Medium +- **Risk:** Low + +### Option C: No change for local-only deployments +Document that the server is intended for local/trusted network use only. +- **Effort:** Trivial +- **Cons:** Doesn't protect deployments that are inadvertently exposed +- **Risk:** Medium + +## Recommended Action + +_Option A at minimum (restrict CORS origin). Option B if public deployment is expected._ + +## Technical Details + +- **Affected file:** `server/server.go` + +## Acceptance Criteria + +- [ ] CORS origin is not `*` in default configuration +- [ ] CORS origin is configurable via environment variable +- [ ] Status endpoint does not expose internal MQTT details without auth (or is documented as intentionally public) + +## Work Log + +- 2026-03-15: Identified by security-sentinel review agent diff --git a/todos/008-pending-p2-unbounded-binary-payload.md b/todos/008-pending-p2-unbounded-binary-payload.md new file mode 100644 index 0000000..91c47e0 --- /dev/null +++ b/todos/008-pending-p2-unbounded-binary-payload.md @@ -0,0 +1,51 @@ +--- +status: pending +priority: p2 +issue_id: "008" +tags: [code-review, security, backend, memory, dos] +dependencies: [] +--- + +# Unbounded Binary Payload Forwarded to All SSE Clients + +## Problem Statement + +When decryption produces bytes that are neither valid protobuf nor ASCII, the raw decrypted bytes are stored verbatim in `BinaryData` and forwarded via SSE to all connected clients. There is no size limit on the encrypted payload or the binary blob. A malicious actor on the MQTT feed could craft large packets, causing memory amplification and excessive bandwidth to all SSE subscribers. + +## Findings + +- **File:** `decoder/decoder.go`, lines 316-318 +- Raw decrypted bytes stored in `BinaryData` field with no size cap +- Serialized by `protojson` and streamed to all SSE clients +- One large malicious packet → all connected clients receive it + +## Proposed Solutions + +### Option A: Add maximum payload size check before decryption (Recommended) +Reject encrypted payloads larger than a configurable limit (e.g., 4KB default). Return a `decodeError` without attempting decryption. +- **Effort:** Small +- **Risk:** Low + +### Option B: Drop binary data from SSE stream entirely +When `BinaryData` would be set, replace it with a size indicator only (e.g., `binaryDataSize: 1234`). Don't send the raw bytes to clients. +- **Effort:** Small +- **Risk:** Low (clients lose access to binary payloads they currently can't decode anyway) + +## Recommended Action + +_Both — add a size check before decryption AND drop binary blobs from SSE output._ + +## Technical Details + +- **Affected file:** `decoder/decoder.go`, lines 316-318 +- Reasonable limit: 1KB for most Meshtastic payloads; 4KB as a generous cap + +## Acceptance Criteria + +- [ ] Encrypted payloads larger than the limit are rejected with a `decodeError` +- [ ] Raw binary bytes are not forwarded to SSE clients +- [ ] Normal Meshtastic packets (all under ~256 bytes) are unaffected + +## Work Log + +- 2026-03-15: Identified by security-sentinel review agent diff --git a/todos/009-pending-p3-no-http-security-headers.md b/todos/009-pending-p3-no-http-security-headers.md new file mode 100644 index 0000000..213fadf --- /dev/null +++ b/todos/009-pending-p3-no-http-security-headers.md @@ -0,0 +1,46 @@ +--- +status: pending +priority: p3 +issue_id: "009" +tags: [code-review, security, http-headers, backend] +dependencies: [001] +--- + +# No HTTP Security Headers + +## Problem Statement + +The server sets no `Content-Security-Policy`, `X-Content-Type-Options`, `X-Frame-Options`, or other security headers. A CSP would meaningfully reduce XSS blast radius (finding 001) and constrain resource loading. + +## Findings + +- **File:** `server/server.go` +- No security middleware layer +- Google Maps API and Fonts load from external origins — a CSP needs to allow these specifically +- `prefab` framework's default header behavior is unknown + +## Proposed Solutions + +### Option A: Add security headers middleware to the prefab server +Inject headers on all responses: `X-Content-Type-Options: nosniff`, `X-Frame-Options: DENY`, `Referrer-Policy: strict-origin-when-cross-origin`, and a CSP allowing `maps.googleapis.com`, `fonts.googleapis.com`, `fonts.gstatic.com`. +- **Effort:** Small +- **Risk:** Low (may need tuning for Google Maps inline styles/scripts) + +## Recommended Action + +_Option A after fixing finding 001 (XSS). CSP is most effective when combined with fixing the actual XSS root cause._ + +## Technical Details + +- **Affected file:** `server/server.go` + +## Acceptance Criteria + +- [ ] All HTTP responses include `X-Content-Type-Options: nosniff` +- [ ] All HTTP responses include `X-Frame-Options: DENY` +- [ ] CSP header allows required Google Maps and Fonts origins +- [ ] Google Maps continues to function with the CSP applied + +## Work Log + +- 2026-03-15: Identified by security-sentinel review agent diff --git a/todos/010-pending-p3-decode-error-leaks-internals.md b/todos/010-pending-p3-decode-error-leaks-internals.md new file mode 100644 index 0000000..f576243 --- /dev/null +++ b/todos/010-pending-p3-decode-error-leaks-internals.md @@ -0,0 +1,45 @@ +--- +status: pending +priority: p3 +issue_id: "010" +tags: [code-review, security, information-disclosure, backend] +dependencies: [] +--- + +# Internal Error Details Leaked to Browser Clients via decodeError + +## Problem Statement + +Full Go error strings including channel names and parse error details are placed in the `decodeError` field and streamed to all browser clients. This leaks which channels are configured, internal protobuf error messages, and potentially version/path information. + +## Findings + +- **File:** `decoder/decoder.go`, lines 313-315 +- `"PRIVATE_CHANNEL: failed to parse decrypted data on unconfigured channel '%s': %v"` — channel name + Go error exposed +- All connected clients receive this in SSE stream +- Confirms channel key configuration to any observer + +## Proposed Solutions + +### Option A: Strip detail from decodeError before SSE; log server-side +Set `decodeError` to a short code (`"PRIVATE_CHANNEL"`, `"PARSE_ERROR"`, `"DECRYPT_FAILED"`) for the wire format. Log the full error server-side at debug level. +- **Effort:** Small +- **Risk:** Low (developers lose some frontend visibility — mitigated by server logs) + +## Recommended Action + +_Option A. Server logs preserve the detail for debugging; clients see only opaque codes._ + +## Technical Details + +- **Affected files:** `decoder/decoder.go`, all `data.DecodeError = fmt.Sprintf(...)` callsites + +## Acceptance Criteria + +- [ ] `decodeError` field sent to clients contains only short error codes, not Go error strings +- [ ] Full error detail is logged at the server +- [ ] Frontend can still distinguish error types from the short codes + +## Work Log + +- 2026-03-15: Identified by security-sentinel review agent diff --git a/todos/011-pending-p3-dead-code-mustparseduration.md b/todos/011-pending-p3-dead-code-mustparseduration.md new file mode 100644 index 0000000..0789f26 --- /dev/null +++ b/todos/011-pending-p3-dead-code-mustparseduration.md @@ -0,0 +1,34 @@ +--- +status: pending +priority: p3 +issue_id: "011" +tags: [code-review, quality, cleanup, backend] +dependencies: [] +--- + +# Dead Code: mustParseDuration in main.go + +## Problem Statement + +`mustParseDuration` is defined in `main.go` but never called. `durationFromEnv` is used everywhere instead. + +## Findings + +- **File:** `main.go`, lines 111-118 +- Function defined, zero call sites +- Leftover scaffolding from earlier iteration + +## Proposed Solutions + +### Option A: Delete the function +- **Effort:** Trivial +- **Risk:** None + +## Acceptance Criteria + +- [ ] `mustParseDuration` removed from `main.go` +- [ ] `make build` still passes + +## Work Log + +- 2026-03-15: Identified by architecture-strategist review agent