search

panva / openid-client

OAuth 2 / OpenID Connect Client API for JavaScript Runtimes
MIT License
1.83k stars 392 forks source link

Multiple authenticate requests from the same session causes state mismatch or undefined session information #154

Closed jmaixl closed 5 years ago

jmaixl commented 5 years ago

Describe the bug

The provided passport strategy errors out when multiple authenticate requests are issued from the same session, and the error depends on the order in which these requests are handled. This is happening before the verify() callback of the strategy.

Steps to reproduce the behaviour:

Invoke authenticate() with 2 requests (A then B) but with the same session (e.g. 2 browser tabs). Proceed as the end-user to authenticate with the identity provider those 2 requests in the following orders:

  1. If we satisfy B first, B authenticates normally, but the first request hits an error: did not find expected authorization request details in session, req.session[...] is undefined because the req.session[sessionKey] was deleted while satisfying B.
  2. If we satisfy A first, then A hits an error "state mismatch" in client.js because req.session[sessionKey] contains B's state now.

Expected behaviour Both requests should authenticate normally regardless of their order.

Environment:

Additional context Could this be fixed by modifying what is persisted in the session? Instead of persisting the state in req.session[sessionKey], persist the state in req.session[sessionKey][state]?

panva commented 5 years ago

I don't see a way around this honestly, as part of the callback there will be cleanup of the temporary session namespace the strategy uses.

Could this be fixed by modifying what is persisted in the session? Instead of persisting the state in req.session[sessionKey], persist the state in req.session[sessionKey][state]?

Not really, see here https://github.com/panva/node-openid-client/blob/v2.4.5/lib/passport_strategy.js#L129 the cleanup.

I'm gonna close this and label as won't fix since it's just exhibiting well defined behaviours during an edge case, not using a state would pose a way bigger issue. And either way, it just surfaces as an issue with the state, but would be the case with everything that's pulled from the session.

jmaixl commented 5 years ago

Yes, I noticed the cleanup as part of the callback - that could be changed to cleanup req.session[sessionKey][state]. But I will respect the wontfix if that' still the final decision.