fix(auth): fail closed in PropertyScopeGuard + scope property reads#120
Merged
Conversation
Ports the hardening from the parallel PR #116 onto the merged guard instead of merging that conflicting duplicate: - PropertyScopeGuard now fails CLOSED when a non-public, property-scoped route has a non-string propertyId (a duplicated ?propertyId=A&propertyId=B parses to an array — previously `typeof !== 'string' -> return true` skipped the membership check entirely) or no authenticated user. @public routes are skipped via Reflector so connect/webhook routes that carry a propertyId aren't broken. - property.controller GET list/detail are now membership-scoped: the row IS the tenant (param is `:id`, not `propertyId`), so the global guard can't bind it. Demo (AUTH off) returns everything. New guard specs prove the array-bypass -> 403 and missing-user -> 403. 822/822 api tests 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.
Ports the genuinely-better hardening from the parallel #116 onto the already-merged guard (from my #115), instead of merging #116 — which is a conflicted duplicate built on the pre-#115 base.
The bug this closes (was live in
main)The merged
PropertyScopeGuarddidif (!propertyId || typeof propertyId !== 'string') return true. A duplicated?propertyId=A&propertyId=Bparses to an array, sotypeof !== 'string'→ the membership check was skipped entirely (fail-open IDOR bypass). Same for a missingreq.user.Changes
property-scope.guard.ts— fails closed: non-string/arraypropertyId→ForbiddenException; missing user on a non-public scoped route →ForbiddenException. Skips@Publicroutes viaReflector(so connect/webhook routes carrying apropertyIdaren't broken). Bypass onAUTH_ENABLED=falseunchanged.property.controller.ts—GET /propertiesandGET /properties/:idare now membership-scoped. The row is the tenant (param is:id, notpropertyId), so the global guard can't bind it — this was the gap fix(auth): enforce property membership on HTTP routes (CRITICAL #1) #116 also caught. Demo (auth off) returns everything.Tests
New guard specs: array-bypass → 403, missing-user → 403, plus the existing cases. 822/822 api tests green, typecheck + lint clean.
Supersedes
Once this lands, #116 should be closed (its value is here, without the merge conflict).
🤖 Generated with Claude Code
Generated by Claude Code