Skip to content

Commit ff4957c

Browse files
committed
fix(security): address self-review findings on the requireAuth flip + destructive-op gates
Correctness (record-level enforcement for the pre-mapped destructive ops): - security-plugin middleware: extend the write-scope stash, the #1994 pre-image RLS check, and the ADR-0055 controlled-by-parent write gate to cover transfer/restore/purge — the object-level RBAC bit was gated but the row-level defenses only listed update/delete/insert, so a granted user could have destroyed out-of-scope rows by id when the M2 ops ship. purge maps onto the delete RLS class, transfer/restore onto update. New unit test proves a not-owned purge is denied by the pre-image check. Callsite coverage for the default flip: - cli serve: carve-out now keys on (tierEnabled('auth') || hasAuthPlugin) — a stack mounting AuthPlugin explicitly under a minimal tier no longer gets an accidental fail-open override. - plugin-dev: mirror the serve carve-out — an auth-less dev stack gets an explicit requireAuth:false so the local data API isn't bricked. - spec: declare ObjectStackDefinitionSchema.api so the documented opt-out (and the enableProjectScoping/projectResolution/enforceProjectMembership knobs serve.ts already reads) survives defineStack strict parsing instead of being silently stripped. Verified: defineStack now returns the api key. - rest-api-plugin: warn only on an explicit opt-out; add a misplaced-key guard for a flat api.requireAuth (silently ignored under the deny default). - fix two in-tree tests that passed requireAuth at the wrong nesting. Cleanup: - derive the modifyAllRecords write-bypass key set from OPERATION_TO_PERMISSION + DESTRUCTIVE_OPERATIONS (module-level Set, no per-call array) so a future destructive op is covered automatically. Docs / provenance: - regenerate the auto-gen references (requireAuth + allow*{Transfer,Restore, Purge} describe text); CHANGELOG entry for the breaking flip; changeset migration note updated to the defineStack-level api opt-out + scope note. Pre-existing anonymous-posture gap on /meta, dispatcher /graphql, and raw hono /data (surfaces that never call enforceAuth) filed as #2567 — out of scope for this flip. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014y5kiH3aPLWtRRRGcVrXcT
1 parent 8585975 commit ff4957c

14 files changed

Lines changed: 169 additions & 40 deletions

File tree

.changeset/require-auth-secure-by-default.md

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,18 +10,28 @@ feat(security)!: `api.requireAuth` now defaults to `true` — anonymous access t
1010
**BREAKING.** The global `requireAuth` default flipped FROM `false` TO `true`
1111
(`RestApiConfigSchema.requireAuth` in `@objectstack/spec`, mirrored by
1212
`RestServer.normalizeConfig` in `@objectstack/rest`). Anonymous requests to
13-
`/data/*` CRUD + batch endpoints are now rejected with HTTP 401 unless the
14-
deployment explicitly opts out.
13+
the `/data/*` CRUD + batch endpoints are now rejected with HTTP 401 unless the
14+
deployment explicitly opts out. (Scope note: this gate covers the REST
15+
`/data/*` surface — the metadata read/write endpoints and the dispatcher
16+
GraphQL route have their own pre-existing anonymous posture, tracked
17+
separately; this flip does not change them.)
1518

1619
**Migration (one line):** a deployment that intentionally serves data publicly
17-
(demo / playground / kiosk) must set the flag explicitly:
20+
(demo / playground / kiosk) sets the flag on the stack config — now a declared
21+
`ObjectStackDefinitionSchema.api` field, so it survives `defineStack` strict
22+
parsing (previously an undeclared top-level `api` key was silently stripped):
1823

1924
```ts
20-
api: { requireAuth: false }
25+
export default defineStack({
26+
//
27+
api: { requireAuth: false },
28+
});
2129
```
2230

2331
The REST plugin logs a boot warning for the explicit opt-out so a fail-open
24-
posture is always visible.
32+
posture is always visible. A misplaced `api.requireAuth` at the plugin level
33+
(one nesting short) is now also called out with a boot warning instead of
34+
being silently ignored.
2535

2636
**What keeps working with no action:**
2737

