Remove `'unsafe-inline'` from default-src #2542

XhmikosR · 2025-06-28T18:35:28Z

It's only needed in style-src for chart.js

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

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

What does this PR aim to accomplish?:

How does this PR accomplish the above?:

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

By submitting this pull request, I confirm the following:

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.
I have commented my proposed changes within the code and I have tested my changes.
I am willing to help maintain this change if there are issues with it later.
It is compatible with the EUPL 1.2 license
I have squashed any insignificant commits. (git rebase)
I have checked that another pull request for this purpose does not exist.
I have considered, and confirmed that this submission will be valuable to others.
I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
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

rrobgill · 2025-06-29T03:36:17Z

src/config/config.c


 	conf->webserver.headers.k = "webserver.headers";
-	conf->webserver.headers.h = "Additional HTTP headers added to the web server responses.\n The headers are added to all responses, including those for the API.\n Note about the default additional headers:\n - X-DNS-Prefetch-Control: off: Usually browsers proactively perform domain name resolution on links that the user may choose to follow. We disable DNS prefetching here.\n - Content-Security-Policy: [...] 'unsafe-inline' is both required by Chart.js styling some elements directly, and index.html containing some inlined Javascript code.\n - X-Frame-Options: DENY: The page can not be displayed in a frame, regardless of the site attempting to do so.\n - X-Xss-Protection: 0: Disables XSS filtering in browsers that support it. This header is usually enabled by default in browsers, and is not recommended as it can hurt the security of the site. (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-XSS-Protection).\n - X-Content-Type-Options: nosniff: Marker used by the server to indicate that the MIME types advertised in the  Content-Type headers should not be changed and be followed. This allows to opt-out of MIME type sniffing, or, in other words, it is a way to say that the webmasters knew what they were doing. Site security testers usually expect this header to be set.\n - Referrer-Policy: strict-origin-when-cross-origin: A referrer will be sent for same-site origins, but cross-origin requests will send no referrer information.\n The latter four headers are set as expected by https://securityheaders.io";
+	conf->webserver.headers.h = "Additional HTTP headers added to the web server responses.\n The headers are added to all responses, including those for the API.\n Note about the default additional headers:\n - X-DNS-Prefetch-Control: off: Usually browsers proactively perform domain name resolution on links that the user may choose to follow. We disable DNS prefetching here.\n - Content-Security-Policy: [...] 'unsafe-inline' is required by Chart.js for styling some elements directly.\n - X-Frame-Options: DENY: The page can not be displayed in a frame, regardless of the site attempting to do so.\n - X-Xss-Protection: 0: Disables XSS filtering in browsers that support it. This header is usually enabled by default in browsers, and is not recommended as it can hurt the security of the site. (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-XSS-Protection).\n - X-Content-Type-Options: nosniff: Marker used by the server to indicate that the MIME types advertised in the  Content-Type headers should not be changed and be followed. This allows to opt-out of MIME type sniffing, or, in other words, it is a way to say that the webmasters knew what they were doing. Site security testers usually expect this header to be set.\n - Referrer-Policy: strict-origin-when-cross-origin: A referrer will be sent for same-site origins, but cross-origin requests will send no referrer information.\n The latter four headers are set as expected by https://securityheaders.io";


Tests will fail unless the text is updated here to match

FTL/test/pihole.toml

Lines 693 to 695 in 1ae3d2f

# - Content-Security-Policy: [...] 'unsafe-inline' is both required by Chart.js

# styling some elements directly, and index.html containing some inlined Javascript

# code.

XhmikosR · 2025-06-29T04:24:29Z

I have already updated that to match the new text, but tests haven't run to check.

…

