Skip to content

Whitelist specific functions for the data controller - #1304

Open
PGijsbers wants to merge 1 commit into
developfrom
whitelist-data-access
Open

Whitelist specific functions for the data controller#1304
PGijsbers wants to merge 1 commit into
developfrom
whitelist-data-access

Conversation

@PGijsbers

@PGijsbers PGijsbers commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

The controller would allow attempts at calling arbitrary functions. This is harmless as long as no new functions get defined on the class - the exposed functions are exactly those we want to allow. However, we noticed from logs that there are attempts at calling other functions, so we are taking preventative measures to close things down so that:

  • should, for some reason, a new function get added to the old data controller, it cannot be accidentally exposed
  • invalid access to functions that don't exist don't result in a PHP error, but rather in a normal response to the client

This patch introduces a whitelist of functions which are allowed to be called this way, and provides clearer error messages if an unknown function is called, or a known function is called incorrectly.

--

I considered using the standard returnError defined on MY_Api_Model but you would need to load in those modules and hack around it not actually being called from the new controller:

Anywhere in the path (e.g., in the constructor, or specifically only when an error is actually raised):

+   $this->load->Model('api/v1/Api_study');  // Any controller that extends MY_Api_Model
+    $this->load->model('Log');
+    $this->load->view('pages/api_new/v1/xml/pre.php');

and then at the call site:

+      $this->controller = 'api_new';
+      $this->page = 'xml';
+      $this->Api_study->returnError(100, $this->Api_study->version);

because at some point the error template path is determined by interpolation of the controller and page.

I figured it's better to keep things simple.

@PGijsbers
PGijsbers force-pushed the whitelist-data-access branch from 34c350e to 462a821 Compare August 4, 2026 14:30
@PGijsbers
PGijsbers requested review from janvanrijn and omosola and removed request for omosola August 4, 2026 14:31
$function_whitelist = array('download', 'view', 'get_csv');

if (!in_array($function, $function_whitelist)) {
http_response_code(404);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we define an XML error for this?

@omosola

omosola commented Aug 4, 2026

Copy link
Copy Markdown

The current associated model (Data_server) used only returns http-related error codes as well (no specific internal error codes), and this code is largely in maintenance mode, so I'm very okay with keeping it simple and not adding extra bulk to use returnError. Agree with the implementation, @PGijsbers 👍

And LGTM 🚢 (pending @janvanrijn)

@janvanrijn janvanrijn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current associated model (Data_server) used only returns http-related error codes as well

That's true, good point.

Let's go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants