-
-
Notifications
You must be signed in to change notification settings - Fork 568
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
Fix the insertion of multiple domains as wildcard #3259
base: development
Are you sure you want to change the base?
Conversation
Signed-off-by: RD WebDesign <[email protected]>
scripts/js/groups-domains.js
Outdated
// Transform domain to wildcard if specified by user | ||
domains[i] = "(\\.|^)" + domains[i].replaceAll(".", "\\.") + "$"; | ||
kind = "regex"; | ||
|
||
// strip leading "*." if specified by user in wildcard mode | ||
if (domains[i].startsWith("*.")) domains[i] = domains[i].substr(2); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the code, I can see this:
If the user enters *.example.com
the domain will be change into a wildcard (line 503) before the *.
is removed (line 506, but the domain will never start with "*." after the previous line), resulting in this regex: (\.|^)*\.example\.com
.
Is this intended?
The question is: *.example.com
should be changed into what regex?
(\.|^)example\.com$
(every subdomains and also the main domain);\.example\.com$
(only subdomains, but not the main domain);(\.|^)*\.example\.com$
(same as above, but more complex to parse);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intended?
I don't think so. I think the order here is wrong. The removal of *.
was meant as a help because users entered domains like this and checked the wildcard box because they think they had to enter *.
for it to become a wildcard.
The question is: *.example.com should be changed into what regex?
I think
(.|^)example.com$ (every subdomains and also the main domain);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I will change the order and remove the *.
text before turning it into a regex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In v5, it was explicitly the latter: all subdomains but not the domain itself. This is rather uncritical and non-breaking here as the checkbox is just a assistance tool for those inexperienced with regex. To me, *.abc.com
does clearly not include abc.com
itself, however, this is just me (and Pi-hole v5). You are perfectly right to ask "what would the user expect to happen". I don't know TBH. But @yubiuser may be right (include also the domain itself).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can discuss this a little more before we decide. I don't want to change the previous behavior unless we have a good reason.
Signed-off-by: RD WebDesign <[email protected]>
What does this PR aim to accomplish?
Fix #3257
When selecting the "Add domain as wildcard" checkbox with multiple domains, only the first one was changed into a wildcard regex.
How does this PR accomplish the above?
Changing
kind
variable from "exact" to "regex" only after the loop.The issue happened because the previous code were modifying this variable inside the loop, but after the first domain, the check failed (the kind was wrongly changed before all domains were parsed) and the rest of the domains were not changed into wildcards.
By submitting this pull request, I confirm the following:
git rebase
)