search

neutronX / django-markdownx

Comprehensive Markdown plugin built for Django
https://neutronx.github.io/django-markdownx/
Other
863 stars 153 forks source link

Don't allow anonymous users to upload files by poking the upload endpoint directly #182

Open lewiscollard opened 4 years ago

lewiscollard commented 4 years ago

By default, it is possible for anyone, authenticated or not, to upload files to the server by poking the markdownx_upload endpoint directly. You can verify this by opening the admin page for any model that has a MarkdownxField, opening the "Log out" admin link in a new tab, then dragging and dropping an image in your old tab. The file will successfully be uploaded, even though that request would have been made unauthenticated.

This might be intentional, but it is surprising behaviour for someone that has added MarkdownX so that they can get a Markdown editor in their admin. I don't know if that is the majority case, but I suspect it's a common enough one that it's probably a bad idea to allow unauthenticated uploads by default - you increase your attack surface and you become a free image host to anyone that knows how to use curl.

Instead, this PR adds a MARKDOWNX_UPLOAD_ALLOW_ANONYMOUS setting, which defaults to False. That means that anyone who does really want to allow anonymous uploads (I could see a use case in, e.g., blog comments) can have it, but the default out-of-the-box setting is safe.

Noise: The MIDDLEWARE noise in this PR is to make testing request.user work in a unit test environment. I also removed some ancient compatibility cruft from the tests.py while I was there.

Apologies for not including the generated docs in this; if I did (possibly because of different mkdocs versions) it would have included far too much auto-generated noise. If I could have some hints for generating them without this noise I would be thankful :)

lewiscollard commented 1 year ago

Rebased on master, conflicts fixed. Is this feature considered worthwhile?

adi- commented 1 year ago

Yeah, thats a good idea to secure this. However, some things needs to be cleared:

lewiscollard commented 1 year ago

is setting False as default a good idea? That could brake functionality of currently run applications but higher the security for new ones.

I don't like breaking anyone's applications, but it's less bad than the alternative, which is to have surprising behaviour by default.

(I wonder if it would be best to not have a default. That way it'll break loudly and immediately, rather than quietly until someone tries to upload an image, for anyone who is using the anonymous uploads feature.)

docs needs a statement that few middlewares for this app are mandatory.

Actually, it only requires that request.user is set, which is available via django.contrib.auth.middleware.AuthenticationMiddleware - which everyone likely already has (the other middleware added was to make it work in a test environment). If they're allowing anonymous uploads then request.user will never be checked as the first clause in the if statement added to markdownx/views.ImageUploadView.dispatch will short-circuit the second one. That fact is documented in 91d5349b2458.