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