Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggested patch in order to solve problem of Safari and HTTP/2 error #16

Open
ubrmensch opened this issue Jul 16, 2020 · 17 comments
Open

Comments

@ubrmensch
Copy link

Hello,

We have been using this library as part of our project in conjunction with phpwkhtmltopdf library.

We recently migrated to a new server, and suddenly the PDF download stopped working over https. After a lot of digging, we realized that the old server was serving https over HTTP/1.0. The new server was serving https over HTTP/2.

As it turns out, the Content-Length header ALSO causes network issues in the latest Safari (which now supports HTTP/2).

We patched the File.php line were the bug is mentioned from this:

    // #84: Content-Length leads to "network connection was lost" on iOS
    $isIOS = preg_match('/i(phone|pad|pod|safari)/i', $_SERVER['HTTP_USER_AGENT']);

to this:

    // #84: Content-Length leads to "network connection was lost" on iOS AND latest Safari.
    $isIOS = preg_match('/(phone|pad|pod|safari)/i', $_SERVER['HTTP_USER_AGENT']);

We removed the first "i" after the preg_match opening bracket because it seemed like it was actually causing the expression to never match. Not sure if that's something that has been dropped in PHP or not. However, once we that simple change, the php downloads worked fine on Safari over https / HTTP/2. From my understanding, all modern servers will only server HTTP/2 over HTTPS so this should not be a problem on regular HTTP/1.0, which is probably the case for most people.

@mikehaertl
Copy link
Owner

Hmm, it seems you already use some patched version. We don't have any safari in the regular expression:

https://github.com/mikehaertl/php-tmpfile/blob/1.1.4/src/File.php#L85

Regarding the issue with HTTP/2 I have to do some research if this can be fixed somehow.

Related: mikehaertl/php-pdftk#84

@mikehaertl
Copy link
Owner

mikehaertl commented Jul 17, 2020

This is really a tough problem: I can not think of a way to reliably detect HTTP/2 requests from PHP.

While there is $_SERVER['SERVER_PROTOCOL'] this is not reliable, because it's a very common scenario that your app sits behind a reverse proxy e.g. Nginx. The value there only reports the backend upstream protocol. Nginx does not support HTTP/2 for upstream requests. So even if your client connection uses HTTP/2 you still would get HTTP/1.0 as SERVER_PROTOCOL.

And even a configuration flag would not help, because if you disable Content-Length completely you would get a problem with all requests from HTTP/1.x.

If someone has an idea let me know. For now I can't think of any.

@mikehaertl
Copy link
Owner

One proper solution would probably be this:

  • We leave away the Content-Length header if SERVER_PROTOCOL is HTTP/2
  • In the other case where a proxy connects with HTTP/1.x but uses HTTP/2 to the client, the proxy is responsible to remove/fix the Content-Length header.

But this would require a smart proxy. It can probably be done with Nginx - but still it's not a very convenient solution.

@ubrmensch
Copy link
Author

ubrmensch commented Jul 17, 2020 via email

@mikehaertl
Copy link
Owner

@ubrmensch The i there is ok, because we want to test for iphone, ipod, ipad. The correct expression should then rather be /(iphone|ipad|ipod|safari)/i. But this makes me wonder if this has changed. If you check the issue linked above, the original report was from a safari user, too. And we fixed it by checking for iphone, ipad, ...

also, just to be clear, this is only a Safari issue. All other browsers we tested safely ignore Content-Length over HTTP/2.

Hmm, I'm not sure. If you check the other issue the last commenter seems to confirm that HTTP/2 is affected in general. I still think, this needs a better fix.

@ubrmensch
Copy link
Author

ubrmensch commented Jul 17, 2020 via email

@mikehaertl
Copy link
Owner

It is not likely that they previous thread was related to Safari because Safari only recently started to support HTTP/2,

Have you checked the screenshots on top of the other issue? It shows "Safari". Oh, and also notice that this is on mobile. Maybe you talk about safari on desktop? As explained before iphone, ipad and ipod are the strings that we want to test for in the other issue - and this worked, so the "i" is correct there.

I will consider adding a check for HTTP/2.

@ubrmensch
Copy link
Author

ubrmensch commented Jul 17, 2020 via email

@ubrmensch
Copy link
Author

ubrmensch commented Jul 17, 2020 via email

@mikehaertl
Copy link
Owner

One more question: Do you use mod_deflate in Apache? Because this would definitely break Content-Length. If so, you could try to disable mod_deflate for the download URL.

Apart from that I still see no good solution. We can not just disable Content-Length for Safari, because this would turn off the progress bar when HTTP/1.1 is used.

@ubrmensch
Copy link
Author

ubrmensch commented Jul 17, 2020 via email

@mikehaertl
Copy link
Owner

It's not PHP specific. Just think about a setup like this:

[Client/User Browser] ----(HTTP/2)---->  [Proxy (e.g. Loadbalancer, Nginx, ...)] ----(HTTP/1.1)---> [App (e.g. docker container)]

So a client connects to your website with HTTP/2. A proxy relays the request to your app (which could even run on the same host) and uses HTTP/1.1 for that upstream request. This is a very common setup for example if your app runs in a docker container. Nginx can not use HTTP/2 for the backend connection.

So your app has no idea what protocol the client really uses - it only "sees" HTTP/1.1 while the client still "sees" HTTP/2. That's why $_SERVER['SERVER_PROTOCOL'] can not be trusted here.

About patreon: I don't have an account, but thanks for the offer. I'm already happy if people find my code useful.

@ubrmensch
Copy link
Author

ubrmensch commented Jul 17, 2020 via email

@mikehaertl
Copy link
Owner

Sorry, we're getting ab it off topic and your scenario is not what I meant. The problem is really with a proxy setup and I suggest you do some reasearch if that's unclear, e.g. here: https://en.wikipedia.org/wiki/Reverse_proxy.

IMO it has not really to do with HTTPS but rather with HTTP/2 and certain browsers. And I'm still curious if you use mod_deflate with apache or the gzip module in Nginx.

@ubrmensch
Copy link
Author

ubrmensch commented Jul 18, 2020 via email

@mikehaertl
Copy link
Owner

You can use this command to test:

curl -I -H 'Accept-Encoding: gzip,deflate' http://yoursite.com/somefile

(from https://serverfault.com/a/334910/101576).

Please use the URL to the PHP script where you send the download. If you see content-encoding: gzip then your server sends compressed data. In that case you can try to exclude that URL from mod_deflate in the Apache config.

@ubrmensch
Copy link
Author

ubrmensch commented Jul 18, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants