search

universam1 / iSpindel

electronic Hydrometer
http://www.ispindel.de
Other
824 stars 323 forks source link

Extra New Line results in inconsistency between Content-Length and real Body length #103

Closed vitotai closed 6 years ago

vitotai commented 6 years ago

https://github.com/universam1/iSpindel/blob/4b0c8ecb5bbf4c9badaa8dd93399bf0e0d22146e/pio/lib/Sender/Sender.cpp#L50

This println will put CRLF at the end of body. The really body length will be length of message +2.

ESPAsyncWebServer will reject the request because of this inconsistency. This is the reason latest version of iSpindel won't work with BrewPiLess.

universam1 commented 6 years ago

Thanks for the investigation, that’s a valid point. Should cover #93

universam1 commented 6 years ago

After some research @vitotai I can say that either various implementations are wrong (f.e. the Ubidots library) or the AsyncWebserver is not reacting correct. Still searching for a clear RFC. I'm thinking of switching to higher level implementation to cover this eventually.

vitotai commented 6 years ago

I won't say any implementation is wrong. It's sort of gray area. I haven't checked the RFC, but I think Content-Length is optional. TCP is reliable and Connection: Closed is included. Either accepting all data available or only data of Content-Length is reasonable, and either way would work because CRLF are space characters. If it were me who implemented the server, I would just ignore the last two bytes or ignore the Content-Length. I am not sure why the println is necessary. If it does, adding 2 byte to the Content-Length is another solution.

FYI, I've modified ESPAsyncWebServer to accept the data by changing a "==" to ">=".

universam1 commented 6 years ago

That’s kind from your side and might solve a too strict implementation. I’m bothering to find a one fits all solution as there are different Webserver implementation out there that do require a content length for valid post. Maybe it is even a solution to omit CRLF at the end, I can see implementations like this. This though needs all kinds of testing again...

universam1 commented 6 years ago

Thanks for the feedback! Here is a bugfixed version, using the http library now to perform the post. Please also monitor the execution time in this regard and let me know how it works. firmware.zip

universam1 commented 6 years ago

any news @pronkster @lekrom @j0hnsmith ?

j0hnsmith commented 6 years ago

my iSpindel is in a brew at the moment (a lager so will take a bit longer), will test and report back when it's done.

universam1 commented 6 years ago

how about updates @pronkster @lekrom @j0hnsmith ? no feedback so far

lekrom commented 6 years ago

Have not tested it yet. Sorry. Busy busy busy...

j0hnsmith commented 6 years ago

@universam1 tested against BPL v2.3.3, works 👍, also POST request takes a 'normal' amount of time.

pronkster commented 6 years ago

I've had no time to test, sorry. I'll try this weekend

universam1 commented 6 years ago

Thank you @j0hnsmith that is really helpful so I can proceed with these changes making sure it won’t break anything! 👍

universam1 commented 6 years ago

assume it is fine, so closing for now