Cookie expiration uses local time instead of UTC

[mattjohnsonpint]

Description

System.Net.Cookie has multiple usages of DateTime.Now, which should generally be avoided in server-side code because they would be impacted by any DST transitions of the local time zone.

runtime/src/libraries/System.Net.Primitives/src/System/Net/Cookie.cs

Line 66 in 0c7c10e

private DateTime m_timeStamp = DateTime.Now; // Do not rename (binary serialization)

runtime/src/libraries/System.Net.Primitives/src/System/Net/Cookie.cs

Lines 198 to 211 in 0c7c10e

	public bool Expired
	{
	get
	{
	return (m_expires != DateTime.MinValue) && (m_expires.ToLocalTime() <= DateTime.Now);
	}
	set
	{
	if (value)
	{
	m_expires = DateTime.Now;
	}
	}
	}

runtime/src/libraries/System.Net.Primitives/src/System/Net/Cookie.cs

Lines 802 to 812 in 0c7c10e

	if (Expires != DateTime.MinValue)
	{
	int seconds = (int)(Expires.ToLocalTime() - DateTime.Now).TotalSeconds;
	if (seconds < 0)
	{
	// This means that the cookie has already expired. Set Max-Age to 0
	// so that the client will discard the cookie immediately.
	seconds = 0;
	}
	result += SeparatorLiteral + CookieFields.MaxAgeAttributeName + EqualsLiteral + seconds.ToString(NumberFormatInfo.InvariantInfo);
	}

All of these should be using DateTime.UtcNow, and should then use ToUniversalTime, not ToLocalTime.

This probably has gone unnoticed because it's a best practice for production servers to have their local time zone set to UTC.
Also, cookies are often set for arbitrarily long timeframes - so an hour off in the expiration might also go unnoticed. Still, it's a bug that should be resolved (IMHO).

Reproduction Steps

The following will usually fail on a computer whose local time zone alternates between two offsets, as is common for daylight saving time. For example, Pacific Standard Time (America/Los_Angeles). It should be good enough to demonstrate. A more robust test would examine the output of TimeZoneInfo.GetAdjustmentRules to choose a target date that ensures crossing a DST transition.

using System.Diagnostics;
using System.Net;
using System.Reflection;

var sec = 6 * 30 * 24 * 60 * 60; // 15552000 (approx 6 months)
var dt = DateTime.UtcNow.AddSeconds(sec); 

var c = new Cookie
{
    Name = "foo",
    Value = "bar",
    Expires = dt
};

// Max-Age is only output in ToServerString, which is non-public.
var str = typeof(Cookie)
    .GetMethod("ToServerString", BindingFlags.Instance | BindingFlags.NonPublic)?
    .Invoke(c, null)
    as string;

// The result could be exact, or could be 1 second off due to delay in code execution.
var expected1 = $"{c}; Max-Age={sec-1}";
var expected2 = $"{c}; Max-Age={sec}";

Debug.Assert(str == expected1 || str == expected2,
    "Cookie Max-Age has been affected by a local DST transition.",
    "Expected \"{0}\" or \"{1}\", got \"{2}\"",
    expected1, expected2, str);

Expected behavior

The assertion should pass, because the Max-Age value should approximate the amount of time originally added to DateTime.UtcNow.

Actual behavior

The assertion fails when there's a DST transition in the local time zone between the time of creation and the expiration date. The Max-Age value is one hour off, because it was generated based on local time, and subtraction of two DateTime values does not consider time zone transitions.

Regression?

Pretty sure it's been here all along. .NET Framework reference sources show the same code.

Known Workarounds

If the server's local time zone is UTC, or another time zone that does not undergo any transitions, then the problem does not manifest.

Configuration

.NET 8 latest. Not platform-specific.

Other information

No response

Tagging subscribers to this area: @dotnet/area-system-datetime
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

System.Net.Cookie has multiple usages of DateTime.Now`, which should generally be avoided in server-side code because they would be impacted by any DST transitions of the local time zone.

runtime/src/libraries/System.Net.Primitives/src/System/Net/Cookie.cs

Line 66 in 0c7c10e

private DateTime m_timeStamp = DateTime.Now; // Do not rename (binary serialization)

runtime/src/libraries/System.Net.Primitives/src/System/Net/Cookie.cs

Lines 198 to 211 in 0c7c10e

	public bool Expired
	{
	get
	{
	return (m_expires != DateTime.MinValue) && (m_expires.ToLocalTime() <= DateTime.Now);
	}
	set
	{
	if (value)
	{
	m_expires = DateTime.Now;
	}
	}
	}

runtime/src/libraries/System.Net.Primitives/src/System/Net/Cookie.cs

Lines 802 to 812 in 0c7c10e

	if (Expires != DateTime.MinValue)
	{
	int seconds = (int)(Expires.ToLocalTime() - DateTime.Now).TotalSeconds;
	if (seconds < 0)
	{
	// This means that the cookie has already expired. Set Max-Age to 0
	// so that the client will discard the cookie immediately.
	seconds = 0;
	}
	result += SeparatorLiteral + CookieFields.MaxAgeAttributeName + EqualsLiteral + seconds.ToString(NumberFormatInfo.InvariantInfo);
	}

All of these should be using DateTime.UtcNow, and should then use ToUniversalTime, not ToLocalTime.

This probably has gone unnoticed because it's a best practice for production servers to have their local time zone set to UTC.
Also, cookies are often set for arbitrarily long timeframes - so an hour off in the expiration might also go unnoticed. Still, it's a bug that should be resolved (IMHO).

Reproduction Steps

The following will usually fail on a computer whose local time zone alternates between two offsets, as is common for daylight saving time. For example, Pacific Standard Time (America/Los_Angeles). It should be good enough to demonstrate. A more robust test would examine the output of TimeZoneInfo.GetAdjustmentRules to choose a target date that ensures crossing a DST transition.

using System.Diagnostics;
using System.Net;
using System.Reflection;

var sec = 6 * 30 * 24 * 60 * 60; // 15552000 (approx 6 months)
var dt = DateTime.UtcNow.AddSeconds(sec); 

var c = new Cookie
{
    Name = "foo",
    Value = "bar",
    Expires = dt
};

// Max-Age is only output in ToServerString, which is non-public.
var str = typeof(Cookie)
    .GetMethod("ToServerString", BindingFlags.Instance | BindingFlags.NonPublic)?
    .Invoke(c, null)
    as string;

// The result could be exact, or could be 1 second off due to delay in code execution.
var expected1 = $"{c}; Max-Age={sec-1}";
var expected2 = $"{c}; Max-Age={sec}";

Debug.Assert(str == expected1 || str == expected2,
    "Cookie Max-Age has been affected by a local DST transition.",
    "Expected \"{0}\" or \"{1}\", got \"{2}\"",
    expected1, expected2, str);

Expected behavior

The assertion should pass, because the Max-Age value should approximate the amount of time originally added to DateTime.UtcNow.

Actual behavior

The assertion fails when there's a DST transition in the local time zone between the time of creation and the expiration date. The Max-Age value is one hour off, because it was generated based on local time, and subtraction of two DateTime values does not consider time zone transitions.

Regression?

Pretty sure it's been here all along. .NET Framework reference sources show the same code.

Known Workarounds

If the server's local time zone is UTC, or another time zone that does not undergo any transitions, then the problem does not manifest.

Configuration

.NET 8 latest. Not platform-specific.

Other information

No response

Author:	mattjohnsonpint
Assignees:	-
Labels:	area-System.DateTime
Milestone:	-

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

System.Net.Cookie has multiple usages of DateTime.Now, which should generally be avoided in server-side code because they would be impacted by any DST transitions of the local time zone.

runtime/src/libraries/System.Net.Primitives/src/System/Net/Cookie.cs

Line 66 in 0c7c10e

private DateTime m_timeStamp = DateTime.Now; // Do not rename (binary serialization)

runtime/src/libraries/System.Net.Primitives/src/System/Net/Cookie.cs

Lines 198 to 211 in 0c7c10e

	public bool Expired
	{
	get
	{
	return (m_expires != DateTime.MinValue) && (m_expires.ToLocalTime() <= DateTime.Now);
	}
	set
	{
	if (value)
	{
	m_expires = DateTime.Now;
	}
	}
	}

runtime/src/libraries/System.Net.Primitives/src/System/Net/Cookie.cs

Lines 802 to 812 in 0c7c10e

	if (Expires != DateTime.MinValue)
	{
	int seconds = (int)(Expires.ToLocalTime() - DateTime.Now).TotalSeconds;
	if (seconds < 0)
	{
	// This means that the cookie has already expired. Set Max-Age to 0
	// so that the client will discard the cookie immediately.
	seconds = 0;
	}
	result += SeparatorLiteral + CookieFields.MaxAgeAttributeName + EqualsLiteral + seconds.ToString(NumberFormatInfo.InvariantInfo);
	}

All of these should be using DateTime.UtcNow, and should then use ToUniversalTime, not ToLocalTime.

This probably has gone unnoticed because it's a best practice for production servers to have their local time zone set to UTC.
Also, cookies are often set for arbitrarily long timeframes - so an hour off in the expiration might also go unnoticed. Still, it's a bug that should be resolved (IMHO).

Reproduction Steps

The following will usually fail on a computer whose local time zone alternates between two offsets, as is common for daylight saving time. For example, Pacific Standard Time (America/Los_Angeles). It should be good enough to demonstrate. A more robust test would examine the output of TimeZoneInfo.GetAdjustmentRules to choose a target date that ensures crossing a DST transition.

using System.Diagnostics;
using System.Net;
using System.Reflection;

var sec = 6 * 30 * 24 * 60 * 60; // 15552000 (approx 6 months)
var dt = DateTime.UtcNow.AddSeconds(sec); 

var c = new Cookie
{
    Name = "foo",
    Value = "bar",
    Expires = dt
};

// Max-Age is only output in ToServerString, which is non-public.
var str = typeof(Cookie)
    .GetMethod("ToServerString", BindingFlags.Instance | BindingFlags.NonPublic)?
    .Invoke(c, null)
    as string;

// The result could be exact, or could be 1 second off due to delay in code execution.
var expected1 = $"{c}; Max-Age={sec-1}";
var expected2 = $"{c}; Max-Age={sec}";

Debug.Assert(str == expected1 || str == expected2,
    "Cookie Max-Age has been affected by a local DST transition.",
    "Expected \"{0}\" or \"{1}\", got \"{2}\"",
    expected1, expected2, str);

Expected behavior

The assertion should pass, because the Max-Age value should approximate the amount of time originally added to DateTime.UtcNow.

Actual behavior

The assertion fails when there's a DST transition in the local time zone between the time of creation and the expiration date. The Max-Age value is one hour off, because it was generated based on local time, and subtraction of two DateTime values does not consider time zone transitions.

Regression?

Pretty sure it's been here all along. .NET Framework reference sources show the same code.

Known Workarounds

If the server's local time zone is UTC, or another time zone that does not undergo any transitions, then the problem does not manifest.

Configuration

.NET 8 latest. Not platform-specific.

Other information

No response

Author:	mattjohnsonpint
Assignees:	-
Labels:	area-System.Net, untriaged
Milestone:	-

[unknovvn]

As a person living in a timezone, which uses daylight saving time - I can agree with @tarekgh that this is a bug, and it should be fixed. Although it is a good practice to have a server's local time zone set as UTC - not everyone follows it, and it could have serious implications on side effects coming with current approach. Change itself is pretty basic and doesn't seem to have any side effects, which makes it a good candidate to be followed and executed ASAP (IMHO). It would also help people (like me) who were implementing custom auth logic and were not sure how to correctly set CookieOptions Expires property.

[rzikm]

per RFC 2616, Section 3.3.1

HTTP applications have historically allowed three different formats for the representation of date/time stamps:

  Sun, 06 Nov 1994 08:49:37 GMT  ; [RFC 822](https://datatracker.ietf.org/doc/html/rfc822), updated by [RFC 1123](https://datatracker.ietf.org/doc/html/rfc1123)
  Sunday, 06-Nov-94 08:49:37 GMT ; [RFC 850](https://datatracker.ietf.org/doc/html/rfc850), obsoleted by [RFC 1036](https://datatracker.ietf.org/doc/html/rfc1036)
  Sun Nov  6 08:49:37 1994       ; ANSI C's asctime() format

....

All HTTP date/time stamps MUST be represented in Greenwich Mean Time
(GMT), without exception. For the purposes of HTTP, GMT is exactly
equal to UTC (Coordinated Universal Time). This is indicated in the
first two formats by the inclusion of "GMT" as the three-letter
abbreviation for time zone, and MUST be assumed when reading the
asctime format.

ensuring UTC is the right thing to do.