Send HTTP 400 response for invalid request #205

markbreedlove · 2018-09-28T18:14:47Z

Given an invalid request, respond with an HTTP 400 error instead of closing the connection without a response.

Before this change, a request with invalid characters would result in the HTTP connection being dropped, as in this example:

$ curl -i -X GET 'http://localhost:8000/example?coordinates=76.916664, 8.483333'
curl: (52) Empty reply from server

Such a request should result in an HTTP 400 (Bad Request).

With a reverse-proxied application, this would typically be presented to the user as a 502 (Bad Gateway) error.

Temporarily pull uvicorn from the forked repository https://github.com/markbreedlove/uvicorn.git, which contains a bugfix for bad requests with invalid URLs. See encode/uvicorn#205 ... This change can be reverted when that PR is merged and a new release of uvicorn comes out with the fix.

tomchristie · 2018-09-28T18:30:30Z

Great work, thanks!

Before we decide on behaviour here I’d like to have a better idea of existing implementation behaviours.

Any of the following would be really useful to know:

How do gunicorn or node deal with that request, and does it make it to the application or not?
How does nginx deal with a request like that?

markbreedlove · 2018-09-28T18:54:10Z

Hi, Tom,

I've been running uvicorn with gunicorn as follows.

In development: gunicorn -k uvicorn.workers.UvicornWorker --log-level=debug --reload dplaapi:app
In production: gunicorn -w 2 -b 0.0.0.0:8000 -k uvicorn.workers.UvicornWorker dplaapi:app
... both exhibiting the same behavior from that curl request.

I'm running in AWS Elastic Beanstalk in production. The application is Dockerized and reverse-proxied with nginx.

Here's a sample line from nginx's access.log:

172.30.0.141 - - [28/Sep/2018:12:21:00 +0000] "GET /v2/items?api_key=[redacted]&sourceResource.spatial.coordinates=76.916664, 8.483333 HTTP/1.1" 502 173 "-" "-"

Here's the corresponding error.log line:

2018/09/28 12:21:00 [error] 22285#0: *15872 upstream prematurely closed connection while reading response header from upstream, client: 172.30.0.141, server: , request: "GET /v2/items?api_key=[redacted]&sourceResource.spatial.coordinates=76.916664, 8.483333 HTTP/1.1", upstream: "http://172.17.0.2:8000/v2/items?api_key=[redacted]&sourceResource.spatial.coordinates=76.916664, 8.483333", host: "api.dp.la"

Here's what gunicorn was printing on stdout; having it bubble up from uvicorn:

[2018-09-28 12:21:00 +0000] [10] [WARNING] Invalid HTTP request received.

The request did not make it past gunicorn and did not get down to my application, so to speak. As far as I can tell, gunicorn (or uvicorn?) never got to the point of making a call on my Api Star ASyncApp object. I'm not sure if that's the best way to describe it -- just let me know if that isn't clear!

tomchristie · 2018-09-28T19:27:59Z

Sorry I wasn’t clear. I meant what response do we get running just regular gunicorn and a plain old hello world wsgi app. That’d be a really useful point of comparison. (Node’s behaviour would be interesting too)

markbreedlove · 2018-10-02T21:17:23Z

Aha, of course, makes sense. Here's what I observe with gunicorn 19.9.0 and the "hello world" app at https://gunicorn.org.

(venv) uvicorn-pull205-test$ cat gunicornapp.py 
def app(environ, start_response):
    data = b"Hello, World!\n"
    start_response("200 OK", [
        ("Content-Type", "text/plain"),
        ("Content-Length", str(len(data)))
    ])
    return iter([data])
(venv) uvicorn-pull205-test$ 
(venv) uvicorn-pull205-test$ gunicorn -w 2 gunicornapp:app
[2018-10-02 17:11:52 -0400] [73491] [INFO] Starting gunicorn 19.9.0
[2018-10-02 17:11:52 -0400] [73491] [INFO] Listening at: http://127.0.0.1:8000 (73491)
[2018-10-02 17:11:52 -0400] [73491] [INFO] Using worker: sync
[2018-10-02 17:11:52 -0400] [73494] [INFO] Booting worker with pid: 73494
[2018-10-02 17:11:52 -0400] [73495] [INFO] Booting worker with pid: 73495
^C[2018-10-02 17:12:30 -0400] [73491] [INFO] Handling signal: int

