openresty / openresty/lua-nginx-module

Bad request handling when subrequest reads the main request's body

Open
#756 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C
Stars
11.8k
Forks
2.1k
Avg merge
6h 1m
Merged PRs (30d)
6

Description

After upgrading to the latest ngx_lua, I've noticed the following bug:

  • A client connects to Nginx with a keep-alive connection.
  • The client performs a request with a body. The Lua handler doesn't read the body, and performs a subrequest.
  • The subrequest reads the request body.
  • The subrequest and main requests end normally.
  • The client sends another request on the same connections (keep-alive). Nginx returns a '400 - bad request' error.

Looking at the code, it appears that the correction to issue #493 produces the issue; look at commit 6fa6e97cc45f89eff2f9f4d64752f30ea18e3d16 or specifically, in the file ngx_http_lua_util.c line 493:
If the request body has been read by the subrequest, the main request's request_body field is NULL, and therefore the code thinks it should discard it, and so it reads extra bytes as the given Content-Length header. This causes the following request to start at an offset, to be parsed incorrectly, and to produce a "400 bad request" error.

I'm not sure how to correct the issue. In my own repository, I've done the following:

  • Added to the module's ctx object the field 'request_body_forwarded', a boolean field.
  • In 'ngx_http_lua_subrequest.c' line 660, setting the flag to 1.
  • In 'ngx_http_lua_util.c line 493', consider this flag as well, meaning discard the body only if this flag is false.
    This seems to fix the issue, but I'm not sure this is the proper fix.
    @agentzh , what do you think?

In order to reproduce, use the following configuration:

server {
       listen 80;
       server_name localhost;

       location / {
         content_by_lua "
         local sr = ngx.location.capture('/sub', {method = ngx.HTTP_POST})
         ngx.say('sr status: ' .. tostring(sr.status))
";
       }

       location /sub {
         content_by_lua "
         local req_body = ngx.req.read_body()
         ngx.say('subrequest! got body: ')
         ngx.say(req_body)
         ";
       }
}

And the following curl command:
curl -v http://127.0.0.1/blah?[0-1] --data-binary 1234567

The second command should fail with a 400 error.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Reproduce the failure with the provided server configuration and curl command, then inspect ngx_http_lua_util.c around line 493 and ngx_http_lua_subrequest.c around line 660. Compare the request-body state between the main request and subrequest, including the proposed request_body_forwarded flag. Done means a second request on the same keep-alive connection no longer receives a 400 error.

Written by the indexing model from the issue text.

Assessment

Tech stack
c, lua, nginx
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.