Allow editing of the CSP trusted image sources. by w1ll-i-code · Pull Request #5337 · Icinga/icingaweb2

w1ll-i-code · 2025-03-20T09:30:15Z

Hi, as discussed with @lippserd, we improved the CSP header and added the ability to whitelist certain trusted domains for the image sources. This closes #5333.

lippserd · 2025-03-20T09:37:34Z

Hi, as discussed with @lippserd, we improved the CSP header and added the ability to whitelist certain trusted domains for the image sources. This closes #5333.

For reference: We discussed a use case where an Icinga Web module depends on features from an external provider, such as OpenStreetMap. Without the ability to modify the CSP header, every user of the module would need to adjust or override the web server configuration. A more effective approach we considered is to implement specific functionality in Icinga Web to modify only certain parts of the header. Additionally, I think such functionality would also be necessary for https://github.com/nbuchwitz/icingaweb2-module-map for example.

nilmerg

There are some style issues as well. Please rebase with main and the actions will run and point this out.

application/forms/Config/General/ApplicationConfigForm.php

library/Icinga/Util/Csp.php

nilmerg · 2025-05-14T08:16:06Z

I just noticed this breaks our own font loading, so all our icons are invisible since font-src: 'self' prohibits them. You can notice this in the menu pretty well for example.

In addition, I've already talked about this with you in person, I'd rather like to make this an automatic whitelisting. With the recent security release, we sandboxed iframes and automatically open them up in case a user configures a navigation item or dashboard with an external link. I'm thinking about the same with img-src and frame-src. So there could be a hook or something similar that would allow your module to announce that it requires images from a specific non-origin host and that gets automatically whitelisted using the desired policy.

So, I'm afraid, we need to re-think that. I also fear complications with child-src and connect-src. I'll promise we figure that out before the next major, so 2.13 will include something to resolve this.

…c scp header

nilmerg

Thanks for keeping at it!

I didn't look at it entirely yet. Only one thing for now:

The way you load navigation items is not correct yet. You forgot dashlets and you only load shared items the user is an owner of.

Take a look at \Icinga\Web\Navigation\Navigation::load for example. Though, please note that for dashlets a more complex solution is needed. Dashlets by modules can be loaded there (and should! Also modules may provide external URLs this way), but not user dashlets. This is done by \Icinga\Web\Widget\Dashboard::load instead.

Also, please remember that there are additional types of navigation items. A module can provide its own as well using \Icinga\Application\Modules\Module::provideNavigationItem. Monitoring's and Icinga DB Web's host and service actions are an example which may also result in an iframe.

So please restore the NavigationController and think about an alternative to the NavigationItemHelper.

cla-bot · 2025-08-06T15:15:25Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Davide Zeni.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email email@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2025-08-06T15:18:00Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Davide Zeni.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email email@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2025-08-06T15:48:02Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Davide Zeni.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email email@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2025-08-06T16:06:09Z

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @zenosaaur

Details

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.
If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

gianlucapiccolo · 2025-08-07T07:37:06Z

hello @nilmerg can you please give us a feedback on the changes we made? Thanks!!

…avigation items

cla-bot · 2025-08-25T11:48:06Z

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @zenosaaur

Details

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.
If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

cla-bot · 2025-09-02T09:37:02Z

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @zenosaaur

Details

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.
If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

Al2Klimov · 2026-03-06T10:17:33Z

library/Icinga/Application/Hook/CspDirectiveHook.php

+
+namespace Icinga\Application\Hook;
+
+abstract class CspDirectiveHook


As this is a new hook, add the new methods just like in #5433:

all()

register()

Implemented in b939340 (#5477)

nilmerg

Please rebase with main as the CI configuration changed.

Didn't test yet but it looks promising already!

v2.13 is on the horizon, so ensure to get back to it soon if you want this being part of it. Or drop a comment to let me delegate this further if you don't have the time. (But then allow collaborator access for us)

nilmerg · 2026-03-11T11:34:55Z

application/controllers/NavigationController.php

        $this->view->title = $navigation->getLabel();
    }
-}
+}


Please restore the empty line at the end of the file as well.

Changes to NavigationControllerhave been reverted in #5477

nilmerg · 2026-03-11T11:58:31Z

