Move session data storage to database #962

Closed
ginatrapani opened this Issue Sep 11, 2011 · 8 comments

Comments

Projects
None yet
3 participants
@ginatrapani
Owner

ginatrapani commented Sep 11, 2011

Right now, ThinkUp uses PHP's $_SESSION to cache data and store things like the user's logged-in email address and application options. This is inherently insecure on shared hosting services because PHP serializes this data to a system directory, which is readable by other PHP scripts and other server users.

The PHP Security Consortium explains further: http://phpsec.org/projects/guide/5.html

Solution:

Instead of storing the sensitive data in PHP's $_SESSION, ThinkUp will store a unique key in the database and on the client as a cookie. When the user requests a page, TU will retrieve session data by this unique key.

Steps will roughly be:

  • Create tu_session_data table with two fields: key and data
  • Create a SessionDataDAO which writes and reads this data
  • Assign users a unique key when they visit or log into ThinkUp and store it as a browser cookie
  • On each page request, use that cookie to retrieve the session data from the database. We can simply rewrite SessionCache::get and SessionCache::put to use this logic instead of its current $_SESSION-based logic.

One advantage to this new system is that we can make a single public session_data row which every user who is not logged in accesses, which will reduce cache population queries, which now happen on a per-user (even if not logged in) basis.

@hemant-singh

This comment has been minimized.

Show comment Hide comment
@hemant-singh

hemant-singh Apr 11, 2013

I am trying to work out on this issue. Where do you suggest (where in the code) should I assign user a unique key and store it in browser? I have done most of the work, but facing some errors and I think the answer to this question will solve it.

I am trying to work out on this issue. Where do you suggest (where in the code) should I assign user a unique key and store it in browser? I have done most of the work, but facing some errors and I think the answer to this question will solve it.

@ginatrapani

This comment has been minimized.

Show comment Hide comment
@ginatrapani

ginatrapani Apr 12, 2013

Owner

To start, let's generate and store this unique key when a user registers or logs in.

Owner

ginatrapani commented Apr 12, 2013

To start, let's generate and store this unique key when a user registers or logs in.

@ginatrapani

This comment has been minimized.

Show comment Hide comment
@ginatrapani

ginatrapani Apr 12, 2013

Owner

All you have to do is store the unique value and email address in the
sessions table. Then when the user logs in we'll use the email address to
determine if the user is valid and is an admin, as we do now.

On Fri, Apr 12, 2013 at 4:08 AM, Hemant Kumar Singh <
notifications@github.com> wrote:

Currently when the user logs out, two keys of the session are unset,
'user' and 'user_is_admin'. Should I do the same for the new type of
sessions and leave the data in the table, or should i delete the complete
row of the session from the table? What do you suggest?


Reply to this email directly or view it on GitHubhttps://github.com/ginatrapani/ThinkUp/issues/962#issuecomment-16287403
.

http://ginatrapani.org

Owner

ginatrapani commented Apr 12, 2013

All you have to do is store the unique value and email address in the
sessions table. Then when the user logs in we'll use the email address to
determine if the user is valid and is an admin, as we do now.

On Fri, Apr 12, 2013 at 4:08 AM, Hemant Kumar Singh <
notifications@github.com> wrote:

Currently when the user logs out, two keys of the session are unset,
'user' and 'user_is_admin'. Should I do the same for the new type of
sessions and leave the data in the table, or should i delete the complete
row of the session from the table? What do you suggest?


Reply to this email directly or view it on GitHubhttps://github.com/ginatrapani/ThinkUp/issues/962#issuecomment-16287403
.

http://ginatrapani.org

@hemant-singh

This comment has been minimized.

Show comment Hide comment
@hemant-singh

hemant-singh Apr 12, 2013

I have written the whole code of storing all the session data in the
database with the generated md5 hash. I have updated all the required
functions. I have tested it and everything is working fine. Currently I am
running tests as mentioned in the documentation. The test reports are
showing a few issues but none of them are in the files created or modified
by me. I'll submit the code as soon as I solve them.

You can check the code if you want to:
https://github.com/hemant-singh/ThinkUp/tree/962-sessions-using-db
If you want to test the code then there is a new sql file for creating the
required table in webapp/install/sql/mysql_migrations/ named
2013-11_create_tu_session_data_issue962.sql
I am first time working with DAO model. I learnt it from the Think Up code
so, at some places the code may not be as efficient, please excuse those.

On 12 Apr 2013 23:25, "Gina Trapani" notifications@github.com wrote:

All you have to do is store the unique value and email address in the
sessions table. Then when the user logs in we'll use the email address to
determine if the user is valid and is an admin, as we do now.

On Fri, Apr 12, 2013 at 4:08 AM, Hemant Kumar Singh <
notifications@github.com> wrote:

Currently when the user logs out, two keys of the session are unset,
'user' and 'user_is_admin'. Should I do the same for the new type of
sessions and leave the data in the table, or should i delete the
complete
row of the session from the table? What do you suggest?


Reply to this email directly or view it on GitHub<
https://github.com/ginatrapani/ThinkUp/issues/962#issuecomment-16287403>
.

http://ginatrapani.org


Reply to this email directly or view it on GitHub.

I have written the whole code of storing all the session data in the
database with the generated md5 hash. I have updated all the required
functions. I have tested it and everything is working fine. Currently I am
running tests as mentioned in the documentation. The test reports are
showing a few issues but none of them are in the files created or modified
by me. I'll submit the code as soon as I solve them.

You can check the code if you want to:
https://github.com/hemant-singh/ThinkUp/tree/962-sessions-using-db
If you want to test the code then there is a new sql file for creating the
required table in webapp/install/sql/mysql_migrations/ named
2013-11_create_tu_session_data_issue962.sql
I am first time working with DAO model. I learnt it from the Think Up code
so, at some places the code may not be as efficient, please excuse those.

On 12 Apr 2013 23:25, "Gina Trapani" notifications@github.com wrote:

All you have to do is store the unique value and email address in the
sessions table. Then when the user logs in we'll use the email address to
determine if the user is valid and is an admin, as we do now.

On Fri, Apr 12, 2013 at 4:08 AM, Hemant Kumar Singh <
notifications@github.com> wrote:

Currently when the user logs out, two keys of the session are unset,
'user' and 'user_is_admin'. Should I do the same for the new type of
sessions and leave the data in the table, or should i delete the
complete
row of the session from the table? What do you suggest?


Reply to this email directly or view it on GitHub<
https://github.com/ginatrapani/ThinkUp/issues/962#issuecomment-16287403>
.

http://ginatrapani.org


Reply to this email directly or view it on GitHub.

@cdmoyer

This comment has been minimized.

Show comment Hide comment
@cdmoyer

cdmoyer Jan 22, 2014

Contributor

Huh. A question, why not just implement a custom session handler that will allow continued usage of $_SESSION. (since php essentially already defines an interface for session handling, and libraries already exist to use a database, memcache, rest api, etc.)

Contributor

cdmoyer commented Jan 22, 2014

Huh. A question, why not just implement a custom session handler that will allow continued usage of $_SESSION. (since php essentially already defines an interface for session handling, and libraries already exist to use a database, memcache, rest api, etc.)

@ginatrapani

This comment has been minimized.

Show comment Hide comment
@ginatrapani

ginatrapani Jan 22, 2014

Owner

Sure, a custom session handler that writes to the new DB table sounds like a good solution.

Owner

ginatrapani commented Jan 22, 2014

Sure, a custom session handler that writes to the new DB table sounds like a good solution.

cdmoyer added a commit to cdmoyer/ThinkUp that referenced this issue Feb 6, 2014

