fix: harden HTTPS CA trust, IPv6 verification, and content-encoding parsing - #3504
Merged
Conversation
ardatan
force-pushed
the
fix/bun-https-cleanup-timeout
branch
from
August 12, 2026 15:47
879661c to
89c70d9
Compare
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change makes libcurl use Node’s default CA store when available. It adds ephemeral certificate generation and shared HTTPS test-server support. Related tests now use protocol-aware requests and bounded HTTPS cleanup. ChangesCA and HTTPS infrastructure
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TestServer
participant CertificateHelper
participant NodeTLS
participant HTTPSClient
TestServer->>CertificateHelper: Generate ephemeral TLS certificates
CertificateHelper-->>TestServer: Return CA and server credentials
TestServer->>NodeTLS: Register the temporary CA
TestServer->>HTTPSClient: Serve and receive HTTPS requests
HTTPSClient-->>TestServer: Return test responses
TestServer->>NodeTLS: Restore default CA certificates
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
ardatan
force-pushed
the
fix/bun-https-cleanup-timeout
branch
from
August 12, 2026 15:47
89c70d9 to
62a2d38
Compare
Avoid flaky timeouts against httpbin.org in CI (especially unit/bun) while still exercising HTTPS fetch without consuming the body.
ardatan
force-pushed
the
fix/bun-https-cleanup-timeout
branch
from
August 12, 2026 15:47
62a2d38 to
ec52ce8
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
✅
|
Contributor
✅
|
Contributor
✅
|
Contributor
✅
|
There was a problem hiding this comment.
Pull request overview
This PR stabilizes the node-fetch “cleanup resources” HTTPS test in CI (notably Bun) by removing reliance on an external https://httpbin.org/get request and instead exercising HTTPS fetch against a local self-signed HTTPS server.
Changes:
- Replaces the external HTTPS request with a local ephemeral HTTPS server using a self-signed certificate.
- Adds Jest
beforeAll/afterAlllifecycle management to create/close the HTTPS server and temporarily relax TLS verification for the suite.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
✅
|
Contributor
✅
|
Contributor
✅
|
Register a shared node:https test server trusted via NODE_EXTRA_CA_CERTS / setDefaultCACertificates, keep httpbin.org HTTPS as best-effort, and make fetchCurl honor extra CA certs through CAINFO_BLOB.
ardatan
changed the title
Avoid NODE_TLS_REJECT_UNAUTHORIZED (CodeQL), skip node:https where pem/CA APIs are unavailable (Bun/Deno/older Node), and keep the server scheme when hitting [::1].
Trust ephemeral test certs only via NODE_EXTRA_CA_CERTS / setDefaultCACertificates in the test server helper.
Use Node's default CA store for libcurl instead of NODE_EXTRA_CA_CERTS, and align HTTPS test helpers on setDefaultCACertificates only.
Avoid dropping the previous libcurl CA path on older runtimes.
Contributor
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@whatwg-node/node-fetch |
0.8.7-alpha-20260812174353-1c03a6e0ec9ef41a744a20f75d8d94c90cb262fc |
npm ↗︎ unpkg ↗︎ |
@whatwg-node/server |
0.11.1-alpha-20260812174353-1c03a6e0ec9ef41a744a20f75d8d94c90cb262fc |
npm ↗︎ unpkg ↗︎ |
…ient Replace pem with an openssl CA+leaf helper and bridge Deno native fetch trust through Deno.createHttpClient, since it ignores tls.setDefaultCACertificates.
Keep node:https in the shared Node test matrix, drop the unused Deno CA bridge, and use https.request when suites hit https:// servers.
Parameters<typeof httpRequest> collapsed overloads and broke typecheck.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/libcurl-default-ca-store.md:
- Line 11: Update the guidance around tls.setDefaultCACertificates() to state
that the example requires Node.js 22.19.0 or 24.5.0 and later, or guard the
setter usage so unsupported Node.js versions do not execute it.
In `@packages/node-fetch/tests/cleanup-resources.spec.ts`:
- Around line 38-46: Update the catch handling in cleanup-resources.spec.ts to
soft-skip supported backend connectivity failures from node-libcurl, undici,
built-in undici, and node:http, normalizing their error shapes alongside the
existing AbortError and TimeoutError cases. Preserve rethrowing unexpected
application errors.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 26a87349-f2f5-47de-b29a-8f9553b76413
📒 Files selected for processing (8)
.changeset/libcurl-default-ca-store.mdpackages/node-fetch/src/fetchCurl.tspackages/node-fetch/tests/cleanup-resources.spec.tspackages/node-fetch/tests/http2.spec.tspackages/server/test/formdata.spec.tspackages/server/test/node.spec.tspackages/server/test/test-server.tspackages/server/test/test-tls-certs.ts
…A docs Address CodeRabbit: document setDefaultCACertificates Node version floor, and treat DNS/connection failures like timeouts in the httpbin cleanup test.
Node 22.23+/24.17+ break IPv6 IP-SAN matching in checkServerIdentity (nodejs/node#64032). Keep the test on HTTP servers.
Work around Node tls.checkServerIdentity IPv6 regression and cover it with unit + https matrix tests.
Cache a one-time tls.checkServerIdentity probe so healthy Node builds keep the built-in verifier.
ardatan
changed the title
Undici HTTPS sends 'br, gzip, deflate'; without trim, later encodings never matched and compression was skipped.
External connectivity failures (socket hang up / other side closed) should not fail CI.
ardatan
changed the title
Avoid flaky external network in CI; trust an ephemeral CA via setDefaultCACertificates.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR improves HTTPS behavior across
@whatwg-node/node-fetchand@whatwg-node/server, and stabilizes the HTTPS-related test matrix.Runtime —
@whatwg-node/node-fetchLibcurl CA store alignment
tls.getCACertificatesis available (Node.js 22.15+ / 23.10+),fetchCurlloads CAs fromtls.getCACertificates('default')viaCAINFO_BLOB.tls.setDefaultCACertificates(...)andNODE_EXTRA_CA_CERTSset before process start (same as Node’s built-inhttpsclient).enginesstill>=18) keeps the previous fallback:NODE_EXTRA_CA_CERTS→CAINFO, elsetls.rootCertificates.IPv6 HTTPS identity workaround (node-http ponyfill)
IP AddressSANs intls.checkServerIdentity.fetch/ undici are unchanged.Runtime —
@whatwg-node/serveruseContentEncodingand decompression helpers trim comma-separatedAccept-Encoding/Content-Encodingtokens.br, gzip, deflate(common for undici over HTTPS): without trim, encodings after the first comma never matched.Tests
Shared
node:httpsTestServertls.setDefaultCACertificatesexists (Node / Bun when exposed); not on Deno.test-tls-certs.ts), trusted viasetDefaultCACertificates.localhost,127.0.0.1, and::1.Cleanup / external HTTPS
node:https(body not consumed).https://httpbin.orgremains best-effort (timeout + soft-skip for connectivity errors).Other suite hardening
http2.spec.tsuses the shared openssl helper +setDefaultCACertificates.httpvshttpsfrom the server URL.node:httpsipv6 stays skipped where undici still hits the Node bug.Changesets
libcurl-default-ca-store@whatwg-node/node-fetchipv6-check-server-identity@whatwg-node/node-fetchtrim-content-encoding-tokens@whatwg-node/serverTest plan
unit / bun,unit / deno,unit / node 18–26greentype check/lint/prettiergreennode:https; external HTTPS soft-skips when unreachablenode:httpswithout explicitaccept-encodingstill compressescheckServerIdentity.spec.tspasses