bookwyrm-social / bookwyrm

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

Wrong CSS paths with S3 object storage (while non-CSS assets paths are correct) #3383

Open lo48576 opened 3 weeks ago

lo48576 commented 3 weeks ago

Description

CSS path is not correct (lacks /static prefix) when using S3-compatible object storage. Paths for other resource files (such as images) looks correct.

For example, /about page contains <link href="https://my-bookwyrm-bucket.s3.ap-northeast-1.example.com/css/themes/bookwyrm-light.css" rel="stylesheet" type="text/css" /> (sensitive parts replaced) while /static/css/themes/bookwyrm-light.css is expected.

Steps to Reproduce

  1. Configure and use S3
    • USE_S3=true and related AWS_* configs
    • Should be unrelated but just in case: DEBUG=false, USE_HTTPS=true, MEDIA_ROOT=images/, ENABLE_THUMBNAIL_GENERATION=true, and AWS_S3_URL_PROTOCOL="https:".
  2. Update to v0.7.4 as usual
    • In my case, from v0.7.3
    • (In case I had it done wrong: the previous of my v0.7.3 installation was v0.7.2)
  3. Access to any pages
    • (Of course non-flower pages)
  4. You should get the broken pages (due to the wrong CSS URL)

Expected behavior

CSS paths are generated as https://....com/static/css/... and the rendering of pages are not broken.

Screenshots

Applied a monkeypatch so no screenshot, sorry... I'll attach a patch below.

Instance

https://bookwyrm.nops.red (already patched)

Additional context

I investigated the code and I believe I've found the cause in django-sass-processor 1.4:

https://github.com/jrief/django-sass-processor/blob/39a479e4ac48d8ed5e8f808508bd7cf790aa0c3d/sass_processor/storage.py#L13-L18

As you see, OPTIONS (storage_options) are taken as sass_processor_storage.get("OPTIONS") or {} where sass_processor_storage = settings.STORAGES.get("sass_processor", {}). This makes sass_src template macro ignore OPTIONS for staticfiles.

This can be worked around with the redundant STORAGES config for sass_processor:

diff --git a/bookwyrm/settings.py b/bookwyrm/settings.py
index c075c9c87..6da6f4bae 100644
--- a/bookwyrm/settings.py
+++ b/bookwyrm/settings.py
@@ -404,6 +404,13 @@ if USE_S3:
                 "default_acl": "public-read",
             },
         },
+        "sass_processor": {
+            "BACKEND": "storages.backends.s3.S3Storage",
+            "OPTIONS": {
+                "location": "static",
+                "default_acl": "public-read",
+            },
+        },
         "exports": {
             "BACKEND": "storages.backends.s3.S3Storage",
             "OPTIONS": {

With this patch applied (on my instance), the problem seems to be fixed and everything is working as expected. I didn't (can't) test other object storage backends, so I don't know whether non-S3 backends are affected or not.


Desktop (please complete the following information): (should be unrelated)

lo48576 commented 3 weeks ago

I'm not familiar with the ecosystems of Python and/or Django, so I identified the cause but I'm not sure how to describe this problem and which component is responsible to fix this and how.

If this should be fixed on sass-processor side, could you escalate this to them please?

joachimesque commented 3 weeks ago

I can confirm the problem on my instance (boitam.eu) and can confirm that your fix works. I don’t have the time/energy to go deeper into that. Perhaps you could open a pull request with that diff?

lo48576 commented 3 weeks ago

Created PR #3384.

oculos commented 3 days ago

It seems that my browser still complains that this file can't be loaded: shepherd.min.js.map While the fix seems to have worked for the css files, it still complains about that one.

lo48576 commented 3 days ago

It seems that my browser still complains that this file can't be loaded: shepherd.min.js.map

Should that file really be served in the first place? I visited some (unbroken) instances from https://joinbookwyrm.com/instances/ and then opened shepherd.minejs from the JS debuggers of the browser, but none of them served the file. (The responses were 404 for some and 403 for others, but it would be due to the difference of access control settings at the static file backend, so no real difference.)

The source map file is something that just helps the browser and developer to inspect the script if available, so it would be requested even when no one told the file is there. Imagine favicon.ico, the browser will request it (and get 404 in not few cases) even when no one provided the link to it.

Additionally, the paths of source map files won't be provided from SASS processor, so I believe it should be a (possibly unrelated) separate issue from this one if it is really an issue.

oculos commented 3 days ago

It seems that my browser still complains that this file can't be loaded: shepherd.min.js.map

Should that file really be served in the first place? I visited some (unbroken) instances from https://joinbookwyrm.com/instances/ and then opened shepherd.minejs from the JS debuggers of the browser, but none of them served the file. (The responses were 404 for some and 403 for others, but it would be due to the difference of access control settings at the static file backend, so no real difference.)

The source map file is something that just helps the browser and developer to inspect the script if available, so it would be requested even when no one told the file is there. Imagine favicon.ico, the browser will request it (and get 404 in not few cases) even when no one provided the link to it.

Additionally, the paths of source map files won't be provided from SASS processor, so I believe it should be a (possibly unrelated) separate issue from this one if it is really an issue.

I imagined that. I don't see any lack of functionality regarding that file. I just thought it was worth mentioning. Great job that you found a fix for the static files, btw!