From 667cb95730b15e7fff1ac22f74b5d1e853b4497d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Malfait?= Date: Fri, 29 May 2026 15:22:50 +0200 Subject: [PATCH] fix(sso): accept HTTP-POST binding and surface descriptive parser errors (#21051) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes https://github.com/twentyhq/twenty/issues/21044 ## Summary - Closes [#490](https://github.com/twentyhq/private-issues/issues/490) — JumpCloud customers (and anyone else whose IdP only advertises `HTTP-POST` for `SingleSignOnService`) could not upload their SAML metadata; the parser silently rejected them with a generic `Invalid file` toast. - The SAML IdP metadata parser now falls back to `HTTP-POST` when `HTTP-Redirect` is not advertised. Both are valid SAML 2.0 bindings. - The parser now returns a descriptive `reason` string (Zod issues + custom errors) instead of an opaque `error: unknown`, and the upload snack bar surfaces it so the customer can self-diagnose (e.g. `entityID: entityID is not a valid URL` if they forgot to fill in their IdP Entity ID). - Added unit tests for HTTP-POST-only metadata, HTTP-Redirect preference, and each descriptive-error path. ## Test plan - [x] `npx jest parseSAMLMetadataFromXMLFile --config=packages/twenty-front/jest.config.mjs` — 8/8 pass - [x] `npx oxlint -c packages/twenty-front/.oxlintrc.json` on changed files — clean - [x] `npx oxfmt --check` on changed files — clean - [ ] Manual: upload the customer's JumpCloud metadata (HTTP-POST only, placeholder `entityID`) and confirm the error now says `Invalid file: entityID: entityID is not a valid URL` instead of `Invalid file` - [ ] Manual: upload metadata with a real `entityID` and HTTP-POST-only binding, confirm the form populates correctly --- .../components/SSO/SettingsSSOSAMLForm.tsx | 4 +- .../parseSAMLMetadataFromXMLFile.test.ts | 96 ++++++++++++++++++- .../utils/parseSAMLMetadataFromXMLFile.ts | 73 +++++++++----- 3 files changed, 148 insertions(+), 25 deletions(-) diff --git a/packages/twenty-front/src/modules/settings/security/components/SSO/SettingsSSOSAMLForm.tsx b/packages/twenty-front/src/modules/settings/security/components/SSO/SettingsSSOSAMLForm.tsx index 6fbf0dc9e5..69ef61f992 100644 --- a/packages/twenty-front/src/modules/settings/security/components/SSO/SettingsSSOSAMLForm.tsx +++ b/packages/twenty-front/src/modules/settings/security/components/SSO/SettingsSSOSAMLForm.tsx @@ -69,9 +69,9 @@ export const SettingsSSOSAMLForm = () => { e.target.value = ''; if (!samlMetadataParsed.success) { return enqueueErrorSnackBar({ - message: t`Invalid File`, + message: t`Invalid file: ${samlMetadataParsed.reason}`, options: { - duration: 2000, + duration: 5000, }, }); } diff --git a/packages/twenty-front/src/modules/settings/security/utils/__tests__/parseSAMLMetadataFromXMLFile.test.ts b/packages/twenty-front/src/modules/settings/security/utils/__tests__/parseSAMLMetadataFromXMLFile.test.ts index e52225f94a..8d0c87fec1 100644 --- a/packages/twenty-front/src/modules/settings/security/utils/__tests__/parseSAMLMetadataFromXMLFile.test.ts +++ b/packages/twenty-front/src/modules/settings/security/utils/__tests__/parseSAMLMetadataFromXMLFile.test.ts @@ -53,12 +53,106 @@ describe('parseSAMLMetadataFromXMLFile', () => { }, }); }); + it('should fall back to HTTP-POST binding when HTTP-Redirect is not advertised', () => { + const xmlString = ` + + + + + test + + + + + +`; + const result = parseSAMLMetadataFromXMLFile(xmlString); + expect(result).toEqual({ + success: true, + data: { + entityID: 'https://test.com', + ssoUrl: 'https://sso.jumpcloud.com/saml2/twenty', + certificate: 'test', + }, + }); + }); + it('should prefer HTTP-Redirect over HTTP-POST when both are advertised', () => { + const xmlString = ` + + + + + test + + + + + + +`; + const result = parseSAMLMetadataFromXMLFile(xmlString); + expect(result).toEqual({ + success: true, + data: { + entityID: 'https://test.com', + ssoUrl: 'https://test.com/redirect', + certificate: 'test', + }, + }); + }); + it('should return a descriptive reason when no supported binding is found', () => { + const xmlString = ` + + + + + test + + + + + +`; + const result = parseSAMLMetadataFromXMLFile(xmlString); + expect(result).toEqual({ + success: false, + reason: + 'No SingleSignOnService with HTTP-Redirect or HTTP-POST binding was found', + }); + }); + it('should return a descriptive reason when entityID is not a valid URL', () => { + const xmlString = ` + + + + + test + + + + + +`; + const result = parseSAMLMetadataFromXMLFile(xmlString); + expect(result).toEqual({ + success: false, + reason: 'entityID: entityID is not a valid URL', + }); + }); + it('should return a descriptive reason when EntityDescriptor is missing', () => { + const xmlString = ``; + const result = parseSAMLMetadataFromXMLFile(xmlString); + expect(result).toEqual({ + success: false, + reason: 'EntityDescriptor element is missing', + }); + }); it('should return error if XML is invalid', () => { const xmlString = 'invalid xml'; const result = parseSAMLMetadataFromXMLFile(xmlString); expect(result).toEqual({ success: false, - error: new Error('Error parsing XML'), + reason: 'File is not valid XML', }); }); }); diff --git a/packages/twenty-front/src/modules/settings/security/utils/parseSAMLMetadataFromXMLFile.ts b/packages/twenty-front/src/modules/settings/security/utils/parseSAMLMetadataFromXMLFile.ts index 1356bd432d..a9383293d9 100644 --- a/packages/twenty-front/src/modules/settings/security/utils/parseSAMLMetadataFromXMLFile.ts +++ b/packages/twenty-front/src/modules/settings/security/utils/parseSAMLMetadataFromXMLFile.ts @@ -2,9 +2,13 @@ import { z } from 'zod'; +const HTTP_REDIRECT_BINDING = + 'urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect'; +const HTTP_POST_BINDING = 'urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST'; + const validator = z.object({ - entityID: z.url(), - ssoUrl: z.url(), + entityID: z.url('entityID is not a valid URL'), + ssoUrl: z.url('SingleSignOnService Location is not a valid URL'), certificate: z.string().min(1), }); @@ -41,61 +45,86 @@ const getAllByPrefixAndKey = ( return Array.from(xmlDoc.getElementsByTagName(`${key}`)); }; +const formatErrorReason = (error: unknown): string => { + if (error instanceof z.ZodError) { + return error.issues + .map((issue) => { + const path = issue.path.join('.'); + return path.length > 0 ? `${path}: ${issue.message}` : issue.message; + }) + .join('; '); + } + if (error instanceof Error) return error.message; + return 'Unknown parsing error'; +}; + export const parseSAMLMetadataFromXMLFile = ( xmlString: string, ): | { success: true; data: z.infer } - | { success: false; error: unknown } => { + | { success: false; reason: string } => { try { const parser = new DOMParser(); const xmlDoc = parser.parseFromString(xmlString, 'application/xml'); if (xmlDoc.getElementsByTagName('parsererror').length > 0) { - throw new Error('Error parsing XML'); + throw new Error('File is not valid XML'); } const entityDescriptor = getByPrefixAndKey(xmlDoc, 'EntityDescriptor'); - if (!entityDescriptor) throw new Error('No EntityDescriptor found'); + if (!entityDescriptor) + throw new Error('EntityDescriptor element is missing'); const IDPSSODescriptor = getByPrefixAndKey(xmlDoc, 'IDPSSODescriptor'); - if (!IDPSSODescriptor) throw new Error('No IDPSSODescriptor found'); + if (!IDPSSODescriptor) + throw new Error('IDPSSODescriptor element is missing'); const keyDescriptors = getByPrefixAndKey(IDPSSODescriptor, 'KeyDescriptor'); - if (!keyDescriptors) throw new Error('No KeyDescriptor found'); + if (!keyDescriptors) throw new Error('KeyDescriptor element is missing'); const keyInfo = getByPrefixAndKey(keyDescriptors, 'KeyInfo'); - if (!keyInfo) throw new Error('No KeyInfo found'); + if (!keyInfo) throw new Error('KeyInfo element is missing'); const x509Data = getByPrefixAndKey(keyInfo, 'X509Data'); - if (!x509Data) throw new Error('No X509Data found'); + if (!x509Data) throw new Error('X509Data element is missing'); const x509Certificate = getByPrefixAndKey( x509Data, 'X509Certificate', )?.textContent?.trim(); - if (!x509Certificate) throw new Error('No X509Certificate found'); + if (!x509Certificate) + throw new Error('X509Certificate is missing or empty'); const singleSignOnServices = getAllByPrefixAndKey( IDPSSODescriptor, 'SingleSignOnService', - ); + ).map((service) => ({ + binding: service.getAttribute('Binding'), + location: service.getAttribute('Location'), + })); + + // Prefer HTTP-Redirect (the default authnRequestBinding on the SP side), + // fall back to HTTP-POST since both are valid SAML 2.0 bindings and many + // IdPs (e.g. JumpCloud) only advertise HTTP-POST. + const ssoUrl = + singleSignOnServices.find((s) => s.binding === HTTP_REDIRECT_BINDING) + ?.location ?? + singleSignOnServices.find((s) => s.binding === HTTP_POST_BINDING) + ?.location; + + if (!ssoUrl) { + throw new Error( + 'No SingleSignOnService with HTTP-Redirect or HTTP-POST binding was found', + ); + } const result = { - ssoUrl: singleSignOnServices - .map((service) => ({ - Binding: service.getAttribute('Binding'), - Location: service.getAttribute('Location'), - })) - .find( - (singleSignOnService) => - singleSignOnService.Binding === - 'urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect', - )?.Location, + ssoUrl, certificate: x509Certificate, entityID: entityDescriptor?.getAttribute('entityID'), }; return { success: true, data: validator.parse(result) }; } catch (error) { - return { success: false, error }; + return { success: false, reason: formatErrorReason(error) }; } };