Skip to content

Commit ccd9397

Browse files
authored
fix(sharing)!: a rule with no criteria shares NOTHING, not every record (#3929)
Closes #3896. A sharing rule stored with `criteria_json: null` evaluated as `find(object, { filter: {} })` under the system context — every record of the object granted to the recipient. Three paths reached that shape without validation: `SharingRuleService.defineRule` (the REST endpoint's target), a direct `sys_sharing_rule` insert (what Setup issues), and the seed bootstrap's own empty-condition branch. Fixed in three layers: `defineRule` rejects a match-all criteria with VALIDATION_FAILED; the evaluator matches nothing and logs why, so rows already stored under-share instead of over-share and the next reconcile revokes their grants; a `sys_sharing_rule` insert guard returns a field-level 400 naming `criteria_json`. 17 new unit tests plus a dogfood test that POSTs the reported body against a booted showcase stack.
1 parent f734677 commit ccd9397

21 files changed

Lines changed: 739 additions & 40 deletions
Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
---
2+
"@objectstack/plugin-sharing": minor
3+
"@objectstack/rest": patch
4+
"@objectstack/spec": patch
5+
---
6+
7+
fix(security)!: a sharing rule with no criteria now shares NOTHING instead of every record (#3896)
8+
9+
`SharingRuleSchema` has always required `condition`, and its doc is explicit
10+
that a predicate the compiler cannot lower is *"skipped and logged — never
11+
seeded as a permissive match-all (ADR-0049)"*. The declared/seed path honoured
12+
that. The two other ways to create a rule did not:
13+
14+
- **`POST {basePath}/sharing/rules`** plucks its body field-by-field into
15+
`SharingRuleService.defineRule`, which validated `name` / `label` / `object` /
16+
`recipientType` / `recipientId` — and not `criteria`. A missing, `null`, or
17+
**misspelled** key (`criterias`) was stored as `criteria_json: null`, answered
18+
`201` with no warning, and evaluated as
19+
`find(object, { filter: {}, context: SYSTEM_CTX })`: every record of the
20+
object, up to 5000, granted to the recipient. Triggering it took a typo, not
21+
an attacker.
22+
- **Authoring a rule in Setup** is a direct `sys_sharing_rule` insert, which
23+
never reaches `defineRule` at all.
24+
25+
Empty criteria is now rejected everywhere a rule can be written, and — because
26+
rules created before this gate are already in the table — the evaluator refuses
27+
to act on one regardless of how it got there.
28+
29+
- **`defineRule` rejects a match-all criteria** with
30+
`VALIDATION_FAILED: criteria is required …`, alongside its other required
31+
fields. Covers the REST endpoint, programmatic callers, and the seeder.
32+
Rejected shapes: missing / `null` / `''` / `{}` / `[]` / `{ $and: [] }` /
33+
unparsable JSON (e.g. a CEL source typed into the Criteria box).
34+
- **The evaluator matches nothing** for such a rule and logs why, so a row
35+
stored before this release under-shares instead of over-sharing: the next
36+
reconcile *revokes* the grants it had materialised. Both evaluation paths are
37+
covered — the bulk `evaluateRule` and the per-record write-hook path.
38+
- **`bindRuleCriteriaGuard`** fails `sys_sharing_rule` inserts with no
39+
criteria as a field-level `VALIDATION_FAILED` (a 400 naming `criteria_json`),
40+
so the Setup path reports the problem instead of saving an inert rule
41+
(ADR-0078). Updates are checked only when the patch supplies
42+
`criteria_json` — switching an over-broad legacy rule off must not require
43+
inventing a criteria for it first.
44+
- **The seed bootstrap's "empty condition = match-all" branch is gone**: a
45+
missing or empty `condition` is now skipped and logged like any other
46+
non-lowerable one.
47+
- `POST {basePath}/sharing/rules` also accepts `criteria_json` as an alias for
48+
`criteria`, matching the snake_case aliases the endpoint already takes for
49+
`object_name` / `recipient_type` / `access_level`.
50+
51+
**Migration.** There is no "share every record" sharing rule, and there never
52+
usefully was one — the shape existed only as a failure mode. A rule that
53+
relied on it must state its predicate (`criteria: { stage: 'won' }`), or, if
54+
the object really should be readable by everyone, use the object's
55+
organization-wide default (`sharingModel`) instead. Rules already stored with
56+
a null `criteria_json` need no data migration: they stop granting on the next
57+
evaluation and their existing grants are revoked.

content/docs/permissions/sharing-rules.mdx

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,24 @@ compiler. A condition the compiler cannot lower is **skipped and logged —
149149
never seeded as a permissive match-all** (ADR-0049): a bad condition
150150
under-shares rather than over-shares.
151151

152+
### There is no "share every record" rule
153+
154+
The predicate is **mandatory on every authoring path**, whether you declare
155+
the rule in code, `POST` it to the REST API, or build it in Setup:
156+
157+
- `defineSharingRule({...})``condition` is required by the schema.
158+
- `POST {basePath}/sharing/rules`a request whose `criteria` is missing,
159+
`null`, empty (`{}`), or unparsable fails with `400 VALIDATION_FAILED`. So
160+
does a **misspelled** key such as `criterias`, which would otherwise be
161+
indistinguishable from "no criteria" (#3896).
162+
- Creating the rule in Setupan empty **Criteria** field is rejected.
163+
164+
A rule that reached the table without a criteriaone stored before this gate
165+
existedshares **nothing** and logs why: the next evaluation revokes the
166+
grants it had issued rather than re-granting the whole object. If you really
167+
do want everyone to read every record of an object, that is the object's
168+
organization-wide default (`sharingModel`), not a sharing rule.
169+
152170
> **Retired shapes.** The pre-ADR-0090 `group` recipient was renamed to
153171
> `team`, and the `guest` recipient was removedanonymous access is served
154172
> by the [public-form grant](/docs/permissions/authorization) and share

docs/audits/2026-06-metadata-functional-completeness.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,8 @@ Investigator 4 flagged, as a P0 security hole, that a criteria sharing rule with
9191

9292
So the natural authoring path **cannot** silently drop the predicate. Residual: only a hand-crafted `{dialect,source:''}` envelope or a direct `sys_sharing_rule` row could reach the match-all branch — worth a one-line belt-and-suspenders guard, **not a P0**. This is consistent with ADR-0049 already being *applied* to `SharingRuleSchema` (#1887).
9393

94+
> **Update (2026-07, #3896) — the residual was bigger than "hand-crafted", and is now closed.** The correction above is right about the *declared* path and wrong about the size of what it left over. The residual was not only a hand-crafted envelope: `POST {basePath}/sharing/rules` plucks its body field-by-field into `SharingRuleService.defineRule`, which validated `name` / `label` / `object` / `recipientType` / `recipientId` and **not** `criteria` — so a missing, `null`, or *misspelled* (`criterias`) key stored `criteria_json: null`, returned `201`, and evaluated as `find(object, { filter: {} })`. Authoring in Setup is a direct `sys_sharing_rule` insert, which reached the same place. Neither needs malice, only a typo. Closed by gating `defineRule` and the table's `beforeInsert`, and by making the evaluator match **nothing** on a match-all criteria so rows already stored under-share instead. The lesson below still holds — but it cuts both ways: "the schema requires it" is a claim about the schema, not about every entry that writes the same row.
95+
9496
**Lesson** (it sets the disposition for the whole catalog): the audit produces *candidates*, not confirmed bugs. The scariest one collapsed on a 3-file read. Every Tier-A/B item gets a verification pass before it becomes a lint rule.
9597

9698
---

packages/plugins/plugin-sharing/src/bootstrap-declared-sharing-rules.ts

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,9 @@
1717
* - a CEL `condition` the canonical compiler cannot lower (functions,
1818
* cross-object traversal) — ADR-0058 D2. Compound predicates (AND/OR,
1919
* comparisons, null, in) DO lower and are enforced (ADR-0058 D3, #1887).
20+
* - a missing or empty `condition`, which lowers to no predicate at all
21+
* (#3896). It used to seed a rule with `criteria_json: null` — the exact
22+
* permissive match-all the rest of this file exists to prevent.
2023
* - defensively, any stale pre-built package that still registers an old
2124
* `owner`-type / unmapped-recipient shape.
2225
*
@@ -28,6 +31,7 @@
2831
import type { SharingRuleService } from './sharing-rule-service.js';
2932
import type { SharingRuleRecipientType, ShareAccessLevel } from '@objectstack/spec/contracts';
3033
import { compileCelToFilter } from '@objectstack/formula';
34+
import { isMatchAllCriteria } from './rule-criteria.js';
3135

3236
const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const;
3337

@@ -111,16 +115,20 @@ export async function bootstrapDeclaredSharingRules(
111115
logger?.warn?.('[sharing-rule] skipped owner-based rule (retired authoring shape — use a criteria rule)', { rule: r.name });
112116
skipped += 1; continue;
113117
}
114-
// criteria rules: translate CEL → filter. Empty condition = match-all (intentional).
115-
let criteria: Record<string, unknown> | undefined;
116-
if (r.condition != null && String(r.condition).trim() !== '') {
117-
const f = celToFilter(r.condition);
118-
if (!f) {
119-
logger?.warn?.('[sharing-rule] skipped (untranslatable CEL condition) [experimental]', { rule: r.name, condition: r.condition });
120-
skipped += 1; continue;
121-
}
122-
criteria = f;
118+
// criteria rules: translate CEL → filter. [#3896] A missing / empty
119+
// condition used to fall through this branch with `criteria` undefined,
120+
// which `defineRule` stored as `criteria_json: null` and the evaluator
121+
// read as the empty filter — the one outcome this file's header forbids.
122+
// It is now skipped like any other non-lowerable condition: the authoring
123+
// schema requires `condition`, so reaching here means a hand-crafted
124+
// `{ dialect, source: '' }` envelope or a stale pre-built package, and
125+
// neither earns a match-all.
126+
const f = celToFilter(r.condition);
127+
if (!f || isMatchAllCriteria(f)) {
128+
logger?.warn?.('[sharing-rule] skipped (missing or untranslatable CEL condition — never seeded as match-all) [experimental]', { rule: r.name, condition: r.condition });
129+
skipped += 1; continue;
123130
}
131+
const criteria: Record<string, unknown> = f;
124132
try {
125133
await ruleService.defineRule({
126134
name: r.name,

packages/plugins/plugin-sharing/src/index.ts

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,19 @@ export {
3030
} from './share-link-routes.js';
3131
export { TeamGraphService, expandPrincipal, type TeamGraphOptions } from './team-graph.js';
3232
export { BusinessUnitGraphService, type BusinessUnitGraphOptions } from './business-unit-graph.js';
33-
export { bindRuleHooks, unbindAllRuleHooks, SHARING_RULE_HOOK_PACKAGE } from './rule-hooks.js';
33+
export {
34+
bindRuleHooks,
35+
unbindAllRuleHooks,
36+
bindRuleCriteriaGuard,
37+
SHARING_RULE_HOOK_PACKAGE,
38+
RULE_CRITERIA_GUARD_PACKAGE,
39+
} from './rule-hooks.js';
40+
export {
41+
parseCriteria,
42+
isMatchAllCriteria,
43+
MATCH_ALL_CRITERIA_MESSAGE,
44+
SharingCriteriaValidationError,
45+
} from './rule-criteria.js';
3446
export {
3547
bindRuleProvenanceStamp,
3648
unbindRuleProvenanceStamp,

packages/plugins/plugin-sharing/src/objects/sys-sharing-rule.object.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,14 +136,23 @@ export const SysSharingRule = ObjectSchema.create({
136136

137137
criteria_json: Field.textarea({
138138
label: 'Criteria',
139+
// [#3896] Mandatory in substance, `required: false` in metadata: the
140+
// column is nullable in every already-deployed tenant (rows predating
141+
// this gate), and flipping it to `required` would only translate into a
142+
// destructive NOT NULL migration that those nulls block. The invariant
143+
// is enforced where it can also explain itself — `bindRuleCriteriaGuard`
144+
// fails the INSERT, and `defineRule` fails the API call.
139145
required: false,
140146
// Rendered as a visual criteria builder scoped to the selected object's
141147
// fields (dependsOn: object_name), storing the same JSON FilterCondition.
142148
// An "Edit as JSON" fallback keeps hand-authored / advanced filters
143149
// editable. Falls back to a textarea when the widget is unavailable.
144150
widget: 'filter-condition',
145151
dependsOn: ['object_name'],
146-
description: 'Which records to share. Leave empty to share every record of the object.',
152+
// Deliberately NOT "leave empty to share everything" (#3896): an empty
153+
// criteria never shared everything on purpose, it just failed open —
154+
// ADR-0049 forbids the shape, and a rule saved without one is rejected.
155+
description: 'Which records to share. Required — a rule must narrow the records it shares, so there is no "share every record" setting.',
147156
group: 'Target',
148157
}),
149158

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Sharing-rule criteria normalisation + the ADR-0049 match-all guard (#3896).
5+
*
6+
* A sharing rule's `criteria` is the ONLY thing standing between a recipient
7+
* and every row of the target object: the evaluator feeds it straight to
8+
* `engine.find(object, { filter, context: SYSTEM_CTX })`. An absent or empty
9+
* predicate is therefore not "no constraint" — it is **share everything**,
10+
* which is exactly the shape `SharingRuleSchema` forbids:
11+
*
12+
* > A `condition` the compiler cannot lower … is skipped and logged — never
13+
* > seeded as a permissive match-all (ADR-0049).
14+
*
15+
* The seed path honoured that; `defineRule` (and through it
16+
* `POST {basePath}/sharing/rules`) and direct `sys_sharing_rule` writes did
17+
* not — a missing, `null`, or misspelled (`criterias`) key was stored as
18+
* `criteria_json: null` and evaluated as `{}`. These helpers are the single
19+
* definition of "this predicate constrains nothing", used by every authoring
20+
* entry (reject) and by the evaluator itself (match nothing, log) so rows
21+
* written before the gate existed cannot over-share either.
22+
*/
23+
24+
/**
25+
* Normalise a stored / submitted criteria value into an engine
26+
* `FilterCondition`, or `undefined` when it carries no usable predicate.
27+
*
28+
* A string is JSON-parsed; an unparsable one (most likely a CEL source typed
29+
* into the Setup textarea, which the v1 evaluator does not lower) yields
30+
* `undefined` rather than being passed through — the engine would ignore it
31+
* and match every row.
32+
*/
33+
export function parseCriteria(raw: unknown): unknown | undefined {
34+
if (raw == null || raw === '') return undefined;
35+
if (typeof raw === 'string') {
36+
const trimmed = raw.trim();
37+
if (!trimmed) return undefined;
38+
try {
39+
return JSON.parse(trimmed);
40+
} catch {
41+
// Treat unparsable strings as opaque — most likely a CEL source
42+
// that v1's evaluator doesn't grok yet; rule will match nothing.
43+
return undefined;
44+
}
45+
}
46+
return raw;
47+
}
48+
49+
/**
50+
* Does this criteria select EVERY record of its object?
51+
*
52+
* True for the shapes that reach the engine as an unconstrained filter:
53+
* missing / `null` / `''` / unparsable JSON / `{}` / `[]` / a non-object
54+
* scalar, and the empty or vacuous boolean combinators (`{ $and: [] }`,
55+
* `{ $or: [ {} ] }`). Any concrete field predicate makes it false.
56+
*
57+
* Conservative by design: it answers "could this fail open?", so an
58+
* ambiguous shape counts as match-all. Both consequences of a false positive
59+
* are safe — an authoring call is rejected with a message naming the field,
60+
* and an already-stored rule under-shares instead of over-sharing.
61+
*/
62+
export function isMatchAllCriteria(raw: unknown): boolean {
63+
const parsed = parseCriteria(raw);
64+
if (parsed == null) return true;
65+
// An array is the implicit-AND condition list: empty (or all-vacuous)
66+
// constrains nothing.
67+
if (Array.isArray(parsed)) return parsed.every((c) => isMatchAllCriteria(c));
68+
// A scalar (`true`, a number, …) is not a predicate the engine narrows on.
69+
if (typeof parsed !== 'object') return true;
70+
71+
const entries = Object.entries(parsed as Record<string, unknown>);
72+
if (entries.length === 0) return true;
73+
for (const [key, value] of entries) {
74+
if (key === '$and') {
75+
// AND of nothing / of vacuous branches ⇒ still everything.
76+
if (!Array.isArray(value) || value.every((c) => isMatchAllCriteria(c))) continue;
77+
return false;
78+
}
79+
if (key === '$or') {
80+
// OR is match-all as soon as ONE branch is; an empty OR has no
81+
// portable meaning, so treat it as unconstrained too.
82+
if (!Array.isArray(value) || value.length === 0 || value.some((c) => isMatchAllCriteria(c))) continue;
83+
return false;
84+
}
85+
// A named field predicate — this narrows the result set.
86+
return false;
87+
}
88+
return true;
89+
}
90+
91+
/**
92+
* The authoring rejection message. Names the offending shape rather than just
93+
* "required": the reported trigger (#3896) was a typo — `criterias` instead of
94+
* `criteria` — which a bare "field missing" would not explain.
95+
*/
96+
export const MATCH_ALL_CRITERIA_MESSAGE =
97+
'criteria is required and must narrow the records it shares — missing, empty, ' +
98+
'or unparsable criteria would share every record of the object, which a sharing ' +
99+
'rule must never do (ADR-0049). Check for a misspelled key (e.g. `criterias`).';
100+
101+
/**
102+
* Field-level rejection for the data-API path.
103+
*
104+
* Structurally what `@objectstack/objectql`'s `ValidationError` is — REST maps
105+
* on `code === 'VALIDATION_FAILED' || name === 'ValidationError'` and forwards
106+
* `fields[]` — but declared locally so a security guard in a plugin never
107+
* depends on another package's build output at runtime.
108+
*/
109+
export class SharingCriteriaValidationError extends Error {
110+
readonly code = 'VALIDATION_FAILED';
111+
readonly fields: Array<{ field: string; code: string; message: string }>;
112+
constructor(field = 'criteria_json', message = MATCH_ALL_CRITERIA_MESSAGE) {
113+
super(message);
114+
this.name = 'ValidationError';
115+
this.fields = [{ field, code: 'required', message }];
116+
}
117+
}

packages/plugins/plugin-sharing/src/rule-hooks.ts

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import type { SharingRuleService } from './sharing-rule-service.js';
44
import type { SharingRuleRow } from '@objectstack/spec/contracts';
5+
import { isMatchAllCriteria, SharingCriteriaValidationError } from './rule-criteria.js';
56

67
const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const;
78

@@ -15,6 +16,12 @@ export const SHARING_RULE_HOOK_PACKAGE = 'plugin-sharing:rules';
1516
*/
1617
export const RULE_REBIND_TRIGGER_PACKAGE = 'plugin-sharing:rule-rebind';
1718

19+
/**
20+
* Package id for the `sys_sharing_rule` criteria guard (#3896). Separate from
21+
* both packages above so neither rebind can unregister it.
22+
*/
23+
export const RULE_CRITERIA_GUARD_PACKAGE = 'plugin-sharing:rule-criteria-guard';
24+
1825
interface MinimalEngine {
1926
registerHook(event: string, handler: (ctx: any) => any | Promise<any>, options?: {
2027
object?: string | string[];
@@ -70,3 +77,40 @@ export function bindRuleHooks(
7077
export function unbindAllRuleHooks(engine: MinimalEngine): number {
7178
return engine.unregisterHooksByPackage(SHARING_RULE_HOOK_PACKAGE);
7279
}
80+
81+
/**
82+
* [#3896] Reject `sys_sharing_rule` writes whose criteria would share every
83+
* record of the target object.
84+
*
85+
* `SharingRuleService.defineRule` gates the programmatic + REST
86+
* (`POST {basePath}/sharing/rules`) entries, but authoring a rule in Setup is
87+
* a plain data INSERT on this table — it never reaches that method. Without
88+
* this hook the UI path keeps producing rules the evaluator now refuses to
89+
* act on: safe, but silently inert, which is its own authoring trap
90+
* (ADR-0078). Failing the write instead tells the admin the criteria is
91+
* missing while they are still looking at the form.
92+
*
93+
* Update semantics are deliberately narrower than insert: only a patch that
94+
* SUPPLIES `criteria_json` is checked. An existing row left over from before
95+
* this guard has a null criteria, and an admin must still be able to
96+
* `active: false` it — demanding a criteria to switch off an over-broad rule
97+
* would be exactly backwards.
98+
*/
99+
export function bindRuleCriteriaGuard(engine: MinimalEngine, logger?: MinimalLogger): void {
100+
if (typeof engine.registerHook !== 'function') return;
101+
if (typeof engine.unregisterHooksByPackage === 'function') {
102+
engine.unregisterHooksByPackage(RULE_CRITERIA_GUARD_PACKAGE);
103+
}
104+
const guard = (insert: boolean) => (ctx: any) => {
105+
const data = ctx?.input?.data;
106+
if (!data || typeof data !== 'object' || Array.isArray(data)) return;
107+
const supplied = Object.prototype.hasOwnProperty.call(data, 'criteria_json');
108+
if (!insert && !supplied) return;
109+
if (!isMatchAllCriteria(data.criteria_json)) return;
110+
throw new SharingCriteriaValidationError();
111+
};
112+
const opts = { object: 'sys_sharing_rule', packageId: RULE_CRITERIA_GUARD_PACKAGE, priority: 100 };
113+
engine.registerHook('beforeInsert', guard(true), opts);
114+
engine.registerHook('beforeUpdate', guard(false), opts);
115+
logger?.info?.('[sharing-rule] criteria guard bound on sys_sharing_rule (ADR-0049)');
116+
}

0 commit comments

Comments
 (0)