mirror of
https://github.com/unraid/api.git
synced 2026-02-05 15:39:05 -06:00
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. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
Eli Bosley
@@ -1,5 +1,5 @@
|
||||
{
|
||||
"version": "4.18.0",
|
||||
"version": "4.18.1",
|
||||
"extraOrigins": [],
|
||||
"sandbox": true,
|
||||
"ssoSubIds": [],
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -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: [
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -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
|
||||
],
|
||||
};
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user