Closed ishanjain28 closed 2 years ago
ishanjain28
commented
2 years ago Video demonstration. Re-opening the client overrides page, Clicking on unblock all button and then saving changes fixes the problem.
https://user-images.githubusercontent.com/7921368/193468277-c9fc8c60-7590-48f6-a754-47576d4a911d.mp4
ishanjain28
commented
2 years ago /add request contains,
{
"ids": [
"127.0.0.0/8"
],
"tags": [
],
"use_global_settings": true,
"use_global_blocked_services": false,
"name": "test2",
"upstreams": [
]
}Clicking on unblock all and updating client sends a request to /update which contains,
{
"name": "test2",
"data": {
"upstreams": [
],
"tags": [
],
"name": "test2",
"blocked_services": [
],
"ids": [
"127.0.0.0/8"
],
"filtering_enabled": false,
"parental_enabled": false,
"safebrowsing_enabled": false,
"safesearch_enabled": false,
"use_global_blocked_services": false,
"use_global_settings": true
}
}Looks like it's just missing a check for the global blocked services somewhere.
ishanjain28
commented
2 years ago Here is the problem,
This adds client specific blocked services but does not remove the inherited global blocked list.
2022/10/02 23:53:28.570807 45142#57 [info] []string{"9gag", "amazon", "bilibili", "cloudflare", "dailymotion", "discord", "disneyplus", "ebay", "epic_games", "facebook", "hulu", "imgur", "instagram", "mail_ru", "netflix", "ok", "origin", "pinterest", "qq", "reddit", "skype", "snapchat", "spotify", "steam", "telegram", "tiktok", "tinder", "twitch", "twitter", "viber", "vimeo", "vk", "wechat", "weibo", "whatsapp", "youtube"}This is the value of Context.filters.BlockedServices when client is configured to not follow global settings.
EugeneOne1
commented
2 years ago @ishanjain28, hello and thanks for the thorough report and for the contribution, but we've already fixed it ourselves in the latest edge build. Could you please check it out?
ishanjain28
commented
2 years ago hey @EugeneOne1
I can replicate this bug in the build here, https://github.com/AdguardTeam/AdGuardHome/releases/tag/v0.108.0-b.16 and I was able to replicate it in a build with the latest commit in the main branch. https://github.com/AdguardTeam/AdGuardHome/commit/2ffea605cffd38604f46338857c772d44494798f
EugeneOne1
commented
2 years ago @ishanjain28, that's rather odd. We aren't being able to reproduce it with the latest commit on several test machines. Are you sure the client's settings haven't changed since the last report, the use_global_blocked_services field in particular? If so, could you please collect a verbose log for us? You may send it to devteam@adguard.com.
ishanjain28
commented
2 years ago @EugeneOne1
I can consistently replicate this on a linux x64 machine and some rpi 3b+.
I have included verbose logs and the config file in this.
ishanjain28
commented
2 years ago In the PR, I have linked to the problematic line. A call to Context.filters.ApplyBlockedServices with nil parameter will add globally blocked services to the list of filters.
This call is made without checking if the client has opted to ignore global blocklists here. https://github.com/AdguardTeam/AdGuardHome/blob/2ffea605cffd38604f46338857c772d44494798f/internal/home/dns.go#L338
EugeneOne1
commented
2 years ago @ishanjain28, firstly, it's quite unsecure to post unredacted configuration file of AdGuard Home to the public space, I'd recommend that you at least change your password.
There are no issues with applying services' rules according to the log. It actually says that the client with address 127.0.0.1 couldn't be found, which is possibly a separate bug. So it just applies the global blocked services for unidentified client, which is an expected bahavior.
By the way, to give a feeedback on your PR, I'd say it introduces a bug, which makes AGH to not apply global services to the non-persistent clients' requests. The logic there is a bit convoluted and really requires attention to all the other parts.
As far as I can see, you've built the AdGuard Home yourself. Just to ensure, haven't you change any code in comparison with master branch before building? Could please also try it on our signed builds? Those may either be installed with the README.md script:
curl -s -S -L https://raw.githubusercontent.com/AdguardTeam/AdGuardHome/master/scripts/install.sh | sh -s -- -c edge -vor downloaded directly from:
https://static.adtidy.org/adguardhome/edge/AdGuardHome_linux_amd64.tar.gz(Replace the linux and amd64 parts with the corresponding values of yours)
If it reproduces there? Thanks.
ishanjain28
commented
2 years ago Hey @EugeneOne1 Noted the comment on publishing unredacted logs.
You are right in that for the first few times, it says it couldn't find 127.0.0.1 in the clients list. This was because I had not added the client when I sent request the first few times.
I had also recorded a video of the whole process along with the logs. I have shared the logs above and the video is here, https://www.youtube.com/watch?v=48BUwk2ABDo
(I couldn't upload it here because of size constraints and the video was still processing when I posted the logs)
By the way, to give a feeedback on your PR, I'd say it introduces a bug, which makes AGH to not apply global services to the non-persistent clients' requests. The logic there is a bit convoluted and really requires attention to all the other parts.
I have tested this in my PR by removing the 127.0.0.0/8 client and,
Lastly, This was on the most recent commit in the branch. I did not have any un-stashed changes when building the project.
EugeneOne1
commented
2 years ago @ishanjain28, well, after digging further, I can confirm the bug. It seems editing the client via UI (at runtime) sets inproper blocked services slice. You may have noticed we've merged your PR, but also modified it a bit. The latest edge build (as per HEAD) should now work as intended, could you please check it again?
UPD: the latest release version contains the fix as well.
ishanjain28
commented
2 years ago Hey, It works perfectly for me now. Thank you!
Prerequisites
[X] I have checked the Wiki and Discussions and found no answer
[X] I have searched other issues and found no duplicates
[X] I want to report a bug and not ask a question
Operating system type
Linux, Other (please mention the version in the description)
CPU architecture
AMD64
Installation
GitHub releases or script from README
Setup
On a router, DHCP is handled by the router
AdGuard Home version
latest beta release(v0.108.0-b.16 Pre-release) and HEAD(2ffea605cffd38604f46338857c772d44494798f)
Description
What did you do?
Expected result
should return the correct IP for cloudflare.com. The client in this case is localhost which comes under the client I added initially and for this client, No services should be blocked.
Actual result
It blocks DNS for all globally blocked services and does not respect client specific overrides.
Screenshots (if applicable)
Additional information
This is happen on the most recent commit and the most recent release. It does not happen in,
v0.107.14