Hi,

On Wed, Feb 22, 2023 at 03:55:28PM +0300, Maxim Dounin wrote:
> # HG changeset patch
> # User Maxim Dounin <mdounin@mdounin.ru>
> # Date 1677070454 -10800
> # Wed Feb 22 15:54:14 2023 +0300
> # Node ID ec3819e66f40924efad183c291c850d9ccef16e7
> # Parent 61bd779a868c4021c232dddfe7abda7e8ad32575
> HTTP/2: finalize request as bad if header validation fails.
>
> Similarly to 7192:d5a535774861, this avoids spurious zero statuses
> in access.log, and in line with other header-related errors.

This is what RFC 9113, 8.2.1. Field Validity, says about header field
validation:

A request or response that contains a field that violates
any of these conditions MUST be treated as malformed (Section 8.1.1).

Also (the previous RFC 7540 did not have this statement):

When a request message violates one of these requirements, an implementation
SHOULD generate a 400 (Bad Request) status code (see Section 15.5.1 of [HTTP])

Considering this, responding with HTTP 400 is certainly an improvement compared
to the current code. Also, it allows to set a custom error page handler.

However, not resetting the stream as before is a less obvious change. If we
consider nginx an intermediary, the RFC 9113 prescribes us to send stream reset:

Intermediaries that process HTTP requests or responses (i.e., any
intermediary not acting as a tunnel) MUST NOT forward a malformed
request or response. Malformed requests or responses that are detected
MUST be treated as a stream error (Section 5.4.2) of type PROTOCOL_ERROR.

This restriction probably makes sense for the intermediaries unable to handle
the error in a more flexible way.

I like the change, but I'd like to hear what you think about the reset issue.

> diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c
> --- a/src/http/v2/ngx_http_v2.c
> +++ b/src/http/v2/ngx_http_v2.c
> @@ -1794,14 +1794,7 @@ ngx_http_v2_state_process_header(ngx_htt
>
> /* TODO Optimization: validate headers while parsing. */
> if (ngx_http_v2_validate_header(r, header) != NGX_OK) {
> - if (ngx_http_v2_terminate_stream(h2c, h2c->state.stream,
> - NGX_HTTP_V2_PROTOCOL_ERROR)
> - == NGX_ERROR)
> - {
> - return ngx_http_v2_connection_error(h2c,
> - NGX_HTTP_V2_INTERNAL_ERROR);
> - }
> -
> + ngx_http_finalize_request(r, NGX_HTTP_BAD_REQUEST);
> goto error;
> }
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

--
Roman Arutyunyan
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel