Last Comment Bug 793826 - Prevent private web service methods from being called
: Prevent private web service methods from being called
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: WebService (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Bugzilla 4.2
Assigned To: Koosha KM
: default-qa
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-24 13:28 PDT by Koosha KM
Modified: 2012-10-12 10:52 PDT (History)
3 users (show)
LpSolit: approval+
LpSolit: approval4.4+
LpSolit: approval4.2+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch - v1 (666 bytes, patch)
2012-09-24 13:28 PDT, Koosha KM
dkl: review+
Details | Diff | Review

Description Koosha KM 2012-09-24 13:28:27 PDT
Created attachment 664188 [details] [diff] [review]
patch - v1

Usually, we use methods (subroutines) that start with '_' for internal purposes only and they are somehow private. But, we don't check if the user is trying to call one of these private methods. It is not reasonable to be able to call these methods as a client. In addition, they might cause some security implications. It would be better to prevent these methods from being called.
Comment 1 Frédéric Buclin 2012-09-24 14:07:29 PDT
It should not be possible to call a private method. If we can, that's a bug.
Comment 2 David Lawrence [:dkl] 2012-09-27 09:13:14 PDT
It is possible to do so as I just verified it. 

One way to enforce would be add code to each private method to check if caller is the current $self otherwise die. 

sub _private_method_foo {
    my ($self) = @_;
    ThrowCodeError("private_method_called")
        unless (caller)[0]->isa( ref($self) );
    return "foo";
}

Or make each private method an anonymous code ref and call it that way such as:

my $private_method_foo = sub {
    return "foo";
};

sub public_method_foo {
    my ($self) = @_;
    return $self->$private_method_foo();
}

Of course Koosha's fix works as long as we use the same private method naming convention throughout which so far it looks like we do. So that would be the quickest fix with the least amount of code change for now.

dkl
Comment 3 David Lawrence [:dkl] 2012-10-09 15:45:45 PDT
Comment on attachment 664188 [details] [diff] [review]
patch - v1

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

Works as described and r=dkl unless LpSolit wants to make major changes across all webservice code and do it a different/better way.
Comment 4 Frédéric Buclin 2012-10-09 15:49:14 PDT
(In reply to David Lawrence [:dkl] from comment #3)
> Works as described and r=dkl unless LpSolit wants to make major changes
> across all webservice code and do it a different/better way.

No, I'm fine with the way it's done in this patch.
Comment 5 Frédéric Buclin 2012-10-12 10:52:27 PDT
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/WebService/Server.pm
Committed revision 8422.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.4/
modified Bugzilla/WebService/Server.pm
Committed revision 8416.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.2/
modified Bugzilla/WebService/Server.pm
Committed revision 8149.

Note You need to log in before you can comment on or make changes to this bug.