New session cannot be destroyed

[oldium]

Session created with local s = require "resty.session".start() cannot be directly destroyed with s:destroy(). It fails with

/usr/local/openresty/luajit/share/lua/5.1/resty/session.lua:2077: unable to destroy nonexistent or closed session

Such session is in state new, not open, which is checked in the destroy() call.

This is problem to libraries like lua-resty-openidc, which can get a started session from the application and cannot determine if calling to s:destroy() is safe or not.

[bungle]

@oldium I think there are two ways to fix this: you call :save if session was inexistent OR lib call :save when the session was new here: https://github.com/bungle/lua-resty-session/blob/master/lib/resty/session.lua#L2726.

Perhaps :destroy could be a noop when there is nothing to destroy.

[bungle]

I think why we skipped the save on start when the session was inexistent was that you rarely want to have session that has no data. But making :destroy a noop may be an option for new sessions (that have not been saved yet).

[oldium]

There is a real issue in lua-resty-openidc library. The library accepts either a session configuration (library starts the session) or a started session (application starts the session). Moreover, in case of logout URI the library is responsible for destroying the session.

So if the application decides to always pass-in the opened session, it might happen that when user without cookies visits directly the logout URI, the application opens the new session when calling lua-resty-openidc library to handle the call, and the library tries to destroy the session, because this is what it should do on the session at logout URI.

There is no session API to know that the session is non-existent (freshly created and not-yet saved) and cannot be destroyed. This is only known to the application by checking the return value of session.start, but this is unknown to the library.

It would be good to behave as no-op in this particular case (destroy on new not-yet-saved session). I think this is a reasonable requirement - simply speaking, you should be able to destroy an opened session (any non-destroyed state).

You could also enhance the session API to add the information that the session was new and not yet saved. Anyway, it makes no sense to me to check if it is valid to call destroy on the session, this should be part of the destroy logic.

This worked in 3.x, but stopped working in 4.x, so for me this is a regression.

[bungle]

it might happen that when user without cookies visits directly the logout URI

@oldium, why aren't you then just calling:

require("resty.session").destroy([{ ... config ... }])

Meanwhile I can try to make the session_object:destroy() more resilient to sloppy usage (e.g. code logic trying to destroy non-existing session - the above helper takes care about that too).

The API changed for reason, and the old code should be checked. It is not 1:1 with new. Thus we released it as major 4.x release. What I mean is that in some ways the error seems to point that you are using library in non-optimal way. I know, that is hard to know from outside or when coming from old version. I can think about making it more resilient.

[oldium]

You are in a library, which received already opened session. So what

it might happen that when user without cookies visits directly the logout URI

@oldium, why aren't you then just calling:

require("resty.session").destroy([{ ... config ... }])

Because lua-resty-openidc library also accepts opened session as a parameter, so user already called require("resty.session").open(...) before.

Meanwhile I can try to make the session_object:destroy() more resilient to sloppy usage (e.g. code logic trying to destroy non-existing session - the above helper takes care about that too).

Yes, please. Please modify the session_object:destroy().

And just a side note - according to lua-resty-session documentation the user can call require "resty.session".open(...) to open existing or create a new session. So it is either an existing or new session, but never non-existing session. The non-existing is an implementation detail (cookie does not exist yet) and is not even part of the session instance API (you cannot check for it on a session instance/object).

The API changed for reason, and the old code should be checked. It is not 1:1 with new. Thus we released it as major 4.x release. What I mean is that in some ways the error seems to point that you are using library in non-optimal way. I know, that is hard to know from outside or when coming from old version. I can think about making it more resilient.

That is fine, I already did this in the lua-resty-openidc Pull Request. This is the last bit to solve after migration I am aware of.

[bungle]

@oldium, a couple of notes:

is not even part of the session instance API

Yes, that is what I am thinking. There is session.state which reports the state but it is not documented. This is other way to solve this.

In theory you could just do:

if session.state == "open" then
  session:destroy()
end

I think it is easy to say that making session:destroy being noop is implementation detail, but it is really more than that. So I am now thinking about perhaps instead of erroring just return nil, "cannot destroy inexisting session" (instead of assert error). Then you can ignore err if you don't care (but then people ask, how can I differentiate this from other errors). Not sure yet which way is the best. Having less magical code has its benefits too.

We could add api to tell if session is fresh, then you could do:

if session:exists() then
  session:destroy()
end

Or add safe parameter to session:destroy(true):

Of course you can also do:

pcall(session.destroy, session)

[oldium]

Yes, anything like that. And having session:exists would be nice anyway 😅