feat(openid-connect): support stateless session revocation

[Hockenba]

Description

Adds opt-in Redis-backed revocation for OpenID Connect cookie sessions.

Setting session.revocation to redis enables lua-resty-session's Redis revocation backend. session.revocation_fail_mode defaults to open and can be set to closed. Redis-backed session storage remains unchanged, and incompatible or incomplete configurations are rejected.

Session options remain a direct pass-through to lua-resty-session. The deprecated session.cookie.lifetime alias continues to write absolute_timeout to the shared configuration, so its warning is not emitted on every request.

This also upgrades lua-resty-session to 4.2.0-1 and lua-resty-openidc to1.9.1-1 and documents the feature in English and Chinese.

Which issue(s) this PR fixes:

N/A

Test plan

• Verify valid default-open, explicit open, explicit closed, and omitted revocation configurations.
• Verify invalid revocation backends and failure modes, missing Redis configuration, and incompatible Redis session storage are rejected.
• Verify login, authenticated access, logout, and replay of the pre-logout cookie redirects with status 302.
• Verified all three revocation test blocks locally: 9 of 9 assertions passed.

Checklist

• I have explained the need for this PR and the problem it solves
• I have explained the changes or the new features added to this PR
• I have added tests corresponding to this change
• I have updated the documentation to reflect this change
• I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

[juzhiyuan]

Hello @Hockenba,

I did a quick review of this PR. It also depends on bungle/lua-resty-session#208, right?

Should we wait for the upstream PR to be merged first?

Update: This PR depends on bungle/lua-resty-session#208 status

https://the-asf.slack.com/archives/CUC5MN17A/p1783014722148709

[janiussyafiq]

Some comments that you can address or state your reasoning on why you think the current approaches should remain:

• I think it would be cleaner to expose the schema in this way rather than the current way since lua-resty-session also do it the same way instead nesting it within redis, WDYT?

"session": {
"storage": "cookie",
"redis": { "host": "127.0.0.1" },
"revocation": "redis",
"revocation_fail_mode": "closed"
}

• Docs comment: can add one section for usage example using this current feature (can refer how other example showcases this), and add docs for chinese version too
• Update PR desc to reflect the newest code submitted for review, some of them were stale
• IMO the tests were bloated and some are mere duplication from lua-resty-session upstream test which have already been covered, might consider rechecking again. e.g.
  • TEST 13-16 exercise lua-resty-session's fail-open/fail-closed behaviour through a stub revocation table and the library's private __set_ngx_header / __set_ngx_var hooks. That path is already covered upstream (spec/07-revocation-2_spec.lua); APISIX's contribution is one field, which TEST 2 already asserts.
  • TEST 13-17 accept skip: revocation not supported in response_body, so they pass without testing anything on lua-resty-session < 4.2.0. With the rockspec pinned to 4.2.0-1 the probe should go; a missing feature should fail, not skip (same reason check-test-code-style.sh bans --- SKIP).
  • The 12 schema/passthrough blocks differ by one or two fields each. t/plugin/openid-connect2.t (TEST 1) folds such variants into a test_cases table in one block; the same would bring this file down to a handful of blocks.
  • Four cases from the test plan have no test: default revocation_fail_mode (every block sets it explicitly), mode=storage on cookie, revocation_fail_mode without redis, invalid redis.mode.
  • etc. You might want to revisit this again

[Hockenba]

Some comments that you can address or state your reasoning on why you think the current approaches should remain:

• I think it would be cleaner to expose the schema in this way rather than the current way since lua-resty-session also do it the same way instead nesting it within redis, WDYT?

"session": {
"storage": "cookie",
"redis": { "host": "127.0.0.1" },
"revocation": "redis",
"revocation_fail_mode": "closed"
}

• Docs comment: can add one section for usage example using this current feature (can refer how other example showcases this), and add docs for chinese version too

• Update PR desc to reflect the newest code submitted for review, some of them were stale

