fix(security): cross-tenant FK ownership in reservation/room/housekeeping (audit #4, #5, #6)#119
Merged
Merged
Conversation
…ping (audit #4,#5,#6) Closes three HIGH findings from the independent Codex security audit. The DB schema FKs only constrain the referenced row id, NOT same-property — so a caller at property A could pass an FK id belonging to property B and the row would insert/update happily. Each service now verifies the FK belongs to the caller's propertyId BEFORE the write. - reservation.service.ts (#4): * create(): assertSamePropertyFk for dto.roomTypeId + dto.ratePlanId before insert. * modify(): same checks when dto.roomTypeId or dto.ratePlanId is supplied. * Shared private helper assertSamePropertyFk(table, id, propertyId, label). - room.service.ts (#5): createRoom verifies dto.roomTypeId is same-property. - housekeeping.service.ts (#6): create() verifies dto.roomId is same-property before generating the checklist or inserting. (Inner generateChecklist room + reservation lookups already scoped on main from PR #117 — defense in depth.) Tests (TDD): - New reservation-fk-ownership.spec.ts — 3 cross-tenant denial cases. - New room-fk-ownership.spec.ts — foreign roomTypeId denied; same-property allowed. - New housekeeping-fk-ownership.spec.ts — foreign roomId denied. - Updated existing housekeeping.service.spec.ts + housekeeping-dashboard.spec.ts mock sequences to account for the new leading FK select call. 826/826 api specs pass. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…llotment (audit follow-on) The Codex re-audit of PR #119 verified that #4, #5, #6 are RESOLVED but adversarially found the SAME anti-pattern in four more service create() methods the original audit missed: - FolioService.create — caller-supplied reservationId / bookingId not scoped - DepositService.recordDeposit — reservationId / paymentId not scoped - RatePlanService.create — roomTypeId not scoped (derived-parent check already was) - AllotmentService.createBlock — groupProfileId / ratePlanId not scoped All four now run the same property-scoped SELECT before the insert. Pattern is uniform: select id where id=$1 AND property_id=$2 → BadRequestException if empty. Also closes test-quality gaps Codex flagged on the original PR: - New modify() test for foreign roomTypeId (only ratePlanId was covered). - New housekeeping auto-checklist branch test that asserts the FK guard trips BEFORE generateChecklist runs (asserts db.select called exactly once). Plus 4 new TDD specs covering the new cross-tenant denials and one existing rate-plan spec updated to provide the FK select mock so it can still reach the derived-without-parent BadRequest path. 835/835 api specs pass. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…inbound mapping
Two more live cross-tenant FK writes Codex found in the adversarial re-audit:
1. AllotmentService.setInventory — accepted caller-supplied roomTypeId without
verifying same-property. The availability lookup would return 0 sellable for a
foreign roomTypeId, but a roomsAllotted=0 request still passed validation and
inserted a cross-property allotmentBlockInventory row.
2. InboundReservationService — handleNewReservation and handleModification both
resolved roomTypeId/ratePlanId from channelConnections.config.{room,rate}Mapping
(operator-supplied JSON) and wrote them straight into reservations. A
misconfigured or maliciously edited mapping could point at a foreign-tenant
id, producing cross-tenant reservation rows on every inbound OTA push.
Both now verify the resolved id belongs to the connection's / request's propertyId
before any write (private assertSamePropertyFk helper mirroring the reservation
service's pattern).
Tests: new setInventory cross-tenant denial; new inbound-reservation-fk-ownership
spec (2 cases: foreign roomTypeId and foreign ratePlanId via mapping). Updated 6
existing inbound-reservation specs + 2 allotment setInventory specs to account
for the new leading FK selects.
838/838 api specs pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…-audit Codex round-3 adversarial sweep of the whole codebase found 4 more places with the same caller-supplied-FK-without-same-property-check anti-pattern: 1. ConnectBookingService.modify — newRoomTypeId / newRatePlanId from the OTAIP agent could point at a foreign tenant. Now verified before re-calc / update. Also tightened the rate plan re-read to filter by propertyId (was bare id). 2. AllotmentService.updateBlock — dto.ratePlanId could redirect a block to a foreign tenant's plan. Now verified before update. 3. CashierService.recordMovement — dto.reservationId could attribute the cash movement (and downstream reporting) to a foreign tenant. Now verified. 4. ChannelService.create / update — operator-supplied roomTypeMapping and ratePlanMapping JSON could embed foreign-tenant ids. New assertMappingOwnership() bulk-verifies every id with an inArray lookup before insert/update. (Inbound-reservation also has a read-time guard from the prior commit — this is defense in depth so the row never lands in the DB.) Also closes the spec-coverage gap Codex flagged: new test asserts handleModification denies a foreign-tenant mapping (the code was already correct, just untested). 845/845 api specs pass. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…ent.createReview) Codex round-4 adversarial sweep found two more FK write paths: 1. RoomingListService.importRoomingList — caller-supplied entry.roomTypeId was inserted into rooming_list_entries without verifying same-property. Now the service checks each entry's roomTypeId; a foreign id is recorded as an 'error' row (matches the existing per-row error handling — does not abort the whole batch). 2. AgentController.createReview — caller-supplied dto.reservationId was inserted into guest_reviews without verifying same-property. Now rejects with BadRequestException. 847/847 api specs pass. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
The last FK ownership hole Codex found in round-5: RatePlanService.update() spreads dto into the update, and UpdateRatePlanDto omits propertyId/roomTypeId but NOT parentRatePlanId. A caller could re-parent a rate plan onto another tenant's chain. Now verifies dto.parentRatePlanId belongs to the request's propertyId before the update, mirroring the create() path's derived-parent check. Codex round-5 verdict on the rest: 'Beyond the rate-plan.update gap above, I did not find another remaining caller- or mapping-supplied FK insert/update in modules/* services that lacks same-property scoping.' This closes that gap. 848/848 api specs pass. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
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
1 participant
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.
What
Closes three HIGH findings from the independent Codex security audit (
~/Desktop/HAIP-codex-security-review.md). The DB schema FKs only constrain the referenced row's id, not its property — so a caller at property A could pass an FK id belonging to property B and the row would insert/update without complaint, leaking that row's details back via subsequent JOIN reads.Findings closed
reservation.service.ts—create()andmodify()now verifydto.roomTypeIdanddto.ratePlanIdbelong todto.propertyIdbefore any insert/update (shared privateassertSamePropertyFkhelper).room.service.ts—createRoom()verifiesdto.roomTypeIdis same-property before insert.housekeeping.service.ts—create()verifiesdto.roomIdis same-property before generating the checklist or inserting. (The innergenerateChecklistroom+reservation lookups were already scoped onmainfrom PR fix(security): deployment & input hardening (PR2) #117 — defense in depth.)Pattern is uniform:
select id from <fk_table> where id = AND property_id =→ if empty,BadRequestException('… not found in this property').Tests (TDD)
reservation-fk-ownership.spec.ts— 3 cases: foreignroomTypeIdon create, foreignratePlanIdon create, foreignratePlanIdon modify.room-fk-ownership.spec.ts— foreignroomTypeIddenied; same-property allowed.housekeeping-fk-ownership.spec.ts— foreignroomIddenied.housekeeping.service.spec.ts+housekeeping-dashboard.spec.tsmock sequences to account for the new leading FK select call.826/826 api specs pass.
Out of scope (next PRs)
This PR is cluster A of 4 follow-on audit clusters:
tax_rulesaddsproperty_id+ normalize "update by id alone" final writes (audit Charge Posting — Room Tariff, Reversals, Locking #9, Payment Module — Authorize/Capture/Void/Refund + Mock Gateway #10 residue)secretat rest, redact in list responses; strip guest PII fromaudit_logs.new_value+ SMTP logs (audit Implement Webhook module — EventEmitter + audit logging #7, Folio Module — CRUD + Auto-Folio Creation on Check-In #8)ParseUUIDPipe+ date validation (audit Settlement Flow + City Ledger Transfer + Folio Routing #11)🤖 Generated with Claude Code