Support for M3U with reverse proxy and https

[costan1974]

When downloading M3U files, if the reverse proxy exposes https, the generated URL is exporting the wrong protocol. This change checks for x-forwarded-proto in the original request and creates the M3U list with the proxy reported protocol.

To allow for this to work with NGINX it sufficent to add the following lines:

            proxy_set_header Host $http_host;
            proxy_set_header X-Forwarded-Proto https;

The first line let peerflix know the original exposed FQDN:PORT name the client used to reach the proxy, the second header let peerflix know that request was https and not http.

[asapach]

Just curious, does the host actually work or should we check for X-Forwarded-Host as well?

[asapach]

I've checked out the docs: https://expressjs.com/en/guide/behind-proxies.html It would be better to set trust proxy option in this case, Maybe we should introduce some kind of CLI option or env variable that would enable this?

[costan1974]

Just curious, does the host actually work or should we check for X-Forwarded-Host as well?

Host works since the proxy is configured to pass it unchanged:

    proxy_set_header Host $http_host;

[costan1974]

I've checked out the docs: https://expressjs.com/en/guide/behind-proxies.html It would be better to set trust proxy option in this case, Maybe we should introduce some kind of CLI option or env variable that would enable this?

This would be ok as well as my patch, but it requires, in case of NGINX reverse proxy, to add the following:

    proxy_set_header X-Forwarded-Host $http_host;

[asapach]

I'd rather use req.protocol and req.hostname to keep the code simple. You'll have to pass host one way or another: either as Host header or as X-Forwarded-Host. I think both should be supported, i.e. if X-Forwarded-Host is not supplied Host will be used.

[costan1974]

I'd rather use req.protocol and req.hostname to keep the code simple. You'll have to pass host one way or another: either as Host header or as X-Forwarded-Host. I think both should be supported, i.e. if X-Forwarded-Host is not supplied Host will be used.

If it fails back transparently I think it should be ok.

I did not know about the "trust proxy" option, but it should be the right way to go.

[asapach]

Could you please try it out and see if it works for you?

[costan1974]

Could you please try it out and see if it works for you?

I'm not sure how to set the "trust proxy" option. Do you have a quick solution for your original code for me to check?

[asapach]

Try this: https://github.com/asapach/peerflix-server/compare/proxy

[costan1974]

It seems to work.

I just added the config

proxy_set_header X-Forwarded-Host $http_host;

Just to make sure the X-Forwarded-Host was correctly set and it worked flawlessly as per my patch.

If you just put a switch on CLI or an ENV var to set it up, it will work with your code (besides of the change req.get('host') to req.host).

The ENV solution is better suited also for the docker case (it just requires to set the var on docker run), that is the one I'm using.

[costan1974]

Ops..

wait.. I did a mistake. It's not working. I just changed bin.js and not the actual index.js.

Your code is not taking into account the port in the Host or X-Forwarded-Host tag.

Let me check a little more.

[costan1974]

It seems that:

req.get('host') will get you the header Host with the full FQDN:PORT syntax, while req.host will get either the X-Forwarded-Host or Host header, depending on trust proxy settings, but without the port#.

So we're back to square one..

At this point I think that my patch is easier to deal with.

[asapach]

OK, then I would change it to:

var proto = req.get('X-Forwarded-Proto') || req.protocol;
var host = req.get('X-Forwarded-Host') || req.get('host');
...

[costan1974]

Correct, but please make sure to use all lowecase chars in X-Forwarded-*, since node.js treats all headers as lowercase.

My first mistake was exactly using some uppercase letters. After debug it was clear you need to lower all cases.

[asapach]

That's strange, because they are using it like this: https://github.com/expressjs/express/blob/cb59086305367d9fcd7d63b53cfca1a3e4ef77d7/lib/request.js#L367 https://github.com/expressjs/express/blob/cb59086305367d9fcd7d63b53cfca1a3e4ef77d7/lib/request.js#L492

[costan1974]

I don't know about the expressjs code, but my trial-and-error patch used the partially uppercase header name and it did not work.

I used debug to dump the requests and the header names in the json logs were lowercase. I then switched to lowercase the code and it worked.

[asapach]

They are converting it to lower-case inside the get() method: https://github.com/expressjs/express/blob/cb59086305367d9fcd7d63b53cfca1a3e4ef77d7/lib/request.js#L52, so I think it should work regardless of the case.

[asapach]

Would you like to update the PR so that I could merge it, or would you prefer me to make the change myself?

[asapach]

Thanks!

[costan1974]

Thanks to you. Nice job!

As soon as docker image is ready I'll upgrade.