feat: Cookies#3406
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds configurable cookie-parser integration, new ChangesCookie Support Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
…ers from original schemas.
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Widened
setCookievalue type fromstringtostring | FlatObjectand noted in JSDoc that Express JSON-serializes object values (matches Express's runtime'j:' + JSON.stringify(value)path). - Retyped the destructured
setCookie/clearCookiein the spec against the middleware's actualexecute()return type instead ofresponseMock["cookie"], which guards the test from drifting if the helper signatures narrow further. - Added a regression case asserting the object-value path passes through to
response.cookie.
Claude Opus | 𝕏
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Replaced
FlatObjectwithz.core.util.JSONTypeforsetCookie's value type, broadening it from string/object to Zod's recursive JSON type (string, number, boolean, null, arrays, nested objects) — which matches Express's runtime branch (typeof val !== "string" ? 'j:' + JSON.stringify(val) : val) exactly. - Updated the JSDoc from "object values" to "non-string values" to reflect the wider type.
- Dropped the now-unused
FlatObjectimport.
Claude Opus | 𝕏
There was a problem hiding this comment.
Reviewed the following changes:
- Added a
getCookie(name)helper tocreateCookieMiddleware's context that readssignedCookies[name] ?? cookies[name], presented as an alternative to declaring cookies ininputSources.??(rather than||) correctly preservesfalsereturned bycookie-parserwhen a signed cookie's signature fails, instead of silently falling through to the raw value. - Folded the previously separate
CookieSecurityREADME example into the main cookies snippet so both the input-source path and thegetCookiepath are shown side-by-side, with thesecurity: { type: "cookie", name: "session" }middleware declaration kept for OpenAPI depiction. - Updated the cookie-middleware test to seed
requestProps.cookies/signedCookiesand assert thatgetCookiereturns the signed value first andundefinedfor missing names; the destructured output now uses the middleware's ownAwaited<ReturnType<...["execute"]>>for type safety. - Renamed the README section anchor
Cookies as an input source→Cookiesand updated the TOC accordingly.
Claude Opus | 𝕏
Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Wired the new cookie feature into the
example/workspace as a live demo: addedloginEndpoint(setCookieviacookieAssistedFactory) and switcheduploadAvatarEndpointtocookieAuthenticatedFactorysosessionarrives as a cookie input. - Added
sessionMiddlewareandcookieAssistingMiddlewareplus matchingcookieAuthenticatedFactory/cookieAssistedFactory; enabledcookies: trueinconfig.ts, added"cookies"topostinput sources and thecookiestag. - Regenerated
example.client.tsandexample.documentation.yamlsnapshots for the new endpoint and cookie-parameter, and added integration tests covering both theSet-Cookieresponse path (login) and cookie-as-input-source path (upload). - Added
@descJSDoc to the new and changed example endpoints.
Claude Opus | 𝕏
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Simplified
loginEndpoint: collapsed the credentials branch intoassert(..., createHttpError(401, "Invalid credentials")), narrowed the output to{ message }, and removed thesuccess: booleanfailure path. Regenerated client/yaml snapshots and updated the integration test to match. Now surfaces a real 401 instead of a 200 withsuccess: false, consistent with how the rest of the example endpoints signal auth failures viahttp-errors. - Aligned
uploadAvatarEndpoint's session check with the same pattern:assert(session.token, createHttpError(401, "Unauthorized"))replaces the barethrow new Error("Unauthorized"), which previously fell through to a 500 in the default result handler.
Claude Opus | 𝕏
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Tightened
loginEndpoint's input schema fromz.string().optional()toz.string().trim().nonempty()for bothusernameandpassword. Regeneratedexample.client.ts(requiredstringtypes) andexample.documentation.yaml(minLength: 1,required: [username, password],requestBody.required: true). The existing integration test passes"admin"/"test"literals, so it remains green.
Claude Opus | 𝕏
|
@pullfrog please review the whole feature again though the lenses of security, performance and stability concerns |
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Narrowed
clearCookie's per-calloverridesfromCookieOptionstoOmit<CookieOptions, "expires" | "maxAge">with an inline comment. Express 5'sres.clearCookieforce-setsexpires = new Date(1)anddelete opts.maxAge(seeexpress/lib/response.js), so any caller-supplied values for those keys are silently discarded — the narrower type now surfaces that at compile time. Addresses the earlier note flagging this Express quirk. Factory-levelbaseOptionskeep the fullCookieOptionstype, which is correct because those values are still meaningful forsetCookie. - Tightened the corresponding test assertion: dropped the
// unstablecomment onexpires: expect.any(Date)and replacedtoBeLessThan(10) // usually 1with exacttoBe(1).Number(new Date(1))is deterministic, so this is a strict improvement.
Claude Opus | 𝕏
There was a problem hiding this comment.
Important
Re-review through the security / performance / stability lenses. Framework-side code is solid — the issues are concentrated in the example and the README, which is what users copy. Two things worth addressing before merge: missing CSRF guidance in the new Cookies section, and an undocumented footgun around mixing cookies and signedCookies in inputSources (later entries win, so ["signedCookies", "cookies"] lets an attacker override a signed value with a forged unsigned one). The login example also models a few patterns that should not be cargo-culted.
TL;DR — Adds optional cookie-parser integration: cookies / signedCookies as input sources, a createCookieMiddleware that exposes getCookie / setCookie / clearCookie into context, and OpenAPI rendering of CookieSecurity request parameters. The framework wiring follows the established optional-peer pattern; review focus is the user-facing surface (README, example).
Key changes
- Optional
cookie-parserintegration —config.cookies(trueorCookieParserOptions) gates aloadPeer<typeof cookieParser>call at startup; the parser is installed beforebeforeRouting. - Cookies as input sources —
"cookies"and"signedCookies"joinInputSource, merged intogetInput()like any other source. createCookieMiddleware(baseOptions?)— returns aMiddlewareexposinggetCookie(signed-first),setCookie,clearCookiewith mergedbaseOptions.CookieSecurityrendered as cookie params —getSecurityNames(containers, type)walksLogicalContainer<Security>without combinatorial expansion;depictRequestParamsswitched from asecurityalternatives list to pre-extractedsecurityHeaders/securityCookiessets.- Example refactor — new
loginEndpointand acookieAuthenticatedFactorywired into the avatar upload to demonstrate cookie-driven auth.
Summary | 33 files | 42 commits | base: master ← feat-cookies
Security
The framework code itself is fine — getCookie correctly preferring signedCookies over cookies makes name collisions safe, and cookie-parser places tampered signed cookies under cookies (signature stripped), so the precedence rejects forgeries. The concerns are documentation and example shape:
- CSRF: enabling cookie-backed auth introduces an attack surface that this framework didn't previously have. The new README section should mention
sameSitedefaults and CSRF tokens for state-changing endpoints. - Input-source ordering:
getInputmerges sources in array order viaObject.assign. If a user writesinputSources: { get: ["signedCookies", "cookies"] }, an attacker can override a signed value by also sending an unsigned cookie of the same name. Worth a one-liner in the docs to putcookiesbeforesignedCookies(or omitcookiesentirely when authentication relies on signing).
README.md · example/endpoints/login.ts · example/middlewares.ts
Performance
Before:
depictRequestParamsreceived the fullAlternatives<Security>(combinatorial expansion viaprocessContainers) just to extract header names.
After:getSecurityNameswalks the rawLogicalContainer<Security>tree once per type, returning aSet;defaultIsHeaderswitched itsfamiliarlookup fromArray.includestoSet.has.
Net win on the documentation path — the combinatorial expansion is now only done once, for the actual depictSecurity call. No hot-path concerns: createCookieParser runs once at startup, and the per-request cost is whatever cookie-parser itself adds (standard Express middleware).
src/security.ts · src/documentation-helpers.ts
Stability
Edge cases I checked:
config.cookies: true(boolean) — destructuring through{ ...(typeof config.cookies === "object" && config.cookies) }correctly collapses to{}, passingundefinedto cookie-parser."signedCookies"listed ininputSourceswithout a secret —request.signedCookiesisundefined,Object.assign(target, undefined)no-ops. Test atcommon-helpers.spec.ts:226covers this."cookies"listed ininputSourceswithoutconfig.cookiesenabled — same graceful absence.- No public-API break that would require a migration rule:
depictRequestParamsanddefaultIsHeaderare internal (not re-exported fromsrc/index.ts).
The implementation is solid. All findings below are about the user-facing example/docs.
src/server-helpers.ts · tests/common-helpers.spec.ts
Claude Opus | 𝕏
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Added
sameSite: "lax"to thecreateCookieMiddlewarerecommended base options in the README example, addressing the prior review's CSRF guidance gap.laxis the standard primary mitigation for cookie-backed auth and aligns with modern browser defaults. - Added a parenthetical
(the order [matters](#customizing-input-sources)!)next to theinputSourcesmention in the new Cookies section, linking to the existing section that already documents the later-wins (Object.assign) precedence. This addresses the prior review's input-source-ordering footgun concern — and the example itself already models the safe ordering by puttingsignedCookieslast, so signed values win on name collision. - TOC anchor link verified (
#customizing-input-sourcesresolves to README L771).
Claude Opus | 𝕏

Related to discussion #710
Includes:
Summary by CodeRabbit
New Features
Documentation
Tests