Skip to content

Commit 29bf914

Browse files
authored
fix: close four label-enforcement bypasses (review round 1) (#2112)
* fix(registry-moderation): enforce image-content automated-block labels 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. * fix(registry-client): send an explicitly empty accept-labelers header 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. * fix(aggregator): honor reviewer override pair in release enforcement SQL 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. * fix(aggregator): keep enforcement labels when capping view label arrays 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). * fix(registry-moderation): add image labels to ModerationLabelValue union 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. * fix(aggregator): validate release/package SQL aliases 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. * fix(aggregator): validate the package enforcement builder's SQL alias 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.
1 parent f71c593 commit 29bf914

11 files changed

Lines changed: 480 additions & 16 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@emdash-cms/registry-client": patch
3+
---
4+
5+
Sends an explicitly empty `atproto-accept-labelers` header when `DiscoveryClient` is configured with `acceptLabelers: ""`, so "accept no labelers" is honored instead of being dropped and letting the aggregator apply its trusted defaults. Omitting the option entirely still sends no header.
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@emdash-cms/registry-moderation": minor
3+
---
4+
5+
Blocks releases labeled `hateful-imagery`, `explicit-imagery`, or `graphic-violence` -- these automated-block labels were issuable by the labeler but silently ignored by release evaluation and enforcement, leaving a policy-blocked release installable. Also recognizes the `content-warning` label as a non-blocking warning, and exposes the `AUTOMATED_BLOCKS` and `WARNINGS` label-value sets for consumers that classify labels directly.

apps/aggregator/src/routes/xrpc/label-enforcement.ts

