bookwyrm-social / bookwyrm

Social reading and reviewing, decentralized with ActivityPub
http://joinbookwyrm.com/
Other
2.22k stars 259 forks source link

Clarification about nginx configuration introduced at 0.7.2 #3391

Open oculos opened 2 months ago

oculos commented 2 months ago

Hi,

On version 0.7.2, there were introduced some changes on the production.conf nginx file.

However, it is very unclear how those changes affect those users that use reverse proxies or who do not run a docker instance - or those, like me, who have it on kubernetes.

On the page with the reverse proxy instructions, for example, production.conf is not even mentioned. Should something be done on the default.conf or nginx.conf?

hughrun commented 1 month ago

Hi @oculos

The change was made to prevent access to any non-image files from the /images directory. This was to close access to any user download files created with the earliest version of account exports (later this was changed so that they are not saved to /images). I would recommend equivalent changes for any reverse-proxy setup.

Given the small team involved in BookWyrm development, it's tricky to keep the docs and code updated for all situations. Development is primarily focussed on Docker deployments, and documentation for other deployments is contributed from the community.

I created an issue at #3279 noting that the docs need to be updated. I authored the change in 0.7.2 but didn't do a great job as I'm very far from an nginx expert. As you can see, we're looking for help to fix this as the reverse-proxy docs are quite out of date.

oculos commented 1 month ago

@hughrun I can try to help here. I'll see if I have any file created there that would fall on that case and see if I can block it on the reverse proxy.

oculos commented 3 weeks ago

Hi agian @hughrun . I wanted to give a look into this, but I realized one thing: When using S3, images are still retrieved from the images directory.

So we need to clarify something else here as well: Do we have to introduce a similar change when using AWS_S3_CUSTOM_DOMAIN? That is, on the domain used to store media? I see on my istance that images are still read from the https:///images/conver/.... My question is if it should be changed or if it should be reading from somwhere else.

oculos commented 3 weeks ago

Hi agian @hughrun . I wanted to give a look into this, but I realized one thing: When using S3, images are still retrieved from the images directory.

So we need to clarify something else here as well: Do we have to introduce a similar change when using AWS_S3_CUSTOM_DOMAIN? That is, on the domain used to store media? I see on my istance that images are still read from the https:///images/conver/.... My question is if it should be changed or if it should be reading from somwhere else.

And the explanation at https://github.com/bookwyrm-social/bookwyrm/releases/tag/v0.7.2 is very confusing. It is asking to look at the line 99 of the production configuration file, suggesting that both /static/ and /images/ should be changed. However, only the block concerning /images/ seemed to have the suggested configuration.

So we need some clarification as to:

Any thoughts here @mouse-reeve ?

hughrun commented 3 weeks ago

@oculos Thanks for looking at this.

Images should always be stored in /images.

The change I made was an attempt to stop anything that is not an image file being accessed from that directory.

Subsequent fixes now essentially make this a legacy issue because we never store user exports there any more, but the problem was that in theory one could initially access user exports from that directory without any authentication.

oculos commented 3 weeks ago

@oculos Thanks for looking at this.

Images should always be stored in /images.

The change I made was an attempt to stop anything that is not an image file being accessed from that directory.

Subsequent fixes now essentially make this a legacy issue because we never store user exports there any more, but the problem was that in theory one could initially access user exports from that directory without any authentication.

I see.

The fix should most likely be somewhere here:

# Reverse-Proxy server
 server {
     listen [::]:8001;
     listen 8001;

     server_name your-domain.com www.your-domain.com;

     location / {
         proxy_pass http://web;
         proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
         proxy_set_header Host $host;
         proxy_redirect off;
     }

     location /images/ {
         alias /app/images/;
     }

     location /static/ {
         alias /app/static/;
     }
}

But I am still not sure how to deal with it.

hughrun commented 3 weeks ago

If I remember correctly the confusion is that they were originally one rule but when I changed it I split it into two rules. Maybe? I did mess it up a few times.

In any case it is only really relevant to restrict file types allowed from the /images directory - static doesn't matter.