A Cookie maxAge of undefined causes incorrect behavior.

[cjbarth]

In lib/response.js in res.cookie() some assumptions are made that the incoming maxAge option will always be a number. However, if maxAge is set to undefined through some process, opts.maxAge /= 1000 returns NaN. maxAge should be verified and/or coerced to be numeric.

[dougwilson]

Yep, makes sense. To throw or to coerce would depend on what happens currently with the various types. Ideally we should throw, but if there are other types currently working or the set-cookie header is still valid then we may just need to coerce for now.

When it is undefined as your description, can you describe what happens? Does it cause a throw currently or does the set-cookie header get set? If it is set, can you paste the value here?

[cjbarth]

The relavent two lines are:

    opts.expires = new Date(Date.now() + opts.maxAge);
    opts.maxAge /= 1000;

If maxAge = undefined, then opts.expires becomes NaN and so does opts.maxAge. These values are then passed to node_modules/cookie/index.js's serialize function, which throws because these are bad values. Depending on how the code is structured, and where in the promise tree one may be, this could cause no response to ever be returned.

[dougwilson]

Ok, so it already throws when maxAge is set to a non number? That would seem to me to be the expected behavior. Is it just that the error message thrown is confusing or something?

[cjbarth]

Throwing is good, and the error is clear, however, there is a dependency on a downstream dependent library to behave correctly instead of Express actually doing the right thing. Express should never be asking for NaN to be set for a cookies expires or maxAge value. Also, it is unexpected that setting a cookie should ever throw when setting maxAge to undefined because that would logically coerce to 0 like null does, thus I didn't wrap my call to res.cookie() in a Try/Catch, so this took me a while to find.

The only symptom I had was Express would never return. This was exspecially strange because, due to a refactor, this value was being set to null, which worked fine since that calculates as 0, and it was changed to undefined, which caused a throw, which is very unexpected from the consumers point of view. The refactor was to be able to turn on Node strictNullsChecks flag.

[dougwilson]

Gotcha. So thinking a bit more of it, do you think that res.cookie({}) should produce the identical output as res.cookie({ maxAge: undefined })? Accessing .maxAge property on both of these example objects results in undefined, so if currently only the second is throwing, should the second produce the same set-cookie header as the first? Why or why not?

[cjbarth]

The only problem with that is, the setting of maxAge: undefined is a deliberate action which caused one behavior for maxAge in opts, verses not setting maxAge at all creates different behaviors. I feel like we should handle the case of deliberate consumer action vs. no consumer action as different.

Really, my problem came from thinking that under the hood null and undefined were largely interchangeable, as I would imagine most developers assume.

Perhaps instead of maxAge in opts it should be opts.maxAge != null, which is normally how I would code that as the intention is very clear. I'm actually surprised that there isn't a numeric check here given what we are doing with the date.

[dougwilson]

Right. The code is from before I working on Express, so cannot answer as to why it was done that way. But that doesn't take away from some of the concerns which is mainly (1) not break current use cases and (2) what should be the right resolution.

[dougwilson]

The main downside to using 'in' is that it may not indicate explicit user action. For example:

$ node -pe 'Object.prototype.maxAge=undefined; ("maxAge" in {})'
true

So just the property existing on the global prototype makes it return true, even though an empty object is what was passed in by the user (and all the user is aware of passing in).

[cjbarth]

That is exactly my point, which is why I made the PR the way I did. It doesn't break any current use-cases, but it does causes undefined to behave like null does, which is what a user would expect. It also checks to make sure that a number isn't used, but that breaks Node 0.10, so I'll probably remove that check as it isn't directly related to the problem under discussion here.

[dougwilson]

It just feels weird. Because if it makes it 0, then the cookie will immediately expire. That change would make there be no possible way to explicitly state that the cookie should be a session length cookie, then?

[cjbarth]

That is a very good point...

If the goal is to get null and undefined to behave the same, when we need to change maxAge in opts to maxAge != null. If the goal is to have undefined behave consistently, then we have to use maxAge !== undefined.

