openresty / lua-nginx-module

Embed the Power of Lua into NGINX HTTP servers
https://openresty.org/
11.33k stars 2.03k forks source link

bugfix: remove extra spaces at the end of the request field name #2257

Closed lynch1981 closed 8 months ago

lynch1981 commented 1 year ago

I hereby granted the copyright of the changes in this pull request to the authors of this lua-nginx-module project.

According to RFC7230 second paragraph of section-[3.2.4] (https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.4). No whitespace is allowed between the header field-name and colon.

I just remove the extra space at the end of the request field-name for user convenience. Nginx won't complain about headers like this, so the response part does not need to be processed. fix #2140

Sorry the previous PR was closed by mistake, so i make a new one

zhuizhuhaomeng commented 1 year ago

The header name will be escaped later. There are other invalid characters that need to be processed if we need to validate the header name.

lynch1981 commented 1 year ago

I checked the test case(t/028-req-header.t) and found that unsafe characters include \r \n \t. It might be better to remove all unsafe characters in header fields. your idea?

zhuizhuhaomeng commented 1 year ago

The header has been escaped here, so you don't need to remove the invalid characters.

ngx_int_t
ngx_http_lua_set_input_header(ngx_http_request_t *r, ngx_str_t key,
    ngx_str_t value, unsigned override)
{
    ngx_http_lua_header_val_t         hv;
    ngx_http_lua_set_header_t        *handlers = ngx_http_lua_set_handlers;
    ngx_int_t                         rc;
    ngx_uint_t                        i;

    dd("set header value: %.*s", (int) value.len, value.data);

    rc = ngx_http_lua_copy_escaped_header(r, &key, 1);
    if (rc != NGX_OK) {
        return NGX_ERROR;
    }

    rc = ngx_http_lua_copy_escaped_header(r, &value, 0);
    if (rc != NGX_OK) {
        return NGX_ERROR;
    }
lynch1981 commented 1 year ago

Thank you for your explanation, but the thing is that if a programmer types a white space in the header by mistake, the upstream server returns a 400 error even though it is escaped, and then he needs to spend some time debugging, and if he is not familiar with the http protocol, he will spend more time. that is what issue 2140 said.

lynch1981 commented 1 year ago

So I think it's programmer-friendly to tolerate these minor mistake

zhuizhuhaomeng commented 1 year ago

The better way is throwing errors instead of silently removing the spaces of other invalid characters. Because it is a per-request operation, it will have an impact on the impact. Therefore, we should reduce unnecessary operations.

lynch1981 commented 11 months ago

@zhuizhuhaomeng so sorry for the late response as I've been busy settling in recently. now i just add logs in debug mode to reduce unnecessary operations

zhuizhuhaomeng commented 8 months ago

@lynch1981 This PR cannot help the end user at all. So, I won't accept it.