On Sun, Jun 29, 2025 at 6:36 AM Rob Gill ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/config/config.c <#2542 (comment)>: > @@ -1028,13 +1028,13 @@ static void initConfig(struct config *conf) conf->webserver.threads.c = validate_stub; // Only type-based checking conf->webserver.headers.k = "webserver.headers"; - conf->webserver.headers.h = "Additional HTTP headers added to the web server responses.\n The headers are added to all responses, including those for the API.\n Note about the default additional headers:\n - X-DNS-Prefetch-Control: off: Usually browsers proactively perform domain name resolution on links that the user may choose to follow. We disable DNS prefetching here.\n - Content-Security-Policy: [...] 'unsafe-inline' is both required by Chart.js styling some elements directly, and index.html containing some inlined Javascript code.\n - X-Frame-Options: DENY: The page can not be displayed in a frame, regardless of the site attempting to do so.\n - X-Xss-Protection: 0: Disables XSS filtering in browsers that support it. This header is usually enabled by default in browsers, and is not recommended as it can hurt the security of the site. (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-XSS-Protection).\n - X-Content-Type-Options: nosniff: Marker used by the server to indicate that the MIME types advertised in the Content-Type headers should not be changed and be followed. This allows to opt-out of MIME type sniffing, or, in other words, it is a way to say that the webmasters knew what they were doing. Site security testers usually expect this header to be set.\n - Referrer-Policy: strict-origin-when-cross-origin: A referrer will be sent for same-site origins, but cross-origin requests will send no referrer information.\n The latter four headers are set as expected by https://securityheaders.io"; + conf->webserver.headers.h = "Additional HTTP headers added to the web server responses.\n The headers are added to all responses, including those for the API.\n Note about the default additional headers:\n - X-DNS-Prefetch-Control: off: Usually browsers proactively perform domain name resolution on links that the user may choose to follow. We disable DNS prefetching here.\n - Content-Security-Policy: [...] 'unsafe-inline' is required by Chart.js for styling some elements directly.\n - X-Frame-Options: DENY: The page can not be displayed in a frame, regardless of the site attempting to do so.\n - X-Xss-Protection: 0: Disables XSS filtering in browsers that support it. This header is usually enabled by default in browsers, and is not recommended as it can hurt the security of the site. (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-XSS-Protection).\n - X-Content-Type-Options: nosniff: Marker used by the server to indicate that the MIME types advertised in the Content-Type headers should not be changed and be followed. This allows to opt-out of MIME type sniffing, or, in other words, it is a way to say that the webmasters knew what they were doing. Site security testers usually expect this header to be set.\n - Referrer-Policy: strict-origin-when-cross-origin: A referrer will be sent for same-site origins, but cross-origin requests will send no referrer information.\n The latter four headers are set as expected by https://securityheaders.io"; Tests will fail unless the text is updated here to match https://github.com/pi-hole/FTL/blob/1ae3d2f21eca83108801e4467c106214c8f971ad/test/pihole.toml#L693-L695 — Reply to this email directly, view it on GitHub <#2542 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACVLNPSB4WPCDUARRTF5NT3F5NMNAVCNFSM6AAAAACALGSALCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDSNRZGA3DSOJYHA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

DL6ER · 2025-06-29T05:09:21Z

Tests running now

XhmikosR · 2025-06-29T05:19:24Z

Now it seems to be OK, CI is failing in "Display structure of downloaded files" and other places which looks like it's unrelated to my changes?

XhmikosR · 2025-06-29T05:20:51Z

Regardless, before landing this please everyone test if things are fine on web. I tested it and it looks like it. Only chart.js needs unsafe inline for styling, so this is a good hardening measure since we are getting rid of the ability to run random JavaScript code.

XhmikosR · 2025-06-30T05:14:34Z

Note that if we hit any issues say for example with images, we could explicitly specify 'unsafe-inline' for image-src then. This is much safer than allowing it by default.

I'm still using this CSP and haven't noticed any issues so far, but the more eyes the better.

yubiuser · 2025-06-30T18:52:12Z

Only chart.js needs unsafe inline for styling

Is it our chart.js or the vendor supplied file? If the former, can we change the code to drop the unsafe-line completely?

XhmikosR · 2025-06-30T18:53:39Z

No, you can't drop it. It's how it works AFAICT.

…

