[BUG] validateInResponseTo = true does not validate responses

[adamandreasson]

Found out that the responseTo was not being validated despite setting validateInResponseTo to true in the config. After some digging around, turns out setting validateInResponseTo to a truthy value is not enough. node-saml expects one of the valid values "never", "ifPresent", or "always". We tried setting it to "always" and now responses are actually validated properly.

I checked @node-saml/node-saml/lib/saml.js and found this method

    mustValidateInResponseTo(hasInResponseTo) {
        return (this.options.validateInResponseTo === types_1.ValidateInResponseTo.always ||
            (this.options.validateInResponseTo === types_1.ValidateInResponseTo.ifPresent && hasInResponseTo));
    }

Commit that changed behaviour in dependency: node-saml/node-saml@496c54e

To Reproduce
Follow the guide in the Readme and setup a basic auth. We noticed that responseTo was not being validated when we were setting up the cacheProvider and saw that it was never used, so that's one way to verify it without testing custom responses.

Expected behavior
Either validateInResponseTo = true should be equivalent to setting it to "always" or the documentation needs to be updated to reflect the values that are actually valid

Environment

• Node.js version: 16.14.2
• passport-saml version: 4.0.4

[cjbarth]

I personally think that adding true to mean always would be a very reasonable addition. Please provide a PR that does this, with test, and that updates the documentation. Having said that, such a PR probably needs to be made at node-saml. I know the old method would indicate that true should be ifPresent, but we're trying to make all the default the securest method possible and make consumers deliberately disable security features to suite their needs and environment.

Please also create a PR here to remove references to validateInReponseTo as we are trying to keep all the documentation in a single place and it currently being here is a relic of from when these two projects were one.

[srd90]

IMHO this issue should have bug and security labels because from library user point of view and based on this issue report inResponseTo validation is now silently dismissed. I.e. after migrating from 3.x to 4.x all client stacks which had and still has truthy instead of one of the enum values are now exposed to replay attack (same auth response can be reposted to establish new authenticated session if notOnOrAfter is still valid and if that check is not disabled).

fwiw, I would suggest that default would be always and node-saml would throw exception if value is something else than one of the enum values. At least starting from 5.x (and fix for 4.x could be to interpret truthy as always).

[adamandreasson]

Agree with @srd90 . This is a vulnerability and it is not obvious to new users that InResponseTo is not being validated despite following best practice in the documentation. Also agree with the suggestions. And the references to validateInResponseTo in this repo's documentation are still relevant since it is referred to in eg. setting up the cache provider.

The easiest fix imo is to update this repo's docs, default in node-saml config should be "always" (it is currently false), and enforce the enums.

[cjbarth]

I would suggest that default would be always and node-saml would throw exception if value is something else than one of the enum values. At least starting from 5.x (and fix for 4.x could be to interpret truthy as always).

I agree with this. We can land the PR to force a selection of an enum type on master, but I think we should do the truthy===always on the 4.x branch. As soon as that is done, I can release 4.0.5.

[cjbarth]

As a note, I'm working on getting some things sorted out with xml-crypto so that I can include that release in 5.x for node-saml and passport-saml. Sorry to all for the delay on that.

[cjbarth]

With the changes in #886 and the updates to node-saml, does everyone agree that this matter is resolved (as soon as we adopt the new version of node-saml here)?