rstudio / redx

dynamic nginx configuration
BSD 2-Clause "Simplified" License
117 stars 27 forks source link

multipart/form-data file upload broken - nginx/redx hangs and never forwards request body #19

Closed whummer closed 4 years ago

whummer commented 9 years ago

It seems that file upload requests of the following form cannot be handled by redx:

POST /upload HTTP/1.1
Content-Type: multipart/form-data; boundary=---a52b3f2495c75e44
Content-Length: 156
...

-----a52b3f2495c75e44
Content-Disposition: form-data; name="uploadfile";filename="uploadfile"
Content-type: plain/text

test123
-----a52b3f2495c75e44--

The headers are properly forwarded to the target backend host, but then redx hangs and the request body is not forwarded (timeout after 30 secs). The reason seems to be an error in lapis/nginx.lua which attempts to parse the multipart message but hangs in the "while true" loop: https://github.com/leafo/lapis/blob/26f4d8f5316969777fc270e262bd6b4a013e0713/lapis/nginx.lua#L18

A dirty "quick fix" is to disable the resty.upload module from being instantiated properly, in nginx.conf:

location / {
        access_by_lua '
            -- fix to disable file upload handling (i.e., force pass-through)
            require("resty.upload").new = function(self, chunk_size)
                return nil, nil
            end
            require("lapis").serve("main")
        ';
        ...
}

This way the resty.upload module is not used in lapis.nginx and the request body is forwarded without being parsed (see line 22): https://github.com/leafo/lapis/blob/26f4d8f5316969777fc270e262bd6b4a013e0713/lapis/nginx.lua#L22

For redx as a reverse proxy, it should not be necessary to parse message contents at all. It would be great if redx could fix this issue (in a more elegant way than the suggested quick fix), or at least provide a configuration option to disable multipart/form-datamessage parsing.

jspiewak commented 7 years ago

The resty.upload module should be able to be disabled when building openresty using the --without-lua_resty_upload option to ./configure.

jspiewak commented 7 years ago

Strike that, that just causes lapis to blow up with a 500 and not log anything...

pyk commented 4 years ago

@jspiewak just reminder, I think this issue should be closed 😁