management: buy_price and stock are unvalidated; a negative stock silently publishes an item as habis #60

New issue

Closed

opened 2026-08-27 16:27:42 +00:00 by yuris · 0 comments

yuris commented

2026-08-27 16:27:42 +00:00

Owner

Found by Reviewer56r1 during round 3 of PR #56, proved by measurement rather than inferred, and deliberately not fixed there to avoid voiding a clean verdict on that PR's final round.

Two claims that were assumed safe and are not

While reviewing #56, the deferral rationale for leaving buy_price and stock on Number(x) || 0 in the inventory forms was recorded as "not published, server-bounded". Both halves are false.

1. Neither field is bounded at the trust boundary

buy_price and stock are z.number().optional() with no .int(), .nonnegative() or .max(). Measured consequence:

PATCH {"buy_price": 9999999999} → bare 500 (int4 overflow surfacing as an unhandled error)

This is the same defect class PR #56 just closed for sell_price, which now carries .int().nonnegative().max(MAX_RUPIAH) on all four write paths with MAX_RUPIAH = 2_147_483_647 single-sourced at lib/.../currency.ts:19.

2. `stock` does reach the storefront

This is the part that matters. Measured:

PATCH {"stock": -5} → 200, persisted, and the item's public availability flipped to "habis"

That is #42's derived-availability rule working exactly as designed — stock > 0 ? "ready" : allowIndent ? "indent" : "habis" — fed a value that nothing rejects. So a negative stock silently marks a real, in-stock product as unavailable to buyers, with no operator-visible error. The reviewer restored the affected row to 12.

stock was excluded from the #43 form work for a good and still-valid reason: it belongs to the FIFO ledger and moves through adjustment-modal.tsx. But "does not belong in that form" is not the same as "cannot be written to a nonsense value" — the REST and MCP paths write it directly.

Why this is filed rather than fixed in #56

#56's round-3 verdict was clean (0 blockers, 0 shoulds) and widening it would have voided that verdict on its fourth round. The deferral may well remain the right call for the form fields. What was wrong was the justification, and an inaccurate "this is safe" note is worse than an honest "this is deferred" one — it stops the next reader from checking.

Scope

Add .int().nonnegative() and an int4 .max() to buy_price and stock across every write path — both REST schemas and both MCP tools — reusing the MAX_RUPIAH constant #56 established rather than declaring a second literal.
Decide deliberately whether stock should be writable at all through REST/MCP given the FIFO ledger owns it. If it stays writable, a negative value must be rejected, not merely bounded. If a negative stock is ever legitimate (a correction?), then the derived-availability rule needs to say what it means publicly, because today it means habis.
The API has no error handler, so thrown errors flatten to bare 500s. That is pre-existing, predates #56, and touches every endpoint — it is noted here as the reason overflow surfaces badly, and is not in scope for this issue. File separately if it should be fixed.

Explicitly NOT this issue

Berat (gram) — see #59. That one is a type=number input misread, and weight_grams genuinely is .int().positive() server-side, so it really was bounded. The two defects cannot share a justification, which is exactly why they are separate issues.

Verification

PATCH and MCP-write buy_price and stock with: a 10-digit value, a negative value, a fractional value, and zero. Assert a readable 4xx, not a 500, and assert the row is untouched on rejection.
After any stock write, read availability back from /api/public/catalog and confirm it reflects a state the operator actually intended.
Confirm no new literal 2147483647 appears; the constant must stay single-source.
Confirm the FIFO ledger and stock-movement history stay consistent with whatever is allowed.

Acceptance

No buy_price or stock write can produce a bare 500 or a silently wrong public availability; every rejection is readable and leaves the row unchanged; the decision on whether stock is writable outside the ledger is recorded here.

