Skip to content

Apply the site boundary to the API, not just to the management pages - #1229

Merged
mastacontrola merged 1 commit into
dev-branchfrom
site-api-scope
Aug 19, 2026
Merged

Apply the site boundary to the API, not just to the management pages#1229
mastacontrola merged 1 commit into
dev-branchfrom
site-api-scope

Conversation

@mastacontrola

Copy link
Copy Markdown
Member

The bug

The site plugin's only filtering hook is registered on HOST_DATA and GROUP_DATA. Both handlers open with global $node; global $sub; and switch on them — the management pages. Nothing under api/ fires those events.

The plugin's other hook (addsiteapi.hook.php) is registered on API_VALID_CLASSES, API_GETTER and the two data mappings. It adds site and sitehostassociation as API classes and stamps siteID onto host payloads. It filters nothing.

So a site-restricted user saw their site in the grid and every host on the server through /fog/host/list.

Reachable without an API token and without uAllowAPI: Route::__construct() skips _testToken()/_testAuth() entirely when self::$FOGUser->isValid(), so the same browser session that renders the filtered grid returns the unfiltered set one URL away. With API credentials, _requireAuthorized() allows a non-admin list, listdetails, search, names, ids, indiv and active on host and group. Both require FOG_API_ENABLED, which is off by default.

Not a regression — the API was never site-scoped on this line. 1.6 does not share it: object scope there lives in the query and the per-object routes are gated at dispatch.

The fix

Core gains one seam and stays ignorant of sites. API_SCOPE_IDS asks whoever is listening which object ids the acting user may see. With no plugin, an unrestricted user, or nobody logged in, the answer stays null and every read behaves exactly as before.

Applied in How Why there
listem(), search() filters the rows neither route has a LIMIT, so this is exact and keeps count honest
names(), ids() folds into the WHERE those two only ever produce ids
runMatches() gates indiv/update/delete/task/cancel one place to audit, beside the uType check and for the same reason

The return is a tri-state and that is the design. null = no boundary, an array narrows, an empty array is a real answer meaning "nothing". if (!$ids) is true for both null and array(), so a caller written that way shows all 2079 hosts to the one user entitled to none. An empty intersection is handed to _buildWhere() as an empty array on purpose — that compiles to WHERE 1=0 rather than dropping the term.

The membership rule moves to Site as static methods and both hooks call it. Two statements of who may see what is a boundary that is decorative the first time they disagree.

Behaviour changes, both toward the safe answer

  • getGroupIDbySite() returns nothing for a user with no hosts, instead of querying GroupAssociation with an empty hostID set.
  • isRestricted() on a user with no restriction row returns false instead of an undefined-index notice.

Verification

Against the 1.5 lab — 2079 hosts, a user entitled to 2:

Route shipped patched admin (patched)
list host 126 2 126
search host "a" 68 2 68
names host 2079 2 2079
ids host 2079 2 2079
ids host (field=name) 2079 2 2079

