kami/Maven
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Read a mailbox read-only, in an IMAP client small enough to audit (#246) #63

Closed
claude wants to merge 1 commits from overnight/email-imap into overnight/money-zenmoney
pull from: overnight/email-imap
merge into: kami:overnight/money-zenmoney
Conversation 2 Commits 1 Files Changed 13
+1165
claude commented 2026-08-01 00:59:43 +02:00
Contributor
Copy Link
Copy Source

First of the email-reader chain (Vikunja #246, docs/plans/01-email-reader.md). This branch is the reading half only — it extracts nothing and writes nothing to the store.

What changed

  • internal/email/imap.go — minimal IMAP4rev1 client: LOGIN, EXAMINE, UID SEARCH SINCE, UID FETCH (BODY.PEEK[]), LOGOUT. Implicit TLS only, certificate verified, per-connection deadline. No IDLE.
  • internal/email/message.go — MIME → plaintext: RFC 2047 headers, quoted-printable/base64, multipart walk preferring text/plain, regex HTML strip, attachments skipped, body truncated to 4000 bytes.
  • internal/email/junk.go — header-only bulk/automated filter.
  • internal/email/fetch.go — FetchSince.Run(password): connect, read, drop. The password is an argument, never a struct field kept alive.
  • Tests + recorded .eml fixtures.

Why this shape

EXAMINE makes the session read-only at the protocol level; BODY.PEEK means reading his mail does not mark it \Seen. Hand-rolled instead of go-imap because this path holds his mailbox credential and reads his private mail — five commands with no dependencies is auditable in one sitting.

Junk is decided by headers before any model sees the message: List-Unsubscribe/List-Id/List-Post, Precedence: bulk, Auto-Submitted, X-Spam-Flag/Status, Gmail category labels. No sender lists, no subject keywords.

Privacy: nothing logs a body, subject or address; the junk reason names a header; an unsupported charset degrades to headers-only rather than mojibake.

Verified

make build and make test (go test -race) both green. internal/email tests cover the .eml fixtures (Russian quoted-printable, HTML-only, multipart with attachment, cp1251, truncation) and the IMAP client against an in-process fake server, asserting EXAMINE/BODY.PEEK/quoted password/SINCE date on the wire. No live IMAP account exists on this box, so the live half is unverified by design.

Vikunja #246

First of the email-reader chain (Vikunja #246, docs/plans/01-email-reader.md). This branch is the reading half only — it extracts nothing and writes nothing to the store. ## What changed - `internal/email/imap.go` — minimal IMAP4rev1 client: LOGIN, EXAMINE, UID SEARCH SINCE, UID FETCH (BODY.PEEK[]), LOGOUT. Implicit TLS only, certificate verified, per-connection deadline. No IDLE. - `internal/email/message.go` — MIME → plaintext: RFC 2047 headers, quoted-printable/base64, multipart walk preferring text/plain, regex HTML strip, attachments skipped, body truncated to 4000 bytes. - `internal/email/junk.go` — header-only bulk/automated filter. - `internal/email/fetch.go` — `FetchSince.Run(password)`: connect, read, drop. The password is an argument, never a struct field kept alive. - Tests + recorded .eml fixtures. ## Why this shape EXAMINE makes the session read-only at the protocol level; BODY.PEEK means reading his mail does not mark it \\Seen. Hand-rolled instead of go-imap because this path holds his mailbox credential and reads his private mail — five commands with no dependencies is auditable in one sitting. Junk is decided by headers before any model sees the message: List-Unsubscribe/List-Id/List-Post, Precedence: bulk, Auto-Submitted, X-Spam-Flag/Status, Gmail category labels. No sender lists, no subject keywords. Privacy: nothing logs a body, subject or address; the junk reason names a header; an unsupported charset degrades to headers-only rather than mojibake. ## Verified `make build` and `make test` (go test -race) both green. `internal/email` tests cover the .eml fixtures (Russian quoted-printable, HTML-only, multipart with attachment, cp1251, truncation) and the IMAP client against an in-process fake server, asserting EXAMINE/BODY.PEEK/quoted password/SINCE date on the wire. No live IMAP account exists on this box, so the live half is unverified by design. Vikunja #246
claude added 1 commit 2026-08-01 00:59:43 +02:00
Read a mailbox read-only, in a client small enough to audit (#246) b4646155b4 
internal/email is the reading half of the email reader: a ~200-line IMAP
client (LOGIN, EXAMINE, UID SEARCH SINCE, UID FETCH BODY.PEEK, LOGOUT), a
MIME-to-plaintext converter, and a header-only junk filter.

Two protocol choices are the design, not shortcuts. EXAMINE instead of
SELECT means the session is read-only at the protocol level, so no command
in it can flip a flag or expunge anything by mistake. BODY.PEEK instead of
BODY means reading a message does not mark it \Seen — Maven reads his mail
and leaves no trace of having done so, and the unread state in his own
client stays his.

Hand-rolled rather than go-imap because this is the one path that holds his
mailbox credential and reads his private mail: five commands with no
dependencies is auditable in a sitting. No IDLE and no cleartext/STARTTLS
either — an option to send his password over a plain socket is an option to
get it wrong once.

Junk is decided by headers alone, before any model is involved:
List-Unsubscribe/List-Id, Precedence: bulk, Auto-Submitted, the spam
headers, and Gmail's own category labels. Sender lists and subject keywords
are deliberately absent — they age badly and they would put his contacts in
a config file. A junk verdict only means "do not spend the model on this";
nothing is deleted and no server flag is touched.

Nothing here logs a body, a subject or an address, the junk reason names a
header rather than content, and an undecodable charset degrades to
headers-only instead of feeding the model mojibake. Verified against
recorded .eml fixtures and an in-process fake IMAP server.
claude reviewed 2026-08-01 11:31:13 +02:00
claude left a comment
Author
Contributor
Copy Link
Copy Source

The hand-rolled client earns its place. EXAMINE instead of SELECT makes read-only a protocol fact rather than a promise. BODY.PEEK[] keeps his unread state his. Implicit TLS with no STARTTLS path removes the one option that could go wrong quietly. The password never lands on the Conn struct, and exec logs neither command nor response. Both obvious paths for a credential into a log file are closed. Junk being decided on headers the sender set on itself, with no keyword or contact lists, is the right rule.

Four findings.

1. Fetch allocates whatever size the server announces, with no cap and no deadline refresh.

n, err := strconv.Atoi(m[1])
buf := make([]byte, n)
if _, err := io.ReadFull(c.r, buf); err != nil {

n comes off the wire. A {2147483647} literal is a 2GB allocation before a single byte is read. That does not need a hostile server. One mail with a 60MB attachment gives mavmaild a 60MB peak RSS. That is on a box already holding a 1.7B model resident. The attachment is then discarded by plaintextBody and the body truncated to MaxBodyBytes of 4000, so the whole allocation exists to be thrown away. zenmoney.diff in PR 62 wraps its read in io.LimitReader(res.Body, 32<<20), and this path is the more exposed one.

The deadline is the second half. setDeadline runs on each readLine, but io.ReadFull gets no refresh. So Timeout stops being an idle timeout and becomes a whole-message budget. A 30MB message on a slow uplink fails at the timeout, however healthy the connection is. It then fails on every poll after that. The mailbox stalls behind one big message.

Cap the literal at something a mail body could plausibly need, and skip the message when the announced size exceeds it. That also fixes the deadline case, because a capped read finishes.

2. FetchSince.Run abandons the poll on the first bad message, and its comment says it does not.

// One unreadable message does not abandon the poll; the rest of the
// mailbox is still worth reading. The error names the UID, not the
// message.
return out, fmt.Errorf("email: fetch uid %d: %w", uid, err)

The comment describes continue. The code returns. Walk it: 40 new UIDs, newest first, UID 900 is the 60MB message from finding 1 and times out. UIDs 899 down to 861 are never fetched. Run returns a non-nil error, and every caller I would expect will treat that as a failed poll. Next poll repeats the same order and dies on the same UID. One oversized message permanently blocks every message behind it. The two findings compound, which is why this one is not just a doc fix.

3. The Gmail category branch in classifyJunk can never fire through this client.

gmailCategories is matched against h.Get("X-GM-LABELS") and h.Get("X-Gmail-Labels"), where h is the header block parsed out of the raw RFC 5322 bytes. X-GM-LABELS is not a header. It is a Gmail IMAP FETCH data item, requested as UID FETCH n (X-GM-LABELS), and it never appears inside the message source. X-Gmail-Labels is a Takeout mbox export header, which is not what arrives over IMAP either. Fetch asks only for BODY.PEEK[]. So the branch is dead against a real Gmail mailbox.

junk_test.go hides this because it builds the header by hand:

{"gmail promo", "From: a@b.c\nX-Gmail-Labels: Inbox,CATEGORY_PROMOTIONS", true, "gmail-category"},

That test asserts the matcher, not the plumbing. Either add X-GM-LABELS to the FETCH item list and carry it into classifyJunk out of band, or drop the branch. Leaving it as is means the doc comment claims a Promotions filter that is not running. Promotions is the largest junk category in a Gmail mailbox.

4. Timeout of zero disables every timeout in the path, and nothing rejects it.

Run validates Addr, User and Mailbox, and says nothing about Timeout. With zero, setDeadline returns immediately without setting anything, and Dial builds a net.Dialer{Timeout: 0}. A dead server then parks the poller on a socket read forever. The session stays authenticated, with his credential live in a TLS state. That is the exact thing the FetchSince doc comment says the connect-read-drop shape exists to avoid. Either default it in Run or reject zero the way the other three fields are rejected.

Smaller notes.

  • Fetch only ever returns bytes it found in a literal. A server answering a small BODY.PEEK[] with a quoted string produces raw == nil. Run reads that as "vanished between SEARCH and FETCH" and skips the message silently. A message that exists and was readable is dropped with no log line.
  • Fetch runs the tagged-completion check against every line it reads. The message bytes go through io.ReadFull rather than the line loop, so that is safe. The exec literal-skip path is the one to keep an eye on if a command is ever added that returns headers.
  • In multipartText, mediaType == "text/html" && !strings.HasPrefix(mediaType, "multipart/") has a second clause that cannot be false when the first is true. The case it looks like it was written for is a nested multipart/alternative whose recursion returned HTML-derived text. That text lands in plain today, so a sibling real text/plain part later in the message is discarded by the if plain == "" guard.
  • decodeBody rejects windows-1251 and returns subject-only. The comment argues the trade and testdata/cp1251.eml covers it. It is still a live gap. cp1251 remains common in Russian mail from older senders, and subject-only means those messages never produce a task candidate in PR 64.
  • quote strips CR and LF from the password rather than rejecting it. A credential file that picked up an embedded newline authenticates as a different string and fails with the server's generic NO. An error naming the problem would save a long debugging session, and it cannot leak the password.
  • untagged(l, "SEARCH") would also match a hypothetical * SEARCHFOO. No server sends one. Mentioning it only because the prefix check is the sort of thing that ages badly next to an extension.
The hand-rolled client earns its place. EXAMINE instead of SELECT makes read-only a protocol fact rather than a promise. `BODY.PEEK[]` keeps his unread state his. Implicit TLS with no STARTTLS path removes the one option that could go wrong quietly. The password never lands on the `Conn` struct, and `exec` logs neither command nor response. Both obvious paths for a credential into a log file are closed. Junk being decided on headers the sender set on itself, with no keyword or contact lists, is the right rule. Four findings. **1. `Fetch` allocates whatever size the server announces, with no cap and no deadline refresh.** ```go n, err := strconv.Atoi(m[1]) buf := make([]byte, n) if _, err := io.ReadFull(c.r, buf); err != nil { ``` `n` comes off the wire. A `{2147483647}` literal is a 2GB allocation before a single byte is read. That does not need a hostile server. One mail with a 60MB attachment gives mavmaild a 60MB peak RSS. That is on a box already holding a 1.7B model resident. The attachment is then discarded by `plaintextBody` and the body truncated to `MaxBodyBytes` of 4000, so the whole allocation exists to be thrown away. `zenmoney.diff` in PR 62 wraps its read in `io.LimitReader(res.Body, 32<<20)`, and this path is the more exposed one. The deadline is the second half. `setDeadline` runs on each `readLine`, but `io.ReadFull` gets no refresh. So `Timeout` stops being an idle timeout and becomes a whole-message budget. A 30MB message on a slow uplink fails at the timeout, however healthy the connection is. It then fails on every poll after that. The mailbox stalls behind one big message. Cap the literal at something a mail body could plausibly need, and skip the message when the announced size exceeds it. That also fixes the deadline case, because a capped read finishes. **2. `FetchSince.Run` abandons the poll on the first bad message, and its comment says it does not.** ```go // One unreadable message does not abandon the poll; the rest of the // mailbox is still worth reading. The error names the UID, not the // message. return out, fmt.Errorf("email: fetch uid %d: %w", uid, err) ``` The comment describes `continue`. The code returns. Walk it: 40 new UIDs, newest first, UID 900 is the 60MB message from finding 1 and times out. UIDs 899 down to 861 are never fetched. `Run` returns a non-nil error, and every caller I would expect will treat that as a failed poll. Next poll repeats the same order and dies on the same UID. One oversized message permanently blocks every message behind it. The two findings compound, which is why this one is not just a doc fix. **3. The Gmail category branch in `classifyJunk` can never fire through this client.** `gmailCategories` is matched against `h.Get("X-GM-LABELS")` and `h.Get("X-Gmail-Labels")`, where `h` is the header block parsed out of the raw RFC 5322 bytes. `X-GM-LABELS` is not a header. It is a Gmail IMAP FETCH data item, requested as `UID FETCH n (X-GM-LABELS)`, and it never appears inside the message source. `X-Gmail-Labels` is a Takeout mbox export header, which is not what arrives over IMAP either. `Fetch` asks only for `BODY.PEEK[]`. So the branch is dead against a real Gmail mailbox. `junk_test.go` hides this because it builds the header by hand: ```go {"gmail promo", "From: a@b.c\nX-Gmail-Labels: Inbox,CATEGORY_PROMOTIONS", true, "gmail-category"}, ``` That test asserts the matcher, not the plumbing. Either add `X-GM-LABELS` to the FETCH item list and carry it into `classifyJunk` out of band, or drop the branch. Leaving it as is means the doc comment claims a Promotions filter that is not running. Promotions is the largest junk category in a Gmail mailbox. **4. `Timeout` of zero disables every timeout in the path, and nothing rejects it.** `Run` validates `Addr`, `User` and `Mailbox`, and says nothing about `Timeout`. With zero, `setDeadline` returns immediately without setting anything, and `Dial` builds a `net.Dialer{Timeout: 0}`. A dead server then parks the poller on a socket read forever. The session stays authenticated, with his credential live in a TLS state. That is the exact thing the `FetchSince` doc comment says the connect-read-drop shape exists to avoid. Either default it in `Run` or reject zero the way the other three fields are rejected. Smaller notes. - `Fetch` only ever returns bytes it found in a literal. A server answering a small `BODY.PEEK[]` with a quoted string produces `raw == nil`. `Run` reads that as "vanished between SEARCH and FETCH" and skips the message silently. A message that exists and was readable is dropped with no log line. - `Fetch` runs the tagged-completion check against every line it reads. The message bytes go through `io.ReadFull` rather than the line loop, so that is safe. The `exec` literal-skip path is the one to keep an eye on if a command is ever added that returns headers. - In `multipartText`, `mediaType == "text/html" && !strings.HasPrefix(mediaType, "multipart/")` has a second clause that cannot be false when the first is true. The case it looks like it was written for is a nested `multipart/alternative` whose recursion returned HTML-derived text. That text lands in `plain` today, so a sibling real `text/plain` part later in the message is discarded by the `if plain == ""` guard. - `decodeBody` rejects windows-1251 and returns subject-only. The comment argues the trade and `testdata/cp1251.eml` covers it. It is still a live gap. cp1251 remains common in Russian mail from older senders, and subject-only means those messages never produce a task candidate in PR 64. - `quote` strips CR and LF from the password rather than rejecting it. A credential file that picked up an embedded newline authenticates as a different string and fails with the server's generic NO. An error naming the problem would save a long debugging session, and it cannot leak the password. - `untagged(l, "SEARCH")` would also match a hypothetical `* SEARCHFOO`. No server sends one. Mentioning it only because the prefix check is the sort of thing that ages badly next to an extension.
kami closed this pull request 2026-08-01 14:51:48 +02:00
kami commented 2026-08-01 14:52:43 +02:00
Owner
Copy Link
Copy Source

Landed on master. The stack was one linear chain, so #84 carried every commit from #50 up, and master now contains this branch in full. Merging this PR on its own is an empty diff, so it is closed rather than merged. The review findings for it were fixed in the 2026-08-01 pass and are on master as commits on the stack tip, not on this branch.

Landed on master. The stack was one linear chain, so #84 carried every commit from #50 up, and master now contains this branch in full. Merging this PR on its own is an empty diff, so it is closed rather than merged. The review findings for it were fixed in the 2026-08-01 pass and are on master as commits on the stack tip, not on this branch.

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
claude
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
kami
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kami/Maven#63
Delete Branch "overnight/email-imap"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?