http icon indicating copy to clipboard operation
http copied to clipboard

`http_client_conformance_tests` `Content-Length` tests are not passable with fetch as per spec

Open Zekfad opened this issue 1 year ago • 9 comments

I'd like to make few of the test (e.g. server headers content length bigger than actual body) optional, because with fetch:

This makes the Content-Length header unreliable to the extent that it was reliable to begin with.

Ref: https://fetch.spec.whatwg.org/#ref-for-handle-content-codings%E2%91%A0 Ref: https://github.com/whatwg/fetch/issues/67

Zekfad avatar Jan 29 '24 00:01 Zekfad

What exact tests would you want to be optional? Maybe you could create a PR to make them optional?

brianquinlan avatar Feb 02 '24 23:02 brianquinlan

I'm not sure how to read the spec...do you not get a content-length header? Or does the browser correct it for you?

brianquinlan avatar Feb 08 '24 17:02 brianquinlan

Content length denotes size of the encoded response body, it's needed, so software know how much bytes they should expect, e.g. allocate the buffer.

Browsers on the other hand handles decoding of body opaquely, and you can no longer expect content length header to be exact precise.

Let's say you have 200 bytes gzipped body, header would be 200, but decoded size is 250bytes, 250 would be the size of body available to browser, and we couldn't peek at the original gzipped form.

Zekfad avatar Feb 08 '24 17:02 Zekfad

So the browser modifies "content-length" to match the actual number of decoded bytes in the body?

brianquinlan avatar Feb 08 '24 19:02 brianquinlan

No, it modifies body, but content-length remain the same. So it's no longer body.size == content-lenght.

Zekfad avatar Feb 08 '24 20:02 Zekfad

But then shouldn't it be possible to make the test fail? Because content-length == 100 but the body is empty?

Like:

    if (responseHeaders['content-length'] case final contentLengthHeader?) {
     if (!_digitRegex.hasMatch(contentLengthHeader) || int.tryParse(contentLengthHeader) != bodySize)
      throw ClientException(
        'Invalid content-length header [$contentLengthHeader].',
        request.url,
      );
    }

brianquinlan avatar Feb 08 '24 21:02 brianquinlan

That wont work, because this check will fail for almost any normal response. Consider deflate/gzipped response, actual bodySize will always differ with content-length, because body is decompressed by browser, but content-length doesn't reflect this decompression and remained as-is.

By the way, that was my first attempt to just fix 1 failing test, and then it got all red 😅

Zekfad avatar Feb 08 '24 21:02 Zekfad

Got it! Do you have access to the content-encoding header? Maybe we could limit the scope of the check to cases where content-encoding is not set?

brianquinlan avatar Feb 08 '24 21:02 brianquinlan

I can try to check that, but my assumption is that browser could try to guess the encoding if header is not present, but that needs confirmation.

Zekfad avatar Feb 08 '24 21:02 Zekfad

Browsers have net::ERR_CONTENT_LENGTH_MISMATCH, I wasn't wrapping errors when read the buffer.

Also incorporated sophisticated check when we have access to required headers:

https://github.com/Zekfad/fetch_client/blob/fde668ca61c4923ece29991314bf9eeb0ed6264e/lib/src/fetch_client.dart#L173-L202

fetch_client now fully conforms to tests.

Zekfad avatar Apr 15 '24 20:04 Zekfad