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

Add X-Frame-Options header to .htaccess #16179

Merged
merged 4 commits into from
Aug 11, 2019
Merged

Conversation

J0WI
Copy link
Contributor

@J0WI J0WI commented Jul 1, 2019

This addresses the some major points of #8207.

This should not change the behavior of a normal request to NextCloud. But it changes the following:

BeforeAfter
X-Frame-Options is only sent by PHP responsesX-Frame-Options is sent on all responses
security headers are only sent on 2xx responsessecurity headers (except dynamically generated CSP) are sent on all responses
If the X-Frame-Options header is added to the nginx example, it's sent twice for PHP responsesThe X-Frame-Options header should be added to the nginx example

This should not affect installations without modHeadersAvailable.

@kesselb kesselb added this to the Nextcloud 17 milestone Jul 1, 2019
@kesselb kesselb added 3. to review Waiting for reviews enhancement pending documentation This pull request needs an associated documentation update labels Jul 1, 2019
@kesselb
Copy link
Contributor

kesselb commented Jul 1, 2019

There is a setupcheck that tests x-frame-options so nginx users should see a warning after upgrade. We should add some hint to release notes about that.

@J0WI
Copy link
Contributor Author

J0WI commented Jul 2, 2019

#4605 (comment)
I'd suggest they shouldn't use iframes or use a dedicated endpoint with a suitable .htaccess/nginx config
But I can't see a benefit if some apps are allowed to change the policy for the whole NextCloud.

@kesselb kesselb requested a review from georgehrke July 2, 2019 10:18
@kesselb
Copy link
Contributor

kesselb commented Jul 2, 2019

#4605 (comment)
I'd suggest they shouldn't use iframes or use a dedicated endpoint with a suitable .htaccess/nginx config
But I can't see a benefit if some apps are allowed to change the policy for the whole NextCloud.

I see the point of the comment but how should an app overwrite the x-frame-options with the current code?

cc @georgehrke because some calendar sharing/embedding.

@J0WI
Copy link
Contributor Author

J0WI commented Jul 2, 2019

I think meanwhile this is done by the frame-ancestors CSP, but I didn't found a source for this.

@J0WI J0WI removed the pending documentation This pull request needs an associated documentation update label Jul 3, 2019
@J0WI
Copy link
Contributor Author

J0WI commented Jul 4, 2019

Docs are already merged.

@MorrisJobke MorrisJobke mentioned this pull request Jul 15, 2019
28 tasks
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me. Altough my apache fu is rather limited.

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, also seems to work fine to still set different headers on the Nextcloud side. 👍

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, also seems to work fine to still set different headers on the Nextcloud side. 👍

Signed-off-by: J0WI <J0WI@users.noreply.github.com>
Signed-off-by: J0WI <J0WI@users.noreply.github.com>
Signed-off-by: J0WI <J0WI@users.noreply.github.com>
Signed-off-by: J0WI <J0WI@users.noreply.github.com>
@rullzer rullzer merged commit 9d6eb2d into nextcloud:master Aug 11, 2019
@welcome
Copy link

welcome bot commented Aug 11, 2019

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22
Most developers hang out on IRC. So join #nextcloud-dev on Freenode for a chat!

@J0WI J0WI deleted the mv-frameoptions branch August 11, 2019 20:13
MichaIng added a commit to MichaIng/DietPi that referenced this pull request Aug 24, 2019
+ DietPi-Software | Nextcloud: Add X-Frame-Options header to Nginx config, which is now required webserver-config wise. Switch to "always" send headers and update some regex to match official docs: nextcloud/server#16179, https://docs.nextcloud.com/server/latest/admin_manual/installation/nginx.html
+ DietPi-Software | Nextcloud: Remove HSTS header completely from the config, since this is simply the wrong place. It is and should be added to the vhost configs when HTTPS is enabled.
@MichaIng
Copy link
Member

@J0WI
Might I asked for the reason to have these headers sent as well with non 2xx responses? If the request fails due to permissions, missing file or is redirected, then those headers have no practical effect, do they? For an attacker they at best provide some additional information about the webserver config and the webserver has a tiny bid more to do for 30x/40x answers.

@J0WI
Copy link
Contributor Author

J0WI commented Aug 25, 2019

Might I asked for the reason to have these headers sent as well with non 2xx responses?

Why would you disable X-Frame-Options, XSS protection or any other security header on non 2xx responses?

For an attacker they at best provide some additional information about the webserver config

Security through obscurity is no good practice. Admins should just keep their services secure.

and the webserver has a tiny bid more to do for 30x/40x answers.

I don't think that this change has a measurable effect on performance.

@MichaIng
Copy link
Member

MichaIng commented Aug 25, 2019

@J0WI

Why would you disable X-Frame-Options, XSS protection or any other security header on non 2xx responses?

As said, because it does not have any practical effect that I am aware of, security-wise. No content is sent by the webserver with 30x/40x answers, so nothing can be done by the client where any of the headers could kick in.

Security through obscurity is no good practice. Admins should just keep their services secure.

Of course, but sending out information for no reason is as well no good practice.

I don't think that this change has a measurable effect on performance.

I agree, however I did not say that there are measurable or strong reasons to remove "always", I was just wondering what was your reason to explicitly do add it. As of above, I just see disadvantages, probably not measurable ones or deal-breakers, but I think you added it for a certain reason, any practical advantage that comes with it, and this is what I was asking for 😉.

IMO the question, before changing something, should be "why" and not "why not" 😉?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants