Harden step completion integrity, tenant isolation, and API error handling#36
Conversation
Agent-Logs-Url: https://github.com/OlympusLedgerOrg/TiM/sessions/9aecc93e-3ae2-4b8e-83d9-bb84283d81ab Co-authored-by: OlympusLedgerOrg <240737312+OlympusLedgerOrg@users.noreply.github.com>
Agent-Logs-Url: https://github.com/OlympusLedgerOrg/TiM/sessions/9aecc93e-3ae2-4b8e-83d9-bb84283d81ab Co-authored-by: OlympusLedgerOrg <240737312+OlympusLedgerOrg@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Hardens core backend behaviors around multi-tenant integrity, atomic step completion/auditing, JWT secret handling, and consistent error responses—reducing cross-tenant risk and improving operational safety for production.
Changes:
- Make step completion tenant-scoped and transactional (including audit logging) and emit sockets only after commit.
- Remove the equipment downtime N+1 query by batching active downtime reads.
- Centralize JWT secret derivation + add production guardrails, and add a global Express error handler that normalizes unexpected failures.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/services/stepService.ts | Tenant-scoped step lookup; wraps step update + audit insert in a Prisma transaction; emit after commit. |
| backend/src/controllers/stepController.ts | Passes tenantId into step completion service. |
| backend/src/services/equipmentService.ts | Replaces per-equipment downtime lookups with a single batched findMany + indexing. |
| backend/src/config/jwt.ts | Introduces shared JWT secret helpers + production validation. |
| backend/src/app.ts | Calls JWT validation on startup; uses shared JWT secret in Socket.IO auth; adds global error handler. |
| backend/src/routes/auth.ts | Uses shared JWT secret when signing login tokens. |
| backend/src/middleware/auth.ts | Uses shared JWT secret for request auth verification. |
| backend/prisma/src/middleware/auth.ts | Switches Prisma-side auth middleware to shared JWT secret helper. |
| backend/tests/completeStep.test.ts | Adds coverage for tenant-scoped lookup and transactional + audit behavior. |
| backend/tests/equipment.test.ts | Updates mocks/assertions to validate batched downtime retrieval. |
| backend/tests/auth.test.ts | Adds coverage asserting sanitized 500 response on unexpected auth failures. |
| backend/tests/jwtConfig.test.ts | Adds tests for production JWT secret guardrails and non-prod allowances. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
|
|
||
| export function getJwtSecret() { |
There was a problem hiding this comment.
getJwtSecret() can still return the insecure default in production if some code path imports it without having called validateJwtSecret() (validation currently only happens in app.ts). Consider running validation inside getJwtSecret() (or at module initialization in config/jwt.ts) so the production guardrail can’t be bypassed by alternative entrypoints.
| export function getJwtSecret() { | |
| export function getJwtSecret() { | |
| validateJwtSecret(); |
| export function getJwtSecret() { | ||
| return new TextEncoder().encode(process.env.JWT_SECRET || INSECURE_DEFAULT_JWT_SECRET); |
There was a problem hiding this comment.
getJwtSecret() allocates a new Uint8Array on every call (new TextEncoder().encode(...)). Since this runs per-request in requireAuth, consider memoizing the encoded secret (and optionally refreshing it only when JWT_SECRET changes) to avoid repeated allocations.
| const downtimeByEquipment = new Map<string, typeof activeDowntimeEvents[0]>(); | ||
| for (const event of activeDowntimeEvents) { | ||
| if (!downtimeByEquipment.has(event.equipmentId)) { | ||
| downtimeByEquipment.set(event.equipmentId, event); | ||
| } |
There was a problem hiding this comment.
The Map value type typeof activeDowntimeEvents[0] is hard to read and can behave unexpectedly with stricter TS options (e.g., noUncheckedIndexedAccess). Prefer a clearer element type like (typeof activeDowntimeEvents)[number] (or a Prisma DowntimeEvent type) for maintainability.
| const originalEnv = process.env; | ||
|
|
||
| beforeEach(() => { | ||
| process.env = { ...originalEnv }; | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| process.env = originalEnv; |
There was a problem hiding this comment.
This test reassigns process.env (not just individual keys). Replacing the process.env object can cause subtle cross-test interference in a shared Jest worker; consider saving/restoring only JWT_SECRET/NODE_ENV (or using a helper) instead of overwriting process.env entirely.
| const originalEnv = process.env; | |
| beforeEach(() => { | |
| process.env = { ...originalEnv }; | |
| }); | |
| afterAll(() => { | |
| process.env = originalEnv; | |
| const originalJwtSecret = process.env.JWT_SECRET; | |
| const originalNodeEnv = process.env.NODE_ENV; | |
| beforeEach(() => { | |
| if (originalJwtSecret === undefined) { | |
| delete process.env.JWT_SECRET; | |
| } else { | |
| process.env.JWT_SECRET = originalJwtSecret; | |
| } | |
| if (originalNodeEnv === undefined) { | |
| delete process.env.NODE_ENV; | |
| } else { | |
| process.env.NODE_ENV = originalNodeEnv; | |
| } | |
| }); | |
| afterAll(() => { | |
| if (originalJwtSecret === undefined) { | |
| delete process.env.JWT_SECRET; | |
| } else { | |
| process.env.JWT_SECRET = originalJwtSecret; | |
| } | |
| if (originalNodeEnv === undefined) { | |
| delete process.env.NODE_ENV; | |
| } else { | |
| process.env.NODE_ENV = originalNodeEnv; | |
| } |
This PR addresses four backend gaps with outsized operational risk: step completion was not tenant-scoped, step state and audit writes were not atomic, equipment work-center reads used an N+1 downtime query pattern, and unhandled backend errors were not normalized. It also closes the insecure production footgun where JWT handling silently fell back to
change-me.Step completion: enforce tenant isolation + atomic audit trail
tenantIdin addition toworkOrderIdandstepIdtenantIdon the audit record for consistency with the rest of the backendEquipment service: remove N+1 downtime queries
findFirst()downtime lookups with one batchedfindMany()equipmentIdviaMap1 + Nto2JWT configuration: centralize secret handling
JWT_SECRETis missing or still set to the insecure defaultExpress error handling: normalize unexpected failures
400500sTest coverage
Example of the step completion change: