Skip to content

Fixes breakage of resolving hostnames to ips on https #183

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ospfranco wants to merge 1 commit into from
Closed

Fixes breakage of resolving hostnames to ips on https #183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,8 @@ getLinkPreview("http://maliciousLocalHostRedirection.com", {

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.

## Redirections

Same to SSRF, following redirections is dangerous, the library errors by default when the response tries to redirect the user. There are however some simple redirections that are valid (e.g. HTTP to HTTPS) and you might want to allow them, you can do it via:
Expand Down
30 changes: 30 additions & 0 deletions __tests__/index.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
});

expect(linkInfo.url).toEqual(`https://www.youtube.com/watch?v=wuClZjOdT30`);
expect(linkInfo.siteName).toEqual(`YouTube`);

Check failure on line 27 in __tests__/index.spec.ts

View workflow job for this annotation

GitHub Actions / tests

error: expect(received).toEqual(expected) 

Expected: "YouTube"
Received: undefined

      at <anonymous> (/home/runner/work/link-preview-js/link-preview-js/__tests__/index.spec.ts:27:31)
expect(linkInfo.title).toEqual(`Geography Now! Germany`);
expect(linkInfo.description).toBeTruthy();
expect(linkInfo.mediaType).toEqual(`video.other`);
Expand Down Expand Up @@ -61,7 +61,7 @@
);

expect(linkInfo.url).toEqual(`https://www.youtube.com/watch?v=wuClZjOdT30`);
expect(linkInfo.title).toEqual(`Geography Now! Germany`);

Check failure on line 64 in __tests__/index.spec.ts

View workflow job for this annotation

GitHub Actions / tests

error: expect(received).toEqual(expected) 

Expected: "Geography Now! Germany"
Received: " - YouTube"

      at <anonymous> (/home/runner/work/link-preview-js/link-preview-js/__tests__/index.spec.ts:64:28)
expect(linkInfo.siteName).toBeTruthy();
expect(linkInfo.description).toBeTruthy();
expect(linkInfo.mediaType).toEqual(`video.other`);
Expand All @@ -79,7 +79,7 @@
);

expect(linkInfo.url).toEqual(`https://www.youtube.com/watch?v=wuClZjOdT30`);
expect(linkInfo.title).toEqual(`Geography Now! Germany`);

Check failure on line 82 in __tests__/index.spec.ts

View workflow job for this annotation

GitHub Actions / tests

error: expect(received).toEqual(expected) 

Expected: "Geography Now! Germany"
Received: " - YouTube"

      at <anonymous> (/home/runner/work/link-preview-js/link-preview-js/__tests__/index.spec.ts:82:28)
expect(linkInfo.siteName).toEqual(`YouTube`);
expect(linkInfo.description).toBeTruthy();
expect(linkInfo.mediaType).toEqual(`video.other`);
Expand Down Expand Up @@ -305,6 +305,36 @@
}
});

it(`should keep the original hostname for HTTPS requests to preserve SNI/TLS`, async () => {
const fetchResponse = new Response(
`<html><head>
<meta property="og:title" content="Resolved host">
<meta property="og:description" content="Resolved address test">
</head></html>`,
{
headers: {
"content-type": "text/html",
},
},
);
Object.defineProperty(fetchResponse, "url", {
value: "https://example.com/",
});
const fetchSpy = jest.spyOn(globalThis, "fetch").mockResolvedValue(fetchResponse);

try {
const response = await getLinkPreview(`https://example.com/`, {
resolveDNSHost: async () => "93.184.216.34",
});

expect((response as any).title).toEqual("Resolved host");
expect(fetchSpy).toHaveBeenCalledTimes(1);
expect(fetchSpy.mock.calls[0][0]).toEqual("https://example.com/");
} finally {
fetchSpy.mockRestore();
}
});

it(`should block resolved local addresses in normalized IPv6 forms`, async () => {
const fetchSpy = jest
.spyOn(globalThis, "fetch")
Expand Down Expand Up @@ -365,7 +395,7 @@
);

expect(res.siteName).toEqual(`New Path Title`);
expect(res.title).toEqual(

Check failure on line 398 in __tests__/index.spec.ts

View workflow job for this annotation

GitHub Actions / tests

error: expect(received).toEqual(expected) 

Expected: "Falling Markets: How To Stop A Buyer From Getting Out | New Path Title"
Received: "Page Not Found | New Path Title"

      at <anonymous> (/home/runner/work/link-preview-js/link-preview-js/__tests__/index.spec.ts:398:23)
`Falling Markets: How To Stop A Buyer From Getting Out | New Path Title`,
);
expect(res.description).toBeTruthy();
Expand Down
9 changes: 9 additions & 0 deletions index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,15 @@ function getValidatedFetchUrl(url: string, resolvedAddressOrUrl?: string) {
}

const parsedUrl = new URL(url);

// 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.
Comment on lines +253 to +256
if (parsedUrl.protocol === "https:") {
return url;
}

parsedUrl.hostname = formatHostnameForUrl(resolvedAddress);
return parsedUrl.href;
}
Expand Down
Loading