Skip to content

Commit ad494d7

Browse files
elibosleyelibosley
authored
fix(oidc): trust Fastify proxy metadata (#1982)
## Summary - trust local proxy metadata through Fastify while keeping the OIDC authorize flow on one shared request-origin derivation path - tighten the request-origin contract so downstream OIDC code always receives a defined host and no longer re-derives origin data - harden the new regression coverage by relaxing brittle async exception assertions and making the authorize integration reply mock pass strict type-checking ## Problem A valid callback like `https://nas.domain.com/graphql/api/auth/oidc/callback` could fail behind the built-in proxy path even though it was allowed in Management Access. The authorize flow had duplicated origin derivation, one layer previously relied on raw forwarded headers, and the follow-up regression coverage still had a few reviewable rough edges around proxy trust clarity and test contracts. ## Fix Use Fastify's explicit `loopback` trust proxy alias, centralize OIDC request-origin extraction, and pass only normalized protocol/host information through the authorize flow. The request-origin utilities now guarantee a defined host, and the OIDC tests assert rejection behavior and reply interactions without coupling to exception classes or unsafe mock casts. ## Testing - `pnpm --filter ./api type-check` - `pnpm --filter ./api exec vitest run src/unraid-api/main.test.ts src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.test.ts src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.test.ts src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.spec.ts src/unraid-api/rest/rest.controller.oidc.integration.test.ts` - `pnpm --filter ./api exec vitest run src/unraid-api/main.test.ts src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.test.ts src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.test.ts src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.spec.ts src/unraid-api/graph/resolvers/sso/core/oidc.service.test.ts src/unraid-api/graph/resolvers/sso/core/oidc.service.integration.test.ts src/unraid-api/rest/rest.controller.test.ts src/unraid-api/rest/rest.controller.oidc.integration.test.ts` ## Deployment Note - The built-in `webgui` mount proxies `/graphql` to the API over `unix:/var/run/unraid-api.sock` and forwards `Host`, so these changes should not disrupt existing users on the standard nginx/socket path. - If a separate deployment relies on trusted `X-Forwarded-*` over a non-loopback TCP proxy hop, `trustProxy` may need to be widened to that explicit trusted network instead of loopback. ## Work Intent - Reference: `[UNRD]` from the reported issue context <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features & Improvements** * Centralized and improved derivation of request origin (protocol/host), with better handling of proxied/forwarded values and trusted request info. * Fastify proxy trust tightened to loopback for more accurate client/proxy behavior. * **Tests** * Added and updated unit/integration tests for OIDC/SSO flows: redirect-URI validation, authorization URL building, request-origin utilities, and proxied-request scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 160c5b1 commit ad494d7

14 files changed

Lines changed: 606 additions & 217 deletions

‎api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.test.ts‎

Lines changed: 45 additions & 107 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
import { UnauthorizedException } from '@nestjs/common';
21
import { Test, TestingModule } from '@nestjs/testing';
32

43
import { beforeEach, describe, expect, it, vi } from 'vitest';
@@ -7,14 +6,13 @@ import { OidcRedirectUriService } from '@app/unraid-api/graph/resolvers/sso/clie
76
import { OidcConfigPersistence } from '@app/unraid-api/graph/resolvers/sso/core/oidc-config.service.js';
87
import { validateRedirectUri } from '@app/unraid-api/utils/redirect-uri-validator.js';
98

10-
// Mock the redirect URI validator
119
vi.mock('@app/unraid-api/utils/redirect-uri-validator.js', () => ({
1210
validateRedirectUri: vi.fn(),
1311
}));
1412

1513
describe('OidcRedirectUriService', () => {
1614
let service: OidcRedirectUriService;
17-
let oidcConfig: any;
15+
let oidcConfig: { getConfig: ReturnType<typeof vi.fn> };
1816

1917
beforeEach(async () => {
2018
vi.clearAllMocks();
@@ -39,19 +37,16 @@ describe('OidcRedirectUriService', () => {
3937
});
4038

4139
describe('getRedirectUri', () => {
42-
it('should return valid redirect URI when validation passes', async () => {
43-
const requestOrigin = 'https://example.com';
44-
const requestHeaders = {
45-
'x-forwarded-proto': 'https',
46-
'x-forwarded-host': 'example.com',
47-
};
48-
40+
it('returns a callback URI when validation passes', async () => {
4941
(validateRedirectUri as any).mockReturnValue({
5042
isValid: true,
5143
validatedUri: 'https://example.com',
5244
});
5345

54-
const result = await service.getRedirectUri(requestOrigin, requestHeaders);
46+
const result = await service.getRedirectUri('https://example.com', {
47+
protocol: 'https',
48+
host: 'example.com',
49+
});
5550

5651
expect(result).toBe('https://example.com/graphql/api/auth/oidc/callback');
5752
expect(validateRedirectUri).toHaveBeenCalledWith(
@@ -63,41 +58,35 @@ describe('OidcRedirectUriService', () => {
6358
);
6459
});
6560

66-
it('should throw UnauthorizedException when validation fails', async () => {
67-
const requestOrigin = 'https://evil.com';
68-
const requestHeaders = {
69-
'x-forwarded-proto': 'https',
70-
'x-forwarded-host': 'example.com',
71-
};
72-
61+
it('throws when validation fails', async () => {
7362
(validateRedirectUri as any).mockReturnValue({
7463
isValid: false,
7564
reason: 'Origin not allowed',
7665
});
7766

78-
await expect(service.getRedirectUri(requestOrigin, requestHeaders)).rejects.toThrow(
79-
UnauthorizedException
80-
);
67+
await expect(
68+
service.getRedirectUri('https://evil.com', {
69+
protocol: 'https',
70+
host: 'example.com',
71+
})
72+
).rejects.toThrow();
8173
});
8274

83-
it('should handle missing allowed origins', async () => {
75+
it('passes through missing allowed origins', async () => {
8476
oidcConfig.getConfig.mockResolvedValue({
8577
providers: [],
8678
defaultAllowedOrigins: undefined,
8779
});
8880

89-
const requestOrigin = 'https://example.com';
90-
const requestHeaders = {
91-
'x-forwarded-proto': 'https',
92-
'x-forwarded-host': 'example.com',
93-
};
94-
9581
(validateRedirectUri as any).mockReturnValue({
9682
isValid: true,
9783
validatedUri: 'https://example.com',
9884
});
9985

100-
const result = await service.getRedirectUri(requestOrigin, requestHeaders);
86+
const result = await service.getRedirectUri('https://example.com', {
87+
protocol: 'https',
88+
host: 'example.com',
89+
});
10190

10291
expect(result).toBe('https://example.com/graphql/api/auth/oidc/callback');
10392
expect(validateRedirectUri).toHaveBeenCalledWith(
@@ -109,114 +98,63 @@ describe('OidcRedirectUriService', () => {
10998
);
11099
});
111100

112-
it('should extract protocol from headers correctly', async () => {
113-
const requestOrigin = 'https://example.com';
114-
const requestHeaders = {
115-
'x-forwarded-proto': ['https', 'http'],
116-
host: 'example.com',
117-
};
118-
101+
it('uses the trusted request origin info provided by Fastify', async () => {
119102
(validateRedirectUri as any).mockReturnValue({
120103
isValid: true,
121-
validatedUri: 'https://example.com',
104+
validatedUri: 'https://nas.domain.com/graphql/api/auth/oidc/callback',
122105
});
123106

124-
const result = await service.getRedirectUri(requestOrigin, requestHeaders);
125-
126-
expect(result).toBe('https://example.com/graphql/api/auth/oidc/callback');
127-
expect(validateRedirectUri).toHaveBeenCalledWith(
128-
'https://example.com',
129-
'https', // Should use first value from array
130-
'example.com',
131-
expect.anything(),
132-
expect.anything()
107+
const result = await service.getRedirectUri(
108+
'https://nas.domain.com/graphql/api/auth/oidc/callback',
109+
{
110+
protocol: 'https',
111+
host: 'nas.domain.com',
112+
}
133113
);
134-
});
135-
136-
it('should use host header as fallback', async () => {
137-
const requestOrigin = 'https://example.com';
138-
const requestHeaders = {
139-
host: 'example.com',
140-
};
141-
142-
(validateRedirectUri as any).mockReturnValue({
143-
isValid: true,
144-
validatedUri: 'https://example.com',
145-
});
146114

147-
const result = await service.getRedirectUri(requestOrigin, requestHeaders);
148-
149-
expect(result).toBe('https://example.com/graphql/api/auth/oidc/callback');
115+
expect(result).toBe('https://nas.domain.com/graphql/api/auth/oidc/callback');
150116
expect(validateRedirectUri).toHaveBeenCalledWith(
151-
'https://example.com',
152-
'https', // Inferred from requestOrigin when x-forwarded-proto not present
153-
'example.com',
117+
'https://nas.domain.com/graphql/api/auth/oidc/callback',
118+
'https',
119+
'nas.domain.com',
154120
expect.anything(),
155121
expect.anything()
156122
);
157123
});
158124

159-
it('should prefer x-forwarded-host over host header', async () => {
160-
const requestOrigin = 'https://example.com';
161-
const requestHeaders = {
162-
'x-forwarded-host': 'forwarded.example.com',
163-
host: 'original.example.com',
164-
};
165-
125+
it('allows host values with ports', async () => {
166126
(validateRedirectUri as any).mockReturnValue({
167127
isValid: true,
168128
validatedUri: 'https://example.com',
169129
});
170130

171-
const result = await service.getRedirectUri(requestOrigin, requestHeaders);
131+
const result = await service.getRedirectUri('https://example.com', {
132+
protocol: 'https',
133+
host: 'forwarded.example.com:8443',
134+
});
172135

173136
expect(result).toBe('https://example.com/graphql/api/auth/oidc/callback');
174137
expect(validateRedirectUri).toHaveBeenCalledWith(
175138
'https://example.com',
176-
'https', // Inferred from requestOrigin when x-forwarded-proto not present
177-
'forwarded.example.com', // Should use x-forwarded-host
139+
'https',
140+
'forwarded.example.com:8443',
178141
expect.anything(),
179142
expect.anything()
180143
);
181144
});
182145

183-
it('should throw when URL construction fails', async () => {
184-
const requestOrigin = 'https://example.com';
185-
const requestHeaders = {};
186-
187-
(validateRedirectUri as any).mockReturnValue({
188-
isValid: true,
189-
validatedUri: 'invalid-url', // Invalid URL
190-
});
191-
192-
await expect(service.getRedirectUri(requestOrigin, requestHeaders)).rejects.toThrow(
193-
UnauthorizedException
194-
);
195-
});
196-
197-
it('should handle array values in headers correctly', async () => {
198-
const requestOrigin = 'https://example.com';
199-
const requestHeaders = {
200-
'x-forwarded-proto': ['https'],
201-
'x-forwarded-host': ['forwarded.example.com', 'another.example.com'],
202-
host: ['original.example.com'],
203-
};
204-
146+
it('throws when URL construction fails after validation', async () => {
205147
(validateRedirectUri as any).mockReturnValue({
206148
isValid: true,
207-
validatedUri: 'https://example.com',
149+
validatedUri: 'invalid-url',
208150
});
209151

210-
const result = await service.getRedirectUri(requestOrigin, requestHeaders);
211-
212-
expect(result).toBe('https://example.com/graphql/api/auth/oidc/callback');
213-
expect(validateRedirectUri).toHaveBeenCalledWith(
214-
'https://example.com',
215-
'https',
216-
'forwarded.example.com', // Should use first value from array
217-
expect.anything(),
218-
expect.anything()
219-
);
152+
await expect(
153+
service.getRedirectUri('https://example.com', {
154+
protocol: 'https',
155+
host: 'example.com',
156+
})
157+
).rejects.toThrow();
220158
});
221159
});
222160
});

‎api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.ts‎

Lines changed: 3 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { Injectable, Logger, UnauthorizedException } from '@nestjs/common';
22

33
import { OidcConfigPersistence } from '@app/unraid-api/graph/resolvers/sso/core/oidc-config.service.js';
4+
import { RequestOriginInfo } from '@app/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.js';
45
import { validateRedirectUri } from '@app/unraid-api/utils/redirect-uri-validator.js';
56

67
@Injectable()
@@ -10,12 +11,9 @@ export class OidcRedirectUriService {
1011

1112
constructor(private readonly oidcConfig: OidcConfigPersistence) {}
1213

13-
async getRedirectUri(
14-
requestOrigin: string,
15-
requestHeaders: Record<string, string | string[] | undefined>
16-
): Promise<string> {
14+
async getRedirectUri(requestOrigin: string, requestOriginInfo: RequestOriginInfo): Promise<string> {
1715
// Extract protocol and host from headers for validation
18-
const { protocol, host } = this.getRequestOriginInfo(requestHeaders, requestOrigin);
16+
const { protocol, host } = requestOriginInfo;
1917

2018
// Get the global allowed origins from OIDC config
2119
const config = await this.oidcConfig.getConfig();
@@ -61,37 +59,4 @@ export class OidcRedirectUriService {
6159
throw new UnauthorizedException('Invalid redirect_uri');
6260
}
6361
}
64-
65-
private getRequestOriginInfo(
66-
requestHeaders: Record<string, string | string[] | undefined>,
67-
requestOrigin?: string
68-
): {
69-
protocol: string;
70-
host: string | undefined;
71-
} {
72-
// Extract protocol from x-forwarded-proto or infer from requestOrigin, default to http
73-
const forwardedProto = requestHeaders['x-forwarded-proto'];
74-
const protocol = forwardedProto
75-
? Array.isArray(forwardedProto)
76-
? forwardedProto[0]
77-
: forwardedProto
78-
: requestOrigin?.startsWith('https')
79-
? 'https'
80-
: 'http';
81-
82-
// Extract host from x-forwarded-host or host header
83-
const forwardedHost = requestHeaders['x-forwarded-host'];
84-
const hostHeader = requestHeaders['host'];
85-
const host = forwardedHost
86-
? Array.isArray(forwardedHost)
87-
? forwardedHost[0]
88-
: forwardedHost
89-
: hostHeader
90-
? Array.isArray(hostHeader)
91-
? hostHeader[0]
92-
: hostHeader
93-
: undefined;
94-
95-
return { protocol, host };
96-
}
9762
}

‎api/src/unraid-api/graph/resolvers/sso/core/oidc.service.integration.test.ts‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,10 @@ describe('OidcService Integration Tests - Enhanced Logging', () => {
248248
providerId: 'auth-url-test',
249249
state: 'test-state',
250250
requestOrigin: 'http://test.local',
251-
requestHeaders: { host: 'test.local' },
251+
requestOriginInfo: {
252+
protocol: 'http',
253+
host: 'test.local',
254+
},
252255
});
253256

254257
// Verify URL building logs
@@ -278,9 +281,9 @@ describe('OidcService Integration Tests - Enhanced Logging', () => {
278281
providerId: 'manual-endpoints',
279282
state: 'test-state',
280283
requestOrigin: 'http://test.local',
281-
requestHeaders: {
282-
'x-forwarded-host': 'test.local',
283-
'x-forwarded-proto': 'http',
284+
requestOriginInfo: {
285+
protocol: 'http',
286+
host: 'test.local',
284287
},
285288
});
286289

‎api/src/unraid-api/graph/resolvers/sso/core/oidc.service.test.ts‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -134,12 +134,16 @@ describe('OidcService Integration', () => {
134134
providerId: 'custom-provider',
135135
state: 'client-state-123',
136136
requestOrigin: 'https://example.com',
137-
requestHeaders: { host: 'example.com' },
137+
requestOriginInfo: {
138+
protocol: 'https',
139+
host: 'example.com',
140+
},
138141
};
139142

140143
const url = await service.getAuthorizationUrl(params);
141144

142145
expect(redirectUriService.getRedirectUri).toHaveBeenCalledWith('https://example.com', {
146+
protocol: 'https',
143147
host: 'example.com',
144148
});
145149

@@ -177,7 +181,10 @@ describe('OidcService Integration', () => {
177181
providerId: 'discovery-provider',
178182
state: 'client-state-123',
179183
requestOrigin: 'https://example.com',
180-
requestHeaders: {},
184+
requestOriginInfo: {
185+
protocol: 'https',
186+
host: 'example.com',
187+
},
181188
};
182189

183190
const url = await service.getAuthorizationUrl(params);
@@ -193,7 +200,10 @@ describe('OidcService Integration', () => {
193200
providerId: 'non-existent',
194201
state: 'state',
195202
requestOrigin: 'https://example.com',
196-
requestHeaders: {},
203+
requestOriginInfo: {
204+
protocol: 'https',
205+
host: 'example.com',
206+
},
197207
};
198208

199209
await expect(service.getAuthorizationUrl(params)).rejects.toThrow(UnauthorizedException);

0 commit comments

Comments
 (0)