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

Django collectstatic uses --link which causes uWSGI security errors #186

Closed
ipmb opened this issue Mar 26, 2024 · 5 comments
Closed

Django collectstatic uses --link which causes uWSGI security errors #186

ipmb opened this issue Mar 26, 2024 · 5 comments
Labels
upstream Issues in upstream components

Comments

@ipmb
Copy link

ipmb commented Mar 26, 2024

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:

@ipmb
Copy link
Author

ipmb commented Mar 26, 2024

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

edmorley commented Mar 26, 2024

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:
unbit/uwsgi#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: Support disabling automatic Django static asset generation #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
Copy link
Author

ipmb commented Mar 26, 2024

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

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 :-)

@edmorley edmorley added the upstream Issues in upstream components label Aug 27, 2024
@edmorley edmorley changed the title Using --link in Django's collectstatic breaks some applications Django collectstatic uses --link which causes uWSGI security errors unless --static-safe is used Sep 7, 2024
@edmorley edmorley changed the title Django collectstatic uses --link which causes uWSGI security errors unless --static-safe is used Django collectstatic uses --link which causes uWSGI security errors Sep 7, 2024
@edmorley
Copy link
Member

edmorley commented Sep 7, 2024

Given that:

  • uWSGI isn't as commonly used as the other web servers (and the set of users using uWSGI + Django + static files will be even smaller than that)
  • this is arguably a uWSGI limitation/rough edge that could/should be fixed upstream (since uWSGI could chose to not call realpath, and still have the security benefits of path checking without causing this issue)
  • there is a fairly easy end-user workaround (adding --static-safe)
  • any solution on the buildpack side would mean worsening the experience for all Django users that don't use uWSGI

...then I think we should wontfix this for now (we can always revisit in the future if needed).

I appreciate you reporting this though - it's good for me to be aware of cases like this, and at least the workaround is now documented in an issue that others might find when searching :-)

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

No branches or pull requests

2 participants