fix(api): comments route SQL injection + broken GROUP BY (500) - #39
fix(api): comments route SQL injection + broken GROUP BY (500)#39rferrari wants to merge 8 commits into
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fetches a single post from HAFSQL with the same column shape, children count and vote aggregation as the feed, then runs it through dealiasSoftPosts so lite-user (@skateuser) identity is preserved. Serves edge-cached (s-maxage=300) so post pages, OG images and Farcaster frames stop hitting Hive RPC directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Covers the happy path (single post by author/permlink) and the 404 (nonexistent permlink) in the existing status-code smoke suite. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Introduces vitest (+ test/test:watch scripts and a config that mirrors the '@/*' tsconfig alias) as the unit-test framework for API routes. Unit tests live in tests/unit/ so route folders with literal-bracket segments (e.g. [author]) don't break glob includes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Covers 200 (normalized shape + 300s edge-cache headers), query parameterization, soft-post de-alias delegation, 404 (short cache), and 500 (no cache). HAFSQL and dealiasSoftPosts are mocked so the suite runs with no live DB. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The comments route interpolated the user-supplied pa/pp query params straight into the SQL string, allowing arbitrary SQL injection (e.g. ?pa=x' OR '1'='1). Bind them as @pa/@pp named parameters via the HAFSQL_Database wrapper instead, matching every other v2 route. Refs SkateHive#22. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The SELECT listed columns (last_edited, cashout_time, tags, payouts, etc.) absent from the GROUP BY, so Postgres rejected every request with "column must appear in the GROUP BY clause" — the endpoint returned 500 for all valid input. Add the missing non-aggregated columns to GROUP BY, matching the feed route. Pre-existing bug, surfaced while fixing SkateHive#22. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Asserts result passthrough (behavior unchanged), that pa/pp are bound as @-parameters and an injection payload never reaches the SQL text, plus the 400 (missing params), 404 (no rows) and 500 paths. HAFSQL mocked. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
1 participant
What
Fixes two bugs in
GET /api/v2/comments, found while reviewing #22:pa/ppquery params were interpolated straight into the SQL string (WHERE c.parent_author = '${parent_author}'). Now bound as@pa/@ppnamed params via theHAFSQL_Databasewrapper, like every other v2 route.GROUP BY→ 500 for all requests (pre-existing). TheSELECTlisted columns (last_edited,cashout_time,tags, payouts, …) missing from theGROUP BY, so Postgres rejected every query with "column must appear in the GROUP BY clause." The endpoint was returning 500 for all valid input. Completed theGROUP BYwith the missing non-aggregated columns (matching the feed route).Why this matters for #22
#22 claims all v2 SQL injection was remediated, but the comments route was still vulnerable — so #22 should stay open until this merges. This closes that gap. (The GROUP BY bug is separate/pre-existing but the endpoint was unusable without fixing it, and it made the fix verifiable end-to-end.)
Commits
fix(api): parameterize comments route query (SQL injection)fix(api): complete GROUP BY in comments route (fixes 500)test(api): add unit tests for comments routeTesting
tests/unit/comments.route.test.ts, 5 tests): result passthrough (behavior unchanged),pa/ppbound as@-params with an injection payload asserted absent from the SQL text, plus 400/404/500 paths. HAFSQL mocked.?pa=web-gnar&pp=sh-20260702t050343→ 200, 2 comments returned.?pa=x' OR '1'='1&pp=...→ 404 "No comments found" — payload treated as a literal, not executed, no data leak.pnpm lintclean;pnpm buildcompiles the route.🤖 Generated with Claude Code