From c7c3bb57ea482633a7acff064b39fbc8d4e07213 Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Wed, 3 Sep 2025 11:56:30 -0400 Subject: [PATCH] fix: update OIDC URL validation and add tests (#1646) - Updated the OIDC issuer URL validation to prevent trailing slashes and whitespace. - Introduced a utility class `OidcUrlPatterns` for managing URL patterns and validation logic. - Added comprehensive tests for the new URL validation logic and examples to ensure correctness. - Bumped version to 4.18.1 in the configuration file. ## Summary by CodeRabbit - New Features - Added strict validation for OIDC issuer URLs in the SSO configuration form, with clearer guidance to avoid trailing slashes. - Bug Fixes - Prevented misconfiguration by rejecting issuer URLs with trailing slashes (e.g., Google issuer), avoiding double slashes in discovery URLs. - Tests - Introduced comprehensive unit tests covering issuer URL validation, patterns, and real-world scenarios to ensure reliability. - Chores - Bumped version to 4.18.1. --- api/dev/configs/api.json | 2 +- .../sso/core/oidc-config.service.test.ts | 87 ++++++++ .../resolvers/sso/core/oidc-config.service.ts | 18 +- .../sso/utils/oidc-url-patterns.util.test.ts | 205 ++++++++++++++++++ .../sso/utils/oidc-url-patterns.util.ts | 59 +++++ pnpm-lock.yaml | 12 + unraid-ui/package.json | 5 +- unraid-ui/src/forms/config.ts | 8 +- 8 files changed, 391 insertions(+), 5 deletions(-) create mode 100644 api/src/unraid-api/graph/resolvers/sso/core/oidc-config.service.test.ts create mode 100644 api/src/unraid-api/graph/resolvers/sso/utils/oidc-url-patterns.util.test.ts create mode 100644 api/src/unraid-api/graph/resolvers/sso/utils/oidc-url-patterns.util.ts diff --git a/api/dev/configs/api.json b/api/dev/configs/api.json index 3aef850b3..0a837375c 100644 --- a/api/dev/configs/api.json +++ b/api/dev/configs/api.json @@ -1,5 +1,5 @@ { - "version": "4.18.0", + "version": "4.18.1", "extraOrigins": [], "sandbox": true, "ssoSubIds": [], diff --git a/api/src/unraid-api/graph/resolvers/sso/core/oidc-config.service.test.ts b/api/src/unraid-api/graph/resolvers/sso/core/oidc-config.service.test.ts new file mode 100644 index 000000000..3d5f71761 --- /dev/null +++ b/api/src/unraid-api/graph/resolvers/sso/core/oidc-config.service.test.ts @@ -0,0 +1,87 @@ +import { ConfigService } from '@nestjs/config'; +import { Test, TestingModule } from '@nestjs/testing'; + +import { UserSettingsService } from '@unraid/shared/services/user-settings.js'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +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 { OidcUrlPatterns } from '@app/unraid-api/graph/resolvers/sso/utils/oidc-url-patterns.util.js'; + +describe('OidcConfigPersistence', () => { + let service: OidcConfigPersistence; + let mockConfigService: ConfigService; + let mockUserSettingsService: UserSettingsService; + let mockValidationService: OidcValidationService; + + beforeEach(async () => { + const module: TestingModule = await Test.createTestingModule({ + providers: [ + OidcConfigPersistence, + { + provide: ConfigService, + useValue: { + get: vi.fn(), + set: vi.fn(), + }, + }, + { + provide: UserSettingsService, + useValue: { + register: vi.fn(), + }, + }, + { + provide: OidcValidationService, + useValue: { + validateProvider: vi.fn(), + }, + }, + ], + }).compile(); + + service = module.get(OidcConfigPersistence); + mockConfigService = module.get(ConfigService); + mockUserSettingsService = module.get(UserSettingsService); + mockValidationService = module.get(OidcValidationService); + + // Mock persist method to avoid file system operations + vi.spyOn(service, 'persist').mockResolvedValue(true); + }); + + describe('URL validation integration', () => { + it('should validate issuer URLs using the shared utility', () => { + // Test that our shared utility correctly validates URLs + // This ensures the pattern we use in the form schema works correctly + const examples = OidcUrlPatterns.getExamples(); + + // Test valid URLs + examples.valid.forEach((url) => { + expect(OidcUrlPatterns.isValidIssuerUrl(url)).toBe(true); + }); + + // Test invalid URLs + examples.invalid.forEach((url) => { + expect(OidcUrlPatterns.isValidIssuerUrl(url)).toBe(false); + }); + }); + + it('should validate the pattern constant matches the regex', () => { + // Ensure the pattern string can be compiled into a valid regex + expect(() => new RegExp(OidcUrlPatterns.ISSUER_URL_PATTERN)).not.toThrow(); + + // Ensure the static regex matches the pattern + const manualRegex = new RegExp(OidcUrlPatterns.ISSUER_URL_PATTERN); + expect(OidcUrlPatterns.ISSUER_URL_REGEX.source).toBe(manualRegex.source); + }); + + it('should reject the specific URL from the bug report', () => { + // Test the exact scenario that caused the original bug + const problematicUrl = 'https://accounts.google.com/'; + const correctUrl = 'https://accounts.google.com'; + + expect(OidcUrlPatterns.isValidIssuerUrl(problematicUrl)).toBe(false); + expect(OidcUrlPatterns.isValidIssuerUrl(correctUrl)).toBe(true); + }); + }); +}); diff --git a/api/src/unraid-api/graph/resolvers/sso/core/oidc-config.service.ts b/api/src/unraid-api/graph/resolvers/sso/core/oidc-config.service.ts index 502e04563..fc4ee4ed5 100644 --- a/api/src/unraid-api/graph/resolvers/sso/core/oidc-config.service.ts +++ b/api/src/unraid-api/graph/resolvers/sso/core/oidc-config.service.ts @@ -12,6 +12,7 @@ import { OidcAuthorizationRule, OidcProvider, } from '@app/unraid-api/graph/resolvers/sso/models/oidc-provider.model.js'; +import { OidcUrlPatterns } from '@app/unraid-api/graph/resolvers/sso/utils/oidc-url-patterns.util.js'; import { createAccordionLayout, createLabeledControl, @@ -620,7 +621,22 @@ export class OidcConfigPersistence extends ConfigFilePersister { type: 'string', title: 'Issuer URL', format: 'uri', - description: 'OIDC issuer URL (e.g., https://accounts.google.com)', + allOf: [ + { + pattern: OidcUrlPatterns.ISSUER_URL_PATTERN, + errorMessage: + 'Must be a valid HTTP or HTTPS URL without trailing slashes or whitespace', + }, + { + not: { + pattern: '\\.well-known', + }, + errorMessage: + 'Cannot contain /.well-known/ paths. Use the base issuer URL instead (e.g., https://accounts.google.com instead of https://accounts.google.com/.well-known/openid-configuration)', + }, + ], + description: + 'OIDC issuer URL (e.g., https://accounts.google.com). Cannot contain /.well-known/ paths - use the base issuer URL instead of the full discovery endpoint. Must not end with a trailing slash.', }, authorizationEndpoint: { anyOf: [ diff --git a/api/src/unraid-api/graph/resolvers/sso/utils/oidc-url-patterns.util.test.ts b/api/src/unraid-api/graph/resolvers/sso/utils/oidc-url-patterns.util.test.ts new file mode 100644 index 000000000..e9d5f393d --- /dev/null +++ b/api/src/unraid-api/graph/resolvers/sso/utils/oidc-url-patterns.util.test.ts @@ -0,0 +1,205 @@ +import { describe, expect, it } from 'vitest'; + +import { OidcUrlPatterns } from '@app/unraid-api/graph/resolvers/sso/utils/oidc-url-patterns.util.js'; + +describe('OidcUrlPatterns', () => { + describe('ISSUER_URL_PATTERN', () => { + it('should be defined as a string', () => { + expect(typeof OidcUrlPatterns.ISSUER_URL_PATTERN).toBe('string'); + expect(OidcUrlPatterns.ISSUER_URL_PATTERN).toBe('^https?://[^/\\s]+(?:/[^/\\s]*)*[^/\\s]$'); + }); + }); + + describe('ISSUER_URL_REGEX', () => { + it('should be a RegExp instance', () => { + expect(OidcUrlPatterns.ISSUER_URL_REGEX).toBeInstanceOf(RegExp); + }); + + it('should match the pattern string', () => { + const regex = new RegExp(OidcUrlPatterns.ISSUER_URL_PATTERN); + expect(OidcUrlPatterns.ISSUER_URL_REGEX.source).toBe(regex.source); + }); + }); + + describe('isValidIssuerUrl', () => { + it('should accept valid URLs without trailing slash', () => { + const validUrls = [ + 'https://accounts.google.com', + 'https://auth.example.com/oidc', + 'https://auth.example.com/realms/master', + 'http://localhost:8080', + 'http://localhost:8080/auth', + 'https://login.microsoftonline.com/common/v2.0', + ]; + + validUrls.forEach((url) => { + expect(OidcUrlPatterns.isValidIssuerUrl(url)).toBe(true); + }); + }); + + it('should reject URLs with trailing slashes', () => { + const invalidUrls = [ + 'https://accounts.google.com/', + 'https://auth.example.com/oidc/', + 'https://auth.example.com/realms/master/', + 'http://localhost:8080/', + 'http://localhost:8080/auth/', + 'https://login.microsoftonline.com/common/v2.0/', + ]; + + invalidUrls.forEach((url) => { + expect(OidcUrlPatterns.isValidIssuerUrl(url)).toBe(false); + }); + }); + + it('should reject URLs with whitespace', () => { + const invalidUrls = [ + 'https://accounts.google.com ', + ' https://accounts.google.com', + 'https://accounts. google.com', + 'https://accounts.google.com\t', + 'https://accounts.google.com\n', + ]; + + invalidUrls.forEach((url) => { + expect(OidcUrlPatterns.isValidIssuerUrl(url)).toBe(false); + }); + }); + + it('should accept both HTTP and HTTPS protocols', () => { + expect(OidcUrlPatterns.isValidIssuerUrl('https://example.com')).toBe(true); + expect(OidcUrlPatterns.isValidIssuerUrl('http://example.com')).toBe(true); + }); + + it('should reject other protocols', () => { + expect(OidcUrlPatterns.isValidIssuerUrl('ftp://example.com')).toBe(false); + expect(OidcUrlPatterns.isValidIssuerUrl('ws://example.com')).toBe(false); + expect(OidcUrlPatterns.isValidIssuerUrl('file://example.com')).toBe(false); + }); + + it('should accept .well-known URLs without trailing slashes', () => { + const wellKnownUrls = [ + 'https://example.com/.well-known/openid-configuration', + 'https://auth.example.com/path/.well-known/openid-configuration', + 'https://example.com/.well-known/jwks.json', + 'https://keycloak.example.com/realms/master/.well-known/openid-configuration', + ]; + + wellKnownUrls.forEach((url) => { + expect(OidcUrlPatterns.isValidIssuerUrl(url)).toBe(true); + }); + }); + + it('should reject .well-known URLs with trailing slashes', () => { + const invalidWellKnownUrls = [ + 'https://example.com/.well-known/openid-configuration/', + 'https://auth.example.com/path/.well-known/openid-configuration/', + 'https://example.com/.well-known/jwks.json/', + 'https://keycloak.example.com/realms/master/.well-known/openid-configuration/', + ]; + + invalidWellKnownUrls.forEach((url) => { + expect(OidcUrlPatterns.isValidIssuerUrl(url)).toBe(false); + }); + }); + + it('should handle complex real-world scenarios', () => { + // Google + expect(OidcUrlPatterns.isValidIssuerUrl('https://accounts.google.com')).toBe(true); + expect(OidcUrlPatterns.isValidIssuerUrl('https://accounts.google.com/')).toBe(false); + + // Microsoft + expect( + OidcUrlPatterns.isValidIssuerUrl('https://login.microsoftonline.com/tenant-id/v2.0') + ).toBe(true); + expect( + OidcUrlPatterns.isValidIssuerUrl('https://login.microsoftonline.com/tenant-id/v2.0/') + ).toBe(false); + + // Auth0 + expect(OidcUrlPatterns.isValidIssuerUrl('https://tenant.auth0.com')).toBe(true); + expect(OidcUrlPatterns.isValidIssuerUrl('https://tenant.auth0.com/')).toBe(false); + + // Keycloak + expect(OidcUrlPatterns.isValidIssuerUrl('https://keycloak.example.com/realms/master')).toBe( + true + ); + expect(OidcUrlPatterns.isValidIssuerUrl('https://keycloak.example.com/realms/master/')).toBe( + false + ); + + // AWS Cognito + expect( + OidcUrlPatterns.isValidIssuerUrl( + 'https://cognito-idp.us-west-2.amazonaws.com/us-west-2_example' + ) + ).toBe(true); + expect( + OidcUrlPatterns.isValidIssuerUrl( + 'https://cognito-idp.us-west-2.amazonaws.com/us-west-2_example/' + ) + ).toBe(false); + }); + }); + + describe('getExamples', () => { + it('should return valid and invalid URL examples', () => { + const examples = OidcUrlPatterns.getExamples(); + + expect(examples).toHaveProperty('valid'); + expect(examples).toHaveProperty('invalid'); + expect(Array.isArray(examples.valid)).toBe(true); + expect(Array.isArray(examples.invalid)).toBe(true); + expect(examples.valid.length).toBeGreaterThan(0); + expect(examples.invalid.length).toBeGreaterThan(0); + }); + + it('should have all valid examples pass validation', () => { + const examples = OidcUrlPatterns.getExamples(); + + examples.valid.forEach((url) => { + expect(OidcUrlPatterns.isValidIssuerUrl(url)).toBe(true); + }); + }); + + it('should have all invalid examples fail validation', () => { + const examples = OidcUrlPatterns.getExamples(); + + examples.invalid.forEach((url) => { + expect(OidcUrlPatterns.isValidIssuerUrl(url)).toBe(false); + }); + }); + }); + + describe('integration with the bug report scenario', () => { + it('should specifically catch the Google trailing slash issue from the bug report', () => { + // The exact scenario from the bug report + const problematicUrl = 'https://accounts.google.com/'; + const correctUrl = 'https://accounts.google.com'; + + expect(OidcUrlPatterns.isValidIssuerUrl(problematicUrl)).toBe(false); + expect(OidcUrlPatterns.isValidIssuerUrl(correctUrl)).toBe(true); + }); + + it('should prevent the double slash in discovery URL construction', () => { + // Simulate what would happen in discovery URL construction + const issuerWithSlash = 'https://accounts.google.com/'; + const issuerWithoutSlash = 'https://accounts.google.com'; + + // This is what would happen in the discovery process + const discoveryWithSlash = `${issuerWithSlash}/.well-known/openid-configuration`; + const discoveryWithoutSlash = `${issuerWithoutSlash}/.well-known/openid-configuration`; + + expect(discoveryWithSlash).toBe( + 'https://accounts.google.com//.well-known/openid-configuration' + ); // Double slash - bad + expect(discoveryWithoutSlash).toBe( + 'https://accounts.google.com/.well-known/openid-configuration' + ); // Single slash - good + + // Our validation should prevent the first scenario + expect(OidcUrlPatterns.isValidIssuerUrl(issuerWithSlash)).toBe(false); + expect(OidcUrlPatterns.isValidIssuerUrl(issuerWithoutSlash)).toBe(true); + }); + }); +}); diff --git a/api/src/unraid-api/graph/resolvers/sso/utils/oidc-url-patterns.util.ts b/api/src/unraid-api/graph/resolvers/sso/utils/oidc-url-patterns.util.ts new file mode 100644 index 000000000..2c7372ee3 --- /dev/null +++ b/api/src/unraid-api/graph/resolvers/sso/utils/oidc-url-patterns.util.ts @@ -0,0 +1,59 @@ +/** + * Utility for OIDC URL validation patterns + */ +export class OidcUrlPatterns { + /** + * Regex pattern for validating OIDC issuer URLs + * - Allows HTTP and HTTPS protocols + * - Prevents trailing slashes + * - Prevents whitespace + * - Allows paths but not ending with slash + */ + static readonly ISSUER_URL_PATTERN = '^https?://[^/\\s]+(?:/[^/\\s]*)*[^/\\s]$'; + + /** + * Compiled regex for issuer URL validation + */ + static readonly ISSUER_URL_REGEX = new RegExp(OidcUrlPatterns.ISSUER_URL_PATTERN); + + /** + * Validate an issuer URL against the pattern + * @param url The URL to validate + * @returns True if the URL is valid, false otherwise + */ + static isValidIssuerUrl(url: string): boolean { + return this.ISSUER_URL_REGEX.test(url); + } + + /** + * Get examples of valid and invalid issuer URLs for documentation/testing + */ + static getExamples() { + return { + valid: [ + // Standard issuer URLs (most common) + 'https://accounts.google.com', + 'https://auth.example.com/oidc', + 'https://auth.example.com/realms/master', + 'http://localhost:8080', + 'http://localhost:8080/auth', + 'https://login.microsoftonline.com/common/v2.0', + 'https://cognito-idp.us-west-2.amazonaws.com/us-west-2_example', + // Well-known URLs are valid at the URL pattern level (schema-level validation handles rejection) + 'https://example.com/.well-known/openid-configuration', + 'https://auth.example.com/path/.well-known/openid-configuration', + 'https://example.com/.well-known/jwks.json', + ], + invalid: [ + 'https://accounts.google.com/', // Trailing slash + 'https://auth.example.com/oidc/', // Trailing slash + 'https://auth.example.com/realms/master/', // Trailing slash + 'http://localhost:8080/', // Trailing slash + 'https://accounts.google.com ', // Trailing whitespace + ' https://accounts.google.com', // Leading whitespace + 'https://accounts. google.com', // Internal whitespace + 'ftp://example.com', // Invalid protocol + ], + }; + } +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 2482ee451..b2d69d4dc 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -864,6 +864,9 @@ importers: '@vueuse/core': specifier: 13.8.0 version: 13.8.0(vue@3.5.20(typescript@5.9.2)) + ajv-errors: + specifier: ^3.0.0 + version: 3.0.0(ajv@8.17.1) class-variance-authority: specifier: 0.7.1 version: 0.7.1 @@ -6062,6 +6065,11 @@ packages: resolution: {integrity: sha512-4I7Td01quW/RpocfNayFdFVk1qSuoh0E7JrbRJ16nH01HhKFQ88INq9Sd+nd72zqRySlr9BmDA8xlEJ6vJMrYA==} engines: {node: '>=8'} + ajv-errors@3.0.0: + resolution: {integrity: sha512-V3wD15YHfHz6y0KdhYFjyy9vWtEVALT9UrxfN3zqlI6dMioHnJrqOYfyPKol3oqrnCM9uwkcdCwkJ0WUcbLMTQ==} + peerDependencies: + ajv: ^8.0.1 + ajv-formats@2.1.1: resolution: {integrity: sha512-Wx0Kx52hxE7C18hkMEggYlEifqWZtYaRgouJor+WMdPnQyEK13vgEWyVNup7SoeeoLMsr4kf5h6dOW11I15MUA==} peerDependencies: @@ -19100,6 +19108,10 @@ snapshots: clean-stack: 2.2.0 indent-string: 4.0.0 + ajv-errors@3.0.0(ajv@8.17.1): + dependencies: + ajv: 8.17.1 + ajv-formats@2.1.1(ajv@8.17.1): optionalDependencies: ajv: 8.17.1 diff --git a/unraid-ui/package.json b/unraid-ui/package.json index d3ec58bb3..14a683f7b 100644 --- a/unraid-ui/package.json +++ b/unraid-ui/package.json @@ -42,9 +42,9 @@ "deploy:storybook:staging": "pnpm build-storybook && wrangler deploy --env staging" }, "peerDependencies": { + "ajv": "8.17.1", "tailwindcss": "4.1.12", - "vue": "3.5.20", - "ajv": "8.17.1" + "vue": "3.5.20" }, "dependencies": { "@headlessui/vue": "1.7.23", @@ -55,6 +55,7 @@ "@jsonforms/vue-vanilla": "3.6.0", "@tailwindcss/cli": "4.1.12", "@vueuse/core": "13.8.0", + "ajv-errors": "^3.0.0", "class-variance-authority": "0.7.1", "clsx": "2.1.1", "dompurify": "3.2.6", diff --git a/unraid-ui/src/forms/config.ts b/unraid-ui/src/forms/config.ts index 897efa51d..28faaa9b7 100644 --- a/unraid-ui/src/forms/config.ts +++ b/unraid-ui/src/forms/config.ts @@ -1,5 +1,6 @@ import { createAjv } from '@jsonforms/core'; import type Ajv from 'ajv'; +import addErrors from 'ajv-errors'; export interface JsonFormsConfig { /** @@ -20,10 +21,15 @@ export interface JsonFormsConfig { * This ensures all JSONForms instances have proper validation and visibility rule support */ export function createJsonFormsAjv(): Ajv { - return createAjv({ + const ajv = createAjv({ allErrors: true, strict: false, }); + + // Add support for custom error messages + addErrors(ajv); + + return ajv; } /**