-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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_header X-Frame-Options SAMEORIGIN is added somewhere in NextCloud 13 #8207
Comments
Actually I found this: |
If I remove this code, then my Nginx works, but it places dual headers and it senses that my config is wrong. |
|
We don't support that this is set by the web server. The Nginx should not add this header. Fro your point in the call route we are not able to detect if it will be added by the web server or not and thus we add it there because this then needs fewer tweaking of the webserver itself. This is thus not a bug but an enhancement to also support web servers that somehow set it. |
Some headers are set in PHP, some are set in .htaccess/nginx. This inconsistency is really bad. I found them on three different locations: server/lib/private/legacy/response.php Lines 251 to 272 in da6c2c9
https://github.com/nextcloud/documentation/blob/b7dec9a1cab21cf637914073b096eeda44608f1c/admin_manual/installation/nginx.rst#nextcloud-in-the-webroot-of-nginx https://github.com/nextcloud/documentation/blob/b7dec9a1cab21cf637914073b096eeda44608f1c/admin_manual/configuration_server/harden_server.rst#serve-security-related-headers-by-the-web-server Things are even worse if you run NextCloud behind a reverse proxy. The headers in the PHP file are not really customizable. |
@MorrisJobke Yeah, it says add it in NGINX, then you tell me to remove from NGINX. so which one we should use??? I think server settings is better, what do you think? |
Right now it is chaos. |
@J0WI yeah, exactly, what i mean, you cought it! |
From a security perspective it also makes more scene to add security settings in the server config, which is harder to hijack in the case of a possible vulnerability in the PHP app. Edit: And the webserver is also able to set the security headers if PHP is broken for whatever reason. |
@J0WI Server is better, but if you want to use PHP, you should comment properly, i think, but of course, we cannot document, and code and fix and etc at once. So understand. I will remove the NGINX settings and I purely set it up for PHP, that's the best right? Like add |
@p3x-robot you can also use
|
well, i just change my server, removed that and that's it. :) it works, awesome. THANKS! |
The biggest problem with that is that also there are users, that can't configure this in the webserver because they use a shared hoster. We try to make it as easy as possible for everybody :/ And sorry for the mess. |
Could you open a ticket (or even better a PR) in the documentation repo to give hints for this setting in the web server, @J0WI? |
@MorrisJobke i just uncommeted that i hacked: |
@MorrisJobke shared hoster tend to allow |
See issue described in nextcloud/server#8207
Is there a way for a php script to test if the headers are set by the webserver? So that php only adds them if they are not set via webserver. We could enable them by default and provide a configuration switch in case you can activate it on the webserver (the developers probably won't like to add another configuration option). |
Not really, because the web server adds them after PHP send out the final response ... so it is more up to the web server to check if it is already set.
That would be the only option to handle this properly. We could even detect this in JS and give the admin that hint. |
You could compare if the headers are only set on PHP reposes or also on static resources. But that's again just a hacky workaround. |
So what's the plan here? A new config option for future versions (NC 14+)? How do we handle this in NC 13, remove the additional headers in the nginx-config from documentation? |
Not sure if this is related or an entire new issue: server/lib/private/legacy/response.php Lines 100 to 103 in 1b074f4
Normally, this line should avoid running into the case shown above: Line 21 in 9d6eb2d
But the environment variable can't be retrieved by Maybe someone who has more experience with Apache / PHP-CGI can tell whether this might be a solution for at least some of the issues discussed here? |
I don't see this with Apache/2.4.25 and PHP-FPM 7.2.21. Are you using mod_cgi? You are aware that this is super slow. |
@kesselb phpinfo says "CGI/FastCGI". If I change it to FPM, neither |
@Harmageddon mind to create a new issue? It seems to be a bit off topic. |
Ok, so I've re-read this a bunch o' times, and STILL have no clue how to fix this ... atm, with NC18b (18.0.0.4), I'm getting reports of
Adding
to my nginx config appears to have NO effect. IIUC, there's a fix somewhere up there ^? I prefer to keep headers in my webserver config -- particularly since it's a proxy in front of a number of various services, including NC. What exactly is required to make these errors go away? EDIT:
None have any effect -- EXCEPT
DOES remove its respective error ... |
@J0WI yep, long ago |
I also cannot reproduce this on my phpfpm setup although I have seen this in the past as well. Could anyone of you share the output of a info.php file with the following content that is added to your webroot and opened in the browser:
|
There also seems to be a clear_env configuration flag for phpfpm, could you also check the value of that? https://www.php.net/manual/en/install.fpm.configuration.php |
for phpinfo(), here,
and,
|
@juliushaertl did you need add'l/other info? |
Is this issue still relevant? The code in question #8207 (comment) has been changed with Nextcloud 17. To any response (from nextcloud) the So if anyone asks "how can I disable the security headers set by nextcloud" the answer is: SetEnv modHeadersAvailable true for apache2 or fastcgi_param modHeadersAvailable true; for nginx. I've just tested both ways and they work for me. Any objections? |
here, with clean install of NC tag
checking
in Nginx config
after login, nav to
still reports
So, clearly, still getting these ^^ Have I missed something in my config? |
How should I know? ;) Feel free to visit help.nextcloud.com and share your configuration. Also make sure that the headers are not sent twice like the other report. With nextcloud 17 the What do you think @juliushaertl @J0WI? |
@pgnd
Also did you check which headers actually arrive at the client (and how often)? |
@MichaIng this is still not correct. I have added the headers as per https://docs.nextcloud.com/server/latest/admin_manual/installation/nginx.html , and also set:
Yet I still get double headers!
Is there a setting we can call that will stop the PHP script from sending its own headers? Cos if I comment out ALL the add_header parts from my nginx config, it works fine! (which means ownCloud itself is adding them!) |
Here the headers are set: server/lib/private/legacy/OC_Response.php Lines 98 to 106 in b219ead
Clearly only if |
@MichaIng Thanks for the reply . It looks like it needed adding into the |
Ah yes that is an alternative, although it is loaded likely by other web applications as well, which do not require it. The community Nginx config contains:
which loads this file. If you add |
Steps to reproduce
In the whole
/etc/nginx
it is only just one place where is:Expected behaviour
On my server it is right and every web sites, like this:
Actual behaviour
But in
NextCloud 13
somewhere it adds in itself and because it adds 2 times for sure and is not NGINX or PHP, because I can show you many pictures, that is right:Actually, I only use in
/etc/nginx
so it not anywhere, I look for, and I only there inNGINX
:So only once!
Server configuration
Operating system:
Linux server 4.12.0-2-amd64 #1 SMP Debian 4.12.13-1 (2017-09-19) x86_64 GNU/Linux
Web server:
nginx/1.13.8
Database:
mariadb Ver 15.1 Distrib 10.1.29-MariaDB, for debian-linux-gnu (x86_64) using readline 5.2
PHP version:
PHP 7.2.1-1 (cli) (built: Jan 5 2018 11:21:04) ( NTS )
Nextcloud version: (see Nextcloud admin page)
13
The text was updated successfully, but these errors were encountered: