yiisoft / yii-web

Yii web components
https://www.yiiframework.com/
BSD 3-Clause "New" or "Revised" License
78 stars 46 forks source link

SapiEmitter: content length from header bug #171

Closed kamarton closed 4 years ago

kamarton commented 4 years ago

What steps will reproduce the problem?

If I set Content-Length: 88 and the body length is 100 then it will send 100 bytes to the output withContent-Lenght: 88 header.

https://github.com/yiisoft/yii-web/blob/e4334574781c20cad6b76026b2651903f5f57b28/src/Emitter/SapiEmitter.php#L46-L55

What is the expected result?

Sending exact data to the output:

Additional info

The emitBody function needs a parameter for the size to be sent.

Q A
Version 3.0.x-dev
PHP version -
Operating system -
samdark commented 4 years ago

I don't think it's a good idea to trim content. If developer is setting content length manually, I'd let him take care of the content as well.

kamarton commented 4 years ago

I don't think it's a good idea to trim content. If developer is setting content length manually, I'd let him take care of the content as well.

Somehow it needs to be resolved because the HTTP client will throw an error if the content length does not match what is specified in the content-length header.

I'll test it with a CLI curl.

samdark commented 4 years ago

How about a browser?

kamarton commented 4 years ago

Chromium browser:

For my nginx, I had to adjust my settings because content-length header was dropped by default.

UPD I checked with the simplest network case. In more complex cases (eg CDN, LB) it is possible for the first proxy to drop the request and respond with a 502 bad gateway to the client. If necessary, I can also test this case.

kamarton commented 4 years ago

I suggests:

roxblnfk commented 4 years ago

I suggests:

  • If $response->getBody()->getSize() is not NULL, always overwrite content-length=body->getsize().
  • In all other cases, ignore what is its value or whether it is set. (content-length) This point may bugs, which is the developer's fault. I recommended that check it after sended body, and if it does not match (content-length != sended body size) thenYii::warning(..)(I don't know what its equivalent in yii3).

Let's think about whether Emitter should change or add headers at all, except for Cookies. Maybe this is Middleware's area of responsibility? Especially if it will emit warnings.


I suggests:

  1. If body is empty then without content-length header
  2. If content-length defined and >0, then emit min('content-length', $response->getBody()->getSize()) bytes
  3. If $response->getBody()->getSize() is not NULL, and content-length is not defined then set content-length: $response->getBody()->getSize() and goto 1.
samdark commented 4 years ago

So behavior we need according to https://tools.ietf.org/html/rfc7230#section-3.3.2:

  1. If there's Transfer-Encoding header, 1xx, 204, 304 response code or when request type is CONNECT (we may ignore this one) - don't send Content-Length.
  2. The client should error when receiving invalid Content-Length according to RFC so either we can try correcting it or we can error with 500. I'm for erroring.
  3. When Content-Length is not specified, send it automatically unless point 1.
samdark commented 4 years ago

If content-length defined and >0, then emit min('content-length', $response->getBody()->getSize()) bytes

If the intent is to correct header value then $response->getBody()->getSize() should be used. But I'd error in this case since something weird is happening and it would be hard to debug if we'll hide it.

samdark commented 4 years ago

Thought more about initial issue:

If I set Content-Length: 88 and the body length is 100 then it will send 100 bytes to the output with Content-Lenght: 88 header.

That's how it should be. We don't know the intent so we can't correct this mistake.

samdark commented 4 years ago

https://github.com/yiisoft/yii-web/issues/179