[DB MIGRATION REQ'D] Implement and Enable DB-Backed Sessions
Creates SessionDAO Interface and MySQL implementation
Creates session table
Adds SessionCache::init() to start sessions and set handlers
Modifies controllers to make use of this

References #962

ginatrapani added a commit that referenced this issue Feb 10, 2014

[DB MIGRATION REQ'D] Implement and Enable DB-Backed Sessions
Creates SessionDAO Interface and MySQL implementation
Creates session table
Adds SessionCache::init() to start sessions and set handlers
Modifies controllers to make use of this

References #962

cdmoyer added a commit to cdmoyer/ThinkUp that referenced this issue Feb 12, 2014

[DB MIGRATION REQ'D] Implement and Enable DB-Backed Sessions
Creates SessionDAO Interface and MySQL implementation
Creates session table
Adds SessionCache::init() to start sessions and set handlers
Modifies controllers to make use of this

References #962
@cdmoyer

This comment has been minimized.

Show comment Hide comment
@cdmoyer

cdmoyer Feb 12, 2014

Contributor

Ahh. I don't understand. Looking at: https://travis-ci.org/ginatrapani/ThinkUp/jobs/18759152

It's marked failed, but I see no failures:

$ php tests/all_unit_tests.php
Model tests
OK
Test cases run: 64/64, Passes: 5211, Failures: 0, Exceptions: 0
Plugin tests
OK
Test cases run: 65/65, Passes: 2695, Failures: 0, Exceptions: 0
Controller tests
rm: cannot remove `build/thinkup/data/.ht*': No such file or directory
rm: cannot remove `build/thinkup/data/.ht*': No such file or directory
rm: cannot remove `build/thinkup/data/.ht*': No such file or directory
rm: cannot remove `build/thinkup/data/.ht*': No such file or directory
rm: cannot remove `build/thinkup/data/.ht*': No such file or directory
rm: cannot remove `build/thinkup/data/.ht*': No such file or directory
rm: cannot remove `build/thinkup/data/.ht*': No such file or directory
rm: cannot remove `build/thinkup/data/.ht*': No such file or directory
rm: cannot remove `build/thinkup/data/.ht*': No such file or directory
rm: cannot remove `build/thinkup/data/.ht*': No such file or directory
OK
Test cases run: 41/41, Passes: 2716, Failures: 0, Exceptions: 0
Total ThinkUp test passes: 10622
Total ThinkUp test failures: 0
Time elapsed: 5 minute(s)
52675 total words of application documentation
Total failures is 0: 0
The command "php tests/all_unit_tests.php" exited with 255.
Done. Your build exited with 1.
Contributor

cdmoyer commented Feb 12, 2014

Ahh. I don't understand. Looking at: https://travis-ci.org/ginatrapani/ThinkUp/jobs/18759152

It's marked failed, but I see no failures:

$ php tests/all_unit_tests.php
Model tests
OK
Test cases run: 64/64, Passes: 5211, Failures: 0, Exceptions: 0
Plugin tests
OK
Test cases run: 65/65, Passes: 2695, Failures: 0, Exceptions: 0
Controller tests
rm: cannot remove `build/thinkup/data/.ht*': No such file or directory
rm: cannot remove `build/thinkup/data/.ht*': No such file or directory
rm: cannot remove `build/thinkup/data/.ht*': No such file or directory
rm: cannot remove `build/thinkup/data/.ht*': No such file or directory
rm: cannot remove `build/thinkup/data/.ht*': No such file or directory
rm: cannot remove `build/thinkup/data/.ht*': No such file or directory
rm: cannot remove `build/thinkup/data/.ht*': No such file or directory
rm: cannot remove `build/thinkup/data/.ht*': No such file or directory
rm: cannot remove `build/thinkup/data/.ht*': No such file or directory
rm: cannot remove `build/thinkup/data/.ht*': No such file or directory
OK
Test cases run: 41/41, Passes: 2716, Failures: 0, Exceptions: 0
Total ThinkUp test passes: 10622
Total ThinkUp test failures: 0
Time elapsed: 5 minute(s)
52675 total words of application documentation
Total failures is 0: 0
The command "php tests/all_unit_tests.php" exited with 255.
Done. Your build exited with 1.
@ginatrapani

This comment has been minimized.

Show comment Hide comment
@ginatrapani

ginatrapani Feb 12, 2014

Owner

Wacky! I'll take a look.

Owner

ginatrapani commented Feb 12, 2014

Wacky! I'll take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment