Harden bucket allocationValue: guard NaN/negative + server-side constraint + form cancel #70

New issue

Open

opened 2026-07-07 22:33:23 +02:00 by myrmidex · 0 comments

myrmidex commented

2026-07-07 22:33:23 +02:00

Owner

Context

Fast-follow from #54's finish-phase (pr-reviewer) review. Three related, non-blocking gaps in the add-bucket flow — none reachable through the shipped UI today, but real defense-in-depth holes.

1. `allocationValue` NaN/negative → silent zero-capacity bucket (main item)

frontend/src/lib/bucketInput.ts toScaledInteger() has no guard: dollarsToCents('abc') → NaN, which JSON.stringify coerces to null on the wire. Backend Bucket::$allocationValue is nullable ?int with no NotNull/PositiveOrZero constraint, so it's accepted, and BucketRoomCalculator treats null as 0 → a silent zero-capacity bucket instead of a rejected request.

Currently mitigated end-to-end only by the browser's native <input type="number" min={0}> constraint validation (blocks non-numeric/negative before handleSubmit). The safety net is entirely a browser behavior the JS functions don't know about, and there's no server-side backstop.

This is the missing counterpart to the existing BufferOnlyForFixedLimit validator family — allocationValue has no equivalent invariant.

Fix:

JS: Number.isFinite + >= 0 guard in toScaledInteger (return/throw or surface a field error rather than emit NaN).
Backend: an allocationValue constraint for fixed_limit/percentage types (required + PositiveOrZero), so a null/negative is a clean 422, not a silent zero bucket. Mirror the existing validator pattern (src/Validator/*).
Tests: add bucketInput.test.ts cases for dollarsToCents('-50') / dollarsToCents('abc') documenting the guarded behavior; backend test asserting the 422.

2. Add-bucket form cancel/close affordance

frontend/src/pages/AddBucketForm.tsx only calls setOpen(false) on successful submit — a user who opens the form and changes their mind can't collapse it back to the "Add bucket" button without navigating away. Not in #54's stated scope (deferred, not a regression). Add a Cancel button that resets fields + setOpen(false).

3. Characterization tests (nice-to-have)

Document the currently-unguarded bucketInput behavior with the negative/non-numeric cases above so a regression is caught if the guard lands or the functions get reused without a type="number" input.

Notes

Surfaced by pr-review of #54 (add-bucket), finish phase.
Relates to #69 (POST owner leak) as another "the safety net isn't where you'd expect" backend hygiene item.

## Context Fast-follow from #54's finish-phase (pr-reviewer) review. Three related, non-blocking gaps in the add-bucket flow — none reachable through the shipped UI today, but real defense-in-depth holes. ## 1. `allocationValue` NaN/negative → silent zero-capacity bucket (main item) `frontend/src/lib/bucketInput.ts` `toScaledInteger()` has no guard: `dollarsToCents('abc')` → `NaN`, which `JSON.stringify` coerces to `null` on the wire. Backend `Bucket::$allocationValue` is nullable `?int` with **no** `NotNull`/`PositiveOrZero` constraint, so it's accepted, and `BucketRoomCalculator` treats null as `0` → a silent zero-capacity bucket instead of a rejected request. Currently mitigated **end-to-end only** by the browser's native `<input type="number" min={0}>` constraint validation (blocks non-numeric/negative before `handleSubmit`). The safety net is entirely a browser behavior the JS functions don't know about, and there's no server-side backstop. This is the missing counterpart to the existing `BufferOnlyForFixedLimit` validator family — `allocationValue` has no equivalent invariant. **Fix:** - JS: `Number.isFinite` + `>= 0` guard in `toScaledInteger` (return/throw or surface a field error rather than emit NaN). - Backend: an `allocationValue` constraint for `fixed_limit`/`percentage` types (required + `PositiveOrZero`), so a null/negative is a clean 422, not a silent zero bucket. Mirror the existing validator pattern (`src/Validator/*`). - Tests: add `bucketInput.test.ts` cases for `dollarsToCents('-50')` / `dollarsToCents('abc')` documenting the guarded behavior; backend test asserting the 422. ## 2. Add-bucket form cancel/close affordance `frontend/src/pages/AddBucketForm.tsx` only calls `setOpen(false)` on successful submit — a user who opens the form and changes their mind can't collapse it back to the "Add bucket" button without navigating away. Not in #54's stated scope (deferred, not a regression). Add a Cancel button that resets fields + `setOpen(false)`. ## 3. Characterization tests (nice-to-have) Document the currently-unguarded `bucketInput` behavior with the negative/non-numeric cases above so a regression is caught if the guard lands or the functions get reused without a `type="number"` input. ## Notes - Surfaced by pr-review of #54 (add-bucket), finish phase. - Relates to #69 (POST owner leak) as another "the safety net isn't where you'd expect" backend hygiene item.