fix(api): stop leaking failed SQL in v1 table 500s and restore the 423 lock field - #6569
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Adds shared Pins the leak and lock behaviors with route and helper tests. Reviewed by Cursor Bugbot for commit 5519d45. Configure here. |
Greptile SummaryThe PR centralizes table orchestration-result error responses so internal failures use generic messages while classified errors retain their status and safe message.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/table/utils.ts | Adds the shared failure-result projection that conceals internal messages, preserves classified errors, and includes lock metadata. |
| apps/sim/app/api/v1/tables/[tableId]/route.ts | Routes delete failures through the shared projection to prevent SQL details from reaching v1 clients and restore the lock field. |
| apps/sim/app/api/v1/tables/[tableId]/rows/[rowId]/route.ts | Applies the same safe projection to row deletion failures. |
| apps/sim/app/api/table/[tableId]/route.ts | Centralizes lock, rename, move, and delete failure rendering while preserving the explicit move-table not-found message. |
| apps/sim/app/api/table/utils.test.ts | Covers generic internal errors, classified failures, status mapping, and 423 lock metadata. |
| apps/sim/app/api/v1/tables/[tableId]/route.test.ts | Adds route-level regression tests for SQL concealment and lock-kind responses. |
| apps/sim/app/api/v2/tables/utils.ts | Replaces the removed row-write alias with its identical underlying classifier. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Table route] --> B[perform mutation]
B -->|Success| C[Success response]
B -->|Failure result| D[orchestrationOutcomeErrorResponse]
D --> E{Error classification}
E -->|Internal or absent| F[Generic fallback and 500]
E -->|Locked| G[Specific error and lock field and 423]
E -->|Other classified| H[Specific safe error and mapped status]
Reviews (2): Last reviewed commit: "chore(tables): tidy v1 error projection ..." | Re-trigger Greptile
…ssage leak
The v1 table routes were rewritten to consume `lib/table/orchestration`
results, and two response behaviors drifted from what the live API returned.
Information disclosure: an unclassified failure's `outcome.error` carries
whatever text the fault happened to have. Drizzle wraps a throw raised inside
a transaction in an error whose own message is the failed statement and its
bound parameters, so `DELETE /api/v1/tables/{tableId}` and
`DELETE /api/v1/tables/{tableId}/rows/{rowId}` returned that verbatim in the
500 body to any API-key holder. Previously these returned a fixed generic
string.
Lost `lock` field: the 423 body used to be `{ error, lock }`. The delete,
row-delete, and column-update routes (v1 and internal) dropped the lock kind
the orchestration result already computes, leaving clients unable to tell
which lock to clear.
Both are fixed at one altitude: `orchestrationOutcomeErrorResponse` in
`app/api/table/utils.ts` is now the only way a table route projects an
orchestration failure onto the wire. It renders the route's fallback for an
unclassified failure and the real message for a classified one (validation,
not-found, conflict, locked keep their specific text), and carries `lock` on a
423. A future route cannot reintroduce either bug by hand-spelling the body.
Duplicate table names on `POST /api/v1/tables` keep answering 409 rather than
reverting to the previous 400. 409 is the correct semantic, and every other v1
duplicate-name surface (knowledge, files, workflow import) already answers 409;
the tables 400 was the outlier. v1 tables appears in no published OpenAPI
document and no in-repo client branches on the status, so the compatibility
cost is limited to a caller matching 400 specifically for a name collision.
fe432f9 to
23b7792
Compare
12e43a6 to
5519d45
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5519d45. Configure here.
262ce32
into
improvement/v2-route-standardization
Stacked on #6568 → #6567 → #6565 → #6560. Review only this PR's own commits; merge after its parents.
v1 is the live public API. It was rewritten to delegate to the shared application layer, and three response behaviors drifted from
origin/main.1. Failed SQL and bound parameters leaked in 500 bodies
performDeleteTable/performDeleteTableRowreturntoError(error).messagefor unclassified throws, and both routes renderedoutcome.errorverbatim. Both delete paths run inside locked transactions, where drizzle wraps a throw in aDrizzleQueryErrorwhose message is the failed SQL.mainreturned a fixed generic string.Reverting the fix shows exactly what any API-key holder could harvest:
2. The 423 lock field was dropped
mainreturned{ error, lock }; these routes returned{ error }only, while still computinglockand discarding it.lockis the only thing telling a client which lock to clear.3. Duplicate table name: 400 → 409 — keeping 409
This changed on
POST /api/v1/tables, and the recommendation is to keep the new status rather than restore parity:OrchestrationErrorTSDoc documents the exact failure mode this replaced: "adding 'already exists' to a message demoted a 409 to a 400." Main's 400 came from string-matching'already exists'— the anti-pattern the typed-code migration exists to kill.Tradeoff accepted: a client matching 400 specifically for a name collision now sees 409 — but such a client already needs a 409 branch for every sibling v1 endpoint.
Fix
One shared projection,
orchestrationOutcomeErrorResponse, the result-returning counterpart to the existing throw-path helper. Applied at nine call sites across three v1 and three internal table routes;statusForOrchestrationErrorno longer appears in any of them. It reusesmessageForOrchestrationError, which genericizes only unclassified/internal— classified errors keep their specific text.Also removes the
rowWriteErrorResponsealias (15 sites): one touched file imported both names for the same function object and used one in PATCH, the other in DELETE.Deliberately not converted:
import,import-csv, andrestorestill hand-roll the projection. Converting them is a behavior change, not a tidy — the hand-rolled form returnsoutcome.errorwhenerrorCodeisundefined, and that widening is the leak fix. It belongs in a change that owns the behavior.Reverting turns 6 tests red. type-check · biome · 1113 tests ·
check:api-validation·check:openapi— all pass.