feat(labeler): notification contact-state foundation (W10.4 slice B)#2068
Conversation
|
github-actions
Bot
added
review/needs-review
Scope checkThis PR changes 1,144 lines across 6 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | aefdcdc | Jul 16 2026, 08:00 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-do | aefdcdc | Jul 16 2026, 08:00 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | aefdcdc | Jul 16 2026, 08:00 AM |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is the right shape for W10.4 slice B: a focused, binding-independent foundation for publisher notification contacts. The migration keeps plaintext emails out of the schema by construction, the HMAC-SHA256 hashing mirrors the existing signing-runtime.ts pattern, and the tests cover the full state machine, suppression idempotency, rate-gate boundaries, and schema assertion. No regressions in the core labeler code paths.
What I checked:
- Migration 0007 schema, CHECK constraints, and absence of plaintext/address columns.
src/notification-contacts.tshashing, contact/suppression primitives, and D1 parameter binding.- Test coverage and miniflare binding configuration.
wrangler.jsoncsecret declaration and generatedworker-configuration.d.ts.
Headline conclusion: the implementation is clean and the slice is mergeable. I have a few non-blocking suggestions to harden the state machine and remove small future footguns.
One note on diff noise: worker-configuration.d.ts was regenerated with a newer workerd runtime version (1.20260611.1 → 1.20260708.1), so most of that file’s diff is unrelated API churn. The meaningful change is the new NOTIFICATION_HASH_PEPPER binding and ProcessEnv pick update. Consider regenerating with the pinned workerd version if that diff is undesirable, though it has no runtime effect given the unchanged compatibility_date.
Changeset: n/a — @emdash-cms/labeler is private.
| .prepare( | ||
| `UPDATE notification_contacts | ||
| SET confirm_token_hash = ?, last_confirm_sent_at_epoch_ms = ? | ||
| WHERE recipient_hash = ?`, |
There was a problem hiding this comment.
[suggestion] recordConfirmSent updates confirm_token_hash for any row matching the recipient hash, regardless of confirm_state. In the documented flow callers gate on canSendConfirm, which only permits unconfirmed contacts, but adding AND confirm_state = 'unconfirmed' hardens the primitive against a buggy caller that skips the predicate. ```suggestion
UPDATE notification_contacts SET confirm_token_hash = ?, last_confirm_sent_at_epoch_ms = ? WHERE recipient_hash = ? AND confirm_state = 'unconfirmed',
| .prepare( | ||
| `UPDATE notification_contacts | ||
| SET confirm_state = 'confirmed', confirm_token_hash = NULL, confirmed_at = ? | ||
| WHERE recipient_hash = ? AND confirm_token_hash = ?`, |
There was a problem hiding this comment.
[suggestion] confirmContact verifes the token hash but not that the contact is still unconfirmed. Defending the state machine at the UPDATE means a stale token on a declined or already-confirmed row cannot be replayed to flip the state. ```suggestion
UPDATE notification_contacts SET confirm_state = 'confirmed', confirm_token_hash = NULL, confirmed_at = ? WHERE recipient_hash = ? AND confirm_state = 'unconfirmed' AND confirm_token_hash = ?,
| VALUES (?, ?, ?, ?) | ||
| ON CONFLICT(recipient_hash) DO NOTHING`, | ||
| ) | ||
| .bind(recipientHashValue, reason, nowIso, Date.parse(nowIso)) |
There was a problem hiding this comment.
[suggestion] suppress derives created_at_epoch_ms via Date.parse(nowIso), which returns NaN for an invalid ISO string. A bad caller string could silently write a 0/NaN epoch or fail at runtime depending on the D1/SQLite behavior. Since other helpers like recordConfirmSent already take an explicit epochMs, consider passing it here too (or at least asserting Number.isFinite(epochMs)). ```suggestion
export async function suppress(
db: D1Database,
recipientHashValue: string,
reason: SuppressionReason,
nowIso: string,
nowEpochMs: number,
): Promise {
await db
.prepare(
INSERT INTO notification_suppressions (recipient_hash, reason, created_at, created_at_epoch_ms) VALUES (?, ?, ?, ?) ON CONFLICT(recipient_hash) DO NOTHING,
)
.bind(recipientHashValue, reason, nowIso, nowEpochMs)
.run();
}
| * so it runs unchanged on workerd. | ||
| */ | ||
| export async function recipientHash(pepper: string, email: string): Promise<string> { | ||
| const key = await crypto.subtle.importKey( |
There was a problem hiding this comment.
[suggestion] recipientHash imports the pepper into a CryptoKey on every call. For W10.5 batch hashing this wastes CPU on identical importKey work. The mapping from pepper string to CryptoKey is deterministic and the key is non-extractable, so caching it in a module-level Map is safe. ```suggestion
const keyCache = new Map<string, CryptoKey>();
export async function recipientHash(pepper: string, email: string): Promise {
let key = keyCache.get(pepper);
if (!key) {
key = await crypto.subtle.importKey(
"raw",
encoder.encode(pepper),
{ name: "HMAC", hash: "SHA-256" },
false,
["sign"],
);
keyCache.set(pepper, key);
}
const signature = await crypto.subtle.sign("HMAC", key, encoder.encode(email.trim().toLowerCase()));
return toHex(signature);
}
Add the opt-in and suppression state for publisher notifications: `notification_contacts` (confirm state machine with single-use, constant-time-compared token hashes and a confirmation rate-limit timestamp) and `notification_suppressions` (bounce/complaint/ unsubscribe/not-me, earliest reason retained), plus the peppered HMAC-SHA256 recipient-hash helper and a typed query layer. Both tables key on the recipient hash only — no plaintext address column exists; plaintext will live solely on the future W10.5 delivery row. Distinct from the operator `notification_outbox` alert subsystem.
- recordConfirmSent/confirmContact gate the UPDATE on confirm_state = 'unconfirmed', so a confirmed or declined row is never reopened and a stale token can never flip it; recordConfirmSent returns whether a row was touched. - suppress takes an explicit finite epochMs instead of re-parsing the ISO string. - recipientHash caches the imported HMAC key per pepper on globalThis.
28bb187 to
6917a83
Compare
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
This PR adds the W10.4 slice B contact-state foundation: HMAC-SHA256 recipient hashing, the double-opt-in state machine, a suppression ledger, and a rate gate. The approach is sound, matches the existing signing-runtime.ts secret-binding pattern, and keeps plaintext out of the schema by construction. Tests cover hash determinism/pepper-sensitivity, token single-use, suppression idempotency, rate-gate boundaries, and the no-plaintext schema assertion.
The prior review found the slice mergeable with a few non-blocking hardening suggestions. On re-reviewing the current head, I found one of those state-machine footguns is an actual gap that should be fixed before callers rely on it, plus two minor hardening notes.
What I checked:
- Migration 0007 schema, CHECK constraints, and absence of plaintext address/email columns.
src/notification-contacts.tshashing, HMAC key cache, state machine, suppression, and D1 parameter binding.wrangler.jsonc/worker-configuration.d.tssecret wiring (the workerd version churn noted in the prior review appears addressed: the regenerated file is now at the pinned1.20260611.1version).vitest.config.tsbinding and test coverage.
Headline conclusion: the slice is fundamentally clean and the right shape for W10.4. The one fix worth making now is adding the missing unconfirmed state guard to declineContact so the state machine is consistent with confirmContact and recordConfirmSent.
| export async function declineContact(db: D1Database, recipientHashValue: string): Promise<void> { | ||
| await db | ||
| .prepare( | ||
| `UPDATE notification_contacts | ||
| SET confirm_state = 'declined', confirm_token_hash = NULL | ||
| WHERE recipient_hash = ?`, | ||
| ) |
There was a problem hiding this comment.
[needs fixing] declineContact currently downgrades a contact to declined regardless of its current state. The migration describes the state machine as unconfirmed -> confirmed | declined, and both recordConfirmSent and confirmContact already guard on confirm_state = 'unconfirmed'. Allowing declined from confirmed means a later opt-out/unsubscribe caller could accidentally revoke an existing opt-in. Add the same state guard, return whether a transition happened, and add a test that declineContact is a no-op on a confirmed row.
| export async function declineContact(db: D1Database, recipientHashValue: string): Promise<void> { | |
| await db | |
| .prepare( | |
| `UPDATE notification_contacts | |
| SET confirm_state = 'declined', confirm_token_hash = NULL | |
| WHERE recipient_hash = ?`, | |
| ) | |
| /** | |
| * Decline an unconfirmed contact, clearing any outstanding confirm token. | |
| * Returns whether the contact was transitioned from `unconfirmed`. | |
| */ | |
| export async function declineContact( | |
| db: D1Database, | |
| recipientHashValue: string, | |
| ): Promise<boolean> { | |
| const result = await db | |
| .prepare( | |
| `UPDATE notification_contacts | |
| SET confirm_state = 'declined', confirm_token_hash = NULL | |
| WHERE recipient_hash = ? AND confirm_state = 'unconfirmed'`, | |
| ) | |
| .bind(recipientHashValue) | |
| .run(); | |
| return result.meta.changes > 0; | |
| } |
| export async function recordConfirmSent( | ||
| db: D1Database, | ||
| recipientHashValue: string, | ||
| tokenHash: string, | ||
| epochMs: number, | ||
| ): Promise<boolean> { | ||
| const result = await db | ||
| .prepare( | ||
| `UPDATE notification_contacts | ||
| SET confirm_token_hash = ?, last_confirm_sent_at_epoch_ms = ? | ||
| WHERE recipient_hash = ? AND confirm_state = 'unconfirmed'`, | ||
| ) | ||
| .bind(tokenHash, epochMs, recipientHashValue) | ||
| .run(); | ||
| return result.meta.changes > 0; |
There was a problem hiding this comment.
[suggestion] suppress rejects non-finite epochMs, but recordConfirmSent accepts any number. Storing NaN or Infinity as last_confirm_sent_at_epoch_ms breaks the numeric rate-gate comparison in canSendConfirm and contradicts the finite assumption. Add the same guard for consistency.
| export async function recordConfirmSent( | |
| db: D1Database, | |
| recipientHashValue: string, | |
| tokenHash: string, | |
| epochMs: number, | |
| ): Promise<boolean> { | |
| const result = await db | |
| .prepare( | |
| `UPDATE notification_contacts | |
| SET confirm_token_hash = ?, last_confirm_sent_at_epoch_ms = ? | |
| WHERE recipient_hash = ? AND confirm_state = 'unconfirmed'`, | |
| ) | |
| .bind(tokenHash, epochMs, recipientHashValue) | |
| .run(); | |
| return result.meta.changes > 0; | |
| export async function recordConfirmSent( | |
| db: D1Database, | |
| recipientHashValue: string, | |
| tokenHash: string, | |
| epochMs: number, | |
| ): Promise<boolean> { | |
| if (!Number.isFinite(epochMs)) { | |
| throw new TypeError("recordConfirmSent requires a finite epochMs"); | |
| } | |
| const result = await db | |
| .prepare( | |
| `UPDATE notification_contacts | |
| SET confirm_token_hash = ?, last_confirm_sent_at_epoch_ms = ? | |
| WHERE recipient_hash = ? AND confirm_state = 'unconfirmed'`, | |
| ) | |
| .bind(tokenHash, epochMs, recipientHashValue) | |
| .run(); | |
| return result.meta.changes > 0; | |
| } |
| .prepare(`SELECT confirm_token_hash FROM notification_contacts WHERE recipient_hash = ?`) | ||
| .bind(recipientHashValue) | ||
| .first<{ confirm_token_hash: string | null }>(); | ||
| if (!row || row.confirm_token_hash === null) return false; |
There was a problem hiding this comment.
[suggestion] The short-circuit return false paths before constantTimeEqual mean the function's elapsed time reveals (a) whether the recipient hash exists and (b) whether it has an active confirmation token, partially defeating the constant-time goal. Since recipient_hash is exposed in confirmation links, consider restructuring so the comparison always runs against a dummy fixed-length value when no real token is present, or document that only the XOR comparison itself — not the whole function — is intended to be constant-time.
github-actions
Bot
added
review/awaiting-author
- declineContact gates on confirm_state = 'unconfirmed' and returns a boolean, so an unsubscribe-path bug can't revoke a confirmed opt-in. - recordConfirmSent rejects a non-finite epochMs (mirrors suppress) to keep NaN out of the rate-gate comparison. - confirmContact always runs the constant-time token compare, against a fixed-length dummy when the row or token is absent, so elapsed time never reveals recipient existence or a pending token.
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
Re-review of W10.4 slice B (contact-state foundation).
Approach judgment: The slice is the right shape for the W10.4 workstream. Keeping contact state hash-keyed, putting plaintext only on the future delivery row, and using raw D1 with parameter binding fits the labeler’s architecture and matches the existing signing-runtime.ts secret-binding pattern. The HMAC-SHA256 recipient hashing is deterministic, pepper-sensitive, and uses the globalThis key-cache idiom consistently.
What I checked
- Migration 0007 schema:
recipient_hashprimary keys, no plaintext columns,CHECKconstraints onconfirm_stateand suppressionreason. src/notification-contacts.ts: hashing, HMAC key cache,recordConfirmSent/confirmContact/declineContactstate guards, token single-use,suppressidempotency, and D1 parameter binding.wrangler.jsonc/worker-configuration.d.tssecret wiring andvitest.config.tstest binding.- Test coverage for the state machine, suppression earliest-reason retention, rate-gate boundaries, and the no-plaintext schema assertion.
Headline conclusion: The prior-review gap is fixed — declineContact now guards on confirm_state = 'unconfirmed' so it cannot revoke a confirmed opt-in, and that path is tested. The remaining issue is a test-only convention note (SQL identifier interpolation). I did not run pnpm typecheck/lint/test because there is no shell in this environment; those passing is unverified from this review.
No changeset is required because @emdash-cms/labeler is private, and there are no user-facing admin UI strings in this slice.
| async function columnNames( | ||
| table: "notification_contacts" | "notification_suppressions", | ||
| ): Promise<string[]> { | ||
| const result = await db().prepare(`PRAGMA table_info(${table})`).all<{ name: string }>(); |
There was a problem hiding this comment.
[suggestion] The columnNames helper interpolates a table name into a PRAGMA table_info(...) string. The table argument is a typed union literal in this file, so there is no runtime injection risk today, but it still violates the repo convention of never interpolating into SQL. Since SQLite PRAGMA table_info cannot parameterize the table name, validate the identifier explicitly.
| const result = await db().prepare(`PRAGMA table_info(${table})`).all<{ name: string }>(); | |
| async function columnNames( | |
| table: "notification_contacts" | "notification_suppressions", | |
| ): Promise<string[]> { | |
| if (!/^[a-z_]+$/.test(table)) { | |
| throw new Error(`invalid table name: ${table}`); | |
| } | |
| const result = await db().prepare(`PRAGMA table_info(${table})`).all<{ name: string }>(); | |
| return result.results.map((row) => row.name); | |
| } |
github-actions
Bot
added
review/awaiting-author
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
Approach judgment: This is the right slice for W10.4. Keeping contact state hash-keyed, leaving plaintext to the future delivery row, and using raw D1 with parameter binding fits the labeler’s architecture and mirrors the existing signing-runtime.ts secret-binding pattern. The HMAC-SHA256 hashing is deterministic, pepper-sensitive, and uses the globalThis key-cache idiom consistently.
What I checked (re-review)
- Migration 0007 schema:
recipient_hashPKs, no plaintext columns,CHECKconstraints onconfirm_stateand suppressionreason. src/notification-contacts.ts: hashing, HMAC key cache,recordConfirmSent/confirmContact/declineContactstate guards, token single-use,suppressidempotency, and D1 parameter binding.wrangler.jsonc/worker-configuration.d.tssecret wiring andvitest.config.tstest binding.- Restated prior-review gap:
declineContactstill guards onconfirm_state = 'unconfirmed', and that path is tested, so the prior fix is still in place.
Headline conclusion: The code is sound for the slice. The only remaining note is a test-only convention suggestion: the PRAGMA table_info helper interpolates the table identifier after validation, which is safe but could be tightened to an allowlist. No blocking issues.
No changeset is required because @emdash-cms/labeler is private, and there are no user-facing admin UI strings in this slice. I did not run pnpm typecheck/lint/test because there is no shell in this environment; those passing is unverified from this review.
| // PRAGMA cannot parameterize the table name; validate the identifier before | ||
| // interpolating. | ||
| if (!/^[a-z][a-z0-9_]*$/.test(table)) throw new TypeError(`invalid table identifier: ${table}`); | ||
| const result = await db().prepare(`PRAGMA table_info(${table})`).all<{ name: string }>(); |
There was a problem hiding this comment.
[suggestion] The PRAGMA table_info(${table}) call validates the identifier before interpolating, so it is not an injection bug. To make the test fully robust and avoid raw identifier interpolation altogether, consider an explicit allowlist of the two table names instead of a regex. This also makes it obvious at a glance that only these tables are ever queried here.
| const result = await db().prepare(`PRAGMA table_info(${table})`).all<{ name: string }>(); | |
| const allowed: Record<typeof table, string> = { | |
| notification_contacts: "notification_contacts", | |
| notification_suppressions: "notification_suppressions", | |
| }; | |
| if (!(table in allowed)) throw new TypeError(`invalid table identifier: ${table}`); | |
| const result = await db().prepare(`PRAGMA table_info(${allowed[table]})`).all<{ name: string }>(); |
github-actions
Bot
added
review/awaiting-author
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
Re-review of W10.4 slice B — contact-state foundation for publisher notifications.
Approach judgment: This remains the right slice. Keeping contact state hash-keyed, leaving plaintext to the future W10.5 delivery row, and using raw D1 prepared statements with parameter binding fits the labeler’s architecture and mirrors the existing signing-runtime.ts secret-binding pattern. The HMAC-SHA256 hashing is deterministic, pepper-sensitive, and uses the globalThis key-cache idiom consistently.
What I checked
- Migration
0007_notification_contacts.sql:recipient_hashPKs, no plaintext columns,CHECKconstraints onconfirm_stateand suppressionreason, forward-only SQL. src/notification-contacts.ts: HMAC key cache, state guards inrecordConfirmSent/confirmContact/declineContact, single-use token, constant-time compare, conditional UPDATE closing the read-then-write race, and D1 parameter binding throughout.wrangler.jsonc/worker-configuration.d.ts:NOTIFICATION_HASH_PEPPERadded as a required secret and typed binding.vitest.config.ts: test binding stub for the new secret.test/notification-contacts.test.ts: full state-machine coverage, wrong-token rejection, token single-use, suppression idempotency/earliest-reason retention, rate-gate boundaries, schema assertion, and finite-epoch validation.
Prior review follow-up: My previous test-only convention note about the PRAGMA table_info helper has been addressed — the helper now validates the table identifier against an explicit allowlist before interpolating it.
Headline conclusion: Clean for the slice. No blocking issues remain. Tooling passes (pnpm typecheck/lint/test) are unverified from this static review.
github-actions
Bot
added
review/approved
467b4d4
into
feat/plugin-registry-labelling-service
1 participant
What does this PR do?
Implements W10.4 slice B — the contact-state foundation for publisher notifications, per the ratified W10.4 decisions paragraph (integration
0a2eba12). Part of the plugin-registry labelling service (umbrella #1909, RFC #694). This slice is the binding-independent half: the opt-in/suppression state and hashing; the contact resolver (slice A) follows the W8.4 slice-3 aggregator binding, and delivery/sending is W10.5.What lands
notification_contacts(recipient-hash PK;confirm_stateunconfirmed/confirmed/declined; single-useconfirm_token_hash;last_confirm_sent_at_epoch_msas the confirmation rate-limit gate) andnotification_suppressions(bounce/complaint/unsubscribe/not_me; earliest reason retained). Both tables are hash-keyed point lookups — no plaintext address column exists by construction (schema-asserted in tests); plaintext will live only on the future W10.5 delivery row. Header comments distinguish these from the operatornotification_outboxalert subsystem, reciprocally to migration 0005's.NOTIFICATION_HASH_PEPPER, wired intowrangler.jsoncrequired secrets following the signing-key pattern); 64-char lowercase hex. Normalization is deliberately minimal per the ratified decision — provider-style dot/plus variants do not collide (tested).ensureContact→recordConfirmSent→confirmContact/declineContact), suppression (isSuppressed/suppress, idempotent), and the purecanSendConfirmrate-gate predicate.confirmContactdoes a constant-time token-hash comparison and a conditional UPDATE gated on the stored hash (closes the read-then-write race); the token clears on success (single-use).Tests
20 new (suite 573→593 workers; console 100 and calibration 39 unchanged): hash determinism/normalization/pepper-sensitivity, the full confirm state machine including wrong-token rejection and token single-use, suppression idempotency and earliest-reason retention, rate-gate boundaries, and the no-plaintext schema assertion.
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure
Screenshots / test output
Try this PR
Open a fresh playground →
A full working EmDash site, deployed from this branch. Each visit gets its own session-scoped sandbox: no login needed and no shared state. Try the admin, edit content, hit the public site.
Tracks
feat/labeler-notification-contacts. Updated automatically when the playground redeploys.