Skip to content
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

added disclaimer for external links #3328

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

VotreCafetier
Copy link

Thank you for your contribution to the Pi-hole Community!

Please read the comments below to help us consider your Pull Request.

We are all volunteers and completing the process outlined will help us review your commits quicker.

Please make sure you

  1. Base your code and PRs against the repositories developmental branch.
  2. Sign Off all commits as we enforce the DCO for all contributions
  3. Sign all your commits as they must have verified signatures
  4. File a pull request for any change that requires changes to our documentation at our documentation repo

What does this PR aim to accomplish?:
This PR aims to prevent unwanted redirection outside of the local environment by introducing an intermediate modal.

How does this PR accomplish the above?:
It intercepts all external links (all links that don't point to itself) click and show a "are you sure" modal (see below)
image

It also adds disclaimer icon to the donation navbar item,
image

and information menu
Screenshot 2025-03-16 at 14 18 05

Link documentation PRs if any are needed to support this PR:
N/A


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.

  • I have read the above and my PR is ready for review. Check this box to confirm

@VotreCafetier VotreCafetier force-pushed the feature/disclaimer-modal branch from d0edac0 to 6607005 Compare March 16, 2025 18:48
@rdwebdesign
Copy link
Member

I don't think this is necessary, specially on the "Donate" menu button (everybody knows this is an external link and not a local Donate link).

@dschaper
Copy link
Member

I like it, seems sensible.

@VotreCafetier
Copy link
Author

I don't think this is necessary, specially on the "Donate" menu button (everybody knows this is an external link and not a local Donate link).

if you are implying the modal, i do agree that the Donate button is a bit much; but query log links, domains link absolutely needs a protection from misclicking.

@VotreCafetier VotreCafetier force-pushed the feature/disclaimer-modal branch 3 times, most recently from 7c8c80d to 64a27fc Compare March 16, 2025 23:28
@VotreCafetier VotreCafetier force-pushed the feature/disclaimer-modal branch from 64a27fc to 8c831bc Compare March 17, 2025 00:07
@darkexplosiveqwx
Copy link

I don't think this is necessary for the donate button and sidebar, GH releases and other official sites.

For me this warning would imply that these sites are not official, but all sites in the sidebar are operated by Pi-hole.

@XhmikosR
Copy link
Contributor

For what is worth, I agree, I don't think this is needed too.

@yubiuser
Copy link
Member

I also think this is not necessary or helpful. What are we protecting users from? It's not that we link to NSFW stuff or cryptominers.
I think we should have trust in our users who set up Pi-hole that the know what the Internet is, how inks work and that they can point to other domains. Especially if the link goes is labeled GitHub or Donate I think we can assume they know that this are external sites.
We discussed to put a stop sign in front of the updater where users could break their system and arguments were this would be too much hand-holding - this PR would be much more hand-holding.

@VotreCafetier
Copy link
Author

I don't think this is necessary for the donate button and sidebar, GH releases and other official sites.

For me this warning would imply that these sites are not official, but all sites in the sidebar are operated by Pi-hole.

I do agree with that point, but it would bring consistency issues since they are external links, but from a trusted source. Maybe just the external logo would suffice for trusted source, but the modal would need another title

@VotreCafetier
Copy link
Author

I also think this is not necessary or helpful. What are we protecting users from? It's not that we link to NSFW stuff or cryptominers. I think we should have trust in our users who set up Pi-hole that the know what the Internet is, how inks work and that they can point to other domains. Especially if the link goes is labeled GitHub or Donate I think we can assume they know that this are external sites. We discussed to put a stop sign in front of the updater where users could break their system and arguments were this would be too much hand-holding - this PR would be much more hand-holding.

While most of our users are advanced, that doesn't mean they are safe from misclicking; especially when navigating quickly, or on mobile. While i did mention it was for security, the primary goal is UX. Yes, the link to Github and Donate is a bit overkill, but as i mentioned in my earlier comment, it's for consistency. A simple icon indicating it's a trusted external source would suffice.

Regarding hand-holding, what’s the point of a web interface if users can already use the command line? What's the point of an "expert" settings section if all users are already expert ? I do get where you come from, but the web interface is designed for advanced users looking for ease of use.

I see two possible solutions: either add a UAC setting to disable the modal, or shortcut override (ex: you hold down shift) while clicking the link

@yubiuser
Copy link
Member

This PR aims to prevent unwanted redirection outside of the local environment

mean they are safe from misclicking

I still don't understand the thread model. What is the danger you see?

@VotreCafetier
Copy link
Author

This PR aims to prevent unwanted redirection outside of the local environment

mean they are safe from misclicking

I still don't understand the thread model. What is the danger you see?

Adlists (/admin/groups/lists) have direct links to third party website
Screenshot 2025-03-17 at 12 44 50

Screenshot 2025-03-17 at 12 49 07

@yubiuser
Copy link
Member

But this are links users have added before by themselves. And if they only add trustworthy lists, this should only point to (txt) lists of domains.

@rdwebdesign
Copy link
Member

Yes, the link to Github and Donate is a bit overkill

Personally, I think it is overkill to include all official Pi-hole links, like: Donate, Pi-hole Docs, Pi-hole website, Pi-hole Discourse and Github links to Pi-hole repositories.

Showing a modal box saying "We are not responsible for the content of external sites" is simply wrong in these cases.

@VotreCafetier
Copy link
Author

But this are links users have added before by themselves. And if they only add trustworthy lists, this should only point to (txt) lists of domains.

yes, we assume they are secure. But the site can always be hijacked, switch, changed, etc. This is more of a prevention than a protection. Last week, I personally noticed that the https://dbl.oisd.nl/ changed to https://big.oisd.nl/ and it made me wonder if the link can be switched up after the addition, so i added a prevention.

@VotreCafetier
Copy link
Author

Yes, the link to Github and Donate is a bit overkill

Personally, I think it is overkill to include all official Pi-hole links, like: Donate, Pi-hole Docs, Pi-hole website, Pi-hole Discourse and Github links to Pi-hole repositories.

Showing a modal box saying "We are not responsible for the content of external sites" is simply wrong in these cases.

Once again, i do agree that the wording is inefficient and overkill for trusted sites. The middle ground i could offer is to remove trusted sites from the list and change up the wording of the modal to include external untrustworthy** sites (or however you want to call it)

@PromoFaux
Copy link
Member

I would expect to, and frequently do, see this kind of modal on a chat or forum application - but to echo the thoughts of others here, I would say it's entirely overkill for the Pi-hole admin dashboard.

At an absolute stretch I could see it being useful on the lists page, but that really is a stretch. Any URL on this list was presumably added by the user themselves. They know it's an external link, why should we remind them of this?

@VotreCafetier
Copy link
Author

I would expect to, and frequently do, see this kind of modal on a chat or forum application - but to echo the thoughts of others here, I would say it's entirely overkill for the Pi-hole admin dashboard.

At an absolute stretch I could see it being useful on the lists page, but that really is a stretch. Any URL on this list was presumably added by the user themselves. They know it's an external link, why should we remind them of this?

UX plain and simple. Got frustrated about accidentally clicking on links on mobile with my big ass thumb while trying to view list info and being redirected god knows where. Let me be clear, this is not a reminder, it's a protection. We don't remind them, we explicitly block the link to show the modal.

I did specify in my earlier comments that it is overkill for trusted external links, like the pi-hole website, repo, etc. and a solution to that is the UAC control system a/o the addition of trusted source in directly in the script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants