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

Clarification about nginx configuration introduced at 0.7.2 #3391

Open
oculos opened this issue Jul 5, 2024 · 7 comments
Open

Clarification about nginx configuration introduced at 0.7.2 #3391

oculos opened this issue Jul 5, 2024 · 7 comments

Comments

@oculos
Copy link

oculos commented Jul 5, 2024

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
Copy link
Contributor

hughrun commented Jul 15, 2024

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
Copy link
Author

oculos commented Jul 16, 2024

@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
Copy link
Author

oculos commented Aug 13, 2024

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://<AWS_S3_CUSTOM_DOMAIN:>/images/conver/....
My question is if it should be changed or if it should be reading from somwhere else.

@oculos
Copy link
Author

oculos commented Aug 13, 2024

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://<AWS_S3_CUSTOM_DOMAIN:>/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:

  • if we should apply the fix for both images and static
  • if a similar change needs to be applied when using S3, especially when using AWS_S3_CUSTOM_DOMAIN.

Any thoughts here @mouse-reeve ?

@hughrun
Copy link
Contributor

hughrun commented Aug 13, 2024

@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
Copy link
Author

oculos commented Aug 13, 2024

@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
Copy link
Contributor

hughrun commented Aug 13, 2024

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.

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

No branches or pull requests

2 participants