Skip to main content

FEN — Implementation & Design Review (July 2026)

Status: recorded · full pass over the design docs (doc/*.md, ARCHITECTURE.md) and the implementation (packages/*, apps/fen, services/fen-provisioner, infra/*) as of 033c47e (branch fix/organiser-display-name, 2026-07-06). This is a follow-up to doc/design-review-findings.md (issues #15, #17–#35). That review's scope was the design at 5db2fed; this one focuses on what has landed since — the M1 flows, VerifyingEventSink, the invite-credential redesign, two-phase rotation, friends list, CI — and on new findings not tracked by an existing issue. Legend: 🔴 high · 🟠 medium · 🟡 low. Every new finding is tracked as a GitHub issue (#64-#73); the issue numbers are listed in the summary table. Companion script: scripts/file-review-issues.sh drafts one GitHub issue per new finding (N-1…N-10).

1. Where the project stands​

The architecture described in ARCHITECTURE.md is now real, end to end, in code: a local-first Flutter app whose only trust anchor is Ed25519 identity keys; a canonical, signable JSONL envelope (fen_events); a pure reducer with roster-authority semantics (fen_ledger); libsodium-backed crypto (fen_crypto); an S3/Garage storage adapter with on-device SigV4 signing (fen_sync); and a Go provisioner that mints per-group and single-use invite credentials against Garage's admin API.

Substantial, well-executed work has landed since the first review:

  • Signature verification at ingest is real (#26 → VerifyingEventSink). Decode → verify-before-decrypt → decrypt → dedupe → roster-authority check, with a typed EventRejection vocabulary that is part of the EventSink interface contract, and a genuine production call site (GroupSyncService, wired into groupBalanceProvider).
  • Membership acceptance is organiser-authoritative (#18): self-certifying MemberJoined only ever creates a pending row; MemberAdmitted (organiser-signed, provisioner-adjudicated winner via GET …/invites/{id}) promotes it. The duplicate-join tiebreak is no longer attacker-winnable.
  • Invites are no longer a permanent compromise (#17): the link carries a short-TTL, single-use Garage credential; the joiner exchanges it via …/invites/{id}/complete (atomic ClaimInvite under the store lock); a reaper denies and deletes used/expired keys.
  • Two-phase storage-key rotation (#15): rotate mints the new key and defers denial of the old one until owner confirmation or a deadline, with close-time demotion to read-only.
  • Provisioner hygiene is genuinely good: replay cache keyed on decoded pubkey/sig bytes (hex-case malleability closed), NUL-separated canonical signing message, constant unauthorized error, secrets never logged, best-effort rollback of orphan buckets/keys, quota per group, X-Forwarded-For untrusted.
  • Canonical serialization remains one of the strongest parts: re-encode-and-compare canonicality gate, decimal-string numbers, fixed-millisecond timestamps, sorted keys, and an explicit "no nulls, no bare numbers" encoder.
  • CI now runs flutter analyze + tests, pure-Dart package tests, and the Go gates (vet/build/test -race) on every PR.

The overall shape is unusually disciplined for a project at this stage: the layering in ARCHITECTURE.md matches the code, security-relevant decisions carry inline citations to the docs and issues that motivated them, and past review findings were actually closed with mechanisms rather than comments.

The findings below are, in that context, the next ring of problems: mostly integration gaps between components that are individually correct, plus a few authorization and durability holes that the existing issue set does not cover.

2. Summary of new findings​

#SevAreaOne-lineTracked
N-1🔴Syncpull() advances the ETag cursor before ingest — a crash or ingest failure permanently loses those events#64
N-2🔴AuthorizationAny active member can author MemberRemoved / MemberLeft-for-others / GroupClosed / GroupSettingsUpdated; reducer applies them without an organiser/self check#65
N-3🔴IntegrationThe app never calls POST /v1/groups; GroupStorageBinder writes random fake S3 credentials — the organiser's device can never sync, and invite minting will 404 against a real provisioner#68
N-4🟠Crypto/specsealEvent() computes aadBytes() but never binds it into the AEAD — §3.10's AAD binding is not on the wire#66
N-5🟠Onboarding/securityInvite deep links auto-join silently — no accept/decline UI (doc/fen.md Flow 4 is specified but unbuilt)#67
N-6🟠Invite flowProvisioner failure after ClaimInvite burns the invite and the client misreports it as inviteAlreadyUsed (a "compromise" signal)#69
N-7🟡Spec/implClosed-group immutability is partial: ExpenseLogged and SettlementRecorded are still accepted after GroupClosed#71
N-8🟡Log integrityauthor_seq contiguity is never checked at ingest — the §3.9 loss-provability mechanism has no implementation#73
N-9🟡Syncflush() dedupe uses a substring contains(event_id) over the whole log file; whole-file read-modify-write makes every flush O(log size)#70
N-10🟡MoneyCurrency-mismatch guard is a bare assert (no-op in release); Money.fromDecimalString silently truncates excess precision#72

3. Findings in detail​

N-1 🔴 pull() advances the sync cursor before ingest​

Tracked: #64

Where: packages/fen_sync/lib/src/sync_manager.dart, StorageBackendSyncManager.pull().

The pull loop does, per changed log file: fetch the object, append its lines to fresh, then immediately cursors.setCursor(groupId, path, etag) — and only after the loop calls sink.ingest(...). Two failure modes lose data permanently:

  1. The app crashes (or is killed) between setCursor and ingest — on the next pull the file's ETag matches the stored cursor, the file is skipped, and the events in it are never fetched again unless that author happens to write more.
  2. ingest throws for an infrastructure reason (the EventSink contract explicitly allows throwing for local-DB failure). The cursor is already advanced; same loss.

The rejection-path design work (typed EventRejection, "never silently dropped") is undermined by the cursor being committed before the sink has accepted the batch. The local group_events table is the durable record; the cursor must only move once the lines it covers are either in that table or deliberately rejected.

Direction. Move setCursor after a successful ingest, per file (fetch file → ingest its lines → set its cursor), or collect (path, etag) pairs and commit them after the sink returns. Re-ingesting a file is already safe (dedupe by event_id), so crash-between-ingest-and-cursor merely re-processes — the safe side of the race.

N-2 🔴 Roster-mutation events have no authorship authorization​

Tracked: #65

Where: packages/fen_ledger/lib/src/event_reducer.dart (_memberStatus, GroupClosedPayload, and the default arm), apps/fen/lib/core/data/verifying_event_sink.dart (_isAuthorized).

VerifyingEventSink._isAuthorized special-cases GroupCreated (trust root), MemberJoined (self-certifying, pending only) and MemberAdmitted (organiser only); every other event type passes if the author is any active member. The reducer then applies:

  • MemberRemoved { memberId: X } — status of X set to removed, regardless of who signed it. Any active member can remove any other member, including the organiser.
  • MemberLeft { memberId: X } — same handler; nothing requires memberId == authorId, so "leaving" on someone else's behalf is accepted.
  • GroupClosed — any active member can freeze the whole group (and since closure gates expense edits, this is a one-event griefing tool).
  • GroupSettingsUpdated — currently a no-op in the reducer, but the same gap will apply the moment it is implemented.

This was named in #26's resolution as "deliberately NOT covered" and it overlaps #31 (permission_rules.dart is stubbed), but neither issue actually tracks roster-mutation authorship as a concrete hole, and the severity changed once VerifyingEventSink became the real production gate: today the E2E-verified pipeline will happily apply a hostile MemberRemoved end to end. Removal also has knock-on effects on _isActiveAuthor — a removed organiser… remains authorized (createdBy check short-circuits), but a removed member's later events are rejected, so a malicious member can silence another member's future events by removing them.

Direction. In _isAuthorized (and mirrored in the reducer for defense in depth): MemberRemoved, GroupClosed, GroupSettingsUpdated require authorId == createdBy; MemberLeft requires payload.memberId == authorId. Add sink tests for each hostile case.

N-3 🔴 Group creation never provisions real storage credentials​

Tracked: #68

Where: apps/fen/lib/core/data/group_storage_binder.dart, apps/fen/lib/core/net/provisioner_client.dart (no createGroup method), services/fen-provisioner/internal/api/server.go (handleCreateGroup).

CreateGroupService binds every new group to the managed backend by writing a group_storage row containing locally generated random hex as access_key_id/secret_access_key. The provisioner's POST /v1/groups — which creates the bucket, quota, and the real shared key — is never called anywhere in the app; ProvisionerClient doesn't even have the method. Consequences, in order of appearance:

  1. The organiser's GroupSyncService.sync() builds an S3Backend from garbage credentials; every flush 403s (silently — sync() swallows into a debug log), so the organiser's GroupCreated/MemberJoined/expenses never reach the bucket.
  2. CreateInviteService.createInvite calls POST /v1/groups/{id}/invites for a group the provisioner has never heard of → 404 not found → invite creation throws. The entire invite → join → admit pipeline, individually well-built, is unreachable end to end against a real provisioner.

The binder's comment honestly declares this "a later milestone's job", but no issue tracks it, and it now sits directly between the two flagship flows (#17/#18 machinery on both sides of it). Anyone running the stack today will observe "everything works locally, nothing syncs" with no surfaced error.

Direction. Add createGroup to ProvisionerClient; call it in CreateGroupService/GroupStorageBinder and store the returned descriptor. Decide the offline story (group creation currently works fully offline — perhaps keep local creation and bind storage lazily on first flush, with a visible "not yet synced" state). Surface persistent flush failures to the UI instead of developer.log.

N-4 🟠 AAD is computed but never bound into the AEAD​

Tracked: #66

Where: apps/fen/lib/core/data/event_envelope_codec.dart (sealEvent), apps/fen/lib/core/data/verifying_event_sink.dart (decrypt call), packages/fen_events/lib/src/serialization.dart (aadBytes), doc/canonical-serialization.md §3.10/§7.

The spec's {a,e,g,i,p} AAD object exists (aadBytes()), but sealEvent() encrypts with no additionalData, and VerifyingEventSink decrypts with none — with an inline comment acknowledging the gap ("tracked separately from #26", though no issue number is cited in the repo). Today this is not independently exploitable, because the Ed25519 signature covers the same fields and is checked before decryption. But the layering intent of §3.10 is that the ciphertext itself refuses to decrypt in the wrong context — a defense that holds even for code paths that decrypt without re-verifying (none today, but nothing enforces that), and for cross-epoch/cross-group confusion once epochs are real.

Direction. Either wire aadBytes() into seal/decrypt behind a fmt bump (fmt=2 with AAD; accept fmt=1 without, for the existing wire history), or amend canonical-serialization.md to state that AAD is intentionally not bound and signature coverage is the sole context binding. The half-state — spec says bound, code computes it and throws it away — is the worst of the three options.

N-5 🟠 Invite links auto-join with zero user consent​

Tracked: #67

Where: apps/fen/lib/features/onboarding/invite_link_listener.dart (_handleUri → unawaited(_join(package))), doc/fen.md Flow 4 (specifies [Join Group] [Decline]).

Any successfully parsed invite link — via Universal Link or, until AASA/assetlinks are real (#34), the hijackable fen:// custom scheme — is joined immediately: the MemberJoined event is written, the single-use invite credential is redeemed with the provisioner, and the device navigates to the joined screen. The user is never asked. Combined with #34 this means tapping a link (or any app that can forge a custom-scheme open) consumes a real invite and inserts an identity into a group. It also burns the legitimate invitee's credential if a link is opened on the wrong device.

The planning notes acknowledge the missing accept/decline screen as future work, but no issue tracks it, and it is a security control (consent before identity-binding + credential redemption), not just UX polish.

Direction. Parse → show group name/inviter → explicit Join/Decline before acceptPackage runs. Keep the parse-only path silent.

N-6 🟠 A failed completion after ClaimInvite burns the invite and misreports compromise​

Tracked: #69

Where: services/fen-provisioner/internal/api/server.go (handleCompleteInvite), apps/fen/lib/features/onboarding/join_invite_service.dart (failure mapping).

ClaimInvite marks the invite used atomically before the Garage calls (correct for single-use). But if DenyBucketKey/GetKeyInfo then fail (502), the joiner never receives the shared credential, and their local join has been rolled back. Every retry now hits ErrInviteAlreadyUsed → the client maps this to inviteAlreadyUsed, which the code documents as "surface as a detectable compromise". So a transient upstream blip:

  1. permanently consumes the invite (the winner recorded is the honest joiner, who has nothing);
  2. tells the honest joiner someone else stole their invite;
  3. leaves the organiser's reconciliation seeing used + a winner pubkey that will never produce a pending MemberJoined (the join was rolled back) — a permanent no-op, and a local_friends placeholder stuck at invited until expiry.

Direction. Make completion resumable: if ClaimInvite finds the invite already used by the same memberPubkey, treat the call as an idempotent retry and return the shared credential again (the reaper already handles denying the invite key). Client-side, distinguish "used by me, retryable" from "used by someone else, compromise".

N-7 🟡 Closed-group immutability is only partial​

Tracked: #71

Where: packages/fen_ledger/lib/src/event_reducer.dart (_expenseLogged, _settlementRecorded), doc/data-model.md §"Group Status" ("GroupClosed — triggers full immutability") vs invariant 4 (lists only ExpenseUpdated/ExpenseDeleted).

The reducer freezes edits and deletes after GroupClosed, but new ExpenseLogged and SettlementRecorded events are still applied. The doc contradicts itself: the payload comment promises full immutability, the invariant table names only two event types. As implemented, any member can keep appending brand-new expenses to a closed (archived, settled) group forever, silently changing its balances. (Settlements after close are arguably a feature — settle-up happens after closing — which is exactly why the spec should say so explicitly.)

Direction. Decide the intended semantics, fix whichever side is wrong, and add reducer tests for post-close ExpenseLogged/SettlementRecorded.

N-8 🟡 author_seq contiguity is never validated​

Tracked: #73

Where: apps/fen/lib/core/data/verifying_event_sink.dart, doc/data-model.md §3.9 ("makes loss provable").

author_seq is assigned, signed, stored — and never read for its stated purpose. No component detects a gap (evidence of suppression, truncation by a bearer-key-wielding member, or storage rollback) or an inconsistency (two different accepted events from one author with the same seq — which the event_id dedupe does not catch, since a malicious author can sign two distinct events with the same seq and different ids). Since any member holds the shared write credential and can truncate others' log files (acknowledged in §4.3), gap detection is the only tamper-evidence mechanism the design has — and it is currently decorative. Related: the settlement completeness gate (#20) is not implemented either, so nothing consumes these counters today.

Direction. Track per-author max contiguous seq at ingest; surface gaps and duplicate seqs as a distinct EventRejection/warning state rather than silence. Coordinate with #19 (dedupe by (author_id, author_seq) would solve both).

N-9 🟡 flush() dedupe-by-substring and O(file) append​

Tracked: #70

Where: packages/fen_sync/lib/src/sync_manager.dart (flush).

Two smaller problems in the append path (both adjacent to, but not covered by, #23):

  1. Idempotent re-flush skips entries when existingText.contains(e.eventId) — a substring match over the entire log file. A UUID-shaped substring can legitimately occur inside another line's base64url ciphertext (the alphabet includes -), in which case a real event is silently never uploaded. Unlikely per-event, but the failure is silent permanent local-only data, and the check is trivially hardenable (parse lines' event_id fields, or match "event_id":"<id>").
  2. Because S3 has no append, every flush downloads the full log and re-uploads existing + new. Cost per flush grows linearly with history (and it round-trips the whole file twice more when the server omits the PUT ETag). With #23's per-event-object redesign on the table, this argues for deciding that redesign before the log format ossifies.

N-10 🟡 Money release-mode arithmetic and silent precision truncation​

Tracked: #72

Where: packages/fen_money/lib/src/money.dart.

Money.+/- guard currency mismatch with assert(...) only — in release builds mixing currencies silently adds minor units. And fromDecimalString('21.999', 'EUR') silently truncates to 21.99 (.substring(0, minorUnitDigits)) instead of rejecting, contradicting the codebase's own "reject, don't sanitize" rule (canonical-serialization.md §9). Today's call sites don't hit either path (single-currency v1, UI-validated input), so this is hardening — but this is exactly the class of layer that must not depend on its callers being polite.

Direction. Throw ArgumentError on mismatch/excess precision unconditionally.

4. Observations not filed as issues​

Recorded here so they aren't lost; each either overlaps an existing issue or is an accepted trade-off that deserves visibility.

Storage credential at rest (overlaps #33). The group's shared S3 secret_access_key lives in plaintext in SQLite (group_storage.descriptor), while the docs' "must never sit in SQLite" list covers only member_privkey and group_key. The storage credential is a bearer write+delete capability over the whole group; if #33's at-rest work happens, the descriptor belongs in scope (or in Keychain alongside the group key).

Epoch is ornamental in v1. Envelopes carry epoch, but VerifyingEventSink derives content_key from the current group key regardless, and ReplayProjector reads events back with epoch: 0. Fine while rotation is out of scope (#22), but the first KeyEpochUpdate implementation must add epoch-aware key selection at decrypt and start persisting the envelope's epoch — worth remembering in #22's scope.

Full replay on every touch. project() (full log fold + projection rebuild) runs on every group view, every expense add, every sink batch (twice when admissions reconcile). Deliberate and documented as the unoptimised spike, and the benchmark exists — but the number of call sites multiplying that cost is growing; snapshot caching will likely be forced earlier than the docs assume.

Attachment fetch never re-verifies the hash. AttachmentStore.fetch(path) returns whatever bytes the backend serves without checking sha256(ciphertext) against the path. AEAD decryption will fail for tampered blobs, so this is defense-in-depth, not a hole — cheap to add when attachments get UI.

Join flow holds a DB transaction across a network call. JoinInviteService runs completeInvite (HTTP) inside the SQLite transaction to get atomic rollback. Correct semantics, but a slow provisioner stalls the writer lock; consider a journal/saga if it shows up in traces.

S3 XML parsing by regex. Keys containing XML-escaped characters would be mis-read. All FEN-generated keys are ASCII-safe, but any member with the bearer key can create arbitrary keys in the bucket; robustness argues for entity-decoding <Key> at least.

GET invite-status carries the signature in query params. Signatures aren't secrets and the replay cache bounds reuse, but URLs land in proxy/access logs; within the replay window a logged URL is a replayable request. Harmless today (read-only endpoint, owner-only data), worth remembering if more GETs are added.

Joiner display names are placeholders. JoinInviteService hardcodes 'Member <pubkey-prefix>' — the joiner is never asked for a name (the organiser-side fix/organiser-display-name work solves the organiser's own name only). The accept/decline screen (N-5) is the natural place to collect it.

5. Suggested order of attack​

  1. N-3 (provision real credentials) — it gates any real end-to-end use of everything built in the last two milestones, and its failure mode is silent.
  2. N-1 (cursor-after-ingest) and N-2 (roster-mutation authz) — both are cheap, contained fixes to the two mechanisms the security story leans on hardest.
  3. N-5 + N-6 (consent UI + resumable completion) — they close the remaining sharp edges of the invite pipeline, which is otherwise in good shape.
  4. N-4, N-7, N-8 with their spec updates — alignment work best done before the wire format and reducer behavior accrete more history.
  5. N-9, N-10 as hardening alongside #23 and normal package work.