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

[jmaixl]

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:

• openid-client version: [latest]
• node version: [v8.7.0]

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]?

• the bug is happening on latest openid-client too.
• i have searched the issues tracker on github for similar issues and couldn't find anything related.

[panva]

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]

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.