CHANGELOG.md

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Changed — Security: anonymous access denied by default (BREAKING) + destructive-op RBAC gates
11+
12+
- **`api.requireAuth` now defaults to `true`** (ADR-0056 D2 flip): anonymous
13+
requests to `/data/*` CRUD + batch endpoints are rejected with HTTP 401
14+
unless the deployment explicitly opts out with `api: { requireAuth: false }`
15+
(the REST plugin logs a boot warning for the explicit opt-out). Share-links,
16+
public forms (via the declaration-derived `publicFormGrant`), and the
17+
control plane (`/auth`, `/health`, `/discovery`) keep working with no
18+
action. `objectstack serve` passes an explicit `false` only for stacks with
19+
no `auth` tier (nothing could authenticate against them). Conformance row
20+
`requireAuth-default-flip``enforced`.
21+
- **`transfer` / `restore` / `purge` are RBAC-gated ahead of the ops** (#1883):
22+
the permission evaluator maps them to `allowTransfer` / `allowRestore` /
23+
`allowPurge` (with the `modifyAllRecords` super-user bypass), so when the
24+
M2 operations ship there is no ungated window. Unmapped destructive
25+
operations keep failing closed (ADR-0049).
26+
1027
### Added — ObjectUI one-month frontend scan + backend alignment summary
1128

1229
Scanning `../objectui` from 2026-05-08 through 2026-06-08 shows a broad Studio

content/docs/references/api/rest-server.mdx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,7 @@ const result = BatchEndpointsConfig.parse(data);
192192
| **enableOpenApi** | `boolean` || Enable OpenAPI 3.1 spec & docs viewer endpoints |
193193
| **enableProjectScoping** | `boolean` || Enable project-scoped routing for data/meta/AI APIs |
194194
| **projectResolution** | `Enum<'required' \| 'optional' \| 'auto'>` || Project ID resolution strategy |
195-
| **requireAuth** | `boolean` || Reject anonymous requests on /data/* with HTTP 401 |
195+
| **requireAuth** | `boolean` || Reject anonymous requests on /data/* with HTTP 401 (secure-by-default; set false to serve data publicly) |
196196
| **documentation** | `Object` | optional | OpenAPI/Swagger documentation config |
197197
| **responseFormat** | `Object` | optional | Response format options |
198198

content/docs/references/security/permission.mdx

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,9 +68,9 @@ const result = FieldPermission.parse(data);
6868
| **allowRead** | `boolean` || Read permission |
6969
| **allowEdit** | `boolean` || Edit permission |
7070
| **allowDelete** | `boolean` || Delete permission |
71-
| **allowTransfer** | `boolean` || [EXPERIMENTAL — not enforced] Change record ownership |
72-
| **allowRestore** | `boolean` || [EXPERIMENTAL — not enforced] Restore from trash (Undelete) |
73-
| **allowPurge** | `boolean` || [EXPERIMENTAL — not enforced] Permanently delete (Hard Delete/GDPR) |
71+
| **allowTransfer** | `boolean` || [RBAC-gated; operation pending M2] Change record ownership |
72+
| **allowRestore** | `boolean` || [RBAC-gated; operation pending M2] Restore from trash (Undelete) |
73+
| **allowPurge** | `boolean` || [RBAC-gated; operation pending M2] Permanently delete (Hard Delete/GDPR) |
7474
| **viewAllRecords** | `boolean` || View All Data (Bypass Sharing) |
7575
| **modifyAllRecords** | `boolean` || Modify All Data (Bypass Sharing) |
7676
| **readScope** | `Enum<'own' \| 'own_and_reports' \| 'unit' \| 'unit_and_below' \| 'org'>` | optional | [ADR-0057 D1] Read depth: own|unit|unit_and_below|org |

packages/cli/src/commands/serve.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1388,7 +1388,13 @@ export default class Serve extends Command {
13881388
// entirely; such auth-less playgrounds get an EXPLICIT `false`
13891389
// (fail-open), which the REST plugin surfaces with a boot warning.
13901390
// Apps can always override via stack `api.requireAuth`.
1391-
const requireAuth = apiConfig.requireAuth ?? (tierEnabled('auth') ? true : false);
1391+
// Auth availability = tier auto-registers it OR the stack mounts
1392+
// AuthPlugin explicitly (hasAuthPlugin, computed above). Keying only on
1393+
// the tier would hand an explicit fail-open to a stack that ships auth
1394+
// via `plugins:` under a minimal tier set — re-opening the very hole
1395+
// the flip closes. Only a stack with NO auth at all gets the carve-out.
1396+
const requireAuth = apiConfig.requireAuth
1397+
?? ((tierEnabled('auth') || hasAuthPlugin) ? true : false);
13921398

13931399
try {
13941400
const { createRestApiPlugin } = await import('@objectstack/rest');

packages/plugins/plugin-dev/src/dev-plugin.ts

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -528,6 +528,7 @@ export class DevPlugin implements Plugin {
528528
// as part of its manifest), so no separate child plugin is needed.
529529

530530
// 4. Auth Plugin
531+
let authMounted = false;
531532
if (enabled('auth')) {
532533
try {
533534
const { AuthPlugin } = await import('@objectstack/plugin-auth') as any;
@@ -536,6 +537,7 @@ export class DevPlugin implements Plugin {
536537
baseUrl: this.options.authBaseUrl,
537538
});
538539
this.childPlugins.push(authPlugin);
540+
authMounted = true;
539541
ctx.logger.info(' ✔ Auth plugin enabled (dev credentials)');
540542
} catch {
541543
ctx.logger.warn(' ✘ @objectstack/plugin-auth not installed — skipping auth');
@@ -601,9 +603,21 @@ export class DevPlugin implements Plugin {
601603
if (enabled('rest')) {
602604
try {
603605
const { createRestApiPlugin } = await import('@objectstack/rest') as any;
604-
const restPlugin = createRestApiPlugin();
606+
// Secure-by-default carve-out (mirrors `objectstack serve`, ADR-0056
607+
// D2): when NO auth mounted in this dev stack (service disabled or
608+
// plugin-auth not installed), nobody could ever authenticate — the
609+
// deny default would brick the local playground's data API. Pass an
610+
// EXPLICIT fail-open for that case; the REST plugin's boot warning
611+
// keeps the posture visible.
612+
const restPlugin = authMounted
613+
? createRestApiPlugin()
614+
: createRestApiPlugin({ api: { api: { requireAuth: false } } as any });
605615
this.childPlugins.push(restPlugin);
606-
ctx.logger.info(' ✔ REST API endpoints enabled (CRUD + metadata)');
616+
ctx.logger.info(
617+
authMounted
618+
? ' ✔ REST API endpoints enabled (CRUD + metadata)'
619+
: ' ✔ REST API endpoints enabled (CRUD + metadata; anonymous ALLOWED — no auth mounted)',
620+
);
607621
} catch {
608622
ctx.logger.debug(' ℹ @objectstack/rest not installed — skipping REST endpoints');
609623
}

packages/plugins/plugin-security/src/permission-evaluator.ts

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,22 @@ const OPERATION_TO_PERMISSION: Record<string, keyof ObjectPermission> = {
3636
*/
3737
const DESTRUCTIVE_OPERATIONS = new Set<string>(['transfer', 'restore', 'purge']);
3838

39+
/**
40+
* Permission keys covered by the `modifyAllRecords` super-user WRITE bypass:
41+
* edit/delete plus the destructive lifecycle class, DERIVED from the two
42+
* constants above so a future destructive op added to the map+set is covered
43+
* automatically (hand-listing it inline is how bypass gaps happen — #1883).
44+
* NOTE this means "Modify All Data" grants (incl. the wildcard on
45+
* organization_admin / admin_full_access defaults) will cover
46+
* transfer/restore/purge the moment the M2 ops ship — Salesforce semantics,
47+
* confirmed in the #1883 disposition; revisit per-op when M2 lands.
48+
*/
49+
const MODIFY_ALL_WRITE_KEYS = new Set<keyof ObjectPermission>([
50+
'allowEdit',
51+
'allowDelete',
52+
...[...DESTRUCTIVE_OPERATIONS].map((op) => OPERATION_TO_PERMISSION[op]),
53+
]);
54+
3955
/**
4056
* [ADR-0066 D2] Resolve the object permission a permission set contributes for
4157
* `objectName`, honouring the secure-by-default posture:
@@ -92,13 +108,9 @@ export class PermissionEvaluator {
92108
// but a `private` object is excluded from a non-super-user wildcard.
93109
const objPerm = resolveObjectPermission(ps, objectName, opts.isPrivate ?? false);
94110
if (objPerm) {
95-
// Check if modifyAllRecords is set (super-user bypass for write ops,
96-
// including the destructive lifecycle class — transfer/restore/purge —
97-
// so a "Modify All Data" grant is not bricked by the #1883 gate)
98-
if (
99-
['allowEdit', 'allowDelete', 'allowTransfer', 'allowRestore', 'allowPurge'].includes(permKey) &&
100-
objPerm.modifyAllRecords
101-
) {
111+
// Super-user WRITE bypass ("Modify All Data") — covers edit/delete and
112+
// the destructive lifecycle class (see MODIFY_ALL_WRITE_KEYS).
113+
if (MODIFY_ALL_WRITE_KEYS.has(permKey) && objPerm.modifyAllRecords) {
102114
return true;
103115
}
104116
// Check if viewAllRecords is set (super-user bypass for read ops)

packages/plugins/plugin-security/src/security-plugin.test.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -292,6 +292,36 @@ describe('SecurityPlugin', () => {
292292
expect(harness.findOne).toHaveBeenCalledTimes(1);
293293
});
294294

295+
it('DENIES a purge of a not-owned row via the pre-image check (#1883 — destructive ops inherit row-level gating)', async () => {
296+
// The destructive lifecycle class (transfer/restore/purge) is pre-wired
297+
// into OPERATION_TO_PERMISSION, so it clears the object-level RBAC gate.
298+
// The record-level pre-image RLS check must therefore ALSO cover it —
299+
// otherwise a grant-holder could destroy out-of-scope rows by id. purge
300+
// maps onto the `delete` RLS class.
301+
const purgerSet: PermissionSet = {
302+
name: 'purger', label: 'Purger', isProfile: true,
303+
objects: { '*': { allowRead: true, allowPurge: true } },
304+
rowLevelSecurity: [
305+
{ name: 'owner_only_deletes', object: '*', operation: 'delete', using: 'created_by = current_user.id' },
306+
],
307+
} as any;
308+
const plugin = new SecurityPlugin({ fallbackPermissionSet: 'purger' });
309+
const harness = makeMiddlewareCtx({
310+
permissionSets: [purgerSet],
311+
objectFields: ownerFields,
312+
findOneImpl: () => null, // row exists but not owned → filtered out → deny
313+
});
314+
await plugin.init(harness.ctx);
315+
await plugin.start(harness.ctx);
316+
const opCtx: any = {
317+
object: 'task', operation: 'purge',
318+
options: { where: { id: 'r1' } },
319+
context: memberCtx,
320+
};
321+
await expect(harness.run(opCtx)).rejects.toMatchObject({ name: 'PermissionDeniedError' });
322+
expect(harness.findOne).toHaveBeenCalledTimes(1);
323+
});
324+
295325
it('SKIPS the check when no RLS policy applies (e.g. modifyAllRecords / admin) — no extra read', async () => {
296326
const adminSet: PermissionSet = {
297327
name: 'admin_full_access', label: 'Admin', isProfile: true,

packages/plugins/plugin-security/src/security-plugin.ts

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -433,7 +433,7 @@ export class SecurityPlugin implements Plugin {
433433
const sc: any = opCtx.context;
434434
if (['find', 'findOne', 'count', 'aggregate'].includes(opCtx.operation)) {
435435
sc.__readScope = this.permissionEvaluator.getEffectiveScope('read', opCtx.object, permissionSets, { isPrivate: secMeta.isPrivate });
436-
} else if (opCtx.operation === 'update' || opCtx.operation === 'delete') {
436+
} else if (['update', 'delete', 'transfer', 'restore', 'purge'].includes(opCtx.operation)) {
437437
sc.__writeScope = this.permissionEvaluator.getEffectiveScope('write', opCtx.object, permissionSets, { isPrivate: secMeta.isPrivate });
438438
}
439439
}
@@ -457,17 +457,28 @@ export class SecurityPlugin implements Plugin {
457457
// applies — e.g. an admin set with no RLS, or `modifyAllRecords`) the
458458
// check is skipped and behaviour is unchanged.
459459
if (
460-
(opCtx.operation === 'update' || opCtx.operation === 'delete') &&
460+
// update/delete today; transfer/restore/purge are pre-wired (#1883) so
461+
// the M2 ops inherit the pre-image check the moment they dispatch —
462+
// the CRUD bit alone must never be the only row-level defense.
463+
['update', 'delete', 'transfer', 'restore', 'purge'].includes(opCtx.operation) &&
461464
permissionSets.length > 0 &&
462465
!!opCtx.context?.userId &&
463466
this.ql
464467
) {
465468
const targetId = this.extractSingleId(opCtx);
466469
if (targetId != null) {
470+
// RLS policies declare select/insert/update/delete — map the
471+
// destructive lifecycle class onto its nearest write class so
472+
// authored policies apply (purge destroys like delete;
473+
// transfer/restore mutate like update).
474+
const rlsOperation =
475+
opCtx.operation === 'purge' ? 'delete'
476+
: opCtx.operation === 'transfer' || opCtx.operation === 'restore' ? 'update'
477+
: opCtx.operation;
467478
const writeFilter = await this.computeRlsFilter(
468479
permissionSets,
469480
opCtx.object,
470-
opCtx.operation,
481+
rlsOperation,
471482
opCtx.context,
472483
);
473484
if (writeFilter) {
@@ -504,7 +515,7 @@ export class SecurityPlugin implements Plugin {
504515
// authored RLS, so the #1994 pre-image check above is a no-op for it; this
505516
// closes the by-id write path by checking the master instead.
506517
if (
507-
(opCtx.operation === 'insert' || opCtx.operation === 'update' || opCtx.operation === 'delete') &&
518+
['insert', 'update', 'delete', 'transfer', 'restore', 'purge'].includes(opCtx.operation) &&
508519
permissionSets.length > 0 &&
509520
!!opCtx.context?.userId &&
510521
this.ql

packages/rest/src/rest-api-plugin.ts

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -215,20 +215,28 @@ export function createRestApiPlugin(config: RestApiPluginConfig = {}): Plugin {
215215
ctx.logger.info('REST API successfully registered');
216216

217217
// ADR-0056 D2 (warn → enforce, ENFORCED): the global default is
218-
// now secure-by-default — anonymous requests to /data/* are
219-
// denied unless the deployment explicitly opts out. The warning
220-
// remains for that explicit opt-out so a fail-open posture is
221-
// always visible in the boot log. (Reads the nested
222-
// `api.api.requireAuth` — the flat read here previously warned
223-
// even when requireAuth was on.)
224-
const effectiveRequireAuth = (config.api as any)?.api?.requireAuth ?? true;
225-
if (!effectiveRequireAuth) {
218+
// secure-by-default — anonymous /data/* is denied unless the
219+
// deployment explicitly opts out. The warning remains for that
220+
// explicit opt-out so a fail-open posture is always visible.
221+
if ((config.api as any)?.api?.requireAuth === false) {
226222
ctx.logger.warn(
227223
'[security] anonymous access to the data API is ALLOWED (api.requireAuth=false, explicit opt-out) — ' +
228224
'objects without OWD/RLS are world-readable. Remove the opt-out for secure-by-default and ' +
229225
'expose public records via share-links / publicSharing / public forms (ADR-0056 D2).',
230226
);
231227
}
228+
// Misplaced-key guard: the effective key is `api.api.requireAuth`
229+
// (RestApiPluginConfig.api is the full RestServerConfig). A flat
230+
// `api.requireAuth` is silently ignored by normalizeConfig — under
231+
// the deny default that turns an INTENDED public deployment into a
232+
// 401 outage with no diagnostic, so name the mistake loudly.
233+
if ((config.api as any)?.requireAuth !== undefined) {
234+
ctx.logger.warn(
235+
'[security] `api.requireAuth` is set at the WRONG nesting level and has NO effect — ' +
236+
'move it to `api.api.requireAuth` (RestServerConfig.api.requireAuth). ' +
237+
`The effective value this boot is ${(config.api as any)?.api?.requireAuth ?? true}.`,
238+
);
239+
}
232240
} catch (err: any) {
233241
ctx.logger.error('Failed to register REST API routes', { error: err.message } as any);
234242
throw err;

0 commit comments

Comments
 (0)