shreemaan-abhishek opened a new pull request, #13806:
URL: https://github.com/apache/apisix/pull/13806
### Description
`feishu-auth` and `dingtalk-auth` implement the OAuth 2.0 authorization code
flow, but neither carried a `state` parameter across it. The plugin emitted the
login redirect with no `state`, 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:
- Before redirecting to `redirect_uri`, generate a random 16-byte `state`,
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).
- When the code arrives in the query string, require the `state` to match
the one bound to the session, and respond `401 {"message":"Invalid state"}`
when it does not. The state is dropped from the session once used.
- When the code arrives in the configured header (`X-Feishu-Code` /
`X-DingTalk-Code`), no `state` is required. Those requests come from
non-browser clients, which cannot be driven cross-site.
This mirrors what `authz-casdoor` already does in this repository
(`apisix/plugins/authz-casdoor.lua:187-196` generates and stores, `:131-144`
validates).
Also in this PR: `feishu-auth` called `sess:delete(...)` on the token
refresh path. `lua-resty-session` 4.x has no `delete` method, so that path
raised `attempt to call method 'delete' (a nil value)` and returned 500.
Replaced with `sess: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 `state` to survive
the round trip: `redirect_uri` receives 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 of `state`. Two consequences
worth calling out:
- A user who was mid-flow across an upgrade (redirected before, returning
after) has no `state` in their session and gets a `401`. Retrying the flow
succeeds.
- Deployments that drive the plugin entirely through the code header are
unaffected.
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
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [x] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible (If not,
please discuss on the [APISIX mailing
list](https://github.com/apache/apisix/tree/master#community) first)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
