Skip to content

Commit

Permalink
refactor(security): extend SAML prefix handling (twentyhq#10047)
Browse files Browse the repository at this point in the history
Revised parsing logic to handle multiple XML prefixes for SAML metadata,
improving flexibility in handling diverse metadata structures. Added
corresponding test case to ensure robustness of the implementation.
  • Loading branch information
AMoreaux authored Feb 5, 2025
1 parent 700eb2d commit f6ce27b
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,31 @@ describe('parseSAMLMetadataFromXMLFile', () => {
},
});
});
it('should parse SAML metadata from XML file with prefix', () => {
const xmlString = `<?xml version="1.0" encoding="UTF-8"?><ns0:EntityDescriptor xmlns:ns0="urn:oasis:names:tc:SAML:2.0:metadata" entityID="https://test.com" validUntil="2026-02-04T17:46:23.000Z">
<ns0:IDPSSODescriptor WantAuthnRequestsSigned="false" protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol">
<ns0:KeyDescriptor use="signing">
<ns2:KeyInfo xmlns:ns2="http://www.w3.org/2000/09/xmldsig#">
<ns2:X509Data>
<ns2:X509Certificate>test</ns2:X509Certificate>
</ns2:X509Data>
</ns2:KeyInfo>
</ns0:KeyDescriptor>
<ns0:NameIDFormat>urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress</ns0:NameIDFormat>
<ns0:SingleSignOnService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect" Location="https://test.com"/>
<ns0:SingleSignOnService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST" Location="https://test.com"/>
</ns0:IDPSSODescriptor>
</ns0:EntityDescriptor>`;
const result = parseSAMLMetadataFromXMLFile(xmlString);
expect(result).toEqual({
success: true,
data: {
entityID: 'https://test.com',
ssoUrl: 'https://test.com',
certificate: 'test',
},
});
});
it('should return error if XML is invalid', () => {
const xmlString = 'invalid xml';
const result = parseSAMLMetadataFromXMLFile(xmlString);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,26 +8,36 @@ const validator = z.object({
certificate: z.string().min(1),
});

const allPrefix = ['md', 'ns0', 'ns2', 'dsig', 'ds'];

const getByPrefixAndKey = (
xmlDoc: Document | Element,
key: string,
prefix = 'md',
prefixList = [...allPrefix],
): Element | undefined => {
if (prefixList.length === 0) return undefined;
return (
xmlDoc.getElementsByTagName(`${prefix}:${key}`)?.[0] ??
xmlDoc.getElementsByTagName(`${key}`)?.[0]
xmlDoc.getElementsByTagName(`${prefixList[0]}:${key}`)?.[0] ??
getByPrefixAndKey(xmlDoc, key, prefixList.slice(1)) ??
xmlDoc.getElementsByTagName(key)?.[0]
);
};

const getAllByPrefixAndKey = (
xmlDoc: Document | Element,
key: string,
prefix = 'md',
) => {
const withPrefix = xmlDoc.getElementsByTagName(`${prefix}:${key}`);
prefixList = [...allPrefix],
): Array<Element> => {
const withPrefix = xmlDoc.getElementsByTagName(`${prefixList[0]}:${key}`);

if (withPrefix.length !== 0) {
return Array.from(withPrefix);
}

if (prefixList.length > 0) {
return getAllByPrefixAndKey(xmlDoc, key, prefixList.slice(1));
}

return Array.from(xmlDoc.getElementsByTagName(`${key}`));
};

Expand All @@ -52,16 +62,15 @@ export const parseSAMLMetadataFromXMLFile = (
const keyDescriptors = getByPrefixAndKey(IDPSSODescriptor, 'KeyDescriptor');
if (!keyDescriptors) throw new Error('No KeyDescriptor found');

const keyInfo = getByPrefixAndKey(keyDescriptors, 'KeyInfo', 'ds');
const keyInfo = getByPrefixAndKey(keyDescriptors, 'KeyInfo');
if (!keyInfo) throw new Error('No KeyInfo found');

const x509Data = getByPrefixAndKey(keyInfo, 'X509Data', 'ds');
const x509Data = getByPrefixAndKey(keyInfo, 'X509Data');
if (!x509Data) throw new Error('No X509Data found');

const x509Certificate = getByPrefixAndKey(
x509Data,
'X509Certificate',
'ds',
)?.textContent?.trim();
if (!x509Certificate) throw new Error('No X509Certificate found');

Expand Down

0 comments on commit f6ce27b

Please sign in to comment.