Follow-ups from the #107 review. None block — the magic-link landing is correct as-shipped — but each closes a defense-in-depth or future-maintainer gap.
1. Add Content-Security-Policy to the magic-link responses
setMagicHeaders sets `Cache-Control`, `X-Frame-Options`, `Referrer-Policy`, and `X-Robots-Tag`. It doesn't set `Content-Security-Policy`. The pages use inline CSS (the 250-line `loftCommonCSS` const), so a strict CSP needs `'unsafe-inline'` for styles, but a CSP that allows inline styles and blocks all scripts would lock down the script-injection class as defense-in-depth even if a future XSS bug slipped past `html.EscapeString`.
Suggested header:
```
Content-Security-Policy: default-src 'self'; img-src 'self' data:; style-src 'unsafe-inline'; script-src 'none'; base-uri 'self'; form-action 'self'
```
`script-src 'none'` is the load-bearing piece — the magic-link pages have no legitimate scripts. `form-action 'self'` constrains the form's POST target to the same origin (the form's action is a hardcoded relative URL today, so this is purely belt-and-suspenders). ~5 lines in `setMagicHeaders`.
2. Truncate the body preview server-side
`writeConfirmPage` reads `msg.BodyText` unbounded:
```go
bodyPreview := msg.BodyText
```
The CSS caps the visual scroll area at `max-height: 260px`, but the HTML response itself is unbounded. A 5MB message body becomes a 5MB+ HTML response on every confirm-page load. Combined with the per-user TTL on tokens this is a low-risk DOS surface, but worth bounding.
Fix: truncate to ~16 KB (rune-aware so we don't split a UTF-8 sequence) with a `...truncated; view full in dashboard` tail.
```go
const maxPreviewBytes = 16 * 1024
if len(msg.BodyText) > maxPreviewBytes {
bodyPreview = string([]rune(msg.BodyText)[:maxPreviewBytes/4]) + "\n\n…truncated; view full in the dashboard."
}
```
Add a regression test that asserts the response stays under, say, 32 KB even when the source body is 1 MB.
3. Normalize the escape contract on `writeMagicMessage`
`writeMagicMessage` currently has an asymmetric API:
- `title` is auto-escaped
- `body` is not — caller must pre-escape if it contains user input
There's a comment explaining the rationale, but every caller that passes user-controlled data already calls `html.EscapeString` first (verified in the review — 19 call sites, all correct). The footgun is that a future contributor could miss the comment and pass an unescaped user string.
Two fixes, either works:
- a. Auto-escape `body` inside `writeMagicMessage`. The 2 callers that currently pre-escape (line 250 `ve.Error()`, line 270 `firstRecipient(...)`) drop the pre-escape and let the function handle it. Single source of truth, hard to misuse.
- b. Rename the parameter to `bodyHTML string` (or take `template.HTML`) so the contract is explicit at every call site.
Option (a) is the lower-friction change.
4. Add an XSS regression test
The escape behavior is correct today (audited in the #107 review). A small test would pin it against future refactors — e.g., if someone ports the inline `fmt.Fprintf` rendering to `html/template` and accidentally puts a value in the wrong context.
Sketch:
```go
func TestMagicLinkConfirmPage_EscapesSubjectAndBody(t *testing.T) {
// Issue a pending message with <script> in subject and body
// GET /api/v1/approve?t=... and assert the rendered HTML
// contains "<script>" but NOT a literal "<script>".
}
```
~30 lines. Same goes for recipients — putting `alice<script>@example.com` as a recipient should round-trip escaped.
Suggested priority
- (1) CSP — defense-in-depth, ~5 lines, very low risk.
- (2) Body truncation — actual DOS surface, ~15 lines + test.
- (3) Escape contract — refactor for maintainability, ~30 lines spanning 19 call sites.
- (4) XSS regression test — small, can land alongside (3).
(1) and (2) could be one PR. (3) + (4) could be a second PR — or just close the issue without doing the refactor since the current callers are all correct.
Follow-ups from the #107 review. None block — the magic-link landing is correct as-shipped — but each closes a defense-in-depth or future-maintainer gap.
1. Add Content-Security-Policy to the magic-link responses
setMagicHeaderssets `Cache-Control`, `X-Frame-Options`, `Referrer-Policy`, and `X-Robots-Tag`. It doesn't set `Content-Security-Policy`. The pages use inline CSS (the 250-line `loftCommonCSS` const), so a strict CSP needs `'unsafe-inline'` for styles, but a CSP that allows inline styles and blocks all scripts would lock down the script-injection class as defense-in-depth even if a future XSS bug slipped past `html.EscapeString`.Suggested header:
```
Content-Security-Policy: default-src 'self'; img-src 'self' data:; style-src 'unsafe-inline'; script-src 'none'; base-uri 'self'; form-action 'self'
```
`script-src 'none'` is the load-bearing piece — the magic-link pages have no legitimate scripts. `form-action 'self'` constrains the form's POST target to the same origin (the form's action is a hardcoded relative URL today, so this is purely belt-and-suspenders). ~5 lines in `setMagicHeaders`.
2. Truncate the body preview server-side
`writeConfirmPage` reads `msg.BodyText` unbounded:
```go
bodyPreview := msg.BodyText
```
The CSS caps the visual scroll area at `max-height: 260px`, but the HTML response itself is unbounded. A 5MB message body becomes a 5MB+ HTML response on every confirm-page load. Combined with the per-user TTL on tokens this is a low-risk DOS surface, but worth bounding.
Fix: truncate to ~16 KB (rune-aware so we don't split a UTF-8 sequence) with a `...truncated; view full in dashboard` tail.
```go
const maxPreviewBytes = 16 * 1024
if len(msg.BodyText) > maxPreviewBytes {
bodyPreview = string([]rune(msg.BodyText)[:maxPreviewBytes/4]) + "\n\n…truncated; view full in the dashboard."
}
```
Add a regression test that asserts the response stays under, say, 32 KB even when the source body is 1 MB.
3. Normalize the escape contract on `writeMagicMessage`
`writeMagicMessage` currently has an asymmetric API:
There's a comment explaining the rationale, but every caller that passes user-controlled data already calls `html.EscapeString` first (verified in the review — 19 call sites, all correct). The footgun is that a future contributor could miss the comment and pass an unescaped user string.
Two fixes, either works:
Option (a) is the lower-friction change.
4. Add an XSS regression test
The escape behavior is correct today (audited in the #107 review). A small test would pin it against future refactors — e.g., if someone ports the inline `fmt.Fprintf` rendering to `html/template` and accidentally puts a value in the wrong context.
Sketch:
```go
func TestMagicLinkConfirmPage_EscapesSubjectAndBody(t *testing.T) {
// Issue a pending message with <script> in subject and body
// GET /api/v1/approve?t=... and assert the rendered HTML
// contains "<script>" but NOT a literal "<script>".
}
```
~30 lines. Same goes for recipients — putting `alice<script>@example.com` as a recipient should round-trip escaped.
Suggested priority
(1) and (2) could be one PR. (3) + (4) could be a second PR — or just close the issue without doing the refactor since the current callers are all correct.