fix(admin/analytics): use sql.raw for date_trunc period to avoid parameter mismatch#2354
Conversation
Coverage Report for API Unit Tests Coverage (./packages/api)
File CoverageNo changed files found. |
Coverage Report for Expo Unit Tests Coverage (./apps/expo)
File CoverageNo changed files found. |
|
Warning
|
| Cohort / File(s) | Summary |
|---|---|
Analytics Query Parameter Handling packages/api/src/routes/admin/analytics/platform.ts |
Updated six analytics queries (users, packs, catalogItems, trips, trailConditionReports, posts) to inject the period parameter via sql.raw(\'${period}'`)instead of direct parameter usage, affectingdate_trunc(), GROUP BY, and ORDER BY` clauses. Minor line-wrapping adjustment included. |
AllTrails Preview Endpoint Auth packages/api/src/routes/alltrails.ts |
Removed isAuthenticated: true from route options to allow unauthenticated access to the /alltrails/preview endpoint. URL validation, SSRF protection, and OG tag extraction logic remain intact. |
Possibly related PRs
- feat(api): AllTrails OG preview endpoint and MCP tool #2310: Introduced the
/alltrails/previewendpoint that is now being modified to remove authentication enforcement in this PR.
Suggested labels
api
Poem
🐰 Through SQL we hop with parameters bound,
No auth walls block our preview's ground,
Date_trunc dances to the SQL song,
While AllTrails pathways flow along!
🎯 2 (Simple) | ⏱️ ~15 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately reflects the primary change: fixing analytics queries by using sql.raw for date_trunc period parameterization to resolve a parameter mismatch issue. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
fix/analytics-date-trunc-parameterization
Warning
Review ran into problems
🔥 Problems
Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.
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 @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/api/src/routes/alltrails.ts (2)
18-22:⚠️ Potential issue | 🟠 MajorPublic unauthenticated proxy — add rate limiting / abuse controls.
Removing
isAuthenticated: trueturns/alltrails/previewinto an open server-side fetcher. Even with thealltrails.comallowlist, an unauthenticated caller can drive arbitrary outbound traffic from the worker (link-preview floods, scraping-as-a-service, cost amplification on Cloudflare). Before shipping, please confirm one of: per-IP/origin rate limiting (e.g. Cloudflare WAF rule or a Durable-Object/KV limiter), a CORS/Originallowlist for first-party clients, or a lightweight signed token from the app — otherwise this endpoint is trivially abusable.Also worth noting: this route change is unrelated to the stated PR objective (analytics
date_truncparameterization). Consider splitting into its own PR so the auth/abuse implications get reviewed independently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/alltrails.ts` around lines 18 - 22, The new public route alltrailsRoutes.post('/preview') removed authentication (isAuthenticated) and can be abused as an open server-side fetch proxy; restore abuse controls by implementing one of: (1) re-introduce authentication (set isAuthenticated: true) or require a lightweight signed token check in the route handler, (2) enforce per-client rate-limiting (per-IP or per-origin) via a Durable Object/KV limiter or Cloudflare WAF rule before allowing requests to alltrailsRoutes or specifically the '/preview' handler, or (3) add a CORS/Origin allowlist check for first-party clients inside the '/preview' handler to reject unknown origins; also consider splitting this change into a separate PR from the analytics/date_trunc work so the auth/abuse implications (alltrailsRoutes and '/preview') are reviewed independently.
82-82:⚠️ Potential issue | 🟡 MinorCap the response body size before
response.text().With no size limit, a large (or hostile mirror returning a huge body within the alltrails.com hostname class) response can balloon worker memory; combined with the now-public route this is an easy DoS vector. Consider streaming with a hard cap (e.g. 1–2 MB) and rejecting oversize responses.
🛡️ Sketch of a bounded reader
- const html = await response.text(); + const MAX_BYTES = 2 * 1024 * 1024; // 2 MB + const reader = response.body?.getReader(); + if (!reader) { + return status(502, { error: 'Empty response from AllTrails' }); + } + const chunks: Uint8Array[] = []; + let received = 0; + while (true) { + const { value, done } = await reader.read(); + if (done) break; + received += value.byteLength; + if (received > MAX_BYTES) { + await reader.cancel(); + return status(502, { error: 'AllTrails response too large' }); + } + chunks.push(value); + } + const html = new TextDecoder('utf-8').decode( + chunks.length === 1 ? chunks[0] : Buffer.concat(chunks.map((c) => Buffer.from(c))), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/alltrails.ts` at line 82, The call to response.text() in the alltrails route can consume an unbounded body and must be replaced with a bounded streaming read: use response.body.getReader() (or a streaming reader) to read chunks into a buffer and stop/abort once a hard limit (e.g. 1–2 MB) is reached, rejecting the request or returning an error; locate the usage of "const html = await response.text()" and replace it with a chunked read that accumulates into a string/Uint8Array up to the cap, closes the reader/stream on overflow, and throws or returns a 413/appropriate error when the body exceeds the limit.
🧹 Nitpick comments (1)
packages/api/src/routes/admin/analytics/platform.ts (1)
47-71: Fix is correct; consider extracting thedate_truncexpression to remove 18× duplication and harden the literal inlining.The root-cause analysis is right: Drizzle's
sqltag binds each${period}to a fresh$Nplaceholder, and PostgreSQL treats each placeholder as a distinct expression, so SELECT's$1and GROUP BY's$2aren't structurally equal — hencecolumn "..." must appear in the GROUP BY clause. Inlining the literal sidesteps that.Two non-blocking suggestions for both
/growthand/activityhandlers:
- Deduplicate: bind the
date_trunc(...)fragment per column once and reuse it inselect,groupBy, andorderBy— currently the same expression is written three times per metric (18 total), which is fragile if anyone later edits one site without the others.- Defense-in-depth:
sql.raw(\'${period}'`)is safe today only becausePeriodSchemaconstrainsperiodto'day' | 'week' | 'month'. Decouple the safety from a remote validator by mapping the enum to a fixedsql` fragment, so a future schema relaxation can't silently turn this into a SQL injection sink.♻️ Sketch combining both improvements (apply analogously in `/activity`)
const db = createDb(); const { period = 'month', range = 12 } = query; const startDate = getStartDate(period, range); + const periodLiteral = { day: sql`'day'`, week: sql`'week'`, month: sql`'month'` }[period]; + const truncBy = (col: AnyPgColumn) => sql`date_trunc(${periodLiteral}, ${col})`; + try { const [userGrowth, packGrowth, catalogGrowth] = await Promise.all([ db .select({ - date: sql<string>`date_trunc(${sql.raw(`'${period}'`)}, ${users.createdAt})::date::text`, + date: sql<string>`${truncBy(users.createdAt)}::date::text`, count: count(), }) .from(users) .where(gte(users.createdAt, startDate)) - .groupBy(sql`date_trunc(${sql.raw(`'${period}'`)}, ${users.createdAt})`) - .orderBy(sql`date_trunc(${sql.raw(`'${period}'`)}, ${users.createdAt})`), + .groupBy(truncBy(users.createdAt)) + .orderBy(truncBy(users.createdAt)),This keeps the literal-inlining behavior (each
truncBy(col)produces fresh, identical SQL text in all three positions), while the only place the period value crosses into SQL is the static map.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/admin/analytics/platform.ts` around lines 47 - 71, Extract the repeated date_trunc fragment into a single reusable SQL expression and replace the inline sql.raw(`'${period}'`) usage with a safe enum-to-fragment map: create a const map (e.g. TRUNC_PERIOD_SQL) that maps allowed period values ('day'|'week'|'month') to prebuilt sql fragments, then build a single truncExpr per metric (e.g. const usersTrunc = sql`date_trunc(${TRUNC_PERIOD_SQL[period]}, ${users.createdAt})::date::text`) and reuse usersTrunc in .select, .groupBy and .orderBy; apply the same pattern for packs and catalogItems and replicate the change in both the /growth and /activity handlers so the literal is centralized and duplication is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/api/src/routes/alltrails.ts`:
- Around line 18-22: The new public route alltrailsRoutes.post('/preview')
removed authentication (isAuthenticated) and can be abused as an open
server-side fetch proxy; restore abuse controls by implementing one of: (1)
re-introduce authentication (set isAuthenticated: true) or require a lightweight
signed token check in the route handler, (2) enforce per-client rate-limiting
(per-IP or per-origin) via a Durable Object/KV limiter or Cloudflare WAF rule
before allowing requests to alltrailsRoutes or specifically the '/preview'
handler, or (3) add a CORS/Origin allowlist check for first-party clients inside
the '/preview' handler to reject unknown origins; also consider splitting this
change into a separate PR from the analytics/date_trunc work so the auth/abuse
implications (alltrailsRoutes and '/preview') are reviewed independently.
- Line 82: The call to response.text() in the alltrails route can consume an
unbounded body and must be replaced with a bounded streaming read: use
response.body.getReader() (or a streaming reader) to read chunks into a buffer
and stop/abort once a hard limit (e.g. 1–2 MB) is reached, rejecting the request
or returning an error; locate the usage of "const html = await response.text()"
and replace it with a chunked read that accumulates into a string/Uint8Array up
to the cap, closes the reader/stream on overflow, and throws or returns a
413/appropriate error when the body exceeds the limit.
---
Nitpick comments:
In `@packages/api/src/routes/admin/analytics/platform.ts`:
- Around line 47-71: Extract the repeated date_trunc fragment into a single
reusable SQL expression and replace the inline sql.raw(`'${period}'`) usage with
a safe enum-to-fragment map: create a const map (e.g. TRUNC_PERIOD_SQL) that
maps allowed period values ('day'|'week'|'month') to prebuilt sql fragments,
then build a single truncExpr per metric (e.g. const usersTrunc =
sql`date_trunc(${TRUNC_PERIOD_SQL[period]}, ${users.createdAt})::date::text`)
and reuse usersTrunc in .select, .groupBy and .orderBy; apply the same pattern
for packs and catalogItems and replicate the change in both the /growth and
/activity handlers so the literal is centralized and duplication is removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fecdaea9-75ff-4fd0-aff6-42efe50ff17b
📒 Files selected for processing (2)
packages/api/src/routes/admin/analytics/platform.tspackages/api/src/routes/alltrails.ts
There was a problem hiding this comment.
Pull request overview
This PR addresses a PostgreSQL query failure in admin platform analytics by ensuring date_trunc uses a consistent, non-parameterized period literal, avoiding placeholder mismatches between SELECT and GROUP BY/ORDER BY.
Changes:
- Inline
periodsafely intodate_trunc()viasql.raw(...)for analytics growth/activity queries. - Update the
/alltrails/previewroute configuration/commenting (including auth behavior).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
packages/api/src/routes/admin/analytics/platform.ts |
Switches date_trunc(period, ...) to use a raw SQL literal for period across growth/activity aggregations. |
packages/api/src/routes/alltrails.ts |
Removes isAuthenticated: true from the AllTrails preview endpoint and adds a “public-route” comment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .groupBy(sql`date_trunc(${sql.raw(`'${period}'`)}, ${trailConditionReports.createdAt})`) | ||
| .orderBy( | ||
| sql`date_trunc(${sql.raw(`'${period}'`)}, ${trailConditionReports.createdAt})`, | ||
| ), |
There was a problem hiding this comment.
groupBy(sql...) is missing its closing backtick/parenthesis here, which will cause a syntax error. Close the sql template and the groupBy(...) call before the multi-line .orderBy(...).
| .groupBy(sql`date_trunc(${sql.raw(`'${period}'`)}, ${posts.createdAt})`) | ||
| .orderBy(sql`date_trunc(${sql.raw(`'${period}'`)}, ${posts.createdAt})`), |
There was a problem hiding this comment.
This groupBy call is missing its closing backtick/parenthesis, which breaks parsing/compilation. Close groupBy(sql...) before chaining .orderBy(...).
| // public-route: link-preview proxy; fetches OG metadata from AllTrails, no user data involved | ||
| export const alltrailsRoutes = new Elysia({ prefix: '/alltrails' }).post( | ||
| '/preview', |
There was a problem hiding this comment.
This PR is scoped to fixing admin analytics date_trunc parameterization, but it also changes the /alltrails/preview route to be unauthenticated. If this auth change is intended, please call it out in the PR description (or split into a separate PR) and confirm that making this endpoint public is acceptable from an abuse/traffic perspective.
| .groupBy(sql`date_trunc(${sql.raw(`'${period}'`)}, ${users.createdAt})`) | ||
| .orderBy(sql`date_trunc(${sql.raw(`'${period}'`)}, ${users.createdAt})`), |
There was a problem hiding this comment.
The groupBy call is missing the closing template-literal backtick/parenthesis, which will cause a TypeScript syntax error and prevent this route from compiling. Close the sql template literal and the groupBy(...) call before chaining .orderBy(...).
| .groupBy(sql`date_trunc(${sql.raw(`'${period}'`)}, ${packs.createdAt})`) | ||
| .orderBy(sql`date_trunc(${sql.raw(`'${period}'`)}, ${packs.createdAt})`), |
There was a problem hiding this comment.
The groupBy call is not properly closed (missing closing backtick/)), which breaks the query builder chain and results in invalid TypeScript. Ensure groupBy(sql...) is fully closed before .orderBy(...).
| .groupBy(sql`date_trunc(${sql.raw(`'${period}'`)}, ${catalogItems.createdAt})`) | ||
| .orderBy(sql`date_trunc(${sql.raw(`'${period}'`)}, ${catalogItems.createdAt})`), |
There was a problem hiding this comment.
This groupBy call appears to be missing its closing backtick/parenthesis, which will make the file fail to parse/compile. Close the sql template literal and groupBy(...) invocation.
| .groupBy(sql`date_trunc(${sql.raw(`'${period}'`)}, ${trips.createdAt})`) | ||
| .orderBy(sql`date_trunc(${sql.raw(`'${period}'`)}, ${trips.createdAt})`), |
There was a problem hiding this comment.
The groupBy invocation isn’t closed (missing closing backtick/)), so the subsequent .orderBy(...) is currently attached to an unterminated expression. Close groupBy(sql...) before chaining.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
packrat-admin | 3e2c9d3 | Commit Preview URL Branch Preview URL |
Apr 27 2026, 03:39 AM |
…ameterization fix(admin/analytics): use sql.raw for date_trunc period to avoid parameter mismatch
Summary
date_trunc(${period}, col)in Drizzle'ssqltag parameterizesperiodas$1,$2, etc. for each occurrenceANALYTICS_GROWTH_ERRORperiodis Zod-validated to['day', 'week', 'month']so it's safe to inline as a literal viasql.raw(\'${period}'`)`/growthand/activityroutesTest plan
/api/admin/analytics/platform/growth?period=month— should return data instead of 500/api/admin/analytics/platform/activity?period=week— same🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
Chores