# Principal Review (principal-1) ## Summary This is a well-conceived consolidation. It replaces a hand-maintained, drifted `jmap_states[]` table in `mboxevent.c` with iteration over the canonical `jmap_data_types` gperf table, factors the state-string formatting into two EXPORTED helpers in the base library so both the http daemon and `mboxevent.c` format states identically, and adds a `UserState/get` method that is a faithful state-string sibling of the existing `UserCounters/get`. The design instinct — make one table the single source of truth, and make the event/API/notification paths all derive from it — is exactly right and pays down real technical debt (the old event table listed superseded `Contact`/`ContactGroup`, omitted `AddressBook`, used the wrong modseq for `Mailbox`, and never emitted the `J` prefix). The big call I want confirmed is a **wire-format change to `vnd.fastmail.jmapStates`** that the commit message documents *partially*: it documents the dropped/added keys and the Mailbox modseq switch, but it does NOT mention that the new event gate also stops `CalendarEventNew` / `CalendarAlarm` (and other pure `EVENT_CALENDAR` / DAV events) from carrying the parameter at all — a narrowing that previously-subscribed consumers will observe. That is my one High finding. Everything else is Low/Info. The offset-read memory safety that the Tech Lead flagged is sound (type-correct base pointer, negative-offset guard, fully-populated counters), and the borrowed-key concern is a non-issue because `hash_insert` copies keys. ## What I Explored - `imap/jmap_data_types.gperf` (the canonical table) and `jmap_data_type_t` in `imap/jmap_api.h` — confirmed `Mailbox` and `Email` both map to `mailmodseq`, `Calendar`/`CalendarEvent` to `caldavmodseq`, and that unlisted/stateless slots get `modseq_offset == -1` via the gperf initializer-suffix. - `struct mboxname_counters` (`imap/mboxname.h:309`) — verified every offset the table references is a real `modseq_t` field, so the offset read is type-correct. - `jmap_modseq` (`imap/jmap_api.c:1270`) — confirmed `Mailbox/get`-style callers with flags=0 resolve to `mailmodseq`/`caldavmodseq`, matching the table. The per-type correspondence the tests assert is correct by construction. - `mboxevent_extract_mailbox` (`imap/mboxevent.c:1696`) and the `EVENT_*` masks (`mboxevent.c:38-48`) — traced which events fill jmapStates and cross-checked the new gate. This is where the `EVENT_CALENDAR` narrowing lives. - All 30+ callers of `mboxevent_extract_mailbox` (append.c, index.c, mboxlist.c, http_dav.c, jmap_calendar.c, itip_support.c, pop3d.c, …) to find which event types reach the fill. - `jmap_api()` accountId authorization (`jmap_api.c:688-721`), `lookup_capabilities`, and `JMAP_NEED_CSTATE`/`req.counters` population — confirmed UserState/get's exposure is identical to UserCounters/get. - `jmap_get_fini` (`jmap_api.c:1726`), `JMAP_GET_INITIALIZER`, `hash_insert` (`lib/hash.c:102`, copies the key) — confirmed no leak and no dangling-key bug. - The `usercounters_get` sibling (`jmap_core.c:180-317`) to compare the two methods' parse/build/fini shape. ## Findings ### Finding 1: Event gate silently stops CalendarEventNew/Alarm from carrying jmapStates - **Severity**: High - **Location**: `imap/mboxevent.c:587-588` (the new mask), interacting with callers `imap/http_dav.c:5196`, `imap/jmap_calendar.c:5460/5534/5933`, `imap/itip_support.c:1360`; mask definitions `mboxevent.c:38-48`; event table `mboxevent.c:1926-1927`. - **Issue**: The new gate restricts `EVENT_JMAP_STATES` to `MESSAGE_EVENTS|FLAGS_EVENTS|MAILBOX_EVENTS|SUBS_EVENTS`. That set does **not** include `EVENT_CALENDAR` (CalendarEventNew), `EVENT_CALENDAR_ALARM`, `EVENT_APPLEPUSHSERVICE_DAV`, etc. Those events call `mboxevent_extract_mailbox` and, before this branch, satisfied the old predicate (`extra_params & JMAPSTATES`, always true when configured) and so carried `vnd.fastmail.jmapStates`. After this change they carry nothing. Note this is correct *as a gate* — the fill at `mboxevent.c:1817` uses the same `mboxevent_expected_param` predicate, so fill and mask can never diverge and the `filled_params()` assert is genuinely avoided — but the chosen set is narrower than "events that carry a mailbox." A calendar event very much carries a mailbox; the comment's stated rationale ("only events that carry a mailbox") does not actually justify excluding `EVENT_CALENDAR`. - **Why It Matters**: This is an undocumented behavioral narrowing of a vendor wire parameter. A push/notification consumer that today learns the new `Calendar`/`CalendarEvent` state from a CalendarEventNew event will silently stop receiving it and must fall back to polling. The commit message carefully documents every *other* wire change but is silent on this one, which suggests it may be an unintended side effect of picking masks that were convenient rather than the true "has a mailbox" set. - **Suggestion**: Either (a) widen the mask to include `EVENT_CALENDAR` (and audit `EVENT_CALENDAR_ALARM`/DAV events) if those should keep emitting jmapStates — the cleanest expression is "every event for which `mboxevent_extract_mailbox` runs and `mboxname_read_counters` succeeds," which you could approximate with the existing `EVENT_URI`-style notion of mailbox-bearing events; or (b) if the narrowing is intentional, say so explicitly in the commit message and the code comment, and add a test that a CalendarEventNew event omits the parameter (mirroring how `MboxEvent/jmap_states` asserts presence on MessageNew). Right now neither the docs nor the tests pin this behavior down. ### Finding 2: Offset read uses signed `off_t` for pointer arithmetic (was `size_t`) - **Severity**: Low - **Location**: `imap/jmap_core.c:309` and `imap/mboxevent.c:559` — both `*(modseq_t *)((off_t) &…counters + dtype->modseq_offset)`. - **Issue**: Casting a pointer to `off_t` (a signed type) and doing integer arithmetic on it is not strictly portable; the idiom for "pointer as integer" is `uintptr_t`, and the idiom for "byte offset into a struct" is `(char *)base + offset`. Notably the old `mboxevent.c` code used `(size_t)` (unsigned), so this is a small regression in idiom. `modseq_offset` is itself `off_t`, which is why `off_t` was reached for, but the result type of the cast should be the pointer-sized integer type, not the file-offset type. The negative-offset guard (`modseq_offset < 0` → return) correctly prevents the `-1` sentinel from ever reaching this arithmetic, so there is no real-world miscompute on any supported platform — this is hygiene, not a live bug. - **Why It Matters**: It is duplicated in two places and reads as a smell to the next maintainer; on an exotic ABI where `sizeof(off_t) < sizeof(void *)` it would truncate. Since the pattern now appears twice, getting the idiom right once and sharing it is worth doing. - **Suggestion**: Factor the read into a single tiny helper next to the table, e.g. `static modseq_t counter_at(const struct mboxname_counters *c, off_t off) { return *(const modseq_t *)((const char *)c + off); }`, and call it from both callbacks. That fixes the cast idiom, removes the duplication, and gives one place to assert `off >= 0` and `off + sizeof(modseq_t) <= sizeof(*c)`. ### Finding 3: Offset-read pattern duplicated across two callbacks - **Severity**: Low - **Location**: `imap/jmap_core.c:301-315` (`add_userstate_cb`) and `imap/mboxevent.c:550-563` (`add_jmap_state_cb`). - **Issue**: The two callbacks are nearly identical: skip `modseq_offset < 0`, read the modseq at the offset, format with `jmap_state_string_cstate`, store into a json object under `dtype->name`. The only differences are the rock shape and that `add_userstate_cb` also honours `jmap_wantprop`. This is the same kind of drift-prone duplication the branch is otherwise eliminating in the gperf table. - **Why It Matters**: If the state-string convention changes again (a second prefix, a new stateless type, a guard refinement), a maintainer must remember to touch both. The whole thesis of this branch is "one source of truth"; the read/format step is the one spot where that thesis isn't applied. - **Suggestion**: Consider a shared `jmap_data_type_state_string(const jmap_data_type_t *dtype, struct conversations_state *cstate, const struct mboxname_counters *counters)` in the base library that returns the malloced state string (or NULL for stateless types). Both callbacks shrink to a wantprop check + `json_object_set_new`. Low priority — the duplication is small and readable — but it's the natural completion of the refactor. ### Finding 4: Two compiled copies of the data-types table — accept, but document the invariant - **Severity**: Info - **Location**: `imap/jmap_data_types.c` (base lib) + `#include` in `imap/jmap_api.c:196` (http daemon); `Makefile.am:1043-1044`. - **Issue**: The hidden-visibility constraint genuinely forces two TUs to embed the gperf table (the base lib can't export it to the daemon, and the daemon can't see the lib's hidden copy). The comments in both `jmap_api.c` and `jmap_data_types.c` explain this well. The residual risk is not the duplication per se — both copies are generated from the same `.gperf`, so they cannot drift in content — but that a future reader may "tidy up" one include and break the daemon or the base lib link. This is a reasonable, well-justified tradeoff; I would not change it. - **Why It Matters**: The two-copies decision is the kind of thing that gets "cleaned up" by someone who doesn't know about the visibility wall, especially since `jmap_data_types.c` looks like an empty file. - **Suggestion**: No code change required. Optionally, a one-line `static_assert` or build-time note isn't worth it; the existing comments are sufficient. I'd only ask that the PR description call out "this table is intentionally compiled twice" so it survives review memory. ### Finding 5: UserState/get reports all types regardless of per-type share visibility - **Severity**: Info - **Location**: `imap/jmap_core.c:301-325` (`add_userstate_cb` / `userstate_get`). - **Issue**: `UserState/get` emits a state for every stateful type backed by the account's counters, without consulting whether the calling user can actually read that type in the target account (a partial-share scenario). I verified this is **identical** to `UserCounters/get`, which exposes the same modseqs for the same account behind the same `JMAP_USERCOUNTERS_EXTENSION` capability gate, and that `jmap_api()` already enforces account-level authorization (`accountNotFound` / `accountNotSupportedByMethod`, `jmap_api.c:711-716`). So there is no marginal information exposure beyond what already ships. - **Why It Matters**: It's worth stating explicitly so the security reviewer and the Tech Lead's clarifying question ("should it omit types the caller can't read?") have a documented answer: the method inherits the account-scoped, capability-gated model of its sibling and adds nothing new. If the project ever decides per-type filtering matters, it must be added to *both* methods, not just this one. - **Suggestion**: No change for this branch. If per-type visibility filtering is desired as policy, file it as a follow-up that covers UserCounters/get too, so the two siblings stay consistent. ### Finding 6: `userstate_get` builds the singleton even when only unknown ids are requested - **Severity**: Info - **Location**: `imap/jmap_core.c:394-407`. - **Issue**: This faithfully mirrors `usercounters_get` (`jmap_core.c:294-307`): when `ids` is present, only an exact `"singleton"` id produces the object; anything else goes to `notFound`. Behavior is correct and consistent with the sibling and with JMAP singleton conventions. Calling out only to confirm I checked it against the Tech Lead's risk item — empty/`null` ids returns the singleton, non-singleton ids land in `notFound`, matching UserCounters. - **Why It Matters**: Consistency between the two singleton methods is the property that matters here, and it holds. - **Suggestion**: None. ## What's Working Well - **Single source of truth.** Driving the event parameter, the JMAP API, and push notifications from one gperf table is the right architecture and directly fixes documented drift (the old `jmap_states[]` was wrong in four distinct ways). The commit message inventory of those four bugs is excellent. - **Helper factoring.** Splitting `_state_string` into `jmap_state_string_prefixed` (mechanical) and `jmap_state_string_cstate` (policy: the `J`-prefix decision) puts the prefix rule in exactly one place, and `jmap_state_string` now delegates to it. Clean separation of mechanism from policy. - **Self-consistent gate.** Using the same `mboxevent_expected_param` predicate for both the mask check and the fill (line 1817) means the "filled-but-unexpected" failure mode is structurally impossible. That's the correct way to avoid the assert (my Finding 1 is about the *chosen set*, not the mechanism). - **UserState/get parse honesty.** The comment explaining why it can't use `jmap_get_parse` (gperf perfect-hash valid-prop set vs. data-type names) is exactly the kind of "why, not what" comment the project standards ask for, and the hand-rolled parser correctly mirrors the sibling's invalidArguments shape and fini path. - **Tests assert correspondence, not constants.** `JMAPCore/user_state` proving UserState agrees with each `Foo/get` rather than hardcoding modseqs is robust and self-maintaining, and the J-prefix cross-check is a nice touch. ## Clarifying Questions 1. Is the `EVENT_CALENDAR` narrowing in Finding 1 intentional? If yes, the commit message should document it alongside the other wire changes and a test should pin it; if no, the mask needs `EVENT_CALENDAR` (and an audit of `EVENT_CALENDAR_ALARM` / DAV events). 2. Is there any external `vnd.fastmail.jmapStates` consumer (push bridge, replication observer, monitoring) that keys off the dropped `Contact`/`ContactGroup`/`Quota`/`Racl` entries or the old `mailfoldersmodseq` Mailbox semantics? The commit treats the change as safe; confirming no internal consumer depends on the old shape would close the loop. ## Questions for Other Reviewers - **Testing**: Does `MboxEvent/jmap_states` (or any existing test) exercise a CalendarEventNew event so the Finding-1 narrowing would have been caught? My read is no — the only jmapStates assertions are on MessageNew. Can you confirm the calendar path is untested for this parameter, and consider adding a negative/positive case? - **Testing**: Flipping `conversations => yes` globally in `MboxEvent.pm::new` — does it perturb any existing MboxEvent test's expected counters/output, or is it inert for tests that don't inspect jmapStates? - **Security**: Do you concur that UserState/get's exposure is strictly equal to UserCounters/get (Finding 5), i.e. no new per-type leak in partial-share accounts?