Recorded review of codeitronics/ai-pr-reviewer-demo#2, a demo pull request with deliberate mistakes, reviewed by deepseek (deepseek-chat).
codeitronics/ai-pr-reviewer-demo #2 · by avnishyadav25 · +29 −1 in 1 file(s)
Add order search, discounts and an admin export
This PR adds three new routes to src/app.js: a paginated order search, a discount endpoint, and an admin CSV export. The main risks are a SQL injection in the /orders search, a missing null check and missing await on the discount endpoint, and an API key logged to stdout in the export route.
src/app.js modified · 8 comments
| @@ -1,5 +1,7 @@ | ||
| 1 | 1 | import express from "express"; |
| 2 | −import { query } from "./db.js"; | |
| 2 | +import { query, update } from "./db.js"; | |
| 3 | + | |
| 4 | +const PAGE_SIZE = 10; | |
| 3 | 5 | |
| 4 | 6 | export function createApp() { |
| 5 | 7 | const app = express(); |
| @@ -13,5 +15,31 @@ export function createApp() { | ||
| 13 | 15 | res.json(order); |
| 14 | 16 | }); |
| 15 | 17 | |
| 18 | + // Search a customer's orders, newest first, 10 per page. | |
| 19 | + app.get("/orders", async (req, res) => { | |
| 20 | + const customer = req.query.customer; | |
| 21 | + const page = Number(req.query.page || 1); | |
| 22 | + const rows = await query(`SELECT * FROM orders WHERE customer = '${customer}' ORDER BY id DESC`); | |
| 23 | + const start = page * PAGE_SIZE; | |
WARNINGbugPagination off-by-one and unbounded page
start = page * PAGE_SIZE skips the first page entirely (page 1 starts at offset 10), and a negative or non-numeric page yields a negative/NaN offset. Compute the offset as (page - 1) * PAGE_SIZE and validate page >= 1. | ||
| 24 | + res.json({ page, orders: rows.slice(start, start + PAGE_SIZE), total: rows.length }); | |
| 25 | + }); | |
| 26 | + | |
| 27 | + // Apply a percentage discount to an order. | |
| 28 | + app.post("/orders/:id/discount", async (req, res) => { | |
| 29 | + const [order] = await query("SELECT * FROM orders WHERE id = $1", [req.params.id]); | |
WARNINGbugNo check that the order exists
If no row matches the id, order is undefined and line 31 throws a TypeError, crashing the request with a 500. Return 404 when no order is found. | ||
| 30 | + const percent = req.body.percent; | |
WARNINGbugUnvalidated percent input
percent comes straight from the request body with no numeric or range validation, so a non-numeric value produces NaN totals and a negative or huge percent can corrupt order totals. Validate it is a finite number within an allowed range. | ||
| 31 | + const total = order.total - (order.total * percent) / 100; | |
| 32 | + update(order.id, { total }); | |
WARNINGbugMissing await on update
update() is not awaited, so the response may be sent before the write completes and any rejection becomes an unhandled promise rejection. Await the call. | ||
| 33 | + res.json({ ...order, total }); | |
| 34 | + }); | |
| 35 | + | |
| 36 | + // Export all orders for the finance team. | |
| 37 | + app.get("/admin/export", async (req, res) => { | |
| 38 | + console.log(`Export requested with key ${req.headers["x-api-key"]}`); | |
CRITICALsecurityAdmin API key written to logs
The x-api-key header is logged before the auth check, leaking the admin credential into log storage where anyone with log access can reuse it. Remove the log line or log only a non-sensitive identifier. | ||
| 39 | + if (req.headers["x-api-key"] != process.env.ADMIN_KEY) return res.status(401).end(); | |
WARNINGsecurityNon-constant-time key comparison
Comparing the API key with != is not constant time and can leak the key via timing. Use a constant-time comparison such as crypto.timingSafeEqual on equal-length buffers. | ||
| 40 | + const rows = await query("SELECT * FROM orders"); | |
| 41 | + res.type("text/csv").send(["id,customer,total,status", ...rows.map((o) => `${o.id},${o.customer},${o.total},${o.status}`)].join("\n")); | |
WARNINGbugCSV export not escaped
Customer and status values are concatenated into CSV without quoting or escaping, so a value containing a comma, quote or newline corrupts the export and can enable CSV injection in spreadsheet tools. Escape/quote fields before joining. | ||
| 42 | + }); | |
| 43 | + | |
| 16 | 44 | return app; |
| 17 | 45 | } |
| 18 | 46 | |
The customer value is interpolated directly into the SQL string, so any caller can inject arbitrary SQL (e.g. `' OR '1'='1` or a UNION to read other tables). Use a parameterized query instead.
const rows = await query("SELECT * FROM orders WHERE customer = $1 ORDER BY id DESC", [customer]);