heroku / buildpacks-python

Heroku's Cloud Native Buildpack for Python applications.
BSD 3-Clause "New" or "Revised" License
26 stars 2 forks source link

Using --link in Django's collectstatic breaks some applications #186

Open ipmb opened 5 months ago

ipmb commented 5 months ago

We have a Django application running with uwsgi and using --static-map to serve the static files.

When upgrading to the CNB buildpack, this functionality broke. We're seeing this error in the logs:

[26/Mar/2024:09:24:24 -0700] - [uwsgi-fileserve] security error: /workspace/project/static/vendor/bootstrap/bootstrap.min.css is not under /workspace/project/staticfiles or a safe path

This is due to using --link in the collectstatic command:

https://github.com/heroku/buildpacks-python/blob/617a3cf78af4d74b9ab4521c12db0070a50b0e79/src/django.rs#L46

ipmb commented 5 months ago

uwsgi provides a --static-safe option to mark paths as safe, but due to the way staticfiles work in Django, it would essentially require marking everything safe which is less than ideal:

--static-safe /layers/heroku_python/dependencies/lib --static-safe /workspace/project
edmorley commented 5 months ago

Thank you for filing an issue!

That error message originates from: https://github.com/unbit/uwsgi/blob/39f3ade88c88693f643e70ecf6c36f9b375f00a2/core/static.c#L643

It looks like uWSGI is calling realpath on the paths before checking that the paths exist within the static directories, which is a bit unfortunate, since it breaks the symlink case. It feels like uWSGI could instead normalise the path (to remove tricks like ../../) to maintain the same security benefits without that limitation.

I checked to see if there was an upstream issue about this already but couldn't see one - want to file one? :-)

That said, looking at the uWSGI repo it doesn't feel like they are keeping up with maintenance given the number of open issues and PRs. I also didn't get a rely to this bug report: https://github.com/unbit/uwsgi/issues/2525

From the buildpack's side, the options are:

  1. Stop using --link entirely. However, this would mean everyone (whether they use uWSGI or not) has a larger app image.
  2. Stop enabling --link by default, but add a buildpack option to opt into enabling it. Downside being everyone (whether they use uWSGI or not) has a larger app image by default, and has to know to opt into the smaller image, plus there being more options to document, maintain and larger matrix of things to test.
  3. Continue using --link by default, but add a buildpack option to opt out of using it. Downside being that uWSGI users need to discover the option even exists, plus there more options to document, maintain and larger matrix of things to test.
  4. Continue using --link, but support disabling the automatic collectstatic feature entirely (which is something we may want to do anyway: #109). Apps that encounter issues with uWSGI can then disable automatic collectstatic and instead run it via an inline buildpack command like so:

    [[io.buildpacks.group]]
    id = "my-app/collectstatic"
    script = { api = "0.10", inline = "./manage.py collectstatic" }
  5. Leave the buildpack implementation as-is, and instead document that uWSGI users should instead set --static-safe via either the Procfile command or in their uWSGI config file.

For getting a sense uWSGI's relative popularity, PyPI stats report monthly downloads as:

However, we don't know what percentage of those uWSGI downloads relate to Django users, or users that might try CNBs.

Out of curiosity, what made you pick uWSGI? It's not a web server I've seen anyone recommend recently, and my sense was that it was waning in popularity.

ipmb commented 5 months ago

It's not well maintained and is waning in popularity. At the time it was chosen for this project that wasn't the case and there has been no reason to switch to something else. It does have some unique options that aren't available with other servers.

Option 4 seems reasonable (but also raises the same issue as 2). I'm still holding out that there will be some post-build command supported by the buildpack instead of needing to resort to inline buildpacks 😄

I assumed this wouldn't get resolved in the buildpack, but wanted to document the issue somewhere. I will probably recommend folks switch to whitenoise for static file hosting which will circumvent the issue.

edmorley commented 5 months ago

Thank you for the extra context. I'll have more of a think about this before picking an option - either way, this issue existing will help others in a similar situation in the meantime :-)