fix(auth): fail closed in PropertyScopeGuard + scope property reads

apps/api/src/modules/auth/property-scope.guard.spec.ts

@@ -5,55 +5,65 @@ 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: '[email protected]', name: 'U', roles: ['front_desk'], propertyIds: ['propA'] };
 const admin = { sub: 'a1', email: '[email protected]', name: 'A', roles: ['admin'], propertyIds: [] };
 
 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);
   });
 });

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<string>('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<boolean>(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<string>('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');
     }
 

apps/api/src/modules/property/property.controller.ts

@@ -6,30 +6,49 @@ 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';
 
 @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<string>('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);
   }