Error in getBodyAsString when content-length is set as a string.

[evert]

From @memmaker on December 25, 2016 22:2

The current release version of SabreDAV bundles a faulty version of sabre-http (4.2.1).

If PHP is set to print out warnings, this release of sabre-http produces warnings in the output if a content-length header has been set, thus rendering the XML output invalid:

stream_get_contents() expects parameter 2 to be integer, string given in /code/vendor/sabre/http/lib/Message.php on line 81

Current versions of sabre-http have fixed this bug by casting the content-length to int.

If we could have the current composer file of SabreDAV pointing to a fixed release, this would not be an issue anymore.

Copied from original issue: fruux/sabre-dav#926

[evert]

odd issue, because I think it should only appear when sabre/http is ran in strict-mode. I'm gonna add some tests to see if I can reproduce this.

[memmaker]

We do actually have declare(strict_types = 1); added to most of our source files (we use PHP 7). But did of course not add this to the SabreDAV classes.

If this helps anything..

[evert]

It shouldn't really make a difference. As far as I know, this error should appear if strict_types is on for that particular class. I noticed I already had a unittest for this issue, but I wasn't testing PHP 7.1 yet. Currently re-running tests to see if this is something new. What version are you running specifically?

[memmaker]

We are running 7.0.10 here.

[evert]

Quick update:

We're getting full marks:

https://travis-ci.org/fruux/sabre-http/builds/187112146

The actual tests that tackles your case is here:

https://github.com/fruux/sabre-http/blob/4.2/tests/HTTP/MessageTest.php#L56

Can you tell me:

• Is there anything else unique about your installation?
• Can you figure out what the actual value is of the Content-Length header in your case?
• Could you otherwise give me a super-simple test-case that I can use to reproduce the error?

It's not hard to add the (int) but I'd like to find out what's actually going on before doing so.

[memmaker]

But nope. As I said before. You guys have already fixed this in your current Sabre HTTP repository. But the version you have bundled and referenced in composer is 4.2.1 which still has this bug.

Hmm... This is hard to reproduce for me since we changed the code around since the error came up and I am not sure if it currently pops up.
If I encounter it again I will start up a xdebug session and look at it more thoroughly.

Our Installation is pretty unique.. Yes.. First thing is we re-did all the storage backends to make them compatible with our mongodb data storage.. I mean of course we extended from your nicely arranged storage backend classes and implemented your very clean written interfaces.. Thanks..;)

Well I could figure out from your source, which checks for existence iirc, that the header has to be set for this to occur. Also we dialed up all our error reporting for dev purposes.

Another pointer: I am pretty sure it was one of the following two CURL requests which actually created the warning I mentioned above.

LIST:
curl --user "felix:felix" -X PROPFIND https://lrx.cyrene.io/dav/root/calendars/felix/default/

OPTIONS:
curl --user "felix:felix" -X OPTIONS https://lrx.cyrene.io/dav/

Getting back to you when I know more.

[evert]

My worry is, is if I just add the (int) to straight up silence to the problem I'm actually obscuring some hidden real issue. So more info and a test-case are definitely appreciated. Hope you find it and happy holidays!

[memmaker]

Oh, well.. I definitely agree to your modus operandi. But really, iirc, it was just a type casting issue. You access the header value which is by definition a string, but then pass it to a function which really expects an integer type for the length. Since content-length inherently really is an int type just casting it would be the correct fix for this problem. And you or your team seem to agree, since you already added that int cast. It is already in your Github code base. So are we discussing if you or your team did right with that fix in the first place?

[evert]

Hi @memmaker , the thing is: PHP will actually always just do the casting if strict_types is off. The reason the cast is added to the master branch, is because sabre/http 5.0 will actually start using strict_types so it made total sense.

If you can find a way to prove that there's an error possible, I'll happily make the change and do the release as early as possible.

[memmaker]

Ahh.. I see. There was information missing from my point of view. Of course, I will look into it.

[petrkotek]

Looks like some clients (e.g. BestSync, WinSCP, SmartFTP, Transmit ... and many more -- see screenshot below) send Content-Length: header (i.e. with empty string as a value) for PROPFIND and MKCOL.

HTTP/1.1 specs says:

Any Content-Length greater than or equal to zero is a valid value.

... so since an empty string is an invalid value, I believe it shouldn't be interpreted as 0 (which is current behaviour -- see https://github.com/fruux/sabre-http/blob/9f1d22de4797ab4e904f445c76af080e45c62988/lib/Message.php#L87)

(Good to mention that most likely the interpretation of an empty string as zero doesn't cause any issues as clients would probably send correct value of there was some request body).

Btw, it's easy to reproduce this using curl:

curl -X PROPFIND -H 'Content-Length:' --digest --user 'user:password' http://<HOSTNAME>/dav

---

List of User Agents sending empty string as Content-Length header value:
.

[evert]

Confirmed. This is likely exactly why @memmaker ran into this. I'm updating the behavior to ignore non-numeric Content-Length headers.

[petrkotek]

Thanks for the fix, that's fantastic!

[memmaker]

Indeed. Thank you very much.