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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lewiscollard
Copy link

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

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

@adi-
Copy link
Member

adi- commented Nov 5, 2023

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

  • is setting False as default a good idea? That could brake functionality of currently run applications but higher the security for new ones.
  • docs needs a statement that few middlewares for this app are mandatory.

@lewiscollard
Copy link
Author

lewiscollard commented Nov 9, 2023

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 91d5349.

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

Successfully merging this pull request may close these issues.

2 participants