On Mon, Jun 30, 2025, 21:52 yubiuser ***@***.***> wrote: *yubiuser* left a comment (pi-hole/FTL#2542) <#2542 (comment)> Only chart.js needs unsafe inline for styling Is it our chart.js or the vendor supplied file? If the former, can we change the code to drop the unsafe-line completely? — Reply to this email directly, view it on GitHub <#2542 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACVLNOSL7NQK72MAKSSPVL3GGBPFAVCNFSM6AAAAACALGSALCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTAMRQGM2DOMZSGQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

XhmikosR · 2025-06-30T18:54:04Z

The vendor code does it.

…

On Mon, Jun 30, 2025, 21:53 XhmikosR ***@***.***> wrote: No, you can't drop it. It's how it works AFAICT. On Mon, Jun 30, 2025, 21:52 yubiuser ***@***.***> wrote: > *yubiuser* left a comment (pi-hole/FTL#2542) > <#2542 (comment)> > > Only chart.js needs unsafe inline for styling > > Is it our chart.js or the vendor supplied file? If the former, can we > change the code to drop the unsafe-line completely? > > — > Reply to this email directly, view it on GitHub > <#2542 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/AACVLNOSL7NQK72MAKSSPVL3GGBPFAVCNFSM6AAAAACALGSALCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTAMRQGM2DOMZSGQ> > . > You are receiving this because you authored the thread.Message ID: > ***@***.***> >

XhmikosR · 2025-07-02T04:37:15Z

I had a closer look and I think we could drop unsafe-inline from style-src later too. Chart.js has an option for this and we also need to make sure we stop using inline styles in web.

But the biggest gain is from this change essentially not allowing unsafe-inline for script-src.

DL6ER · 2025-07-02T09:22:40Z

Chart.js has an option for this and we also need to make sure we stop using inline styles in web.

This option has been added for Chart.js v2.9 (chartjs/Chart.js#6048). We are currently using v4.5 and disableCSSInjection does not exist in the Chart.js source code. The related paragraph that was there on v2.9 is also gone in the most recent version of Chart.js.

I can confirm that default-src 'self'; alone causes the charts to be drawn much larger and that the change you are proposing seems to have no negative effect on the Charts themselves.

However, we do get many

Content-Security-Policy: The page’s settings blocked an event handler (script-src-attr) from being executed because it violates the following directive: “default-src 'self'”.

on http://pihole/api/docs and the page is overall broken.

The failing CI tests should be resolves once #2543 has been merged and the PR being rebased on latest development thereafter.

XhmikosR · 2025-07-02T10:42:47Z

Yeah, that's why I suggest my current patch for now.

If/when we have time later we could look into dropping unsafe-inline from style-src too.

XhmikosR · 2025-07-02T15:29:29Z

on http://pihole/api/docs and the page is overall broken.

Oh, missed this. I thought the CSP affected only the web. Not sure I have the bandwidth to check the API docs myself. But ideally they shouldn't use the same CSP as the Web.

DL6ER · 2025-07-03T10:41:17Z

But ideally they shouldn't use the same CSP as the Web.

It is the same webserver. Unfortunately, CivetWeb does not support different headers for different directories.

XhmikosR · 2025-07-03T13:53:46Z

How can I debug the API docs? From a quick look it's those onclick handlers AFAICT that need to be moved to the JS.

XhmikosR · 2025-07-05T15:48:54Z

src/api/docs/content/pi-hole.js

+
+	docEl.addEventListener('after-try', (event) => {
+		console.log(event.detail.response);
+		if(event.detail.response.status === 401) {


Unrelated to this patch, but should be addressed later, this throws an error if there's a network error. We should add a try/catch here.

XhmikosR · 2025-07-05T15:49:39Z

src/api/docs/content/index.html

                <div>
-                    <button onclick="document.getElementById('thedoc').setAttribute('theme', 'dark')" class="btn">Dark Theme</button>
-                    <button onclick="document.getElementById('thedoc').setAttribute('theme', 'light')" class="btn">Light Theme</button>
+                    <button type="button" class="btn" id="darkThemeBtn">Dark Theme</button>


These changes are not related to the inline JS move, but the default button type is submit.

XhmikosR · 2025-07-06T16:12:29Z

It's a PITA to handle the API docs CSP along with the Web CSP, but since we can't have a separate CSP...

I tried my best from my limited testing since I don't have a binary from this branch to try and I could only test by specifying the header on the Web.

I could split the second patch since it can go alone if you guys prefer. And then we can test the API docs further.

XhmikosR · 2025-07-08T05:29:11Z

I split the JS changes to #2559 so that we land that without changing the CSP first. Then I will rebase this PR and see if there are any further issues but since I don't have a binary I can only test with the Web Header.

PromoFaux · 2025-07-08T07:30:55Z

since I don't have a binary

... then build it? ;)

There is a devcontainer on FTL repo so you don't need any special tools to build it. Open in devcontainer and ./build.sh - presto you will have a binary.

yubiuser · 2025-07-09T10:41:52Z

but since I don't have a binary I can only test with the Web Header.

We upload all the binaries build during the CI/CD process. For this PR and the current run the binaries are at
https://github.com/pi-hole/FTL/actions/runs/16134770328

XhmikosR · 2025-07-09T12:33:55Z

Perfect, thanks. I was waiting for the CI.

I've split the JS changes to a separate PR (#2559) if you prefer which we can land independently.

Feel free to test it further you guys too.

XhmikosR · 2025-07-09T18:29:12Z

Note that this fixes 2 CSP violations on the API docs:

regarding the font that rapidoc loads
rapidoc is using data SVGs for the dropdown icons

The web interface seems to behave the same, but the more eyes the better.

This PR alone will tighten things security-wise, and fix almost all console errors on the API page. The remaining one is fixed by #2557 .

XhmikosR · 2025-07-10T14:27:50Z

Actually, I notice now that rapidoc has an option to not load the fonts from the CDN. load-fonts https://rapidocweb.com/api.html#att-colors

Perhaps we should specify this and drop the CSP whitelist since we don't need it on the Web?

EDIT: I added a second patch. I suggest that we first land #2559, then I rebase and squash any patches here if we decide to go with no font approach, otherwise I'll drop the patch.

Also, minor tweaks Signed-off-by: XhmikosR <[email protected]>

It's only needed in style-src for chart.js. Also add `font-src https://fonts.gstatic.com` and img-src data: since the API docs need it. Signed-off-by: XhmikosR <[email protected]>

Signed-off-by: XhmikosR <[email protected]>

rrobgill reviewed Jun 29, 2025

View reviewed changes

yubiuser mentioned this pull request Jun 30, 2025

Fix foreign fork PRs #2543

Merged

5 tasks

XhmikosR commented Jul 5, 2025

View reviewed changes

XhmikosR added 3 commits July 10, 2025 17:29

Move inline JavaScript to pi-hole.js

3936f52

Also, minor tweaks Signed-off-by: XhmikosR <[email protected]>

Remove 'unsafe-inline' from default-src

5472f5a

It's only needed in style-src for chart.js. Also add `font-src https://fonts.gstatic.com` and img-src data: since the API docs need it. Signed-off-by: XhmikosR <[email protected]>

CSP: drop font-src and use RapidDoc's load-fonts = false option

7377e08

Signed-off-by: XhmikosR <[email protected]>

DL6ER mentioned this pull request Jul 18, 2025

Improve default CSP headers #2575

Merged

5 tasks

	# - Content-Security-Policy: [...] 'unsafe-inline' is both required by Chart.js
	# styling some elements directly, and index.html containing some inlined Javascript
	# code.

Uh oh!

Remove 'unsafe-inline' from default-src #2542

Remove 'unsafe-inline' from default-src #2542

Uh oh!

Conversation

XhmikosR commented Jun 28, 2025

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

Uh oh!

rrobgill Jun 29, 2025

Choose a reason for hiding this comment

Uh oh!

XhmikosR commented Jun 29, 2025 via email

Uh oh!

DL6ER commented Jun 29, 2025

Uh oh!

XhmikosR commented Jun 29, 2025

Uh oh!

XhmikosR commented Jun 29, 2025

Uh oh!

XhmikosR commented Jun 30, 2025

Uh oh!

yubiuser commented Jun 30, 2025

Uh oh!

XhmikosR commented Jun 30, 2025 via email

Uh oh!

XhmikosR commented Jun 30, 2025 via email

Uh oh!

XhmikosR commented Jul 2, 2025

Uh oh!

DL6ER commented Jul 2, 2025

Uh oh!

XhmikosR commented Jul 2, 2025

Uh oh!

XhmikosR commented Jul 2, 2025

Uh oh!

DL6ER commented Jul 3, 2025

Uh oh!

XhmikosR commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

XhmikosR Jul 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XhmikosR Jul 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XhmikosR commented Jul 6, 2025

Uh oh!

XhmikosR commented Jul 8, 2025

Uh oh!

PromoFaux commented Jul 8, 2025

Uh oh!

yubiuser commented Jul 9, 2025

Uh oh!

XhmikosR commented Jul 9, 2025

Uh oh!

XhmikosR commented Jul 9, 2025

Uh oh!

XhmikosR commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Remove `'unsafe-inline'` from default-src #2542

Remove `'unsafe-inline'` from default-src #2542

XhmikosR commented Jul 3, 2025 •

edited

Loading

XhmikosR Jul 5, 2025 •

edited

Loading

XhmikosR Jul 5, 2025 •

edited

Loading

XhmikosR commented Jul 10, 2025 •

edited

Loading