HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Cookie encryption library

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
encryptioncookielibrary

Problem

Recently I needed to save session state in cookies, instead of server side. I looked around and didn't see anything similar, so I decided to write something to handle the encryption, decryption, and validation of the cookies. I wanted some opinions before/if I release it on packagist.

Any feedback would be great, thanks

https://github.com/Rosio/Encrypted-Cookies

Below are the important files, the exceptions are just child classes of Exception.

EncryptedCookie.php

```
name = $name;

$this->domain = '';
$this->path = '/';
$this->expiration = 0;
$this->isSecure = false;
$this->isHttpOnly = true;
}

/**
* Load a cookie's data if available.
*
* @throws InputTamperedException If The returned cookie was modified locally.
*/
function load ()
{
if (!$this->getCookieStorage()->has($this->name))
return false;

$data = $this->getCookieStorage()->get($this->name);

try {
$data = $this->getCryptoSystem()->decrypt($data);
}
catch (\Exception $e) {
return false;
}

$this->data = $data;

return $this;
}

/**
* Save a cookie's data.
*
* @throws InputTooLargeException If the encrypted cookie is larger than 4kb (the max size a cookie can be).
*/
function save ()
{
$this->getCookieStorage()->set($this);

return $this;
}

/**
* Get the data for the cookie in its encrypted form.
* @return string
*
* @throws InputTooLargeException If the encrypted cookie's data will be truncated by the browser.
*/
function getEncryptedData ()
{
$edata = $this->getCryptoSystem()->encrypt($this->data, $this->expiration);

if (strlen($edata) >= 4096)
throw new InputTooLargeException('Total encrypted data must be less than 4kb, or it will be truncated on the client.');

return $edata;
}

/* ===

Solution

Overall your design looks good for me. Nothing really bad here. Some suggestions on your architecture though:

Your cookie implementation is dependent on the CryptoSystem and the CookieStorage. But then a Cookie doesn't actually care about how it is stored or if its content is encrypted. This makes your cookie class very long though with three different responsibilities: representation of the cookie itself, storing & encryption. Furthermore, storing and encryption just delegate.

I'd consider a cookie as a simple value object which can be created everywhere (without a factory). The storage on the other hand is responsible for how the information is stored. One possibility of this how seems to be encrypted for me.

My two recommendations regarding the architecture are:

  • Remove storage and encryption from the cookie class



  • Make a cookie-storage that encrypts / decrypts on save/load



This puts each responsibility in exactly one class and slightly less coupling. Furthermore you the factory is not required any more :) .The result would end up like:

class Cookie
{
    protected $name;
    protected $data;

    protected $domain;
    protected $path;
    protected $expiration;
    protected $isSecure;
    protected $isHttpOnly;

    // getters/setters here
}

class EncryptedCookieStorage {
    public function __construct(iCryptoSystem $crypto, $group = null) {};
}

// usage
$cookie = new Cookie();
$storage->save($cookie);


Some other remarks not related to this suggestion:

  • I usually prefer constructor injection of required dependencies and setter injection for optional dependencies. If your class requires more then two dependencies it most likely has too many responsibilities.



  • I use protected only if I want subclasses to modify variables. This is almost never the case though. Someone sublcassing my class might forget some integrity checks or whatever. Subclasses should use getters/setters for this in my opinion.



  • I hope all your exceptions inherit from a common abstraction specific for this component.



  • Not sure if cookie expiration is really an exception



  • Do type-validation on your scalars (or where you expect scalars): CookieFactory::get(new \Exception) is probably not what you expect.

Code Snippets

class Cookie
{
    protected $name;
    protected $data;

    protected $domain;
    protected $path;
    protected $expiration;
    protected $isSecure;
    protected $isHttpOnly;

    // getters/setters here
}

class EncryptedCookieStorage {
    public function __construct(iCryptoSystem $crypto, $group = null) {};
}

// usage
$cookie = new Cookie();
$storage->save($cookie);

Context

StackExchange Code Review Q#42704, answer score: 3

Revisions (0)

No revisions yet.