Also verified:

  • A caller's own filter is intersected, not replaced: ids?id=1,99999[1] for both users.
  • A restricted user belonging to no site gets 0 on every route, not 2079.
  • With nobody logged in — the daemons and status/*.php, which reach Route::ids() constantly — _scopeIDs() returns null and ids('host') still answers 2079.
  • The per-object gate allows host 1 (in scope) and denies host 7075 (out of scope).
  • The management hook's own helpers return the same answers after being made to delegate.

Driver, fixture and teardown: scripts/background_scripts/{probe,fixture,teardown}_site_api_scope.*

Tests

tests/site-api-scope.test.php — 28 checks, static so it runs in the pre-commit hook. Pins that each read route still consults the boundary, the dispatcher still gates per-object routes, the null comparisons keep both halves, the deny-all branch still returns an array, _buildWhere() still compiles an empty IN to 1=0, and the membership rule is stated once.

Eight mutations checked, all caught: listem() ignoring the boundary, names() dropping the WHERE narrowing, the dispatch gate removed, the tri-state tested for falsiness (two forms), deny-all returning null, the plugin no longer answering the event, and the boundary applied with nobody logged in.

Full suite: 16 passed, 0 failed.

Downstream

None. API_SCOPE_IDS is a new event with one listener; Route::$validClasses is unchanged, so FogApi's hardcoded class list needs no sync.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GN7hADN5QLzoxXWDA6xeUk

The site plugin's only filtering hook is registered on HOST_DATA and
GROUP_DATA, and both handlers switch on the global $node/$sub the
management pages set. Nothing under api/ fires those events, and the
plugin's other hook -- API_VALID_CLASSES, API_GETTER, the two data
mappings -- adds `site` as an API class and stamps siteID onto host
payloads without filtering anything.

So a site-restricted user saw their site in the grid and every host on
the server through /fog/host/list, on the same credentials. No API token
and no uAllowAPI needed either: Route skips API auth entirely when a
management session is already valid, so the same browser that shows the
filtered grid returns the unfiltered set one URL away.

Core gains one seam and stays ignorant of sites. API_SCOPE_IDS asks
whoever is listening which object ids the acting user may see; with no
plugin, or an unrestricted user, or nobody logged in, the answer stays
null and every read behaves exactly as before.

  listem(), search()   filter the rows -- neither route has a LIMIT, so
                       this is exact and keeps 'count' honest
  names(), ids()       fold it into the WHERE, since those two only ever
                       produce ids
  runMatches()         gates indiv/update/delete/task/cancel in one
                       place, beside the uType check and for the same
                       reason

The return is a tri-state and that is the whole design: null means no
boundary, an array narrows, and an EMPTY array is a real answer meaning
"nothing". `if (!$ids)` is true for both null and array(), so a caller
written that way shows all 2079 hosts to the one user entitled to none.
An empty intersection is passed to _buildWhere() as an empty array on
purpose -- that compiles to `WHERE 1=0` rather than dropping the term.

The membership rule moves to Site as static methods and BOTH hooks call
it, because two statements of who may see what is a boundary that is
decorative the first time they disagree. The management hook's helpers
are now thin wrappers, and its search branch intersects against the same
host set instead of asking SiteHostAssociation a second question of its
own. Two behaviour changes fall out, both toward the safe answer:
groupIDsForSites() returns nothing for a user with no hosts rather than
querying GroupAssociation with an empty hostID set, and isRestricted()
on a user with no restriction row returns false instead of a notice.

Verified against the 1.5 lab -- 2079 hosts, a user entitled to 2:

                     shipped      patched      admin
  list host          126          2            126
  search host "a"    68           2            68
  names host         2079         2            2079
  ids host           2079         2            2079
  ids host name      2079         2            2079

A caller's own filter is intersected, not replaced: ids?id=1,99999
returns [1] for both users. A restricted user belonging to NO site gets 0
on every route, not 2079. With nobody logged in -- the daemons and the
status endpoints, which reach Route::ids() constantly -- _scopeIDs()
returns null and ids('host') still answers 2079. The per-object gate
allows host 1 and denies host 7075.

Driver, fixture and teardown:
scripts/background_scripts/{probe,fixture,teardown}_site_api_scope.*

tests/site-api-scope.test.php pins the shape statically, so it runs in
the pre-commit hook: each read route still consults the boundary, the
dispatcher still gates per-object routes, the null comparisons keep both
halves, the deny-all branch still returns an array, and the membership
rule is stated once. Seven mutations checked, all caught.

Co-Authored-By: Claude <noreply@anthropic.com>
@mastacontrola
mastacontrola merged commit d000ecf into dev-branch Aug 19, 2026
3 checks passed
@mastacontrola
mastacontrola deleted the site-api-scope branch August 19, 2026 21:56
mastacontrola pushed a commit that referenced this pull request Aug 19, 2026
#1229 built the boundary on dev-branch. Updates the
scan's dev-branch section from "flagged, no fix proposed" to what landed
and what it was verified against.

Co-Authored-By: Claude <noreply@anthropic.com>
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.

2 participants