Skip to content
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

Inconsistent X-Frame-Options Header #715

Closed
PascalAlbrechtComparis opened this issue Jun 8, 2023 · 4 comments
Closed

Inconsistent X-Frame-Options Header #715

PascalAlbrechtComparis opened this issue Jun 8, 2023 · 4 comments

Comments

@PascalAlbrechtComparis
Copy link

I noticed that the SecurityHeadersAttribute is setting X-Frame-Options to SAMEORIGIN:
https://github.com/DuendeSoftware/IdentityServer.Templates/blob/f1ebc1c3d408716e28869c68cbdb2f073c32ebff/src/IdentityServerEntityFramework/Pages/SecurityHeadersAttribute.cs#L26
However, based on the Content-Security-Policy containing frame-ancestors 'none', I would expect X-Frame-Options to be DENY:
https://github.com/DuendeSoftware/IdentityServer.Templates/blob/f1ebc1c3d408716e28869c68cbdb2f073c32ebff/src/IdentityServerEntityFramework/Pages/SecurityHeadersAttribute.cs#L30
Could you check if this is unintentional?

@brockallen brockallen transferred this issue from DuendeSoftware/IdentityServer.Templates Jun 8, 2023
@josephdecock
Copy link
Member

Thanks for bringing this to our attention, we'll investigate and update here.

@AndersAbel
Copy link
Member

@PascalAlbrechtComparis you are right, this is inconsistent. On a modern browser that supports CSP, the CSP frame-ancestors 'none' will take precedence and the more permissive X-Frame-Options: SAMEORIGIN will be ignored.

This code is part of our templates, which is meant to be exactly a template or starting point for a developer to customise the way they want. So it is perfectly fine for anyone finding these values inconsistent to change them in their own code created from the template.

But our template code should not be confusing in the first place, I will look into updating the templates to use DENY.

@AndersAbel
Copy link
Member

Proposed fix in DuendeSoftware/IdentityServer#1389

@PascalAlbrechtComparis
Copy link
Author

@AndersAbel Thank you for the clarification. We have changed this in our implementation already.

It certainly helps if the template is consistent in this regard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants