Opened 9 years ago

Last modified 8 years ago

#5328 in_progress enhancement

White/blacklisting mimetypes in attachments and check file size

Reported by: molgrum Owned by: molgrum
Priority: major Milestone:
Component: programming Keywords: whitelist, blacklist, mimetype, mimetypes, attachment, attachments
Cc: Parent Tickets:


I have working (non-elegant) code working for doing a white/blacklist of mimetypes. Also checks max_file_size parameter!

Attachments (2)

Change History (11)

comment:1 by molgrum, 9 years ago

The non-elegant part is the javascript code that does an alert() instead of adding HTML.

comment:2 by molgrum, 9 years ago

Status: newreview

comment:3 by molgrum, 9 years ago

Just FYI, you should apply both patches in the order they were submitted. Sorry for the inconvenience but I'm fresh at GIT!

comment:4 by molgrum, 9 years ago

To add multi-line parameters into mediagoblin.ini (which you probably want to do when listing many mimetypes), read this:

comment:5 by Christopher Allan Webber, 8 years ago

Milestone: 0.9.0

The general idea here looks good; I agree that we should provide a good way to blacklist/whitelist based on mimetype. Nonetheless, this needs some work:

  • A lot of divergences from PEP-8, please read up!
  • UNSAFE_MIMETYPES / SAFE_MIMETYPES defined, then immediately has its value replaced unconditionally. So why not just define it once each?
  • not sure what bMimeAllowed means or does?
  • The body of the conditionals for whitelist/blacklist appears identical, so maybe you could group the conditional check to something like:
   if (whitelist_check_here() OR blacklist_check_here()):
  • tests are needed, I think

I think I need to wrap my head around the JS changes a bit more, but that's on me, not you.

Doing a check in JS before we hit the server is a great idea to reduce waiting on a big upload that doesn't go through!

comment:6 by Christopher Allan Webber, 8 years ago

Milestone: 0.9.0

comment:7 by Loic Dachary, 8 years ago

Type: defectenhancement

comment:8 by Loic Dachary, 8 years ago

bin/py.test --boxed --cov-report=term-missing --cov=mediagoblin mediagoblin/tests

shows that the test are not broken (which is good ;-) but also shows that the tests do not cover the modified files:

mediagoblin/edit/ 217 87 60% 59, 79, 96, 107-111, 116-214, 232-233, 243, 256, 258-260, 291-300, 313-333, 360-365, 375-419, 432, 461, 478, 493, 538-539

comment:9 by Loic Dachary, 8 years ago

Owner: set to molgrum
Status: reviewin_progress
Note: See TracTickets for help on using tickets.