On 06/08/2016 10:07 AM, Petr Spacek wrote:
On 7.6.2016 15:11, Stanislav Laznicka wrote:
Hello,

Thank you for your patch. As the thin-client patches were pushed in the
meantime, the patch won't apply. Could you please send a rebased version?

Also, I have a few comments to the patch:

1) I think that the commit message should be rather a brief conclusion to the
changes made in the commit. This could help for faster orientation in the
changes that were made to a certain part of code should you be searching for a
bug introduced by a commit. Should some more info be required, it can be added
to the ticket. Could you therefore shorten the commit message?
(My personal opinion, no golden standard.)

Honestly I disagree with Standa. Yes, the commit message seems to be a bit
long but *tickets* are not the best place to put *technical* information into.

Tickets are planning tool but keep in mind that Trac may/will vanish one day
and all we will have will be (Git?) repo.

I would recommend putting the comment about expected input format into code
comments somewhere around batch command definition.

This would reduce commit message (roughly, the text needs to be adapted) to
part starting 'The code did not check the format of ' ... which is perfectly
reasonable description of the change.

IMHO.
Petr^2 Spacek


2) Please do not add the tickets to comments in the code. You can use git
blame -L or git log -L to see in which commits were the changes introduced to
a certain part of a file, these commits should include the ticket number if
more info is needed.

Standa


On 05/27/2016 03:53 PM, Florence Blanc-Renaud wrote:
Hi all,

the following patch checks the format of parameters passed to a method
called through the batch command. I picked the ConversionError for invalid
parameters format but this choice can be discussed if you have better
suggestions...

Fixes: https://fedorahosted.org/freeipa/ticket/5810
--
Florence Blanc-Renaud
Identity Management Team, Red Hat






Hi,

please find an updated patch version with a less verbose commit msg. I also removed the reference to ticket # in the code.

Flo.

From 927a918ee1a51add5f45786919fe75920ffe5f78 Mon Sep 17 00:00:00 2001
From: Florence Blanc-Renaud <fren...@redhat.com>
Date: Fri, 27 May 2016 08:19:39 +0200
Subject: [PATCH] batch command can be used to trigger internal errors on
 server

In ipalib, the batch command expects a specific format for arguments.
The code did not check the format of the parameters, which could trigger
internal errors on the server.
With this fix:
- a ConversionError is raised if the arg passed to batch() is not a list of
dict
- the result appended to the batch results is a ConversionError if the
'params' does not contain a tuple(list,dict)

https://fedorahosted.org/freeipa/ticket/5810
---
 ipaserver/plugins/batch.py | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/ipaserver/plugins/batch.py b/ipaserver/plugins/batch.py
index 84a65057588e01e18a937c6e79f0974b25c529ed..aebdc2f72615eeafeec614fe8b33df4ada7688c7 100644
--- a/ipaserver/plugins/batch.py
+++ b/ipaserver/plugins/batch.py
@@ -90,6 +90,12 @@ class batch(Command):
     def execute(self, methods=None, **options):
         results = []
         for arg in (methods or []):
+            # As take_args = Any, no check is done before
+            # Need to make sure that methods contain dict objects
+            if not isinstance(arg, dict):
+                raise errors.ConversionError(
+                    name='methods',
+                    error=_(u'must contain dict objects'))
             params = dict()
             name = None
             try:
@@ -100,9 +106,21 @@ class batch(Command):
                 name = arg['method']
                 if name not in self.Command:
                     raise errors.CommandError(name=name)
-                a, kw = arg['params']
-                newkw = dict((str(k), v) for k, v in kw.items())
-                params = api.Command[name].args_options_2_params(*a, **newkw)
+
+                # If params are not formated as a tuple(list, dict)
+                # the following lines will raise an exception
+                # that triggers an internal server error
+                # Raise a ConversionError instead to report the issue
+                # to the client
+                try:
+                    a, kw = arg['params']
+                    newkw = dict((str(k), v) for k, v in kw.items())
+                    params = api.Command[name].args_options_2_params(
+                        *a, **newkw)
+                except (AttributeError, ValueError, TypeError):
+                    raise errors.ConversionError(
+                        name='params',
+                        error=_(u'must contain a tuple (list, dict)'))
                 newkw.setdefault('version', options['version'])
 
                 result = api.Command[name](*a, **newkw)
-- 
2.5.5

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to