From 74fc3889b946e8f224e65ef8d3d44125404aa4fc Mon Sep 17 00:00:00 2001 From: Val <68596159+valya@users.noreply.github.com> Date: Wed, 23 Oct 2024 12:22:15 +0100 Subject: [PATCH] fix(core): Sanitise IdP provided information in SAML test pages (#11171) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: कारतोफ्फेलस्क्रिप्ट™ --- .../__tests__/saml.controller.ee.test.ts | 77 +++++++++++++++++++ .../src/sso/saml/routes/saml.controller.ee.ts | 25 +++--- .../saml/views/saml-connection-test-failed.ts | 42 ---------- .../views/saml-connection-test-success.ts | 33 -------- .../saml-connection-test-failed.handlebars | 30 ++++++++ .../saml-connection-test-success.handlebars | 27 +++++++ 6 files changed, 150 insertions(+), 84 deletions(-) create mode 100644 packages/cli/src/sso/saml/routes/__tests__/saml.controller.ee.test.ts delete mode 100644 packages/cli/src/sso/saml/views/saml-connection-test-failed.ts delete mode 100644 packages/cli/src/sso/saml/views/saml-connection-test-success.ts create mode 100644 packages/cli/templates/saml-connection-test-failed.handlebars create mode 100644 packages/cli/templates/saml-connection-test-success.handlebars diff --git a/packages/cli/src/sso/saml/routes/__tests__/saml.controller.ee.test.ts b/packages/cli/src/sso/saml/routes/__tests__/saml.controller.ee.test.ts new file mode 100644 index 0000000000..c4a33ed441 --- /dev/null +++ b/packages/cli/src/sso/saml/routes/__tests__/saml.controller.ee.test.ts @@ -0,0 +1,77 @@ +import { type Response } from 'express'; +import { mock } from 'jest-mock-extended'; + +import type { User } from '@/databases/entities/user'; +import { UrlService } from '@/services/url.service'; +import { mockInstance } from '@test/mocking'; + +import { SamlService } from '../../saml.service.ee'; +import { getServiceProviderConfigTestReturnUrl } from '../../service-provider.ee'; +import type { SamlConfiguration } from '../../types/requests'; +import type { SamlUserAttributes } from '../../types/saml-user-attributes'; +import { SamlController } from '../saml.controller.ee'; + +const urlService = mockInstance(UrlService); +urlService.getInstanceBaseUrl.mockReturnValue(''); +const samlService = mockInstance(SamlService); +const controller = new SamlController(mock(), samlService, mock(), mock()); + +const user = mock({ + id: '123', + password: 'password', + authIdentities: [], + role: 'global:owner', +}); + +const attributes: SamlUserAttributes = { + email: 'test@example.com', + firstName: 'Test', + lastName: 'User', + userPrincipalName: 'upn:test@example.com', +}; + +describe('Test views', () => { + test('Should render success with template', async () => { + const req = mock(); + const res = mock(); + + req.body.RelayState = getServiceProviderConfigTestReturnUrl(); + samlService.handleSamlLogin.mockResolvedValueOnce({ + authenticatedUser: user, + attributes, + onboardingRequired: false, + }); + + await controller.acsPost(req, res); + + expect(res.render).toBeCalledWith('saml-connection-test-success', attributes); + }); + + test('Should render failure with template', async () => { + const req = mock(); + const res = mock(); + + req.body.RelayState = getServiceProviderConfigTestReturnUrl(); + samlService.handleSamlLogin.mockResolvedValueOnce({ + authenticatedUser: undefined, + attributes, + onboardingRequired: false, + }); + + await controller.acsPost(req, res); + + expect(res.render).toBeCalledWith('saml-connection-test-failed', { message: '', attributes }); + }); + + test('Should render error with template', async () => { + const req = mock(); + const res = mock(); + + req.body.RelayState = getServiceProviderConfigTestReturnUrl(); + samlService.handleSamlLogin.mockRejectedValueOnce(new Error('Test Error')); + + await controller.acsPost(req, res); + + expect(res.render).toBeCalledWith('saml-connection-test-failed', { message: 'Test Error' }); + }); +}); diff --git a/packages/cli/src/sso/saml/routes/saml.controller.ee.ts b/packages/cli/src/sso/saml/routes/saml.controller.ee.ts index 77c8c19291..c7b954914b 100644 --- a/packages/cli/src/sso/saml/routes/saml.controller.ee.ts +++ b/packages/cli/src/sso/saml/routes/saml.controller.ee.ts @@ -10,6 +10,7 @@ import { AuthError } from '@/errors/response-errors/auth.error'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { EventService } from '@/events/event.service'; import { AuthenticatedRequest } from '@/requests'; +import { sendErrorResponse } from '@/response-helper'; import { UrlService } from '@/services/url.service'; import { @@ -26,8 +27,6 @@ import { import type { SamlLoginBinding } from '../types'; import { SamlConfiguration } from '../types/requests'; import { getInitSSOFormView } from '../views/init-sso-post'; -import { getSamlConnectionTestFailedView } from '../views/saml-connection-test-failed'; -import { getSamlConnectionTestSuccessView } from '../views/saml-connection-test-success'; @RestController('/sso/saml') export class SamlController { @@ -92,7 +91,7 @@ export class SamlController { /** * Assertion Consumer Service endpoint */ - @Get('/acs', { middlewares: [samlLicensedMiddleware], skipAuth: true }) + @Get('/acs', { middlewares: [samlLicensedMiddleware], skipAuth: true, usesTemplates: true }) async acsGet(req: SamlConfiguration.AcsRequest, res: express.Response) { return await this.acsHandler(req, res, 'redirect'); } @@ -100,7 +99,7 @@ export class SamlController { /** * Assertion Consumer Service endpoint */ - @Post('/acs', { middlewares: [samlLicensedMiddleware], skipAuth: true }) + @Post('/acs', { middlewares: [samlLicensedMiddleware], skipAuth: true, usesTemplates: true }) async acsPost(req: SamlConfiguration.AcsRequest, res: express.Response) { return await this.acsHandler(req, res, 'post'); } @@ -120,9 +119,12 @@ export class SamlController { // if RelayState is set to the test connection Url, this is a test connection if (isConnectionTestRequest(req)) { if (loginResult.authenticatedUser) { - return res.send(getSamlConnectionTestSuccessView(loginResult.attributes)); + return res.render('saml-connection-test-success', loginResult.attributes); } else { - return res.send(getSamlConnectionTestFailedView('', loginResult.attributes)); + return res.render('saml-connection-test-failed', { + message: '', + attributes: loginResult.attributes, + }); } } if (loginResult.authenticatedUser) { @@ -148,16 +150,21 @@ export class SamlController { userEmail: loginResult.attributes.email ?? 'unknown', authenticationMethod: 'saml', }); - throw new AuthError('SAML Authentication failed'); + // Need to manually send the error response since we're using templates + return sendErrorResponse(res, new AuthError('SAML Authentication failed')); } catch (error) { if (isConnectionTestRequest(req)) { - return res.send(getSamlConnectionTestFailedView((error as Error).message)); + return res.render('saml-connection-test-failed', { message: (error as Error).message }); } this.eventService.emit('user-login-failed', { userEmail: 'unknown', authenticationMethod: 'saml', }); - throw new AuthError('SAML Authentication failed: ' + (error as Error).message); + // Need to manually send the error response since we're using templates + return sendErrorResponse( + res, + new AuthError('SAML Authentication failed: ' + (error as Error).message), + ); } } diff --git a/packages/cli/src/sso/saml/views/saml-connection-test-failed.ts b/packages/cli/src/sso/saml/views/saml-connection-test-failed.ts deleted file mode 100644 index 4ce2a3e3ac..0000000000 --- a/packages/cli/src/sso/saml/views/saml-connection-test-failed.ts +++ /dev/null @@ -1,42 +0,0 @@ -import type { SamlUserAttributes } from '../types/saml-user-attributes'; - -export function getSamlConnectionTestFailedView( - message: string, - attributes?: SamlUserAttributes, -): string { - return ` - - - n8n - SAML Connection Test Result - - - -
-

SAML Connection Test failed

-

${message ?? 'A common issue could be that no email attribute is set'}

- -

- ${ - attributes - ? ` -

Here are the attributes returned by your SAML IdP:

-
    -
  • Email: ${attributes?.email ?? '(n/a)'}
    • -
  • First Name: ${attributes?.firstName ?? '(n/a)'}
    • -
  • Last Name: ${attributes?.lastName ?? '(n/a)'}
    • -
  • UPN: ${attributes?.userPrincipalName ?? '(n/a)'}
    • -
` - : '' - } -
- - - `; -} diff --git a/packages/cli/src/sso/saml/views/saml-connection-test-success.ts b/packages/cli/src/sso/saml/views/saml-connection-test-success.ts deleted file mode 100644 index f647527cd0..0000000000 --- a/packages/cli/src/sso/saml/views/saml-connection-test-success.ts +++ /dev/null @@ -1,33 +0,0 @@ -import type { SamlUserAttributes } from '../types/saml-user-attributes'; - -export function getSamlConnectionTestSuccessView(attributes: SamlUserAttributes): string { - return ` - - - n8n - SAML Connection Test Result - - - -
-

SAML Connection Test was successful

- -

-

Here are the attributes returned by your SAML IdP:

-
    -
  • Email: ${attributes.email ?? '(n/a)'}
    • -
  • First Name: ${attributes.firstName ?? '(n/a)'}
    • -
  • Last Name: ${attributes.lastName ?? '(n/a)'}
    • -
  • UPN: ${attributes.userPrincipalName ?? '(n/a)'}
    • -
-
- - - `; -} diff --git a/packages/cli/templates/saml-connection-test-failed.handlebars b/packages/cli/templates/saml-connection-test-failed.handlebars new file mode 100644 index 0000000000..f93cea5c2d --- /dev/null +++ b/packages/cli/templates/saml-connection-test-failed.handlebars @@ -0,0 +1,30 @@ + + + n8n - SAML Connection Test Result + + + +
+

SAML Connection Test failed

+

{{#if message}}{{message}}{{else}}A common issue could be that no email attribute is set{{/if}}

+ +

+ {{#with attributes}} +

Here are the attributes returned by your SAML IdP:

+
    +
  • Email: {{#if email}}{{email}}{{else}}(n/a){{/if}}
    • +
  • First Name: {{#if firstName}}{{firstName}}{{else}}(n/a){{/if}}
    • +
  • Last Name: {{#if lastName}}{{lastName}}{{else}}(n/a){{/if}}
    • +
  • UPN: {{#if userPrincipalName}}{{userPrincipalName}}{{else}}(n/a){{/if}}
    • + {{/with}} +
+
+ + diff --git a/packages/cli/templates/saml-connection-test-success.handlebars b/packages/cli/templates/saml-connection-test-success.handlebars new file mode 100644 index 0000000000..e65d29483d --- /dev/null +++ b/packages/cli/templates/saml-connection-test-success.handlebars @@ -0,0 +1,27 @@ + + + n8n - SAML Connection Test Result + + + +
+

SAML Connection Test was successful

+ +

+

Here are the attributes returned by your SAML IdP:

+
    +
  • Email: {{#if email}}{{email}}{{else}}(n/a){{/if}}
    • +
  • First Name: {{#if firstName}}{{firstName}}{{else}}(n/a){{/if}}
    • +
  • Last Name: {{#if lastName}}{{lastName}}{{else}}(n/a){{/if}}
    • +
  • UPN: {{#if userPrincipalName}}{{userPrincipalName}}{{else}}(n/a){{/if}}
    • +
+
+ +