What this pass was actually for

Unit tests answer "does this function do what I told it to do." They don't answer "what does helmet actually send," "does this connection pool actually shrink over time," or "what does this endpoint do under ten concurrent requests instead of one." Those questions only answer themselves if you go measure — read the real response headers, watch the real pool, run a real load test. This pass found four bugs that every existing test suite had no way of catching, because none of them were about whether the code was correct. They were about what the code actually does once it's running.

The header that would have silently broken production

CORS was already configured correctly — Access-Control-Allow-Origin locked to the frontend's real origin, credentials allowed, nothing permissive. That should have been the end of the story. It wasn't, because CORS isn't the only header that decides whether a browser will let JavaScript read a cross-origin response.

helmet()'s default Cross-Origin-Resource-Policy is same-origin. That header is enforced independently of CORS, by the browser, and it doesn't care what Access-Control-Allow-Origin says. In development, frontend and backend both run on localhost, so this never fires — the resource policy is satisfied by default. The first time it would have mattered is the first real deploy, frontend on Vercel and backend on Render, two different origins. Every API response would have been correctly CORS-approved and then silently discarded by the browser anyway, and the failure mode is close to the worst kind: no server-side error, no failed request in the network tab's status column, just a fetch that resolves to nothing usable.

I only found this by pulling actual response headers with curl -i against a running server and reading every line, not by reading helmet's documentation and assuming the defaults were fine. The fix is one line:

app.use(helmet({ crossOriginResourcePolicy: { policy: "cross-origin" } }));

CORS still does the real access-control work — this just gets a stricter, unrelated header out of its way.

A SQL injection vector that was never exploited, and shouldn't get the chance to be

buildUpdateQuery is a small helper shared by three landlord-admin repos — it turns an updates object into a parameterized SET column = $1, other = $2 clause. The values are always parameterized correctly. The column names, though, go straight into the SQL string, because you can't parameterize an identifier in Postgres.

Every one of the 16 call sites today passes hardcoded literal keys — { status: newStatus }, never req.body directly. So there's no exploitable path right now. But "safe by convention" is a fact about today's callers, not a property of the function, and the whole point of a shared helper is that people reuse it without re-deriving that history. The fix closes the gap at the one place it can be closed once, instead of trusting every future call site to remember:

const SAFE_COLUMN_NAME = /^[a-z_][a-z0-9_]*$/i;

for (const field of fields) {
  if (!SAFE_COLUMN_NAME.test(field)) {
    throw new Error(`buildUpdateQuery: unsafe column name "${field}"`);
  }
}

This is the least dramatic finding in this pass and arguably the most important one to have written down: it's a bug that doesn't exist yet, in a codebase that's still small enough that "doesn't exist yet" is still true everywhere. That won't stay true by itself.

Two things that fail slowly instead of loudly

Startup did a connectivity check against Postgres using db.connect(), logged "Connected to PostgreSQL," and moved on. pool.connect() returns a checked-out client that has to be released explicitly — and this one never was. That's a connection leaked for the entire lifetime of the process, permanently, on every boot. Pool size 10 quietly became pool size 9 with no error anywhere, because nothing was wrong yet — just one connection fewer than configured, which only becomes visible the day the app is under enough concurrent load to actually need all 10. The fix is smaller than the bug: pool.query("SELECT 1") acquires and releases automatically, so the same healthcheck stops costing anything.

The Redis client had the opposite problem — not too eager to hold a resource, too patient to let one go. maxRetriesPerRequest: null tells ioredis to retry a command forever if Redis is unreachable, rather than ioredis's own default of 20 attempts. Every authenticated request in this app checks a session in Redis, so "retry forever" doesn't mean "Redis is slow" during an outage — it means every single authenticated request hangs indefinitely, with no timeout, until Redis comes back. Removing that override was the whole fix; the library's own default is already the right call for a web app instead of a queue worker.

Neither of these failed a test. Both would have failed a pager.

The 500 that vanished

