fix(agent): magic-link result page escapes body once, adds CSP, caps preview

internal/agent/export_test.go

@@ -47,6 +47,28 @@ func MarkSideEffectCommittedForTest(w http.ResponseWriter) {
 	markSideEffectCommitted(w)
 }
 
+// WriteMagicMessageForTest exposes writeMagicMessage so the external
+// agent_test package can verify the magic-link result page's HTML-escape
+// contract directly (writeMagicMessage is the single place that escapes
+// body; driving every handler path that can reach it would duplicate that
+// coverage without testing anything writeMagicMessage itself doesn't
+// already own).
+func WriteMagicMessageForTest(w http.ResponseWriter, status int, title, body string) {
+	writeMagicMessage(w, status, title, body)
+}
+
+// TruncatePreviewBytesForTest exposes truncatePreviewBytes for a direct
+// unit test of the rune-boundary-safe byte cap on the confirm page's body
+// preview.
+func TruncatePreviewBytesForTest(s string, maxBytes int) string {
+	return truncatePreviewBytes(s, maxBytes)
+}
+
+// MaxBodyPreviewBytesForTest is the confirm page's body-preview byte cap,
+// exposed so tests assert against the real constant instead of a hardcoded
+// copy that could drift from it.
+const MaxBodyPreviewBytesForTest = maxBodyPreviewBytes
+
 // BuildBlockedOutboundEventForTest exposes buildBlockedOutboundEvent (and the
 // blockAuditID soft-ref it is keyed on) so the external agent_test package can
 // drive the exact event the emit path constructs through a real outbox +

internal/agent/hitl_magic_api.go

@@ -306,19 +306,19 @@ func (a *API) magicApprove(w http.ResponseWriter, r *http.Request, messageID, us
 		return
 	}
 	if supErr := a.checkSuppressionStrict(r.Context(), userID, agent.ID, sendReq); supErr != nil {
-		writeMagicMessage(w, supErr.Status, "Cannot send", html.EscapeString(supErr.Msg))
+		writeMagicMessage(w, supErr.Status, "Cannot send", supErr.Msg)
 		return
 	}
 	// Flow-cap re-check — same rationale and ordering as ApprovePendingCore
 	// (after suppression, with the final recipient set): the hold was never
 	// metered at accept, so approve time is the enforcement point.
 	if qerr := a.checkApproveQuota(r.Context(), userID,
 		identity.UniqueRecipientCount(sendReq.To, sendReq.CC, sendReq.BCC)); qerr != nil {
-		writeMagicMessage(w, qerr.Status, "Cannot send", html.EscapeString(qerr.Msg))
+		writeMagicMessage(w, qerr.Status, "Cannot send", qerr.Msg)
 		return
 	}
 	if uerr := prepareManagedUnsubscribe(r.Context(), a.unsubscribeIssuer, a.fromDomain, userID, agent, &sendReq, true); uerr != nil {
-		writeMagicMessage(w, uerr.Status, "Cannot send", html.EscapeString(uerr.Msg))
+		writeMagicMessage(w, uerr.Status, "Cannot send", uerr.Msg)
 		return
 	}
 	sent, handled, aerr := a.approveOutboundAsyncWithRequest(r.Context(), agent, messageID, userID, draft, identity.PendingApprovalEdit{}, sendReq, nil)
@@ -330,7 +330,7 @@ func (a *API) magicApprove(w http.ResponseWriter, r *http.Request, messageID, us
 		log.Printf("[mail:%s] dir=outbound type=%s status=%s agent=%s to_count=%d to_domains=%v approved=magic-link:user:%s delivery=async",
 			sent.ID, sent.Type, sent.Status, agent.EmailAddress(), len(sent.ToRecipients), logredact.AddressDomains(sent.ToRecipients), userID)
 		writeMagicResult(w, http.StatusOK, "Approved",
-			fmt.Sprintf("Your message to %s has been queued for delivery.", html.EscapeString(firstRecipient(sent.ToRecipients))),
+			fmt.Sprintf("Your message to %s has been queued for delivery.", firstRecipient(sent.ToRecipients)),
 			viewMessageCTA(agent.EmailAddress(), draft.ConversationID, sent.ID))
 		return
 	}
@@ -349,8 +349,7 @@ func (a *API) magicApprove(w http.ResponseWriter, r *http.Request, messageID, us
 		default:
 			var ve *outbound.ValidationError
 			if errors.As(err, &ve) {
-				writeMagicMessage(w, http.StatusBadRequest, "Cannot send",
-					html.EscapeString(ve.Error()))
+				writeMagicMessage(w, http.StatusBadRequest, "Cannot send", ve.Error())
 				return
 			}
 			log.Printf("[api] magic-approve send failed: msg=%s err=%v", messageID, err)
@@ -367,7 +366,7 @@ func (a *API) magicApprove(w http.ResponseWriter, r *http.Request, messageID, us
 
 	writeMagicResult(w, http.StatusOK,
 		"Approved",
-		fmt.Sprintf("Your message to %s has been sent.", html.EscapeString(firstRecipient(sent.ToRecipients))),
+		fmt.Sprintf("Your message to %s has been sent.", firstRecipient(sent.ToRecipients)),
 		viewMessageCTA(agent.EmailAddress(), draft.ConversationID, sent.ID))
 }
 
@@ -386,7 +385,7 @@ func writeMagicApproveError(w http.ResponseWriter, messageID string, err error)
 	default:
 		var ve *outbound.ValidationError
 		if errors.As(err, &ve) {
-			writeMagicMessage(w, http.StatusBadRequest, "Cannot send", html.EscapeString(ve.Error()))
+			writeMagicMessage(w, http.StatusBadRequest, "Cannot send", ve.Error())
 			return
 		}
 		log.Printf("[api] magic-approve accept failed: msg=%s err=%v", messageID, err)
@@ -783,6 +782,11 @@ func setMagicHeaders(w http.ResponseWriter, status int) {
 	w.Header().Set("Referrer-Policy", "no-referrer")
 	// Discourage indexing if a link ever leaks publicly.
 	w.Header().Set("X-Robots-Tag", "noindex, nofollow")
+	// Defense in depth: these pages emit no <script> and only an inline
+	// <style> block, so a CSP costs nothing today and blocks any future
+	// contributor from silently reintroducing an injectable sink.
+	w.Header().Set("Content-Security-Policy",
+		"default-src 'self'; img-src 'self' data:; style-src 'unsafe-inline'; script-src 'none'; base-uri 'self'; form-action 'self'")
 	w.WriteHeader(status)
 }
 
@@ -842,10 +846,10 @@ func writeMagicResult(w http.ResponseWriter, status int, title, body string, cta
 `,
 		html.EscapeString(bannerEyebrowFor(status)),
 		html.EscapeString(title),
-		// `body` is sometimes a pre-escaped fragment (the validation-error
-		// path runs html.EscapeString itself before calling us). Don't
-		// double-escape.
-		body,
+		// writeMagicResult is the sole place that escapes body: every
+		// caller passes the raw string, so there is exactly one place
+		// that can forget to.
+		html.EscapeString(body),
 		html.EscapeString(cta.Href),
 		html.EscapeString(cta.Label),
 	)
@@ -895,6 +899,7 @@ func writeConfirmPage(w http.ResponseWriter, status int, action, token string, m
 	if bodyPreview == "" && msg.BodyHTML != "" {
 		bodyPreview = "(HTML only; view full message in the dashboard)"
 	}
+	bodyPreview = truncatePreviewBytes(bodyPreview, maxBodyPreviewBytes)
 
 	toList := strings.Join(msg.ToRecipients, ", ")
 	ccList := strings.Join(msg.CC, ", ")
@@ -965,6 +970,32 @@ func writeConfirmPage(w http.ResponseWriter, status int, action, token string, m
 	)
 }
 
+// maxBodyPreviewBytes caps the confirm page's body preview so a reviewer
+// opening a held message with an unusually large body doesn't pull the
+// whole thing into one HTTP response every time the page loads. The
+// preview is a decision aid, not the message itself: the full body stays
+// reachable from the dashboard.
+const maxBodyPreviewBytes = 16384
+
+// truncatePreviewBytes returns a prefix of s at most maxBytes long,
+// stopping at a rune boundary so a multi-byte UTF-8 sequence is never
+// split. Byte length, not rune count: bodyPreview renders inside a
+// bounded HTTP response and the concern is response size, not display
+// width.
+func truncatePreviewBytes(s string, maxBytes int) string {
+	if len(s) <= maxBytes {
+		return s
+	}
+	n := 0
+	for i := range s {
+		if i > maxBytes {
+			break
+		}
+		n = i
+	}
+	return s[:n] + "\n[truncated]"
+}
+
 func firstRecipient(rs []string) string {
 	if len(rs) == 0 {
 		return "the recipient"

internal/agent/hitl_magic_api_test.go

@@ -2,13 +2,15 @@ package agent_test
 
 import (
 	"context"
+	"html"
 	"io"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"strings"
 	"testing"
 	"time"
+	"unicode/utf8"
 
 	"github.com/gorilla/mux"
 	"github.com/jackc/pgx/v5"
@@ -183,6 +185,20 @@ func issuePending(t *testing.T, store *identity.Store, agentID string) *identity
 	return msg
 }
 
+// issuePendingWithBody mirrors issuePending but lets the caller control
+// BodyText, for the body-preview truncation test below.
+func issuePendingWithBody(t *testing.T, store *identity.Store, agentID, bodyText string) *identity.Message {
+	t.Helper()
+	msg, err := store.CreatePendingOutboundMessage(context.Background(), agentID,
+		[]string{"alice@example.com"}, nil, nil,
+		"Held", bodyText, "", nil,
+		"send", "body-preview-cap-test", "", "", 3600)
+	if err != nil {
+		t.Fatal(err)
+	}
+	return msg
+}
+
 func readBody(t *testing.T, resp *http.Response) string {
 	t.Helper()
 	defer resp.Body.Close()
@@ -839,6 +855,89 @@ func TestMagicLinkNoCacheAndSecurityHeaders(t *testing.T) {
 		if got := resp.Header.Get("X-Robots-Tag"); !strings.Contains(got, "noindex") {
 			t.Errorf("%s: X-Robots-Tag = %q, want containing noindex", path, got)
 		}
+		if got := resp.Header.Get("Content-Security-Policy"); got == "" {
+			t.Errorf("%s: Content-Security-Policy header missing", path)
+		} else {
+			for _, directive := range []string{"script-src 'none'", "default-src 'self'", "form-action 'self'"} {
+				if !strings.Contains(got, directive) {
+					t.Errorf("%s: Content-Security-Policy = %q, want containing %q", path, got, directive)
+				}
+			}
+		}
+	}
+}
+
+// --- writeMagicResult's escape contract (issue #118 item 3) ---
+//
+// writeMagicResult is the single function that renders `body` into the
+// result page; every caller must pass the raw, unescaped value. These
+// tests drive it directly (via the export_test.go seam) rather than
+// through every handler path that can reach it, since the escaping is
+// owned entirely by this one function.
+
+func TestMagicResultEscapesBodyExactlyOnce(t *testing.T) {
+	cases := map[string]string{
+		"script_tag": `<script>alert(document.cookie)</script>`,
+		"ampersand":  "Tom & Jerry",
+		"quote_attr": `"><img src=x onerror=alert(1)>`,
+	}
+	for name, body := range cases {
+		t.Run(name, func(t *testing.T) {
+			rec := httptest.NewRecorder()
+			agent.WriteMagicMessageForTest(rec, http.StatusBadRequest, "Cannot send", body)
+			got := rec.Body.String()
+
+			if strings.Contains(got, body) {
+				t.Fatalf("result page contains the raw, unescaped body %q:\n%s", body, got)
+			}
+			want := html.EscapeString(body)
+			if !strings.Contains(got, want) {
+				t.Fatalf("result page missing the escaped body %q:\n%s", want, got)
+			}
+			if doubled := html.EscapeString(want); strings.Contains(got, doubled) {
+				t.Fatalf("result page double-escaped the body (found %q):\n%s", doubled, got)
+			}
+		})
+	}
+}
+
+// --- Confirm-page body-preview byte cap (issue #118 item 2) ---
+
+func TestTruncatePreviewBytesStaysUnderCapOnRuneBoundary(t *testing.T) {
+	// 金 is 3 bytes in UTF-8; a cap that isn't a multiple of 3 forces the
+	// truncation point to fall mid-rune unless it backs off to the last
+	// complete rune, exactly what the maintainer's own suggested
+	// `[:maxPreviewBytes/4]` snippet (rune-count, not byte-count) got wrong.
+	s := strings.Repeat("金", 10)
+	got := agent.TruncatePreviewBytesForTest(s, 7)
+	if !utf8.ValidString(got) {
+		t.Fatalf("truncated string is not valid UTF-8: %q", got)
+	}
+	if !strings.HasPrefix(s, strings.TrimSuffix(got, "\n[truncated]")) {
+		t.Fatalf("truncated output is not a prefix of the input: got %q", got)
+	}
+
+	if got := agent.TruncatePreviewBytesForTest("short", 100); got != "short" {
+		t.Errorf("under the cap should be returned unchanged, got %q", got)
+	}
+}
+
+func TestMagicLinkConfirmPageCapsBodyPreview(t *testing.T) {
+	server, store, signer, _ := setupMagicLinkAPI(t)
+	a, _ := prepareHITLAgent(t, store, "preview-cap")
+	big := strings.Repeat("a", agent.MaxBodyPreviewBytesForTest*2)
+	msg := issuePendingWithBody(t, store, a.ID, big)
+
+	tok, _ := signer.Sign(msg.ID, approvaltoken.ActionApprove, time.Now().Add(1*time.Hour))
+	resp, _ := http.Get(server.URL + "/v1/approve?t=" + url.QueryEscape(tok))
+	body := readBody(t, resp)
+
+	if strings.Contains(body, big) {
+		t.Fatalf("confirm page echoed the full %d-byte body untruncated", len(big))
+	}
+	if len(body) >= len(big) {
+		t.Errorf("confirm page response is %d bytes, want well under the %d-byte body (preview should be capped at %d)",
+			len(body), len(big), agent.MaxBodyPreviewBytesForTest)
 	}
 }