What
Both login and login_callback call sess:save() and ignore its result. resty.session's save returns nil, err on real failure paths: key derivation, encryption, MAC, cookie encoding, storage.
In login_callback the failure is silent and the consequence compounds: the code logs "login finish" and redirects as though the login succeeded, but no cookie was set, so the user arrives at the application unauthenticated and the application starts a new login. A deterministic failure, an oversized session built from large IdP attribute statements for instance, turns that into an infinite redirect loop through the IdP with nothing in the logs.
With replay_dict configured (#50), each lap also spends that lap's assertion, which contradicts the rule #50 settled: an assertion is spent only where it actually authenticates somebody. The replay angle itself is minor, since the browser follows the redirect and no longer holds the response to re-present.
In login the same ignored save means the callback later answers 503 for want of saml_state, a dead end rather than a loop.
What it should do
login_callback: check sess:save(). On failure, delete the replay keys spent for this response (spend_assertions returning its key list makes that small), log the error, and exit 500. An honest failure instead of a silent loop.login: check sess:save() and fail the request with a logged error rather than redirecting to the IdP with state that was never kept.
Notes
Raised by Copilot reviewing #50. The ignored save predates #50 on both paths; #50 is only where the spent-assertion consequence was added.
What
Both
loginandlogin_callbackcallsess:save()and ignore its result.resty.session'ssavereturnsnil, erron real failure paths: key derivation, encryption, MAC, cookie encoding, storage.In
login_callbackthe failure is silent and the consequence compounds: the code logs "login finish" and redirects as though the login succeeded, but no cookie was set, so the user arrives at the application unauthenticated and the application starts a new login. A deterministic failure, an oversized session built from large IdP attribute statements for instance, turns that into an infinite redirect loop through the IdP with nothing in the logs.With
replay_dictconfigured (#50), each lap also spends that lap's assertion, which contradicts the rule #50 settled: an assertion is spent only where it actually authenticates somebody. The replay angle itself is minor, since the browser follows the redirect and no longer holds the response to re-present.In
loginthe same ignoredsavemeans the callback later answers 503 for want ofsaml_state, a dead end rather than a loop.What it should do
login_callback: checksess:save(). On failure, delete the replay keys spent for this response (spend_assertionsreturning its key list makes that small), log the error, and exit 500. An honest failure instead of a silent loop.login: checksess:save()and fail the request with a logged error rather than redirecting to the IdP with state that was never kept.Notes
Raised by Copilot reviewing #50. The ignored
savepredates #50 on both paths; #50 is only where the spent-assertion consequence was added.