From d7c54e829c73365e824d1a2e69ef0bafdcdf18e5 Mon Sep 17 00:00:00 2001 From: Valerii Strilets Date: Sun, 6 Sep 2026 09:49:55 +0300 Subject: [PATCH] fix: reject prototype paths, fail closed on deny, and run unit tests in CI check() and ~any/~all only walk own properties, so inherited names such as `constructor` or `toString` can no longer resolve as rules. hydrate() skips `__proto__` keys from untrusted JSON. An empty subtree denies `~all`. Fastify and Elysia respond 403 when a custom onForbidden neither replies nor returns. Express and Node forward async setup/check errors to next(err). Add a Test workflow so pnpm test runs on every PR. Co-Authored-By: Claude Fable 5.1 --- .github/workflows/test.yml | 31 +++++++++++++++++++++ permix/src/core/check.ts | 27 ++++++++++++++----- permix/src/core/permix.test.ts | 39 +++++++++++++++++++++++++++ permix/src/core/rules.ts | 6 ++++- permix/src/elysia/permix.test.ts | 25 +++++++++++++++++ permix/src/elysia/permix.ts | 11 +++++++- permix/src/express/permix.test.ts | 25 +++++++++++++++++ permix/src/express/permix.ts | 45 ++++++++++++++++++------------- permix/src/fastify/permix.test.ts | 29 ++++++++++++++++++++ permix/src/fastify/permix.ts | 4 +++ permix/src/node/permix.test.ts | 17 ++++++++++++ permix/src/node/permix.ts | 45 ++++++++++++++++++------------- 12 files changed, 259 insertions(+), 45 deletions(-) create mode 100644 .github/workflows/test.yml diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 00000000..62cf2e79 --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,31 @@ +name: Test + +on: + pull_request: + types: [opened, synchronize] + branches: + - main + +jobs: + test: + runs-on: ubuntu-latest + + steps: + - name: Checkout code + uses: actions/checkout@v7 + with: + ref: ${{ github.event.pull_request.head.sha }} + + - name: Setup Node.js + uses: actions/setup-node@v7 + with: + node-version: 24 + + - name: Setup pnpm + uses: pnpm/action-setup@v6 + + - name: Install dependencies + run: pnpm install --frozen-lockfile + + - name: Test + run: pnpm test diff --git a/permix/src/core/check.ts b/permix/src/core/check.ts index cff802b9..caa11179 100644 --- a/permix/src/core/check.ts +++ b/permix/src/core/check.ts @@ -42,6 +42,14 @@ export function callRuleWithoutData(rule: () => unknown): boolean { } } +// Own-property lookup only, so paths like `post.constructor` or `toString` +// never resolve through the prototype chain. +function ownChild(parent: object, key: string): Rule | undefined { + return Object.hasOwn(parent, key) + ? (parent as Record)[key] + : undefined +} + function walk(rules: Rules, inputArgs: unknown[]): boolean { let args = inputArgs const first = args[0] @@ -51,11 +59,12 @@ function walk(rules: Rules, inputArgs: unknown[]): boolean { const last = parts.at(-1) if (isSpecialSymbol(last)) { - let subtree: Rule = rules + let subtree: Rule | undefined = rules for (let i = 0; i < parts.length - 1; i++) { - if (subtree && typeof subtree === 'object') { - subtree = (subtree as Record)[parts[i]] - } + subtree = + subtree && typeof subtree === 'object' + ? ownChild(subtree, parts[i]) + : undefined } if (subtree === undefined) { @@ -71,11 +80,15 @@ function walk(rules: Rules, inputArgs: unknown[]): boolean { if (typeof rule === 'function') { return void out.push(callRuleWithoutData(rule)) } - for (const key in rule) { + for (const key of Object.keys(rule)) { visit(rule[key]) } } visit(subtree) + // An empty subtree grants nothing, even for `~all`. + if (out.length === 0) { + return false + } return last === '~all' ? out.every(Boolean) : out.some(Boolean) } @@ -84,10 +97,10 @@ function walk(rules: Rules, inputArgs: unknown[]): boolean { } } - let rule: Rule = rules + let rule: Rule | undefined = rules let i = 0 for (; i < args.length && typeof rule === 'object'; i++) { - rule = rule[String(args[i])] + rule = ownChild(rule, String(args[i])) } if (typeof rule === 'boolean') { diff --git a/permix/src/core/permix.test.ts b/permix/src/core/permix.test.ts index aef3d742..1096b5b8 100644 --- a/permix/src/core/permix.test.ts +++ b/permix/src/core/permix.test.ts @@ -757,3 +757,42 @@ describe('deep rules', () => { }) }) }) + +describe('prototype safety', () => { + const permix = createPermix<{ + post: ['create'] + }>() + + permix.setup({ post: { create: true } }) + + it('should not resolve inherited names as rules', () => { + // @ts-expect-error not a defined path + expect(() => permix.check('toString')).toThrow(PermixRuleNotDefinedError) + // @ts-expect-error not a defined path + expect(() => permix.check('post.constructor')).toThrow( + PermixRuleNotDefinedError + ) + // @ts-expect-error not a defined path + expect(() => permix.check('constructor.~any')).toThrow( + PermixRuleNotDefinedError + ) + }) + + it('should ignore __proto__ keys when hydrating', () => { + const state = JSON.parse( + '{"__proto__":{"admin":true},"post":{"create":false}}' + ) + permix.hydrate(state) + + expect(permix.check('post.create')).toBe(false) + expect(Object.getPrototypeOf(permix.getRules())).toBe(Object.prototype) + expect((permix.getRules() as any).admin).toBeUndefined() + }) + + it('should deny ~all on an empty subtree', () => { + permix.hydrate({ post: {} } as any) + + expect(permix.check('post.~all')).toBe(false) + expect(permix.check('post.~any')).toBe(false) + }) +}) diff --git a/permix/src/core/rules.ts b/permix/src/core/rules.ts index 3ecbd78b..1856d3c8 100644 --- a/permix/src/core/rules.ts +++ b/permix/src/core/rules.ts @@ -60,7 +60,11 @@ export function hydrateRules( state: DehydratedState ): Rules { const result: Record = {} - for (const key in state as Record) { + for (const key of Object.keys(state)) { + // Untrusted JSON: assigning `__proto__` would swap the prototype. + if (key === '__proto__') { + continue + } const value = (state as Record)[key] result[key] = typeof value === 'boolean' diff --git a/permix/src/elysia/permix.test.ts b/permix/src/elysia/permix.test.ts index 10088806..e74d0a3d 100644 --- a/permix/src/elysia/permix.test.ts +++ b/permix/src/elysia/permix.test.ts @@ -334,3 +334,28 @@ describe('key exposure', () => { expect(permix.key).toBeTypeOf('symbol') }) }) + +describe('fail closed', () => { + it('should respond 403 when a custom onForbidden returns nothing', async () => { + const permix = createPermix({ + onForbidden: () => {}, + }) + + const app = new Elysia() + .onBeforeHandle( + permix.setupMiddleware({ + post: { create: false, read: false, update: false }, + user: { delete: false }, + }) + ) + .post('/posts', () => ({ success: true }), { + beforeHandle: permix.checkMiddleware('post.create'), + }) + + const res = await app.handle( + new Request('http://localhost/posts', { method: 'POST' }) + ) + expect(res.status).toBe(403) + await expect(res.json()).resolves.toStrictEqual({ error: 'Forbidden' }) + }) +}) diff --git a/permix/src/elysia/permix.ts b/permix/src/elysia/permix.ts index 1cc77df0..060bb20b 100644 --- a/permix/src/elysia/permix.ts +++ b/permix/src/elysia/permix.ts @@ -82,7 +82,16 @@ function buildPermix( const allowed = permix.check(...args) if (!allowed) { - return await onForbidden({ context, ...createCheckContext(...args) }) + const result = await onForbidden({ + context, + ...createCheckContext(...args), + }) + // Fail closed if a custom handler returned nothing. + if (result === undefined) { + context.set.status = 'Forbidden' + return { error: 'Forbidden' } + } + return result } } diff --git a/permix/src/express/permix.test.ts b/permix/src/express/permix.test.ts index 3ac31fb7..a2e886a4 100644 --- a/permix/src/express/permix.test.ts +++ b/permix/src/express/permix.test.ts @@ -497,3 +497,28 @@ describe('key exposure', () => { expect(permix.key).toBeTypeOf('symbol') }) }) + +describe('async errors', () => { + it('should forward a rejected setup callback to next(err)', async () => { + const permix = createPermix() + const app = express() + + app.use( + permix.setupMiddleware(async () => { + throw new Error('session lookup failed') + }) + ) + app.get('/', (_req, res) => { + res.json({ ok: true }) + }) + + const errorHandler: ErrorRequestHandler = (err, _req, res, _next) => { + res.status(500).json({ error: err.message }) + } + app.use(errorHandler) + + const response = await request(app).get('/') + expect(response.status).toBe(500) + expect(response.body).toStrictEqual({ error: 'session lookup failed' }) + }) +}) diff --git a/permix/src/express/permix.ts b/permix/src/express/permix.ts index ac25552b..2ab64fdf 100644 --- a/permix/src/express/permix.ts +++ b/permix/src/express/permix.ts @@ -60,15 +60,21 @@ function buildPermix( | Rules ): Handler { return async (req, res, next) => { - const rules = - typeof callbackOrRules === 'function' - ? await callbackOrRules({ req, res, next }) - : callbackOrRules - const instance = createPermixCore(rules) - instance.hook('check', (context) => { - hooks.callHook('check', context) - }) - ;(req as any)[resolveKey()] = instance + try { + const rules = + typeof callbackOrRules === 'function' + ? await callbackOrRules({ req, res, next }) + : callbackOrRules + const instance = createPermixCore(rules) + instance.hook('check', (context) => { + hooks.callHook('check', context) + }) + ;(req as any)[resolveKey()] = instance + } catch (error) { + // Express 4 does not catch async rejections; forward them. + next(error) + return + } next() } } @@ -83,15 +89,18 @@ function buildPermix( return } - const allowed = permix.check(...args) - - if (!allowed) { - await onForbidden({ - req, - res, - next, - ...createCheckContext(...args), - }) + try { + if (!permix.check(...args)) { + await onForbidden({ + req, + res, + next, + ...createCheckContext(...args), + }) + return + } + } catch (error) { + next(error) return } diff --git a/permix/src/fastify/permix.test.ts b/permix/src/fastify/permix.test.ts index 26f93dbc..9d7bbfa8 100644 --- a/permix/src/fastify/permix.test.ts +++ b/permix/src/fastify/permix.test.ts @@ -385,3 +385,32 @@ describe('key exposure', () => { expect(permix.key).toBeTypeOf('symbol') }) }) + +describe('fail closed', () => { + it('should send 403 when a custom onForbidden does not reply', async () => { + const permix = createPermix({ + onForbidden: () => {}, + }) + + const app = Fastify() + + await app.register( + permix.setupMiddleware({ + post: { create: false, read: false, update: false }, + user: { delete: false }, + }) + ) + + app.post( + '/posts', + { preHandler: permix.checkMiddleware('post.create') }, + (_req, reply) => { + reply.send({ success: true }) + } + ) + + const response = await app.inject({ method: 'POST', url: '/posts' }) + expect(response.statusCode).toBe(403) + expect(response.json()).toStrictEqual({ error: 'Forbidden' }) + }) +}) diff --git a/permix/src/fastify/permix.ts b/permix/src/fastify/permix.ts index 700a566d..ad2e7a43 100644 --- a/permix/src/fastify/permix.ts +++ b/permix/src/fastify/permix.ts @@ -110,6 +110,10 @@ function buildPermix( if (!allowed) { await onForbidden({ request, reply, ...createCheckContext(...args) }) + // Fail closed if a custom handler forgot to reply. + if (!reply.sent) { + reply.status(403).send({ error: 'Forbidden' }) + } } } diff --git a/permix/src/node/permix.test.ts b/permix/src/node/permix.test.ts index 18165c19..1c47af72 100644 --- a/permix/src/node/permix.test.ts +++ b/permix/src/node/permix.test.ts @@ -366,3 +366,20 @@ describe('key exposure', () => { expect(permix.key).toBeTypeOf('symbol') }) }) + +describe('async errors', () => { + it('should forward a rejected setup callback to next(err)', async () => { + const permix = createPermix() + const req = createMockRequest() + const res = createMockResponse() + const next = createMockNext() + const error = new Error('session lookup failed') + + await permix.setupMiddleware(async () => { + throw error + })(req, res, next) + + expect(next).toHaveBeenCalledWith(error) + expect(permix.get(req)).toBeNull() + }) +}) diff --git a/permix/src/node/permix.ts b/permix/src/node/permix.ts index 135c2020..91d93ad3 100644 --- a/permix/src/node/permix.ts +++ b/permix/src/node/permix.ts @@ -70,15 +70,21 @@ function buildPermix( | Rules ): Handler { return async (req, res, next) => { - const rules = - typeof callbackOrRules === 'function' - ? await callbackOrRules({ req, res, next }) - : callbackOrRules - const instance = createPermixCore(rules) - instance.hook('check', (context) => { - hooks.callHook('check', context) - }) - ;(req as any)[resolveKey()] = instance + try { + const rules = + typeof callbackOrRules === 'function' + ? await callbackOrRules({ req, res, next }) + : callbackOrRules + const instance = createPermixCore(rules) + instance.hook('check', (context) => { + hooks.callHook('check', context) + }) + ;(req as any)[resolveKey()] = instance + } catch (error) { + // Express 4 does not catch async rejections; forward them. + next(error) + return + } next() } } @@ -93,15 +99,18 @@ function buildPermix( return } - const allowed = permix.check(...args) - - if (!allowed) { - await onForbidden({ - req, - res, - next, - ...createCheckContext(...args), - }) + try { + if (!permix.check(...args)) { + await onForbidden({ + req, + res, + next, + ...createCheckContext(...args), + }) + return + } + } catch (error) { + next(error) return }