Closed delucca closed 4 months ago
Thanks for reporting!
Can you provide steps to reproduce? We often need a reproducible example, e.g. some code that allows someone else to recreate your problem by just copying and pasting it. If it involves more than a couple of different file, create a new repository on GitHub and add a link to that.
I'm not sure what the problem is but it would be awesome if you'd try a fix!
@mcollina sure 😄
I was trying to debug it for a while and just stopped. Next monday I'll try to create a sample repo with it and share with you over here 😄
@mcollina The browser always sends content-length: 0
for DELETE
requests. But @fastify/http-proxy
can't do this request, because of undici issue. So I can't use proxy (without rewriteRequestHeaders
options) for DELETE
requests. Is it bug or not?
replyOptions: {
rewriteRequestHeaders(originalReq, headers) {
const rewrittenHeaders = { ...headers }
// undici issue https://github.com/nodejs/undici/issues/2046
if (originalReq.method === 'DELETE') {
rewrittenHeaders['content-length'] = undefined
}
return rewrittenHeaders
},
}
The browser always sends content-length: 0 for DELETE requests
Is this documented somewhere? is there a repro I can check?
Is this documented somewhere?
Sorry, my fault, looks like it's only Safari problem.
is there a repro I can check?
This example works in Chrome, but fails in Safari (Version 16.4 (18615.1.26.110.1)).
Console output for Safari
[Info] status – 500 (index.html, line 8)
[Error] error – "Request body length does not match content-length header" (anonymous function) (index.html:13)
Console output for Chrome
status 200
index.html:13 error undefined
// server
const Fastify = require('fastify');
const server = Fastify();
server.register(require('@fastify/cors'), {
origin: "*",
methods: ["DELETE"]
});
server.register(require('@fastify/http-proxy'), {
upstream: 'http://localhost:3000/api2',
prefix: '/api', // optional
http2: false, // optional
// NOTE: server fix for Safari
// replyOptions: {
// rewriteRequestHeaders(originalReq, headers) {
// const rewrittenHeaders = { ...headers }
// // undici issue https://github.com/nodejs/undici/issues/2046
// if (originalReq.method === 'DELETE') {
// rewrittenHeaders['content-length'] = undefined
// }
// return rewrittenHeaders
// },
// },
});
server.route({
method: 'DELETE',
path: '/api2',
handler(request, reply) {
reply.send({ status: 'deleted' });
},
});
server.listen({ port: 3000 });
<!-- client -->
<html>
<head></head>
<body>
<script>
window.onload = function onLoad() {
fetch('http://localhost:3000/api', { method: 'DELETE' })
.then((result) => {
console.info('status', result.status)
return result.json()
})
.then((error) => {
console.error('error', error.message)
})
}
</script>
</body>
</html>
This matches our expectations... I guess no one uses Safari. I think we should just remove the content-length
for delete by default, at least until safari gets updated.
Alternatively we should just document this oddity, but I think it's better to have the patch for everyone.
I think this should be the default implementation of replyOptions.rewriteRequestHeaders
, meaning that an end user that overwrite that method would need to take care of this.
In this way we allow a fallback to prevsiou behavior.
@annahassel @delucca Would you like to send a PR along those lines?
I have a kinda similar issue, all DELETE requests failing with an error:
{
"statusCode":500,
"code":"FST_REPLY_FROM_INTERNAL_SERVER_ERROR",
"error":"Internal Server Error",
"message":"write after end"
}
GET, POST, PATCH, PUT methods work as intended.
Reproduce:
import fastify from 'fastify';
import proxy from '@fastify/http-proxy';
const proxyServer = fastify();
proxyServer.register(proxy, {
upstream: 'http://localhost:3001/example2',
prefix: '/example1',
http2: true,
});
proxyServer.listen({ port: 3000 });
const http2Server = fastify({ http2: true });
http2Server.route({
method: ['GET', 'PATCH', 'POST', 'PUT', 'DELETE'],
url: '/example2',
handler(_, reply) {
reply.send({ status: 'you got it!' });
},
});
http2Server.listen({ port: 3001 });
Curl:
curl --request DELETE --url http://localhost:3000/example1 # fail
curl --request POST --url http://localhost:3000/example1 # ok
Is it even correct to use a proxy this way? HTTP/1.1 => Proxy => Http/2
and reverse. Or am I doing something wrong?
PS: The upstream on which I received this behaviour supports versions 1.1
and 2
, but all proxied requests are sent with version 2
. It would be great if I could choose the version depending on the request being proxied.
Does curl
includes a Content-Length
header?
Nope, here's two traces of POST and DELETE requests from reproduce above:
DELETE
:
DELETE /example1 HTTP/1.1
Host: localhost:3000
User-Agent: curl/7.88.1
Accept: */*
HTTP/1.1 500 Internal Server Error
content-type: application/json; charset=utf-8
content-length: 124
Date: Mon, 29 Jan 2024 22:52:42 GMT
Connection: keep-alive
Keep-Alive: timeout=72
{"statusCode":500,"code":"FST_REPLY_FROM_INTERNAL_SERVER_ERROR","error":"Internal Server Error","message":"write after end"}
POST
:
POST /example1 HTTP/1.1
Host: localhost:3000
User-Agent: curl/7.88.1
Accept: */*
HTTP/1.1 200 OK
content-type: application/json; charset=utf-8
content-length: 24
date: Mon, 29 Jan 2024 22:54:27 GMT
Connection: keep-alive
Keep-Alive: timeout=72
{"status":"you got it!"}
I currently do not have much time to debug this. However a PR would be nice.
I've had a look at this and it seems to be caused by this call to req.end
in the HTTP2 request handler from @fastify/reply-from
.
The DELETE request does succeed without that call to .end
, but it's not clear to me why it is needed, and why all other non-GET (i.e. POST, PATCH...) requests work fine with it.
Update: It seems that is indeed only GET and DELETE for which .end
should not be called. I have opened a PR in fastify/fastify-reply-from
to address this.
Prerequisites
Fastify version
3.x.x
Plugin version
7.1.0
Node.js version
16.x.x
Operating system
Linux
Operating system version (i.e. 20.04, 11.3, 10)
Fedora 35
Description
When running any
DELETE
request without a body with HTTP2 we got the following error:If we run the same request with HTTP1, the error doesn't happen
Even if we use
http2: false
on the proxy options, we still got that error if the request was executed with HTTP2Also, seems like Undici doesn't support HTTP2 yet, maybe that's the reason?
Steps to Reproduce
DELETE
endpointDELETE
endpoint using HTTP2Expected Behavior
The request should be executed as expected