The error handler shaped a correct response for the client — right status code, a message, a stack trace in development only — and then did nothing else. It never wrote the error anywhere. In production, a crash 900 requests deep would produce exactly one artifact: a 500 in the pino-http request-completion line, with no message and no stack, because pino-http logs the request, not the error object, unless something explicitly hands it one. Whatever actually broke would be unrecoverable information the moment the response went out.

if (req.log) {
  req.log.error({ err }, err.message || "Unhandled error");
}

One line, and I verified it live rather than trusting the diff: hit a route that doesn't exist, and the log now contains a real ERROR-level entry with the full stack, keyed to that request's own logger context.

While chasing this I added the health-check endpoint this app didn't have at all — GET /api/health, no DB or Redis dependency, for whatever eventually monitors uptime. First pass, I put it after the global rate limiter, which is itself a small version of the same bug class: a monitor polling that route regularly would burn through the same 100-requests-per-15-minutes budget as real traffic and start reporting the service as down because of its own healthcheck. Moved it ahead of the limiter before it ever shipped anywhere.

What ten concurrent logins actually cost

Everything above was found by reading. This part required running the app under load and writing down what happened, using autocannon against the local dev server.

GET /api/health — no database, no Redis, nothing but Express and the middleware stack — sustained ~4,389 requests/sec at 50 concurrent connections, p50 latency 9ms, p99 29ms. That's the ceiling: the most this stack can do when there's nothing real to compute.

POST /landlord-admin/auth/login — the same stack, but doing real work — sustained ~6 requests/sec at 10 concurrent connections. p50 latency 1.61 seconds. p99 1.84 seconds. A single login request, sent alone with no concurrency at all, took ~0.56 seconds. Ten concurrent logins didn't just queue linearly behind that baseline — latency roughly tripled.

That ratio is the actual finding, not just the raw numbers. Password hashing here uses bcrypt at cost factor 12, which is deliberately slow and deliberately CPU-bound — that's the entire security property of a work-factor hash. Node runs that kind of blocking native work on libuv's threadpool, which defaults to 4 threads (UV_THREADPOOL_SIZE is unset anywhere in this app). Ten concurrent bcrypt.compare calls competing for four threads means most of them are queued, not running, for most of the request — which is exactly the roughly-3x latency inflation the numbers show. The database and Redis are not the bottleneck here. Four threads and a cost-12 hash are.

This doesn't mean cost factor 12 is wrong — it's the OWASP-recommended floor for bcrypt today, and lowering it trades real credential-cracking resistance for throughput, which is the wrong trade to make first. What it means is that this specific endpoint has a real, now-measured ceiling under concurrent load, and if login ever needs to survive a traffic spike, UV_THREADPOOL_SIZE and the shape of that thread contention are the actual lever — not the database.

What's still honest gaps, not fixes

  • No error-tracking or APM service. Structured logs exist now and errors are captured server-side, but nothing aggregates them, alerts on a spike, or gives a stack trace outside a log file someone has to go read. Adding one is a real decision — cost, another vendor, another set of keys — not something to fold silently into a hardening pass.
  • One frontend npm audit finding, left unfixed on purpose. react-router-dom's advisory (GHSA-qwww-vcr4-c8h2) is scoped to "RSC Mode" — React Server Components — which this app doesn't use; it's a plain Vite SPA behind <BrowserRouter>. The only available fix is a forced downgrade to a version before the one currently installed, which is a semver-major regression for a threat model that doesn't apply here. Documented instead of silently ignored or reflexively forced.
  • The load test ran against one machine, one process, one login. It tells you the shape of the bottleneck, not a production capacity number — a real number needs a real deploy target and more than one account hammering it at once.

The pattern underneath all four

None of these four bugs were about logic being wrong. The code did exactly what it was written to do — check in with Postgres at startup, retry a Redis command that failed, format an error response, hash a password securely. Every one of them was a gap between "correct" and "correct under the conditions production actually creates": a different origin, a long-running process, an outage, a concurrent request. That's the actual argument for measuring instead of reading — these aren't the kind of bug a code review catches, because the code reads exactly right.

Live link coming soon