Make the checks on incoming messages private, and run each one once #503

The checks SAML runs on an incoming message are protected methods that take the whole xml2js document. #500 calls one of them from a second place, and reviewing it showed how loose that surface is. Changing it breaks subclasses, so it waits for v6. #504 deprecates overriding these methods in 5.2.0.

What is wrong with it

A subclass can switch a check off. These methods are protected, so an override with an empty body removes the check:

• verifyLogoutResponse: the status, the issuer, and that InResponseTo names a pending request
• verifyLogoutRequest: the issuer, NotBefore and NotOnOrAfter
• verifyIssuer, validateInResponseTo, mustValidateInResponseTo
• checkTimestampsValidityError, checkAudienceValidityError

The code has already moved security-relevant work out of overridable methods three times, each with a comment saying why: retiring request IDs in validatePostResponseAsync and in validateRedirectAsync, and refusing an unsigned query in resolveRedirectArguments. passport-saml overrides none of the methods above, and the README documents no override of them.

Two checks run twice.

• POST LogoutResponse, once Check a POST-binding LogoutResponse's status and issuer, as the Redirect binding does #500 merges: validatePostResponseAsync calls validateInResponseTo on the InResponseTo it reads at the start, and verifyLogoutResponse calls it again on the same attribute.
• Redirect LogoutRequest: validateRedirectAsync calls verifyLogoutRequest(doc). Then processValidlySignedSamlLogoutAsync hands the request to processValidlySignedPostRequestAsync, which calls it again.

Both copies read the same value, so neither is a bug today. But a reader has to establish that, and a fix to one copy doesn't reach the other.

One outcome is decided in two places. A LogoutResponse resolves as { profile: null, loggedOut: true } inline in validatePostResponseAsync for POST, and in processValidlySignedSamlLogoutAsync for Redirect.

The checks take Record<string, any>. XMLOutput is untyped, so TypeScript checks no field read. verifyLogoutResponse read Status[0].StatusCode[0].$.Value with no guard, so a LogoutResponse without a Status threw a TypeError instead of the status error until #500. Taking the whole document also hides which fields a check reads, and whether they came from the bytes the signature covers.

Names. doc is the DOM in validatePostResponseAsync, but the xml2js object in validateRedirectAsync, where the DOM is dom. processValidlySignedPostRequestAsync also processes Redirect-binding LogoutRequests.

The change

• Make every method Deprecate overriding the methods that validate an incoming message #504 deprecates private: the ones listed above, and calcMaxAgeAssertionTime, getKeyInfosAsPem, getSignedAssertion, hasValidSignatureForRedirect, validateSignatureForRedirect and the three processValidlySigned* methods.
• Pass each check the values it checks, typed, in place of the document. For example, verifyLogoutResponse would take the status code and the issuer. The caller reads them from the verified bytes, and the check's signature shows what it depends on.
• Run each check once per message, and decide the result of a LogoutResponse in one place for both bindings.
• Keep per-message values in arguments, not on the instance. One SAML instance serves concurrent validations, which interleave at every await, so a message stored on this could be checked in place of another request's.
• Name the DOM and the xml2js object the same way in every method. Name the LogoutRequest processor for what it does, not for POST.
• Keep a processValidlySigned* method as an extension point only for a use case that checking the public method's result cannot serve. test/typeSurface.spec.ts pins processValidlySignedAssertionAsync and processValidlySignedSamlLogoutAsync as override forms. The second argument of processValidlySignedAssertionAsync is the response as received; it exists for getSamlResponseXml(), which Remove Profile.getSamlResponseXml() #424 removes.

#419 and #495 also change hasValidSignatureForRedirect, so do this together with them.

One related part needs no major and can land first. In the no-assertion path of validatePostResponseAsync, #502 parses responseVerifiedXml ?? xml. That is safe because every branch that returns checks validSignature first, but the next branch added there has to remember to. Give the unsigned root its own branch, which only builds the status error or throws, and parse only the verified bytes in the other.

Tests that move with it

• test/typeSurface.spec.ts, "keeps compiling a subclass written against the previous signatures": drop or rewrite the override of each method whose signature changes.
• test/inResponseTo.spec.ts, "through an override of processValidlySignedSamlLogoutAsync": keep it if that method stays overridable, since it shows that retiring a request survives an override.
• test/samlTests.spec.ts, "resolveAndParseKeyInfosToPem": it stubs processValidlySignedPostRequestAsync and the xml module to reach getKeyInfosAsPem. Rewrite it to call validatePostRequestAsync with a real signed request.

Release notes

A subclass that overrides any of the methods made private stops compiling and has to drop the override. Those methods are the checks themselves, so overriding them could only weaken validation.

🤖 Generated with Claude Code