Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

OAuth redirect lost sometimes due to session store race (#2514) #2515

Merged
merged 1 commit into from
Jan 6, 2022

Conversation

mterrel
Copy link
Contributor

@mterrel mterrel commented Dec 23, 2021

Summary

This PR solves the issue (described in greater detail in #2514) where Feathers OAuth does not always redirect to the path requested by the redirect query parameter and instead redirects to the root of the site specified by authentication.oauth.redirect. This happens only a small percentage of the time and only when using an external Express session store that is somewhat slow.

The issue is due to a race condition where the Express session created during a request to /oauth/:name does not get saved before the browser makes a subsequent request in the OAuth flow (to /oauth/connect/:name) and that causes Express to create a new session, effectively discarding the original session which contains the redirect parameter.

The fix is for Feathers to explicitly save the Express session in the /oauth/:name handler before responding with the redirect to /oauth/connect/:name.

This fix is consistent with guidance from Express session's docs (see bolded phrase):

Session.save(callback)
Save the session back to the store, replacing the contents on the store with the contents in memory (though a store may do something else--consult the store's documentation for exact behavior).
This method is automatically called at the end of the HTTP response if the session data has been altered (though this behavior can be altered with various options in the middleware constructor). Because of this, typically this method does not need to be called.
There are some cases where it is useful to call this method, for example, redirects, long-lived requests or in WebSockets.

Also see issues like https://github.com/expressjs/session/issues/660

Fixes: #2514

Other Information

Tests run

  • npm run test

Note on TypeScript version

The newer version of TypeScript that gets installed with a fresh clone of this repo does not compile successfully due to TS changing the default type of the error in catch blocks to unknown. So I ran my tests by pinning TypeScript to an older version.

@mterrel
Copy link
Contributor Author

mterrel commented Jan 5, 2022

@daffl In order to get CI to run, you may want to look at adding "useUnknownInCatchVariables": false to tsconfig.json. Or, alternately, you'll need to go touch pretty much all your catch blocks on the crow branch. Let me know if I can be of help.

@daffl daffl merged commit 67a7e31 into feathersjs:crow Jan 6, 2022
@mterrel mterrel deleted the issue-2514 branch January 6, 2022 01:19
daffl pushed a commit that referenced this pull request Jan 6, 2022
@daffl
Copy link
Member

daffl commented Jan 6, 2022

Released in 4.5.12. Thank you for digging into this.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants