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

Remove Access-Control-Allow-Origin #2484

Closed
wants to merge 1 commit into from
Closed

Remove Access-Control-Allow-Origin #2484

wants to merge 1 commit into from

Conversation

jvoisin
Copy link
Contributor

@jvoisin jvoisin commented Dec 23, 2020

It doesn't server any purpose, and allows website to
probe if AdGuard Home is currently running on the LAN of the
user by bruteforcing common IP addresses (192.168.1.0/24 and 192.168.0.0/24)
until one of them returns AGH's html.

It doesn't server any purpose, and allows website to
probe if AdGuard Home is currently running on the LAN of the
user by bruteforcing common IP addresses (192.168.1.0/24 and 192.168.0.0/24)
until one of them returns AGH's html.
@codecov-io
Copy link

Codecov Report

Merging #2484 (b1c7ebd) into master (e829e7a) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2484   +/-   ##
=======================================
  Coverage   38.66%   38.66%           
=======================================
  Files          84       84           
  Lines        9471     9470    -1     
=======================================
  Hits         3662     3662           
+ Misses       5351     5350    -1     
  Partials      458      458           
Impacted Files Coverage Δ
internal/home/control.go 0.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e829e7a...b1c7ebd. Read the comment docs.

@ameshkov
Copy link
Member

@ainar-g please check the commits history. I don't remember why we added, but I believe there was a reason for this.

@ainar-g ainar-g self-assigned this Feb 5, 2021
adguard pushed a commit that referenced this pull request Feb 5, 2021
Merge in DNS/adguard-home from 2484-access-control to master

Updates #2484.

Squashed commit of the following:

commit 4f0c6dd
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Fri Feb 5 12:42:22 2021 +0300

    home: don't allow all origins
@ainar-g
Copy link
Contributor

ainar-g commented Feb 5, 2021

Thanks for the contribution! I've analyzed the history of that line and discovered that we do need to set the header in some cases (the main one being the state after the user enables HTTPS redirect) but probably not to *. (And the code also required some refactoring.) See bc01f4f. I will close this PR, but feel free to reopen if you think that something else could be improved there.

@ainar-g ainar-g closed this Feb 5, 2021
heyxkhoa pushed a commit to heyxkhoa/AdGuardHome that referenced this pull request Mar 20, 2023
Merge in DNS/adguard-home from 2484-access-control to master

Updates AdguardTeam#2484.

Squashed commit of the following:

commit 4f0c6dd
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Fri Feb 5 12:42:22 2021 +0300

    home: don't allow all origins
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

4 participants