Allow modifying existing CspBuilder directives

[mryandot]

Our team maintains a Razor Component Library for our web template which allows us to share a common look and feel, dependencies, etc. We also include some common extension methods to help with configuring applications. I'd like to add this library to that template and include some useful extension methods to make it easier to configure custom policies, specifically the Content-Security-Policy. I just don't see a good way to add items to an existing CspBuilder directive since it wipes out the previous policy. Without the ability to grab existing policies and add onto them, we're pretty much stuck with documenting it and having each developer do it themselves, and if anything changes each web application would need to be updated.

This is not a duplicate of #250 since it was dealing with the AddSecurityHeaderPolicies method specifically. I didn't see any code in there that would fix my issue here.

Some examples of extension methods would be:

• EnableTemplate() to add our template-specific requirements like ImageSrc().Self().Data().
• EnableBrowserLink() to add the connect-src items for Visual Studio's Browser Link feature when debugging.
• EnableGoogleAnalytics() to track the defaults for using Google Tag Manager which can be a bit hairy.

Another reason we'd like this is to allow us to use Google Analytics with a nonce. Right now we have to use unsafe-inline, use custom hashes which need maintained, or use our own middleware which would be redundant with this.

There are several ways I could work around it, but the most likely method is reflection; other options seem even worse. Just documenting it is also an option, but not only do we have issues trying to get some developers to read documentation, it's easy to miss a project when they all use their own custom code to do the same thing. Other ideas like our own custom builder would require cloning about half the project into ours which would seem to me to defeat the purpose.

I'm more than willing to work on a PR if you have a suggestion on how you'd like it integrated; I'm assuming you wouldn't want me to change the behavior of the AddDirective methods. The easiest alternative I can think of would be to add a bool useExisting = false parameter to all the directive methods. All the other options include duplicating a bunch of methods.

Other options would be a bit more invasive. Right now the AddDirective<T> method is private, so adding a public AddOrGetDirective<T> or TryGetExisting<TDirective> method seems questionable. You could potentially add a public Merge method that would allow you to merge it with another CspBuilder, but that still only helps to a point since there's no way to get the existing CspBuilder out of the ContentSecurityPolicyHeader either.

One way or another I also just want to give you huge thanks for the library. Even if I do end up just documenting things, your library will make it easier for my developers than just pointing them at MDN. I'm glad I ran across an NDC talk the plugged it; I'd somehow missed it on your blog, and my searches were focused specifically on the Content-Security-Policy which didn't seem to include it, at least near the top.

[andrewlock]

Hi @mryandot ! Thanks for trying out the library and the kind words. I'm actually on holiday at the moment, so will take a proper look when I'm back, but I have a suspicion that this could be quite tricky, as you suspected. 🤔 The original design of the library was intentionally simplistic and doesn't cater well to these scenarios.

One aspect that comes to mind is that you can configure multiple header policies (of which the CSP is just over). We'd need to consider how developers select the correct policy to modify in your scenario, as well as making it mutable.

I'll have a think and a look and will see what we can do when I'm back!

[mryandot]

Enjoy your holiday!

I just wanted to update you while I'm working on it. Just to see what I actually needed, I went ahead and (hopefully) temporarily implemented using reflection. All I needed was access to the CspBuilder._directives private dictionary and the CspDirectiveBuilderBase.Directive internal property so I could add new policies if they didn't exist. I then wrapped them in a GetOrAddDirective<T> method with a flag to reset the policy if it was set to 'none'. I also included a HasDirective<T> method so I could more easily add a default policy only if it didn't already have one.

That worked for me in my narrow use case because I didn't need to worry about any of the special policies like AddCustomDirective since I wasn't using any of them. I could also ignore source builder directives since WithNonce already checked for itself in the SourceBuildersCollection.

I would love to see something implemented that wouldn't need me to reflect in for safety/stability, but the more I look at it, the more I'm stumped on a clean and simple way to include it in the API that doesn't involve duplicating, or adding a flag to, all the various Add methods. Either way, thanks for looking at it.

