linuxserver / docker-babybuddy

GNU General Public License v3.0
38 stars 12 forks source link

change proxy_set_header Host from $host to $http_host #19

Closed jcgoette closed 2 years ago

jcgoette commented 2 years ago

linuxserver.io



Description:

Change internal nginx.conf to use proxy_set_header Host $http_host; vs proxy_set_header Host $host;.

Benefits of this PR and context:

See https://github.com/babybuddy/babybuddy/issues/446 and https://github.com/jcgoette/baby_buddy_homeassistant/issues/60.

How Has This Been Tested?

Local testing with docker-compose checking ./api/ before and after changes.

However, I trust LSIO expertise over my own. Please let me know if I am missing something.

Source / References:

See https://github.com/babybuddy/babybuddy/issues/446 and https://github.com/jcgoette/baby_buddy_homeassistant/issues/60.

LinuxServer-CI commented 2 years ago

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/babybuddy/v1.10.2-pkg-91145b21-pr-19/index.html https://ci-tests.linuxserver.io/lspipepr/babybuddy/v1.10.2-pkg-91145b21-pr-19/shellcheck-result.xml

jcgoette commented 2 years ago

Checking on feedback for this.

aptalca commented 2 years ago

Thanks for the PR.

I need to look into it and do some testing, but proxy_set_header Host $host; is the proper method. If that doesn't work, it likely suggests wrong headers on the reverse proxy or an issue with the app being proxied.

With that said, I don't believe we have any issues with reverse proxying via SWAG

jcgoette commented 2 years ago

Sounds good. I appreciate you checking the PR.

aptalca commented 2 years ago

@jcgoette I looked into this and while it is a bit of a hack, it is needed in this case. Nothing else is telling babybuddy what port it's being accessed at externally. Other apps get around it by allowing an external url setting or an external port setting. In this case, babybuddy only knows the url and the proto from the headers, and thus leaves out the port. This hack makes the port as part of the url.

Thanks

cdubz commented 2 years ago

@aptalca thanks for reviewing this so thoroughly. Do you know if there anything in particular that could be done differently on the app side so this hack isn't need? Or is it not a big deal?

aptalca commented 2 years ago

As it is, it is not a big deal. When we reverse proxy over https with a proper domain (and port 443), we override that header with $host, so that's proper. Only for local access or using a custom port, this hack gets the job done.

jcgoette commented 2 years ago

@aptalca thanks for the in-depth double check.

jcgoette commented 2 years ago

@aptalca , sorry to bother, but what is CI process to get this commit into Docker image? I pulled @latest and /config/nginx/site-confs/default still shows proxy_set_header Host $host;.

Did I do something wrong?

aptalca commented 2 years ago

That file is user customizable so it doesn't get auto updated. If you delete it and restart the container, you'll get the latest version

jcgoette commented 2 years ago

If I understand correctly, any user with this issue will need to either 1. docker exec, remove this file, and restart container or 2. docker rm and rebuild?

Was there a change I could have made to make this automatic with latest image?

aptalca commented 2 years ago

No need to docker exec. The file is in the config folder that should be mapped to a folder on host. It's under /config/nginx/site-confs/default

jcgoette commented 2 years ago

Doh. Thanks for taking the time to school me. :)