fix(auth): tenant-bind Connect API + authenticate OTA inbound (CRITICAL #2, #3)#118
Merged
Conversation
#2, #3) Closes the two remaining CRITICAL findings from the independent Codex security audit (~/Desktop/HAIP-codex-security-review.md). 850/850 api specs pass. CRITICAL #2 — Connect API was scoped only by attacker-supplied propertyId: - New `connect_credentials` table (sha256-hashed, NEVER plaintext) binds a key to a single propertyId. Legacy `CONNECT_API_KEY` env stays valid as a 'platform' scope (cross-tenant by design — used by the demo gateway). - `ApiKeyGuard` now async + DB-aware: resolves the presented key to a `ConnectPrincipal = { scope: 'property' | 'platform', propertyId? }`. - New `ConnectScopeGuard` (mounted after ApiKeyGuard on ConnectController): for scope='property', any `params.id` / `query.propertyId` / `body.propertyId` must equal the credential's propertyId; `:confirmationNumber` routes are resolved through `bookings` and rejected if they belong to another tenant. Non-scalar propertyId rejected outright (matches PropertyScopeGuard hardening). - `GET /connect/properties` scope-filters for property credentials. CRITICAL #3 — OTA inbound webhooks were effectively unauthenticated: - Shared `verifyBasicAuth` / `verifyHmacSignature` util in `channel/adapters/inbound-auth.util.ts`. Constant-time compares. Fail closed on missing/malformed input. - Booking.com: Basic Auth verified against the RESOLVED connection's `config.inboundAuth` (so tenant A's creds can't inject for tenant B even if they know B's hotel code). Cancellations gated too. `sendErrorXml` now honors the actual code (was silently downgrading 401 to 500). - Expedia: HMAC-SHA256 of raw body verified via `x-expedia-signature` against the RESOLVED connection's shared secret. Routing-by-hotelId stays as merged. Demo parity: AUTH_ENABLED=false is a no-op in both new guards and both controllers, so the existing demo stack remains untouched. Deployment notes (in PR body): run the `0002_connect_security.sql` migration on Supabase; issue per-property Connect credentials to real subscribers (mint script TODO) — the demo gateway can keep using the platform CONNECT_API_KEY. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Independent Codex re-audit verdict on PR #118: substantive holes closed but six residual gaps. This commit closes them. #2 — ConnectScopeGuard: - Removed the over-aggressive 'params.id' check (was 403-ing on subscription routes where ':id' is a subscription UUID, not a propertyId). - Moved the /connect/properties/:id membership enforcement into the controller method where the route semantics are explicit. - Removed @optional() on DRIZZLE so the booking-by-confirmation lookup is truly fail-closed if DB is somehow not wired (throws 500 instead of allow). #3 — OTA inbound auth: - Constant-time compares now hash-then-compare with sha256 so the LENGTH of the stored secret never leaks via response timing (was: early-return on length mismatch in safeEqualStr and the explicit length check in verifyHmacSignature). - verifyBasicAuth no longer short-circuits the password check on username mismatch — both comparisons always run. - Booking.com reservation batch now returns clean 401 (not 500) when every failure was an auth failure. - Two new controller-level specs exercise the auth path with AUTH_ENABLED=true (Codex flagged that Expedia tests previously ran with auth off). 861/861 api specs pass. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…rch enumeration) Last residual cross-tenant hole flagged by the Codex re-audit: AgentSearchDto.propertyId is OPTIONAL, so a property-scoped credential could call POST /connect/search WITHOUT specifying propertyId and the search service would enumerate availability/rates/content across every active tenant. Fix: the controller now pins dto.propertyId to principal.propertyId for scope='property' callers, regardless of what (if anything) was sent. Platform-scope (the demo gateway and other trusted server-side callers) is unchanged — cross-tenant by design. New connect.controller.spec.ts exercises: - search pinning (omitted / present / platform scope) - /properties/:id membership (deny B from A, allow A from A, platform allows any) - /properties list (property scope sees only own tenant; platform lists all) 869/869 api specs pass. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…ot reauthorize Closes the last residual flagged by the Codex re-audit on PR #118 (#3 RESOLVED, #2 was PARTIAL on this one footgun): if a per-property credential was revoked but its RAW value also happened to match `CONNECT_API_KEY`, the previous ApiKeyGuard logic silently re-authorized it as scope='platform' via the env fallback. Real misconfiguration risk (operator revokes a credential that was mistakenly copy-pasted into the env). ApiKeyGuard now: - Looks up the credential WITHOUT pre-filtering by is_active=true. - If a row exists, that decides the outcome: - active && !revoked → scope='property' (as before) - inactive OR revoked → THROW 401 immediately, no fallthrough - Only when there is NO row at all do we consult the platform env key. Two new tests assert both terminal-deny cases (revoked + inactive) even when the raw value matches CONNECT_API_KEY. 870/870 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 the two remaining CRITICALs from the independent Codex security audit (#116 closed #1). 850/850 api specs pass; pre-existing media
Express.Multer+property.content_updated+ S3 typecheck errors are onmain(not from this diff).CRITICAL #2 — Connect API tenant-bound
connect_credentialstable (sha256 hash, never plaintext) binds each opaque API key to a singlepropertyId.CONNECT_API_KEYenv stays valid as ascope='platform'cross-tenant key (the demo gateway and other trusted server-side callers keep working).ApiKeyGuardis now async + DB-aware: resolves the key →ConnectPrincipalattached toreq.connect.ConnectScopeGuard(after ApiKeyGuard onConnectController): forscope='property', anyparams.id/query.propertyId/body.propertyIdmust equal the credential'spropertyId;/bookings/:confirmationNumberroutes resolve throughbookingsand reject if the booking belongs to another tenant. Non-scalar values rejected outright (matchesPropertyScopeGuardhardening).GET /connect/propertiesscope-filters to the one tenant for property credentials.CRITICAL #3 — OTA inbound webhooks authenticated
Routing-by-hotelId was already done correctly on
main(good). What was missing — caller authentication — is now closed:Authorization: Basicverified against the RESOLVED connection'sconfig.inboundAuth = { username, password }. Tenant A's creds can't inject reservations for tenant B even if the attacker knows B's hotel code. Cancellations gated identically. Also fixed:sendErrorXmlwas silently downgrading 401→500.x-expedia-signatureagainst the resolved connection's shared secret.channel/adapters/inbound-auth.util.ts(constant-time compares, fail-closed on missing/malformed input).Demo parity
AUTH_ENABLED=falseis a no-op in both new guards and both inbound controllers, so the existing demo stack is untouched.Tests (TDD)
api-key.guard.spec.ts— 7 tests (property/platform/revoked/unknown/fail-closed)connect-scope.guard.spec.ts— 13 tests incl. headline tenant-A-cred-denied-tenant-B-data + confirmationNumber-route cross-tenant denial + array-bypass fail-closedinbound-auth.util.spec.ts— 14 tests (Basic Auth + HMAC, tamper + wrong-secret + missing creds)Deployment notes
packages/database/src/migrations/0002_connect_security.sqlon the Supabase twin (and prod) before this lands.CONNECT_API_KEYas a platform-scope key — no change for the existing ChatGPT demo path).config.inboundAuth.🤖 Generated with Claude Code