fix(security): financial input validation (positive amounts, mass-assignment, confirmation entropy)#121
Merged
Conversation
A 5-reviewer adversarial pass confirmed tenant isolation is solid and found a financial input-validation cluster (real, within-tenant integrity bugs). Fixes: - Positive-amount validation: new shared @IsMoneyString validator (decimal.js). payment create/authorize amount > 0; reservation totalAmount >= 0; folio charge amount must be a valid decimal AND the service rejects non-positive unless type=adjustment or a reversal (negatives previously inverted folio balances / manufactured credit). - Kill setRateOverride mass-assignment: real SetRateOverrideDto replaces @Body() any, so the ValidationPipe strips unknown keys and bounds adjustmentValue (was spread verbatim into the config JSON). - Confirmation numbers are now 128-bit random (Crockford base32), not HAIP-{timestamp}-{4hex}, closing within-tenant booking enumeration. - Rate limiter ignores X-Forwarded-For unless RATE_LIMIT_TRUST_PROXY=true, so the brute-force brake can't be bypassed by rotating the header. - TaxController: ParseUUIDPipe on all id/propertyId params (malformed -> 400). - folio postCharge validates originalChargeId whenever present, not only on reversals (removes a dangling cross-tenant soft reference). New specs (money validator, confirmation-token entropy, rate-limit XFF, folio charge sign rules) + existing suite: 913 green; typecheck + lint clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the financial input-validation cluster the 5-reviewer adversarial pass found after confirming tenant isolation is solid. These are real, exploitable within-tenant integrity bugs (not cross-tenant). One bounded PR — no loop.
HIGH
@IsMoneyStringvalidator (parses viadecimal.js, rejects NaN/Infinity and — by default — zero/negatives).paymentcreate + authorizeamount→ must be > 0.reservation.totalAmount→ ≥ 0 (comp rooms allowed).foliochargeamount: still a valid decimal at the DTO layer (credits are legit), but the service now rejects a non-positive amount unlesstype='adjustment'or a reversal. (Negatives previously inverted folio balances / manufactured credit; the refund path was already guarded, these weren't.)taxAmount≥ 0.setRateOverridemass-assignment. RealSetRateOverrideDtoreplaces@Body() override: any, so the global ValidationPipe strips unknown keys and boundsadjustmentValue(was spread verbatim into theconfigJSON column).MEDIUM
HAIP-{timestamp}-{4hex}(~16 bits) — closes within-tenant booking enumeration. Generation-only change; existing rows still resolve.X-Forwarded-ForunlessRATE_LIMIT_TRUST_PROXY=true(default off) — a client can't rotate the header to dodge the limit.ParseUUIDPipeon every id/propertyIdparam (malformed → 400, not a 500).LOW
folio.postChargevalidatesoriginalChargeIdwhenever present (not only on reversals) — removes a dangling cross-tenant soft reference.Out of scope (noted)
Guest email-oracle on
/connect/book(guests are cross-property by design — product call), connect-gpt guest-address log redaction (separate tool).Verify
New specs:
is-money-string.validator,confirmation-token(entropy/format/no-collision), rate-limit XFF default-ignore, folio charge sign rules. 913 api tests green, typecheck + lint clean. Demo unaffected (validation only tightens inputs; AUTH-off paths unchanged).🤖 Generated with Claude Code
Generated by Claude Code