mirror of
https://github.com/karatektus/pl_mail.git
synced 2026-10-25 01:54:10 +00:00
feat(search): show which words a result matched on
A row asserted a match and then showed something else. The preview was the opening hundred characters of the body, which for most hits does not contain the search term at all, and nothing was marked — so the only way to learn why a conversation was in the list was to open it. Every occurrence is marked now, in the subject and in the preview, and the preview is a window of the message AROUND the match. The fragment comes from the same Postgres `ts_headline` the JMAP snippet has always used, over the same query that found the mail, so the marking is stemmed exactly as the search is and cannot mark something the search did not match on. **It also closes a hole that was open in the JMAP API.** `ts_headline` is a highlighter and not an escaper: it inserts the delimiters it is given and returns the rest of the document byte for byte. Asking it for `StartSel=<mark>` produced a string in which the sender's markup and ours were the same kind of thing, and the check that stood in for escaping — "does it contain `<mark>`" — could not tell them apart. A subject or text part carrying `<img src=x onerror=…>` shipped it to every client as snippet HTML, and per RFC 8621 a client is right to render that. The class docblock claimed the opposite and had done since it was written. The delimiters are two control characters now. The whole string is escaped and only then are they swapped, pairs only, in one class that owns both the options and the conversion so they cannot be changed apart. `toHtml()` answers JMAP; `toMarkup()` answers Twig, which is what lets the row print a fragment without `|raw` — that file still does not contain one. Three things worth knowing: - `ShortWord` went from 3 to 0. At 3 `ts_headline` trims short leading words, so "Oak for the alcove" searched for `alcove` came back as the bare word — a row that has rewritten the subject rather than highlighted it. - A highlighted subject is used only when it still equals the thread's subject with the markers removed; anything else falls back. And it goes to a separate variable, because `subject` is also spent on `aria-label`, where a screen reader announcing the markup would be a worse regression than the one this fixes. - The headline replaces the preview only when it actually contains a marker. That makes the two documented `body_text` regressions self-sorting: a bulk sender's "this email is only available in HTML" does not contain the term, so it does not win. One batched query per account per page, not one per row: the budget test is a hard ceiling and a per-row headline would blow it instantly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
a1af660b45
commit
8dc22b49c0
12 files changed
+980
-25
No files matched your search
@@ -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 <mark>, 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,
|
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.
|
where --unread-bar-w is 3px; it is 0px at the other two and paints nothing.
|
||||||
|
|||||||
@@ -1,4 +1,4 @@
|
|||||||
<!-- translated-from: features/mail.md sha1:18987bc807bfd29e4f726b4a17bc34276b36ef3f -->
|
<!-- translated-from: features/mail.md sha1:cbfea3b01388d5fcb0bc6f3a896fb7d494ed1f77 -->
|
||||||
|
|
||||||
# Mail
|
# 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 —
|
Eine Abfrage, die aus nichts als einem halb getippten Operator besteht — `from:` ohne Wert —
|
||||||
liefert eine leere Seite und nicht das ganze Postfach.
|
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
|
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
|
**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
|
angekommen ist. Was du wählst, wird für deine nächste Suche gemerkt, und das Blättern behält es
|
||||||
|
|||||||
@@ -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
|
A query that is nothing but a half-typed operator — `from:` with no value — returns an empty page
|
||||||
rather than the whole mailbox.
|
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
|
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
|
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
|
is remembered for your next search, and paging keeps it. Switching orders starts again at the first
|
||||||
|
|||||||
@@ -12,6 +12,7 @@ use App\Entity\User\User;
|
|||||||
use App\Repository\Mail\MessageThreadRepository;
|
use App\Repository\Mail\MessageThreadRepository;
|
||||||
use App\Service\Mail\ThreadListRenderer;
|
use App\Service\Mail\ThreadListRenderer;
|
||||||
use App\Service\Search\SearchQueryParser;
|
use App\Service\Search\SearchQueryParser;
|
||||||
|
use App\Service\Search\SearchResultHighlights;
|
||||||
use Doctrine\ORM\EntityManagerInterface;
|
use Doctrine\ORM\EntityManagerInterface;
|
||||||
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
|
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
|
||||||
use Symfony\Component\HttpFoundation\Request;
|
use Symfony\Component\HttpFoundation\Request;
|
||||||
@@ -31,6 +32,7 @@ final class SearchController extends AbstractController
|
|||||||
private readonly SemanticQuery $semantic,
|
private readonly SemanticQuery $semantic,
|
||||||
private readonly SemanticCoverage $coverage,
|
private readonly SemanticCoverage $coverage,
|
||||||
private readonly EmbeddingCatchUp $catchUp,
|
private readonly EmbeddingCatchUp $catchUp,
|
||||||
|
private readonly SearchResultHighlights $highlights,
|
||||||
) {}
|
) {}
|
||||||
|
|
||||||
#[Route('', name: '', methods: ['GET'])]
|
#[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.
|
// at a time on top of everything the inbox was already paying.
|
||||||
$this->threadRepository->preloadForRows($threads);
|
$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
|
// Through ThreadListRenderer, not $this->render(): a row whose subject
|
||||||
// and sender the user has just read in a result list has been SHOWN,
|
// 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
|
// and leaving it badged would mean finding your own search results
|
||||||
@@ -194,6 +215,7 @@ final class SearchController extends AbstractController
|
|||||||
'sort' => $sort,
|
'sort' => $sort,
|
||||||
'semantic' => $report,
|
'semantic' => $report,
|
||||||
'semantic_only' => $results->semanticOnly,
|
'semantic_only' => $results->semanticOnly,
|
||||||
|
'highlights' => $highlights,
|
||||||
]);
|
]);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -9,6 +9,7 @@ use App\Jmap\Method\JmapMethod;
|
|||||||
use App\Jmap\Protocol\Exception\MethodException;
|
use App\Jmap\Protocol\Exception\MethodException;
|
||||||
use App\Jmap\Protocol\JmapContext;
|
use App\Jmap\Protocol\JmapContext;
|
||||||
use App\Repository\Mail\MessageRepository;
|
use App\Repository\Mail\MessageRepository;
|
||||||
|
use App\Service\Search\SearchHighlighter;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* "SearchSnippet/get" (RFC 8621 §5).
|
* "SearchSnippet/get" (RFC 8621 §5).
|
||||||
@@ -26,6 +27,14 @@ use App\Repository\Mail\MessageRepository;
|
|||||||
* Per the spec the strings are HTML, with `<mark>` around each hit. Everything
|
* Per the spec the strings are HTML, with `<mark>` around each hit. Everything
|
||||||
* else is escaped: this is message content being handed back for display, and
|
* 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.
|
* 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=<mark>` 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
|
final class SearchSnippetGetMethod implements JmapMethod
|
||||||
{
|
{
|
||||||
@@ -39,6 +48,7 @@ final class SearchSnippetGetMethod implements JmapMethod
|
|||||||
public function __construct(
|
public function __construct(
|
||||||
private readonly AccountResolver $accountResolver,
|
private readonly AccountResolver $accountResolver,
|
||||||
private readonly MessageRepository $messages,
|
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
|
private function snippets(int $accountId, array $requested, string $text): array
|
||||||
{
|
{
|
||||||
// Options chosen so a snippet is a snippet: one fragment, short enough
|
// The options are the highlighter's, not this method's: they name the
|
||||||
// to sit on a list row, and no ellipsis of our own — the client decides
|
// delimiters it converts, and the web search page asks for the same
|
||||||
// how to truncate for its width.
|
// ones. Two callers spelling them out separately is how one of them
|
||||||
$options = 'StartSel=<mark>, StopSel=</mark>, MaxWords=24, MinWords=8, '
|
// ends up highlighting nothing.
|
||||||
.'ShortWord=3, MaxFragments=1, FragmentDelimiter=" … "';
|
|
||||||
|
|
||||||
$rows = $this->messages->findSearchHeadlines(
|
$rows = $this->messages->findSearchHeadlines(
|
||||||
$accountId,
|
$accountId,
|
||||||
array_map('intval', $requested),
|
array_map('intval', $requested),
|
||||||
$text,
|
$text,
|
||||||
$options,
|
SearchHighlighter::HEADLINE_OPTIONS,
|
||||||
);
|
);
|
||||||
|
|
||||||
$list = [];
|
$list = [];
|
||||||
@@ -124,9 +132,10 @@ final class SearchSnippetGetMethod implements JmapMethod
|
|||||||
'emailId' => $id,
|
'emailId' => $id,
|
||||||
// Null rather than the whole field when nothing matched in it:
|
// Null rather than the whole field when nothing matched in it:
|
||||||
// a "snippet" that is just the subject again tells the reader
|
// a "snippet" that is just the subject again tells the reader
|
||||||
// nothing about why the message came back.
|
// nothing about why the message came back. toHtml() answers
|
||||||
'subject' => $this->highlighted($row['subject']),
|
// null for exactly that case.
|
||||||
'preview' => $this->highlighted($row['preview']),
|
'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, '<mark>') ? $value : null;
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* The free-text term the query ran on, or null when it had none.
|
* The free-text term the query ran on, or null when it had none.
|
||||||
*
|
*
|
||||||
|
|||||||
@@ -684,6 +684,13 @@ statement that creates the need for it.
|
|||||||
**stemmed**: a search for `running` marks `run`, and a term whose stem
|
**stemmed**: a search for `running` marks `run`, and a term whose stem
|
||||||
differs from the literal text still highlights. Re-implementing either in PHP
|
differs from the literal text still highlights. Re-implementing either in PHP
|
||||||
would let a snippet highlight something the search did not match on.
|
would let a snippet highlight something the search did not match on.
|
||||||
|
- **A snippet is HTML, and `<mark>` 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
|
- **Calendars are advertised as `urn:plmail:params:jmap:calendars`, not
|
||||||
`urn:ietf:params:jmap:calendars`.** JMAP for Calendars is an unratified draft
|
`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
|
whose object shape is still moving; claiming its URN would promise a contract
|
||||||
|
|||||||
@@ -0,0 +1,180 @@
|
|||||||
|
<?php
|
||||||
|
|
||||||
|
declare(strict_types=1);
|
||||||
|
|
||||||
|
namespace App\Service\Search;
|
||||||
|
|
||||||
|
use Twig\Markup;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Postgres `ts_headline` output, turned into highlight HTML that is safe to
|
||||||
|
* hand to a reader.
|
||||||
|
*
|
||||||
|
* `ts_headline` is a highlighter, not an escaper. It walks the document with
|
||||||
|
* the same parser the tsvector was built with, inserts StartSel before and
|
||||||
|
* StopSel after each matching lexeme, and returns EVERYTHING ELSE VERBATIM —
|
||||||
|
* angle brackets, quotes, attributes and all. Asking it for
|
||||||
|
* `StartSel=<mark>` therefore produces a string in which our markup and the
|
||||||
|
* sender's markup are indistinguishable:
|
||||||
|
*
|
||||||
|
* ts_headline('english', '<img src=x onerror=alert(1)> chargecloud invoice',
|
||||||
|
* websearch_to_tsquery('english', 'chargecloud'), 'StartSel=<mark>, …')
|
||||||
|
* → 'onerror=alert(1)> <mark>chargecloud</mark> 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 <mark>" 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.'/',
|
||||||
|
'<mark>$1</mark>',
|
||||||
|
$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);
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -0,0 +1,268 @@
|
|||||||
|
<?php
|
||||||
|
|
||||||
|
declare(strict_types=1);
|
||||||
|
|
||||||
|
namespace App\Service\Search;
|
||||||
|
|
||||||
|
use App\Entity\Mail\Message;
|
||||||
|
use App\Entity\Mail\MessageThread;
|
||||||
|
use App\Repository\Mail\MessageRepository;
|
||||||
|
use Twig\Markup;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The highlighted subject and the match-aware preview for one page of search
|
||||||
|
* results.
|
||||||
|
*
|
||||||
|
* What it fixes: a result row used to show the opening hundred characters of
|
||||||
|
* the newest message, which for most hits does not contain the search term at
|
||||||
|
* all. The row said a conversation matched and then showed something else, so
|
||||||
|
* the only way to find out why it was there was to open it — which is the
|
||||||
|
* failure a ranked search exists to avoid. Gmail's answer, and now this one, is
|
||||||
|
* a window of body text AROUND the match with every occurrence marked.
|
||||||
|
*
|
||||||
|
* WHY THIS IS NOT HUNG ON THE THREAD. MessageThread is a mapped entity shared
|
||||||
|
* with every list in the application; "how this particular search found you" is
|
||||||
|
* a property of the search and not of the conversation, and a thread hydrated
|
||||||
|
* here goes into Doctrine's identity map carrying the answer to somebody else's
|
||||||
|
* question for the rest of the request. Same reasoning, and the same shape of
|
||||||
|
* answer, as the `semanticOnly` id list — see
|
||||||
|
* MessageThreadRepository::searchPage().
|
||||||
|
*
|
||||||
|
* THE BRIDGE FROM THREADS TO MESSAGES. `ts_headline` runs over a MESSAGE: the
|
||||||
|
* subject and text part it highlights are columns of `message`, and the search
|
||||||
|
* vector it agrees with is generated from those same columns. A result page is
|
||||||
|
* a list of THREADS and spans every account in a unified mailbox, so this walks
|
||||||
|
* the rows the page is already holding — `preloadForRows()` has hydrated their
|
||||||
|
* messages by the time this runs, so the ids cost nothing — groups them by the
|
||||||
|
* THREAD's account, and asks for one batch of headlines per account.
|
||||||
|
*
|
||||||
|
* The thread's account rather than each message's: a thread belongs to exactly
|
||||||
|
* one account (that is what `uniq_message_thread_provider_key_account` means),
|
||||||
|
* so the two agree, and reading `$message->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<int, array{subject: ?Markup, snippet: ?Markup}>
|
||||||
|
*/
|
||||||
|
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<int, list<int>>
|
||||||
|
*/
|
||||||
|
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<Message>
|
||||||
|
*/
|
||||||
|
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<int, array{id: int|string, subject: mixed, preview: mixed}> $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);
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -199,6 +199,35 @@ Threads only. A bare Message row is a message, and only a conversation arrives.
|
|||||||
{% set rowId = item.id %}
|
{% set rowId = item.id %}
|
||||||
{% endif %}
|
{% 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 `<mark>` 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
|
{# `data-enter-NEW`, not `data-enter`, and the difference is the whole reason
|
||||||
this can exist at all.
|
this can exist at all.
|
||||||
|
|
||||||
@@ -471,7 +500,7 @@ Threads only. A bare Message row is a message, and only a conversation arrives.
|
|||||||
<div class="min-w-0 flex flex-col @xl:flex-row @xl:items-baseline @xl:gap-2">
|
<div class="min-w-0 flex flex-col @xl:flex-row @xl:items-baseline @xl:gap-2">
|
||||||
<span class="truncate text-sm mt-1 @xl:mt-0 @xl:flex-shrink-0 @xl:max-w-[45%]
|
<span class="truncate text-sm mt-1 @xl:mt-0 @xl:flex-shrink-0 @xl:max-w-[45%]
|
||||||
{{ unread ? 'font-semibold text-ink' : 'font-normal text-ink-soft' }}">
|
{{ unread ? 'font-semibold text-ink' : 'font-normal text-ink-soft' }}">
|
||||||
{{ subject }}
|
{{ subjectLine }}
|
||||||
{% if count > 1 %}
|
{% if count > 1 %}
|
||||||
<span class="ml-1 text-xs font-medium text-ink-faint">{{ 'thread_row.message_count'|trans({'{ count }': count}) }}</span>
|
<span class="ml-1 text-xs font-medium text-ink-faint">{{ 'thread_row.message_count'|trans({'{ count }': count}) }}</span>
|
||||||
{% endif %}
|
{% endif %}
|
||||||
|
|||||||
@@ -124,10 +124,26 @@
|
|||||||
a row the words found needs no badge — see the provenance column in
|
a row the words found needs no badge — see the provenance column in
|
||||||
MessageThreadRepository::buildSearchSql().
|
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 %}
|
{% for thread in threads %}
|
||||||
{% include '_partials/_thread_row.html.twig' with {
|
{% include '_partials/_thread_row.html.twig' with {
|
||||||
thread: thread,
|
thread: thread,
|
||||||
meaning_match: thread.id in semantic_only|default([]),
|
meaning_match: thread.id in semantic_only|default([]),
|
||||||
|
highlight: row_highlights[thread.id]|default(null),
|
||||||
} %}
|
} %}
|
||||||
{% endfor %}
|
{% endfor %}
|
||||||
{% endif %}
|
{% endif %}
|
||||||
|
|||||||
@@ -0,0 +1,248 @@
|
|||||||
|
<?php
|
||||||
|
|
||||||
|
declare(strict_types=1);
|
||||||
|
|
||||||
|
namespace App\Tests\Controller\Mail;
|
||||||
|
|
||||||
|
use App\Domain\Enum\Mail\LabelRole;
|
||||||
|
use App\Domain\Enum\Mail\MessageCategory;
|
||||||
|
use App\Domain\Enum\Mail\ThreadingMethod;
|
||||||
|
use App\Entity\Mail\Message;
|
||||||
|
use App\Entity\Mail\MessageThread;
|
||||||
|
use App\Tests\Support\Mail\SeedsMarkerFixtures;
|
||||||
|
use DateTimeImmutable;
|
||||||
|
use Doctrine\DBAL\Connection;
|
||||||
|
use Doctrine\ORM\EntityManagerInterface;
|
||||||
|
use Symfony\Bundle\FrameworkBundle\KernelBrowser;
|
||||||
|
use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* A search result says why it is a search result.
|
||||||
|
*
|
||||||
|
* The row used to show the first hundred characters of the newest message,
|
||||||
|
* which for most hits does not contain the term that found it: the list
|
||||||
|
* asserted a match and then showed something else, and the only way to find
|
||||||
|
* out why a conversation was there was to open it. The fragment now comes from
|
||||||
|
* `ts_headline`, which is a window of the body AROUND the match, and every
|
||||||
|
* occurrence in it — and in the subject — is marked.
|
||||||
|
*
|
||||||
|
* Two claims, and the second is the one that constrains the implementation:
|
||||||
|
*
|
||||||
|
* 1. the fragment is match-aware, so the words beside the term are shown and
|
||||||
|
* the opening of the body is not;
|
||||||
|
* 2. NOTHING ELSE CHANGES. The row partial is shared with ten other lists and
|
||||||
|
* with the per-row turbo-streams; all of them pass no highlight and must
|
||||||
|
* render exactly as they did. The inbox case below is what holds that.
|
||||||
|
*
|
||||||
|
* Against the real database because `ts_headline` is the subject: which words
|
||||||
|
* land either side of a match, and whether a short subject survives being
|
||||||
|
* fragmented at all, are Postgres's answers and not ours.
|
||||||
|
*/
|
||||||
|
final class SearchHighlightTest extends WebTestCase
|
||||||
|
{
|
||||||
|
use SeedsMarkerFixtures;
|
||||||
|
|
||||||
|
/** Far enough down the body that the fragment around the term excludes it. */
|
||||||
|
private const string OPENING = 'Kopfzeile ganz oben und sonst nichts.';
|
||||||
|
|
||||||
|
private const string TERM = 'chargecloud';
|
||||||
|
|
||||||
|
private KernelBrowser $client;
|
||||||
|
private Connection $connection;
|
||||||
|
|
||||||
|
protected function setUp(): void
|
||||||
|
{
|
||||||
|
$this->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(
|
||||||
|
'<mark>chargecloud</mark>',
|
||||||
|
$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('<mark>alcove</mark>', $html);
|
||||||
|
self::assertStringContainsString('Oak for the <mark>alcove</mark>', $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('<mark>', $html);
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The other ten lists that share this row partial.
|
||||||
|
*
|
||||||
|
* They pass no highlight, so nothing may be marked in them — a `<mark>` 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('<mark>', $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 <img src=x onerror=alert(1)> die chargecloud Rechnung fuer August.',
|
||||||
|
);
|
||||||
|
|
||||||
|
$html = $this->search(self::TERM);
|
||||||
|
|
||||||
|
self::assertStringContainsString('<img src=x onerror=alert(1)>', $html);
|
||||||
|
self::assertStringNotContainsString('<img src=x', $html);
|
||||||
|
}
|
||||||
|
|
||||||
|
// ── fixtures ──────────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
/** The rendered search page for a query. */
|
||||||
|
private function search(string $query): string
|
||||||
|
{
|
||||||
|
$this->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;
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -0,0 +1,133 @@
|
|||||||
|
<?php
|
||||||
|
|
||||||
|
declare(strict_types=1);
|
||||||
|
|
||||||
|
namespace App\Tests\Jmap\Method\Mail;
|
||||||
|
|
||||||
|
use App\Jmap\Method\Mail\SearchSnippetGetMethod;
|
||||||
|
use App\Tests\Jmap\JmapTestCase;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* A snippet is message content, and it may not carry markup of its own.
|
||||||
|
*
|
||||||
|
* This class docblock made that claim for months while the opposite shipped.
|
||||||
|
* `ts_headline` is a highlighter, not an escaper: it inserts StartSel and
|
||||||
|
* StopSel around the matching lexemes and returns the rest of the document
|
||||||
|
* byte for byte — so asking it for `StartSel=<mark>` produces a string in
|
||||||
|
* which the sender's `<img src=x onerror=…>` and our `<mark>` are the same
|
||||||
|
* kind of thing, and the "does it contain <mark>" 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 (`<b>` 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 <b>chargecloud</b> und <img src=x onerror=alert(1)> im Anhang';
|
||||||
|
|
||||||
|
private const string BODY = 'Anbei <img src=x onerror=alert(1)> 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(
|
||||||
|
'<mark>chargecloud</mark>',
|
||||||
|
$subject,
|
||||||
|
'the term the search matched on is what <mark> is for',
|
||||||
|
);
|
||||||
|
|
||||||
|
self::assertStringNotContainsString(
|
||||||
|
'<img',
|
||||||
|
$subject,
|
||||||
|
'the sender\'s markup reached a client as live HTML — ts_headline does not escape',
|
||||||
|
);
|
||||||
|
|
||||||
|
self::assertStringContainsString(
|
||||||
|
'<img src=x onerror=alert(1)>',
|
||||||
|
$subject,
|
||||||
|
'escaped, the tag is shown; it is the one thing a snippet must not be able to do',
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
/** The same document, the same treatment — the body is the longer half. */
|
||||||
|
public function testMarkupInAMatchedBodyIsEscapedRatherThanShippedAsHtml(): void
|
||||||
|
{
|
||||||
|
$snippet = $this->snippetFor(self::SUBJECT, self::BODY);
|
||||||
|
|
||||||
|
$preview = (string) $snippet['preview'];
|
||||||
|
|
||||||
|
self::assertStringContainsString('<mark>chargecloud</mark>', $preview);
|
||||||
|
self::assertStringNotContainsString('<img', $preview);
|
||||||
|
self::assertStringContainsString('<img', $preview);
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* `ts_headline` answers with the opening words of a field that holds no
|
||||||
|
* hit, and a preview that is just the first line again says nothing about
|
||||||
|
* why the message came back. The absence of a marker is the only signal
|
||||||
|
* there is, so it has to mean null and not "here is some text anyway".
|
||||||
|
*/
|
||||||
|
public function testAFieldWithNoHitIsNullRatherThanItsOpeningWords(): void
|
||||||
|
{
|
||||||
|
$snippet = $this->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<array{emailId: string, subject: ?string, preview: ?string}> $list */
|
||||||
|
$list = $result['list'];
|
||||||
|
|
||||||
|
self::assertCount(1, $list, 'the message seeded above should be the one snippet');
|
||||||
|
|
||||||
|
return $list[0];
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in new issue
Block a user