AdguardTeam / AdguardTeam/HostlistCompiler

Validator bypasses hostname checks for ||domain-style rules missing a ^ terminator

Ouverte
#129 0 commentaires 0 réactions 1 personne assignée Réclamée par @maximtop Voir sur GitHub
Priority: P4
Langage dominant
JavaScript
Étoiles
232
Forks
30
Métriques de merge des PR
Aucune PR mergée en 30 j

Description

The "uBlock₀ filters – Badware risks" list (uAssets [`badware.txt`](https://github.com/uBlockOrigin/uAssets/blob/master/filters/badware.txt), sourced into AdGuard DNS via [`HostlistsRegistry` filter #50](https://github.com/AdguardTeam/HostlistsRegistry/blob/main/assets/filter_50.txt)) contains this rule, targeting phishing lookalikes of `roblox.com` such as `roblox.com.gr`:

```
||roblox.com.$all
```

The trailing dot with no `^` terminator is intentional, valid uBO/Adblock syntax: it requires something to follow `roblox.com.`, which real Roblox subdomains never have (they precede `roblox.com`, not follow it) but arbitrary lookalike TLDs do (`roblox.com.gr`, `roblox.com.br`, etc.). This lets one rule catch the whole class of `roblox.com.*` typosquats without matching the real domain or its real subdomains — using `^` instead would only match `roblox.com` exactly, which is the opposite of what's needed here.

`HostlistCompiler` correctly strips the unsupported `$all` modifier for DNS use, producing:

```
||roblox.com.
```

but this compiled rule ends up blocking `clientsettingscdn.roblox.com` (legitimate Roblox domain) on AdGuard DNS (confirmed via the "Check the domain" tool) while leaving `roblox.com` and other subdomains unaffected — behavior the source uBO syntax was never meant to produce.

**Root cause**

In `src/transformations/validate.js`, `validAdblockRule` has this check:

```js
// Check if the pattern does not start with the domain prefix and does not contain a domain separator
if (!_.startsWith(props.pattern, DOMAIN_PREFIX) || sepIdx === -1) {
return true;
}
```

where `DOMAIN_PREFIX = '||'` and `DOMAIN_SEPARATOR = '^'`. Since `||roblox.com.` starts with `||` but contains no `^` anywhere, `sepIdx === -1`, and the function returns `true` (valid) immediately — bypassing every hostname-level check that follows.

More importantly, this is a case where the validator's own model doesn't match the DNS matching engine's behavior. The compiler's `EXACT_DOMAIN_PATTERN` regex and its comments show awareness of trailing-dot FQDN notation, but only for `^`-terminated rules (e.g. `||example.org.^`), where `extractDomainPattern` explicitly normalizes the trailing dot away, treating it as equivalent to no dot at all (line 228-229: *"Allow trailing dot as absolute FQDN marker by normalizing it away for validation"*). But per my testing of both `urlfilter` and `dnsfilter`, a trailing dot *without* `^` is not equivalent to no dot — it changes the rule's matching scope entirely (requiring something to follow, rather than matching the bare domain). A rule in this form isn't a broken domain rule that happens to skip validation; it's a different, valid rule type — a suffix/TLD-variant matcher — that the validator has no model for at all, so it can't verify whether the DNS-compiled output does what the original rule intended.

**Suggested fix**

Rules starting with `DOMAIN_PREFIX` but lacking `DOMAIN_SEPARATOR` shouldn't get an automatic pass. At minimum, apply the same `checkChars`/hostname sanity checks used for anchored rules; ideally, recognize trailing-dot-no-caret patterns as their own category and verify their DNS-compiled behavior actually matches the semantics they have in uBO (only matching hostnames with a suffix beyond the given domain), rather than silently passing them through as if they were ordinary domain-anchored rules.

If this is a known/expected side effect of how non-DNS-oriented filter rules get converted for DNS matching, consider adding a warning in the AdGuard DNS UI when a list contains rules that upstream maintainers have explicitly stated aren't meant for DNS use — uAssets maintainers told me directly in https://github.com/uBlockOrigin/uAssets/issues/33636: *"Filters in uAssets are not meant to be used in DNS. Remove that list in your DNS settings."* Without a warning, users keep filing reports against uAssets for behavior that's actually specific to the DNS conversion, creating avoidable maintenance burden for filter list maintainers who have no way to fix it on their end without weakening what the rule is designed to catch.

Image

Guide de contribution

Aucun guide de contribution indexé pour ce dépôt

Évaluation

Cette issue n'a pas encore été évaluée.

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.