management: create_purchase echoes raw drizzle SQL to the MCP client #77

New issue

Closed

opened 2026-08-29 06:38:01 +00:00 by yuris · 0 comments

yuris commented

2026-08-29 06:38:01 +00:00

Owner

Found by AuthorErr while fixing #70, by going looking for the class after failing to reproduce the instance. Audited and deliberately not patched there, because it is not on the Hono error path #70 owns.

The defect

apps/management/lib/mcp/tools/purchases.ts:159 echoes a raw drizzle error — including the Failed query: text and the SQL — straight back to the MCP client.

That is the actual instance of the class #70 was filed about. #70's stated symptom, "no response body leaks driver internals", turned out not to reproduce on the Hono REST path: Hono's default error response emits a fixed string, and no Postgres text reached a caller at d0b5825. The leak is here instead, on the MCP surface, one layer over.

Why it matters more than the REST case would have

An MCP client is an LLM agent. So a raw Failed query: update "inventory" set "buy_price" = $1, "stock" = stock + $2 does not land in a log a human reads later — it lands in a model's context window, where it is:

schema disclosure — column names, table names and statement shape, handed to whatever is driving the tool
an invitation to retry creatively. A model that sees the failing SQL will often try to work around it rather than report it, which is precisely the behaviour you do not want against an inventory ledger
unactionable for the operator, who gets driver text instead of "that quantity is out of range"

The REST equivalent would have been bad. This version is worse because of who reads it.

Three defects in this codebase are the same shape at different layers, and each was filed only after someone measured rather than assumed:

This issue — a tool echoes too much, verbatim, to a caller that will act on it.
#70 — the Hono app said too little: every thrown error flattened to a bare 500, so getSession throwing on a stale jwks row was indistinguishable from a broken server.
#66 — apps/store's fetchCatalog refused to hear anything specific: a bare catch rewrote every throw, including a Next static-generation bailout, into one friendly message. That masking mis-stated the same error three times across three artifacts before review caught it.

One surface says nothing useful, one says too much, one refuses to listen. The general rule the three of them point at: decide deliberately what each boundary tells its caller, and make the untrusted-consumer case the one you design for.

Scope

Replace the raw echo with a readable, redacted message naming the field and the rule, matching what PR #68 established for the REST and MCP validation paths ("path": ["items", 0, "buyPrice"], Too big: expected number to be <=2147483647).
Log the full driver error server-side, at full fidelity. Redacting the response is not a reason to lose the diagnostic.
Audit the class rather than this line. Grep every MCP tool for a handler that stringifies a caught error into content. This codebase has a repeated pattern of one instance being fixed and its twin surviving, and #68's own write-path count was wrong four times for exactly this reason. List every site found, even those left alone, with the reason.
Check the WhatsApp bot path for the same shape while you are there: its nodes call the services directly and a raw driver error reaching a reply would be the same disclosure to the same kind of consumer. Note that #71 is bounding those service inputs, which reduces how often a driver error can occur but does not change what happens when one does.

Explicitly not this issue

#70 — the Hono onError handler. It is the right fix for the bare-500 half and it does not cover the MCP tool surface.
#71 — service-layer bounds. Fewer driver errors, same handling of the ones that remain.

Verification

Trigger a genuine driver error through create_purchase over a real MCP client and assert the response body contains no SQL, no Failed query:, no column or table names, and no connection string fragments — scan for a fixed list of leak fragments rather than eyeballing it.
Assert the full error still reaches the server log.
Assert PR #68's readable field-named rejections are unchanged — they are the shape callers may already depend on, and they must not be flattened into the new generic message.
Assert the row is untouched: compare a full-table md5(string_agg(...)) before and after, plus counts on purchase_items, inventory_lots, stock_movements and purchases. A purchase write touches four tables, so "returned an error" and "left nothing behind" are different claims.
Make it fail on purpose: confirm the redaction path actually executes, rather than assuming the branch is reachable.

Acceptance

No MCP tool response body contains driver text, SQL, or schema identifiers; the full error is still logged server-side; PR #68's field-named rejections are unchanged; and every MCP handler that stringifies a caught error is listed here with its disposition.

Second surface: the uploads route echoes R2 configuration and the endpoint host

Added at Reviewer76r2's condition during review of PR #76 — it agreed the leak should be tracked here rather than narrowed in that PR, provided this issue actually records the case by name. As originally filed it named only lib/mcp/tools/purchases.ts:159, so the comment in uploads.ts pointed at an issue that did not contain the defect it referenced.

Measured on a real request:

