-
Notifications
You must be signed in to change notification settings - Fork 76
Description
We're using node-saml as a "plugin" for SAML authentication at our company.
We've recently had an issue with one of our clients who is sending multiple Audiences in one AudienceRestriction:
<saml2:Conditions NotBefore="2023-12-20T20:44:20.578Z" NotOnOrAfter="2023-12-20T20:49:20.577Z">
<saml2:AudienceRestriction>
<saml2:Audience>
acadeum
</saml2:Audience>
<saml2:Audience>
https://students.acadeum.com
</saml2:Audience>
</saml2:AudienceRestriction>
</saml2:Conditions>At first, we thought that such input is invalid, but having googled for it, looks like it is valid according to SAML specification:
https://stackoverflow.com/questions/43082519/does-ping-support-multiple-audience-restriction-values-in-saml
Furthermore, after looking at node-saml source code, we can see how it simply discards any "audience"s except the first one:
Lines 1229 to 1237 in e691ccf
| if (restriction.Audience[0]._ !== expectedAudience) { | |
| return new Error( | |
| "SAML assertion audience mismatch. Expected: " + | |
| expectedAudience + | |
| " Received: " + | |
| restriction.Audience[0]._, | |
| ); | |
| } | |
| return null; |
if (restriction.Audience[0]._ !== expectedAudience) {
return new Error(
"SAML assertion audience mismatch. Expected: " +
expectedAudience +
" Received: " +
restriction.Audience[0]._,
);
}
return null;It's not clear why it was written the way it is, but we think that the code above should be corrected:
let i = 0;
while (true) {
if (restriction.Audience[i]._ === expectedAudience) {
break;
}
if (!restriction.Audience[i + 1]) {
return new Error(
"SAML assertion audience mismatch. Expected: " +
expectedAudience +
" Received: " +
restriction.Audience[i]._,
);
}
i++;
}
return null;We've submitted the code patch above in the form of a pull request.