From 028ccecb5b6a67a94525cf636054bfe643d3134b Mon Sep 17 00:00:00 2001 From: Isaac Connor Date: Fri, 28 Aug 2026 19:54:07 -0400 Subject: [PATCH] fix(api): answer 400 naming the field instead of 500 on an unknown filter FilterComponent::buildFilter() checked the operator against an allow list but never the field, so an unrecognised name went straight into the SQL: GET /api/events/index/MonitorName =:Monitor-1.json PDOException SQLSTATE[42S22]: Column not found: 1054 Unknown column 'MonitorName' in 'where clause' -> HTTP 500 A caller filtering on a monitor-scoped attribute (MonitorName, Monitor, Rack, Experiment, Server, Storage) or simply mistyping a column got an opaque 500 with a stack trace and nothing naming the offending field. Take an optional list of permitted fields and reject anything else with BadRequestException. EventsController::index() passes the Events columns from the model schema plus the two names it resolves itself: DateTime, the pseudo-attribute it turns into an overlap test, and GroupId, served by the Groups_Monitors join. A field written Model.Field is checked on the column. The two other callers of buildFilter pass no list and are unchanged. The invalid-argument and invalid-operator paths threw a plain Exception, which was also a 500 for what is bad input; they throw BadRequestException now too. Verified against a live install: MonitorName / Rack / NotAColumn 400, "Unknown filter field: " DateTime >=/<= over a window 200, 7 rows, overlap intact Cause, Event.Cause, GroupId 200 ?limit=2&sort=...&direction=desc 200, 2 rows, pagination unaffected Plus an assert script over the validation itself, including that a caller passing no list keeps the previous pass-through behaviour. Filters sent as a query string rather than named parameters go through a different branch, on raw $_REQUEST, and are deliberately left unvalidated: that array also carries auth and cache-busting parameters, so rejecting unknown names there would break existing callers. ?MonitorName=x is still a 500 and wants its own change. (cherry picked from commit 4d82e1d3855c1634e0bf28729fcb6566a7d13771) --- .../Controller/Component/FilterComponent.php | 27 ++++++++++++++++--- web/api/app/Controller/EventsController.php | 9 ++++++- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/web/api/app/Controller/Component/FilterComponent.php b/web/api/app/Controller/Component/FilterComponent.php index c649a1f8b..ba8a9bc56 100644 --- a/web/api/app/Controller/Component/FilterComponent.php +++ b/web/api/app/Controller/Component/FilterComponent.php @@ -73,8 +73,16 @@ class FilterComponent extends Component { ); // Build a CakePHP find() condition based on the named parameters - // that are passed in - public function buildFilter($namedParams) { + // that are passed in. + // + // $validFields, when given, is the list of field names a caller may filter + // on. Without it an unknown name is passed straight through into the SQL and + // surfaces as a PDOException, so a caller asking for a field that does not + // exist - or one that lives on Monitors rather than the model being queried, + // such as MonitorName - got an opaque 500 with a stack trace rather than a + // message naming the field. Callers that have not been given a list keep the + // old behaviour. + public function buildFilter($namedParams, $validFields = null) { $conditions = array(); if ($namedParams) { foreach ($namedParams as $attribute => $value) { @@ -85,14 +93,25 @@ class FilterComponent extends Component { } $matches = NULL; if (preg_match('/^(?P[a-z0-9\.]+)(?P.+)?$/i', $lhs, $matches) !== 1) { - throw new Exception('Invalid argument before `:`: ' . $lhs); + throw new BadRequestException('Invalid argument before `:`: ' . $lhs); } $operator = isset($matches['operator']) ? trim($matches['operator']) : ''; // Only allow operators on our allow list. No operator // specified defaults to `=` by cakePHP. if ($operator != '' && !in_array($operator, $this->twoOperandSQLOperands)) { - throw new Exception('Invalid operator: ' . $operator); + throw new BadRequestException('Invalid operator: ' . $operator); + } + + if ($validFields !== null) { + // A field may be written Model.Field; only the column is checked. + $field = $matches['field']; + $bare = (false !== strpos($field, '.')) + ? substr($field, strrpos($field, '.') + 1) + : $field; + if (!in_array($bare, $validFields)) { + throw new BadRequestException('Unknown filter field: ' . $bare); + } } if (($operator == 'LIKE') and (false === strpos($value, '%'))) { $value = '%'.$value.'%'; diff --git a/web/api/app/Controller/EventsController.php b/web/api/app/Controller/EventsController.php index 937863c5f..c4a8641b1 100644 --- a/web/api/app/Controller/EventsController.php +++ b/web/api/app/Controller/EventsController.php @@ -74,7 +74,14 @@ class EventsController extends AppController { unset($named_params[$k]); } } - $conditions = $this->FilterComponent->buildFilter($named_params); + // Events columns, plus the two names this action resolves itself: DateTime + // is a pseudo-attribute turned into an overlap test below, and GroupId is + // served by the Groups_Monitors join added further down. Anything else + // would reach MySQL as an unknown column and come back as a 500. + $valid_fields = array_keys($this->Event->schema()); + $valid_fields[] = 'DateTime'; + $valid_fields[] = 'GroupId'; + $conditions = $this->FilterComponent->buildFilter($named_params, $valid_fields); #ZM\Debug(print_r($conditions, true)); foreach ($conditions as $k=>$v) { if ( 0 === strpos($k, 'DateTime') ) {