From ee28822d57a81045d83c2ca6da7ac8c46e8f76df Mon Sep 17 00:00:00 2001 From: Cam Reeves Date: Mon, 18 May 2026 13:24:00 +1000 Subject: [PATCH 1/2] fix(security): restrict driver listing, driver readme, zone triggers and module_id lookups to support users MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported by DataArt pentest 2026-05 (McKinsey Converge) as H1 finding — "Missing function-level access controls". Four engine endpoints were authenticated but reachable by any standard user, exposing internal infrastructure detail that should be scoped to operators: - GET /api/engine/v2/drivers (H1 #6, #9) - GET /api/engine/v2/drivers/:id/readme (H1 #10) — README contents can include configuration notes / credential examples - GET /api/engine/v2/zones/:id/triggers (H1 #7) — internal automation state - GET /api/engine/v2/systems?module_id=… (H1 #11) — system-by-module lookup; only this branch of /systems is gated, the rest of the index query is unchanged Drivers and zone-triggers are gated with an additional `before_action :check_support`. Systems gates only the module_id path inline so the rest of the index (the standard system search used by end-users) is unaffected. Three sibling endpoints flagged in the same pentest are deliberately NOT changed here per Converge team review: - /metadata/:id (room/zone metadata) stays can_read_guest - /systems?q=…&fields=… stays can_read (search-by-name is a normal user flow) - /users?q=… and /users/:id stay as-is (controller already strips PII via to_public_struct for non-admin callers and filters to the caller's authority) Co-Authored-By: Claude Opus 4.7 (1M context) --- spec/controllers/drivers_spec.cr | 35 +++++++++++++++++++++ spec/controllers/systems_spec.cr | 10 ++++++ spec/controllers/zones_spec.cr | 28 +++++++++++++++++ src/placeos-rest-api/controllers/drivers.cr | 8 +++++ src/placeos-rest-api/controllers/systems.cr | 9 ++++++ src/placeos-rest-api/controllers/zones.cr | 9 ++++-- 6 files changed, 97 insertions(+), 2 deletions(-) diff --git a/spec/controllers/drivers_spec.cr b/spec/controllers/drivers_spec.cr index 40d6aeca..89a987e1 100644 --- a/spec/controllers/drivers_spec.cr +++ b/spec/controllers/drivers_spec.cr @@ -133,6 +133,41 @@ module PlaceOS::Api end end + describe "access control", tags: "access-control" do + it "GET /drivers returns 403 for non-admin / non-support callers" do + _, headers = Spec::Authentication.authentication(sys_admin: false, support: false) + result = client.get(Drivers.base_route, headers: headers) + result.status_code.should eq 403 + end + + it "GET /drivers/:id/readme returns 403 for non-admin / non-support callers" do + repository = Model::Generator.repository(type: Model::Repository::Type::Driver) + repository.uri = "https://github.com/PlaceOS/drivers" + repository.save! + + driver = Model::Driver.new( + name: "Restricted Readme", + role: Model::Driver::Role::Logic, + commit: "HEAD", + module_name: "AutoRelease", + file_name: "drivers/place/auto_release.cr", + ) + driver.repository = repository + driver.save! + + _, headers = Spec::Authentication.authentication(sys_admin: false, support: false) + path = File.join(Drivers.base_route, driver.id.as(String), "readme") + result = client.get(path, headers: headers) + result.status_code.should eq 403 + end + + it "GET /drivers is allowed for support-only callers" do + _, headers = Spec::Authentication.authentication(sys_admin: false, support: true) + result = client.get(Drivers.base_route, headers: headers) + result.success?.should be_true + end + end + describe "scopes" do before_each do HttpMocks.core_compiled diff --git a/spec/controllers/systems_spec.cr b/spec/controllers/systems_spec.cr index 48c45439..7c30ce7c 100644 --- a/spec/controllers/systems_spec.cr +++ b/spec/controllers/systems_spec.cr @@ -437,6 +437,16 @@ module PlaceOS::Api id.should eq email end + it "module_id is 403 for non-admin / non-support callers" do + mod = Model::Generator.module.save! + module_id = mod.id.as(String) + _, headers = Spec::Authentication.authentication(sys_admin: false, support: false) + + params = HTTP::Params.encode({"module_id" => module_id}) + result = client.get("#{Systems.base_route}?#{params}", headers: headers) + result.status_code.should eq 403 + end + it "module_id filters systems by modules" do Model::ControlSystem.clear num_systems = 5 diff --git a/spec/controllers/zones_spec.cr b/spec/controllers/zones_spec.cr index c7eeffd7..cebb3ceb 100644 --- a/spec/controllers/zones_spec.cr +++ b/spec/controllers/zones_spec.cr @@ -624,6 +624,34 @@ module PlaceOS::Api end end + describe "GET /zones/:id/triggers access control" do + it "returns 403 for non-admin / non-support callers" do + zone = Model::Generator.zone.save! + _, headers = Spec::Authentication.authentication(sys_admin: false, support: false) + + result = client.get( + path: "#{Zones.base_route}#{zone.id}/triggers", + headers: headers, + ) + result.status_code.should eq 403 + + zone.destroy + end + + it "is allowed for support-only callers" do + zone = Model::Generator.zone.save! + _, headers = Spec::Authentication.authentication(sys_admin: false, support: true) + + result = client.get( + path: "#{Zones.base_route}#{zone.id}/triggers", + headers: headers, + ) + result.success?.should be_true + + zone.destroy + end + end + describe "scopes" do Spec.test_controller_scope(Zones) Spec.test_update_write_scope(Zones) diff --git a/src/placeos-rest-api/controllers/drivers.cr b/src/placeos-rest-api/controllers/drivers.cr index dc01cbe7..a5fd7acd 100644 --- a/src/placeos-rest-api/controllers/drivers.cr +++ b/src/placeos-rest-api/controllers/drivers.cr @@ -13,6 +13,14 @@ module PlaceOS::Api before_action :check_admin, except: [:index, :show, :readme] + # Restrict driver listing and readme access to admin/support users. These + # endpoints expose internal infrastructure detail (driver inventory, driver + # README files that may include configuration notes or credential examples) + # that should not be available to standard authenticated users. + # Reported by DataArt pentest 2026-05 (McKinsey Converge) as H1 finding — + # "Missing function-level access controls". + before_action :check_support, only: [:index, :readme] + ############################################################################################### @[AC::Route::Filter(:before_action, except: [:index, :create])] diff --git a/src/placeos-rest-api/controllers/systems.cr b/src/placeos-rest-api/controllers/systems.cr index 94153e96..48bac5f0 100644 --- a/src/placeos-rest-api/controllers/systems.cr +++ b/src/placeos-rest-api/controllers/systems.cr @@ -257,6 +257,15 @@ module PlaceOS::Api @[AC::Param::Info(description: "return systems which are signage", example: "true")] signage : Bool? = nil, ) : Array(::PlaceOS::Model::ControlSystem) + # Restrict module_id-based lookups to admin/support. Looking up which + # system contains a given module is an internal infrastructure query that + # should not be exposed to standard authenticated users. Reported by + # DataArt pentest 2026-05 (McKinsey Converge) as H1 finding — + # "Missing function-level access controls". + if module_id && !user_support? + raise Error::Forbidden.new("module_id lookups require admin or support") + end + elastic = ::PlaceOS::Model::ControlSystem.elastic query = ::PlaceOS::Model::ControlSystem.elastic.query(search_params) diff --git a/src/placeos-rest-api/controllers/zones.cr b/src/placeos-rest-api/controllers/zones.cr index 398b6822..844b4371 100644 --- a/src/placeos-rest-api/controllers/zones.cr +++ b/src/placeos-rest-api/controllers/zones.cr @@ -13,10 +13,15 @@ module PlaceOS::Api # Scopes ############################################################################################### - before_action :can_read, only: [:index, :show] + before_action :can_read, only: [:index, :show, :trigger_instances] before_action :can_write, only: [:create, :update, :destroy, :remove] - before_action :check_support, only: [:zone_execute] + # Restrict zone trigger listing to admin/support. `:trigger_instances` (the + # GET /zones/:id/triggers route) exposes internal automation/PlaceOS-driver + # state that should not be available to standard authenticated users. + # Reported by DataArt pentest 2026-05 (McKinsey Converge) as H1 finding — + # "Missing function-level access controls". + before_action :check_support, only: [:zone_execute, :trigger_instances] # Response helpers ############################################################################################### From 24d033fc82867040119d8c1df18ac206ba8f16a4 Mon Sep 17 00:00:00 2001 From: Cam Reeves Date: Mon, 18 May 2026 14:22:10 +1000 Subject: [PATCH 2/2] review: drop module_id gate and scrub external references from comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - revert the Systems#index module_id check_support guard — the leak is low-impact and the inline branch complicates index. Drops the matching spec too. - shorten the security comments on drivers#index/readme and the zones :trigger_instances filter; remove the external project/report attribution. Co-Authored-By: Claude Opus 4.7 (1M context) --- spec/controllers/systems_spec.cr | 10 ---------- src/placeos-rest-api/controllers/drivers.cr | 4 +--- src/placeos-rest-api/controllers/systems.cr | 9 --------- src/placeos-rest-api/controllers/zones.cr | 7 ++----- 4 files changed, 3 insertions(+), 27 deletions(-) diff --git a/spec/controllers/systems_spec.cr b/spec/controllers/systems_spec.cr index 7c30ce7c..48c45439 100644 --- a/spec/controllers/systems_spec.cr +++ b/spec/controllers/systems_spec.cr @@ -437,16 +437,6 @@ module PlaceOS::Api id.should eq email end - it "module_id is 403 for non-admin / non-support callers" do - mod = Model::Generator.module.save! - module_id = mod.id.as(String) - _, headers = Spec::Authentication.authentication(sys_admin: false, support: false) - - params = HTTP::Params.encode({"module_id" => module_id}) - result = client.get("#{Systems.base_route}?#{params}", headers: headers) - result.status_code.should eq 403 - end - it "module_id filters systems by modules" do Model::ControlSystem.clear num_systems = 5 diff --git a/src/placeos-rest-api/controllers/drivers.cr b/src/placeos-rest-api/controllers/drivers.cr index a5fd7acd..ed44c4f0 100644 --- a/src/placeos-rest-api/controllers/drivers.cr +++ b/src/placeos-rest-api/controllers/drivers.cr @@ -13,12 +13,10 @@ module PlaceOS::Api before_action :check_admin, except: [:index, :show, :readme] - # Restrict driver listing and readme access to admin/support users. These + # Restrict driver listing and readme access to admin/support. These # endpoints expose internal infrastructure detail (driver inventory, driver # README files that may include configuration notes or credential examples) # that should not be available to standard authenticated users. - # Reported by DataArt pentest 2026-05 (McKinsey Converge) as H1 finding — - # "Missing function-level access controls". before_action :check_support, only: [:index, :readme] ############################################################################################### diff --git a/src/placeos-rest-api/controllers/systems.cr b/src/placeos-rest-api/controllers/systems.cr index 48bac5f0..94153e96 100644 --- a/src/placeos-rest-api/controllers/systems.cr +++ b/src/placeos-rest-api/controllers/systems.cr @@ -257,15 +257,6 @@ module PlaceOS::Api @[AC::Param::Info(description: "return systems which are signage", example: "true")] signage : Bool? = nil, ) : Array(::PlaceOS::Model::ControlSystem) - # Restrict module_id-based lookups to admin/support. Looking up which - # system contains a given module is an internal infrastructure query that - # should not be exposed to standard authenticated users. Reported by - # DataArt pentest 2026-05 (McKinsey Converge) as H1 finding — - # "Missing function-level access controls". - if module_id && !user_support? - raise Error::Forbidden.new("module_id lookups require admin or support") - end - elastic = ::PlaceOS::Model::ControlSystem.elastic query = ::PlaceOS::Model::ControlSystem.elastic.query(search_params) diff --git a/src/placeos-rest-api/controllers/zones.cr b/src/placeos-rest-api/controllers/zones.cr index 844b4371..fd59a613 100644 --- a/src/placeos-rest-api/controllers/zones.cr +++ b/src/placeos-rest-api/controllers/zones.cr @@ -16,11 +16,8 @@ module PlaceOS::Api before_action :can_read, only: [:index, :show, :trigger_instances] before_action :can_write, only: [:create, :update, :destroy, :remove] - # Restrict zone trigger listing to admin/support. `:trigger_instances` (the - # GET /zones/:id/triggers route) exposes internal automation/PlaceOS-driver - # state that should not be available to standard authenticated users. - # Reported by DataArt pentest 2026-05 (McKinsey Converge) as H1 finding — - # "Missing function-level access controls". + # Restrict zone trigger listing to admin/support — `:trigger_instances` + # (GET /zones/:id/triggers) exposes internal automation/driver state. before_action :check_support, only: [:zone_execute, :trigger_instances] # Response helpers