Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:56745
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.161.170 as permitted sender)
MIME-Version: 1.0
Sender: yohgaki@gmail.com
In-Reply-To: <4EDA9788.5070901@sugarcrm.com>
References: <CAGa2bXYDbDnTi1aF4Ts9oShyy=2NKPaTc34_+Hg7ZN0v+_MGSQ@mail.gmail.com>
 <4EBDC283.3040700@yahoo.co.jp> <CAGa2bXaMKzQCO=6k4WrB2W3Mp6R8R7oJz7LQBnv_4UKCk0_VxQ@mail.gmail.com>
 <4ED727FB.7030001@uw.no> <CAGa2bXb9k=V1X-jfx5NM=0RrRo2RwxyG6HjaKWLZHLb2Tpe7jQ@mail.gmail.com>
 <4EDA9788.5070901@sugarcrm.com>
Date: Sun, 4 Dec 2011 17:32:13 +0900
Message-ID: <CAGa2bXaK8UDUo2Z-1zHF2qWZHoXyqYRfheb=TUtqbQRxvooSyQ@mail.gmail.com>
To: Stas Malyshev <smalyshev@sugarcrm.com>
Cc: "internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=ISO-8859-1
Subject: Re: [PHP-DEV] Strict session?
From: yohgaki@ohgaki.net (Yasuo Ohgaki)

Hi Stats,

I updated the patch to address discussion.

https://gist.github.com/1379668

2011/12/4 Stas Malyshev <smalyshev@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.

I updated the patch and fixed them all.

There is no API incompatibility also, since I added
PS_MOD_SID2/PS_MOD_FUNCS_SID2. Thanks Daniel K.

> 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.

I consolidated key validation function into session.c.

Session ID was created by php_session_create_id() function by default.
So there weren't 3 creation function, but key validation functions.

I hope you feel comfortable with new patch.

>
> 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 for catching this.

I added fallback to php_session_create_id() with SHA1 hash as hash
function. Even if users/ps_module writers did something wrong in their
code, it always has valid PS(id). It may be better to raise notice or
warning error, but I don't add it. Any comments?

I think the patch is much better than the original thanks to discussion.

Any comments are appreciated.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net