From d1b54f7dfe94183773ed43683395c77beb759640 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carl-Gerhard=20Lindesva=CC=88rd?= Date: Tue, 22 Sep 2026 21:31:44 +0200 Subject: [PATCH 1/5] fix(security): close webhook template sandbox escape and share password bypass Webhook JavaScript templates (GHSA-mc99-9jf5-22cq, GHSA-fmf9-23m7-xg84): the validator only refused 'constructor' when it was the callee of a member call or the target of an assignment. Reading it into a local, destructuring it, a sequence-expression callee, a call-of-call and a tagged template all walked past the checks and reached the Function constructor in the worker. The validator now refuses reading constructor/__proto__/prototype/caller/ callee through any member access or destructuring pattern, refuses dynamic computed keys (obj[expr]) so a forbidden name cannot be assembled at run time, refuses tagged templates, and only accepts identifier, member and inline-arrow callees. Share password cookie (GHSA-p6c2-mq9r-cx3r): the overview, dashboard and report share procedures and the db share-access validators unlocked a password-protected share whenever a cookie named shared-- existed, whatever its value. The cookie is now an HMAC over the share type, id and current password hash keyed by COOKIE_SECRET, verified with a constant-time compare, so it cannot be forged, cannot be replayed against another share, and expires when the password changes. Co-Authored-By: Claude Fable 5.1 --- packages/common/package.json | 3 +- packages/common/server/share-access.ts | 56 ++++++++++ packages/db/src/services/share.service.ts | 20 +++- packages/js-runtime/src/validate.test.ts | 101 +++++++++++++++++- packages/js-runtime/src/validate.ts | 120 ++++++++++++++++++++-- packages/trpc/src/routers/auth.ts | 27 +++-- packages/trpc/src/routers/share.test.ts | 59 ++++++++++- packages/trpc/src/routers/share.ts | 41 ++++++-- 8 files changed, 397 insertions(+), 30 deletions(-) create mode 100644 packages/common/server/share-access.ts diff --git a/packages/common/package.json b/packages/common/package.json index ec0ce3c01..f9772bc07 100644 --- a/packages/common/package.json +++ b/packages/common/package.json @@ -7,7 +7,8 @@ ".": "./index.ts", "./server": "./server/index.ts", "./server/get-client-ip": "./server/get-client-ip.ts", - "./server/safe-fetch": "./server/safe-fetch.ts" + "./server/safe-fetch": "./server/safe-fetch.ts", + "./server/share-access": "./server/share-access.ts" }, "scripts": { "test": "vitest", diff --git a/packages/common/server/share-access.ts b/packages/common/server/share-access.ts new file mode 100644 index 000000000..e160982d9 --- /dev/null +++ b/packages/common/server/share-access.ts @@ -0,0 +1,56 @@ +import { createHmac, timingSafeEqual } from 'node:crypto'; + +export type ShareType = 'overview' | 'dashboard' | 'report'; + +export interface ShareAccessInput { + type: ShareType; + /** The public share id (the value in the share link). */ + id: string; + /** The stored argon2 hash of the share password. */ + passwordHash: string; +} + +/** + * Proof that a viewer entered the password of a password-protected share. + * + * The cookie value is an HMAC over the share type, share id and the current + * password hash, so it cannot be forged without COOKIE_SECRET, cannot be + * replayed against a different share, and stops working when the owner + * changes or removes the password. Checking only that the cookie exists is + * what let anyone bypass the password (GHSA-p6c2-mq9r-cx3r). + */ +export function shareAccessCookieName(type: ShareType, id: string): string { + return `shared-${type}-${id}`; +} + +function getCookieSecret(): string { + const secret = process.env.COOKIE_SECRET; + if (!secret) { + throw new Error('COOKIE_SECRET environment variable is not set'); + } + return secret; +} + +export function createShareAccessToken({ + type, + id, + passwordHash, +}: ShareAccessInput): string { + return createHmac('sha256', getCookieSecret()) + .update(`share-access:${type}:${id}:${passwordHash}`) + .digest('base64url'); +} + +export function hasShareAccess( + cookies: Record | undefined, + share: ShareAccessInput, +): boolean { + const presented = cookies?.[shareAccessCookieName(share.type, share.id)]; + if (!presented) { + return false; + } + + const expected = Buffer.from(createShareAccessToken(share)); + const actual = Buffer.from(presented); + return actual.length === expected.length && timingSafeEqual(actual, expected); +} diff --git a/packages/db/src/services/share.service.ts b/packages/db/src/services/share.service.ts index 2e936991d..3a53f399c 100644 --- a/packages/db/src/services/share.service.ts +++ b/packages/db/src/services/share.service.ts @@ -1,3 +1,5 @@ +import { hasShareAccess } from '@openpanel/common/server/share-access'; + import { db } from '../prisma-client'; import { getProjectAccess } from './access.service'; @@ -159,7 +161,11 @@ export async function validateShareAccess( } // If password is set, require cookie OR member access - const hasCookie = !!ctx.cookies[`shared-dashboard-${shareId}`]; + const hasCookie = hasShareAccess(ctx.cookies, { + type: 'dashboard', + id: shareId, + passwordHash: dashboardShare.password, + }); const hasMemberAccess = ctx.session?.userId && (await getProjectAccess({ @@ -197,7 +203,11 @@ export async function validateShareAccess( } // If password is set, require cookie OR member access - const hasCookie = !!ctx.cookies[`shared-report-${shareId}`]; + const hasCookie = hasShareAccess(ctx.cookies, { + type: 'report', + id: shareId, + passwordHash: reportShare.password, + }); const hasMemberAccess = ctx.session?.userId && (await getProjectAccess({ @@ -246,7 +256,11 @@ export async function validateOverviewShareAccess( } // If password is set, require cookie OR member access - const hasCookie = !!ctx.cookies[`shared-overview-${shareId}`]; + const hasCookie = hasShareAccess(ctx.cookies, { + type: 'overview', + id: shareId, + passwordHash: share.password, + }); const hasMemberAccess = ctx.session?.userId && (await getProjectAccess({ diff --git a/packages/js-runtime/src/validate.test.ts b/packages/js-runtime/src/validate.test.ts index 2a4bb8cc7..2907fd0b0 100644 --- a/packages/js-runtime/src/validate.test.ts +++ b/packages/js-runtime/src/validate.test.ts @@ -145,7 +145,7 @@ describe('validate', () => { `(payload) => payload['constructor']['constructor']('return 1')()`, ); expect(result.valid).toBe(false); - expect(result.error).toContain('Computed property access'); + expect(result.error).toContain('not allowed'); }); it('should block a computed call target reached by optional chaining', () => { @@ -153,7 +153,7 @@ describe('validate', () => { `(payload) => payload?.['constructor']['constructor']('return 1')()`, ); expect(result.valid).toBe(false); - expect(result.error).toContain('Computed property access'); + expect(result.error).toContain('not allowed'); }); it('should block a computed key written with escape sequences', () => { @@ -161,7 +161,7 @@ describe('validate', () => { `(payload) => payload['\\u0063onstructor']['constructor']('return 1')()`, ); expect(result.valid).toBe(false); - expect(result.error).toContain('Computed property access'); + expect(result.error).toContain('not allowed'); }); it('should block a computed call target on a nested value', () => { @@ -193,6 +193,101 @@ describe('validate', () => { }); }); + describe('Reads that reach the Function constructor', () => { + // Every form here stores or forwards a reference instead of calling it + // as a member, which the call checks alone never see. See + // GHSA-mc99-9jf5-22cq / GHSA-fmf9-23m7-xg84. + it('should block reading .constructor into a local', () => { + const result = validate( + '(payload) => { const F = payload.constructor.constructor; return F; }', + ); + expect(result.valid).toBe(false); + expect(result.error).toContain("'constructor'"); + }); + + it('should block reading .constructor through optional chaining', () => { + const result = validate('(payload) => payload?.constructor'); + expect(result.valid).toBe(false); + expect(result.error).toContain("'constructor'"); + }); + + it('should block reading .constructor off a literal', () => { + const result = validate('(payload) => [].constructor'); + expect(result.valid).toBe(false); + expect(result.error).toContain("'constructor'"); + }); + + it('should block reading __proto__ and prototype', () => { + expect(validate('(payload) => payload.__proto__').valid).toBe(false); + expect(validate('(payload) => payload.name.prototype').valid).toBe(false); + }); + + it('should block destructuring a forbidden key', () => { + const result = validate( + '(payload) => { const { constructor: C } = payload; return C; }', + ); + expect(result.valid).toBe(false); + expect(result.error).toContain("Destructuring 'constructor'"); + }); + + it('should block destructuring with a computed key', () => { + const result = validate( + '(payload) => { const { [payload.k]: v } = payload; return v; }', + ); + expect(result.valid).toBe(false); + expect(result.error).toContain('Computed keys in destructuring'); + }); + + it('should block dynamic computed reads', () => { + const result = validate('(payload) => payload[payload.key]'); + expect(result.valid).toBe(false); + expect(result.error).toContain('Dynamic computed property access'); + }); + + it('should block a template literal key with substitutions', () => { + const result = validate('(payload) => payload[`${payload.key}`]'); + expect(result.valid).toBe(false); + expect(result.error).toContain('Dynamic computed property access'); + }); + + it('should block a sequence-expression callee', () => { + const result = validate('(payload) => (0, payload.name.toUpperCase)()'); + expect(result.valid).toBe(false); + expect(result.error).toContain('Calling the result of an expression'); + }); + + it('should block calling the result of a call', () => { + const result = validate('(payload) => JSON.parse(payload.raw)()'); + expect(result.valid).toBe(false); + expect(result.error).toContain('Calling the result of an expression'); + }); + + it('should block tagged template literals', () => { + const result = validate('(payload) => payload.name.trim`x`'); + expect(result.valid).toBe(false); + expect(result.error).toContain('Tagged template literals'); + }); + + it('should still allow literal keys and numeric indexes', () => { + const result = validate( + `(payload) => ({ a: payload['name'], b: payload.items[0], c: payload.items.at(-1) })`, + ); + expect(result.valid).toBe(true); + }); + + it('should still allow calling a local arrow function', () => { + const result = validate( + '(payload) => { const fmt = (v) => v.trim(); return fmt(payload.name); }', + ); + expect(result.valid).toBe(true); + }); + + it('should still allow an immediately invoked arrow function', () => { + const result = validate('(payload) => (() => payload.name)()'); + expect(result.valid).toBe(true); + }); + }); + describe('Prototype writes', () => { it('should block writing through __proto__', () => { const result = validate( diff --git a/packages/js-runtime/src/validate.ts b/packages/js-runtime/src/validate.ts index 5e1e96208..a3c5d7379 100644 --- a/packages/js-runtime/src/validate.ts +++ b/packages/js-runtime/src/validate.ts @@ -12,13 +12,28 @@ import { } from './ast-walker'; /** - * Property names that must never be written through. Assigning to any of - * these reaches objects shared with the rest of the worker process. + * Property names that must never be read or written through. Reading + * 'constructor' off any value walks up to the Function constructor, and + * assigning through '__proto__' or 'prototype' reaches objects shared with + * the rest of the worker process. Neither is ever needed by a template. */ -const FORBIDDEN_WRITE_PROPERTIES = new Set([ +const FORBIDDEN_PROPERTIES = new Set([ '__proto__', 'constructor', 'prototype', + 'caller', + 'callee', +]); + +/** + * Callee shapes a call expression may have. Anything else (a sequence + * expression, the result of another call, ...) hides what is being invoked. + */ +const ALLOWED_CALLEE_TYPES = new Set([ + 'Identifier', + 'MemberExpression', + 'OptionalMemberExpression', + 'ArrowFunctionExpression', ]); /** @@ -41,6 +56,9 @@ function staticPropertyName( if (prop.type === 'StringLiteral') { return prop.value as string; } + if (prop.type === 'NumericLiteral') { + return String(prop.value); + } if (prop.type === 'TemplateLiteral') { const expressions = prop.expressions as unknown[]; const quasis = prop.quasis as Record[]; @@ -54,6 +72,32 @@ function staticPropertyName( return prop.type === 'Identifier' ? (prop.name as string) : undefined; } +/** + * The static key of a property inside an object literal or destructuring + * pattern, or undefined when it is computed from an expression. + */ +function staticObjectPropertyKey( + prop: Record +): string | undefined { + const key = prop.key as Record | undefined; + if (!key) { + return undefined; + } + if (prop.computed) { + if (key.type === 'StringLiteral' || key.type === 'NumericLiteral') { + return String(key.value); + } + return undefined; + } + if (key.type === 'Identifier') { + return key.name as string; + } + if (key.type === 'StringLiteral' || key.type === 'NumericLiteral') { + return String(key.value); + } + return undefined; +} + /** * Walk an assignment target back down its member chain and return the first * forbidden property name it passes through, if any. payload.__proto__.x is a @@ -70,7 +114,7 @@ function forbiddenPropertyInChain( current.type === 'OptionalMemberExpression') ) { const name = staticPropertyName(current); - if (name && FORBIDDEN_WRITE_PROPERTIES.has(name)) { + if (name && FORBIDDEN_PROPERTIES.has(name)) { return name; } current = current.object as Record | undefined; @@ -271,6 +315,54 @@ export function validate(code: string): { } } + // Every property read is checked, not only the ones that are called or + // assigned. A read of 'constructor' can be stored in a local and invoked + // later, so the read itself is what has to be refused. A key that is + // only known at run time (obj[expr]) could be any of these names, so it + // is refused as well; literal keys and numeric indexes still work. + if ( + node.type === 'MemberExpression' || + node.type === 'OptionalMemberExpression' + ) { + const name = staticPropertyName(node); + if (node.computed && name === undefined) { + validationError = + 'Dynamic computed property access (obj[expr]) is not allowed. Use a literal key such as obj.key, obj["key"] or arr[0].'; + return; + } + if (name !== undefined && FORBIDDEN_PROPERTIES.has(name)) { + validationError = `Accessing '${name}' is not allowed.`; + return; + } + } + + // Destructuring is a read too: const { constructor: C } = payload. + if (node.type === 'ObjectPattern') { + const properties = node.properties as Record[]; + for (const prop of properties) { + if (prop.type !== 'ObjectProperty') { + continue; + } + const name = staticObjectPropertyKey(prop); + if (prop.computed && name === undefined) { + validationError = + 'Computed keys in destructuring patterns are not allowed.'; + return; + } + if (name !== undefined && FORBIDDEN_PROPERTIES.has(name)) { + validationError = `Destructuring '${name}' is not allowed.`; + return; + } + } + } + + // A tagged template (tag`...`) is a call that the call checks below + // never see. Templates have no use for it. + if (node.type === 'TaggedTemplateExpression') { + validationError = 'Tagged template literals are not allowed.'; + return; + } + // Check method calls on global objects (like Math.random, JSON.parse) // Handles both regular calls and optional chaining (?.) if ( @@ -282,6 +374,22 @@ export function validate(code: string): { callee.type === 'MemberExpression' || callee.type === 'OptionalMemberExpression'; + // @babel/parser emits an 'Import' callee for import(); keep the + // dedicated error for it. + if (callee.type === 'Import') { + validationError = 'Dynamic import() is not allowed'; + return; + } + + // The callee must name what is being invoked: a local or allowed + // global identifier, a member expression checked below, or an inline + // arrow function. (0, x)(...) and f()(...) are refused. + if (!ALLOWED_CALLEE_TYPES.has(callee.type as string)) { + validationError = + 'Calling the result of an expression is not allowed. Call a named function or method directly.'; + return; + } + if (isMemberExpr) { const obj = callee.object as Record; const prop = callee.property as Record; @@ -351,8 +459,8 @@ export function validate(code: string): { } // Block writes that reach the prototype chain: payload.__proto__.x = 1, - // payload['constructor'].prototype.y = 2. Reading these is already - // handled by the call and 'new' checks above. + // payload['constructor'].prototype.y = 2. The member-read check above + // already refuses these; this keeps the clearer assignment message. if ( node.type === 'AssignmentExpression' || node.type === 'UpdateExpression' diff --git a/packages/trpc/src/routers/auth.ts b/packages/trpc/src/routers/auth.ts index 421afe480..b22982845 100644 --- a/packages/trpc/src/routers/auth.ts +++ b/packages/trpc/src/routers/auth.ts @@ -21,6 +21,10 @@ import { verifyTotpCode, } from '@openpanel/auth'; import { generateSecureId } from '@openpanel/common/server'; +import { + createShareAccessToken, + shareAccessCookieName, +} from '@openpanel/common/server/share-access'; import { connectUserToOrganization, db, @@ -637,19 +641,15 @@ export const authRouter = createTRPCRouter({ .mutation(async ({ input, ctx }) => { const { password, shareId, shareType = 'overview' } = input; let share: { password: string | null; public: boolean } | null = null; - let cookieName = ''; if (shareType === 'overview') { share = await getShareOverviewById(shareId); - cookieName = `shared-overview-${shareId}`; } else if (shareType === 'dashboard') { const { getShareDashboardById } = await import('@openpanel/db'); share = await getShareDashboardById(shareId); - cookieName = `shared-dashboard-${shareId}`; } else if (shareType === 'report') { const { getShareReportById } = await import('@openpanel/db'); share = await getShareReportById(shareId); - cookieName = `shared-report-${shareId}`; } if (!share) { @@ -670,10 +670,21 @@ export const authRouter = createTRPCRouter({ throw new TRPCAccessError('Incorrect password'); } - ctx.setCookie(cookieName, '1', { - maxAge: 60 * 60 * 24 * 7, - ...COOKIE_OPTIONS, - }); + // The cookie value is an HMAC bound to this share and its current + // password hash; the share procedures verify it rather than trusting + // that the cookie exists. + ctx.setCookie( + shareAccessCookieName(shareType, shareId), + createShareAccessToken({ + type: shareType, + id: shareId, + passwordHash: share.password, + }), + { + maxAge: 60 * 60 * 24 * 7, + ...COOKIE_OPTIONS, + }, + ); return true; }), diff --git a/packages/trpc/src/routers/share.test.ts b/packages/trpc/src/routers/share.test.ts index 1f22ecff3..79b33bbcf 100644 --- a/packages/trpc/src/routers/share.test.ts +++ b/packages/trpc/src/routers/share.test.ts @@ -30,8 +30,11 @@ vi.mock('@openpanel/db', () => ({ runWithAlsSession: (_id: unknown, fn: () => unknown) => fn(), })); +import { createShareAccessToken } from '@openpanel/common/server/share-access'; import { shareRouter } from './share'; +process.env.COOKIE_SECRET ??= 'test-cookie-secret'; + const PASSWORD_HASH = '$argon2id$v=19$m=19456,t=2,p=1$NTb6p8dXsP2b1WpDU22i/w$ILi3tmTMYg5TSrjvVktAbixLHPH5PjvvbQypR9bZuoQ'; @@ -134,7 +137,11 @@ describe('share lookups do not leak the password hash', () => { }); const res = await anonCaller({ - 'shared-report-SECRT1': '1', + 'shared-report-SECRT1': createShareAccessToken({ + type: 'report', + id: 'SECRT1', + passwordHash: PASSWORD_HASH, + }), }).report({ shareId: 'SECRT1' }); expect(res.requiresPassword).toBe(false); @@ -142,6 +149,56 @@ describe('share lookups do not leak the password hash', () => { expect(JSON.stringify(res)).not.toContain('$argon2id'); }); + it('stays locked when the unlock cookie is forged (GHSA-p6c2-mq9r-cx3r)', async () => { + shareReportFindUnique.mockResolvedValue({ + ...privatePasswordProtectedShare, + public: true, + }); + + // The cookie used to be checked for presence only, so any value unlocked + // the share. + for (const forged of ['1', 'true', 'anything']) { + const res = await anonCaller({ + 'shared-report-SECRT1': forged, + }).report({ shareId: 'SECRT1' }); + expect(res.requiresPassword).toBe(true); + expect(res).not.toHaveProperty('report'); + } + }); + + it('stays locked when the cookie was minted for a different share', async () => { + shareReportFindUnique.mockResolvedValue({ + ...privatePasswordProtectedShare, + public: true, + }); + + const res = await anonCaller({ + 'shared-report-SECRT1': createShareAccessToken({ + type: 'report', + id: 'OTHER1', + passwordHash: PASSWORD_HASH, + }), + }).report({ shareId: 'SECRT1' }); + expect(res.requiresPassword).toBe(true); + }); + + it('stays locked when the password changed after the cookie was issued', async () => { + shareReportFindUnique.mockResolvedValue({ + ...privatePasswordProtectedShare, + public: true, + password: `${PASSWORD_HASH}rotated`, + }); + + const res = await anonCaller({ + 'shared-report-SECRT1': createShareAccessToken({ + type: 'report', + id: 'SECRT1', + passwordHash: PASSWORD_HASH, + }), + }).report({ shareId: 'SECRT1' }); + expect(res.requiresPassword).toBe(true); + }); + it('never selects the row by reportId, so object ids are not addressable', async () => { // The union input used to accept { reportId }, which let anyone knowing a // report id read the share row without ever seeing a share link. diff --git a/packages/trpc/src/routers/share.ts b/packages/trpc/src/routers/share.ts index df122efe3..cbfd8773b 100644 --- a/packages/trpc/src/routers/share.ts +++ b/packages/trpc/src/routers/share.ts @@ -15,6 +15,7 @@ import { } from '@openpanel/validation'; import { hashPassword } from '@openpanel/auth'; +import { hasShareAccess } from '@openpanel/common/server/share-access'; import { z } from 'zod'; import { requireProjectAccess } from '../access'; import { @@ -75,8 +76,14 @@ export const shareRouter = createTRPCRouter({ throw new TRPCNotFoundError('Share not found'); } - const hasAccess = !!ctx.cookies[`shared-overview-${share.id}`]; - if (share.password && !hasAccess) { + if ( + share.password && + !hasShareAccess(ctx.cookies, { + type: 'overview', + id: share.id, + passwordHash: share.password, + }) + ) { return lockedShare(share.id, share.organization, share.project); } @@ -160,8 +167,14 @@ export const shareRouter = createTRPCRouter({ throw new TRPCNotFoundError('Dashboard share not found'); } - const hasAccess = !!ctx.cookies[`shared-dashboard-${share.id}`]; - if (share.password && !hasAccess) { + if ( + share.password && + !hasShareAccess(ctx.cookies, { + type: 'dashboard', + id: share.id, + passwordHash: share.password, + }) + ) { return lockedShare(share.id, share.organization, share.project); } @@ -254,8 +267,14 @@ export const shareRouter = createTRPCRouter({ } // Check password access - const hasAccess = !!ctx.cookies[`shared-dashboard-${share.id}`]; - if (share.password && !hasAccess) { + if ( + share.password && + !hasShareAccess(ctx.cookies, { + type: 'dashboard', + id: share.id, + passwordHash: share.password, + }) + ) { throw new TRPCAccessError('Password required'); } @@ -283,8 +302,14 @@ export const shareRouter = createTRPCRouter({ throw new TRPCNotFoundError('Report share not found'); } - const hasAccess = !!ctx.cookies[`shared-report-${share.id}`]; - if (share.password && !hasAccess) { + if ( + share.password && + !hasShareAccess(ctx.cookies, { + type: 'report', + id: share.id, + passwordHash: share.password, + }) + ) { return lockedShare(share.id, share.organization, share.project); } From abe17c5e56d4e549b81a7c9b96c294aff1130f71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carl-Gerhard=20Lindesva=CC=88rd?= Date: Tue, 22 Sep 2026 21:46:54 +0200 Subject: [PATCH 2/5] fix(js-runtime): allowlist the template language and run it in an isolated context The validator was a denylist: it looked for known-bad shapes and let every other node through, which is how each new escape got in. It is now an allowlist of AST node types. Anything not on the list (tagged templates, sequence expressions, switch, labels, var, ++, delete, computed keys, object methods, TypeScript syntax, ...) is refused by default. The language is deliberately small: object/array literals, spread, property access with literal keys, template strings, ternaries, logical and arithmetic operators, const/let, if, and the allowlisted built-in methods. A template cannot call a function it defined itself: local identifiers are never callable, so there is no recursion, no IIFE and no "store a reference now, call it later" path. Inline arrows only appear as callbacks to the allowlisted array methods. All templates currently saved in production are in the test suite as fixtures. execute() now runs the template in a fresh V8 context via node:vm with eval and new Function disabled, a 250ms timeout and a 1MB output cap. The payload crosses in as JSON and the result crosses out as JSON, so the template never touches a host-realm object. This is defense in depth behind the validator, not a boundary on its own. Co-Authored-By: Claude Fable 5.1 --- packages/js-runtime/src/execute.ts | 55 ++- packages/js-runtime/src/validate.test.ts | 110 ++++- packages/js-runtime/src/validate.ts | 587 ++++++++++++----------- 3 files changed, 463 insertions(+), 289 deletions(-) diff --git a/packages/js-runtime/src/execute.ts b/packages/js-runtime/src/execute.ts index 48fada97e..929a98a09 100644 --- a/packages/js-runtime/src/execute.ts +++ b/packages/js-runtime/src/execute.ts @@ -1,5 +1,18 @@ +import { runInNewContext } from 'node:vm'; + import { validate } from './validate'; +/** + * Wall-clock budget for one template run. The validator refuses loops, + * recursion and calls to template-defined functions, so an honest template + * finishes in a few milliseconds; this is the backstop for a pathological + * regex or a huge .repeat(). + */ +const EXECUTION_TIMEOUT_MS = 250; + +/** Cap on the serialized output so a template cannot balloon a webhook body. */ +const MAX_OUTPUT_BYTES = 1_000_000; + /** * Executes a JavaScript function template * @param code - JavaScript function code (arrow function or function expression) @@ -11,26 +24,42 @@ export function execute( payload: Record, ): unknown { // Templates are checked when they are saved, but the stored string is what - // ends up in new Function() here. Check it again at run time rather than - // trusting whatever passed validation at save time. + // ends up being run here. Check it again at run time rather than trusting + // whatever passed validation at save time. const validation = validate(code); if (!validation.valid) { throw new Error(`Invalid JavaScript template: ${validation.error}`); } try { - // Create the function code that will be executed - // 'use strict' ensures 'this' is undefined (not global object) - const funcCode = ` - 'use strict'; - return (${code})(payload); - `; - - // Create function with safe globals in scope - const func = new Function('payload', funcCode); + // The template runs in a fresh V8 context with its own globals, with + // eval/new Function disabled there. The payload crosses in as JSON and + // the result crosses out as JSON, so the template never holds a + // reference to a host-realm object and nothing it returns carries a host + // prototype or function back into the worker. This is not a security + // boundary on its own (the validator is), but it means a validator miss + // lands in an empty realm instead of the worker process. + const script = `'use strict'; JSON.stringify((${code})(JSON.parse(payloadJson)));`; + const resultJson: unknown = runInNewContext( + script, + { payloadJson: JSON.stringify(payload) }, + { + timeout: EXECUTION_TIMEOUT_MS, + contextCodeGeneration: { strings: false, wasm: false }, + microtaskMode: 'afterEvaluate', + }, + ); - // Execute the function - return func(payload); + if (resultJson === undefined) { + return undefined; + } + if (typeof resultJson !== 'string') { + throw new Error('Template did not return a JSON-serializable value'); + } + if (resultJson.length > MAX_OUTPUT_BYTES) { + throw new Error('Template output is too large'); + } + return JSON.parse(resultJson); } catch (error) { throw new Error( `Error executing JavaScript template: ${ diff --git a/packages/js-runtime/src/validate.test.ts b/packages/js-runtime/src/validate.test.ts index 2907fd0b0..9a550fbfc 100644 --- a/packages/js-runtime/src/validate.test.ts +++ b/packages/js-runtime/src/validate.test.ts @@ -245,7 +245,7 @@ describe('validate', () => { }); it('should block a template literal key with substitutions', () => { - const result = validate('(payload) => payload[`${payload.key}`]'); + const result = validate(`(payload) => payload[\`\${payload.key}\`]`); expect(result.valid).toBe(false); expect(result.error).toContain('Dynamic computed property access'); }); @@ -275,19 +275,68 @@ describe('validate', () => { expect(result.valid).toBe(true); }); - it('should still allow calling a local arrow function', () => { + it('should block calling a local variable', () => { const result = validate( '(payload) => { const fmt = (v) => v.trim(); return fmt(payload.name); }', ); - expect(result.valid).toBe(true); + expect(result.valid).toBe(false); + expect(result.error).toContain("Calling 'fmt'"); }); - it('should still allow an immediately invoked arrow function', () => { + it('should block an immediately invoked arrow function', () => { const result = validate('(payload) => (() => payload.name)()'); + expect(result.valid).toBe(false); + expect(result.error).toContain('Calling the result of an expression'); + }); + + it('should block calling a global that is not a function', () => { + const result = validate('(payload) => Math(payload.name)'); + expect(result.valid).toBe(false); + expect(result.error).toContain("Calling 'Math'"); + }); + + it('should block a static method on a shadowed global', () => { + const result = validate( + '(payload) => { const JSON = payload; return JSON.parse(payload.raw); }', + ); + expect(result.valid).toBe(false); + }); + + it('should block new Date when Date is shadowed', () => { + const result = validate( + '(payload) => { const Date = payload; return new Date(); }', + ); + expect(result.valid).toBe(false); + }); + + it('should still allow callbacks passed to allowed methods', () => { + const result = validate( + '(payload) => payload.tags.map((t) => t.toUpperCase()).filter((t) => t.length > 1)', + ); expect(result.valid).toBe(true); }); }); + describe('Allowlist fallbacks', () => { + it('should reject syntax outside the allowlist by default', () => { + // A class field, a label, a switch: none of these need a dedicated + // rule, they are refused because they are not on the list. + expect(validate('(payload) => { switch (payload.name) { default: return 1; } }').valid).toBe(false); + expect(validate('(payload) => { x: return 1; }').valid).toBe(false); + expect(validate('(payload) => { var x = 1; return x; }').valid).toBe(false); + expect(validate('(payload) => { let i = 0; i++; return i; }').valid).toBe(false); + expect(validate('(payload) => delete payload.name').valid).toBe(false); + expect(validate('(payload) => ({ [payload.k]: 1 })').valid).toBe(false); + expect(validate('(payload) => ({ f() { return 1; } })').valid).toBe(false); + expect(validate('async (payload) => payload').valid).toBe(false); + }); + + it('should reject TypeScript syntax', () => { + const result = validate('(payload: any) => payload'); + expect(result.valid).toBe(false); + }); + }); + describe('Prototype writes', () => { it('should block writing through __proto__', () => { const result = validate( @@ -528,4 +577,57 @@ describe('execute', () => { }).toThrow('Invalid JavaScript template'); }); }); + + describe('Templates in use in production', () => { + // Real templates customers have saved. Every one must keep validating + // and executing whenever the allowlist is tightened. + const productionTemplates = [ + `(payload) => ({ name: payload.name || 'identify', profileId: payload.profileId, timestamp: new Date(payload.createdAt).toISOString(), properties: { ...(payload.properties || {}), country: payload.country, city: payload.city, device: payload.device, os: payload.os, browser: payload.browser, path: payload.path, firstName: payload.profile ? payload.profile.firstName : undefined, lastName: payload.profile ? payload.profile.lastName : undefined, email: payload.profile ? payload.profile.email : undefined } })`, + `(payload) => { if (!payload.profileId || !payload.profileId.includes('@')) return null; return { email_address: payload.profileId, fields: { "city": payload.city || "", "country": payload.country || "" } }; }`, + '(payload) => ({ event: payload.name, email: payload.properties?.email ?? null, occurredAt: payload.createdAt })', + `(payload) => { return { event_name: payload.name, first_name: payload.properties?.first_name || '', last_name: payload.properties?.last_name || '', email: payload.properties?.email || '', phone: payload.properties?.phone || '', sms_consent: payload.properties?.sms_consent || '', form_name: payload.properties?.form_name || '', form_id: payload.properties?.form_id || '', page_url: payload.properties?.page_url || '' }; }`, + ]; + + it.each(productionTemplates)('validates and runs: %s', (code) => { + expect(validate(code)).toEqual({ valid: true }); + expect(() => execute(code, basePayload)).not.toThrow(); + }); + + it('produces the expected shape for the identify template', () => { + const result = execute(productionTemplates[0]!, basePayload) as Record< + string, + unknown + >; + expect(result.name).toBe('page_view'); + expect(result.timestamp).toBe('2024-01-15T10:30:00.000Z'); + expect(result.properties).toMatchObject({ + plan: 'premium', + city: 'New York', + firstName: 'John', + }); + }); + + it('returns null when the template returns null', () => { + expect(execute(productionTemplates[1]!, basePayload)).toBeNull(); + }); + }); + + describe('Isolation', () => { + it('does not hand the template a host object', () => { + // The payload crosses into the context as JSON, so mutations never + // reach the caller's object. + const payload = { name: 'x', nested: { a: 1 } }; + execute('(payload) => { payload.nested.a = 2; return payload; }', payload); + expect(payload.nested.a).toBe(1); + }); + + it('stops a template that runs too long', () => { + expect(() => + execute( + "(payload) => 'a'.repeat(100000000).replace(/(a+)+b/, '')", + {}, + ), + ).toThrow('Error executing JavaScript template'); + }); + }); }); diff --git a/packages/js-runtime/src/validate.ts b/packages/js-runtime/src/validate.ts index a3c5d7379..5263b2b30 100644 --- a/packages/js-runtime/src/validate.ts +++ b/packages/js-runtime/src/validate.ts @@ -1,21 +1,132 @@ import { parse } from '@babel/parser'; -import { - ALLOWED_GLOBALS, - ALLOWED_INSTANCE_METHODS, - ALLOWED_METHODS, -} from './constants'; import { collectDeclaredIdentifiers, isPropertyKey, walkNode, } from './ast-walker'; +import { + ALLOWED_GLOBALS, + ALLOWED_INSTANCE_METHODS, + ALLOWED_METHODS, +} from './constants'; + +/** + * The template language is an allowlist, not a denylist. + * + * Every AST node type the walker meets must be in ALLOWED_NODE_TYPES or the + * template is refused, so a syntax form nobody thought about (a tagged + * template, a sequence expression, a class field, ...) is rejected by + * default instead of walking past the checks. What is left is deliberately + * small: build an object from the payload with property access, literals, + * template strings, ternaries, and the allowlisted built-in methods. + * + * There is no way to call a function the template defined itself. Callbacks + * to .map() and friends are the only place an inline arrow may appear, and + * only allowlisted methods can invoke them. That removes recursion, IIFEs and + * every "store a reference now, call it later" escape in one rule. + */ +const ALLOWED_NODE_TYPES = new Set([ + // Structure + 'File', + 'Program', + 'ExpressionStatement', + 'EmptyStatement', + 'ArrowFunctionExpression', + 'BlockStatement', + 'ReturnStatement', + 'IfStatement', + 'VariableDeclaration', + 'VariableDeclarator', + + // Values + 'Identifier', + 'NumericLiteral', + 'StringLiteral', + 'BooleanLiteral', + 'NullLiteral', + 'RegExpLiteral', + 'TemplateLiteral', + 'TemplateElement', + 'ObjectExpression', + 'ObjectProperty', + 'ArrayExpression', + 'SpreadElement', + + // Access and calls + 'MemberExpression', + 'OptionalMemberExpression', + 'CallExpression', + 'OptionalCallExpression', + 'NewExpression', + + // Operators + 'BinaryExpression', + 'LogicalExpression', + 'UnaryExpression', + 'ConditionalExpression', + 'AssignmentExpression', + + // Destructuring in const declarations and callback parameters + 'ObjectPattern', + 'ArrayPattern', + 'RestElement', + 'AssignmentPattern', +]); + +/** + * Friendlier messages for the things people are most likely to try. Anything + * not listed here or in ALLOWED_NODE_TYPES gets a generic refusal. + */ +const REJECTION_MESSAGES: Record = { + ImportDeclaration: 'import/export statements are not allowed', + ExportNamedDeclaration: 'import/export statements are not allowed', + ExportDefaultDeclaration: 'import/export statements are not allowed', + ExportAllDeclaration: 'import/export statements are not allowed', + Import: 'Dynamic import() is not allowed', + ImportExpression: 'Dynamic import() is not allowed', + FunctionDeclaration: + 'Named function declarations are not allowed inside the function body.', + FunctionExpression: + 'Function expressions are not allowed. Use arrow functions instead: (payload) => { ... }', + ObjectMethod: + 'Methods in object literals are not allowed. Use a plain property instead.', + WhileStatement: + 'Loops are not allowed. Use array methods like .map(), .filter(), .reduce() instead.', + DoWhileStatement: + 'Loops are not allowed. Use array methods like .map(), .filter(), .reduce() instead.', + ForStatement: + 'Loops are not allowed. Use array methods like .map(), .filter(), .reduce() instead.', + ForInStatement: + 'Loops are not allowed. Use array methods like .map(), .filter(), .reduce() instead.', + ForOfStatement: + 'Loops are not allowed. Use array methods like .map(), .filter(), .reduce() instead.', + SwitchStatement: + 'switch statements are not allowed. Use if or a ternary expression instead.', + TryStatement: 'try/catch statements are not allowed', + ThrowStatement: 'throw statements are not allowed', + WithStatement: 'with statements are not allowed', + ClassDeclaration: 'Class definitions are not allowed', + ClassExpression: 'Class definitions are not allowed', + AwaitExpression: 'async/await is not allowed', + YieldExpression: 'Generators are not allowed', + ThisExpression: + "'this' keyword is not allowed. Use the payload parameter instead.", + Super: "'super' is not allowed.", + MetaProperty: 'new.target and import.meta are not allowed.', + TaggedTemplateExpression: 'Tagged template literals are not allowed.', + SequenceExpression: 'Comma (sequence) expressions are not allowed.', + UpdateExpression: + 'Increment and decrement operators (++, --) are not allowed.', + LabeledStatement: 'Labels are not allowed.', + DebuggerStatement: 'debugger statements are not allowed.', +}; /** * Property names that must never be read or written through. Reading * 'constructor' off any value walks up to the Function constructor, and * assigning through '__proto__' or 'prototype' reaches objects shared with - * the rest of the worker process. Neither is ever needed by a template. + * the rest of the process. A template never needs any of them. */ const FORBIDDEN_PROPERTIES = new Set([ '__proto__', @@ -25,25 +136,35 @@ const FORBIDDEN_PROPERTIES = new Set([ 'callee', ]); -/** - * Callee shapes a call expression may have. Anything else (a sequence - * expression, the result of another call, ...) hides what is being invoked. - */ -const ALLOWED_CALLEE_TYPES = new Set([ - 'Identifier', - 'MemberExpression', - 'OptionalMemberExpression', - 'ArrowFunctionExpression', +/** Globals that may be called directly, as in parseInt(payload.count). */ +const ALLOWED_GLOBAL_FUNCTIONS = new Set([ + 'parseInt', + 'parseFloat', + 'isNaN', + 'isFinite', +]); + +const ALLOWED_UNARY_OPERATORS = new Set(['!', '-', '+', 'typeof']); + +const ALLOWED_ASSIGNMENT_OPERATORS = new Set([ + '=', + '+=', + '-=', + '*=', + '/=', + '??=', + '||=', + '&&=', ]); +type Node = Record; + /** * The static name of a member expression's property, or undefined when the key * is only known at run time (obj[someVariable]). */ -function staticPropertyName( - member: Record -): string | undefined { - const prop = member.property as Record | undefined; +function staticPropertyName(member: Node): string | undefined { + const prop = member.property as Node | undefined; if (!prop) { return undefined; } @@ -51,8 +172,7 @@ function staticPropertyName( // A literal key can be resolved. Babel has already decoded any \u / \x // escapes into StringLiteral.value by this point. A template literal // with no substitutions (`__proto__`) is just as static as a string - // literal and must resolve the same way, or it walks past this check - // unseen. + // literal and must resolve the same way. if (prop.type === 'StringLiteral') { return prop.value as string; } @@ -61,9 +181,9 @@ function staticPropertyName( } if (prop.type === 'TemplateLiteral') { const expressions = prop.expressions as unknown[]; - const quasis = prop.quasis as Record[]; + const quasis = prop.quasis as Node[]; if (expressions.length === 0 && quasis.length === 1) { - const value = quasis[0]!.value as Record; + const value = quasis[0]!.value as Node; return value.cooked as string; } } @@ -76,25 +196,17 @@ function staticPropertyName( * The static key of a property inside an object literal or destructuring * pattern, or undefined when it is computed from an expression. */ -function staticObjectPropertyKey( - prop: Record -): string | undefined { - const key = prop.key as Record | undefined; +function staticObjectPropertyKey(prop: Node): string | undefined { + const key = prop.key as Node | undefined; if (!key) { return undefined; } - if (prop.computed) { - if (key.type === 'StringLiteral' || key.type === 'NumericLiteral') { - return String(key.value); - } - return undefined; - } - if (key.type === 'Identifier') { - return key.name as string; - } if (key.type === 'StringLiteral' || key.type === 'NumericLiteral') { return String(key.value); } + if (!prop.computed && key.type === 'Identifier') { + return key.name as string; + } return undefined; } @@ -103,10 +215,8 @@ function staticObjectPropertyKey( * forbidden property name it passes through, if any. payload.__proto__.x is a * write to 'x' but goes through '__proto__', so the whole chain matters. */ -function forbiddenPropertyInChain( - target: Record -): string | undefined { - let current: Record | undefined = target; +function forbiddenPropertyInChain(target: Node): string | undefined { + let current: Node | undefined = target; while ( current && @@ -117,7 +227,107 @@ function forbiddenPropertyInChain( if (name && FORBIDDEN_PROPERTIES.has(name)) { return name; } - current = current.object as Record | undefined; + current = current.object as Node | undefined; + } + + return undefined; +} + +/** Validate the one statement at the root: a single arrow function. */ +function validateRoot(program: Node): string | undefined { + const body = program.body as Node[]; + + if (body.length === 0) { + return 'Code cannot be empty'; + } + + if (body.length > 1) { + return 'Code must contain only a single function. Multiple statements are not allowed.'; + } + + const rootStatement = body[0]!; + + if (rootStatement.type !== 'ExpressionStatement') { + if (rootStatement.type === 'VariableDeclaration') { + return 'Variable declarations (const, let, var) are not allowed. Use a direct function expression instead.'; + } + if (rootStatement.type === 'FunctionDeclaration') { + return 'Function declarations are not allowed. Use an arrow function or function expression instead: (payload) => { ... } or function(payload) { ... }'; + } + return 'Code must be a function expression or arrow function'; + } + + const rootExpression = rootStatement.expression as Node; + if (rootExpression.type !== 'ArrowFunctionExpression') { + if (rootExpression.type === 'FunctionExpression') { + return 'Function expressions are not allowed. Use arrow functions instead: (payload) => { ... }'; + } + return 'Code must be an arrow function, e.g.: (payload) => { ... }'; + } + + return undefined; +} + +/** Check a call's callee names something on the allowlist. */ +function validateCall( + callee: Node, + declaredIdentifiers: Set, +): string | undefined { + if (callee.type === 'Import') { + return 'Dynamic import() is not allowed'; + } + + // parseInt(x) and friends. A local variable is never callable: the only + // functions a template can hold are inline arrows, and those are only ever + // invoked by the allowlisted array methods they are passed to. + if (callee.type === 'Identifier') { + const name = callee.name as string; + if (declaredIdentifiers.has(name)) { + return `Calling '${name}' is not allowed. Only the built-in methods can be called.`; + } + if (!ALLOWED_GLOBAL_FUNCTIONS.has(name)) { + return `Calling '${name}' is not allowed. Only safe built-in functions are permitted.`; + } + return undefined; + } + + if ( + callee.type !== 'MemberExpression' && + callee.type !== 'OptionalMemberExpression' + ) { + return 'Calling the result of an expression is not allowed. Call a named function or method directly.'; + } + + // A computed key (obj[expr]()) cannot be matched against the allowlist. + if (callee.computed) { + return 'Computed property access on a call target is not allowed. Use a literal method name, e.g. value.toUpperCase().'; + } + + const obj = callee.object as Node; + const prop = callee.property as Node; + if (prop.type !== 'Identifier') { + return 'Calling the result of an expression is not allowed. Call a named function or method directly.'; + } + const methodName = prop.name as string; + + // Static method on an allowed global: Math.round(), JSON.parse(). A local + // that shadows the global name is an ordinary value and takes the instance + // branch below. + if ( + obj.type === 'Identifier' && + ALLOWED_GLOBALS.has(obj.name as string) && + !declaredIdentifiers.has(obj.name as string) + ) { + const objName = obj.name as string; + if (!ALLOWED_METHODS[objName]?.has(methodName)) { + return `Method '${objName}.${methodName}' is not allowed. Only safe methods are permitted.`; + } + return undefined; + } + + // Instance method on a value: arr.map(), str.toLowerCase(), arr?.map() + if (!ALLOWED_INSTANCE_METHODS.has(methodName)) { + return `Method '.${methodName}()' is not allowed. Only safe methods are permitted.`; } return undefined; @@ -136,66 +346,14 @@ export function validate(code: string): { } try { - // Parse the code to AST const ast = parse(code, { sourceType: 'module', allowReturnOutsideFunction: true, - plugins: ['typescript'], }); - // Validate root structure: must be exactly one function expression - const program = ast.program; - const body = program.body; - - if (body.length === 0) { - return { valid: false, error: 'Code cannot be empty' }; - } - - if (body.length > 1) { - return { - valid: false, - error: - 'Code must contain only a single function. Multiple statements are not allowed.', - }; - } - - const rootStatement = body[0]!; - - // Must be an expression statement containing a function - if (rootStatement.type !== 'ExpressionStatement') { - if (rootStatement.type === 'VariableDeclaration') { - return { - valid: false, - error: - 'Variable declarations (const, let, var) are not allowed. Use a direct function expression instead.', - }; - } - if (rootStatement.type === 'FunctionDeclaration') { - return { - valid: false, - error: - 'Function declarations are not allowed. Use an arrow function or function expression instead: (payload) => { ... } or function(payload) { ... }', - }; - } - return { - valid: false, - error: 'Code must be a function expression or arrow function', - }; - } - - const rootExpression = rootStatement.expression; - if (rootExpression.type !== 'ArrowFunctionExpression') { - if (rootExpression.type === 'FunctionExpression') { - return { - valid: false, - error: - 'Function expressions are not allowed. Use arrow functions instead: (payload) => { ... }', - }; - } - return { - valid: false, - error: 'Code must be an arrow function, e.g.: (payload) => { ... }', - }; + const rootError = validateRoot(ast.program as unknown as Node); + if (rootError) { + return { valid: false, error: rootError }; } // Collect all declared identifiers (variables, parameters) @@ -203,95 +361,37 @@ export function validate(code: string): { let validationError: string | undefined; - // Walk the AST to check for allowed patterns only walkNode(ast, (node, parent) => { // Skip if we already found an error if (validationError) { return; } - // Block import/export declarations - if ( - node.type === 'ImportDeclaration' || - node.type === 'ExportDeclaration' - ) { - validationError = 'import/export statements are not allowed'; - return; - } - - // Block dynamic import(). @babel/parser emits 'Import' as the callee of - // the surrounding CallExpression; 'ImportExpression' is the ESTree shape. - if (node.type === 'Import' || node.type === 'ImportExpression') { - validationError = 'Dynamic import() is not allowed'; - return; - } - - // Block function declarations inside the function body - // (FunctionDeclaration creates a named function, not allowed) - if (node.type === 'FunctionDeclaration') { - validationError = - 'Named function declarations are not allowed inside the function body.'; - return; - } + const type = node.type as string; - // Block loops - use array methods like .map(), .filter() instead - if ( - node.type === 'WhileStatement' || - node.type === 'DoWhileStatement' || - node.type === 'ForStatement' || - node.type === 'ForInStatement' || - node.type === 'ForOfStatement' - ) { + // Anything outside the allowlist is refused, with a friendlier message + // where we have one. + if (!ALLOWED_NODE_TYPES.has(type)) { validationError = - 'Loops are not allowed. Use array methods like .map(), .filter(), .reduce() instead.'; - return; - } - - // Block advanced/dangerous features - if (node.type === 'TryStatement') { - validationError = 'try/catch statements are not allowed'; - return; - } - - if (node.type === 'ThrowStatement') { - validationError = 'throw statements are not allowed'; - return; - } - - if (node.type === 'WithStatement') { - validationError = 'with statements are not allowed'; - return; - } - - if (node.type === 'ClassDeclaration' || node.type === 'ClassExpression') { - validationError = 'Class definitions are not allowed'; + REJECTION_MESSAGES[type] ?? + `'${type}' syntax is not allowed. Templates support property access, literals, template strings, ternaries and the built-in methods.`; return; } - if (node.type === 'AwaitExpression') { + if (type === 'ArrowFunctionExpression' && node.async) { validationError = 'async/await is not allowed'; return; } - if (node.type === 'YieldExpression') { - validationError = 'Generators are not allowed'; - return; - } - - // Block 'this' keyword - arrow functions don't have their own 'this' - // but we block it entirely to prevent any scope leakage - if (node.type === 'ThisExpression') { - validationError = - "'this' keyword is not allowed. Use the payload parameter instead."; + if (type === 'VariableDeclaration' && node.kind === 'var') { + validationError = "'var' is not allowed. Use const or let instead."; return; } // Check identifiers that reference globals - if (node.type === 'Identifier') { + if (type === 'Identifier') { const name = node.name as string; - // Block 'arguments' - not available in arrow functions anyway - // but explicitly block to prevent any confusion if (name === 'arguments') { validationError = "'arguments' is not allowed. Use explicit parameters instead."; @@ -308,7 +408,6 @@ export function validate(code: string): { return; } - // Check if it's an allowed global if (!ALLOWED_GLOBALS.has(name)) { validationError = `Use of '${name}' is not allowed. Only safe built-in functions are permitted.`; return; @@ -316,135 +415,62 @@ export function validate(code: string): { } // Every property read is checked, not only the ones that are called or - // assigned. A read of 'constructor' can be stored in a local and invoked - // later, so the read itself is what has to be refused. A key that is - // only known at run time (obj[expr]) could be any of these names, so it - // is refused as well; literal keys and numeric indexes still work. - if ( - node.type === 'MemberExpression' || - node.type === 'OptionalMemberExpression' - ) { + // assigned: a read of 'constructor' can be stored and used later. A key + // that is only known at run time (obj[expr]) could be any of these + // names, so it is refused too; literal keys and numeric indexes work. + if (type === 'MemberExpression' || type === 'OptionalMemberExpression') { const name = staticPropertyName(node); - if (node.computed && name === undefined) { + if (name === undefined) { validationError = 'Dynamic computed property access (obj[expr]) is not allowed. Use a literal key such as obj.key, obj["key"] or arr[0].'; return; } - if (name !== undefined && FORBIDDEN_PROPERTIES.has(name)) { + if (FORBIDDEN_PROPERTIES.has(name)) { validationError = `Accessing '${name}' is not allowed.`; return; } } + // Object literal keys must be static too; { [expr]: v } is refused. + if ( + type === 'ObjectProperty' && + parent?.type === 'ObjectExpression' && + staticObjectPropertyKey(node) === undefined + ) { + validationError = + 'Computed keys in object literals are not allowed. Use a literal key.'; + return; + } + // Destructuring is a read too: const { constructor: C } = payload. - if (node.type === 'ObjectPattern') { - const properties = node.properties as Record[]; + if (type === 'ObjectPattern') { + const properties = node.properties as Node[]; for (const prop of properties) { if (prop.type !== 'ObjectProperty') { continue; } const name = staticObjectPropertyKey(prop); - if (prop.computed && name === undefined) { + if (name === undefined) { validationError = 'Computed keys in destructuring patterns are not allowed.'; return; } - if (name !== undefined && FORBIDDEN_PROPERTIES.has(name)) { + if (FORBIDDEN_PROPERTIES.has(name)) { validationError = `Destructuring '${name}' is not allowed.`; return; } } } - // A tagged template (tag`...`) is a call that the call checks below - // never see. Templates have no use for it. - if (node.type === 'TaggedTemplateExpression') { - validationError = 'Tagged template literals are not allowed.'; + if (type === 'CallExpression' || type === 'OptionalCallExpression') { + validationError = validateCall(node.callee as Node, declaredIdentifiers); return; } - // Check method calls on global objects (like Math.random, JSON.parse) - // Handles both regular calls and optional chaining (?.) - if ( - node.type === 'CallExpression' || - node.type === 'OptionalCallExpression' - ) { - const callee = node.callee as Record; - const isMemberExpr = - callee.type === 'MemberExpression' || - callee.type === 'OptionalMemberExpression'; - - // @babel/parser emits an 'Import' callee for import(); keep the - // dedicated error for it. - if (callee.type === 'Import') { - validationError = 'Dynamic import() is not allowed'; - return; - } - - // The callee must name what is being invoked: a local or allowed - // global identifier, a member expression checked below, or an inline - // arrow function. (0, x)(...) and f()(...) are refused. - if (!ALLOWED_CALLEE_TYPES.has(callee.type as string)) { - validationError = - 'Calling the result of an expression is not allowed. Call a named function or method directly.'; - return; - } - - if (isMemberExpr) { - const obj = callee.object as Record; - const prop = callee.property as Record; - const computed = callee.computed as boolean; - - // A computed key (obj[expr]()) cannot be matched against the - // allowlist, because the property name is only known at run time. - // Refuse it rather than letting it past the checks below unseen. - if (computed) { - validationError = - 'Computed property access on a call target is not allowed. Use a literal method name, e.g. value.toUpperCase().'; - return; - } - - // Static method call on global object: Math.random(), JSON.parse() - if (obj.type === 'Identifier' && prop.type === 'Identifier') { - const objName = obj.name as string; - const methodName = prop.name as string; - - // Check if it's a call on an allowed global object - if (ALLOWED_GLOBALS.has(objName) && ALLOWED_METHODS[objName]) { - if (!ALLOWED_METHODS[objName].has(methodName)) { - validationError = `Method '${objName}.${methodName}' is not allowed. Only safe methods are permitted.`; - return; - } - } - } - - // Instance method call: arr.map(), str.toLowerCase(), arr?.map() - // We allow these if the method name is in ALLOWED_INSTANCE_METHODS - if (prop.type === 'Identifier') { - const methodName = prop.name as string; - - // If calling on something other than an allowed global, - // check if the method is in the allowed instance methods - if ( - obj.type !== 'Identifier' || - !ALLOWED_GLOBALS.has(obj.name as string) - ) { - if (!ALLOWED_INSTANCE_METHODS.has(methodName)) { - validationError = `Method '.${methodName}()' is not allowed. Only safe methods are permitted.`; - return; - } - } - } - } - } - // Check 'new' expressions - only allow new Date() - if (node.type === 'NewExpression') { - const callee = node.callee as Record; + if (type === 'NewExpression') { + const callee = node.callee as Node; - // Anything other than a bare identifier (a member expression, a - // parenthesised expression, another call) names a constructor we - // cannot resolve, so it can never be the Date we allow. if (callee.type !== 'Identifier') { validationError = "The target of 'new' must be a plain identifier. Only 'new Date()' is permitted."; @@ -452,20 +478,37 @@ export function validate(code: string): { } const name = callee.name as string; - if (name !== 'Date') { + if (name !== 'Date' || declaredIdentifiers.has(name)) { validationError = `'new ${name}()' is not allowed. Only 'new Date()' is permitted.`; return; } } - // Block writes that reach the prototype chain: payload.__proto__.x = 1, - // payload['constructor'].prototype.y = 2. The member-read check above - // already refuses these; this keeps the clearer assignment message. - if ( - node.type === 'AssignmentExpression' || - node.type === 'UpdateExpression' - ) { - const target = (node.left ?? node.argument) as Record; + if (type === 'UnaryExpression') { + const operator = node.operator as string; + if (!ALLOWED_UNARY_OPERATORS.has(operator)) { + validationError = `The '${operator}' operator is not allowed.`; + return; + } + } + + // Plain assignments to locals and to static properties of locals are + // fine; anything that reaches the prototype chain is not. + if (type === 'AssignmentExpression') { + const operator = node.operator as string; + if (!ALLOWED_ASSIGNMENT_OPERATORS.has(operator)) { + validationError = `The '${operator}' assignment operator is not allowed.`; + return; + } + const target = node.left as Node; + if ( + target.type !== 'Identifier' && + target.type !== 'MemberExpression' + ) { + validationError = + 'Assignments must target a variable or a property, e.g. out.event = payload.name.'; + return; + } const reached = forbiddenPropertyInChain(target); if (reached) { validationError = `Assigning through '${reached}' is not allowed.`; From 7c952c08f7ff90de02ea410950a7ff797cf20d7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carl-Gerhard=20Lindesva=CC=88rd?= Date: Tue, 22 Sep 2026 21:47:22 +0200 Subject: [PATCH 3/5] test(js-runtime): build the placeholder-key fixture without a template literal Co-Authored-By: Claude Fable 5.1 --- packages/js-runtime/src/validate.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/js-runtime/src/validate.test.ts b/packages/js-runtime/src/validate.test.ts index 9a550fbfc..4fc354b22 100644 --- a/packages/js-runtime/src/validate.test.ts +++ b/packages/js-runtime/src/validate.test.ts @@ -245,7 +245,9 @@ describe('validate', () => { }); it('should block a template literal key with substitutions', () => { - const result = validate(`(payload) => payload[\`\${payload.key}\`]`); + // Built by concatenation so the source file itself has no placeholder. + const code = ['(payload) => payload[`$', '{payload.key}`]'].join(''); + const result = validate(code); expect(result.valid).toBe(false); expect(result.error).toContain('Dynamic computed property access'); }); From f6aebb9122d6bbd6b686b177f7a49b2c95e9031b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carl-Gerhard=20Lindesva=CC=88rd?= Date: Tue, 22 Sep 2026 22:27:42 +0200 Subject: [PATCH 4/5] fix(js-runtime): refuse writes to globals and stray arrows, count output in bytes Review follow-ups on the allowlist validator: - An assignment could target a property of an allowed global (Math.round = (x) => Math.round(x)), replacing a built-in for the rest of the run and looping until the timeout. Assignments now have to be rooted at a local variable. - A nested arrow is now only accepted as an argument passed directly to a call. Stored or assigned arrows are never callable, so they had no legitimate use. - The output cap measured UTF-16 code units; it now measures UTF-8 bytes, which is what goes on the wire. Co-Authored-By: Claude Fable 5.1 --- packages/js-runtime/src/execute.ts | 2 +- packages/js-runtime/src/validate.test.ts | 33 +++++++++++++++- packages/js-runtime/src/validate.ts | 48 ++++++++++++++++++++++-- 3 files changed, 78 insertions(+), 5 deletions(-) diff --git a/packages/js-runtime/src/execute.ts b/packages/js-runtime/src/execute.ts index 929a98a09..625e42161 100644 --- a/packages/js-runtime/src/execute.ts +++ b/packages/js-runtime/src/execute.ts @@ -56,7 +56,7 @@ export function execute( if (typeof resultJson !== 'string') { throw new Error('Template did not return a JSON-serializable value'); } - if (resultJson.length > MAX_OUTPUT_BYTES) { + if (Buffer.byteLength(resultJson, 'utf8') > MAX_OUTPUT_BYTES) { throw new Error('Template output is too large'); } return JSON.parse(resultJson); diff --git a/packages/js-runtime/src/validate.test.ts b/packages/js-runtime/src/validate.test.ts index 4fc354b22..3a7fd2eda 100644 --- a/packages/js-runtime/src/validate.test.ts +++ b/packages/js-runtime/src/validate.test.ts @@ -279,7 +279,7 @@ describe('validate', () => { it('should block calling a local variable', () => { const result = validate( - '(payload) => { const fmt = (v) => v.trim(); return fmt(payload.name); }', + '(payload) => { const fmt = payload.name; return fmt(payload.name); }', ); expect(result.valid).toBe(false); expect(result.error).toContain("Calling 'fmt'"); @@ -311,6 +311,30 @@ describe('validate', () => { expect(result.valid).toBe(false); }); + it('should block assigning to a method on a global', () => { + const result = validate( + '(payload) => { Math.round = (x) => Math.round(x); return Math.round(1); }', + ); + expect(result.valid).toBe(false); + expect(result.error).toContain('only target local variables'); + }); + + it('should block assigning to a global itself', () => { + const result = validate('(payload) => { JSON = payload; return 1; }'); + expect(result.valid).toBe(false); + }); + + it('should block an arrow stored in a variable', () => { + const result = validate('(payload) => { const f = (x) => x; return 1; }'); + expect(result.valid).toBe(false); + expect(result.error).toContain('callbacks passed directly'); + }); + + it('should block an arrow in an object literal', () => { + const result = validate('(payload) => ({ f: (x) => x })'); + expect(result.valid).toBe(false); + }); + it('should still allow callbacks passed to allowed methods', () => { const result = validate( '(payload) => payload.tags.map((t) => t.toUpperCase()).filter((t) => t.length > 1)', @@ -623,6 +647,13 @@ describe('execute', () => { expect(payload.nested.a).toBe(1); }); + it('measures the output cap in UTF-8 bytes', () => { + // 600k three-byte characters is 600k UTF-16 units but 1.8MB on the wire. + expect(() => + execute("(payload) => 'ࠀ'.repeat(600000)", {}), + ).toThrow('too large'); + }); + it('stops a template that runs too long', () => { expect(() => execute( diff --git a/packages/js-runtime/src/validate.ts b/packages/js-runtime/src/validate.ts index 5263b2b30..ee12cc5ca 100644 --- a/packages/js-runtime/src/validate.ts +++ b/packages/js-runtime/src/validate.ts @@ -233,6 +233,19 @@ function forbiddenPropertyInChain(target: Node): string | undefined { return undefined; } +/** The identifier at the root of a member chain: payload in payload.a.b. */ +function chainRoot(target: Node): Node | undefined { + let current: Node | undefined = target; + while ( + current && + (current.type === 'MemberExpression' || + current.type === 'OptionalMemberExpression') + ) { + current = current.object as Node | undefined; + } + return current; +} + /** Validate the one statement at the root: a single arrow function. */ function validateRoot(program: Node): string | undefined { const body = program.body as Node[]; @@ -358,6 +371,9 @@ export function validate(code: string): { // Collect all declared identifiers (variables, parameters) const declaredIdentifiers = collectDeclaredIdentifiers(ast); + const rootArrow = ( + ((ast.program as unknown as Node).body as Node[])[0]! as Node + ).expression as Node; let validationError: string | undefined; @@ -378,9 +394,24 @@ export function validate(code: string): { return; } - if (type === 'ArrowFunctionExpression' && node.async) { - validationError = 'async/await is not allowed'; - return; + if (type === 'ArrowFunctionExpression') { + if (node.async) { + validationError = 'async/await is not allowed'; + return; + } + // Besides the root, an arrow may only be a callback handed straight + // to a call, e.g. arr.map((x) => ...). One stored in a variable or + // assigned onto a global is never callable and is only useful for + // wrapping a built-in in itself. + const isDirectCallback = + (parent?.type === 'CallExpression' || + parent?.type === 'OptionalCallExpression') && + (parent.arguments as Node[]).includes(node); + if (node !== rootArrow && !isDirectCallback) { + validationError = + 'Arrow functions are only allowed as callbacks passed directly to a method, e.g. arr.map((x) => x.name).'; + return; + } } if (type === 'VariableDeclaration' && node.kind === 'var') { @@ -514,6 +545,17 @@ export function validate(code: string): { validationError = `Assigning through '${reached}' is not allowed.`; return; } + // Math.round = ..., JSON.parse = ...: writing to a global replaces a + // built-in for the rest of the run. Only locals may be written. + const root = chainRoot(target); + if ( + root?.type !== 'Identifier' || + !declaredIdentifiers.has(root.name as string) + ) { + validationError = + 'Assignments may only target local variables and their properties.'; + return; + } } }); From 466d54392a957a726905a3cb6a32d3dd10ccb206 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carl-Gerhard=20Lindesva=CC=88rd?= Date: Tue, 22 Sep 2026 22:16:01 +0200 Subject: [PATCH 5/5] fix(security): close the remaining triage advisories Unauthenticated reads (GHSA-vrrm-p9p4-2gfg, GHSA-c98x-mph7-r6vp, GHSA-r4g5-vgpj-923m): reference.getChartReferences had no access check at all, and event.bots let any anonymous caller through as long as a shareOverview row existed, ignoring public and password. Both now go through hasAnonymousShareAccessToProject, which requires a public share for the project that is either password-free or unlocked with a verified share-access cookie. Members still need project access. ClickHouse escaping (GHSA-7fm7-rprq-g8rm, GHSA-gvwr-5684-wjqc, GHSA-gvr5-22f4-pj6g): conversion.service interpolated event names, dates and the project id raw inside quotes; they are now sqlstring-escaped. The cohort profile filter column is an identifier and cannot be escaped, so profileColumnAccess now enforces the same column allowlist that filter-where.service uses and throws on anything else. sankey.service replaced quote-doubling, which ClickHouse's backslash escapes defeat, with sqlstring.escape in all eight places. Bull Board (GHSA-r627-6vrh-65p9): the queue dashboard can add, retry and clean jobs and was mounted with no auth. It now mounts only when BULLBOARD_USERNAME and BULLBOARD_PASSWORD are set, behind HTTP Basic auth with constant-time comparison, and /healthcheck and /metrics are registered before it so they stay reachable. The Coolify template provides generated credentials; docs updated. Logs (GHSA-xr2x-w49w-hp2c): the migration banner printed full DATABASE_URL and CLICKHOUSE_URL; credentials and credential-like query params are now redacted. The email fallback dumped recipient, subject and template data (including password-reset links); it now logs a one-line warning with a redacted recipient. Unsubscribe tokens (GHSA-cv3v-4j56-hr88 side note): the HMAC secret fell back to a literal default, so links were forgeable. It now requires UNSUBSCRIBE_SECRET or COOKIE_SECRET and fails the request otherwise. The Coolify worker gets COOKIE_SECRET so it signs with the same key the API verifies with. Co-Authored-By: Claude Fable 5.1 --- .../self-hosting/environment-variables.mdx | 16 +++- apps/worker/src/index.ts | 70 ++++++++++------ apps/worker/src/utils/basic-auth.test.ts | 43 ++++++++++ apps/worker/src/utils/basic-auth.ts | 34 ++++++++ packages/db/code-migrations/migrate.ts | 36 +++++++- packages/db/src/services/cohort.service.ts | 23 +++++- .../db/src/services/conversion.service.ts | 16 ++-- packages/db/src/services/sankey.service.ts | 33 ++++---- packages/db/src/services/share.service.ts | 51 ++++++++++++ packages/email/src/index.tsx | 20 +++-- packages/email/src/unsubscribe.ts | 21 +++-- packages/trpc/src/routers/event.ts | 22 ++--- packages/trpc/src/routers/reference.test.ts | 82 +++++++++++++++++++ packages/trpc/src/routers/reference.ts | 31 ++++++- self-hosting/coolify.yml | 5 ++ 15 files changed, 426 insertions(+), 77 deletions(-) create mode 100644 apps/worker/src/utils/basic-auth.test.ts create mode 100644 apps/worker/src/utils/basic-auth.ts create mode 100644 packages/trpc/src/routers/reference.test.ts diff --git a/apps/public/content/docs/self-hosting/environment-variables.mdx b/apps/public/content/docs/self-hosting/environment-variables.mdx index 66b60fc59..798b85c1c 100644 --- a/apps/public/content/docs/self-hosting/environment-variables.mdx +++ b/apps/public/content/docs/self-hosting/environment-variables.mdx @@ -703,13 +703,27 @@ Port for the worker service to listen on. WORKER_PORT=3000 ``` +### BULLBOARD_USERNAME / BULLBOARD_PASSWORD + +**Type**: `string` +**Required**: No +**Default**: None + +Credentials for the Bull Board queue dashboard served on the worker port. The dashboard can add, retry and clean jobs, so it is only mounted when both variables are set. Without them the worker logs a warning at startup and serves only `/healthcheck` and `/metrics`. The browser prompts for these credentials (HTTP Basic auth). + +**Example**: +```bash +BULLBOARD_USERNAME=ops +BULLBOARD_PASSWORD=$(openssl rand -hex 16) +``` + ### DISABLE_BULLBOARD **Type**: `boolean` **Required**: No **Default**: `false` -Disable BullMQ board UI. Set to `true` or `1` to disable the queue monitoring dashboard. +Disable the Bull Board queue dashboard entirely and silence the startup warning about missing credentials. Set to `true` or `1`. **Example**: ```bash diff --git a/apps/worker/src/index.ts b/apps/worker/src/index.ts index 0a38bffa2..23eac47f5 100644 --- a/apps/worker/src/index.ts +++ b/apps/worker/src/index.ts @@ -24,6 +24,7 @@ import { bootCron } from './boot-cron'; import { bootDebugRoutes } from './boot-debug'; import { bootWorkers } from './boot-workers'; import { register } from './metrics'; +import { basicAuth } from './utils/basic-auth'; import { isShuttingDown } from './utils/graceful-shutdown'; import { logger } from './utils/logger'; import { getEventsHeartbeat } from './utils/worker-heartbeat'; @@ -45,31 +46,6 @@ async function start() { bootDebugRoutes(app); } - if ( - process.env.DISABLE_BULLBOARD !== '1' && - process.env.DISABLE_BULLBOARD !== 'true' - ) { - const serverAdapter = new ExpressAdapter(); - serverAdapter.setBasePath('/'); - createBullBoard({ - queues: [ - ...eventsGroupQueues.map( - (queue) => new BullBoardGroupMQAdapter(queue) as any - ), - new BullMQAdapter(sessionsQueue), - new BullMQAdapter(cronQueue), - new BullMQAdapter(notificationQueue), - new BullMQAdapter(importQueue), - new BullMQAdapter(insightsQueue), - new BullMQAdapter(gscQueue), - new BullMQAdapter(cohortComputeQueue), - ], - serverAdapter, - }); - - app.use('/', serverAdapter.getRouter()); - } - app.get('/metrics', (req, res) => { res.set('Content-Type', register.contentType); register @@ -127,6 +103,50 @@ async function start() { failedDependencies, workingDependencies, }); + + // Bull Board exposes every queue with add/retry/clean enabled, so it never + // mounts without credentials (GHSA-r627-6vrh-65p9). /metrics and + // /healthcheck are registered above so the auth guard does not cover them. + const bullboardDisabled = + process.env.DISABLE_BULLBOARD === '1' || + process.env.DISABLE_BULLBOARD === 'true'; + const bullboardUsername = process.env.BULLBOARD_USERNAME; + const bullboardPassword = process.env.BULLBOARD_PASSWORD; + + const hasBullboardCredentials = Boolean(bullboardUsername && bullboardPassword); + + if (!bullboardDisabled && !hasBullboardCredentials) { + logger.warn( + 'Bull Board is not mounted: set BULLBOARD_USERNAME and BULLBOARD_PASSWORD to enable the queue dashboard, or DISABLE_BULLBOARD=true to silence this warning', + ); + } + + if (!bullboardDisabled && bullboardUsername && bullboardPassword) { + const serverAdapter = new ExpressAdapter(); + serverAdapter.setBasePath('/'); + createBullBoard({ + queues: [ + ...eventsGroupQueues.map( + (queue) => new BullBoardGroupMQAdapter(queue) as any + ), + new BullMQAdapter(sessionsQueue), + new BullMQAdapter(cronQueue), + new BullMQAdapter(notificationQueue), + new BullMQAdapter(importQueue), + new BullMQAdapter(insightsQueue), + new BullMQAdapter(gscQueue), + new BullMQAdapter(cohortComputeQueue), + ], + serverAdapter, + }); + + app.use( + '/', + basicAuth(bullboardUsername, bullboardPassword), + serverAdapter.getRouter(), + ); + } + }); // Kubernetes liveness — shallow, event loop only. diff --git a/apps/worker/src/utils/basic-auth.test.ts b/apps/worker/src/utils/basic-auth.test.ts new file mode 100644 index 000000000..e10a092bd --- /dev/null +++ b/apps/worker/src/utils/basic-auth.test.ts @@ -0,0 +1,43 @@ +import { describe, expect, it, vi } from 'vitest'; +import { basicAuth } from './basic-auth'; + +function run(header?: string) { + const req = { headers: { authorization: header } } as never; + const res = { set: vi.fn(), status: vi.fn().mockReturnThis(), send: vi.fn() }; + const next = vi.fn(); + basicAuth('ops', 's3cret')(req, res as never, next); + return { res, next }; +} + +const encode = (s: string) => `Basic ${Buffer.from(s).toString('base64')}`; + +describe('basicAuth', () => { + it('lets the right credentials through', () => { + const { next, res } = run(encode('ops:s3cret')); + expect(next).toHaveBeenCalled(); + expect(res.status).not.toHaveBeenCalled(); + }); + + it('rejects a missing header', () => { + const { next, res } = run(undefined); + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(401); + expect(res.set).toHaveBeenCalledWith('WWW-Authenticate', expect.stringContaining('Basic')); + }); + + it('rejects a wrong password, wrong user and malformed value', () => { + for (const header of [encode('ops:nope'), encode('root:s3cret'), encode('no-colon'), 'Bearer abc']) { + const { next, res } = run(header); + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(401); + } + }); + + it('allows a colon inside the password', () => { + const req = { headers: { authorization: encode('ops:a:b') } } as never; + const res = { set: vi.fn(), status: vi.fn().mockReturnThis(), send: vi.fn() }; + const next = vi.fn(); + basicAuth('ops', 'a:b')(req, res as never, next); + expect(next).toHaveBeenCalled(); + }); +}); diff --git a/apps/worker/src/utils/basic-auth.ts b/apps/worker/src/utils/basic-auth.ts new file mode 100644 index 000000000..b6b2235ac --- /dev/null +++ b/apps/worker/src/utils/basic-auth.ts @@ -0,0 +1,34 @@ +import { timingSafeEqual } from 'node:crypto'; +import type { NextFunction, Request, Response } from 'express'; + +function safeEqual(a: string, b: string): boolean { + const bufA = Buffer.from(a); + const bufB = Buffer.from(b); + return bufA.length === bufB.length && timingSafeEqual(bufA, bufB); +} + +/** + * HTTP Basic auth for operator-only routes such as Bull Board. Browsers + * prompt for the credentials, so no extra UI is needed. + */ +export function basicAuth(username: string, password: string) { + return (req: Request, res: Response, next: NextFunction) => { + const [scheme, encoded] = (req.headers.authorization ?? '').split(' '); + if (scheme === 'Basic' && encoded) { + const decoded = Buffer.from(encoded, 'base64').toString('utf8'); + const separator = decoded.indexOf(':'); + const givenUser = decoded.slice(0, separator); + const givenPassword = decoded.slice(separator + 1); + if ( + separator !== -1 && + safeEqual(givenUser, username) && + safeEqual(givenPassword, password) + ) { + next(); + return; + } + } + res.set('WWW-Authenticate', 'Basic realm="OpenPanel worker"'); + res.status(401).send('Unauthorized'); + }; +} diff --git a/packages/db/code-migrations/migrate.ts b/packages/db/code-migrations/migrate.ts index 1598f0602..874431d3a 100644 --- a/packages/db/code-migrations/migrate.ts +++ b/packages/db/code-migrations/migrate.ts @@ -14,6 +14,38 @@ import { printBoxMessage, } from './helpers'; +const CREDENTIAL_QUERY_PARAMS = new Set(['password', 'sslpassword', 'user']); + +/** + * Connection URLs carry credentials; the startup banner must not put them in + * container and CI logs. Keeps scheme, host, port and database, drops the + * rest. ClickHouse accepts a comma-separated list, so each URL is handled. + */ +function redactConnectionUrls(value: string | undefined): string { + if (!value) { + return '(not set)'; + } + return value + .split(',') + .map((raw) => { + try { + const url = new URL(raw.trim()); + const params = new URLSearchParams(url.search); + for (const key of [...params.keys()]) { + if (CREDENTIAL_QUERY_PARAMS.has(key.toLowerCase())) { + params.set(key, '***'); + } + } + const query = params.toString(); + const auth = url.username ? '***@' : ''; + return `${url.protocol}//${auth}${url.host}${url.pathname}${query ? `?${query}` : ''}`; + } catch { + return '(unparseable url)'; + } + }) + .join(','); +} + async function migrate() { const args = process.argv.slice(2); const migration = args.filter((arg) => !arg.startsWith('--'))[0]; @@ -58,8 +90,8 @@ async function migrate() { ]); printBoxMessage('🌍 Environment', [ - `POSTGRES: ${process.env.DATABASE_URL}`, - `CLICKHOUSE: ${process.env.CLICKHOUSE_URL}`, + `POSTGRES: ${redactConnectionUrls(process.env.DATABASE_URL)}`, + `CLICKHOUSE: ${redactConnectionUrls(process.env.CLICKHOUSE_URL)}`, ]); if (!getIsSelfHosting()) { diff --git a/packages/db/src/services/cohort.service.ts b/packages/db/src/services/cohort.service.ts index 7c72b9c4e..32fe26837 100644 --- a/packages/db/src/services/cohort.service.ts +++ b/packages/db/src/services/cohort.service.ts @@ -303,9 +303,22 @@ export function buildEventCriteriaQuery( `; } +/** Columns of the profiles table a cohort filter may reference directly. */ +const PROFILE_FILTER_COLUMNS = new Set([ + 'id', + 'first_name', + 'last_name', + 'email', + 'avatar', + 'created_at', + 'last_seen_at', +]); + // SQL for a profile filter's column: either a properties Map lookup or a -// plain column, qualified with the table name. -function profileColumnAccess(name: string): string { +// plain column, qualified with the table name. The column name is an +// identifier and cannot be escaped like a value, so it must come from the +// allowlist (GHSA-gvwr-5684-wjqc). +export function profileColumnAccess(name: string): string { const normalizedName = name.replace(/^profile\./, 'profiles.'); if (normalizedName.startsWith('profiles.properties.')) { const propKey = normalizedName.replace('profiles.properties.', ''); @@ -313,7 +326,11 @@ function profileColumnAccess(name: string): string { // user-controlled — a quote in it must not terminate the literal. return `profiles.properties[${sqlstring.escape(propKey)}]`; } - return normalizedName; + const column = normalizedName.replace(/^profiles\./, ''); + if (!PROFILE_FILTER_COLUMNS.has(column)) { + throw new Error(`Unknown profile filter column: ${name}`); + } + return `profiles.${column}`; } function buildProfileCohortHavingClause( diff --git a/packages/db/src/services/conversion.service.ts b/packages/db/src/services/conversion.service.ts index fc721722d..9637d31e6 100644 --- a/packages/db/src/services/conversion.service.ts +++ b/packages/db/src/services/conversion.service.ts @@ -134,12 +134,14 @@ export class ConversionService { const funnelWindowSeconds = funnelWindow * 3600; // Build funnel conditions + const eventAName = sqlstring.escape(eventA.name); + const eventBName = sqlstring.escape(eventB.name); const conditionA = whereA - ? `(events.name = '${eventA.name}' AND ${whereA})` - : `events.name = '${eventA.name}'`; + ? `(events.name = ${eventAName} AND ${whereA})` + : `events.name = ${eventAName}`; const conditionB = whereB - ? `(events.name = '${eventB.name}' AND ${whereB})` - : `events.name = '${eventB.name}'`; + ? `(events.name = ${eventBName} AND ${whereB})` + : `events.name = ${eventBName}`; const groupJoin = needsGroupArrayJoin ? `ARRAY JOIN groups AS _group_id LEFT ANY JOIN (SELECT id, name, type, properties FROM ${TABLE_NAMES.groups} FINAL WHERE project_id = ${sqlstring.escape(projectId)}) AS _g ON _g.id = _group_id` @@ -175,9 +177,9 @@ export class ConversionService { ${profileJoin} ${groupJoin} ${cohortJoinsSql} - WHERE project_id = '${projectId}' - AND events.name IN ('${eventA.name}', '${eventB.name}') - AND created_at BETWEEN toDateTime('${startDate}') AND toDateTime('${endDate}') + WHERE project_id = ${sqlstring.escape(projectId)} + AND events.name IN (${eventAName}, ${eventBName}) + AND created_at BETWEEN toDateTime(${sqlstring.escape(startDate)}) AND toDateTime(${sqlstring.escape(endDate)}) GROUP BY ${group}${breakdownExpressions.length ? `, ${breakdownExpressions.join(', ')}` : ''}) `), ) diff --git a/packages/db/src/services/sankey.service.ts b/packages/db/src/services/sankey.service.ts index af1844612..ee0df68eb 100644 --- a/packages/db/src/services/sankey.service.ts +++ b/packages/db/src/services/sankey.service.ts @@ -1,5 +1,6 @@ import { chartColors } from '@openpanel/constants'; import { type IChartEventFilter, zChartEvent } from '@openpanel/validation'; +import sqlstring from 'sqlstring'; import { z } from 'zod'; import { TABLE_NAMES, ch } from '../clickhouse/client'; import { clix } from '../clickhouse/query-builder'; @@ -58,13 +59,13 @@ export class SankeyService { if (include && include.length > 0) { const eventNames = [...include, startEventName, endEventName] .filter((item) => item !== undefined) - .map((e) => `'${e!.replace(/'/g, "''")}'`) + .map((e) => sqlstring.escape(e)) .join(', '); return `name IN (${eventNames})`; } if (exclude.length > 0) { const excludedNames = exclude - .map((e) => `'${e.replace(/'/g, "''")}'`) + .map((e) => sqlstring.escape(e)) .join(', '); return `name NOT IN (${excludedNames})`; } @@ -102,40 +103,40 @@ export class SankeyService { const defaultSliceExpr = `arraySlice(events_deduped, 1, ${steps})`; if (mode === 'after' && startEvent) { - const escapedStartEvent = startEvent.name.replace(/'/g, "''"); + const escapedStartEvent = sqlstring.escape(startEvent.name); const sessionFilter = hasStartEventCTE ? 'session_id IN (SELECT session_id FROM start_event_sessions)' - : `arrayExists(x -> x = '${escapedStartEvent}', events_deduped)`; - const eventsSliceExpr = `arraySlice(events_deduped, arrayFirstIndex(x -> x = '${escapedStartEvent}', events_deduped), ${steps})`; + : `arrayExists(x -> x = ${escapedStartEvent}, events_deduped)`; + const eventsSliceExpr = `arraySlice(events_deduped, arrayFirstIndex(x -> x = ${escapedStartEvent}, events_deduped), ${steps})`; return { sessionFilter, eventsSliceExpr }; } if (mode === 'before' && startEvent) { - const escapedStartEvent = startEvent.name.replace(/'/g, "''"); + const escapedStartEvent = sqlstring.escape(startEvent.name); const sessionFilter = hasStartEventCTE ? 'session_id IN (SELECT session_id FROM start_event_sessions)' - : `arrayExists(x -> x = '${escapedStartEvent}', events_deduped)`; + : `arrayExists(x -> x = ${escapedStartEvent}, events_deduped)`; const eventsSliceExpr = `arraySlice( events_deduped, - greatest(1, arrayFirstIndex(x -> x = '${escapedStartEvent}', events_deduped) - ${steps} + 1), - arrayFirstIndex(x -> x = '${escapedStartEvent}', events_deduped) - greatest(1, arrayFirstIndex(x -> x = '${escapedStartEvent}', events_deduped) - ${steps} + 1) + 1 + greatest(1, arrayFirstIndex(x -> x = ${escapedStartEvent}, events_deduped) - ${steps} + 1), + arrayFirstIndex(x -> x = ${escapedStartEvent}, events_deduped) - greatest(1, arrayFirstIndex(x -> x = ${escapedStartEvent}, events_deduped) - ${steps} + 1) + 1 )`; return { sessionFilter, eventsSliceExpr }; } if (mode === 'between' && startEvent && endEvent) { - const escapedStartEvent = startEvent.name.replace(/'/g, "''"); - const escapedEndEvent = endEvent.name.replace(/'/g, "''"); + const escapedStartEvent = sqlstring.escape(startEvent.name); + const escapedEndEvent = sqlstring.escape(endEvent.name); let sessionFilter = ''; if (hasStartEventCTE && hasEndEventCTE) { sessionFilter = 'session_id IN (SELECT session_id FROM start_event_sessions) AND session_id IN (SELECT session_id FROM end_event_sessions)'; } else if (hasStartEventCTE) { - sessionFilter = `session_id IN (SELECT session_id FROM start_event_sessions) AND arrayExists(x -> x = '${escapedEndEvent}', events_deduped)`; + sessionFilter = `session_id IN (SELECT session_id FROM start_event_sessions) AND arrayExists(x -> x = ${escapedEndEvent}, events_deduped)`; } else if (hasEndEventCTE) { - sessionFilter = `arrayExists(x -> x = '${escapedStartEvent}', events_deduped) AND session_id IN (SELECT session_id FROM end_event_sessions)`; + sessionFilter = `arrayExists(x -> x = ${escapedStartEvent}, events_deduped) AND session_id IN (SELECT session_id FROM end_event_sessions)`; } else { - sessionFilter = `arrayExists(x -> x = '${escapedStartEvent}', events_deduped) AND arrayExists(x -> x = '${escapedEndEvent}', events_deduped)`; + sessionFilter = `arrayExists(x -> x = ${escapedStartEvent}, events_deduped) AND arrayExists(x -> x = ${escapedEndEvent}, events_deduped)`; } return { sessionFilter, eventsSliceExpr: defaultSliceExpr }; } @@ -172,8 +173,8 @@ export class SankeyService { }>([ 'session_id', 'events', - `arrayFirstIndex(x -> x = '${startEvent.name.replace(/'/g, "''")}', events) as start_index`, - `arrayFirstIndex(x -> x = '${endEvent.name.replace(/'/g, "''")}', events) as end_index`, + `arrayFirstIndex(x -> x = ${sqlstring.escape(startEvent.name)}, events) as start_index`, + `arrayFirstIndex(x -> x = ${sqlstring.escape(endEvent.name)}, events) as end_index`, ]) .from('session_paths') .having('start_index', '>', 0) diff --git a/packages/db/src/services/share.service.ts b/packages/db/src/services/share.service.ts index 3a53f399c..f6ba3b6eb 100644 --- a/packages/db/src/services/share.service.ts +++ b/packages/db/src/services/share.service.ts @@ -3,6 +3,57 @@ import { hasShareAccess } from '@openpanel/common/server/share-access'; import { db } from '../prisma-client'; import { getProjectAccess } from './access.service'; +export type ShareKind = 'overview' | 'dashboard' | 'report'; + +/** + * Whether an anonymous viewer may read project-scoped side data (chart + * annotations, bot events) for a project. + * + * The viewer is allowed only if the project has at least one share of the + * given kinds that is public and, when password-protected, has been unlocked + * with a verified cookie. A share row merely existing is not enough: shares + * are toggled off by setting `public: false` (the row stays), and a password + * share must not leak through a side endpoint that the share page itself + * would refuse. + */ +export async function hasAnonymousShareAccessToProject( + projectId: string, + cookies: Record | undefined, + kinds: ShareKind[] = ['overview', 'dashboard', 'report'], +): Promise { + const select = { id: true, password: true } as const; + const where = { projectId, public: true } as const; + + const [overviews, dashboards, reports] = await Promise.all([ + kinds.includes('overview') + ? db.shareOverview.findMany({ where, select }) + : [], + kinds.includes('dashboard') + ? db.shareDashboard.findMany({ where, select }) + : [], + kinds.includes('report') + ? db.shareReport.findMany({ where, select }) + : [], + ]); + + const candidates: { type: ShareKind; id: string; password: string | null }[] = + [ + ...overviews.map((s) => ({ type: 'overview' as const, ...s })), + ...dashboards.map((s) => ({ type: 'dashboard' as const, ...s })), + ...reports.map((s) => ({ type: 'report' as const, ...s })), + ]; + + return candidates.some( + (share) => + !share.password || + hasShareAccess(cookies, { + type: share.type, + id: share.id, + passwordHash: share.password, + }), + ); +} + export function getShareOverviewById(id: string) { return db.shareOverview.findFirst({ where: { diff --git a/packages/email/src/index.tsx b/packages/email/src/index.tsx index d27437dab..89d5e47de 100644 --- a/packages/email/src/index.tsx +++ b/packages/email/src/index.tsx @@ -8,6 +8,15 @@ import { db } from '@openpanel/db'; import { type TemplateKey, type Templates, templates } from './emails'; import { getUnsubscribeUrl } from './unsubscribe'; +/** a***@example.com, enough to correlate a log line without exposing the address. */ +function redactEmail(email: string): string { + const at = email.indexOf('@'); + if (at <= 0) { + return '***'; + } + return `${email[0]}***${email.slice(at)}`; +} + export * from './unsubscribe'; const FROM = process.env.EMAIL_SENDER ?? 'hello@openpanel.dev'; @@ -95,11 +104,12 @@ export async function sendEmail( } if (!process.env.RESEND_API_KEY) { - console.log('No SMTP_HOST or RESEND_API_KEY found, here is the data'); - console.log('Template:', template); - console.log('Subject: ', subject); - console.log('To: ', to); - console.log('Data: ', JSON.stringify(data, null, 2)); + // Never dump the payload: template data carries password-reset and + // unsubscribe links, and the recipient is personal data + // (GHSA-xr2x-w49w-hp2c). + console.warn( + `Email not sent (email_provider_not_configured): template=${templateKey} to=${redactEmail(to)}`, + ); return null; } diff --git a/packages/email/src/unsubscribe.ts b/packages/email/src/unsubscribe.ts index 1a6ced129..7a09323e2 100644 --- a/packages/email/src/unsubscribe.ts +++ b/packages/email/src/unsubscribe.ts @@ -1,14 +1,23 @@ import { createHmac, timingSafeEqual } from 'crypto'; -const SECRET = - process.env.UNSUBSCRIBE_SECRET || - process.env.COOKIE_SECRET || - process.env.SECRET || - 'default-secret-change-in-production'; +/** + * Read at call time, not import time, so a missing secret fails the one + * request that needs it with a clear message instead of silently signing + * with a well-known default that anyone could forge. + */ +function getSecret(): string { + const secret = process.env.UNSUBSCRIBE_SECRET || process.env.COOKIE_SECRET; + if (!secret) { + throw new Error( + 'UNSUBSCRIBE_SECRET or COOKIE_SECRET must be set to sign unsubscribe links', + ); + } + return secret; +} export function generateUnsubscribeToken(email: string, category: string): string { const data = `${email}:${category}`; - return createHmac('sha256', SECRET).update(data).digest('hex'); + return createHmac('sha256', getSecret()).update(data).digest('hex'); } export function verifyUnsubscribeToken( diff --git a/packages/trpc/src/routers/event.ts b/packages/trpc/src/routers/event.ts index 018b8aaf4..0177061e7 100644 --- a/packages/trpc/src/routers/event.ts +++ b/packages/trpc/src/routers/event.ts @@ -3,9 +3,6 @@ import sqlstring from 'sqlstring'; import { z } from 'zod'; import { - type IServiceProfile, - type IServiceSession, - TABLE_NAMES, chQuery, convertClickhouseDateToJs, db, @@ -15,8 +12,12 @@ import { getEventList, getEventMetasCached, getSettingsForProject, + hasAnonymousShareAccessToProject, pagesService, sessionService, + TABLE_NAMES, + type IServiceProfile, + type IServiceSession, } from '@openpanel/db'; import { zChartEventFilter, @@ -290,13 +291,14 @@ export const eventRouter = createTRPCRouter({ throw new TRPCForbiddenError('You do not have access to this project'); } } else { - const share = await db.shareOverview.findFirst({ - where: { - projectId, - }, - }); - - if (!share) { + // Anonymous callers only see bot events through an unlocked public + // overview share; the row existing is not enough (GHSA-r4g5-vgpj-923m). + const allowed = await hasAnonymousShareAccessToProject( + projectId, + ctx.cookies, + ['overview'], + ); + if (!allowed) { throw new TRPCForbiddenError('You do not have access to this project'); } } diff --git a/packages/trpc/src/routers/reference.test.ts b/packages/trpc/src/routers/reference.test.ts new file mode 100644 index 000000000..607292493 --- /dev/null +++ b/packages/trpc/src/routers/reference.test.ts @@ -0,0 +1,82 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const { referenceFindMany, hasAnonymousShareAccessToProject, getProjectAccess } = + vi.hoisted(() => ({ + referenceFindMany: vi.fn(), + hasAnonymousShareAccessToProject: vi.fn(), + getProjectAccess: vi.fn(), + })); + +vi.mock('@openpanel/db', () => ({ + db: { reference: { findMany: referenceFindMany } }, + getChartStartEndDate: () => ({ + startDate: '2024-01-01T00:00:00.000Z', + endDate: '2024-02-01T00:00:00.000Z', + }), + getSettingsForProject: vi.fn().mockResolvedValue({ timezone: 'UTC' }), + hasAnonymousShareAccessToProject, + getProjectAccess, + getOrganizationAccess: vi.fn(), + getClientAccess: vi.fn(), + canWriteProject: vi.fn(), + runWithAlsSession: (_id: unknown, fn: () => unknown) => fn(), +})); + +import { referenceRouter } from './reference'; + +// An unauthenticated request carries an empty session, not a missing one. +const caller = (session: { userId: string | null }, cookies = {}) => + referenceRouter.createCaller({ + req: { log: { info: vi.fn(), error: vi.fn() } }, + res: {}, + session, + setCookie: vi.fn(), + cookies, + } as never); + +const input = { + projectId: 'victim-project', + range: '30d' as const, + startDate: null, + endDate: null, +}; + +describe('reference.getChartReferences (GHSA-vrrm-p9p4-2gfg)', () => { + beforeEach(() => { + vi.clearAllMocks(); + referenceFindMany.mockResolvedValue([{ title: 'Deploy v2' }]); + }); + + it('refuses an anonymous caller when the project has no unlocked share', async () => { + hasAnonymousShareAccessToProject.mockResolvedValue(false); + await expect(caller({ userId: null }).getChartReferences(input)).rejects.toThrow( + 'do not have access', + ); + expect(referenceFindMany).not.toHaveBeenCalled(); + }); + + it('serves an anonymous caller who holds an unlocked public share', async () => { + hasAnonymousShareAccessToProject.mockResolvedValue(true); + const cookies = { 'shared-overview-abc': 'token' }; + const res = await caller({ userId: null }, cookies).getChartReferences(input); + expect(res).toEqual([{ title: 'Deploy v2' }]); + expect(hasAnonymousShareAccessToProject).toHaveBeenCalledWith( + 'victim-project', + cookies, + ); + }); + + it('refuses a member without access to the project', async () => { + getProjectAccess.mockResolvedValue(null); + await expect( + caller({ userId: 'u1' }).getChartReferences(input), + ).rejects.toThrow('do not have access'); + expect(hasAnonymousShareAccessToProject).not.toHaveBeenCalled(); + }); + + it('serves a member with access', async () => { + getProjectAccess.mockResolvedValue({ level: 'read' }); + const res = await caller({ userId: 'u1' }).getChartReferences(input); + expect(res).toEqual([{ title: 'Deploy v2' }]); + }); +}); diff --git a/packages/trpc/src/routers/reference.ts b/packages/trpc/src/routers/reference.ts index 8f208a3cf..4a61f4908 100644 --- a/packages/trpc/src/routers/reference.ts +++ b/packages/trpc/src/routers/reference.ts @@ -1,6 +1,11 @@ import { z } from 'zod'; -import { db, getChartStartEndDate, getSettingsForProject } from '@openpanel/db'; +import { + db, + getChartStartEndDate, + getSettingsForProject, + hasAnonymousShareAccessToProject, +} from '@openpanel/db'; import { zCreateReference, zRange } from '@openpanel/validation'; import { getProjectAccess, requireProjectAccess } from '../access'; @@ -107,7 +112,29 @@ export const referenceRouter = createTRPCRouter({ range: zRange, }), ) - .query(async ({ input: { projectId, ...input } }) => { + .query(async ({ input: { projectId, ...input }, ctx }) => { + // Public so that share pages can draw annotations on their charts, but + // never without a check: a member needs project access, an anonymous + // viewer needs an unlocked public share for the project + // (GHSA-vrrm-p9p4-2gfg). + if (ctx.session.userId) { + const access = await getProjectAccess({ + userId: ctx.session.userId, + projectId, + }); + if (!access) { + throw new TRPCForbiddenError('You do not have access to this project'); + } + } else { + const allowed = await hasAnonymousShareAccessToProject( + projectId, + ctx.cookies, + ); + if (!allowed) { + throw new TRPCForbiddenError('You do not have access to this project'); + } + } + const { timezone } = await getSettingsForProject(projectId); const { startDate, endDate } = getChartStartEndDate(input, timezone); return db.reference.findMany({ diff --git a/self-hosting/coolify.yml b/self-hosting/coolify.yml index aeef6b301..49ca43e87 100644 --- a/self-hosting/coolify.yml +++ b/self-hosting/coolify.yml @@ -205,9 +205,14 @@ services: environment: # FQDN - SERVICE_FQDN_OPBULLBOARD + # Bull Board is only served with credentials (HTTP Basic auth) + - BULLBOARD_USERNAME=${SERVICE_USER_BULLBOARD} + - BULLBOARD_PASSWORD=${SERVICE_PASSWORD_BULLBOARD} # Common - NODE_ENV=production - SELF_HOSTED=true + # Must match the API's secret: unsubscribe links in worker emails are verified by the API + - COOKIE_SECRET=${SERVICE_BASE64_COOKIESECRET} # URLs - DATABASE_URL=postgres://${SERVICE_USER_POSTGRES}:${SERVICE_PASSWORD_POSTGRES}@opdb:5432/${OPENPANEL_POSTGRES_DB:-openpanel-db}?schema=public - DATABASE_URL_DIRECT=postgres://${SERVICE_USER_POSTGRES}:${SERVICE_PASSWORD_POSTGRES}@opdb:5432/${OPENPANEL_POSTGRES_DB:-openpanel-db}?schema=public