php-http / message

HTTP Message related tools
http://php-http.org
MIT License
1.29k stars 41 forks source link

Cookie for root path (/) matches only exact match. #65

Closed Maff- closed 7 years ago

Maff- commented 7 years ago
Q A
Bug? yes
New Feature? no
Version v1.4.0

Actual Behavior

When having a Cookie with the path set to the domain root (e.g. /), and checking to see if another path matches (i.e. /cookies) it will always fail. (Except when checking an exact match, /).

Expected Behavior

I expect path /cookies to match a Cookie with path /.

Steps to Reproduce

$cookie = new Cookie('foo', 'bar', null, 'example.com', '/');
var_dump($cookie->matchPath('/cookies')); // = false, but should be true
// spec\Http\Message\CookieSpec
function it_matches_the_root_path()
{
    $this->beConstructedWith('name', 'value', null, null, '/');
    $this->matchPath('/')->shouldReturn(true);
    // we should add this line below
    $this->matchPath('/cookies')->shouldReturn(true);
}

Possible Solutions

A solution shouldn't be to hard to find. My guess is that adding a trailing slash isn't completely correct, but reading the linked rfc section doesn't make things easier. 😕

current code. not a solution!:

/**
 * Checks whether this cookie is meant for this path.
 * @see http://tools.ietf.org/html/rfc6265#section-5.1.4
 */
public function matchPath($path)
{
    return $this->path === $path || (strpos($path, $this->path.'/') === 0);
}
sagikazarmark commented 7 years ago

You seem to be right. The problem is that '/' becomes '//'. The trailing slash is necessary because of how the comparison works, otherwise /cookies and /cookiesAndCakes would be a match as well. Adding rtrim sounds like a solution to me, but we need to add some test cases as well.

Also, I am not 100% sure that only 1.4 is affected and this is something that I would backport as a fix as well to all affected minor versions.

Maff- commented 7 years ago

Hi Márk, thank you for your response. I've created a PR with the proposed fix.

sagikazarmark commented 7 years ago

Fixed in https://github.com/php-http/message/commit/5fd6c761b994b16afaae4a236abb650e5a068b5a