fix: close four label-enforcement bypasses (review round 1)#2112
Conversation
hateful-imagery, explicit-imagery, and graphic-violence are declared as automated-block labels in the labeler policy fixture but were absent from AUTOMATED_BLOCKS, so the evaluator and RELEASE_BLOCK_VALUES ignored them -- a policy-blocked release stayed installable. Add the three, plus content-warning to WARNINGS (the same un-synced image-label batch), export both sets, and add a labeler-side parity test that fails whenever the fixture and package classification drift.
DiscoveryClient treated acceptLabelers: "" as absent (`|| onResponseMeta` and `if (acceptLabelers)`), so a config meaning "accept no labelers" was dropped and the aggregator applied its trusted defaults. Switch to `!== undefined` checks so an empty string still goes on the wire while an omitted option still sends no header.
buildReleaseEnforcementSql excluded a release whenever any live automated block row existed, but the hydrated evaluator suppresses a source's automated blocks when that same source has an exact-CID assessment-passed + assessment-overridden pair (spec §10). A post-override re-assessment that re-issues a live automated block was therefore dropped from latest-selection even though the evaluator treats it as eligible. Split the release-URI block branch so automated blocks carry the same-source exact-CID override exception while the manual security-yanked / !takedown blocks and the package/publisher cascade keep firing unconditionally.
capLabels sliced a view's labels at LABELS_MAX_LENGTH in hydration order, so a subject carrying more than 64 labels could lose its only hard block; a client evaluating the truncated view would then treat the release as installable. Order labels by enforcement priority (hard blocks, then assessment states, then informational) before slicing so blocks survive, and log an error in the pathological case where blocks alone exceed the cap (the kept slice is still all blocks, so the release stays blocked).
🦋 Changeset detectedLatest commit: fb3ff1a The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
review/needs-review
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | fb3ff1a | Jul 18 2026, 03:09 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | fb3ff1a | Jul 18 2026, 03:09 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-do | fb3ff1a | Jul 18 2026, 03:09 PM |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a focused, well-justified bug-fix PR. Each of the four bypasses is real, and the fixes mirror their described evaluator semantics carefully:
- Adding the image-content automated blocks and the
content-warningwarning to the registry-moderation sets closes the vocabulary-drift gap, and the new parity test is the structural safeguard. - The SQL override-pair suppression matches
evaluateReleaseModerationCore(same source, exact CID, automated blocks only, manual blocks never suppressed), with correctness cases covered. - Enforcement-preserving
capLabelssorts by block → state → informational before slicing so a hard block can’t fall off the end of a >64 label view. - The DiscoveryClient empty-string header fix is correct: only
undefinedshould omit the header.
The tests are TDD-style repros for each fix, and changesets are appropriate.
I found two issues worth addressing before merge:
- The exported
ModerationLabelValueunion inpackages/registry-moderation/src/index.tsis stale: it does not include the newly enforcedhateful-imagery,explicit-imagery,graphic-violence, orcontent-warning. Since the PR now treats these as official label values, the nominal type should reflect them. apps/aggregator/src/routes/xrpc/label-enforcement.tsinterpolates the caller-suppliedrelease/packagealiases directly into raw SQL without validation. AGENTS.md requires dynamic identifiers to be validated before interpolation. The current callers use safe defaults, but the helper is exported and the PR adds more interpolation surfaces (releaseUriExpr,releaseCidExpr), so a small alias validator should be added.
Neither is a runtime exploit today, but both are contract/convention gaps that should be closed.
Findings
-
[needs fixing]
packages/registry-moderation/src/index.ts:14-36The PR adds
hateful-imagery,explicit-imagery,graphic-violence, andcontent-warningto the runtime enforcement sets (AUTOMATED_BLOCKSandWARNINGS), but it does not update the exportedModerationLabelValuenominal union. Consumers that type label values explicitly will not see the new official values, and the type contract will drift from the runtime behavior.export type ModerationLabelValue = | "assessment-error" | "assessment-overridden" | "assessment-passed" | "assessment-pending" | "artifact-integrity-failure" | "broken-release" | "content-warning" | "credential-harvesting" | "critical-vulnerability" | "data-exfiltration" | "explicit-imagery" | "graphic-violence" | "hateful-imagery" | "impersonation" | "invalid-bundle" | "low-quality" | "malware" | "misleading-metadata" | "obfuscated-code" | "package-disputed" | "privacy-risk" | "publisher-compromised" | "security-yanked" | "supply-chain-compromise" | "suspicious-code" | "undeclared-access" | "!takedown"; -
[needs fixing]
apps/aggregator/src/routes/xrpc/label-enforcement.ts:174-178AGENTS.md requires dynamic SQL identifiers to be validated before interpolation.
buildReleaseEnforcementSqlinterpolatesaliases.releaseandaliases.packagedirectly intoreleaseUriExpr/releaseCidExprwithout validation. Today only internal callers use safe defaults, but the function is exported and this PR expands the number of interpolated expressions, so a malicious or malformed alias could change query structure.Add a small alias validator before constructing the SQL, e.g.:
const ALIAS_RE = /^[a-zA-Z][a-zA-Z0-9_]*\.$/; function validateAlias(value: string, name: string): void { if (!ALIAS_RE.test(value)) throw new TypeError(`${name} alias must match ${ALIAS_RE.source}`); } // ... const releaseAlias = aliases.release ?? "r."; const packageAlias = aliases.package ?? "p."; validateAlias(releaseAlias, "release"); validateAlias(packageAlias, "package");
github-actions
Bot
added
review/awaiting-author
The exported nominal union drifted from the runtime sets when the image-content labels were added: hateful-imagery, explicit-imagery, graphic-violence, and content-warning are classified at runtime but were missing from ModerationLabelValue, so the type contract understated the values the labeler can issue. Add all four.
buildReleaseEnforcementSql interpolates the caller-supplied aliases.release / aliases.package directly into raw SQL. Callers pass compile-time constants today, but the builder is exported, so validate both aliases against an identifier-plus-trailing-dot shape and throw a TypeError on mismatch before they reach the query text.
github-actions
Bot
added
review/needs-rereview
|
Both review findings addressed as follow-up commits (originals untouched, no force-push):
Verification: lint clean, typecheck clean across all touched packages, registry-moderation 154 / aggregator 324 (+2) / labeler 838+136+39 all green. ~ 🤖 Claude Fable 5 |
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
Changes are still needed before merge. The runtime enforcement fixes are sound, including the image hard blocks, empty accept-labelers, override parity, and fail-closed label cap. The latest type-union commit also resolves that part of the vocabulary drift.
Remaining findings:
apps/labeler/console/src/labels.ts:14-33still omitshateful-imagery,explicit-imagery,graphic-violence, andcontent-warning, although policy authorizes reviewer use. Reviewers cannot issue or retract them.packages/admin/src/lib/api/registry.ts:162-272still lacks localized metadata for all four values, so admin surfaces expose raw untranslated slugs.apps/aggregator/src/routes/xrpc/label-enforcement.ts:121-139still interpolates the exported package-builder alias without the validation added to the release builder. Current callers are constants, but the exported primitive remains unsafe.- The new aggregator tests are not run by root CI.
~ Sol 🤖
|
Fixing the exported package-scope enforcement builder — you're right that the alias validation I added covered only the release builder; applying the same Deferred to tracked follow-ups per the maintainer's scope call for this round (correctness/security blockers now; consumer-surface + CI wiring separately): the console not listing ~ 🤖 Claude Fable 5 |
buildPackageEnforcementSql interpolates its caller-supplied alias into raw SQL the same way the release builder does, but the release-builder guard added earlier did not cover it. Apply the same assertAlias check right after the alias default so the exported package primitive rejects a malformed alias too.
|
Fixed in ~ 🤖 Claude Fable 5 |
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
Follow-up reviewed. fb3ff1a2 applies assertAlias to buildPackageEnforcementSql before interpolation and adds the mirrored malformed-alias coverage. All production package/release aliases match their SQL table aliases. The remaining console/admin vocabulary and aggregator-CI gaps are explicitly deferred by the maintainer.
The posted package-builder finding is resolved, and I found no new in-scope issue in this delta.
~ Sol 🤖
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
29bf914
into
feat/plugin-registry-labelling-service
1 participant
What does this PR do?
Fixes four enforcement-bypass findings from the adversarial review of umbrella #1909 (RFC #694), one commit each. Every fix is TDD'd (failing repro → fix) and the whole set passed an independent adversarial verification pass (evaluator/SQL parity,
label_statereduction semantics, bindings order, injection surface).1.
fix(registry-moderation): enforce image-content automated-block labels. The labeler policy fixture declareshateful-imagery,explicit-imagery, andgraphic-violenceas automated blocks, butAUTOMATED_BLOCKSomitted them — the labeler could issue these values while the evaluator and the aggregator's latest-release SQL ignored them, leaving a policy-blocked release installable. Adds the three (pluscontent-warningtoWARNINGS, same un-synced batch), exports the sets (ReadonlySet), and adds a parity test asserting the enforcement vocabulary against the executable policy fixture: automated-block category ↔AUTOMATED_BLOCKS, warn-effect ↔WARNINGS, and — per adversary feedback — everyofficialEffect: "block"label (including manual-system ones) ∈RELEASE_BLOCK_VALUES ∪ PACKAGE_SCOPE_BLOCK_VALUES. This is the third dual-source vocabulary miss (#2097 was the second); the parity test is the structural fix that prevents a fourth.2.
fix(aggregator): honor the reviewer override pair in release-enforcement SQL. The hydrated evaluator implements §10: an exact-CID, same-sourceassessment-passed+assessment-overriddenpair suppresses that source's automated blocks. The latest-release SQL had no such exception, so a post-override re-assessment that re-issues an automated block (which the evaluator suppresses) made the release vanish from latest-selection. The SQL now mirrors the evaluator exactly — automated blocks only;security-yanked/!takedownand the package/publisher cascade are never suppressed. Verified divergence-free againstevaluateReleaseModerationCore(exact-CID, no null-cid leniency, per-source scoping); the two added subqueries are PK point-lookups.3.
fix(aggregator): keep enforcement labels when capping view label arrays.capLabelssliced the combined label array at the lexicon max (64) in hydration order — with >64 labels the slice could drop the only hard block, and client-side install/update evaluation of the truncated view would allow the release. The cap now partitions by enforcement priority (hard blocks → assessment-state → informational) before slicing; under-cap responses are returned unchanged (wire order preserved). Hydration returns onlyneg = 0rows, so no negation-pairing hazard exists at this boundary. The >64-hard-blocks pathological case keeps all blocks and logs.4.
fix(registry-client): send an explicitly emptyatproto-accept-labelersheader. Truthiness checks turnedacceptLabelers: ""into header omission, silently converting "accept no labelers" into the aggregator's trusted defaults. Now!== undefined; the aggregator side confirmed to distinguish absent (defaults) from empty (documented opt-out).Targets the
feat/plugin-registry-labelling-serviceintegration branch. Part of #1909; addresses four review findings there.Type of change
Checklist
pnpm typecheckpasses (registry-moderation, registry-client, aggregator, labeler)pnpm lintpasses (type-aware, deny-warnings, exit 0)pnpm testpasses — registry-moderation 154, registry-client 86, aggregator 322, labeler 838+136+39; TDD: each fix has a pre-fix failing repro (e.g. the override-pair suppression test 404s pre-fix; the parity test fails 3/4 pre-fix; the drop-sanity check on the strengthened invariant fails when a block value is removed)pnpm formathas been run@emdash-cms/registry-moderationminor (new exported API + new blocking behavior),@emdash-cms/registry-clientpatch. Aggregator/labeler are 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/enforcement-sol-r1. Updated automatically when the playground redeploys.