Skip to content
This repository

Cookies are stored and set across domains (security issue) #342

Closed
kainosnoema opened this Issue · 2 comments

3 participants

Evan Owen Mikeal Rogers Jeremy Stashewsky
Evan Owen

The cookie jar implementation (apparently taken directly from LearnBoost/tobi?) only differentiates cookies by path, so cookies are being stored and set across domains.

https://github.com/mikeal/request/blob/master/vendor/cookie/jar.js#L46-55

CookieJar.prototype.get = function(req){
  var path = url.parse(req.url).pathname
    , now = new Date
    , specificity = {};
  return this.cookies.filter(function(cookie){
    if (0 == path.indexOf(cookie.path) && now < cookie.expires
      && cookie.path.length > (specificity[cookie.name] || 0))
      return specificity[cookie.name] = cookie.path.length;
  });
};

This is a major security issue, as well as causing all kinds of problems for long-running processes that hit multiple domains. Cookies build up over time and since every cookie is being sent to every domain, requests fail when the cookie size exceeds server limits.

Since LearnBoost/tobi is a functional testing library, its cookie jar implementation was never intended to be used across multiple domains. The fix here would be to either find a library that handles cookies properly (I haven't found one that's any good yet), write a new cookie jar implementation, or at least fix the current one to respect domains and possibly even limit the number of cookies that can be set per-domain as most browsers do.

I'd be happy to work on a pull-request for this, but I wanted to report it first due to its severity.

Additionally, I found it surprising that cookies were enabled by default for a server-side library. Most use-cases probably don't need cookies, so it only makes sense that the cookie jar should be disabled by default. Just a thought.

Mikeal Rogers
Owner

for real? this sucks.

Paul Springett paulspringett referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
Jeremy Stashewsky stash closed this
Jeremy Stashewsky
Collaborator

@kainosnoema this should be fixed now with the tough-cookie jar implementation. Going to close without investigating but please re-open if it's still happening (unlikely)

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.