Closed
Bug 471942
Opened 16 years ago
Closed 16 years ago
Make the WebService validate and properly convert input parameters
Categories
(Bugzilla :: WebService, enhancement)
Tracking
()
RESOLVED
FIXED
Bugzilla 3.4
People
(Reporter: mkanat, Assigned: mkanat)
References
Details
(Whiteboard: relnote comment 1)
Attachments
(1 file, 2 obsolete files)
5.71 KB,
patch
|
dkl
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #355198 -
Flags: review?(dkl)
Assignee | ||
Comment 1•16 years ago
|
||
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.
Keywords: relnote
Whiteboard: relnote comment 1
Updated•16 years ago
|
Attachment #355198 -
Flags: review?(dkl) → review-
Comment 2•16 years ago
|
||
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•16 years ago
|
||
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
Assignee | ||
Comment 4•16 years ago
|
||
Oh, thanks for catching those!
Attachment #355198 -
Attachment is obsolete: true
Attachment #355684 -
Flags: review?(dkl)
Updated•16 years ago
|
Attachment #355684 -
Flags: review?(dkl) → review-
Comment 5•16 years ago
|
||
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.
Assignee | ||
Comment 6•16 years ago
|
||
Ah, yeah...testing. Testing is a good thing... :-)
Attachment #355684 -
Attachment is obsolete: true
Attachment #355861 -
Flags: review?(dkl)
Comment 7•16 years ago
|
||
Comment on attachment 355861 [details] [diff] [review] v3 Looks good to me and works.
Attachment #355861 -
Flags: review?(dkl) → review+
Comment 8•16 years ago
|
||
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•16 years ago
|
||
(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
Assignee | ||
Updated•16 years ago
|
Flags: approval+
Assignee | ||
Comment 10•16 years ago
|
||
Um, yes, I did indeed mean that. :-) I will fix it on checkin.
Assignee | ||
Comment 11•16 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 12•15 years ago
|
||
Added to the release notes for Bugzilla 3.4 in bug 494037.
Keywords: relnote
You need to log in
before you can comment on or make changes to this bug.
Description
•