fix(labeler): notification contact-resolution, unsubscribe, and verification-trust fixes (review round 1)#2116
Conversation
A one-click unsubscribe POST carries the recipient hash (c) in the List-Unsubscribe header URL's query string and only List-Unsubscribe=One-Click in the body. The POST parser read c from the form body only, so a one-click request returned success without suppressing the recipient. Read c (and the confirm token) from the body first, falling back to the query.
A release subject's rkey is slug:version, but contactTargetFromUri passed the whole rkey as the package slug, so getPackage missed the package and its security[]/authors[] contacts were skipped in favor of the publisher-profile fallback. Parse the parent package slug (before the version delimiter) so a release notice reaches the package's contacts; a package rkey and a malformed rkey both degrade safely.
An emergency takedown issuance that resolves no publisher contact recorded only a generic undeliverable notification row, so the signal that manual outreach is needed was easy to miss. Emit a dedicated takedown-no-contact operational event (severity high) plus its operator-alert outbox row on that path. A takedown retract and a takedown that does resolve a contact emit nothing, and a deduped replay does not re-emit.
A publisher's verification claim upgraded a contact past double opt-in on the strength of any in-force claim. Verification claims are self-assertable — the aggregator indexes any issuer, including self-issued ones — so a publisher could name a victim address in its profile, self-issue a claim, and have an unconfirmed notice sent there. Upgrade only on a claim whose issuer is in the configured trust set, that is unexpired, and whose bound displayName still matches the publisher's current identity. No issuer is trusted by default, so every address falls back to double opt-in. The publisher view carries a current displayName but no handle (the aggregator's identity-event ingestion is unbuilt), so the binding checks displayName only; handle-binding is deferred until the view carries a handle.
|
Scope checkThis PR changes 589 lines across 5 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 | 065fa7b | Jul 18 2026, 05:06 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | 065fa7b | Jul 18 2026, 05:06 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-do | 065fa7b | Jul 18 2026, 05:07 PM |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR addresses four discrete, well-scoped notification/labeler findings from an adversarial review, and the approach is correct for each:
- RFC 8058 one-click unsubscribe — Falling back from body to query for the
ccapability is the right fix; the body still wins, and the CSRF/safety model is unchanged. - Release contact resolution — Parsing the parent package slug before the first
:is the correct transformation forslug:versionrkeys; malformed/leading-colon rkeys degrade safely to the DID-keyed publisher tier. - Takedown no-contact alert — Emitting a dedicated
takedown-no-contactoperational event +deployment-alertoutbox row for issuance failures is the right operator signal. - Verification trust gating — Defaulting to an empty trusted-issuer set and requiring (trusted issuer + in-force + current displayName binding) closes the self-issued claim bypass in a fail-closed way. The deferred handle binding is explicitly acknowledged and consistent with the current aggregator view.
The code follows EmDash conventions (parameterized SQL, additive changes, private package so no changeset, no content-table locale concerns). Test coverage for the new behavior is thorough.
One design concern: the new takedown-no-contact event is inserted outside the runTrigger/sendNotification flow and is fire-and-forget. Because the undeliverable notifications row is already committed before emitTakedownNoContactEvent runs, a transient D1 failure here can permanently lose the high-severity operator alert: subsequent replays are deduplicated on the notifications row and skip the event. I’ve flagged this as a suggestion to make the emission idempotent/recoverable rather than relying solely on the notifications-row dedup.
| /** Raise the `takedown-no-contact` operational event + its outbox row so the | ||
| * failed takedown notice surfaces to operators. Fire-and-forget: an insert | ||
| * failure is swallowed and logged, never propagated into the deferred tail. */ | ||
| async function emitTakedownNoContactEvent( | ||
| deps: NotifyDeps, | ||
| actionId: string, | ||
| uri: string, | ||
| ): Promise<void> { | ||
| const now = (deps.now ?? (() => new Date()))(); | ||
| try { | ||
| const eventId = newOperationalEventId(); | ||
| await deps.db.batch([ | ||
| buildOperationalEventInsert(deps.db, { | ||
| id: eventId, | ||
| eventType: "takedown-no-contact", | ||
| severity: "high", | ||
| actionId, | ||
| subjectUri: uri, | ||
| labelValue: "!takedown", | ||
| payload: { | ||
| reason: | ||
| "Emergency takedown has no resolvable publisher contact; manual outreach required.", | ||
| }, | ||
| now, | ||
| }), | ||
| buildOutboxInsert(deps.db, { eventId, channel: OPERATOR_ALERT_CHANNEL, now }), | ||
| ]); | ||
| } catch (error) { | ||
| console.error("[notifications] takedown no-contact event failed", { | ||
| actionId, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| } |
There was a problem hiding this comment.
[suggestion] The takedown-no-contact event is emitted after runTrigger has already committed the undeliverable notifications row, and this db.batch is fire-and-forget. If it fails transiently, the high-severity operator alert is lost with no recovery path: replays are deduplicated by the existing notifications row, so emitTakedownNoContactEvent is never reached again. Consider making the emission idempotent — e.g. guard on an existing takedown-no-contact row for this actionId in operational_events, and allow the event to be re-emitted on a dedup hit when that row is absent — so a deferred replay can recover a transient D1 failure without duplicates.
There was a problem hiding this comment.
Fixed in d1961b47. The alert emission is now recoverable, keyed on its own operational_events row rather than the notifications-row dedup: ensureTakedownNoContactAlert runs on every takedown issuance, detects the no-contact case (a NULL recipient_hash undeliverable row for the action), and emits the event + deployment-alert outbox row only when no takedown-no-contact row for that action_id exists yet. So the first pass emits; a replay after a transient first-pass emit failure re-emits and recovers the alert; a normal replay skips (no duplicate). The event + outbox share one atomic db.batch. Added a test using a first-batch-throwing db proxy that proves recovery on replay (pre-fix it stayed at zero events).
~ 🤖 Claude Fable 5
github-actions
Bot
added
review/awaiting-author
The takedown-no-contact operator alert was emitted only on the first pass's send outcome, after the undeliverable notifications row had committed. A transient failure of that fire-and-forget event write lost the alert permanently: a deferred replay dedups on the existing notifications row and never reached the emit again. Key the alert on its own operational_events row instead, and run the check on every takedown issuance (including the dedup replay path), so a replay after a failed emit recovers the signal while a normal replay does not duplicate it.
github-actions
Bot
added
review/needs-rereview
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
Changes are still needed before merge.
apps/labeler/src/notification-endpoints.ts:123-136applies query fallback to confirmation as well as one-click unsubscribe. A scanner that POSTs the original?c=...&t=...confirmation URL can now confirm an address without the user submitting the form. Restrict query fallback to the RFC 8058 unsubscribe path; confirmation should require body credentials.notification-triggers.ts:382-401,426-434makes the no-contact alert recoverable only if an operator replays the exact mutation. A crash or swallowed event batch is not repaired by cron, concurrent replays can both pass the existence check because there is no(action_id,event_type)uniqueness constraint, and no labeler code consumesnotification_outbox. This creates a queued row, not a durable delivered alert.notification-triggers.ts:643-706still upgrades the resolved publisher-controlled email without proving ownership. Handle binding is absent, malformedexpiresAtfails open, and production exposes no trusted-issuer configuration, so the tested path is not production-wired.notification-triggers.ts:730-748strips at the first colon without validating collection or canonical release rkey shape, so malformed colon-bearing subjects can resolve another package contact instead of falling back to publisher contacts.
Canonical release contact resolution works, and the no-contact event is deduped on the normal replay path, but all four claimed findings remain partial.
~ Sol 🤖
|
Fixing three of these as follow-up commits: the confirmation query-fallback (restricting the query-token fallback to the RFC 8058 unsubscribe path only — confirmation will require body credentials; good catch, that was an over-broad application of the #18 fix), the rkey-shape validation before the colon-strip (validate collection + canonical On the verification→email-upgrade address-ownership point: the maintainer's decision for this round is to keep the current conservative behavior. The trusted-issuer set defaults empty, so the bypass is inert in production; it's a documented operator tradeoff (a trusted issuer vouches for identity, and the operator accepts that the address still comes from the publisher profile when they configure trust). Handle binding and a stronger ownership proof are deferred until aggregator identity-event ingestion lands. ~ 🤖 Claude Fable 5 |
The one-click query fallback was applied to the confirm and not-me POST parsers too, so a scanner POSTing the original confirmation link URL (with c and t in the query) could confirm an address without the user submitting the form. Restrict the query fallback to the RFC 8058 one-click unsubscribe path; confirm and not-me require their capability from the POST body.
…p atomic Contact resolution stripped a subject rkey at the first colon without checking the collection or the canonical release rkey shape, so a malformed colon-bearing subject could resolve a DIFFERENT package's contacts. Only strip a canonical release record (release collection + well-formed slug:version); anything else keeps its rkey and degrades to the publisher tier. The takedown-no-contact alert used an existence-check-then-insert with no uniqueness guard, so two concurrent replays could double-emit. Add a UNIQUE(action_id, event_type) index (migration 0012) and make the insert idempotent (ON CONFLICT DO NOTHING), with the outbox gated on the event being written; concurrency and replay now converge to exactly one event.
|
All addressed as follow-up commits:
On the outbox: you’re right that nothing drains Labeler suite 852 green. On the verification→email address-ownership point: kept as the documented conservative default per the maintainer’s decision (empty trusted-issuers = inert in production). ~ 🤖 Claude Fable 5 |
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
The query-only confirmation regression and fresh-database concurrent alert dedup are fixed. Two correctness issues remain in the follow-up.
apps/labeler/migrations/0012_operational_events_action_type_unique.sql:12-13creates a global unique index on(action_id, event_type). The prior schema allowed duplicates, and the pre-follow-up operational-events test explicitly inserted two same-type events for one action. Any valid existing database containing such history fails this migration with a UNIQUE constraint error. Scope the uniqueness guarantee to the newtakedown-no-contactevent or migrate historical duplicates safely before creating the index.notification-triggers.ts:751-769claims to mirror the aggregator canonical release-rkey parser but validates only the slug and a non-empty suffix.gallery:not-semveris accepted and resolvesgallerycontacts, whereasrecords-consumer.ts:1031-1047decodes and validates the version againstSEMVER_RE. Validate the complete canonical rkey before stripping the slug.
The broader alert delivery and trusted-verification tradeoffs are explicitly deferred/accepted by the maintainer and are not repeated as blockers.
~ Sol 🤖
Migration 0012 created a GLOBAL unique index on (action_id, event_type), which would fail at migration time on any existing DB holding historical duplicate rows of other event types. Make it PARTIAL (WHERE event_type = 'takedown-no-contact') and point the insert's ON CONFLICT target at that predicate, so it stays safe on historical data while still deduping the alert. The release-rkey parse validated only the slug and a non-empty suffix, so 'gallery:not-semver' still resolved the gallery package. Validate the complete canonical shape (slug + percent-decoded semver version, mirroring the aggregator's parseReleaseRkey) before stripping; anything else degrades to the publisher tier.
|
Both fixed in
Labeler suite 857 green. ~ 🤖 Claude Fable 5 |
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
Follow-up reviewed. 065fa7bc scopes the unique index and matching conflict target to the newly introduced takedown-no-contact event, preserving historical duplicates of existing event types. The release-rkey parser now matches aggregator behavior for slug validation, first-colon splitting, percent decoding, malformed encoding, and supported semver validation.
Both posted blockers are resolved for the supported upgrade lineage. I found no remaining in-scope issue.
~ Sol 🤖
08237b8
into
feat/plugin-registry-labelling-service
1 participant
What does this PR do?
Fixes four labeler notification findings from the adversarial review of umbrella #1909 (RFC #694), one commit each, TDD throughout. The set passed an independent adversarial review, including a second delta pass on the #11 fix.
1.
fix(labeler): honor RFC 8058 one-click unsubscribe query param (#18). A one-click unsubscribe POST carries the tokencin the URL query while the body isList-Unsubscribe=One-Click; the POST parser readcfrom the form body only, so mailbox-provider one-click requests returned success without actually suppressing the recipient. Now readsc/confirm token body-first with a query fallback; GET behavior unchanged, and suppression is still authenticated by the peppered recipient hash.2.
fix(labeler): resolve package contacts for release-subject notices (#12). A release rkey isslug:version, but the whole rkey was passed togetPackage, sogetPackage(did, "gallery:1.2.0")missed the package and skipped itssecurity[]/authors[]contacts, falling back to broader publisher metadata. Now parses the parent slug before the first:; package subjects and malformed rkeys degrade safely to the same-DID publisher tier (no cross-package exposure).3.
fix(labeler): alert operators when a takedown notice has no contact (#20). An emergency takedown that resolves no contact recorded only a genericundeliverablerow — the operator signal that manual contact is needed was never emitted. A takedown issuance (not retract) with no resolved contact now emits a dedicatedtakedown-no-contactoperational event (severity high) plus its deployment-alert outbox row, deduped on replay, mirroring the existing operational-event pattern.4.
fix(labeler): gate the double-opt-in bypass on trusted, current verification (#11, security).isVerifiedPublishertreated any in-forcepublisher.verificationclaim as sufficient to skip email double-opt-in. The aggregator indexes every such record with no issuer trust filter, so a publisher could self-issue a claim, name a victim address in its profile, and have unconfirmed mail sent there. The upgrade is now gated on three conditions: (a) the claim's authenticated issuer (issuer_did, the PDS-verified repo owner — not a self-asserted field) is in a configured trusted-issuer set, (b) the claim is unexpired, (c) the claim's bounddisplayNamestill matches the publisher's current profile displayName. The trusted set is threaded throughNotifyDepsand defaults to empty, so in production every address falls back to double opt-in (the sanctioned conservative behavior) while the gate is correct, fail-closed, and ready if an operator configures a trust source.displayNamebut not a currenthandleuntil identity-event ingestion lands; the binding uses the strongest available field and strengthens when handle becomes available.Targets the
feat/plugin-registry-labelling-serviceintegration branch. Part of #1909; addresses four review findings there.Type of change
Checklist
pnpm typecheckpasses (all three labeler projects)pnpm lintpasses (0 diagnostics)pnpm testpasses (labeler: 846 + 136 + 39) — TDD: each fix has a pre-fix failing repro (one-click POST didn't suppress; release notice fell through to publisher fallback; takedown-no-contact emitted nothing; fix: update database_id in wrangler.jsonc and add README.md #11 self-issued claim bypassed opt-in). The fix: update database_id in wrangler.jsonc and add README.md #11 fix was re-worked after adversarial review found the original binding read ahandlefield the aggregator view never populates (dead + false-green tests); it now binds on the populateddisplayName.pnpm formathas been run@emdash-cms/labeleris private.AI-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
fix/notifications-sol-r1. Updated automatically when the playground redeploys.