Fixes breakage of resolving hostnames to ips on https#183
Closed
ospfranco wants to merge 1 commit into
Closed
Conversation
There was a problem hiding this comment.
Pull request overview
Resolves the regression reported in issue #182 where resolveDNSHost caused HTTPS fetches to fail by rewriting the request hostname to an IP address (breaking TLS/SNI and certificate validation).
Changes:
- Preserve the original hostname for HTTPS URLs when
resolveDNSHostreturns an IP, while still performing SSRF preflight validation. - Add a regression test ensuring HTTPS requests are not rewritten to an IP.
- Document the HTTPS behavior in the SSRF section of the README.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| README.md | Documents that HTTPS requests keep the original hostname to avoid TLS/SNI breakage. |
| index.ts | Updates getValidatedFetchUrl to skip hostname-to-IP rewrites for https: URLs. |
| tests/index.spec.ts | Adds a test asserting HTTPS fetch URLs are not rewritten to the resolved IP. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+253
to
+256
| // Rewriting the hostname to a bare IP breaks TLS: the handshake sends the IP as | ||
| // the SNI server name and certificate validation fails, since certificates are | ||
| // issued for hostnames, not IPs. The resolved address is still used above (via | ||
| // throwOnLoopback) to block the request, so HTTPS requests keep their hostname. |
|
|
||
| This might add some latency to your request but prevents loopback attacks. | ||
|
|
||
| Note: for `https://` URLs the request is still made against the original hostname (not the resolved IP), since TLS validates the certificate and SNI against the hostname. The resolved address is only used to detect and block loopback/private targets before the request is made. |
Collaborator
Author
|
yeah, welp, I guess this solution doesn't work. |
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.
Fixes #182