library/Icinga/Application/Hook/CspDirectiveHook.php

@@ -0,0 +1,18 @@
+<?php
+/* Icinga Web 2 | (c) 2022 Icinga GmbH | GPLv2+ */


Suggested change

/* Icinga Web 2 | (c) 2022 Icinga GmbH | GPLv2+ */

// SPDX-FileCopyrightText: 2026 Icinga GmbH <https://icinga.com>

// SPDX-License-Identifier: GPL-3.0-or-later

Neither I'd add GPLv3 files in a (yet) GPLv2 project (our corporate lawyer has left the chat 🙈), nor I'd mix license header formats in one and the same project. Latter especially because the GPLv3 PR (which doesn't even exist yet, by the way!) will likely use xargs to apply the same command to all files. The more files have the same header format, the better.

nilmerg · 2026-03-11T13:32:08Z

library/Icinga/Util/Csp.php

+        $user = Auth::getInstance()->getUser();
+        $menuItems = [];
+        if ($user === null) {
+            return $menuItems;
+        }
+        $navigationType = Navigation::getItemTypeConfiguration();
+        foreach ($navigationType as $type => $_) {
+            $config = Config::navigation($type, $user->getUsername());
+            $config->getConfigObject()->setKeyColumn('name');
+            foreach ($config->select() as $itemConfig) {
+                if ($itemConfig->get("target", "") !== "_blank") {
+                    $menuItems[] = ["name" => $itemConfig->get('name'), "url" => $itemConfig->get('url')];
+                }
+            }
+            $configShared = Config::navigation($type);
+            $configShared->getConfigObject()->setKeyColumn('name');
+            foreach ($configShared->select() as $itemConfig) {
+                if (Icinga::app()->hasAccessToSharedNavigationItem($itemConfig, $config) && $itemConfig->get("target", "") !== "_blank") {
+                    $menuItems[] = ["name" => $itemConfig->get('name'), "url" => $itemConfig->get('url')];
+                }
+            }
+        }
+        return $menuItems;
+    }


I was rather thinking about a solution like this: (Untested)

I didn't ensure this fits with your remaining code, so adjust it accordingly as you see fit.

Suggested change

$user = Auth::getInstance()->getUser();

$menuItems = [];

if ($user === null) {

return $menuItems;

}

$navigationType = Navigation::getItemTypeConfiguration();

foreach ($navigationType as $type => $_) {

$config = Config::navigation($type, $user->getUsername());

$config->getConfigObject()->setKeyColumn('name');

foreach ($config->select() as $itemConfig) {

if ($itemConfig->get("target", "") !== "_blank") {

$menuItems[] = ["name" => $itemConfig->get('name'), "url" => $itemConfig->get('url')];

}

}

$configShared = Config::navigation($type);

$configShared->getConfigObject()->setKeyColumn('name');

foreach ($configShared->select() as $itemConfig) {

if (Icinga::app()->hasAccessToSharedNavigationItem($itemConfig, $config) && $itemConfig->get("target", "") !== "_blank") {

$menuItems[] = ["name" => $itemConfig->get('name'), "url" => $itemConfig->get('url')];

}

}

}

return $menuItems;

}

if (! Auth::isAuthenticated()) {

return [];

}

$origins = [];

foreach (Navigation::load($type) as $navItem) {

foreach (self::yieldNavigation($navItem) as $name => $url) {

$origins[] = $url->getScheme() . '://' . $url->getHost();

}

}

return $origins;

}

protected static function yieldNavigation(NavigationItem $item): Generator

{

if ($item->hasChildren()) {

foreach ($item as $child) {

yield from self::yieldNavigation($child);

}

} else {

if ($item->getTarget() !== '_blank' && $item->getUrl()->isExternal()) {

yield $item->getName() => $item->getUrl();

}

}

}

Implemented your suggestion in #5477

nilmerg · 2026-03-11T14:02:03Z

library/Icinga/Application/Web.php

    }

-    private function hasAccessToSharedNavigationItem(&$config, Config $navConfig)
+    public function hasAccessToSharedNavigationItem(&$config, Config $navConfig)


I don't think that's needed if you use my proposal.

Yes. Reverted it in #5477

nilmerg · 2026-03-11T14:05:01Z

library/Icinga/Util/Csp.php

