-
-
Notifications
You must be signed in to change notification settings - Fork 772
Content security policy headers (CSP) #6752
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
base: develop
Are you sure you want to change the base?
Conversation
|
With this, how exactly does the configured process in the persona bar work with the fact that anyone could manipulate this via other methods in the skin, module, etc? Is the portal done first and then changes applied? How is the users input validated in the personarbar validated? is it serialized to an object with detailed validation errors? |
Yes, the portal settings are applied first in the OnInit of the page (default.aspx).
The library containt a policy parser. But actully no validation error is showed to the user that enter the settings. |
|
@sachatrauwaen: can you add a switch for the admin that governs whether a module can enlarge the CSP scope or not? That way the admin decides what is allowed and knows what is going to happen. |
In reality there 2 ways to add policies to CSP : 1) site settings 2) api (used by the core, modules, skins, editor providers and potentialy all other extensions). The site settings where actually for define the default strict policy, policy for content (images, iframe, css) and policy for modules that not automatically add there policy. When we want to add a "switch for the admin that governs whether a module can enlarge the CSP scope or not" it is for all policies added by api not only modules. So we add only the policy from the site settings. |
# Conflicts: # DNN Platform/Website/Default.aspx.cs
…rge the CSP scope or not
DNN Platform/DotNetNuke.ContentSecurityPolicy/SourceCspContributor.cs
Outdated
Show resolved
Hide resolved
DNN Platform/DotNetNuke.ContentSecurityPolicy/ContentSecurityPolicy.cs
Outdated
Show resolved
Hide resolved
DNN Platform/DotNetNuke.ContentSecurityPolicy/CspDirectiveType.cs
Outdated
Show resolved
Hide resolved
DNN Platform/DotNetNuke.ContentSecurityPolicy/CspParsingExample.cs
Outdated
Show resolved
Hide resolved
DNN Platform/DotNetNuke.ContentSecurityPolicy/CspPolicyExample.cs
Outdated
Show resolved
Hide resolved
| if (this.nonce == null) | ||
| { | ||
| var nonceBytes = new byte[32]; | ||
| var generator = System.Security.Cryptography.RandomNumberGenerator.Create(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, RandomNumberGenerator is a class that implements IDisposable, it should at minimum be within a using to prevent resource leaking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i add using
| } | ||
|
|
||
| // Basic domain validation | ||
| var domainRegex = new Regex(@"^(https?://)?([a-zA-Z0-9-]+\.)+[a-zA-Z]{2,}(:\d+)?(/.*)?$"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am no expert in CSP but I think this regex might be wrong and maybe not optimal for performance too. I believe HostSource should support extensionless domains like localhost and wildcards like *.somecdn.com or simply *, domains with long TLDs like something.technology, IP and port, IPV6, etc. So it that should be a supported scenario I would love that we have some tests for a list of those that should be supported.
Maybe we could allow some of CSP specific scenarios like "*" and similar, then run Uri.TryCreate(value) to validate the more normal ones and if it still does not pass, we can do IPAddress.TryParse(value)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, this is in a hot path for execution, compiling this regex at minimum should be done for performance, and limit processing time as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I change it.
But I was thinking that all the regexp validation is maybe overkill. And because it is evaluated on each request , its maybe better to remove it.
What do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think given it is executed on EVERY request, I'd be very inclined to NOT do this validation and test that it was correct, or find a way to validate at a different stage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i remove the syntax checks from all request. They only be done on site settings save.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still see it in this file, do you mean that this only happens when saving settings for this specific place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you are looking at the outdated file?
Normally you see the constructor with the checkSyntax parameter.
public CspSource(CspSourceType type, string value = null, bool checkSyntax = false)
By default no syntax check is done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's right
| /// </summary> | ||
| private void ValidatePluginTypes(string value) | ||
| { | ||
| string[] validPluginTypes = { "application/pdf", "image/svg+xml" }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct to hardcode only those mime-types, I am not sure but should this allow other types like word/excel, etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified
DNN Platform/DotNetNuke.ContentSecurityPolicy/IContentSecurityPolicy.cs
Outdated
Show resolved
Hide resolved
DNN Platform/DotNetNuke.ContentSecurityPolicy/ReportingCspContributor.cs
Outdated
Show resolved
Hide resolved
DNN Platform/DotNetNuke.ContentSecurityPolicy/ReportingCspContributor.cs
Outdated
Show resolved
Hide resolved
DNN Platform/DotNetNuke.ContentSecurityPolicy/ReportingEndpointContributor.cs
Outdated
Show resolved
Hide resolved
DNN Platform/DotNetNuke.ContentSecurityPolicy/ReportingEndpointContributor.cs
Outdated
Show resolved
Hide resolved
DNN Platform/DotNetNuke.ContentSecurityPolicy/ReportingEndpointContributor.cs
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,16 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
DNN Platform/Website/Default.aspx.cs
Outdated
| // set global page settings | ||
| this.InitializePage(); | ||
|
|
||
| if (!this.PortalSettings.CspHeaderFixed && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code for when CspHeaderFixed is true is pretty far from another block of code that handles when it is false. In am not sure if it needs to be that way because there is an important order or operations, but if not, can we possibly bring those together in a private method and improve the readability of this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, for the common part
DNN Platform/DotNetNuke.ContentSecurityPolicy/ContentSecurityPolicyParser.cs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| // Basic domain validation | ||
| var domainRegex = new Regex(@"^(https?://)?([a-zA-Z0-9-]+\.)+[a-zA-Z]{2,}(:\d+)?(/.*)?$"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, this is in a hot path for execution, compiling this regex at minimum should be done for performance, and limit processing time as well
DNN Platform/DotNetNuke.ContentSecurityPolicy/DocumentCspContributor.cs
Outdated
Show resolved
Hide resolved
DNN Platform/DotNetNuke.ContentSecurityPolicy/ReportingCspContributor.cs
Outdated
Show resolved
Hide resolved
enhance mine type validation replace french doc with english replace regexp for url validation with Uri.TryCreate cleanup
syntax check is only done when saving site settings
A couple of additions
uniformize auto csp headers for locked or not locked
| <value>The changes to the CSP settings will be discarded. Do you wish to continue?</value> | ||
| </data> | ||
| <data name="plCspHeaderFixed.Text" xml:space="preserve"> | ||
| <value>Lock CSP Header</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thowing out an idea here, I think Lock is better than others, but still not sure it is fully understood by others, some thoughts after checking with my team
- Disallow Module/Theme/System Overrides
- Strict CSP Header
I think the first is the most "proper" as the name explains but its wordy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strict : can sound as strict in a security point of view (that is not wath we want to tell, most of the time it will be less secure because it need to contain policy for all use cases)
Alternative can be : Disallow Extensions overrides (wich is also more correct, because all extesions are disallowed and now the framework will override to avoid crash of dnn)


Summary
Create a possibility to generate Content security policy (csp) headers .
Content Security Policy is a crucial security standard that helps protect your web applications from various types of attacks, including Cross-Site Scripting (XSS), clickjacking, and other code injection attacks. It works by allowing you to specify which resources (scripts, styles, images, etc.) your browser should be allowed to load.
more info about Content security policy
Description of solution
The intend is to manage CSP for WebForms and MVC pipeline.
Webforms can not be very struct in the policy that can be used. Here script-src 'unsafe-inline' and 'unsafe-eval' will be automatically added to csp. It is not added in the settings because it's specific for webforms.
In the future MVC pipeline can be very strict. Here no js evaluation will be used and all inline javascript will be marked with a nonce.
This is the default csp for the setting
default-src 'self'; script-src 'self' 'report-sample'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; font-src 'self'; object-src 'none'; base-uri 'none'; form-action 'self'; frame-ancestors 'none'; frame-src 'self'; connect-src 'self';3 http headers will be managed : Content-Security-Policy, Content-Security-Policy-Report-Only and Reporting-Endpoints
Persona bar - Security settings
Implementation
It commes in a new project for csp management and a test project.
The IContentSecurityPolicy service that can be used with DI had all the stuff for skin and module developers to contribute to the policy.
For webforms skin developers actually the way to contribute is
Details of DotNetNuke.ContentSecurityPolicy library
The
DotNetNuke.ContentSecurityPolicylibrary provides a fluent API for building and emitting Content Security Policy (CSP) headers in DNN. TheIContentSecurityPolicyinterface is the main entry point to compose directives, manage sources, configure reporting, and generate final header strings.Interface:
IContentSecurityPolicyNamespace:
DotNetNuke.ContentSecurityPolicyProperties
SourceCspContributorfordefault-src.SourceCspContributorforscript-src.SourceCspContributorforstyle-src.SourceCspContributorforimg-src.SourceCspContributorforconnect-src.SourceCspContributorforfont-src.SourceCspContributorforobject-src.SourceCspContributorformedia-src.SourceCspContributorforframe-src.SourceCspContributorforframe-ancestors.SourceCspContributorforform-action.SourceCspContributorforbase-uri.Methods
Inline,Self,Nonce).plugin-types(e.g.,application/pdf).sandboxoptions (e.g.,allow-scripts allow-same-origin).form-actionsource.frame-ancestorssource.report-togroup name to the policy.IContentSecurityPolicyfor chaining.Content-Security-Policyheader value.upgrade-insecure-requestsdirective.Working with sources
Directive properties expose a
SourceCspContributor, which supports adding/removing sources such as:AddSelf()→'self'AddNone()→'none'AddInline()→'unsafe-inline'AddEval()→'unsafe-eval'AddStrictDynamic()→'strict-dynamic'AddNonce(string)→'nonce-<value>'AddHash(string)→'sha256-...','sha384-...','sha512-...'AddHost(string)→example.com,https://cdn.example.comAddScheme(string)→https:,data:,blob:RemoveSources(CspSourceType)to remove by typeSee:
CspSourceType.cs,CspSource.cs,SourceCspContributor.cs.Usage examples
Configure a baseline policy with a nonce
Parse and merge an existing CSP header
Remove an unsafe source
Notes
Noncein your inline tags:<script nonce="{policy.Nonce}">.AddHeadersis useful to import settings from configuration and extend them programmatically.Update 22 october 2025
There are 2 point of views :
The site settings are there for the site admin to enforce some policies. In this case, the policy need to include all requirements (from skins, modules, skin object, content, ...) and all pages (not only dnn pages but also edit urls need to be included in this policies to get them work properly). Actually this can already done with a the web.config setting. Or you need different policies for different use cases/ user profils (Anonymous, loged in, page admin, module editor, host, ...).
(The point of view of this PR) The site settings are there for defining a starting policy where skins, modules, ... can add dynamically there policy. So the policy will be adjusted automatically to make the site work.
What is sure is that modules and skins will paticipate in the ability to make the policies strict.
Knowing witch policies are required by a skin or modules is key even what kind of content is permited, if you want to be strict.
Know that browsers include also a way to report csp policy violations to the browser console and to a endpoint.
Fixes #6720