feat: age-restrict effect for media moderation - #8
Conversation
|
slowing our roll on this one. preceding work needs to land and operating state on staging needs to be confirmed first:
once the pipeline is running end-to-end on staging and we can validate verdicts transparently, this is the next piece to wire up. |
90de023 to
5f2c92a
Compare
dcadenas
left a comment
dcadenas
left a comment
There was a problem hiding this comment.
The new age-restrict path can declare a restrict verdict without ever restricting the media, and in one case it pages a human for an enforcement action that never happened.
The two rules guard on a content-hash check that passes even when the hash is missing, and they no longer require the e-tag their effects depend on, so the verdict survives while the enforcement silently drops.
The sink also sends the media hash to relay-manager with no format check, unlike the sibling path that validates the same field one module over.
Both must be addressed before this can merge, with the specifics in the inline comments.
| not LabelRejected, | ||
| LabelTargetEvent != None, | ||
| LabelTargetEvent != '', | ||
| LabelContentHash != '', |
There was a problem hiding this comment.
The restrict verdict can be declared even when nothing actually gets restricted. This guard only checks LabelContentHash != '', and that check does not do what it looks like.
LabelContentHash is optional, so a missing $.label_content_hash becomes a failed value that resolves to None. None != '' is True, so the guard passes when the hash is absent. Only a literal empty string makes it false.
The guard also no longer requires the e-tag, but every effect in the then block needs it. AgeRestrictNostrEvent and both LabelAdd calls drop when $.label_target_event is absent, while DeclareVerdict('restrict') has no failing dependency and always survives. Two outcomes follow, both breaking the rule's intent that a confirmed restrict means the media was age-restricted:
- e-tag absent, hash present: the age-restrict POST and both labels drop, only the restrict verdict survives.
restrictis a priority-high ticket, so a human is paged for an enforcement action that never happened. - hash absent, e-tag present: the guard still passes, the media POST drops, but the labels and restrict verdict apply. The event is marked
age_restrictedwhile the media is never sent to relay-manager.
Require both fields with engine-honored checks. A bare != '' does not exclude an absent value, so pair it with != None:
| LabelContentHash != '', | |
| LabelTargetEvent != None, | |
| LabelTargetEvent != '', | |
| LabelContentHash != None, | |
| LabelContentHash != '', |
Then route partial data deliberately instead of letting it reach a restrict, mirroring the CSAM rules below: a hash with no e-tag should go to flag_for_review, and neither field present should emit no restrict at all.
osprey/divine/rules/rules/content/label_routing.sml
Lines 120 to 153 in 88c9aae
Do not use required=False on LabelTargetEventEntity as the fix. For Entity[str] that raises rather than returning None, so the labels still drop and only the error count changes.
Apply the same guard and fallback to ConfirmedViolence at line 73.
There was a problem hiding this comment.
fixed in c7a7952. adding the age-restrict effect had dropped the LabelTargetEvent guards and left only LabelContentHash != '', which passes when the hash is absent (None != ''). both ConfirmedNudity and ConfirmedViolence now require LabelTargetEvent and LabelContentHash with != None and != ''. and i mirrored the CSAM hash-only path: a hash with no event target routes to flag_for_review (new ConfirmedAgeRestrictHashOnly{Null,Empty}Target rules), neither field present fires nothing. didn't use required=False on the entity, per your note.
|
|
||
| def _age_restrict_media(self, effect: AgeRestrictEffect) -> None: | ||
| payload: Dict[str, Any] = { | ||
| 'sha256': effect.sha256, |
There was a problem hiding this comment.
Validate effect.sha256 before POSTing it. The sibling moderation path already validates the same field, and this new path skips that.
_age_restrict_media sends effect.sha256 straight to /api/moderate-media with no format check. CheckModerationResult rejects anything that is not 64-character hex first, using _HEX64_RE = re.compile(r'^[0-9a-f]{64}$', re.IGNORECASE):
osprey/divine/plugins/src/udfs/check_moderation_result.py
Lines 52 to 54 in 88c9aae
The hash comes from the kind 1985 label, set by the bridge from the x tag or metadata sha256 with no validation. A present-but-malformed value (wrong length, non-hex, uppercase) reaches the POST unchanged. The missing case already drops before the sink, so this is specifically a malformed hash on a trusted-signer label. It is rare, but the repo already treats this field as untrusted one module over, and sending a bad hash means a wasted enforcement call plus a bounded retry burst.
Add the same check at the top of _age_restrict_media and skip with a log when it does not match:
def _age_restrict_media(self, effect: AgeRestrictEffect) -> None:
if not _HEX64_RE.match(effect.sha256):
logger.warning(f'Skipping age-restrict, malformed sha256: {effect.sha256[:20]}...')
return
...Reuse _HEX64_RE by importing it or moving it to a shared module. Do not redefine the regex here.
There was a problem hiding this comment.
fixed in c7a7952. _age_restrict_media now validates effect.sha256 against the hex64 pattern before POSTing and skips with a log on a malformed value, same as CheckModerationResult. pulled the regex out to a shared media_hash.HEX64_RE so the sink and that udf use one definition instead of redefining it.
The age-restrict PR incorrectly removed LabelTargetEvent != None/'' guards from ConfirmedCSAM, ConfirmedAIGenerated, and RejectedLabel. Those rules call BanNostrEvent or LabelAdd with LabelTargetEvent as the entity -- without the guard, a label event with no target would attempt enforcement on a null/empty ID. The requirement was to gate nudity/violence on LabelContentHash (for the moderate-media endpoint), not to remove target event guards from unrelated rules. Also fixes _ban_pubkey type annotation (params is List, not str).
88c9aae to
532d6df
Compare
…idate sha256 label_routing.sml: ConfirmedNudity and ConfirmedViolence dropped the LabelTargetEvent guards when the age-restrict effect was added, leaving only `LabelContentHash != ''`. Since LabelContentHash is optional, a missing value resolves to None and `None != ''` is True, so a `restrict` verdict could be declared while the age-restrict POST and labels silently dropped (and a hash-without-event would page a human for an enforcement action that never happened). Require both LabelTargetEvent and LabelContentHash with `!= None` plus `!= ''`, and route hash-only-no-target labels to flag_for_review, mirroring the CSAM hash-only rules. relay_manager_sink.py: validate effect.sha256 against the hex64 pattern before POSTing to moderate-media, matching CheckModerationResult; skip with a log on a malformed (present but non-hex/wrong-length) hash. Extracted the shared regex to media_hash.HEX64_RE so the sink and CheckModerationResult use one definition.
532d6df to
c7a7952
Compare
dcadenas
left a comment
dcadenas
left a comment
There was a problem hiding this comment.
🔞 clean age-restrict wiring, null vs empty target handled right.
|
Holding this — not ready to merge yet. It was approved and queued before RoostOrg shipped Coop 1.0 (June 9) and before we pivoted to realigning staging onto Roost's official 1.0 images (tracked in support-trust-safety#153). This is part of the enforcement last-mile (it provides the Age-Restrict route that maps to the COOP Age-Restrict action). We want it to land as a coordinated step after the 1.0 cutover — fresh-DB rebuild on official images, with the COOP CUSTOM_ACTIONs re-provisioned — so the full Osprey → COOP → adapter → relay loop is validated against 1.0, rather than landing ahead of the surface it enforces against. Converting to draft to hold; will re-request review when the cutover lands. |
|
@mbradley Is this PR still relevant or should it be closed? Thanks! |
|
@mbradley Following up on the status of this PR. Should we keep it open and finish it up or close it out? Thanks! |
The label-routing rules gate enforcement on who signed the kind 1985 label rather than on its source metadata, which is attacker-controlled. That gate is correct, but the pubkey was hardcoded to the production identity, and no label signed by that key has ever reached the staging relay. So the rules could never fire outside production. The PR's unchecked staging validation was not an oversight, it was unachievable: any attempt would have silently done nothing and looked like a config problem. The trusted set now comes from DIVINE_TRUSTED_MODERATION_PUBKEYS and defaults to the production identity, so production behaviour and deployment are unchanged. An override replaces the default rather than extending it, so staging does not implicitly keep trusting production. Malformed entries are dropped, and if that leaves nothing the set is empty and no label is trusted: for an enforcement gate, failing closed beats silently falling back to a key the operator did not intend. Accepting a list means the moderation identity can be rotated by trusting both keys through the changeover instead of a hard cutover. Parsing lives in a helper with no Osprey imports so it is unit testable without the engine, and reuses media_hash.HEX64_RE rather than adding a second copy of the same pattern. All three guards are mutation-checked.
|
Closing in favour of #25, which is this work rebased onto current main and carried further. This branch had gone 91 days and was conflicting, and the approval on it no longer described what would land: 28 commits have arrived since, including a fix for a silent drop that an independent review found in the newer work. Rewriting this branch in place would have kept an approval attached to code that no longer exists, so a fresh PR seemed more honest than a force-push. The four commits here are the ancestors of what is in #25, and the conflict this branch carried in |
3 participants
Summary
AgeRestrictNostrEventUDF andAgeRestrictEffecteffect type for media moderationRelayManagerSinkto call the media moderation endpoint with theAGE_RESTRICTEDaction and a validated sha256 content hashMotivation
The moderation pipeline needs a distinct age-restriction outcome for media content. This keeps age restriction separate from ban behavior and ensures enforcement only fires when the label contains the fields needed by the media moderation endpoint.
Closes #14
Test plan
python-qualityui-qualityrust-qualitydivine-integration-testsintegration-testsValidation
This box sat unchecked because it was unachievable on staging, not merely undone. The rules gate on the label's signer matching the trusted moderation identity, which was hardcoded to the production pubkey. No label signed by that key has ever reached the staging relay, so the rules could never fire there and any attempt would have silently done nothing.
Making the signer configurable (
DIVINE_TRUSTED_MODERATION_PUBKEYS, added on this branch) is what unblocks it, by letting a local run trust a throwaway key. Recipe preserved atsupport-trust-safety/docs/moderation/local-validation-harness.md.Run against this branch composed with #17, #18 and #20 (132 tests pass together), injecting labels straight into Kafka:
/api/moderate-mediacall,AGE_RESTRICTED, correct hashLabelContentHashguard added in review, workingThe middle row is the one this PR's review added and nothing else exercised.
Caveat: the stub is our side of the contract. This proves the effect fires correctly and calls relay-manager with the right shape, not that relay-manager, moderation-service and Blossom then do the right thing with it.
Two notes for merge time. This branch conflicts with #20 in
divine_register_plugins.py(both append to the UDF registry) — mechanical, but whoever merges second hits it. And this adds a sixth writer to the globalhuman_reviewedlabel, which is the defect where one category's dismissal silences four others; not introduced here, but worth not compounding silently.