bungle / lua-resty-session

Session library for OpenResty – flexible and secure
BSD 2-Clause "Simplified" License
320 stars 111 forks source link

Fixed some problems, when same lua script using session handles different hosts #4

Closed kipras closed 9 years ago

kipras commented 9 years ago

Session module creates a module local data structure, that contains initial configuration. When lua code cache is enabled (default) the same module instance is used for every request and it ends up using the same values from that data structure.

This patch fixes cookie.secure and cookie.domain, both of which can differ when handling different requests.

However, to support different configurations - the entire session data structure must be initialized differently. Perhaps allow the user to specify a key by which a specific instance of session static data should be retrieved. Or use a default host + ssl_session_id key.

bungle commented 9 years ago

Hi, thanks. I will look at this.

bungle commented 9 years ago

Hi, I'm not sure I understand this patch. Renaming fields doesn't make this local issue go away (require "resty.session" caching).

You can specify session specific parameters right now by giving them as opts to session.start, e.g.:

local session = require "resty.session".start{ cookie = {
    secure = false,
    domain = "yourdomain.com"
}}

But you are right, from Nginx side of the things the defaults are read only once and after that they are used as defaults if not overwritten with session.start options.

kipras commented 9 years ago

Hey, it's not a rename but a move of the field, from inner session.cookie table to parent session table, which has a metatable. The reason why it fixes the issue is that within session.start(opts) there is for example (assume passed opts are empty):

self.cookie.domain = ngx_var.host

What this effectively does is store ngx.var.host (of the current request) to module local session.cookie table, because session.cookie is retrieved by the metatable __index method, and is therefore used by all requests (instead of using some sort of local copy).

However, if you do:

self.cookie_domain = ngx_var.host

what happens is that if there is no session.cookie_domain set in the module local table - then a new cookie_domain value is created, but not in the module local session data structure, but in the self table, which is local to session.start() and therefore only used for this request.

Actually, at first i did

session.cookie.__index = session.cookie
...
function session.start(opts)
    ...
    if self.cookie.domain == nil then
        self.cookie = setmetatable({}, session.cookie)
        self.cookie.domain = ngx_var.host
    end

but this current approach seemed cleaner.

However, you are right - you can just pass these parameters as opts, and i think i will do that instead :) Because this fix only fixes these two properties, and we should probably either fix all of them or none of them.

It is a bit cumbersome still, because if you want to pass a single value from an inner table (e.g. cookie.domain) - you have to pass all of cookie values. It should probably merge those values (override session.cookie values with values from opts.cookie) somehow?

bungle commented 9 years ago

Hi,

Thanks. I can see what you are talking about here now. Yes, this is a problem. I will look at it. And you are right that it is cumbersome, and the merge is better. Thanks for pointing this out. And thanks for the pull-request. I will look if there is a better way to handle this.

bungle commented 9 years ago

Hi,

I just checked in my first solution to this problem (see: https://github.com/bungle/lua-resty-session/commit/89b5c213da384ce50fba4069b2d5e659294f5dbd).

I added a new function as well session.new:

local session = require "resty.session"
-- session.new doesn't attach to current session yet
local s = session.new{ cookie = { persistent = true } }
-- because we didn't yet start the session, we can set config parameters invidually too
s.cookie.domain = "mydomain.com"
-- here we attach to the current session (or in case it is absent, start a new one),
-- see we call this with colon (:) here (which is same as calling s.start(s))
s:start()
s.data.uid = 124
s:save()

Or you can use session.start directly as before (but you don't need to supply all parameters):

local session = require "resty.session"
-- here we call start with dot (.)
local s = session.start{ cookie = { persistent = true, domain = "mydomain.com" } } }
s.data.uid = 124
s:save()

I didn't find (at least yet), a more elegant way to do this. Now, we should document that nginx side of configs are used as defaults, and those defaults are read only once (reading them on each request comes with a performance penalty, and that's the reason we try to avoid it as much as possible). So if you have multiple sites with different configurations, set only the 'common' configs from the Nginx side, and others from Lua code by etiher passing them to session.new or session.start.

Please let me know what you think about it (the latest commit). And if you know a more elegant way to do this, let me know that as well

Regards Aapo

bungle commented 9 years ago

I just released version 1.4 that contains fixes to this issue. Please reopen if there are still problems with it or that you feel that there is a better way to solve this. Thank you again for finding this out!

kipras commented 9 years ago

The updated version fixes the problem. Thanks!