Mimetype attachment restriction

Development
#1

Hello all,

I am currently working on the integration of mimetype restriction when manipulating attachments.
See the design page.

The goal is allow admins to define allow/block lists of mimetypes at space level (similarly to what is currently done for file size), and to introduce the corresponding check on the UI.

I’ll summarize below the Implementation section of the design page as it contains points for which I’d like to have your opinion.

xwiki.properties

attachment.mimetype.allowlist=
attachment.mimetype.blocklist=

XClass

Attachment.Code.AttachmentMimetypeRestrictionClass

Two text fields:

  • allowedMimetypes
  • blockedMimetypes

Initialized using a mandatory document initializer in xwiki-platform-attachment-api.

Prevent non-admin users to update Attachment.Code.AttachmentMimetypeRestrictionClass XObject

Same as for rights, (see org.xwiki.security.authorization.internal.RightsFilterListener). A listener listen for document creation and update, and check whether the user performing the action is allowed to edit XWikiRights XObject (i.e., is an admin).

I propose to do the same for mime type restrictions XObjects.

Prevent non-admins users to edit the Attachment.Code.AttachmentMimetypeRestrictionClass document

I propose to allow the edit right to the XWiki.XWikiAdminGroup in the document initializer, so that this rights is disabled for every other users.

Prevent Attachment.Code.AttachmentMimetypeRestrictionClass value inconsistencies

It could be possible for admins to be less restrictive on a given space than it parent (and it is easy to become Admins of a space without being globally admin).

  • Option A1: no restrictions
  • Option A2: always be more restrictive than the parent space
  • Option A3: always be more restrictive than a given XObject (configurable?)
  • Option A4: always be more restrictive than the xwiki.properties

The same is true for file size restrictions.

Prevent inconsistencies on attachment move

Currently, no check is done on move, which means that you can upload a large file in a space, and move it in a space where it wouldn’t be allowed.

Without further controls, the same will be true for mimetype restrictions.

  • Option B1: no restrictions
  • Option B2: add a control to check if the move attachment passes the restriction check on the target space and raise an error if it does not.

Note that option 2 might lead to a situation where an attachment cannot be moved or renamed anymore if the restrictions of its space are made stricter after upload.

Summary

Do you agree on:

  • the xwiki.properties key names
  • the location and name of the new XClass

What options do you think are best regarding:

  • Attachment.Code.AttachmentMimetypeRestrictionClass value inconsistencies (Ax)
  • inconsistencies on attachment move (Bx)

Do you agree on my proposed solution to prevent non-admin users to:

  • update Attachment.Code.AttachmentMimetypeRestrictionClass XObject?
  • edit the Attachment.Code.AttachmentMimetypeRestrictionClass document

Thanks

1 Like
#2

+1

+1 except:

those should be Static List IMO

IMO A4 is probably best option: I’m a bit afraid that A2 creates problem for Admins to configure their wiki. But I do believe that configurations provided at farm level should always be respected.

I do think we should have B2 (and same for size restrictions)

Actually you could have a check when modifying the value to only allow it if all existing attachments are respecting it. Now that’s probably too expensive for a config provided at wiki level. In any case, when the move is performed by an admin we could imagine that you only get a warning but don’t prevent doing it.

+1

1 Like
#3

I’m currently using static list fields to store the allowed and blocked attachments mimetypes. Then, each value is used as a regex which is then matched again the mimetype of the attachment when they are validated.
While powerful, using regex has several issues:

  1. The values are used both server-side as Java regex, and client-side as Javascript regex. While they overlap a lot, the two regex languages are not strictly the same and the could lead to some issues
  2. Regex are very powerful, but can lead to performance issues (for instance, a user defining a complex regex), and not all admins know how to use them.

I propose the make things slightly less powerful and to instead use '*' as a joker. So instead of defining image/.*, users would define image/*.

This is slightly less powerful, but I believe it would make things easier for users while also improving the maintainability and stability of our codebase.

WDYT?

#4

* wildcard sounds enough to me for this use case

#5

Same for me.