#172: Correctly expire cookies for nullified sessions

[jmitchell38488]

This references task #172.

When a session is set to null, any existing cookies should not only be set to null, but expired. The only way to correctly expire a cookie is by setting the expires flag on the cookie to a date in the past.

I updated the cookie expire logic, since the cookies would just have the data set to null, which wouldn't remove the cookies from the users browser.

By overriding the maxAge value to false and setting the expires to a date in the past, the cookies will be updated by the browser and removed. The expiry date is set as a non-exported file constant.

Signed cookies set by the module cookie will also be removed with this update.

Test case has been added to the test suite.

[coveralls]

Coverage remained the same at 100.0% when pulling 42a192f on jmitchell38488:gh-172 into 10bb122 on koajs:master.

[ejose19]

I've tested this and seems both koa:sess and koa:sess.sig gets properly cleared. I digged a bit more on why koa:sess.sig was not getting an empty string like koa:sess but seems this is handled directly by KeyGrip/cookie module, so no further improvement can be done on this side.

[jmitchell38488]

@ejose19 specifically the cookie module when you set signed to true. The signature cookie was being correctly set, since it was generating a hashed signature of the cookie data. In this case, the hash would be of an empty cookie. The koa:sess cookie would be unset, by setting an empty string, but the koa:sess.sig cookie would remain, as it had data.

Cookies.prototype.set = function(name, value, opts) {
  var res = this.response
    , req = this.request
    , headers = res.getHeader("Set-Cookie") || []
    , secure = this.secure !== undefined ? !!this.secure : req.protocol === 'https' || req.connection.encrypted
    , cookie = new Cookie(name, value, opts)
    , signed = opts && opts.signed !== undefined ? opts.signed : !!this.keys

  if (typeof headers == "string") headers = [headers]

  if (!secure && opts && opts.secure) {
    throw new Error('Cannot send secure cookie over unencrypted connection')
  }

  cookie.secure = secure
  if (opts && "secure" in opts) cookie.secure = opts.secure

  if (opts && "secureProxy" in opts) {
    deprecate('"secureProxy" option; use "secure" option, provide "secure" to constructor if needed')
    cookie.secure = opts.secureProxy
  }

  pushCookie(headers, cookie)

  if (opts && signed) {
    if (!this.keys) throw new Error('.keys required for signed cookies');
    cookie.value = this.keys.sign(cookie.toString())
    cookie.name += ".sig"
    pushCookie(headers, cookie)
  }

  var setHeader = res.set ? http.OutgoingMessage.prototype.setHeader : res.setHeader
  setHeader.call(res, 'Set-Cookie', headers)
  return this
};

[ejose19]

@jmitchell38488 Yes, I understood that part, that's why I said nothing can be done on this module, the cookie is signing a empty string basically so it's expected.

[ejose19]

Looking good, however you should squash all those 5 commits in a single one so this pull request reach the main library in a clean state.

[jmitchell38488]

Looking good, however you should squash all those 5 commits in a single one so this pull request reach the main library in a clean state.

Sure, will do it so that it doesn't have to be done when merged in

[dead-horse]

can we have an assert to check if signed cookie is also set to expired?

[dead-horse]

const COOKIE_EXP_DATE = new Date('Thu, 01 Jan 1970 00:00:00 GMT');

so we don't need to create a date instance every time.

[dead-horse]

others LGTM

[jmitchell38488]

@dead-horse updated