Module development: Any reasons creating catch-all action is a bad idea?

Question

When developing a website in EE, I often create custom modules to accomplish some task, such as processing a custom form, etc. I started creating 'actions' for each form, but this grew inconvenient during development, because I would have to uninstall/reinstall, or write some 'update' code, to incorporate new action_ids (or manually add them to the exp_actions table.

My current workaround is, for any new module, to create one catch-all 'action'. So if my module was called 'Module', my action would be called 'module_do'. In my mod.module.php file, I would declare that function like so:

function mailchimp_api_do(){

    $function = $this->EE->input->get_post('function',TRUE);

    $accepted_functions = array(
        'do_that_one_thing_you_do',
        'do_that_other_thing_you_do'
    );

    if(in_array($function,$accepted_functions)){
        //call that function
        $this->$function();
    }

}

That way, I could code up my forms to post to a URL like, for instance:

• /?ACT=##&function=do_that_one_thing_you_do
• /?ACT=##&function=do_that_other_thing_you_do

But if I used a function not in my $accepted_function array, like:

• /?ACT=##&function=do_something_you_dont_do

Nothing would happen.

So, as I build out my site, I could add new actions, sort of, by just adding to that $accepted_functions array, and using &function=my_new_function in my URLs...

This has worked nicely for me, and I haven't yet developed any modules that are out in the wild for others to use. My question is, is this a bad practice, or are there any reasons this is bad form, or unsecure?

Thanks!

Answer 1

It's really your choice here. I imagine that this framework is best for smaller addons. One point of access is always slightly frowned upon; because it creates a SPOF in your addon.

Trust me, I totally get the code-install-test-uninstall-ad.nauseam development path of EE modules, I'm sick of it too. However, I've found, that paying more careful attention to the stage 1 planning of a larger module usually allows me to get all of my ACTion IDs in order on first install. Of course, the occasional product requires I add one extra because of misstep, but I'm mostly over those days.

From a security standpoint, not accounting for module architecture, EE is safe. It hasn't ever had a major security vulnerability. I can imagine, since you're taking direct GET strings that literally look like a vulnerability point:

http://www.main-domain.com/index.php?ACT=1&function=HOLY_CRAP_IM_EXECUTING_THIS_FUNCTION

You might get some people trying to exploit you there, but still, as long CSRF protection is running, you should be safe. But still, obfuscation is a good part of security, it deturs many lazy/half-assed people, as well as aids in passing automated security audits.

Edit: expanding edition Another issue with not obfuscating this type of GET query is that it allows an attacker to analyze the flow of your addon. For example...

.../index.php?ACT=1&function=start_of_addon_functionality&param1=a&param2=b
.../index.php?ACT=1&function=middle_of_addon_functionality&param1=a&param2=b
.../index.php?ACT=1&function=end_of_addon_functionality&param1=a&param2=b

Seeing this flow, it's possible for an attacker to try and hit your addon's second or third process function before hitting the first one, creating erroneous data input or straight-up fatal errors. And it can only get worse. Most likely harmless, but it always depends on what your addon actually does.

If your usage tends to be more form submittall, then just going with POST over HTTPS (SSL connections encrypt about everything, including headers and post data) would protect you.

tl:dr; You should be fine, but I wouldn't necessarily go that route. But there are situations where it's might be more appropriate or safe (private, intranet site for employees, etc).

Edit Might I add that going with your method, you're missing a debug path. You should definitely add something like:

// after you check if your `function` GET param is actually a function in your function array

$this->return_data = array (
    'code'      => "404",
    'message'   => "Function not found"
);

ee()->output->send_ajax_response($this->return_data);

Edit One more, just for fun, if you put a module like this out in the wild with that GET setup, just for shits and giggles, see if people try it as an attack vector!

// goes just before my last code block
ee()->load->library('logger');

ee()->logger->developer(__METHOD__ . ":" . __LINE__ . ": 404 function call: " . $_SERVER['REMOTE_ADDR'] . " : " . $function);