Skip to content

Commit d301937

Browse files
fix: PR B — trust boundaries (round 4, bugs 5, 6, 8, 9) (#79)
Bug 5 (HIGH): WebSocket joinProperty unauthenticated - New WsAuthService (JWKS-backed, mirrors JwtStrategy) - Gateway verifies JWT on handleConnection, disconnects invalid sockets - joinProperty rejects unless user's JWT has the requested property_id (platform/admin roles bypass). AUTH_ENABLED=false preserves dev bypass. Bug 6 (HIGH): Guest PII cross-property leakage - All /guests/:id reads/updates/deletes + search now require propertyId - assertGuestAtProperty verifies ≥1 reservation at requesting property - Search uses inArray subquery scoped to reservations.propertyId - Create intentionally unscoped (walk-ins have no reservation yet) Bug 8 (HIGH): Stripe webhook signature verification broken - express.raw installed on /api/v1/webhooks/stripe before global JSON - Controller uses raw Buffer for stripe.webhooks.constructEvent Bug 9 (HIGH): JWT audience not validated - passport-jwt audience option enforced - azp claim also validated in validate() for Keycloak compatibility - KEYCLOAK_AUDIENCE documented in .env.example CLAUDE.md: guests exception corrected to require reservation-link check. 553/553 tests passing, build + typecheck clean. Co-authored-by: Dušan Milićević <dusanmilicevic33@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent b3e146f commit d301937

15 files changed

Lines changed: 439 additions & 123 deletions

.env.example

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,10 @@ AUTH_ENABLED=false
1919
KEYCLOAK_URL=http://localhost:8080
2020
KEYCLOAK_REALM=haip
2121
KEYCLOAK_CLIENT_ID=haip-api
22+
# KEYCLOAK_AUDIENCE — optional explicit override for the expected `aud` claim.
23+
# If unset, KEYCLOAK_CLIENT_ID is used. JWTs whose `aud`/`azp` does not match
24+
# this value are rejected (prevents cross-client token replay within the realm).
25+
# KEYCLOAK_AUDIENCE=haip-api
2226

2327
# Stripe Payment Gateway
2428
# STRIPE_MODE controls which gateway is used:

CLAUDE.md

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,12 @@ for methods called only internally, because future controllers may call them.
5050
lookups (that creates a confused-deputy bug — the attacker supplies the id,
5151
the server derives a matching propertyId, scoping becomes a no-op).
5252
- Exceptions — document the reason in a code comment when you deviate:
53-
- `guests` table: cross-property by design (one person stays at multiple hotels)
53+
- `guests` table: the ROW is cross-property by design (one person stays at
54+
multiple hotels), but API access MUST verify a reservation link at the
55+
requesting property — i.e. scope reads/updates/deletes by "has this guest
56+
at least one reservation at `propertyId`?". Creation is the only exception
57+
(walk-ins have no reservation yet); the linking reservation is created
58+
immediately after.
5459
- `properties` table: the property IS the tenant
5560
- Connect API (`/api/v1/connect/*`): bearer-credential model via
5661
`confirmationNumber` — scoped by credential possession, not propertyId

apps/api/package.json

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,12 +29,14 @@
2929
"@nestjs/serve-static": "^5.0.4",
3030
"@nestjs/swagger": "^7.4.0",
3131
"@nestjs/websockets": "^10.0.0",
32+
"@types/jsonwebtoken": "^9.0.10",
3233
"class-transformer": "^0.5.1",
3334
"class-validator": "^0.14.1",
3435
"decimal.js": "^10.4.3",
3536
"drizzle-orm": "^0.39.0",
3637
"eventemitter2": "^6.4.9",
3738
"fast-xml-parser": "^5.5.10",
39+
"jsonwebtoken": "^9.0.3",
3840
"jwks-rsa": "^4.0.1",
3941
"passport": "^0.7.0",
4042
"passport-jwt": "^4.0.1",

apps/api/src/main.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,26 @@
11
import { NestFactory } from '@nestjs/core';
22
import { ValidationPipe } from '@nestjs/common';
33
import { SwaggerModule, DocumentBuilder } from '@nestjs/swagger';
4+
// express is the underlying HTTP adapter used by NestJS's platform-express.
5+
// We don't declare @types/express directly; import the runtime bindings and
6+
// type them loosely here — these middlewares are only referenced from main.ts.
7+
// eslint-disable-next-line @typescript-eslint/no-var-requires
8+
const { json, raw } = require('express') as {
9+
json: (...args: any[]) => any;
10+
raw: (...args: any[]) => any;
11+
};
412
import { AppModule } from './app.module';
513

614
async function bootstrap() {
715
const app = await NestFactory.create(AppModule);
816

17+
// Stripe webhook signature verification requires the exact raw request body.
18+
// Install raw-body middleware for the webhook path BEFORE the global JSON
19+
// parser so req.body is a Buffer for that route; every other route still
20+
// receives parsed JSON.
21+
app.use('/api/v1/webhooks/stripe', raw({ type: 'application/json' }));
22+
app.use(json());
23+
924
// Global prefix for all routes
1025
app.setGlobalPrefix('api/v1');
1126

apps/api/src/modules/auth/auth.module.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { APP_GUARD } from '@nestjs/core';
55
import { JwtStrategy } from './jwt.strategy';
66
import { JwtAuthGuard } from './auth.guard';
77
import { RolesGuard } from './roles.guard';
8+
import { WsAuthService } from './ws-auth.service';
89

910
/**
1011
* Authentication & Authorization module.
@@ -46,6 +47,11 @@ import { RolesGuard } from './roles.guard';
4647
// Global guards — applied to ALL endpoints
4748
{ provide: APP_GUARD, useClass: JwtAuthGuard },
4849
{ provide: APP_GUARD, useClass: RolesGuard },
50+
// WebSocket token verifier — used by gateways that cannot rely on
51+
// passport HTTP guards (e.g. EventsGateway). Registered always; the
52+
// gateway itself honours AUTH_ENABLED=false as a dev bypass.
53+
WsAuthService,
4954
],
55+
exports: [WsAuthService],
5056
})
5157
export class AuthModule {}

apps/api/src/modules/auth/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,3 +5,4 @@ export { Public } from './public.decorator';
55
export { Roles } from './roles.decorator';
66
export { CurrentUser } from './current-user.decorator';
77
export type { AuthUser } from './current-user.decorator';
8+
export { WsAuthService } from './ws-auth.service';

apps/api/src/modules/auth/jwt.strategy.ts

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { Injectable } from '@nestjs/common';
1+
import { Injectable, UnauthorizedException } from '@nestjs/common';
22
import { ConfigService } from '@nestjs/config';
33
import { PassportStrategy } from '@nestjs/passport';
44
import { ExtractJwt, Strategy } from 'passport-jwt';
@@ -10,18 +10,30 @@ import type { AuthUser } from './current-user.decorator';
1010
*
1111
* Fetches Keycloak's public signing keys via JWKS endpoint.
1212
* Extracts user info and realm roles from the JWT claims.
13+
*
14+
* Audience enforcement:
15+
* - `aud` is validated against KEYCLOAK_CLIENT_ID (or KEYCLOAK_AUDIENCE if set).
16+
* - Keycloak also emits `azp` (authorized party) identifying the client that
17+
* obtained the token. We enforce `azp === expected` in validate() so tokens
18+
* from other realm clients cannot be replayed against this API.
1319
*/
1420
@Injectable()
1521
export class JwtStrategy extends PassportStrategy(Strategy, 'jwt') {
22+
private readonly expectedAudience: string;
23+
1624
constructor(configService: ConfigService) {
1725
const keycloakUrl = configService.get<string>('KEYCLOAK_URL', 'http://localhost:8080');
1826
const realm = configService.get<string>('KEYCLOAK_REALM', 'haip');
1927
const issuer = `${keycloakUrl}/realms/${realm}`;
28+
const audience =
29+
configService.get<string>('KEYCLOAK_AUDIENCE') ||
30+
configService.get<string>('KEYCLOAK_CLIENT_ID', 'haip-api');
2031

2132
super({
2233
jwtFromRequest: ExtractJwt.fromAuthHeaderAsBearerToken(),
2334
ignoreExpiration: false,
2435
issuer,
36+
audience,
2537
algorithms: ['RS256'],
2638
secretOrKeyProvider: passportJwtSecret({
2739
cache: true,
@@ -30,13 +42,20 @@ export class JwtStrategy extends PassportStrategy(Strategy, 'jwt') {
3042
jwksUri: `${issuer}/protocol/openid-connect/certs`,
3143
}),
3244
});
45+
46+
this.expectedAudience = audience;
3347
}
3448

3549
/**
36-
* Passport calls this after JWT signature is verified.
50+
* Passport calls this after JWT signature + issuer + audience are verified.
51+
* We additionally enforce `azp` (Keycloak's authorized-party claim) so that
52+
* access tokens minted for a different realm client cannot pass through.
3753
* Returns the user object attached to req.user.
3854
*/
3955
validate(payload: any): AuthUser {
56+
if (payload.azp && payload.azp !== this.expectedAudience) {
57+
throw new UnauthorizedException('Invalid token audience (azp mismatch)');
58+
}
4059
return {
4160
sub: payload.sub,
4261
email: payload.email ?? '',
Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
import { Injectable, Logger } from '@nestjs/common';
2+
import { ConfigService } from '@nestjs/config';
3+
import * as jwt from 'jsonwebtoken';
4+
import jwksClient, { JwksClient, SigningKey } from 'jwks-rsa';
5+
import type { AuthUser } from './current-user.decorator';
6+
7+
/**
8+
* Token verifier used by non-HTTP entrypoints (e.g. WebSocket gateway) that
9+
* cannot go through the passport HTTP strategy. Mirrors JwtStrategy's
10+
* validation rules: Keycloak issuer, audience (aud + azp), RS256 signature
11+
* fetched from JWKS.
12+
*/
13+
@Injectable()
14+
export class WsAuthService {
15+
private readonly logger = new Logger(WsAuthService.name);
16+
private readonly issuer: string;
17+
private readonly expectedAudience: string;
18+
private readonly jwks: JwksClient;
19+
20+
constructor(configService: ConfigService) {
21+
const keycloakUrl = configService.get<string>('KEYCLOAK_URL', 'http://localhost:8080');
22+
const realm = configService.get<string>('KEYCLOAK_REALM', 'haip');
23+
this.issuer = `${keycloakUrl}/realms/${realm}`;
24+
this.expectedAudience =
25+
configService.get<string>('KEYCLOAK_AUDIENCE') ||
26+
configService.get<string>('KEYCLOAK_CLIENT_ID', 'haip-api');
27+
this.jwks = jwksClient({
28+
jwksUri: `${this.issuer}/protocol/openid-connect/certs`,
29+
cache: true,
30+
rateLimit: true,
31+
jwksRequestsPerMinute: 5,
32+
});
33+
}
34+
35+
/**
36+
* Verify a bearer token and return the extracted AuthUser.
37+
* Throws on invalid signature, issuer, audience, or expiration.
38+
*/
39+
async verify(token: string): Promise<AuthUser> {
40+
const payload = await new Promise<any>((resolve, reject) => {
41+
jwt.verify(
42+
token,
43+
(header, cb) => {
44+
if (!header.kid) {
45+
cb(new Error('Token has no kid'));
46+
return;
47+
}
48+
this.jwks.getSigningKey(header.kid, (err: Error | null, key?: SigningKey) => {
49+
if (err || !key) {
50+
cb(err ?? new Error('Signing key not found'));
51+
return;
52+
}
53+
cb(null, key.getPublicKey());
54+
});
55+
},
56+
{
57+
algorithms: ['RS256'],
58+
issuer: this.issuer,
59+
audience: this.expectedAudience,
60+
},
61+
(err, decoded) => {
62+
if (err) reject(err);
63+
else resolve(decoded);
64+
},
65+
);
66+
});
67+
68+
if (payload.azp && payload.azp !== this.expectedAudience) {
69+
throw new Error('Invalid token audience (azp mismatch)');
70+
}
71+
72+
return {
73+
sub: payload.sub,
74+
email: payload.email ?? '',
75+
name: payload.name ?? payload.preferred_username ?? '',
76+
roles: payload.realm_access?.roles ?? [],
77+
propertyIds: payload.property_ids ?? undefined,
78+
};
79+
}
80+
}

apps/api/src/modules/events/events.gateway.ts

Lines changed: 93 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,24 @@ import {
88
MessageBody,
99
} from '@nestjs/websockets';
1010
import { Logger } from '@nestjs/common';
11+
import { ConfigService } from '@nestjs/config';
1112
import { Server, Socket } from 'socket.io';
13+
import { WsAuthService } from '../auth/ws-auth.service';
14+
import type { AuthUser } from '../auth/current-user.decorator';
1215

16+
/**
17+
* Events gateway — real-time PMS event broadcasts.
18+
*
19+
* Security model:
20+
* - On connect: verify a JWT passed via `handshake.auth.token` or `?token=`.
21+
* Invalid/missing tokens cause immediate disconnect.
22+
* - On joinProperty: the requested propertyId must appear in the user's
23+
* JWT `property_ids` custom claim (mapped onto AuthUser.propertyIds).
24+
* Admin/platform roles can join any property.
25+
* - Dev bypass: when AUTH_ENABLED=false (matching the HTTP JwtAuthGuard),
26+
* connections skip verification to keep local dev frictionless. Production
27+
* defaults to enforced.
28+
*/
1329
@WebSocketGateway({
1430
cors: {
1531
origin: ['http://localhost:5173', 'http://localhost:3000'],
@@ -18,12 +34,42 @@ import { Server, Socket } from 'socket.io';
1834
})
1935
export class EventsGateway implements OnGatewayConnection, OnGatewayDisconnect {
2036
private readonly logger = new Logger(EventsGateway.name);
37+
private readonly authEnabled: boolean;
2138

2239
@WebSocketServer()
2340
server!: Server;
2441

25-
handleConnection(client: Socket) {
26-
this.logger.log(`Client connected: ${client.id}`);
42+
constructor(
43+
private readonly wsAuth: WsAuthService,
44+
private readonly configService: ConfigService,
45+
) {
46+
this.authEnabled =
47+
this.configService.get<string>('AUTH_ENABLED', 'true') !== 'false';
48+
}
49+
50+
async handleConnection(client: Socket) {
51+
if (!this.authEnabled) {
52+
this.logger.log(`Client connected (auth disabled): ${client.id}`);
53+
return;
54+
}
55+
56+
const token = this.extractToken(client);
57+
if (!token) {
58+
this.logger.warn(`Rejecting WS ${client.id}: no token`);
59+
client.disconnect(true);
60+
return;
61+
}
62+
63+
try {
64+
const user = await this.wsAuth.verify(token);
65+
client.data.user = user;
66+
this.logger.log(`Client connected: ${client.id} (sub=${user.sub})`);
67+
} catch (err: any) {
68+
this.logger.warn(
69+
`Rejecting WS ${client.id}: token verification failed — ${err?.message ?? err}`,
70+
);
71+
client.disconnect(true);
72+
}
2773
}
2874

2975
handleDisconnect(client: Socket) {
@@ -36,6 +82,25 @@ export class EventsGateway implements OnGatewayConnection, OnGatewayDisconnect {
3682
@MessageBody() data: { propertyId: string },
3783
) {
3884
if (!data?.propertyId) return;
85+
86+
if (this.authEnabled) {
87+
const user = client.data.user as AuthUser | undefined;
88+
if (!user) {
89+
client.emit('error', { message: 'Not authenticated' });
90+
return;
91+
}
92+
if (!this.userCanAccessProperty(user, data.propertyId)) {
93+
this.logger.warn(
94+
`WS ${client.id} (sub=${user.sub}) denied joinProperty ${data.propertyId}`,
95+
);
96+
client.emit('error', {
97+
message: 'Forbidden: not a member of this property',
98+
propertyId: data.propertyId,
99+
});
100+
return;
101+
}
102+
}
103+
39104
const room = `property:${data.propertyId}`;
40105
client.join(room);
41106
this.logger.debug(`Client ${client.id} joined room ${room}`);
@@ -60,4 +125,30 @@ export class EventsGateway implements OnGatewayConnection, OnGatewayDisconnect {
60125
timestamp: new Date().toISOString(),
61126
});
62127
}
128+
129+
private extractToken(client: Socket): string | null {
130+
const authToken = (client.handshake.auth as any)?.token;
131+
if (typeof authToken === 'string' && authToken.length > 0) {
132+
return authToken.replace(/^Bearer\s+/i, '');
133+
}
134+
const queryToken = client.handshake.query?.['token'];
135+
if (typeof queryToken === 'string' && queryToken.length > 0) {
136+
return queryToken;
137+
}
138+
const headerAuth = client.handshake.headers?.authorization;
139+
if (typeof headerAuth === 'string' && headerAuth.length > 0) {
140+
return headerAuth.replace(/^Bearer\s+/i, '');
141+
}
142+
return null;
143+
}
144+
145+
private userCanAccessProperty(user: AuthUser, propertyId: string): boolean {
146+
// Platform-level roles that bypass property scoping (same intent as the
147+
// HTTP RolesGuard — admins can cross tenants for ops).
148+
const platformRoles = new Set(['admin', 'platform_admin', 'superadmin']);
149+
if (user.roles?.some((r) => platformRoles.has(r))) return true;
150+
151+
const allowed = user.propertyIds ?? [];
152+
return allowed.includes(propertyId);
153+
}
63154
}

apps/api/src/modules/events/events.module.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
11
import { Module } from '@nestjs/common';
2+
import { ConfigModule } from '@nestjs/config';
23
import { EventsGateway } from './events.gateway';
34
import { EventsService } from './events.service';
5+
import { AuthModule } from '../auth/auth.module';
46

57
@Module({
8+
imports: [ConfigModule, AuthModule],
69
providers: [EventsGateway, EventsService],
710
exports: [EventsGateway],
811
})

0 commit comments

Comments
 (0)