mirror of
https://github.com/ipnet-mesh/meshcore-hub.git
synced 2026-03-28 17:42:56 +01:00
402 lines
18 KiB
YAML
402 lines
18 KiB
YAML
# 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"
|