fix(security): restrict driver listing, driver readme, zone triggers, and systems module_id lookups to support users#435
Merged
Conversation
…and module_id lookups to support users 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) <noreply@anthropic.com>
stakach
requested changes
May 18, 2026
| # 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 — |
Member
There was a problem hiding this comment.
remove client and pen test information from comments
| # "Missing function-level access controls". | ||
| if module_id && !user_support? | ||
| raise Error::Forbidden.new("module_id lookups require admin or support") | ||
| end |
Member
There was a problem hiding this comment.
let's undo this change. It's not a big issue and complicates the controller.
- 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) <noreply@anthropic.com>
github-actions
Bot
added
type: bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds support-level access checks to four engine endpoints flagged in the DataArt pentest of McKinsey Converge (2026-05) as H1 — Missing function-level access controls. All four are authenticated, but the previous gate was just `can_read` (or no role check at all on the module_id branch), so any standard user could enumerate internal infrastructure detail.
Why `Systems#index` is gated inline rather than via `before_action`
`/systems` has many legitimate non-support use cases (end-user system search by name, by zone, by email). The pentest flagged specifically the `module_id` query parameter, which is an operator-style "which system contains this module?" lookup. Gating the whole index would break normal users. The inline check only raises Forbidden when `module_id` is present and the caller is not admin/support.
Sibling endpoints intentionally NOT changed
Per the Converge team review, three sibling endpoints flagged in the same H1 batch stay as-is:
Content-Typestarts with media-type 'application/json' #8) — room/zone metadata stays `can_read_guest`Test plan
🤖 Generated with Claude Code