Skip to content

test(email): add unit tests for webmail server security primitives#219

Open
ahmedhesham6 wants to merge 2 commits into
oblien:mainfrom
ahmedhesham6:test/email-server-coverage
Open

test(email): add unit tests for webmail server security primitives#219
ahmedhesham6 wants to merge 2 commits into
oblien:mainfrom
ahmedhesham6:test/email-server-coverage

Conversation

@ahmedhesham6

@ahmedhesham6 ahmedhesham6 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Refs #216 — picking up the call for test coverage before the stable release.

What and why

apps/email/server had no tests. This adds 43 across the five modules where a silent regression is most expensive, all pure logic with no IMAP/SMTP or network dependency:

module tests what it protects
crypto.ts 9 AES-256-GCM encryption of stored IMAP passwords
rate-limit.ts 8 the sign-in brute-force guard
sanitize.ts 12 the XSS boundary for HTML from arbitrary senders
client-ip.ts 5 the key every rate limiter buckets on
schemas.ts 9 sign-in validation and settings bounds

No file under apps/email/server/src is modified.

Cases aim at boundaries rather than happy paths — GCM tamper detection in the IV, ciphertext and tag separately; the exact 28/27-byte payload framing; IV uniqueness across identical plaintexts; the limiter's exact max/max+1 and the exact rollover instant; the retryAfter floor; that x-forwarded-for stays untrusted even when x-real-ip is absent; and that signInSchema keeps rejecting client-supplied imapHost/smtpHost — the check that stops a phishing page redirecting a password to an attacker-controlled mail host.

How I verified it

Every test was checked by mutating the source and confirming it fails. Reverted after each, and re-run after the vitest conversion below with identical counts:

mutation result
honour x-forwarded-for as a fallback 2 fail
drop .strict() from signInSchema 1 fail
remove the rel/target link hardening 6 fail
rate-limit off-by-one (>=>) 7 fail
skip GCM setAuthTag 4 fail
fixed IV instead of randomBytes 1 fail

Also run and green:

  • bun run test at the repo root — 8/8 workspaces
  • bun run build at the repo root — 12/12 tasks
  • tsc --noEmit in apps/email/server — exit 0
  • bun format — clean
  • release bundle (apps/email/dist) contains no test files and no vitest

Runner

Tests use vitest, matching the seven other workspaces that test. vitest ^4.0.18 is added as a devDependency using the same spec as the rest, resolving to 4.0.18 exactly as at root.

Two files needed more than an import swap, both worth a reviewer's eye:

  • rate-limitsetSystemTime/Bun.sleep became vi fake timers, which makes the GC case stronger: it previously slept 20ms of real time and hoped the sweep had run, and now advances the timer queue so the interval actually fires. This needs advanceTimersByTime, not setSystemTime — the latter moves Date.now() while leaving the timer pending, so nothing is collected and the test passes vacuously.
  • client-iphono/bun's barrel also pulls in the SSG and websocket adapters, which dereference the Bun global at import, so the file threw Bun is not defined under vitest's Node runtime before any test ran. It is mocked down to the single function used, getConnInfo, whose own module is runtime-agnostic. Everything under test (header precedence, trimming, the throwing-fallback path) is clientIp's own logic and stays real — the x-forwarded-for mutation still fails 2 tests through the mock.

@types/bun stays in place: the server genuinely runs on Bun (bun:sqlite, hono/bun). Only the test runner moved.

