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

Adding CORS support via environment variable #1822

Merged
merged 2 commits into from Jul 4, 2021
Merged

Adding CORS support via environment variable #1822

merged 2 commits into from Jul 4, 2021

Conversation

adyanth
Copy link
Contributor

@adyanth adyanth commented Jun 7, 2021

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)
  • I have Signed Off all commits. (git commit --signoff)

What does this PR aim to accomplish?:

This adds support for enabling CORS on per host/domain basis
Fixes #1820

How does this PR accomplish the above?:

This uses an additional environment variable (introduced in pi-hole/docker-pi-hole#864 and pi-hole/docker-pi-hole#867) to get this list of domains where CORS can be enabled.

What documentation changes (if any) are needed to support this PR?:

Need to document the presence and usage of the new environment variable CORS_HOSTS.

Ex: CORS_HOSTS=test.domain.com,example.com.

  • {Please delete this quoted section when opening your pull request}
  • You must follow the template instructions. Failure to do so will result in your issue being closed.
  • Please respect that Pi-hole is developed by volunteers, who can only reply in their spare time.
  • Detail helps us understand an issue quicker, but please ensure it's relevant.

Signed-off-by: Adyanth H <33192449+adyanth@users.noreply.github.com>
@adyanth
Copy link
Contributor Author

adyanth commented Jun 15, 2021

Bump, Can someone review this?

Copy link
Member

@PromoFaux PromoFaux left a comment

Choose a reason for hiding this comment

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

I'm unsure how I feel about this idea overall.

What is stopping you from using http://pi.hole instead of your own custom domain name?

scripts/pi-hole/php/auth.php Outdated Show resolved Hide resolved
@adyanth
Copy link
Contributor Author

adyanth commented Jun 16, 2021

I have two of them in HA, which is why I cannot use pi.hole. And I'm using https with a certificate for my lan domain along with my actual domain, both of which can be used to access it (from the internet, protected by SASE).

If we are using nginx, we can change the nginx conf bypass CORS check just by using the config file as below, which is what I use on my other pihole instance on raspberry pi. But sadly that cannot be done on the docker image which uses lighttpd.

        # Enable PHP
        location ~ \.php$ {
               include fastcgi_params;
               # Bypass CORS Check
               fastcgi_param HTTP_ORIGIN "";

               fastcgi_param SCRIPT_FILENAME $document_root/$fastcgi_script_name;
               fastcgi_pass unix:/run/php/php7.3-fpm.sock;
               fastcgi_param FQDN true;
        }

This may be a vulnerability too? Where an empty origin is always successful in auth.php.

Signed-off-by: Adyanth H <33192449+adyanth@users.noreply.github.com>
@PromoFaux
Copy link
Member

This may be a vulnerability too? Where an empty origin is always successful in auth.php.

Are you saying that this PR could introduce a vuln?

@adyanth
Copy link
Contributor Author

adyanth commented Jul 4, 2021

This may be a vulnerability too? Where an empty origin is always successful in auth.php.

Are you saying that this PR could introduce a vuln?

Hey @PromoFaux , no, that was referring what I am already using with the traditional install + nginx. That was how I was working around for the CORS issue by setting the http origin empty in the PHP environment. Maybe that is a trusted operation, and another discussion altogether.

This PR adds support to set accepted names for the Host header to support FQDN other than pi.hole, tested for the docker install with lighttpd.

@PromoFaux
Copy link
Member

One last check, to make sure I'm not entirely missing something here...

Isn't this what the VIRTUAL_HOST env var is used for over on docker?

@PromoFaux
Copy link
Member

No, scratch that. I see now.

@PromoFaux
Copy link
Member

@DL6ER, Something to keep in mind for API / V6 (description of issue here: bastienwirtz/homer#194)

@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-ftl-v5-9-web-v5-6-and-core-v5-4-released/49544/1

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

Successfully merging this pull request may close these issues.

None yet

3 participants