Fix https breakage of resolved ips#184
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses the HTTPS/TLS regression introduced by DNS-rebinding hardening when resolveDNSHost returns an IP, and restructures the package into explicit Node vs mobile entrypoints to avoid bundler/runtime breakage from Node-only dependencies.
Changes:
- Introduces a Node-specific entrypoint that pins HTTPS connections to the validated IP at the connection layer (via
undicidispatcher) while preserving the original hostname for SNI/TLS. - Adds a
/mobileentrypoint that avoidsundici/Node-only dependencies and omitsresolveDNSHost. - Updates package exports/build/test tooling to support the new entrypoints and migrate tests to
bun:test.
Reviewed changes
Copilot reviewed 14 out of 17 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Switches TS ambient types to Bun (impacts editor/test typing). |
| tsconfig.build.json | Adds a build-specific TS config excluding tests. |
| shared.ts | Extracts shared parsing/types used by both entrypoints. |
| README.md | Documents the new entrypoints and clarifies HTTPS hostname/SNI behavior. |
| package.json | Adds exports for /node and /mobile, adds undici, changes scripts/engines/version. |
| node.ts | New Node entrypoint implementing dispatcher-based HTTPS pinning. |
| mobile.ts | New mobile/browser-safe entrypoint without Node-only deps or DNS pinning. |
| mise.toml | Adds Bun tool configuration. |
| jest.config.js | Removes Jest configuration (tests moved off Jest). |
| index.ts | Makes the default entrypoint re-export the Node entrypoint. |
| bun.lock | Updates lockfile for Bun + new dependencies. |
| bump-version.sh | Removes version bump script. |
| .github/workflows/tests.yml | Updates CI setup to use mise and run Bun tests. |
| tests/mobile.spec.ts | Adds tests for the /mobile entrypoint behavior and dependency constraints. |
| tests/index.spec.ts | Updates tests for Bun and adds coverage for the HTTPS pinning regression. |
| tests/fixtures/verify-undici-pinning.node.cjs | Adds a Node-only fixture to verify undici lookup override behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Addresses #182
By also adding undici as a dep we now break other runtimes. This PR also tries to add a new entrypoint
/mobilethat does not require undici and skips all the DNS protection code because mobile runtimes do not run localhost servers, so they should be redundant