... other terminal ...

~$ curl -i 'http://localhost:8000/?x=y z'
HTTP/1.1 400 Bad Request
Connection: close
Content-Type: text/html
Content-Length: 198

<html>
  <head>
    <title>Bad Request</title>
  </head>
  <body>
    <h1><p>Bad Request</p></h1>
    Invalid HTTP Version &#x27;Invalid HTTP Version: &#x27;z HTTP/1.1&#x27;&#x27;
  </body>
</html>

I'll check Node.js a bit later -- looks like tomorrow morning at this point ...

markbreedlove · 2018-10-02T21:31:35Z

Actually, this is easy enough in Node and I can do it now rather than later. I'm using this Hello World from https://nodejs.org/en/about/ ...

uvicorn-pull205-test$ cat nodeapp.js 
const http = require('http');

const hostname = '127.0.0.1';
const port = 3000;

const server = http.createServer((req, res) => {
  res.statusCode = 200;
  res.setHeader('Content-Type', 'text/plain');
  res.end('Hello World\n');
});

server.listen(port, hostname, () => {
  console.log(`Server running at http://${hostname}:${port}/`);
});
uvicorn-pull205-test$ 
uvicorn-pull205-test$ node nodeapp.js 
Server running at http://127.0.0.1:3000/
^C

... in another terminal ...

~$ curl -i 'http://localhost:3000/?x=y z'
curl: (52) Empty reply from server

So Node just closes the connection, whereas gunicorn responds with a 400.

I'm using Node v8.11.3.

Given an invalid request, respond with an HTTP 400 error instead of closing the connection without a response.

tomchristie · 2019-01-22T00:11:04Z

Okay, I like this, yup. Looks like there's a test failure for us to dig into.

gvbgduh · 2019-09-19T06:26:33Z

This could possibly be related to #344

Kludex · 2021-12-22T12:29:50Z

I'm interested on this. How Hypercorn deals with it: https://gitlab.com/pgjones/hypercorn/-/blob/main/src/hypercorn/protocol/h11.py#L147-153

Kludex · 2021-12-22T12:30:53Z

@markbreedlove Sorry for taking so long. Would you mind rebasing it?

euri10 · 2022-02-11T08:16:09Z

closed by #1352

markbreedlove force-pushed the send-400-for-invalid-request branch from 09577e1 to 0c84898 Compare October 22, 2018 18:41

Send HTTP 400 response for invalid request

8aa7bea

Given an invalid request, respond with an HTTP 400 error instead of closing the connection without a response.

markbreedlove force-pushed the send-400-for-invalid-request branch from 0c84898 to 8aa7bea Compare November 5, 2018 21:17

euri10 mentioned this pull request Dec 27, 2020

Sporatic errors with nginx connection pipelining #344

Closed

Kludex mentioned this pull request Jan 31, 2022

WARNING: Invalid HTTP request received. Only when running GET request from inside of Excel #1093

Closed

bertramr mentioned this pull request Feb 1, 2022

Send HTTP 400 response for invalid request #1352

Merged

euri10 closed this Feb 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send HTTP 400 response for invalid request #205

Send HTTP 400 response for invalid request #205

markbreedlove commented Sep 28, 2018

tomchristie commented Sep 28, 2018

markbreedlove commented Sep 28, 2018

tomchristie commented Sep 28, 2018

markbreedlove commented Oct 2, 2018

markbreedlove commented Oct 2, 2018 •

edited

tomchristie commented Jan 22, 2019

gvbgduh commented Sep 19, 2019

Kludex commented Dec 22, 2021

Kludex commented Dec 22, 2021

euri10 commented Feb 11, 2022

Send HTTP 400 response for invalid request #205

Send HTTP 400 response for invalid request #205

Conversation

markbreedlove commented Sep 28, 2018

tomchristie commented Sep 28, 2018

markbreedlove commented Sep 28, 2018

tomchristie commented Sep 28, 2018

markbreedlove commented Oct 2, 2018

markbreedlove commented Oct 2, 2018 • edited

tomchristie commented Jan 22, 2019

gvbgduh commented Sep 19, 2019

Kludex commented Dec 22, 2021

Kludex commented Dec 22, 2021

euri10 commented Feb 11, 2022

markbreedlove commented Oct 2, 2018 •

edited