tubesync icon indicating copy to clipboard operation
tubesync copied to clipboard

Fix headers when using another proxy with HTTPS

Open controlol opened this issue 3 years ago • 3 comments

Fixes #112

Also includes a fix for domains not matching any CSRF _TRUSTED_ORIGINS with updated readme

Improves readme grammar. This requires some attention by the @meeb because there were some things I simply did not understand. These lines are commented and I suggest they are changed or left out before merging.

Also properly outputs Nginx logs to a file and link the output to /dev/stdout. Nginx runs as daemon and is properly stopped by S6-overlay. A message where TubeSync can be found is printed after the container is live.

Catch requests exception to prevent the stacktrace to be printed. And always include localhost-like in ALLOWED_HOSTS to prevent proxy (the internal Nginx process) and healthcheck errors.

controlol avatar Dec 07 '21 15:12 controlol

Hi, thanks for the PR. Your suggestion for passing on upstream X-Forwarded-Proto headers to account for upstream TLS termination looks reasonable.

I'll look into the rest of these more closely over the weekend. Many of the extra commits seem to be pretty cosmetic. Some initial thoughts:

  • I explicitly wanted the requests exception (if one was raised by the healthcheck) to output details to the container log
  • I personally am not a fan of running services as daemons in containers (even when using s6) but that's probably more of a philosophical difference.
  • I am absolutely not convinced on injecting localhost and friends into CSRF_TRUSTED_ORIGINS and adding a warning to the README.
  • I would also disagree with some of your README changes which I do not feel are improvements.

I'll certainly merge any typos or improvements. Thanks for taking the time.

meeb avatar Dec 07 '21 17:12 meeb

As a solution to point one and two I would suggest to let your python code prints less informative messages and only print errors if there are any. Ending with a message where (a url) the user can connect to the website. If you do that Nginx doesn't have to be a daemon, which indeed is preferred in a docker environment. In the above scenario you can revert all changes made in the following files:

  • config/root/etc/cont-finish.d/nginx.sh
  • config/root/etc/services.d/nginx/run
  • tubesync/healthcheck.py

For point three, I found a solution which I will commit to this PR. I am not familiar with Django (or Python in general), but I think this is how it should have been done.

And you are right about the readme. I never expected you to include all the changes, but I hope you keep at least the headers in the installation section.

controlol avatar Dec 08 '21 10:12 controlol

I'm attempting to review this, but I really don't want to merge all of these commits and then have to manually undo more than half of them. Per-commit comments:

  • This is fine, I don't particularly care but if you want to submit a PR to fix spacing sure go for it: https://github.com/meeb/tubesync/pull/190/commits/02b41dae0b592df427229ea2a2e936d19b332e29
  • These comments were for others who might view the file, they are obvious, but verbosity isn't always bad: https://github.com/meeb/tubesync/pull/190/commits/4444367d67228ded11459e78d4a92ba826db1f5f
  • This is fine: https://github.com/meeb/tubesync/pull/190/commits/c7a281c78e8cf8d00a66f25554fb01eb9fb314ca
  • I don't see the point of this and it contains a typo: https://github.com/meeb/tubesync/pull/190/commits/fe7baed7f96077f8f945756d7c2422839898e309
  • Won't merge: https://github.com/meeb/tubesync/pull/190/commits/4856e55d6cb21f38a1ba4bdd478116fc4af2520f https://github.com/meeb/tubesync/pull/190/commits/f83a2063c3d5887f9bc83cf253991fc9494c4034
  • I'd prefer to keep nginx as a foreground process, won't merge: https://github.com/meeb/tubesync/pull/190/commits/402a38ab57db9b31591f6a2a92bf773c2b09af81 https://github.com/meeb/tubesync/pull/190/commits/cd86e91f5bd8565efa820dec1ceeda298c099062 https://github.com/meeb/tubesync/pull/190/commits/ca7bd75a721c1a6b9418ba699153a302cce1fea9 https://github.com/meeb/tubesync/pull/190/commits/bf2cc9204ad600486b4a6ae3523cca4c8d8c47d8
  • Won't merge, introduces an extra container layer and isn't needed, I believe this was just for very old docker installs: https://github.com/meeb/tubesync/pull/190/commits/7e7f0521aedee0cae41cc840219614feef6edaa6
  • I'm not entirely sure I understand why https://github.com/meeb/tubesync/pull/190/commits/8965cf9e29dc8292e58d19126ce5169a582ac8ad https://github.com/meeb/tubesync/pull/190/commits/f9223e0badc8fe87b930ca5c48ca2a70d254f3f8 might be desired. The forms have CSRF tokens already so validating the origin additionally is... fine but probably not needed. If the point here was to always mandate that ALLOWED_HOSTS should always contain 127.0.0.1 and localhost then that's probably not a good way to do this.
  • This is fine, my typo: https://github.com/meeb/tubesync/pull/190/commits/bcc448842844baa8308c473b6b7c075a5db26c5b
  • Not needed pending the ALLOWED_HOSTS changes being updated: https://github.com/meeb/tubesync/pull/190/commits/69a6586c753ec1968edf6178a4a7ad15c76634df
  • Personal preference, but sure don't mind: https://github.com/meeb/tubesync/pull/190/commits/5e8c9fe53e8001bc01f60da5dbb4d7f54fa2bb04
  • I can see how some of my existing README text could be confusing and I'm happy to update it but I'm not sure about some of your suggestions in https://github.com/meeb/tubesync/pull/190/commits/07e2217f232b58aea5b8faf67a481563d696e1d9 and https://github.com/meeb/tubesync/pull/190/commits/47b1b2faf4bb82805e05ddca24662dbfca6e6c02 - I'll probably not merge these, but just re-write parts of it you've highlighted as possibly confusing again.
  • This is fine, my typo: https://github.com/meeb/tubesync/pull/190/commits/8c77a06e41933e25d8920094b31c7027478f7545
  • Redis is indeed bundled by design, this is part of the future requirement of switching to celery for the task queue and I'm just doing it in stages. You are correct that Redis is not currently being used in the container at the moment but there's no need to merge: https://github.com/meeb/tubesync/pull/190/commits/c37bbadc6107a9923fa2356f92be9cf65fcae6e0
  • Note that with https://github.com/meeb/tubesync/pull/190/commits/95b77765c73e33768904c0cfe280aee1e5e1d6f9 some people do actually run TubeSync in a container and bypass nginx (by exposing the WSGI server directly). This change would break that. ALLOWED_HOSTS is required to be editable by env vars.

In summary, If you could please submit a PR with the following changes I'd be happy to merge them:

https://github.com/meeb/tubesync/pull/190/commits/02b41dae0b592df427229ea2a2e936d19b332e29 https://github.com/meeb/tubesync/pull/190/commits/4444367d67228ded11459e78d4a92ba826db1f5f https://github.com/meeb/tubesync/pull/190/commits/c7a281c78e8cf8d00a66f25554fb01eb9fb314ca https://github.com/meeb/tubesync/pull/190/commits/bcc448842844baa8308c473b6b7c075a5db26c5b https://github.com/meeb/tubesync/pull/190/commits/5e8c9fe53e8001bc01f60da5dbb4d7f54fa2bb04 https://github.com/meeb/tubesync/pull/190/commits/8c77a06e41933e25d8920094b31c7027478f7545

Thanks again for taking the time to suggest these. If you can't submit a PR with just the above commits in I can replicate them easily enough directly as well.

Once merged, I'll rewrite the parts of the README you've highlighted as confusing.

meeb avatar Dec 20 '21 05:12 meeb