fix(feishu-auth, dingtalk-auth): bind the authorization code to the session that started the login - #13806
Open
shreemaan-abhishek wants to merge 1 commit into
Open
Conversation
…ession that started the login Both plugins emitted the login redirect and consumed the callback without a state parameter, so a code from the query string was accepted regardless of which session it arrived on. Generate a random state before redirecting, carry it on the redirect URL, and require it back on codes taken from the query string. Codes taken from the configured header are exempt, since those requests come from non-browser clients. Also replaces sess:delete(), which does not exist in lua-resty-session 4.x and crashed the feishu-auth token refresh path.
dosubot
Bot
added
the
size:XL
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
feishu-authanddingtalk-authimplement the OAuth 2.0 authorization code flow, but neither carried astateparameter across it. The plugin emitted the login redirect with nostate, and on the way back it took the code straight out of the query string and exchanged it, with nothing tying that code to the browser that started the flow. A code obtained elsewhere was therefore accepted on any session.This PR closes that gap:
redirect_uri, generate a random 16-bytestate, store it in the session cookie, and append it to the redirect URL (?state=or&state=depending on whether the configured URL already has a query string).stateto match the one bound to the session, and respond401 {"message":"Invalid state"}when it does not. The state is dropped from the session once used.X-Feishu-Code/X-DingTalk-Code), nostateis required. Those requests come from non-browser clients, which cannot be driven cross-site.This mirrors what
authz-casdooralready does in this repository (apisix/plugins/authz-casdoor.lua:187-196generates and stores,:131-144validates).Also in this PR:
feishu-authcalledsess:delete(...)on the token refresh path.lua-resty-session4.x has nodeletemethod, so that path raisedattempt to call method 'delete' (a nil value)and returned 500. Replaced withsess:set(key, nil), which is the supported way to drop a key. This is exercised by the new tests.Behaviour change
Deployments that use the query-string code path now need
stateto survive the round trip:redirect_urireceives it as a query parameter, and the login page is expected to pass it through to the IdP so that it comes back on the callback. This is the standard OAuth handling ofstate. Two consequences worth calling out:statein their session and gets a401. Retrying the flow succeeds.There is no new configuration option: an opt-out would just be a switch for turning the check back off.
No plugin schema fields were added or changed.
Which issue(s) this PR fixes:
Fixes #
Checklist