[andrewlock]

Hey, I'm back now and was thinking about this and the short answer is you're right, it's kind of a pain 😅 The original API surface wasn't really designed with updating in mind 🤔 I think the simplest "fix" would be to add the TryGetExisting<TDirective> option that you suggested. But I'm wondering if that's the most future-proof solution 🤔 It seems like I may need to do a major version bump (due to #267), so I'm wondering if there's anything else to consider as well 🤔

Would TryGetExisting<TDirective>() be sufficient do you think? 🤔 Generalising it to support Customdirective would probably require passing the expected directive in I guess e.g. TryGetExisting<TDirective>(string directive)

[mryandot]

It would encapsulate enough to let me do what I need right now. Assuming you're planning to return a directive or a null, HasDirective is just an is not null check, and I could do what I'm doing now with GetOrAddDirective using something like:

var d = cspBuilder.TryGetExistng<ScriptSourceDirectiveBuilder>() ?? cspBuilder.AddScriptSrc();
if (d.BlockResources) d.BlockResources = false;

The only down side is I'd have to write specific extension methods for each directive, or add a switch expression in the method since knowing ScriptSourceDirectiveBuilder is added with AddScriptSrc() isn't easily determined without reflection. Since each call to GetOrAddDirective would be replaced by one or two lines, I don't see that as an issue, but thought I would mention it in case it triggers other thoughts.

Since your other issue is about reducing the range/damage of foot-guns, it might also be worth bringing up the fact that calling .None() on any policy is allowed with others like .From(), but when it generates, only the .None() is honored. That's probably the right choice from a safety/security perspective, and previously something like that would likely have been caught in a code review, but this change would potentially move those calls far enough apart to miss. It might be worth throwing an exception if they try to add other sources when BlockResources is true, or vice versa. You could also just throw when you build the policy but that would make it a bit more work to track down the offending policy call.

[andrewlock]

The only down side is I'd have to write specific extension methods for each directive, or add a switch expression in the method since knowing ScriptSourceDirectiveBuilder is added with AddScriptSrc() isn't easily determined without reflection

Would you need to know that? 🤔 I would assume that in each of your helpers you would know what you need to add and could call the correct method?

It might be worth throwing an exception if they try to add other sources when BlockResources is true, or vice versa. You could also just throw when you build the policy but that would make it a bit more work to track down the offending policy call.

Yeah, it's something else that bugs me about the API today tbh. I was actually considering an alternative way of handling it solely through typing, so that you can call From() or None() on the original directive, but after calling None() you can't call From(), and vice versa. The trouble is, that would probably require a lot of API changes and be generally a bit more cumbersome to use so I'm not sure about it.. it would make it much harder to use incorrectly though, so maybe it's worth it 🤷‍♂️

Since your other issue is about reducing the range/damage of foot-guns

Just FYI, another possibility I'm toying with is doing this entirely through analyzers for now to avoid having to make breaking changes

[mryandot]

Would you need to know that?

No; like I said, it's a one- or two-line replacement so I don't see it as an issue; just thought I'd mention it, and probably shouldn't have. 🫢

I was actually considering an alternative way of handling it solely through typing

I like the idea in principal, and I think it fits well with the original design, but it would complicate TryGetExisting; you would have to have a way to get the right type for the current state of the existing policy. All I can think of off the top of my head would be to either call it with each possible builder type we could get back until it matches, or calling it with a base/marker type, then use a pattern matching switch expression the handle each possibility. Both would be a little clunky, but it's also an edge case, so I would gladly accept that over reflection into private implementation details. I definitely wouldn't want this request to cause problems for the design of the main use case.

Analyzers would also be a great solution, though they could still miss bad policy combinations if they're not all applied at once and/or in the same method.

All of it depends on how much time you're willing to invest vs. what it's fair to expect developers to know when implementing security policies. After all, there are many valid security policies that still aren't good, so some knowledge and understanding will always be required, even if the API tries to hold your hand most of the way. 😄