471942 - Make the WebService validate and properly convert input parameters

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[Max Kanat-Alexander]

Attachment: v1

The following patch makes the WebService:

1) Accept empty ints, doubles, and dateTimes and converts that to undef 
   instead of an empty string.

2) Converts input dateTimes into a format that a database can use, instead
   of the XML-RPC format (which nothing can use)

3) Validates that input parameters are in the right format, and throws
   an error if they are not. This should help people detect errors in
   their applications that are integrating with Bugzilla.

Comment 1

[Max Kanat-Alexander]

We'll have to relnote that the webservice is now strict and complains about bad input, and also that it now allows certain forms of undef.

Comment 2

[David Lawrence [:dkl]]

Comment on attachment 355198 [details] [diff] [review]
v1

>Index: Bugzilla/WebService.pm
>+# Some method arguments need to be converted in some way, when they are input.
>+sub decode_value {
>+    my $self = shift;
>+    my $value = $self->SUPER::decode_value(@_);
>+    
>+    my (undef, $type) = @_;
>+    # We only validate/convert certain types here.
>+    return $value if $type !~ /^(?:int|i4|boolean|double|dateTime\.iso8601)$/;
>+    

Getting the following warning in /var/log/httpd/error_log:

Use of uninitialized value $type in pattern match (m//) at /var/www/html/bugzilla/Bugzilla/WebService.pm line 134

After using Data::Dumper, I see that $type is always undefined so you are not parsing @_ properly. The @_ is actually
a very large data structure.

In XMLRPC::Lite::Deserializer::decode_value we have 


sub decode_value {
    my $self = shift;
    my $ref = shift;
    my($name, $attrs, $children, $value) = @$ref;

    ...

    elsif ($name =~ /^(?:int|i4|boolean|string|double|dateTime\.iso8601|methodName)$/) {
        return $value;
    }

    ...
}

So it would seem then that Bugzilla::WebService::XMLRPC::Deserializer::decode_value would need to be:

# Some method arguments need to be converted in some way, when they are input.
sub decode_value {
    my $self = shift;
    my $value = $self->SUPER::decode_value(@_);
 
    my ($type) = @{$_[0]};

    # We only validate/convert certain types here.
    return $value if $type !~ /^(?:int|i4|boolean|double|dateTime\.iso8601)$/;

    ...
}

Dave

Comment 3

[David Lawrence [:dkl]]

I tried the above suggested fix myself and it did work as expected for valid values. But when I passed an invalid value for 'i4' as a test, the code complained about not being to find ThrowUserError() so you need to also include Bugzilla::Error.

Application failed during request deserialization: Undefined subroutine &Bugzilla::WebService::XMLRPC::Dezerializer::ThrowUserError called at /var/www/html/bugzilla/Bugzilla/WebService.pm line 146.

Dave

Comment 4

[Max Kanat-Alexander]

Attachment: v2

Oh, thanks for catching those!

Comment 5

[David Lawrence [:dkl]]

Comment on attachment 355684 [details] [diff] [review]
v2

Bugzilla::Error needs to be added in Bugzilla::WebService::XMLRPC::Deserializer
package namespace. Adding to Bugzilla::WebService only still causes the code to die unable to find the ThrowUserError subroutine.

Comment 6

[Max Kanat-Alexander]

Attachment: v3

Ah, yeah...testing. Testing is a good thing... :-)

Comment 7

[David Lawrence [:dkl]]

Comment on attachment 355861 [details] [diff] [review]
v3

Looks good to me and works.

Comment 8

[Reed Loden [:reed]]

Comment on attachment 355861 [details] [diff] [review]
v3

>+    $retval{'deserializer'} = Bugzilla::WebService::XMLRPC::Dezerializer->new;

Bugzilla::WebService::XMLRPC::Dezerializer ?? Don't you mean Deserializer?

...

>+package Bugzilla::WebService::XMLRPC::Dezerializer;

Same.

Comment 9

[David Lawrence [:dkl]]

(In reply to comment #8)
> (From update of attachment 355861 [details] [diff] [review])
> >+    $retval{'deserializer'} = Bugzilla::WebService::XMLRPC::Dezerializer->new;
> 
> Bugzilla::WebService::XMLRPC::Dezerializer ?? Don't you mean Deserializer?
> 
> ...
> 
> >+package Bugzilla::WebService::XMLRPC::Dezerializer;
> 
> Same.

Heh, I can't believe I didn't catch that. Technically it still works which is why I didn't catch it. Max, please fix that on checkin :)

Dave

Comment 10

[Max Kanat-Alexander]

Um, yes, I did indeed mean that. :-) I will fix it on checkin.

Comment 11

[Max Kanat-Alexander]

Zank you for catching ze Dezerializer. :-) I blame Moose and Squirrel. ;-)

Checking in Bugzilla/WebService.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService.pm,v  <--  WebService.pm
new revision: 1.16; previous revision: 1.15
done
Checking in Bugzilla/WebService/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Constants.pm,v  <--  Constants.pm
new revision: 1.22; previous revision: 1.21
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.271; previous revision: 1.270
done

Comment 12

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.4 in bug 494037.