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

nginx_proxy: Fix issue #3132 #3356

Merged
merged 1 commit into from
Dec 14, 2023
Merged

nginx_proxy: Fix issue #3132 #3356

merged 1 commit into from
Dec 14, 2023

Conversation

mancontr
Copy link
Contributor

Issue #3132, affecting nginx_proxy, has been causing other addons to break for a long time. For example, part of the esphome addon is not usable through the nginx_proxy because of this bug. All the details are in the issue.

The fix is very simple: replace $host with $http_host, so the port gets included and the socket connection is not detected as cross-origin.

I've had this fix applied on a custom config since the issue was opened, without any problems, but I think it would be nice to fix it here for everyone.

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mancontr

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@home-assistant home-assistant bot marked this pull request as draft December 13, 2023 21:41
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@mancontr mancontr marked this pull request as ready for review December 13, 2023 21:44
@agners
Copy link
Member

agners commented Dec 14, 2023

Nice catch!

Without understanding too much here, from this StackOverflow thread it seems that another difference is that $http_host is from the client request. Can it be trusted? 🤔

@agners
Copy link
Member

agners commented Dec 14, 2023

Please also bump the version in config.yaml and add your change to CHANGELOG.md.

@mancontr
Copy link
Contributor Author

I updated the changelog and bumped the version, following the example from the v3.4.0 release, which was somewhat similar.

Regarding the safety considerations of the change: both $host and $http_host use values provided by the client, so there isn't much of a change (other than adding the port). Of course neither value can be trusted, but we're merely forwarding them to the addon, which can receive them directly from a client when not using the proxy, so AFAIK we shouldn't be creating any new problems by doing this.

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

Successfully merging this pull request may close these issues.

2 participants