Skip to content

Conversation

@gpoole
Copy link

@gpoole gpoole commented Feb 9, 2020

The Nginx config for the proxied FPM images does not set HTTPS as enabled by default. This caused an issue for me in the client login screen where clicking "Accept" would seem to time out. Chrome shows an error in the console like Refused to send form data to 'http://cloud.domainname/' because it violates the following Content Security Policy directive: "form-action 'self'".

This change will pass the original request protocol via the FastCGI HTTPS param, so I think it should cover both HTTP and HTTPS.

The details of the error and suggested fix are described in nextcloud/server#17409.

Signed-off-by: Greg Poole <m4dm4n@gmail.com>
@J0WI
Copy link
Contributor

J0WI commented Jun 5, 2020

@J0WI J0WI closed this Jun 5, 2020
@gpoole
Copy link
Author

gpoole commented Jun 5, 2020 via email

@J0WI
Copy link
Contributor

J0WI commented Jun 5, 2020

There's an ENV for it. We use the nginx config proposed by upstream.

@gpoole
Copy link
Author

gpoole commented Jun 5, 2020

In the upstream config you've linked to HTTPS is turned on with fastcgi_param HTTPS on, whereas in the examples in this repo, which I've updated in this PR, it's commented out.

@J0WI
Copy link
Contributor

J0WI commented Jun 5, 2020

That's because the example is without https.

@gpoole
Copy link
Author

gpoole commented Jun 6, 2020

The example I linked to is using the letsencrypt companion to terminate HTTPS, it is using HTTPS.

@gpoole
Copy link
Author

gpoole commented Jun 7, 2020

I'm not saying this was intentional on your part, but as a first time contributor to this project your approach to this PR came across as a bit dismissive. I don't mean to waste your time arguing over trivial changes and I completely understand if you don't want to merge my change - it's your decision - but I put a small amount of time and effort into sharing a change I thought would be helpful with the community, so I would have appreciated a quick note in your closing like "thanks for the suggestion, we don't want to merge the change because we try to keep the configuration as close to the upstream one as possible and this is too different" in your original response. From my perspective it's difficult to know what your motivation for closing the PR is unless you explain it, so it also helps me figure out what to do or not do in the future or maybe learn something useful.

@J0WI
Copy link
Contributor

J0WI commented Jun 8, 2020

The example I linked to is using the letsencrypt companion to terminate HTTPS, it is using HTTPS.

In the upstream config you've linked to HTTPS is turned on with fastcgi_param HTTPS on, whereas in the examples in this repo, which I've updated in this PR, it's commented out.

You're right, something is a bit obscure here. I closed a bunch of issues that seems to be obsolete since #1048 and I'm sorry if I was to fast with this one. I think this change is not required if you're using the ENV to tell Nextcloud to use HTTPS. Maybe fastcgi_param HTTPS $https if_not_empty; is more suitable for a generic example.

If I'm wrong or if there are common cases where this config is still useful, I'd like to ask you to open a PR in the documenation repo first.

@gpoole
Copy link
Author

gpoole commented Jun 9, 2020

That's fair enough, I see what you're saying. Thanks for hearing me out and taking the time to respond in detail, I appreciate it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants