Edgewall Software
Modify

Opened 12 years ago

Last modified 12 years ago

#9281 new enhancement

Allow IAttachmentManipulator to easily check on the uploaded attachment's file data

Reported by: Carsten Klein <carsten.klein@…> Owned by:
Priority: normal Milestone: next-major-releases
Component: attachment Version: 0.12dev
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

Currently, both the filename and the file data is not available to the manipulator, unless it would reimplement part of the logic found in AttachmentModule#_do_save.

_do_save already does some verification on the uploaded file, which I presume is present in some temporary location on the local filesystem.

My proposal would be to also pass in the upload file object associated with that file to the manipulator so that it is able to verify its contents, for example scanning it with an antivirus scanner and so on.

See also ticket #9280.

Attachments (0)

Change History (4)

comment:1 by Remy Blank, 12 years ago

Milestone: 0.12next-major-0.1X

Please don't set the milestone arbitrarily. Better leave it empty, we'll take care of it.

comment:2 by Carsten Klein <carsten.klein@…>, 12 years ago

Will do.

I was not sure whether the unbound method from IAttachmentManipulator could be changed so that it will feature all of the required parameters to the validate_attachment method, hence I simply included it with the doc string.

Here is a proposed patch OTOH and untested:

  • attachment.py

     
    3434from trac.perm import PermissionError, IPermissionPolicy
    3535from trac.resource import *
    3636from trac.search import search_to_sql, shorten_result
    37 from trac.util import get_reporter_id, create_unique_file
     37from trac.util import get_reporter_id, create_unique_file, arity
    3838from trac.util.datefmt import format_datetime, from_utimestamp, \
    3939                              to_datetime, to_utimestamp, utc
    4040from trac.util.text import exception_to_unicode, pretty_size, print_table, \
     
    8585        detected. `field` can be any of `description`, `username`, `filename`,
    8686        `content`, or `None` to indicate an overall problem with the
    8787        attachment. Therefore, a return value of `[]` means everything is
    88         OK."""
     88        OK.
     89       
     90        New style manipulators can also validate the uploaded file's content.
     91        In order to do this, they will have to implement a third parameter
     92        to this method, e.g.
     93       
     94        def validate_attachment(self, req, attachment, upload):
     95            ...
     96        """
    8997
    9098class ILegacyAttachmentPolicyDelegate(Interface):
    9199    """Interface that can be used by plugins to seemlessly participate to the
     
    637645        attachment.ipnr = req.remote_addr
    638646
    639647        # Validate attachment
     648        def validate(manipulator):
     649
     650            def validate_old(req, attachment):
     651                return manipulator.validate_attachment(req, attachment)
     652
     653            def validate_new(req, attachment, upload):
     654                return manipulator.validate_attachment(req, attachment,
     655                                                      upload)
     656            if arity(manipulator.validate_attachment) == 2:
     657                return validate_old
     658            return validate_new
     659
    640660        for manipulator in self.manipulators:
    641             for field, message in manipulator.validate_attachment(req,
    642                                                                   attachment):
    643                 if field:
    644                     raise InvalidAttachment(_('Attachment field %(field)s is '
    645                                               'invalid: %(message)s',
    646                                               field=field, message=message))
    647                 else:
    648                     raise InvalidAttachment(_('Invalid attachment: %(message)s',
     661                for field, message in validate(manipulator):
     662                    if field:
     663                        raise InvalidAttachment(_('Attachment field %(field)s '
     664                                                  'is invalid: %(message)s',
     665                                                  field=field, message=message))
     666                    else:
     667                        raise InvalidAttachment(_('Invalid attachment: %(message)s',
    649668                                              message=message))
    650669
    651670        if req.args.get('replace'):

comment:3 by Carsten Klein <carsten.klein@…>, 12 years ago

Just tested and it turned out that the function was not iterable, of course *sigh*:

  • attachment.py

     
    3434from trac.perm import PermissionError, IPermissionPolicy
    3535from trac.resource import *
    3636from trac.search import search_to_sql, shorten_result
    37 from trac.util import get_reporter_id, create_unique_file
     37from trac.util import get_reporter_id, create_unique_file, arity
    3838from trac.util.datefmt import format_datetime, from_utimestamp, \
    3939                              to_datetime, to_utimestamp, utc
    4040from trac.util.text import exception_to_unicode, pretty_size, print_table, \
     
    8585        detected. `field` can be any of `description`, `username`, `filename`,
    8686        `content`, or `None` to indicate an overall problem with the
    8787        attachment. Therefore, a return value of `[]` means everything is
    88         OK."""
     88        OK.
     89       
     90        New style manipulators can also validate the uploaded file's content.
     91        In order to do this, they will have to implement a third parameter
     92        to this method, e.g.
     93       
     94        def validate_attachment(self, req, attachment, upload):
     95            ...
     96        """
    8997
    9098class ILegacyAttachmentPolicyDelegate(Interface):
    9199    """Interface that can be used by plugins to seemlessly participate to the
     
    637645        attachment.ipnr = req.remote_addr
    638646
    639647        # Validate attachment
     648        def validate(manipulator):
     649
     650            def validate_old():
     651                return manipulator.validate_attachment(req, attachment)
     652
     653            def validate_new():
     654                return manipulator.validate_attachment(req, attachment,
     655                                                      upload)
     656            if arity(manipulator.validate_attachment) == 2:
     657                return validate_old
     658            return validate_new
     659
    640660        for manipulator in self.manipulators:
    641             for field, message in manipulator.validate_attachment(req,
    642                                                                   attachment):
    643                 if field:
    644                     raise InvalidAttachment(_('Attachment field %(field)s is '
    645                                               'invalid: %(message)s',
    646                                               field=field, message=message))
    647                 else:
    648                     raise InvalidAttachment(_('Invalid attachment: %(message)s',
     661                for field, message in validate(manipulator)():
     662                    if field:
     663                        raise InvalidAttachment(_('Attachment field %(field)s '
     664                                                  'is invalid: %(message)s',
     665                                                  field=field, message=message))
     666                    else:
     667                        raise InvalidAttachment(_('Invalid attachment: %(message)s',
    649668                                              message=message))
    650669
    651670        if req.args.get('replace'):

comment:4 by Carsten Klein <carsten.klein@…>, 12 years ago

Dear me, the validate() function is suboptimal in that it should directly call the chosen manipulator and return the result from that call

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.