diff --git a/apps/api/src/modules/auth/property-scope.guard.spec.ts b/apps/api/src/modules/auth/property-scope.guard.spec.ts index 0312530..774508f 100644 --- a/apps/api/src/modules/auth/property-scope.guard.spec.ts +++ b/apps/api/src/modules/auth/property-scope.guard.spec.ts @@ -5,12 +5,17 @@ import { PropertyScopeGuard } from './property-scope.guard'; function ctx(request: any): ExecutionContext { return { switchToHttp: () => ({ getRequest: () => request }), + getHandler: () => undefined, + getClass: () => undefined, } as unknown as ExecutionContext; } -function guardWith(authEnabled: string) { +/** Build + invoke the guard with a given AUTH_ENABLED, request, and @Public flag. */ +function run(authEnabled: string, request: any, isPublic = false) { const config = { get: (_k: string, def?: string) => (authEnabled ?? def) } as any; - return new PropertyScopeGuard(config); + const reflector = { getAllAndOverride: () => isPublic } as any; + const guard = new PropertyScopeGuard(reflector, config); + return guard.canActivate(ctx(request)); } const member = { sub: 'u1', email: 'u@x.com', name: 'U', roles: ['front_desk'], propertyIds: ['propA'] }; @@ -18,42 +23,47 @@ const admin = { sub: 'a1', email: 'a@x.com', name: 'A', roles: ['admin'], proper describe('PropertyScopeGuard', () => { it('bypasses entirely when AUTH_ENABLED=false', () => { - const guard = guardWith('false'); - // foreign property, but auth off → allowed (demo) - expect(guard.canActivate(ctx({ user: member, query: { propertyId: 'propB' } }))).toBe(true); + expect(run('false', { user: member, query: { propertyId: 'propB' } })).toBe(true); }); it('allows a member to access their own property', () => { - const guard = guardWith('true'); - expect(guard.canActivate(ctx({ user: member, query: { propertyId: 'propA' } }))).toBe(true); + expect(run('true', { user: member, query: { propertyId: 'propA' } })).toBe(true); }); it('rejects a non-member accessing a foreign property', () => { - const guard = guardWith('true'); - expect(() => guard.canActivate(ctx({ user: member, query: { propertyId: 'propB' } }))).toThrow( + expect(() => run('true', { user: member, query: { propertyId: 'propB' } })).toThrow( ForbiddenException, ); }); it('reads propertyId from the body when not in the query', () => { - const guard = guardWith('true'); - expect(() => guard.canActivate(ctx({ user: member, body: { propertyId: 'propB' } }))).toThrow( + expect(() => run('true', { user: member, body: { propertyId: 'propB' } })).toThrow( ForbiddenException, ); }); it('lets platform admins cross properties', () => { - const guard = guardWith('true'); - expect(guard.canActivate(ctx({ user: admin, query: { propertyId: 'propZ' } }))).toBe(true); + expect(run('true', { user: admin, query: { propertyId: 'propZ' } })).toBe(true); }); it('skips routes that carry no propertyId', () => { - const guard = guardWith('true'); - expect(guard.canActivate(ctx({ user: member, query: {} }))).toBe(true); + expect(run('true', { user: member, query: {} })).toBe(true); }); - it('skips @Public routes that have no authenticated user', () => { - const guard = guardWith('true'); - expect(guard.canActivate(ctx({ query: { propertyId: 'propB' } }))).toBe(true); + it('skips @Public routes even when they carry a propertyId and no user', () => { + expect(run('true', { query: { propertyId: 'propB' } }, /* isPublic */ true)).toBe(true); + }); + + // --- fail-closed hardening (ported from PR #116) --- + + it('FAILS CLOSED on a duplicated/array propertyId (?propertyId=A&propertyId=B)', () => { + // member IS a member of propA, but the array form must not bypass the check. + expect(() => + run('true', { user: member, query: { propertyId: ['propA', 'propB'] } }), + ).toThrow(ForbiddenException); + }); + + it('FAILS CLOSED on a non-public route with a propertyId but no authenticated user', () => { + expect(() => run('true', { query: { propertyId: 'propA' } })).toThrow(ForbiddenException); }); }); diff --git a/apps/api/src/modules/auth/property-scope.guard.ts b/apps/api/src/modules/auth/property-scope.guard.ts index f7cc823..52a6f63 100644 --- a/apps/api/src/modules/auth/property-scope.guard.ts +++ b/apps/api/src/modules/auth/property-scope.guard.ts @@ -4,7 +4,9 @@ import { ExecutionContext, ForbiddenException, } from '@nestjs/common'; +import { Reflector } from '@nestjs/core'; import { ConfigService } from '@nestjs/config'; +import { IS_PUBLIC_KEY } from './public.decorator'; import type { AuthUser } from './current-user.decorator'; import { userCanAccessProperty } from './property-access'; @@ -20,38 +22,58 @@ import { userCanAccessProperty } from './property-access'; * already does for socket events: a non-platform user must have the property in * their `property_ids` claim. * - * - Bypassed entirely when `AUTH_ENABLED=false` (demo). - * - Skips `@Public()` routes (no principal to scope — e.g. inbound OTA webhooks). + * Fail-closed: when a non-public route carries a `propertyId`, the request MUST + * have an authenticated member and the value MUST be a single string. A missing + * user, or a non-scalar value (e.g. an array from a duplicated + * `?propertyId=A&propertyId=B`), is rejected — never silently allowed. + * + * - Bypassed when `AUTH_ENABLED=false` (demo) or on `@Public()` routes (which + * authenticate by their own mechanism — health, inbound OTA webhooks, connect). * - Skips routes that carry no `propertyId` (not property-scoped). * - Platform admins bypass (see `userCanAccessProperty`). */ @Injectable() export class PropertyScopeGuard implements CanActivate { - constructor(private readonly configService: ConfigService) {} + constructor( + private readonly reflector: Reflector, + private readonly configService: ConfigService, + ) {} canActivate(context: ExecutionContext): boolean { - const authEnabled = this.configService.get('AUTH_ENABLED', 'true'); - if (authEnabled === 'false') { + // @Public routes authenticate by their own mechanism and may legitimately + // carry a propertyId with no JWT user — don't fail them closed here. + const isPublic = this.reflector.getAllAndOverride(IS_PUBLIC_KEY, [ + context.getHandler(), + context.getClass(), + ]); + if (isPublic) { return true; } - const request = context.switchToHttp().getRequest(); - const user: AuthUser | undefined = request.user; - // Unauthenticated routes (e.g. @Public webhooks) carry no principal to scope; - // leave authentication of those to their own mechanism. - if (!user) { + const authEnabled = this.configService.get('AUTH_ENABLED', 'true'); + if (authEnabled === 'false') { return true; } - const propertyId: unknown = + const request = context.switchToHttp().getRequest(); + const raw: unknown = request.query?.propertyId ?? request.body?.propertyId ?? request.params?.propertyId; // Routes without a propertyId aren't property-scoped (e.g. global admin lists, // /properties). Nothing to enforce here. - if (!propertyId || typeof propertyId !== 'string') { + if (raw === undefined || raw === null) { return true; } - if (!userCanAccessProperty(user, propertyId)) { + const user: AuthUser | undefined = request.user; + if (!user) { + throw new ForbiddenException('No authenticated user'); + } + // A duplicated query param (`?propertyId=A&propertyId=B`) parses to an array; + // reject anything non-scalar rather than skipping the membership check. + if (typeof raw !== 'string') { + throw new ForbiddenException('Invalid propertyId'); + } + if (!userCanAccessProperty(user, raw)) { throw new ForbiddenException('You do not have access to this property'); } diff --git a/apps/api/src/modules/property/property.controller.ts b/apps/api/src/modules/property/property.controller.ts index 5cd49bc..f996197 100644 --- a/apps/api/src/modules/property/property.controller.ts +++ b/apps/api/src/modules/property/property.controller.ts @@ -6,9 +6,13 @@ import { Param, Body, ParseUUIDPipe, + ForbiddenException, } from '@nestjs/common'; import { ApiTags, ApiOperation, ApiResponse } from '@nestjs/swagger'; +import { ConfigService } from '@nestjs/config'; import { Roles } from '../auth/roles.decorator'; +import { CurrentUser, type AuthUser } from '../auth/current-user.decorator'; +import { userCanAccessProperty } from '../auth/property-access'; import { PropertyService } from './property.service'; import { CreatePropertyDto } from './dto/create-property.dto'; import { UpdatePropertyDto } from './dto/update-property.dto'; @@ -16,20 +20,35 @@ import { UpdatePropertyDto } from './dto/update-property.dto'; @ApiTags('properties') @Controller('properties') export class PropertyController { - constructor(private readonly propertyService: PropertyService) {} + constructor( + private readonly propertyService: PropertyService, + private readonly configService: ConfigService, + ) {} + + private authOn(): boolean { + return this.configService.get('AUTH_ENABLED', 'true') !== 'false'; + } @Get() @ApiOperation({ summary: 'Get all active properties' }) @ApiResponse({ status: 200, description: 'List of properties' }) - getAllProperties() { - return this.propertyService.findAll(); + async getAllProperties(@CurrentUser() user?: AuthUser) { + const all = await this.propertyService.findAll(); + // The `properties` row IS the tenant, so the global PropertyScopeGuard can't + // bind it (the id param is `:id`, not `propertyId`). Scope the list to the + // caller's memberships here; demo (auth off) returns everything. + if (!this.authOn() || !user) return all; + return (all as Array<{ id: string }>).filter((p) => userCanAccessProperty(user, p.id)); } @Get(':id') @ApiOperation({ summary: 'Get property by ID' }) @ApiResponse({ status: 200, description: 'Property found' }) @ApiResponse({ status: 404, description: 'Property not found' }) - getPropertyById(@Param('id', ParseUUIDPipe) id: string) { + getPropertyById(@Param('id', ParseUUIDPipe) id: string, @CurrentUser() user?: AuthUser) { + if (this.authOn() && user && !userCanAccessProperty(user, id)) { + throw new ForbiddenException('You do not have access to this property'); + } return this.propertyService.findById(id); }