• IMO the tests were bloated and some are mere duplication from lua-resty-session upstream test which have already been covered, might consider rechecking again. e.g.

  • TEST 13-16 exercise lua-resty-session's fail-open/fail-closed behaviour through a stub revocation table and the library's private __set_ngx_header / __set_ngx_var hooks. That path is already covered upstream (spec/07-revocation-2_spec.lua); APISIX's contribution is one field, which TEST 2 already asserts.
  • TEST 13-17 accept skip: revocation not supported in response_body, so they pass without testing anything on lua-resty-session < 4.2.0. With the rockspec pinned to 4.2.0-1 the probe should go; a missing feature should fail, not skip (same reason check-test-code-style.sh bans --- SKIP).
  • The 12 schema/passthrough blocks differ by one or two fields each. t/plugin/openid-connect2.t (TEST 1) folds such variants into a test_cases table in one block; the same would bring this file down to a handful of blocks.
  • Four cases from the test plan have no test: default revocation_fail_mode (every block sets it explicitly), mode=storage on cookie, revocation_fail_mode without redis, invalid redis.mode.
  • etc. You might want to revisit this again

You make a lot of great points. It is a little late in my time zone, so I'll refine this tomorrow based on your comments.

One thing: the Chinese documentation needs to the created and pushed to this PR by someone who actually knowns Chinese. I do not know a lick of Chinese and I am sure we want better than what Google Translate will spit out for me.

[janiussyafiq]

You make a lot of great points. It is a little late in my time zone, so I'll refine this tomorrow based on your comments.

One thing: the Chinese documentation needs to the created and pushed to this PR by someone who actually knowns Chinese. I do not know a lick of Chinese and I am sure we want better than what Google Translate will spit out for me.

Thanks for the fast reply. For Chinese translation u can ask your agent to do it on your behalf, it usually does it right the first time if the change is not so complex. If there's any issue then maintainers will give some feedback.

[Hockenba]

You make a lot of great points. It is a little late in my time zone, so I'll refine this tomorrow based on your comments.
One thing: the Chinese documentation needs to the created and pushed to this PR by someone who actually knowns Chinese. I do not know a lick of Chinese and I am sure we want better than what Google Translate will spit out for me.

Thanks for the fast reply. For Chinese translation u can ask your agent to do it on your behalf, it usually does it right the first time if the change is not so complex. If there's any issue then maintainers will give some feedback.

I wasn't sure how good agents are at English to Chinese translation. Sounds good. Will do.

I'll be spending my workday tomorrow refactoring and addressing your comments with either commits or follow up discussion (Eastern Standard Time). So be on the look out for that. Thanks.

[Hockenba]

You make a lot of great points. It is a little late in my time zone, so I'll refine this tomorrow based on your comments.
One thing: the Chinese documentation needs to the created and pushed to this PR by someone who actually knowns Chinese. I do not know a lick of Chinese and I am sure we want better than what Google Translate will spit out for me.

Thanks for the fast reply. For Chinese translation u can ask your agent to do it on your behalf, it usually does it right the first time if the change is not so complex. If there's any issue then maintainers will give some feedback.

I wasn't sure how good agents are at English to Chinese translation. Sounds good. Will do.

I'll be spending my workday tomorrow refactoring and addressing your comments with either commits or follow up discussion (Eastern Standard Time). So be on the look out for that. Thanks.

Done. Refactored it quite a bit. Cut out all the unnecessary bloat. Ready for review again.

[janiussyafiq]

Thanks for addressing my concerns, however few issues remain:

