Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .editorconfig
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,6 @@ charset = utf-8
insert_final_newline = true
trim_trailing_whitespace = true

[{*.js,*.json,*.yml}]
[{*.js,*.ts,*.json,*.yml}]
indent_size = 2
indent_style = space
13 changes: 12 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,24 @@
"uid-safe": "~2.1.5"
},
"devDependencies": {
"@types/cookie": "^0.6.0",
"@types/cookie-signature": "^1.0.7",
"@types/debug": "^4.1.12",
"@types/depd": "^1.1.37",
"@types/express": "^5.0.0",
"@types/node": "^18.19.80",
"@types/on-headers": "~1.0.2",
"@types/parseurl": "~1.3.3",
"@types/uid-safe": "~2.1.5",
"after": "0.8.2",
"cookie-parser": "1.4.6",
"eslint": "8.56.0",
"eslint-plugin-markdown": "3.0.1",
"express": "4.17.3",
"mocha": "10.2.0",
"nyc": "15.1.0",
"supertest": "6.3.4"
"supertest": "6.3.4",
"typescript": "5.8.2"
},
"files": [
"session/",
Expand All @@ -38,6 +48,7 @@
},
"scripts": {
"lint": "eslint . && node ./scripts/lint-readme.js",
"check-types": "tsc --project ./tsconfig.json",
"test": "./test/support/gencert.sh && mocha --require test/support/env --check-leaks --no-exit --reporter spec test/",
"test-ci": "nyc --reporter=lcov --reporter=text npm test",
"test-cov": "nyc npm test",
Expand Down
95 changes: 95 additions & 0 deletions session/cookie-options.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
export declare interface CookieOptions {
/**
* Specifies the number (in milliseconds) to use when calculating the `Expires Set-Cookie` attribute.
* This is done by taking the current server time and adding `maxAge` milliseconds to the value to calculate an `Expires` datetime. By default, no maximum age is set.
*
* If both `expires` and `maxAge` are set in the options, then the last one defined in the object is what is used.
* `maxAge` should be preferred over `expires`.
*
* @see expires
*/
maxAge?: number | undefined;

/**
* Specifies the `boolean` value for the [`Partitioned` `Set-Cookie`](https://tools.ietf.org/html/draft-cutler-httpbis-partitioned-cookies/)
* attribute. When truthy, the `Partitioned` attribute is set, otherwise it is not.
* By default, the `Partitioned` attribute is not set.
*
* **Note** This is an attribute that has not yet been fully standardized, and may
* change in the future. This also means many clients may ignore this attribute until
* they understand it.
*/
partitioned?: boolean | undefined;

/**
* Specifies the `string` to be the value for the [`Priority` `Set-Cookie` attribute](https://tools.ietf.org/html/draft-west-cookie-priority-00#section-4.1).
*
* - `'low'` will set the `Priority` attribute to `Low`.
* - `'medium'` will set the `Priority` attribute to `Medium`, the default priority when not set.
* - `'high'` will set the `Priority` attribute to `High`.
*
* More information about the different priority levels can be found in
* [the specification](https://tools.ietf.org/html/draft-west-cookie-priority-00#section-4.1).
*
* **Note** This is an attribute that has not yet been fully standardized, and may change in the future.
* This also means many clients may ignore this attribute until they understand it.
*/
priority?: "low" | "medium" | "high" | undefined;

signed?: boolean | undefined;

/**
* Specifies the boolean value for the `HttpOnly Set-Cookie` attribute. When truthy, the `HttpOnly` attribute is set, otherwise it is not.
* By default, the `HttpOnly` attribute is set.
*
* Be careful when setting this to `true`, as compliant clients will not allow client-side JavaScript to see the cookie in `document.cookie`.
*/
httpOnly?: boolean | undefined;

/**
* Specifies the value for the `Path Set-Cookie` attribute.
* By default, this is set to '/', which is the root path of the domain.
*/
path?: string | undefined;

/**
* Specifies the value for the `Domain Set-Cookie` attribute.
* By default, no domain is set, and most clients will consider the cookie to apply to only the current domain.
*/
domain?: string | undefined;

/**
* Specifies the boolean value for the `Secure Set-Cookie` attribute. When truthy, the `Secure` attribute is set, otherwise it is not. By default, the `Secure` attribute is not set.
* Be careful when setting this to true, as compliant clients will not send the cookie back to the server in the future if the browser does not have an HTTPS connection.
*
* Please note that `secure: true` is a **recommended option**.
* However, it requires an https-enabled website, i.e., HTTPS is necessary for secure cookies.
* If `secure` is set, and you access your site over HTTP, **the cookie will not be set**.
*
* The cookie.secure option can also be set to the special value `auto` to have this setting automatically match the determined security of the connection.
* Be careful when using this setting if the site is available both as HTTP and HTTPS, as once the cookie is set on HTTPS, it will no longer be visible over HTTP.
* This is useful when the Express "trust proxy" setting is properly setup to simplify development vs production configuration.
*
* If you have your node.js behind a proxy and are using `secure: true`, you need to set "trust proxy" in express. Please see the [README](https://github.com/expressjs/session) for details.
*
* Please see the [README](https://github.com/expressjs/session) for an example of using secure cookies in production, but allowing for testing in development based on NODE_ENV.
*/
secure?: boolean | "auto" | undefined;

encode?: ((val: string) => string) | undefined;

/**
* Specifies the boolean or string to be the value for the `SameSite Set-Cookie` attribute.
* - `true` will set the `SameSite` attribute to `Strict` for strict same site enforcement.
* - `false` will not set the `SameSite` attribute.
* - `lax` will set the `SameSite` attribute to `Lax` for lax same site enforcement.
* - `none` will set the `SameSite` attribute to `None` for an explicit cross-site cookie.
* - `strict` will set the `SameSite` attribute to `Strict` for strict same site enforcement.
*
* More information about the different enforcement levels can be found in the specification.
*
* **Note:** This is an attribute that has not yet been fully standardized, and may change in the future.
* This also means many clients may ignore this attribute until they understand it.
*/
sameSite?: boolean | "lax" | "strict" | "none" | undefined;
}
192 changes: 114 additions & 78 deletions session/cookie.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,145 +8,181 @@
'use strict';

/**
* Module dependencies.
* @import { CookieSerializeOptions } from "cookie"
* @import { CookieOptions } from "./cookie-options"
*/

var cookie = require('cookie')
var deprecate = require('depd')('express-session')

/**
* Initialize a new `Cookie` with the given `options`.
*
* @param {IncomingMessage} req
* @param {Object} options
* @api private
* Cookie TODO: add description
* @class
* @implements CookieOptions
*/

var Cookie = module.exports = function Cookie(options) {
this.path = '/';
this.maxAge = null;
this.httpOnly = true;

if (options) {
if (typeof options !== 'object') {
throw new TypeError('argument options must be a object')
}
class Cookie {
/** @type {Date | undefined} @private */
_expires;
/** @type {number | undefined} */
originalMaxAge;
/** @type {boolean | undefined} */
partitioned;
/** @type { "low" | "medium" | "high" | undefined} */
priority;
/** @type {boolean | undefined} */
signed; // FIXME: how this is used??
/** @type {boolean} */
httpOnly;
/** @type {string} */
path;
/** @type {string | undefined} */
domain;
/** @type {boolean | "auto" | undefined} */
secure;
/** @type {((val: string) => string) | undefined} */
encode;
/** @type {boolean | "lax" | "strict" | "none" | undefined} */
sameSite;

for (var key in options) {
if (key !== 'data') {
this[key] = options[key]
/**
* Initialize a new `Cookie` with the given `options`.
* @param {CookieOptions} options
* @private
*/
constructor(options) {
if (options) {
if (typeof options !== 'object') {
throw new TypeError('argument options must be a object')
}
console.log(`CookieOptions: ${JSON.stringify(options)}`)
this.maxAge = options.maxAge
this.originalMaxAge ??= options.maxAge // FIXME: rethink this

this.partitioned = options.partitioned
this.priority = options.priority
this.secure = options.secure
this.httpOnly = options.httpOnly ?? true
this.domain = options.domain
this.path = options.path || '/'
this.sameSite = options.sameSite
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a for loop here is not good from the type checking point of view (the for loop doesn't understand what type keys are).

The Object.assign(this, options) does work here, but then we need to skip options.data property some how. For some reason, people have been passing it here and it fails as data is readonly for Cookie.

As a good result, this fixes the problem with data field, which was special case. In addition, the code is quite clean.

As a down side, now these properties are hardcoded in quite many places and changes to those require care.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the second iteration, I used the old code, just added type overwrites, so that the for loop works. I think the outdated code is kind of better, but I prefer the less rewriting style at this point.


this.signed = options.signed // FIXME: how this is used??
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason there are undocumented properties in the type file. I'm didn't yet search what these were, why and for what purpose.

this.encode = options.encode // FIXME: is this used / real ??
} else {
this.path = '/'
this.httpOnly = true
}
}

if (this.originalMaxAge === undefined || this.originalMaxAge === null) {
this.originalMaxAge = this.maxAge
/**
* Initialize a new `Cookie` using stored cookie data.
* @param {CookieOptions & {expires?: string, originalMaxAge?: number}} data
* @returns {Cookie}
* @protected
*/
static fromJSON(data) {
console.log(`Cookie.fromJSON: ${JSON.stringify(data)}`)
const { expires, originalMaxAge, ...options } = data
const cookie = new Cookie(options)
cookie.expires = expires ? new Date(expires) : undefined
cookie.originalMaxAge = originalMaxAge
return cookie
}
};

/*!
* Prototype.
*/

Cookie.prototype = {

/**
* Set expires `date`.
*
* @param {Date} date
* @api public
* @param {Date | null | undefined} date
* @public
*/

set expires(date) {
this._expires = date;
this.originalMaxAge = this.maxAge;
},
this._expires = date || undefined
this.originalMaxAge = this.maxAge
}

/**
* Get expires `date`.
* Get expires `Date` object to be the value for the `Expires Set-Cookie` attribute.
* By default, no expiration is set, and most clients will consider this a "non-persistent cookie" and will delete it on a condition like exiting a web browser application.
*
* @return {Date}
* @api public
* @return {Date | undefined}
* @public
*/

get expires() {
return this._expires;
},
return this._expires
}

/**
* Set expires via max-age in `ms`.
*
* @param {Number} ms
* @api public
* @param {number | undefined} ms
* @public
*/

set maxAge(ms) {
if (ms && typeof ms !== 'number' && !(ms instanceof Date)) {
throw new TypeError('maxAge must be a number or Date')
}

if (ms instanceof Date) {
deprecate('maxAge as Date; pass number of milliseconds instead')
if (ms !== undefined) {
if (typeof ms !== 'number') {
throw new TypeError('maxAge must be a number')
}
this.expires = new Date(Date.now() + ms)
} else {
this.expires = undefined
}

this.expires = typeof ms === 'number'
? new Date(Date.now() + ms)
: ms;
},
}

/**
* Get expires max-age in `ms`.
*
* @return {Number}
* @api public
* @return {number | undefined}
* @public
*/

get maxAge() {
return this.expires instanceof Date
? this.expires.valueOf() - Date.now()
: this.expires;
},
: this.expires
}

/**
* Return cookie data object.
*
* @return {Object}
* @api private
* @return {CookieSerializeOptions}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This interface was and is still used for multiple purposes. The code isn't very clear who it's serving.

Specifying this type, it makes it clear it has a single purpose. As a result, the property could be renamed.

For now, I used this for toJSON, but it might be better to separate these two.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second iteration went back to the current API, which is a mix. As a result, it now uses object as type.

* @private
*/

get data() {
if (this.secure === 'auto') {
throw new Error("Invalid runtime state, the Cookie.secure == 'auto', which should not be possible.")
}
return {
originalMaxAge: this.originalMaxAge,
partitioned: this.partitioned,
priority: this.priority
, expires: this._expires
, expires: this.expires
, secure: this.secure
, httpOnly: this.httpOnly
, domain: this.domain
, path: this.path
, sameSite: this.sameSite
}
},

/**
* Return a serialized cookie string.
*
* @return {String}
* @api public
*/

serialize: function(name, val){
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this is used by external Store implementations. In this repo, this was used only by the test code. The version in index.js calls cookie.serialize directly on purpose or by accident.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First commit removed this method, but the second is more about just adding types and converting to a class, hence this method is back. The above comment still applies, and this method probably could be removed.

return cookie.serialize(name, val, this.data);
},
}

/**
* Return JSON representation of this cookie.
*
* @return {Object}
* @api private
* Used by `JSON.stringify`
*
* @returns {Object}
* @protected
*/

toJSON: function(){
return this.data;
toJSON() {
const data = {
...this.data,
expires: this.expires,
originalMaxAge: this.originalMaxAge,
}
console.log(`Cookie.toJSON: ${JSON.stringify(data)}`)
return data
}
};
}

module.exports = Cookie
Loading