me-no-dev / ESPAsyncWebServer

Async Web Server for ESP8266 and ESP32
3.74k stars 1.22k forks source link

Safari can't open the page. The error was: "cannot parse response". #1438

Open sourcesimian opened 1 week ago

sourcesimian commented 1 week ago

On iOS 18.1 Safari returns Safari can't open the page. The error was: "cannot parse response". for chunked responses.

This appears to be caused by https://github.com/me-no-dev/ESPAsyncWebServer/blob/7f3753454b1f176c4b6d6bcd1587a135d95ca63c/src/WebResponses.cpp#L320-L321

which can introduce trailing whitespace padding for chunk sizes less than 0x1000, which RFC 2616 (Section 3.6.1) seems to say that no extra whitespace is allowed after the chunk size other than the CRLF.

Reproducer (open: http://127.0.0.1:8088) demo-bad.http.txt

while true; do nc -l 8088 < ./demo-bad.http.txt; done

Working: demo-good.http.txt

while true; do nc -l 8088 < ./demo-good.http.txt; done
UnexpectedMaker commented 4 days ago

Yeah, I was just about to report this - IOS18.x won't load ANY website now that uses this web server. What a PITA :(

Did you try removing line 321 and did it work?

The issue I see here is that serving pages without full chunks can be slow or stall, so removing the whitespace could add issues in browsers that currently work ok.

sourcesimian commented 3 days ago

I didn't recompile without line 321, however that is the effective delta of demo-bad.http.txt and demo-good.http.txt above. Both bad and good work fine on Chrome (128.0.6613.139), Firefox (130.0) and Safari (17.5 - 19618.2.12.11.6). It is just Safari 18.1 that has an issue with the trailing whitespaces (demo-bad.txt).

The comment on line 313 which says HTTP 1.1 allows leading zeros in chunk length. Or spaces may be added. is part of the same changeset that introduced the tailing whitespace. But I what I see in RFC2616 or RFC7230 disagrees. Worse, this code is 7+ years old.

From my read of RFC7230 4.1 Safari 18.1 is strictly correct: trailing whitespace after the chunk size is not permissible. Chunk size may be followed by an extension, but that requires a ';' to immediately follow the size. So this does seems like a pretty clear cut fix, however I accept that there may be more to this than I am seeing.

UnexpectedMaker commented 3 days ago

I can confirm removing the spaces is a BAD IDEA - It does make it work in Safari 18, but it causes random chunk issues and missing or corrupted data in the browser, as I expected :(

@me-no-dev I know you are not working on this anymore - but any idea what we can do about this?

It means any ALL ESPAsyncWebServer projects serving to Apple devices using their latest OS's won't work anymore.

me-no-dev commented 3 days ago

@UnexpectedMaker what if you replace the space with \r or \n if those are permitted?

me-no-dev commented 3 days ago

this line: https://github.com/me-no-dev/ESPAsyncWebServer/blob/7f3753454b1f176c4b6d6bcd1587a135d95ca63c/src/WebResponses.cpp#L321

UnexpectedMaker commented 3 days ago

@UnexpectedMaker what if you replace the space with \r or \n if those are permitted?

Thanks for the quick response :) I've tried a few things, but not that .. I'll try it tomorrow and report back. Cheers!

UnexpectedMaker commented 2 days ago

@me-no-dev Unfortunately, any combination of using those makes it worse as it doesn't fix Safari 18, but also breaks other browsers :(

sourcesimian commented 2 days ago

Perhaps add leading zeros: sprintf(..., "%04x", ...) the comment on line 313 suggests it?

UnexpectedMaker commented 2 days ago

But we don't know how many we need until after we calc outLen. We might add extra that takes us over the chunk size and then have a new chunk.

So we need to pre-calc what outLen will be before we sprintf?

sourcesimian commented 2 days ago

I agree that the exiting code is broken for readLen > 0xFFFF, but perhaps its a separate less urgent issue since it has been there for ~7 years.

FYI: I've tried compiling and running both removing line 321 and also the %04x. The first breaks as you said, the second works in my use case.

me-no-dev commented 2 days ago

code is broken for readLen > 0xFFFF

readLen will never be that long

electrokean commented 2 days ago

If the chunks are never 64k or more, then just changing it to use a format of %04x seems like it should work. There is nothing I see in RFC7230 or RFC5234 that disallows this. An alternative would be to change line 321 to check the size and re-print the number with sufficient leading zeros using a precision parameter over the previous sprintfoutput (ugly). I can explain better if needed, but the format syntax would be "%0*x", and the parameter before the value to be printed is the precision (i.e. total number of digits). Even better would be to calculate the correct headLenin the first place, but I didn't read through the code to see how that logic works.

sourcesimian commented 1 day ago

readLen will never be that long

Thanks for confirming @me-no-dev, makes sense since this library targeted at embedded systems (BTW it is an awesome library, thank you!!!!)

I'm using %04x in a fork, it appears to work well.