with the skill
## Summary
Adds `GET /orders/search` with auth, account scoping, and pagination. Scoping by `account_id` and requiring login are the right instincts. However, there is a SQL injection and a pagination bug that both need fixing before merge.
## Strengths
- 🎉 Results are scoped to `current_user.account_id`, so tenants can't see each other's orders.
- 🎉 The endpoint is behind `@login_required`, and the 1-based paging is documented in the PR description and a code comment.
## Required Changes
🔴 **[blocking] SQL injection via `q`.** `term` goes straight into the query through an f-string. Something like `q=%'; DROP TABLE orders; --` or a `UNION SELECT` against other tables is possible, and because this is an admin API the blast radius is large. It also bypasses the `account_id` scoping, since an attacker can add `OR 1=1`. Please use bound parameters. `account_id` and the pagination values should be parameterized too. They're likely ints today, but this keeps the query safe if that changes.
🔴 **[blocking] Off-by-one in pagination.** The PR says pages are 1-based, but `offset = page * PAGE_SIZE` means page 1 starts at row 20, so the first 20 results are never reachable. It should be `(page - 1) * PAGE_SIZE`.
🔴 **[blocking] Unvalidated `page`.**
- `?page=abc` raises `ValueError`, which becomes a 500.
- `?page=0` (or negative) gives a negative offset once the fix above is in, and Postgres errors on that, also a 500.
Using `request.args.get("page", 1, type=int)` and clamping to `>= 1` (or returning a 400) would handle both.
Show the rest (55 more lines)
## Suggested fix
```python
@bp.get("/orders/search")
@login_required
def search_orders():
term = request.args.get("q", "")
page = max(request.args.get("page", 1, type=int), 1)
offset = (page - 1) * PAGE_SIZE
# Escape LIKE wildcards so users searching "50%_off" get literal matches
escaped = term.replace("\\", "\\\\").replace("%", r"\%").replace("_", r"\_")
sql = (
"SELECT id, customer_name, total FROM orders "
"WHERE account_id = %s AND customer_name ILIKE %s "
"ORDER BY created_at DESC, id DESC "
"LIMIT %s OFFSET %s"
)
params = (current_user.account_id, f"%{escaped}%", PAGE_SIZE, offset)
with get_conn() as conn:
rows = conn.execute(sql, params).fetchall()
return jsonify([dict(r) for r in rows])
```
The `%s` placeholder style assumes psycopg. If `get_conn` wraps SQLAlchemy or another driver, use that driver's style, such as `text()` with named params.
## Suggestions
💡 **[important] Stable ordering.** `ORDER BY created_at DESC` alone is non-deterministic when timestamps tie, so rows can repeat or vanish across pages. I added `id DESC` as a tiebreaker in the fix above.
💡 **Pagination metadata.** The response is a bare list, so the UI can't tell whether there's a next page without making an extra request. Consider returning `{"results": [...], "page": n, "page_size": 20}`, plus `has_more` (fetch `PAGE_SIZE + 1` rows) or a total count if the UI needs one.
💡 **Cap on `page`.** Very large offsets are slow in Postgres. A sane upper bound, or keyset pagination later, is worth considering.
## Questions
❓ Is `@login_required` enough for an admin API, or should this check a role/permission (e.g. `admin_required`)? I'm not sure how `current_user` is modeled here.
❓ What should an empty `q` do? Currently it returns all of the account's orders. That may be fine, but if not, return a 400 or require a minimum length.
❓ Is `customer_name` indexed? `ILIKE '%term%'` can't use a btree index, so on a large `orders` table this will seq-scan. A `pg_trgm` GIN index would fix that. It's not blocking if the table is small.
❓ Could we add tests covering:
- page 1 returns the first 20 rows
- page 2 returns the next 20
- invalid or zero `page` values
- a `q` containing a quote or `%`
- one account can't see another account's orders
The cross-account and injection tests would have caught the first two issues here.
## Verdict
🔄 **Request changes.** Fix the injection, the offset math, and the `page` validation. The rest is non-blocking. Happy to pair on the parameterization if the DB wrapper makes it awkward.