Found by `Reviewer56r1` during round 3 of PR #56, proved by measurement rather than inferred, and deliberately **not** fixed there to avoid voiding a clean verdict on that PR's final round. ## Two claims that were assumed safe and are not While reviewing #56, the deferral rationale for leaving `buy_price` and `stock` on `Number(x) || 0` in the inventory forms was recorded as *"not published, server-bounded"*. **Both halves are false.** ### 1. Neither field is bounded at the trust boundary `buy_price` and `stock` are `z.number().optional()` with no `.int()`, `.nonnegative()` or `.max()`. Measured consequence: - `PATCH {"buy_price": 9999999999}` → **bare 500** (int4 overflow surfacing as an unhandled error) This is the same defect class PR #56 just closed for `sell_price`, which now carries `.int().nonnegative().max(MAX_RUPIAH)` on all four write paths with `MAX_RUPIAH = 2_147_483_647` single-sourced at `lib/.../currency.ts:19`. ### 2. `stock` **does** reach the storefront This is the part that matters. Measured: - `PATCH {"stock": -5}` → **200, persisted, and the item's public `availability` flipped to `"habis"`** That is #42's derived-availability rule working exactly as designed — `stock > 0 ? "ready" : allowIndent ? "indent" : "habis"` — fed a value that nothing rejects. So **a negative stock silently marks a real, in-stock product as unavailable to buyers**, with no operator-visible error. The reviewer restored the affected row to `12`. `stock` was excluded from the #43 form work for a good and still-valid reason: it belongs to the FIFO ledger and moves through `adjustment-modal.tsx`. But "does not belong in that form" is not the same as "cannot be written to a nonsense value" — the REST and MCP paths write it directly. ## Why this is filed rather than fixed in #56 #56's round-3 verdict was clean (0 blockers, 0 shoulds) and widening it would have voided that verdict on its fourth round. The deferral may well remain the right call for the *form* fields. What was wrong was the **justification**, and an inaccurate "this is safe" note is worse than an honest "this is deferred" one — it stops the next reader from checking. ## Scope - Add `.int().nonnegative()` and an int4 `.max()` to `buy_price` and `stock` across every write path — both REST schemas and both MCP tools — reusing the `MAX_RUPIAH` constant #56 established rather than declaring a second literal. - Decide deliberately whether `stock` should be writable at all through REST/MCP given the FIFO ledger owns it. If it stays writable, a negative value must be rejected, not merely bounded. If a negative stock is ever legitimate (a correction?), then the derived-availability rule needs to say what it means publicly, because today it means `habis`. - The API has **no error handler**, so thrown errors flatten to bare 500s. That is pre-existing, predates #56, and touches every endpoint — it is noted here as the reason overflow surfaces badly, and is **not** in scope for this issue. File separately if it should be fixed. ## Explicitly NOT this issue `Berat (gram)` — see **#59**. That one is a `type=number` input misread, and `weight_grams` genuinely **is** `.int().positive()` server-side, so it really was bounded. The two defects cannot share a justification, which is exactly why they are separate issues. ## Verification - `PATCH` and MCP-write `buy_price` and `stock` with: a 10-digit value, a negative value, a fractional value, and zero. Assert a readable 4xx, not a 500, and assert the row is untouched on rejection. - After any `stock` write, read `availability` back from `/api/public/catalog` and confirm it reflects a state the operator actually intended. - Confirm no new literal `2147483647` appears; the constant must stay single-source. - Confirm the FIFO ledger and stock-movement history stay consistent with whatever is allowed. ## Acceptance No `buy_price` or `stock` write can produce a bare 500 or a silently wrong public `availability`; every rejection is readable and leaves the row unchanged; the decision on whether `stock` is writable outside the ledger is recorded here.

yuris referenced this issue

2026-08-27 16:47:03 +00:00

data: no published item has category, brand, fitment, oemNumbers or weightGrams — three of four facets are empty in production #61

yuris referenced this issue

2026-08-27 18:21:22 +00:00

feat(management): product image upload to R2 from both stok forms #62

yuris referenced this issue

2026-08-27 19:38:26 +00:00

feat(management): product image upload to R2 from both stok forms #62

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

2026-08-28 02:02:26 +00:00

fix(management): refuse unparseable numeric input in both inventory forms; bound buy_price, stock and weight_grams #68