Skip to content
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.

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

fix(proxy): workaround undici issue with DELETE and content-length of 0 #605

Closed
wants to merge 2 commits into from

Conversation

jasenmichael
Copy link

Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ 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

This pull request addresses the issue of proxy DELETE requests failing when no body or "content-length" header is present.

example error (this PR solves) when making a proxy DELETE request with nuxt3/nitro using h3:

export default defineNuxtConfig({
  routeRules: {
    '/api/**': { proxy: `${apiUrl}/**`},
  }
}
[nuxt] [request error] [unhandled] [500] fetch failed
  at Object.fetch (node:internal/deps/undici/undici:11372:11)  
  at process.processTicksAndRejections (node:internal/process/task_queues:95:5)  
  at async sendProxy (./node_modules/h3/dist/index.mjs:1084:20)  
  at async Object.handler (./node_modules/h3/dist/index.mjs:1680:19)  
  at async Server.toNodeHandle (./node_modules/h3/dist/index.mjs:1890:7)

Fixes:

  • Ensures an empty body and "content-length" header for proxy DELETE requests.
  • Adds tests to verify the fix.
  • DELETE errors related to async sendProxy (./node_modules/h3/dist/index.mjs)

Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly. (Check this box if you've also made documentation changes)

@@ -240,6 +240,43 @@ describe("", () => {

expect(resText).toEqual(message);
});

it("can proxy DELETE request", async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests seem to be pass even by commenting out the changes in utils/proxy we might add something that fails without it.

@@ -57,6 +57,12 @@ export async function proxyRequest(
opts.headers,
);

// DELETE method must always have an empty object in the body, and a "content-length" header.
if (method === "DELETE" && body === undefined) {
body = Buffer.from("{}");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By HTTP spec, it is not disallowed. However conventionally, HTTP servers fail when DELETE omits the body. I'm not sure using a JavaScript HTTP object would be best solution.

@pi0
Copy link
Member

pi0 commented Jan 4, 2024

Thanks for working on this PR dear @jasenmichael.

I'm not sure if you have already followed but this is relevant issue in undici that causes error (nodejs/undici#2046)

With nodejs/undici#2305, Node.js should now allow content-length of null instead of 0 to explicitly say there is no content which we could try to use instead.

@pi0 pi0 changed the title Fix delete proxy fix(proxy): workaround undici issue with DELETE and content-length of 0 Jan 4, 2024
@pi0 pi0 marked this pull request as draft January 4, 2024 20:29
@guibulator
Copy link

So nodejs/undici#2305 has been merged since https://github.com/nodejs/undici/releases/tag/v5.27.1.
Nitropack is already using undici 5.28.1. https://github.com/unjs/nitro/blob/b53e00191d0752c7ae7463a907f0858c8bb8182e/package.json#L59
I tried with only setting content-length to 0 or null with no body and it doesn't work. I can only make it work with the fix proposed in this MR.

@pi0
Copy link
Member

pi0 commented Jan 24, 2024

Nitropack is already using undici 5.28.1

it doesn't depend on any specific version of undici. it is only a dev dependency of the repository itself for another reason (miniflare used in test)

@pi0
Copy link
Member

pi0 commented Jan 24, 2024

Thanks for your PR dear @jasenmichael. earlier in comments, new tests pass with/without the changes (#605 (comment)) also we cannot do it automatically to provide a probably unwanted body with data of "{}" to the DELETE requests only to bypass a bug in Node.js/undici. (#605 (comment))

This is an important issue and I hope to find a fix but let's track in #375

@pi0 pi0 closed this Jan 24, 2024
@jasenmichael
Copy link
Author

Thanks for your PR dear @jasenmichael. earlier in comments, new tests pass with/without the changes (#605 (comment)) also we cannot do it automatically to provide a probably unwanted body with data of "{}" to the DELETE requests only to bypass a bug in Node.js/undici. (#605 (comment))

This is an important issue and I hope to find a fix but let's track in #375

Thank you, and yes I realized quickly this pr was not an actual solution, but more of a "use-case workaround", and would break any proxy that requires a DELETE request body to be empty.

After further debugging, I have NOT been able to re-create the issue in a nitro app. Proxied DELETE requests work fine directly in a nitro app, but not through Nuxt3 (both using the same versions of nitro and h3). I spun up the nitro app using the same routeRules used in Nuxt3.

here is my routeRules configuration:

// DELETE does not work through nuxt3, but does work directly in a nitro app.
routeRules: {
  "/dashboard/**": { proxy: "http://localhost:8055/**" },
}

Here is the error that Nuxt3 gives when making a proxied DELETE request:

[nuxt] [request error] [unhandled] [500] fetch failed
  at Object.fetch (node:internal/deps/undici/undici:14062:11)
  at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
  at async sendProxy (./node_modules/h3/dist/index.mjs:506:20)
  at async Object.handler (./node_modules/h3/dist/index.mjs:1247:19)
  at async Server.toNodeHandle (./node_modules/h3/dist/index.mjs:1322:7)

NOTE: When using Nuxt3, I see DELETE requests fail in the browser network tab, but if I edit and re-send with an empty object in the body it succeeds. My pr circumvents this only.

Since this is a proxy, I do not have control over DELETE requests, so sending an empty body is not an option.

Another point to make is - my server (that I am proxying to) does not require a body for DELETE requests, but still succeeds when I send an empty body. A proxied DELETE request through Nuxt3 works ONLY if there is something in the body.

My conclusion is - the failure only occurs when a proxied DELETE request is made through nuxt3 with an empty body.

related: #375

cheers,
btw, unjs is a serious game-changer! excellent work folks πŸ™

@pi0
Copy link
Member

pi0 commented Jan 25, 2024

Thanks for kind words and more details. Well I have to say it is now even more interesting! I will try to make same reproduction as you have with Nuxt. If you have better one, please feel free to share it in #375 that would make help a lot in order to followup.

@trc-mathieu
Copy link

Tested with nuxt 3.10 and node 20.11.0 and delete now works fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants