Skip to content

Commit

Permalink
fix: Define default value for Cookie Max-Age (#10775)
Browse files Browse the repository at this point in the history
The Cookie interface is updated with a constant value to represent an
undefined Max-Age, and its JavaDocs are updated to explicitly state
that the undefined value should not be encoded.

The CookieHttpCookieAdapter implementation is updated to explicitly set
the required undefined max age value in its constructor.

The new constant is consistent with the behavior of the Netty Cookie
implementation, and is meant to enforce consistency between it and the
HttpCookie based implementation, if only by explicitly stating the
intended contract.

The CookieFactory service definition is removed from the http module
as HttpCookieFactory already gets loaded by default (if no other
service definitions are loaded) via CookieFactory.INSTANCE. This
allows other explicit services definitions such as that in http-netty
to reliably override the default implementation.

Tests are added to verify the undefined max age value in both
implementations, and to ensure that NettyServerCookieEncoder can
correctly encode a Cookie created by the default HttpCookieFactory.

* test: DefaultServerCookieEncoder encoding test

* Only set if -1
  • Loading branch information
jeremyg484 authored and sdelamo committed Apr 29, 2024
1 parent 87a1dbc commit 585c99b
Show file tree
Hide file tree
Showing 7 changed files with 112 additions and 3 deletions.
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package io.micronaut.http.netty.cookies

import io.micronaut.http.cookie.Cookie
import io.micronaut.http.cookie.CookieFactory
import spock.lang.Specification

Expand All @@ -9,4 +10,9 @@ class CookieFactorySpec extends Specification {
expect:
CookieFactory.INSTANCE instanceof NettyCookieFactory
}

void "default cookie is a netty cookie with max age undefined"() {
expect:
Cookie.of("SID", "31d4d96e407aad42").getMaxAge() == Cookie.UNDEFINED_MAX_AGE
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package io.micronaut.http.netty.cookies

import io.micronaut.http.cookie.Cookie
import io.micronaut.http.cookie.HttpCookieFactory
import io.micronaut.http.cookie.SameSite
import io.micronaut.http.cookie.ServerCookieEncoder
import spock.lang.Specification
Expand Down Expand Up @@ -46,6 +47,41 @@ class NettyServerCookieEncoderSpec extends Specification {
expected == result || expected2 == result || expected3 == result
}

void "netty server cookie encoder can correctly encode a cookie from HttpCookieFactory"() {
given:
HttpCookieFactory factory = new HttpCookieFactory();
ServerCookieEncoder cookieEncoder = new NettyServerCookieEncoder()

when:
Cookie cookie = factory.create("SID", "31d4d96e407aad42").path("/").domain("example.com")

then:
"SID=31d4d96e407aad42; Path=/; Domain=example.com" == cookieEncoder.encode(cookie)[0]

when:
cookie = factory.create("SID", "31d4d96e407aad42").path("/").domain("example.com").sameSite(SameSite.Strict)

then:
"SID=31d4d96e407aad42; Path=/; Domain=example.com; SameSite=Strict" == cookieEncoder.encode(cookie)[0]

when:
cookie = factory.create("SID", "31d4d96e407aad42").path("/").secure().httpOnly()

then: 'Netty uses HTTPOnly instead of HttpOnly'
"SID=31d4d96e407aad42; Path=/; Secure; HTTPOnly" == cookieEncoder.encode(cookie)[0]

when:
long maxAge = 2592000
cookie = factory.create("id", "a3fWa").maxAge(maxAge)
String result = cookieEncoder.encode(cookie).get(0)
String expected = "id=a3fWa; Max-Age=2592000; " + Cookie.ATTRIBUTE_EXPIRES + "=" + expires(maxAge)
String expected2 = "id=a3fWa; Max-Age=2592000; " + Cookie.ATTRIBUTE_EXPIRES + "=" + expires(maxAge + 1) // To prevent flakiness
String expected3 = "id=a3fWa; Max-Age=2592000; " + Cookie.ATTRIBUTE_EXPIRES + "=" + expires(maxAge - 1) // To prevent flakiness

then:
expected == result || expected2 == result || expected3 == result
}

void "ServerCookieEncoder is NettyServerCookieEncoder"() {
expect:
ServerCookieEncoder.INSTANCE instanceof NettyServerCookieEncoder
Expand Down
14 changes: 12 additions & 2 deletions http/src/main/java/io/micronaut/http/cookie/Cookie.java
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@
*/
public interface Cookie extends Comparable<Cookie>, Serializable {

/**
* Constant for undefined MaxAge attribute value.
*/
long UNDEFINED_MAX_AGE = Long.MIN_VALUE;

/**
* @see <a href="https://tools.ietf.org/html/rfc6265#section-4.1.1">The Secure Attribute</a>.
*/
Expand Down Expand Up @@ -66,7 +71,7 @@ public interface Cookie extends Comparable<Cookie>, Serializable {
* @see <a href="https://datatracker.ietf.org/doc/html/rfc6265#section-5.2.2">The Max-Age Attribute</a>
*/
String ATTRIBUTE_MAX_AGE = "Max-Age";

/**
* @return The name of the cookie
*/
Expand Down Expand Up @@ -110,6 +115,10 @@ public interface Cookie extends Comparable<Cookie>, Serializable {
boolean isSecure();

/**
* Gets the maximum age of the cookie in seconds. If the max age has not been explicitly set,
* then the value returned will be {@link #UNDEFINED_MAX_AGE}, indicating that the Max-Age
* Attribute should not be written.
*
* @return The maximum age of the cookie in seconds
*/
long getMaxAge();
Expand All @@ -136,7 +145,8 @@ default Optional<SameSite> getSameSite() {
}

/**
* Sets the max age of the cookie in seconds.
* Sets the max age of the cookie in seconds. When not explicitly set, the max age will default
* to {@link #UNDEFINED_MAX_AGE} and cause the Max-Age Attribute not to be encoded.
*
* @param maxAge The max age
* @return This cookie
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ class CookieHttpCookieAdapter implements Cookie {

public CookieHttpCookieAdapter(HttpCookie httpCookie) {
this.httpCookie = httpCookie;
if (httpCookie.getMaxAge() == -1) { // HttpCookie.UNDEFINED_MAX_AGE = -1
this.httpCookie.setMaxAge(Cookie.UNDEFINED_MAX_AGE);
}
}

@Override
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package io.micronaut.http.cookie

import spock.lang.Specification

import java.time.LocalDateTime
import java.time.ZoneId
import java.time.ZonedDateTime
import java.time.format.DateTimeFormatter

class DefaultServerCookieEncoderSpec extends Specification {

void "DefaultServerCookieEncoder can correctly encode a cookie from HttpCookieFactory"() {
given:
HttpCookieFactory factory = new HttpCookieFactory();
ServerCookieEncoder cookieEncoder = new DefaultServerCookieEncoder()

when:
Cookie cookie = factory.create("SID", "31d4d96e407aad42").path("/").domain("example.com")

then:
"SID=31d4d96e407aad42; Path=/; Domain=example.com" == cookieEncoder.encode(cookie)[0]

when:
cookie = factory.create("SID", "31d4d96e407aad42").path("/").domain("example.com").sameSite(SameSite.Strict)

then:
"SID=31d4d96e407aad42; Path=/; Domain=example.com; SameSite=Strict" == cookieEncoder.encode(cookie)[0]

when:
cookie = factory.create("SID", "31d4d96e407aad42").path("/").secure().httpOnly()

then: 'Netty uses HTTPOnly instead of HttpOnly'
"SID=31d4d96e407aad42; Path=/; Secure; HttpOnly" == cookieEncoder.encode(cookie)[0]

when:
long maxAge = 2592000
cookie = factory.create("id", "a3fWa").maxAge(maxAge)
String result = cookieEncoder.encode(cookie).get(0)
String expected = "id=a3fWa; Max-Age=2592000; " + Cookie.ATTRIBUTE_EXPIRES + "=" + expires(maxAge)
String expected2 = "id=a3fWa; Max-Age=2592000; " + Cookie.ATTRIBUTE_EXPIRES + "=" + expires(maxAge + 1) // To prevent flakiness
String expected3 = "id=a3fWa; Max-Age=2592000; " + Cookie.ATTRIBUTE_EXPIRES + "=" + expires(maxAge - 1) // To prevent flakiness

then:
expected == result || expected2 == result || expected3 == result
}

private static String expires(Long maxAgeSeconds) {
ZoneId gmtZone = ZoneId.of("GMT")
LocalDateTime localDateTime = LocalDateTime.now(gmtZone).plusSeconds(maxAgeSeconds)
ZonedDateTime gmtDateTime = ZonedDateTime.of(localDateTime, gmtZone)
DateTimeFormatter formatter = DateTimeFormatter.ofPattern("EEE, dd MMM yyyy HH:mm:ss 'GMT'")
gmtDateTime.format(formatter)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ void testAdapter() {
assertFalse(cookie.isHttpOnly());
assertFalse(cookie.isSecure());
assertTrue(cookie.getSameSite().isEmpty());
assertEquals(Cookie.UNDEFINED_MAX_AGE, cookie.getMaxAge());

cookie = cookie.value("bar")
.httpOnly()
Expand Down

0 comments on commit 585c99b

Please sign in to comment.