MEDIA_ROOT defaulting to empty string: isn't that dangerous ?

Hey !

(I hope this isn’t a duplicate, but didn’t find much discussion about this neither in issues nor here)

Isn’t defaulting to an empty string for MEDIA_ROOT dangerous ?

Out of the box, if you just add a “FileField” to your model and don’t define MEDIA_ROOT in you settings, files are uploaded to the current working directory (which in most cases would be your source directory). Also, if you serve media filesin such a setup, you’ll be leaking your source code.

In amateur deployments (writable source dir, hardcoded config, etc.) that could easily become very serious. Usually Django is super helpful in helping devs no to shoot themsleves in the foot.

I would suggest:

  • to default MEDIA_ROOT to None instead of “”, so that functions like django.views.static.serve fail
  • to add a check MEDIA_ROOT != current directory, in the same way there is a check that MEDIA_ROOT != STATIC_ROOT (not sure if we could go further - is there a way to determine whether a MEDIA_ROOT is safe ?)

IMO these would be reasonably backwards compatible, anyone using media files should definitely have set they MEDIA_ROOT setting.

Let me know what you think ! Happy to open an issue if there seem to be some consensus.

Note: see CRITICAL: fallback to MEDIA_ROOT should be clearly documented to prevent leaking the source folder · Issue #65 · kimetrica/django-binary-database-files · GitHub which is where I stumbled upon this (TLDR: didn’t know that app also serves files from MEDIA_ROOT, which I hadn’t configured, then realised i was leaking my source files)

I’m in favour of cleaning this up. At the very least I think we could add a check to flag this and then people can silence it if they really want the current behaviour. Though if we change this, people can always explicitly set MEDIA_ROOT to "".

1 Like

Let me know what you think ! Happy to open an issue if there seem to
be some consensus.

I vote for addressing this. Maybe with checks and warnings in a first
release and then later changing MEDIA_ROOT to a default that requires a
developer to explicitly and consciously changing it to make file
transfers work.