Clever/saml2

v2.0.5 introduces uncaught exception for missing subject

realityendshere opened this issue · 4 comments

realityendshere commented

How I expect it to work:

In v2.0.3, any SAML assertion with a missing Subject would cause an error to be caught and returned in the callback. ("Expected 1 Subject; found 0")

This allows my SAML code to properly handle the error/rejected promise and report it back to the API consumer.

How it appears to be working:

In v2.0.5, Line 494: user.name_id = get_name_id validated_assertion. If there is not exactly one subject in the SAML assertion, an uncaught exception is thrown here. This makes it difficult to capture what went wrong and report it back to the API consumer.

Steps to reproduce

Send a SAML assertion with an entirely missing Subject node using a Promise.

return new Promise((resolve, reject) => {
  serviceProvider.post_assert(identityProvider, options, function (err, saml_response) {
    return err ? reject(err) : resolve(saml_response);
  });
});

An exception is thrown within the node module. It never returns an error to the Promise. Is this intentional?

Suggested Fix

Wrap the call to user.name_id = get_name_id validated_assertion in a try-catch block. Return the exception to the callback function.

Thanks!

jonlanglois commented

I introduced this change of behavior unintentionally...not sure exactly why I removed the try/catch there, but the test suite all passed and so did my project's tests so I must have thought it was unneeded.

I agree that passing an error into the callback is correct--it shouldn't be throwing an exception. So I'll put the try/catch back in.

But, per the spec, Subject is an optional element. I think the correct behavior is actually for get_name_id() to return a null in this case, rather than throwing an error. If your application requires a nameid then you're free to reject the assertion after examining the returned user object. But that's a functional change, not really appropriate for a patch version. I'll just file an issue for it.

jonlanglois commented

PR #193.

  • 👍2
mcab commented

Just merged #193, which is now tagged under 2.0.7.

Thanks for the report @realityendshere, and for the fix @jonlanglois!

Feel free to reopen if there's still an issue.

realityendshere commented

Many thanks, @jonlanglois and @mcab!

In the context of the project impacted by this issue:

v2.0.5 (with the issue) fails the project test suite.
v2.0.7 (with the fix) passes the project test suite.

I call that a success!

I'll let you know if anything unexpected pops up after deployment to production.

Thanks again and Happy 2021!

  • 🎉1