Setup
This PR was mostly written by an AI assistant from the ticket “Let account admins export their orders as CSV.” The author tested it by exporting their own test account and opening the file in a spreadsheet.
Context you need:
- The product is multi-tenant. Every order, customer, and user belongs to exactly one account.
requireUserchecks the session and setsreq.user, which hasid,accountId, androle(adminormember).- The largest accounts have about 80,000 orders.
audit.logwrites to the compliance audit trail that enterprise customers rely on.
--- a/src/routes/orders.ts
+++ b/src/routes/orders.ts
@@ -1,5 +1,32 @@
import { Router } from 'express';
import { db } from '../db';
import { requireUser } from '../auth';
+import { audit } from '../audit';
+
+const CSV_HEADER = 'order_id,customer_name,customer_email,total,item_count';
export const router = Router();
+
+router.get('/accounts/:accountId/orders/export', requireUser, async (req, res) => {
+ const { accountId } = req.params;
+ const orders = await db.order.findMany({ where: { accountId } });
+
+ const rows = [];
+ for (const order of orders) {
+ const customer = await db.customer.findUnique({ where: { id: order.customerId } });
+ const items = await db.orderItem.findMany({ where: { orderId: order.id } });
+ rows.push(toCsvRow(order, customer, items));
+ }
+
+ try {
+ await audit.log('orders.export', { accountId, count: rows.length });
+ } catch (e) {}
+
+ res.setHeader('Content-Type', 'text/csv');
+ res.send([CSV_HEADER, ...rows].join('\n'));
+});
+
+function toCsvRow(order, customer, items) {
+ return [order.id, customer.name, customer.email, order.total, items.length].join(',');
+}
Outcome
A PR shaped like this one was approved after a review that commented on variable names and suggested a streaming response “for later.” The missing authorization check went unnoticed until a customer’s security team found it three months later: any logged-in user of any account could download any other account’s customers and orders by changing the ID in the URL.
The fix shipped in a day. The incident response, customer notifications, and audit took the rest of the quarter.
Senior commentary
“With any endpoint that takes an ID from the URL, my first question is: who is allowed to ask for this ID? Here the answer is ‘anyone who’s logged in’, because requireUser only checks that you’re someone. It doesn’t check that you belong to accountId, and it doesn’t check that you’re an admin, which is what the ticket asked for.
“Then I look at the loop. Two queries per order means 160,000 queries for the biggest account, all in one request. That’s an outage you’ll cause on a Monday when an admin clicks Export.
“The empty catch is easy to skim past, and it’s the one compliance people care about: the export succeeds and leaves no audit record. I don’t mind best-effort logging in general. I do mind it for the audit trail of a bulk data export.
“Notice that none of these show up when you test with your own small account. That’s why ‘I tested it’ isn’t enough for a review.”
Discussion questions
- Which flaw would you insist on fixing before merge, and which could become follow-up tickets? Why?
- How would you test this endpoint so the authorization gap fails a test?
- The author tested it with a small account. What kind of test data would have exposed the performance problem?
- When is swallowing an error the right call? Why not here?
Answer key
| # | Flaw | Severity |
|---|---|---|
| 1 | Missing authorization. Any logged-in user can export any account by changing :accountId. It never checks req.user.accountId === accountId, and never checks that the user is an admin. | High |
| 2 | N+1 queries. Two queries per order: about 160,000 for the largest account, in a single request. Load customers and items in bulk or with a join. | High |
| 3 | Audit failure swallowed. The empty catch lets the export succeed with no audit record. For a compliance trail, fail the export (or queue the audit write reliably) instead. | Medium |
| 4 | CSV injection and broken escaping. Names containing commas, quotes, or newlines corrupt the file, and values starting with =, +, -, or @ can run as formulas in spreadsheet apps. Use a CSV library and neutralise formula prefixes. | Medium |
| 5 | Unbounded memory. Loads every order and builds the whole file in memory. Stream rows, or run large exports as a background job. | Medium |
| 6 | Null customer. If a customer was deleted, customer is null and the whole export crashes on customer.name. | Low |