POST /api/v1/uploads/receipt
  -> 422 {"error":"Missing R2 configuration environment variables"}

Two sites, one shape:

app/api/[[...route]]/uploads.ts:57 echoes whatever uploadFile threw straight into the response body: const message = err instanceof Error ? err.message : "Upload gagal".
lib/storage/r2.ts:56 throws "Missing R2 configuration environment variables" — infrastructure state, not an operator instruction — and client.send() in the same function surfaces AWS SDK errors whose messages name the <CLOUDFLARE_ACCOUNT_ID>.r2.cloudflarestorage.com endpoint. So a misconfiguration or an SDK fault discloses the account identifier to any caller of the upload route.

This predates #70 and is not caused by the onError handler. PR #76 corrected only the false comment that claimed the branch was operator-facing and "not driver text"; the behaviour is unchanged and is tracked here.

Why it belongs with `uploadFile`, not with the route

The route cannot tell which of uploadFile's throws are safe to echo — that is knowledge the function has and the caller does not. r2.ts throws at three sites (:56, :77, :86) plus whatever the SDK raises, and only some of those are things an operator can act on ("file too large" yes; "missing environment variables" no). Fixing this at the route means re-deriving that classification at every call site, which is the same transport-versus-service mistake #71 was filed for.

Added scope

Classify uploadFile's failures at the function: operator-actionable versus infrastructure. Return or throw a typed distinction the caller can branch on, rather than a bare string.
The route echoes only the operator-actionable message; everything else becomes a redacted response with the detail logged server-side.
Assert no response body contains the R2 endpoint host, the account id, or a bucket name — scan for a fixed fragment list rather than eyeballing, as with the MCP case above.
Check the same shape on the product-photo upload path, not just the receipt path.

Added acceptance

No upload response body contains R2 configuration state, the r2.cloudflarestorage.com endpoint, the account id, or a bucket name; operator-actionable failures still name what the operator must change; and the classification lives in uploadFile rather than being re-derived per route.

