diff --git a/assets/styles/app.css b/assets/styles/app.css index f99c5552..888dddae 100644 --- a/assets/styles/app.css +++ b/assets/styles/app.css @@ -4462,6 +4462,42 @@ label:has(> input.sr-only) { } } +/* + The words a search result matched on, in its subject and in its preview. + + `[data-unread]`, which is the row itself: the attribute is written on mail + rows and nowhere else, so this reaches the subject span and the snippet span + without either of them needing a hook of its own — no markup change, and + therefore no change at all to the ~ten other lists that share that partial. + A bare `mark` selector would have been shorter and wrong: a sanitised message + body can contain a sender's own , and the reading pane is not ours to + restyle. + + NO PADDING, and that is a constraint rather than a preference. The snippet + span carries `truncate` — `overflow: hidden` — so a highlight with vertical + padding paints outside the line box and is sliced off along its top and + bottom edge. The emphasis comes from the two things that cost no box: a wash + of the theme's accent, and the text stepping up from the row's faint ink to + its full ink. Built from --rgb-accent and --rgb-ink so all forty-odd themes + and the user's own accent colour are right by construction, light and dark + alike — a fixed yellow would be Word's highlighter on a dark surface. + + Unlayered, like the rules above and for the same reason: Tailwind's utilities + live in a layer, `text-ink-faint` is on the same element, and a layered rule + loses to it however specific it is. + + box-decoration-break so a hit that wraps across the second preview line — the + two-line setting — carries the wash on both halves instead of one long box. +*/ +[data-unread] mark { + background-color: rgb(var(--rgb-accent) / 0.22); + color: rgb(var(--rgb-ink)); + padding: 0; + border-radius: 0.2rem; + -webkit-box-decoration-break: clone; + box-decoration-break: clone; +} + /* The accent bar down an unread row's leading edge — Strong emphasis only, where --unread-bar-w is 3px; it is 0px at the other two and paints nothing. diff --git a/docs/de/features/mail.md b/docs/de/features/mail.md index 7393df93..73acb202 100644 --- a/docs/de/features/mail.md +++ b/docs/de/features/mail.md @@ -1,4 +1,4 @@ - + # Mail @@ -451,6 +451,17 @@ findet er wenig bis nichts, und das ist immerhin die Wahrheit. Eine Abfrage, die aus nichts als einem halb getippten Operator besteht — `from:` ohne Wert — liefert eine leere Seite und nicht das ganze Postfach. +**Eine Trefferzeile zeigt dir, warum sie da ist.** Jedes Vorkommen deiner Wörter ist im Betreff und +in der Vorschau markiert, und die Vorschau ist ein Ausschnitt der Nachricht rund um den Treffer +statt ihrer ersten Zeile — die steht meist ganz woanders, und genau sie hat die Zeile früher +gezeigt. Markiert wird mit derselben Stammformbildung, mit der auch gesucht wird: Eine Suche nach +`running` markiert `run`. + +Eine Zeile, die über etwas gefunden wurde, das die Vorschau nicht zeigen kann — den Absendernamen, +ein Wort nur im Betreff, oder eine Bedeutungssuche ohne eines deiner Wörter darin — behält die +gewohnte Vorschau. Genauso jede Zeile, wenn deine Abfrage nur aus Operatoren besteht: `is:unread` +hat kein Wort zum Markieren. + Ergebnisse kommen **Neueste zuerst**. Der Schalter neben der Seitenblätterung stellt auf **Relevanteste zuerst** um, also auf den Volltext-Rang — der beste Treffer führt, wann immer er angekommen ist. Was du wählst, wird für deine nächste Suche gemerkt, und das Blättern behält es diff --git a/docs/features/mail.md b/docs/features/mail.md index 0621f84a..9f3ef8e0 100644 --- a/docs/features/mail.md +++ b/docs/features/mail.md @@ -411,6 +411,15 @@ search had been ignored. As free text it finds little or nothing, which is at le A query that is nothing but a half-typed operator — `from:` with no value — returns an empty page rather than the whole mailbox. +**A result row shows you why it is there.** Every occurrence of your words is marked in the subject +and in the preview, and the preview is a window of the message around the match rather than its +opening line — which is usually somewhere else entirely, and is what the row used to show. The +marking follows the same stemming the search does, so a search for `running` marks `run`. + +A row that matched on something the preview cannot show — the sender's name, a word only in the +subject, or a meaning match with none of your words in it — keeps the ordinary preview. So does +every row when your query is only operators: `is:unread` has no word to mark. + Results are **Most recent** first. The switch beside the pagination changes that to **Most relevant**, which is full-text rank — the best match leads, whenever it arrived. Whichever you pick is remembered for your next search, and paging keeps it. Switching orders starts again at the first diff --git a/src/Controller/Mail/SearchController.php b/src/Controller/Mail/SearchController.php index dd0b7ff4..eba0b86d 100644 --- a/src/Controller/Mail/SearchController.php +++ b/src/Controller/Mail/SearchController.php @@ -12,6 +12,7 @@ use App\Entity\User\User; use App\Repository\Mail\MessageThreadRepository; use App\Service\Mail\ThreadListRenderer; use App\Service\Search\SearchQueryParser; +use App\Service\Search\SearchResultHighlights; use Doctrine\ORM\EntityManagerInterface; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; use Symfony\Component\HttpFoundation\Request; @@ -31,6 +32,7 @@ final class SearchController extends AbstractController private readonly SemanticQuery $semantic, private readonly SemanticCoverage $coverage, private readonly EmbeddingCatchUp $catchUp, + private readonly SearchResultHighlights $highlights, ) {} #[Route('', name: '', methods: ['GET'])] @@ -136,6 +138,25 @@ final class SearchController extends AbstractController // at a time on top of everything the inbox was already paying. $this->threadRepository->preloadForRows($threads); + // WHY THE ROW CAN SAY WHY IT IS HERE. Without this a result shows the + // opening line of its newest message, which usually does not contain + // the term that found it — the list asserts a match and then shows + // something else. One batched `ts_headline` per account turns that into + // a window around the match with every occurrence marked. + // + // AFTER the preload, and that is an ordering constraint rather than a + // preference: this reads `thread.messages` for the ids, so run before + // it, the page pays a lazy load per row and the search list is back to + // the hundred-and-sixty-seven-query shape preloadForRows() exists to + // prevent. See ThreadListQueryBudgetTest. + // + // Handed to the template as a map keyed by thread id, next to + // `semantic_only` and for the same reason: a highlight is a property of + // THIS search, and hanging it on the shared MessageThread entity would + // leave it in Doctrine's identity map for every other list in the + // request to find. See MessageThreadRepository::searchPage(). + $highlights = $this->highlights->forPage($threads, $parsed->freeText); + // Through ThreadListRenderer, not $this->render(): a row whose subject // and sender the user has just read in a result list has been SHOWN, // and leaving it badged would mean finding your own search results @@ -194,6 +215,7 @@ final class SearchController extends AbstractController 'sort' => $sort, 'semantic' => $report, 'semantic_only' => $results->semanticOnly, + 'highlights' => $highlights, ]); } diff --git a/src/Jmap/Method/Mail/SearchSnippetGetMethod.php b/src/Jmap/Method/Mail/SearchSnippetGetMethod.php index e74e2ca5..c107beb2 100644 --- a/src/Jmap/Method/Mail/SearchSnippetGetMethod.php +++ b/src/Jmap/Method/Mail/SearchSnippetGetMethod.php @@ -9,6 +9,7 @@ use App\Jmap\Method\JmapMethod; use App\Jmap\Protocol\Exception\MethodException; use App\Jmap\Protocol\JmapContext; use App\Repository\Mail\MessageRepository; +use App\Service\Search\SearchHighlighter; /** * "SearchSnippet/get" (RFC 8621 §5). @@ -26,6 +27,14 @@ use App\Repository\Mail\MessageRepository; * Per the spec the strings are HTML, with `` around each hit. Everything * else is escaped: this is message content being handed back for display, and * the one thing it must not be able to do is carry markup of its own. + * + * That last sentence was a claim and not a fact for as long as this class asked + * `ts_headline` for `StartSel=` directly. `ts_headline` does not escape + * its document; it inserts the delimiters and hands back everything else + * exactly as it found it, so a mail whose subject or text part contained markup + * shipped that markup to every client as snippet HTML. The escaping now happens + * in SearchHighlighter, which is also where the options string lives — the + * conversion and the delimiters it converts cannot be changed apart. */ final class SearchSnippetGetMethod implements JmapMethod { @@ -39,6 +48,7 @@ final class SearchSnippetGetMethod implements JmapMethod public function __construct( private readonly AccountResolver $accountResolver, private readonly MessageRepository $messages, + private readonly SearchHighlighter $highlighter, ) { } @@ -100,17 +110,15 @@ final class SearchSnippetGetMethod implements JmapMethod */ private function snippets(int $accountId, array $requested, string $text): array { - // Options chosen so a snippet is a snippet: one fragment, short enough - // to sit on a list row, and no ellipsis of our own — the client decides - // how to truncate for its width. - $options = 'StartSel=, StopSel=, MaxWords=24, MinWords=8, ' - .'ShortWord=3, MaxFragments=1, FragmentDelimiter=" … "'; - + // The options are the highlighter's, not this method's: they name the + // delimiters it converts, and the web search page asks for the same + // ones. Two callers spelling them out separately is how one of them + // ends up highlighting nothing. $rows = $this->messages->findSearchHeadlines( $accountId, array_map('intval', $requested), $text, - $options, + SearchHighlighter::HEADLINE_OPTIONS, ); $list = []; @@ -124,9 +132,10 @@ final class SearchSnippetGetMethod implements JmapMethod 'emailId' => $id, // Null rather than the whole field when nothing matched in it: // a "snippet" that is just the subject again tells the reader - // nothing about why the message came back. - 'subject' => $this->highlighted($row['subject']), - 'preview' => $this->highlighted($row['preview']), + // nothing about why the message came back. toHtml() answers + // null for exactly that case. + 'subject' => $this->highlighter->toHtml($row['subject']), + 'preview' => $this->highlighter->toHtml($row['preview']), ]; } @@ -137,19 +146,6 @@ final class SearchSnippetGetMethod implements JmapMethod ]; } - /** - * `ts_headline` returns the text either way; only a string containing a - * marker actually had a hit in that field. - */ - private function highlighted(mixed $value): ?string - { - if (false === is_string($value) || '' === $value) { - return null; - } - - return str_contains($value, '') ? $value : null; - } - /** * The free-text term the query ran on, or null when it had none. * diff --git a/src/Jmap/README.md b/src/Jmap/README.md index 013726e1..36fa7865 100644 --- a/src/Jmap/README.md +++ b/src/Jmap/README.md @@ -684,6 +684,13 @@ statement that creates the need for it. **stemmed**: a search for `running` marks `run`, and a term whose stem differs from the literal text still highlights. Re-implementing either in PHP would let a snippet highlight something the search did not match on. +- **A snippet is HTML, and `` is the only markup in it.** `ts_headline` + escapes nothing — it inserts its delimiters and returns the rest of the + document verbatim — so the delimiters it is asked for are two control + characters, the whole string is escaped, and only then are they swapped for + the tags. A client may render these strings as HTML; a sender's own markup + inside one arrives escaped and is displayed rather than parsed. See + `App\Service\Search\SearchHighlighter`, which the web search page shares. - **Calendars are advertised as `urn:plmail:params:jmap:calendars`, not `urn:ietf:params:jmap:calendars`.** JMAP for Calendars is an unratified draft whose object shape is still moving; claiming its URN would promise a contract diff --git a/src/Service/Search/SearchHighlighter.php b/src/Service/Search/SearchHighlighter.php new file mode 100644 index 00000000..ecf98943 --- /dev/null +++ b/src/Service/Search/SearchHighlighter.php @@ -0,0 +1,180 @@ +` therefore produces a string in which our markup and the + * sender's markup are indistinguishable: + * + * ts_headline('english', ' chargecloud invoice', + * websearch_to_tsquery('english', 'chargecloud'), 'StartSel=, …') + * → 'onerror=alert(1)> chargecloud invoice' + * + * That is not a hypothetical: it is what this codebase shipped to every JMAP + * client until this class existed, under a docblock claiming the opposite. A + * "does it contain " test cannot tell the two apart, because by then + * there is nothing left to tell apart. + * + * THE SENTINEL PATTERN, and it only works in this order: + * + * 1. ask `ts_headline` for delimiters that are NOT markup — two control + * characters, so nothing in the returned string is HTML yet; + * 2. escape the whole string, which is now uniformly untrusted text; + * 3. swap the (untouched, because `htmlspecialchars` has no opinion about + * control characters) sentinels for the real tags. + * + * STX (0x02) and ETX (0x03) are the delimiters because they cannot occur in + * displayable mail text: MIME transfer decoding produces them only from a + * binary part, and a binary part is not `body_text`. If one somehow survives + * anyway it is harmless here — the swap below only converts BALANCED pairs + * and drops anything left over, so the worst case is a character disappearing + * from a preview, not an unclosed tag. + * + * The options string is a constant on this class rather than a literal at the + * two call sites, because a snippet built with one pair of sentinels and + * converted with another is a snippet that silently shows `\x02` to a user or, + * worse, highlights nothing and looks like the search matched the wrong thing. + * The two halves of one decision live in one file. + */ +final class SearchHighlighter +{ + /** @see self::HEADLINE_OPTIONS — the marker `ts_headline` writes. */ + private const string START = "\x02"; + + /** Its closing half. */ + private const string STOP = "\x03"; + + /** + * What to pass `ts_headline` as its options argument. + * + * One fragment, short enough to sit on a list row, and no ellipsis of our + * own — the client (or the CSS) decides how to truncate for its width. + * + * `ShortWord=0` is the one number here that is not about size. It is what + * makes a headline over a SUBJECT usable as that subject: at the default 3 + * the fragment has its leading and trailing short words trimmed off, so + * "Oak for the alcove" searched for `alcove` comes back as just "alcove", + * and a row rendering that has quietly rewritten the subject line rather + * than highlighted it. Measured against Postgres 18, not reasoned about. + * The cost is that a body fragment may now begin with "the", which is + * cosmetic and true to the text. + */ + public const string HEADLINE_OPTIONS = 'StartSel="'.self::START.'", StopSel="'.self::STOP.'", ' + .'MaxWords=24, MinWords=8, ShortWord=0, MaxFragments=1, FragmentDelimiter=" … "'; + + /** + * Highlight HTML for a JSON payload, or null when this field had no hit. + * + * A plain string, deliberately: this is what goes into a JMAP response, + * where `Twig\Markup` would be serialised as an object and the "already + * escaped" promise it carries means nothing. See toMarkup() for the web. + * + * Null rather than the field's text when nothing matched, because + * `ts_headline` answers either way — it falls back to the opening words of + * the document — and a "snippet" that is just the first line again tells + * the reader nothing about why the message came back. The absence of a + * sentinel is the only honest signal that this field is not the reason. + * + * `mixed` because the value arrives from a DBAL row, where every column is + * typed as whatever the driver felt like. + */ + public function toHtml(mixed $headline): ?string + { + if (false === is_string($headline) || '' === $headline) { + return null; + } + + if (false === str_contains($headline, self::START)) { + return null; + } + + $escaped = htmlspecialchars( + $this->oneLine($headline), + // ENT_SUBSTITUTE is load-bearing on mail. Without it a body whose + // text part is not valid UTF-8 — which is most of what a broken + // sender produces — makes htmlspecialchars return the empty + // string, and the row loses its preview with nothing logged. + \ENT_QUOTES | \ENT_SUBSTITUTE | \ENT_HTML5, + 'UTF-8', + ); + + // Pairs only. A lone sentinel cannot open a tag that never closes, + // which is the one way a stray control character in a body could still + // reach the page as structure rather than as text. + // + // No `u` modifier, deliberately: the pattern is pure ASCII, a control + // byte cannot be part of a multi-byte sequence, and `u` would make the + // whole replacement return null on the invalid UTF-8 that real mail + // arrives in — losing the highlight on exactly the messages that need + // the most care. + $marked = preg_replace( + '/'.self::START.'([^'.self::START.self::STOP.']*)'.self::STOP.'/', + '$1', + $escaped, + ) ?? $escaped; + + return str_replace([self::START, self::STOP], '', $marked); + } + + /** + * The same HTML, wrapped so Twig prints it as markup. + * + * `Twig\Markup` rather than `|raw` at the call site, and the difference is + * not style. `|raw` in a template is a switch somebody can copy onto the + * next value that reaches that line; a Markup instance is a promise made + * by the one object that actually did the escaping. The row template has + * no `|raw` in it and this is what keeps it that way. + */ + public function toMarkup(mixed $headline): ?Markup + { + $html = $this->toHtml($headline); + + return null === $html ? null : new Markup($html, 'UTF-8'); + } + + /** + * The headline as the reader would see it, with the markers taken out. + * + * For the one caller that has to compare a headline with the string it was + * made from — a subject highlight may only replace a subject when it IS + * that subject, see SearchResultHighlights. + */ + public function withoutMarkers(mixed $headline): ?string + { + if (false === is_string($headline)) { + return null; + } + + return str_replace([self::START, self::STOP], '', $headline); + } + + /** + * Wrapping, tabs and the non-breaking spaces mail is full of, collapsed to + * single spaces. + * + * A text part is hard-wrapped at 72 columns and a headline taken out of it + * carries those newlines. HTML would collapse them anyway on the web, but + * a JMAP client is handed the string and a test asserting on one should + * not have to know where the sender's line breaks fell. + * + * Runs BEFORE the escape and cannot disturb the sentinels: control + * characters are not whitespace to `\s`. + */ + private function oneLine(string $headline): string + { + $collapsed = preg_replace('/[\s\x{00A0}]+/u', ' ', $headline) ?? $headline; + + return trim($collapsed); + } +} diff --git a/src/Service/Search/SearchResultHighlights.php b/src/Service/Search/SearchResultHighlights.php new file mode 100644 index 00000000..2274d032 --- /dev/null +++ b/src/Service/Search/SearchResultHighlights.php @@ -0,0 +1,268 @@ +account` on fifty rows is a lazy + * load per row waiting to happen — the exact regression + * ThreadListQueryBudgetTest was written to catch. + * + * ONE QUERY PER ACCOUNT PER PAGE, which for the common single-account mailbox + * is one. A headline per row would be fifty and would blow that budget on its + * own. + * + * A FRAGMENT OF `body_text`, WHERE THE ROW'S PREVIEW IS MADE OF `bodyHtmlSafe`, + * and the two disagree in ways that are already written down: the text part is + * the half of a mail most recipients never see, so for bulk senders it is + * routinely a sentence saying the mail is only available in HTML, and for some + * it is HTML pasted in as text. That is why MessageSnippet prefers the rendered + * body, and none of it is undone here — because this class REPLACES the preview + * only when the fragment actually contains a hit, and a hit in `body_text` is + * the literal reason the row is in the list: `search_vector` is generated from + * subject, from_name, from_address and `body_text`, so the words the search + * matched on are in that column and nowhere else. + * + * So the junk cases sort themselves out rather than needing to be recognised. + * "This email is only available in HTML" does not contain the search term, so + * the headline has no marker, so the row keeps the preview it has today. A text + * part full of markup that DOES contain the term shows that markup as visible, + * escaped text — which is exactly what the row already does with such a message + * elsewhere, and is the honest answer: the reader is being shown the text the + * search matched, character for character. There is deliberately no heuristic + * here that tries to tell a good text part from a bad one; the marker is the + * whole test. + */ +final class SearchResultHighlights +{ + /** + * How far back down a conversation a fragment is worth looking for. + * + * Not a correctness limit — it is a ceiling on `ts_headline`, which parses + * a whole body per call, so an unbounded version costs one document per + * message on the page rather than per row. Newest first, because the row + * already previews the newest message and a fragment from six replies ago + * explains the row less the further back it comes from. A thread longer + * than this whose match is older simply keeps today's preview, which is + * the behaviour every other list has. + */ + private const int MAX_MESSAGES_PER_THREAD = 5; + + public function __construct( + private readonly MessageRepository $messages, + private readonly SearchHighlighter $highlighter, + ) { + } + + /** + * Highlights for the threads on one page, keyed by thread id. + * + * A thread appears in the map only when something was actually marked, so + * a caller can treat "absent" and "nothing matched in this row" as the same + * thing — which is what lets the shared row template fall through to the + * behaviour every other list has. + * + * @param MessageThread[] $threads + * + * @return array + */ + public function forPage(array $threads, string $freeText): array + { + $freeText = trim($freeText); + + // An operator-only search — `from:alice`, `is:unread` — has no term to + // mark. `ts_headline` would answer with the opening words of every body + // on the page, which is the preview the row already has, bought with an + // extra query. Nothing to highlight means no work at all. + if ('' === $freeText || [] === $threads) { + return []; + } + + $candidates = $this->candidates($threads); + + if ([] === $candidates) { + return []; + } + + $headlines = []; + + foreach ($candidates as $accountId => $messageIds) { + $rows = $this->messages->findSearchHeadlines( + $accountId, + $messageIds, + $freeText, + SearchHighlighter::HEADLINE_OPTIONS, + ); + + foreach ($rows as $row) { + $headlines[(int) $row['id']] = $row; + } + } + + $highlights = []; + + foreach ($threads as $thread) { + $id = $thread->id; + + if (null === $id) { + continue; + } + + $highlight = $this->forThread($thread, $headlines); + + if (null !== $highlight['subject'] || null !== $highlight['snippet']) { + $highlights[$id] = $highlight; + } + } + + return $highlights; + } + + /** + * The message ids to ask about, grouped by account id. + * + * @param MessageThread[] $threads + * + * @return array> + */ + private function candidates(array $threads): array + { + $candidates = []; + + foreach ($threads as $thread) { + $accountId = $thread->account?->id; + + if (null === $accountId || null === $thread->id) { + continue; + } + + foreach ($this->newestFirst($thread) as $message) { + $candidates[$accountId][] = (int) $message->id; + } + } + + return $candidates; + } + + /** + * The messages of one thread that are worth a headline, newest first. + * + * The association is ordered OLDEST first — `#[ORM\OrderBy]` on receivedAt, + * which preloadMessages() has to repeat because a fetch join does not + * inherit it — so newest first is that reversed. Both loops in this class + * read the same list, or the one that asks for headlines and the one that + * reads them back would disagree about which messages exist. + * + * @return list + */ + private function newestFirst(MessageThread $thread): array + { + $messages = array_values(array_filter( + array_reverse($thread->messages->toArray()), + // A thread can hold a message that has never been flushed — the + // compose dock persists a draft into a live thread — and an id-less + // one has nothing for `WHERE m.id IN (…)` to find. + static fn ($message): bool => null !== $message->id, + )); + + return array_slice($messages, 0, self::MAX_MESSAGES_PER_THREAD); + } + + /** + * Which of a thread's messages gets to explain the row. + * + * The two fields are decided independently, and that is deliberate: a + * conversation can have the term in the subject of every message and in the + * body of only the oldest, and highlighting the subject from the newest + * while previewing the fragment from the one that actually carries it is + * the answer that describes the row best. + * + * @param array $headlines + * + * @return array{subject: ?Markup, snippet: ?Markup} + */ + private function forThread(MessageThread $thread, array $headlines): array + { + $subject = null; + $snippet = null; + + foreach ($this->newestFirst($thread) as $message) { + $row = $headlines[(int) $message->id] ?? null; + + if (null === $row) { + continue; + } + + $snippet ??= $this->highlighter->toMarkup($row['preview']); + $subject ??= $this->subject($thread, $row['subject']); + + if (null !== $subject && null !== $snippet) { + break; + } + } + + return ['subject' => $subject, 'snippet' => $snippet]; + } + + /** + * A highlighted subject, but only when it is still the row's subject. + * + * `ts_headline` is a FRAGMENTING function even over a field as short as a + * subject: it will happily return the middle twenty-four words of a long + * one, and it is given the MESSAGE's subject while the row draws the + * THREAD's — which differ the moment somebody replies and the conversation + * keeps the original wording. Printing either would silently rewrite the + * subject line rather than highlight it, and a subject the reader cannot + * trust is worse than one that is not marked. + * + * So: the same string with markers in it, or nothing. `ShortWord=0` in the + * options is what makes the common case pass this guard at all — see + * SearchHighlighter::HEADLINE_OPTIONS. + */ + private function subject(MessageThread $thread, mixed $headline): ?Markup + { + if (null === $thread->subject || '' === $thread->subject) { + return null; + } + + if ($this->highlighter->withoutMarkers($headline) !== $thread->subject) { + return null; + } + + return $this->highlighter->toMarkup($headline); + } +} diff --git a/templates/_partials/_thread_row.html.twig b/templates/_partials/_thread_row.html.twig index ea8c90f0..41944237 100644 --- a/templates/_partials/_thread_row.html.twig +++ b/templates/_partials/_thread_row.html.twig @@ -199,6 +199,35 @@ Threads only. A bare Message row is a message, and only a conversation arrives. {% set rowId = item.id %} {% endif %} +{# +Search results, and the reason a row can show WHY it matched. + +`highlight` is `{ subject, snippet }`, either half of which may be null, built +by App\Service\Search\SearchResultHighlights from Postgres `ts_headline` over +the message the search actually hit. Only the search list passes it; every +other list — inbox, sent, drafts, trash, archive, spam, starred, snoozed, +labels, accounts, and the per-row turbo-streams under thread/status/ — passes +nothing, gets null here, and renders exactly as it did before this existed. + +The values are `Twig\Markup`: already escaped, with `` put back in +afterwards by the one class that did the escaping. That is what lets this print +them without `|raw`, which this file still does not contain. A sender's own +markup inside a fragment therefore arrives as visible, escaped text, which is +the same treatment the plain preview gives it — see the striptags note above. + +`subjectLine` and not `subject` itself, and that distinction is load-bearing: +`subject` is also spent on `aria-label` and on the select checkbox's label, and +a screen reader announcing "Oak for the <mark>alcove</mark>" would +be a worse regression than the one this feature fixes. +#} +{% set highlight = highlight|default(null) %} +{% set subjectLine = highlight.subject|default(subject) %} +{# No |slice on this one, unlike the plain preview above: the fragment is HTML + now, and cutting it at a hundred characters can land inside a tag. Its length + is bounded by MaxWords in SearchHighlighter::HEADLINE_OPTIONS instead, and + the row goes on truncating with CSS the way it always has. #} +{% set snippet = highlight.snippet|default(snippet) %} + {# `data-enter-NEW`, not `data-enter`, and the difference is the whole reason this can exist at all. @@ -471,7 +500,7 @@ Threads only. A bare Message row is a message, and only a conversation arrives.
- {{ subject }} + {{ subjectLine }} {% if count > 1 %} {{ 'thread_row.message_count'|trans({'{ count }': count}) }} {% endif %} diff --git a/templates/search/search.html.twig b/templates/search/search.html.twig index d2fb2239..e990c3be 100644 --- a/templates/search/search.html.twig +++ b/templates/search/search.html.twig @@ -124,10 +124,26 @@ a row the words found needs no badge — see the provenance column in MessageThreadRepository::buildSearchSql(). #} + {# + `highlights` is the other thing only this list passes: the subject + and the fragment of body text that actually matched, already marked + up and already escaped — see App\Service\Search\SearchHighlighter, + which is why there is no `|raw` here or in the row. + + Keyed by thread id and looked up rather than carried on the thread, + for the same reason as `semantic_only` above. A row the highlighter + found nothing in is simply absent from the map and draws exactly as + it does in every other list. + #} + {# Pulled out of the loop rather than defaulted inside it: `{}` in the + middle of an `include ... with` argument is one set of brackets away + from subscripting the empty hash instead of the map. #} + {% set row_highlights = highlights|default({}) %} {% for thread in threads %} {% include '_partials/_thread_row.html.twig' with { thread: thread, meaning_match: thread.id in semantic_only|default([]), + highlight: row_highlights[thread.id]|default(null), } %} {% endfor %} {% endif %} diff --git a/tests/Controller/Mail/SearchHighlightTest.php b/tests/Controller/Mail/SearchHighlightTest.php new file mode 100644 index 00000000..d8602b31 --- /dev/null +++ b/tests/Controller/Mail/SearchHighlightTest.php @@ -0,0 +1,248 @@ +client = static::createClient(); + + // The fixtures live in an open transaction on this connection, and a + // reboot between requests would take it with them. + $this->client->disableReboot(); + + $container = self::getContainer(); + $this->em = $container->get(EntityManagerInterface::class); + $this->connection = $container->get(Connection::class); + + $this->connection->beginTransaction(); + + $this->user = $this->seedUser(); + $this->account = $this->seedAccount(); + $this->inbox = $this->seedLabel('Inbox', LabelRole::Inbox); + + $this->client->loginUser($this->user); + } + + protected function tearDown(): void + { + if (true === $this->connection->isTransactionActive()) { + $this->connection->rollBack(); + } + + parent::tearDown(); + } + + /** + * The point of the whole feature: the preview moves to the match. + */ + public function testTheSnippetIsAWindowAroundTheMatchRatherThanTheOpeningOfTheBody(): void + { + $this->seedThread('Oak for the alcove'); + + $html = $this->search(self::TERM); + + self::assertStringContainsString( + 'chargecloud', + $html, + 'the term the search matched on is not marked in the row', + ); + + self::assertStringContainsString( + 'invoice for August', + $html, + 'the words beside the match are what make the row explain itself', + ); + + self::assertStringNotContainsString( + self::OPENING, + $html, + 'the row is still previewing the top of the body instead of the match', + ); + } + + /** + * The subject is highlighted in place, whole. + * + * `ts_headline` fragments a subject as readily as a body — at the default + * ShortWord it answers "Oak for the alcove" searched for `alcove` with the + * single word "alcove" — so a row printing the headline unguarded would + * silently rewrite the subject line. Both halves are asserted here: the + * mark, and the words around it that must have survived. + */ + public function testTheSubjectIsHighlightedWithoutBeingRewritten(): void + { + $this->seedThread('Oak for the alcove'); + + $html = $this->search('alcove'); + + self::assertStringContainsString('alcove', $html); + self::assertStringContainsString('Oak for the alcove', $html); + } + + /** + * An operator-only search has no term, so there is nothing to mark and no + * headline query to pay for — the row falls back to the preview every other + * list shows. + */ + public function testAnOperatorOnlySearchHighlightsNothing(): void + { + $this->seedThread('Oak for the alcove'); + + $html = $this->search('from:sender@example.test'); + + self::assertStringContainsString(self::OPENING, $html, 'the plain preview should be back'); + self::assertStringNotContainsString('', $html); + } + + /** + * The other ten lists that share this row partial. + * + * They pass no highlight, so nothing may be marked in them — a `` in + * the inbox would mean the search's per-page data had found its way onto + * the shared MessageThread entity, which is the failure the map-keyed-by-id + * shape exists to prevent. + */ + public function testAnOrdinaryListIsUntouched(): void + { + $this->seedThread('Oak for the alcove'); + + $this->client->request('GET', '/mail/inbox'); + + self::assertResponseIsSuccessful(); + + $html = (string) $this->client->getResponse()->getContent(); + + self::assertStringContainsString(self::OPENING, $html, 'the inbox lost its preview'); + self::assertStringNotContainsString('', $html); + } + + /** + * A message whose text part is markup. + * + * The fragment is body_text, which is the half of a mail some senders fill + * with HTML — and Twig cannot escape it, because it arrives as Markup that + * has already been escaped. If that escaping is ever dropped the tag lands + * in the list as an element, so this is the row-level twin of the JMAP + * test: see SearchSnippetGetMethodTest. + */ + public function testMarkupInAMatchedTextPartIsShownAsTextInTheRow(): void + { + $this->seedThread( + 'Rechnung', + 'Anbei die chargecloud Rechnung fuer August.', + ); + + $html = $this->search(self::TERM); + + self::assertStringContainsString('<img src=x onerror=alert(1)>', $html); + self::assertStringNotContainsString('client->request('GET', '/mail/search?q='.urlencode($query)); + + self::assertResponseIsSuccessful(); + + return (string) $this->client->getResponse()->getContent(); + } + + /** + * One conversation whose body buries the term a long way from the top. + * + * The padding is what makes the first claim testable: with the term in the + * opening line, the old preview and the new fragment would show the same + * words and the test would pass against both. + */ + private function seedThread(string $subject, ?string $body = null): MessageThread + { + $thread = new MessageThread(); + $thread->account = $this->account; + $thread->subject = $subject; + $thread->normalizedSubject = mb_strtolower($subject); + $thread->threadingMethod = ThreadingMethod::SubjectFallback; + $thread->lastMessageAt = new DateTimeImmutable('-2 hours'); + $thread->category = MessageCategory::Primary; + $thread->messageCount = 1; + $thread->unreadCount = 0; + $thread->addLabel($this->inbox); + + $this->em->persist($thread); + + $message = new Message(); + $message->account = $this->account; + $message->thread = $thread; + $message->subject = $subject; + $message->fromAddress = 'sender@example.test'; + $message->fromName = 'Sender'; + $message->receivedAt = new DateTimeImmutable('-2 hours'); + $message->sentAt = $message->receivedAt; + $message->seenAt = $message->receivedAt; + $message->flags = []; + $message->hasAttachments = false; + $message->bodyText = $body ?? ( + self::OPENING.' ' + .str_repeat('padding word here plus more filler. ', 10) + .'The chargecloud invoice for August is attached and needs paying.' + ); + + $thread->addMessage($message); + $this->em->persist($message); + + $this->em->flush(); + $this->em->clear(); + + return $thread; + } +} diff --git a/tests/Jmap/Method/Mail/SearchSnippetGetMethodTest.php b/tests/Jmap/Method/Mail/SearchSnippetGetMethodTest.php new file mode 100644 index 00000000..f688ebfc --- /dev/null +++ b/tests/Jmap/Method/Mail/SearchSnippetGetMethodTest.php @@ -0,0 +1,133 @@ +` produces a string in + * which the sender's `` and our `` are the same + * kind of thing, and the "does it contain " check that stood in for + * escaping cannot tell them apart. + * + * Per RFC 8621 §5 these strings ARE HTML, so a client is right to render them. + * That is what makes this a security bug rather than a display one: the mail + * does not have to be opened for the markup to run. + * + * Against the real database rather than a double, because the thing under test + * is what Postgres actually does with a document containing markup — which is + * not what the reasonable guess says. Well-formed tags are dropped by the + * parser (`` vanishes, leaving a double space); malformed ones, which is + * most of what a real sender produces, come through untouched. A PHP fake of + * `ts_headline` would have to encode one of those guesses and would then agree + * with itself forever. + */ +final class SearchSnippetGetMethodTest extends JmapTestCase +{ + /** + * Unclosed quotes and an unquoted attribute, which is what stops the + * Postgres parser from classifying this as a tag and dropping it. Verified + * against Postgres 18: it comes back verbatim. + */ + private const string SUBJECT = 'Angebot chargecloud und im Anhang'; + + private const string BODY = 'Anbei die chargecloud Rechnung fuer August.'; + + private SearchSnippetGetMethod $method; + + protected function setUp(): void + { + parent::setUp(); + + $this->method = self::getContainer()->get(SearchSnippetGetMethod::class); + } + + public function testMarkupInAMatchedSubjectIsEscapedRatherThanShippedAsHtml(): void + { + $snippet = $this->snippetFor(self::SUBJECT, self::BODY); + + $subject = (string) $snippet['subject']; + + self::assertStringContainsString( + 'chargecloud', + $subject, + 'the term the search matched on is what is for', + ); + + self::assertStringNotContainsString( + 'snippetFor(self::SUBJECT, self::BODY); + + $preview = (string) $snippet['preview']; + + self::assertStringContainsString('chargecloud', $preview); + self::assertStringNotContainsString('snippetFor('Nothing to do with it', self::BODY); + + self::assertNull($snippet['subject']); + self::assertNotNull($snippet['preview']); + } + + /** + * One message, seeded with the given text, put through the method. + * + * @return array{emailId: string, subject: ?string, preview: ?string} + */ + private function snippetFor(string $subject, string $body): array + { + $message = $this->receivedMessage(); + $message->subject = $subject; + $message->bodyText = $body; + + $this->em->flush(); + + $result = $this->method->handle( + [ + 'accountId' => $this->accountId(), + 'emailIds' => [(string) $message->id], + 'filter' => ['text' => 'chargecloud'], + ], + $this->context(), + ); + + /** @var list $list */ + $list = $result['list']; + + self::assertCount(1, $list, 'the message seeded above should be the one snippet'); + + return $list[0]; + } +}