Why 500 responses and not 401?

[jonasfj]

I used saml.oktadev.com to test an app I'm developing that'll be a service provider.
Running saml.oktadev.com with "Run security validation" I notice a few issues:

1. I see a lot of responses like this, which suggests that "invalid signature" errors results in 500 responses rather than 401 responses. I guess it's not a huge security issue, but it's certainly bad practice and makes it hard to debug the app (generally we don't want 500 responses unless we have a bug).

POST /sso-login/callback 500 4.825 ms - 857
Error: Invalid signature
    at .../node_modules/passport-saml/lib/passport-saml/saml.js:451:15

Also I noticed that when it ran the last test where it reported a warning:

"The SP does not support a compressed SAMLResponse"

I fully understand if passport-saml doesn't support compressed responses. But rather than returning a 500 response we should probably respond 400, or at the very least 401.
(granted I can better undstand that a 500 is acceptable in this case).

POST /sso-login/callback 500 7.027 ms - 1220
TypeError: Cannot read property 'documentElement' of null
    at SelectNodes (.../node_modules/passport-saml/node_modules/xml-crypto/node_modules/xpath.js/xpath.js:4315:8)
    at [object Object].SAML.validateSignature (.../node_modules/passport-saml/lib/passport-saml/saml.js:371:20)
    at .../node_modules/passport-saml/lib/passport-saml/saml.js:433:35

My code is setup as outlined in readme:

  passport.use(new SamlStrategy({
    issuer: 'http://http://tc-auth.ngrok.io',
    path: '/sso-login/callback',
    entryPoint: 'http://idp.oktadev.com',
    cert: 'MIIDPDCCA.....'
  }, function(info, done) {
    ...
    done(null, {...});
  });

  app.get('/sso-login', passport.authenticate('saml', {
    failureRedirect:  '/',
    failureFlash: true
  }), (req, res) => {
    res.redirect('/');
  });

[ploer]

I think passport is just propagating the error out into your app's error-handling code, which probably defaults to returning a 500. But I guess it probably doesn't make sense to separate these cases out there.

So I think I agree in theory -- passport-saml should call strategy.fail() in at least the signature verification case, rather than just an error.

If you want to put together a PR, I think the change would look something like changing cases where we throw Invalid Signature to instead throw a type that could be filtered out in 'redirectIfSuccess' and used to call self.fail() instead of self.error().