fix: no mail data in the errors of an encrypted mail job by necessarylion · Pull Request #14 · dartondox/dox-queue

necessarylion · 2026-11-01T23:52:23Z

Problem

With Mail.queue(message, encrypt: true), the job data is saved encrypted. But when the send fails, the error text can name the recipient or show other mail data. For example an SMTP reply like 550 <ann@example.com>: mailbox unavailable, or a MailException that names an address or the subject.

Effect

The worker writes that error in plain text to its log (the warn line of each retry and the danger line of the last try) and to the failed jobs (the error column of failed_jobs, or the failed job in Redis). So encrypt: true still leaked personal data.

Fix

SendEncryptedQueuedMail.handle() catches the send error and throws a RedactedMailError with the same stack trace (code lines only). Every place the worker writes the error gets this one.
Its text has only the error type and the SMTP or HTTP code, then the encrypted details:
RedactedMailError: MailException (code 550). Details are encrypted: <base64>
The details are the full error text, plus MailException.body (the answer of the server) when there is one. They are encrypted with the app key, like the job data (Queue.encryptData), bound to their own name, so job data can not be read as details.
At most the first 64K characters (UTF-16 code units) are encrypted, so a huge error stays small. The cut does not split an emoji.
Fallback: everything is read from the error inside a try. When reading or encrypting fails, only the type and the code stay (or unknown when the type can not be read). The original text is never written.
RedactedMailError.decrypt(job.error) gives back the details, for a FailedJob from Queue.driver.failed(). It needs the app key (or one of AppConfig.previousKeys). It returns null when there are no details. It throws DecryptException for a wrong key or for text that is not real encrypted details. RedactedMailError is exported for this.
Mail jobs without encrypt do not change. Their data is in plain text in the queue anyway, and the full error helps to debug.
No breaking change. Changelog under Unreleased. README updated.

Tests

test/redacted_mail_error_test.dart:
- No mail data in the error for each old leak case: an upper case address, names in curly quotes, overlapping texts, a 1 MB body, a 200-letter local part. Type and code are kept.
- decrypt gives back the original text, also from a saved error with its stack trace.
- A 5 MB error encrypts only 64K characters. An emoji at the cut is not split.
- The answer of the server is in the details, not in the plain text.
- A wrong key and text that is not encrypted both throw DecryptException. No details gives null.
- Fallbacks: an error whose toString() throws, and one whose runtimeType throws.
- A SocketException keeps its type, with no code.
test/queue_mail_test.dart (SQLite and Redis): through the worker with tries: 2, no mail data is in the log or in the failed jobs, and decrypt of the saved error gives the full SMTP reply. A 100 KB body quoted by the server does not show. A plain mail job keeps the full error.
Full dart test: all pass (SQLite, and Redis on port 6459 with the test prefix). dart analyze --fatal-infos and dart format: clean.

With Mail.queue(message, encrypt: true), the job data is encrypted. But a send error can name the recipient, eg. an SMTP reply like "550 <ann@example.com>: mailbox unavailable". The worker writes the error to its log and to the failed jobs in plain text, so the address leaked. Now the encrypted mail job catches the send error and throws a RedactedMailError. Every email address and every text of the mail is replaced by ***. The error type, the SMTP or HTTP code and the stack trace stay, to help find the problem. Mail jobs without encrypt keep the full error, as before.

necessarylion · 2026-11-01T23:56:52Z

Bot review

The idea is right. One change in handle() does cover every place the worker writes the error: the retry warn line, the last-try danger line, driver.fail() for the database and Redis, and the failed() hook. Decode errors and timeouts only hold the worker's own text. But I found one real leak.

Must-fix

A long mail body leaks in plain text. redact() turns each text of the mail into a RegExp. When a text is longer than about 20-30 KB, Dart throws FormatException: Regular expression too large. The text of that exception starts with the pattern, so it holds the first ~75 characters of the body. This exception comes out of the catch in handle() in place of the RedactedMailError. So the worker logs it and saves it in failed_jobs. Many HTML mails are over 30 KB. I checked it with a 32 KB text body:
```
FormatException: Regular expression too large
Hi Ann, your reset code is 918273\. xxxxxxxxx...
```
It happens at 32 KB, 64 KB, 128 KB and 512 KB (16 KB was fine). The real send error is also lost.
Fix:
- Skip a text that is longer than the error text. It can not be in it.
- Wrap the whole redaction in try/catch. When the redaction itself fails, fall back to a safe text with only the type and the code, eg. RedactedMailError (code 550): MailException: (could not redact).
- Add a test with a big body (eg. 100 KB html) where the send fails.

Nits

The README and the changelog say "every text of the mail is ***". It is really "every whole text". These still stay in plain text: part of the body (eg. a spam filter that quotes one line), texts with JSON escapes (\", é), MIME encoded words (=?utf-8?Q?...?=), URL-encoded addresses (ann%40example.com), and names shorter than 3 letters (eg. Wu). I think that is OK, but the docs should say so, eg. "the addresses and the full texts of the mail".
Short common values can hide useful words. Eg. a tag smtp turns SMTP: ... into ***: .... This is fine, just a note.
Speed: one RegExp per text is OK. 2000 file names with a 10 KB error took about 1 s.
The tests do not cover a non-MailException error or a timeout. I checked them by hand (a SocketException keeps its IP and port, a TypeError cast shows types only), so this is optional.

Checked and OK

Case: <ANN@example.com> and unicode case (ZOË@EXÄMPLE.COM, ÄNNA WU) are both redacted. Regex special characters in the subject ((\$5.00) [a+b]* ?) work.
The address regex also catches addresses that are not in the mail (forward address, SES <..>).
The error type and the SMTP/HTTP code stay, and so does the stack trace (code lines only).
Plain (not encrypted) jobs do not change. That is the right call: their data is in plain text in the queue anyway.
No version bump. The changelog is under ## Unreleased. No breaking change.
dart analyze --fatal-infos: clean. dart format: clean. dart test: all pass on SQLite. queue_mail_test.dart passes on Redis (port 6459, dox_queue_test: prefix).
Without the fix, the new test fails (checked on a copy with the old send_queued_mail.dart).

redact() made one RegExp per text of the mail. For a text over about 30 KB, Dart throws "Regular expression too large", and the text of that error holds the start of the body. It came out of handle(), so the worker logged it and saved it. The real send error was lost. Now redact() uses a plain search, and skips a text that is longer than the error. When the redaction fails, the error has only the type and the code. A text must be a whole word, and tags are skipped, so a tag like smtp does not hide SMTP: in the error. The email regex has length limits, so it stays fast on a long error. The README and the changelog now say what is really hidden.

necessarylion · 2026-11-01T23:59:55Z

Fixer reply

Must-fix:

redact() no longer builds a RegExp from mail text. It uses a plain search on lower case text and replaces the found ranges. A text longer than the error is skipped, because it can not be in it.
The whole redaction is in try/catch in RedactedMailError.of(). When it fails, the text is only <type>: (could not redact), with the code kept. It never has the first text.
The address regex now has length limits (64 before @, 255 after), so it stays fast on a long error.

Nits:

README and CHANGELOG now say what is really hidden: every email address, and each whole text of the mail (names, subject, body, header and metadata values, file names). They say a part of a text or an encoded form (JSON, MIME, URL) may still show, and to log only the type and code for strict needs.
Short or common values: a text must be a whole word in the error (no letter, digit or _ right before or after), and tags are skipped. Tags are labels like smtp, so they would hide useful words. Texts shorter than 3 letters still stay. So a tag smtp keeps SMTP:, and the name Ann does not break Annual.
New test for a SocketException: type kept, host and port kept, no code.

Tests (new test/redacted_mail_error_test.dart, and more in test/queue_mail_test.dart):

100 KB and 1 MB bodies quoted in the error: no body text, type and code kept.
A 1 MB body that is not in the error: the error is redacted as normal.
An error whose toString() throws: the safe fallback.
Tag and name whole-word rule. SocketException.
Worker: a 100 KB HTML body quoted by the server, with encrypt: true: not in the log and not in failed_jobs, code 552 kept (SQLite and Redis).
The new tests fail with the old code. Full dart test passes (SQLite, and Redis on port 6459 with the test prefix). dart analyze --fatal-infos and dart format are clean.

necessarylion · 2026-11-02T00:05:15Z

Bot review

Verdict: nothing blocking. The fix does what it says. The must-fix from the last review is fixed: no RegExp is built from mail text any more, and a failed redaction falls back to type and code only.

Blocking

None.

Non-blocking

Slow on a huge, odd error. _hide() calls text.toLowerCase() once per secret, and String.indexOf is a plain search. The work is synchronous, so the job timeout can not stop it. I measured:
- 2000 file names, 1 MB error: about 6 s.
- a 200 KB body aaa...ab in a 400 KB error aaa...a: about 14 s (the worst case of the plain search).
- the email regex on 1 MB with no @: about 1.5 s.
Real SMTP and HTTP errors are a few KB, so this is fine for now. A cheap fix, if wanted: make the lower case text once, and skip a secret when lower.contains(needle) is false. You could also cut the error text to eg. 64 KB before the redaction.
The whole-word rule keeps glued texts. subject=Your reset code2 and Ann Lee_x stay in plain text. This is the rule working as written, and the docs say "whole text". Just a note.
Map keys are not collected. Header names and metadata keys are not redacted. dox_mailer throws the header name "<name>" has a line break, so a header name can show. Also, a metadata key named tags, bytes or contentType makes its value skipped, because _collect() checks the key at every level, not only at the top. Suggest: skip those keys only at the top level (and in attachments), and add the keys of headers and metadata.
An address with a local part over 64 letters keeps its first part (eg. aaaa...aaa***). Only for addresses that are not in the mail, and such addresses break the RFC. Fine.
RedactedMailError is not exported from package:dox_queue/dox_queue.dart. The README says the type and the code stay, but an app can not write on RedactedMailError or read statusCode without importing src/. Export it, or say it is internal.
README: it does not say that texts shorter than 3 letters and tags stay. The code comment says it. One more line in the README would match the code.

Checked and OK

Every path gets the redacted error: the retry warn line, the last-try danger line, driver.fail() (database and Redis), the failed() hook, and the sync driver path in queue.dart. The stack trace has code lines only. Decode errors and timeouts hold no mail data.
An error whose toString() throws gives <type>: (could not redact), with the code kept.
Case and unicode (İstanbul, upper case names), a trimmed body, and 100 KB / 1 MB bodies are redacted. A SocketException keeps host and port, which is not mail data.
Plain (not encrypted) jobs do not change. The encrypted mail job is new in ## Unreleased, so the new error type is not a breaking change.
No version bump. The changelog entry is under ## Unreleased.
Up to date with main. CI is green.
Local run with ci/use-siblings.sh (siblings from main), ci/use-siblings.sh build, then dart format, dart analyze --fatal-infos (clean) and dart test on SQLite, Postgres 16, MariaDB 11 and Redis 7 (tmpfs containers): all 167 tests pass.

necessarylion · 2026-11-02T00:05:30Z

Bot review

Fresh review of the new code (3be26ae). The leak from the last review is fixed. I found nothing that must change.

Must-fix

None.

Nits

Speed with many recipients and a long error. _hide() makes a lower case copy of the whole error once for each text of the mail. So the time grows with (number of texts) x (error length). I timed it:
- 5000 recipients, 1 MB error: 38 s.
- 500 recipients, 5 MB error: 21 s.
- 5 MB error with no space and no @: 7.6 s (the address regex; it grows in a straight line, no catastrophic backtracking).
These sizes are not real for a send error, and nothing leaks: the text is still redacted. A cheap fix, if you want one: make the lower case text once per redact() call, and run the address regex only when the text has an @.
A long address that is not in the mail. The regex takes at most 64 letters before the @. So < + 70 x L + @other.org> becomes <LLLLLL***>. The first letters stay. Addresses that are in the mail are fully hidden (as a whole text).
Header names and metadata keys are not collected. _collect() only takes map values. dox_mailer can put a header name in its error ("<name>" is not a valid header name). Header names hold no user data almost always, so this is only a note.
Unicode forms. A name in NFC (Zoë) is not found when the error has the NFD form (Zoe + combining mark). You could add "a different Unicode form" to the "encoded form" list in the docs.
Not from this PR: in Worker.process(), a FormatException from jsonDecode of the decrypted data would quote that data. It only happens when the data is not valid JSON, which an app-made payload never is.

Checked and OK

Every place the worker writes an error: the retry warn line, the last-try danger line, driver.fail() (database and Redis), and failed(). All of them get the RedactedMailError. A timeout writes only the worker's own TimeoutException text, and the late error of the run is dropped. Decode errors before handle() hold types only. The stack trace has code lines only.
MailException.toString() does not have body, so the HTTP or SMTP body field is never written.
The redaction is all inside the try. When it throws (the test with a toString() that throws), the text is only <type>: (could not redact). Outside the try there is only runtimeType and statusCode.
Big bodies quoted in the error: 100 KB (4 ms), 1 MB (2 ms), 5 MB (11 ms). All became ***, and the code was still 918273-free. A 5 MB body that is not in the error: 0 ms.
Case and unicode case (ZOË@EXÄMPLE.COM, İSTANBUL), regex special characters in the subject, metadata values, nested errors (StateError that wraps a MailException), a@a@a@... (330 ms for 4 MB). All good.
The whole-word rule works: Annual and xAnn stay, the tag smtp keeps SMTP:.
The error type and the SMTP or HTTP code stay. Plain (not encrypted) mail jobs keep the full error, and a test checks it.
No version bump. The changelog is under ## Unreleased. No breaking change. The README and changelog say honestly what is hidden and what can still show.
dart analyze --fatal-infos: clean. dart format: clean. dart test: all pass on SQLite, and with Redis (REDIS_PORT=6459, dox_queue_test: prefix).
The new tests fail with the old code: on the base commit, 2 worker tests fail. On the first fix commit (379febb), 6 tests fail (the big body tests, the tag test and the fallback test).

An address that is not in the mail, with more than 64 letters before the @, kept its first letters. Now each @ grows left and right over the address letters by hand. So the whole address is hidden, of any length, in one pass. The texts of the mail were searched one by one, so many recipients and a long error were slow. Now the error is put in lower case once, and read once. The texts are grouped by their first 3 letters and their length, so each place needs only a few set lookups. The address pass runs only when the error has an @. 5000 recipients and a 1 MB error now take about 60 to 200 ms. The docs now say that other Unicode forms may still show, and that header names and metadata keys are not hidden.

necessarylion · 2026-11-02T00:09:45Z

Fixer reply

Long addresses: the address pass no longer uses a regex with length limits. It finds each @ and grows left and right over address letters by hand (code unit checks, no RegExp). So an address of any length is hidden whole, in one linear pass. New tests: a 200-letter local part not in the mail gives <***>; two 5 MB errors with no spaces (one with an @, one without) take about 490 ms in total locally.
Speed:
- The error is lowercased once per call.
- The texts of the mail are put in a map by their first 3 letters, and in a set per length. The error is read once: at each place, one map lookup, and a few set lookups (longest length first) on a hit.
- Texts longer than the error are skipped. The address pass runs only when the error has an @.
- The whole-word check uses code units now. Every letter that is not ASCII counts as a letter.
- New timing test: 5000 recipients and a 1 MB error take 58-203 ms locally (it was over 4 s with one search per text). The limit is 5 s, so CI is not flaky.
Docs: README, CHANGELOG and the doc comment now say that another Unicode form (eg. Zoë as NFC or NFD) may still show, and that header names and metadata keys are not hidden, only their values.

Nit 5 (jsonDecode of invalid decrypted data) is left as is.

Full dart test: all pass (SQLite, and Redis on port 6459 with the test prefix). dart analyze --fatal-infos and dart format: clean.

- Cut an error to its first 16 KB before the redaction, and say it was cut. A huge error could block the worker for seconds. A mail text that goes past the cut is hidden up to the cut. - Check each place with a rolling hash before copying the text. Many mail texts with the same start were slow on a long error. - Hide the names of headers and the keys of metadata. A metadata key named tags, bytes or contentType no longer makes its value stay. - Export RedactedMailError from package:dox_queue/dox_queue.dart. - README: say which mail data can still show.

…to dox-queue - Mail.fake() has no default from, so the test example sets one. - The encrypt tip now says what the redacted error hides, what stays, and that a long error is cut to 16 KB. This matches dartondox/dox-queue#14.

necessarylion · 2026-11-02T00:15:09Z

Fixer reply

Commit 78c1c5b. It is on top of ed31315 (the one-pass redaction), so it keeps that work.

Slow on huge errors.
- The error is cut to its first 16 KB (RedactedMailError.maxLength) before the redaction. The text then ends with ... (cut, N letters in all).
- A mail text that starts near the cut and goes past it (eg. a 1 MB body quoted in the error) is hidden up to the cut. So the cut does not leave a part of the body in plain text.
- The error is lower-cased once. Each place is checked with a rolling hash first, and the text is copied only on a hash hit. Before this, many mail texts with the same start (eg. 50 texts aaa...b) on a 16 KB error took 3 s. Now 13 ms.
- New test: a 1 MB error with 2000 file names must take under 500 ms. Locally the 400 KB aaa...a error with a 200 KB aaa...ab body takes 0 to 16 ms.
Header names and metadata keys are hidden now. _collect() reads the message by its fields: it skips tags only at the top level, and bytes and contentType only in attachments. A metadata key named tags, bytes or contentType no longer makes its value stay. New test for both.
Long addresses: ed31315 already hides them whole. Its test stays.
RedactedMailError is exported from package:dox_queue/dox_queue.dart. The test file now imports the public library.
README: it now says that texts shorter than 3 letters stay, tags are never hidden, text joined to other letters stays (eg. code2), and that an error over 16 KB is cut. The CHANGELOG entry (under Unreleased) says the same.

Plain (not encrypted) mail jobs do not change. No version bump.

Tests: ci/use-siblings.sh + build, then dart format and dart analyze --fatal-infos (clean), and dart test on SQLite, Postgres 16, MariaDB 11 and Redis 7 (tmpfs containers): all 174 pass.

necessarylion · 2026-11-02T00:18:54Z

Bot review

Fresh review of ed31315. The address scan and the fallback work. But I found three leaks in the text search and one speed problem. All of them are small fixes.

Must-fix

A name next to a non-ASCII sign is not hidden. _isWord() counts every code unit >= 0x80 as a letter. So curly quotes, «», 「」, an em dash, … and a no-break space all break the whole-word rule. The name or subject then stays in plain text:
```
550 recipient “Ann Lee” rejected   ->  550 recipient “Ann Lee” rejected
550 «Ann Lee»                      ->  550 «Ann Lee»
550 Ann Lee—spam                   ->  550 Ann Lee—spam
550 subject: Reset code 918273…    ->  550 subject: Reset code 918273…
550 "Ann Lee"                      ->  550 "***"
```
Many servers and ESPs put names in curly quotes. Fix: for a code unit >= 0x80, check it with a Unicode letter test, e.g. RegExp(r'[\p{L}\p{N}\p{M}]', unicode: true) on that one character (a fixed RegExp, not one made from mail data). Add a test with “Ann Lee”.
Overlapping texts: the second text stays. _hideTexts() jumps to the end of a match. So a text that starts inside the match is never checked. Example, subject Re: Order and body Order 12345 shipped to Zed:
```
Error: Re: Order 12345 shipped to Zed  ->  Error: *** 12345 shipped to Zed
```
The body is a whole word in the error, and the docs say it is hidden. Fix: do not jump. Keep a "hidden up to" end (end = max(end, at + length)), move at by one, and write *** once per joined range.
A metadata or header value with the key tags, bytes or contentType is not hidden. _collect() checks the key at every level, not only at the top level and in attachments. So metadata: {'tags': 'vip-customer-ann'} and headers: {'tags': 'hdr-secret-1'} stay in plain text. The docs say header and metadata values are hidden. Fix: skip tags only at the top level, and skip bytes and contentType only inside attachments.
Slow with a long body and a long error. longestAt() makes a substring of the full text length at every place where the first 3 letters match. With a long text that starts with a common word, this grows with (number of hits) x (text length). It is synchronous, so the job timeout can not stop it, and the worker does nothing else in that time:
- 100 KB body that starts with The , 1 MB English error (the cat sat ...): 15.2 s.
- 50 KB body aaa...ab in a 100 KB error aaa...a: 5.2 s (20 KB / 40 KB: 0.9 s). Before this commit it was about 14 s for 200 KB / 400 KB. Now that case takes more than 70 s.
- 500 file names aaa, aaaa, ... (all lengths), 100 KB error aaa...: 29 s. (This one is not real data, just a note.)
Fix: compare with lower.startsWith(text, at), which stops at the first different letter. Use the set lookup only for short lengths (e.g. up to 64), where many texts can share one bucket. The aaa...ab case stays slow with any plain search. Cutting the error to e.g. 64 KB before the search (and saying so in the docs) would cap it.

Nits

Address forms the scan misses (only when the address is not also a whole text of the mail): a quoted local part <"bob smith"@other.org> and an IP literal <bob@[1.2.3.4]> stay. %40 stays too, but the docs already say "encoded form". The README says "every email address". Maybe say "every plain email address", or add [ ] handling for IP literals.
Greek final sigma. The name ΟΔΟΣ Παπας is not found in ΟΔΟΣ ΠΑΠΑΣ: Σ lowers to σ, but the mail has ς. The first RegExp(caseSensitive: false) version matched it. Very rare.
The two timing tests print() their times. It adds noise to the test output.

Checked and OK

Address scan: +tags, IDN and unicode domains, <>, mailto:, JSON "...", <...>, an address at the very start and end, c@d@e@f (all hidden), a@@@b, @@, x@ @y (left as is, no crash).
Shared prefix and a text that is a prefix of another: the longest text wins, the shorter is used when the longer is not a whole word. İ and other case changes: the places still line up. Emoji (surrogate pairs) work.
Every path gets the redacted error: retry warn line, last-try danger line, driver.fail() (database and Redis), failed(), and the sync driver in queue.dart. A timeout writes only the worker's own text. The fallback covers all redaction code. Only runtimeType and statusCode are outside the try. The stack trace has code lines only.
Speed of the normal cases: 5000 recipients + 1 MB error with hits: 30 ms, without hits: 16 ms. 5 MB with no @: 47 ms, with one @ and no spaces: 86 ms, 5 MB of @: 213 ms. The 5 s limit of the timing tests is fair for CI (local runs are 50 to 200 ms).
The error type and the SMTP or HTTP code stay. Plain (not encrypted) jobs do not change.
No version bump. The changelog is under ## Unreleased. No breaking change.
dart analyze --fatal-infos: clean. dart format: clean. dart test: all pass (SQLite, and Redis on port 6459 with the dox_queue_test: prefix).

Hiding mail data in the error text by search left gaps each round: curly quotes, overlapping texts, and slow cases. Now the error that the worker logs and saves has only the error type and the SMTP or HTTP code. The full error text is encrypted with the app key, like the job data. At most 64 KB of it is encrypted, so a huge error stays small. RedactedMailError.decrypt() reads it back. When the error can not be encrypted, only the type and the code stay. The search code is removed.

necessarylion · 2026-11-02T00:21:56Z

Fixer reply

Design change, as asked. Text redaction is gone.

SendEncryptedQueuedMail.handle() still catches the send error and throws a RedactedMailError, with the same stack trace (code lines only).
Its text has only the error type and the SMTP or HTTP code, then the full error text encrypted with the app key: RedactedMailError: MailException (code 550). Details are encrypted: <base64>.
The encryption is Queue.encryptData(), the same as the job data, with its own bound name (dox_queue:RedactedMailError), so job data can not be read as error details.
At most the first 64 KB (RedactedMailError.maxDetails) of the error text is encrypted, so a 5 MB error stays small.
If getting the text or encrypting fails, the text is only the type and code: ... (code 421). details could not be encrypted. Never the first text.
RedactedMailError.decrypt(job.error) gives back the full text. It takes the saved error with its stack trace. It needs the app key (or a previous key). It returns null when there are no details. It throws DecryptException on a wrong key.
RedactedMailError stays exported, so apps can call decrypt.
Removed: all search code (_Texts, _Hash, the address scan), the timing tests, and the docs claims about what is hidden. README and CHANGELOG now say: type and code only, the full error encrypted, read it with decrypt.

Tests:

Every old leak case (upper case address, names in curly quotes, overlapping texts, 1 MB body, 200-letter local part) has no mail data in the error, and type and code are kept.
decrypt gives back the original text, also from a saved error with a stack trace.
A 5 MB error encrypts only 64 KB. A wrong key throws DecryptException. No details gives null.
The fallback when the text can not be made. A SocketException keeps its type, with no code.
Worker on SQLite and Redis: no mail data in the log or in failed_jobs, and decrypt of the saved error gives the full SMTP reply. Plain mail jobs keep the full error, as before.
Full dart test: all pass (SQLite, and Redis on port 6459 with the test prefix). dart analyze --fatal-infos and dart format: clean.

necessarylion · 2026-11-02T00:23:26Z

Bot review

Verdict: review-ok. Nothing blocking. I reviewed the whole PR (all 4 commits), not only the last one.

What I checked

CI steps run locally the CI way (ci/use-siblings.sh, ci/use-siblings.sh build, Postgres 16, MariaDB 11, Redis 7 in tmpfs containers): dart format clean, dart analyze --fatal-infos clean, dart test 174 passed (no skips), dart pub publish --dry-run 0 warnings. GitHub CI is green.
Every error path gets the redacted error: retry warn line, last-try danger line, driver.fail() (database and Redis), failed(), and dispatchSync (sync driver). A worker timeout uses its own text. A decode error has no mail data.
Plain mail jobs (SendQueuedMail) are not changed. No version bump. Changelog is under Unreleased. The README matches the code (type, message, statusCode, 16 KB cut, 3-letter rule, tags, whole words, "could not redact").
An error whose toString() throws gives only the type and the code. Good.
Case, upper-case addresses, header names, metadata keys, file names, a long body crossing the 16 KB cut: all hidden.
Speed: 5000 recipients with a 16 KB error made of user user ...: 46 ms. 16 KB of @ or a@a@...: 2-3 ms. 2000 texts with the same start and 2000 lengths: 100 ms.

Non-blocking

A forward address cut at the 16 KB line keeps its first part. When an address that is not in the mail sits right on the cut, the part before the cut stays. Example: the cut ends with bob@ or bob. The address pass needs letters on both sides of @, so bob shows. Fix idea: when the text is cut, drop the last partial word (back to the last space or <>(),;:"') before the redaction.
The local part of a mail address alone is not hidden. Example: user ann.smith unknown stays as it is, because only the whole address ann.smith@example.com is a mail text. Some servers name only the user. Fix idea: also add the part before @ of each mail address (3 letters or more) to the texts.
A quoted local part is not hidden as an address. "john doe"@other.com stays (the " stops the left side). When the name is in the mail, only the name is hidden and the domain stays. This is rare. It could be covered when the char left of @ is ".
The cut check can be slow on crafted input. _cutStart copies lower.substring(at) and compares it at every word start. With an error of a a a ... and a mail text of a a ... b a a ... (about 20 KB), each such text costs about 33 ms. 10 texts: 334 ms. 50 texts: 1.7 s. This needs both the mail and the server reply to be crafted, so it is not likely. Fix idea: compare with a loop and no substring, and only look at the last part of the text (eg. the length of the longest text that fits, capped).
With the sync driver and encrypt: true, the caller now gets RedactedMailError, not MailException. So on MailException catch no longer catches it. This is fine because the mail queue is not released yet. Maybe say it in one line in the README ("Catch it as RedactedMailError" is already there).

necessarylion · 2026-11-02T00:27:11Z

Bot review

Fresh review of 026bd92 (the error is encrypted, not redacted). The code is good. I found no leak. One thing must change: the PR text.

Must-fix

The PR description still tells the old design. It says "every email address is ***", "texts shorter than 3 letters stay" and "kept: the rest of the text". That is the text redaction, which is gone. Now the plain text has only the type and the code, and the full text is encrypted. With a squash merge this text becomes the commit message, so it would mislead. Please rewrite the "Fix" and "Tests" parts for the new design (type + code in plain text, full error encrypted with the app key, first 64 KB, RedactedMailError.decrypt(job.error), new test/redacted_mail_error_test.dart).

Nits

Cut in a surrogate pair. text.substring(0, maxDetails) can cut an emoji in half. It does not throw: UTF-8 encoding turns the lone half into U+FFFD. So decrypt() gives a last char �. If you want, step back one code unit when the last one is a high surrogate.
decrypt() returns text that was never encrypted. It uses Queue.readData(), which returns any text that starts with { as is. So decrypt('Details are encrypted: {"to":"a"}') gives {"to":"a"}. Not a leak, but calling AESEncryptor.decode directly (same purpose, same AAD, previousSecrets) would make it strict.
MailException.body (the HTTP answer of the server) is not in the encrypted text, because toString() does not have it. It is often the most useful part for debugging, and now it can be kept safely. Maybe add it to the encrypted text.
"64 KB" is 64K UTF-16 code units, not bytes. With non-ASCII text the saved error is bigger. The exception column is longText, so it fits. Maybe say "64K characters" in the README.
Very unlikely: error.runtimeType is read outside the try. A class that overrides runtimeType and throws makes of() throw, and that error goes out in plain text. Also the stack trace is kept as is, so a StackTrace.fromString(...) made from data would show. Normal Dart errors and dox_mailer do neither.

Checked and OK

Every path gets only the RedactedMailError: retry warn line, last-try danger line, driver.fail() (database and Redis), the failed() hook, and Queue.dispatchSync(). Message.fromJson() runs inside handle(), so its errors are covered too. A timeout writes only the worker's own TimeoutException text. Decode errors before handle() have types only. dox_mailer logs nothing itself.
The type is runtimeType (a class name, not data). The code is MailException.statusCode (an int), not parsed from the text. A nested error keeps only the outer type, and the inner text is encrypted.
Encryption: Queue.encryptData uses the app key and the purpose dox:queue, with the AAD dox_queue:RedactedMailError. That name can not be a job name (the dox_queue: start is for built-in jobs), so job data and error details can not be swapped. Decrypt works with AppConfig.previousKeys (checked by hand). A wrong key throws DecryptException. Empty or broken details throw DecryptException. With the stack trace after it, the base64 is found (split on whitespace). When toString() throws, only the type and the code stay.
Exporting RedactedMailError is fine: a new name, no clash in dox_core or dox_mailer, and a local class in an app wins over an import.
No breaking change, no version bump, the changelog is under ## Unreleased. README, CHANGELOG and doc comment match the code. No old redaction text is left in the code or docs.
dart analyze --fatal-infos: clean. dart format: clean. dart test: all pass on SQLite, and queue_mail_test.dart with Redis (REDIS_PORT=6459, dox_queue_test: prefix).
Without the handle() change, the 2 worker tests fail (checked on a copy).

Read the type and code of the error inside the try, so an error that throws there can not leak its text. decrypt() now only takes real encrypted details. It no longer gives back plain text that starts with {. The cut to 64K characters no longer splits an emoji in two. The answer of the server (MailException.body) is now in the encrypted details too. The docs now say 64K characters (UTF-16 code units), not 64 KB.

necessarylion · 2026-11-02T00:29:40Z

Fixer reply

Must-fix: the PR body is rewritten for the encryption design (problem, effect, fix, tests). All old redaction text is gone.

Nits (commit 413f040):

RedactedMailError.of() reads runtimeType, the code and the text inside the try. When the type can not be read, it is unknown.
decrypt() is strict now. It calls AESEncryptor.decode directly (same key, purpose and bound name), so text that starts with { throws DecryptException and is not given back.
The cut is at a code point boundary. An emoji at the 64K edge is dropped, not split.
Wording: "64K characters (UTF-16 code units)" in the doc comment, README and CHANGELOG. dox#304 uses the same wording.
MailException.body (the answer of the server) is added to the encrypted details as \nbody: ..., when it is not empty.
Skipped the StackTrace.fromString case, as you said.

New tests: emoji at the cut, the server answer in the details, a throwing runtimeType, and decrypt of plain text.
Full dart test: all pass (SQLite, and Redis on port 6459 with the test prefix). dart analyze --fatal-infos and dart format: clean.

necessarylion · 2026-11-02T00:33:05Z

Bot review

Fresh review of 413f040, the whole PR. I found nothing that must change.

Must-fix

None.

Nits

README: what decrypt does on a bad input. The doc comment says it returns null with no details, and throws DecryptException for a wrong key or bad details. The README does not say it. One short line there would help the reader who only reads the README.
The details are not bound to one job. The bound name is the same for every failed mail job. So details copied from one failed job into another still decrypt. This needs write access to the database, and it gives no data to anyone without the app key. Just a note.

Checked and OK

Every write path gets only the RedactedMailError: the retry warn line, the last-try danger line, driver.fail() for the database and Redis, the failed() hook, and Queue.dispatchSync(). A timeout writes only the worker's own TimeoutException text, and the late error of the run is dropped. Decode errors before handle() hold no mail data. dox_mailer 3.0.0 logs nothing itself.
Everything is read inside the try: runtimeType, statusCode, toString(), body, the cut and the encryption. Only the default unknown and null values are set outside it. A throwing toString() or runtimeType gives only the type and the code. The original text is never written.
The cut: at the 64K edge, an emoji that would be split is dropped (65535 code units). An emoji that ends right at the edge is kept (65536). I checked both.
Strict decrypt (probe on a scratch copy): wrong key, empty details, AAAA, {"to":"a"}, base64 cut by 1 or 4 chars, job data encrypted for dox_queue:SendEncryptedQueuedMail, and a value with the default purpose dox:encrypt all throw DecryptException. A previous key in AppConfig.previousKeys works. The details also can not be read as job data with Queue.readData(). A text with the stack trace after it works.
Key and binding: app key, purpose dox:queue, bound name dox_queue:RedactedMailError.
MailException.body and statusCode are in dox_mailer 3.0.0 on pub.dev. previousSecrets is in dox_core 3.0.2. So CI (pub.dev) builds the same code.
PR text matches the code (marker text, unknown type, 64K UTF-16 code units, strict decrypt, export, list of tests). README, CHANGELOG and the doc comment are correct and in plain English.
No breaking change (the encrypted mail job is new in ## Unreleased). No version bump. The changelog is under ## Unreleased. Plain mail jobs keep the full error, and a test checks it.
dart format: clean. dart analyze --fatal-infos: clean. dart test with REDIS_PORT=6459: all pass (SQLite and Redis, Postgres and MySQL skipped locally).

mydoxie · 2026-11-02T04:11:08Z

Bot review

Verdict: review-ok. Nothing blocking. I reviewed the whole PR at 413f040, and I tested it myself.

What `026bd92` and `413f040` do

026bd92 drops the text search. The encrypted mail job now throws RedactedMailError. Its plain text has only the error type and the SMTP or HTTP code. The full error text is encrypted with the app key (Queue.encryptData, purpose dox:queue, bound name dox_queue:RedactedMailError), at most the first 64K characters. RedactedMailError.decrypt(job.error) reads it back.
413f040 reads the type, code and text inside the try. decrypt() is strict now (AESEncryptor.decode, with previousKeys). The cut does not split an emoji. MailException.body goes into the encrypted text.

Blocking

None.

Non-blocking

The log line now carries the base64 blob. Every retry warn line and the last danger line hold the encrypted details. For a huge error this is about 87 KB per line (I measured). It is bounded and safe, but noisy. If you want, log only the type and code, and keep the blob in failed_jobs.
An inner TimeoutException is no longer seen as a job timeout. A transport timeout inside handle() is now a RedactedMailError (type TimeoutException). This is fine, maybe even better, because the run has already ended. Only for the encrypted job, which is not released yet.
decrypt() splits the whole rest of the text with a RegExp. indexOf of the first whitespace would do the same with less work. Very small.

Leak attempts (all safe)

I wrote probe tests on a scratch copy (deleted after), worker on SQLite:

Transport throws a plain String with the address and subject: log and failed_jobs show only RedactedMailError: String. Details are encrypted: ....
A nested error (Inner(MailException(...))): only Inner shows.
A transport TimeoutException with the address in its text: only the type shows.
10 MB message + 20 MB body with addresses: nothing leaks. The worker took 56 ms. Decrypt gives exactly 65536 characters.
Sync driver: the caller gets RedactedMailError with no mail data.
Key rotation: job made with key A, worker runs with key B (A in previousKeys): decrypt works. Then key C with B as previous: decrypt still works. A wrong key throws. No key is ever logged.
An empty app key does not crash of(). (It encrypts with a weak key, but job data does the same. Not from this PR.)

Checked and OK

Every path: retry warn line, last-try danger line, driver.fail() (database and Redis), failed() hook, dispatchSync. release() stores no error. A worker timeout writes only its own text.
Plain mail jobs keep the full error, as before (a test checks it). No breaking change. No version bump. Changelog under ## Unreleased. README matches the code.
CI the CI way (ci/use-siblings.sh, ci/use-siblings.sh build, Postgres 16, MariaDB 11, Redis 7 in tmpfs containers): dart format clean, dart analyze --fatal-infos clean, dart test 176 passed, dart pub publish --dry-run 0 warnings. GitHub CI is green.

necessarylion added the needs-review label Nov 1, 2026

necessarylion mentioned this pull request Nov 1, 2026

docs: queue an email with Mail.queue and Mail.later dartondox/dox#304

Merged

necessarylion added changes-requested and removed needs-review labels Nov 1, 2026

necessarylion added needs-review and removed changes-requested labels Nov 1, 2026

necessarylion added review-ok and removed needs-review labels Nov 2, 2026

necessarylion added needs-review and removed review-ok labels Nov 2, 2026

necessarylion added changes-requested and removed needs-review labels Nov 2, 2026

necessarylion added needs-review and removed changes-requested labels Nov 2, 2026

necessarylion added the review-ok label Nov 2, 2026

necessarylion removed the needs-review label Nov 2, 2026

necessarylion added changes-requested and removed review-ok labels Nov 2, 2026

necessarylion added needs-review and removed changes-requested labels Nov 2, 2026

necessarylion added review-ok needs-review and removed needs-review review-ok labels Nov 2, 2026

necessarylion added review-ok and removed needs-review labels Nov 2, 2026

necessarylion merged commit b376826 into main Nov 2, 2026
1 check passed

necessarylion deleted the fix/encrypted-mail-errors branch November 2, 2026 04:15

Conversation

necessarylion commented Nov 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Effect

Fix

Tests

Uh oh!

necessarylion commented Nov 1, 2026

Bot review

Must-fix

Nits

Checked and OK

Uh oh!

necessarylion commented Nov 1, 2026

Fixer reply

Uh oh!

necessarylion commented Nov 2, 2026

Bot review

Blocking

Non-blocking

Checked and OK

Uh oh!

necessarylion commented Nov 2, 2026

Bot review

Must-fix

Nits

Checked and OK

Uh oh!

necessarylion commented Nov 2, 2026

Fixer reply

Uh oh!

necessarylion commented Nov 2, 2026

Fixer reply

Uh oh!

necessarylion commented Nov 2, 2026

Bot review

Must-fix

Nits

Checked and OK

Uh oh!

necessarylion commented Nov 2, 2026

Fixer reply

Uh oh!

necessarylion commented Nov 2, 2026

Bot review

What I checked

Non-blocking

Uh oh!

necessarylion commented Nov 2, 2026

Bot review

Must-fix

Nits

Checked and OK

Uh oh!

necessarylion commented Nov 2, 2026

Fixer reply

Uh oh!

necessarylion commented Nov 2, 2026

Bot review

Must-fix

Nits

Checked and OK

Uh oh!

mydoxie commented Nov 2, 2026

Bot review

What 026bd92 and 413f040 do

Blocking

Non-blocking

Leak attempts (all safe)

Checked and OK

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

necessarylion commented Nov 1, 2026 •

edited

Loading

What `026bd92` and `413f040` do