Skip to content

fix(context): preserve prepared Set-Cookie headers when assigning raw Response#4994

Open
xu91102 wants to merge 1 commit into
honojs:mainfrom
xu91102:fix/context-prepared-set-cookie
Open

fix(context): preserve prepared Set-Cookie headers when assigning raw Response#4994
xu91102 wants to merge 1 commit into
honojs:mainfrom
xu91102:fix/context-prepared-set-cookie

Conversation

@xu91102

@xu91102 xu91102 commented Jun 2, 2026

Copy link
Copy Markdown

Fixes #4992

What this fixes

When middleware prepares cookies before next() and the handler assigns a raw Response to c.res, the prepared Set-Cookie headers can be lost if c.res has not been initialized yet.

Cause

The Context.res setter only merged headers from this.#res. In this path, cookies added by setCookie(c, ...) are still in #preparedHeaders, so assigning a raw Response skips them.

Fix

  • Preserve prepared Set-Cookie values when assigning a raw Response before c.res is initialized.
  • Keep the existing overwrite behavior for ordinary prepared headers.
  • Avoid duplicating cookies when c.res has already been read.
  • Add regression tests for the lost-cookie path and the no-duplicate path.

The author should do the following, if applicable

  • Add tests
  • Run tests
  • bun run format:fix && bun run lint:fix to format the code
  • Add TSDoc/JSDoc to document the code

Verified locally with:

  • npx vitest --run src/context.test.ts -t "raw Response"
  • npx vitest --run src/hono.test.ts -t "Overwrite the response from middleware after next"
  • npx vitest --run src/hono.test.ts src/context.test.ts
  • npx eslint src/context.ts src/context.test.ts
  • npx tsc --noEmit --pretty false
  • npx prettier --check src/context.ts src/context.test.ts
  • git diff --check
  • Remove-Item Env:NO_COLOR -ErrorAction SilentlyContinue; npx vitest --run

Full Vitest result: 144 files passed, 4525 tests passed, 33 skipped.

@codecov

codecov Bot commented Jun 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.98%. Comparing base (5ef800e) to head (7c4c4ef).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4994   +/-   ##
=======================================
  Coverage   78.98%   78.98%           
=======================================
  Files         154      154           
  Lines       10568    10571    +3     
  Branches     2212     2214    +2     
=======================================
+ Hits         8347     8350    +3     
  Misses       2221     2221           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@techzealot

techzealot commented Jun 3, 2026

Copy link
Copy Markdown

Thanks for the fix! I tested it locally (hono 4.12.18, dist/context.js patched manually) and unfortunately both scenarios still lose the handler's Set-Cookie headers.

Reproduction script

import { Hono } from 'hono'

// ─── Scenario B: #res created before c.header() ───
{
  const app = new Hono()
  app.use(async (c, next) => {
    c.res.headers.set('X-Something', 'true') // triggers #res creation
    c.header('Set-Cookie', 'mw_cookie=hello; Path=/', { append: true })
    await next()
  })
  app.get('/test', (c) => {
    const res = new Response('ok')
    res.headers.append('set-cookie', 'handler_cookie=world; Path=/; HttpOnly')
    return res
  })
  const res = await app.request('/test')
  console.log('[Scenario B]', res.headers.getSetCookie())
}

// ─── Scenario A: only #preparedHeaders, #res never created ───
{
  const app = new Hono()
  app.use(async (c, next) => {
    c.header('Set-Cookie', 'mw_cookie=hello; Path=/', { append: true })
    await next()
  })
  app.get('/test', (c) => {
    const res = new Response('ok')
    res.headers.append('set-cookie', 'handler_cookie=world; Path=/; HttpOnly')
    return res
  })
  const res = await app.request('/test')
  console.log('[Scenario A]', res.headers.getSetCookie())
}

Output with the official fix applied

[Scenario B] [ 'mw_cookie=hello; Path=/' ]          ❌ handler cookie lost
[Scenario A] [ 'mw_cookie=hello; Path=/' ]          ❌ handler cookie lost

Root cause

The set-cookie branch in the fix still calls headers.getSetCookie() on the context side only (this.#res.headers or this.#preparedHeaders), then unconditionally deletes all set-cookie from _res_res.headers.getSetCookie() is never called, so the handler's cookies are silently dropped:

if (k === 'set-cookie') {
  const cookies = headers.getSetCookie()   // ← only context's cookies
  _res.headers.delete('set-cookie')        // ← handler's cookies gone
  for (const cookie of cookies) {
    _res.headers.append('set-cookie', cookie) // ← only context's re-appended
  }
}

Suggested correct fix

Scenario B — save handler's cookies before delete:

if (k === 'set-cookie') {
  const handlerCookies = _res.headers.getSetCookie()
  const contextCookies = this.#res.headers.getSetCookie()
  _res.headers.delete('set-cookie')
  for (const cookie of handlerCookies) _res.headers.append('set-cookie', cookie)
  for (const cookie of contextCookies) _res.headers.append('set-cookie', cookie)
}

Scenario A — use append instead of delete + append, since #preparedHeaders only holds context-side cookies:

if (k === 'set-cookie') {
  _res.headers.append('set-cookie', v)
}

Full corrected setter:

set res(_res: Response | undefined) {
  if (this.#res && _res) {
    _res = createResponseInstance(_res.body, _res)
    for (const [k, v] of this.#res.headers.entries()) {
      if (k === 'content-type') continue
      if (k === 'set-cookie') {
        const handlerCookies = _res.headers.getSetCookie()
        const contextCookies = this.#res.headers.getSetCookie()
        _res.headers.delete('set-cookie')
        for (const cookie of handlerCookies) _res.headers.append('set-cookie', cookie)
        for (const cookie of contextCookies) _res.headers.append('set-cookie', cookie)
      } else {
        _res.headers.set(k, v)
      }
    }
  } else if (this.#preparedHeaders && _res) {
    _res = createResponseInstance(_res.body, _res) 
    for (const [k, v] of this.#preparedHeaders.entries()) {
      if (k === 'content-type') continue
      if (k === 'set-cookie') {
        _res.headers.append('set-cookie', v)
      } else {
        _res.headers.set(k, v)
      }
    }
  }
  this.#res = _res
  this.finalized = true
}

Output with the corrected fix

[Scenario B] [ 'handler_cookie=world; Path=/; HttpOnly', 'mw_cookie=hello; Path=/' ]  ✅
[Scenario A] [ 'handler_cookie=world; Path=/; HttpOnly', 'mw_cookie=hello; Path=/' ]  ✅

Both scenarios pass. Would appreciate a re-check on this — happy to provide any additional info.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

set res() setter loses Set-Cookie headers when middleware sets cookies before next() and handler returns a raw Response

2 participants