Skip to content

fix(agent): bind native response defaults on Node - #4434

Merged
kojiwakayama merged 6 commits into
mainfrom
codex/issue-1020-node-response-defaults
Sep 7, 2026
Merged

kojiwakayama merged 6 commits into
mainfrom
codex/issue-1020-node-response-defaults

Conversation

@kojiwakayama

@kojiwakayama kojiwakayama commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

Node's native response-option conversion can inherit headers and status text from Object.prototype, including CORS headers for a denied origin. Framework responses now receive fresh, explicit defaults. Inherited option accessors cause a TypeError before native conversion can invoke them; non-writable fields retain Node's native failure behavior.

Defaults apply only to omitted values. Invalid statuses and malformed headers still fail, and host errors propagate unchanged. A pure internal option builder provides independently tested validation while the existing integration regression exercises native response construction across runtimes.

Validation:

  • Focused Deno suite: 58 steps passed. Focused Node suite: 22 tests passed. Bun: 3 registered test files passed.
  • Option-builder unit coverage: 100% lines, branches and functions. Each call receives independent headers.
  • Full lint:ci, typecheck, semantic test audit, API-reference and diff checks passed; no gate exclusions or baselines changed.
  • The final CI-produced npm artifact passed 60 response-option/path cases on Node 22.3.0, 24.14.0 and 24.20.0, plus ordinary CORS/body and host-error-identity controls. Synthetic data, isolated local processes, zero network calls.
  • Independent reviews approved the response correction and helper extraction.

Implementation and deployed verification are tracked in https://github.com/veryfront/veryfront-issue-inbox/issues/1036. Keep that issue open until the published artifact passes staging verification.

The separate native credential-processing boundary remains unresolved in https://github.com/veryfront/veryfront-issue-inbox/issues/1037. This PR does not establish complete shared-realm isolation.

The passing Sonar gate reports three minor style suggestions that are intentionally not applied: indexed iteration avoids replaceable iterator hooks, and undefined-only defaults preserve native behavior for invalid null values. Replacing those constructs with for-of or nullish coalescing would change the security/compatibility contract.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your trial has ended. Reactivate Greptile to resume code reviews.

@coderabbitai

coderabbitai Bot commented Sep 7, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 8 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: 5c19e80c-ffa3-41ad-92b7-b788a55f437b

📥 Commits

Reviewing files that changed from the base of the PR and between 33b8ef6 and edbf81f.

📒 Files selected for processing (7)
  • docs/guides/agent-service-runtime.md
  • scripts/test/run-suite.test.ts
  • scripts/test/run-suite.ts
  • src/agent/service/definition.ts
  • src/agent/service/response-init.test.ts
  • src/agent/service/response-init.ts
  • tests/integration/agent/service-response-init.test.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown

📦 Client bundle boundary

Entrypoint Modules Source size Server leaks
src/index.client.ts 289 2303 KiB ✅ 0

A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in scripts/lint/client-bundle-baseline.json to burn down.

@gitar-bot

gitar-bot Bot commented Sep 7, 2026

Copy link
Copy Markdown

Note

Automatic reviews are paused because your trial's included automatic processing has been used for this period. Upgrade now, or comment "Gitar review" to run a review anytime.
Learn more

Code Review ✅ Approved

Fixes Node response initialization to prevent inheriting headers and status text from Object.prototype by supplying explicit defaults for omitted values. No issues found.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Important

Your trial ends in 2 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.

Was this helpful? React with 👍 / 👎 | Gitar

@kojiwakayama

Copy link
Copy Markdown
Contributor Author

@codex review

Copy link
Copy Markdown
Contributor

Automated review — Score: 87/100

Focused, well-tested prototype-pollution fix for Response init construction on Node; minor process nits only.

  • Strength — root cause fixed at the single choke point. All Response construction in src/agent/service/definition.ts goes through createNativeResponse, so explicitly setting headers/status/statusText there closes the gap everywhere (liveness/readiness, CORS preflight, 404, and CORS-rewrapped host responses), not just for one call site.
  • Strength — idiomatic, not a one-off hack. The fix reuses the file's pre-existing EmptyHeadersInit null-prototype sentinel (already used for RequestInit.headers), so the pattern is consistent with how this module already defends against prototype pollution.
  • Strength — the new test actually proves the vulnerability. tests/integration/agent/service-response-init.test.ts pollutes Object.prototype.headers/statusText directly and asserts no leakage across liveness/readiness/404/custom-handler/preflight/shutdown responses, plus separately covers host-error propagation and native status/status-text validation (with a sensible Bun-vs-Node branch for status-text validation differences). This is real regression coverage, not a smoke test.
  • Strength — wiring is complete. Docs (agent-service-runtime.md) and both runtime test-registration lists (run-suite.ts and run-suite.test.ts) were updated together, so the new test actually runs under the Node/Bun legacy-runtime suites the PR description claims were validated.
  • Minor concern — no CHANGELOG entry. CONTRIBUTING.md's PR checklist calls for a CHANGELOG update on fixes; this isn't CI-enforced but is worth a one-line addition given this is a real security fix.
  • Minor concern — undocumented rationale. The comment above createNativeResponse asserts "Node converts the init into an ordinary dictionary internally" without a reference (Node source line, spec note, or the linked issue). A short pointer would help whoever next touches this function understand why the old status === undefined && statusText === undefined early-return had to go, rather than risk reverting it as a "simplification."

