fix: harden private analytics query validation#1799
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds layered input validation and sanitization for private analytics endpoints: new validation utilities and schemas, unsafe-text detection, normalized analytics limits, and safe PostgREST filter escaping; endpoints now validate and reject unsafe/invalid bodies before permission checks. Unit tests added for validation scenarios. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Endpoint as Endpoint Handler
participant Validator as Validation Layer
participant Backend as Backend Service
participant DB as Database
Client->>Endpoint: POST /private/(stats|devices) with body
Endpoint->>Validator: parseBody() → safeParse(schema)
alt invalid schema
Validator-->>Endpoint: validation errors
Endpoint-->>Client: 400 invalid_body (details)
else valid schema
Endpoint->>Validator: hasUnsafe* checks
alt unsafe content
Validator-->>Endpoint: unsafe detected
Endpoint-->>Client: 400 invalid_body (unsafe content)
else safe
Endpoint->>Endpoint: permission checks
Endpoint->>Backend: query with sanitized params (escaped search, normalized limit)
Backend->>DB: execute SQL with safe filters
DB-->>Backend: results
Backend-->>Endpoint: response
Endpoint-->>Client: 200 OK
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
supabase/functions/_backend/private/stats.ts (1)
136-145:⚠️ Potential issue | 🔴 CriticalFix type mismatch causing pipeline failure.
The schema allows
rangeStartandrangeEndto bestring | number, butreadStatsexpectsstring | undefined. This causes the TypeScript errors on lines 138-139. Apply the same conversion pattern used in the/exporthandler (lines 172-173).🐛 Proposed fix for type mismatch
return c.json(await readStats(c, { app_id: body.appId, - start_date: body.rangeStart, - end_date: body.rangeEnd, + start_date: body.rangeStart !== undefined ? String(body.rangeStart) : undefined, + end_date: body.rangeEnd !== undefined ? String(body.rangeEnd) : undefined, deviceIds: body.devicesId, search: body.search, order: body.order, limit: body.limit, actions: body.actions, }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/stats.ts` around lines 136 - 145, The TypeScript types for rangeStart/rangeEnd from the request body (body.rangeStart/body.rangeEnd) can be string|number but readStats expects string|undefined; fix by converting these fields to strings the same way as the /export handler does before calling readStats: pass start_date: body.rangeStart ? String(body.rangeStart) : undefined and end_date: body.rangeEnd ? String(body.rangeEnd) : undefined when calling readStats (reference the readStats call and the request body properties body.rangeStart/body.rangeEnd).
🧹 Nitpick comments (3)
supabase/functions/_backend/private/devices.ts (1)
59-72: Consider extracting duplicate helpers to shared utility.The
hasControlCharsandhasUnsafeQueryTextfunctions are duplicated instats.ts. Consider extracting them to a shared module (e.g.,utils/validation.ts) to maintain DRY principles and ensure consistent behavior across endpoints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/devices.ts` around lines 59 - 72, Extract the duplicated helpers hasControlChars and hasUnsafeQueryText into a new shared module (e.g., utils/validation.ts), export both functions, and replace the local implementations in devices.ts and stats.ts with imports from that module; ensure the new hasUnsafeQueryText keeps the same signature (value: string | undefined, maxLength = MAX_QUERY_TEXT_LENGTH) and that MAX_QUERY_TEXT_LENGTH is either exported from the same shared module or imported where used so behavior remains identical.supabase/functions/_backend/private/stats.ts (1)
59-72: Duplicate validation helpers - same observation as devices.ts.These helper functions (
hasControlChars,hasUnsafeQueryText) are identical to those indevices.ts. Consider extracting to a shared utility module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/stats.ts` around lines 59 - 72, Extract the duplicate validation helpers into a shared utility module and replace the copies with imports: move hasControlChars and hasUnsafeQueryText into a new shared file (e.g., validationUtils) and export them, then update this file and devices.ts to import hasControlChars and hasUnsafeQueryText from that module; ensure the exported function names remain unchanged so callers (hasControlChars, hasUnsafeQueryText) continue to work and keep the default parameter MAX_QUERY_TEXT_LENGTH handled by either exporting the constant or passing it through the shared helper.tests/private-analytics-validation.unit.test.ts (1)
41-47: Note: Concurrent tests may conflict with shared mock state.While I suggested using
it.concurrent(), be aware thatbeforeEachwithvi.clearAllMocks()can cause race conditions when tests run concurrently with shared mock references. Consider either:
- Using isolated mock instances per test, or
- Keeping synchronous execution if mock state isolation is difficult
The current synchronous approach is actually safer given the shared mock pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/private-analytics-validation.unit.test.ts` around lines 41 - 47, The shared mock state in beforeEach (vi.clearAllMocks(), checkPermissionMock, countDevicesMock, readDevicesMock, readStatsMock) can race when tests use it.concurrent(); either avoid concurrent tests or make each test use isolated mock instances—so either change tests back to synchronous it(...) or refactor to create fresh mocks inside each test (e.g., construct new checkPermissionMock/countDevicesMock/readDevicesMock/readStatsMock per test or wrap mock setup inside a factory called from each test) and remove reliance on global vi.clearAllMocks() to ensure no cross-test interference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/functions/_backend/private/stats.ts`:
- Line 54: ALLOWED_STATS_ACTIONS is currently a mutable string[] so
z.enum(ALLOWED_STATS_ACTIONS) can't infer literal types; edit the array
definition in stats_actions.ts (symbol: ALLOWED_STATS_ACTIONS) to append "as
const" (make it readonly literal tuple) so Zod 4's z.enum() receives literal
types, then keep using z.enum(ALLOWED_STATS_ACTIONS) in stats.ts unchanged.
In `@tests/private-analytics-validation.unit.test.ts`:
- Around line 50-128: Replace the synchronous tests using it(...) with
concurrent execution by changing each it(...) to it.concurrent(... ) for the
tests that call statsApp.request and devicesApp.request (the blocks invoking
statsApp.request/postJson, devicesApp.request/postJson and asserting via
expectInvalidBody, checkPermissionMock, readStatsMock, readDevicesMock,
countDevicesMock); ensure the existing beforeEach mock resets still run so mocks
used by expectInvalidBody and the checkPermissionMock/read* mocks remain
isolated when tests run in parallel.
---
Outside diff comments:
In `@supabase/functions/_backend/private/stats.ts`:
- Around line 136-145: The TypeScript types for rangeStart/rangeEnd from the
request body (body.rangeStart/body.rangeEnd) can be string|number but readStats
expects string|undefined; fix by converting these fields to strings the same way
as the /export handler does before calling readStats: pass start_date:
body.rangeStart ? String(body.rangeStart) : undefined and end_date:
body.rangeEnd ? String(body.rangeEnd) : undefined when calling readStats
(reference the readStats call and the request body properties
body.rangeStart/body.rangeEnd).
---
Nitpick comments:
In `@supabase/functions/_backend/private/devices.ts`:
- Around line 59-72: Extract the duplicated helpers hasControlChars and
hasUnsafeQueryText into a new shared module (e.g., utils/validation.ts), export
both functions, and replace the local implementations in devices.ts and stats.ts
with imports from that module; ensure the new hasUnsafeQueryText keeps the same
signature (value: string | undefined, maxLength = MAX_QUERY_TEXT_LENGTH) and
that MAX_QUERY_TEXT_LENGTH is either exported from the same shared module or
imported where used so behavior remains identical.
In `@supabase/functions/_backend/private/stats.ts`:
- Around line 59-72: Extract the duplicate validation helpers into a shared
utility module and replace the copies with imports: move hasControlChars and
hasUnsafeQueryText into a new shared file (e.g., validationUtils) and export
them, then update this file and devices.ts to import hasControlChars and
hasUnsafeQueryText from that module; ensure the exported function names remain
unchanged so callers (hasControlChars, hasUnsafeQueryText) continue to work and
keep the default parameter MAX_QUERY_TEXT_LENGTH handled by either exporting the
constant or passing it through the shared helper.
In `@tests/private-analytics-validation.unit.test.ts`:
- Around line 41-47: The shared mock state in beforeEach (vi.clearAllMocks(),
checkPermissionMock, countDevicesMock, readDevicesMock, readStatsMock) can race
when tests use it.concurrent(); either avoid concurrent tests or make each test
use isolated mock instances—so either change tests back to synchronous it(...)
or refactor to create fresh mocks inside each test (e.g., construct new
checkPermissionMock/countDevicesMock/readDevicesMock/readStatsMock per test or
wrap mock setup inside a factory called from each test) and remove reliance on
global vi.clearAllMocks() to ensure no cross-test interference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a81d5e3-790c-4830-bbe9-40b3f8a53d02
📒 Files selected for processing (4)
supabase/functions/_backend/private/devices.tssupabase/functions/_backend/private/stats.tssupabase/functions/_backend/utils/cloudflare.tstests/private-analytics-validation.unit.test.ts
| rangeStart: z.optional(z.union([safeQueryDateSchema, z.coerce.number()])), | ||
| rangeEnd: z.optional(z.union([safeQueryDateSchema, z.coerce.number()])), | ||
| limit: z.optional(z.coerce.number().check(z.minimum(1), z.maximum(MAX_QUERY_LIMIT))), | ||
| actions: z.optional(z.array(z.enum(ALLOWED_STATS_ACTIONS))), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify ALLOWED_STATS_ACTIONS is defined as const for Zod 4 compatibility
rg -n 'ALLOWED_STATS_ACTIONS' --type=ts -A3 supabase/functions/_backend/plugins/stats_actions.tsRepository: Cap-go/capgo
Length of output: 135
🏁 Script executed:
#!/bin/bash
# Get full definition of ALLOWED_STATS_ACTIONS
rg -n 'ALLOWED_STATS_ACTIONS' --type=ts -A20 supabase/functions/_backend/plugins/stats_actions.tsRepository: Cap-go/capgo
Length of output: 477
🏁 Script executed:
#!/bin/bash
# Verify the z.enum() usage context in stats.ts at line 54
cat -n supabase/functions/_backend/private/stats.ts | sed -n '50,60p'Repository: Cap-go/capgo
Length of output: 599
Add as const to ALLOWED_STATS_ACTIONS definition for Zod 4 type compatibility.
ALLOWED_STATS_ACTIONS in stats_actions.ts is currently defined as a regular array without as const, causing TypeScript to infer it as string[] instead of a literal union type. Zod 4's z.enum() requires literal types for proper type inference. Update the definition to:
export const ALLOWED_STATS_ACTIONS = [
'ping',
'delete',
'reset',
// ... rest of actions
] as const🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/functions/_backend/private/stats.ts` at line 54,
ALLOWED_STATS_ACTIONS is currently a mutable string[] so
z.enum(ALLOWED_STATS_ACTIONS) can't infer literal types; edit the array
definition in stats_actions.ts (symbol: ALLOWED_STATS_ACTIONS) to append "as
const" (make it readonly literal tuple) so Zod 4's z.enum() receives literal
types, then keep using z.enum(ALLOWED_STATS_ACTIONS) in stats.ts unchanged.
Comment on lines
+50
to
+128
| it('rejects malformed deviceIds on /private/stats', async () => { | ||
| const response = await statsApp.request(postJson('http://local/', { | ||
| appId: 'com.example.app', | ||
| devicesId: ['1) OR 1=1 --'], | ||
| })) | ||
|
|
||
| await expectInvalidBody(response) | ||
| expect(checkPermissionMock).not.toHaveBeenCalled() | ||
| expect(readStatsMock).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('rejects malformed actions on /private/stats', async () => { | ||
| const response = await statsApp.request(postJson('http://local/', { | ||
| appId: 'com.example.app', | ||
| actions: ['get', '\' OR 1=1 --'], | ||
| })) | ||
|
|
||
| await expectInvalidBody(response) | ||
| expect(checkPermissionMock).not.toHaveBeenCalled() | ||
| expect(readStatsMock).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('rejects non-numeric limits on /private/stats', async () => { | ||
| const response = await statsApp.request(postJson('http://local/', { | ||
| appId: 'com.example.app', | ||
| limit: '1 UNION SELECT 1', | ||
| })) | ||
|
|
||
| await expectInvalidBody(response) | ||
| expect(checkPermissionMock).not.toHaveBeenCalled() | ||
| expect(readStatsMock).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('rejects control characters in /private/stats search', async () => { | ||
| const response = await statsApp.request(postJson('http://local/', { | ||
| appId: 'com.example.app', | ||
| search: 'bad\u0000query', | ||
| })) | ||
|
|
||
| await expectInvalidBody(response) | ||
| expect(checkPermissionMock).not.toHaveBeenCalled() | ||
| expect(readStatsMock).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('rejects malformed deviceIds on /private/stats/export', async () => { | ||
| const response = await statsApp.request(postJson('http://local/export', { | ||
| appId: 'com.example.app', | ||
| devicesId: ['1) OR 1=1 --'], | ||
| format: 'json', | ||
| })) | ||
|
|
||
| await expectInvalidBody(response) | ||
| expect(checkPermissionMock).not.toHaveBeenCalled() | ||
| expect(readStatsMock).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('rejects malformed deviceIds on /private/devices', async () => { | ||
| const response = await devicesApp.request(postJson('http://local/', { | ||
| appId: 'com.example.app', | ||
| devicesId: ['1) OR 1=1 --'], | ||
| })) | ||
|
|
||
| await expectInvalidBody(response) | ||
| expect(checkPermissionMock).not.toHaveBeenCalled() | ||
| expect(readDevicesMock).not.toHaveBeenCalled() | ||
| expect(countDevicesMock).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('rejects non-numeric limits on /private/devices', async () => { | ||
| const response = await devicesApp.request(postJson('http://local/', { | ||
| appId: 'com.example.app', | ||
| limit: '1 UNION SELECT 1', | ||
| })) | ||
|
|
||
| await expectInvalidBody(response) | ||
| expect(checkPermissionMock).not.toHaveBeenCalled() | ||
| expect(readDevicesMock).not.toHaveBeenCalled() | ||
| expect(countDevicesMock).not.toHaveBeenCalled() | ||
| }) |
Contributor
There was a problem hiding this comment.
Use it.concurrent() for parallel test execution.
Per coding guidelines, all test files run in parallel and should use it.concurrent() instead of it() to maximize parallelism. Since these tests use independent mocks reset in beforeEach, they should be safe to run concurrently.
💚 Proposed fix to use concurrent tests
describe('private analytics route validation', () => {
- it('rejects malformed deviceIds on /private/stats', async () => {
+ it.concurrent('rejects malformed deviceIds on /private/stats', async () => {
const response = await statsApp.request(postJson('http://local/', {
appId: 'com.example.app',
devicesId: ['1) OR 1=1 --'],
}))
// ... rest of test
})
- it('rejects malformed actions on /private/stats', async () => {
+ it.concurrent('rejects malformed actions on /private/stats', async () => {
// ... rest of tests similarly
})
// Apply same change to all other it() calls
})As per coding guidelines: "ALL TEST FILES RUN IN PARALLEL; use it.concurrent() instead of it() to maximize parallelism"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/private-analytics-validation.unit.test.ts` around lines 50 - 128,
Replace the synchronous tests using it(...) with concurrent execution by
changing each it(...) to it.concurrent(... ) for the tests that call
statsApp.request and devicesApp.request (the blocks invoking
statsApp.request/postJson, devicesApp.request/postJson and asserting via
expectInvalidBody, checkPermissionMock, readStatsMock, readDevicesMock,
countDevicesMock); ensure the existing beforeEach mock resets still run so mocks
used by expectInvalidBody and the checkPermissionMock/read* mocks remain
isolated when tests run in parallel.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a416923f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| rangeStart: z.optional(z.union([safeQueryDateSchema, z.coerce.number()])), | ||
| rangeEnd: z.optional(z.union([safeQueryDateSchema, z.coerce.number()])), | ||
| limit: z.optional(z.coerce.number().check(z.minimum(1), z.maximum(MAX_QUERY_LIMIT))), | ||
| actions: z.optional(z.array(z.enum(ALLOWED_STATS_ACTIONS))), |
There was a problem hiding this comment.
Using z.enum(ALLOWED_STATS_ACTIONS) here makes /private/stats and /private/stats/export reject valid action filters such as backend_refusal, keyMismatch, disablePlatformElectron, and customIdBlocked: those values are present in the database enum (supabase/functions/_backend/utils/supabase.types.ts:4474-4479) but missing from ALLOWED_STATS_ACTIONS (supabase/functions/_backend/plugins/stats_actions.ts:4-65). This is user-visible because the dashboard already submits backend_refusal (src/components/tables/LogTable.vue:213), so that filter now fails with 400 invalid_body instead of returning logs.
Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
supabase/functions/_backend/private/stats.ts (1)
28-28: Prefer shared analytics limit normalization to avoid policy drift.
MAX_QUERY_LIMITand manual clamping are duplicated here. Reusing the central analytics limit helper keeps/private/statsbehavior aligned with other analytics endpoints.♻️ Suggested consolidation
+import { MAX_ANALYTICS_QUERY_LIMIT, normalizeAnalyticsLimit } from '../utils/cloudflare.ts' -const MAX_QUERY_LIMIT = 50_000 +const MAX_QUERY_LIMIT = MAX_ANALYTICS_QUERY_LIMIT -limit: z.optional(z.coerce.number().check(z.minimum(1), z.maximum(MAX_QUERY_LIMIT))), +limit: z.optional(z.coerce.number().check(z.minimum(1), z.maximum(MAX_QUERY_LIMIT))), -const limit = Math.min(Math.max(body.limit ?? 10_000, 1), 50_000) +const limit = normalizeAnalyticsLimit(body.limit ?? 10_000)Also applies to: 53-53, 171-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/stats.ts` at line 28, Remove the local MAX_QUERY_LIMIT constant and any manual clamping logic in this file and instead import and call the shared analytics limit normalization helper from the common analytics utilities (e.g., normalizeAnalyticsLimit / clampAnalyticsLimit) wherever limits are currently enforced (the spots referencing MAX_QUERY_LIMIT and manual clamp logic). Replace the manual comparisons with a single call to that helper, passing through the incoming limit/params, so /private/stats uses the centralized normalization and stays consistent with other analytics endpoints.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/organization/GroupsRbacManager.vue`:
- Around line 57-60: The detached assignment of supabase.rpc to
deleteGroupWithBindings loses the Supabase client context and will throw at
runtime; replace the detached reference with a bound wrapper that calls
supabase.rpc with the correct this/context (e.g., define deleteGroupWithBindings
as a function that invokes supabase.rpc(fn, args) or use Function.prototype.bind
to bind supabase.rpc to the supabase client), ensuring callers of
deleteGroupWithBindings (the code that currently invokes it) receive a function
that preserves the Supabase instance context.
In `@supabase/functions/_backend/private/stats.ts`:
- Around line 125-131: The Supabase branch in readStatsSB is interpolating
params.search directly into query.or, allowing injection; fix it by escaping the
search string before building the .or() filter (use the same escapeSqlString()
helper used in the CloudFlare implementation) or by switching to Postgrest
parameter binding instead of string interpolation; update the code path that
constructs the filter (look for readStatsSB and the call site that does
query.or(`version_build.ilike.${params.search}%,...`)) to pass an
escaped/parameterized value for params.search and ensure any percent/like
wildcards are concatenated after escaping.
---
Nitpick comments:
In `@supabase/functions/_backend/private/stats.ts`:
- Line 28: Remove the local MAX_QUERY_LIMIT constant and any manual clamping
logic in this file and instead import and call the shared analytics limit
normalization helper from the common analytics utilities (e.g.,
normalizeAnalyticsLimit / clampAnalyticsLimit) wherever limits are currently
enforced (the spots referencing MAX_QUERY_LIMIT and manual clamp logic). Replace
the manual comparisons with a single call to that helper, passing through the
incoming limit/params, so /private/stats uses the centralized normalization and
stays consistent with other analytics endpoints.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2392a609-98e5-4b14-8059-b0cb38641532
📒 Files selected for processing (2)
src/components/organization/GroupsRbacManager.vuesupabase/functions/_backend/private/stats.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/functions/_backend/private/stats.ts`:
- Around line 35-36: The request handler currently accepts rangeStart/rangeEnd
as safeQueryDateSchema or z.coerce.number() and then stringifies them, which
lets numeric epoch strings and invalid short strings slip through and break
readStats/readStatsSB; update the handler to validate and normalize rangeStart
and rangeEnd into ISO date strings before calling readStats/readStatsSB (e.g.,
parse into a Number or Date, ensure !isNaN(date.getTime()), then format via
date.toISOString()), return a 400 for invalid dates, and reuse the same
normalization helper for the /export endpoint so both readStats and export
receive validated ISO dates instead of raw coerced strings.
In `@supabase/functions/_backend/utils/privateAnalyticsValidation.ts`:
- Line 20: The queryLimitSchema currently uses z.coerce.number() which allows
non-integer values (e.g., 1.5 or coercible booleans) and can slip invalid LIMIT
values into SQL; update queryLimitSchema to require integers by adding .int()
after z.coerce.number(), keeping the existing minimum/maximum checks against
MAX_QUERY_LIMIT so only integer values between 1 and MAX_QUERY_LIMIT pass
validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83aab509-8f52-4480-9760-fa439ef78679
📒 Files selected for processing (5)
supabase/functions/_backend/private/devices.tssupabase/functions/_backend/private/stats.tssupabase/functions/_backend/utils/privateAnalyticsValidation.tssupabase/functions/_backend/utils/supabase.tstests/private-analytics-validation.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/private-analytics-validation.unit.test.ts
- supabase/functions/_backend/private/devices.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
supabase/functions/_backend/private/stats.ts (1)
35-36:⚠️ Potential issue | 🟠 MajorNormalize
rangeStart/rangeEndbefore callingreadStats.Lines 35-36 accept any short string or a number, but Lines 102-103 and 137-138 immediately
String(...)the parsed value. That turns valid epoch-millisecond numbers into"1710000000000", whichnew Date(...)rejects downstream, and numeric strings still bypass validation because the string branch wins the union. Parse these here and pass ISO strings only, or reject them up front.🗓️ Possible fix
+function normalizeRangeDate(value: string | number | undefined): string | undefined { + if (value === undefined) + return undefined + + const date = typeof value === 'string' && /^\d+$/.test(value) + ? new Date(Number(value)) + : new Date(value) + + if (Number.isNaN(date.getTime())) + throw simpleError('invalid_body', 'Invalid body') + + return date.toISOString() +} + app.post('/', middlewareV2(['read', 'write', 'all', 'upload']), async (c) => { const bodyRaw = await parseBody<DataStats>(c) const parsed = statsBodySchema.safeParse(bodyRaw) @@ - const startDate = body.rangeStart !== undefined ? String(body.rangeStart) : undefined - const endDate = body.rangeEnd !== undefined ? String(body.rangeEnd) : undefined + const startDate = normalizeRangeDate(body.rangeStart) + const endDate = normalizeRangeDate(body.rangeEnd) @@ - const startDate = body.rangeStart !== undefined ? String(body.rangeStart) : undefined - const endDate = body.rangeEnd !== undefined ? String(body.rangeEnd) : undefined + const startDate = normalizeRangeDate(body.rangeStart) + const endDate = normalizeRangeDate(body.rangeEnd)Run this to reproduce the current
Datebehavior:#!/bin/bash set -euo pipefail sed -n '27,38p;102,103p;137,138p' supabase/functions/_backend/private/stats.ts node <<'EOF' const samples = [1710000000000, '1710000000000', 'abc', '2024-01-01T00:00:00Z'] for (const value of samples) { const date = new Date(value) console.log(JSON.stringify(value), Number.isNaN(date.getTime()) ? 'Invalid Date' : date.toISOString()) } EOFBased on learnings: "All changes to public APIs and plugin interfaces MUST be backward compatible; support both old and new behavior using version detection".
Also applies to: 102-103, 137-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/stats.ts` around lines 35 - 36, The parsed inputs rangeStart and rangeEnd (from safeQueryDateSchema / z.coerce.number()) must be normalized to ISO date strings before calling readStats: in the handler that reads rangeStart/rangeEnd, detect numeric epoch inputs (number or numeric string), convert them to a Date and toISOString(), and for string inputs validate/normalize them against safeQueryDateSchema (reject if invalid) so readStats always receives ISO timestamps; update the code paths that call readStats (references to readStats and the variables rangeStart/rangeEnd) to pass the normalized ISO strings or return a validation error instead of blindly String(...) casting.supabase/functions/_backend/utils/privateAnalyticsValidation.ts (1)
20-20:⚠️ Potential issue | 🟠 MajorReject non-integer
limitvalues at the schema boundary.Line 20 still lets malformed limits through.
z.coerce.number()accepts decimals, and coercible values liketruecan become1, so inputs that should be rejected can still reach the analytics query builders. Tighten this to integer-only input instead of relying on downstream normalization.In Zod 4.3.x / zod/mini, what inputs does z.coerce.number() accept (especially decimals and booleans), and what is the recommended way to validate a SQL LIMIT as an integer while only coercing numeric strings?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/utils/privateAnalyticsValidation.ts` at line 20, queryLimitSchema currently uses z.coerce.number() which will coerce booleans and decimals; replace it with a preprocess that only coerces numeric strings then validates integer bounds, e.g. use z.preprocess(val => typeof val === "string" ? Number(val) : val, z.number().int().min(1).max(MAX_QUERY_LIMIT)) to ensure only integer LIMITs (and to keep MAX_QUERY_LIMIT) and reject booleans/decimals at the schema boundary; update the export for queryLimitSchema accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/functions/_backend/utils/supabase.ts`:
- Around line 16-18: The helper function quotePostgrestFilterValue contains
malformed String.raw usage causing a parse error; update
quotePostgrestFilterValue to perform two sequential replacements on the input
(escape backslashes first, then escape double quotes) using standard replace
calls (e.g., replace backslash with double-backslash via a global regex, then
replace " with escaped quote) and return the value wrapped in double quotes so
the produced string is correctly escaped for PostgREST filters.
---
Duplicate comments:
In `@supabase/functions/_backend/private/stats.ts`:
- Around line 35-36: The parsed inputs rangeStart and rangeEnd (from
safeQueryDateSchema / z.coerce.number()) must be normalized to ISO date strings
before calling readStats: in the handler that reads rangeStart/rangeEnd, detect
numeric epoch inputs (number or numeric string), convert them to a Date and
toISOString(), and for string inputs validate/normalize them against
safeQueryDateSchema (reject if invalid) so readStats always receives ISO
timestamps; update the code paths that call readStats (references to readStats
and the variables rangeStart/rangeEnd) to pass the normalized ISO strings or
return a validation error instead of blindly String(...) casting.
In `@supabase/functions/_backend/utils/privateAnalyticsValidation.ts`:
- Line 20: queryLimitSchema currently uses z.coerce.number() which will coerce
booleans and decimals; replace it with a preprocess that only coerces numeric
strings then validates integer bounds, e.g. use z.preprocess(val => typeof val
=== "string" ? Number(val) : val, z.number().int().min(1).max(MAX_QUERY_LIMIT))
to ensure only integer LIMITs (and to keep MAX_QUERY_LIMIT) and reject
booleans/decimals at the schema boundary; update the export for queryLimitSchema
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1d8e57d-3d20-4db9-9e65-b430b280b97c
📒 Files selected for processing (4)
supabase/functions/_backend/private/stats.tssupabase/functions/_backend/utils/privateAnalyticsValidation.tssupabase/functions/_backend/utils/supabase.tstests/private-analytics-validation.unit.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
supabase/functions/_backend/utils/supabase.ts (1)
16-22: Escaping helpers look correct; consider escaping LIKE wildcards.The
quotePostgrestFilterValuefunction correctly escapes backslashes before quotes (proper order) and wraps the result in double-quotes. The previous syntax issue withString.rawhas been resolved.However,
buildIlikePrefixPatterndoes not escape LIKE pattern metacharacters (%and_) within the search value. This means a user searching fortest_userwould matchtestzuser,test1user, etc. While not a security risk (values are properly quoted), it may cause unexpected search behavior.Optional: Escape LIKE wildcards for literal matching
function buildIlikePrefixPattern(value: string): string { - return quotePostgrestFilterValue(`${value}%`) + // Escape LIKE metacharacters for literal matching, then add trailing wildcard + const escapedValue = value.replace(/[%_]/g, '\\$&') + return quotePostgrestFilterValue(`${escapedValue}%`) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/utils/supabase.ts` around lines 16 - 22, buildIlikePrefixPattern should escape SQL LIKE metacharacters so user input chars '%' and '_' are treated literally; update buildIlikePrefixPattern (or add a small helper) to first replace '%' -> '\%' and '_' -> '\_' in the input value, then append the trailing '%' for prefix matching and pass the result to quotePostgrestFilterValue so backslashes and quotes are still properly escaped by that function. Ensure the wildcard-escaping happens before calling quotePostgrestFilterValue (or documented helper name) so escaping order is correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@supabase/functions/_backend/utils/supabase.ts`:
- Around line 16-22: buildIlikePrefixPattern should escape SQL LIKE
metacharacters so user input chars '%' and '_' are treated literally; update
buildIlikePrefixPattern (or add a small helper) to first replace '%' -> '\%' and
'_' -> '\_' in the input value, then append the trailing '%' for prefix matching
and pass the result to quotePostgrestFilterValue so backslashes and quotes are
still properly escaped by that function. Ensure the wildcard-escaping happens
before calling quotePostgrestFilterValue (or documented helper name) so escaping
order is correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce761f54-dc5d-42d0-a343-c400707e6309
📒 Files selected for processing (1)
supabase/functions/_backend/utils/supabase.ts
Member
Author
|
CI status on March 16, 2026: the GitHub Actions suite is green, including |
|
This was referenced Mar 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary (AI generated)
Motivation (AI generated)
Private analytics read endpoints accepted query-bound values without runtime validation, which left Cloudflare query construction relying too heavily on downstream escaping.
Business Impact (AI generated)
This reduces the risk of cross-tenant analytics data exposure for authenticated dashboard and API-key users and hardens a sensitive backend reporting path.
Test Plan (AI generated)
bun lint:backendbunx eslint tests/private-analytics-validation.unit.test.ts tests/private-error-cases.test.ts tests/stats-export.test.tsbunx vitest run tests/private-analytics-validation.unit.test.ts tests/stats-export.test.tsScreenshots (AI generated)
Checklist (AI generated)
bun run lint:backend && bun run lint.Generated with AI
Summary by CodeRabbit
Bug Fixes
Tests