Regex blacklist patterns in non-default group not working for clients in default group

[ashmrtn]

In raising this issue, I confirm the following (please check boxes, eg [X]) Failure to fill the template will close your issue:

• I have read and understood the contributors guide.
• The issue I am reporting can be replicated
• The issue I am reporting isn't a duplicate

How familiar are you with the codebase?:

2

---

[BUG] Expected Behaviour:
Blacklist regex patterns that are in a group other than the default group should be blocked for clients that are in the default group/no group

[BUG] Actual Behaviour:
Clients in the default group/no group can access domains that should be blocked by blacklist regex patterns in a group other than default

[BUG] Steps to reproduce:

1. create new regex blacklist pattern
2. create a new group
3. add new regex pattern to a group other than the default group - do not add the pattern to both the new group and the default group, just add it to the new group
4. try to access a domain that is blocked by the regex pattern with a client in the default group/not explicitly assigned to a group

This bug likely also appears for whitelisted regex patterns that are in the non-default group, though I haven't explicitly tested that situation.

---

I believe the queries to gravity's database for regex domains is a little too strict. The sql statement SELECT id from %s WHERE group_id IN (%s); in gravityDB_get_regex_client_groups selects only regex domains that match exactly the group of the client, even if the client is in the special "all groups" group with id 0.

I believe swapping out the linked line with something like the following would solve the problem:

if (strcmp(groups, "0") == 0) {
  // Client is in default group, allow all regex domains.
  if(asprintf(&querystr, "SELECT DISTINCT id from %s;", table) < 1)
  {
    logg("gravityDB_get_regex_client_groups(%s) - asprintf() error for client in default group", table);
    return false;
  }
} else {
  // Client belongs to a specific group or groups, select only those regex domains in the group(s).
  if(asprintf(&querystr, "SELECT id from %s WHERE group_id IN (%s);", table, groups) < 1)
  {
    logg("gravityDB_get_regex_client_groups(%s, %s) - asprintf() error", table, groups);
    return false;
  }
}

This template was created based on the work of udemy-dl.

[yubiuser]

I think this is by design.
If the regex is not in the group "default" clients in that group will not be affected by this regex.
There is no special group id for "all groups", it's just a web presentation of clients being member of every group - including the default one (which has id 0).

[ashmrtn]

Doesn't that kind of defeat the purpose of groups though? If you wanted to use groups to organize and easily enable/disable the rules, then you'd have to add them to the rule to other groups while leaving it in the default group. If you wanted to disable the rules in the new group used for organization, you'd have to disable not only the new group containing them, but also the default group.

Looking at it from the other side, if users have to have the client in the group that contains the regex rules, then the user would have to manually add all clients to the group in the web UI. This would have management issues as users may not know client IPs or the IPs could change (if using DHCP) and users may not notice when new clients connect to the network

[yubiuser]

Sorry, I quite don't understand which behavior you would like to see.

This would have management issues as users may not know client IPs or the IPs could change (if using DHCP) and users may not notice when new clients connect to the network

For users with non-deterministic DHCP servers there is already something in the pihole pipeline:
https://discourse.pi-hole.net/t/feedback-for-allow-defining-clients-by-their-mac-address-host-name-and-networking-interface/32324/83

[ashmrtn]

My apologies!

Let's say that I have some number of domains that I would like to block (or whitelist) and they fit a regex pattern. To be organized, I'd also like to add them to a group (and remove them from the default group just to be extra-organized) so that I can easily control whether they are enabled or not, but I'd still like them to apply to all clients that use Pi-hole as a DNS.

The documentation on group management is a little on the thin side currently, but from what I pieced together from the Pi-hole documentation and the PR for the groups feature, it sounds like clients that don't belong to any group should use all groups that are enabled.

However, in practice, it appears that rules in groups only apply to clients explicitly added to that group. Furthermore, given the current setup of the clients portion of the web UI, there is no way to tell the system that all clients that connect to the Pi-hole should be added to some set of groups in addition to the default group.

To be clear, I'm not advocating that this feature needs to be added if it wasn't already on the roadmap. However, I do find it moderately confusing that the system behaves the way it does, especially given that the PR notes I found seem to imply otherwise.

If a documentation update stating the current behavior is the way to resolve this confusion, then I'd be happy to help with that as well

[DL6ER]

Yes, how it is currently implemented is the way it is designed and expected to work.

and remove them from the default group just to be extra-organized

Don't do this. Clients unknown belong to the default group. Known but unconfigured ones, as well.

Imagine the contrary of blacklist regex, whitelist regex. You may have a group "allow Google ads" because someone in your network insist on using them. You don't want this rule to become active for all unconfigured clients, right?

Or take special blocklists, more strict than what you normally have for some clients in some special group only (hence, they are in the respective client's group but not the default one). They may be targeting IoT devices or something.

I think there are good reasons for how it is right now, but I'm open for discussions about improving the documentation about it.

[yubiuser]

there is no way to tell the system that all clients that connect to the Pi-hole should be added to some set of groups in addition to the default group.

Your not alone on this one: here is a workaround. As noted in the discussion it would be great if you could open a feature request on that.
https://discourse.pi-hole.net/t/group-management-question/32062

[ashmrtn]

Imagine the contrary of blacklist regex, whitelist regex. You may have a group "allow Google ads" because someone in your network insist on using them. You don't want this rule to become active for all unconfigured clients, right?

Thanks for the counter-example. I neglected to think of that situation.

I think an update to the documentation would be useful. Currently (at least to me), it is unclear whether groups act as a sort of organizational/"sub-list" feature for lists, or if they are separate entities altogether. In particular (per the "Group Management" doc):

This allows not only grouping them to highlight their relationship, but also enabling/disabling them together if one, for instance, wants to visit a specific service only temporarily.

makes it seem almost like they are a sort of sub-grouping of blacklist/whitelist within the larger set of entries that can be easily toggled.

From this perspective, it seems that this functionality was imagined to have a sort of double-duty. The first was to provide organizational groupings within sets of domains (sort of a "sub-list" if you will) that could be easily toggled on/off. The second was to provide isolated groups of whitelist/blacklist entries that could be tied to specific clients added to that group.

Setting user expectations with notes about how there is no default way to add clients to groups and how rules only apply to clients added to the group that the rules are in would help quite a bit I think

[dschaper]

We write the documentation as the developers and how we see the code working. The documentation is open sourced just like the rest of the project. Writing the documentation from the user perspective would be very helpful and we invite you to contribute in that way.

https://github.com/pi-hole/docs