Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] From multiple Audience variants, only the first one gets looked at #339

Closed
catamphetamine opened this issue Dec 21, 2023 · 0 comments · Fixed by #340
Closed

[BUG] From multiple Audience variants, only the first one gets looked at #339

catamphetamine opened this issue Dec 21, 2023 · 0 comments · Fixed by #340
Labels
bug Something isn't working

Comments

@catamphetamine
Copy link
Contributor

catamphetamine commented Dec 21, 2023

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:

node-saml/src/saml.ts

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant