-
Notifications
You must be signed in to change notification settings - Fork 476
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUG] multi-saml strategy cannot be used by concurrent requests #425
Comments
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 |
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) |
* Fix multi saml strategy race conditions #425 * Add warning for MultiSaml in Readme
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/ |
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#L34In case of concurrent HTTP calls is there any chance that this value gets overridden by a subsequent call ?
cc @stavros-wb
The text was updated successfully, but these errors were encountered: