From b7bfecb8cfcacf4cb9c9042a6d0cb485b1537019 Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Tue, 31 Mar 2026 16:36:38 -0400 Subject: [PATCH 1/5] fix(oidc): normalize forwarded redirect host headers - Purpose: make OIDC authorize redirect URI validation work when requests arrive through reverse proxies that send forwarded header chains. - Before: the REST authorize endpoint used raw x-forwarded-host and x-forwarded-proto header values, so comma-separated or array-style forwarded headers could be passed straight into URL validation. - Problem: proxied domain-based logins could fail with a misleading 'Invalid redirect_uri format' error even when the callback URI in Management Access was correct, while direct IP-based access still worked. - Change: trim forwarded header chains to the first hop before validation and preserve the endpoint's existing host/protocol fallback behavior for non-proxied localhost and IP flows. - Verification: added controller tests covering comma-separated and array-valued forwarded headers and ran the focused Vitest suites for the REST controller and redirect URI validator. --- .../unraid-api/rest/rest.controller.test.ts | 38 +++++++++++++++++++ api/src/unraid-api/rest/rest.controller.ts | 10 ++++- 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/api/src/unraid-api/rest/rest.controller.test.ts b/api/src/unraid-api/rest/rest.controller.test.ts index 2d1b0d3cfc..edb3998926 100644 --- a/api/src/unraid-api/rest/rest.controller.test.ts +++ b/api/src/unraid-api/rest/rest.controller.test.ts @@ -191,6 +191,44 @@ describe('RestController', () => { expect(OidcRequestHandler.handleAuthorize).toHaveBeenCalled(); }); + it('should handle comma-separated forwarded headers from reverse proxies', async () => { + const mockRequest = createMockRequest(undefined, { + 'x-forwarded-proto': 'https, http', + 'x-forwarded-host': 'nas.mydomain.com, 10.0.0.15', + host: '127.0.0.1:3001', + }); + + await controller.oidcAuthorize( + 'test-provider', + 'test-state', + 'https://nas.mydomain.com/graphql/api/auth/oidc/callback', + mockRequest, + mockReply as FastifyReply + ); + + expect(mockReply.status).toHaveBeenCalledWith(302); + expect(OidcRequestHandler.handleAuthorize).toHaveBeenCalled(); + }); + + it('should handle array-valued forwarded headers', async () => { + const mockRequest = createMockRequest(undefined, { + 'x-forwarded-proto': ['https', 'http'], + 'x-forwarded-host': ['nas.mydomain.com', '10.0.0.15'], + host: '127.0.0.1:3001', + }); + + await controller.oidcAuthorize( + 'test-provider', + 'test-state', + 'https://nas.mydomain.com/graphql/api/auth/oidc/callback', + mockRequest, + mockReply as FastifyReply + ); + + expect(mockReply.status).toHaveBeenCalledWith(302); + expect(OidcRequestHandler.handleAuthorize).toHaveBeenCalled(); + }); + it('should reject malformed redirect_uri', async () => { const mockRequest = createMockRequest('unraid.mytailnet.ts.net'); diff --git a/api/src/unraid-api/rest/rest.controller.ts b/api/src/unraid-api/rest/rest.controller.ts index 3edc414f31..75cdf9a32e 100644 --- a/api/src/unraid-api/rest/rest.controller.ts +++ b/api/src/unraid-api/rest/rest.controller.ts @@ -77,8 +77,14 @@ export class RestController { } // Security validation: validate redirect_uri with support for allowed origins - const protocol = (req.headers['x-forwarded-proto'] as string) || 'http'; - const host = (req.headers['x-forwarded-host'] as string) || req.headers.host || req.hostname; + const forwardedProto = String(req.headers['x-forwarded-proto'] || '') + .split(',')[0] + ?.trim(); + const forwardedHost = String(req.headers['x-forwarded-host'] || '') + .split(',')[0] + ?.trim(); + const protocol = forwardedProto || 'http'; + const host = forwardedHost || req.headers.host || req.hostname; // Get allowed origins from OIDC config const config = await this.oidcConfig.getConfig(); From 6c8cbb4b13ce537266e1e03cf2980f144a87cc32 Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Wed, 1 Apr 2026 12:19:41 -0400 Subject: [PATCH 2/5] fix(oidc): trust Fastify proxy metadata - Purpose: make OIDC authorize redirect validation rely on trusted Fastify request origin data instead of raw forwarded headers. - Before: the REST authorize path and redirect URI service could derive origin information differently, so proxied custom-domain requests could pass one validation layer and fail the next with a generic provider/configuration error. - Why: using raw x-forwarded-* values in a security decision is unsafe unless the proxy is trusted, and duplicating origin parsing across layers made the bug easy to reintroduce. - What: configure Fastify trustProxy for loopback proxies, centralize request-origin extraction in a shared helper, pass normalized origin info through the OIDC authorize flow, and add controller/service/bootstrap regression coverage. - How: RestController, OidcRequestHandler, OidcService, and OidcRedirectUriService now consume the same trusted protocol/host values from Fastify request metadata, with focused Vitest coverage for proxied domain flows and bootstrap configuration. --- .../client/oidc-redirect-uri.service.test.ts | 151 ++++---------- .../sso/client/oidc-redirect-uri.service.ts | 37 +--- .../sso/core/oidc.service.integration.test.ts | 11 +- .../resolvers/sso/core/oidc.service.test.ts | 16 +- .../graph/resolvers/sso/core/oidc.service.ts | 10 +- .../utils/oidc-request-handler.util.spec.ts | 42 ++-- .../sso/utils/oidc-request-handler.util.ts | 34 +-- .../utils/oidc-request-origin.util.test.ts | 53 +++++ .../sso/utils/oidc-request-origin.util.ts | 33 +++ api/src/unraid-api/main.test.ts | 105 ++++++++++ api/src/unraid-api/main.ts | 14 +- .../rest.controller.oidc.integration.test.ts | 194 ++++++++++++++++++ .../unraid-api/rest/rest.controller.test.ts | 37 +++- api/src/unraid-api/rest/rest.controller.ts | 13 +- 14 files changed, 530 insertions(+), 220 deletions(-) create mode 100644 api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.test.ts create mode 100644 api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.ts create mode 100644 api/src/unraid-api/main.test.ts create mode 100644 api/src/unraid-api/rest/rest.controller.oidc.integration.test.ts diff --git a/api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.test.ts b/api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.test.ts index 0bc0f08fae..72e0756f06 100644 --- a/api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.test.ts +++ b/api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.test.ts @@ -7,14 +7,13 @@ import { OidcRedirectUriService } from '@app/unraid-api/graph/resolvers/sso/clie import { OidcConfigPersistence } from '@app/unraid-api/graph/resolvers/sso/core/oidc-config.service.js'; import { validateRedirectUri } from '@app/unraid-api/utils/redirect-uri-validator.js'; -// Mock the redirect URI validator vi.mock('@app/unraid-api/utils/redirect-uri-validator.js', () => ({ validateRedirectUri: vi.fn(), })); describe('OidcRedirectUriService', () => { let service: OidcRedirectUriService; - let oidcConfig: any; + let oidcConfig: { getConfig: ReturnType }; beforeEach(async () => { vi.clearAllMocks(); @@ -39,19 +38,16 @@ describe('OidcRedirectUriService', () => { }); describe('getRedirectUri', () => { - it('should return valid redirect URI when validation passes', async () => { - const requestOrigin = 'https://example.com'; - const requestHeaders = { - 'x-forwarded-proto': 'https', - 'x-forwarded-host': 'example.com', - }; - + it('returns a callback URI when validation passes', async () => { (validateRedirectUri as any).mockReturnValue({ isValid: true, validatedUri: 'https://example.com', }); - const result = await service.getRedirectUri(requestOrigin, requestHeaders); + const result = await service.getRedirectUri('https://example.com', { + protocol: 'https', + host: 'example.com', + }); expect(result).toBe('https://example.com/graphql/api/auth/oidc/callback'); expect(validateRedirectUri).toHaveBeenCalledWith( @@ -63,41 +59,35 @@ describe('OidcRedirectUriService', () => { ); }); - it('should throw UnauthorizedException when validation fails', async () => { - const requestOrigin = 'https://evil.com'; - const requestHeaders = { - 'x-forwarded-proto': 'https', - 'x-forwarded-host': 'example.com', - }; - + it('throws when validation fails', async () => { (validateRedirectUri as any).mockReturnValue({ isValid: false, reason: 'Origin not allowed', }); - await expect(service.getRedirectUri(requestOrigin, requestHeaders)).rejects.toThrow( - UnauthorizedException - ); + await expect( + service.getRedirectUri('https://evil.com', { + protocol: 'https', + host: 'example.com', + }) + ).rejects.toThrow(UnauthorizedException); }); - it('should handle missing allowed origins', async () => { + it('passes through missing allowed origins', async () => { oidcConfig.getConfig.mockResolvedValue({ providers: [], defaultAllowedOrigins: undefined, }); - const requestOrigin = 'https://example.com'; - const requestHeaders = { - 'x-forwarded-proto': 'https', - 'x-forwarded-host': 'example.com', - }; - (validateRedirectUri as any).mockReturnValue({ isValid: true, validatedUri: 'https://example.com', }); - const result = await service.getRedirectUri(requestOrigin, requestHeaders); + const result = await service.getRedirectUri('https://example.com', { + protocol: 'https', + host: 'example.com', + }); expect(result).toBe('https://example.com/graphql/api/auth/oidc/callback'); expect(validateRedirectUri).toHaveBeenCalledWith( @@ -109,114 +99,63 @@ describe('OidcRedirectUriService', () => { ); }); - it('should extract protocol from headers correctly', async () => { - const requestOrigin = 'https://example.com'; - const requestHeaders = { - 'x-forwarded-proto': ['https', 'http'], - host: 'example.com', - }; - + it('uses the trusted request origin info provided by Fastify', async () => { (validateRedirectUri as any).mockReturnValue({ isValid: true, - validatedUri: 'https://example.com', + validatedUri: 'https://nas.domain.com/graphql/api/auth/oidc/callback', }); - const result = await service.getRedirectUri(requestOrigin, requestHeaders); - - expect(result).toBe('https://example.com/graphql/api/auth/oidc/callback'); - expect(validateRedirectUri).toHaveBeenCalledWith( - 'https://example.com', - 'https', // Should use first value from array - 'example.com', - expect.anything(), - expect.anything() + const result = await service.getRedirectUri( + 'https://nas.domain.com/graphql/api/auth/oidc/callback', + { + protocol: 'https', + host: 'nas.domain.com', + } ); - }); - - it('should use host header as fallback', async () => { - const requestOrigin = 'https://example.com'; - const requestHeaders = { - host: 'example.com', - }; - - (validateRedirectUri as any).mockReturnValue({ - isValid: true, - validatedUri: 'https://example.com', - }); - const result = await service.getRedirectUri(requestOrigin, requestHeaders); - - expect(result).toBe('https://example.com/graphql/api/auth/oidc/callback'); + expect(result).toBe('https://nas.domain.com/graphql/api/auth/oidc/callback'); expect(validateRedirectUri).toHaveBeenCalledWith( - 'https://example.com', - 'https', // Inferred from requestOrigin when x-forwarded-proto not present - 'example.com', + 'https://nas.domain.com/graphql/api/auth/oidc/callback', + 'https', + 'nas.domain.com', expect.anything(), expect.anything() ); }); - it('should prefer x-forwarded-host over host header', async () => { - const requestOrigin = 'https://example.com'; - const requestHeaders = { - 'x-forwarded-host': 'forwarded.example.com', - host: 'original.example.com', - }; - + it('allows host values with ports', async () => { (validateRedirectUri as any).mockReturnValue({ isValid: true, validatedUri: 'https://example.com', }); - const result = await service.getRedirectUri(requestOrigin, requestHeaders); + const result = await service.getRedirectUri('https://example.com', { + protocol: 'https', + host: 'forwarded.example.com:8443', + }); expect(result).toBe('https://example.com/graphql/api/auth/oidc/callback'); expect(validateRedirectUri).toHaveBeenCalledWith( 'https://example.com', - 'https', // Inferred from requestOrigin when x-forwarded-proto not present - 'forwarded.example.com', // Should use x-forwarded-host + 'https', + 'forwarded.example.com:8443', expect.anything(), expect.anything() ); }); - it('should throw when URL construction fails', async () => { - const requestOrigin = 'https://example.com'; - const requestHeaders = {}; - - (validateRedirectUri as any).mockReturnValue({ - isValid: true, - validatedUri: 'invalid-url', // Invalid URL - }); - - await expect(service.getRedirectUri(requestOrigin, requestHeaders)).rejects.toThrow( - UnauthorizedException - ); - }); - - it('should handle array values in headers correctly', async () => { - const requestOrigin = 'https://example.com'; - const requestHeaders = { - 'x-forwarded-proto': ['https'], - 'x-forwarded-host': ['forwarded.example.com', 'another.example.com'], - host: ['original.example.com'], - }; - + it('throws when URL construction fails after validation', async () => { (validateRedirectUri as any).mockReturnValue({ isValid: true, - validatedUri: 'https://example.com', + validatedUri: 'invalid-url', }); - const result = await service.getRedirectUri(requestOrigin, requestHeaders); - - expect(result).toBe('https://example.com/graphql/api/auth/oidc/callback'); - expect(validateRedirectUri).toHaveBeenCalledWith( - 'https://example.com', - 'https', - 'forwarded.example.com', // Should use first value from array - expect.anything(), - expect.anything() - ); + await expect( + service.getRedirectUri('https://example.com', { + protocol: 'https', + host: 'example.com', + }) + ).rejects.toThrow(UnauthorizedException); }); }); }); diff --git a/api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.ts b/api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.ts index 0ec41dc499..55448b85e6 100644 --- a/api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.ts +++ b/api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.ts @@ -1,6 +1,7 @@ import { Injectable, Logger, UnauthorizedException } from '@nestjs/common'; import { OidcConfigPersistence } from '@app/unraid-api/graph/resolvers/sso/core/oidc-config.service.js'; +import { RequestOriginInfo } from '@app/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.js'; import { validateRedirectUri } from '@app/unraid-api/utils/redirect-uri-validator.js'; @Injectable() @@ -12,10 +13,10 @@ export class OidcRedirectUriService { async getRedirectUri( requestOrigin: string, - requestHeaders: Record + requestOriginInfo: RequestOriginInfo ): Promise { // Extract protocol and host from headers for validation - const { protocol, host } = this.getRequestOriginInfo(requestHeaders, requestOrigin); + const { protocol, host } = requestOriginInfo; // Get the global allowed origins from OIDC config const config = await this.oidcConfig.getConfig(); @@ -62,36 +63,4 @@ export class OidcRedirectUriService { } } - private getRequestOriginInfo( - requestHeaders: Record, - requestOrigin?: string - ): { - protocol: string; - host: string | undefined; - } { - // Extract protocol from x-forwarded-proto or infer from requestOrigin, default to http - const forwardedProto = requestHeaders['x-forwarded-proto']; - const protocol = forwardedProto - ? Array.isArray(forwardedProto) - ? forwardedProto[0] - : forwardedProto - : requestOrigin?.startsWith('https') - ? 'https' - : 'http'; - - // Extract host from x-forwarded-host or host header - const forwardedHost = requestHeaders['x-forwarded-host']; - const hostHeader = requestHeaders['host']; - const host = forwardedHost - ? Array.isArray(forwardedHost) - ? forwardedHost[0] - : forwardedHost - : hostHeader - ? Array.isArray(hostHeader) - ? hostHeader[0] - : hostHeader - : undefined; - - return { protocol, host }; - } } diff --git a/api/src/unraid-api/graph/resolvers/sso/core/oidc.service.integration.test.ts b/api/src/unraid-api/graph/resolvers/sso/core/oidc.service.integration.test.ts index 53beb93c00..a867ddbd75 100644 --- a/api/src/unraid-api/graph/resolvers/sso/core/oidc.service.integration.test.ts +++ b/api/src/unraid-api/graph/resolvers/sso/core/oidc.service.integration.test.ts @@ -248,7 +248,10 @@ describe('OidcService Integration Tests - Enhanced Logging', () => { providerId: 'auth-url-test', state: 'test-state', requestOrigin: 'http://test.local', - requestHeaders: { host: 'test.local' }, + requestOriginInfo: { + protocol: 'http', + host: 'test.local', + }, }); // Verify URL building logs @@ -278,9 +281,9 @@ describe('OidcService Integration Tests - Enhanced Logging', () => { providerId: 'manual-endpoints', state: 'test-state', requestOrigin: 'http://test.local', - requestHeaders: { - 'x-forwarded-host': 'test.local', - 'x-forwarded-proto': 'http', + requestOriginInfo: { + protocol: 'http', + host: 'test.local', }, }); diff --git a/api/src/unraid-api/graph/resolvers/sso/core/oidc.service.test.ts b/api/src/unraid-api/graph/resolvers/sso/core/oidc.service.test.ts index 920c9bfaef..14ca9f1b78 100644 --- a/api/src/unraid-api/graph/resolvers/sso/core/oidc.service.test.ts +++ b/api/src/unraid-api/graph/resolvers/sso/core/oidc.service.test.ts @@ -134,12 +134,16 @@ describe('OidcService Integration', () => { providerId: 'custom-provider', state: 'client-state-123', requestOrigin: 'https://example.com', - requestHeaders: { host: 'example.com' }, + requestOriginInfo: { + protocol: 'https', + host: 'example.com', + }, }; const url = await service.getAuthorizationUrl(params); expect(redirectUriService.getRedirectUri).toHaveBeenCalledWith('https://example.com', { + protocol: 'https', host: 'example.com', }); @@ -177,7 +181,10 @@ describe('OidcService Integration', () => { providerId: 'discovery-provider', state: 'client-state-123', requestOrigin: 'https://example.com', - requestHeaders: {}, + requestOriginInfo: { + protocol: 'https', + host: 'example.com', + }, }; const url = await service.getAuthorizationUrl(params); @@ -193,7 +200,10 @@ describe('OidcService Integration', () => { providerId: 'non-existent', state: 'state', requestOrigin: 'https://example.com', - requestHeaders: {}, + requestOriginInfo: { + protocol: 'https', + host: 'example.com', + }, }; await expect(service.getAuthorizationUrl(params)).rejects.toThrow(UnauthorizedException); diff --git a/api/src/unraid-api/graph/resolvers/sso/core/oidc.service.ts b/api/src/unraid-api/graph/resolvers/sso/core/oidc.service.ts index aea581a6b8..ca1432cf6b 100644 --- a/api/src/unraid-api/graph/resolvers/sso/core/oidc.service.ts +++ b/api/src/unraid-api/graph/resolvers/sso/core/oidc.service.ts @@ -13,13 +13,14 @@ import { OidcProvider } from '@app/unraid-api/graph/resolvers/sso/models/oidc-pr import { OidcSessionService } from '@app/unraid-api/graph/resolvers/sso/session/oidc-session.service.js'; import { OidcStateExtractor } from '@app/unraid-api/graph/resolvers/sso/session/oidc-state-extractor.util.js'; import { OidcStateService } from '@app/unraid-api/graph/resolvers/sso/session/oidc-state.service.js'; +import { RequestOriginInfo } from '@app/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.js'; import { ErrorExtractor } from '@app/unraid-api/utils/error-extractor.util.js'; export interface GetAuthorizationUrlParams { providerId: string; state: string; requestOrigin: string; - requestHeaders: Record; + requestOriginInfo: RequestOriginInfo; } export interface HandleCallbackParams { @@ -48,7 +49,7 @@ export class OidcService { ) {} async getAuthorizationUrl(params: GetAuthorizationUrlParams): Promise { - const { providerId, state, requestOrigin, requestHeaders } = params; + const { providerId, state, requestOrigin, requestOriginInfo } = params; const provider = await this.oidcConfig.getProvider(providerId); if (!provider) { @@ -56,7 +57,10 @@ export class OidcService { } // Use requestOrigin with validation - const redirectUri = await this.redirectUriService.getRedirectUri(requestOrigin, requestHeaders); + const redirectUri = await this.redirectUriService.getRedirectUri( + requestOrigin, + requestOriginInfo + ); this.logger.debug(`Using redirect URI for authorization: ${redirectUri}`); this.logger.debug(`Request origin was: ${requestOrigin}`); diff --git a/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.spec.ts b/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.spec.ts index 54b370d761..4dbdd2c433 100644 --- a/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.spec.ts +++ b/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.spec.ts @@ -19,13 +19,12 @@ describe('OidcRequestHandler', () => { }); describe('extractRequestInfo', () => { - it('should extract request info from headers', () => { + it('should extract request info from trusted Fastify request values', () => { const mockReq = { - headers: { - 'x-forwarded-proto': 'https', - 'x-forwarded-host': 'example.com:8443', - }, - protocol: 'http', + headers: {}, + protocol: 'https', + host: 'example.com:8443', + hostname: 'example.com', url: '/callback?code=123&state=456', } as unknown as FastifyRequest; @@ -37,12 +36,11 @@ describe('OidcRequestHandler', () => { expect(result.baseUrl).toBe('https://example.com:8443'); }); - it('should fall back to request properties when headers are missing', () => { + it('should fall back to request hostname when host is missing', () => { const mockReq = { - headers: { - host: 'localhost:3000', - }, + headers: {}, protocol: 'http', + hostname: 'localhost:3000', url: '/callback?code=123&state=456', } as FastifyRequest; @@ -57,6 +55,7 @@ describe('OidcRequestHandler', () => { it('should use defaults when all headers are missing', () => { const mockReq = { headers: {}, + protocol: 'http', url: '/callback?code=123&state=456', } as FastifyRequest; @@ -151,7 +150,10 @@ describe('OidcRequestHandler', () => { }; const mockReq = { - headers: { 'x-forwarded-proto': 'https', 'x-forwarded-host': 'example.com' }, + headers: {}, + protocol: 'https', + host: 'example.com', + hostname: 'example.com', url: '/authorize', } as unknown as FastifyRequest; @@ -169,9 +171,9 @@ describe('OidcRequestHandler', () => { providerId: 'provider123', state: 'state456', requestOrigin: 'https://example.com/callback', - requestHeaders: { - 'x-forwarded-proto': 'https', - 'x-forwarded-host': 'example.com', + requestOriginInfo: { + protocol: 'https', + host: 'example.com', }, }); expect(mockLogger.debug).toHaveBeenCalledWith( @@ -194,9 +196,12 @@ describe('OidcRequestHandler', () => { handleCallback: vi.fn().mockResolvedValue('paddedToken123'), }; - const mockReq: Pick = { + const mockReq: Pick = { id: '123', - headers: { 'x-forwarded-proto': 'https', 'x-forwarded-host': 'example.com' }, + headers: {}, + protocol: 'https', + host: 'example.com', + hostname: 'example.com', url: '/callback?code=123&state=456', }; @@ -217,10 +222,7 @@ describe('OidcRequestHandler', () => { state: 'state456', requestOrigin: 'https://example.com', fullCallbackUrl: 'https://example.com/callback?code=123&state=456', - requestHeaders: { - 'x-forwarded-proto': 'https', - 'x-forwarded-host': 'example.com', - }, + requestHeaders: {}, }); expect(mockLogger.debug).toHaveBeenCalledWith('Callback request - Provider: provider123'); }); diff --git a/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.ts b/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.ts index 02d58b127e..29aa000158 100644 --- a/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.ts +++ b/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.ts @@ -2,15 +2,13 @@ import { Logger } from '@nestjs/common'; import type { FastifyRequest } from '@app/unraid-api/types/fastify.js'; import { OidcService } from '@app/unraid-api/graph/resolvers/sso/core/oidc.service.js'; +import { + extractRequestInfo, + extractRequestOriginInfo, + RequestInfo, +} from '@app/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.js'; import { OidcStateExtractor } from '@app/unraid-api/graph/resolvers/sso/session/oidc-state-extractor.util.js'; -export interface RequestInfo { - protocol: string; - host: string; - fullUrl: string; - baseUrl: string; -} - export interface OidcFlowResult { providerId: string; requestInfo: RequestInfo; @@ -29,25 +27,7 @@ export class OidcRequestHandler { * Extract request information from Fastify request headers */ static extractRequestInfo(req: FastifyRequest): RequestInfo { - // Handle potentially comma-separated forwarded headers (take first value) - const forwardedProto = String(req.headers['x-forwarded-proto'] || '') - .split(',')[0] - ?.trim(); - const forwardedHost = String(req.headers['x-forwarded-host'] || '') - .split(',')[0] - ?.trim(); - - const protocol = forwardedProto || req.protocol || 'http'; - const host = forwardedHost || req.headers.host || 'localhost:3000'; - const fullUrl = `${protocol}://${host}${req.url}`; - const baseUrl = `${protocol}://${host}`; - - return { - protocol, - host, - fullUrl, - baseUrl, - }; + return extractRequestInfo(req); } /** @@ -72,7 +52,7 @@ export class OidcRequestHandler { providerId, state, requestOrigin: redirectUri, - requestHeaders: req.headers as Record, + requestOriginInfo: extractRequestOriginInfo(req), }); logger.log(`Redirecting to OIDC provider: ${authUrl}`); diff --git a/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.test.ts b/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.test.ts new file mode 100644 index 0000000000..6a9ddd5e2a --- /dev/null +++ b/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, it } from 'vitest'; + +import type { FastifyRequest } from '@app/unraid-api/types/fastify.js'; +import { + extractRequestInfo, + extractRequestOriginInfo, +} from '@app/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.js'; + +describe('oidc-request-origin util', () => { + it('prefers Fastify protocol and host values', () => { + const req = { + protocol: 'https', + host: 'nas.domain.com', + hostname: 'nas.domain.com', + } as FastifyRequest; + + expect(extractRequestOriginInfo(req)).toEqual({ + protocol: 'https', + host: 'nas.domain.com', + }); + }); + + it('falls back to hostname and headers.host when needed', () => { + const req = { + headers: { + host: 'localhost:3000', + }, + hostname: 'localhost', + protocol: 'http', + } as FastifyRequest; + + expect(extractRequestOriginInfo(req)).toEqual({ + protocol: 'http', + host: 'localhost', + }); + }); + + it('extracts full request info with fallback values', () => { + const req = { + headers: {}, + hostname: 'localhost:3000', + protocol: 'http', + url: '/callback?code=123&state=456', + } as FastifyRequest; + + expect(extractRequestInfo(req)).toEqual({ + protocol: 'http', + host: 'localhost:3000', + fullUrl: 'http://localhost:3000/callback?code=123&state=456', + baseUrl: 'http://localhost:3000', + }); + }); +}); diff --git a/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.ts b/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.ts new file mode 100644 index 0000000000..02db62bea7 --- /dev/null +++ b/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.ts @@ -0,0 +1,33 @@ +import type { FastifyRequest } from '@app/unraid-api/types/fastify.js'; + +export interface RequestOriginInfo { + protocol: string; + host: string | undefined; +} + +export interface RequestInfo extends RequestOriginInfo { + fullUrl: string; + baseUrl: string; +} + +export function extractRequestOriginInfo(req: FastifyRequest): RequestOriginInfo { + return { + protocol: req.protocol || 'http', + host: req.host || req.hostname || req.headers.host || 'localhost:3000', + }; +} + +export function extractRequestInfo(req: FastifyRequest): RequestInfo { + const { protocol, host } = extractRequestOriginInfo(req); + + const resolvedHost = host || 'localhost:3000'; + const fullUrl = `${protocol}://${resolvedHost}${req.url}`; + const baseUrl = `${protocol}://${resolvedHost}`; + + return { + protocol, + host: resolvedHost, + fullUrl, + baseUrl, + }; +} diff --git a/api/src/unraid-api/main.test.ts b/api/src/unraid-api/main.test.ts new file mode 100644 index 0000000000..c11313c89c --- /dev/null +++ b/api/src/unraid-api/main.test.ts @@ -0,0 +1,105 @@ +import { ConfigService } from '@nestjs/config'; +import { EventEmitter2 } from '@nestjs/event-emitter'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const createMock = vi.fn(); +const fastifyAdapterConstructor = vi.fn().mockImplementation((options) => ({ + options, +})); + +vi.mock('@nestjs/core', () => ({ + NestFactory: { + create: createMock, + }, +})); + +vi.mock('@nestjs/platform-fastify/index.js', () => ({ + FastifyAdapter: fastifyAdapterConstructor, +})); + +vi.mock('@app/unraid-api/app/app.module.js', () => ({ + AppModule: class AppModule {}, +})); + +vi.mock('@app/unraid-api/exceptions/graphql-exceptions.filter.js', () => ({ + GraphQLExceptionsFilter: class GraphQLExceptionsFilter {}, +})); + +vi.mock('@app/unraid-api/exceptions/http-exceptions.filter.js', () => ({ + HttpExceptionFilter: class HttpExceptionFilter {}, +})); + +vi.mock('@app/core/log.js', () => ({ + apiLogger: { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + }, +})); + +vi.mock('@app/environment.js', () => ({ + LOG_LEVEL: 'INFO', + PORT: '3001', +})); + +describe('bootstrapNestServer', () => { + const originalProcessSend = process.send; + + beforeEach(() => { + vi.resetModules(); + vi.clearAllMocks(); + process.send = vi.fn().mockReturnValue(true) as typeof process.send; + }); + + afterEach(() => { + process.send = originalProcessSend; + }); + + it('creates the Fastify adapter with an explicit trustProxy policy', async () => { + const server = { + register: vi.fn().mockResolvedValue(undefined), + addHook: vi.fn(), + listen: vi.fn().mockResolvedValue('http://0.0.0.0:3001'), + }; + + const emitter = { + emit: vi.fn(), + }; + + const app = { + enableShutdownHooks: vi.fn(), + useGlobalPipes: vi.fn(), + getHttpAdapter: vi.fn().mockReturnValue({ + getInstance: vi.fn().mockReturnValue(server), + }), + enableCors: vi.fn(), + useLogger: vi.fn(), + useGlobalInterceptors: vi.fn(), + flushLogs: vi.fn(), + useGlobalFilters: vi.fn(), + init: vi.fn().mockResolvedValue(undefined), + get: vi.fn((token: unknown) => { + if (token === EventEmitter2) { + return emitter; + } + + if (token === ConfigService) { + return { + get: vi.fn(), + }; + } + + return {}; + }), + }; + + createMock.mockResolvedValue(app); + + const { bootstrapNestServer } = await import('@app/unraid-api/main.js'); + await bootstrapNestServer(); + + expect(fastifyAdapterConstructor).toHaveBeenCalledWith({ + trustProxy: '127.0.0.1/8,::1/128', + }); + }); +}); diff --git a/api/src/unraid-api/main.ts b/api/src/unraid-api/main.ts index ca87d9dde2..aef8ccff6a 100644 --- a/api/src/unraid-api/main.ts +++ b/api/src/unraid-api/main.ts @@ -63,10 +63,16 @@ export async function bootstrapNestServer(): Promise { import('@app/unraid-api/exceptions/http-exceptions.filter.js'), ]); - const app = await NestFactory.create(AppModule, new FastifyAdapter(), { - bufferLogs: false, - ...(LOG_LEVEL !== 'TRACE' ? { logger: false } : {}), - }); + const app = await NestFactory.create( + AppModule, + new FastifyAdapter({ + trustProxy: '127.0.0.1/8,::1/128', + }), + { + bufferLogs: false, + ...(LOG_LEVEL !== 'TRACE' ? { logger: false } : {}), + } + ); app.enableShutdownHooks(['SIGINT', 'SIGTERM', 'SIGQUIT']); // Enable validation globally diff --git a/api/src/unraid-api/rest/rest.controller.oidc.integration.test.ts b/api/src/unraid-api/rest/rest.controller.oidc.integration.test.ts new file mode 100644 index 0000000000..d30c7cc263 --- /dev/null +++ b/api/src/unraid-api/rest/rest.controller.oidc.integration.test.ts @@ -0,0 +1,194 @@ +import { Test, TestingModule } from '@nestjs/testing'; + +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import type { FastifyReply, FastifyRequest } from '@app/unraid-api/types/fastify.js'; +import { OidcAuthorizationService } from '@app/unraid-api/graph/resolvers/sso/auth/oidc-authorization.service.js'; +import { OidcClaimsService } from '@app/unraid-api/graph/resolvers/sso/auth/oidc-claims.service.js'; +import { OidcTokenExchangeService } from '@app/unraid-api/graph/resolvers/sso/auth/oidc-token-exchange.service.js'; +import { OidcClientConfigService } from '@app/unraid-api/graph/resolvers/sso/client/oidc-client-config.service.js'; +import { OidcRedirectUriService } from '@app/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.js'; +import { OidcConfigPersistence } from '@app/unraid-api/graph/resolvers/sso/core/oidc-config.service.js'; +import { OidcValidationService } from '@app/unraid-api/graph/resolvers/sso/core/oidc-validation.service.js'; +import { OidcService } from '@app/unraid-api/graph/resolvers/sso/core/oidc.service.js'; +import { + AuthorizationOperator, + OidcProvider, +} from '@app/unraid-api/graph/resolvers/sso/models/oidc-provider.model.js'; +import { OidcSessionService } from '@app/unraid-api/graph/resolvers/sso/session/oidc-session.service.js'; +import { OidcStateService } from '@app/unraid-api/graph/resolvers/sso/session/oidc-state.service.js'; +import { RestController } from '@app/unraid-api/rest/rest.controller.js'; +import { RestService } from '@app/unraid-api/rest/rest.service.js'; + +describe('RestController OIDC authorize integration', () => { + let controller: RestController; + let oidcConfig: { + getConfig: ReturnType; + getProvider: ReturnType; + }; + let oidcStateService: { + generateSecureState: ReturnType; + }; + let mockReply: Partial; + + const provider: OidcProvider = { + id: 'test-provider', + name: 'Test Provider', + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + authorizationEndpoint: 'https://provider.example.com/oauth/authorize', + tokenEndpoint: 'https://provider.example.com/oauth/token', + scopes: ['openid', 'profile'], + authorizationRules: [ + { + claim: 'email', + operator: AuthorizationOperator.EQUALS, + value: ['user@example.com'], + }, + ], + }; + + const createMockRequest = ( + hostname?: string, + headers: Record = {}, + options: { + host?: string; + protocol?: string; + } = {} + ): FastifyRequest => + ({ + headers, + hostname, + host: options.host ?? hostname ?? headers.host, + url: '/graphql/api/auth/oidc/authorize/test-provider', + protocol: options.protocol ?? 'https', + }) as FastifyRequest; + + beforeEach(async () => { + vi.restoreAllMocks(); + + oidcConfig = { + getConfig: vi.fn().mockResolvedValue({ + providers: [provider], + defaultAllowedOrigins: [], + }), + getProvider: vi.fn().mockResolvedValue(provider), + }; + + oidcStateService = { + generateSecureState: vi.fn().mockResolvedValue('test-provider:signed-state'), + }; + + const module: TestingModule = await Test.createTestingModule({ + controllers: [RestController], + providers: [ + OidcService, + OidcRedirectUriService, + { + provide: RestService, + useValue: { + getCustomizationStream: vi.fn(), + }, + }, + { + provide: OidcConfigPersistence, + useValue: oidcConfig, + }, + { + provide: OidcSessionService, + useValue: { + createSession: vi.fn(), + }, + }, + { + provide: OidcStateService, + useValue: oidcStateService, + }, + { + provide: OidcValidationService, + useValue: { + validateProvider: vi.fn(), + performDiscovery: vi.fn(), + }, + }, + { + provide: OidcAuthorizationService, + useValue: { + checkAuthorization: vi.fn(), + }, + }, + { + provide: OidcClientConfigService, + useValue: { + getOrCreateConfig: vi.fn(), + clearCache: vi.fn(), + }, + }, + { + provide: OidcTokenExchangeService, + useValue: { + exchangeCodeForTokens: vi.fn(), + }, + }, + { + provide: OidcClaimsService, + useValue: { + parseIdToken: vi.fn(), + validateClaims: vi.fn(), + }, + }, + ], + }).compile(); + + controller = module.get(RestController); + + mockReply = { + status: vi.fn().mockReturnThis(), + header: vi.fn().mockReturnThis(), + send: vi.fn().mockReturnThis(), + }; + }); + + it('authorizes successfully through both redirect validation layers for proxied domains', async () => { + const mockRequest = createMockRequest(undefined, { + 'x-forwarded-proto': 'https, http', + 'x-forwarded-host': 'nas.domain.com, 10.0.0.15', + host: '127.0.0.1:3001', + }, { + host: 'nas.domain.com', + protocol: 'https', + }); + + await controller.oidcAuthorize( + provider.id, + 'client-state-123', + 'https://nas.domain.com/graphql/api/auth/oidc/callback', + mockRequest, + mockReply as FastifyReply + ); + + expect(oidcStateService.generateSecureState).toHaveBeenCalledWith( + provider.id, + 'client-state-123', + 'https://nas.domain.com/graphql/api/auth/oidc/callback' + ); + + expect(mockReply.status).toHaveBeenCalledWith(302); + expect(mockReply.header).toHaveBeenCalledWith( + 'Location', + expect.stringContaining('https://provider.example.com/oauth/authorize') + ); + + const locationHeader = vi + .mocked(mockReply.header) + .mock.calls.find(([name]) => name === 'Location')?.[1]; + + expect(locationHeader).toBeTypeOf('string'); + + const locationUrl = new URL(locationHeader as string); + expect(locationUrl.searchParams.get('redirect_uri')).toBe( + 'https://nas.domain.com/graphql/api/auth/oidc/callback' + ); + expect(locationUrl.searchParams.get('state')).toBe('test-provider:signed-state'); + }); +}); diff --git a/api/src/unraid-api/rest/rest.controller.test.ts b/api/src/unraid-api/rest/rest.controller.test.ts index edb3998926..a7efe610a0 100644 --- a/api/src/unraid-api/rest/rest.controller.test.ts +++ b/api/src/unraid-api/rest/rest.controller.test.ts @@ -17,13 +17,20 @@ describe('RestController', () => { let oidcConfig: OidcConfigPersistence; let mockReply: Partial; - // Helper function to create a mock request with the desired hostname - const createMockRequest = (hostname?: string, headers: Record = {}): FastifyRequest => { + const createMockRequest = ( + hostname?: string, + headers: Record = {}, + options: { + host?: string; + protocol?: string; + } = {} + ): FastifyRequest => { return { headers, hostname, + host: options.host ?? hostname ?? headers.host, url: '/test', - protocol: 'https', + protocol: options.protocol ?? 'https', } as FastifyRequest; }; @@ -191,11 +198,14 @@ describe('RestController', () => { expect(OidcRequestHandler.handleAuthorize).toHaveBeenCalled(); }); - it('should handle comma-separated forwarded headers from reverse proxies', async () => { + it('should handle proxied requests using trusted Fastify request values', async () => { const mockRequest = createMockRequest(undefined, { 'x-forwarded-proto': 'https, http', 'x-forwarded-host': 'nas.mydomain.com, 10.0.0.15', host: '127.0.0.1:3001', + }, { + host: 'nas.mydomain.com', + protocol: 'https', }); await controller.oidcAuthorize( @@ -210,11 +220,14 @@ describe('RestController', () => { expect(OidcRequestHandler.handleAuthorize).toHaveBeenCalled(); }); - it('should handle array-valued forwarded headers', async () => { + it('should handle trusted request values with explicit proxy chains present', async () => { const mockRequest = createMockRequest(undefined, { 'x-forwarded-proto': ['https', 'http'], 'x-forwarded-host': ['nas.mydomain.com', '10.0.0.15'], host: '127.0.0.1:3001', + }, { + host: 'nas.mydomain.com', + protocol: 'https', }); await controller.oidcAuthorize( @@ -287,7 +300,7 @@ describe('RestController', () => { }); it('should allow localhost with different ports', async () => { - const mockRequest = createMockRequest('localhost'); + const mockRequest = createMockRequest('localhost', {}, { protocol: 'http' }); await controller.oidcAuthorize( 'test-provider', @@ -309,7 +322,7 @@ describe('RestController', () => { }); it('should allow IP addresses with different ports', async () => { - const mockRequest = createMockRequest('192.168.1.100'); + const mockRequest = createMockRequest('192.168.1.100', {}, { protocol: 'http' }); await controller.oidcAuthorize( 'test-provider', @@ -382,12 +395,14 @@ describe('RestController', () => { shouldSucceed: true, }, { - name: 'respects protocol and hostname from headers', + name: 'respects protocol and hostname from trusted request values', requestHost: undefined, headers: { 'x-forwarded-proto': 'https', 'x-forwarded-host': 'proxy.local', }, + trustedHost: 'proxy.local', + trustedProtocol: 'https', redirectUri: 'https://proxy.local/graphql/api/auth/oidc/callback', allowedOrigins: [], expectedStatus: 302, @@ -401,7 +416,11 @@ describe('RestController', () => { const mockRequest = createMockRequest( testCase.requestHost, - testCase.headers || {} + testCase.headers || {}, + { + host: testCase.trustedHost, + protocol: testCase.trustedProtocol, + } ); vi.mocked(oidcConfig.getConfig).mockResolvedValueOnce({ diff --git a/api/src/unraid-api/rest/rest.controller.ts b/api/src/unraid-api/rest/rest.controller.ts index 75cdf9a32e..5a662bb6b8 100644 --- a/api/src/unraid-api/rest/rest.controller.ts +++ b/api/src/unraid-api/rest/rest.controller.ts @@ -77,14 +77,7 @@ export class RestController { } // Security validation: validate redirect_uri with support for allowed origins - const forwardedProto = String(req.headers['x-forwarded-proto'] || '') - .split(',')[0] - ?.trim(); - const forwardedHost = String(req.headers['x-forwarded-host'] || '') - .split(',')[0] - ?.trim(); - const protocol = forwardedProto || 'http'; - const host = forwardedHost || req.headers.host || req.hostname; + const requestInfo = OidcRequestHandler.extractRequestInfo(req); // Get allowed origins from OIDC config const config = await this.oidcConfig.getConfig(); @@ -93,8 +86,8 @@ export class RestController { // Validate the redirect URI using the centralized validator const validation = validateRedirectUri( params.redirectUri, - protocol, - host, + requestInfo.protocol, + requestInfo.host, this.oidcLogger, allowedOrigins ); From 12918ed0c0d8e07ada7ad9742957a7b33bde7830 Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Wed, 1 Apr 2026 12:23:32 -0400 Subject: [PATCH 3/5] chore(format): run prettier on OIDC proxy files - Purpose: align the recent OIDC proxy-trust changes with the formatter output used by the API package. - Before: several touched files still had non-Prettier wrapping and import ordering from the manual edits. - Why: leaving formatting drift in the PR adds noise to review and makes later diffs less clear. - What: ran Prettier on the changed OIDC files and committed the resulting formatting-only updates. - How: applied package-local Prettier output to the touched tests and utilities without changing runtime behavior. --- .../sso/client/oidc-redirect-uri.service.ts | 6 +-- .../utils/oidc-request-handler.util.spec.ts | 5 ++- .../sso/utils/oidc-request-handler.util.ts | 2 +- api/src/unraid-api/main.test.ts | 1 + .../rest.controller.oidc.integration.test.ts | 20 ++++++---- .../unraid-api/rest/rest.controller.test.ts | 40 +++++++++++-------- 6 files changed, 43 insertions(+), 31 deletions(-) diff --git a/api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.ts b/api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.ts index 55448b85e6..4502097219 100644 --- a/api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.ts +++ b/api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.ts @@ -11,10 +11,7 @@ export class OidcRedirectUriService { constructor(private readonly oidcConfig: OidcConfigPersistence) {} - async getRedirectUri( - requestOrigin: string, - requestOriginInfo: RequestOriginInfo - ): Promise { + async getRedirectUri(requestOrigin: string, requestOriginInfo: RequestOriginInfo): Promise { // Extract protocol and host from headers for validation const { protocol, host } = requestOriginInfo; @@ -62,5 +59,4 @@ export class OidcRedirectUriService { throw new UnauthorizedException('Invalid redirect_uri'); } } - } diff --git a/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.spec.ts b/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.spec.ts index 4dbdd2c433..5bd5367de5 100644 --- a/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.spec.ts +++ b/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.spec.ts @@ -196,7 +196,10 @@ describe('OidcRequestHandler', () => { handleCallback: vi.fn().mockResolvedValue('paddedToken123'), }; - const mockReq: Pick = { + const mockReq: Pick< + FastifyRequest, + 'id' | 'headers' | 'url' | 'protocol' | 'host' | 'hostname' + > = { id: '123', headers: {}, protocol: 'https', diff --git a/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.ts b/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.ts index 29aa000158..b7dafc129f 100644 --- a/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.ts +++ b/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.ts @@ -2,12 +2,12 @@ import { Logger } from '@nestjs/common'; import type { FastifyRequest } from '@app/unraid-api/types/fastify.js'; import { OidcService } from '@app/unraid-api/graph/resolvers/sso/core/oidc.service.js'; +import { OidcStateExtractor } from '@app/unraid-api/graph/resolvers/sso/session/oidc-state-extractor.util.js'; import { extractRequestInfo, extractRequestOriginInfo, RequestInfo, } from '@app/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.js'; -import { OidcStateExtractor } from '@app/unraid-api/graph/resolvers/sso/session/oidc-state-extractor.util.js'; export interface OidcFlowResult { providerId: string; diff --git a/api/src/unraid-api/main.test.ts b/api/src/unraid-api/main.test.ts index c11313c89c..c1c04a2896 100644 --- a/api/src/unraid-api/main.test.ts +++ b/api/src/unraid-api/main.test.ts @@ -1,5 +1,6 @@ import { ConfigService } from '@nestjs/config'; import { EventEmitter2 } from '@nestjs/event-emitter'; + import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; const createMock = vi.fn(); diff --git a/api/src/unraid-api/rest/rest.controller.oidc.integration.test.ts b/api/src/unraid-api/rest/rest.controller.oidc.integration.test.ts index d30c7cc263..bf0af82358 100644 --- a/api/src/unraid-api/rest/rest.controller.oidc.integration.test.ts +++ b/api/src/unraid-api/rest/rest.controller.oidc.integration.test.ts @@ -150,14 +150,18 @@ describe('RestController OIDC authorize integration', () => { }); it('authorizes successfully through both redirect validation layers for proxied domains', async () => { - const mockRequest = createMockRequest(undefined, { - 'x-forwarded-proto': 'https, http', - 'x-forwarded-host': 'nas.domain.com, 10.0.0.15', - host: '127.0.0.1:3001', - }, { - host: 'nas.domain.com', - protocol: 'https', - }); + const mockRequest = createMockRequest( + undefined, + { + 'x-forwarded-proto': 'https, http', + 'x-forwarded-host': 'nas.domain.com, 10.0.0.15', + host: '127.0.0.1:3001', + }, + { + host: 'nas.domain.com', + protocol: 'https', + } + ); await controller.oidcAuthorize( provider.id, diff --git a/api/src/unraid-api/rest/rest.controller.test.ts b/api/src/unraid-api/rest/rest.controller.test.ts index a7efe610a0..63b6cd1a68 100644 --- a/api/src/unraid-api/rest/rest.controller.test.ts +++ b/api/src/unraid-api/rest/rest.controller.test.ts @@ -199,14 +199,18 @@ describe('RestController', () => { }); it('should handle proxied requests using trusted Fastify request values', async () => { - const mockRequest = createMockRequest(undefined, { - 'x-forwarded-proto': 'https, http', - 'x-forwarded-host': 'nas.mydomain.com, 10.0.0.15', - host: '127.0.0.1:3001', - }, { - host: 'nas.mydomain.com', - protocol: 'https', - }); + const mockRequest = createMockRequest( + undefined, + { + 'x-forwarded-proto': 'https, http', + 'x-forwarded-host': 'nas.mydomain.com, 10.0.0.15', + host: '127.0.0.1:3001', + }, + { + host: 'nas.mydomain.com', + protocol: 'https', + } + ); await controller.oidcAuthorize( 'test-provider', @@ -221,14 +225,18 @@ describe('RestController', () => { }); it('should handle trusted request values with explicit proxy chains present', async () => { - const mockRequest = createMockRequest(undefined, { - 'x-forwarded-proto': ['https', 'http'], - 'x-forwarded-host': ['nas.mydomain.com', '10.0.0.15'], - host: '127.0.0.1:3001', - }, { - host: 'nas.mydomain.com', - protocol: 'https', - }); + const mockRequest = createMockRequest( + undefined, + { + 'x-forwarded-proto': ['https', 'http'], + 'x-forwarded-host': ['nas.mydomain.com', '10.0.0.15'], + host: '127.0.0.1:3001', + }, + { + host: 'nas.mydomain.com', + protocol: 'https', + } + ); await controller.oidcAuthorize( 'test-provider', From 30390a4122b29d4862e53dfeab10222b8757a127 Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Wed, 1 Apr 2026 12:42:39 -0400 Subject: [PATCH 4/5] test(oidc): fix strict typing in authorize integration test - Purpose: unblock the failing API CI type-check after adding the proxied-domain integration coverage. - Before: the new integration test read the mocked Location header through an optional chained lookup that Vitest handled at runtime but strict TypeScript rejected in CI. - Why: the API workflow runs Version 4.8.4 tsc: The TypeScript Compiler - Version 4.8.4 COMMON COMMANDS tsc Compiles the current project (tsconfig.json in the working directory.) tsc app.ts util.ts Ignoring tsconfig.json, compiles the specified files with default compiler options. tsc -b Build a composite project in the working directory. tsc --init Creates a tsconfig.json with the recommended settings in the working directory. tsc -p ./path/to/tsconfig.json Compiles the TypeScript project located at the specified path. tsc --help --all An expanded version of this information, showing all possible compiler options tsc --noEmit tsc --target esnext Compiles the current project, with additional settings. COMMAND LINE FLAGS --help, -h Print this message. --watch, -w Watch input files. --all Show all compiler options. --version, -v Print the compiler's version. --init Initializes a TypeScript project and creates a tsconfig.json file. --project, -p Compile the project given the path to its configuration file, or to a folder with a 'tsconfig.json'. --build, -b Build one or more projects and their dependencies, if out of date --showConfig Print the final configuration instead of building. COMMON COMPILER OPTIONS --pretty Enable color and formatting in TypeScript's output to make compiler errors easier to read. type: boolean default: true --target, -t Set the JavaScript language version for emitted JavaScript and include compatible library declarations. one of: es3, es5, es6/es2015, es2016, es2017, es2018, es2019, es2020, es2021, es2022, esnext default: es3 --module, -m Specify what module code is generated. one of: none, commonjs, amd, umd, system, es6/es2015, es2020, es2022, esnext, node16, nodenext default: undefined --lib Specify a set of bundled library declaration files that describe the target runtime environment. one or more: es5, es6/es2015, es7/es2016, es2017, es2018, es2019, es2020, es2021, es2022, esnext, dom, dom.iterable, webworker, webworker.importscripts, webworker.iterable, scripthost, es2015.core, es2015.collection, es2015.generator, es2015.iterable, es2015.promise, es2015.proxy, es2015.reflect, es2015.symbol, es2015.symbol.wellknown, es2016.array.include, es2017.object, es2017.sharedmemory, es2017.string, es2017.intl, es2017.typedarrays, es2018.asyncgenerator, es2018.asynciterable/esnext.asynciterable, es2018.intl, es2018.promise, es2018.regexp, es2019.array, es2019.object, es2019.string, es2019.symbol/esnext.symbol, es2020.bigint/esnext.bigint, es2020.date, es2020.promise, es2020.sharedmemory, es2020.string, es2020.symbol.wellknown, es2020.intl, es2020.number, es2021.promise/esnext.promise, es2021.string, es2021.weakref/esnext.weakref, es2021.intl, es2022.array/esnext.array, es2022.error, es2022.intl, es2022.object, es2022.sharedmemory, es2022.string/esnext.string, esnext.intl default: undefined --allowJs Allow JavaScript files to be a part of your program. Use the 'checkJS' option to get errors from these files. type: boolean default: false --checkJs Enable error reporting in type-checked JavaScript files. type: boolean default: false --jsx Specify what JSX code is generated. one of: preserve, react, react-native, react-jsx, react-jsxdev default: undefined --declaration, -d Generate .d.ts files from TypeScript and JavaScript files in your project. type: boolean default: `false`, unless `composite` is set --declarationMap Create sourcemaps for d.ts files. type: boolean default: false --emitDeclarationOnly Only output d.ts files and not JavaScript files. type: boolean default: false --sourceMap Create source map files for emitted JavaScript files. type: boolean default: false --outFile Specify a file that bundles all outputs into one JavaScript file. If 'declaration' is true, also designates a file that bundles all .d.ts output. --outDir Specify an output folder for all emitted files. --removeComments Disable emitting comments. type: boolean default: false --noEmit Disable emitting files from a compilation. type: boolean default: false --strict Enable all strict type-checking options. type: boolean default: false --types Specify type package names to be included without being referenced in a source file. --esModuleInterop Emit additional JavaScript to ease support for importing CommonJS modules. This enables 'allowSyntheticDefaultImports' for type compatibility. type: boolean default: false You can learn about all of the compiler options at https://aka.ms/tsc, so the test suite must satisfy compile-time nullability rules even when runtime assertions already guarantee the value. - What: narrowed the reply header mock explicitly before reading call arguments and removed the unsafe cast when constructing the redirect URL. - How: assert the header mock exists, derive the matching call from the narrowed mock, and guard the extracted header value before passing it to . --- .../rest.controller.oidc.integration.test.ts | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/api/src/unraid-api/rest/rest.controller.oidc.integration.test.ts b/api/src/unraid-api/rest/rest.controller.oidc.integration.test.ts index bf0af82358..035621d346 100644 --- a/api/src/unraid-api/rest/rest.controller.oidc.integration.test.ts +++ b/api/src/unraid-api/rest/rest.controller.oidc.integration.test.ts @@ -183,13 +183,27 @@ describe('RestController OIDC authorize integration', () => { expect.stringContaining('https://provider.example.com/oauth/authorize') ); - const locationHeader = vi - .mocked(mockReply.header) - .mock.calls.find(([name]) => name === 'Location')?.[1]; + const headerMock = mockReply.header; + expect(headerMock).toBeDefined(); + if (!headerMock) { + throw new Error('Expected reply header mock to be set'); + } + + const locationHeaderCall = vi + .mocked(headerMock) + .mock.calls.find(([name]) => name === 'Location'); + + expect(locationHeaderCall).toBeDefined(); + + const locationHeader = locationHeaderCall?.[1]; expect(locationHeader).toBeTypeOf('string'); - const locationUrl = new URL(locationHeader as string); + if (typeof locationHeader !== 'string') { + throw new Error('Expected redirect Location header to be set'); + } + + const locationUrl = new URL(locationHeader); expect(locationUrl.searchParams.get('redirect_uri')).toBe( 'https://nas.domain.com/graphql/api/auth/oidc/callback' ); From 182f27d7bcb6dd533c1f4beb546d6d77af557bfa Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Wed, 1 Apr 2026 13:05:56 -0400 Subject: [PATCH 5/5] fix(oidc): tighten proxy trust and test contracts - Purpose: address the remaining review feedback on proxy trust configuration, request-origin typing, and OIDC regression test brittleness. - Before: Fastify trustProxy used a misleading loopback CIDR string, request-origin utilities still carried an unnecessary optional host contract, and a few tests were more tightly coupled to implementation details than needed. - Why: the runtime behavior was mostly correct, but the follow-up cleanup reduces ambiguity around trusted proxy behavior and keeps the new OIDC coverage maintainable under strict type-checking. - What: switch Fastify trustProxy to the explicit loopback alias, make RequestOriginInfo.host always defined, reuse normalized request info in the authorize handler, relax async error assertions, and harden the authorize integration reply mock. - How: update the bootstrap/test expectations, simplify request-origin construction, pass only protocol/host from normalized request info, and rework the integration test spies so they satisfy both Vitest and TypeScript without unsafe call-site assumptions. --- .../client/oidc-redirect-uri.service.test.ts | 5 +- .../sso/utils/oidc-request-handler.util.ts | 6 ++- .../sso/utils/oidc-request-origin.util.ts | 9 ++-- api/src/unraid-api/main.test.ts | 2 +- api/src/unraid-api/main.ts | 2 +- .../rest.controller.oidc.integration.test.ts | 47 +++++++++++-------- 6 files changed, 40 insertions(+), 31 deletions(-) diff --git a/api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.test.ts b/api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.test.ts index 72e0756f06..0f6016e4f7 100644 --- a/api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.test.ts +++ b/api/src/unraid-api/graph/resolvers/sso/client/oidc-redirect-uri.service.test.ts @@ -1,4 +1,3 @@ -import { UnauthorizedException } from '@nestjs/common'; import { Test, TestingModule } from '@nestjs/testing'; import { beforeEach, describe, expect, it, vi } from 'vitest'; @@ -70,7 +69,7 @@ describe('OidcRedirectUriService', () => { protocol: 'https', host: 'example.com', }) - ).rejects.toThrow(UnauthorizedException); + ).rejects.toThrow(); }); it('passes through missing allowed origins', async () => { @@ -155,7 +154,7 @@ describe('OidcRedirectUriService', () => { protocol: 'https', host: 'example.com', }) - ).rejects.toThrow(UnauthorizedException); + ).rejects.toThrow(); }); }); }); diff --git a/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.ts b/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.ts index b7dafc129f..d96d6a2034 100644 --- a/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.ts +++ b/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-handler.util.ts @@ -5,7 +5,6 @@ import { OidcService } from '@app/unraid-api/graph/resolvers/sso/core/oidc.servi import { OidcStateExtractor } from '@app/unraid-api/graph/resolvers/sso/session/oidc-state-extractor.util.js'; import { extractRequestInfo, - extractRequestOriginInfo, RequestInfo, } from '@app/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.js'; @@ -52,7 +51,10 @@ export class OidcRequestHandler { providerId, state, requestOrigin: redirectUri, - requestOriginInfo: extractRequestOriginInfo(req), + requestOriginInfo: { + protocol: requestInfo.protocol, + host: requestInfo.host, + }, }); logger.log(`Redirecting to OIDC provider: ${authUrl}`); diff --git a/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.ts b/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.ts index 02db62bea7..7e73612770 100644 --- a/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.ts +++ b/api/src/unraid-api/graph/resolvers/sso/utils/oidc-request-origin.util.ts @@ -2,7 +2,7 @@ import type { FastifyRequest } from '@app/unraid-api/types/fastify.js'; export interface RequestOriginInfo { protocol: string; - host: string | undefined; + host: string; } export interface RequestInfo extends RequestOriginInfo { @@ -20,13 +20,12 @@ export function extractRequestOriginInfo(req: FastifyRequest): RequestOriginInfo export function extractRequestInfo(req: FastifyRequest): RequestInfo { const { protocol, host } = extractRequestOriginInfo(req); - const resolvedHost = host || 'localhost:3000'; - const fullUrl = `${protocol}://${resolvedHost}${req.url}`; - const baseUrl = `${protocol}://${resolvedHost}`; + const fullUrl = `${protocol}://${host}${req.url}`; + const baseUrl = `${protocol}://${host}`; return { protocol, - host: resolvedHost, + host, fullUrl, baseUrl, }; diff --git a/api/src/unraid-api/main.test.ts b/api/src/unraid-api/main.test.ts index c1c04a2896..cddfa1a90d 100644 --- a/api/src/unraid-api/main.test.ts +++ b/api/src/unraid-api/main.test.ts @@ -100,7 +100,7 @@ describe('bootstrapNestServer', () => { await bootstrapNestServer(); expect(fastifyAdapterConstructor).toHaveBeenCalledWith({ - trustProxy: '127.0.0.1/8,::1/128', + trustProxy: 'loopback', }); }); }); diff --git a/api/src/unraid-api/main.ts b/api/src/unraid-api/main.ts index aef8ccff6a..339a58614f 100644 --- a/api/src/unraid-api/main.ts +++ b/api/src/unraid-api/main.ts @@ -66,7 +66,7 @@ export async function bootstrapNestServer(): Promise { const app = await NestFactory.create( AppModule, new FastifyAdapter({ - trustProxy: '127.0.0.1/8,::1/128', + trustProxy: 'loopback', }), { bufferLogs: false, diff --git a/api/src/unraid-api/rest/rest.controller.oidc.integration.test.ts b/api/src/unraid-api/rest/rest.controller.oidc.integration.test.ts index 035621d346..b602736756 100644 --- a/api/src/unraid-api/rest/rest.controller.oidc.integration.test.ts +++ b/api/src/unraid-api/rest/rest.controller.oidc.integration.test.ts @@ -29,7 +29,10 @@ describe('RestController OIDC authorize integration', () => { let oidcStateService: { generateSecureState: ReturnType; }; - let mockReply: Partial; + let headerSpy: ReturnType; + let sendSpy: ReturnType; + let statusSpy: ReturnType; + let mockReply: FastifyReply; const provider: OidcProvider = { id: 'test-provider', @@ -142,11 +145,26 @@ describe('RestController OIDC authorize integration', () => { controller = module.get(RestController); - mockReply = { - status: vi.fn().mockReturnThis(), - header: vi.fn().mockReturnThis(), - send: vi.fn().mockReturnThis(), - }; + statusSpy = vi.fn(); + headerSpy = vi.fn(); + sendSpy = vi.fn(); + + const reply = {} as FastifyReply; + + reply.status = ((statusCode: number) => { + statusSpy(statusCode); + return reply; + }) as FastifyReply['status']; + reply.header = ((name: string, value: string) => { + headerSpy(name, value); + return reply; + }) as FastifyReply['header']; + reply.send = ((payload?: unknown) => { + sendSpy(payload); + return reply; + }) as FastifyReply['send']; + + mockReply = reply; }); it('authorizes successfully through both redirect validation layers for proxied domains', async () => { @@ -168,7 +186,7 @@ describe('RestController OIDC authorize integration', () => { 'client-state-123', 'https://nas.domain.com/graphql/api/auth/oidc/callback', mockRequest, - mockReply as FastifyReply + mockReply ); expect(oidcStateService.generateSecureState).toHaveBeenCalledWith( @@ -177,22 +195,13 @@ describe('RestController OIDC authorize integration', () => { 'https://nas.domain.com/graphql/api/auth/oidc/callback' ); - expect(mockReply.status).toHaveBeenCalledWith(302); - expect(mockReply.header).toHaveBeenCalledWith( + expect(statusSpy).toHaveBeenCalledWith(302); + expect(headerSpy).toHaveBeenCalledWith( 'Location', expect.stringContaining('https://provider.example.com/oauth/authorize') ); - const headerMock = mockReply.header; - expect(headerMock).toBeDefined(); - - if (!headerMock) { - throw new Error('Expected reply header mock to be set'); - } - - const locationHeaderCall = vi - .mocked(headerMock) - .mock.calls.find(([name]) => name === 'Location'); + const locationHeaderCall = vi.mocked(headerSpy).mock.calls.find(([name]) => name === 'Location'); expect(locationHeaderCall).toBeDefined();