Repository: unjs/h3
Pull request #746 (2024-05-05 13:44:51)

fix(proxy): better error when upstream proxy fails
<!---
☝️ PR title should follow conventional commits (https://conventionalcommits.org) 
-->

### 🔗 Linked issue

#722

<!-- Please ensure there is an open issue and mention its number as #123 -->

### ❓ Type of change

<!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply. -->

- [ ] 📖 Documentation (updates to the documentation, readme, or JSdoc annotations)
- [x] 🐞 Bug fix (a non-breaking change that fixes an issue)
- [x] 👌 Enhancement (improving an existing functionality like performance)
- [ ] ✨ New feature (a non-breaking change that adds functionality)
- [ ] 🧹 Chore (updates to the build process or auxiliary tools and libraries)
- [ ] ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

### 📚 Description

<!-- Describe your changes in detail -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If it resolves an open issue, please link to the issue here. For example "Resolves #1337" -->

This PR should ensure that `proxyRequest`s fail gracefully instead of leading to an unhandled node error crashing the server. 

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [x] I have linked an issue or discussion.
- [ ] I have updated the documentation accordingly.


cloudflare-pages[bot]: ## Deploying unjs-h3 with &nbsp;<a href="https://pages.dev"><img alt="Cloudflare Pages" src="https://user-images.githubusercontent.com/23264/106598434-9e719e00-654f-11eb-9e59-6167043cfa01.png" width="16"></a> &nbsp;Cloudflare Pages

<table><tr><td><strong>Latest commit:</strong> </td><td>
<code>5ae0345</code>
</td></tr>
<tr><td><strong>Status:</strong></td><td>⚡️&nbsp; Build in progress...</td></tr>
</table>

[View logs](https://dash.cloudflare.com/?to=/:account/pages/view/unjs-h3/70d40dc8-0e06-4128-9baf-14238617cfe2)

manniL: Updated to `sendProxy`. 

> I think only we can assume only proxy had been failed and perhaps we can double check if there is not already set statusCode and message on error (that currently is being ingored)

You mean an existing one in `event.node.statusCode` / `statusMessage` or in the error directly? If the latter, which values should I check?

Patch:
diff --git a/src/utils/proxy.ts b/src/utils/proxy.ts
index b5287a7..0216e2d 100644
--- a/src/utils/proxy.ts
+++ b/src/utils/proxy.ts
@@ -4,6 +4,7 @@ import { getRequestHeaders } from "./request";
 import { splitCookiesString } from "./cookie";
 import { sanitizeStatusMessage, sanitizeStatusCode } from "./sanitize";
 import { getRequestWebStream, readRawBody } from "./body";
+import { createError } from "../error";
 
 export type Duplex = "half" | "full";
 
@@ -81,11 +82,20 @@ export async function sendProxy(
   target: string,
   opts: ProxyOptions = {},
 ) {
-  const response = await _getFetch(opts.fetch)(target, {
-    headers: opts.headers as HeadersInit,
-    ignoreResponseError: true, // make $ofetch.raw transparent
-    ...opts.fetchOptions,
-  });
+  let response: Response | undefined;
+  try {
+    response = await _getFetch(opts.fetch)(target, {
+      headers: opts.headers as HeadersInit,
+      ignoreResponseError: true, // make $ofetch.raw transparent
+      ...opts.fetchOptions,
+    });
+  } catch (error) {
+    throw createError({
+      status: 502,
+      statusMessage: "Bad Gateway",
+      cause: error,
+    });
+  }
   event.node.res.statusCode = sanitizeStatusCode(
     response.status,
     event.node.res.statusCode,


Test patch:
diff --git a/test/proxy.test.ts b/test/proxy.test.ts
index 82c1c39..fa11e46 100644
--- a/test/proxy.test.ts
+++ b/test/proxy.test.ts
@@ -1,7 +1,7 @@
 import { Server } from "node:http";
 import { readFile } from "node:fs/promises";
 import supertest, { SuperTest, Test } from "supertest";
-import { describe, it, expect, beforeEach, afterEach } from "vitest";
+import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
 import { fetch } from "node-fetch-native";
 import {
   createApp,
@@ -16,7 +16,9 @@ import {
 } from "../src";
 import { sendProxy, proxyRequest } from "../src/utils/proxy";
 
-describe("", () => {
+const spy = vi.spyOn(console, "error");
+
+describe("proxy", () => {
   let app: App;
   let request: SuperTest<Test>;
 
@@ -33,6 +35,9 @@ describe("", () => {
       },
       toNodeListener(app),
     );
+    server.on("error", (error) => {
+      console.log("[server error]", error);
+    });
     await new Promise((resolve) => {
       server.listen(0, () => resolve(undefined));
     });
@@ -242,6 +247,29 @@ describe("", () => {
 
       expect(resText).toEqual(message);
     });
+
+    it(
+      "can handle failed proxy requests gracefully",
+      async () => {
+        spy.mockReset();
+        app.use(
+          "/",
+          eventHandler((event) => {
+            return proxyRequest(
+              event,
+              "https://this-url-does-not-exist.absudiasdjadioasjdoiasd.test",
+            );
+          }),
+        );
+
+        await fetch(`${url}/`, {
+          method: "GET",
+        });
+
+        expect(spy).not.toHaveBeenCalled();
+      },
+      60 * 1000,
+    );
   });
 
   describe("multipleCookies", () => {