From 9f7a8094799972274d7a92de2b22cf10795d4ff0 Mon Sep 17 00:00:00 2001 From: Matt Castelaz Date: Tue, 20 Oct 2026 02:49:58 +0000 Subject: [PATCH 1/5] fix(auth): validate tenantId and providerId against URL path traversal --- src/auth/auth-api-request.ts | 11 +++-- src/auth/auth-config.ts | 18 +++++-- src/auth/tenant-manager.ts | 5 +- src/utils/validator.ts | 14 ++++++ test/unit/auth/auth-api-request.spec.ts | 53 ++++++++++++++++---- test/unit/auth/auth-config.spec.ts | 34 ++++++++++--- test/unit/auth/tenant-manager.spec.ts | 66 +++++++++++++++++-------- test/unit/utils/validator.spec.ts | 27 +++++++++- 8 files changed, 180 insertions(+), 48 deletions(-) diff --git a/src/auth/auth-api-request.ts b/src/auth/auth-api-request.ts index 082a8ade6b..72e0457eca 100644 --- a/src/auth/auth-api-request.ts +++ b/src/auth/auth-api-request.ts @@ -419,7 +419,7 @@ function validateCreateEditRequest(request: any, writeOperationType: WriteOperat } } if (typeof request.tenantId !== 'undefined' && - !validator.isNonEmptyString(request.tenantId)) { + !validator.isTenantId(request.tenantId)) { throw new FirebaseAuthError(authClientErrorCode.INVALID_TENANT_ID); } // For any invalid parameter, use the external key name in the error description. @@ -2212,7 +2212,7 @@ export class AuthRequestHandler extends AbstractAuthRequestHandler { * @returns A promise that resolves with the tenant information. */ public getTenant(tenantId: string): Promise { - if (!validator.isNonEmptyString(tenantId)) { + if (!validator.isTenantId(tenantId)) { return Promise.reject(new FirebaseAuthError(authClientErrorCode.INVALID_TENANT_ID)); } return this.invokeRequestHandler(this.authResourceUrlBuilder, GET_TENANT, {}, { tenantId }) @@ -2262,7 +2262,7 @@ export class AuthRequestHandler extends AbstractAuthRequestHandler { * @returns A promise that resolves when the tenant is deleted. */ public deleteTenant(tenantId: string): Promise { - if (!validator.isNonEmptyString(tenantId)) { + if (!validator.isTenantId(tenantId)) { return Promise.reject(new FirebaseAuthError(authClientErrorCode.INVALID_TENANT_ID)); } return this.invokeRequestHandler(this.authResourceUrlBuilder, DELETE_TENANT, undefined, { tenantId }) @@ -2298,7 +2298,7 @@ export class AuthRequestHandler extends AbstractAuthRequestHandler { * @returns A promise that resolves with the modified tenant object. */ public updateTenant(tenantId: string, tenantOptions: UpdateTenantRequest): Promise { - if (!validator.isNonEmptyString(tenantId)) { + if (!validator.isTenantId(tenantId)) { return Promise.reject(new FirebaseAuthError(authClientErrorCode.INVALID_TENANT_ID)); } try { @@ -2334,6 +2334,9 @@ export class TenantAwareAuthRequestHandler extends AbstractAuthRequestHandler { * @constructor */ constructor(app: App, private readonly tenantId: string, emHost?: string | null) { + if (!validator.isTenantId(tenantId)) { + throw new FirebaseAuthError(authClientErrorCode.INVALID_TENANT_ID); + } super(app, emHost); } diff --git a/src/auth/auth-config.ts b/src/auth/auth-config.ts index b7520f8471..8b3621aeb0 100644 --- a/src/auth/auth-config.ts +++ b/src/auth/auth-config.ts @@ -1178,7 +1178,12 @@ export class SAMLConfig implements SAMLAuthProviderConfig { * @returns Whether the provider ID corresponds to a SAML provider. */ public static isProviderId(providerId: any): providerId is string { - return validator.isNonEmptyString(providerId) && providerId.indexOf('saml.') === 0; + // Single dots are permitted within the provider ID suffix (e.g., "saml.my.provider"), + // so we check !providerId.endsWith('.') separately to reject "saml.", "saml..", and + // trailing-dot segments without introducing multi-quantifier regex backtracking. + return validator.isNonEmptyString(providerId) && + !providerId.endsWith('.') && + /^saml\.[a-zA-Z0-9-_.~]+$/.test(providerId); } /** @@ -1216,7 +1221,7 @@ export class SAMLConfig implements SAMLAuthProviderConfig { } // Required fields. if (validator.isNonEmptyString(options.providerId)) { - if (options.providerId.indexOf('saml.') !== 0) { + if (!SAMLConfig.isProviderId(options.providerId)) { throw new FirebaseAuthError( authClientErrorCode.INVALID_PROVIDER_ID, '"SAMLAuthProviderConfig.providerId" must be a valid non-empty string prefixed with "saml.".', @@ -1429,7 +1434,12 @@ export class OIDCConfig implements OIDCAuthProviderConfig { * @returns Whether the provider ID corresponds to an OIDC provider. */ public static isProviderId(providerId: any): providerId is string { - return validator.isNonEmptyString(providerId) && providerId.indexOf('oidc.') === 0; + // Single dots are permitted within the provider ID suffix (e.g., "oidc.my.provider"), + // so we check !providerId.endsWith('.') separately to reject "oidc.", "oidc..", and + // trailing-dot segments without introducing multi-quantifier regex backtracking. + return validator.isNonEmptyString(providerId) && + !providerId.endsWith('.') && + /^oidc\.[a-zA-Z0-9-_.~]+$/.test(providerId); } /** @@ -1469,7 +1479,7 @@ export class OIDCConfig implements OIDCAuthProviderConfig { } // Required fields. if (validator.isNonEmptyString(options.providerId)) { - if (options.providerId.indexOf('oidc.') !== 0) { + if (!OIDCConfig.isProviderId(options.providerId)) { throw new FirebaseAuthError( !options.providerId ? authClientErrorCode.MISSING_PROVIDER_ID : authClientErrorCode.INVALID_PROVIDER_ID, '"OIDCAuthProviderConfig.providerId" must be a valid non-empty string prefixed with "oidc.".', diff --git a/src/auth/tenant-manager.ts b/src/auth/tenant-manager.ts index 6d124d30ea..df6c37b018 100644 --- a/src/auth/tenant-manager.ts +++ b/src/auth/tenant-manager.ts @@ -81,6 +81,9 @@ export class TenantAwareAuth extends BaseAuth { * @internal */ constructor(app: App, tenantId: string, emHost?: string | null) { + if (!validator.isTenantId(tenantId)) { + throw new FirebaseAuthError(authClientErrorCode.INVALID_TENANT_ID); + } const emIsSet = emHost !== undefined; super(app, new TenantAwareAuthRequestHandler( app, tenantId, emHost), createFirebaseTokenGenerator( @@ -175,7 +178,7 @@ export class TenantManager { * @returns The `TenantAwareAuth` instance corresponding to this tenant identifier. */ public authForTenant(tenantId: string): TenantAwareAuth { - if (!validator.isNonEmptyString(tenantId)) { + if (!validator.isTenantId(tenantId)) { throw new FirebaseAuthError(authClientErrorCode.INVALID_TENANT_ID); } if (typeof this.tenantsMap[tenantId] === 'undefined') { diff --git a/src/utils/validator.ts b/src/utils/validator.ts index 15b2f53e68..0a273d2775 100644 --- a/src/utils/validator.ts +++ b/src/utils/validator.ts @@ -297,3 +297,17 @@ export function isTaskId(taskId: any): boolean { const VALID_TASK_ID_REGEX = /^[A-Za-z0-9_-]+$/; return VALID_TASK_ID_REGEX.test(taskId); } + +/** + * Validates that a string is a valid tenant ID. + * + * @param tenantId - The string to validate. + * @returns Whether the string is a valid tenant ID. + */ +export function isTenantId(tenantId: any): tenantId is string { + // Reject trailing dots separately to forbid ".", "..", and trailing-dot segments + // without introducing multi-quantifier regex backtracking. + return isNonEmptyString(tenantId) && + !tenantId.endsWith('.') && + /^[a-zA-Z0-9-_.~]+$/.test(tenantId); +} diff --git a/test/unit/auth/auth-api-request.spec.ts b/test/unit/auth/auth-api-request.spec.ts index 4027118106..0c3a99fc0e 100644 --- a/test/unit/auth/auth-api-request.spec.ts +++ b/test/unit/auth/auth-api-request.spec.ts @@ -907,6 +907,20 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { return handler.init(mockApp); }).not.to.throw(Error); }); + + if (!handler.supportsTenantManagement) { + const invalidTenantIds = [ + null, NaN, 0, 1, true, false, '', '.', '..', '...', 'victim.', '..\n', + '../other', 'tenant/1', 'tenant\\1', 'tenant?foo=bar', 'tenant#frag', 'tenant%2e%2e', + ]; + invalidTenantIds.forEach((invalidTenantId) => { + it('should throw given invalid tenantId: ' + JSON.stringify(invalidTenantId), () => { + expect(() => { + return new TenantAwareAuthRequestHandler(mockApp, invalidTenantId as any); + }).to.throw('The tenant ID must be a valid non-empty string.'); + }); + }); + } }); describe('Emulator Support', () => { @@ -3375,7 +3389,9 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { }); const invalidProviderIds = [ - null, NaN, 0, 1, true, false, '', 'saml.provider', ['oidc.provider'], [], {}, { a: 1 }, _.noop]; + null, NaN, 0, 1, true, false, '', 'oidc.', 'oidc..', 'oidc.provider.', 'oidc.\n', + '\uFEFFoidc.provider', 'oidc.provider/../other', 'oidc.provider?foo=bar', + 'oidc.provider#frag', 'oidc.provider%2fother', 'saml.provider', ['oidc.provider'], [], {}, { a: 1 }, _.noop]; invalidProviderIds.forEach((invalidProviderId) => { it('should be rejected given an invalid provider ID:' + JSON.stringify(invalidProviderId), () => { const expectedError = new FirebaseAuthError(authClientErrorCode.INVALID_PROVIDER_ID); @@ -3552,7 +3568,9 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { }); const invalidProviderIds = [ - null, NaN, 0, 1, true, false, '', 'saml.provider', ['oidc.provider'], [], {}, { a: 1 }, _.noop]; + null, NaN, 0, 1, true, false, '', 'oidc.', 'oidc..', 'oidc.provider.', 'oidc.\n', + '\uFEFFoidc.provider', 'oidc.provider/../other', 'oidc.provider?foo=bar', + 'oidc.provider#frag', 'oidc.provider%2fother', 'saml.provider', ['oidc.provider'], [], {}, { a: 1 }, _.noop]; invalidProviderIds.forEach((invalidProviderId) => { it('should be rejected given an invalid provider ID:' + JSON.stringify(invalidProviderId), () => { const expectedError = new FirebaseAuthError(authClientErrorCode.INVALID_PROVIDER_ID); @@ -3842,7 +3860,9 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { }); const invalidProviderIds = [ - null, NaN, 0, 1, true, false, '', 'saml.provider', ['oidc.provider'], [], {}, { a: 1 }, _.noop]; + null, NaN, 0, 1, true, false, '', 'oidc.', 'oidc..', 'oidc.provider.', 'oidc.\n', + '\uFEFFoidc.provider', 'oidc.provider/../other', 'oidc.provider?foo=bar', + 'oidc.provider#frag', 'oidc.provider%2fother', 'saml.provider', ['oidc.provider'], [], {}, { a: 1 }, _.noop]; invalidProviderIds.forEach((invalidProviderId) => { it('should be rejected given an invalid provider ID:' + JSON.stringify(invalidProviderId), () => { const expectedError = new FirebaseAuthError(authClientErrorCode.INVALID_PROVIDER_ID); @@ -3939,7 +3959,9 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { }); const invalidProviderIds = [ - null, NaN, 0, 1, true, false, '', 'oidc.provider', ['saml.provider'], [], {}, { a: 1 }, _.noop]; + null, NaN, 0, 1, true, false, '', 'saml.', 'saml..', 'saml.provider.', 'saml.\n', + '\uFEFFsaml.provider', 'saml.provider/../other', 'saml.provider?foo=bar', + 'saml.provider#frag', 'saml.provider%2fother', 'oidc.provider', ['saml.provider'], [], {}, { a: 1 }, _.noop]; invalidProviderIds.forEach((invalidProviderId) => { it('should be rejected given an invalid provider ID:' + JSON.stringify(invalidProviderId), () => { const expectedError = new FirebaseAuthError(authClientErrorCode.INVALID_PROVIDER_ID); @@ -4110,7 +4132,9 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { }); const invalidProviderIds = [ - null, NaN, 0, 1, true, false, '', 'oidc.provider', ['saml.provider'], [], {}, { a: 1 }, _.noop]; + null, NaN, 0, 1, true, false, '', 'saml.', 'saml..', 'saml.provider.', 'saml.\n', + '\uFEFFsaml.provider', 'saml.provider/../other', 'saml.provider?foo=bar', + 'saml.provider#frag', 'saml.provider%2fother', 'oidc.provider', ['saml.provider'], [], {}, { a: 1 }, _.noop]; invalidProviderIds.forEach((invalidProviderId) => { it('should be rejected given an invalid provider ID:' + JSON.stringify(invalidProviderId), () => { const expectedError = new FirebaseAuthError(authClientErrorCode.INVALID_PROVIDER_ID); @@ -4386,7 +4410,9 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { }); const invalidProviderIds = [ - null, NaN, 0, 1, true, false, '', 'oidc.provider', ['saml.provider'], [], {}, { a: 1 }, _.noop]; + null, NaN, 0, 1, true, false, '', 'saml.', 'saml..', 'saml.provider.', 'saml.\n', + '\uFEFFsaml.provider', 'saml.provider/../other', 'saml.provider?foo=bar', + 'saml.provider#frag', 'saml.provider%2fother', 'oidc.provider', ['saml.provider'], [], {}, { a: 1 }, _.noop]; invalidProviderIds.forEach((invalidProviderId) => { it('should be rejected given an invalid provider ID:' + JSON.stringify(invalidProviderId), () => { const expectedError = new FirebaseAuthError(authClientErrorCode.INVALID_PROVIDER_ID); @@ -4607,7 +4633,10 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { }); }); - const invalidTenantIds = [null, NaN, 0, 1, true, false, '', ['tenant-id'], [], {}, { a: 1 }, _.noop]; + const invalidTenantIds = [ + null, NaN, 0, 1, true, false, '', '.', '..', '...', 'victim.', '..\n', + '../other', 'tenant/1', 'tenant\\1', 'tenant?foo=bar', 'tenant#frag', 'tenant%2e%2e', + ['tenant-id'], [], {}, { a: 1 }, _.noop]; invalidTenantIds.forEach((invalidTenantId) => { it('should be rejected given an invalid tenant ID:' + JSON.stringify(invalidTenantId), () => { const expectedError = new FirebaseAuthError(authClientErrorCode.INVALID_TENANT_ID); @@ -4777,7 +4806,10 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { }); }); - const invalidTenantIds = [null, NaN, 0, 1, true, false, '', ['tenant-id'], [], {}, { a: 1 }, _.noop]; + const invalidTenantIds = [ + null, NaN, 0, 1, true, false, '', '.', '..', '...', 'victim.', '..\n', + '../other', 'tenant/1', 'tenant\\1', 'tenant?foo=bar', 'tenant#frag', 'tenant%2e%2e', + ['tenant-id'], [], {}, { a: 1 }, _.noop]; invalidTenantIds.forEach((invalidTenantId) => { it('should be rejected given an invalid tenant ID:' + JSON.stringify(invalidTenantId), () => { const expectedError = new FirebaseAuthError(authClientErrorCode.INVALID_TENANT_ID); @@ -5031,7 +5063,10 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { }); }); - const invalidTenantIds = [null, NaN, 0, 1, true, false, '', ['tenant-id'], [], {}, { a: 1 }, _.noop]; + const invalidTenantIds = [ + null, NaN, 0, 1, true, false, '', '.', '..', '...', 'victim.', '..\n', + '../other', 'tenant/1', 'tenant\\1', 'tenant?foo=bar', 'tenant#frag', 'tenant%2e%2e', + ['tenant-id'], [], {}, { a: 1 }, _.noop]; invalidTenantIds.forEach((invalidTenantId) => { it('should be rejected given an invalid tenant ID:' + JSON.stringify(invalidTenantId), () => { const expectedError = new FirebaseAuthError(authClientErrorCode.INVALID_TENANT_ID); diff --git a/test/unit/auth/auth-config.spec.ts b/test/unit/auth/auth-config.spec.ts index 786f18cef2..1436630084 100644 --- a/test/unit/auth/auth-config.spec.ts +++ b/test/unit/auth/auth-config.spec.ts @@ -651,12 +651,18 @@ describe('SAMLConfig', () => { }); describe('isProviderId()', () => { - it('should return true on valid SAML provider ID', () => { - expect(SAMLConfig.isProviderId('saml.provider')).to.be.true; + const validProviderIds = ['saml.provider', 'saml.my-provider_1.v2~test']; + validProviderIds.forEach((validProviderId) => { + it(`should return true on valid SAML provider ID "${validProviderId}"`, () => { + expect(SAMLConfig.isProviderId(validProviderId)).to.be.true; + }); }); const invalidProviderIds = [ - null, NaN, 0, 1, true, false, '', 'incorrectsaml.', 'saml', 'oidc.provider', 'other', [], [1, 'a'], + null, NaN, 0, 1, true, false, '', 'incorrectsaml.', 'saml', 'saml.', 'saml..', 'saml.provider.', + 'saml.\n', 'saml.provider\n', '\uFEFFsaml.provider', 'saml../other', 'saml.provider/../other', + 'saml.provider/other', 'saml.provider\\other', 'saml.provider?foo=bar', 'saml.provider#frag', + 'saml.provider%2fother', 'oidc.provider', 'other', [], [1, 'a'], {}, { a: 1 }, _.noop]; invalidProviderIds.forEach((invalidProviderId) => { it(`should return false on invalid SAML provider ID "${JSON.stringify(invalidProviderId)}"`, () => { @@ -784,7 +790,10 @@ describe('SAMLConfig', () => { }); const invalidProviderIds = [ - null, NaN, 0, 1, true, false, '', 'oidc.provider', 'other', [], [1, 'a'], {}, { a: 1 }, _.noop]; + null, NaN, 0, 1, true, false, '', 'saml.', 'saml..', 'saml.provider.', 'saml.\n', + '\uFEFFsaml.provider', 'saml.provider/../other', 'saml.provider?foo=bar', + 'saml.provider#frag', 'saml.provider%2fother', 'oidc.provider', 'other', + [], [1, 'a'], {}, { a: 1 }, _.noop]; invalidProviderIds.forEach((providerId) => { it('should throw on invalid providerId:' + JSON.stringify(providerId), () => { const invalidClientRequest = deepCopy(clientRequest) as any; @@ -993,12 +1002,18 @@ describe('OIDCConfig', () => { }); describe('isProviderId()', () => { - it('should return true on valid OIDC provider ID', () => { - expect(OIDCConfig.isProviderId('oidc.provider')).to.be.true; + const validProviderIds = ['oidc.provider', 'oidc.my-provider_1.v2~test']; + validProviderIds.forEach((validProviderId) => { + it(`should return true on valid OIDC provider ID "${validProviderId}"`, () => { + expect(OIDCConfig.isProviderId(validProviderId)).to.be.true; + }); }); const invalidProviderIds = [ - null, NaN, 0, 1, true, false, '', 'incorrectoidc.', 'oidc', 'saml.provider', 'other', [], [1, 'a'], + null, NaN, 0, 1, true, false, '', 'incorrectoidc.', 'oidc', 'oidc.', 'oidc..', 'oidc.provider.', + 'oidc.\n', 'oidc.provider\n', '\uFEFFoidc.provider', 'oidc../other', 'oidc.provider/../other', + 'oidc.provider/other', 'oidc.provider\\other', 'oidc.provider?foo=bar', 'oidc.provider#frag', + 'oidc.provider%2fother', 'saml.provider', 'other', [], [1, 'a'], {}, { a: 1 }, _.noop]; invalidProviderIds.forEach((invalidProviderId) => { it(`should return false on invalid OIDC provider ID "${JSON.stringify(invalidProviderId)}"`, () => { @@ -1162,7 +1177,10 @@ describe('OIDCConfig', () => { }); const invalidProviderIds = [ - null, NaN, 0, 1, true, false, '', 'other', 'saml.provider', [], [1, 'a'], {}, { a: 1 }, _.noop]; + null, NaN, 0, 1, true, false, '', 'oidc.', 'oidc..', 'oidc.provider.', 'oidc.\n', + '\uFEFFoidc.provider', 'oidc.provider/../other', 'oidc.provider?foo=bar', + 'oidc.provider#frag', 'oidc.provider%2fother', 'other', 'saml.provider', + [], [1, 'a'], {}, { a: 1 }, _.noop]; invalidProviderIds.forEach((providerId) => { it('should throw on invalid providerId:' + JSON.stringify(providerId), () => { const invalidClientRequest = deepCopy(clientRequest) as any; diff --git a/test/unit/auth/tenant-manager.spec.ts b/test/unit/auth/tenant-manager.spec.ts index c97db18182..f1ed258683 100644 --- a/test/unit/auth/tenant-manager.spec.ts +++ b/test/unit/auth/tenant-manager.spec.ts @@ -29,7 +29,7 @@ import { AuthRequestHandler } from '../../../src/auth/auth-api-request'; import { TenantServerResponse } from '../../../src/auth/tenant'; import { authClientErrorCode, FirebaseAuthError } from '../../../src/auth/error'; import { - CreateTenantRequest, UpdateTenantRequest, ListTenantsResult, Tenant, TenantManager, + CreateTenantRequest, UpdateTenantRequest, ListTenantsResult, Tenant, TenantAwareAuth, TenantManager, } from '../../../src/auth/index'; chai.should(); @@ -69,8 +69,16 @@ describe('TenantManager', () => { return mockApp.delete(); }); + const MALFORMED_TENANT_IDS = [ + '.', '..', '...', 'victim.', '..\n', '../other', 'tenant/1', 'tenant\\1', + 'tenant?foo=bar', 'tenant#frag', 'tenant%2e%2e', + ]; + describe('authForTenant()', () => { - const invalidTenantIds = [null, NaN, 0, 1, true, false, '', [], [1, 'a'], {}, { a: 1 }, _.noop]; + const invalidTenantIds = [ + null, NaN, 0, 1, true, false, '', [], [1, 'a'], {}, { a: 1 }, _.noop, + ...MALFORMED_TENANT_IDS, + ]; invalidTenantIds.forEach((invalidTenantId) => { it('should throw given invalid tenant ID: ' + JSON.stringify(invalidTenantId), () => { expect(() => { @@ -79,6 +87,15 @@ describe('TenantManager', () => { }); }); + MALFORMED_TENANT_IDS.forEach((invalidTenantId) => { + it('TenantAwareAuth constructor should throw given malformed tenant ID: ' + + JSON.stringify(invalidTenantId), () => { + expect(() => { + return new TenantAwareAuth(mockApp, invalidTenantId); + }).to.throw('The tenant ID must be a valid non-empty string.'); + }); + }); + it('should return a TenantAwareAuth with the expected tenant ID', () => { expect(tenantManager.authForTenant(TENANT_ID).tenantId).to.equal(TENANT_ID); }); @@ -116,15 +133,17 @@ describe('TenantManager', () => { .should.eventually.be.rejected.and.have.property('code', 'auth/invalid-tenant-id'); }); - it('should be rejected given an invalid tenant ID', () => { - const invalidTenantId = ''; - return tenantManager.getTenant(invalidTenantId) - .then(() => { - throw new Error('Unexpected success'); - }) - .catch((error) => { - expect(error).to.have.property('code', 'auth/invalid-tenant-id'); - }); + const invalidTenantIds = ['', ...MALFORMED_TENANT_IDS]; + invalidTenantIds.forEach((invalidTenantId) => { + it('should be rejected given an invalid tenant ID: ' + JSON.stringify(invalidTenantId), () => { + return tenantManager.getTenant(invalidTenantId) + .then(() => { + throw new Error('Unexpected success'); + }) + .catch((error) => { + expect(error).to.have.property('code', 'auth/invalid-tenant-id'); + }); + }); }); it('should be rejected given an app which returns null access tokens', () => { @@ -321,7 +340,10 @@ describe('TenantManager', () => { .should.eventually.be.rejected.and.have.property('code', 'auth/invalid-tenant-id'); }); - const invalidTenantIds = [null, NaN, 0, 1, true, false, '', ['tenant-id'], [], {}, { a: 1 }, _.noop]; + const invalidTenantIds = [ + null, NaN, 0, 1, true, false, '', ['tenant-id'], [], {}, { a: 1 }, _.noop, + ...MALFORMED_TENANT_IDS, + ]; invalidTenantIds.forEach((invalidTenantId) => { it('should be rejected given an invalid tenant ID:' + JSON.stringify(invalidTenantId), () => { return tenantManager.deleteTenant(invalidTenantId as any) @@ -500,15 +522,17 @@ describe('TenantManager', () => { .should.eventually.be.rejected.and.have.property('code', 'auth/invalid-tenant-id'); }); - it('should be rejected given an invalid tenant ID', () => { - const invalidTenantId = ''; - return tenantManager.updateTenant(invalidTenantId, tenantOptions) - .then(() => { - throw new Error('Unexpected success'); - }) - .catch((error) => { - expect(error).to.have.property('code', 'auth/invalid-tenant-id'); - }); + const invalidTenantIds = ['', ...MALFORMED_TENANT_IDS]; + invalidTenantIds.forEach((invalidTenantId) => { + it('should be rejected given an invalid tenant ID: ' + JSON.stringify(invalidTenantId), () => { + return tenantManager.updateTenant(invalidTenantId, tenantOptions) + .then(() => { + throw new Error('Unexpected success'); + }) + .catch((error) => { + expect(error).to.have.property('code', 'auth/invalid-tenant-id'); + }); + }); }); it('should be rejected given no TenantOptions', () => { diff --git a/test/unit/utils/validator.spec.ts b/test/unit/utils/validator.spec.ts index 316c793474..858b8dbd6f 100644 --- a/test/unit/utils/validator.spec.ts +++ b/test/unit/utils/validator.spec.ts @@ -24,7 +24,7 @@ import chaiAsPromised from 'chai-as-promised'; import { isArray, isNonEmptyArray, isBoolean, isNumber, isString, isNonEmptyString, isNonNullObject, isEmail, isPassword, isURL, isUid, isPhoneNumber, isObject, isBuffer, - isUTCDateString, isISODateString, + isUTCDateString, isISODateString, isTenantId, } from '../../../src/utils/validator'; @@ -531,3 +531,28 @@ describe('isISODateString()', () => { expect(isISODateString(validISODateString)).to.be.true; }); }); + +describe('isTenantId()', () => { + it('should return false given no argument', () => { + expect(isTenantId(undefined as any)).to.be.false; + }); + + const invalidTenantIds = [ + null, NaN, 0, 1, true, false, '', [], ['a'], {}, { a: 1 }, _.noop, + '.', '..', '...', 'victim.', '..\n', '../other', 'tenant/other', + 'tenant\\other', 'tenant?foo=bar', 'tenant#frag', 'tenant%2fother', + 'tenant id', + ]; + invalidTenantIds.forEach((invalidTenantId) => { + it('should return false given an invalid tenant ID: ' + JSON.stringify(invalidTenantId), () => { + expect(isTenantId(invalidTenantId)).to.be.false; + }); + }); + + const validTenantIds = ['tenant1', 'tenant-1', 'TENANT_1', 'tenant.1~foo']; + validTenantIds.forEach((validTenantId) => { + it('should return true given a valid tenant ID: ' + JSON.stringify(validTenantId), () => { + expect(isTenantId(validTenantId)).to.be.true; + }); + }); +}); From 3bd9647fdf6e3b3fd961999355cd664791265502 Mon Sep 17 00:00:00 2001 From: Matt Castelaz Date: Tue, 20 Oct 2026 14:33:07 +0000 Subject: [PATCH 2/5] fix(auth): move hyphen to end of regex character class --- src/auth/auth-config.ts | 4 ++-- src/utils/validator.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/auth/auth-config.ts b/src/auth/auth-config.ts index 8b3621aeb0..a30d54f19e 100644 --- a/src/auth/auth-config.ts +++ b/src/auth/auth-config.ts @@ -1183,7 +1183,7 @@ export class SAMLConfig implements SAMLAuthProviderConfig { // trailing-dot segments without introducing multi-quantifier regex backtracking. return validator.isNonEmptyString(providerId) && !providerId.endsWith('.') && - /^saml\.[a-zA-Z0-9-_.~]+$/.test(providerId); + /^saml\.[a-zA-Z0-9_.~-]+$/.test(providerId); } /** @@ -1439,7 +1439,7 @@ export class OIDCConfig implements OIDCAuthProviderConfig { // trailing-dot segments without introducing multi-quantifier regex backtracking. return validator.isNonEmptyString(providerId) && !providerId.endsWith('.') && - /^oidc\.[a-zA-Z0-9-_.~]+$/.test(providerId); + /^oidc\.[a-zA-Z0-9_.~-]+$/.test(providerId); } /** diff --git a/src/utils/validator.ts b/src/utils/validator.ts index 0a273d2775..e58c938a76 100644 --- a/src/utils/validator.ts +++ b/src/utils/validator.ts @@ -309,5 +309,5 @@ export function isTenantId(tenantId: any): tenantId is string { // without introducing multi-quantifier regex backtracking. return isNonEmptyString(tenantId) && !tenantId.endsWith('.') && - /^[a-zA-Z0-9-_.~]+$/.test(tenantId); + /^[a-zA-Z0-9_.~-]+$/.test(tenantId); } From 2e70d6b0688dbee965ad8bb2842577ea66641de3 Mon Sep 17 00:00:00 2001 From: Matt Castelaz Date: Tue, 20 Oct 2026 16:09:04 +0000 Subject: [PATCH 3/5] fix(auth): refine providerId validation and add uploadAccount tenantId tests --- src/auth/auth-api-request.ts | 5 +++- src/auth/auth-config.ts | 21 +++------------ test/unit/auth/auth-api-request.spec.ts | 35 +++++++++++++++++++++++++ test/unit/utils/validator.spec.ts | 4 +-- 4 files changed, 45 insertions(+), 20 deletions(-) diff --git a/src/auth/auth-api-request.ts b/src/auth/auth-api-request.ts index 72e0457eca..e94389f0f8 100644 --- a/src/auth/auth-api-request.ts +++ b/src/auth/auth-api-request.ts @@ -200,6 +200,9 @@ class TenantAwareAuthResourceUrlBuilder extends AuthResourceUrlBuilder { */ constructor(protected app: App, protected version: string, protected tenantId: string, emHost?: string) { super(app, version, emHost); + if (!validator.isTenantId(tenantId)) { + throw new FirebaseAuthError(authClientErrorCode.INVALID_TENANT_ID); + } if (emHost) { this.urlFormat = utils.formatString(FIREBASE_AUTH_EMULATOR_TENANT_URL_FORMAT, { host: emHost @@ -2334,10 +2337,10 @@ export class TenantAwareAuthRequestHandler extends AbstractAuthRequestHandler { * @constructor */ constructor(app: App, private readonly tenantId: string, emHost?: string | null) { + super(app, emHost); if (!validator.isTenantId(tenantId)) { throw new FirebaseAuthError(authClientErrorCode.INVALID_TENANT_ID); } - super(app, emHost); } /** diff --git a/src/auth/auth-config.ts b/src/auth/auth-config.ts index a30d54f19e..c675229d66 100644 --- a/src/auth/auth-config.ts +++ b/src/auth/auth-config.ts @@ -1220,15 +1220,8 @@ export class SAMLConfig implements SAMLAuthProviderConfig { } } // Required fields. - if (validator.isNonEmptyString(options.providerId)) { - if (!SAMLConfig.isProviderId(options.providerId)) { - throw new FirebaseAuthError( - authClientErrorCode.INVALID_PROVIDER_ID, - '"SAMLAuthProviderConfig.providerId" must be a valid non-empty string prefixed with "saml.".', - ); - } - } else if (!ignoreMissingFields) { - // providerId is required and not provided correctly. + if (!(ignoreMissingFields && typeof options.providerId === 'undefined') && + (!validator.isNonEmptyString(options.providerId) || !SAMLConfig.isProviderId(options.providerId))) { throw new FirebaseAuthError( !options.providerId ? authClientErrorCode.MISSING_PROVIDER_ID : authClientErrorCode.INVALID_PROVIDER_ID, '"SAMLAuthProviderConfig.providerId" must be a valid non-empty string prefixed with "saml.".', @@ -1478,14 +1471,8 @@ export class OIDCConfig implements OIDCAuthProviderConfig { } } // Required fields. - if (validator.isNonEmptyString(options.providerId)) { - if (!OIDCConfig.isProviderId(options.providerId)) { - throw new FirebaseAuthError( - !options.providerId ? authClientErrorCode.MISSING_PROVIDER_ID : authClientErrorCode.INVALID_PROVIDER_ID, - '"OIDCAuthProviderConfig.providerId" must be a valid non-empty string prefixed with "oidc.".', - ); - } - } else if (!ignoreMissingFields) { + if (!(ignoreMissingFields && typeof options.providerId === 'undefined') && + (!validator.isNonEmptyString(options.providerId) || !OIDCConfig.isProviderId(options.providerId))) { throw new FirebaseAuthError( !options.providerId ? authClientErrorCode.MISSING_PROVIDER_ID : authClientErrorCode.INVALID_PROVIDER_ID, '"OIDCAuthProviderConfig.providerId" must be a valid non-empty string prefixed with "oidc.".', diff --git a/test/unit/auth/auth-api-request.spec.ts b/test/unit/auth/auth-api-request.spec.ts index 0c3a99fc0e..8527049117 100644 --- a/test/unit/auth/auth-api-request.spec.ts +++ b/test/unit/auth/auth-api-request.spec.ts @@ -1573,6 +1573,36 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { }); } + if (handler.supportsTenantManagement) { + it('should fail client-side validation when user records have malformed tenant IDs', () => { + const invalidTenantIds = [ + '', '.', '..', '...', 'victim.', '..\n', '../other', 'tenant/1', + 'tenant\\1', 'tenant?foo=bar', 'tenant#frag', 'tenant%2e%2e', + ]; + const testUsers: UserImportRecord[] = invalidTenantIds.map((invalidTenantId, i) => ({ + uid: `user${i}`, + tenantId: invalidTenantId, + })); + const expectedResult = { + successCount: 0, + failureCount: invalidTenantIds.length, + errors: invalidTenantIds.map((_, index) => ({ + index, + error: new FirebaseAuthError(authClientErrorCode.INVALID_TENANT_ID), + })), + }; + const stub = sinon.stub(HttpClient.prototype, 'send'); + stubs.push(stub); + + const requestHandler = handler.init(mockApp); + return requestHandler.uploadAccount(testUsers) + .then((result) => { + expectUserImportResult(result, expectedResult); + expect(stub).to.have.not.been.called; + }); + }); + } + it('should resolve successfully when 1000 UserImportRecords are provided', () => { const expectedResult = utils.responseFrom({}); const stub = sinon.stub(HttpClient.prototype, 'send').resolves(expectedResult); @@ -1762,6 +1792,10 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { ], }, }, + { + uid: 'user23', + tenantId: '', + }, ] as any; const validOptions = { hash: { @@ -1856,6 +1890,7 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { `Unsupported second factor "${JSON.stringify(testUsers[22].multiFactor.enrolledFactors[0])}" provided.`, ), }, + { index: 23, error: new FirebaseAuthError(authClientErrorCode.INVALID_TENANT_ID) }, ], }; const stub = sinon.stub(HttpClient.prototype, 'send'); diff --git a/test/unit/utils/validator.spec.ts b/test/unit/utils/validator.spec.ts index 858b8dbd6f..fc692325ef 100644 --- a/test/unit/utils/validator.spec.ts +++ b/test/unit/utils/validator.spec.ts @@ -539,8 +539,8 @@ describe('isTenantId()', () => { const invalidTenantIds = [ null, NaN, 0, 1, true, false, '', [], ['a'], {}, { a: 1 }, _.noop, - '.', '..', '...', 'victim.', '..\n', '../other', 'tenant/other', - 'tenant\\other', 'tenant?foo=bar', 'tenant#frag', 'tenant%2fother', + '.', '..', '...', 'victim.', '..\n', 'tenant1\n', '\uFEFFtenant1', '../other', 'tenant/other', + 'tenant\\other', 'tenant?foo=bar', 'tenant#frag', 'tenant%2e%2e', 'tenant%2fother', 'tenant id', ]; invalidTenantIds.forEach((invalidTenantId) => { From 7bb81c2f5b5eaaaca9144c15f51b1b948544e53b Mon Sep 17 00:00:00 2001 From: Matt Castelaz Date: Tue, 20 Oct 2026 20:53:00 +0000 Subject: [PATCH 4/5] fix(auth): simplify providerId validation and clean up test inputs --- src/auth/auth-config.ts | 4 +-- test/unit/auth/auth-api-request.spec.ts | 44 ++++++++++++------------- test/unit/auth/auth-config.spec.ts | 20 +++++------ test/unit/auth/tenant-manager.spec.ts | 4 +-- test/unit/utils/validator.spec.ts | 4 +-- 5 files changed, 38 insertions(+), 38 deletions(-) diff --git a/src/auth/auth-config.ts b/src/auth/auth-config.ts index c675229d66..037e58a2bc 100644 --- a/src/auth/auth-config.ts +++ b/src/auth/auth-config.ts @@ -1221,7 +1221,7 @@ export class SAMLConfig implements SAMLAuthProviderConfig { } // Required fields. if (!(ignoreMissingFields && typeof options.providerId === 'undefined') && - (!validator.isNonEmptyString(options.providerId) || !SAMLConfig.isProviderId(options.providerId))) { + !SAMLConfig.isProviderId(options.providerId)) { throw new FirebaseAuthError( !options.providerId ? authClientErrorCode.MISSING_PROVIDER_ID : authClientErrorCode.INVALID_PROVIDER_ID, '"SAMLAuthProviderConfig.providerId" must be a valid non-empty string prefixed with "saml.".', @@ -1472,7 +1472,7 @@ export class OIDCConfig implements OIDCAuthProviderConfig { } // Required fields. if (!(ignoreMissingFields && typeof options.providerId === 'undefined') && - (!validator.isNonEmptyString(options.providerId) || !OIDCConfig.isProviderId(options.providerId))) { + !OIDCConfig.isProviderId(options.providerId)) { throw new FirebaseAuthError( !options.providerId ? authClientErrorCode.MISSING_PROVIDER_ID : authClientErrorCode.INVALID_PROVIDER_ID, '"OIDCAuthProviderConfig.providerId" must be a valid non-empty string prefixed with "oidc.".', diff --git a/test/unit/auth/auth-api-request.spec.ts b/test/unit/auth/auth-api-request.spec.ts index 8527049117..82f82e687c 100644 --- a/test/unit/auth/auth-api-request.spec.ts +++ b/test/unit/auth/auth-api-request.spec.ts @@ -910,8 +910,8 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { if (!handler.supportsTenantManagement) { const invalidTenantIds = [ - null, NaN, 0, 1, true, false, '', '.', '..', '...', 'victim.', '..\n', - '../other', 'tenant/1', 'tenant\\1', 'tenant?foo=bar', 'tenant#frag', 'tenant%2e%2e', + null, NaN, 0, 1, true, false, '', '.', '..', '...', 'tenant.', '..\n', + 'tenant/1', 'tenant\\1', 'tenant?foo=bar', 'tenant#frag', 'tenant%2f1', ]; invalidTenantIds.forEach((invalidTenantId) => { it('should throw given invalid tenantId: ' + JSON.stringify(invalidTenantId), () => { @@ -1576,8 +1576,8 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { if (handler.supportsTenantManagement) { it('should fail client-side validation when user records have malformed tenant IDs', () => { const invalidTenantIds = [ - '', '.', '..', '...', 'victim.', '..\n', '../other', 'tenant/1', - 'tenant\\1', 'tenant?foo=bar', 'tenant#frag', 'tenant%2e%2e', + '', '.', '..', '...', 'tenant.', '..\n', 'tenant/1', + 'tenant\\1', 'tenant?foo=bar', 'tenant#frag', 'tenant%2f1', ]; const testUsers: UserImportRecord[] = invalidTenantIds.map((invalidTenantId, i) => ({ uid: `user${i}`, @@ -3425,8 +3425,8 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { const invalidProviderIds = [ null, NaN, 0, 1, true, false, '', 'oidc.', 'oidc..', 'oidc.provider.', 'oidc.\n', - '\uFEFFoidc.provider', 'oidc.provider/../other', 'oidc.provider?foo=bar', - 'oidc.provider#frag', 'oidc.provider%2fother', 'saml.provider', ['oidc.provider'], [], {}, { a: 1 }, _.noop]; + '\uFEFFoidc.provider', 'oidc.provider/1', 'oidc.provider?foo=bar', + 'oidc.provider#frag', 'oidc.provider%2f1', 'saml.provider', ['oidc.provider'], [], {}, { a: 1 }, _.noop]; invalidProviderIds.forEach((invalidProviderId) => { it('should be rejected given an invalid provider ID:' + JSON.stringify(invalidProviderId), () => { const expectedError = new FirebaseAuthError(authClientErrorCode.INVALID_PROVIDER_ID); @@ -3604,8 +3604,8 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { const invalidProviderIds = [ null, NaN, 0, 1, true, false, '', 'oidc.', 'oidc..', 'oidc.provider.', 'oidc.\n', - '\uFEFFoidc.provider', 'oidc.provider/../other', 'oidc.provider?foo=bar', - 'oidc.provider#frag', 'oidc.provider%2fother', 'saml.provider', ['oidc.provider'], [], {}, { a: 1 }, _.noop]; + '\uFEFFoidc.provider', 'oidc.provider/1', 'oidc.provider?foo=bar', + 'oidc.provider#frag', 'oidc.provider%2f1', 'saml.provider', ['oidc.provider'], [], {}, { a: 1 }, _.noop]; invalidProviderIds.forEach((invalidProviderId) => { it('should be rejected given an invalid provider ID:' + JSON.stringify(invalidProviderId), () => { const expectedError = new FirebaseAuthError(authClientErrorCode.INVALID_PROVIDER_ID); @@ -3896,8 +3896,8 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { const invalidProviderIds = [ null, NaN, 0, 1, true, false, '', 'oidc.', 'oidc..', 'oidc.provider.', 'oidc.\n', - '\uFEFFoidc.provider', 'oidc.provider/../other', 'oidc.provider?foo=bar', - 'oidc.provider#frag', 'oidc.provider%2fother', 'saml.provider', ['oidc.provider'], [], {}, { a: 1 }, _.noop]; + '\uFEFFoidc.provider', 'oidc.provider/1', 'oidc.provider?foo=bar', + 'oidc.provider#frag', 'oidc.provider%2f1', 'saml.provider', ['oidc.provider'], [], {}, { a: 1 }, _.noop]; invalidProviderIds.forEach((invalidProviderId) => { it('should be rejected given an invalid provider ID:' + JSON.stringify(invalidProviderId), () => { const expectedError = new FirebaseAuthError(authClientErrorCode.INVALID_PROVIDER_ID); @@ -3995,8 +3995,8 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { const invalidProviderIds = [ null, NaN, 0, 1, true, false, '', 'saml.', 'saml..', 'saml.provider.', 'saml.\n', - '\uFEFFsaml.provider', 'saml.provider/../other', 'saml.provider?foo=bar', - 'saml.provider#frag', 'saml.provider%2fother', 'oidc.provider', ['saml.provider'], [], {}, { a: 1 }, _.noop]; + '\uFEFFsaml.provider', 'saml.provider/1', 'saml.provider?foo=bar', + 'saml.provider#frag', 'saml.provider%2f1', 'oidc.provider', ['saml.provider'], [], {}, { a: 1 }, _.noop]; invalidProviderIds.forEach((invalidProviderId) => { it('should be rejected given an invalid provider ID:' + JSON.stringify(invalidProviderId), () => { const expectedError = new FirebaseAuthError(authClientErrorCode.INVALID_PROVIDER_ID); @@ -4168,8 +4168,8 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { const invalidProviderIds = [ null, NaN, 0, 1, true, false, '', 'saml.', 'saml..', 'saml.provider.', 'saml.\n', - '\uFEFFsaml.provider', 'saml.provider/../other', 'saml.provider?foo=bar', - 'saml.provider#frag', 'saml.provider%2fother', 'oidc.provider', ['saml.provider'], [], {}, { a: 1 }, _.noop]; + '\uFEFFsaml.provider', 'saml.provider/1', 'saml.provider?foo=bar', + 'saml.provider#frag', 'saml.provider%2f1', 'oidc.provider', ['saml.provider'], [], {}, { a: 1 }, _.noop]; invalidProviderIds.forEach((invalidProviderId) => { it('should be rejected given an invalid provider ID:' + JSON.stringify(invalidProviderId), () => { const expectedError = new FirebaseAuthError(authClientErrorCode.INVALID_PROVIDER_ID); @@ -4446,8 +4446,8 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { const invalidProviderIds = [ null, NaN, 0, 1, true, false, '', 'saml.', 'saml..', 'saml.provider.', 'saml.\n', - '\uFEFFsaml.provider', 'saml.provider/../other', 'saml.provider?foo=bar', - 'saml.provider#frag', 'saml.provider%2fother', 'oidc.provider', ['saml.provider'], [], {}, { a: 1 }, _.noop]; + '\uFEFFsaml.provider', 'saml.provider/1', 'saml.provider?foo=bar', + 'saml.provider#frag', 'saml.provider%2f1', 'oidc.provider', ['saml.provider'], [], {}, { a: 1 }, _.noop]; invalidProviderIds.forEach((invalidProviderId) => { it('should be rejected given an invalid provider ID:' + JSON.stringify(invalidProviderId), () => { const expectedError = new FirebaseAuthError(authClientErrorCode.INVALID_PROVIDER_ID); @@ -4669,8 +4669,8 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { }); const invalidTenantIds = [ - null, NaN, 0, 1, true, false, '', '.', '..', '...', 'victim.', '..\n', - '../other', 'tenant/1', 'tenant\\1', 'tenant?foo=bar', 'tenant#frag', 'tenant%2e%2e', + null, NaN, 0, 1, true, false, '', '.', '..', '...', 'tenant.', '..\n', + 'tenant/1', 'tenant\\1', 'tenant?foo=bar', 'tenant#frag', 'tenant%2f1', ['tenant-id'], [], {}, { a: 1 }, _.noop]; invalidTenantIds.forEach((invalidTenantId) => { it('should be rejected given an invalid tenant ID:' + JSON.stringify(invalidTenantId), () => { @@ -4842,8 +4842,8 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { }); const invalidTenantIds = [ - null, NaN, 0, 1, true, false, '', '.', '..', '...', 'victim.', '..\n', - '../other', 'tenant/1', 'tenant\\1', 'tenant?foo=bar', 'tenant#frag', 'tenant%2e%2e', + null, NaN, 0, 1, true, false, '', '.', '..', '...', 'tenant.', '..\n', + 'tenant/1', 'tenant\\1', 'tenant?foo=bar', 'tenant#frag', 'tenant%2f1', ['tenant-id'], [], {}, { a: 1 }, _.noop]; invalidTenantIds.forEach((invalidTenantId) => { it('should be rejected given an invalid tenant ID:' + JSON.stringify(invalidTenantId), () => { @@ -5099,8 +5099,8 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { }); const invalidTenantIds = [ - null, NaN, 0, 1, true, false, '', '.', '..', '...', 'victim.', '..\n', - '../other', 'tenant/1', 'tenant\\1', 'tenant?foo=bar', 'tenant#frag', 'tenant%2e%2e', + null, NaN, 0, 1, true, false, '', '.', '..', '...', 'tenant.', '..\n', + 'tenant/1', 'tenant\\1', 'tenant?foo=bar', 'tenant#frag', 'tenant%2f1', ['tenant-id'], [], {}, { a: 1 }, _.noop]; invalidTenantIds.forEach((invalidTenantId) => { it('should be rejected given an invalid tenant ID:' + JSON.stringify(invalidTenantId), () => { diff --git a/test/unit/auth/auth-config.spec.ts b/test/unit/auth/auth-config.spec.ts index 1436630084..c8233c8d89 100644 --- a/test/unit/auth/auth-config.spec.ts +++ b/test/unit/auth/auth-config.spec.ts @@ -660,9 +660,9 @@ describe('SAMLConfig', () => { const invalidProviderIds = [ null, NaN, 0, 1, true, false, '', 'incorrectsaml.', 'saml', 'saml.', 'saml..', 'saml.provider.', - 'saml.\n', 'saml.provider\n', '\uFEFFsaml.provider', 'saml../other', 'saml.provider/../other', - 'saml.provider/other', 'saml.provider\\other', 'saml.provider?foo=bar', 'saml.provider#frag', - 'saml.provider%2fother', 'oidc.provider', 'other', [], [1, 'a'], + 'saml.\n', 'saml.provider\n', '\uFEFFsaml.provider', 'saml.provider/1', + 'saml.provider\\1', 'saml.provider?foo=bar', 'saml.provider#frag', + 'saml.provider%2f1', 'oidc.provider', 'other', [], [1, 'a'], {}, { a: 1 }, _.noop]; invalidProviderIds.forEach((invalidProviderId) => { it(`should return false on invalid SAML provider ID "${JSON.stringify(invalidProviderId)}"`, () => { @@ -791,8 +791,8 @@ describe('SAMLConfig', () => { const invalidProviderIds = [ null, NaN, 0, 1, true, false, '', 'saml.', 'saml..', 'saml.provider.', 'saml.\n', - '\uFEFFsaml.provider', 'saml.provider/../other', 'saml.provider?foo=bar', - 'saml.provider#frag', 'saml.provider%2fother', 'oidc.provider', 'other', + '\uFEFFsaml.provider', 'saml.provider/1', 'saml.provider?foo=bar', + 'saml.provider#frag', 'saml.provider%2f1', 'oidc.provider', 'other', [], [1, 'a'], {}, { a: 1 }, _.noop]; invalidProviderIds.forEach((providerId) => { it('should throw on invalid providerId:' + JSON.stringify(providerId), () => { @@ -1011,9 +1011,9 @@ describe('OIDCConfig', () => { const invalidProviderIds = [ null, NaN, 0, 1, true, false, '', 'incorrectoidc.', 'oidc', 'oidc.', 'oidc..', 'oidc.provider.', - 'oidc.\n', 'oidc.provider\n', '\uFEFFoidc.provider', 'oidc../other', 'oidc.provider/../other', - 'oidc.provider/other', 'oidc.provider\\other', 'oidc.provider?foo=bar', 'oidc.provider#frag', - 'oidc.provider%2fother', 'saml.provider', 'other', [], [1, 'a'], + 'oidc.\n', 'oidc.provider\n', '\uFEFFoidc.provider', 'oidc.provider/1', + 'oidc.provider\\1', 'oidc.provider?foo=bar', 'oidc.provider#frag', + 'oidc.provider%2f1', 'saml.provider', 'other', [], [1, 'a'], {}, { a: 1 }, _.noop]; invalidProviderIds.forEach((invalidProviderId) => { it(`should return false on invalid OIDC provider ID "${JSON.stringify(invalidProviderId)}"`, () => { @@ -1178,8 +1178,8 @@ describe('OIDCConfig', () => { const invalidProviderIds = [ null, NaN, 0, 1, true, false, '', 'oidc.', 'oidc..', 'oidc.provider.', 'oidc.\n', - '\uFEFFoidc.provider', 'oidc.provider/../other', 'oidc.provider?foo=bar', - 'oidc.provider#frag', 'oidc.provider%2fother', 'other', 'saml.provider', + '\uFEFFoidc.provider', 'oidc.provider/1', 'oidc.provider?foo=bar', + 'oidc.provider#frag', 'oidc.provider%2f1', 'other', 'saml.provider', [], [1, 'a'], {}, { a: 1 }, _.noop]; invalidProviderIds.forEach((providerId) => { it('should throw on invalid providerId:' + JSON.stringify(providerId), () => { diff --git a/test/unit/auth/tenant-manager.spec.ts b/test/unit/auth/tenant-manager.spec.ts index f1ed258683..6f8683075d 100644 --- a/test/unit/auth/tenant-manager.spec.ts +++ b/test/unit/auth/tenant-manager.spec.ts @@ -70,8 +70,8 @@ describe('TenantManager', () => { }); const MALFORMED_TENANT_IDS = [ - '.', '..', '...', 'victim.', '..\n', '../other', 'tenant/1', 'tenant\\1', - 'tenant?foo=bar', 'tenant#frag', 'tenant%2e%2e', + '.', '..', '...', 'tenant.', '..\n', 'tenant/1', 'tenant\\1', + 'tenant?foo=bar', 'tenant#frag', 'tenant%2f1', ]; describe('authForTenant()', () => { diff --git a/test/unit/utils/validator.spec.ts b/test/unit/utils/validator.spec.ts index fc692325ef..9a8c9886bd 100644 --- a/test/unit/utils/validator.spec.ts +++ b/test/unit/utils/validator.spec.ts @@ -539,8 +539,8 @@ describe('isTenantId()', () => { const invalidTenantIds = [ null, NaN, 0, 1, true, false, '', [], ['a'], {}, { a: 1 }, _.noop, - '.', '..', '...', 'victim.', '..\n', 'tenant1\n', '\uFEFFtenant1', '../other', 'tenant/other', - 'tenant\\other', 'tenant?foo=bar', 'tenant#frag', 'tenant%2e%2e', 'tenant%2fother', + '.', '..', '...', 'tenant.', '..\n', 'tenant1\n', '\uFEFFtenant1', 'tenant/1', + 'tenant\\1', 'tenant?foo=bar', 'tenant#frag', 'tenant%2f1', 'tenant id', ]; invalidTenantIds.forEach((invalidTenantId) => { From b719c03fd7a1059970c1adb34cd23b860d45efb0 Mon Sep 17 00:00:00 2001 From: Matt Castelaz Date: Wed, 21 Oct 2026 15:45:16 +0000 Subject: [PATCH 5/5] fix(auth): extract isSafeUrlSegment helper in validator --- src/auth/auth-config.ts | 14 ++++---------- src/utils/validator.ts | 21 ++++++++++++++++----- test/unit/utils/validator.spec.ts | 27 ++++++++++++++++++++++++++- 3 files changed, 46 insertions(+), 16 deletions(-) diff --git a/src/auth/auth-config.ts b/src/auth/auth-config.ts index 037e58a2bc..4aca54b618 100644 --- a/src/auth/auth-config.ts +++ b/src/auth/auth-config.ts @@ -1178,12 +1178,9 @@ export class SAMLConfig implements SAMLAuthProviderConfig { * @returns Whether the provider ID corresponds to a SAML provider. */ public static isProviderId(providerId: any): providerId is string { - // Single dots are permitted within the provider ID suffix (e.g., "saml.my.provider"), - // so we check !providerId.endsWith('.') separately to reject "saml.", "saml..", and - // trailing-dot segments without introducing multi-quantifier regex backtracking. return validator.isNonEmptyString(providerId) && - !providerId.endsWith('.') && - /^saml\.[a-zA-Z0-9_.~-]+$/.test(providerId); + providerId.startsWith('saml.') && + validator.isSafeUrlSegment(providerId); } /** @@ -1427,12 +1424,9 @@ export class OIDCConfig implements OIDCAuthProviderConfig { * @returns Whether the provider ID corresponds to an OIDC provider. */ public static isProviderId(providerId: any): providerId is string { - // Single dots are permitted within the provider ID suffix (e.g., "oidc.my.provider"), - // so we check !providerId.endsWith('.') separately to reject "oidc.", "oidc..", and - // trailing-dot segments without introducing multi-quantifier regex backtracking. return validator.isNonEmptyString(providerId) && - !providerId.endsWith('.') && - /^oidc\.[a-zA-Z0-9_.~-]+$/.test(providerId); + providerId.startsWith('oidc.') && + validator.isSafeUrlSegment(providerId); } /** diff --git a/src/utils/validator.ts b/src/utils/validator.ts index e58c938a76..3ed8c998a1 100644 --- a/src/utils/validator.ts +++ b/src/utils/validator.ts @@ -298,6 +298,21 @@ export function isTaskId(taskId: any): boolean { return VALID_TASK_ID_REGEX.test(taskId); } +/** + * Validates that a string is a safe single URL path segment containing only + * unreserved URI characters (RFC 3986) and not ending with a dot. + * + * @param segment - The string to validate. + * @returns Whether the string is a safe URL path segment. + */ +export function isSafeUrlSegment(segment: any): segment is string { + // Reject trailing dots separately to forbid ".", "..", and trailing-dot segments + // without introducing multi-quantifier regex backtracking. + return isNonEmptyString(segment) && + !segment.endsWith('.') && + /^[a-zA-Z0-9_.~-]+$/.test(segment); +} + /** * Validates that a string is a valid tenant ID. * @@ -305,9 +320,5 @@ export function isTaskId(taskId: any): boolean { * @returns Whether the string is a valid tenant ID. */ export function isTenantId(tenantId: any): tenantId is string { - // Reject trailing dots separately to forbid ".", "..", and trailing-dot segments - // without introducing multi-quantifier regex backtracking. - return isNonEmptyString(tenantId) && - !tenantId.endsWith('.') && - /^[a-zA-Z0-9_.~-]+$/.test(tenantId); + return isSafeUrlSegment(tenantId); } diff --git a/test/unit/utils/validator.spec.ts b/test/unit/utils/validator.spec.ts index 9a8c9886bd..6f96f4061f 100644 --- a/test/unit/utils/validator.spec.ts +++ b/test/unit/utils/validator.spec.ts @@ -24,7 +24,7 @@ import chaiAsPromised from 'chai-as-promised'; import { isArray, isNonEmptyArray, isBoolean, isNumber, isString, isNonEmptyString, isNonNullObject, isEmail, isPassword, isURL, isUid, isPhoneNumber, isObject, isBuffer, - isUTCDateString, isISODateString, isTenantId, + isUTCDateString, isISODateString, isSafeUrlSegment, isTenantId, } from '../../../src/utils/validator'; @@ -532,6 +532,31 @@ describe('isISODateString()', () => { }); }); +describe('isSafeUrlSegment()', () => { + it('should return false given no argument', () => { + expect(isSafeUrlSegment(undefined as any)).to.be.false; + }); + + const invalidSegments = [ + null, NaN, 0, 1, true, false, '', [], ['a'], {}, { a: 1 }, _.noop, + '.', '..', '...', 'segment.', '..\n', 'segment1\n', '\uFEFFsegment1', 'segment/1', + 'segment\\1', 'segment?foo=bar', 'segment#frag', 'segment%2f1', + 'segment id', + ]; + invalidSegments.forEach((invalidSegment) => { + it('should return false given an invalid URL segment: ' + JSON.stringify(invalidSegment), () => { + expect(isSafeUrlSegment(invalidSegment)).to.be.false; + }); + }); + + const validSegments = ['segment1', 'segment-1', 'SEGMENT_1', 'segment.1~foo']; + validSegments.forEach((validSegment) => { + it('should return true given a valid URL segment: ' + JSON.stringify(validSegment), () => { + expect(isSafeUrlSegment(validSegment)).to.be.true; + }); + }); +}); + describe('isTenantId()', () => { it('should return false given no argument', () => { expect(isTenantId(undefined as any)).to.be.false;