Nothing here blocks merging; the two notes above are cheap to address before or shortly after merge.


Generated by Claude Code

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b9f453c9eb

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/agent/service/definition.ts Outdated
@codecov

codecov Bot commented Sep 7, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your trial has ended. Reactivate Greptile to resume code reviews.

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aed5f7eb16

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread docs/guides/agent-service-runtime.md Outdated

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your trial has ended. Reactivate Greptile to resume code reviews.

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit: eb40283065

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your trial has ended. Reactivate Greptile to resume code reviews.

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 👍

Reviewed commit: b7ea2a94de

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your trial has ended. Reactivate Greptile to resume code reviews.

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Chef's kiss.

Reviewed commit: 397cddb446

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your trial has ended. Reactivate Greptile to resume code reviews.

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: edbf81f0d4

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@sonarqubecloud

sonarqubecloud Bot commented Sep 7, 2026

Copy link
Copy Markdown

@kojiwakayama
kojiwakayama added this pull request to the merge queue Sep 7, 2026
Merged via the queue into main with commit 6be0e33 Sep 7, 2026
71 checks passed
@kojiwakayama
kojiwakayama deleted the codex/issue-1020-node-response-defaults branch September 7, 2026 08:38
@kwakayama

Copy link
Copy Markdown
Contributor

Independent Codex review

Reviewed SHA: edbf81f0d4859d0637e2ba62e30ebfe4fdd77e11
Base SHA: 33b8ef63420c04a6c3ae1937e35daa0a3ca2fc91
Verdict: APPROVE
Score: 98/100

Findings

No actionable findings.

  1. Correctness and completeness (40/40): buildResponseInit inspects the captured native Object.prototype without invoking accessors, rejects accessor descriptors, and supplies fresh null-prototype headers plus explicit status and statusText defaults (src/agent/service/response-init.ts:22-34). createNativeResponse applies it at the shared constructor boundary used by health, readiness, preflight, missing-route, and CORS-wrapped host responses (src/agent/service/definition.ts:241-247,617,652-660,720-741). Explicit invalid values still reach native validation.
  2. Regression tests and verification (20/20): Unit tests cover independent headers, explicit valid and invalid values, inherited data properties, and getter/setter rejection without invocation (src/agent/service/response-init.test.ts:6-69). The cross-runtime integration test covers denied-origin CORS injection across liveness, readiness, shutdown, preflight, missing and custom routes, plus allowed-origin behavior, response preservation, malformed headers, invalid statuses, and exact host-error identity (tests/integration/agent/service-response-init.test.ts:14-185). The suite planners register it for both Node and Bun.
  3. Reliability and security (15/15): The change repairs the native conversion boundary directly. It has no swallowed exception, weaker alternate path, retry, or success-shaped fallback. Accessor pollution fails closed before project code executes.
  4. Standards and maintainability (15/15): The helper is small, internal, dependency-free, and uses captured intrinsics consistent with the surrounding trust-boundary code. Format, focused lint, diff whitespace, and standalone helper type checks pass.
  5. Scope, docs, and rollout (8/10): The guide accurately documents writable data-property override, accessor failure, non-writable native failure, and unchanged error semantics. The linked issue correctly remains open for published-artifact and two-replica staging verification, which is outside this merged source review.

Verification and gaps

  • Local Node v25.9.0 probe reproduced the vulnerable native behavior: inherited Access-Control-Allow-Origin and Access-Control-Allow-Credentials appeared on a plain Response.
  • The same local probe exercised the patched helper with 15 assertions: injected CORS headers and status text were absent, defaults were own null-prototype values, inherited accessors were never invoked and raised the documented TypeError, non-writable inherited data retained native failure, and status 99 retained native RangeError.
  • Passed locally: focused Deno type check for response-init.ts, changed-file Deno lint, changed-file format check, and git diff --check.
  • The checkout has Deno 2.9.4 while the repository pins 2.7.7, so direct full-module Deno tests hit unrelated runtime brand-check and React declaration failures. Exact-head CI supplies the authoritative pinned-toolchain evidence: 43 checks passed with 0 failures and 0 pending, including typecheck, lint, Deno integration, Node, minimum Node sandbox, Bun, coverage, CodeQL, Sonar, and npm compatibility artifact.
  • Review-thread pagination is complete and both prior threads are resolved. PR head remained stable during evidence retrieval.
  • Remaining rollout gap: RunResumeSessionManager: abort with DOMException AbortError, not custom Error #1036 still requires verification of the exact published package on both ready staging replicas before closure.

Review-Gate:
Reviewer: Codex
Reviewed-SHA: edbf81f
Score: 98/100
Actionable-Findings: 0
Verdict: APPROVE

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.

2 participants