Gate device custom_id from /stats#1615
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip We've launched Issue Planner and it is currently in beta. Please try it out and share your feedback on Discord! Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds an app-level switch to prevent unauthenticated /stats telemetry from persisting device-supplied custom_id, while keeping current behavior by default.
Changes:
- Add
apps.allow_device_custom_id(defaulttrue) and a newstats_actionenum valuecustomIdBlocked. - Update
/statsingestion to optionally stripcustom_idand emit acustomIdBlockedstat when blocked. - Regenerate Supabase types and add test coverage for the blocked behavior.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/stats.test.ts |
Adds a test ensuring custom_id is not persisted and customIdBlocked is emitted when the app disables it. |
supabase/migrations/20260210132811_stats_customid_guard.sql |
Introduces apps.allow_device_custom_id and adds customIdBlocked to stats_action. |
supabase/functions/deno.lock |
Adds/updates Deno lockfile for functions dependencies. |
supabase/functions/_backend/utils/supabase.types.ts |
Updates backend-generated DB types for the new column and enum value. |
supabase/functions/_backend/utils/supabase.ts |
Adjusts device upsert behavior to only persist custom_id when explicitly provided. |
supabase/functions/_backend/utils/postgres_schema.ts |
Updates Drizzle schema for the new apps.allow_device_custom_id column. |
supabase/functions/_backend/utils/pg.ts |
Extends getAppOwnerPostgres to return allow_device_custom_id with replica-safe access. |
supabase/functions/_backend/public/app/put.ts |
Allows updating allow_device_custom_id via the app settings update endpoint. |
supabase/functions/_backend/public/app/index.ts |
Extends PUT body typing to include allow_device_custom_id. |
supabase/functions/_backend/plugins/stats.ts |
Implements gating logic in /stats, emits customIdBlocked, and routes to primary DB when needed. |
src/types/supabase.types.ts |
Updates frontend-generated DB types for the new column and enum value. |
Comments suppressed due to low confidence (1)
supabase/functions/_backend/plugins/stats.ts:111
allowDeviceCustomIdFromPg()is invoked insidepost(), so in batch mode it can execute once per event (and even in single mode it adds an extraappsread). Since all events in a batch are forced to share the sameapp_id, andgetAppOwnerPostgres()already reads fromapps(now includingallow_device_custom_id), this introduces avoidable extra DB roundtrips. Consider fetchingallow_device_custom_idonce per request (or reuseappOwner.allow_device_custom_idwhenappOwneris available), and only fall back to a dedicated query in the cachedcancelledbranch wheregetAppOwnerPostgres()isn’t called.
const allowDeviceCustomId = requestedCustomId === '' ? true : await allowDeviceCustomIdFromPg(drizzleClient, app_id)
if (cachedStatus === 'cancelled') {
const statsActions: StatsActions[] = [{ action: 'needPlanUpgrade' }]
// Keep behavior backward compatible (default allow=true), but allow owners to
// disable custom_id persistence from unauthenticated /stats traffic.
if (!allowDeviceCustomId && requestedCustomId !== '') {
device.custom_id = undefined
statsActions.push({ action: 'customIdBlocked' })
}
await sendStatsAndDevice(c, device, statsActions)
return { success: false, error: 'need_plan_upgrade', message: PLAN_ERROR }
}
const appOwner = await getAppOwnerPostgres(c, app_id, drizzleClient as ReturnType<typeof getDrizzleClient>, planActions)
if (!appOwner) {
| async function post(c: Context, drizzleClient: ReturnType<typeof getDrizzleClient>, body: AppStats): Promise<PostResult> { | ||
| const device = makeDevice(body) | ||
| const { app_id, action, version_name, old_version_name, plugin_version } = body | ||
| const requestedCustomId = typeof body.custom_id === 'string' ? body.custom_id.trim() : '' |
There was a problem hiding this comment.
requestedCustomId is computed with .trim(), but device.custom_id is left untrimmed. This means a whitespace-only custom_id (e.g. " ") is treated as "not provided" for gating (so the app flag isn’t checked) while still being persisted to devices.custom_id via sendStatsAndDevice/trackDevicesSB. Consider normalizing device.custom_id to the trimmed value (or undefined when trim() results in empty) before calling sendStatsAndDevice, and use that normalized value consistently for the allow check.
| const requestedCustomId = typeof body.custom_id === 'string' ? body.custom_id.trim() : '' | |
| const rawCustomId = typeof body.custom_id === 'string' ? body.custom_id : '' | |
| const requestedCustomId = rawCustomId.trim() | |
| device.custom_id = requestedCustomId === '' ? undefined : requestedCustomId |
![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: 43d032a76b
ℹ️ 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".
| await setAppStatus(c, app_id, 'cloud') | ||
| const statsActions: StatsActions[] = [] | ||
| if (!allowDeviceCustomId && requestedCustomId !== '') { | ||
| device.custom_id = undefined |
There was a problem hiding this comment.
When allow_device_custom_id is false, this branch sets device.custom_id = undefined and still sends the device through sendStatsAndDevice. In workerd production paths, trackDevicesCF normalizes undefined to '' before writing Analytics Engine blobs, and readDevicesCF uses argMax(blob5, timestamp), so blocked /stats events still mutate custom_id by clearing it. That means the new guard does not actually make custom_id immutable for Cloudflare-backed traffic.
Useful? React with 👍 / 👎.
| return { success: true, isOnprem: true } | ||
| } | ||
|
|
||
| const allowDeviceCustomId = requestedCustomId === '' ? true : await allowDeviceCustomIdFromPg(drizzleClient, app_id) |
There was a problem hiding this comment.
The gate treats a trimmed custom_id of '' as “not provided”, so requests like custom_id: ' ' skip the blocking path even when allow_device_custom_id is false. Because the raw value is then passed through to persistence (and trackDevicesSB only treats exact '' as empty), a client can still write whitespace custom_id values via unauthenticated /stats, bypassing the intended protection.
Useful? React with 👍 / 👎.
|
Addressed review comments:
|
|
CI fix: Added |
|
Summary (AI generated)
apps.allow_device_custom_id(defaulttrue) to gate device-suppliedcustom_idpersistence from unauthenticated/stats.custom_id, strip it and emitcustomIdBlockedstat action.Motivation (AI generated)
Prevent persistent poisoning of
devices.custom_idvia service-role upsert from public stats ingestion while keeping current behavior by default.Business Impact (AI generated)
Reduces customer-visible data integrity risk from unauthenticated telemetry without breaking existing clients.
Test plan (AI generated)
bun run lint:backend && bun run lintbun test:backendScreenshots (AI generated)
Checklist (AI generated)
bun run lint:backend && bun run lint.accordingly.
my tests
Generated with AI