Skip to content

Commit 50b5e03

Browse files
fix(service-storage,plugin-audit): a write refusal on a parent the caller cannot read names nothing (#21769)
Fixes #21755 Clause-②: no ## What this changes Two parent-derived write gates refuse an update or a delete by a caller who neither wrote the row nor can edit its parent record: - the attachment gate on `sys_attachment` (`installAttachmentAccessHooks`, `@objectstack/service-storage`); - the comment gate on `sys_comment` (`installCommentAccessHooks`, `@objectstack/plugin-audit`). This is the sibling check the card asked for. It was measured to have the same shape, so it is fixed here too. The named refusal carries the parent record's object and id in its message, and the parent's object in the envelope. For a caller who cannot read the parent, the read door answers "not found" for the row itself, so the write door named what the read door hides. The by-id write pre-image check in plugin-security already refuses such a write first for every principal its row filter binds: an `org_member` under the shipped ownership floor. A principal it does not bind, such as a session outside the floor's domain, got the gate's named refusal instead. This follows triage's ruled direction (comment 5981792733): - **A caller who cannot read the parent** now gets the platform's not-visible refusal: `PERMISSION_DENIED`, 403, and the `record_access_denied` sentence from the shared Operation Message Catalog. That is the pre-image check's own answer. No parent identity appears in the message, `details`, `developerMessage` or the envelope `object`. The door fills in the route's object, as it does for the pre-image check. - **A caller who can read the parent but may not edit it** keeps the named refusal: `ATTACHMENT_DELETE_DENIED` for an attachment delete, and `RECORD_NOT_ACCESSIBLE` for an attachment update and for a comment update or delete. - **Who may write does not change.** The new branch only replaces a refusal the caller was already getting. The uploader and author shortcut and the parent-editor limb are untouched. The gate asks whether the caller can read the parent only for a row it is about to refuse. - **A comment whose thread names no record** (an unparseable `thread_id`) is read by nobody, because the read middleware drops it. A non-author's write on it now gets the not-visible refusal too, and the thread value is no longer echoed back. This is the same class in the same function. ### How "can the caller read the parent" is decided The gate uses the evaluator the read door already uses, not a second one: - **Attachment kit.** The read-visibility middleware's inline caller-scoped parent probe moves into `resolveReadableParentIds`, beside `attachmentParentOf` (the rule for a row that names no readable parent). The middleware and the write gate both call it. The middleware's behaviour is unchanged, and its tests pass unedited. - **Comment kit.** The write gate calls the existing shared `resolveReadableParentIds`, the evaluator the comment read middleware and the activity read gate already use. Both strip the operation-private keys from the caller's envelope, as the read probe always did. ### The refusal's producer plugin-security's `PermissionDeniedError` cannot be reached from either package: neither depends on plugin-security, and the spec contract was off-limits for this card. The sentence's producer can be reached, and it is the one used: `renderOperationMessage({ messageKey: 'record_access_denied' })` from `@objectstack/spec/system`, rendered through the same locale and deployment-override ladder. plugin-sharing and plugin-approvals already render their refusals this way. The envelope carries the pre-image check's standard code and status (`PERMISSION_DENIED`, 403, `name: 'PermissionDeniedError'`), so every door's existing permission-denied branch answers it. The door pin below compares the two answers field by field, so they cannot drift apart unnoticed. Both installers take an optional fourth argument, a lazily resolved i18n lookup, which each plugin wires in `start()`. The operator's half, the sentence naming the parent, is logged server-side at `warn`, as the pre-image check logs its own. ## The nonexistent-id comparison (triage's second pin) Measured at the REST data door on this head, for a session outside the floor's domain that holds the delete grants: | Write | Parent unreadable (this PR) | Id that does not exist | |---|---|---| | delete | 403 `PERMISSION_DENIED` | 404 `RECORD_NOT_FOUND` | | update | 403 `PERMISSION_DENIED` | 404 `RECORD_NOT_FOUND` | The two answers differ. The platform's write-door doctrine is the by-id write pre-image check. It answers a target that is gone and a target that is hidden with the same refusal: 403 `PERMISSION_DENIED` (`record_access_denied`). This PR gives the unreadable case that answer. The nonexistent id answers 404 for this principal class because the pre-image check does not bind it (no row filter applies), so the engine's not-found gate answers first. The `org_member` path shows the same split on update: 403 for an unreadable parent, 404 for an id that does not exist. It does not show it on delete, where both answer 403. Per the ruling, that residual split is class-level and outside this card, and it is reported to the dispatcher for its own card. It carries no parent identity. ## Tests (head `56d3cdfc`) - `pnpm --filter @objectstack/service-storage test`: 42 files, 650 tests passed. - `pnpm --filter @objectstack/plugin-audit test`: 39 files, 630 tests passed. - `pnpm --filter @objectstack/service-storage typecheck`, `pnpm --filter @objectstack/plugin-audit typecheck` (both include `check:test-typecheck: OK`) and `pnpm --filter @objectstack/dogfood typecheck`: all exit 0. - Dogfood, against the rebuilt `dist/` of both packages: the new door pin plus the six existing attachment and comment dogfood files. 7 files, 63 tests passed, 1 skipped. The skipped one is the existing cross-tenant block, gated on `organizationsAvailable`. **New pins** - **Unit, in each package**, in a block titled "a refusal on a parent the caller cannot read names nothing". It covers both verbs: - The full envelope: `code`, `status`, `statusCode`, `name`, and the message equal to the catalog sentence. - No `object`, `details` or `developerMessage` on the error, and no parent identity anywhere on it. - A reader who may not edit keeps the named code. - The uploader, author and parent editor still pass, and the read probe is never asked for them. - The probe is one caller-scoped engine read of the parent, without the operation-private keys. - A row that names no parent, the degraded mode with no sharing service, the caller's locale, a deployment override, and a failing i18n lookup. - The same cases through a wired `ObjectQL`. - **Door**, in the new `packages/qa/dogfood/test/parent-derived-write-refusal-not-visible.dogfood.test.ts`. It boots the same stack twice: once outside the floor's domain, and once org-bound as the reference. - It first proves each boot's principal domain and grants with `assertArmed`. - For each of {attachment, comment} × {delete, update}, it compares the outside caller's answer field by field with the pre-image check's answer to an `org_member`. - It asserts that the body contains neither the parent's object nor its id, and that the row survived. - A reader who may not edit keeps `ATTACHMENT_DELETE_DENIED` and `RECORD_NOT_ACCESSIBLE`. **Red before the fix.** Run against `dist/` built from the base: 4 failed, 2 passed. The failures were the four outside-caller cells, each answering the named refusal with the parent in the message and the envelope. The two reader cells passed. **Ablations.** Each mutation went through `scripts/ablation-replace.mjs` and put the named refusal back where the not-visible one now is. - **Attachment gate, unit:** 8 failed, 52 passed. Restore proved: the blob equals HEAD and `git diff HEAD` is empty. - **Comment gate, unit:** 8 failed, 44 passed. Restore proved the same way. - **Door, both gates mutated at once:** - After the mutated rebuild, `ablation-dist-preflight` found the marker in the built files of both packages. The door pin then failed 4 and passed 2. - The restore was proved by blob against HEAD. - After the rebuild, preflight `--absent` showed the marker gone from all built files and the tree clean. The door pin then passed 6 of 6. - The mutated service-storage build failed its DTS step with TS6133, because the mutation left a parameter unused. That happened after the JS was emitted, and the preflight proved the marker was in the JS the suite consumes. **Gates.** `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` derived 97 commands, and every one exited 0 on this head. Two of them, `check:skill-examples` and `check:dual-build-cjs-loads`, first answered PREREQUISITE NOT MET (exit 3) until the package dists they read were built. The `--ran` reconciliation: 97 derived, 97 run, 0 NOT-MEASURED, 0 UNRUN, with a recorded exit code for every command. **Lint, narrowed and declared.** `eslint --no-inline-config --format json` on the seven changed `.ts` files reported 7 files, 0 errors and 0 warnings. - **Population:** the `**/*.{ts,…}` and `packages/**/*.{ts,…}` blocks of `eslint.config.mjs` cover all seven, and all seven were reported, so none is ignored. - **Invariance:** the config never enables type-aware linting (no `parserOptions.project`), so this diff cannot move the verdict of a file it does not touch. The full `pnpm lint` is left to CI. **Fixture triage of existing cases.** - Cases whose subject is the named refusal or the edit verdict now declare the parent readable, and keep their codes. - The degraded-mode cases and the dangling-thread case pinned exactly the branch this changes, so their expected code moves to `PERMISSION_DENIED`. - The wired stub drivers learn a lone `$in` operator value. Every other operator value still throws. **No regression in the parent-editor delete alternate.** The alternate match that landed just before this keeps its pins green, unedited, and so does the attachments permission-matrix dogfood file. ## Docs and changeset - `content/docs/permissions/attachments-access.mdx`: the `PERMISSION_DENIED` row of the delete table said the refusal happens "before the attachment gate runs". That is now only true for the principals the pre-image check binds, so the row is rewritten. One sentence adds that an update follows the same rule. - `.changeset/21755-attachment-refusal-not-visible.md`: `patch` for both packages. ## Acceptance notes - **Error class.** `PermissionDeniedError` lives in plugin-security, so the two gates build its envelope from the standard code and the catalog sentence instead of throwing the class. A not-visible refusal producer in a lower shared package would let them throw the one class. Noted, not filed. Taker: none. --- _Generated by [Claude Code](https://claude.ai/code/session_011K3zqE8Pv1Evw5hc8tZCnN)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent 4331a6b commit 50b5e03

9 files changed

Lines changed: 1206 additions & 78 deletions

File tree

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
'@objectstack/service-storage': patch
3+
'@objectstack/plugin-audit': patch
4+
---
5+
6+
A write refusal on an attachment or a comment no longer names a parent record the caller cannot read (#21755).
7+
8+
Clause-②: no
9+
10+
- **What changed.** The attachment gate (`sys_attachment`, `@objectstack/service-storage`) and the comment gate (`sys_comment`, `@objectstack/plugin-audit`) refuse an update or a delete by a caller who neither wrote the row nor can edit its parent record. That refusal names the parent record. A caller who cannot read the parent now gets the platform's not-visible refusal instead. This is the answer the row-level write check gives the principals it covers: `PERMISSION_DENIED` (403), with the same localized `record_access_denied` sentence. It names neither the parent nor the row's link to it, in the message or in the envelope.
11+
- **What did not change.** A caller who can read the parent but may not edit it keeps the named refusal: `ATTACHMENT_DELETE_DENIED` for an attachment delete, and `RECORD_NOT_ACCESSIBLE` for an attachment update and for a comment update or delete. Who may update or delete is unchanged.
12+
- **A comment whose thread names no record** is read by nobody, so a non-author's write on it now gets the not-visible refusal too, and the thread value is not echoed back.
13+
- **Localization.** `installAttachmentAccessHooks` and `installCommentAccessHooks` accept an optional fourth argument: a lazily resolved i18n lookup. With it, the sentence honours a deployment's `errors.record_access_denied` override, as the row-level write check's sentence does. Without it, the built-in catalog still renders the caller's locale.

‎content/docs/permissions/attachments-access.mdx‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,11 @@ editable by design). A multi-delete requires *every* matched row to pass.
7171
| Code | Status | When |
7272
| --- | --- | --- |
7373
| `ATTACHMENT_DELETE_DENIED` | 403 | The caller can read the attachment, but is neither the uploader nor able to edit the parent record |
74-
| `PERMISSION_DENIED` | 403 | The caller cannot read the parent record, so the attachment is not visible to them; the delete is refused before the attachment gate runs, and the parent is not named |
74+
| `PERMISSION_DENIED` | 403 | The caller cannot read the parent record, so the attachment is not visible to them. This is the platform's not-visible refusal whichever layer gives it — the row-level write check that runs before the gate, or the gate itself for a caller that check does not cover — and it names neither the parent nor the attachment's link to it |
75+
76+
An update of another user's attachment follows the same rule — the uploader or
77+
a parent editor — and a caller who cannot read the parent gets the same
78+
not-visible refusal.
7579

7680
The platform baseline also ships a parent-blind row-level delete floor
7781
(`owner_only_deletes`: you may delete only the rows you created, for members

‎packages/plugins/plugin-audit/src/audit-plugin.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,16 @@ export class AuditPlugin implements Plugin {
272272
}
273273
},
274274
ctx.logger,
275+
// [#21755] The deployment's i18n lookup for the not-visible refusal's
276+
// sentence — resolved per refusal (ADR-0029 D8: i18n may register
277+
// after this plugin), the override address plugin-security's own
278+
// not-visible refusal renders through.
279+
() => {
280+
const i18n = ctx.getService<II18nService>('i18n');
281+
const t = i18n?.t;
282+
if (typeof t !== 'function') return undefined;
283+
return (key: string, loc: string, params?: Record<string, unknown>) => t.call(i18n, key, loc, params);
284+
},
275285
);
276286
if (typeof (engine as any).registerMiddleware === 'function') {
277287
installCommentReadVisibility(engine as any, ctx.logger);

‎packages/plugins/plugin-audit/src/comment-access-hooks.test.ts‎

Lines changed: 300 additions & 13 deletions
Large diffs are not rendered by default.

‎packages/plugins/plugin-audit/src/comment-access-hooks.ts‎

Lines changed: 113 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,11 @@
3434
* refusal reaches this handler through the `dispatchUnscopedMultiWrite`
3535
* whole-operation dispatch both registrations declare (#9719/commit c7655d472 built
3636
* it for delete; #9974 ruled it onto update).
37+
* * Both write verbs refuse a caller who cannot READ the comment's parent —
38+
* or whose thread names no parent at all, which nobody reads — with the
39+
* platform's not-visible refusal instead of the named one (#21755); see
40+
* {@link refuseNotVisible}. The named refusal is what a caller who can
41+
* read the parent, but may not edit it, receives.
3742
* - {@link installCommentReadVisibility} — the read side: a
3843
* `find`/`findOne`/`count`/`aggregate` middleware that intersects the query
3944
* with the threads whose parent record the caller can actually read.
@@ -61,8 +66,10 @@
6166
*/
6267

6368
import { withoutOperationPrivateKeys } from '@objectstack/core';
69+
import type { StandardErrorCode } from '@objectstack/spec/api';
6470
import type { ISharingService } from '@objectstack/spec/contracts';
6571
import type { ExecutionContext } from '@objectstack/spec/kernel';
72+
import { renderOperationMessage, type ValidationMessageTranslator } from '@objectstack/spec/system';
6673

6774
/** Minimal engine surface these installers need — duck-typed (like
6875
* service-storage's attachment seams) so tests can fake it and so plugin-audit
@@ -189,6 +196,61 @@ function forbid(message: string, object?: string): never {
189196
throw err;
190197
}
191198

199+
/**
200+
* The code of the platform's not-visible refusal: the one plugin-security's
201+
* by-id write pre-image check throws when the caller's own read visibility
202+
* does not reach the target row (`PermissionDeniedError`, 403).
203+
*/
204+
const NOT_VISIBLE_CODE: StandardErrorCode = 'PERMISSION_DENIED';
205+
const NOT_VISIBLE_STATUS = 403;
206+
207+
/**
208+
* [#21755] Refuse a write on a comment whose parent record the caller cannot
209+
* READ — with the platform's not-visible refusal, never the gate's named one.
210+
*
211+
* The named refusal ({@link forbid}) names the parent record (`object/id`) in
212+
* the message, or the raw `thread_id` of a thread that names none, and the
213+
* parent's object on the envelope. That is honest to a caller who can read the
214+
* parent and may not edit it; to a caller who cannot, it is a disclosure across
215+
* the read boundary — the read door answers that same caller "not found" for
216+
* the comment, because a comment's visibility IS its parent's
217+
* ({@link installCommentReadVisibility}, which also hides every thread naming
218+
* no parent). The by-id write pre-image check already refuses such a write
219+
* before this gate runs for every principal its row filter binds; this is the
220+
* same answer for the principals it does not bind.
221+
*
222+
* The same answer, not a lookalike: the sentence is rendered by the shared
223+
* Operation Message Catalog under the pre-image check's own key
224+
* (`record_access_denied`, which names nothing), through the same
225+
* locale/override ladder, and the envelope carries the pre-image check's code
226+
* and status. Nothing about the parent rides on it — no `object` (the doors
227+
* fill the ROUTE's object, exactly as for the pre-image check), no `details`,
228+
* no `developerMessage`. The operator's half is logged by the caller instead.
229+
*
230+
* Who may write does not change: this replaces the refusal a caller was
231+
* already getting, on exactly the rows that were already refused.
232+
*/
233+
function refuseNotVisible(
234+
callerCtx: ExecutionContext,
235+
messageTranslator: (() => ValidationMessageTranslator | undefined) | undefined,
236+
): never {
237+
let translate: ValidationMessageTranslator | undefined;
238+
try {
239+
translate = messageTranslator?.();
240+
} catch {
241+
// i18n is optional and late-bound; the built-in catalog still renders the
242+
// caller's locale without it.
243+
translate = undefined;
244+
}
245+
const locale = typeof callerCtx?.locale === 'string' ? callerCtx.locale : undefined;
246+
const err: any = new Error(renderOperationMessage({ messageKey: 'record_access_denied' }, { locale, translate }));
247+
err.name = 'PermissionDeniedError';
248+
err.code = NOT_VISIBLE_CODE;
249+
err.status = NOT_VISIBLE_STATUS;
250+
err.statusCode = NOT_VISIBLE_STATUS;
251+
throw err;
252+
}
253+
192254
function asIdList(id: unknown): Array<string | number> | null {
193255
if (typeof id === 'string' || typeof id === 'number') return [id];
194256
if (id && typeof id === 'object' && Array.isArray((id as any).$in)) {
@@ -295,11 +357,18 @@ async function callerCanRead(ctx: any, target: CommentThreadTarget): Promise<boo
295357
* order does not matter, and returns `null` on a deployment without it — in
296358
* which case the edit checks degrade to caller-scoped parent READ visibility,
297359
* still strictly tighter than no gate at all.
360+
*
361+
* `messageTranslator` resolves the deployment's i18n lookup for the
362+
* not-visible refusal's sentence ({@link refuseNotVisible}) — lazily, per
363+
* refusal, because the i18n service is contributed by another plugin that may
364+
* start after this one. Absent, the built-in catalog still renders the
365+
* caller's locale.
298366
*/
299367
export function installCommentAccessHooks(
300368
engine: CommentAccessEngine,
301369
getSharing: () => CommentSharingLike | null | undefined,
302370
logger: CommentAccessLogger,
371+
messageTranslator?: () => ValidationMessageTranslator | undefined,
303372
): void {
304373
/** May the caller EDIT the parent record behind `target`? Sharing's
305374
* `canEdit` when the service is present, else caller-scoped parent read
@@ -412,16 +481,52 @@ export function installCommentAccessHooks(
412481
verb: 'update' | 'delete',
413482
): Promise<void> => {
414483
const userId = ctx.session.userId as string | undefined;
484+
const callerCtx = callerContext(ctx);
415485
const canEditCache = new Map<string, boolean>();
486+
/** Parent READ verdicts, asked only for a row about to be refused. */
487+
const canReadCache = new Map<string, boolean>();
488+
/** [#21755] Can the caller READ `target`? Decided by the same evaluator
489+
* the read door uses ({@link resolveReadableParentIds}), so the write door
490+
* never names what the read door hides. */
491+
const callerReadsParent = async (target: CommentThreadTarget): Promise<boolean> => {
492+
const cacheKey = `${target.object}:${target.recordId}`;
493+
let canRead = canReadCache.get(cacheKey);
494+
if (canRead === undefined) {
495+
canRead = !!(await resolveReadableParentIds(engine, callerCtx, new Map([[target.object, new Set([target.recordId])]])))
496+
.get(target.object)
497+
?.has(target.recordId);
498+
canReadCache.set(cacheKey, canRead);
499+
}
500+
return canRead;
501+
};
502+
/** [#21755] Answer the platform's not-visible refusal; the operator's half
503+
* — the sentence naming the parent or the thread — stays server-side. */
504+
// A function DECLARATION, not an arrow: TypeScript narrows on a call to a
505+
// `never`-returning function only when its type is declared, and the
506+
// dangling-thread branch below relies on that narrowing.
507+
function answerNotVisible(row: Record<string, unknown>, namedMessage: string): never {
508+
logger.warn(`[audit] comment access: ${namedMessage} (the caller cannot read the parent; answered not-visible)`, {
509+
operation: verb,
510+
object: 'sys_comment',
511+
recordId: row.id,
512+
userId: userId ?? 'unknown',
513+
});
514+
return refuseNotVisible(callerCtx, messageTranslator);
515+
}
416516
for (const row of rows) {
417517
if (userId && row.author_id === userId) continue; // authors govern their own words
418518

419519
const threadId = row.thread_id;
420520
const target = parseCommentThreadId(threadId);
421521
if (!target) {
422522
// A dangling thread has no parent to inherit authority from, and the
423-
// caller is not the author → nobody but system may touch it.
424-
forbid(
523+
// caller is not the author → nobody but system may touch it. Nobody
524+
// READS it either (the read middleware drops every thread naming no
525+
// record), so the refusal is the not-visible one: naming the thread
526+
// would hand its raw value to a caller the read door answers "not
527+
// found" (#21755).
528+
answerNotVisible(
529+
row,
425530
`Cannot ${verb} comment ${row.id}: its thread ${JSON.stringify(threadId ?? null)} names no record, ` +
426531
'so only its author may modify it',
427532
);
@@ -433,11 +538,13 @@ export function installCommentAccessHooks(
433538
canEditCache.set(cacheKey, allowed);
434539
}
435540
if (!allowed) {
436-
forbid(
541+
const namedMessage =
437542
`Cannot ${verb} comment ${row.id}: only its author or a user who can edit the parent record ` +
438-
`(${target.object}/${target.recordId}) may ${verb} it`,
439-
target.object,
440-
);
543+
`(${target.object}/${target.recordId}) may ${verb} it`;
544+
// The named refusal is for a caller who can READ the parent and may
545+
// not edit it; one who cannot read it gets the not-visible refusal.
546+
if (!(await callerReadsParent(target))) answerNotVisible(row, namedMessage);
547+
forbid(namedMessage, target.object);
441548
}
442549
}
443550
};

0 commit comments

Comments
 (0)