1. I ran the test locally and TEST 3 seems to fail. You can open up the PR against your fork and ensure CI green.
2. build_session_opts now does core.table.clone per request, so the session.cookie.lifetime is deprecated warning fires on every request for users still on the alias (previously once, because absolute_timeout was written back into the shared conf).
3. On the schema shape I still think an explicit session.revocation enum ({"redis"}) with revocation_fail_mode defaulting to open is the better contract. Every other session.* key is a 1:1 forward to lua-resty-session. This is the only derived one (by setting revocation field implicitly.
4. --- no_error_log [crit] on TEST 1 replaces the default [error] gate; nothing in the flow logs [error], so it only hides regressions. Drop it, and assert the replay is 302 rather than ~= 200.

[Hockenba]

Thanks for addressing my concerns, however few issues remain:

1. I ran the test locally and TEST 3 seems to fail. You can open up the PR against your fork and ensure CI green.
2. build_session_opts now does core.table.clone per request, so the session.cookie.lifetime is deprecated warning fires on every request for users still on the alias (previously once, because absolute_timeout was written back into the shared conf).
3. On the schema shape I still think an explicit session.revocation enum ({"redis"}) with revocation_fail_mode defaulting to open is the better contract. Every other session.* key is a 1:1 forward to lua-resty-session. This is the only derived one (by setting revocation field implicitly.
4. --- no_error_log [crit] on TEST 1 replaces the default [error] gate; nothing in the flow logs [error], so it only hides regressions. Drop it, and assert the replay is 302 rather than ~= 200.

1-4 should be addressed.

[janiussyafiq]

Thanks, 1-4 look good and openid-connect-revocation.t passes locally. One issue with the new default = "open" on revocation_fail_mode is that it is applied unconditionally, so every openid-connect route now carries "session":{"revocation_fail_mode":"open"} even with revocation disabled (this breaks t/plugin/openid-connect.t TEST 25 "Check defaults"). The intent is "open only when revocation is redis", and lua-resty-session already does that internally (defaults to open, only consulted on the revocation path). Please drop the schema default, document it as the default in the table, and adjust the TEST 2 assertions to expect nil. Also run the full t/plugin/openid-connect*.t set before pushing.

[Hockenba]

Thanks, 1-4 look good and openid-connect-revocation.t passes locally. One issue with the new default = "open" on revocation_fail_mode is that it is applied unconditionally, so every openid-connect route now carries "session":{"revocation_fail_mode":"open"} even with revocation disabled (this breaks t/plugin/openid-connect.t TEST 25 "Check defaults"). The intent is "open only when revocation is redis", and lua-resty-session already does that internally (defaults to open, only consulted on the revocation path). Please drop the schema default, document it as the default in the table, and adjust the TEST 2 assertions to expect nil. Also run the full t/plugin/openid-connect*.t set before pushing.

Should be all addressed now. I ran all the openid-connect tests, and they all passed. No idea why the CI tests failed, but none are related to my changes or even the openid-connect plugin.

[Copilot]

Pull request overview

Adds Redis-backed revocation for stateless OpenID Connect cookie sessions.

Changes:

• Adds revocation schema validation and session option forwarding.
• Upgrades lua-resty-session to 4.2.0-1.
• Adds integration tests and bilingual documentation.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
apisix/plugins/openid-connect.lua	Defines revocation configuration.
apisix-master-0.rockspec	Upgrades the session dependency.
t/plugin/openid-connect-revocation.t	Tests configuration and revocation.
docs/en/latest/plugins/openid-connect.md	Documents the feature in English.
docs/zh/latest/plugins/openid-connect.md	Documents the feature in Chinese.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Hockenba]

Not sure if the AI is correct (will verify in the morning), but this may be intentional best-effort logout design—either by the project or something we may want ourselves—so users aren't blocked when Redis is down. The client cookie still clears, and from the UI's perspective, the logout happened successfully. A bug-free UI won't randomly reuse deleted cookies, and this scenario is really aimed at bad actors rather than normal operation.

The replay risk only exists if the session value was stolen and reused. Even then, fail-open or fail-closed doesn't change the fact that we can't revoke it without Redis up—so we have to eat that risk either way. If we fail closed, it doesn't stop the replay attack; it just tells the user their logout failed and leaves them retrying over and over again, which isn't great for UX.

If we prefer stricter failure handling regardless, we can add it now or defer it. Either direction works depending on what makes sense for the project.

[juzhiyuan]

cc @shreemaan-abhishek kindly have a look, thanks

[janiussyafiq]

I think the concern is valid, could u address this

[Hockenba]

A proper fix depends on a change in lua-resty-openidc, since that’s where the destroy() error isn’t being propagated. I’m pausing this PR for now and will work on fixing it there first.

[Hockenba]

@janiussyafiq@juzhiyuan See zmartzone/lua-resty-openidc#571. Looks like https://github.com/kevinlzw is the maintainer of this library. I reached out to him by email.

[Hockenba]

@janiussyafiq@juzhiyuan BTW, said PR is merged, and I have updated lua-resty-openidc to the latest version. Along with the changes and test to detect that errors and return correctly. Should be ready to continue with the review.

[janiussyafiq]

could u rebase/merge with master to fix CI

[Hockenba]

could u rebase/merge with master to fix CI

@janiussyafiq Done.