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

Make additional_headers configurable #2236

Merged
merged 7 commits into from
Feb 22, 2025
Merged

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Feb 21, 2025

What does this implement/fix?

Make additional_headers property of the embedded web server CivetWeb editable by users via the new config option webserver.headers. The default value of this variable has some best-practice headers.

Related issue or feature (if applicable): #2215

Pull request in docs with documentation (if applicable): 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.
  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)

Checklist:

  • The code change is tested and works locally.
  • I based my code and PRs against the repositories developmental branch.
  • I signed off all commits. Pi-hole enforces the DCO for all contributions
  • I signed all my commits. Pi-hole requires signatures to verify authorship
  • I have read the above and my PR is ready for review.

Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

It seems something was wrong with #2207

Despite being on this branch I get a lot of log output which was introduced with the mentioned PR

2025-02-21 22:49:54.001 CET [126824/T126940] INFO: NTP server listening on :::123 (IPv6)
2025-02-21 22:51:26.592 CET [126824/T126952] INFO: Local URI: "/admin/"
2025-02-21 22:51:26.643 CET [126824/T126953] INFO: Local URI: "/admin/login"
2025-02-21 22:51:26.724 CET [126824/T126952] INFO: Local URI: "/admin/vendor/fonts/SourceSansPro/SourceSansPro.css"
2025-02-21 22:51:26.724 CET [126824/T126953] INFO: Local URI: "/admin/vendor/nprogress/nprogress.min.css"
2025-02-21 22:51:26.724 CET [126824/T126954] INFO: Local URI: "/admin/vendor/bootstrap/css/bootstrap.min.css"
2025-02-21 22:51:26.762 CET [126824/T126952] INFO: Local URI: "/admin/vendor/animate/animate.min.css"
2025-02-21 22:51:26.763 CET [126824/T126953] INFO: Local URI: "/admin/vendor/bstreeview/bstreeview.min.css"
2025-02-21 22:51:26.764 CET [126824/T126954] INFO: Local URI: "/admin/vendor/select2/select2.min.css"
2025-02-21 22:51:26.797 CET [126824/T126952] INFO: Local URI: "/admin/style/pi-hole.css"
2025-02-21 22:51:26.797 CET [126824/T126953] INFO: Local URI: "/admin/vendor/adminLTE/AdminLTE.min.css"
2025-02-21 22:51:26.806 CET [126824/T126954] INFO: Local URI: "/admin/style/themes/default-dark.css"
2025-02-21 22:51:26.830 CET [126824/T126953] INFO: Local URI: "/admin/vendor/waitMe/waitMe.min.css"
2025-02-21 22:51:26.830 CET [126824/T126952] INFO: Local URI: "/admin/vendor/nprogress/nprogress.min.js"
2025-02-21 22:51:26.840 CET [126824/T126954] INFO: Local URI: "/admin/vendor/jquery/jquery.min.js"
2025-02-21 22:51:26.860 CET [126824/T126952] INFO: Local URI: "/admin/vendor/bootstrap/js/bootstrap.min.js"
2025-02-21 22:51:26.860 CET [126824/T126953] INFO: Local URI: "/admin/vendor/adminLTE/adminlte.min.js"
2025-02-21 22:51:26.872 CET [126824/T126954] INFO: Local URI: "/admin/vendor/bootstrap-notify/bootstrap-notify.min.js"
2025-02-21 22:51:26.889 CET [126824/T126952] INFO: Local URI: "/admin/vendor/font-awesome/all.min.js"
2025-02-21 22:51:26.889 CET [126824/T126953] INFO: Local URI: "/admin/scripts/js/utils.js"
2025-02-21 22:51:26.901 CET [126824/T126954] INFO: Local URI: "/admin/vendor/waitMe/waitMe.min.js"
2025-02-21 22:51:26.918 CET [126824/T126953] INFO: Local URI: "/admin/scripts/js/login.js"

Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

I tried this branch, but could not get custom headers injected. Instead, even with no configuration change, 'All Settings' were unusable and gave the following error

2025-02-21_23-26

Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@DL6ER
Copy link
Member Author

DL6ER commented Feb 22, 2025

grafik

Interesting that it can be null immediately after the if (typeof allowed === object)...

@DL6ER DL6ER force-pushed the new/webserver_headers branch from d9fd060 to 23683eb Compare February 22, 2025 09:04
…itable by users via the new config option webserver.headers

Signed-off-by: DL6ER <dl6er@dl6er.de>
…heck for this during config parsing.

Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER DL6ER force-pushed the new/webserver_headers branch from 23683eb to 10cfd31 Compare February 22, 2025 09:08
Copy link

Conflicts have been resolved.

@DL6ER
Copy link
Member Author

DL6ER commented Feb 22, 2025

Rebased on latest development and added an automatic check to ensure the CI complains loudly if the allowed property of config items needing it is again forgotten in some future PR.

@DL6ER DL6ER requested a review from yubiuser February 22, 2025 09:09
@DL6ER
Copy link
Member Author

DL6ER commented Feb 22, 2025

grafik

Signed-off-by: DL6ER <dl6er@dl6er.de>
Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

Works, but 2 'issues'

  1. It needs a full FTL restart, the 'automatic' restart after the configuration has changed is not enough
  2. The moment, one presses 'save&apply' from 'All Settings' things go bad - the web interface does not include/render the \r\n which then don't get saved into the toml file and break the headers

2025-02-22_11-06

Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER
Copy link
Member Author

DL6ER commented Feb 22, 2025

The escaping needs to be done in Javascript, I guess. Will look at this later

edit Yes, this is a web issue, see the correct return by the API:
grafik

@DL6ER
Copy link
Member Author

DL6ER commented Feb 22, 2025

@yubiuser See my last commit: Instead of a long string, webserver.headers is now a list of individual headers. FTL will then take care of stapling them together, inserting the correct line endings itself.

grafik

…idual headers. FTL will then take care of inserting the correct line endings itself

Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER DL6ER force-pushed the new/webserver_headers branch from 1089471 to 7ce1ef5 Compare February 22, 2025 18:41
Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

Really good. Works fine now. Last issue: When switching to this branch, the headers are empty by default

#     array of HTTP headers
  headers = [] ### CHANGED, default = [ "Content-Security-Policy: default-src 'self' 'unsafe-inline';", "X-Frame-Options: DENY", "X-XSS-Protection: 0", "X-Content-Type-Options: nosniff", "Referrer-Policy: strict-origin-when-cross-origin" ]

@DL6ER
Copy link
Member Author

DL6ER commented Feb 22, 2025

You actually found a very old bug that is dating years back but we never noticed it because all other JSON arrays are by default empty. What happened here was that FTL reset the array content before parsing the existing TOML. This usually works. We have to reset the array before reading from the config file to avoid adding the config file entries on top of the default values.

The glitch is not that, when the key is new and, hence, not present in pihole.toml, the value remains empty after the reset as there is nothing to be read from the TOML. The default value is already gone by this reset.

The fix is simple: reset the array only when the key actually exists in the config file.

@DL6ER DL6ER merged commit 39a852e into development Feb 22, 2025
19 checks passed
@DL6ER DL6ER deleted the new/webserver_headers branch February 22, 2025 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to enable/allow X-Frame Options (iFrame)
2 participants