Skip to content

Loading…

[HttpFoundation] Serialization error with PdoSessionStorage #3000

Closed
pkruithof opened this Issue · 11 comments

3 participants

@pkruithof

When using PdoSessionStorage, an exception is thrown by PHP when the session is serialized:

PDOException: You cannot serialize or unserialize PDO instances

I think the Session class needs to check if the storage can be serialized, maybe by using a NotSerializable interface, as @fabpot blogged about a while ago.

FYI: this issue is caused by Symfony\Bundle\FrameworkBundle\DataCollector\RequestDataCollector::serialize() and only occurs when I forward a request. I'm using the latest Symfony version to date (2.0.8).

@ghost

What would you suggest doing here? Make an effort to serialize it or throw an error? I guess the reason PDO is not serializable is that it would require access to the DB connection details which should not be available for display. Without those, you cant really unserialize and so an Exception is correct.

@pkruithof

Maybe it's better to delegate the serialization to the storage object. That way the PdoSessionStorage class can provide the serialized data without the connection details.

@ghost

I'm not sure the problem is where there serialization happens, it's that you cannot serialize PDO instances in the first plac because they contain sensitive data which would be required for the unserialization process. The real question is, what do do in this circumstance. The storage object contains the PDO instance, if we blot it out, then when we unserialize the object we will have a non-functional storage device. I mean you could use reflection to extract out the PDO privates, but then it's a security issue. Maybe I don't see it clearly yet, but I don't see a way forward (unless we change the PDO object to a proxy - but the issue of serialising sensitive data would remain anyway). Have you any suggestions?

@ghost

The example on fabpot's blog is ok for the case of exceptions, but the forward action surely needs to unserialize the session object so just blotting out the PDO object is not really ok, if I'm seeing it correctly?

@pkruithof

I think you're right. However I'm not really familiar with the inner workings of the sessions, maybe @fabpot or @stof have a suggestion for this?

@pkruithof

Any new thoughts on this yet?

@ghost Unknown referenced this issue
Merged

Session cleanup #3333

@ghost

So basically, we can add the Serializable interface to PDOSessionStorage, it will stop the exception but afaik, it will not be enough because we actually need to unserialize again. Is there any particular reason we have to serilaize the session in order to forward a request anyhow? /cc @stof /cc @fabpot

@stof
Symfony member

is it needed to forward the request ? The place where the exception occured here is the DataCollector. In this place, we only need to be able to display the content of the session in the profiler. We don't need to be able to access the database again (as the database would have changed anyway). So serializing the session does not need to serialize the storage for the profiler, but only its content.

@fabpot do we serialize the session in another place ?

@ghost

If the unserialisation doesn't matter then there are two approached we can take.

Either, simply add the \Serializable interface to any storage driver that is not serialisable, or add a method to the SessionStorageInterface, as isSerializable() which the Session class can process. I can make a PR after I know if unserialisation matters or not.

Remember this issue exists already since Symfony 2.0

/cc @fabpot

