From 2733bb6ded8b6563432758db19ad2f8fb047c42d Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Fri, 5 Jun 2026 08:20:41 +0800 Subject: [PATCH] fix(security): close four P0 launch-readiness findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P0-1 plugin-auth: generateSecret() throws (fails boot) when OS_AUTH_SECRET is unset and NODE_ENV==='production' instead of using a predictable dev-secret- (session forgery). Dev/test fallback unchanged. P0-2 plugin-security: permission-resolution catch now FAILS CLOSED — logs ERROR and throws PermissionDeniedError rather than return next(), so a degraded metadata service can't bypass RBAC/RLS. System ops still bypass. P0-3 driver-sql: contains/$contains escapes LIKE metacharacters (% _ \) and binds explicit ESCAPE '\' (SQLite needs it) so '%' matches literally, not every row (filter bypass). P0-4 driver-mongodb: field-operator translator rejects unknown $-operators instead of passing them through, blocking $where/$function/$expr (server-side JS execution / query-intent bypass). Each finding was hand-verified against main first. +12 regression tests across the four packages. docs/launch-readiness.md updated: P0-1..4 marked Verify ✅ (fixed), Sign-off left for the team. Co-Authored-By: Claude Opus 4.8 --- .changeset/p0-security-hardening.md | 27 ++++++++++ docs/launch-readiness.md | 8 +-- .../driver-mongodb/src/mongodb-filter.test.ts | 18 +++++++ .../driver-mongodb/src/mongodb-filter.ts | 7 ++- .../src/sql-driver-like-escape.test.ts | 51 +++++++++++++++++++ packages/plugins/driver-sql/src/sql-driver.ts | 18 ++++++- .../plugin-auth/src/auth-manager.test.ts | 35 +++++++++++++ .../plugins/plugin-auth/src/auth-manager.ts | 33 +++++++----- .../src/security-plugin.test.ts | 42 +++++++++++++-- .../plugin-security/src/security-plugin.ts | 19 +++++-- 10 files changed, 228 insertions(+), 30 deletions(-) create mode 100644 .changeset/p0-security-hardening.md create mode 100644 packages/plugins/driver-sql/src/sql-driver-like-escape.test.ts diff --git a/.changeset/p0-security-hardening.md b/.changeset/p0-security-hardening.md new file mode 100644 index 000000000..911f66433 --- /dev/null +++ b/.changeset/p0-security-hardening.md @@ -0,0 +1,27 @@ +--- +'@objectstack/plugin-auth': patch +'@objectstack/plugin-security': patch +'@objectstack/driver-sql': patch +'@objectstack/driver-mongodb': patch +--- + +fix(security): close four P0 launch-readiness findings + +- **plugin-auth (P0-1):** `generateSecret()` now throws (fails boot) when no + `OS_AUTH_SECRET` is set and `NODE_ENV==='production'`, instead of silently + falling back to a predictable `dev-secret-` (session forgery). The + dev/test fallback is unchanged. +- **plugin-security (P0-2):** the permission-resolution `catch` now **fails + closed** — it logs at ERROR and throws `PermissionDeniedError` rather than + `return next()`. A degraded metadata service can no longer let every + authenticated request bypass RBAC/RLS. System operations still bypass as before. +- **driver-sql (P0-3):** the `contains` / `$contains` operator now escapes LIKE + metacharacters (`%` / `_` / `\`) in the user value and binds an explicit + `ESCAPE '\'`, so a value of `%` matches literally instead of every row + (filter bypass). Correct across SQLite/MySQL/Postgres. +- **driver-mongodb (P0-4):** the field-operator translator now rejects unknown + `$`-operators instead of passing them through, blocking `$where` / `$function` + / `$expr` (server-side JS execution / query-intent bypass). All legitimate + ObjectQL operators remain allowlisted. + ++12 regression tests across the four packages. diff --git a/docs/launch-readiness.md b/docs/launch-readiness.md index 83be39b7b..f2fb573ce 100644 --- a/docs/launch-readiness.md +++ b/docs/launch-readiness.md @@ -55,7 +55,7 @@ fix or acceptance.** - **Action:** Throw (fail boot) when no secret is configured and `NODE_ENV === 'production'`; add a pre-boot config validation. Document `OS_AUTH_SECRET` as a required go-live env var. -- **Owner:** _______ · Verify ☐ · Sign-off ☐ · Notes: _______ +- **Owner:** _______ · Verify ✅ (confirmed real @ `main`) · Sign-off ☐ · Notes: Fixed — `generateSecret()` throws in production; +3 tests. Awaiting human sign-off. ### P0-2 — Metadata-service failure bypasses all RBAC/RLS (fail-open) - **Area:** `plugin-security` — `src/security-plugin.ts:309–312` @@ -65,7 +65,7 @@ fix or acceptance.** - **Action:** Add a circuit-breaker + ERROR-level alerting; add an integration test asserting "metadata service down ⇒ request denied", not allowed. Decide fail-closed vs. fail-open explicitly and record the decision. -- **Owner:** _______ · Verify ☐ · Sign-off ☐ · Notes: _______ +- **Owner:** _______ · Verify ✅ (confirmed real @ `main`) · Sign-off ☐ · Notes: Decision = **fail-closed**. `catch` now logs ERROR + throws `PermissionDeniedError`; system ops still bypass. +2 tests. Awaiting human sign-off. ### P0-3 — Unescaped LIKE metacharacters in `contains` - **Area:** `driver-sql` — `src/sql-driver.ts:1565, 1656` @@ -75,7 +75,7 @@ fix or acceptance.** (HIGH, data) - **Action:** Escape `%` and `_` (and the escape char) before building the LIKE pattern; add a test with a `%`/`_` payload. -- **Owner:** _______ · Verify ☐ · Sign-off ☐ · Notes: _______ +- **Owner:** _______ · Verify ✅ (confirmed real @ `main`) · Sign-off ☐ · Notes: Fixed — escape `%`/`_`/`\` + explicit `ESCAPE '\'` (SQLite needs it); +3 tests. Awaiting human sign-off. ### P0-4 — MongoDB filter passes arbitrary `$` operators through - **Area:** `driver-mongodb` — `src/mongodb-filter.ts:82–84` @@ -85,7 +85,7 @@ fix or acceptance.** - **Action:** Allowlist safe operators (`$eq/$ne/$gt/$gte/$lt/$lte/$in/$nin/$and/$or/...`); reject unknown ones at filter-build time. If MongoDB is not a v1 driver, mark Roadmap instead. -- **Owner:** _______ · Verify ☐ · Sign-off ☐ · Notes: _______ +- **Owner:** _______ · Verify ✅ (confirmed real @ `main`) · Sign-off ☐ · Notes: Fixed — translator now rejects unknown `$`-operators (blocks `$where`/`$function`); +4 tests. Awaiting human sign-off. (Still confirm whether MongoDB is a v1 driver.) ### P0-5 — Realtime & feed are in-memory only (no cluster coordination) - **Area:** `service-realtime` (`in-memory-realtime-adapter.ts`), `service-feed` diff --git a/packages/plugins/driver-mongodb/src/mongodb-filter.test.ts b/packages/plugins/driver-mongodb/src/mongodb-filter.test.ts index 5db603903..a04db278b 100644 --- a/packages/plugins/driver-mongodb/src/mongodb-filter.test.ts +++ b/packages/plugins/driver-mongodb/src/mongodb-filter.test.ts @@ -180,4 +180,22 @@ describe('MongoDB Filter Translator', () => { }); }); }); + + describe('operator allowlist (P0-4)', () => { + it('rejects $where (server-side JS execution)', () => { + expect(() => translateFilter({ name: { $where: 'this.x == 1' } })).toThrow(/unsupported filter operator '\$where'/); + }); + + it('rejects $function', () => { + expect(() => translateFilter({ name: { $function: { body: 'fn', args: [], lang: 'js' } } })).toThrow(/unsupported filter operator/); + }); + + it('rejects unknown $-operators rather than passing them through', () => { + expect(() => translateFilter({ name: { $expr: 1 } })).toThrow(/unsupported filter operator '\$expr'/); + }); + + it('still accepts every allowlisted field operator', () => { + expect(() => translateFilter({ a: { $eq: 1 }, b: { $in: [1, 2] }, c: { $contains: 'x' }, d: { $exists: true } })).not.toThrow(); + }); + }); }); diff --git a/packages/plugins/driver-mongodb/src/mongodb-filter.ts b/packages/plugins/driver-mongodb/src/mongodb-filter.ts index 952590afd..d6a736fcb 100644 --- a/packages/plugins/driver-mongodb/src/mongodb-filter.ts +++ b/packages/plugins/driver-mongodb/src/mongodb-filter.ts @@ -165,8 +165,11 @@ function translateFieldOperators(ops: Record): Record { + let driver: SqlDriver; + let knex: any; + + beforeEach(async () => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + knex = (driver as any).knex; + await knex.schema.createTable('docs', (t: any) => { + t.string('id').primary(); + t.string('title'); + }); + await knex('docs').insert([ + { id: '1', title: '50% off sale' }, // literal % + { id: '2', title: 'plain title' }, // no metacharacter + { id: '3', title: 'a_b underscore' }, // literal _ + ]); + }); + + afterEach(async () => { + await knex.destroy(); + }); + + it('a "%" value matches only rows containing a literal %, not every row', async () => { + const r = await driver.find('docs', { where: { title: { $contains: '%' } } }); + expect(r.map((x: any) => x.id)).toEqual(['1']); + }); + + it('a "_" value matches only rows containing a literal _, not any single char', async () => { + const r = await driver.find('docs', { where: { title: { $contains: '_' } } }); + expect(r.map((x: any) => x.id)).toEqual(['3']); + }); + + it('an ordinary substring still matches normally', async () => { + const r = await driver.find('docs', { where: { title: { $contains: 'sale' } } }); + expect(r.map((x: any) => x.id)).toEqual(['1']); + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index 4a40b805d..e445ede96 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -1562,7 +1562,7 @@ export class SqlDriver implements IDataDriver { const methodNotIn = nextJoin === 'or' ? 'orWhereNotIn' : 'whereNotIn'; if (op === 'contains') { - b[method](field, 'like', `%${value}%`); + this.applyContainsLike(b, method, field, value); return; } @@ -1596,6 +1596,20 @@ export class SqlDriver implements IDataDriver { } } + /** + * Apply a `contains` substring match as a parameterized `LIKE '%…%'`, escaping + * the LIKE metacharacters `%` / `_` (and the escape char `\`) in the user value + * so they match literally instead of acting as wildcards — otherwise a value of + * `%` matches every row (a filter-bypass, P0). Binds an explicit `ESCAPE '\'` + * because SQLite does not honour a default escape character (MySQL/Postgres do, + * but the explicit clause is correct for all three). + */ + private applyContainsLike(builder: any, method: string, field: string, value: unknown): void { + const escaped = String(value).replace(/[\\%_]/g, '\\$&'); + const rawMethod = method.startsWith('or') ? 'orWhereRaw' : 'whereRaw'; + builder[rawMethod]('?? LIKE ? ESCAPE ?', [field, `%${escaped}%`, '\\']); + } + protected applyFilterCondition(builder: Knex.QueryBuilder, condition: any, logicalOp: 'and' | 'or' = 'and', tableHint?: string | null) { if (!condition || typeof condition !== 'object') return; const table = tableHint ?? this.tableNameForBuilder(builder); @@ -1653,7 +1667,7 @@ export class SqlDriver implements IDataDriver { break; } case '$contains': - (builder as any)[method](field, 'like', `%${opValue}%`); + this.applyContainsLike(builder, method, field, opValue); break; default: (builder as any)[method](field, coerced); diff --git a/packages/plugins/plugin-auth/src/auth-manager.test.ts b/packages/plugins/plugin-auth/src/auth-manager.test.ts index 9a56d010d..3a4b3d73d 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.test.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.test.ts @@ -1143,4 +1143,39 @@ describe('AuthManager', () => { expect(als.getStore()).toBeUndefined(); }); }); + + describe('generateSecret – production secret guard (P0-1)', () => { + const ENV_KEYS = ['NODE_ENV', 'OS_AUTH_SECRET', 'AUTH_SECRET', 'BETTER_AUTH_SECRET'] as const; + let saved: Record; + + beforeEach(() => { + saved = Object.fromEntries(ENV_KEYS.map((k) => [k, process.env[k]])); + for (const k of ['OS_AUTH_SECRET', 'AUTH_SECRET', 'BETTER_AUTH_SECRET']) delete process.env[k]; + }); + afterEach(() => { + for (const k of ENV_KEYS) { + if (saved[k] === undefined) delete process.env[k]; + else process.env[k] = saved[k]; + } + }); + + it('throws (fails boot) in production when no secret is configured', () => { + process.env.NODE_ENV = 'production'; + const m = new AuthManager({ baseUrl: 'http://localhost:3000' } as any); + expect(() => (m as any).generateSecret()).toThrow(/OS_AUTH_SECRET is required in production/); + }); + + it('falls back to an ephemeral dev secret outside production', () => { + process.env.NODE_ENV = 'development'; + const m = new AuthManager({ baseUrl: 'http://localhost:3000' } as any); + expect((m as any).generateSecret()).toMatch(/^dev-secret-/); + }); + + it('uses OS_AUTH_SECRET when set, even in production', () => { + process.env.NODE_ENV = 'production'; + process.env.OS_AUTH_SECRET = 'a-strong-production-secret-value'; + const m = new AuthManager({ baseUrl: 'http://localhost:3000' } as any); + expect((m as any).generateSecret()).toBe('a-strong-production-secret-value'); + }); + }); }); diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 358c4448c..66e4decde 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -1051,23 +1051,28 @@ export class AuthManager { */ private generateSecret(): string { const envSecret = readEnvWithDeprecation('OS_AUTH_SECRET', ['AUTH_SECRET', 'BETTER_AUTH_SECRET']); - - if (!envSecret) { - // In production, a secret MUST be provided - // For development/testing, we'll use a fallback but warn about it - const fallbackSecret = 'dev-secret-' + Date.now(); - - console.warn( - '⚠️ WARNING: No OS_AUTH_SECRET environment variable set! ' + - 'Using a temporary development secret. ' + - 'This is NOT secure for production use. ' + - 'Please set OS_AUTH_SECRET in your environment variables.' + if (envSecret) return envSecret; + + // No secret configured. In production this is FATAL: a predictable + // `dev-secret-` makes session tokens forgeable (session + // forgery). Refuse to boot rather than run insecurely. + if (process.env.NODE_ENV === 'production') { + throw new Error( + '[auth] OS_AUTH_SECRET is required in production but is not set. ' + + 'Refusing to boot with a temporary development secret — session tokens ' + + 'would be forgeable. Set OS_AUTH_SECRET to a strong random value.' ); - - return fallbackSecret; } - return envSecret; + // Development / test only: fall back to an ephemeral secret, loudly. + const fallbackSecret = 'dev-secret-' + Date.now(); + console.warn( + '⚠️ WARNING: No OS_AUTH_SECRET environment variable set! ' + + 'Using a temporary development secret. ' + + 'This is NOT secure for production use. ' + + 'Please set OS_AUTH_SECRET in your environment variables.' + ); + return fallbackSecret; } /** diff --git a/packages/plugins/plugin-security/src/security-plugin.test.ts b/packages/plugins/plugin-security/src/security-plugin.test.ts index 63326442c..6ae0d3c66 100644 --- a/packages/plugins/plugin-security/src/security-plugin.test.ts +++ b/packages/plugins/plugin-security/src/security-plugin.test.ts @@ -23,7 +23,7 @@ describe('SecurityPlugin', () => { const plugin = new SecurityPlugin(); const manifestService = { register: vi.fn() }; const ctx: any = { - logger: { info: vi.fn(), warn: vi.fn() }, + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, registerService: vi.fn(), getService: vi.fn().mockImplementation((name: string) => { if (name === 'manifest') return manifestService; @@ -41,7 +41,7 @@ describe('SecurityPlugin', () => { const plugin = new SecurityPlugin(); const manifestService = { register: vi.fn() }; const ctx: any = { - logger: { info: vi.fn(), warn: vi.fn() }, + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, registerService: vi.fn(), getService: vi.fn().mockImplementation((name: string) => { if (name === 'manifest') return manifestService; @@ -57,7 +57,7 @@ describe('SecurityPlugin', () => { const plugin = new SecurityPlugin(); const manifestService = { register: vi.fn() }; const ctx: any = { - logger: { info: vi.fn(), warn: vi.fn() }, + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, registerService: vi.fn(), getService: vi.fn().mockImplementation((name: string) => { if (name === 'manifest') return manifestService; @@ -74,7 +74,7 @@ describe('SecurityPlugin', () => { const registerMiddleware = vi.fn(); const manifestService = { register: vi.fn() }; const ctx: any = { - logger: { info: vi.fn(), warn: vi.fn() }, + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, registerService: vi.fn(), getService: vi.fn().mockImplementation((name: string) => { if (name === 'manifest') return manifestService; @@ -127,7 +127,7 @@ describe('SecurityPlugin', () => { services['org-scoping'] = { name: 'com.objectstack.org-scoping' }; } const ctx: any = { - logger: { info: vi.fn(), warn: vi.fn() }, + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, registerService: vi.fn(), getService: (name: string) => { if (!(name in services)) throw new Error(`service not registered: ${name}`); @@ -344,6 +344,38 @@ describe('SecurityPlugin', () => { }); }); + it('fails CLOSED when permission resolution throws — denies, never bypasses (P0-2)', async () => { + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + const harness = makeMiddlewareCtx({ permissionSets: [tenantPolicySet] }); + await plugin.init(harness.ctx); + await plugin.start(harness.ctx); + // Simulate the permission/metadata subsystem failing mid-resolution. + (plugin as any).permissionEvaluator = { + resolvePermissionSets: async () => { throw new Error('metadata service unavailable'); }, + }; + const opCtx: any = { + object: 'task', + operation: 'find', + data: {}, + context: { userId: 'u1', tenantId: 'org-1', roles: ['member'], permissions: [] }, + }; + // Resolution failed → the request must be DENIED, not waved through. + await expect(harness.run(opCtx)).rejects.toThrow(/permission subsystem unavailable/); + expect(harness.ctx.logger.error).toHaveBeenCalled(); + }); + + it('a system operation still bypasses security regardless (P0-2)', async () => { + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + const harness = makeMiddlewareCtx({ permissionSets: [tenantPolicySet] }); + await plugin.init(harness.ctx); + await plugin.start(harness.ctx); + (plugin as any).permissionEvaluator = { + resolvePermissionSets: async () => { throw new Error('should not be called'); }, + }; + const opCtx: any = { object: 'task', operation: 'find', context: { isSystem: true } }; + await expect(harness.run(opCtx)).resolves.toBeDefined(); // bypass short-circuits before resolution + }); + it('FLS write — multiple forbidden fields are all listed in the error', async () => { const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); const harness = makeMiddlewareCtx({ diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index e02916a40..9bdc3e32b 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -307,9 +307,22 @@ export class SecurityPlugin implements Plugin { permissionSets = fallback; } } catch (e) { - // If metadata service is misconfigured, log and continue without permission checks - // rather than blocking all operations - return next(); + // Fail CLOSED. A permission-resolution failure must DENY the request, + // never bypass the checks (that would let a degraded metadata service + // expose every tenant's data). System/bootstrap operations already + // short-circuited above (`opCtx.context?.isSystem`), so reaching here + // means an authenticated user request whose RBAC/RLS could not be + // resolved — deny it and alert. + ctx.logger.error( + `[security] permission resolution failed for operation '${opCtx.operation}' on ` + + `object '${opCtx.object}' (user ${opCtx.context?.userId ?? 'unknown'}) — ` + + `denying request (fail-closed)`, + e instanceof Error ? e : new Error(String(e)), + ); + throw new PermissionDeniedError( + `[Security] Access denied: permission subsystem unavailable for ` + + `operation '${opCtx.operation}' on object '${opCtx.object}'`, + ); } // 2. CRUD permission check