Lines changed: 100 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
import { NSID } from "@emdash-cms/registry-lexicons";
2121
import {
22+
AUTOMATED_BLOCKS,
2223
PACKAGE_SCOPE_BLOCK_VALUES,
2324
RELEASE_BLOCK_VALUES,
2425
type AcceptedLabelerPolicy,
@@ -32,6 +33,41 @@ const HYDRATION_CHUNK_SIZE = 50;
3233
* hydration results — redaction decisions must see the full label set. */
3334
export const LABELS_MAX_LENGTH = 64;
3435

36+
/** Release-scope automated-block values (`RELEASE_BLOCK_VALUES` minus the
37+
* manual `security-yanked` / `!takedown`). The override exception below
38+
* applies only to these — the manual values are never suppressed. */
39+
const AUTOMATED_BLOCK_VALUES: readonly string[] = [...AUTOMATED_BLOCKS];
40+
/** The manual release blocks the reviewer override pair never suppresses. */
41+
const MANUAL_RELEASE_BLOCK_VALUES: readonly string[] = RELEASE_BLOCK_VALUES.filter(
42+
(value) => !AUTOMATED_BLOCKS.has(value),
43+
);
44+
/** Assessment-state label values: `assessment-pending` / `assessment-error`
45+
* steer eligibility, and `assessment-passed` / `assessment-overridden` form
46+
* the reviewer override pair. Enforcement-relevant for view truncation. */
47+
const ASSESSMENT_STATE_VALUES: readonly string[] = [
48+
"assessment-pending",
49+
"assessment-passed",
50+
"assessment-overridden",
51+
"assessment-error",
52+
];
53+
54+
const ENFORCEMENT_HARD_BLOCK_VALUES: ReadonlySet<string> = new Set([
55+
...RELEASE_BLOCK_VALUES,
56+
...PACKAGE_SCOPE_BLOCK_VALUES,
57+
]);
58+
const ENFORCEMENT_STATE_VALUES: ReadonlySet<string> = new Set(ASSESSMENT_STATE_VALUES);
59+
60+
/** Truncation priority for a view's `labels`: hard blocks (0) rank ahead of
61+
* assessment states (1) ahead of informational labels (2). Blocks decide a
62+
* client's install/serve refusal, so the lexicon `maxLength` cap must never
63+
* drop one in favour of a display-only label; states change eligibility and
64+
* complete the override pair. Used by `capLabels`. */
65+
export function labelTruncationPriority(val: string): 0 | 1 | 2 {
66+
if (ENFORCEMENT_HARD_BLOCK_VALUES.has(val)) return 0;
67+
if (ENFORCEMENT_STATE_VALUES.has(val)) return 1;
68+
return 2;
69+
}
70+
3571
export interface EnforcementSql {
3672
sql: string;
3773
bindings: unknown[];
@@ -88,6 +124,7 @@ export function buildPackageEnforcementSql(
88124
alias = "p.",
89125
): EnforcementSql {
90126
if (accepted.length === 0) return { sql: "", bindings: [] };
127+
assertAlias(alias);
91128
const srcs = accepted.map((policy) => policy.did);
92129
const sql = `
93130
AND NOT EXISTS (
@@ -111,12 +148,33 @@ export interface ReleaseEnforcementAliases {
111148
package?: string;
112149
}
113150

151+
/** A SQL row alias with its trailing dot (e.g. `"r."`). */
152+
const ALIAS_PATTERN = /^[a-zA-Z][a-zA-Z0-9_]*\.$/;
153+
154+
/** Guards a caller-supplied alias before it is interpolated into raw SQL.
155+
* Callers pass compile-time constants today, but the builders are exported. */
156+
function assertAlias(alias: string): void {
157+
if (!ALIAS_PATTERN.test(alias))
158+
throw new TypeError("SQL alias must be an identifier followed by '.'");
159+
}
160+
114161
/**
115162
* `NOT EXISTS` clause excluding a release whose own URI carries a
116163
* `RELEASE_BLOCK_VALUES` label, or whose parent package URI / publisher DID
117164
* carries a `PACKAGE_SCOPE_BLOCK_VALUES` label (cascade), from an accepted
118165
* source. Requires the `packages` row joined under `aliases.package` — the
119166
* package-scope branch's CID comparison reads its `signature_metadata`.
167+
*
168+
* Mirrors the hydrated evaluator's §10 override rule (see
169+
* `evaluateReleaseModerationCore` in `@emdash-cms/registry-moderation`): a
170+
* source's exact-CID `assessment-passed` + `assessment-overridden` pair
171+
* suppresses that same source's automated blocks for the release. Without
172+
* this, a post-override re-assessment that re-issues a live automated block
173+
* would keep the release out of latest-selection even though the evaluator
174+
* treats it as eligible. The exception is scoped exactly as the evaluator
175+
* scopes it: automated blocks on the release URI only — the manual
176+
* `security-yanked` / `!takedown` release blocks and the package/publisher
177+
* cascade are never suppressed by the pair.
120178
*/
121179
export function buildReleaseEnforcementSql(
122180
accepted: AcceptedLabelerPolicy[],
@@ -126,7 +184,11 @@ export function buildReleaseEnforcementSql(
126184
if (accepted.length === 0) return { sql: "", bindings: [] };
127185
const releaseAlias = aliases.release ?? "r.";
128186
const packageAlias = aliases.package ?? "p.";
187+
assertAlias(releaseAlias);
188+
assertAlias(packageAlias);
129189
const srcs = accepted.map((policy) => policy.did);
190+
const releaseUriExpr = `'at://' || ${releaseAlias}did || '/${NSID.packageRelease}/' || ${releaseAlias}rkey`;
191+
const releaseCidExpr = `json_extract(${releaseAlias}signature_metadata, '$.cid')`;
130192
const sql = `
131193
AND NOT EXISTS (
132194
SELECT 1 FROM label_state ls
@@ -135,9 +197,35 @@ export function buildReleaseEnforcementSql(
135197
AND (ls.exp_epoch_ms IS NULL OR ls.exp_epoch_ms > ?)
136198
AND (
137199
(
138-
ls.uri = 'at://' || ${releaseAlias}did || '/${NSID.packageRelease}/' || ${releaseAlias}rkey
139-
AND ls.val IN (${inClause(RELEASE_BLOCK_VALUES)})
140-
AND (ls.cid IS NULL OR ls.cid = json_extract(${releaseAlias}signature_metadata, '$.cid'))
200+
ls.uri = ${releaseUriExpr}
201+
AND ls.val IN (${inClause(AUTOMATED_BLOCK_VALUES)})
202+
AND (ls.cid IS NULL OR ls.cid = ${releaseCidExpr})
203+
AND NOT (
204+
EXISTS (
205+
SELECT 1 FROM label_state pass
206+
WHERE pass.src = ls.src
207+
AND pass.uri = ${releaseUriExpr}
208+
AND pass.val = 'assessment-passed'
209+
AND pass.cid = ${releaseCidExpr}
210+
AND pass.neg = 0
211+
AND (pass.exp_epoch_ms IS NULL OR pass.exp_epoch_ms > ?)
212+
)
213+
AND EXISTS (
214+
SELECT 1 FROM label_state ovr
215+
WHERE ovr.src = ls.src
216+
AND ovr.uri = ${releaseUriExpr}
217+
AND ovr.val = 'assessment-overridden'
218+
AND ovr.cid = ${releaseCidExpr}
219+
AND ovr.neg = 0
220+
AND (ovr.exp_epoch_ms IS NULL OR ovr.exp_epoch_ms > ?)
221+
)
222+
)
223+
)
224+
OR
225+
(
226+
ls.uri = ${releaseUriExpr}
227+
AND ls.val IN (${inClause(MANUAL_RELEASE_BLOCK_VALUES)})
228+
AND (ls.cid IS NULL OR ls.cid = ${releaseCidExpr})
141229
)
142230
OR
143231
(
@@ -150,7 +238,15 @@ export function buildReleaseEnforcementSql(
150238
`;
151239
return {
152240
sql,
153-
bindings: [...srcs, nowMs, ...RELEASE_BLOCK_VALUES, ...PACKAGE_SCOPE_BLOCK_VALUES],
241+
bindings: [
242+
...srcs,
243+
nowMs,
244+
...AUTOMATED_BLOCK_VALUES,
245+
nowMs,
246+
nowMs,
247+
...MANUAL_RELEASE_BLOCK_VALUES,
248+
...PACKAGE_SCOPE_BLOCK_VALUES,
249+
],
154250
};
155251
}
156252

apps/aggregator/src/routes/xrpc/views.ts

Lines changed: 35 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
import { type AggregatorDefs, NSID } from "@emdash-cms/registry-lexicons";
2222

2323
import { isPlainObject, parseSignatureMetadataCid } from "../../utils.js";
24-
import { LABELS_MAX_LENGTH, type LabelView } from "./label-enforcement.js";
24+
import { LABELS_MAX_LENGTH, labelTruncationPriority, type LabelView } from "./label-enforcement.js";
2525

2626
/** Subset of columns from `packages` we read for `packageView`. Selecting
2727
* exactly these columns keeps the SQL query auditable and cheap. */
@@ -166,15 +166,42 @@ export function publisherUri(row: Pick<PublisherRow, "did">) {
166166
* labels are the union of multiple hydrated subjects (e.g. a release's own
167167
* URI plus its parent package and publisher DID). Hydration returns
168168
* untruncated per-subject sets so redaction decisions see every label;
169-
* this boundary cap trims only the final view array. */
169+
* this boundary cap trims only the final view array.
170+
*
171+
* The trim is enforcement-preserving: a plain slice can drop the only hard
172+
* block when a subject carries more than `LABELS_MAX_LENGTH` labels, and a
173+
* client evaluating the truncated view would then treat the release as
174+
* installable. Labels are ordered by `labelTruncationPriority` (blocks, then
175+
* assessment states, then informational) before slicing, so blocks survive.
176+
* Hydrated labels never carry negations (`hydrateLabels` returns only
177+
* `neg = 0` rows), so ordering can't split a label from its negation. In the
178+
* pathological case of more than `LABELS_MAX_LENGTH` hard blocks the kept
179+
* slice is still all blocks — the release stays blocked, only the exact set
180+
* is lossy — and we log an error. */
170181
function capLabels(labels: LabelView[], uri: string): LabelView[] {
171182
if (labels.length <= LABELS_MAX_LENGTH) return labels;
172-
console.warn("[aggregator] view labels truncated to maxLength", {
173-
uri,
174-
count: labels.length,
175-
cap: LABELS_MAX_LENGTH,
176-
});
177-
return labels.slice(0, LABELS_MAX_LENGTH);
183+
const ordered = labels.toSorted(
184+
(left, right) => labelTruncationPriority(left.val) - labelTruncationPriority(right.val),
185+
);
186+
const kept = ordered.slice(0, LABELS_MAX_LENGTH);
187+
const droppedBlocks = ordered
188+
.slice(LABELS_MAX_LENGTH)
189+
.filter((label) => labelTruncationPriority(label.val) === 0).length;
190+
if (droppedBlocks > 0) {
191+
console.error("[aggregator] view label cap dropped hard-block labels", {
192+
uri,
193+
count: labels.length,
194+
cap: LABELS_MAX_LENGTH,
195+
droppedBlocks,
196+
});
197+
} else {
198+
console.warn("[aggregator] view labels truncated to maxLength", {
199+
uri,
200+
count: labels.length,
201+
cap: LABELS_MAX_LENGTH,
202+
});
203+
}
204+
return kept;
178205
}
179206

180207
/** `LabelView.src` is `string`; the lexicon's `Label.src` is a branded DID
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
/**
2+
* Unit tests for the enforcement SQL builders' input guards. The builders are
3+
* exported and interpolate the caller-supplied row alias into raw SQL, so a
4+
* malformed alias must be rejected rather than reach the query text.
5+
*/
6+
7+
import { type AcceptedLabelerPolicy } from "@emdash-cms/registry-moderation";
8+
import { describe, expect, it } from "vitest";
9+
10+
import {
11+
buildPackageEnforcementSql,
12+
buildReleaseEnforcementSql,
13+
} from "../src/routes/xrpc/label-enforcement.js";
14+
15+
const accepted: AcceptedLabelerPolicy[] = [{ did: "did:web:labels.example", redact: false }];
16+
17+
describe("buildReleaseEnforcementSql alias validation", () => {
18+
it("rejects an alias that is not an identifier followed by a dot", () => {
19+
expect(() =>
20+
buildReleaseEnforcementSql(accepted, 0, { release: "r; DROP TABLE releases;--" }),
21+
).toThrow(TypeError);
22+
expect(() => buildReleaseEnforcementSql(accepted, 0, { package: "p" })).toThrow(TypeError);
23+
});
24+
25+
it("accepts the default aliases", () => {
26+
const { sql } = buildReleaseEnforcementSql(accepted, 0);
27+
expect(sql).toContain("NOT EXISTS");
28+
});
29+
});
30+
31+
describe("buildPackageEnforcementSql alias validation", () => {
32+
it("rejects an alias that is not an identifier followed by a dot", () => {
33+
expect(() => buildPackageEnforcementSql(accepted, 0, "p; DROP TABLE packages;--")).toThrow(
34+
TypeError,
35+
);
36+
expect(() => buildPackageEnforcementSql(accepted, 0, "p")).toThrow(TypeError);
37+
});
38+
39+
it("accepts the default alias", () => {
40+
const { sql } = buildPackageEnforcementSql(accepted, 0);
41+
expect(sql).toContain("NOT EXISTS");
42+
});
43+
});

apps/aggregator/test/read-api.test.ts

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -850,6 +850,115 @@ describe("getLatestRelease", () => {
850850
const body = (await res.json()) as Record<string, unknown>;
851851
expect(body["version"]).toBe("1.0.0");
852852
});
853+
854+
// §10 override rule: a source's exact-CID assessment-passed +
855+
// assessment-overridden pair suppresses that source's automated blocks for
856+
// the release, mirroring `evaluateReleaseModerationCore`. The enforcement
857+
// SQL must not exclude a release whose only live block is so suppressed.
858+
const OVERRIDE_CID = "bafoverridecid";
859+
860+
it("includes a release whose re-issued automated block is suppressed by a same-source exact-CID override pair", async () => {
861+
await seedLabeler(LABELER_DID, true);
862+
await seedPackage({ slug: "demo", latestVersion: "1.0.0" });
863+
await seedRelease({ version: "1.0.0", cid: OVERRIDE_CID });
864+
const uri = releaseUri("demo", "1.0.0");
865+
await seedLabelState({ uri, val: "assessment-passed", cid: OVERRIDE_CID });
866+
await seedLabelState({ uri, val: "assessment-overridden", cid: OVERRIDE_CID });
867+
await seedLabelState({ uri, val: "malware", cid: OVERRIDE_CID });
868+
869+
const res = await SELF.fetch(
870+
`https://test/xrpc/${NSID.aggregatorGetLatestRelease}?did=${DID_A}&package=demo`,
871+
);
872+
expect(res.status).toBe(200);
873+
const body = (await res.json()) as Record<string, unknown>;
874+
expect(body["version"]).toBe("1.0.0");
875+
});
876+
877+
it("includes a release carrying an override pair and no block", async () => {
878+
await seedLabeler(LABELER_DID, true);
879+
await seedPackage({ slug: "demo", latestVersion: "1.0.0" });
880+
await seedRelease({ version: "1.0.0", cid: OVERRIDE_CID });
881+
const uri = releaseUri("demo", "1.0.0");
882+
await seedLabelState({ uri, val: "assessment-passed", cid: OVERRIDE_CID });
883+
await seedLabelState({ uri, val: "assessment-overridden", cid: OVERRIDE_CID });
884+
885+
const res = await SELF.fetch(
886+
`https://test/xrpc/${NSID.aggregatorGetLatestRelease}?did=${DID_A}&package=demo`,
887+
);
888+
expect(res.status).toBe(200);
889+
const body = (await res.json()) as Record<string, unknown>;
890+
expect(body["version"]).toBe("1.0.0");
891+
});
892+
893+
it("excludes a release with an automated block and no override pair", async () => {
894+
await seedLabeler(LABELER_DID, true);
895+
await seedPackage({ slug: "demo", latestVersion: "1.0.0" });
896+
await seedRelease({ version: "1.0.0", cid: OVERRIDE_CID });
897+
await seedLabelState({ uri: releaseUri("demo", "1.0.0"), val: "malware", cid: OVERRIDE_CID });
898+
899+
const res = await SELF.fetch(
900+
`https://test/xrpc/${NSID.aggregatorGetLatestRelease}?did=${DID_A}&package=demo`,
901+
);
902+
expect(res.status).toBe(404);
903+
});
904+
905+
it("still excludes when the override pair is from a different source than the block", async () => {
906+
const OTHER_LABELER_DID = "did:web:other-labels.example";
907+
await seedLabeler(LABELER_DID, true);
908+
await seedLabeler(OTHER_LABELER_DID, true);
909+
await seedPackage({ slug: "demo", latestVersion: "1.0.0" });
910+
await seedRelease({ version: "1.0.0", cid: OVERRIDE_CID });
911+
const uri = releaseUri("demo", "1.0.0");
912+
await seedLabelState({ uri, val: "malware", cid: OVERRIDE_CID });
913+
await seedLabelState({
914+
uri,
915+
val: "assessment-passed",
916+
cid: OVERRIDE_CID,
917+
src: OTHER_LABELER_DID,
918+
});
919+
await seedLabelState({
920+
uri,
921+
val: "assessment-overridden",
922+
cid: OVERRIDE_CID,
923+
src: OTHER_LABELER_DID,
924+
});
925+
926+
const res = await SELF.fetch(
927+
`https://test/xrpc/${NSID.aggregatorGetLatestRelease}?did=${DID_A}&package=demo`,
928+
);
929+
expect(res.status).toBe(404);
930+
});
931+
932+
it("still excludes when the override pair CID does not match the release", async () => {
933+
await seedLabeler(LABELER_DID, true);
934+
await seedPackage({ slug: "demo", latestVersion: "1.0.0" });
935+
await seedRelease({ version: "1.0.0", cid: OVERRIDE_CID });
936+
const uri = releaseUri("demo", "1.0.0");
937+
await seedLabelState({ uri, val: "malware", cid: OVERRIDE_CID });
938+
await seedLabelState({ uri, val: "assessment-passed", cid: "bafstalecid" });
939+
await seedLabelState({ uri, val: "assessment-overridden", cid: "bafstalecid" });
940+
941+
const res = await SELF.fetch(
942+
`https://test/xrpc/${NSID.aggregatorGetLatestRelease}?did=${DID_A}&package=demo`,
943+
);
944+
expect(res.status).toBe(404);
945+
});
946+
947+
it("never suppresses a security-yanked release block, even with an override pair", async () => {
948+
await seedLabeler(LABELER_DID, true);
949+
await seedPackage({ slug: "demo", latestVersion: "1.0.0" });
950+
await seedRelease({ version: "1.0.0", cid: OVERRIDE_CID });
951+
const uri = releaseUri("demo", "1.0.0");
952+
await seedLabelState({ uri, val: "assessment-passed", cid: OVERRIDE_CID });
953+
await seedLabelState({ uri, val: "assessment-overridden", cid: OVERRIDE_CID });
954+
// `security-yanked` is issued CID-less (policy cidRule: forbidden).
955+
await seedLabelState({ uri, val: "security-yanked" });
956+
957+
const res = await SELF.fetch(
958+
`https://test/xrpc/${NSID.aggregatorGetLatestRelease}?did=${DID_A}&package=demo`,
959+
);
960+
expect(res.status).toBe(404);
961+
});
853962
});
854963

855964
describe("searchPackages", () => {

0 commit comments

Comments
 (0)