@fabpot fabpot added a commit that referenced this issue
@fabpot fabpot merged branch stof/session_cleanup (PR #3333)
Commits
-------

2c767d1 [HttpFoundation] Fixed closeSession for the Memcached storage
ec44e68 [HttpFoundation] Fixed the use of the prefix for the Memcached storage
5808773 [HttpFoundation] Fixed a typo and updated the phpdoc for the session
0550bef Removed methods duplicated from parent class

Discussion
----------

Session cleanup

This cleans the phpdoc of the refactored session by using inheritdoc in all appropriate places. For the SessionInterface, I added the ``@api`` tag in all methods as the Session class had them.
It also fixes a few typos in the variable names.

I figured a few things:

- the Session class implements some methods that are not part of the SessionInterface. Is it intended or simply a left-over ?
- the MemcachedSessionStorage uses a ``prefix`` property which does not exist. This is clearly a copy-paste from the MemcacheSessionStorage which has it. It also call the ``Memcached#close`` method which does not exist according to PhpStorm. @drak could you check this class ? I don't know Memcached at all.
- as I said on the refactoring PR, the Serializable implementation of the Session class seems totally wrong as the SessionStorage is not serializable.

/cc @drak

---------------------------------------------------------------------------

by drak at 2012-02-12T02:09:38Z

@stof there is a ticket about this, the problem exists from Symfony 2.0 also - refs #3000 PDOSessionStorage

---------------------------------------------------------------------------

by drak at 2012-02-12T02:10:12Z

@stof I will look at the memcache issue and make a quick PR

---------------------------------------------------------------------------

by stof at 2012-02-12T02:11:07Z

@drak the issue could exist with any SessionStorage as your SessionStorageInterface does not enforce making it serializable

---------------------------------------------------------------------------

by stof at 2012-02-12T02:14:15Z

@drak note that this PR already fixes 2 typo in the memcached storage. It seems like some tests are missing for it as they should have been found (they would have raised a notice in the constructor)

---------------------------------------------------------------------------

by drak at 2012-02-12T02:14:50Z

Afaik, only PDO is not serializable@ but the `Serializable` is on the SessionInterface.  The problem is with #3000, the serialization is necessary but impossible.

---------------------------------------------------------------------------

by stof at 2012-02-12T02:21:42Z

@drak what about a Mongo storage or things like that ?

---------------------------------------------------------------------------

by drak at 2012-02-12T02:23:58Z

@stof Since you've started this PR would you mind removing `$prefix.` from the `*session()` methods in `MemcachedSessionStorage` - they are not required because Memcached handles prefixes itself.  I'll write some tests in another PR for them.  I guess I omitted it because at the time I didn't have them on my test env.

---------------------------------------------------------------------------

by drak at 2012-02-12T02:24:35Z

@stof - Let's take the serialization issue to #3000 because it's being discussed there.

---------------------------------------------------------------------------

by stof at 2012-02-12T02:27:27Z

@drak what about the ``close()`` method which does not exist in the Memcached class ?

---------------------------------------------------------------------------

by drak at 2012-02-12T03:58:11Z

The method just needs to `return true;`.
f6ee66f
@fabpot
Symfony member

I'm not able to reproduce the error on master and I don't see where the session is actually serialized (even in 2.0). Serializing the session is anyway a bad idea; what we need to do is serialize the data within the session but not the session itself. So, I've removed the \Serialize interface for the session for now and if it breaks something, I will happily fix it in the proper way.

@fabpot
Symfony member

Closing this issue as a fix has been committed with no negative feedback on it.

@fabpot fabpot closed this
@mmucklo mmucklo pushed a commit that referenced this issue
@fabpot fabpot merged branch stof/session_cleanup (PR #3333)
Commits
-------

2c767d1 [HttpFoundation] Fixed closeSession for the Memcached storage
ec44e68 [HttpFoundation] Fixed the use of the prefix for the Memcached storage
5808773 [HttpFoundation] Fixed a typo and updated the phpdoc for the session
0550bef Removed methods duplicated from parent class

Discussion
----------

Session cleanup

This cleans the phpdoc of the refactored session by using inheritdoc in all appropriate places. For the SessionInterface, I added the ``@api`` tag in all methods as the Session class had them.
It also fixes a few typos in the variable names.

I figured a few things:

- the Session class implements some methods that are not part of the SessionInterface. Is it intended or simply a left-over ?
- the MemcachedSessionStorage uses a ``prefix`` property which does not exist. This is clearly a copy-paste from the MemcacheSessionStorage which has it. It also call the ``Memcached#close`` method which does not exist according to PhpStorm. @drak could you check this class ? I don't know Memcached at all.
- as I said on the refactoring PR, the Serializable implementation of the Session class seems totally wrong as the SessionStorage is not serializable.

/cc @drak

---------------------------------------------------------------------------

by drak at 2012-02-12T02:09:38Z

@stof there is a ticket about this, the problem exists from Symfony 2.0 also - refs #3000 PDOSessionStorage

---------------------------------------------------------------------------

by drak at 2012-02-12T02:10:12Z

@stof I will look at the memcache issue and make a quick PR

---------------------------------------------------------------------------

by stof at 2012-02-12T02:11:07Z

@drak the issue could exist with any SessionStorage as your SessionStorageInterface does not enforce making it serializable

---------------------------------------------------------------------------

by stof at 2012-02-12T02:14:15Z

@drak note that this PR already fixes 2 typo in the memcached storage. It seems like some tests are missing for it as they should have been found (they would have raised a notice in the constructor)

---------------------------------------------------------------------------

by drak at 2012-02-12T02:14:50Z

Afaik, only PDO is not serializable@ but the `Serializable` is on the SessionInterface.  The problem is with #3000, the serialization is necessary but impossible.

---------------------------------------------------------------------------

by stof at 2012-02-12T02:21:42Z

@drak what about a Mongo storage or things like that ?

---------------------------------------------------------------------------

by drak at 2012-02-12T02:23:58Z

@stof Since you've started this PR would you mind removing `$prefix.` from the `*session()` methods in `MemcachedSessionStorage` - they are not required because Memcached handles prefixes itself.  I'll write some tests in another PR for them.  I guess I omitted it because at the time I didn't have them on my test env.

---------------------------------------------------------------------------

by drak at 2012-02-12T02:24:35Z

@stof - Let's take the serialization issue to #3000 because it's being discussed there.

---------------------------------------------------------------------------

by stof at 2012-02-12T02:27:27Z

@drak what about the ``close()`` method which does not exist in the Memcached class ?

---------------------------------------------------------------------------

by drak at 2012-02-12T03:58:11Z

The method just needs to `return true;`.
49e3071
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.