You’re truly awesome! I’ll give the patch a try tomorrow - and thanks for the other bits and pieces of information, especially regarding the expectations as well.
I wish you an awesome Sunday! Best Regards, Lucas Rolff > On 10 Jul 2022, at 10:35, Maxim Dounin <mdou...@mdounin.ru> wrote: > > Hello! > > On Fri, Jul 08, 2022 at 07:13:33PM +0000, Lucas Rolff wrote: > >> I’m having an nginx instance where I utilise the nginx slice >> module to slice upstream mp4 files when using proxy_cache. >> >> However, I have an interesting origin where if sending a range >> request (which happens when the slice module is enabled), to a >> file that’s less than the slice range, the origin returns a 200 >> OK, but with the range related headers such as content-range, >> but obviously the full file is returned since it’s within the >> requested range. >> >> When playing the MP4s through Google Chrome and Firefox it works >> fine when going through the nginx proxy instance, however, it >> somehow breaks Safari (both on MacOS, and iOS) - I guess Safari >> is more strict. >> When playing directly through the origin it works fine in all >> browsers. >> >> The md5 of response from the origin remains the same, so it’s >> not that the response itself is an invalid MP4 file, and even if >> you compare the cache files on disk with a “working” origin and >> the “broken” origin (one sends a 206 Partial Content, another >> sends 200 OK) - the content of the cache files remain the same, >> except obviously the header section of the cache file. >> >> The origin returns a 206 status code, only if the file exceeds >> the slice size, so if I configure a slice size of 5 megabyte, >> only files above 5 megabytes will give 206s. Anything under 5 >> megabytes will result in a 200 OK with content-range and the >> correct content-length, >> >> Looking in the slice module itself I see: >> https://github.com/nginx/nginx/blob/master/src/http/modules/ngx_http_slice_filter_module.c#L116-L126 >> >> >> if (r->headers_out.status != NGX_HTTP_PARTIAL_CONTENT) { >> if (r == r->main) { >> ngx_http_set_ctx(r, NULL, ngx_http_slice_filter_module); >> return ngx_http_next_header_filter(r); >> } >> >> ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, >> "unexpected status code %ui in slice response", >> r->headers_out.status); >> return NGX_ERROR; >> } >> >> This seems like the slice module expects a 206 status code to be >> returned, > > For the main request, the code accepts two basic valid variants: > > - 206, so the slice module will combine multiple responses to > range requests as needed; > > - anything else, so the slice module will give up and simply > return the response to the client. > > If the module sees a non-206 response to a subrequest, this is an > error, as the slice module expects underlying resources to be > immutable, and does not expect that some ranges can be requested, > while some other aren't. This isn't something related to your > case though. > >> however, later in the same function >> https://github.com/nginx/nginx/blob/master/src/http/modules/ngx_http_slice_filter_module.c#L200-L211 >> >> >> if (r->headers_out.status == NGX_HTTP_PARTIAL_CONTENT) { >> if (ctx->start + (off_t) slcf->size <= r->headers_out.content_offset) >> { >> ctx->start = slcf->size >> * (r->headers_out.content_offset / slcf->size); >> } >> >> ctx->end = r->headers_out.content_offset >> + r->headers_out.content_length_n; >> >> } else { >> ctx->end = cr.complete_length; >> } >> >> There it will do an else statement if the status code isn’t 206. >> So would this piece of code ever be reached, since there’s the initial error? > > Following the initial check, r->headers_out.status is explicitly > changed to NGX_HTTP_OK. Later on the > ngx_http_next_header_filter() call might again change > r->headers_out.status as long as the client used a range request, > and this is what checked here. > >> Additionally I don’t see in RFC7233 that 206 responses are an >> absolute requirement, additionally I don’t see content-range >> being prohibited/forbidden to be used for 200 OK responses. >> Now, if one have a secondary proxy that modifies the response >> headers in between the origin returning 200 OK with the >> Content-Range header, and then strip out the Content-Range >> header, the nginx slice module seems to handle it fine, so >> somehow the combination of 200 OK and a Content-Range header >> being present seems to break the slice module from functioning. >> >> I’m just curious why this happens within the slice module, and >> if there’s any possible solution for it (like allowing the >> combination of 200 OK and Content-Range, since those two would >> still indicate that the origin/upstream supports range requests) >> - obviously it would be nice to fix the upstream server but >> sometimes that’s sadly not possible. > >> From the above explanation it is probably already clear that > "disabling slice when an origin returns 200 OK" is what actually > happens. > > The issue does not appear without the slice module in your testing > because the Content-Range header seems to be only present in your > backend 200 responses when there was a Range header in the > request, and this is what happens only with the slice module. > > I've done some limited testing with Safari and manually added > Content-Range header, and there seem to be at least two issues: > > - Range filter in nginx does not expect the Content-Range header > to be already present in 200 responses and simply adds another > one. This results in incorrect range responses with multiple > Content-Range headers, and this breaks Safari. > > - Safari also breaks if its test request with "Range: bytes=0-1" > results in 200 with the Content-Range header. > > My initial fix was to simply disable handling of 200 responses > with Content-Range headers in the range filter, so such responses > wouldn't be touched at all. This is perfectly correct and > probably the most secure thing to do, but does not work with > Safari due to the second issue outlined above. > > Another approach would be to clear pre-existing Content-Range > headers in the range filter. This seems to work, at least in my > testing. See below for the patch. > >> I know the parts of the slice module haven’t been touched for >> years, so obviously it works for most people, just dipping my >> toes here to see if there’s a possible solution other than >> disabling slice when an origin returns 200 OK for files smaller >> than the slice size. > > Note that that slice module is generally unsafe to use for > arbitrary upstream servers: it relies on expectations which are > beyond the HTTP standard requirements. In particular: > > - It requires resources to be immutable, so different range > responses can be combined together. > > - It does not try to handle edge cases, such as 416 returned by > the upstream on empty files (which is correct per RFC, but > requires complicated additional handling to convert 416 to 200, so > it is better to just return 200 OK). > > In general, the slice module is to be used only in your own > infrastructure when you control the backend and can be sure that > the slice module expectations are met. As such, disabling it for > backends which do something unexpected might actually be a good > idea. On the other hand, in this particular case the nginx > behaviour can be adjusted to handle things gracefully. > > Below is a patch to clear pre-existing Content-Range headers > in the range filter. Please test if it works for you. > > # HG changeset patch > # User Maxim Dounin <mdou...@mdounin.ru> > # Date 1657439390 -10800 > # Sun Jul 10 10:49:50 2022 +0300 > # Node ID 219217ea49a8d648f5cadd046f1b1294ef05693c > # Parent 9d98d524bd02a562d9cd83f4e369c7e992c5753b > Range filter: clearing of pre-existing Content-Range headers. > > Some servers might emit Conten-Range header on 200 responses, and this > does not seem to contradict RFC 9110: as per RFC 9110, the Content-Range > header have no meaning for status codes other than 206 and 417. Previously > this resulted in duplicate Content-Range headers in nginx responses handled > by the range filter. Fix is to clear pre-existing headers. > > diff --git a/src/http/modules/ngx_http_range_filter_module.c > b/src/http/modules/ngx_http_range_filter_module.c > --- a/src/http/modules/ngx_http_range_filter_module.c > +++ b/src/http/modules/ngx_http_range_filter_module.c > @@ -425,6 +425,10 @@ ngx_http_range_singlepart_header(ngx_htt > return NGX_ERROR; > } > > + if (r->headers_out.content_range) { > + r->headers_out.content_range->hash = 0; > + } > + > r->headers_out.content_range = content_range; > > content_range->hash = 1; > @@ -582,6 +586,11 @@ ngx_http_range_multipart_header(ngx_http > r->headers_out.content_length = NULL; > } > > + if (r->headers_out.content_range) { > + r->headers_out.content_range->hash = 0; > + r->headers_out.content_range = NULL; > + } > + > return ngx_http_next_header_filter(r); > } > > @@ -598,6 +607,10 @@ ngx_http_range_not_satisfiable(ngx_http_ > return NGX_ERROR; > } > > + if (r->headers_out.content_range) { > + r->headers_out.content_range->hash = 0; > + } > + > r->headers_out.content_range = content_range; > > content_range->hash = 1; > > -- > Maxim Dounin > http://mdounin.ru/ > _______________________________________________ > nginx mailing list -- nginx@nginx.org > To unsubscribe send an email to nginx-le...@nginx.org _______________________________________________ nginx mailing list -- nginx@nginx.org To unsubscribe send an email to nginx-le...@nginx.org
