Managing method security "hole" (restrictedMethods) when using pageType as a model
Permalink
I'm working on a project now where most objects (though they're pretty complex) are respresented self contained pages. The problem I've just run into is that there seems to be a security hole / bug / oversight that doesn't allow me to restrict methods from the users.
I started with a Community model and a CommunityPageType. There was a db table behind Community and a lot of convoluted functions to get the "other" one and make sure they were linked (Community has an ID, and the page type is the cID). Community handled most of the programmatic stuff while the page type was responsible for the front-end actions.
But then I realized that the c5 ideal was one step further, where the page type *is* the model. So I refactored, got rid of my Community, and my CommunityPageType is basically the model mixed with a few action functions that the "user" accesses via URLs. It *seems* a lot slicker.
Then I started thinking about security. And that there's nothing to prevent a user from accessing a "programmatic-only" method via the URL. It seems like somebody had thought of this before because there's vestigial code lying around (restrictedMethods) described as "you can't pass a method that's in the restrictedMethods array". However, the variable is still around, and a method that uses it, but that method is no longer actually called.
So... am I "breaking" the MVC concept by using page type as a model? Is there a way of securing this? Is the restrictedMethods a security bug that "should" be working but got overlooked? Thanks for your input.
I started with a Community model and a CommunityPageType. There was a db table behind Community and a lot of convoluted functions to get the "other" one and make sure they were linked (Community has an ID, and the page type is the cID). Community handled most of the programmatic stuff while the page type was responsible for the front-end actions.
But then I realized that the c5 ideal was one step further, where the page type *is* the model. So I refactored, got rid of my Community, and my CommunityPageType is basically the model mixed with a few action functions that the "user" accesses via URLs. It *seems* a lot slicker.
Then I started thinking about security. And that there's nothing to prevent a user from accessing a "programmatic-only" method via the URL. It seems like somebody had thought of this before because there's vestigial code lying around (restrictedMethods) described as "you can't pass a method that's in the restrictedMethods array". However, the variable is still around, and a method that uses it, but that method is no longer actually called.
So... am I "breaking" the MVC concept by using page type as a model? Is there a way of securing this? Is the restrictedMethods a security bug that "should" be working but got overlooked? Thanks for your input.
if it is private, or protected, or if the function starts with on_ or __ then it will 404, here is the code that preforms the checking: https://github.com/concrete5/concrete5/blob/master/web/concrete/libr...
Thanks to both of you.
I should have said that I considered this.
On one hand, I can't declare protected as, just like any other model, it has a number of methods that will get called programmatically from other models / controllers. And these are exactly the same methods that I don't want a user to call (and this is a school grading app, so i need to be especially concerned about security).
Similarly, prefacing my model's methods with __ or on_ seems a bit messy (__getModelVaidation, on_deletePage()).
I've looked at this some more... and I'm still not sure if this is the best way... but looking at the code, it seems a lot cleaner than what I had before. I looked at some blocks, and that's basically what I want to be doing, where they mix actions (view, action_submit_form) with model methods (getJavascriptString()).
I think that my only solution will be to override setupandrun and reimplement the protectedMethods variable... but I'm still not sure why this is in the code but not used... did it get bypassed accidently and thus it's a bug?
James
I should have said that I considered this.
On one hand, I can't declare protected as, just like any other model, it has a number of methods that will get called programmatically from other models / controllers. And these are exactly the same methods that I don't want a user to call (and this is a school grading app, so i need to be especially concerned about security).
Similarly, prefacing my model's methods with __ or on_ seems a bit messy (__getModelVaidation, on_deletePage()).
I've looked at this some more... and I'm still not sure if this is the best way... but looking at the code, it seems a lot cleaner than what I had before. I looked at some blocks, and that's basically what I want to be doing, where they mix actions (view, action_submit_form) with model methods (getJavascriptString()).
I think that my only solution will be to override setupandrun and reimplement the protectedMethods variable... but I'm still not sure why this is in the code but not used... did it get bypassed accidently and thus it's a bug?
James
I see your point. But isn't it only the controller and it's methods which is accessible via the URI. I'd 'protect' the methods I was concerned about in the controller as opposed to making them private, and extend them in a another class as public methods - not a controller, so that my application still has access internally. Does that not give you the protection you need?
I really hope this gets addressed with something. Controllers need something like this.
Then a check before running.
For backwards compatibility only enforce it if that exists. I don't want to prove this or discuss it too much to get this thread too much attention, but hopefully this gets address.
And @jshannon, that's exactly how I do what you are doing with the page type controller.
class SomePageTypeController extends Controller { protected $uri_accessible = array( 'methodOne', 'methodTwo', 'methodThree' );
Then a check before running.
For backwards compatibility only enforce it if that exists. I don't want to prove this or discuss it too much to get this thread too much attention, but hopefully this gets address.
And @jshannon, that's exactly how I do what you are doing with the page type controller.
That's a good idea. Or a boolean that forces only methods that are proceeded with action_ to be callable (the same way we default to for block type passthru methods)
I agree Andrew. action_ would be more consistent.
C
C
boolean with action_ is probably more consistent. I would definitely go for that.
I just wacked this together that you can probably throw in a helper somewhere.
Then call it like this.
It's black listing not white listing and not super pretty, but at least it black boxes it a little until a patch goes through.
public function secure_uri_action($method_name) { $req = Request::get(); $task = substr('/' . $req->getRequestPath(), strlen($req->getRequestCollectionPath()) + 1); $task = str_replace('-/', '', $task); $taskparts = explode('/', $task); $method = $taskparts[0]; if($method === $method_name) { $v = View::getInstance(); $v->render('/page_not_found'); } }
Then call it like this.
public function my_method() { Loader::helper('my_helper')->secure_uri_action('my_method'); // stuff below won't happen }
It's black listing not white listing and not super pretty, but at least it black boxes it a little until a patch goes through.
This is a really interesting discussion for me, but can't help thinking OOP principles already cover us. Protect and extend if you need still need internal access to something?
I must be missing something; why would it be better to explicitly blacklist or whitelist methods for access via URI rather than declare a method as protected and subclass it IF you needed access to it internally. I am missing something and I can't put my finger on it. Would appreciate a steer.
I must be missing something; why would it be better to explicitly blacklist or whitelist methods for access via URI rather than declare a method as protected and subclass it IF you needed access to it internally. I am missing something and I can't put my finger on it. Would appreciate a steer.
Yeah... I think that "in principle", you're probably right. And I do feel a bit dirty about having the controller and model combined. However, with c5's attribute architecture, it works well.
However, if you're going to do it the "right" way, I don't think that would include extending... it would be complete separation. Having a class of protected methods and an extending class with a bunch of public function a() { parent::a(); } feels a bit dirty, too.
Plus (more importantly), I wanted to avoid that because right now I can call $view->controller or Loader::controller(Page::getByPath()) and get my "model".
I'm still stumbling around on this design pattern, so I'm not so sure either.
But to get back to the original discussion.... Take a look, for example, at the FormBlockController. There are the actions, a bunch of protected methods, and a getJavascriptString(). That's, presumably, called by the block's view with something like $controller->get...(). It'd just be a lot more work to extend the blockcontroller and instantiate that new object (let alone dealing with the fact that it's a different instance). If there's a good solution to this, I'm willing to go that direction.
James
However, if you're going to do it the "right" way, I don't think that would include extending... it would be complete separation. Having a class of protected methods and an extending class with a bunch of public function a() { parent::a(); } feels a bit dirty, too.
Plus (more importantly), I wanted to avoid that because right now I can call $view->controller or Loader::controller(Page::getByPath()) and get my "model".
I'm still stumbling around on this design pattern, so I'm not so sure either.
But to get back to the original discussion.... Take a look, for example, at the FormBlockController. There are the actions, a bunch of protected methods, and a getJavascriptString(). That's, presumably, called by the block's view with something like $controller->get...(). It'd just be a lot more work to extend the blockcontroller and instantiate that new object (let alone dealing with the fact that it's a different instance). If there's a good solution to this, I'm willing to go that direction.
James
@mkly - I don't really care for it. (your fix)
you already have to call action_ in a blocktype method. it would be far more consistent to use the same line of thinking for page_types and single_pages.
We don't need more and more and more bastardized "specialty functions"
We have to be careful not to turn C5 in to Joomla 2.0.
Anytime we can have repeatable patterns of programming, we should.
C
you already have to call action_ in a blocktype method. it would be far more consistent to use the same line of thinking for page_types and single_pages.
We don't need more and more and more bastardized "specialty functions"
We have to be careful not to turn C5 in to Joomla 2.0.
Anytime we can have repeatable patterns of programming, we should.
C
I think that was introduced as a quick fix, and it's not a bad one.
Personally, I thought I'd be able to add the following to an extension of controller:
Basically, just change the $method that it's looking for. That kinda works. However, in controller there's a *private* method (setupandrun() or something) that checks for the existence and returns a 404. So, $task is delete, and runTask() properly calls action_delete... but before it even gets there, a 404 is thrown since "delete" doesn't exist... think it's just a matter of tweaking a few more lines in the core...
James
Personally, I thought I'd be able to add the following to an extension of controller:
foreach($methodArray as $method) { $method = ($this->restrictToActionMethods && $method != 'view') ? "action_$method" : $method; // <--------- new line if (is_callable(array($this, "$method"))) { if(!is_array($params)) { $params = array($params); } return call_user_func_array(array($this, $method), $params); } }
Basically, just change the $method that it's looking for. That kinda works. However, in controller there's a *private* method (setupandrun() or something) that checks for the existence and returns a 404. So, $task is delete, and runTask() properly calls action_delete... but before it even gets there, a 404 is thrown since "delete" doesn't exist... think it's just a matter of tweaking a few more lines in the core...
James
Just changing this https://github.com/concrete5/concrete5/blob/master/web/concrete/libr... to:
work?
if ($cl->getName() != 'Controller' && strpos($method, 'action_') == 0 && $r->isPublic()) {
work?
That'll probably fix the 404, but then you'd still need my code to call the actual function. Or you can probably integrate my code into the same area, and just modify $task first. Plus, I think, to mirror blocks, we should keep view() the same and check the $var as discussed...
But, yeah... this isn't a big change...
But, yeah... this isn't a big change...
@ChadStrat: Indeed. This was only meant as something temporary. Don't be alarmed. :)
you'll have to forgive me...I was damaged for life by Joomla! ha ha
I love this communities commitment to design patterns. it's great to see.
C
I love this communities commitment to design patterns. it's great to see.
C
Hope that helps.