From 93bcbc0485a1a079f242bd99420f6f308607e3a2 Mon Sep 17 00:00:00 2001 From: Chris Date: Fri, 24 Apr 2026 13:15:58 +0300 Subject: [PATCH] fix(oidc): normalize OIDC_SCOPE to handle whitespace issues (#1060) Adds automatic scope normalization to prevent URL encoding issues when OIDC_SCOPE contains extra whitespace, tabs, or newlines. This addresses issue #1056 where spaces in the scope value could cause authentication failures in some environments. Changes: - Added normalizeScope() function to trim and collapse whitespace - Automatically adds 'openid' scope if missing with warning - Updated both single and multi-provider configurations - Added comprehensive tests for scope normalization edge cases - Added service tests to verify authorization URL construction - Updated documentation with scope formatting guidance Fixes #1056 --- backend/modules/oidc/providerConfig.js | 21 ++- .../unit/modules/oidc/providerConfig.test.js | 80 ++++++++ .../tests/unit/modules/oidc/service.test.js | 178 ++++++++++++++++++ docs/10-oidc-sso.md | 19 +- 4 files changed, 293 insertions(+), 5 deletions(-) create mode 100644 backend/tests/unit/modules/oidc/service.test.js diff --git a/backend/modules/oidc/providerConfig.js b/backend/modules/oidc/providerConfig.js index 69b0751..1113c34 100644 --- a/backend/modules/oidc/providerConfig.js +++ b/backend/modules/oidc/providerConfig.js @@ -6,6 +6,21 @@ function parseCommaSeparated(value) { .filter(Boolean); } +function normalizeScope(scope) { + if (!scope) return 'openid profile email'; + + const normalized = scope.trim().split(/\s+/).filter(Boolean).join(' '); + + if (!normalized.includes('openid')) { + console.warn( + `OIDC scope does not include 'openid'. Adding it automatically. Original scope: "${scope}"` + ); + return `openid ${normalized}`; + } + + return normalized; +} + function loadProvidersFromEnv() { if (process.env.OIDC_ENABLED !== 'true') { console.log( @@ -24,9 +39,7 @@ function loadProvidersFromEnv() { issuer: process.env[`OIDC_PROVIDER_${i}_ISSUER`], clientId: process.env[`OIDC_PROVIDER_${i}_CLIENT_ID`], clientSecret: process.env[`OIDC_PROVIDER_${i}_CLIENT_SECRET`], - scope: - process.env[`OIDC_PROVIDER_${i}_SCOPE`] || - 'openid profile email', + scope: normalizeScope(process.env[`OIDC_PROVIDER_${i}_SCOPE`]), autoProvision: process.env[`OIDC_PROVIDER_${i}_AUTO_PROVISION`] !== 'false', adminEmailDomains: parseCommaSeparated( @@ -63,7 +76,7 @@ function loadProvidersFromEnv() { issuer: process.env.OIDC_ISSUER_URL, clientId: process.env.OIDC_CLIENT_ID, clientSecret: process.env.OIDC_CLIENT_SECRET, - scope: process.env.OIDC_SCOPE || 'openid profile email', + scope: normalizeScope(process.env.OIDC_SCOPE), autoProvision: process.env.OIDC_AUTO_PROVISION !== 'false', adminEmailDomains: parseCommaSeparated( process.env.OIDC_ADMIN_EMAIL_DOMAINS diff --git a/backend/tests/unit/modules/oidc/providerConfig.test.js b/backend/tests/unit/modules/oidc/providerConfig.test.js index af01a60..763e305 100644 --- a/backend/tests/unit/modules/oidc/providerConfig.test.js +++ b/backend/tests/unit/modules/oidc/providerConfig.test.js @@ -86,6 +86,60 @@ describe('OIDC Provider Configuration', () => { expect(provider.scope).toBe('openid profile email groups'); }); + it('should normalize scope with extra whitespace', () => { + process.env.OIDC_ENABLED = 'true'; + process.env.OIDC_PROVIDER_NAME = 'Test'; + process.env.OIDC_PROVIDER_SLUG = 'test'; + process.env.OIDC_ISSUER_URL = 'https://auth.example.com'; + process.env.OIDC_CLIENT_ID = 'test-id'; + process.env.OIDC_CLIENT_SECRET = 'test-secret'; + process.env.OIDC_SCOPE = ' openid profile email '; + + providerConfig.reloadProviders(); + const provider = providerConfig.getProvider('test'); + + expect(provider.scope).toBe('openid profile email'); + }); + + it('should add openid scope if missing', () => { + const consoleWarnSpy = jest + .spyOn(console, 'warn') + .mockImplementation(); + + process.env.OIDC_ENABLED = 'true'; + process.env.OIDC_PROVIDER_NAME = 'Test'; + process.env.OIDC_PROVIDER_SLUG = 'test'; + process.env.OIDC_ISSUER_URL = 'https://auth.example.com'; + process.env.OIDC_CLIENT_ID = 'test-id'; + process.env.OIDC_CLIENT_SECRET = 'test-secret'; + process.env.OIDC_SCOPE = 'profile email'; + + providerConfig.reloadProviders(); + const provider = providerConfig.getProvider('test'); + + expect(provider.scope).toBe('openid profile email'); + expect(consoleWarnSpy).toHaveBeenCalledWith( + expect.stringContaining("does not include 'openid'") + ); + + consoleWarnSpy.mockRestore(); + }); + + it('should handle scope with tabs and newlines', () => { + process.env.OIDC_ENABLED = 'true'; + process.env.OIDC_PROVIDER_NAME = 'Test'; + process.env.OIDC_PROVIDER_SLUG = 'test'; + process.env.OIDC_ISSUER_URL = 'https://auth.example.com'; + process.env.OIDC_CLIENT_ID = 'test-id'; + process.env.OIDC_CLIENT_SECRET = 'test-secret'; + process.env.OIDC_SCOPE = 'openid\tprofile\nemail'; + + providerConfig.reloadProviders(); + const provider = providerConfig.getProvider('test'); + + expect(provider.scope).toBe('openid profile email'); + }); + it('should parse admin email domains', () => { process.env.OIDC_ENABLED = 'true'; process.env.OIDC_PROVIDER_NAME = 'Google'; @@ -227,6 +281,32 @@ describe('OIDC Provider Configuration', () => { expect(corp.autoProvision).toBe(false); expect(corp.adminEmailDomains).toEqual(['corp.com']); }); + + it('should normalize scopes in multi-provider configuration', () => { + process.env.OIDC_ENABLED = 'true'; + + process.env.OIDC_PROVIDER_1_NAME = 'Google'; + process.env.OIDC_PROVIDER_1_SLUG = 'google'; + process.env.OIDC_PROVIDER_1_ISSUER = 'https://accounts.google.com'; + process.env.OIDC_PROVIDER_1_CLIENT_ID = 'google-id'; + process.env.OIDC_PROVIDER_1_CLIENT_SECRET = 'google-secret'; + process.env.OIDC_PROVIDER_1_SCOPE = ' openid profile email '; + + process.env.OIDC_PROVIDER_2_NAME = 'Okta'; + process.env.OIDC_PROVIDER_2_SLUG = 'okta'; + process.env.OIDC_PROVIDER_2_ISSUER = 'https://company.okta.com'; + process.env.OIDC_PROVIDER_2_CLIENT_ID = 'okta-id'; + process.env.OIDC_PROVIDER_2_CLIENT_SECRET = 'okta-secret'; + process.env.OIDC_PROVIDER_2_SCOPE = 'openid profile email groups'; + + providerConfig.reloadProviders(); + + const google = providerConfig.getProvider('google'); + const okta = providerConfig.getProvider('okta'); + + expect(google.scope).toBe('openid profile email'); + expect(okta.scope).toBe('openid profile email groups'); + }); }); describe('getProvider', () => { diff --git a/backend/tests/unit/modules/oidc/service.test.js b/backend/tests/unit/modules/oidc/service.test.js new file mode 100644 index 0000000..987d77d --- /dev/null +++ b/backend/tests/unit/modules/oidc/service.test.js @@ -0,0 +1,178 @@ +const { Issuer } = require('openid-client'); +const oidcService = require('../../../../modules/oidc/service'); +const providerConfig = require('../../../../modules/oidc/providerConfig'); +const stateManager = require('../../../../modules/oidc/stateManager'); + +jest.mock('../../../../modules/oidc/providerConfig'); +jest.mock('../../../../modules/oidc/stateManager'); + +describe('OIDC Service - Authorization URL Construction', () => { + let originalEnv; + let mockIssuer; + let mockClient; + + beforeEach(() => { + originalEnv = { ...process.env }; + process.env.BASE_URL = 'https://todo.example.com'; + + mockClient = { + authorizationUrl: jest.fn(), + callback: jest.fn(), + }; + + mockIssuer = { + Client: jest.fn(() => mockClient), + metadata: { + issuer: 'https://auth.example.com', + authorization_endpoint: 'https://auth.example.com/authorize', + token_endpoint: 'https://auth.example.com/token', + }, + }; + + jest.spyOn(Issuer, 'discover').mockResolvedValue(mockIssuer); + + oidcService.clearIssuerCache(); + }); + + afterEach(() => { + process.env = originalEnv; + jest.restoreAllMocks(); + oidcService.clearIssuerCache(); + }); + + describe('initiateAuthFlow with scope containing spaces', () => { + it('should properly encode scope with spaces in authorization URL', async () => { + const mockProvider = { + slug: 'test-provider', + name: 'Test Provider', + issuer: 'https://auth.example.com', + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + scope: 'openid profile email', + }; + + providerConfig.getProvider.mockReturnValue(mockProvider); + + stateManager.createState.mockResolvedValue({ + state: 'test-state-123', + nonce: 'test-nonce-456', + }); + + const mockAuthUrl = + 'https://auth.example.com/authorize?client_id=test-client-id&scope=openid%20profile%20email&response_type=code&redirect_uri=https%3A%2F%2Ftodo.example.com%2Fapi%2Foidc%2Fcallback%2Ftest-provider&state=test-state-123&nonce=test-nonce-456'; + + mockClient.authorizationUrl.mockReturnValue(mockAuthUrl); + + const result = await oidcService.initiateAuthFlow('test-provider'); + + expect(mockClient.authorizationUrl).toHaveBeenCalledWith({ + scope: 'openid profile email', + state: 'test-state-123', + nonce: 'test-nonce-456', + }); + + expect(result.authUrl).toContain('scope=openid%20profile%20email'); + + expect(result.authUrl).not.toContain('scope=openid profile email'); + }); + + it('should handle scope with plus signs correctly', async () => { + const mockProvider = { + slug: 'test-provider', + name: 'Test Provider', + issuer: 'https://auth.example.com', + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + scope: 'openid+profile+email', + }; + + providerConfig.getProvider.mockReturnValue(mockProvider); + + stateManager.createState.mockResolvedValue({ + state: 'test-state-123', + nonce: 'test-nonce-456', + }); + + const mockAuthUrl = + 'https://auth.example.com/authorize?client_id=test-client-id&scope=openid%2Bprofile%2Bemail&response_type=code&redirect_uri=https%3A%2F%2Ftodo.example.com%2Fapi%2Foidc%2Fcallback%2Ftest-provider&state=test-state-123&nonce=test-nonce-456'; + + mockClient.authorizationUrl.mockReturnValue(mockAuthUrl); + + const result = await oidcService.initiateAuthFlow('test-provider'); + + expect(mockClient.authorizationUrl).toHaveBeenCalledWith({ + scope: 'openid+profile+email', + state: 'test-state-123', + nonce: 'test-nonce-456', + }); + + expect(result.authUrl).toBeDefined(); + }); + + it('should handle custom scopes with spaces', async () => { + const mockProvider = { + slug: 'test-provider', + name: 'Test Provider', + issuer: 'https://auth.example.com', + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + scope: 'openid profile email groups offline_access', + }; + + providerConfig.getProvider.mockReturnValue(mockProvider); + + stateManager.createState.mockResolvedValue({ + state: 'test-state-123', + nonce: 'test-nonce-456', + }); + + const mockAuthUrl = + 'https://auth.example.com/authorize?client_id=test-client-id&scope=openid%20profile%20email%20groups%20offline_access&response_type=code&redirect_uri=https%3A%2F%2Ftodo.example.com%2Fapi%2Foidc%2Fcallback%2Ftest-provider&state=test-state-123&nonce=test-nonce-456'; + + mockClient.authorizationUrl.mockReturnValue(mockAuthUrl); + + const result = await oidcService.initiateAuthFlow('test-provider'); + + expect(mockClient.authorizationUrl).toHaveBeenCalledWith({ + scope: 'openid profile email groups offline_access', + state: 'test-state-123', + nonce: 'test-nonce-456', + }); + + expect(result.authUrl).toContain( + 'scope=openid%20profile%20email%20groups%20offline_access' + ); + }); + }); + + describe('edge cases', () => { + it('should handle scope with leading/trailing spaces', async () => { + const mockProvider = { + slug: 'test-provider', + name: 'Test Provider', + issuer: 'https://auth.example.com', + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + scope: ' openid profile email ', + }; + + providerConfig.getProvider.mockReturnValue(mockProvider); + + stateManager.createState.mockResolvedValue({ + state: 'test-state-123', + nonce: 'test-nonce-456', + }); + + mockClient.authorizationUrl.mockReturnValue( + 'https://auth.example.com/authorize?scope=openid%20profile%20email' + ); + + const result = await oidcService.initiateAuthFlow('test-provider'); + + const scopeArgument = + mockClient.authorizationUrl.mock.calls[0][0].scope; + + expect(scopeArgument.trim()).toBe('openid profile email'); + }); + }); +}); diff --git a/docs/10-oidc-sso.md b/docs/10-oidc-sso.md index 4c706c8..0fdab61 100644 --- a/docs/10-oidc-sso.md +++ b/docs/10-oidc-sso.md @@ -169,11 +169,28 @@ OIDC_PROVIDER_3_AUTO_PROVISION=true | `OIDC_ISSUER_URL` | Yes | - | OIDC discovery endpoint | | `OIDC_CLIENT_ID` | Yes | - | OAuth client ID | | `OIDC_CLIENT_SECRET` | Yes | - | OAuth client secret | -| `OIDC_SCOPE` | No | `openid profile email` | OAuth scopes | +| `OIDC_SCOPE` | No | `openid profile email` | OAuth scopes (space-separated) | | `OIDC_AUTO_PROVISION` | No | `true` | Auto-create users on first login | | `OIDC_ADMIN_EMAIL_DOMAINS` | No | - | Comma-separated domains for auto-admin | | `BASE_URL` | Yes | - | Tududi base URL (for OAuth callbacks) | +**Scope Formatting:** + +The `OIDC_SCOPE` parameter accepts space-separated OAuth scopes. Tududi automatically normalizes the scope value by: +- Trimming leading/trailing whitespace +- Collapsing multiple spaces into single spaces +- Ensuring `openid` is always included (adding it if missing) +- Properly URL-encoding the scope in authorization requests + +Examples of valid scope formats: +```bash +OIDC_SCOPE=openid profile email +OIDC_SCOPE="openid profile email groups" +OIDC_SCOPE=openid profile email # Extra spaces are automatically normalized +``` + +**Note:** Do not manually URL-encode the scope value (e.g., using `%20` or `+`). Use regular spaces - Tududi handles the encoding automatically. + **Important:** The `BASE_URL` variable must be set for OAuth redirects to work: ```bash BASE_URL=http://localhost:3002 # Development