Feedback version 1, small changes to default

[damienbod]

Hi @andrewlock I think it's great. I have two small changes I think would help.

I think the default UI headers should be removed because without a good CSP, the security is still very weak and gives a false sense of security.

https://github.com/andrewlock/NetEscapades.AspNetCore.SecurityHeaders/blob/main/src/NetEscapades.AspNetCore.SecurityHeaders/HeaderPolicyCollectionExtensions.cs#L40

I would like to add the following to the default API security headers:

  .AddReferrerPolicyStrictOriginWhenCrossOrigin()
  .AddCrossOriginOpenerPolicy(builder => builder.SameOrigin())
  .AddCrossOriginEmbedderPolicy(builder => builder.RequireCorp())
  .AddCrossOriginResourcePolicy(builder => builder.SameOrigin())

I can make a PR if you think either of these ideas are good.

Greetings Damien

[andrewlock]

Thanks for the feedback @damienbod!

I think the default UI headers should be removed because without a good CSP, the security is still very weak and gives a false sense of security.

I totally understand the sentiment here, and it's an option I considered. In the end I decided against it because:

1. It's a big breaking change - anyone updating their app will almost certainly break it.
2. The security of the default headers is definitely not as good as it can be (weak CSP) but it's still better than if you apply nothing, and it's hard to know what you "should" apply.
3. With the current design, you get the "default" headers. That was a big ask from multiple people early in the library, we would have to lose that.

One thing I considered was logging a warning at runtime when you use the default headers without a more strict CSP. That gets quite tricky to do well though, especially with the new design where you can change things at runtime.

Maybe a simple "solution" would be an analyzer that detects when you call AddDefaultSecurityHeaders() and if you don't add a CSP it adds a warning 🤔 I'm not sure there's a "good" solution, but I'm also not sure removing the default headers is a good option. Like I say, I totally understand your position - it's better to force people to think about it properly - I'm just not sure that will have the desired effect, and instead makes the library less "usable"

I would like to add the following to the default API security headers:

For the referrer, we're already adding no-referrer to the default API headers, which I think is a stricter (and better) option?

In terms of COOP etc, I didn't add those mostly because they're not in the recommended OWASP headers for REST responses.

Also, the OWASP recommendations about the headers say:

As this headers are very related to browsers, it may not make sense to be applied to REST APIs or clients that are not browsers.

That said, that applies to other HTML-only headers they do recommend (like Referrer-Policy etc), as a defence in depth approach, so that seems like it may just be an oversight on their part? 🤔

In their HTTP recommendations, they suggest the following:

  .AddCrossOriginOpenerPolicy(builder => builder.SameOrigin())
  .AddCrossOriginEmbedderPolicy(builder => builder.RequireCorp())
  .AddCrossOriginResourcePolicy(builder => builder.SameSite()) // 👈 Note not SameOrigin()

Do you have any thoughts on the same-site vs same-origin for CORP? 🤔 OWASP suggest the more lenient option, but given that these don't apply to APIs (in theory) I'm inclined to go with your suggestion.

[damienbod]

@andrewlock Sound good to me. I would prefer the SameOrigin() which is more strict, if you want to weaken the security header configuration, then this is an active choice. For the default APIs, I don't see any reason not for doing this.

But I understand when you want to stay with the suggested OWASP recommendations.

Greetings Damien

[andrewlock]

I would prefer the SameOrigin() which is more strict, if you want to weaken the security header configuration, then this is an active choice. For the default APIs, I don't see any reason not for doing this.

Yeah, you're right, I agree, I can't see a reason for making this same-site for an API, thanks 👍

[andrewlock]

Fixed in #204