diff --git a/internal/agent/export_test.go b/internal/agent/export_test.go index df2493484..a1c02d686 100644 --- a/internal/agent/export_test.go +++ b/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 + diff --git a/internal/agent/hitl_magic_api.go b/internal/agent/hitl_magic_api.go index 29f0ca6c8..364af9bc1 100644 --- a/internal/agent/hitl_magic_api.go +++ b/internal/agent/hitl_magic_api.go @@ -306,7 +306,7 @@ 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 @@ -314,11 +314,11 @@ func (a *API) magicApprove(w http.ResponseWriter, r *http.Request, messageID, us // 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 `, + "ampersand": "Tom & Jerry", + "quote_attr": `">`, + } + 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) } }