docs(spec): add design for XSS sink remediation and CSP
This commit is contained in:
@@ -0,0 +1,168 @@
|
||||
# XSS Sink Remediation + CSP
|
||||
|
||||
**Date:** 2026-07-08
|
||||
**Status:** Approved
|
||||
**Finding:** Latent XSS sink in `utils.showAlert` (`templates/index.html`) — unsanitized
|
||||
value written to `innerHTML`, no CSP served. Proven exploitable to token exfiltration
|
||||
via direct `localStorage` seeding, but no network-accessible write path to the seeded
|
||||
value exists today, so not externally exploitable. Treat as defense-in-depth.
|
||||
|
||||
## Goal
|
||||
|
||||
Eliminate the `innerHTML` sink structurally and add a nonce-based Content-Security-Policy
|
||||
so that even if a future code path writes attacker-controlled data into the alert
|
||||
message, script execution and exfiltration are blocked.
|
||||
|
||||
## Non-goals
|
||||
|
||||
- Changes to the OIDC flow, token validation, or `AUTHORIZED_USERS`.
|
||||
- Refactoring the single-file frontend layout.
|
||||
- Adding a JS test framework.
|
||||
|
||||
## Approach
|
||||
|
||||
- **Sink:** DOM-built alerts (no `innerHTML`), chosen over escape-and-allowlist because
|
||||
it removes the sink structurally rather than guarding it.
|
||||
- **CSP:** Per-request nonce, chosen over hash (survives script edits) and over
|
||||
`'unsafe-inline'` (which would not block the proven `onerror` exfil path).
|
||||
|
||||
## Design
|
||||
|
||||
### 1. Sink fix — `templates/index.html`
|
||||
|
||||
Replace `showAlert` (currently at line 162) with a version that builds the alert via
|
||||
`document.createElement` and renders all dynamic content through `textContent` /
|
||||
`createTextNode`. Accept either a plain string or a structured `{title, body}`:
|
||||
|
||||
```js
|
||||
showAlert(element, message, type = 'info') {
|
||||
element.replaceChildren();
|
||||
const alert = document.createElement('div');
|
||||
alert.className = `alert alert-${type}`;
|
||||
alert.setAttribute('role', 'alert');
|
||||
if (typeof message === 'string') {
|
||||
alert.textContent = message;
|
||||
} else if (message && typeof message === 'object') {
|
||||
if (message.title) {
|
||||
const strong = document.createElement('strong');
|
||||
strong.textContent = message.title;
|
||||
alert.appendChild(strong);
|
||||
}
|
||||
if (message.body) {
|
||||
if (message.title) {
|
||||
alert.appendChild(document.createTextNode(' '));
|
||||
}
|
||||
alert.appendChild(document.createTextNode(message.body));
|
||||
}
|
||||
}
|
||||
element.appendChild(alert);
|
||||
}
|
||||
```
|
||||
|
||||
`type` is interpolated into `className` (a DOM property, not parsed as HTML), so it is
|
||||
safe; it is also only ever a literal (`'success'`, `'danger'`, `'warning'`, `'info'`)
|
||||
at the call sites.
|
||||
|
||||
#### Caller updates
|
||||
|
||||
Three callers use `<strong>` for bold formatting and must switch to the structured form:
|
||||
|
||||
| Line | Current | New |
|
||||
|------|---------|-----|
|
||||
| 468 | `` `<strong>Secret Phrase:</strong> ${cachedSecret}` `` | `{ title: 'Secret Phrase:', body: cachedSecret }` |
|
||||
| 584 | `` `<strong>Secret Phrase:</strong> ${data.secret_phrase}` `` | `{ title: 'Secret Phrase:', body: data.secret_phrase }` |
|
||||
| 621 | `` `<strong>1 Bitcoin (BTC) = CHF ${price.toLocaleString(...)}</strong>` `` | `` { title: `1 Bitcoin (BTC) = CHF ${price.toLocaleString(...)}` } `` |
|
||||
|
||||
The remaining 11 callers pass plain strings and require no change — the `typeof message
|
||||
=== 'string'` branch renders them as `textContent`.
|
||||
|
||||
No caller performs HTML escaping; `textContent` makes it unnecessary. The sink is gone:
|
||||
there is no `innerHTML` assignment anywhere in `showAlert`.
|
||||
|
||||
### 2. CSP — `main.py`
|
||||
|
||||
Modify `read_root()` (`main.py:326-331`) to generate a per-request nonce, inject it into
|
||||
the inline `<script>` tag, and set the CSP header on the response.
|
||||
|
||||
Add `import secrets` to the imports at the top of `main.py`.
|
||||
|
||||
```python
|
||||
@app.get("/", response_class=HTMLResponse)
|
||||
async def read_root():
|
||||
"""Serve the frontend HTML file"""
|
||||
nonce = secrets.token_urlsafe(16)
|
||||
with open("templates/index.html", "r") as file:
|
||||
html_content = file.read()
|
||||
html_content = html_content.replace(
|
||||
'<script>', f'<script nonce="{nonce}">', 1
|
||||
)
|
||||
csp = (
|
||||
"default-src 'self'; "
|
||||
f"script-src 'self' 'nonce-{nonce}' https://cdn.jsdelivr.net; "
|
||||
"style-src 'self' https://cdn.jsdelivr.net; "
|
||||
"connect-src 'self' https://login.infomaniak.com https://api.coingecko.com; "
|
||||
"img-src 'self'; "
|
||||
"object-src 'none'; "
|
||||
"base-uri 'self'; "
|
||||
"frame-ancestors 'none'"
|
||||
)
|
||||
return HTMLResponse(
|
||||
content=html_content,
|
||||
status_code=200,
|
||||
headers={"Content-Security-Policy": csp},
|
||||
)
|
||||
```
|
||||
|
||||
#### Why this is correct
|
||||
|
||||
- The `replace('<script>', ..., 1)` targets only the bare inline tag at `index.html:62`.
|
||||
The Bootstrap tag at line 8 is `<script src=...>`, which does not match the literal
|
||||
`<script>` and is left untouched. A `src`-backed script is authorized by the
|
||||
`https://cdn.jsdelivr.net` entry in `script-src`, not by the nonce.
|
||||
- The nonce is fresh per request (`secrets.token_urlsafe`), so an attacker who cannot
|
||||
read the response body cannot guess it.
|
||||
- `connect-src` allows the two `fetch` origins used by the frontend
|
||||
(`api.coingecko.com` for the BTC price, `login.infomaniak.com` for any client-side
|
||||
OIDC call) plus `'self'` for the app's own endpoints (`/validate-token`,
|
||||
`/refresh-token`, `/config`, `/health`).
|
||||
- The OIDC login redirect is a top-level navigation (`window.location`), which is not
|
||||
restricted by `connect-src` or `default-src`; no `form-action` / `navigate-to`
|
||||
entry is needed.
|
||||
- `object-src 'none'`, `base-uri 'self'`, `frame-ancestors 'none'` are standard
|
||||
hardening.
|
||||
|
||||
#### Scope
|
||||
|
||||
Only the HTML route gets the CSP header. JSON API endpoints (`/health`,
|
||||
`/validate-token`, etc.) do not serve HTML and do not need a CSP header; they keep
|
||||
their existing behavior. This can be widened to a middleware later if desired, but is
|
||||
not required by this finding.
|
||||
|
||||
### 3. Verification
|
||||
|
||||
- **pytest (automated):** add a test asserting that `GET /` returns a
|
||||
`Content-Security-Policy` header containing `nonce-` and that the response body
|
||||
contains `<script nonce="`. Run the full suite (`./run_tests.sh`) and confirm it
|
||||
stays green. Mind the `lru_cache` cache-clear and `CLIENT_ID` gotchas from
|
||||
`AGENTS.md` when adding the test.
|
||||
- **Frontend (manual):** no JS test framework in the repo. Verify by loading the app
|
||||
in a browser that:
|
||||
- The secret-phrase success alert still shows **Secret Phrase:** in bold followed by
|
||||
the phrase.
|
||||
- The BTC price alert still shows the price line in bold.
|
||||
- Error alerts (e.g., trigger a token refresh failure) still render the message text.
|
||||
- The browser console shows no CSP violations during a normal login flow.
|
||||
|
||||
## Files touched
|
||||
|
||||
- `templates/index.html` — `showAlert` rewrite + 3 caller updates.
|
||||
- `main.py` — `import secrets`, `read_root()` nonce injection + CSP header.
|
||||
- `tests/` — one new test for the CSP header + nonce injection on `GET /`.
|
||||
|
||||
## Risk / rollback
|
||||
|
||||
- Low risk. The `showAlert` signature stays backward-compatible for string callers.
|
||||
- If the nonce injection breaks a downstream proxy that rewrites the HTML, rolling back
|
||||
is a single-file revert of `read_root()` plus the `showAlert` block.
|
||||
- If a CSP directive turns out to block a legitimate resource, the directive can be
|
||||
widened without touching the sink fix.
|
||||
Reference in New Issue
Block a user