Skip to content

Conversation

@mhassan1
Copy link

Description

This PR fixes the ResponseService.updateHeaders method to not try to mutate response headers, since it is forbidden by the fetch spec.

Resolves #1389.

Motivation

Users who install incompatible (i.e. modern) versions of @hono/node-server will end up with response errors when using Portkey.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)

How Has This Been Tested?

  • Unit Tests
  • Integration Tests
  • Manual Testing

Screenshots (if applicable)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Related Issues

#1389

@matter-code-review
Copy link
Contributor

Code Quality bug fix

Description

Summary By MatterAI MatterAI logo

🔄 What Changed

Added workaround for immutable Headers object in Response by cloning the response before header manipulation. Introduced @hono/node-server dependency to support runtime requirements. Enhanced updateHeaders to safely modify headers without triggering immutability errors.

🔍 Impact of the Change

Fixes potential runtime failures when modifying response headers in Node.js environment. Ensures compliance with Web API standards while working around environment-specific limitations. Improves reliability of header injection logic used for telemetry and caching.

📁 Total Files Changed

File ChangeLog
Deps Update package.json Added @hono/node-server to support Node runtime compatibility
Header Clone responseService.ts Clones Response object to bypass immutable headers guard before mutation
Test Expand responseService.test.ts Added test cases for header removal, brotli/gzip handling, and edge conditions

🧪 Test Added/Recommended

Added

  • Test for adding required headers (LAST_USED_OPTION_INDEX) with cache status and retry attempt
  • Test for removing problematic headers: content-length, transfer-encoding
  • Test for stripping brotli and gzip content-encoding when present
  • Test for conditional cache status header suppression when undefined
  • Test for provider header removal when provider is empty or null

🔒Security Vulnerabilities

N/A

⏳ Estimated code review effort

LOW (~7 minutes)

Tip

Quality Recommendations

  1. Consider abstracting the header cloning logic into a utility function for reuse across services

  2. Add explicit type guard for response.body to ensure stream compatibility during cloning

  3. Include performance benchmarking for header manipulation under high load

♫ Tanka Poem

Code flows like stream,
Headers once locked, now freed—
Clone breaks the guard.
New server joins the dance,
Silent fix, system sings. 🎵

Sequence Diagram

sequenceDiagram
    participant Client
    participant ResponseService
    participant Response
    participant Headers

    Client->>ResponseService: updateHeaders(response, cacheStatus, retryAttempt)
    ResponseService->>Response: new Response(body, {status, statusText, headers: new Headers(existing)})
    Response-->>ResponseService: Cloned response with mutable headers
    ResponseService->>Headers: append(LAST_USED_OPTION_INDEX, retryAttempt)
    ResponseService->>Headers: delete('content-length', 'transfer-encoding', 'content-encoding' if brotli/gzip)
    ResponseService->>Headers: set(CACHE_STATUS, cacheStatus) if defined
    ResponseService-->>Client: Modified response with updated headers
Loading

@matter-code-review
Copy link
Contributor

✅ Reviewed the changes: Fix for immutable header guard by cloning Response and Headers objects. Test cases updated accordingly.

cacheStatus: string | undefined,
retryAttempt: number
) {
// Clone response and headers to work around `immutable` header guard
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expensive operation, have to see if there are better ways to solve this, dont want to. be slowing down the fastest gateway xd

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

understood. i attempted to access the header guard to modify it in place, but undici hides it in a private field (link) and deletes the getters and setters (link).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should work:

// Clone headers to work around `immutable` header guard
const kHeaders = Object.getOwnPropertySymbols(response).find((symbol) => symbol.toString() === 'Symbol(headers)');
Object.defineProperty(response, kHeaders, {
  value: new Headers(response.headers),
  writable: true,
  enumerable: true,
  configurable: true,
});

do we like that better? the benefit is that we don't need to construct a new response, but it is making some unsafe assumptions about internals.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another option:

// Capture internal `getHeadersGuard` and `setHeadersGuard`
let getHeadersGuard, setHeadersGuard
const deleteProperty = Reflect.deleteProperty
Reflect.deleteProperty = function (...args) {
  if (args[1] === 'getHeadersGuard') getHeadersGuard = args[0].getHeadersGuard
  if (args[1] === 'setHeadersGuard') setHeadersGuard = args[0].setHeadersGuard
  return deleteProperty.call(this, ...args)
}

// then
setHeadersGuard(response.headers, 'response')

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.

TypeError: immutable with incompatible version of @hono/node-server

2 participants