Principal-architect read-through of the codebase as of c9eb1b8 / 3a7cc54. The whole-repo design is solid — helmet, rate-limit, body cap, parameterized SQL via Sequelize, soft-delete pattern, zod whitelists, structured logging, graceful shutdown all in place. Below are the gaps worth working through in priority order. Each item becomes its own PR; this issue is the tracker.
Priority 1 — Security
P1-A. authKey stored in plaintext (CRITICAL).
ApiKey.akKEY and ApiMaster.amKEY hold the raw token string. Any DB leak / backup snapshot / read replica compromise immediately yields usable tokens for every active operator. auth.isMaster / getCompanyId query WHERE amKEY = ? against that plaintext.
Fix: application-side SHA-256 the incoming token before lookup; backfill existing rows. Token-format-preserving (no schema column rename). One-shot migration.
P1-B. 4xx error responses can leak internals via err.message.
error-handler.js returns err.message || 'Bad Request' for status < 500. A future middleware or library that throws with a Sequelize-style "column "X" does not exist" message would expose the column verbatim.
Fix: allowlist of safe-to-pass-through error types; everything else gets a generic message and the detail goes to the log.
P1-C. Query-string authKey= is not redacted from logs.
logger.js redacts header paths but pino-http's req.url serializer includes the raw query string. A misbehaving SDK that sends ?authKey=… in a GET ends up with the token in pino's structured log.
Fix: custom URL serializer in pino-http that strips authKey= (and apiKey, api_key, token) query params before logging.
Priority 2 — DRY / maintainability
P2-D. ~600 lines of auth-resolution boilerplate duplicated across 13 controllers.
auth.resolveAuth middleware exists but isn't used. Every controller method re-implements: extract authKey, isMaster check, getCompanyId check, compare to scope param. Bug-fix-once means changing 13 files.
Fix: mount resolveAuth on /v1/* (sets req.isMaster / req.companyId), refactor controllers to read those instead.
P2-E. Soft-delete WHERE <arch>: false is hand-applied to every query.
13 entities × ~5 endpoints = ~65 places where forgetting the arch filter silently returns deleted rows. Bound to drift.
Fix: Sequelize default scope { where: { <archCol>: false } } per model; explicit .unscoped() for the rare admin endpoint that needs to see archived rows.
Priority 3 — Functionality / API
P3-F. Link header pagination exists on customer + timeentry/bycompany only. 14 other list endpoints still emit body envelope only.
Fix: roll the helper through every controller with a findAndCountAll block (stashed WIP from iteration #30 — git stash pop).
P3-G. Idempotency-Key header not honored on POSTs. Network-retry semantics are the SDK author's problem; standard Stripe-style key support shifts that to the server.
Fix: middleware that hashes (key + route + body) → cache (in-memory LRU for now) → replay cached response.
P3-H. Bulk operations exist for Customer only. TimeEntry / Worker / Job / Invoice etc. all see the same ETL-import use case.
Fix: generalize the bulk pattern; apply across entities.
Priority 4 — Observability / operations
P4-I. /healthz doesn't report migration status. Operators can't tell at a glance if pending migrations exist.
Fix: include migrations.applied, migrations.pending in the response.
P4-J. No /metrics endpoint. Prometheus / OTel scraping needs request count + latency histograms.
Fix: add prom-client (or compatible) with default Node metrics + HTTP request histogram.
P4-K. Entities have no createdAt / updatedAt. Every model is timestamps: false. Audit "who/what/when" beyond pino access logs is impossible.
Fix: migration adding createdAt / updatedAt columns + Sequelize default; backfill existing rows from auditable proxies where available, NULL elsewhere.
Priority 5 — DX
P5-L. No ESLint / Prettier config. Style drift across 30+ controllers / schemas; ruff equivalent missing.
Fix: ESLint + recommended preset; CI gate on both forges.
P5-M. vi.mock('db.config.js') doesn't intercept nested CJS requires. Documented limitation from #62; API tests that try to drive DB-backed paths fall through to the real (broken) Sequelize. Tests that look like they verify behavior actually only verify the auth-contract and zod-validation slices.
Fix: restructure the auth lookups to take an injectable DB dependency (or move to ESM and benefit from real vi.mock interception), so the API test suite can cover real handler paths.
Plan: each item gets its own PR with Closes #<this> referencing this tracker on merge.
Proudly Made in Nebraska. Go Big Red! 🌽 https://xkcd.com/2347/
Principal-architect read-through of the codebase as of c9eb1b8 / 3a7cc54. The whole-repo design is solid — helmet, rate-limit, body cap, parameterized SQL via Sequelize, soft-delete pattern, zod whitelists, structured logging, graceful shutdown all in place. Below are the gaps worth working through in priority order. Each item becomes its own PR; this issue is the tracker.
Priority 1 — Security
P1-A.
authKeystored in plaintext (CRITICAL).ApiKey.akKEYandApiMaster.amKEYhold the raw token string. Any DB leak / backup snapshot / read replica compromise immediately yields usable tokens for every active operator.auth.isMaster/getCompanyIdqueryWHERE amKEY = ?against that plaintext.Fix: application-side SHA-256 the incoming token before lookup; backfill existing rows. Token-format-preserving (no schema column rename). One-shot migration.
P1-B. 4xx error responses can leak internals via
err.message.error-handler.jsreturnserr.message || 'Bad Request'for status < 500. A future middleware or library that throws with a Sequelize-style "column "X" does not exist" message would expose the column verbatim.Fix: allowlist of safe-to-pass-through error types; everything else gets a generic message and the detail goes to the log.
P1-C. Query-string
authKey=is not redacted from logs.logger.jsredacts header paths but pino-http'sreq.urlserializer includes the raw query string. A misbehaving SDK that sends?authKey=…in a GET ends up with the token in pino's structured log.Fix: custom URL serializer in pino-http that strips
authKey=(andapiKey,api_key,token) query params before logging.Priority 2 — DRY / maintainability
P2-D. ~600 lines of auth-resolution boilerplate duplicated across 13 controllers.
auth.resolveAuthmiddleware exists but isn't used. Every controller method re-implements: extract authKey, isMaster check, getCompanyId check, compare to scope param. Bug-fix-once means changing 13 files.Fix: mount
resolveAuthon/v1/*(setsreq.isMaster/req.companyId), refactor controllers to read those instead.P2-E. Soft-delete
WHERE <arch>: falseis hand-applied to every query.13 entities × ~5 endpoints = ~65 places where forgetting the arch filter silently returns deleted rows. Bound to drift.
Fix: Sequelize default scope
{ where: { <archCol>: false } }per model; explicit.unscoped()for the rare admin endpoint that needs to see archived rows.Priority 3 — Functionality / API
P3-F.
Linkheader pagination exists on customer + timeentry/bycompany only. 14 other list endpoints still emit body envelope only.Fix: roll the helper through every controller with a
findAndCountAllblock (stashed WIP from iteration #30 —git stash pop).P3-G.
Idempotency-Keyheader not honored on POSTs. Network-retry semantics are the SDK author's problem; standard Stripe-style key support shifts that to the server.Fix: middleware that hashes (key + route + body) → cache (in-memory LRU for now) → replay cached response.
P3-H. Bulk operations exist for Customer only. TimeEntry / Worker / Job / Invoice etc. all see the same ETL-import use case.
Fix: generalize the bulk pattern; apply across entities.
Priority 4 — Observability / operations
P4-I.
/healthzdoesn't report migration status. Operators can't tell at a glance if pending migrations exist.Fix: include
migrations.applied,migrations.pendingin the response.P4-J. No
/metricsendpoint. Prometheus / OTel scraping needs request count + latency histograms.Fix: add
prom-client(or compatible) with default Node metrics + HTTP request histogram.P4-K. Entities have no
createdAt/updatedAt. Every model istimestamps: false. Audit "who/what/when" beyond pino access logs is impossible.Fix: migration adding
createdAt/updatedAtcolumns + Sequelize default; backfill existing rows from auditable proxies where available, NULL elsewhere.Priority 5 — DX
P5-L. No ESLint / Prettier config. Style drift across 30+ controllers / schemas; ruff equivalent missing.
Fix: ESLint + recommended preset; CI gate on both forges.
P5-M.
vi.mock('db.config.js')doesn't intercept nested CJS requires. Documented limitation from #62; API tests that try to drive DB-backed paths fall through to the real (broken) Sequelize. Tests that look like they verify behavior actually only verify the auth-contract and zod-validation slices.Fix: restructure the auth lookups to take an injectable DB dependency (or move to ESM and benefit from real vi.mock interception), so the API test suite can cover real handler paths.
Plan: each item gets its own PR with
Closes #<this>referencing this tracker on merge.Proudly Made in Nebraska. Go Big Red! 🌽 https://xkcd.com/2347/