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.
2025-12-30 21:19:49 -06:00 · 2025-09-03 11:56:30 -04:00
parent 99dbad57d5
commit c7c3bb57ea
8 changed files with 391 additions and 5 deletions
--- 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": [],
--- 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
@@ -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>(OidcConfigPersistence);
+        mockConfigService = module.get<ConfigService>(ConfigService);
+        mockUserSettingsService = module.get<UserSettingsService>(UserSettingsService);
+        mockValidationService = module.get<OidcValidationService>(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);
+        });
+    });
+});
--- 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<OidcConfig> {
                                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: [
--- 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
@@ -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);
+        });
+    });
+});
--- 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
@@ -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
+            ],
+        };
+    }
+}
--- 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
--- 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",
--- 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;
 }

 /**