Add support for blocked replies with NOERROR code and empty answer - #8245
Add support for blocked replies with NOERROR code and empty answer#8245agross wants to merge 1 commit into
Conversation
| filtering.BlockingModeDefault, | ||
| filtering.BlockingModeNXDOMAIN, | ||
| filtering.BlockingModeREFUSED, | ||
| filtering.BlockingModeNOERROR, |
There was a problem hiding this comment.
The PR adds filtering.BlockingModeNOERROR to the list of valid blocking modes, but I don't see a corresponding implementation in the filtering package. Make sure this mode is properly implemented in the filtering package to handle blocked domains with NOERROR responses without answer sections.
There was a problem hiding this comment.
This is a new approach to responding to filtered queries, not a new method for filtering.
4deb43d to
f53fdba
Compare
8793a81 to
524f303
Compare
3d04800 to
2576ed7
Compare
63d944c to
3c7d59d
Compare
791c59e to
d303033
Compare
a6a6155 to
d1cec80
Compare
de66c70 to
7098c29
Compare
c3dce85 to
55e2fa5
Compare
a03dc9d to
e584b53
Compare
|
5 months later, and not a single reply from the team. Is this of no interest, @KirillCAdGuard, @kpokas-adguard, @AleksandrKalininAdGuard? |
e584b53 to
fb1cc0a
Compare
1afb685 to
b372707
Compare
4d25df9 to
3bf55c9
Compare
Sil3ntVip3r
left a comment
There was a problem hiding this comment.
I found one confirmed integration omission and one material regression gap on current head 3bf55c966b07fc358e888bd33803be1dd59df903:
- The runtime now accepts and returns
blocking_mode: "noerror", but the blocking-mode enums inopenapi/openapi.yamlandopenapi/next.yamlstill omit it. Consequently, schema-generated clients cannot represent the new value, andclient_v2's generated type, constants, summary, and options do not expose it. Please update the API schemas/documentation and the generated/current-UI integration along with the legacy UI. - No backend test on this head exercises the new mode through configuration or DNS response construction. The smallest regression coverage would round-trip
noerrorthrough the DNS config set/get path, then assert that blockedA,AAAA, andHTTPSqueries returnRcodeSuccess, an empty Answer section, and exactly the expected SOA in Authority (including the configured blocked-response TTL).
I do not see a demonstrated DNS response bug in the current implementation; this is an API/UI integration defect plus unprotected externally visible behavior.
|
@Sil3ntVip3r Thanks for your review and token spend. |
Sil3ntVip3r
left a comment
There was a problem hiding this comment.
I re-reviewed exact head
656965535fc2bb249052de4e59d47ada7f9579af. The functional concerns from my
previous review are resolved: both OpenAPI schemas and UIs expose noerror, the
configuration test round-trips it, and backend regressions cover blocked A,
AAAA, and HTTPS responses with NOERROR, an empty Answer section, one SOA,
and the configured TTL.
One small documentation update remains before this public behavior is ready:
CHANGELOG.mddoes not mention the new user-facing blocking mode under
Unreleased.client_v2/AGENTS.mdrequires the root changelog to be updated
for user-facing changes.openapi/CHANGELOG.mddoes not record thatDNSConfig.blocking_modenow
accepts"noerror".AGHTechDoc.mdstill lists only the old blocking-mode values in both request
examples and the mode description.
The smallest fix is an Added entry referencing #8024, one v0.107.79 API
changelog bullet, and noerror plus its empty-answer behavior in the two
technical-document examples/description. Please run make md-lint and
make txt-lint after those edits; no additional production test is needed for
this documentation-only change.
Independent validation on this clean exact head:
PASS: focused NOERROR/configuration backend tests under -race
PASS: go test -race -count=1 ./internal/dnsforward
PASS: make go-check
PASS: focused current-UI helper test, ESLint, and TypeScript typecheck
PASS: full current-UI suite (59 files / 620 tests)
PASS: fresh API generation matched the committed blocking-mode type
PASS: git diff --check
Once the three documentation locations are updated, I expect this to be ready
for approval.
|
I prepared the remaining documentation-only patch against exact head diff --git a/AGHTechDoc.md b/AGHTechDoc.md
--- a/AGHTechDoc.md
+++ b/AGHTechDoc.md
@@ -1077,7 +1077,7 @@ Response:
"protection_enabled": true | false,
"ratelimit": 1234,
- "blocking_mode": "default" | "refused" | "nxdomain" | "null_ip" | "custom_ip",
+ "blocking_mode": "default" | "refused" | "nxdomain" | "null_ip" | "custom_ip" | "noerror",
"blocking_ipv4": "1.2.3.4",
@@ -1103,7 +1103,7 @@ Request:
"protection_enabled": true | false,
"ratelimit": 1234,
- "blocking_mode": "default" | "refused" | "nxdomain" | "null_ip" | "custom_ip",
+ "blocking_mode": "default" | "refused" | "nxdomain" | "null_ip" | "custom_ip" | "noerror",
"blocking_ipv4": "1.2.3.4",
@@ -1124,6 +1124,7 @@ Response:
* NXDOMAIN: Respond with NXDOMAIN code
* Null IP: Respond with zero IP address (0.0.0.0 for A; :: for AAAA)
* Custom IP: Respond with a manually set IP address
+* NOERROR: Respond with NOERROR code and an empty answer
diff --git a/CHANGELOG.md b/CHANGELOG.md
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -26,6 +26,9 @@ NOTE: Add new changes BELOW THIS COMMENT.
### Added
+- The `NOERROR` blocking mode, which returns an empty answer for blocked
+ requests ([#8024]).
+
- Bootstrap servers configuration now supports comments.
@@ -44,6 +47,7 @@ NOTE: Add new changes BELOW THIS COMMENT.
- Blocked requests without an EDNS(0) OPT record ([#8183]).
+[#8024]: https://github.com/AdguardTeam/AdGuardHome/issues/8024
[#8183]: https://github.com/AdguardTeam/AdGuardHome/issues/8183
diff --git a/openapi/CHANGELOG.md b/openapi/CHANGELOG.md
--- a/openapi/CHANGELOG.md
+++ b/openapi/CHANGELOG.md
@@ -4,6 +4,9 @@
## v0.107.79: API changes
+- The `blocking_mode` field in `DNSConfig` now accepts `noerror`, which
+ returns a successful response with an empty answer for blocked requests.
+
- Field `bootstrap_dns` in `POST /control/dns_config` now accepts comments. A comment must start with the `#` symbol.Validation on that exact head: This is the complete remaining change from my review; no production-code or |
2a4080b to
c2802a5
Compare
Sil3ntVip3r
left a comment
There was a problem hiding this comment.
Rechecked exact current head c2802a5a7acd281f6dbb047239d10d205d5e4888.
The implementation itself is in good shape: make go-check passed; the current dashboard check passed (lint, typecheck, 620 tests); legacy-dashboard lint, typecheck, and 35 tests passed; and make md-lint, make txt-lint, and git diff --check passed.
The rewritten head still omits the three documentation updates from my prior review/comment: its changed-file list does not include AGHTechDoc.md, CHANGELOG.md, or openapi/CHANGELOG.md; the technical-document examples still omit noerror; and neither changelog records the new public mode. The complete minimal patch is already posted above and still applies conceptually to this head.
That documentation-only update is the sole remaining blocker. Please apply it (or an equivalent update) and rerun the two documentation lints; no production-code or additional regression-test change is needed. I expect to approve once it lands.
|
@Sil3ntVip3r The remaining blocker is the AdGuard team's lack of interest. |
c2802a5 to
68d4457
Compare
|
你好 |
91513e6 to
324cebd
Compare
Hello,
I'm not a Go developer by trade and not a DNS expert; please be gentle.
I found #8024 is affecting me.
The default behavior of returning the null address makes localhost reachable under blocked names. This causes errors and unwanted behavior, like being able to ping blocked hosts (by talking to localhost).
Returning NXDOMAIN or REFUSED, on the other hand, will cause extra DNS queries with the search domain appended, i.e.,
blocked.example.combecomesblocked.example.com.local.domain.com.By returning NOERROR without an answer section, these problems go away.
With this patch, AdGuard will return a response similar to resolving a non-existing name, i.e., asking a question but not getting an answer:
If NOERROR is selected and a blocked name is requested, it looks like this:
Closes #8024