+            $errorSource = sprintf("Navigation item %s", $navigationItem['name']);
+
+            $host = parse_url($navigationItem["url"], PHP_URL_HOST);
+            // Make sure $url is actually valid;
+            if (filter_var($navigationItem["url"], FILTER_VALIDATE_URL) === false) {
+                Logger::debug("$errorSource: Skipping invalid url: $host");
+                continue;
+            }
+
+            $scheme = parse_url($navigationItem["url"], PHP_URL_SCHEME);
+

+            if ($host === null) {
+                continue;
+            }


The validation is already performed at least partially in the other methods. Please make sure to do this only once. Preferably during the fetch logic, as in my proposal.

lippserd · 2026-03-11T14:10:34Z

Please excuse me for not informing everyone, but I discussed with WP that we will take over this PR.

cla-bot bot added the cla/signed label Mar 20, 2025

w1ll-i-code force-pushed the 5333-allow-customization-of-the-csp branch 3 times, most recently from 168628c to 43c748e Compare March 20, 2025 11:13

lippserd requested a review from nilmerg May 13, 2025 14:30

nilmerg requested changes May 13, 2025

View reviewed changes

w1ll-i-code force-pushed the 5333-allow-customization-of-the-csp branch 3 times, most recently from 4ffe492 to d31bc33 Compare May 13, 2025 16:12

w1ll-i-code force-pushed the 5333-allow-customization-of-the-csp branch from d31bc33 to bbf35b5 Compare May 15, 2025 10:10

Allow modules to adjust the CSP headers through a dedicated hook.

2a9adc0

w1ll-i-code force-pushed the 5333-allow-customization-of-the-csp branch from bbf35b5 to 2a9adc0 Compare May 15, 2025 10:12

w1ll-i-code requested a review from nilmerg May 15, 2025 11:51

Add additional validation for the url before using it in the frame-sr…

f373d30

…c scp header

nilmerg requested changes May 21, 2025

View reviewed changes

cla-bot bot removed the cla/signed label Aug 6, 2025

zenosaaur force-pushed the 5333-allow-customization-of-the-csp branch from 3e68e02 to 47c9736 Compare August 6, 2025 15:48

zenosaaur force-pushed the 5333-allow-customization-of-the-csp branch from 47c9736 to 78bfc90 Compare August 6, 2025 16:06

Allow editing of the CSP trusted image sources

78bfc90

Refactor CSP validation logic and improve access control for shared n…

ecba402

…avigation items

Refactor CSP handling to improve user checks

c35cf02

Al2Klimov self-requested a review February 16, 2026 15:16

Al2Klimov mentioned this pull request Mar 4, 2026

Introduce \Icinga\Application\Hook\Essentials #5474

Open

Al2Klimov requested changes Mar 6, 2026

View reviewed changes

nilmerg requested changes Mar 11, 2026

View reviewed changes

TheSyscall mentioned this pull request Mar 12, 2026

Allow custom csp header #5477

Open


		namespace Icinga\Application\Hook;

		abstract class CspDirectiveHook

		@@ -0,0 +1,18 @@
		<?php
		/* Icinga Web 2 \| (c) 2022 Icinga GmbH \| GPLv2+ */

	/* Icinga Web 2 \| (c) 2022 Icinga GmbH \| GPLv2+ */
	// SPDX-FileCopyrightText: 2026 Icinga GmbH <https://icinga.com>
	// SPDX-License-Identifier: GPL-3.0-or-later

Conversation

w1ll-i-code commented Mar 20, 2025

Uh oh!

lippserd commented Mar 20, 2025

Uh oh!

nilmerg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nilmerg commented May 14, 2025

Uh oh!

nilmerg left a comment

Choose a reason for hiding this comment

Uh oh!

cla-bot bot commented Aug 6, 2025

Uh oh!

cla-bot bot commented Aug 6, 2025

Uh oh!

cla-bot bot commented Aug 6, 2025

Uh oh!

cla-bot bot commented Aug 6, 2025

Uh oh!

gianlucapiccolo commented Aug 7, 2025

Uh oh!

cla-bot bot commented Aug 25, 2025

Uh oh!

cla-bot bot commented Sep 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nilmerg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lippserd commented Mar 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants