fix cookies' secure detect

[dead-horse]

koa don't change the original request object, need to pass koa's request object to cookies.

(but you can't replace res with response, so this is kind of weird. :(

[juliangruber]

seems like there should be a less wtf way to do this

[dead-horse]

1. koa set req.protocol, but we don't want to change the original request object, right?
2. cookies' constructor support specified secure. like new Cookies(req, res, keys, request.secure)

both sound wtf to me. :(

[dead-horse]

or maybe we just set req.protocol before new Cookies(req, res, keys). /cc @jonathanong @dougwilson any idea?

[dougwilson]

So, I wound recommend keeping the first argument as the raw Node.js object, since that is what is documented to work with the module (and it could start using a new property at any time, breaking Koa integration).

The root issue has been brought up various times and it is quite an annoying issue :) I can see any of the following a better fix than the proposal in this PR:

1. Add the ability to just skip the secure check in the cookies module (you can do this currently by just setting { secureProxy: true } instead of { secure: true } when calling the cookies module, but it's kinda "eh".
2. Just change the secure setting in cookies to not check the connection at all, and just have the consumer of the module make the appropriate decision. The secureProxy setting is literally this, so the proposal is to just rename secureProxy to secure, effectively.
3. Add the ability to specify a function to the cookies call that would pass it a req, res and get a return value of if it's secure or not.

[dead-horse]

@dougwilson 1&2 both need user set some options when using cookies.set ?

3 sound more reasonable, but compare to specify a function, why not just specify secure like new Cookies(req, res, keys, secure), because we already know req and res before create cookies object and can figure out it's secure or not.

[dougwilson]

why not just specify secure like new Cookies(req, res, keys, secure) , because we already know req and res before create cookies object and can figure out it's secure or not.

That is option 2. The consumer of the module is Koa, because "the module" = cookies.

[dougwilson]

As in, Koa could easily do the following:

opts.secureProxy = opts.secure && request.secure
opts.secure = undefined

[dead-horse]

people use this.cookies.set() need to set opts.secureProxy = ops.secure && request.secure. otherwise koa need to override cookies.set to handle this logic?

[dougwilson]

So @jonathanong was pushing me to take a serious look into this situation. Let me know what you think of the following proposal that would get this fixed as well as let the cookies module figure out it's path to 1.0 independently of fixing this issue:

1. Add a new options parameter to the Cookies constructor.
2. Accept a new isSecure option, defaulting to undefined. Setting to a Boolean will be the answer for the secure checks in .set
3. I think this pull request can then boil down to changing the line context.cookies = new Cookies(req, res, this.keys); to context.cookies = new Cookies(req, res, this.keys, { isSecure: request.protocol === 'https' });

If the static isSecure does not work, that can also be a function. Initial thoughts?

[dougwilson]

Also, I know some of this has been discussed above, but really this is a summary of various proposals from above put down as something to get done now :)

[jonathanong]

yes taht should be fine. i was thinking about how this would interact with proxy-addr, but i guess koa will handle it. let me look into proxy-addr

[dougwilson]

My assumption would be that cookies should maybe be made dumber and not doing the secure check? If we do that, then one could use proxy-addr perhaps like the following:

new Cookies(req, res, keys, {
  isSecure: proxyAddr.isSecure(req, rules)
})

Or something.

[jonathanong]

yeah i like cookies being dumb. after all, request.secure should be proxyAddr.isSecure(req, rules) anyways

[jonathanong]

@dead-horse after pillarjs/cookies#68 we should be good to go :)

[dougwilson]

Ok, with the current changes proposed in pillarjs/cookies#68 this pull request would be

context.cookies = new Cookies(req, res, { keys: this.keys, secure: request.secure });

[dead-horse]

upgrade to [email protected]. /cc @dougwilson @jonathanong

[PlasmaPower]

What exactly does this do? res.socket is already defined as socket 2 lines up, right?

[dead-horse]

some test case will init context with specific req without socket like https://github.com/koajs/koa/blob/master/test/request/path.js#L26

[PlasmaPower]

Oh oops, I missed that. Thanks for the help.

[dougwilson]

Looks good to me.

[jonathanong]

👍

[fengmk2]

Wow! Wait for a long time.