fix(coop): resolve the reported event's author instead of trusting the reporter's claim - #20
Conversation
A kind-1984 report carries a p-tag naming the account being reported, but
the reporter writes that tag and nothing checks it against the reported
event. The bridge copies it to reported_pubkey, Osprey exposes it as
ReportedPubkey, and COOPSink passes it on as COOP's creator, which drives
Unban-User and Unsuspend-User. Reversals were therefore targeting
unverified, reporter-controlled input.
The honest failure is more likely than the adversarial one: no rule
requires a p-tag, so an e-tag-only report fell through to the wrapper's
own signer and named the reporter as the offender.
ResolveEventAuthor resolves the signer from the reported event itself via
funnelcake's GET /api/event/{id}. An attacker picks which event to report
but cannot change who signed it. That endpoint serves from ClickHouse, so
a missing event is a definitive 404 rather than the "no events, but we may
have been cut off" ambiguity of a relay subscription that times out before
EOSE. The response is still verified: the returned event's id must equal
the id requested, or we would only have traded a reporter-controlled
pubkey for a relay-controlled one.
Everything fails closed. A missing e-tag, an unfound event, an unreachable
API or a mismatched response all yield '', COOP receives no creator, and
the enforcement adapter refuses loudly rather than guessing. Falling back
to the claimed value is what caused this, so there is no fallback. The
decline is logged with the full event id, since otherwise the only signal
is an enforcement refused much later. Failures are not cached, so a
transient error retries instead of pinning an empty answer for the TTL.
Labels had the same defect from the other direction: on a kind-1985 event
the wrapper's signer is our own moderation identity, so enforcing on it
would target us. Both wrapper kinds now resolve from their target, and the
key order mirrors _resolve_content_id so userId always describes the same
event as contentId. content.pubkey now describes event_id for the same
reason; the reporter's claim stays in reported_pubkey, labelled as a claim.
ReportedPubkey is left in place and marked as the claimed value. Migrating
the rules that enforce on it is separate work and touches label entity
types.
No new dependency and no new endpoint: funnelcake already exposes this
route, unauthenticated, verified against the live API.
Findings from an independent adversarial review, all reproduced first.
author_for_features inferred "is this a wrapper?" from which optional
derived features happened to be populated. That is unsound, because a
wrapper can legitimately carry none of its target features:
- A hash-only CSAM label has LabelTargetEvent = None by construction.
ConfirmedCSAMHashOnlyNullTarget is a live rule emitting an actionable
verdict, so this reached COOP with creator set to our own moderation
identity, 8fd5eb6d8f362163bc00a5ab6b4a3167dbf32d00ec4efdbcf43b3c9514433b7e.
An Unban-User on such an item targets us, and it passes the adapter's
hex guard cleanly so nothing downstream catches it.
- NIP-56 permits a report with only a p-tag or with neither tag, so a
report could carry no target features and yield the reporter.
Now keyed on Kind, with the marker set only as a backstop for an absent or
unrecognised Kind. This matters beyond this change: the adapter defers
moving forward Ban/Suspend onto creator pending "verification of how osprey
derives creator" (s-t-s#190), and once that lands the same defect would aim
the purging banpubkey at our own identity.
One of the old tests pinned the broken behaviour as correct, asserting on a
feature dict indistinguishable from a tag-less kind-1984. Corrected, and
the rest now carry Kind as real features always do.
Failures are cached briefly rather than not at all. Not caching them was an
amplification vector: reports e-tagging random 64-char hex ids are all
guaranteed misses, each an outbound request at a rate the reporter chooses.
A 30s negative TTL caps repeats while still letting a transient failure
recover, instead of pinning an empty answer for the full 300s.
DIVINE_RELAY_API_URL no longer defaults to production. Defaulting there
meant local and staging workers querying prod, and on staging the events
are absent so the feature would look wired while returning '' for
everything. Unset now fails closed with a single warning, matching
RelayManagerSink and COOPSink, and the var is declared in docker-compose.
The local e2e used a placeholder e-tag, so it was rejected as malformed
before any lookup: the suite passed while exercising none of the resolution
path. It now uses a real 64-char hex id and reaches the resolution and
fail-closed paths for real.
Mutation testing drove three of these: two guards initially survived because
the marker backstop masked them, and the bool special-case in Kind coercion
turned out to be unfalsifiable dead code, since isinstance(True, int) is
already True and 1 is not a wrapper kind. Removed rather than kept.
Verified in the local stack: rules compile, worker steady, e2e passes, and
the report row carries the claimed pubkey with an empty authoritative one.
Second independent pass. All reproduced before acting on them.
The marker backstop covered report bodies but no label ones, so a label
carrying only L/l tags with an unparseable Kind still fell through to
Pubkey and yielded our own moderation identity. Added LabelNamespace,
LabelValue and ReportReason. Deliberately NOT LabelSignerPubkey: it reads
$.pubkey and is therefore populated on every event, so treating it as a
marker would make every ordinary note look wrapped and stop resolving
authors for real content. The reviewer's mutation deleting the label
markers previously survived the suite; it now fails.
Kind coercion used str.isdigit(), which is True for characters int()
rejects. _kind('2') raised ValueError, and because author_for_features is
called above COOPSink's own try block that escaped push() entirely, to be
retried deterministically and dropped. Now isascii() and isdecimal().
Eviction now discards negative entries ahead of positive ones. The
negative TTL added in the previous commit shared one bounded cache, so a
storm of distinct-id misses evicted real answers: measured 0 of 50
legitimate entries surviving 1200 misses. That made honest traffic
re-fetch and raised load on the relay API rather than lowering it, which
is the opposite of the intent.
Stated plainly in the module docstring that negative caching does NOT stop
an attacker driving outbound requests. Distinct random ids are all
guaranteed misses and still cost one request each; caching only collapses
repeats of the same id, which is not what an attacker sends. Bounding that
needs a rate cap or circuit breaker and is not implemented here. Better a
known limitation than a future reader assuming the TTL solved it.
COOPSink's content-id resolution now delegates to content_id_for_features,
keyed on Kind like the author. Previously one keyed on Kind and the other
on feature presence, so a crafted action could produce a contentId and a
userId describing two different events. The previous commit deleted the
docstring sentence asserting they stayed in step instead of preserving the
invariant; it now holds by construction.
The missing-URL warning moved to __init__, so it is guaranteed at startup
like COOPSink's rather than firing on the first report. A worker that
processed none never warned at all, and unlike a disabled sink the
pipeline keeps running and looks healthy while producing items with no
creator. Also captured to Sentry so the silently-off state is alertable.
Signature verification is deliberately not implemented, and the docstrings
no longer imply otherwise. The id check binds the response to the request,
so a relay cannot substitute a different stored event; it does not stop a
compromised relay fabricating one. Closing that needs byte-exact canonical
NIP-01 serialization, where a subtle error fails closed into no
enforcement at all, plus a Schnorr dependency. It belongs with a decision
about whether the relay sits inside our trust boundary.
Verified in the local stack: warning now appears at startup before the
sinks, rules compile, e2e passes, 89 tests, all mutations killed including
the one that survived the reviewer's own sweep.
|
@dcadenas bump! |
dcadenas
left a comment
dcadenas
left a comment
There was a problem hiding this comment.
Approving. Nothing here blocks. Everything below is either a deployment precondition for whoever ships it, or a follow-up for you to take or decline.
I re-derived the central claims from source rather than taking the description's word for them, and they hold. Two of them turned out to be understated — details below.
What I ran
At head 9c11718, in a clean worktree off main (6186d04):
| Check | Result |
|---|---|
pytest divine/plugins/tests -q |
89 passed |
ruff check divine/plugins |
clean |
ruff format --check divine/plugins |
14 files already formatted |
osprey-cli push-rules divine/rules --dry-run |
OK! Rules validated., 6 warnings |
same, against main's divine/rules |
6 warnings — identical, so no new warnings |
mypy on the three changed Python files |
2 import-untyped errors, see N4 |
Two setup notes for anyone reproducing the rules compile, since neither is obvious: gh pr checkout 20 resolves against the upstream parent and fails, so fetch the branch directly; and you must uv pip uninstall example-plugins after installing divine/plugins, because both ship a top-level udfs module and collide — the same collision divine/Dockerfile.worker already works around.
Two claims that are stronger than you wrote them
Resolution does not just short-circuit on non-report events — the UDF never runs at all.
The SML comment says a missing event id "returns '' without any network call". What actually happens is better than that. ReportedEvent is required=False and typed str, so on a non-report the path is absent, rvalue_type_checker.check(None) is false for str, and json_utils.get_from_data raises ExpectedUdfException (json_utils.py:57-67). That is the engine's "expected failure that prevents dependents from running", so ResolveEventAuthor is never constructed into the call at all. Worth knowing because it also explains the ClickHouse behaviour: the feature resolves to None on non-reports, and the sink drops None (clickhouse_output_sink.py:73-74), so those rows keep the column default. On an actual report the value is a real string — which is exactly why your ALTER TABLE ordering note is correct and not merely cautious.
The contentId/userId "by construction" claim survives a specific attack I tried on it.
Keying the author off ReportedEvent while keying contentId off ReportedEventId looks like the drift the docstring says it prevents. It isn't: they are two SML names over the same JSON path $.reported_event_id (kind1984_report.sml:3-15). Might be worth a one-line comment saying so, since the next reader will trip on it too.
I also went looking for the failure that would quietly break this everywhere — EntityJson returning a truthy EntityT wrapper for a missing path, which would make every ordinary note look wrapped and collapse author resolution to '' across the board. It does not happen: get_outputs() unwraps PostExecutionConvertible to .id before features reach a sink (execution_context.py:154-159), and the wrapper-only fields are required=True, so they resolve to None on non-wrappers. Your test_backstop_does_not_treat_ordinary_content_as_wrapped covers it.
Deployment preconditions
D1 — the ALTER TABLE really must precede the new image. You state this; I confirmed the mechanism above. Flagging it again only because it is the one ordering that fails loudly and mid-batch.
D2 — without DIVINE_RELAY_API_URL set, merging this does not unblock the adapter change. You say iac needs it; the consequence is worth spelling out for whoever schedules that. Unset, every report and label submits with userId=''. COOP's submission path treats the empty string as no creator deliberately — the truthiness check in submitContent.ts is commented as covering "undefined, null, and the empty string, which users sometimes send us" — and userId is not in the route's required list. So items still land in MRT and nothing breaks; reversals simply refuse for everything. That is the intended fail-closed state, but it means the dependent PR stays functionally blocked until the env var lands.
I traced the rest of that chain and it holds: userId here becomes the submission's creator, ActionPublisher.getUserFromActionTarget reads creator off the full submission, and the adapter PR reads creator.id with a 64-hex check and refuses on absent or malformed. No forward-path regression, since that PR deliberately keeps forward Ban/Suspend on the relay read.
Notes — none blocking
N1. None of the 587 lines of new tests are run by CI. This is the first test file under divine/plugins/tests/. .pre-commit-config.yaml has no pytest hook and its mypy hook excludes divine/.*; divine-integration-tests.yml only checks that plugin files exist. So the suite carrying the whole security argument runs when someone remembers to run it. Adding a job is a workflow change and out of scope for this PR — but it is the note I would most want acted on afterwards.
N2. The UDF wrapper itself is untested. Keeping reported_author.py import-free so it tests without the engine clearly paid off. The cost is that resolve_event_author.py has real logic nothing touches: URL construction, the 404-to-None mapping, raise_for_status, and the log-injection guard in execute(). Small surface, but the guard is the kind of thing that regresses without anyone noticing.
N3. Moderator-visible change that isn't in the description. A report with a p tag and no e tag previously produced an MRT item whose creator was the claimed pubkey; now it produces one with no creator, so user-level actions from that item are no-ops. The claimed value is still there for the human as content.reported_pubkey. I think this is the right trade — the old behaviour acted on an unverified claim — but moderators will notice it before they read the PR.
N4. reported_author.py is untyped from mypy's perspective. It is a top-level module under src/ rather than inside a package, so neither src/udfs/py.typed nor src/services/py.typed covers it, and importers get import-untyped. CI does not catch this because of the divine/.* exclusion. Cosmetic, and the fix is a layout judgment call that is yours.
N5. Two optional hardening bits. _fetch does not pass allow_redirects=False, and _TIMEOUT = 1.5 as a scalar applies separately to connect and read, so worst case is roughly double. Neither is a practical concern against a first-party API under gevent, and check_moderation_result.py — the existing precedent in this repo — has the same shape. Mentioning them only so the choice is deliberate.
N6. One description correction. The test plan says the recipe is preserved at support-trust-safety/docs/moderation/local-validation-harness.md. That file is on a branch, not that repo's default branch, so anyone following the pointer today will not find it.
What this PR does not close, and does not claim to
divine/rules/rules/reports/auto_hide.sml:41 still calls BanNostrEvent(..., pubkey=ReportedPubkey, ...) — the destructive forward path, still on the unverified claim. You say this is deliberate and that #18 handles the worst of it; #18 is open, not merged, so at the moment this lands the gap is still open on that path.
Two things bound it, and both are worth stating explicitly rather than leaving implicit for a future reader of this thread. That rule additionally requires HasLabel(entity=Pubkey, label='trusted_reporter'). And ReportedPubkey is a required=True EntityJson, so a report with no p tag fails the dependency rather than misfiring on the reporter — the honest failure mode you describe as the likelier one is already closed on that path, even before this change.
What I could not verify
I did not stand up the local stack. Your composed run against a stub is the only evidence for the end-to-end path, and as you note it proves our side of the contract only. push-rules --dry-run validates that the rules compile; it does not execute them, so nothing here says the UDF behaves correctly against a live API.
Approving on that basis. Merge when the two deployment preconditions are scheduled.
2 participants
What
Resolve the author of a reported event from the event itself, rather than trusting the pubkey the reporter wrote into the report.
Why
A kind-1984 report names the account being reported in a
ptag, but the reporter writes that tag and nothing checks it against the reported event. That value flows: bridgereported_pubkey→ OspreyReportedPubkey→ COOPSinkuserId→ COOP'screator, which is what the enforcement adapter targets for Unban-User and Unsuspend-User.The likelier failure is the honest one rather than the adversarial one. No rule requires a
ptag, so a report carrying only anetag fell through to the wrapper's own signer, which named the reporter as the offender.An attacker chooses which event to report, but cannot change who signed it. So the event is the authoritative source and the tag is a claim.
How
ResolveEventAuthorreads funnelcake'sGET /api/event/{id}, which serves from ClickHouse and so gives a definitive found or not-found, rather than the "no events, but we may have been cut off" ambiguity of a relay subscription that times out before EOSE.The response is bound to the request: the returned event's id must equal the id asked for, or the relay could answer with a different stored event and thereby choose the pubkey we enforce against. It does not verify the signature, so a compromised relay could still fabricate a body carrying the requested id; that limit is stated in the module docstring rather than implied away.
Everything fails closed. A missing
etag, an unfound event, an unreachable API, or a mismatched response all yield'', COOP receives no creator, and the adapter refuses loudly rather than acting on a guess. Falling back to the claimed value is what caused this, so there is no fallback.Two things found while building it
Labels had the same defect from the other direction. On a kind-1985 event the wrapper's signer is our own moderation identity, so anything enforcing on it would target us. Both wrapper kinds now resolve from their target.
content.pubkeydescribed a different event thancontent.event_id. Pre-existing, but this change would have widened the gap. It now describesevent_id; the reporter's unverified claim stays inreported_pubkey, labelled as a claim.Deliberately not included
The SML rules that still enforce on
ReportedPubkeyare not migrated. That is a separate change touching label entity types, and the most destructive of those consumers is already addressed in #18.ReportedPubkeystays in place, now marked as the claimed value.Configuration
DIVINE_RELAY_API_URLis required, with no default. Defaulting to production would mean a local or staging worker querying prod, and on staging the events it needs are absent, so the feature would look wired while returning nothing. Unset fails closed, warns once at startup, and reports to Sentry, matching howRelayManagerSinkandCOOPSinkbehave when unconfigured. Declared blank indivine/docker-compose.yaml; iac needs it set for staging and production.Correction: the websocket rationale expires
An earlier version of this description argued for the REST approach partly on the grounds that no websocket client exists in the osprey worker environment. That is true of
mainand stops being true the moment #17 lands, since it addswebsocket-client==1.9.0to the plugins.The choice still stands on its stronger merit: funnelcake's
GET /api/event/{id}serves from ClickHouse, so a missing event is a definitive 404 rather than the "no events, but we may have been cut off" ambiguity of a subscription that times out before EOSE. That reason does not expire; the dependency one did.Deployment note
Two ClickHouse columns are added (
ReportedAuthorPubkey,LabelTargetAuthorPubkey). The sink writes every extracted feature, so theALTER TABLEmust run before the new worker image starts, or batches fail in that window. Both are idempotentADD COLUMN IF NOT EXISTS.Test plan
ruff checkandruff formatcleanKind, and removing negative caching all turn the suite redtest-local.shpasses, and a report row carries the claimed pubkey with an empty authoritative onesupport-trust-safety/docs/moderation/local-validation-harness.md.The local run now covers the success path, not just fail-closed. Composed with #8, #17 and #18 (132 tests pass together), pointed at a stub serving
GET /api/event/{id}, it shows the resolved author travelling the whole chain:The case that matters most is the kind-1985 label above: before this change
userIdthere would have been the label's signer, which is our own moderation identity. Note the stub is our side of the contract only, so this proves we make the right call, not that the far end acts correctly on it.Two things composing the branches surfaced, worth knowing at merge time: #8 and this PR both append to the UDF registry in
divine_register_plugins.py, and #17 and this PR both rewrite the same block ofcoop_sink.py(#17 adds media enrichment while still carrying theuserIdlines this PR replaces). Both conflicts are mechanical, but whoever merges second will hit them.Review history
Three self-review rounds plus an independent adversarial pass. Findings acted on include a wrapper-detection hole that returned our own moderation identity for hash-only CSAM labels, an amplification vector in the cache, and log injection from the attacker-controlled event id. The
Kindkeying in particular is a prerequisite for s-t-s#190: once forward Ban/Suspend move ontocreator, the old behaviour would have aimed a destructive ban at our own identity.