Hi Stats, 2011/12/4 Stas Malyshev <smalys...@sugarcrm.com>: > Hi! > > My main concern with this change is that it is binary incompatible with > existing session implementation, which means it would be hard to get it into > 5.3 and 5.4. While I understand sometimes adding handlers is inevitable, in > this case I'm not sure why it is required. So far the only reason I > understand was "If we do this, users cannot know if ps_modules is new one > that prevents adoption or not, especially for third party ps_modules." but > this is definitely a documentation issue and even with new API you can not > really know that - you just know there's a handler but you can not know the > module implements it properly - maybe it just always returns OK.
Programmer has freedom to do bad thing, but it's important for better security to have dedicated API for validation. From my experience, it's much easier let programmers to use API, but to code properly. Programmers may use strict session using methods that are written in wiki page. However, most of applications don't have proper protection. One reason would be lack of API, I suppose. > > Maybe we can make separate binary-compatible patch for 5.3/5.4 branches and > patch with new APIs for trunk? The user-space handlers for 5.3/5.4 would If we care about binary API compatibility, how about make PS_MOD_SID2/PS_MOD_FUNCS_SID2 macros? Then we can forget about ABI. > still have to do the checks (though stock modules will have the checks > built-in) but for trunk they could do it by defining validation callbacks. > > I also still not sure why default SID creation function was changed to 3 > identical cloned functions. I do not think it's the right way - if some > macro is not allowing us to leave it as is, we can have another macro, but I > do not see how splitting one standard SID creation function into a number of > identical clones is beneficial. I think it needs to be changed. Ok. I'll make a default char validation function for this and make it call from mod_*.c If ps_module writers would like to use special chars for whatever reason, they may use their own function. > > For user module, with the patch if you add validation handler you also have > to add create_sid handler - which most users don't even want to change. We > should allow for using standard SID creation functions there (maybe by > passing NULL there? or by having some function that implements > php_session_create_id, maybe even better), and we shouldn't check > validate_sid when we mean create_sid - if we allow to set create_sid, we > should also support case where we set create_sid but not validate_sid. > Also, all other modules when SID creation fails set invalid_sid and return > NULL, while new user module SID creation returns "" and doesn't set > invalid_sid. Why is that? > > We also probably need to deal then with the question what should be done > when create_sid fails (I'd say we can not do much but issue fatal error but > maybe there are any better ideas) if we allow it to fail. We don't seem to > check invalid_sid now after create_sid, so if we're setting it, we must > check it. Thank you. I'll take care of this. If script or ps_module returns empty SID, it should generate SID by using default SID creation function. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
