diff --git a/src/permissions/website.test.ts b/src/permissions/website.test.ts index dbc53cab2..b9c00a103 100644 --- a/src/permissions/website.test.ts +++ b/src/permissions/website.test.ts @@ -279,12 +279,11 @@ describe('canTransferWebsiteToUser', () => { ).resolves.toBe(false); }); - // NOTE: admins are NOT short-circuited here; there is no isAdmin bypass in this function. - test('denies an admin transferring a user-owned website (no team)', async () => { - vi.mocked(getWebsite).mockResolvedValue({ userId: 'admin-1' } as any); + test('allows admins without any lookup', async () => { await expect( canTransferWebsiteToUser({ user: adminUser }, 'website-1', 'admin-1'), - ).resolves.toBe(false); + ).resolves.toBe(true); + expect(getWebsite).not.toHaveBeenCalled(); }); test('allows a team owner transferring a team website to themselves', async () => { @@ -316,6 +315,13 @@ describe('canTransferWebsiteToTeam', () => { await expect(canTransferWebsiteToTeam({}, 'website-1', 'team-1')).resolves.toBe(false); }); + test('allows admins without any lookup', async () => { + await expect( + canTransferWebsiteToTeam({ user: adminUser }, 'website-1', 'team-1'), + ).resolves.toBe(true); + expect(getWebsite).not.toHaveBeenCalled(); + }); + test('denies when website is missing', async () => { vi.mocked(getWebsite).mockResolvedValue(null as any); await expect( diff --git a/src/permissions/website.ts b/src/permissions/website.ts index e549b615e..c01c3d70d 100644 --- a/src/permissions/website.ts +++ b/src/permissions/website.ts @@ -83,7 +83,9 @@ export async function canViewBatchWebsites({ user, shareToken }: Auth, websiteId websites.filter(website => website.userId === user.id).map(website => website.id), ); const teamIds = Array.from( - new Set(websites.map(website => website.teamId).filter((teamId): teamId is string => Boolean(teamId))), + new Set( + websites.map(website => website.teamId).filter((teamId): teamId is string => Boolean(teamId)), + ), ); const teamUsers = teamIds.length ? await prisma.client.teamUser.findMany({ @@ -187,6 +189,10 @@ export async function canTransferWebsiteToUser({ user }: Auth, websiteId: string return false; } + if (user.isAdmin) { + return true; + } + const website = await getWebsite(websiteId); if (!website) { @@ -207,6 +213,10 @@ export async function canTransferWebsiteToTeam({ user }: Auth, websiteId: string return false; } + if (user.isAdmin) { + return true; + } + const website = await getWebsite(websiteId); if (!website) { diff --git a/src/queries/sql/getChannelExpandedMetrics.test.ts b/src/queries/sql/getChannelExpandedMetrics.test.ts index b9b403138..452071e86 100644 --- a/src/queries/sql/getChannelExpandedMetrics.test.ts +++ b/src/queries/sql/getChannelExpandedMetrics.test.ts @@ -102,8 +102,13 @@ describe('getChannelExpandedMetrics postgres branch', () => { await getChannelExpandedMetrics('website-1', {}); const [query] = prismaRawQuery.mock.calls[0]; + // LIKE wildcards/backslashes are escaped so values match literally (e.g. the + // underscores in PAID_AD_PARAMS like `ad_id=` become `ad\_id=`). + const escapeLike = (val: string) => + val.replace(/\\/g, '\\\\').replace(/%/g, '\\%').replace(/_/g, '\\_').replace(/'/g, "''"); + for (const val of PAID_AD_PARAMS) { - expect(query).toContain(`url_query ilike '%${val}%'`); + expect(query).toContain(`url_query ilike '%${escapeLike(val)}%'`); } for (const domainList of [ LLM_DOMAINS, @@ -114,10 +119,22 @@ describe('getChannelExpandedMetrics postgres branch', () => { VIDEO_DOMAINS, ]) { for (const val of domainList) { - expect(query).toContain(`referrer_domain ilike '%${val}%'`); + expect(query).toContain(`referrer_domain ilike '%${escapeLike(val)}%'`); } } }); + + test('returns the query rows unchanged without a spurious y column', async () => { + const rows = [ + { name: 'direct', pageviews: 10, visitors: 4, visits: 5, bounces: 1, totaltime: 100 }, + ]; + prismaRawQuery.mockResolvedValue(rows); + + const result = await getChannelExpandedMetrics('website-1', {}); + + expect(result).toEqual(rows); + expect(result[0]).not.toHaveProperty('y'); + }); }); describe('getChannelExpandedMetrics clickhouse branch', () => { diff --git a/src/queries/sql/getChannelExpandedMetrics.ts b/src/queries/sql/getChannelExpandedMetrics.ts index 71985a831..6f225f9eb 100644 --- a/src/queries/sql/getChannelExpandedMetrics.ts +++ b/src/queries/sql/getChannelExpandedMetrics.ts @@ -138,7 +138,7 @@ async function relationalQuery( `, queryParams, FUNCTION_NAME, - ).then(results => results.map(item => ({ ...item, y: Number(item.y) }))); + ); } async function clickhouseQuery( @@ -222,9 +222,16 @@ async function clickhouseQuery( } function toClickHouseStringArray(arr: string[]): string { - return arr.map(p => `'${p.replace(/'/g, "\\'")}'`).join(', '); + // Escape backslashes first, then single quotes, for ClickHouse string literals. + return arr.map(p => `'${p.replace(/\\/g, '\\\\').replace(/'/g, "\\'")}'`).join(', '); +} + +function escapePostgresLikeValue(val: string) { + // Escape LIKE wildcards/backslashes so the value is matched literally, then + // escape single quotes for the surrounding SQL string literal. + return val.replace(/\\/g, '\\\\').replace(/%/g, '\\%').replace(/_/g, '\\_').replace(/'/g, "''"); } function toPostgresPositionClause(column: string, arr: string[]) { - return arr.map(val => `${column} ilike '%${val.replace(/'/g, "''")}%'`).join(' OR\n '); + return arr.map(val => `${column} ilike '%${escapePostgresLikeValue(val)}%'`).join(' OR\n '); } diff --git a/src/queries/sql/getChannelMetrics.test.ts b/src/queries/sql/getChannelMetrics.test.ts index c5367dfca..618f35a0e 100644 --- a/src/queries/sql/getChannelMetrics.test.ts +++ b/src/queries/sql/getChannelMetrics.test.ts @@ -101,11 +101,16 @@ describe('getChannelMetrics postgres branch', () => { await getChannelMetrics('website-1', {}); const [query] = prismaRawQuery.mock.calls[0]; + // LIKE wildcards/backslashes are escaped so values match literally (e.g. the + // underscores in PAID_AD_PARAMS like `ad_id=` become `ad\_id=`). + const escapeLike = (val: string) => + val.replace(/\\/g, '\\\\').replace(/%/g, '\\%').replace(/_/g, '\\_').replace(/'/g, "''"); + for (const val of PAID_AD_PARAMS) { - expect(query).toContain(`url_query ilike '%${val}%'`); + expect(query).toContain(`url_query ilike '%${escapeLike(val)}%'`); } for (const val of SEARCH_DOMAINS) { - expect(query).toContain(`referrer_domain ilike '%${val}%'`); + expect(query).toContain(`referrer_domain ilike '%${escapeLike(val)}%'`); } for (const domainList of [ LLM_DOMAINS, @@ -115,7 +120,7 @@ describe('getChannelMetrics postgres branch', () => { VIDEO_DOMAINS, ]) { for (const val of domainList) { - expect(query).toContain(`referrer_domain ilike '%${val}%'`); + expect(query).toContain(`referrer_domain ilike '%${escapeLike(val)}%'`); } } // referral clause uses a literal array, not a constant @@ -132,21 +137,19 @@ describe('getChannelMetrics postgres branch', () => { expect(result).toEqual([{ x: 'direct', y: 42 }]); }); - // NOTE: toPostgresLikeClause only escapes single quotes (' -> ''). LIKE - // wildcards (% and _) and backslashes pass through unescaped. This is safe - // today because the helper is private and only ever receives the hardcoded - // constant arrays, but it would be an escaping gap if user input reached it. - // See getChannelMetrics.ts:180-182. - test('single-quote escaping doubles quotes (documenting current helper behavior)', async () => { - // None of the real constants contain quotes, so assert the round-trip of - // the escaping regex against every constant value: no bare `''` artifacts - // should appear inside a domain fragment. + // toPostgresLikeClause escapes LIKE wildcards (% and _) and backslashes so the + // value matches literally, then escapes single quotes for the SQL string + // literal. The hardcoded constants contain none of these, so their rendered + // form is unchanged; this asserts each constant appears intact in the clause. + test('renders an ilike clause for each escaped constant value', async () => { await getChannelMetrics('website-1', {}); const [query] = prismaRawQuery.mock.calls[0]; + const escapeLike = (val: string) => + val.replace(/\\/g, '\\\\').replace(/%/g, '\\%').replace(/_/g, '\\_').replace(/'/g, "''"); + for (const val of [...PAID_AD_PARAMS, ...LLM_DOMAINS, ...SEARCH_DOMAINS]) { - const escaped = val.replace(/'/g, "''"); - expect(query).toContain(`'%${escaped}%'`); + expect(query).toContain(`'%${escapeLike(val)}%'`); } }); }); @@ -194,15 +197,15 @@ describe('getChannelMetrics clickhouse branch', () => { ); }); - // NOTE: toClickHouseStringArray escapes single quotes as \' but leaves - // backslashes and other metacharacters untouched (getChannelMetrics.ts:176-178). - // Safe today (constants only), documented here as current behavior. - test('clickhouse array escaping only handles single quotes', async () => { + // toClickHouseStringArray escapes backslashes and single quotes for ClickHouse + // string literals. The constants contain neither, so they render unchanged. + test('clickhouse array escaping handles backslashes and single quotes', async () => { await getChannelMetrics('website-1', {}); const [query] = clickhouseRawQuery.mock.calls[0]; for (const val of [...VIDEO_DOMAINS, ...SHOPPING_DOMAINS]) { - expect(query).toContain(`'${val.replace(/'/g, "\\'")}'`); + const escaped = val.replace(/\\/g, '\\\\').replace(/'/g, "\\'"); + expect(query).toContain(`'${escaped}'`); } }); }); diff --git a/src/queries/sql/getChannelMetrics.ts b/src/queries/sql/getChannelMetrics.ts index 98e2274a7..b369f26c9 100644 --- a/src/queries/sql/getChannelMetrics.ts +++ b/src/queries/sql/getChannelMetrics.ts @@ -174,9 +174,16 @@ async function clickhouseQuery( } function toClickHouseStringArray(arr: string[]): string { - return arr.map(p => `'${p.replace(/'/g, "\\'")}'`).join(', '); + // Escape backslashes first, then single quotes, for ClickHouse string literals. + return arr.map(p => `'${p.replace(/\\/g, '\\\\').replace(/'/g, "\\'")}'`).join(', '); +} + +function escapePostgresLikeValue(val: string) { + // Escape LIKE wildcards/backslashes so the value is matched literally, then + // escape single quotes for the surrounding SQL string literal. + return val.replace(/\\/g, '\\\\').replace(/%/g, '\\%').replace(/_/g, '\\_').replace(/'/g, "''"); } function toPostgresLikeClause(column: string, arr: string[]) { - return arr.map(val => `${column} ilike '%${val.replace(/'/g, "''")}%'`).join(' OR\n '); + return arr.map(val => `${column} ilike '%${escapePostgresLikeValue(val)}%'`).join(' OR\n '); }