Wiring

  • @zero/server is not a turbo workspace memberworkspaces is apps/* and it sits a level deeper, so a test script there alone would never run in CI. The @repo/email orchestrator forwards to it with the same cd server && pattern its existing scripts use. @repo/email now appears in the turbo test graph.
  • It also keeps its own bun.lock rather than being hoisted from root, so vitest had to be installed there; that is the lockfile change in the diff.
  • tsconfig.json did not include test/, so the tests were invisible to tsc. Adding it surfaced one real type error (HeadersInit is not in scope under lib: ["ES2022"]), now fixed. build is tsc --noEmit and the release entrypoint is server/src/main.ts, so nothing under test/ ships.

One deliberate choice worth flagging

Where sanitize-html's real behavior is weaker than one might assume, the test asserts what actually happens and comments the residual risk, rather than asserting a guarantee that does not hold. Two such cases: <style> is allowed and retains javascript: in CSS, and a percent-encoded scheme (JaVaScRiPt%3A…) survives the scheme check.

I have not changed any sanitizer behavior here — that is a behavior change and per CONTRIBUTING belongs in its own issue, opened as #220. If you would rather these tests assert the hardened behavior instead, say so and I will follow whichever way you decide.

Tests also avoid the .dev-secrets.json write hazard in src/env.ts by setting SESSION_ENCRYPTION_KEY and BRANDING_ADMIN_TOKEN before that module loads; confirmed no .dev-secrets.json or data/ directory is created by a run.

Unrelated pre-existing issue spotted

apps/email's lint script runs cd server && bun run lint, but neither server nor client defines a lint script, so it fails on clean main. It is not in the turbo graph so CI never calls it. Left alone as out of scope — happy to send a one-line fix separately if useful.

apps/email/server had no tests. This adds 43 covering the modules where a
silent regression is most costly, all pure logic with no IMAP/SMTP or
network dependency.

- crypto.ts      session credential encryption (AES-256-GCM)
- rate-limit.ts  the sign-in brute-force guard
- sanitize.ts    the XSS boundary for HTML from arbitrary senders
- client-ip.ts   the key every rate limiter buckets on
- schemas.ts     sign-in validation and settings bounds

Cases target the boundaries rather than the happy path: GCM tamper
detection in the IV, ciphertext and tag separately; the exact 28/27-byte
payload framing; IV uniqueness across identical plaintexts; the rate
limiter's exact max/max+1 and rollover instants plus the retryAfter
floor; that x-forwarded-for stays untrusted even with no x-real-ip; and
that signInSchema keeps rejecting client-supplied imapHost/smtpHost,
which is what stops a phishing page redirecting a password to an
attacker-controlled mail host.

Each test was checked by mutating the source and confirming it fails:
trusting x-forwarded-for fails 2, dropping .strict() fails 1, removing
the rel/target link hardening fails 6, a rate-limit off-by-one fails 7,
and disabling the GCM auth tag fails 4.

Where sanitize-html's real behavior is weaker than one might assume, the
test asserts what actually happens and comments the residual risk rather
than asserting an untrue guarantee: <style> is allowed and keeps
javascript: in CSS, and a percent-encoded scheme survives the scheme
check. No sanitizer behavior is changed here.

Wiring notes: @zero/server is nested below the apps/* workspace glob, so
it is not a turbo member and a test script there alone would never run in
CI; the @repo/email orchestrator forwards to it using the same
"cd server &&" pattern as its lint script. tsconfig.json did not include
test/, so the tests were unchecked by tsc -- adding it surfaced one real
type error, now fixed. build is tsc --noEmit and the release entrypoint
is server/src/main.ts, so nothing under test/ ships.

Refs oblien#216

No files under apps/email/server/src were modified.
…rkspace

The first pass used `bun test`, on the reasoning that the package is
Bun-native (bun:sqlite, hono/bun, @types/bun) and had no vitest
dependency. That was the wrong call: seven of the eight workspaces that
test at all use `vitest run`, so a lone Bun runner would have been an
odd one out for a contributor to trip over.

Converted all five files. Two needed more than an import swap:

- rate-limit: `setSystemTime`/`Bun.sleep` -> vi fake timers. The GC case
  is stronger for it. It used to sleep 20ms of real time and hope the
  sweep had run; it now advances the timer queue so the interval
  actually fires. Note this needs advanceTimersByTime, not
  setSystemTime -- the latter moves Date.now() while leaving the timer
  pending, so nothing is ever collected and the test passes vacuously.

- client-ip: `hono/bun`'s barrel also pulls the SSG and websocket
  adapters, which dereference the `Bun` global at import, so the file
  threw `Bun is not defined` under vitest's Node runtime before any test
  ran. Mocked to the one function used, getConnInfo, whose own module is
  runtime-agnostic. The logic under test (header precedence, trimming,
  the throwing-fallback path) is all clientIp's own and stays real -- I
  re-ran the x-forwarded-for mutation through the mock and it still
  fails 2 tests, same as before.

vitest ^4.0.18 added as a devDependency, matching the spec used by the
other seven. apps/email/server keeps its own bun.lock (it is not a
member of the root apps/* workspace glob), so the dependency had to be
installed there rather than inherited by hoisting -- hence the lockfile
change. Pinned resolution is 4.0.18, identical to root; a plain install
had drifted to 4.1.10.

@types/bun stays: the server genuinely runs on Bun at runtime. Only the
test runner moved.

Re-verified all six mutations under the new runner, with failure counts
unchanged: XFF trust 2, .strict() 1, rel/target 6, off-by-one 7,
setAuthTag 4, static IV 1. Root `bun run test` 8/8 and `bun run build`
12/12 pass; the release bundle contains no test files and no vitest.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant