778112 - All web service update() methods except Bug.update crash on invalid param names

Status: RESOLVED DUPLICATE of bug 783222

(Bugzilla :: WebService, defect)

Description

[Koosha KM]

If I want to update the 'name' param but I make a typo 'namee', the update method crashes. This is triggered in Group.update, User.update, and Product.update. Invalid param names cause set_all to call the corresponding nonexistent set_ method which results in a crash.
This is due to not checking the input parameters' names.

I couldn't trigger this bug on Bug.update.

Comment 1

[Koosha KM]

Attachment: patch - v1

This bug must be patched ASAP so that we can work on other web service module implementations.

Comment 2

[David Lawrence [:dkl]]

I dont think this additional complexity is what is needed to fix this issue. The reason the error is generated is because Bugzilla::Object::set_all tries to execute the set_ method without validating whether it can or not first. 

Bug.update works because Bugzilla::Bug::set_all cleans up the params before executing Bugzilla::Object::set_all:

    my %normal_set_all;
    foreach my $name (keys %$params) {
        # These are handled separately below.
        if ($self->can("set_$name")) {
            $normal_set_all{$name} = $params->{$name};
        }
    }

We need to update Bugzilla::Object::set_all to validate the method before running and throw a proper error message if it doesn't exist. Maybe something like this:

=== modified file 'Bugzilla/Object.pm'
--- Bugzilla/Object.pm	2012-02-28 23:00:12 +0000
+++ Bugzilla/Object.pm	2012-07-27 21:39:51 +0000
@@ -327,6 +327,11 @@
         # other set method.
         next if !exists $field_values{$key};
         my $method = "set_$key";
+        if (!$self->can($method)) {
+            my $class = ref $self;
+            ThrowCodeError('param_invalid', { param    => $key, 
+                                              function => "${class}::${method}" });
+        }
         $self->$method($field_values{$key}, \%field_values);
     }
     Bugzilla::Hook::process('object_end_of_set_all',

dkl

Comment 3

[Koosha KM]

But, I think invalid param names should be neglected silently.

Comment 4

[Frédéric Buclin]

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #3)
> But, I think invalid param names should be neglected silently.

This would need some discussion. On the other hand, one could say that invalid arguments must generate an error, because they do not follow the API. This is not a light decision to take and would impact all third-party applications interacting with Bugzilla via WebServices.

I would tend to agree with dkl and throw an error if unexpected arguments are passed to the method.

Comment 5

[Reed Loden [:reed]]

(In reply to Frédéric Buclin from comment #4)
> I would tend to agree with dkl and throw an error if unexpected arguments
> are passed to the method.

Yep, agree. Otherwise, a typo could result in unwanted results without any type of reason why.

Comment 6

[Frédéric Buclin]

Comment on attachment 646543 [details] [diff] [review]
patch - v1

After some discussion on IRC with all core developers, we all agree that unexpected arguments passed to a method must be caught and an error be thrown instead of silently ignoring them.

Comment 7

[Koosha KM]

A bad thing with set_all() is that it sets the mapped params. So, throwing an error in set_all() body is not a good idea since it makes inconsistencies with input param names. We may, instead, throw the error in the calling side before calling set_all().

What do you think?

Comment 8

[Koosha KM]

We can add new function to Bugzilla::WebService::Util like 'check_update_params' which will be passed the $params and UPDATE_PARAMS and throw an error if any of the supplied update params does not exist.

This utility function could be handy for further web service modules.

Comment 9

[David Lawrence [:dkl]]

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #8)
> We can add new function to Bugzilla::WebService::Util like
> 'check_update_params' which will be passed the $params and UPDATE_PARAMS and
> throw an error if any of the supplied update params does not exist.
> 
> This utility function could be handy for further web service modules.

I can see value in having a utility function for doing the check of update params. And this will give us the param enforcement we are looking for. I still feel we need the fix in Bugzilla::Object::set_all though which could be a separate bug. 

dkl

Comment 10

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #9)
> still feel we need the fix in Bugzilla::Object::set_all though which could
> be a separate bug. 

Why in a separate bug? This bug looks like the appropriate place to fix this.

Comment 11

[Koosha KM]

(In reply to David Lawrence [:dkl] from comment #9)
> still feel we need the fix in Bugzilla::Object::set_all though which could
> be a separate bug. 
> 
> dkl

Did you read my comment 7? Any suggestion?

Comment 12

[Frédéric Buclin]

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #11)
> Did you read my comment 7? Any suggestion?

I replied to this on IRC.

Comment 13

[Koosha KM]

(In reply to Frédéric Buclin from comment #12)
> I replied to this on IRC.

I think I wasn't on IRC to see your answer! :-(

Comment 14

[Koosha KM]

Sorry. I thought dkl wrote this!

Comment 15

[Koosha KM]

Attachment: patch - v1.1

How about this one?

Comment 16

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #10)
> (In reply to David Lawrence [:dkl] from comment #9)
> > still feel we need the fix in Bugzilla::Object::set_all though which could
> > be a separate bug. 
> 
> Why in a separate bug? This bug looks like the appropriate place to fix this.

I was meaning that the change for Bugzilla::Object::set_all should be a separate bug and patch as it can affect other code in general not related to the WebService API. Your latest patch is missing the change so we will keep the two fixes separate.

dkl

Comment 17

[David Lawrence [:dkl]]

Comment on attachment 651408 [details] [diff] [review]
patch - v1.1

Review of attachment 651408 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good and works as expected. r=dkl

::: Bugzilla/WebService/Util.pm
@@ +203,5 @@
> +=head2 check_update_keys
> +
> +This method requires two input params: the first one is a hash reference and 
> +the second one is an array reference (an array of strings). 
> +The method throws error on hash entries whose keys are not specified in that 

s/that/the/

Comment 18

[Frédéric Buclin]

Comment on attachment 651408 [details] [diff] [review]
patch - v1.1

>=== modified file 'Bugzilla/WebService/Product.pm'

>+    my $values = translate($values, MAPPED_FIELDS);

I don't see why this code suddenly appears here. Moreover, you are redefining $values using "my", which is non-sense.



>=== modified file 'Bugzilla/WebService/User.pm'

>+    my $values = translate($values, MAPPED_FIELDS);

Same here.



>=== modified file 'Bugzilla/WebService/Util.pm'

>+sub check_update_keys ($$) {

I think it should be renamed check_params() or something like that, because it may be reused for other purpose later, not only for update().

Comment 19

[Koosha KM]

(In reply to Frédéric Buclin from comment #18)
> Comment on attachment 651408 [details] [diff] [review]
> patch - v1.1
> 
> >=== modified file 'Bugzilla/WebService/Product.pm'
> 
> >+    my $values = translate($values, MAPPED_FIELDS);
> 
> I don't see why this code suddenly appears here. Moreover, you are
> redefining $values using "my", which is non-sense.
> 
> 
> 
> >=== modified file 'Bugzilla/WebService/User.pm'
> 
> >+    my $values = translate($values, MAPPED_FIELDS);
> 
> Same here.
> 

My inadvertent mistake. These are coming from my previous patch where I defined two different hashes for input and output of translate().

> 
> 
> >=== modified file 'Bugzilla/WebService/Util.pm'
> 
> >+sub check_update_keys ($$) {
> 
> I think it should be renamed check_params() or something like that, because
> it may be reused for other purpose later, not only for update().

I'm not sure if that would be a good idea since this function throws error about "update fields".

Comment 20

[Koosha KM]

Attachment: patch - v1.2

Comment 21

[David Lawrence [:dkl]]

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #19)
> > I think it should be renamed check_params() or something like that, because
> > it may be reused for other purpose later, not only for update().
> 
> I'm not sure if that would be a good idea since this function throws error
> about "update fields".

I agree with LpSolit that this should be called check_params instead as the internal code works with whatever params you pass in and is not specifically related to updating code.

You can change the error thrown to be more generic such as:

		[% ELSIF error == "check_param_unknown" %]
		'[% field FILTER html %]' is not a valid parameter.

or you can just reuse 'param_invalid'.

dkl

Comment 22

[Koosha KM]

Attachment: patch - v1.2.1

OK. Making the new method more general.

Comment 23

[Koosha KM]

Comment on attachment 652594 [details] [diff] [review]
patch - v1.2.1

+    my $function = (caller 1)[3];
+
+    $function =~ s/^Bugzilla::WebService:://;
+    $function =~ s/::/\./;
+

I don't have a good feeling about this code! We could define a new error which doesn't show the name of the function. Or, we could pass the name of the function to this new method.

Comment 24

[Koosha KM]

Attachment: patch - v1.2.1 alternative

Comment 25

[Koosha KM]

Attachment: patch - v1.2.1

I inadvertently made the previous patch obsolete.

Comment 26

[Koosha KM]

Attachment: patch - v1.2.1

Comment 27

[Frédéric Buclin]

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #7)
> A bad thing with set_all() is that it sets the mapped params. So, throwing
> an error in set_all() body is not a good idea since it makes inconsistencies
> with input param names.

This argumentation is incorrect. If you pass an invalid argument, it won't match anything anyway and so we don't care about internal vs API variable names. The error message would be the same as from Bugzilla::Object. The correct fix is to implement what dkl suggested in comment 2 and which I approved in comment 10. The check is done in a unique and central place, while your checks are spread across each WS module, with a hardcoded list of acceptable fields which will quickly be out of sync with the rest of the codebase.

dkl's fix won't regress anything, because Bugzilla will throw an error in all cases, either because it crashed, or because we caught the invalid name.

Comment 28

[Frédéric Buclin]

Comment on attachment 652604 [details] [diff] [review]
patch - v1.2.1 alternative

r- per my comment 27.

Comment 29

[Frédéric Buclin]

Comment on attachment 652608 [details] [diff] [review]
patch - v1.2.1

r- per my comment 27.