Safari issue with 304 responses

[nathanejohnson]

According to the rfc: https://datatracker.ietf.org/doc/html/rfc7232#section-4.1

A 304 response cannot contain a message-body; it is always terminated
by the first empty line after the header fields.

It looks like Mongoose is sending back a body on 304.

And this is the error message I see in Safari:

I was able to reproduce this issue putting together a little test program that would listen on 80 and send back the same response that I pulled out of wireshark, and updating my /etc/hosts file to point to localhost instead of the openevse. If I set Content Length to 0 and removed the "Not Modified" line, Safari no longer gave the parse error.

It looks like the 7.x Mongoose doesn't have this issue, but it's kind of hard to say for sure since it looks like a pretty substantial rewrite. They also seem to do a better job of sending back more of the response headers on 304, like the etag header etc.

This Mac issue was mentioned in #930 but I think these might be separate, unrelated issues at this point.

[nathanejohnson]

it does look like with the latest Mongoose 304 now has an empty body:

https://github.com/cesanta/mongoose/blob/7.17/mongoose.c#L1977

[nathanejohnson]

Also, for what it's worth, it looks like 6.18 mongoose no longer sends a body:

https://github.com/cesanta/mongoose/blob/6.18/mongoose.c#L8372

looks like it was fixed as a side effect of ensuring connections weren't closed on 304 with this commit:

cesanta/mongoose@8fb58eb

@jeremypoulter if you could bump your arduino mongoose wrapper to be up to the latest 6.x, I think this bug will be fixed. I know you've been working on a 7.x update, but I suspect a 6.18 update will a much smaller change.

[jeremypoulter]

Thanks, I have started taking a look at updating Mongoose jeremypoulter/ArduinoMongoose#39 not quite as easy as I would like as I had made a quite a few modifications

[nathanejohnson]

So another possible wrinkle, according to this RFC:

https://datatracker.ietf.org/doc/html/rfc9110#section-8.6

A server MAY send a Content-Length header field in a 304 (Not Modified) response to a conditional GET request (Section 15.4.5); a server MUST NOT send Content-Length in such a response unless its field value equals the decimal number of octets that would have been sent in the content of a 200 (OK) response to the same request.

From my limited testing, Safari doesn't seem to mind a Content-Length of 0 on a 304 response as long as there is no body, but I guess a strict interpretation of the RFC would have the server either omit Content-Length altogether or set it to the size of the original document. I might play with this some more today after work.

[nathanejohnson]

So I updated my test program to be a little more useful, it actually proxies connections so that the websocket connection works but I can play with responses coming back. It looks like Safari completely ignores Content-Length headers on 304, provided the body is not there. Chrome is the same.

[jeremypoulter]

Have a PR (#964) with version 6.18 of Mongoose

[nathanejohnson]

Very nice! Would you like me to try that branch locally? I've never built this firmware but I could probably figure it out from the GitHub workflows

[jeremypoulter]

You can download test binaries from the Workflow runs https://github.com/OpenEVSE/openevse_esp32_firmware/actions/runs/14742273648

[nathanejohnson]

So unfortunately it doesn't seem to have changed the behavior. It's still sending a body with the literal text "Not Modified".

[nathanejohnson]

This sends the 304 here:

https://github.com/OpenEVSE/openevse_esp32_firmware/blob/jeremypoulter/issue963/src/web_server_static.cpp#L65

and this is what writes the body:

https://github.com/jeremypoulter/ArduinoMongoose/blob/v0.0.21/src/MongooseHttpServer.cpp#L432

[nathanejohnson]

I'm running a local build with this small tweak:

jeremypoulter/ArduinoMongoose#40

It's finally working on Safari with 304 responses.

[jeremypoulter]

Thanks I will get that merged and a new version out

[nathanejohnson]

I commented in the PR, but I just wanted to chime in here to say I'm running the latest build and the issue seems to be fixed! Thanks again!