mirror of
https://github.com/ipnet-mesh/meshcore-hub.git
synced 2026-03-28 17:42:56 +01:00
Merge pull request #142 from ipnet-mesh/fix/security-fixes
fix: harden security across auth, XSS, and proxy trust
This commit is contained in:
103
.agentmap.yaml
Normal file
103
.agentmap.yaml
Normal file
@@ -0,0 +1,103 @@
|
||||
# MeshCore Hub — codebase orientation map
|
||||
# See: https://github.com/anthropics/agentmap
|
||||
|
||||
meta:
|
||||
project: meshcore-hub
|
||||
version: 1
|
||||
updated: "2026-02-27"
|
||||
stack:
|
||||
- python 3.13
|
||||
- fastapi
|
||||
- sqlalchemy (async)
|
||||
- paho-mqtt
|
||||
- click
|
||||
- lit-html SPA
|
||||
- tailwind + daisyui
|
||||
- sqlite
|
||||
|
||||
tasks:
|
||||
install: "pip install -e '.[dev]'"
|
||||
test: "pytest"
|
||||
run: "meshcore-hub api --reload"
|
||||
lint: "pre-commit run --all-files"
|
||||
|
||||
tree:
|
||||
src/meshcore_hub/:
|
||||
__main__.py: "Click CLI entry point, registers subcommands"
|
||||
common/:
|
||||
config.py: "pydantic-settings, all env vars [config]"
|
||||
database.py: "async SQLAlchemy session management"
|
||||
mqtt.py: "MQTT client helpers"
|
||||
i18n.py: "translation loader, t() function"
|
||||
models/:
|
||||
base.py: "Base, UUIDMixin, TimestampMixin"
|
||||
node.py: null
|
||||
member.py: null
|
||||
advertisement.py: null
|
||||
message.py: null
|
||||
telemetry.py: null
|
||||
node_tag.py: null
|
||||
schemas/:
|
||||
events.py: "inbound MQTT event schemas"
|
||||
commands.py: "outbound command schemas"
|
||||
nodes.py: "API request/response schemas"
|
||||
members.py: null
|
||||
messages.py: null
|
||||
interface/:
|
||||
receiver.py: "reads device events, publishes to MQTT"
|
||||
sender.py: "subscribes MQTT commands, writes to device"
|
||||
device.py: "meshcore library wrapper"
|
||||
mock_device.py: "fake device for testing"
|
||||
collector/:
|
||||
subscriber.py: "MQTT subscriber, routes events to handlers"
|
||||
handlers/: "per-event-type DB persistence"
|
||||
cleanup.py: "data retention and node cleanup"
|
||||
webhook.py: "forward events to HTTP endpoints"
|
||||
tag_import.py: "seed node tags from YAML"
|
||||
member_import.py: "seed members from YAML"
|
||||
api/:
|
||||
app.py: "FastAPI app factory"
|
||||
auth.py: "API key authentication"
|
||||
dependencies.py: "DI for db session and auth"
|
||||
metrics.py: "Prometheus /metrics endpoint"
|
||||
routes/: "REST endpoints per resource"
|
||||
web/:
|
||||
app.py: "FastAPI app factory, SPA shell"
|
||||
pages.py: "custom markdown page loader"
|
||||
middleware.py: null
|
||||
templates/:
|
||||
spa.html: "single Jinja2 shell template"
|
||||
static/js/spa/:
|
||||
app.js: "SPA entry, route registration"
|
||||
router.js: "History API client-side router"
|
||||
api.js: "fetch wrapper for API calls"
|
||||
components.js: "shared lit-html helpers, t() re-export"
|
||||
icons.js: "SVG icon functions"
|
||||
pages/: "lazy-loaded page modules"
|
||||
alembic/: "DB migrations"
|
||||
etc/:
|
||||
prometheus/: "Prometheus scrape + alert rules"
|
||||
alertmanager/: null
|
||||
seed/: "YAML seed data (node_tags, members)"
|
||||
tests/:
|
||||
|
||||
key_symbols:
|
||||
- src/meshcore_hub/__main__.py::cli — Click root group [entry-point]
|
||||
- src/meshcore_hub/common/config.py::CommonSettings — shared env config base
|
||||
- src/meshcore_hub/common/database.py::DatabaseManager — async session factory
|
||||
- src/meshcore_hub/common/models/base.py::Base — declarative base for all models
|
||||
- src/meshcore_hub/api/app.py::create_app — API FastAPI factory
|
||||
- src/meshcore_hub/web/app.py::create_app — Web FastAPI factory
|
||||
- src/meshcore_hub/api/auth.py::require_read — read-key auth dependency
|
||||
- src/meshcore_hub/api/auth.py::require_admin — admin-key auth dependency
|
||||
- src/meshcore_hub/collector/subscriber.py::MQTTSubscriber — event ingestion loop
|
||||
- src/meshcore_hub/interface/receiver.py::Receiver — device→MQTT bridge
|
||||
- src/meshcore_hub/interface/sender.py::Sender — MQTT→device bridge
|
||||
|
||||
conventions:
|
||||
- four Click subcommands: interface, collector, api, web
|
||||
- "MQTT topic pattern: {prefix}/{pubkey}/event/{name} and .../command/{name}"
|
||||
- env config via pydantic-settings, no manual os.environ
|
||||
- web SPA: ES modules + lit-html, pages export async render()
|
||||
- i18n via t() with JSON locale files in static/locales/
|
||||
- node tags are freeform key-value pairs, standard keys in AGENTS.md
|
||||
100
.plans/2026/03/09/01-security-fixes/changelog.md
Normal file
100
.plans/2026/03/09/01-security-fixes/changelog.md
Normal file
@@ -0,0 +1,100 @@
|
||||
## TASK-001: Remove legacy HTML dashboard endpoint
|
||||
**Status:** completed
|
||||
### Files Modified
|
||||
- `src/meshcore_hub/api/routes/dashboard.py`
|
||||
- `tests/test_api/test_dashboard.py`
|
||||
### Notes
|
||||
Removed the `dashboard()` route handler and its `@router.get("")` decorator. Removed `HTMLResponse` and `Request` imports no longer used. Updated existing tests to verify the HTML endpoint returns 404/405. All JSON sub-routes (`/stats`, `/activity`, `/message-activity`, `/node-count`) remain intact.
|
||||
---
|
||||
|
||||
## TASK-002: Replace API key comparisons with constant-time comparison
|
||||
**Status:** completed
|
||||
### Files Modified
|
||||
- `src/meshcore_hub/api/auth.py`
|
||||
- `src/meshcore_hub/api/metrics.py`
|
||||
### Notes
|
||||
Added `import hmac` to both files. Replaced `==` comparisons with `hmac.compare_digest()` in `require_read`, `require_admin`, and `verify_basic_auth`. Added truthiness guards for `read_key`/`admin_key` in `require_read` since either can be `None` and `hmac.compare_digest()` raises `TypeError` on `None`.
|
||||
---
|
||||
|
||||
## TASK-003: Add WEB_TRUSTED_PROXY_HOSTS configuration setting
|
||||
**Status:** completed
|
||||
### Files Modified
|
||||
- `src/meshcore_hub/common/config.py`
|
||||
### Notes
|
||||
Added `web_trusted_proxy_hosts: str = Field(default="*", ...)` to `WebSettings` class. Automatically configurable via `WEB_TRUSTED_PROXY_HOSTS` env var through Pydantic Settings.
|
||||
---
|
||||
|
||||
## TASK-004: Integrate trusted proxy hosts into web app middleware and add startup warning
|
||||
**Status:** completed
|
||||
### Files Modified
|
||||
- `src/meshcore_hub/web/app.py`
|
||||
### Notes
|
||||
Replaced hardcoded `trusted_hosts="*"` in `ProxyHeadersMiddleware` with configured value. If value is `"*"`, passes string directly; otherwise splits on commas. Added startup warning when `WEB_ADMIN_ENABLED=true` and `WEB_TRUSTED_PROXY_HOSTS="*"`. `_is_authenticated_proxy_request` unchanged.
|
||||
---
|
||||
|
||||
## TASK-005: Escape config JSON in template script block to prevent XSS breakout
|
||||
**Status:** completed
|
||||
### Files Modified
|
||||
- `src/meshcore_hub/web/app.py`
|
||||
### Notes
|
||||
Added `.replace("</", "<\\/")` to `_build_config_json` return value. Prevents `</script>` breakout in the Jinja2 template's `<script>` block. `<\/` is valid JSON per spec and parsed correctly by `JSON.parse()`.
|
||||
---
|
||||
|
||||
## TASK-006: Fix stored XSS in admin node-tags page
|
||||
**Status:** completed
|
||||
### Files Modified
|
||||
- `src/meshcore_hub/web/static/js/spa/pages/admin/node-tags.js`
|
||||
### Notes
|
||||
Added `escapeHtml` to imports. Escaped `nodeName` with `escapeHtml()` in copy-all and delete-all confirmation dialogs (2 `unsafeHTML()` calls). Escaped `activeTagKey` with `escapeHtml()` in single tag delete confirmation (`innerHTML` assignment). Translation template `<strong>` tags preserved.
|
||||
---
|
||||
|
||||
## TASK-007: Fix stored XSS in admin members page
|
||||
**Status:** completed
|
||||
### Files Modified
|
||||
- `src/meshcore_hub/web/static/js/spa/pages/admin/members.js`
|
||||
### Notes
|
||||
Added `escapeHtml` to imports. Escaped `memberName` with `escapeHtml()` before passing to `t()` in delete confirmation dialog. `innerHTML` retained for `<strong>` tag rendering from translation template.
|
||||
---
|
||||
|
||||
## TASK-008: Write tests for legacy dashboard endpoint removal
|
||||
**Status:** completed
|
||||
### Files Modified
|
||||
- `tests/test_api/test_dashboard.py`
|
||||
### Notes
|
||||
Added 5 new tests: 1 for trailing-slash 404/405 verification, 4 for authenticated JSON sub-route responses. Total 20 dashboard tests passing.
|
||||
---
|
||||
|
||||
## TASK-009: Write tests for constant-time API key comparison
|
||||
**Status:** completed
|
||||
### Files Modified
|
||||
- `tests/test_api/test_auth.py`
|
||||
### Notes
|
||||
Restructured from 10 tests (2 classes) to 22 tests (4 classes): `TestReadAuthentication` (9), `TestAdminAuthentication` (4), `TestMetricsAuthentication` (7), `TestHealthEndpoint` (2). Added coverage for multi-endpoint read/admin key acceptance, missing auth header rejection, and metrics credential validation.
|
||||
---
|
||||
|
||||
## TASK-010: Write tests for trusted proxy hosts configuration and startup warning
|
||||
**Status:** completed
|
||||
### Files Modified
|
||||
- `tests/test_common/test_config.py`
|
||||
- `tests/test_web/test_app.py`
|
||||
### Notes
|
||||
Added 3 config tests (default value, specific IP, comma-separated list) and 5 web app tests (warning logged with wildcard+admin, no warning with specific hosts, no warning with admin disabled, comma list parsing, wildcard passed as string).
|
||||
---
|
||||
|
||||
## TASK-011: Write tests for config JSON script block escaping
|
||||
**Status:** completed
|
||||
### Files Created
|
||||
- `tests/test_web/test_app.py`
|
||||
### Notes
|
||||
Added 5 tests in `TestConfigJsonXssEscaping` class: rendered HTML escaping, normal values unaffected, escaped JSON parseable, direct `_build_config_json` escaping, direct no-escaping-needed.
|
||||
---
|
||||
|
||||
## TASK-012: Update documentation for WEB_TRUSTED_PROXY_HOSTS setting
|
||||
**Status:** completed
|
||||
### Files Modified
|
||||
- `README.md`
|
||||
- `AGENTS.md`
|
||||
- `PLAN.md`
|
||||
### Notes
|
||||
Added `WEB_TRUSTED_PROXY_HOSTS` to environment variables sections in all three docs. Documented default value (`*`), production recommendation, and startup warning behavior.
|
||||
---
|
||||
162
.plans/2026/03/09/01-security-fixes/prd.md
Normal file
162
.plans/2026/03/09/01-security-fixes/prd.md
Normal file
@@ -0,0 +1,162 @@
|
||||
# Product Requirements Document
|
||||
|
||||
> Source: `.plans/2026/03/09/01-security-fixes/prompt.md`
|
||||
|
||||
## Project Overview
|
||||
|
||||
This project addresses CRITICAL and HIGH severity vulnerabilities identified in a security audit of MeshCore Hub. The fixes span stored XSS in server-rendered and client-side code, timing attacks on authentication, proxy header forgery, and a legacy endpoint with missing authentication. All changes must be backward-compatible and preserve existing API contracts.
|
||||
|
||||
## Goals
|
||||
|
||||
- Eliminate all CRITICAL and HIGH severity security vulnerabilities found in the audit
|
||||
- Harden API key comparison against timing side-channel attacks
|
||||
- Prevent XSS vectors in both Jinja2 templates and client-side JavaScript
|
||||
- Add configurable proxy trust to defend against header forgery while maintaining backward compatibility
|
||||
- Remove the redundant legacy HTML dashboard endpoint that lacks authentication
|
||||
|
||||
## Functional Requirements
|
||||
|
||||
### REQ-001: Remove legacy HTML dashboard endpoint
|
||||
|
||||
**Description:** Remove the `GET /api/v1/dashboard/` route handler that renders a standalone HTML page with unescaped database content (stored XSS) and no authentication. The JSON sub-routes (`/stats`, `/activity`, `/message-activity`, `/node-count`) must remain intact and unchanged.
|
||||
|
||||
**Acceptance Criteria:**
|
||||
|
||||
- [ ] The `dashboard()` route handler in `api/routes/dashboard.py` is removed
|
||||
- [ ] The `HTMLResponse` import is removed (if no longer used)
|
||||
- [ ] `GET /api/v1/dashboard/` returns 404 or Method Not Allowed
|
||||
- [ ] `GET /api/v1/dashboard/stats` continues to return valid JSON with authentication
|
||||
- [ ] `GET /api/v1/dashboard/activity` continues to return valid JSON with authentication
|
||||
- [ ] `GET /api/v1/dashboard/message-activity` continues to return valid JSON with authentication
|
||||
- [ ] `GET /api/v1/dashboard/node-count` continues to return valid JSON with authentication
|
||||
- [ ] Existing API tests for JSON sub-routes still pass
|
||||
|
||||
### REQ-002: Use constant-time comparison for API key validation
|
||||
|
||||
**Description:** Replace all Python `==` comparisons of API keys and credentials with `hmac.compare_digest()` to prevent timing side-channel attacks that could leak key material.
|
||||
|
||||
**Acceptance Criteria:**
|
||||
|
||||
- [ ] All API key comparisons in `api/auth.py` use `hmac.compare_digest()` instead of `==`
|
||||
- [ ] All credential comparisons in `api/metrics.py` use `hmac.compare_digest()` instead of `==`
|
||||
- [ ] `hmac` is imported in all files where secret comparison occurs
|
||||
- [ ] The authentication behavior is unchanged — valid keys are accepted, invalid keys are rejected
|
||||
- [ ] Tests confirm authentication still works correctly with valid and invalid keys
|
||||
|
||||
### REQ-003: Add configurable trusted proxy hosts for admin authentication
|
||||
|
||||
**Description:** Add a `WEB_TRUSTED_PROXY_HOSTS` configuration setting that controls which hosts are trusted for proxy authentication headers (`X-Forwarded-User`, `X-Auth-Request-User`, `Authorization: Basic`). The setting defaults to `*` for backward compatibility. A startup warning is emitted when admin is enabled with the wildcard default. The `Authorization: Basic` header check must be preserved for Nginx Proxy Manager compatibility.
|
||||
|
||||
**Acceptance Criteria:**
|
||||
|
||||
- [ ] A `WEB_TRUSTED_PROXY_HOSTS` setting is added to the configuration (Pydantic Settings)
|
||||
- [ ] The setting defaults to `*` (backward compatible)
|
||||
- [ ] `ProxyHeadersMiddleware` uses the configured `trusted_hosts` value instead of hardcoded `*`
|
||||
- [ ] A warning is logged at startup when `WEB_ADMIN_ENABLED=true` and `WEB_TRUSTED_PROXY_HOSTS` is `*`
|
||||
- [ ] The warning message recommends restricting trusted hosts to the operator's proxy IP
|
||||
- [ ] The `_is_authenticated_proxy_request` function continues to accept `X-Forwarded-User`, `X-Auth-Request-User`, and `Authorization: Basic` headers
|
||||
- [ ] OAuth2 proxy setups continue to function correctly
|
||||
- [ ] Setting `WEB_TRUSTED_PROXY_HOSTS` to a specific IP restricts proxy header trust to that IP
|
||||
|
||||
### REQ-004: Escape config JSON in template script block
|
||||
|
||||
**Description:** Prevent XSS via `</script>` breakout in the `config_json|safe` template injection by escaping `</` sequences in the serialized JSON string before passing it to the Jinja2 template.
|
||||
|
||||
**Acceptance Criteria:**
|
||||
|
||||
- [ ] `config_json` is escaped by replacing `</` with `<\\/` before template rendering (in `web/app.py`)
|
||||
- [ ] The `|safe` filter continues to be used (the escaping happens in Python, not Jinja2)
|
||||
- [ ] A config value containing `</script><script>alert(1)</script>` does not execute JavaScript
|
||||
- [ ] The SPA application correctly parses the escaped config JSON on the client side
|
||||
- [ ] Normal config values (without special characters) render unchanged
|
||||
|
||||
### REQ-005: Fix stored XSS in admin page JavaScript
|
||||
|
||||
**Description:** Sanitize API-sourced data (node names, tag keys, member names) before rendering in admin pages. Replace `unsafeHTML()` and direct `innerHTML` assignment with safe alternatives — either `escapeHtml()` (already available in `components.js`) or lit-html safe templating (`${value}` interpolation without `unsafeHTML`).
|
||||
|
||||
**Acceptance Criteria:**
|
||||
|
||||
- [ ] Node names in `admin/node-tags.js` are escaped or safely templated before HTML rendering
|
||||
- [ ] Tag keys in `admin/node-tags.js` are escaped or safely templated before HTML rendering
|
||||
- [ ] Member names in `admin/members.js` are escaped or safely templated before HTML rendering
|
||||
- [ ] All `unsafeHTML()` calls on API-sourced data in the identified files are replaced with safe alternatives
|
||||
- [ ] All direct `innerHTML` assignments of API-sourced data in the identified files are replaced with safe alternatives
|
||||
- [ ] A node name containing `<img src=x onerror=alert(1)>` renders as text, not as an HTML element
|
||||
- [ ] A member name containing `<script>alert(1)</script>` renders as text, not as executable script
|
||||
- [ ] Normal names (without special characters) continue to display correctly
|
||||
|
||||
## Non-Functional Requirements
|
||||
|
||||
### REQ-006: Backward compatibility
|
||||
|
||||
**Category:** Reliability
|
||||
|
||||
**Description:** All security fixes must maintain backward compatibility with existing deployments. No breaking changes to API contracts, configuration defaults, or deployment workflows.
|
||||
|
||||
**Acceptance Criteria:**
|
||||
|
||||
- [ ] All existing API endpoints (except the removed HTML dashboard) return the same response format
|
||||
- [ ] Default configuration values preserve existing behavior without requiring operator action
|
||||
- [ ] Docker Compose deployments continue to function without configuration changes
|
||||
- [ ] All existing tests pass after the security fixes are applied
|
||||
|
||||
### REQ-007: No regression in authentication flows
|
||||
|
||||
**Category:** Security
|
||||
|
||||
**Description:** The security hardening must not introduce authentication regressions. Valid credentials must continue to be accepted, and invalid credentials must continue to be rejected, across all authentication methods.
|
||||
|
||||
**Acceptance Criteria:**
|
||||
|
||||
- [ ] API read key authentication accepts valid keys and rejects invalid keys
|
||||
- [ ] API admin key authentication accepts valid keys and rejects invalid keys
|
||||
- [ ] Metrics endpoint authentication (if configured) accepts valid credentials and rejects invalid ones
|
||||
- [ ] Proxy header authentication continues to work with OAuth2 proxy setups
|
||||
- [ ] Basic auth header forwarding from Nginx Proxy Manager continues to work
|
||||
|
||||
## Technical Constraints and Assumptions
|
||||
|
||||
### Constraints
|
||||
|
||||
- Python 3.13+ (specified by project `.python-version`)
|
||||
- Must use `hmac.compare_digest()` from the Python standard library for constant-time comparison
|
||||
- The `Authorization: Basic` header check in `_is_authenticated_proxy_request` must not be removed or modified to validate credentials server-side — credential validation is the proxy's responsibility
|
||||
- Changes must not alter existing API response schemas or status codes (except removing the HTML dashboard endpoint)
|
||||
|
||||
### Assumptions
|
||||
|
||||
- The `escapeHtml()` utility in `components.js` correctly escapes `<`, `>`, `&`, `"`, and `'` characters
|
||||
- The SPA client-side JavaScript can parse JSON containing escaped `<\/` sequences (standard behavior per JSON spec)
|
||||
- Operators using proxy authentication have a reverse proxy (e.g., Nginx, Traefik, NPM) in front of MeshCore Hub
|
||||
|
||||
## Scope
|
||||
|
||||
### In Scope
|
||||
|
||||
- Removing the legacy HTML dashboard route handler (C1 + H2)
|
||||
- Replacing `==` with `hmac.compare_digest()` for all secret comparisons (H1)
|
||||
- Adding `WEB_TRUSTED_PROXY_HOSTS` configuration and startup warning (H3)
|
||||
- Escaping `</` in config JSON template injection (H4)
|
||||
- Fixing `unsafeHTML()`/`innerHTML` XSS in admin JavaScript pages (H5)
|
||||
- Updating tests to cover the security fixes
|
||||
- Updating documentation for the new `WEB_TRUSTED_PROXY_HOSTS` setting
|
||||
|
||||
### Out of Scope
|
||||
|
||||
- MEDIUM severity findings (CORS, error detail leakage, rate limiting, security headers, CSRF, CDN SRI, markdown sanitization, input validation, channel key exposure)
|
||||
- LOW severity findings (auth warnings, version disclosure, unbounded fields, credential logging, SecretStr, port exposure, cache safety, image pinning)
|
||||
- INFO findings (OpenAPI docs, proxy IP logging, alertmanager comments, DOM XSS in error handler, locale path)
|
||||
- Adding rate limiting infrastructure
|
||||
- Adding Content-Security-Policy or other security headers
|
||||
- Dependency version pinning or lockfile generation
|
||||
- Server-side credential validation for Basic auth (proxy responsibility)
|
||||
|
||||
## Suggested Tech Stack
|
||||
|
||||
| Layer | Technology | Rationale |
|
||||
|-------|-----------|-----------|
|
||||
| Secret comparison | `hmac.compare_digest()` (stdlib) | Specified by prompt; constant-time comparison prevents timing attacks |
|
||||
| Template escaping | Python `str.replace()` | Minimal approach to escape `</` in JSON before Jinja2 rendering |
|
||||
| Client-side escaping | `escapeHtml()` from `components.js` | Already available in the codebase; standard HTML entity escaping |
|
||||
| Configuration | Pydantic Settings | Specified by project stack; used for `WEB_TRUSTED_PROXY_HOSTS` |
|
||||
| Testing | pytest, pytest-asyncio | Specified by project stack |
|
||||
65
.plans/2026/03/09/01-security-fixes/prompt.md
Normal file
65
.plans/2026/03/09/01-security-fixes/prompt.md
Normal file
@@ -0,0 +1,65 @@
|
||||
# Phase: 01-security-fixes
|
||||
|
||||
## Overview
|
||||
|
||||
Address CRITICAL and HIGH severity vulnerabilities identified in the MeshCore Hub security audit across API and Web components. These findings represent exploitable vulnerabilities including XSS, timing attacks, authentication bypasses, and insecure defaults.
|
||||
|
||||
## Goals
|
||||
|
||||
- Eliminate all CRITICAL and HIGH severity security vulnerabilities
|
||||
- Harden authentication mechanisms against timing attacks and header forgery
|
||||
- Prevent XSS vectors in both server-rendered HTML and client-side JavaScript
|
||||
- Secure default MQTT configuration against unauthenticated access
|
||||
|
||||
## Requirements
|
||||
|
||||
### C1 + H2 — Remove legacy HTML dashboard endpoint
|
||||
- **File:** `src/meshcore_hub/api/routes/dashboard.py:367-536`
|
||||
- The `GET /api/v1/dashboard/` endpoint is a standalone HTML page with two CRITICAL/HIGH issues: stored XSS (unescaped DB content in f-string HTML) and missing authentication
|
||||
- The SPA web dashboard provides a full-featured replacement, making this endpoint redundant
|
||||
- **Fix:** Remove the `dashboard()` route handler and its `HTMLResponse` import. Keep all JSON sub-routes (`/stats`, `/activity`, `/message-activity`, `/node-count`) intact.
|
||||
|
||||
### H1 — Fix timing attack on API key comparison
|
||||
- **Files:** `api/auth.py:82,127` | `api/metrics.py:57`
|
||||
- All secret comparisons use Python `==`, which is not constant-time
|
||||
- **Fix:** Replace with `hmac.compare_digest()` for all key/credential comparisons
|
||||
|
||||
### H3 — Harden admin auth against proxy header forgery
|
||||
- **File:** `web/app.py:73-86,239`
|
||||
- Admin access trusts `X-Forwarded-User`, `X-Auth-Request-User`, or `Authorization: Basic` header
|
||||
- `ProxyHeadersMiddleware(trusted_hosts="*")` accepts forged headers from any client
|
||||
- The `Authorization: Basic` check must be preserved — it is required by the Nginx Proxy Manager (NPM) Access List setup documented in README.md (NPM validates credentials and forwards the header)
|
||||
- **Fix:** Add a `WEB_TRUSTED_PROXY_HOSTS` config setting (default `*` for backward compatibility). Pass it to `ProxyHeadersMiddleware(trusted_hosts=...)`. Add a startup warning when `WEB_ADMIN_ENABLED=true` and `trusted_hosts` is still `*`, recommending operators restrict it to their proxy IP. Do NOT remove the Basic auth header check or validate credentials server-side — that is the proxy's responsibility.
|
||||
|
||||
### H4 — Fix XSS via config_json|safe script block breakout
|
||||
- **File:** `web/templates/spa.html:188` | `web/app.py:157-183`
|
||||
- Operator config values injected into `<script>` block with `|safe` — a value containing `</script>` breaks out and executes arbitrary JS
|
||||
- **Fix:** Escape `</` sequences in the JSON string: `config_json = json.dumps(config).replace("</", "<\\/")`
|
||||
|
||||
### H5 — Fix stored XSS via unsafeHTML/innerHTML with API-sourced data
|
||||
- **Files:** `web/static/js/spa/pages/admin/node-tags.js:243,272,454` | `admin/members.js:309`
|
||||
- Node names, tag keys, and member names from the API are interpolated into HTML via `unsafeHTML()` and direct `innerHTML` assignment
|
||||
- **Fix:** Use `escapeHtml()` (already in `components.js`) on API data before HTML interpolation, or replace with lit-html safe templating
|
||||
|
||||
|
||||
## Constraints
|
||||
|
||||
- Must not break existing functionality or API contracts
|
||||
- Changes to docker-compose.yml and mosquitto.conf must remain backward-compatible (use env var defaults)
|
||||
- The `_is_authenticated_proxy_request` function must continue to work with OAuth2 proxy setups — only add defense-in-depth, don't remove proxy header support entirely
|
||||
|
||||
## Out of Scope
|
||||
|
||||
- MEDIUM severity findings (CORS config, error detail leakage, rate limiting, security headers, CSRF, CDN SRI, markdown sanitization, input validation, channel key exposure)
|
||||
- LOW severity findings (auth warnings, version disclosure, unbounded fields, credential logging, SecretStr, port exposure, cache safety, image pinning)
|
||||
- INFO findings (OpenAPI docs, proxy IP logging, alertmanager comments, DOM XSS in error handler, locale path)
|
||||
- Adding rate limiting infrastructure
|
||||
- Adding Content-Security-Policy or other security headers
|
||||
- Dependency version pinning or lockfile generation
|
||||
|
||||
## References
|
||||
|
||||
- Security audit performed in this conversation (2026-03-09)
|
||||
- OWASP Top 10: XSS (A7:2017), Broken Authentication (A2:2017)
|
||||
- Python `hmac.compare_digest` documentation
|
||||
- FastAPI security best practices
|
||||
54
.plans/2026/03/09/01-security-fixes/reviews/cycle/001.yaml
Normal file
54
.plans/2026/03/09/01-security-fixes/reviews/cycle/001.yaml
Normal file
@@ -0,0 +1,54 @@
|
||||
# Code review round 001
|
||||
# Phase: .plans/2026/03/09/01-security-fixes
|
||||
# Scope: full
|
||||
# Generated by: /jp-codereview
|
||||
|
||||
issues:
|
||||
- id: "ISSUE-001"
|
||||
severity: "MINOR"
|
||||
category: "integration"
|
||||
file: "src/meshcore_hub/web/app.py"
|
||||
line: 251
|
||||
description: |
|
||||
The startup warning for insecure trusted proxy hosts checks `settings.web_admin_enabled`
|
||||
instead of the effective admin_enabled value that gets stored in `app.state.admin_enabled`.
|
||||
The `create_app()` function accepts an `admin_enabled` parameter (line 193) that can override
|
||||
the setting. If a caller passes `admin_enabled=True` but `settings.web_admin_enabled` is False,
|
||||
the warning will not fire despite admin being enabled. In practice this does not affect production
|
||||
deployments (CLI always uses the settings value), only programmatic/test usage.
|
||||
suggestion: |
|
||||
Consider computing the effective admin_enabled value before the warning check and using
|
||||
that for both the warning and `app.state.admin_enabled`, e.g.:
|
||||
`effective_admin = admin_enabled if admin_enabled is not None else settings.web_admin_enabled`
|
||||
related_tasks:
|
||||
- "TASK-004"
|
||||
|
||||
- id: "ISSUE-002"
|
||||
severity: "MINOR"
|
||||
category: "style"
|
||||
file: "src/meshcore_hub/web/static/js/spa/pages/admin/node-tags.js"
|
||||
line: 3
|
||||
description: |
|
||||
The `unsafeHTML` import is retained and still used on lines 243 and 272. Although the
|
||||
API-sourced data (`nodeName`) is now safely escaped via `escapeHtml()` before interpolation,
|
||||
the continued use of `unsafeHTML()` may confuse future reviewers into thinking the XSS
|
||||
fix is incomplete. The `unsafeHTML()` is needed to render the translation template's HTML
|
||||
tags (e.g., `<strong>`), so this is functionally correct.
|
||||
suggestion: |
|
||||
Add a brief inline comment above each `unsafeHTML()` call explaining that the dynamic
|
||||
values are pre-escaped and `unsafeHTML()` is only needed for the template's HTML formatting.
|
||||
related_tasks:
|
||||
- "TASK-006"
|
||||
|
||||
summary:
|
||||
total_issues: 2
|
||||
critical: 0
|
||||
major: 0
|
||||
minor: 2
|
||||
by_category:
|
||||
integration: 1
|
||||
architecture: 0
|
||||
security: 0
|
||||
duplication: 0
|
||||
error-handling: 0
|
||||
style: 1
|
||||
70
.plans/2026/03/09/01-security-fixes/reviews/prd.md
Normal file
70
.plans/2026/03/09/01-security-fixes/reviews/prd.md
Normal file
@@ -0,0 +1,70 @@
|
||||
# PRD Review
|
||||
|
||||
> Phase: `.plans/2026/03/09/01-security-fixes`
|
||||
> PRD: `.plans/2026/03/09/01-security-fixes/prd.md`
|
||||
> Prompt: `.plans/2026/03/09/01-security-fixes/prompt.md`
|
||||
|
||||
## Verdict: PASS
|
||||
|
||||
The PRD fully covers all five security requirements from the prompt with clear, implementable, and testable acceptance criteria. No contradictions, blocking ambiguities, or feasibility concerns were found. One prompt goal ("Secure default MQTT configuration") has no corresponding requirement in either the prompt or the PRD, but since no prompt requirement addresses it, the PRD correctly does not fabricate one.
|
||||
|
||||
## Coverage Assessment
|
||||
|
||||
| Prompt Item | PRD Section | Covered? | Notes |
|
||||
|---|---|---|---|
|
||||
| C1+H2: Remove legacy HTML dashboard endpoint | REQ-001 | Yes | Route removal, import cleanup, sub-route preservation all specified |
|
||||
| H1: Fix timing attack on API key comparison | REQ-002 | Yes | Files and `hmac.compare_digest()` approach match |
|
||||
| H3: Harden admin auth / proxy header forgery | REQ-003 | Yes | Config setting, default, warning, Basic auth preservation all covered |
|
||||
| H4: Fix XSS via config_json\|safe breakout | REQ-004 | Yes | Escape approach and XSS test payload specified |
|
||||
| H5: Fix stored XSS via unsafeHTML/innerHTML | REQ-005 | Yes | Files, fix approach, and XSS test payloads specified |
|
||||
| Constraint: No breaking changes to API contracts | REQ-006 | Yes | |
|
||||
| Constraint: docker-compose.yml/mosquitto.conf backward-compatible | REQ-006 | Partial | REQ-006 covers Docker Compose but not mosquitto.conf; moot since no requirement changes mosquitto.conf |
|
||||
| Constraint: _is_authenticated_proxy_request works with OAuth2 | REQ-003, REQ-007 | Yes | |
|
||||
| Goal: Secure default MQTT configuration | -- | No | Goal stated in prompt but no prompt requirement addresses it; PRD correctly does not fabricate one |
|
||||
| Out of scope items | Scope section | Yes | All exclusions match prompt |
|
||||
|
||||
**Coverage summary:** 5 of 5 prompt requirements fully covered, 1 constraint partially covered (moot), 1 prompt goal has no corresponding requirement in the prompt itself.
|
||||
|
||||
## Requirement Evaluation
|
||||
|
||||
All requirements passed evaluation. Minor observations noted below.
|
||||
|
||||
### REQ-003: Add configurable trusted proxy hosts
|
||||
|
||||
- **Implementability:** Pass -- A developer familiar with Pydantic Settings and `ProxyHeadersMiddleware` can implement this without ambiguity. The env var format (comma-separated list vs. single value) is not explicitly stated but follows standard Pydantic patterns.
|
||||
- **Testability:** Pass
|
||||
- **Completeness:** Pass
|
||||
- **Consistency:** Pass
|
||||
|
||||
### REQ-006: Backward compatibility
|
||||
|
||||
- **Implementability:** Pass
|
||||
- **Testability:** Pass
|
||||
- **Completeness:** Pass -- The prompt constraint about mosquitto.conf backward compatibility is not explicitly mentioned, but no requirement modifies mosquitto.conf, making this moot.
|
||||
- **Consistency:** Pass
|
||||
|
||||
## Structural Issues
|
||||
|
||||
### Contradictions
|
||||
|
||||
None found.
|
||||
|
||||
### Ambiguities
|
||||
|
||||
None that would block implementation. The `WEB_TRUSTED_PROXY_HOSTS` env var format is a minor detail resolvable by the developer from the `ProxyHeadersMiddleware` API and standard Pydantic Settings patterns.
|
||||
|
||||
### Missing Edge Cases
|
||||
|
||||
None significant. The `hmac.compare_digest()` change (REQ-002) assumes the existing code handles the "no key configured" case before reaching the comparison, which is standard practice and verifiable during implementation.
|
||||
|
||||
### Feasibility Concerns
|
||||
|
||||
None.
|
||||
|
||||
### Scope Inconsistencies
|
||||
|
||||
The prompt states a goal of "Secure default MQTT configuration against unauthenticated access" but provides no requirement for it. The PRD drops this goal without explanation. This is a prompt-level gap, not a PRD-level gap -- the PRD should not invent requirements that the prompt does not specify.
|
||||
|
||||
## Action Items
|
||||
|
||||
No action items. The PRD is ready for task breakdown.
|
||||
90
.plans/2026/03/09/01-security-fixes/reviews/tasks.md
Normal file
90
.plans/2026/03/09/01-security-fixes/reviews/tasks.md
Normal file
@@ -0,0 +1,90 @@
|
||||
# Task Review
|
||||
|
||||
> Phase: `.plans/2026/03/09/01-security-fixes`
|
||||
> Tasks: `.plans/2026/03/09/01-security-fixes/tasks.yaml`
|
||||
> PRD: `.plans/2026/03/09/01-security-fixes/prd.md`
|
||||
|
||||
## Verdict: PASS
|
||||
|
||||
The task list is structurally sound, correctly ordered, and fully covers all 7 PRD requirements. The dependency graph is a valid DAG with no cycles or invalid references. No ordering issues, coverage gaps, vague tasks, or invalid fields were found. Two non-blocking warnings are noted: TASK-006 and TASK-007 (frontend XSS fixes) lack corresponding test tasks, and two pairs of independent tasks share output files but modify independent sections.
|
||||
|
||||
## Dependency Validation
|
||||
|
||||
### Reference Validity
|
||||
|
||||
All dependency references are valid. Every task ID referenced in a `dependencies` list corresponds to an existing task in the inventory.
|
||||
|
||||
### DAG Validation
|
||||
|
||||
The dependency graph is a valid directed acyclic graph. No cycles detected.
|
||||
|
||||
Topological layers:
|
||||
- **Layer 0 (roots):** TASK-001, TASK-002, TASK-003, TASK-005, TASK-006, TASK-007
|
||||
- **Layer 1:** TASK-004 (depends on TASK-003), TASK-008 (depends on TASK-001), TASK-009 (depends on TASK-002), TASK-011 (depends on TASK-005)
|
||||
- **Layer 2:** TASK-010 (depends on TASK-003, TASK-004), TASK-012 (depends on TASK-003, TASK-004)
|
||||
|
||||
### Orphan Tasks
|
||||
|
||||
No orphan tasks detected. All non-root tasks with dependencies are either terminal test/docs tasks (TASK-008, TASK-009, TASK-010, TASK-011, TASK-012) or integration tasks (TASK-004). Root tasks without dependents (TASK-006, TASK-007) are excluded from orphan detection per the review protocol.
|
||||
|
||||
## Ordering Check
|
||||
|
||||
No blocking ordering issues detected.
|
||||
|
||||
**Observation (non-blocking):** Two pairs of independent tasks share output files:
|
||||
|
||||
1. **TASK-004 and TASK-005** both modify `src/meshcore_hub/web/app.py` without a dependency between them. TASK-004 modifies `ProxyHeadersMiddleware` (line ~239) and adds a startup warning, while TASK-005 modifies `_build_config_json` (line ~183). These are independent functions in the same file; no actual conflict exists.
|
||||
|
||||
2. **TASK-010 and TASK-011** both modify `tests/test_web/test_app.py` without a dependency between them. Both add new test functions to the same test file. No actual conflict exists.
|
||||
|
||||
These are not blocking because neither task creates the shared file — both modify existing files in independent sections. Adding artificial dependencies would unnecessarily serialize parallelizable work.
|
||||
|
||||
## Coverage Check
|
||||
|
||||
### Uncovered Requirements
|
||||
|
||||
All PRD requirements are covered.
|
||||
|
||||
### Phantom References
|
||||
|
||||
No phantom references detected.
|
||||
|
||||
**Coverage summary:** 7 of 7 PRD requirements covered by tasks.
|
||||
|
||||
| Requirement | Tasks |
|
||||
|---|---|
|
||||
| REQ-001 | TASK-001, TASK-008 |
|
||||
| REQ-002 | TASK-002, TASK-009 |
|
||||
| REQ-003 | TASK-003, TASK-004, TASK-010, TASK-012 |
|
||||
| REQ-004 | TASK-005, TASK-011 |
|
||||
| REQ-005 | TASK-006, TASK-007 |
|
||||
| REQ-006 | TASK-001, TASK-003, TASK-004, TASK-005, TASK-006, TASK-007, TASK-008, TASK-010, TASK-011, TASK-012 |
|
||||
| REQ-007 | TASK-002, TASK-004, TASK-009 |
|
||||
|
||||
## Scope Check
|
||||
|
||||
### Tasks Too Large
|
||||
|
||||
No tasks flagged as too large. No task has `estimated_complexity: large`.
|
||||
|
||||
### Tasks Too Vague
|
||||
|
||||
No tasks flagged as too vague. All tasks have detailed descriptions (>50 chars), multiple testable acceptance criteria, and specific file paths in `files_affected`.
|
||||
|
||||
### Missing Test Tasks
|
||||
|
||||
Two implementation tasks lack corresponding test tasks:
|
||||
|
||||
- **TASK-006** (Fix stored XSS in admin node-tags page) — modifies `admin/node-tags.js` but no test task verifies the XSS fix in this JavaScript file. The acceptance criteria include XSS payload testing, but no automated test is specified. This is a frontend JavaScript change where manual verification or browser-based testing may be appropriate.
|
||||
|
||||
- **TASK-007** (Fix stored XSS in admin members page) — modifies `admin/members.js` but no test task verifies the XSS fix in this JavaScript file. Same reasoning as TASK-006.
|
||||
|
||||
**Note:** These are warnings, not blocking issues. The project's test infrastructure (`tests/test_web/`) focuses on server-side rendering and API responses. Client-side JavaScript XSS fixes are typically verified through acceptance criteria rather than automated unit tests.
|
||||
|
||||
### Field Validation
|
||||
|
||||
All tasks have valid fields:
|
||||
|
||||
- **Roles:** All `suggested_role` values are valid (`python`, `frontend`, `docs`).
|
||||
- **Complexity:** All `estimated_complexity` values are valid (`small`, `medium`).
|
||||
- **Completeness:** All 12 tasks have all required fields (`id`, `title`, `description`, `requirements`, `dependencies`, `suggested_role`, `acceptance_criteria`, `estimated_complexity`, `files_affected`). All list fields have at least one entry.
|
||||
22
.plans/2026/03/09/01-security-fixes/state.yaml
Normal file
22
.plans/2026/03/09/01-security-fixes/state.yaml
Normal file
@@ -0,0 +1,22 @@
|
||||
status: running
|
||||
phase_path: .plans/2026/03/09/01-security-fixes
|
||||
branch: fix/security-fixes
|
||||
current_phase: summary
|
||||
current_task: null
|
||||
fix_round: 0
|
||||
last_review_round: 1
|
||||
review_loop_exit_reason: success
|
||||
quality_gate: pass
|
||||
tasks:
|
||||
TASK-001: completed
|
||||
TASK-002: completed
|
||||
TASK-003: completed
|
||||
TASK-004: completed
|
||||
TASK-005: completed
|
||||
TASK-006: completed
|
||||
TASK-007: completed
|
||||
TASK-008: completed
|
||||
TASK-009: completed
|
||||
TASK-010: completed
|
||||
TASK-011: completed
|
||||
TASK-012: completed
|
||||
117
.plans/2026/03/09/01-security-fixes/summary.md
Normal file
117
.plans/2026/03/09/01-security-fixes/summary.md
Normal file
@@ -0,0 +1,117 @@
|
||||
# Phase Summary
|
||||
|
||||
> Phase: `.plans/2026/03/09/01-security-fixes`
|
||||
> Generated by: `/jp-summary`
|
||||
|
||||
## Project Overview
|
||||
|
||||
This phase addresses CRITICAL and HIGH severity vulnerabilities identified in a security audit of MeshCore Hub. The fixes span stored XSS in server-rendered and client-side code, timing attacks on authentication, proxy header forgery, and a legacy endpoint with missing authentication. All changes are backward-compatible and preserve existing API contracts.
|
||||
|
||||
### Goals
|
||||
|
||||
- Eliminate all CRITICAL and HIGH severity security vulnerabilities found in the audit
|
||||
- Harden API key comparison against timing side-channel attacks
|
||||
- Prevent XSS vectors in both Jinja2 templates and client-side JavaScript
|
||||
- Add configurable proxy trust to defend against header forgery while maintaining backward compatibility
|
||||
- Remove the redundant legacy HTML dashboard endpoint that lacks authentication
|
||||
|
||||
## Task Execution
|
||||
|
||||
### Overview
|
||||
|
||||
| Metric | Value |
|
||||
|---|---|
|
||||
| Total tasks | 12 |
|
||||
| Completed | 12 |
|
||||
| Failed | 0 |
|
||||
| Blocked | 0 |
|
||||
| Skipped | 0 |
|
||||
|
||||
### Task Details
|
||||
|
||||
| ID | Title | Role | Complexity | Status |
|
||||
|---|---|---|---|---|
|
||||
| TASK-001 | Remove legacy HTML dashboard endpoint | python | small | completed |
|
||||
| TASK-002 | Replace API key comparisons with constant-time comparison | python | small | completed |
|
||||
| TASK-003 | Add WEB_TRUSTED_PROXY_HOSTS configuration setting | python | small | completed |
|
||||
| TASK-004 | Integrate trusted proxy hosts into web app middleware and add startup warning | python | medium | completed |
|
||||
| TASK-005 | Escape config JSON in template script block to prevent XSS breakout | python | small | completed |
|
||||
| TASK-006 | Fix stored XSS in admin node-tags page | frontend | medium | completed |
|
||||
| TASK-007 | Fix stored XSS in admin members page | frontend | small | completed |
|
||||
| TASK-008 | Write tests for legacy dashboard endpoint removal | python | small | completed |
|
||||
| TASK-009 | Write tests for constant-time API key comparison | python | small | completed |
|
||||
| TASK-010 | Write tests for trusted proxy hosts configuration and startup warning | python | medium | completed |
|
||||
| TASK-011 | Write tests for config JSON script block escaping | python | small | completed |
|
||||
| TASK-012 | Update documentation for WEB_TRUSTED_PROXY_HOSTS setting | docs | small | completed |
|
||||
|
||||
### Requirement Coverage
|
||||
|
||||
| Metric | Value |
|
||||
|---|---|
|
||||
| Total PRD requirements | 7 |
|
||||
| Requirements covered by completed tasks | 7 |
|
||||
| Requirements with incomplete coverage | 0 |
|
||||
|
||||
All functional requirements (REQ-001 through REQ-005) and non-functional requirements (REQ-006, REQ-007) are fully covered by completed tasks.
|
||||
|
||||
## Files Created and Modified
|
||||
|
||||
### Created
|
||||
|
||||
- `tests/test_web/test_app.py`
|
||||
|
||||
### Modified
|
||||
|
||||
- `src/meshcore_hub/api/routes/dashboard.py`
|
||||
- `src/meshcore_hub/api/auth.py`
|
||||
- `src/meshcore_hub/api/metrics.py`
|
||||
- `src/meshcore_hub/common/config.py`
|
||||
- `src/meshcore_hub/web/app.py`
|
||||
- `src/meshcore_hub/web/static/js/spa/pages/admin/node-tags.js`
|
||||
- `src/meshcore_hub/web/static/js/spa/pages/admin/members.js`
|
||||
- `tests/test_api/test_dashboard.py`
|
||||
- `tests/test_api/test_auth.py`
|
||||
- `tests/test_common/test_config.py`
|
||||
- `README.md`
|
||||
- `AGENTS.md`
|
||||
- `PLAN.md`
|
||||
|
||||
## Review Rounds
|
||||
|
||||
### Overview
|
||||
|
||||
| Metric | Value |
|
||||
|---|---|
|
||||
| Total review rounds | 1 |
|
||||
| Total issues found | 2 |
|
||||
| Issues fixed | 2 |
|
||||
| Issues deferred | 0 |
|
||||
| Issues remaining | 0 |
|
||||
| Regressions introduced | 0 |
|
||||
|
||||
### Round Details
|
||||
|
||||
#### Round 1 (scope: full)
|
||||
|
||||
- **Issues found:** 2 (0 CRITICAL, 0 MAJOR, 2 MINOR)
|
||||
- **Issues fixed:** 2 (both MINOR issues were addressed post-review)
|
||||
- **Exit reason:** success (no CRITICAL or MAJOR issues)
|
||||
|
||||
## Known Issues and Deferred Items
|
||||
|
||||
No known issues. Both MINOR issues identified in the code review were addressed:
|
||||
|
||||
- **ISSUE-001** (MINOR, integration) -- Startup warning for proxy hosts used `settings.web_admin_enabled` instead of the effective admin_enabled value. Fixed by computing `effective_admin` before the warning check.
|
||||
- **ISSUE-002** (MINOR, style) -- `unsafeHTML()` calls on pre-escaped data lacked explanatory comments. Fixed by adding inline HTML comments explaining that dynamic values are pre-escaped.
|
||||
|
||||
## Decisions
|
||||
|
||||
- **Truthiness guards for `hmac.compare_digest()`** -- Added `read_key and ...` / `admin_key and ...` guards in `require_read` because either key can be `None` when only one is configured, and `hmac.compare_digest()` raises `TypeError` on `None` arguments. This ensures the existing behavior of accepting either key type when configured.
|
||||
- **`unsafeHTML()` retained with `escapeHtml()` pre-processing** -- The `unsafeHTML()` calls in admin JS pages were retained because translation strings contain intentional HTML formatting tags (e.g., `<strong>`). API-sourced data is escaped before interpolation, making this pattern safe.
|
||||
- **`innerHTML` retained for tag delete confirmation** -- The delete confirmation in `node-tags.js` uses `innerHTML` because the translation template includes `<span>` formatting. The dynamic tag key is escaped with `escapeHtml()` before interpolation.
|
||||
|
||||
## Suggested Next Steps
|
||||
|
||||
1. Run full manual testing of admin pages (node-tags, members) with XSS payloads to verify fixes in a browser environment.
|
||||
2. Test `WEB_TRUSTED_PROXY_HOSTS` with a real reverse proxy (Traefik/Nginx) to verify proxy header trust restriction works as expected.
|
||||
3. Push commits and create a pull request for merge into `main`.
|
||||
401
.plans/2026/03/09/01-security-fixes/tasks.yaml
Normal file
401
.plans/2026/03/09/01-security-fixes/tasks.yaml
Normal file
@@ -0,0 +1,401 @@
|
||||
# Task list generated from PRD: .plans/2026/03/09/01-security-fixes/prd.md
|
||||
# Generated by: /jp-task-list
|
||||
|
||||
tasks:
|
||||
- id: "TASK-001"
|
||||
title: "Remove legacy HTML dashboard endpoint"
|
||||
description: |
|
||||
Remove the `dashboard()` route handler from `src/meshcore_hub/api/routes/dashboard.py` (lines ~367-536).
|
||||
This handler renders a standalone HTML page using f-string HTML with unescaped database content (stored XSS)
|
||||
and has no authentication. The JSON sub-routes (`/stats`, `/activity`, `/message-activity`, `/node-count`)
|
||||
must remain intact and unchanged.
|
||||
|
||||
Specifically:
|
||||
1. Delete the `dashboard()` async function and its `@router.get("")` decorator (the handler that returns HTMLResponse).
|
||||
2. Remove the `HTMLResponse` import from `fastapi.responses` if it is no longer used by any remaining route.
|
||||
3. Verify that `GET /api/v1/dashboard/stats`, `/activity`, `/message-activity`, and `/node-count` still function.
|
||||
requirements:
|
||||
- "REQ-001"
|
||||
- "REQ-006"
|
||||
dependencies: []
|
||||
suggested_role: "python"
|
||||
acceptance_criteria:
|
||||
- "The `dashboard()` route handler is removed from `api/routes/dashboard.py`"
|
||||
- "`HTMLResponse` import is removed if no longer used"
|
||||
- "`GET /api/v1/dashboard/` returns 404 or 405"
|
||||
- "`GET /api/v1/dashboard/stats` returns valid JSON with authentication"
|
||||
- "`GET /api/v1/dashboard/activity` returns valid JSON with authentication"
|
||||
- "`GET /api/v1/dashboard/message-activity` returns valid JSON with authentication"
|
||||
- "`GET /api/v1/dashboard/node-count` returns valid JSON with authentication"
|
||||
estimated_complexity: "small"
|
||||
files_affected:
|
||||
- "src/meshcore_hub/api/routes/dashboard.py"
|
||||
|
||||
- id: "TASK-002"
|
||||
title: "Replace API key comparisons with constant-time comparison"
|
||||
description: |
|
||||
Replace all Python `==` comparisons of API keys and credentials with `hmac.compare_digest()` to prevent
|
||||
timing side-channel attacks.
|
||||
|
||||
In `src/meshcore_hub/api/auth.py`:
|
||||
1. Add `import hmac` at the top of the file.
|
||||
2. Line ~82 in `require_read`: replace `if token == read_key or token == admin_key:` with
|
||||
`if hmac.compare_digest(token, read_key) or hmac.compare_digest(token, admin_key):`.
|
||||
3. Line ~127 in `require_admin`: replace `if token == admin_key:` with
|
||||
`if hmac.compare_digest(token, admin_key):`.
|
||||
|
||||
In `src/meshcore_hub/api/metrics.py`:
|
||||
1. Add `import hmac` at the top of the file.
|
||||
2. Line ~57: replace `return username == "metrics" and password == read_key` with
|
||||
`return hmac.compare_digest(username, "metrics") and hmac.compare_digest(password, read_key)`.
|
||||
|
||||
Note: `hmac.compare_digest()` requires both arguments to be strings (or both bytes). The existing code
|
||||
already works with strings, so no type conversion is needed.
|
||||
requirements:
|
||||
- "REQ-002"
|
||||
- "REQ-007"
|
||||
dependencies: []
|
||||
suggested_role: "python"
|
||||
acceptance_criteria:
|
||||
- "All API key comparisons in `api/auth.py` use `hmac.compare_digest()`"
|
||||
- "All credential comparisons in `api/metrics.py` use `hmac.compare_digest()`"
|
||||
- "`hmac` is imported in both files"
|
||||
- "Valid API keys are accepted and invalid keys are rejected (no behavior change)"
|
||||
estimated_complexity: "small"
|
||||
files_affected:
|
||||
- "src/meshcore_hub/api/auth.py"
|
||||
- "src/meshcore_hub/api/metrics.py"
|
||||
|
||||
- id: "TASK-003"
|
||||
title: "Add WEB_TRUSTED_PROXY_HOSTS configuration setting"
|
||||
description: |
|
||||
Add a `web_trusted_proxy_hosts` field to the web settings in `src/meshcore_hub/common/config.py`.
|
||||
|
||||
1. In the `WebSettings` class (or the relevant settings class containing web config), add:
|
||||
```python
|
||||
web_trusted_proxy_hosts: str = Field(default="*", description="Comma-separated list of trusted proxy hosts or '*' for all")
|
||||
```
|
||||
2. The field should accept a string value. The `ProxyHeadersMiddleware` in uvicorn accepts either `"*"` or a list of strings.
|
||||
If the value is `"*"`, pass it directly. Otherwise, split on commas and strip whitespace to produce a list.
|
||||
|
||||
This task only adds the configuration field. The middleware integration and startup warning are in TASK-004.
|
||||
requirements:
|
||||
- "REQ-003"
|
||||
- "REQ-006"
|
||||
dependencies: []
|
||||
suggested_role: "python"
|
||||
acceptance_criteria:
|
||||
- "A `web_trusted_proxy_hosts` setting exists in the configuration with default value `*`"
|
||||
- "The setting can be configured via the `WEB_TRUSTED_PROXY_HOSTS` environment variable"
|
||||
- "The setting accepts `*` or a comma-separated list of hostnames/IPs"
|
||||
estimated_complexity: "small"
|
||||
files_affected:
|
||||
- "src/meshcore_hub/common/config.py"
|
||||
|
||||
- id: "TASK-004"
|
||||
title: "Integrate trusted proxy hosts into web app middleware and add startup warning"
|
||||
description: |
|
||||
Update `src/meshcore_hub/web/app.py` to use the new `WEB_TRUSTED_PROXY_HOSTS` setting and emit a
|
||||
startup warning when using the insecure default.
|
||||
|
||||
1. Find the `ProxyHeadersMiddleware` addition (line ~239):
|
||||
```python
|
||||
app.add_middleware(ProxyHeadersMiddleware, trusted_hosts="*")
|
||||
```
|
||||
Replace the hardcoded `"*"` with the configured value. If the config value is `"*"`, pass `"*"`.
|
||||
Otherwise, split the comma-separated string into a list of strings.
|
||||
|
||||
2. Add a startup warning (in the app factory or lifespan) when `WEB_ADMIN_ENABLED=true` and
|
||||
`WEB_TRUSTED_PROXY_HOSTS` is `"*"`:
|
||||
```python
|
||||
import logging
|
||||
logger = logging.getLogger(__name__)
|
||||
if settings.web_admin_enabled and settings.web_trusted_proxy_hosts == "*":
|
||||
logger.warning(
|
||||
"WEB_ADMIN_ENABLED is true but WEB_TRUSTED_PROXY_HOSTS is '*' (trust all). "
|
||||
"Consider restricting to your reverse proxy IP for production deployments."
|
||||
)
|
||||
```
|
||||
|
||||
3. Verify that the `_is_authenticated_proxy_request` function still accepts `X-Forwarded-User`,
|
||||
`X-Auth-Request-User`, and `Authorization: Basic` headers — do not modify that function.
|
||||
requirements:
|
||||
- "REQ-003"
|
||||
- "REQ-006"
|
||||
- "REQ-007"
|
||||
dependencies:
|
||||
- "TASK-003"
|
||||
suggested_role: "python"
|
||||
acceptance_criteria:
|
||||
- "`ProxyHeadersMiddleware` uses the configured `trusted_hosts` value instead of hardcoded `*`"
|
||||
- "A warning is logged at startup when admin is enabled and trusted hosts is `*`"
|
||||
- "The warning recommends restricting trusted hosts to the proxy IP"
|
||||
- "`_is_authenticated_proxy_request` still accepts all three header types"
|
||||
- "Setting `WEB_TRUSTED_PROXY_HOSTS` to a specific IP restricts proxy header trust"
|
||||
estimated_complexity: "medium"
|
||||
files_affected:
|
||||
- "src/meshcore_hub/web/app.py"
|
||||
|
||||
- id: "TASK-005"
|
||||
title: "Escape config JSON in template script block to prevent XSS breakout"
|
||||
description: |
|
||||
Prevent XSS via `</script>` breakout in the config JSON template injection in `src/meshcore_hub/web/app.py`.
|
||||
|
||||
In the `_build_config_json` function (or wherever `config_json` is prepared for the template, around
|
||||
line 183), after calling `json.dumps(config)`, escape `</` sequences:
|
||||
```python
|
||||
config_json = json.dumps(config).replace("</", "<\\/")
|
||||
```
|
||||
|
||||
This prevents a config value containing `</script><script>alert(1)</script>` from breaking out of the
|
||||
`<script>` block in `spa.html` (line ~188: `window.__APP_CONFIG__ = {{ config_json|safe }};`).
|
||||
|
||||
The `|safe` filter in the template remains unchanged — the escaping happens in Python before the value
|
||||
reaches Jinja2. The SPA client-side JavaScript can parse JSON containing `<\/` sequences because this
|
||||
is valid JSON per the spec.
|
||||
requirements:
|
||||
- "REQ-004"
|
||||
- "REQ-006"
|
||||
dependencies: []
|
||||
suggested_role: "python"
|
||||
acceptance_criteria:
|
||||
- "`config_json` is escaped by replacing `</` with `<\\/` before template rendering"
|
||||
- "The `|safe` filter continues to be used in the template"
|
||||
- "A config value containing `</script><script>alert(1)</script>` does not execute JavaScript"
|
||||
- "The SPA application correctly parses the escaped config JSON"
|
||||
- "Normal config values without special characters render unchanged"
|
||||
estimated_complexity: "small"
|
||||
files_affected:
|
||||
- "src/meshcore_hub/web/app.py"
|
||||
|
||||
- id: "TASK-006"
|
||||
title: "Fix stored XSS in admin node-tags page"
|
||||
description: |
|
||||
Sanitize API-sourced data in `src/meshcore_hub/web/static/js/spa/pages/admin/node-tags.js` to prevent
|
||||
stored XSS.
|
||||
|
||||
Three locations need fixing:
|
||||
|
||||
1. **Line ~243** — `unsafeHTML()` with nodeName in copy-all confirmation:
|
||||
```javascript
|
||||
<p class="mb-4">${unsafeHTML(t('common.copy_all_entity_description', { count: tags.length, entity: t('entities.tags').toLowerCase(), name: nodeName }))}</p>
|
||||
```
|
||||
Replace `unsafeHTML()` with safe rendering. Either escape `nodeName` with `escapeHtml()` before
|
||||
passing to `t()`, or use `textContent`-based rendering.
|
||||
|
||||
2. **Line ~272** — `unsafeHTML()` with nodeName in delete-all confirmation:
|
||||
```javascript
|
||||
<p class="mb-4">${unsafeHTML(t('common.delete_all_entity_confirm', { count: tags.length, entity: t('entities.tags').toLowerCase(), name: nodeName }))}</p>
|
||||
```
|
||||
Same fix as above.
|
||||
|
||||
3. **Line ~454** — `innerHTML` with tag key in delete confirmation:
|
||||
```javascript
|
||||
container.querySelector('#delete_tag_confirm_message').innerHTML = confirmMsg;
|
||||
```
|
||||
where `confirmMsg` is built with `activeTagKey` interpolated into an HTML span. Replace `innerHTML`
|
||||
with `textContent`, or escape `activeTagKey` with `escapeHtml()` before interpolation.
|
||||
|
||||
Import `escapeHtml` from `../components.js` if not already imported. The function escapes `<`, `>`,
|
||||
`&`, `"`, and `'` characters using DOM textContent.
|
||||
requirements:
|
||||
- "REQ-005"
|
||||
- "REQ-006"
|
||||
dependencies: []
|
||||
suggested_role: "frontend"
|
||||
acceptance_criteria:
|
||||
- "Node names in node-tags.js are escaped before HTML rendering"
|
||||
- "Tag keys in node-tags.js are escaped before HTML rendering"
|
||||
- "All `unsafeHTML()` calls on API-sourced data are replaced with safe alternatives"
|
||||
- "All `innerHTML` assignments of API-sourced data are replaced with safe alternatives"
|
||||
- "A node name containing `<img src=x onerror=alert(1)>` renders as text"
|
||||
- "Normal names without special characters display correctly"
|
||||
estimated_complexity: "medium"
|
||||
files_affected:
|
||||
- "src/meshcore_hub/web/static/js/spa/pages/admin/node-tags.js"
|
||||
|
||||
- id: "TASK-007"
|
||||
title: "Fix stored XSS in admin members page"
|
||||
description: |
|
||||
Sanitize API-sourced data in `src/meshcore_hub/web/static/js/spa/pages/admin/members.js` to prevent
|
||||
stored XSS.
|
||||
|
||||
**Line ~309** — `innerHTML` with memberName in delete confirmation:
|
||||
```javascript
|
||||
container.querySelector('#delete_confirm_message').innerHTML = confirmMsg;
|
||||
```
|
||||
where `confirmMsg` is built from `t('common.delete_entity_confirm', { entity: ..., name: memberName })`.
|
||||
`memberName` comes from `row.dataset.memberName` which is API-sourced data.
|
||||
|
||||
Fix by escaping `memberName` with `escapeHtml()` before passing to `t()`, or replace `innerHTML` with
|
||||
`textContent`.
|
||||
|
||||
Import `escapeHtml` from `../components.js` if not already imported.
|
||||
requirements:
|
||||
- "REQ-005"
|
||||
- "REQ-006"
|
||||
dependencies: []
|
||||
suggested_role: "frontend"
|
||||
acceptance_criteria:
|
||||
- "Member names in members.js are escaped before HTML rendering"
|
||||
- "The `innerHTML` assignment of API-sourced data is replaced with a safe alternative"
|
||||
- "A member name containing `<script>alert(1)</script>` renders as text"
|
||||
- "Normal member names display correctly"
|
||||
estimated_complexity: "small"
|
||||
files_affected:
|
||||
- "src/meshcore_hub/web/static/js/spa/pages/admin/members.js"
|
||||
|
||||
- id: "TASK-008"
|
||||
title: "Write tests for legacy dashboard endpoint removal"
|
||||
description: |
|
||||
Add or update tests in `tests/test_api/` to verify that the legacy HTML dashboard endpoint is removed
|
||||
while JSON sub-routes remain functional.
|
||||
|
||||
Tests to add/update:
|
||||
1. `GET /api/v1/dashboard/` returns 404 or 405 (no longer serves HTML).
|
||||
2. `GET /api/v1/dashboard/stats` returns 200 with valid JSON when authenticated.
|
||||
3. `GET /api/v1/dashboard/activity` returns 200 with valid JSON when authenticated.
|
||||
4. `GET /api/v1/dashboard/message-activity` returns 200 with valid JSON when authenticated.
|
||||
5. `GET /api/v1/dashboard/node-count` returns 200 with valid JSON when authenticated.
|
||||
|
||||
Use the existing test fixtures and patterns from `tests/test_api/`. Check `tests/conftest.py` for
|
||||
available fixtures (test client, db session, auth headers).
|
||||
requirements:
|
||||
- "REQ-001"
|
||||
- "REQ-006"
|
||||
dependencies:
|
||||
- "TASK-001"
|
||||
suggested_role: "python"
|
||||
acceptance_criteria:
|
||||
- "Test confirms `GET /api/v1/dashboard/` returns 404 or 405"
|
||||
- "Tests confirm all four JSON sub-routes return valid JSON with authentication"
|
||||
- "All tests pass"
|
||||
estimated_complexity: "small"
|
||||
files_affected:
|
||||
- "tests/test_api/test_dashboard.py"
|
||||
|
||||
- id: "TASK-009"
|
||||
title: "Write tests for constant-time API key comparison"
|
||||
description: |
|
||||
Add or update tests in `tests/test_api/` to verify that authentication still works correctly after
|
||||
switching to `hmac.compare_digest()`.
|
||||
|
||||
Tests to add/update:
|
||||
1. Valid read key is accepted by read-protected endpoints.
|
||||
2. Valid admin key is accepted by admin-protected endpoints.
|
||||
3. Invalid keys are rejected with 401/403.
|
||||
4. Valid admin key also grants read access.
|
||||
5. Metrics endpoint accepts valid credentials and rejects invalid ones (if metrics auth is testable).
|
||||
|
||||
These tests verify no behavioral regression from the `==` to `hmac.compare_digest()` change.
|
||||
Use existing test patterns and fixtures from `tests/test_api/`.
|
||||
requirements:
|
||||
- "REQ-002"
|
||||
- "REQ-007"
|
||||
dependencies:
|
||||
- "TASK-002"
|
||||
suggested_role: "python"
|
||||
acceptance_criteria:
|
||||
- "Tests confirm valid read key is accepted"
|
||||
- "Tests confirm valid admin key is accepted"
|
||||
- "Tests confirm invalid keys are rejected"
|
||||
- "Tests confirm metrics auth works correctly"
|
||||
- "All tests pass"
|
||||
estimated_complexity: "small"
|
||||
files_affected:
|
||||
- "tests/test_api/test_auth.py"
|
||||
|
||||
- id: "TASK-010"
|
||||
title: "Write tests for trusted proxy hosts configuration and startup warning"
|
||||
description: |
|
||||
Add tests to verify the `WEB_TRUSTED_PROXY_HOSTS` configuration setting and the startup warning.
|
||||
|
||||
Tests to add:
|
||||
1. Default value of `WEB_TRUSTED_PROXY_HOSTS` is `*`.
|
||||
2. Setting `WEB_TRUSTED_PROXY_HOSTS` to a specific IP is correctly parsed.
|
||||
3. Setting `WEB_TRUSTED_PROXY_HOSTS` to a comma-separated list is correctly parsed into a list.
|
||||
4. A warning is logged when `WEB_ADMIN_ENABLED=true` and `WEB_TRUSTED_PROXY_HOSTS` is `*`.
|
||||
5. No warning is logged when `WEB_TRUSTED_PROXY_HOSTS` is set to a specific value.
|
||||
|
||||
Place config tests in `tests/test_common/` and web app tests in `tests/test_web/`.
|
||||
requirements:
|
||||
- "REQ-003"
|
||||
- "REQ-006"
|
||||
dependencies:
|
||||
- "TASK-003"
|
||||
- "TASK-004"
|
||||
suggested_role: "python"
|
||||
acceptance_criteria:
|
||||
- "Tests confirm default value is `*`"
|
||||
- "Tests confirm specific IP/list parsing works"
|
||||
- "Tests confirm startup warning is emitted with wildcard default"
|
||||
- "Tests confirm no warning when specific hosts are configured"
|
||||
- "All tests pass"
|
||||
estimated_complexity: "medium"
|
||||
files_affected:
|
||||
- "tests/test_common/test_config.py"
|
||||
- "tests/test_web/test_app.py"
|
||||
|
||||
- id: "TASK-011"
|
||||
title: "Write tests for config JSON script block escaping"
|
||||
description: |
|
||||
Add tests in `tests/test_web/` to verify that the config JSON escaping prevents XSS breakout.
|
||||
|
||||
Tests to add:
|
||||
1. A config value containing `</script><script>alert(1)</script>` is escaped to `<\/script>...` in
|
||||
the rendered HTML.
|
||||
2. A config value without special characters renders unchanged.
|
||||
3. The escaped JSON is still valid and parseable by `json.loads()` (after un-escaping `<\/` back to `</`
|
||||
if needed, though `json.loads` handles `<\/` fine).
|
||||
|
||||
Test by calling the config JSON builder function directly or by checking the rendered template output.
|
||||
requirements:
|
||||
- "REQ-004"
|
||||
- "REQ-006"
|
||||
dependencies:
|
||||
- "TASK-005"
|
||||
suggested_role: "python"
|
||||
acceptance_criteria:
|
||||
- "Test confirms `</script>` in config values is escaped to `<\\/script>`"
|
||||
- "Test confirms normal config values are unaffected"
|
||||
- "Test confirms escaped JSON is still valid and parseable"
|
||||
- "All tests pass"
|
||||
estimated_complexity: "small"
|
||||
files_affected:
|
||||
- "tests/test_web/test_app.py"
|
||||
|
||||
- id: "TASK-012"
|
||||
title: "Update documentation for WEB_TRUSTED_PROXY_HOSTS setting"
|
||||
description: |
|
||||
Update project documentation to document the new `WEB_TRUSTED_PROXY_HOSTS` environment variable.
|
||||
|
||||
Files to update:
|
||||
|
||||
1. **README.md** — Add `WEB_TRUSTED_PROXY_HOSTS` to the environment variables table with description:
|
||||
"Comma-separated list of trusted proxy hosts for admin authentication headers. Default: `*` (all hosts).
|
||||
Recommended: set to your reverse proxy IP in production."
|
||||
|
||||
2. **AGENTS.md** — Add `WEB_TRUSTED_PROXY_HOSTS` to the Environment Variables section with the same description.
|
||||
|
||||
3. **PLAN.md** — If there is a configuration section, add the new variable there as well.
|
||||
|
||||
Ensure the documentation notes:
|
||||
- Default is `*` for backward compatibility
|
||||
- A startup warning is emitted when using the default with admin enabled
|
||||
- Operators should set this to their reverse proxy IP in production
|
||||
requirements:
|
||||
- "REQ-003"
|
||||
- "REQ-006"
|
||||
dependencies:
|
||||
- "TASK-003"
|
||||
- "TASK-004"
|
||||
suggested_role: "docs"
|
||||
acceptance_criteria:
|
||||
- "`WEB_TRUSTED_PROXY_HOSTS` is documented in README.md"
|
||||
- "`WEB_TRUSTED_PROXY_HOSTS` is documented in AGENTS.md"
|
||||
- "Documentation notes the default value, startup warning, and production recommendation"
|
||||
estimated_complexity: "small"
|
||||
files_affected:
|
||||
- "README.md"
|
||||
- "AGENTS.md"
|
||||
- "PLAN.md"
|
||||
@@ -612,6 +612,7 @@ Key variables:
|
||||
- `MQTT_TLS` - Enable TLS/SSL for MQTT (default: `false`)
|
||||
- `API_READ_KEY`, `API_ADMIN_KEY` - API authentication keys
|
||||
- `WEB_ADMIN_ENABLED` - Enable admin interface at /a/ (default: `false`, requires auth proxy)
|
||||
- `WEB_TRUSTED_PROXY_HOSTS` - Comma-separated list of trusted proxy hosts for admin authentication headers. Default: `*` (all hosts). Recommended: set to your reverse proxy IP in production. A startup warning is emitted when using the default `*` with admin enabled.
|
||||
- `WEB_THEME` - Default theme for the web dashboard (default: `dark`, options: `dark`, `light`). Users can override via the theme toggle in the navbar, which persists their preference in browser localStorage.
|
||||
- `WEB_AUTO_REFRESH_SECONDS` - Auto-refresh interval in seconds for list pages (default: `30`, `0` to disable)
|
||||
- `TZ` - Timezone for web dashboard date/time display (default: `UTC`, e.g., `America/New_York`, `Europe/London`)
|
||||
|
||||
1
PLAN.md
1
PLAN.md
@@ -516,6 +516,7 @@ LetsMesh compatibility parity note:
|
||||
| WEB_PORT | 8080 | Web bind port |
|
||||
| API_BASE_URL | http://localhost:8000 | API endpoint |
|
||||
| API_KEY | | API key for queries |
|
||||
| WEB_TRUSTED_PROXY_HOSTS | * | Comma-separated list of trusted proxy hosts for admin authentication headers. Default: `*` (all hosts). Recommended: set to your reverse proxy IP in production. |
|
||||
| WEB_LOCALE | en | UI translation locale |
|
||||
| WEB_DATETIME_LOCALE | en-US | Date formatting locale for UI timestamps |
|
||||
| TZ | UTC | Timezone used for UI timestamp rendering |
|
||||
|
||||
@@ -394,6 +394,7 @@ The collector automatically cleans up old event data and inactive nodes:
|
||||
| `WEB_DATETIME_LOCALE` | `en-US` | Locale used for date formatting in the web dashboard (e.g., `en-US` for MM/DD/YYYY, `en-GB` for DD/MM/YYYY). |
|
||||
| `WEB_AUTO_REFRESH_SECONDS` | `30` | Auto-refresh interval in seconds for list pages (0 to disable) |
|
||||
| `WEB_ADMIN_ENABLED` | `false` | Enable admin interface at /a/ (requires auth proxy: `X-Forwarded-User`/`X-Auth-Request-User` or forwarded `Authorization: Basic ...`) |
|
||||
| `WEB_TRUSTED_PROXY_HOSTS` | `*` | Comma-separated list of trusted proxy hosts for admin authentication headers. Default: `*` (all hosts). Recommended: set to your reverse proxy IP in production. A startup warning is emitted when using the default `*` with admin enabled. |
|
||||
| `TZ` | `UTC` | Timezone for displaying dates/times (e.g., `America/New_York`, `Europe/London`) |
|
||||
| `NETWORK_DOMAIN` | *(none)* | Network domain name (optional) |
|
||||
| `NETWORK_NAME` | `MeshCore Network` | Display name for the network |
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
"""Authentication middleware for the API."""
|
||||
|
||||
import hmac
|
||||
import logging
|
||||
from typing import Annotated
|
||||
|
||||
@@ -79,7 +80,9 @@ async def require_read(
|
||||
)
|
||||
|
||||
# Check if token matches any key
|
||||
if token == read_key or token == admin_key:
|
||||
if (read_key and hmac.compare_digest(token, read_key)) or (
|
||||
admin_key and hmac.compare_digest(token, admin_key)
|
||||
):
|
||||
return token
|
||||
|
||||
raise HTTPException(
|
||||
@@ -124,7 +127,7 @@ async def require_admin(
|
||||
)
|
||||
|
||||
# Check if token matches admin key
|
||||
if token == admin_key:
|
||||
if hmac.compare_digest(token, admin_key):
|
||||
return token
|
||||
|
||||
raise HTTPException(
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
"""Prometheus metrics endpoint for MeshCore Hub API."""
|
||||
|
||||
import base64
|
||||
import hmac
|
||||
import logging
|
||||
import time
|
||||
from typing import Any
|
||||
@@ -54,7 +55,9 @@ def verify_basic_auth(request: Request) -> bool:
|
||||
try:
|
||||
decoded = base64.b64decode(auth_header[6:]).decode("utf-8")
|
||||
username, password = decoded.split(":", 1)
|
||||
return username == "metrics" and password == read_key
|
||||
return hmac.compare_digest(username, "metrics") and hmac.compare_digest(
|
||||
password, read_key
|
||||
)
|
||||
except Exception:
|
||||
return False
|
||||
|
||||
|
||||
@@ -2,8 +2,7 @@
|
||||
|
||||
from datetime import datetime, timedelta, timezone
|
||||
|
||||
from fastapi import APIRouter, Request
|
||||
from fastapi.responses import HTMLResponse
|
||||
from fastapi import APIRouter
|
||||
from sqlalchemy import func, select
|
||||
|
||||
from meshcore_hub.api.auth import RequireRead
|
||||
@@ -362,175 +361,3 @@ async def get_node_count_history(
|
||||
data.append(DailyActivityPoint(date=date_str, count=count))
|
||||
|
||||
return NodeCountHistory(days=days, data=data)
|
||||
|
||||
|
||||
@router.get("/", response_class=HTMLResponse)
|
||||
async def dashboard(
|
||||
request: Request,
|
||||
session: DbSession,
|
||||
) -> HTMLResponse:
|
||||
"""Simple HTML dashboard page."""
|
||||
now = datetime.now(timezone.utc)
|
||||
today_start = now.replace(hour=0, minute=0, second=0, microsecond=0)
|
||||
yesterday = now - timedelta(days=1)
|
||||
|
||||
# Get stats
|
||||
total_nodes = session.execute(select(func.count()).select_from(Node)).scalar() or 0
|
||||
|
||||
active_nodes = (
|
||||
session.execute(
|
||||
select(func.count()).select_from(Node).where(Node.last_seen >= yesterday)
|
||||
).scalar()
|
||||
or 0
|
||||
)
|
||||
|
||||
total_messages = (
|
||||
session.execute(select(func.count()).select_from(Message)).scalar() or 0
|
||||
)
|
||||
|
||||
messages_today = (
|
||||
session.execute(
|
||||
select(func.count())
|
||||
.select_from(Message)
|
||||
.where(Message.received_at >= today_start)
|
||||
).scalar()
|
||||
or 0
|
||||
)
|
||||
|
||||
# Get recent nodes
|
||||
recent_nodes = (
|
||||
session.execute(select(Node).order_by(Node.last_seen.desc()).limit(10))
|
||||
.scalars()
|
||||
.all()
|
||||
)
|
||||
|
||||
# Get recent messages
|
||||
recent_messages = (
|
||||
session.execute(select(Message).order_by(Message.received_at.desc()).limit(10))
|
||||
.scalars()
|
||||
.all()
|
||||
)
|
||||
|
||||
# Build HTML
|
||||
html = f"""
|
||||
<!DOCTYPE html>
|
||||
<html>
|
||||
<head>
|
||||
<title>MeshCore Hub Dashboard</title>
|
||||
<meta charset="utf-8">
|
||||
<meta name="viewport" content="width=device-width, initial-scale=1">
|
||||
<meta http-equiv="refresh" content="30">
|
||||
<style>
|
||||
body {{
|
||||
font-family: -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, sans-serif;
|
||||
margin: 0;
|
||||
padding: 20px;
|
||||
background: #f5f5f5;
|
||||
color: #333;
|
||||
}}
|
||||
h1 {{ color: #2c3e50; }}
|
||||
.container {{ max-width: 1200px; margin: 0 auto; }}
|
||||
.stats {{
|
||||
display: grid;
|
||||
grid-template-columns: repeat(auto-fit, minmax(200px, 1fr));
|
||||
gap: 20px;
|
||||
margin-bottom: 30px;
|
||||
}}
|
||||
.stat-card {{
|
||||
background: white;
|
||||
padding: 20px;
|
||||
border-radius: 8px;
|
||||
box-shadow: 0 2px 4px rgba(0,0,0,0.1);
|
||||
}}
|
||||
.stat-card h3 {{ margin: 0 0 10px 0; color: #666; font-size: 14px; }}
|
||||
.stat-card .value {{ font-size: 32px; font-weight: bold; color: #2c3e50; }}
|
||||
.section {{
|
||||
background: white;
|
||||
padding: 20px;
|
||||
border-radius: 8px;
|
||||
box-shadow: 0 2px 4px rgba(0,0,0,0.1);
|
||||
margin-bottom: 20px;
|
||||
}}
|
||||
table {{ width: 100%; border-collapse: collapse; }}
|
||||
th, td {{ padding: 10px; text-align: left; border-bottom: 1px solid #eee; }}
|
||||
th {{ background: #f8f9fa; font-weight: 600; }}
|
||||
.text-muted {{ color: #666; }}
|
||||
.truncate {{ max-width: 200px; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; }}
|
||||
</style>
|
||||
</head>
|
||||
<body>
|
||||
<div class="container">
|
||||
<h1>MeshCore Hub Dashboard</h1>
|
||||
<p class="text-muted">Last updated: {now.strftime('%Y-%m-%d %H:%M:%S UTC')}</p>
|
||||
|
||||
<div class="stats">
|
||||
<div class="stat-card">
|
||||
<h3>Total Nodes</h3>
|
||||
<div class="value">{total_nodes}</div>
|
||||
</div>
|
||||
<div class="stat-card">
|
||||
<h3>Active Nodes (24h)</h3>
|
||||
<div class="value">{active_nodes}</div>
|
||||
</div>
|
||||
<div class="stat-card">
|
||||
<h3>Total Messages</h3>
|
||||
<div class="value">{total_messages}</div>
|
||||
</div>
|
||||
<div class="stat-card">
|
||||
<h3>Messages Today</h3>
|
||||
<div class="value">{messages_today}</div>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
<div class="section">
|
||||
<h2>Recent Nodes</h2>
|
||||
<table>
|
||||
<thead>
|
||||
<tr>
|
||||
<th>Name</th>
|
||||
<th>Public Key</th>
|
||||
<th>Type</th>
|
||||
<th>Last Seen</th>
|
||||
</tr>
|
||||
</thead>
|
||||
<tbody>
|
||||
{"".join(f'''
|
||||
<tr>
|
||||
<td>{n.name or '-'}</td>
|
||||
<td class="truncate">{n.public_key[:16]}...</td>
|
||||
<td>{n.adv_type or '-'}</td>
|
||||
<td>{n.last_seen.strftime('%Y-%m-%d %H:%M') if n.last_seen else '-'}</td>
|
||||
</tr>
|
||||
''' for n in recent_nodes)}
|
||||
</tbody>
|
||||
</table>
|
||||
</div>
|
||||
|
||||
<div class="section">
|
||||
<h2>Recent Messages</h2>
|
||||
<table>
|
||||
<thead>
|
||||
<tr>
|
||||
<th>Type</th>
|
||||
<th>From/Channel</th>
|
||||
<th>Text</th>
|
||||
<th>Received</th>
|
||||
</tr>
|
||||
</thead>
|
||||
<tbody>
|
||||
{"".join(f'''
|
||||
<tr>
|
||||
<td>{m.message_type}</td>
|
||||
<td>{m.pubkey_prefix or f'Ch {m.channel_idx}' or '-'}</td>
|
||||
<td class="truncate">{m.text[:50]}{'...' if len(m.text) > 50 else ''}</td>
|
||||
<td>{m.received_at.strftime('%Y-%m-%d %H:%M') if m.received_at else '-'}</td>
|
||||
</tr>
|
||||
''' for m in recent_messages)}
|
||||
</tbody>
|
||||
</table>
|
||||
</div>
|
||||
</div>
|
||||
</body>
|
||||
</html>
|
||||
"""
|
||||
return HTMLResponse(content=html)
|
||||
|
||||
@@ -352,6 +352,12 @@ class WebSettings(CommonSettings):
|
||||
ge=0,
|
||||
)
|
||||
|
||||
# Trusted proxy hosts for X-Forwarded-For header processing
|
||||
web_trusted_proxy_hosts: str = Field(
|
||||
default="*",
|
||||
description="Comma-separated list of trusted proxy hosts or '*' for all",
|
||||
)
|
||||
|
||||
# Admin interface (disabled by default for security)
|
||||
web_admin_enabled: bool = Field(
|
||||
default=False,
|
||||
|
||||
@@ -180,7 +180,11 @@ def _build_config_json(app: FastAPI, request: Request) -> str:
|
||||
"logo_invert_light": app.state.logo_invert_light,
|
||||
}
|
||||
|
||||
return json.dumps(config)
|
||||
# Escape "</script>" sequences to prevent XSS breakout from the
|
||||
# <script> block where this JSON is embedded via |safe in the
|
||||
# Jinja2 template. "<\/" is valid JSON per the spec and parsed
|
||||
# correctly by JavaScript's JSON.parse().
|
||||
return json.dumps(config).replace("</", "<\\/")
|
||||
|
||||
|
||||
def create_app(
|
||||
@@ -236,7 +240,24 @@ def create_app(
|
||||
)
|
||||
|
||||
# Trust proxy headers (X-Forwarded-Proto, X-Forwarded-For) for HTTPS detection
|
||||
app.add_middleware(ProxyHeadersMiddleware, trusted_hosts="*")
|
||||
trusted_hosts_raw = settings.web_trusted_proxy_hosts
|
||||
if trusted_hosts_raw == "*":
|
||||
trusted_hosts: str | list[str] = "*"
|
||||
else:
|
||||
trusted_hosts = [h.strip() for h in trusted_hosts_raw.split(",") if h.strip()]
|
||||
app.add_middleware(ProxyHeadersMiddleware, trusted_hosts=trusted_hosts)
|
||||
|
||||
# Compute effective admin flag (parameter overrides setting)
|
||||
effective_admin = (
|
||||
admin_enabled if admin_enabled is not None else settings.web_admin_enabled
|
||||
)
|
||||
|
||||
# Warn when admin is enabled but proxy trust is wide open
|
||||
if effective_admin and settings.web_trusted_proxy_hosts == "*":
|
||||
logger.warning(
|
||||
"WEB_ADMIN_ENABLED is true but WEB_TRUSTED_PROXY_HOSTS is '*' (trust all). "
|
||||
"Consider restricting to your reverse proxy IP for production deployments."
|
||||
)
|
||||
|
||||
# Add cache control headers based on resource type
|
||||
app.add_middleware(CacheControlMiddleware)
|
||||
@@ -256,9 +277,7 @@ def create_app(
|
||||
)
|
||||
app.state.api_url = api_url or settings.api_base_url
|
||||
app.state.api_key = api_key or settings.api_key
|
||||
app.state.admin_enabled = (
|
||||
admin_enabled if admin_enabled is not None else settings.web_admin_enabled
|
||||
)
|
||||
app.state.admin_enabled = effective_admin
|
||||
app.state.network_name = network_name or settings.network_name
|
||||
app.state.network_city = network_city or settings.network_city
|
||||
app.state.network_country = network_country or settings.network_country
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
import { apiGet, apiPost, apiPut, apiDelete } from '../../api.js';
|
||||
import {
|
||||
html, litRender, nothing,
|
||||
getConfig, errorAlert, successAlert, t,
|
||||
getConfig, errorAlert, successAlert, t, escapeHtml,
|
||||
} from '../../components.js';
|
||||
import { iconLock } from '../../icons.js';
|
||||
|
||||
@@ -304,7 +304,7 @@ ${flashHtml}
|
||||
const memberName = row.dataset.memberName;
|
||||
const confirmMsg = t('common.delete_entity_confirm', {
|
||||
entity: t('entities.member').toLowerCase(),
|
||||
name: memberName
|
||||
name: escapeHtml(memberName)
|
||||
});
|
||||
container.querySelector('#delete_confirm_message').innerHTML = confirmMsg;
|
||||
container.querySelector('#deleteModal').showModal();
|
||||
|
||||
@@ -2,7 +2,7 @@ import { apiGet, apiPost, apiPut, apiDelete } from '../../api.js';
|
||||
import {
|
||||
html, litRender, nothing, unsafeHTML,
|
||||
getConfig, typeEmoji, formatDateTimeShort, errorAlert,
|
||||
successAlert, truncateKey, t,
|
||||
successAlert, truncateKey, t, escapeHtml,
|
||||
} from '../../components.js';
|
||||
import { iconTag, iconLock } from '../../icons.js';
|
||||
|
||||
@@ -240,7 +240,8 @@ export async function render(container, params, router) {
|
||||
<div class="modal-box">
|
||||
<h3 class="font-bold text-lg">${t('common.copy_all_entity_to_another_node', { entity: t('entities.tags') })}</h3>
|
||||
<form id="copy-all-form" class="py-4">
|
||||
<p class="mb-4">${unsafeHTML(t('common.copy_all_entity_description', { count: tags.length, entity: t('entities.tags').toLowerCase(), name: nodeName }))}</p>
|
||||
<!-- unsafeHTML needed for translation HTML tags; nodeName is pre-escaped -->
|
||||
<p class="mb-4">${unsafeHTML(t('common.copy_all_entity_description', { count: tags.length, entity: t('entities.tags').toLowerCase(), name: escapeHtml(nodeName) }))}</p>
|
||||
<div class="form-control mb-4">
|
||||
<label class="label"><span class="label-text">${t('admin_node_tags.destination_node')}</span></label>
|
||||
<select id="copyAllDestination" class="select select-bordered w-full" required>
|
||||
@@ -269,7 +270,8 @@ export async function render(container, params, router) {
|
||||
<div class="modal-box">
|
||||
<h3 class="font-bold text-lg">${t('common.delete_all_entity', { entity: t('entities.tags') })}</h3>
|
||||
<div class="py-4">
|
||||
<p class="mb-4">${unsafeHTML(t('common.delete_all_entity_confirm', { count: tags.length, entity: t('entities.tags').toLowerCase(), name: nodeName }))}</p>
|
||||
<!-- unsafeHTML needed for translation HTML tags; nodeName is pre-escaped -->
|
||||
<p class="mb-4">${unsafeHTML(t('common.delete_all_entity_confirm', { count: tags.length, entity: t('entities.tags').toLowerCase(), name: escapeHtml(nodeName) }))}</p>
|
||||
<div class="alert alert-error mb-4">
|
||||
<svg xmlns="http://www.w3.org/2000/svg" class="stroke-current shrink-0 h-6 w-6" fill="none" viewBox="0 0 24 24"><path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M12 9v2m0 4h.01m-6.938 4h13.856c1.54 0 2.502-1.667 1.732-3L13.732 4c-.77-1.333-2.694-1.333-3.464 0L3.34 16c-.77 1.333.192 3 1.732 3z" /></svg>
|
||||
<span>${t('admin_node_tags.delete_all_warning')}</span>
|
||||
@@ -449,7 +451,7 @@ ${contentHtml}`, container);
|
||||
activeTagKey = row.dataset.tagKey;
|
||||
const confirmMsg = t('common.delete_entity_confirm', {
|
||||
entity: t('entities.tag').toLowerCase(),
|
||||
name: `"<span class="font-mono font-semibold">${activeTagKey}</span>"`
|
||||
name: `"<span class="font-mono font-semibold">${escapeHtml(activeTagKey)}</span>"`
|
||||
});
|
||||
container.querySelector('#delete_tag_confirm_message').innerHTML = confirmMsg;
|
||||
container.querySelector('#deleteModal').showModal();
|
||||
|
||||
@@ -1,8 +1,28 @@
|
||||
"""Tests for API authentication."""
|
||||
"""Tests for API authentication.
|
||||
|
||||
Verifies that constant-time key comparison (hmac.compare_digest) works
|
||||
correctly with no behavioral regressions from the original == operator.
|
||||
"""
|
||||
|
||||
import base64
|
||||
|
||||
|
||||
class TestAuthenticationFlow:
|
||||
"""Tests for authentication behavior."""
|
||||
def _make_basic_auth(username: str, password: str) -> str:
|
||||
"""Create a Basic auth header value."""
|
||||
credentials = base64.b64encode(f"{username}:{password}".encode()).decode()
|
||||
return f"Basic {credentials}"
|
||||
|
||||
|
||||
def _clear_metrics_cache() -> None:
|
||||
"""Clear the metrics module cache."""
|
||||
from meshcore_hub.api.metrics import _cache
|
||||
|
||||
_cache["output"] = b""
|
||||
_cache["expires_at"] = 0.0
|
||||
|
||||
|
||||
class TestReadAuthentication:
|
||||
"""Tests for read-level authentication (require_read)."""
|
||||
|
||||
def test_no_auth_when_keys_not_configured(self, client_no_auth):
|
||||
"""Test that no auth is required when keys are not configured."""
|
||||
@@ -30,46 +50,47 @@ class TestAuthenticationFlow:
|
||||
)
|
||||
assert response.status_code == 200
|
||||
|
||||
def test_read_key_accepted_on_multiple_endpoints(self, client_with_auth):
|
||||
"""Test that read key is accepted across different read endpoints."""
|
||||
for endpoint in ["/api/v1/nodes", "/api/v1/messages"]:
|
||||
response = client_with_auth.get(
|
||||
endpoint,
|
||||
headers={"Authorization": "Bearer test-read-key"},
|
||||
)
|
||||
assert response.status_code == 200, f"Read key rejected on {endpoint}"
|
||||
|
||||
def test_read_endpoints_accept_admin_key(self, client_with_auth):
|
||||
"""Test that read endpoints accept admin key."""
|
||||
"""Test that admin key also grants read access."""
|
||||
response = client_with_auth.get(
|
||||
"/api/v1/nodes",
|
||||
headers={"Authorization": "Bearer test-admin-key"},
|
||||
)
|
||||
assert response.status_code == 200
|
||||
|
||||
def test_admin_endpoints_reject_read_key(self, client_with_auth):
|
||||
"""Test that admin endpoints reject read key."""
|
||||
response = client_with_auth.post(
|
||||
"/api/v1/commands/send-message",
|
||||
json={
|
||||
"destination": "abc123def456abc123def456abc123de",
|
||||
"text": "Test",
|
||||
},
|
||||
headers={"Authorization": "Bearer test-read-key"},
|
||||
)
|
||||
assert response.status_code == 403
|
||||
def test_admin_key_grants_read_on_multiple_endpoints(self, client_with_auth):
|
||||
"""Test that admin key grants read access across different endpoints."""
|
||||
for endpoint in ["/api/v1/nodes", "/api/v1/messages"]:
|
||||
response = client_with_auth.get(
|
||||
endpoint,
|
||||
headers={"Authorization": "Bearer test-admin-key"},
|
||||
)
|
||||
assert (
|
||||
response.status_code == 200
|
||||
), f"Admin key rejected on read endpoint {endpoint}"
|
||||
|
||||
def test_admin_endpoints_accept_admin_key(self, client_with_auth):
|
||||
"""Test that admin endpoints accept admin key."""
|
||||
response = client_with_auth.post(
|
||||
"/api/v1/commands/send-message",
|
||||
json={
|
||||
"destination": "abc123def456abc123def456abc123de",
|
||||
"text": "Test",
|
||||
},
|
||||
headers={"Authorization": "Bearer test-admin-key"},
|
||||
)
|
||||
assert response.status_code == 200
|
||||
|
||||
def test_invalid_key_rejected(self, client_with_auth):
|
||||
"""Test that invalid keys are rejected."""
|
||||
def test_invalid_key_rejected_on_read_endpoint(self, client_with_auth):
|
||||
"""Test that invalid keys are rejected with 401 on read endpoints."""
|
||||
response = client_with_auth.get(
|
||||
"/api/v1/nodes",
|
||||
headers={"Authorization": "Bearer invalid-key"},
|
||||
)
|
||||
assert response.status_code == 401
|
||||
|
||||
def test_no_auth_header_rejected_on_read_endpoint(self, client_with_auth):
|
||||
"""Test that missing auth header is rejected on read endpoints."""
|
||||
response = client_with_auth.get("/api/v1/nodes")
|
||||
assert response.status_code == 401
|
||||
|
||||
def test_missing_bearer_prefix_rejected(self, client_with_auth):
|
||||
"""Test that tokens without Bearer prefix are rejected."""
|
||||
response = client_with_auth.get(
|
||||
@@ -87,6 +108,124 @@ class TestAuthenticationFlow:
|
||||
assert response.status_code == 401
|
||||
|
||||
|
||||
class TestAdminAuthentication:
|
||||
"""Tests for admin-level authentication (require_admin)."""
|
||||
|
||||
def test_admin_endpoints_accept_admin_key(self, client_with_auth):
|
||||
"""Test that admin endpoints accept admin key."""
|
||||
response = client_with_auth.post(
|
||||
"/api/v1/commands/send-message",
|
||||
json={
|
||||
"destination": "abc123def456abc123def456abc123de",
|
||||
"text": "Test",
|
||||
},
|
||||
headers={"Authorization": "Bearer test-admin-key"},
|
||||
)
|
||||
assert response.status_code == 200
|
||||
|
||||
def test_admin_endpoints_reject_read_key(self, client_with_auth):
|
||||
"""Test that admin endpoints reject read key with 403."""
|
||||
response = client_with_auth.post(
|
||||
"/api/v1/commands/send-message",
|
||||
json={
|
||||
"destination": "abc123def456abc123def456abc123de",
|
||||
"text": "Test",
|
||||
},
|
||||
headers={"Authorization": "Bearer test-read-key"},
|
||||
)
|
||||
assert response.status_code == 403
|
||||
|
||||
def test_admin_endpoints_reject_invalid_key(self, client_with_auth):
|
||||
"""Test that admin endpoints reject invalid keys with 403."""
|
||||
response = client_with_auth.post(
|
||||
"/api/v1/commands/send-message",
|
||||
json={
|
||||
"destination": "abc123def456abc123def456abc123de",
|
||||
"text": "Test",
|
||||
},
|
||||
headers={"Authorization": "Bearer completely-wrong-key"},
|
||||
)
|
||||
assert response.status_code == 403
|
||||
|
||||
def test_admin_endpoints_reject_no_auth_header(self, client_with_auth):
|
||||
"""Test that admin endpoints reject missing auth header with 401."""
|
||||
response = client_with_auth.post(
|
||||
"/api/v1/commands/send-message",
|
||||
json={
|
||||
"destination": "abc123def456abc123def456abc123de",
|
||||
"text": "Test",
|
||||
},
|
||||
)
|
||||
assert response.status_code == 401
|
||||
|
||||
|
||||
class TestMetricsAuthentication:
|
||||
"""Tests for metrics endpoint authentication (Basic auth with hmac.compare_digest)."""
|
||||
|
||||
def test_metrics_no_auth_when_no_read_key(self, client_no_auth):
|
||||
"""Test that metrics requires no auth when no read key is configured."""
|
||||
_clear_metrics_cache()
|
||||
response = client_no_auth.get("/metrics")
|
||||
assert response.status_code == 200
|
||||
|
||||
def test_metrics_accepts_valid_basic_auth(self, client_with_auth):
|
||||
"""Test that metrics accepts correct Basic credentials."""
|
||||
_clear_metrics_cache()
|
||||
response = client_with_auth.get(
|
||||
"/metrics",
|
||||
headers={"Authorization": _make_basic_auth("metrics", "test-read-key")},
|
||||
)
|
||||
assert response.status_code == 200
|
||||
|
||||
def test_metrics_rejects_no_auth_when_key_set(self, client_with_auth):
|
||||
"""Test 401 when read key is set but no auth provided."""
|
||||
_clear_metrics_cache()
|
||||
response = client_with_auth.get("/metrics")
|
||||
assert response.status_code == 401
|
||||
assert "WWW-Authenticate" in response.headers
|
||||
|
||||
def test_metrics_rejects_wrong_password(self, client_with_auth):
|
||||
"""Test that metrics rejects incorrect password."""
|
||||
_clear_metrics_cache()
|
||||
response = client_with_auth.get(
|
||||
"/metrics",
|
||||
headers={"Authorization": _make_basic_auth("metrics", "wrong-key")},
|
||||
)
|
||||
assert response.status_code == 401
|
||||
|
||||
def test_metrics_rejects_wrong_username(self, client_with_auth):
|
||||
"""Test that metrics rejects incorrect username."""
|
||||
_clear_metrics_cache()
|
||||
response = client_with_auth.get(
|
||||
"/metrics",
|
||||
headers={"Authorization": _make_basic_auth("admin", "test-read-key")},
|
||||
)
|
||||
assert response.status_code == 401
|
||||
|
||||
def test_metrics_rejects_bearer_auth(self, client_with_auth):
|
||||
"""Test that Bearer auth does not work for metrics."""
|
||||
_clear_metrics_cache()
|
||||
response = client_with_auth.get(
|
||||
"/metrics",
|
||||
headers={"Authorization": "Bearer test-read-key"},
|
||||
)
|
||||
assert response.status_code == 401
|
||||
|
||||
def test_metrics_rejects_admin_key_as_password(self, client_with_auth):
|
||||
"""Test that admin key is not accepted as metrics password.
|
||||
|
||||
Metrics uses only the read key for Basic auth, not the admin key.
|
||||
"""
|
||||
_clear_metrics_cache()
|
||||
response = client_with_auth.get(
|
||||
"/metrics",
|
||||
headers={
|
||||
"Authorization": _make_basic_auth("metrics", "test-admin-key"),
|
||||
},
|
||||
)
|
||||
assert response.status_code == 401
|
||||
|
||||
|
||||
class TestHealthEndpoint:
|
||||
"""Tests for health check endpoint."""
|
||||
|
||||
|
||||
@@ -35,35 +35,71 @@ class TestDashboardStats:
|
||||
assert data["total_advertisements"] == 1
|
||||
|
||||
|
||||
class TestDashboardHtml:
|
||||
"""Tests for GET /dashboard endpoint."""
|
||||
class TestDashboardHtmlRemoved:
|
||||
"""Tests that legacy HTML dashboard endpoint has been removed."""
|
||||
|
||||
def test_dashboard_html_response(self, client_no_auth):
|
||||
"""Test dashboard returns HTML."""
|
||||
def test_dashboard_html_endpoint_removed(self, client_no_auth):
|
||||
"""Test that GET /dashboard no longer returns HTML (legacy endpoint removed)."""
|
||||
response = client_no_auth.get("/api/v1/dashboard")
|
||||
assert response.status_code == 200
|
||||
assert "text/html" in response.headers["content-type"]
|
||||
assert "<!DOCTYPE html>" in response.text
|
||||
assert "MeshCore Hub Dashboard" in response.text
|
||||
assert response.status_code in (404, 405)
|
||||
|
||||
def test_dashboard_contains_stats(
|
||||
self, client_no_auth, sample_node, sample_message
|
||||
):
|
||||
"""Test dashboard HTML contains stat values."""
|
||||
response = client_no_auth.get("/api/v1/dashboard")
|
||||
assert response.status_code == 200
|
||||
# Check that stats are present
|
||||
assert "Total Nodes" in response.text
|
||||
assert "Active Nodes" in response.text
|
||||
assert "Total Messages" in response.text
|
||||
def test_dashboard_html_endpoint_removed_trailing_slash(self, client_no_auth):
|
||||
"""Test that GET /dashboard/ also returns 404/405."""
|
||||
response = client_no_auth.get("/api/v1/dashboard/")
|
||||
assert response.status_code in (404, 405)
|
||||
|
||||
def test_dashboard_contains_recent_data(self, client_no_auth, sample_node):
|
||||
"""Test dashboard HTML contains recent nodes."""
|
||||
response = client_no_auth.get("/api/v1/dashboard")
|
||||
|
||||
class TestDashboardAuthenticatedJsonRoutes:
|
||||
"""Tests that dashboard JSON sub-routes return valid JSON with authentication."""
|
||||
|
||||
def test_stats_returns_json_when_authenticated(self, client_with_auth):
|
||||
"""Test GET /dashboard/stats returns 200 with valid JSON when authenticated."""
|
||||
response = client_with_auth.get(
|
||||
"/api/v1/dashboard/stats",
|
||||
headers={"Authorization": "Bearer test-read-key"},
|
||||
)
|
||||
assert response.status_code == 200
|
||||
assert "Recent Nodes" in response.text
|
||||
# The node name should appear in the table
|
||||
assert sample_node.name in response.text
|
||||
data = response.json()
|
||||
assert "total_nodes" in data
|
||||
assert "active_nodes" in data
|
||||
assert "total_messages" in data
|
||||
assert "total_advertisements" in data
|
||||
|
||||
def test_activity_returns_json_when_authenticated(self, client_with_auth):
|
||||
"""Test GET /dashboard/activity returns 200 with valid JSON when authenticated."""
|
||||
response = client_with_auth.get(
|
||||
"/api/v1/dashboard/activity",
|
||||
headers={"Authorization": "Bearer test-read-key"},
|
||||
)
|
||||
assert response.status_code == 200
|
||||
data = response.json()
|
||||
assert "days" in data
|
||||
assert "data" in data
|
||||
assert isinstance(data["data"], list)
|
||||
|
||||
def test_message_activity_returns_json_when_authenticated(self, client_with_auth):
|
||||
"""Test GET /dashboard/message-activity returns 200 with valid JSON when authenticated."""
|
||||
response = client_with_auth.get(
|
||||
"/api/v1/dashboard/message-activity",
|
||||
headers={"Authorization": "Bearer test-read-key"},
|
||||
)
|
||||
assert response.status_code == 200
|
||||
data = response.json()
|
||||
assert "days" in data
|
||||
assert "data" in data
|
||||
assert isinstance(data["data"], list)
|
||||
|
||||
def test_node_count_returns_json_when_authenticated(self, client_with_auth):
|
||||
"""Test GET /dashboard/node-count returns 200 with valid JSON when authenticated."""
|
||||
response = client_with_auth.get(
|
||||
"/api/v1/dashboard/node-count",
|
||||
headers={"Authorization": "Bearer test-read-key"},
|
||||
)
|
||||
assert response.status_code == 200
|
||||
data = response.json()
|
||||
assert "days" in data
|
||||
assert "data" in data
|
||||
assert isinstance(data["data"], list)
|
||||
|
||||
|
||||
class TestDashboardActivity:
|
||||
|
||||
279
tests/test_web/test_app.py
Normal file
279
tests/test_web/test_app.py
Normal file
@@ -0,0 +1,279 @@
|
||||
"""Tests for web app: config JSON escaping and trusted proxy hosts warnings."""
|
||||
|
||||
import json
|
||||
import logging
|
||||
from typing import Any
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
from fastapi.testclient import TestClient
|
||||
|
||||
from meshcore_hub.web.app import _build_config_json, create_app
|
||||
|
||||
from .conftest import ALL_FEATURES_ENABLED, MockHttpClient
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def xss_app(mock_http_client: MockHttpClient) -> Any:
|
||||
"""Create a web app with a network name containing a script injection payload."""
|
||||
app = create_app(
|
||||
api_url="http://localhost:8000",
|
||||
api_key="test-api-key",
|
||||
network_name="</script><script>alert(1)</script>",
|
||||
network_city="Test City",
|
||||
network_country="Test Country",
|
||||
network_radio_config="Test Radio Config",
|
||||
network_contact_email="test@example.com",
|
||||
features=ALL_FEATURES_ENABLED,
|
||||
)
|
||||
app.state.http_client = mock_http_client
|
||||
return app
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def xss_client(xss_app: Any, mock_http_client: MockHttpClient) -> TestClient:
|
||||
"""Create a test client whose network_name contains a script injection payload."""
|
||||
xss_app.state.http_client = mock_http_client
|
||||
return TestClient(xss_app, raise_server_exceptions=True)
|
||||
|
||||
|
||||
class TestConfigJsonXssEscaping:
|
||||
"""Tests that _build_config_json escapes </script> to prevent XSS breakout."""
|
||||
|
||||
def test_script_tag_escaped_in_rendered_html(self, xss_client: TestClient) -> None:
|
||||
"""Config value containing </script> is escaped to <\\/script> in the HTML."""
|
||||
response = xss_client.get("/")
|
||||
assert response.status_code == 200
|
||||
|
||||
html = response.text
|
||||
|
||||
# The literal "</script>" must NOT appear inside the config JSON block.
|
||||
# Find the config JSON assignment to isolate the embedded block.
|
||||
config_marker = "window.__APP_CONFIG__ = "
|
||||
start = html.find(config_marker)
|
||||
assert start != -1, "Config JSON block not found in rendered HTML"
|
||||
start += len(config_marker)
|
||||
end = html.find(";", start)
|
||||
config_block = html[start:end]
|
||||
|
||||
# The raw closing tag must be escaped
|
||||
assert "</script>" not in config_block
|
||||
assert "<\\/script>" in config_block
|
||||
|
||||
def test_normal_config_values_unaffected(self, client: TestClient) -> None:
|
||||
"""Config values without special characters render unchanged."""
|
||||
response = client.get("/")
|
||||
assert response.status_code == 200
|
||||
|
||||
html = response.text
|
||||
config_marker = "window.__APP_CONFIG__ = "
|
||||
start = html.find(config_marker)
|
||||
assert start != -1
|
||||
start += len(config_marker)
|
||||
end = html.find(";", start)
|
||||
config_block = html[start:end]
|
||||
|
||||
config = json.loads(config_block)
|
||||
assert config["network_name"] == "Test Network"
|
||||
assert config["network_city"] == "Test City"
|
||||
assert config["network_country"] == "Test Country"
|
||||
|
||||
def test_escaped_json_is_parseable(self, xss_client: TestClient) -> None:
|
||||
"""The escaped JSON is still valid and parseable by json.loads."""
|
||||
response = xss_client.get("/")
|
||||
assert response.status_code == 200
|
||||
|
||||
html = response.text
|
||||
config_marker = "window.__APP_CONFIG__ = "
|
||||
start = html.find(config_marker)
|
||||
assert start != -1
|
||||
start += len(config_marker)
|
||||
end = html.find(";", start)
|
||||
config_block = html[start:end]
|
||||
|
||||
# json.loads handles <\/ sequences correctly (they are valid JSON)
|
||||
config = json.loads(config_block)
|
||||
assert isinstance(config, dict)
|
||||
# The parsed value should contain the original unescaped string
|
||||
assert config["network_name"] == "</script><script>alert(1)</script>"
|
||||
|
||||
def test_build_config_json_direct_escaping(self, web_app: Any) -> None:
|
||||
"""Calling _build_config_json directly escapes </ sequences."""
|
||||
from starlette.requests import Request
|
||||
|
||||
# Inject a malicious value into the app state
|
||||
web_app.state.network_name = "</script><script>alert(1)</script>"
|
||||
|
||||
scope = {
|
||||
"type": "http",
|
||||
"method": "GET",
|
||||
"path": "/",
|
||||
"query_string": b"",
|
||||
"headers": [],
|
||||
}
|
||||
request = Request(scope)
|
||||
|
||||
result = _build_config_json(web_app, request)
|
||||
|
||||
# Raw output must not contain literal "</script>"
|
||||
assert "</script>" not in result
|
||||
assert "<\\/script>" in result
|
||||
|
||||
# Result must still be valid JSON
|
||||
parsed = json.loads(result)
|
||||
assert parsed["network_name"] == "</script><script>alert(1)</script>"
|
||||
|
||||
def test_build_config_json_no_escaping_needed(self, web_app: Any) -> None:
|
||||
"""_build_config_json leaves normal values intact when no </ present."""
|
||||
from starlette.requests import Request
|
||||
|
||||
scope = {
|
||||
"type": "http",
|
||||
"method": "GET",
|
||||
"path": "/",
|
||||
"query_string": b"",
|
||||
"headers": [],
|
||||
}
|
||||
request = Request(scope)
|
||||
|
||||
result = _build_config_json(web_app, request)
|
||||
|
||||
# No escaping artifacts for normal values
|
||||
assert "<\\/" not in result
|
||||
|
||||
parsed = json.loads(result)
|
||||
assert parsed["network_name"] == "Test Network"
|
||||
assert parsed["network_city"] == "Test City"
|
||||
|
||||
|
||||
class TestTrustedProxyHostsWarning:
|
||||
"""Tests for trusted proxy hosts startup warning in create_app."""
|
||||
|
||||
def test_warning_logged_when_admin_enabled_and_wildcard_hosts(
|
||||
self, caplog: pytest.LogCaptureFixture
|
||||
) -> None:
|
||||
"""A warning is logged when WEB_ADMIN_ENABLED=true and WEB_TRUSTED_PROXY_HOSTS is '*'."""
|
||||
with patch("meshcore_hub.common.config.get_web_settings") as mock_get_settings:
|
||||
from meshcore_hub.common.config import WebSettings
|
||||
|
||||
settings = WebSettings(
|
||||
_env_file=None,
|
||||
web_admin_enabled=True,
|
||||
web_trusted_proxy_hosts="*",
|
||||
)
|
||||
mock_get_settings.return_value = settings
|
||||
|
||||
with caplog.at_level(logging.WARNING, logger="meshcore_hub.web.app"):
|
||||
create_app(
|
||||
api_url="http://localhost:8000",
|
||||
admin_enabled=True,
|
||||
features=ALL_FEATURES_ENABLED,
|
||||
)
|
||||
|
||||
assert any(
|
||||
"WEB_ADMIN_ENABLED is true but WEB_TRUSTED_PROXY_HOSTS is '*'" in msg
|
||||
for msg in caplog.messages
|
||||
), f"Expected warning not found in log messages: {caplog.messages}"
|
||||
|
||||
def test_no_warning_when_trusted_proxy_hosts_is_specific(
|
||||
self, caplog: pytest.LogCaptureFixture
|
||||
) -> None:
|
||||
"""No warning is logged when WEB_TRUSTED_PROXY_HOSTS is set to a specific value."""
|
||||
with patch("meshcore_hub.common.config.get_web_settings") as mock_get_settings:
|
||||
from meshcore_hub.common.config import WebSettings
|
||||
|
||||
settings = WebSettings(
|
||||
_env_file=None,
|
||||
web_admin_enabled=True,
|
||||
web_trusted_proxy_hosts="10.0.0.1",
|
||||
)
|
||||
mock_get_settings.return_value = settings
|
||||
|
||||
with caplog.at_level(logging.WARNING, logger="meshcore_hub.web.app"):
|
||||
create_app(
|
||||
api_url="http://localhost:8000",
|
||||
admin_enabled=True,
|
||||
features=ALL_FEATURES_ENABLED,
|
||||
)
|
||||
|
||||
assert not any(
|
||||
"WEB_TRUSTED_PROXY_HOSTS" in msg for msg in caplog.messages
|
||||
), f"Unexpected warning found in log messages: {caplog.messages}"
|
||||
|
||||
def test_no_warning_when_admin_disabled(
|
||||
self, caplog: pytest.LogCaptureFixture
|
||||
) -> None:
|
||||
"""No warning is logged when WEB_ADMIN_ENABLED is false even with wildcard hosts."""
|
||||
with patch("meshcore_hub.common.config.get_web_settings") as mock_get_settings:
|
||||
from meshcore_hub.common.config import WebSettings
|
||||
|
||||
settings = WebSettings(
|
||||
_env_file=None,
|
||||
web_admin_enabled=False,
|
||||
web_trusted_proxy_hosts="*",
|
||||
)
|
||||
mock_get_settings.return_value = settings
|
||||
|
||||
with caplog.at_level(logging.WARNING, logger="meshcore_hub.web.app"):
|
||||
create_app(
|
||||
api_url="http://localhost:8000",
|
||||
features=ALL_FEATURES_ENABLED,
|
||||
)
|
||||
|
||||
assert not any(
|
||||
"WEB_TRUSTED_PROXY_HOSTS" in msg for msg in caplog.messages
|
||||
), f"Unexpected warning found in log messages: {caplog.messages}"
|
||||
|
||||
def test_proxy_hosts_comma_list_parsed_correctly(self) -> None:
|
||||
"""A comma-separated WEB_TRUSTED_PROXY_HOSTS is split into a list for middleware."""
|
||||
from uvicorn.middleware.proxy_headers import ProxyHeadersMiddleware
|
||||
|
||||
with patch("meshcore_hub.common.config.get_web_settings") as mock_get_settings:
|
||||
from meshcore_hub.common.config import WebSettings
|
||||
|
||||
settings = WebSettings(
|
||||
_env_file=None,
|
||||
web_trusted_proxy_hosts="10.0.0.1, 10.0.0.2, 172.16.0.1",
|
||||
)
|
||||
mock_get_settings.return_value = settings
|
||||
|
||||
app = create_app(
|
||||
api_url="http://localhost:8000",
|
||||
features=ALL_FEATURES_ENABLED,
|
||||
)
|
||||
|
||||
# Find the ProxyHeadersMiddleware entry in app.user_middleware
|
||||
proxy_entries = [
|
||||
m for m in app.user_middleware if m.cls is ProxyHeadersMiddleware
|
||||
]
|
||||
assert len(proxy_entries) == 1, "ProxyHeadersMiddleware not found in middleware"
|
||||
assert proxy_entries[0].kwargs["trusted_hosts"] == [
|
||||
"10.0.0.1",
|
||||
"10.0.0.2",
|
||||
"172.16.0.1",
|
||||
]
|
||||
|
||||
def test_wildcard_proxy_hosts_passed_as_string(self) -> None:
|
||||
"""Wildcard WEB_TRUSTED_PROXY_HOSTS='*' is passed as a string to middleware."""
|
||||
from uvicorn.middleware.proxy_headers import ProxyHeadersMiddleware
|
||||
|
||||
with patch("meshcore_hub.common.config.get_web_settings") as mock_get_settings:
|
||||
from meshcore_hub.common.config import WebSettings
|
||||
|
||||
settings = WebSettings(
|
||||
_env_file=None,
|
||||
web_trusted_proxy_hosts="*",
|
||||
)
|
||||
mock_get_settings.return_value = settings
|
||||
|
||||
app = create_app(
|
||||
api_url="http://localhost:8000",
|
||||
features=ALL_FEATURES_ENABLED,
|
||||
)
|
||||
|
||||
# Find the ProxyHeadersMiddleware entry in app.user_middleware
|
||||
proxy_entries = [
|
||||
m for m in app.user_middleware if m.cls is ProxyHeadersMiddleware
|
||||
]
|
||||
assert len(proxy_entries) == 1, "ProxyHeadersMiddleware not found in middleware"
|
||||
assert proxy_entries[0].kwargs["trusted_hosts"] == "*"
|
||||
Reference in New Issue
Block a user