Fix validation of adlist url

[mdujava]

By submitting this pull request, I confirm the following:
please fill any appropriate checkboxes, e.g: [X]

• I have read and understood the contributors guide, as well as this entire template.
• I have made only one major change in my proposed changes.
• I have commented my proposed changes within the code.
• I have tested my proposed changes, and have included unit tests where possible.
• I am willing to help maintain this change if there are issues with it later.
• I give this submission freely and claim no ownership.
• It is compatible with the EUPL 1.2 license
• I have squashed any insignificant commits. (git rebase)

Please make sure you Sign Off all commits. Pi-hole enforces the DCO.

---

What does this PR aim to accomplish?:
This PR is fixing #3911 as last verification of url, before downloading adlist from it, has missing good character, therefor valid url with basicauth user:pass (eg. https://user:pass@fqdn/dir/file.list) is evaluated as invalid.

How does this PR accomplish the above?:
Regex which is used as validator is updated to accept @ as valid character. As char @ is missing, adding it to the regex will fix the issue.

What documentation changes (if any) are needed to support this PR?:
This is fixing back-end URL validation and there is no change in user interaction, therefor no documentation change needed.

---

• You must follow the template instructions. Failure to do so will result in your pull request being closed.
• Please respect that Pi-hole is developed by volunteers, who can only reply in their spare time.

[dschaper]

It's not a good character.

https://stackoverflow.com/a/19511469

You need to percent encode in your configuration, not allow an invalid character in the URL.

[mdujava]

Hi, I rewrote the fix inspired from: https://github.com/pi-hole/pi-hole/blob/4a7ef7bb18a923b51cc3708d56de5bafbeb94a4f/gravity.sh#L634-L639 for removal of schema and bascauth before validating against unchanged regex for valid chars.

[dschaper]

If you want @ in the URI then you need to percent encode it yourself. Adding the @ character is a no from me.

[mdujava]

curl https://user:password@fqdn.com/path is working fine, lets encode it: https://user:password%40fqdn.com/path this will return curl: (6) Could not resolve host: user; Unknown error

3.2.(1.)? section of RFC 3986 is allowing @ right before host for including User Information.

authority = [ userinfo "@" ] host [ ":" port ]

If Pi-hole should not support this feature why https://github.com/pi-hole/pi-hole/blob/4a7ef7bb18a923b51cc3708d56de5bafbeb94a4f/gravity.sh#L634 is acknowledging that it's valid part or URL.

[dschaper]

curl: (6) Could not resolve host: user;

That's probably because you didn't quote the URL?

curl "https://user:password%40fqdn.com/path" ?

[dschaper]

If Pi-hole should not support this feature why

That tells me that code has been there for over 3 years, so either you're the first person in 3 years to find it doesn't work, or there's something else that needs to be addressed before changing the code.

[mdujava]

still the same error code if using curl 7.29.0 (armv7hl-redhat-linux-gnu) libcurl/7.29.0 NSS/3.53.1 zlib/1.2.7 libidn/1.28 libssh2/1.8.0

on curl 7.68.0 is different error: curl: (3) URL using bad/illegal format or missing URL

---

Yeah I am probably the first one. I want to host mine adlist, but I don't want it to be public, so for authentication I am using basicauth.

There should not be url encoding of @:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication#Access_using_credentials_in_the_URL
https://en.wikipedia.org/wiki/Basic_access_authentication#URL_encoding
https://en.wikipedia.org/wiki/URL#Syntax

@ is reserved char per https://tools.ietf.org/html/rfc3986#section-2.2, you are right that you should not have @ in the URL path/parameters part.

Per 2.2 @ is reserved char, that can be used in URL at most one time and few more restrictions. Therefor checking existing goodchars only in fqdn/path?args#fragment was easier/quicker to implement.

I think I can not provide more information than this. If this is not sufficient, feel free to close this PR.

[DL6ER]

An alternative solution would be to check for @ only within the allowed URL path, i.e., between :// and the next /. We can just replace it by a valud char (say X) it if it appears only once. I'm thinking about something like

url="https://user:pass@fqdn/dir/file.list"
check_url="$(sed -r "s#(.*)@#\1X#" <<< "${url}")"

the result is https://user:passXfqdn/dir/file.list which would pass the original regex we have in place (test the output of this, not $url itself!).

Note that it will only replace one @ so https://user:pass@@fqdn/dir/file.list would be rejected by the regex as one @ would remain in the output. If $url does not contain any @, the sed will just echo what you put in.

What do you think?

[mdujava]

url="https://user:pass@fqdn/dir/file.list"
check_url="$(sed -r "s#(.*)@#\1X#" <<< "${url}")"

This is not consicering the placement of @ relative to /.

What do you think?

This looks way better. I will play with it to ensure the place of @.

[mdujava]

It's not a good character.

https://stackoverflow.com/a/19511469

both ways are possible as first answer shows, you can encode it as %40 or not even in path section.

Both of those URLs will redirect you to wikipedia page "@".

• http://en.wikipedia.org/wiki/%22@%22
• http://en.wikipedia.org/wiki/%22%40%22

[dschaper]

And does curl accept both forms?

[mdujava]

Yes

$ curl -O "https://en.wikipedia.org/wiki/%22@%22" -O "https://en.wikipedia.org/wiki/%22%40%22"
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 78120  100 78120    0     0   286k      0 --:--:-- --:--:-- --:--:--  286k
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 78120  100 78120    0     0  1556k      0 --:--:-- --:--:-- --:--:-- 1556k

$ md5sum %22*
5abc9c22c81f3f12deb5aeb001600950  %22@%22
5abc9c22c81f3f12deb5aeb001600950  %22%40%22

[pralor-bot]

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-core-v5-2-3-web-v5-3-and-ftl-v5-4-released/43009/1