notice: a cron run can say something, without ever holding a token - #92
Conversation
![flaredispatch-fractalboxdev[bot]](https://avatars.githubusercontent.com/in/4273665?s=60&v=4){kind=small}
There was a problem hiding this comment.
17353f2 to
843003a
Compare
There was a problem hiding this comment.
AI code review — 🛑 Request changes
Risk tier: full · 2 critical · 0 warnings · 0 suggestions
Reviewers: security 0 · performance
1. 🛑 Critical — Notice retries can silently lose messages after receiver-side claim
📍 apps/dispatcher/specs/slack-origin.md:177-180
The documented receiver behavior claims 'deliveryId' before posting, while the dispatcher treats every subsequent 409 as 'already delivered'. If the receiver fails or times out after claiming but before Slack accepts the post, a retry receives 409 and this side reports success even though no notice was published. This is unsafe for rollout of the scheduled digest path; make claiming recoverable/transactional or distinguish an unposted claim from a delivered duplicate before treating 409 as success.
2. 🛑 Critical — Sanitize notice link labels before allowing them into Slack markup
📍 apps/dispatcher/src/slack-notify.ts:255-256
Validation only checks that 'link.label' is non-empty and at most 80 characters. The receiver renders links as Slack 'url|label' markup, so a model-authored label can contain '', '', '|', or '!channel'/'@U…' and potentially inject markup or mentions. Escape the label at the receiver, or reject/encode these characters here and add tests for hostile labels.
00136af to
469fd5a
Compare
843003a to
a284976
Compare
469fd5a to
b70d4c1
Compare
a284976 to
36d4831
Compare
Review addressed — both CRITICALs fixed, plus four more the review team foundBoth bot CRITICALs are fixed, and a parallel review (correctness / security / tests / architecture) turned up a delivery-id bug that three of the four reviewers found independently. Four commits on top of CRITICAL 2 — hostile link labelsConfirmed against the receiver's own parser: the mirrored
CRITICAL 1 — 409 after a claimFixable here in part, and the part that isn't is now written down instead of assumed. The receiver's store is in another repo, so this PR cannot implement the claim. What it can do is stop resting on an assumption it never stated. The spec now requires the two-phase split: Worth noting for the record: the result shape was already honest — fractalbot#27 owes the implementation. It is unmerged and undeployed, so the requirement lands before anything depends on the weaker behavior. Also fixed — from the review team
Test gaps closed alongside: the notice's own wrangler var was never exercised (a one-token copy-paste passing Deliberately not fixed
Both deferred items sit in |
db60ecc to
68c1613
Compare
There was a problem hiding this comment.
AI code review — 🛑 Request changes
Risk tier: full · 1 critical · 0 warnings · 0 suggestions
Reviewers: security 0 · performance 0 · code-quality
1. 🛑 Critical — Receiver-side label sanitization is still required before enabling notices
📍 apps/dispatcher/specs/slack-origin.md:169-175
The notice contract explicitly permits the receiver to render link labels inside Slack markup while its current length-only validation does not reject '', '', '|', or control characters. Emitter-side validation is not sufficient for a shared signing key: any other valid producer, replayed legacy payload, or future emitter can submit a hostile label and trigger mentions or markup injection. Require the receiver to escape or reject labels before posting, and block production enablement until the receiver contract and tests enforce that behavior.
The emit half of the FlareDispatch → Slack hop only ever pointed one way.
`slack-notify.ts` resolved its target from the `SlackOrigin` on the execution
and replied in the thread a dispatch came from — so a scheduled run, which has
neither, could not reach it at all. `org-spec-audit` rendered a digest, committed
it as a file, and had no way to tell anyone it existed.
The `notice` capability is the other direction. A run says WHAT to publish and
WHAT KIND of thing it is:
notice.publish({ useCase, text, dedupeKey, links })
and nothing else — no channel, no thread, no recipient, no URL, and no way to
express one. `useCase` is a routing key the receiver resolves against a map in
its own deploy config; the emit side is the untrusted half (its `text` is
model-authored), so it may describe what it wants said and never who hears it.
`text` stays data: the receiver escapes it, links ride typed, and this repo
builds no Slack markup and gains no Slack credential.
Best-effort throughout, like the completion-notify email: the service is total,
a failure is a logged line, and a deploy with no ingress degrades to a logged
no-op. The questions PR is the record; the notice is the announcement, and empty
still means silent.
The delivery id is derived, never drawn — `<run>:<dedupeKey>`, with the run's
day string as the key. The receiver claims it before posting, so a retried
Workflow step re-sends identical bytes and earns a 409 (read here as *already
delivered*) instead of posting the digest twice. `Date.now()` and
`Math.random()` are unavailable on a replayed step anyway.
The credential never moves. runtime-cf holds a closure over `emitSlackNotice`,
not a secret or a URL — the same seam `mailbox.signToken` uses, and the reason
ADR-0006's never-store list stays true by construction. The notice ingress gets
its own CONFIG_KV key (`slack-notice.url`): a different endpoint, outside the
origin-gated policy's namespace, with a blast radius an operator must be able to
move without silencing in-thread verdicts.
`slack-notify.notice.test.ts` carries fractalbot's `deriveNotifyKey`,
`verifyNotifySignature` and `parseNotice` verbatim. The two repos deploy
separately and share no package, so that is the only place the agreement is
checkable — and the failure it guards is silent, a 401 forever with nothing to
say why.
Depends on fractalbot#27 (the receiver).
… the span `text` is safe because the receiver escapes `&<>` wholesale before posting. A link label is not: it lands INSIDE the `<url|label>` span that escaping produces, so `>` closes the link early and `<` opens a fresh one. The receiver's own `parseNotice` (mirrored verbatim in the notice test) bounds a label by length alone — which leaves a model-authored label as a way back into markup, and into `<!channel>` / `<@U…>`, for anything that only escaped the body. `validateSlackNotice` now refuses `<`, `>`, `|` and control characters in a label, and refuses a blank one rather than only an empty one. This is the one rule in that function that is deliberately STRICTER than the receiver rather than a mirror of it: it is the half this repo controls, and a label is short display prose that never needs those characters. `&` stays legal — Slack parses markup before it decodes entities, so `<` is literal text and "Q&A" is a label people write. Tests cover the hostile shapes (span-closing, mention-opening, second `|`, bare angle brackets, newline, CR, NUL, DEL), the legitimate ones that must keep working, and the blank-but-non-empty label.
The 409-as-duplicate reading was justified by "the receiver claims the id before
it posts, so the message is already in the room". That justification is wrong in
exactly one case, and it is the case that matters: a receiver that dies after
claiming and before Slack accepts the post leaves a mark that answers 409
forever, so the retry reads as handled and the notice is never published. A
silence with a success beside it is the worst failure this capability has.
Nothing here can implement the receiver's store, so this commit fixes the two
things that are on this side of the line.
The contract now states the obligation instead of assuming it. The delivery id
carries two states — `claimed` (attempted, outcome unknown) and `delivered`
(Slack accepted it) — and **409 is reserved for `delivered`**. A `claimed`
id must stay re-attemptable: the next POST takes over the claim or gets a
retryable 5xx, never a 409. The spec names the residual window this leaves (a
post Slack accepted whose `delivered` write did not land can double-post) and
why an announcement takes that trade: a visible duplicate is cheaper than an
invisible silence.
The prose stops overclaiming. `duplicate` already mapped to
`{ delivered: false, duplicate: true }` — the data was honest and only the words
around it were not. The log line, the capability docs and the code comments now
say what a 409 actually is: the receiver's word that it handled this id, not a
post this dispatcher witnessed. `delivered` is set by a 2xx we received and by
nothing else.
Implementing the split is fractalbot's half (specs/flare-dispatch-notify.md);
that receiver is unmerged and undeployed, so the requirement lands before
anything depends on the weaker behavior.
…air collide
`noticeDeliveryId(run, dedupeKey)` left `useCase` out of the id the receiver
dedups on. `dedupeKey` is documented as "a scheduled run's day string", so one
run publishing two kinds of notice on one tick supplies the same key twice and
mints one id twice: the second post is answered 409, which reads as already
delivered. A dropped message reported as fine, with nothing having gone wrong.
Latent while `org-spec-audit` publishes once; guaranteed the first time any run
publishes twice. The id is now `<run>:<useCase>:<dedupeKey>` — still clock-free,
still stable across a retry, still inside the receiver's charset.
The same silence had a second road in. Charset coercion and the 128-char bound
are both many-to-one, so `2026/08/08` and `2026-08-08` produced one id, as did
any two keys sharing their first 128 characters — a permanent 409 for whichever
arrived second. When either repair actually loses information the id now carries
an FNV-1a tag of the original, so distinct inputs keep distinct ids while the
function stays pure (a replayed Workflow step has neither a clock nor entropy,
and rebuilds the id exactly).
The outcome branch moves from a bare `switch` to
`Match.discriminatorsExhaustive("outcome")`, matching `describeRefusal` in
sandbox-facade.ts: the vocabulary is the receiver's to grow, and a new outcome
should break the build rather than hand a run `undefined` as its NoticeResult.
Tests cover the two-use-cases-one-tick case, both collision roads, determinism
and charset after tagging, and a synchronous throw from the injected closure —
a different path from the rejected promise already covered.
Both callbacks authenticate by an HMAC over the body carried in a header, and `readUrl` accepted whatever string was in CONFIG_KV. `slack-notice.url` (or the verdict's) set to `http://` put the payload and its signature on the wire in clear for anyone on the path to read and replay. Both now resolve to `undefined` unless the value starts with `https://`, which degrades to the same best-effort silence as an unset key — a wrongly-configured endpoint is not worth a leaked signature. `postSigned` also left `redirect` at the default, so a 3xx from the receiver replayed the body and the `X-FlareDispatch-Signature` header to an origin the receiver picked rather than the one an operator configured. It is now `redirect: "manual"`, so a 3xx reads as an ordinary non-2xx and is logged like any other refusal; moving an endpoint is a config change, not a redirect. Also covered, all previously untested: the notice's own wrangler var (`SLACK_NOTICE_URL`) and CONFIG_KV winning over it — without this a one-token copy-paste passing `env.SLACK_NOTIFY_URL` to the notice reader passes the whole suite; the four `validateSlackNotice` branches nothing reached (non-finite `sentAt`, out-of-charset `run` and `executionId`, an over-long label and url); and the mirrored receiver parser asserted on DISAGREEMENT rather than only on one accepted payload — a mirror that regressed to `return true` used to stay green. That last test also pins the one place the two sides intentionally differ: the receiver still accepts a hostile link label, and we do not. Fixes a stale pointer in the module header — the cross-repo parity test is in slack-notify.notice.test.ts, not slack-notify.test.ts.
Both surfaces derived under one HKDF label, so one key signed both. The verdict body names a destination — `SlackVerdictPayload.origin` carries `channel` and `thread_ts` — and the notice body deliberately cannot. Under a shared key that gap does not hold: anything able to sign a notice could sign a verdict naming any channel the bot can see, and the notice's whole "the shape is the security property" argument was worth nothing, because a shape bounds only while nothing else can sign a different shape with the same key. Notices now derive under `flare-dispatch/slack-notice/v1`. The verdict label is untouched, so receivers already deployed keep verifying verdicts. `postSigned` takes the label as a required argument with no default — a default would silently be wrong for whichever surface forgot to pass it, and the failure mode is a 401 nobody reads. The cross-repo mirror test now pins both label strings as literals rather than importing the constants (importing them makes the mirror a tautology that follows a rename and stays green), and asserts the forgery each label denies the other: the notice key cannot verify a channel-naming verdict, and the verdict key cannot verify a notice. Receiver-side change required — fractalbot#27 must derive notices under the new label or every notice 401s silently and indefinitely. Tracked in #113.
1ea2dab to
76bd06f
Compare
…o PR Suppression and the notice landed in separate PRs, so nothing pinned their order. The notice step sits after the `proposed.length === 0` early return, which is the behavior that matters: re-broadcasting a question a human already declined is the louder half of re-proposing it, and the half nobody can close. One assertion, on the existing cooldown test that already proves no PR opens. Also corrects the two step comments the rebase left stale: the questions file is no longer "the message a Slack consumer posts" now that the run announces it directly, and the notice step is 7, not a second 6.
1 participant
Important
The receiver must change before this deploys, or every notice 401s forever.
Notices now derive their signing key under the HKDF label
flare-dispatch/slack-notice/v1(was
flare-dispatch/slack-notify/v1). fractalbot#27'sderiveNotifyKeymust derive under thatexact string. A failed notice is correctly never fatal, so a receiver on the old label goes
silent with nothing turning red and nothing paging. Deploy the receiver first. Tracked in
#113. Verdicts are unaffected — their label is unchanged, so already-deployed receivers keep
verifying them.
Nothing here works until fractalbot#27 is deployed and
slack-notice.urlpoints at it; nothinghere breaks in the meantime either — an unconfigured deploy makes
notice.publisha logged no-op.Problem & Insight
The FlareDispatch → Slack hop existed in one direction only.
slack-notify.tsresolved its targetfrom the
SlackOriginon the execution and replied in the thread a dispatch came from. A crontick has no origin and no thread, so a scheduled run could not reach it at all —
org-spec-auditrendered a digest of open questions, committed it as a file, and had no way to tell anyone.
The obvious fix is the forbidden one. This repo holds no Slack credential on purpose, in two places
(
slack-notify.ts, substrate ADR-0006). So the run does not get a token; it gets a shape thatcannot express a destination:
No channel, no thread, no recipient, no URL.
useCaseis a routing key the receiver resolvesagainst a map in its own deploy config. That is the whole security property: the emit side is the
untrusted half — its
textis model-authored — so it may say what it wants published and never whohears it.
A shape bounds only while nothing else can sign a different shape with the same key. Both
callbacks originally derived under one HKDF label, so one key signed both — and
SlackVerdictPayload.origincarrieschannel+thread_ts. A notice-key holder could thereforesign a verdict naming any channel the bot can see, which is exactly the workspace-wide write the
notice's shape was designed to deny. The two payloads are not equally dangerous and must not share
a key.
Take
The credential never moves.
runtime-cfholds a closure over the dispatcher'semitSlackNotice,not a secret and not a URL — the same construction
mailbox.signTokenuses. ADR-0006's never-storelist stays true by construction rather than by discipline.
One secret, two derived keys. The notice derives under
flare-dispatch/slack-notice/v1, theverdict keeps
flare-dispatch/slack-notify/v1, both off the sameikm.postSignedtakes thelabel as a required argument with no default — a default would silently be wrong for whichever
surface forgot to pass it, and the failure mode is a 401 nobody reads. Two labels off one secret is
domain separation; two secrets would be a second thing to rotate for separation HKDF already gives.
textstays data. Nothing here builds Slack markup; the receiver escapes&<>wholesale, andlinks ride in a typed
links[]field it renders from a validated https URL.org-spec-auditemitsthe same rendering it commits — one wording, two destinations, so nobody has to ask which copy is
real.
The delivery id is derived, never drawn.
<run>:<dedupeKey>, with the run's day string as thekey. The receiver claims it before posting, so a retried Workflow step re-sends identical bytes and
earns a 409 — read here as already delivered, not as a failure.
Date.now()andMath.random()are unavailable on a replayed step anyway, which makes derivation the only construction correct on
every path.
Origin-gated and un-originated stay separate, now in the key as well as the URL. The verdict
callback is untouched and still gated on
payload.sourceat the finalize boundary. The notice isreached only from inside a run and gated on nothing but config — because a cron run has no origin
to gate on. Its ingress gets its own CONFIG_KV key,
slack-notice.url, outside theslack-origin.*policy namespace: pointing it at a staging receiver now hands that receiver a credential that can
publish a use case and cannot name a channel.
Best-effort, end to end. The service is total. No ingress, no key, a 5xx, a timeout, even a
defect from the closure — all resolve to a logged line and
delivered: false. The questions PR isthe durable record; the notice is the announcement. Empty still means silent.
Key actions
packages/core/src/services/notice.ts— theNoticeServiceinterface,NoticeTag andnoticeaccessor namespace;
notice-fake.ts+CFRuntimeTestwiring.packages/runtime-cf/src/notice-cf.ts— the live Layer: delivery-id derivation, degradation,total-by-construction result mapping.
apps/dispatcher/src/slack-notify.ts— generalized, and the label split at the one derivationsite.
runs/org-spec-audit.ts— publishes the digest it already renders, keyed on the day it alreadycomputes.
apps/dispatcher/src/slack-notify.notice.test.ts— carries fractalbot'sderiveNoticeKey,verifyNotifySignatureandparseNoticeverbatim. It pins both label strings as literalsrather than importing the constants: importing them makes the mirror a tautology that follows a
rename and stays green. It also asserts the forgery each label denies the other — the notice key
cannot verify a channel-naming verdict, and the verdict key cannot verify a notice.
apps/dispatcher/specs/slack-origin.md§ The notice signs under its own key (including thedeploy-ordering note), § Its own URL, and a dogfood sequence.
No Slack token, webhook URL or workspace credential is added. No
wrangler.jsoncor CONFIG_KVchanges — turning this on is an operator act.
pnpm lint && pnpm typecheck && pnpm test: 160 files,1978 passing.
Rebased onto
mainpast #89 (squash-merged) and #91.org-spec-auditnow announces thesuppression-filtered rendering, so a tick where the ledger declined everything opens no PR and
says nothing — the two halves stay one wording.