It seems if we want to preserve session cookies and the current null behavior, option 2 is our only choice, though it feels like the wrong 'correct' choice as a consumer would encounter different behavior with the both falsey and non-numeric null and undefined.

Is this a breaking change that should go in v.Next?

[dougwilson]

If it's a breaking change, then yes. If not, we can land in 4.x. We would want to land as much as possible in 4.x, ideally. If there is a breaking change here, we'll want to lay it out what is changing and such so it's documented for when the 4 to 5 migration guide is written.

[cjbarth]

Technically all of this is a change in behavior, but I think we can probably change maxAge in opts to maxAge !== undefined and fix the specific case of setting undefined and expecting the same behavior as not having set maxAge at all.

My case where I expected null and undefined to behave the same could be argued to be a breaking change to make work, but could also easily be argued that null working is a fluke as we would never want to be adding null to anything or dividing null by anything, and thus is a bug that should be fixed.

Thus the question is: is maxAge being anything but a number and us not handling that case, but relying on JavaScript implementation details, a bug? I would argue that if it wasn't set to a number it probably shouldn't be used at all and the bug is insufficient checks which should be fixed whenever, however, I'll concede to your more seasoned opinion. I probably shouldn't have been passing null or undefined when what I really wanted was 0.

I'll update my PR at your direction.

[dougwilson]

Yes, I mean, fixing a bug is typically always a breaking change in the pure sense. That doesn't mean we don't fix them anyway, of course :) If it's iffy but desirable, they are typically included in minor versions instead of patch versions, but still fixed within the major.

is maxAge being anything but a number and us not handling that case, but relying on JavaScript implementation details, a bug?

So in a way we relay on javascript details for almost everything, since this is javascript. I know that's not what you mean, but just not sure how to really answer that question generically.

For the specific of "should we be dividing a non-number by a number, I would say no, unless there is some special reason, we should above it. That would mean the act of doing null / 1000 etc. is a bug at least in the logic. The second question is how does this logic bug manifest to our users?

Assuming that is passed in doesn't have a special valueOf() defined (like a Date object) the outcome of the division is going to be NaN and even though the cookie module catches that, ideally this module wouldn't pass that down, specifically because it created the value of NaN (not the user).

This would, to me, say everything except numbers and null are basically always going to result in NaN and thus we can just not do the division on the value. The handling of null is the only tricky one, because null actually coerces into the number 0, so maxAge: null results in 0 in Express (but no max age in cookie module).

I would say from that, we have two divisions of changes:

(1) stop dividing all non-numbers and non-null values and let maxAge pass into cookie module as-is (and don't populate expires). This would let cookie module handle the validation and error instead of re-implementing it here. This will still cause things like { maxAge: function () {} } throw, but that feels right to me because it's unsupported to pass a function as the value. I don't think any behavior will change from this, apart from the undefined behavior no longer throwing.

(2) ideally null to Express would just work the same as null to the cookie module for this value, that is it acts like undefined. This is the "tricky" bug because perhaps people are using that to delete cookies, like perhaps (res.cookie('foo', '', { maxAge: null })). Yes, weird, but based on other things changed and then ha to revert in the past I would not be surprised. We should fix this behavior in the 5.0 branch at minimum. Coming back to 4.x it's either (a) do nothing (b) add deprecation warning to null value or (c) change null behavior. I lay those out because I'm undecided on the action on this for 4.x

[cjbarth]

I follow your logic and would vote against option 2a for sure, that just pushes the problem down the road.

For option 1, I would say that is a rather significant breaking change because the units that are used in cookie and those that Express accepts are different. Additionally, cookie won't set expires unless specifically instructed to do so, and Express will automatically calculate Expires if maxAge is specified (which enhances compatibility with older IE, so good). Thus, I would say that we don't want to depend entirely on cookie, thus option 1 is a no-go IMO.

If we are going to fix this behavior in the 5.0 branch, then I say we just put a deprecation warning here identifying the desired behavior (must pass a number, else maxAge and expires aren't set) and leave it at that so we don't have to roll anything back when 5.0 is released. That is to say, option 2b.

[dougwilson]

I think you may have mis-read my number 1 (or maybe I wasn't clear?). Numbers would still be divided and expires still calculated with number 1.

[cjbarth]

I now see what you mean. In that case, this does now seem to make sense. That seems like it would be the right long-term solution (if cookie is going to process cookies, let them). However, for now, it seems we still need to put in a warning perhaps pointing to the documentation for cookie.

[davidmashe]

no discussion for some time on the issue, closing.

[cjbarth]

I'm still interested in this being resolved. Please reopen.

[ghinks]

thank you @cjbarth I have a tiny example of this here. It looks like the initial action to take would be to make a deprecation notice in express 4. Would you like to raise PR for this?

[dougwilson]

I would like to add that we are working hard to get 5.0 out the door as soon as possible. Unless something critical comes up, 4.18 will be the last minor release of 4.x prior to the final 5.0, which means that for the underlying breaking change to even be considered for 5.0 at this point, we would want to get a 4.x-targeting deprecation ready quickly to get into the 4.18 release. There is always 6.0, and don't let that scare you, either; we have also been at work discussing and then planning after 5.0 in order for 6.0 to not sit anywhere near as long as 5.0 did 😂

[cjbarth]

This isn't much notice, especially given how long this item has sat dormant. I'll see what I can do about a deprecation notice in the next day or two.

Just to be clear, what we are looking for right now in the 4.x branch is to have a deprecated warning issued if any non-number is passed in.

Then, in the 5.x branch, detect if the value passed in is a non-number, and then avoid division on that number and pass it directly to cookie and let them handle the case; we'd then change the warning from a 'deprecation' warning to just a regular one, since we'd still want to warn that something strange is going on (getting a non-number and expecting an number).

Is that what we'd all like to see?

[cjbarth]

Just checking again before I proceed; is the above how we'd like to proceed?

[dougwilson]

Last night I was just going over this issue again to try and wrap my mind around all the topics we spoke about.

Before I write up my thoughts here, I did want to ask to confirm something. From your POV, what do you believe the goal is? This is just to get back in sync with each other.

I believe at least one goal (maybe the only one?) is that setting maxAge to undefined should produce a cookie header of max-age=0?

If that is the case, I think we need to revisit a little bit, as that is counter to what the underlying cookie module does on that value. Typically in javascript { foo: undefined } and {} both have an undefined foo property.

I believe that a specific undefined should work the same as if the property was not specified.

As for null that one is up for debate, but seems to make to most sense to signal a session-length expiration cookie.

For other non numbers like strings and stuff, those should error out for sure. I believe the underlying module will already do this, but we could likely guard for it higher?

[cjbarth]

Goals:

1. Never perform math on a non-number, including null and undefined
2. A property being defined as undefined and a property not defined should be treated the same
3. Behavior for null and undefined should be either treated (or coalesced to be) the same, or be the problem of the downstream library, or other?

The problem is that some of these might be considered a breaking change, though I would argue that really we just have a bug.

The other problem is for 3, what should the correct behavior be? Should they be coalesced to 0, or sent to the downstream library. If they are just sent along, how do we handle the case of undefined being set and undefined resulting from a property not being set. I could see an argument for them being coalesced to 0 because that is the only logical meaning, but OTOH undefined often means to just use the default (which I'd have to refresh my memory of what that is).

Does that make sense?

[dougwilson]

I know this has kind of gotten forgotten in the pile of issues, but I am here trying to go through them all :)

So yea, the goals there I think make sense. Basically this module is using maxAge to both add expires and manipulate it to represent the same value in the underlying library (i.e. milliseconds to seconds). As such, it should just use the same logic to determine if maxAge is "there" as the underlying library does -- I updated your PR as such.

This makes it such that maxAge: undefined works the same as any other option, same with null. As on top of that, keeps the same number coercion the underlying library does.