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

Fix/tweak blocking page #3403

Merged
merged 1 commit into from
Jul 2, 2020
Merged

Fix/tweak blocking page #3403

merged 1 commit into from
Jul 2, 2020

Conversation

XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented May 18, 2020

  • fix favicon
  • add meta charset
  • add html lang
  • remove unneeded html end tags
  • fix viewport tag to allow zooming
  • compress the "blocked by Pi-hole" SVG

Signed-off-by: XhmikosR xhmikosr@gmail.com

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.


Some fixes for latest AdminLTE and general tweaks

@XhmikosR XhmikosR changed the title Tweak blocking page Tweak blocking page and lighttp configs May 19, 2020
@XhmikosR
Copy link
Contributor Author

XhmikosR commented May 19, 2020

Alright, I pushed a couple more patches.

If you view the diff, remember to add ?w=1 to get the non-whitespace diff, i.e https://github.com/pi-hole/pi-hole/pull/3403/files?w=1

http://192.168.1.XX/admin/scripts/vendor/dataTables.bootstrap.min.js
HTTP/1.1 200 OK
Vary: Accept-Encoding
Content-Encoding: gzip
Last-Modified: Tue, 19 May 2020 08:40:08 GMT
ETag: "3570972622"
Content-Type: text/javascript; charset=utf-8
Accept-Ranges: bytes
Expires: Tue, 19 May 2020 09:23:10 GMT
Cache-Control: max-age=0
X-Pi-hole: The Pi-hole Web interface is working!
X-Frame-Options: DENY
Content-Length: 1560
Date: Tue, 19 May 2020 09:23:10 GMT
Server: lighttpd/1.4.53

/CC @DL6ER @dschaper @PromoFaux for testing and review.

EDIT: only thing I forgot to double check is if the Access-Control-Allow-Origin is set for fonts. It should be, but we should confirm.

EDIT2: the strickler-ci warnings should be ignored. We don't support such ancient browsers, and even if they work, they'll just use the fallback font. The rest are probably valid but I don't want to spend more time on this code.

EDIT3: we could probably not used max-age=0 and make it a little higher. Ideally, we should cache bust the static files and use a long expiring cache-control header, but that's for another day.

@XhmikosR XhmikosR changed the title Tweak blocking page and lighttp configs Tweak blocking page and lighttpd configs May 19, 2020
@XhmikosR
Copy link
Contributor Author

OK, I went ahead and split the changes to only keep the blocking page ones only.

I'll move the rest of the lighttpd changes to a new PR.

@XhmikosR XhmikosR changed the title Tweak blocking page and lighttpd configs Fix/tweak blocking page May 22, 2020
@XhmikosR XhmikosR mentioned this pull request May 22, 2020
8 tasks
<link rel='shortcut icon' href='admin/img/favicons/favicon.ico' type='image/x-icon'>
</head>
<body id='splashpage'>
<img src='admin/img/logo.svg' alt='Pi-hole logo' width='256' height='377'>
Copy link
Contributor Author

@XhmikosR XhmikosR May 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reminds me, it seems to be broken when a not found page is shown since it needs to be /admin/img/logo.svg, relative to the server root like the link bellow this line.

Try for example going to http://IP/admin/foo.

Let me know how to proceed @PromoFaux @DL6ER

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PromoFaux @DL6ER for follow-up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, a lot of me wants the block page to die. It had it's purpose a few years ago, but with the ever increasing uptake of https, the page serves very little purpose (aside from for redirecting from pi.hole to pi.hole/admin

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're not alone in that view.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should just remove it for 5.1, at least for new installs. We could update the installer to not overwrite anything that is already in the /var/www/html/pihole, and keep it in the repo as a starting point for anyone that really wants make use of a custom block page. /shrug

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't use this feature myself, so if you guys think it's better to drop it, I trust you better. That being said, this PR should streamline some stuff. If we decide to drop it, I can rebase #3425 to not depend on this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be fine with dropping it completely but I think that's something we need to get community feedback from first. It will affect users and in some cases it is used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will be users who still use it. But as Promo suggested removing it for new installs and leave old untouched. might be a compromise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how we will handle the blockpage in version 6. The page now is just a glorified 404 error page and really it's a hacky way of doing things. I guess we could have PH7/civetweb display a blockpage but the idea with version 6 is that we no longer use port 80. (Using port 443 is an entirely different discussion.)

So there's a good chance it will go away for that reason, and it would be nice to not step on real 404 errors...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the meantime, I've tested these changes and the block page still works as it used to, so.. I will approve for now.

We can continue the conversation when we're closer to discussion 6.0 in detail I guess!

* fix favicon
* add meta charset
* add html lang
* add a page title
* remove unneeded html end tags
* fix viewport tag to allow zooming
* compress the "blocked by Pi-hole" SVG
* remove trailing spaces
* switch to double colon pseudo elements (works from IE9 and newer)
* add missing vendor prefixes
* other minor tweaks
* add `Access-Control-Allow-Origin` header to all font types

Signed-off-by: XhmikosR <xhmikosr@gmail.com>
@XhmikosR XhmikosR requested review from DL6ER and PromoFaux June 4, 2020 13:26
@PromoFaux PromoFaux merged commit f5a5f68 into pi-hole:development Jul 2, 2020
@XhmikosR XhmikosR deleted the XhmikosR-patch-1 branch July 2, 2020 09:28
@PromoFaux PromoFaux mentioned this pull request Jul 5, 2020
@pralor-bot
Copy link

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

https://discourse.pi-hole.net/t/pi-hole-5-1-released/35577/1

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.

None yet

6 participants