Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

binary safe session persistence #33

Merged

Conversation

dnsl48
Copy link
Contributor

@dnsl48 dnsl48 commented Jun 27, 2019

FIX AWS SDK PHP DynamoDb StandardSessionConnection handles serialized sessions as strings, which is not binary safe

fixes #32

@dnsl48 dnsl48 force-pushed the fix/session-binary-safety branch from 38b706f to e304c55 Compare June 27, 2019 03:27
code/DynamoDbClient.php Show resolved Hide resolved
@dnsl48 dnsl48 force-pushed the fix/session-binary-safety branch from e304c55 to a1a0eac Compare July 1, 2019 03:21
@dnsl48
Copy link
Contributor Author

dnsl48 commented Jul 1, 2019

ready for a review

@dnsl48 dnsl48 force-pushed the fix/session-binary-safety branch 2 times, most recently from 59b958f to 4031818 Compare July 1, 2019 03:55
@dnsl48 dnsl48 force-pushed the fix/session-binary-safety branch from 4031818 to fc97543 Compare July 1, 2019 04:02
@chillu
Copy link
Member

chillu commented Jul 1, 2019

Works as advertised :) Was particularly interested in the transition of existing sessions once the new functionality is used. Tested with DynamoDB Local, see #34.

Add the following code to PageController->init():

$this->getRequest()->getSession()->set('foo', 'bar');

Use master branch. Visit /?flush=1. Observe session being created with string values:

aws dynamodb scan --table-name mysession --endpoint-url http://localhost:8000
{
    "Count": 1, 
    "Items": [
        {
            "expires": {
                "N": "1562024548"
            }, 
            "data": {
                "S": "foo|s:3:\"bar\";HTTP_USER_AGENT|s:121:\"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/75.0.3770.100 Safari/537.36\";"
            }, 
            "id": {
                "S": "PHPSESSID_7r6bcvrbb1cu8anch8img3lerp"
            }
        }
    ], 
    "ScannedCount": 1, 
    "ConsumedCapacity": null
}

Switch to PR branch. Visit /?flush=1 (in same browser session). Observe session data in DynamoDB hasn't changed.

Read out existing session value (in controller or elsewhere):

var_dump($this->getRequest()->getSession()->get('foo'));die();

Change the PageController->init() to a different session value:

$this->getRequest()->getSession()->set('foo', 'baz');

Visit /?flush=1. Observe session data changed to binary:

{
    "Count": 1, 
    "Items": [
        {
            "expires": {
                "N": "1562024689"
            }, 
            "data": {
                "B": "Zm9vfHM6MzoiYmF6IjtIVFRQX1VTRVJfQUdFTlR8czoxMjE6Ik1vemlsbGEvNS4wIChNYWNpbnRvc2g7IEludGVsIE1hYyBPUyBYIDEwXzE0XzUpIEFwcGxlV2ViS2l0LzUzNy4zNiAoS0hUTUwsIGxpa2UgR2Vja28pIENocm9tZS83NS4wLjM3NzAuMTAwIFNhZmFyaS81MzcuMzYiOw=="
            }, 
            "id": {
                "S": "PHPSESSID_7r6bcvrbb1cu8anch8img3lerp"
            }
        }
    ], 
    "ScannedCount": 1, 
    "ConsumedCapacity": null
}

Read out session value, observe new value:

var_dump($this->getRequest()->getSession()->get('foo'));die();

@chillu chillu merged commit 793f933 into silverstripe:master Jul 1, 2019
@chillu chillu deleted the fix/session-binary-safety branch July 1, 2019 23:25
@dnsl48 dnsl48 mentioned this pull request Jul 3, 2019
dnsl48 added a commit to open-sausages/silverstripe-dynamodb that referenced this pull request Jul 11, 2019
…sion-binary-safety"

This reverts commit 793f933, reversing
changes made to 785c697.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Session storage is not binary safe
2 participants