Repository navigation
feat: add HostCookie and SecureCookie as sealed subclasses of Cookie - #137
Conversation
Make Cookie a sealed class with HostCookie (__Host- prefix) and SecureCookie (__Secure- prefix) as permitted subclasses per RFC 6265bis §4.1.3. Prefix constraints are enforced structurally by the type system rather than runtime validation. - HostCookie: secure=true, path="/", no domain, __Host- auto-prepended - SecureCookie: secure=true, __Secure- auto-prepended - Cookie.toHttpString() warns when Set-Cookie exceeds 4096 bytes closes #126 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Extract warnIfOversized() so subclasses check the final output including __Host-/__Secure- prefix - Fix HostCookie Javadoc: remove httponly from example output Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
setDomain(null) means "remove Domain attribute", which is consistent with __Host- requirements. Only non-null values are rejected. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract buildHttpString() for the attribute serialization without size check. Subclasses call buildHttpString() + prefix + warnIfOversized() so the warning fires exactly once on the final output. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the enkan-web cookie model to support RFC 6265bis cookie prefixes by introducing dedicated cookie types and making Cookie a sealed base type, plus adds oversized-cookie warning logging.
Changes:
- Make
Cookieasealedclass and introduceHostCookie(__Host-) andSecureCookie(__Secure-) subclasses that enforce prefix-specific constraints via overridden setters. - Add
Cookie.buildHttpString()for prefixable serialization andwarnIfOversized()to log when aSet-Cookievalue exceeds 4096 bytes. - Add unit tests covering
HostCookieandSecureCookiebehaviors.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| enkan-web/src/main/java/enkan/data/Cookie.java | Seals Cookie, factors out serialization, and adds oversize warning logging. |
| enkan-web/src/main/java/enkan/data/HostCookie.java | Adds __Host- cookie type enforcing Secure + Path=/ + no Domain, and prepends prefix in output. |
| enkan-web/src/main/java/enkan/data/SecureCookie.java | Adds __Secure- cookie type enforcing Secure and prepends prefix in output. |
| enkan-web/src/test/java/enkan/data/HostCookieTest.java | Tests for HostCookie constraints and serialization. |
| enkan-web/src/test/java/enkan/data/SecureCookieTest.java | Tests for SecureCookie secure enforcement and serialization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static HostCookie create(String name, String value) { | ||
| return new HostCookie(name, value); | ||
| } |
There was a problem hiding this comment.
HostCookie.create accepts any token name, including one already starting with "__Host-". That produces an invalid Set-Cookie value like "__Host-__Host-..." because toHttpString() always prepends the prefix. Consider validating/rejecting (or stripping) the prefix in create()/constructor to prevent double-prefixing.
| public static SecureCookie create(String name, String value) { | ||
| return new SecureCookie(name, value); | ||
| } |
There was a problem hiding this comment.
SecureCookie.create accepts a name that already includes the "__Secure-" prefix, which will result in a doubled prefix in the serialized header ("__Secure-__Secure-..."). Add a guard (or prefix-stripping) in create()/constructor so callers can't accidentally generate an invalid cookie name.
| @Override | ||
| public void setPath(String path) { | ||
| if (!"/".equals(path)) { | ||
| throw new IllegalArgumentException("__Host- cookies must have Path=/"); | ||
| } |
There was a problem hiding this comment.
HostCookie.setPath throws a NullPointerException when called with null (because "/".equals(path) is evaluated). Since __Host- cookies require Path=/, null should be rejected deterministically (e.g., IllegalArgumentException with a clear message) rather than NPE.
| * Logs a warning if the Set-Cookie header exceeds the 4096-byte limit. | ||
| * | ||
| * @param setCookieValue the complete Set-Cookie header value | ||
| */ | ||
| protected void warnIfOversized(String setCookieValue) { | ||
| int byteLength = setCookieValue.getBytes(StandardCharsets.UTF_8).length; | ||
| if (byteLength > MAX_COOKIE_SIZE) { | ||
| LOG.warning("Set-Cookie header for '" + getName() + "' is " + byteLength | ||
| + " bytes, exceeding the 4096-byte limit. Browsers may reject this cookie."); |
There was a problem hiding this comment.
warnIfOversized() logs the cookie name via getName(), which for HostCookie/SecureCookie is the unprefixed name (e.g., "token"), even though the actual Set-Cookie value starts with "__Host-token"/"__Secure-token". This makes the warning harder to correlate with the emitted header. Consider logging the effective cookie name (including any prefix), and use MAX_COOKIE_SIZE in the message instead of hardcoding "4096" to avoid drift.
| * Logs a warning if the Set-Cookie header exceeds the 4096-byte limit. | |
| * | |
| * @param setCookieValue the complete Set-Cookie header value | |
| */ | |
| protected void warnIfOversized(String setCookieValue) { | |
| int byteLength = setCookieValue.getBytes(StandardCharsets.UTF_8).length; | |
| if (byteLength > MAX_COOKIE_SIZE) { | |
| LOG.warning("Set-Cookie header for '" + getName() + "' is " + byteLength | |
| + " bytes, exceeding the 4096-byte limit. Browsers may reject this cookie."); | |
| * Logs a warning if the Set-Cookie header exceeds the configured byte limit. | |
| * | |
| * @param setCookieValue the complete Set-Cookie header value | |
| */ | |
| protected void warnIfOversized(String setCookieValue) { | |
| int byteLength = setCookieValue.getBytes(StandardCharsets.UTF_8).length; | |
| if (byteLength > MAX_COOKIE_SIZE) { | |
| int separatorIndex = setCookieValue.indexOf('='); | |
| String effectiveCookieName = separatorIndex >= 0 | |
| ? setCookieValue.substring(0, separatorIndex) | |
| : getName(); | |
| LOG.warning("Set-Cookie header for '" + effectiveCookieName + "' is " + byteLength | |
| + " bytes, exceeding the " + MAX_COOKIE_SIZE | |
| + "-byte limit. Browsers may reject this cookie."); |
| public String toHttpString() { | ||
| String result = buildHttpString(); | ||
| warnIfOversized(result); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
New behavior: Cookie.toHttpString()/warnIfOversized() is supposed to warn when the Set-Cookie value exceeds 4096 bytes, but there are currently no unit tests asserting the warning behavior (no 4096/oversize-related assertions found under enkan-web/src/test/java). Add a test that attaches a temporary java.util.logging.Handler to Cookie's logger and asserts a warning is emitted for an oversized cookie.
- HostCookie.create() / SecureCookie.create() reject names already starting with __Host- / __Secure- to prevent double-prefixing - HostC 8000 ookie.setPath(null) throws IllegalArgumentException instead of relying on "/".equals(null) behavior - warnIfOversized() extracts the effective cookie name (including prefix) from the Set-Cookie string and uses MAX_COOKIE_SIZE constant in the log message - Add size warning tests to CookieTest Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Browsers send cookies with the full prefixed name (e.g. __Host-token),
but application code sets and reads cookies by the unprefixed name.
CookiesMiddleware now strips the prefix when building the cookie map
so that request.getCookies().get("token") works transparently.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cookie.create() now receives the prefix-stripped name so that getName() is consistent between send-side (HostCookie/SecureCookie) and receive-side. Move prefix constants to class top with other constants. Add tests for getName() consistency and name collision. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| String key = stripPrefix(rawName); | ||
| Cookie cookie = Cookie.create(key, value); | ||
| cookies.put(key, cookie); |
There was a problem hiding this comment.
Request cookie parsing now strips __Host- / __Secure- and stores the cookie under the unprefixed key. This is a breaking behavioral change (previously the raw name was the map key) and it defeats the security purpose of the prefixes by allowing collisions with a non-prefixed cookie of the same name. Prefer preserving the raw cookie name as the key (and, if needed, add a separate helper API for prefix handling) rather than normalizing names during parsing.
| String key = stripPrefix(rawName); | |
| Cookie cookie = Cookie.create(key, value); | |
| cookies.put(key, cookie); | |
| Cookie cookie = Cookie.create(rawName, value); | |
| cookies.put(rawName, cookie); |
| @Test | ||
| void hostPrefixCookieParsedByStrippedName() { | ||
| MiddlewareChain<HttpRequest, HttpResponse, ?, ?> chain = new DefaultMiddlewareChain<>(new AnyPredicate<>(), null, | ||
| (Endpoint<HttpRequest, HttpResponse>) req -> { | ||
| // Application should access by unprefixed name | ||
| Cookie cookie = req.getCookies().get("token"); | ||
| assertThat(cookie).isNotNull(); | ||
| assertThat(cookie.getValue()).isEqualTo("abc123"); | ||
| // Prefixed name should not be a key | ||
| assertThat(req.getCookies()).doesNotContainKey("__Host-token"); | ||
| return HttpResponse.of("ok"); | ||
| }); | ||
| request.getHeaders().put("Cookie", "__Host-token=abc123"); | ||
| middleware.handle(request, chain); | ||
| } |
There was a problem hiding this comment.
This test asserts that a __Host- cookie is accessed via an unprefixed key and that the prefixed name is not present. That behavior removes the main security property of the prefix (distinct, non-spoofable name) and can allow a plain cookie with the same unprefixed name to override it. If parsing is updated to preserve raw names, this test should be adjusted accordingly.
| void prefixedAndPlainCookieCollisionLastWins() { | ||
| // When both __Host-token and token exist, last one in the header wins (HashMap put) | ||
| MiddlewareChain<HttpRequest, HttpResponse, ?, ?> chain = new DefaultMiddlewareChain<>(new AnyPredicate<>(), null, | ||
| (Endpoint<HttpRequest, HttpResponse>) req -> { | ||
| Cookie cookie = req.getCookies().get("token"); | ||
| assertThat(cookie).isNotNull(); | ||
| assertThat(cookie.getValue()).isEqualTo("plain"); | ||
| return HttpResponse.of("ok"); | ||
| }); | ||
| request.getHeaders().put("Cookie", "__Host-token=prefixed; token=plain"); |
There was a problem hiding this comment.
This test allows __Host-token and token to collide and relies on “last wins”. That makes application behavior depend on header ordering and can reintroduce cookie-injection/session-fixation risks that the __Host- prefix is designed to prevent. Consider keeping prefixed and non-prefixed cookies distinct in the request cookie map to avoid collisions.
| void prefixedAndPlainCookieCollisionLastWins() { | |
| // When both __Host-token and token exist, last one in the header wins (HashMap put) | |
| MiddlewareChain<HttpRequest, HttpResponse, ?, ?> chain = new DefaultMiddlewareChain<>(new AnyPredicate<>(), null, | |
| (Endpoint<HttpRequest, HttpResponse>) req -> { | |
| Cookie cookie = req.getCookies().get("token"); | |
| assertThat(cookie).isNotNull(); | |
| assertThat(cookie.getValue()).isEqualTo("plain"); | |
| return HttpResponse.of("ok"); | |
| }); | |
| request.getHeaders().put("Cookie", "__Host-token=prefixed; token=plain"); | |
| void prefixedAndPlainCookiesWithDifferentNamesDoNotCollide() { | |
| MiddlewareChain<HttpRequest, HttpResponse, ?, ?> chain = new DefaultMiddlewareChain<>(new AnyPredicate<>(), null, | |
| (Endpoint<HttpRequest, HttpResponse>) req -> { | |
| Cookie prefixedCookie = req.getCookies().get("token"); | |
| Cookie plainCookie = req.getCookies().get("session"); | |
| assertThat(prefixedCookie).isNotNull(); | |
| assertThat(prefixedCookie.getValue()).isEqualTo("prefixed"); | |
| assertThat(plainCookie).isNotNull(); | |
| assertThat(plainCookie.getValue()).isEqualTo("plain"); | |
| return HttpResponse.of("ok"); | |
| }); | |
| request.getHeaders().put("Cookie", "__Host-token=prefixed; session=plain"); |
| public void setSecure(boolean secure) { | ||
| if (!secure) { | ||
| throw new UnsupportedOperationException("__Secure- cookies must have the Secure attribute"); | ||
| } |
There was a problem hiding this comment.
setSecure(true) is currently a no-op. Consider delegating the allowed value to super.setSecure(true) so the method preserves base-class semantics and can re-establish the invariant if the underlying state is ever altered (e.g., via deserialization).
| } | |
| } | |
| super.setSecure(true); |
| @Override | ||
| public void setDomain(String domain) { | ||
| if (domain != null) { | ||
| throw new UnsupportedOperationException("__Host- cookies must not have a Domain attribute"); | ||
| } | ||
| } |
There was a problem hiding this comment.
setDomain(null) is implemented as a no-op. For stronger invariants (and to preserve base-class semantics), consider calling super.setDomain(null) when domain == null so any prior value is explicitly cleared (e.g., after deserialization).
| public void setSecure(boolean secure) { | ||
| if (!secure) { | ||
| throw new UnsupportedOperationException("__Host- cookies must have the Secure attribute"); | ||
| } |
There was a problem hiding this comment.
setSecure(true) is implemented as a no-op. Consider delegating the allowed value to super.setSecure(true) so the invariant is actively enforced (and can be restored if state is altered via serialization/reflection).
| } | |
| } | |
| super.setSecure(true); |
| public void setPath(String path) { | ||
| if (path == null || !"/".equals(path)) { | ||
| throw new IllegalArgumentException("__Host- cookies must have Path=/"); | ||
| } |
There was a problem hiding this comment.
When path is "/", setPath currently does nothing. Consider calling super.setPath("/") in the allowed case so the invariant is enforced even if the underlying field was modified (e.g., by deserialization).
| } | |
| } | |
| super.setPath("/"); |
| Logger logger = Logger.getLogger(Cookie.class.getName()); | ||
| List<LogRecord> records = new ArrayList<>(); | ||
| Handler handler = new Handler() { | ||
| @Override public void publish(LogRecord record) { records.add(record); } | ||
| @Override public void flush() {} |
There was a problem hiding this comment.
The JUL logger capture here can be environment-dependent: logger/handler levels aren’t set and parent handlers aren’t disabled, so the WARNING may not be delivered to this handler (or may produce noisy output). Consider temporarily setting logger and handler levels appropriately and restoring useParentHandlers/levels in the finally block (same pattern applies to the next test).
| public String toHttpString() { | ||
| String result = buildHttpString(); | ||
| warnIfOversized(result); | ||
| return result; | ||
| } | ||
|
|
There was a problem hiding this comment.
With HostCookie / SecureCookie added, Cookie.toHttpString() still does not validate prefix constraints when a caller uses Cookie.create("__Host-...", ...) or Cookie.create("__Secure-...", ...). That means it’s still easy to emit non-compliant prefixed cookies while bypassing the new types. Consider either rejecting those prefixes in the base Cookie API or adding explicit validation in toHttpString() when getName() starts with a known prefix.
| public String toHttpString() { | |
| String result = buildHttpString(); | |
| warnIfOversized(result); | |
| return result; | |
| } | |
| public String toHttpString() { | |
| validatePrefixConstraints(); | |
| String result = buildHttpString(); | |
| warnIfOversized(result); | |
| return result; | |
| } | |
| private void validatePrefixConstraints() { | |
| String name = getName(); | |
| if (name == null) { | |
| return; | |
| } | |
| if (name.startsWith("__Host-")) { | |
| if (!isSecure()) { | |
| throw new IllegalStateException("__Host- cookies must have the Secure attribute"); | |
| } | |
| if (getDomain() != null) { | |
| throw new IllegalStateException("__Host- cookies must not have a Domain attribute"); | |
| } | |
| if (!"/".equals(getPath())) { | |
| throw new IllegalStateException("__Host- cookies must have Path=/"); | |
| } | |
| } else if (name.startsWith("__Secure-") && !isSecure()) { | |
| throw new IllegalStateException("__Secure- cookies must have the Secure attribute"); | |
| } | |
| } |
Prefix stripping in parseCookies() defeated the security purpose of
__Host-/__Secure- prefixes by allowing name collisions. Reverted to
preserve raw cookie names as map keys.
Additional fixes:
- Cookie.toHttpString() validates prefix constraints so that
Cookie.create("__Host-...", ...) cannot bypass the type system
- Setters in HostCookie/SecureCookie delegate to super for
deserialization resilience
- Logger tests properly configure level and useParentHandlers
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CookiesMiddleware now parses __Host- and __Secure- cookies into their respective sealed subtypes, stripping the prefix for the application- facing name. This preserves type safety on the receive side and allows pattern matching on the cookie type. Prefixed cookies take precedence over plain cookies with the same stripped name, preventing injection attacks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Cookiea sealed class withHostCookieandSecureCookieas permitted subclasses per RFC 6265bis §4.1.3HostCookie(__Host-prefix):secure=true,path="/", nodomain— all enforced by the type. Prefix auto-prepended intoHttpString().SecureCookie(__Secure-prefix):secure=trueenforced by the type. Prefix auto-prepended intoHttpString().Cookie.toHttpString()now emits a warning log when the Set-Cookie header exceeds 4096 bytesUsage
closes #126
Test plan
🤖 Generated with Claude Code