[BUG] multi-saml strategy cannot be used by concurrent requests
gunzip opened this issue · 4 comments
It looks like Multisaml strategy (which is a singleton object) calls override the shared property this._saml: https://github.com/bergie/passport-saml/blob/master/multiSamlStrategy.js#L34
In case of concurrent HTTP calls is there any chance that this value gets overridden by a subsequent call ?
cc @stavros-wb
Yes.. 😔 We should do something like:
newStrategy = { ...self, _saml: new saml.SAML(Object.assign({}, self._options, samlOptions)) };
self.constructor.super_.prototype.authenticate.call(newStrategy, req, options);The tricky part here is, we can't instantiate N strategies, cause they all get registered in passport and can override each other there. We only want to have the _saml property change dynamically and reuse the same methods for the rest of the calls.
Do you think this can be considered more a bug or a vulnerability ? I see at least the chance of a DoS (making fast calls to the login endpoint that override the saml client options for previous requests).
Anyway the MultiSamlStrategy as is should not be used in production, so I wonder if it's better to get rid of the modue entirely to discourage unsecure setups.
About using multiple passport strategy it is actually possible if you create multiple Passport() instances like in the example by @sp90 here: #276 (comment)
Given that a PR was merged to address this, is there more work associated with this, or can it be closed?
Was this fixed, and if so does this make the warning on the website invalid? Just checking because the link is still valid but I see this was closed 2 years ago.
https://www.passportjs.org/packages/passport-saml/