Found by `AuthorErr` while fixing **#70**, by going looking for the class after failing to reproduce the instance. Audited and deliberately **not** patched there, because it is not on the Hono error path #70 owns. ## The defect `apps/management/lib/mcp/tools/purchases.ts:159` echoes a **raw drizzle error** — including the `Failed query:` text and the SQL — straight back to the MCP client. That is the actual instance of the class #70 was filed about. #70's stated symptom, *"no response body leaks driver internals"*, turned out not to reproduce on the Hono REST path: Hono's default error response emits a fixed string, and no Postgres text reached a caller at `d0b5825`. The leak is here instead, on the MCP surface, one layer over. ## Why it matters more than the REST case would have An MCP client is an **LLM agent**. So a raw `Failed query: update "inventory" set "buy_price" = $1, "stock" = stock + $2` does not land in a log a human reads later — it lands in a model's context window, where it is: - **schema disclosure** — column names, table names and statement shape, handed to whatever is driving the tool - **an invitation to retry creatively.** A model that sees the failing SQL will often try to work around it rather than report it, which is precisely the behaviour you do not want against an inventory ledger - **unactionable for the operator**, who gets driver text instead of "that quantity is out of range" The REST equivalent would have been bad. This version is worse because of who reads it. ## Related, and worth reading together Three defects in this codebase are the same shape at different layers, and each was filed only after someone measured rather than assumed: - **This issue** — a tool echoes too much, verbatim, to a caller that will act on it. - **#70** — the Hono app said too *little*: every thrown error flattened to a bare 500, so `getSession` throwing on a stale `jwks` row was indistinguishable from a broken server. - **#66** — `apps/store`'s `fetchCatalog` refused to hear anything specific: a bare `catch` rewrote every throw, including a Next static-generation bailout, into one friendly message. That masking mis-stated the same error three times across three artifacts before review caught it. One surface says nothing useful, one says too much, one refuses to listen. The general rule the three of them point at: **decide deliberately what each boundary tells its caller, and make the untrusted-consumer case the one you design for.** ## Scope - Replace the raw echo with a readable, redacted message naming the field and the rule, matching what PR #68 established for the REST and MCP validation paths (`"path": ["items", 0, "buyPrice"]`, `Too big: expected number to be <=2147483647`). - Log the full driver error **server-side**, at full fidelity. Redacting the response is not a reason to lose the diagnostic. - **Audit the class rather than this line.** Grep every MCP tool for a handler that stringifies a caught error into `content`. This codebase has a repeated pattern of one instance being fixed and its twin surviving, and #68's own write-path count was wrong four times for exactly this reason. List every site found, even those left alone, with the reason. - Check the WhatsApp bot path for the same shape while you are there: its nodes call the services directly and a raw driver error reaching a reply would be the same disclosure to the same kind of consumer. Note that **#71** is bounding those service inputs, which reduces how often a driver error can occur but does not change what happens when one does. ## Explicitly not this issue - **#70** — the Hono `onError` handler. It is the right fix for the bare-500 half and it does not cover the MCP tool surface. - **#71** — service-layer bounds. Fewer driver errors, same handling of the ones that remain. ## Verification - Trigger a genuine driver error through `create_purchase` over a real MCP client and assert the response body contains **no** SQL, no `Failed query:`, no column or table names, and no connection string fragments — scan for a fixed list of leak fragments rather than eyeballing it. - Assert the full error still reaches the server log. - Assert PR #68's readable field-named rejections are unchanged — they are the shape callers may already depend on, and they must not be flattened into the new generic message. - Assert the row is untouched: compare a full-table `md5(string_agg(...))` before and after, plus counts on `purchase_items`, `inventory_lots`, `stock_movements` and `purchases`. A purchase write touches four tables, so "returned an error" and "left nothing behind" are different claims. - **Make it fail on purpose**: confirm the redaction path actually executes, rather than assuming the branch is reachable. ## Acceptance No MCP tool response body contains driver text, SQL, or schema identifiers; the full error is still logged server-side; PR #68's field-named rejections are unchanged; and every MCP handler that stringifies a caught error is listed here with its disposition. --- ## Second surface: the uploads route echoes R2 configuration and the endpoint host Added at `Reviewer76r2`'s condition during review of PR #76 — it agreed the leak should be tracked here rather than narrowed in that PR, **provided this issue actually records the case by name.** As originally filed it named only `lib/mcp/tools/purchases.ts:159`, so the comment in `uploads.ts` pointed at an issue that did not contain the defect it referenced. Measured on a real request: ``` POST /api/v1/uploads/receipt -> 422 {"error":"Missing R2 configuration environment variables"} ``` Two sites, one shape: - **`app/api/[[...route]]/uploads.ts:57`** echoes whatever `uploadFile` threw straight into the response body: `const message = err instanceof Error ? err.message : "Upload gagal"`. - **`lib/storage/r2.ts:56`** throws `"Missing R2 configuration environment variables"` — infrastructure state, not an operator instruction — and `client.send()` in the same function surfaces AWS SDK errors whose messages **name the `<CLOUDFLARE_ACCOUNT_ID>.r2.cloudflarestorage.com` endpoint**. So a misconfiguration or an SDK fault discloses the account identifier to any caller of the upload route. This predates #70 and is not caused by the `onError` handler. PR #76 corrected only the **false comment** that claimed the branch was operator-facing and "not driver text"; the behaviour is unchanged and is tracked here. ### Why it belongs with `uploadFile`, not with the route The route cannot tell which of `uploadFile`'s throws are safe to echo — that is knowledge the function has and the caller does not. `r2.ts` throws at three sites (`:56`, `:77`, `:86`) plus whatever the SDK raises, and only some of those are things an operator can act on ("file too large" yes; "missing environment variables" no). Fixing this at the route means re-deriving that classification at every call site, which is the same transport-versus-service mistake **#71** was filed for. ### Added scope - Classify `uploadFile`'s failures at the function: operator-actionable versus infrastructure. Return or throw a typed distinction the caller can branch on, rather than a bare string. - The route echoes only the operator-actionable message; everything else becomes a redacted response with the detail logged server-side. - **Assert no response body contains the R2 endpoint host, the account id, or a bucket name** — scan for a fixed fragment list rather than eyeballing, as with the MCP case above. - Check the same shape on the product-photo upload path, not just the receipt path. ### Added acceptance No upload response body contains R2 configuration state, the `r2.cloudflarestorage.com` endpoint, the account id, or a bucket name; operator-actionable failures still name what the operator must change; and the classification lives in `uploadFile` rather than being re-derived per route.

yuris referenced this issue

2026-09-02 10:46:57 +00:00

fix(management): bind numeric inventory writes at the service, not the transport #78

yuris referenced this issue from a commit

2026-09-02 11:09:59 +00:00

fix(management): normalise the ZodError envelope across both zod majors

yuris referenced this issue from a commit

2026-09-28 20:48:12 +00:00

fix(security): redact raw SQL in MCP tools and R2 config in uploads (#77)

yuris referenced this issue from a pull request that will close it,

2026-09-28 20:48:20 +00:00

fix(security): redact raw SQL in MCP tools and R2 config in uploads #118