fix: prevent sending set-cookie header when cookie value is unchanged... #1558
Conversation
WalkthroughAdds change-detection to cookie setters so the cookie jar is only updated when a value actually changes; initializes jar entries with a shallow copy; updates setter type signatures; and adds tests and a changelog entry for version 1.4.17 fixing repeated Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant CookieJar
rect rgb(232,244,255)
Note over Client,Server: Request that modifies cookie
Client->>Server: POST /cookie (no cookie / new value)
Server->>CookieJar: Check current entry vs new value
CookieJar-->>Server: Changed (deep compare true)
Server->>CookieJar: Initialize shallow copy (if needed) and update entry
Server-->>Client: 200 OK + Set-Cookie
end
rect rgb(240,255,240)
Note over Client,Server: Request that does not modify cookie
Client->>Server: GET /cookie (with cookie)
Server->>CookieJar: Check current entry vs requested access (no mutation)
CookieJar-->>Server: No change detected
Server-->>Client: 200 OK (no Set-Cookie)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cookies.ts (1)
278-281: Array comparison for secrets uses reference equality.The
secretssetter uses===comparison, which means settingsecretsto a new array with identical content (e.g.,['a', 'b']vs.['a', 'b']) will trigger aset-cookieheader even though the values are the same.While this is less critical than the Date issue (since changing secrets is less common and typically intentional), consider if deep equality would be beneficial for consistency with the
valuesetter's approach for objects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)src/cookies.ts(2 hunks)test/cookie/unchanged.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/cookie/unchanged.test.ts (1)
src/cookies.ts (2)
value(154-156)value(158-182)
🔇 Additional comments (8)
test/cookie/unchanged.test.ts (4)
5-45: LGTM! Comprehensive test for read-only cookie access.This test properly validates the core fix: a POST request that sets a cookie should emit
set-cookie, while a subsequent GET that only reads the cookie should not emitset-cookie. The test correctly extracts the cookie from the POST response and passes it to the GET request.
47-73: LGTM! Good coverage for unvalidated cookie access.This test complements the first by testing cookie behavior without schema validation, ensuring that simply reading
session.valuedoesn't triggerset-cookie, while writing does.
75-91: LGTM! Essential test for same-value detection.This test is crucial for validating the change-detection logic. Setting a cookie to its existing value should not emit
set-cookie, which this test properly verifies.
93-108: LGTM! Validates that actual changes still work.This test ensures the change-detection logic doesn't break the normal case: when a cookie value actually changes,
set-cookieshould be emitted. Good coverage of both positive and negative cases.src/cookies.ts (3)
158-182: Change detection implementation is pragmatic but has known limitations.The implementation uses
===for primitives andJSON.stringify()for deep equality on objects. While this approach works for most cookie use cases, be aware of these limitations:
- Property order: Objects with identical properties in different order will be considered equal (generally fine with V8's property ordering)
- Special types:
Dateobjects,RegExp, functions, symbols, andundefinedvalues won't be compared semantically- Performance:
JSON.stringify()can be expensive for large objects, though cookie values are typically small- Circular references: Handled gracefully with try-catch
For typical cookie values (simple objects with strings/numbers), this approach is reasonable and pragmatic.
145-145: LGTM! Shallow copy prevents unintended mutations.Changing from direct assignment to
{ ...this.initial }ensures that modifying the jar entry doesn't affect the initial configuration object. Since cookie properties are flat (no nested objects), a shallow copy is sufficient and more performant than a deep copy.
198-271: LGTM! Consistent change-detection pattern for cookie properties.All setters (maxAge, domain, path, secure, httpOnly, sameSite, priority, partitioned) follow a consistent pattern:
- Check equality with
===- Early return if unchanged
- Initialize jar entry with shallow copy if needed
- Set new value
The explicit type annotations improve code clarity and type safety. For these primitive and string literal types,
===comparison is appropriate and efficient.CHANGELOG.md (1)
1-4: LGTM! Appropriate changelog entry.The changelog entry clearly documents the bug fix with a reference to the related issue. The description accurately reflects the change: preventing
set-cookieheaders when cookie values haven't been modified.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/cookies.ts (1)
284-288: Consider deep equality for array secrets.The
secretssetter uses===to compare values. This works correctly for string secrets, but for array secrets, it compares by reference:cookie.secrets = ['key1', 'key2'] // Creates jar entry cookie.secrets = ['key1', 'key2'] // Creates jar entry again (different array reference)While this may not be critical (secrets arrays are typically constant references in production code), you could improve consistency by adding array comparison similar to the object comparison in the
valuesetter:if (Array.isArray(current) && Array.isArray(secrets)) { if (current.length === secrets.length && current.every((val, idx) => val === secrets[idx])) return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cookies.ts(2 hunks)test/validator/cookie.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/validator/cookie.test.ts (2)
src/cookies.ts (1)
set(299-309)test/utils.ts (1)
req(1-2)
🔇 Additional comments (5)
test/validator/cookie.test.ts (1)
466-502: Well-structured test for expires change detection.The test effectively validates that the
expiressetter:
- Skips updates when timestamps are identical (lines 476-482)
- Updates when timestamps differ (lines 485-487)
- Skips updates when both values are undefined (lines 490-494)
The use of reference equality (
toBe) to verify the jar wasn't recreated is a clever way to confirm no-op behavior.src/cookies.ts (4)
144-148: LGTM: Shallow copy prevents unintended mutations.Using
{ ...this.initial }instead ofthis.initialensures the jar entry is independent of the initial template.
158-182: Change detection logic is sound for typical cookie values.The implementation correctly:
- Performs simple equality check first (line 163)
- Uses deep equality for objects via
JSON.stringify(lines 166-177)- Safely handles stringify failures with try-catch
- Only mutates the jar when a real change occurs
Note:
JSON.stringifyhas known limitations (key order sensitivity, doesn't handle Date/undefined/functions, can be slow for large objects), but for typical cookie values (strings, numbers, simple objects), this is a pragmatic approach.
188-196: Correctly implements timestamp-based comparison.The expires setter now properly:
- Handles both values being undefined (line 191)
- Compares Date objects by timestamp using
getTime()(line 192)- Only updates when the timestamp actually changes
This correctly addresses the previous review feedback about Date object reference comparison.
202-278: Consistent change detection across all property setters.All property setters (
maxAge,domain,path,secure,httpOnly,sameSite,priority,partitioned) now follow a consistent pattern:
- Check for equality using
===- Early return on no-op
- Initialize jar entry with shallow copy
- Update value
Type signatures have been properly updated with explicit types (e.g.,
Date | undefined,boolean | undefined, etc.).
commit: |
2 participants
Description
Fixes #1556
Prevents Elysia from sending
set-cookieheader when cookie values are not actually modified.Problem
When using cookie validation with
t.Optional(), Elysia was sendingset-cookieheader in every request, even for GET requests that only read cookie values. This happened because the validation code would set cookie values back after validation, even when values didn't change.Solution
Modified the
Cookieclass insrc/cookies.tsto check if values actually changed before creating entries in the cookie jar:===comparisonJSON.stringify()for deep equality checkChanges
Cookieclass to check for value changestest/cookie/unchanged.test.tsTest Results
All 4 new test cases pass:
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.