security(core): scope auth tokens with explicit CRUD permissions and server-assigned login (24.05) - #7964
Open
ar2rsawseen wants to merge 16 commits into
Open
security(core): scope auth tokens with explicit CRUD permissions and server-assigned login (24.05)#7964ar2rsawseen wants to merge 16 commits into
ar2rsawseen wants to merge 16 commits into
Conversation
…server-assigned login Token scope was expressed as an app list and an endpoint regex, which verify_token compared against the request path and, when present, the request's app_id. That is a data-scoping control: it does not describe what the resolved member may do, and rights.js did not carry it past token resolution, so a token authorized whatever its owner could. Login capability was likewise inferred from the token's purpose string, which is supplied by the caller at creation. Tokens now carry explicit permissions and an explicit login capability. - token_permission stores a member.permission-shaped grant on the token. rights.js intersects it with the owner's own permissions as soon as the member is loaded, before any validator authorizes anything, so every existing feature check becomes scope-aware without being touched. The intersected member does not carry global_admin. The intersection is recomputed per request, so it follows the owner's current permissions. - A grant is bounded by the credential that creates it, not by the owner. Because the intersection has already happened, params.member is that credential's authority, and isPermissionSubset refuses anything wider. An "all" grant may only be passed on by a holder of "all", since it covers features that do not exist yet. - can_login is a server-assigned property of the token. It is set where the server establishes or propagates a session (setLoggedInVariables, the renderer, the ban-warning mail), and by /i/token/create only when the creating credential holds it and the child is not narrowed. purpose returns to being a description. - Token creation, listing and deletion require a full-permission credential, since each of them hands out, exposes or revokes the owner's credentials. This branch also takes hasAdminAccess from 5e60144, which is on later versions but was never backported here. It treated an app with no permission entry for the member as one they administer. The permission model depends on that being correct, since an intersected member expresses "no access" as the absence of an entry. Tokens created before this change are unaffected: with no token_permission they are neither intersected nor re-scoped, the endpoint regex still applies to them, and /login/token still accepts the unrestricted login-purpose tokens it accepted previously. The token manager grants app, CRUD and feature permissions using the same permission grid and helpers as user management, instead of endpoint regexes. Covered by unit tests for the permission algebra and the token record, and by API tests for the create, login, list and delete boundaries. Reported through the security bug bounty program (received 2026-08-18). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ants A token created without an explicit permission carries no token_permission, and a token with no token_permission is bounded only by its owner. A scoped credential could therefore create a child wider than itself by simply omitting the parameter: the subset check had no permission object to compare and so never ran, which meant the omission itself was the way around it. Refuse instead - a credential that is itself scoped has to say what it grants. Also brings the existing token-list test to the behaviour this model defines. /o/token/list returns whole token documents and a document's _id is the token itself, so listing is restricted to a full-permission credential; an endpoint-scoped token is refused there even when its endpoint matches. The login test now supplies the permission the token already holds, so what it asserts is the login grant being refused rather than the missing permission. Found by the regression tests added with the permission model. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit 7d70a17)
Three layout defects in the permission drawer. - The two permission cards overflowed the drawer. is-autosized gives each width:100%, and a flex item's default min-width:auto stops it shrinking below its own text, so the card with the longer description pushed the other one off screen. They now share the line evenly and are allowed to shrink. - The feature grid's column headers and its per-feature checkboxes were laid out independently, flex-end against space-between, so the checkboxes did not sit under the headers. Both now use one name column plus four equal cells, and in the header the column name sits above its checkbox rather than beside it, since inline the label widens the cell and pushes the checkbox off the centre the rows align to. - The app selector was a plain el-select and showed no app icon. It now uses cly-app-select, which is the component that renders them. That last one needs cly-checklistbox to accept an option-prefix slot. cly-select-x forwards the slot only to cly-listbox, which serves single-select, so no multi-select app dropdown in the dashboard can show icons today. The slot is additive and renders only when a caller passes one, so every existing checklistbox is unchanged. The token-manager stylesheet existed but was empty and was never imported by manifest.scss. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit 3c81f38)
The existing cases cover the token manager itself - what a token may create, list, delete and
sign in with. They say nothing about what a token can actually reach, which is the point of
the model, so this adds the same checks against endpoints that go through the ordinary
validators.
A token is scoped along three axes, and each is now exercised on a real endpoint:
- app: a token granted read on one app is refused on another app of the same owner
- CRUD type: a read token is refused an update, and an update token is refused a delete
- feature: a token granted core is refused an events endpoint, and vice versa
with the granted combination allowed through in each case.
Another app is refused by two independent layers, and both are covered. The token's app list
is derived from its permission, so verify_token normally rejects the request before a member
is loaded ("Token not valid"). Passing apps explicitly widens that list without widening the
permission, which gets past the app check and leaves the permission intersection to refuse it
("User does not have right") - so the second layer is not merely shadowed by the first.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
(cherry picked from commit ce2fa9d)
…hanging under a token Three areas the earlier cases left open. Token shapes. A token is created in a number of shapes - one feature, several features, several apps, a whole CRUD type - and each is now minted and then exercised on a real endpoint of a real feature: alerts carries create, read and update, events carries update and delete, so every CRUD type is covered on a plugin and on core. Each token reaches the endpoint it was granted, is refused another access type of the same feature, and is refused another feature entirely. The "all" grant is checked to reach features that were never named, and a two-app token to reach both apps while still being held to its one feature. The login path. /login/token is the only way a token becomes a dashboard session and /session is what that session is then checked with, and neither is reached through the api_key. A token carrying login permission is redeemed and the session it opens is then confirmed live; a scoped token is refused at the same door. Permissions changing under a token. The intersection is recomputed per request rather than frozen at creation, so a member is created, mints a token with the permissions they hold, loses the app, and the token loses it with them without being touched. One note on the assertion used for the granted case. "Not 401" is too weak on its own: an endpoint whose plugin is not loaded answers 400 "Invalid path", which let an earlier draft of these tests pass without ever reaching a validator. The check now rejects that answer explicitly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit 150563d)
Two things that made these cases depend on the environment rather than on the code. The plugin cases need that plugin installed. Where it is not, its endpoints answer 400 "Invalid path", so they are now skipped explicitly after a single probe rather than run against an endpoint that was never there. The core cases do not depend on any plugin and always run. The two core cases checked their "another feature" refusal against /o/app_users/download, which rejects the request on its own parameters before any validator sees it, so the refusal proved nothing. They now check it against a core read, which reaches the validator. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit b467847)
Adding the option-prefix slot wrapped the option label in two spans across several lines, so the rendered el-checkbox__label gained nested nodes and surrounding whitespace. Callers that assert on that label exactly stopped matching: the alerts UI test looks for /^8.0.9$/ inside el-checkbox__label and timed out. The slot is now emitted bare, with no wrapper and no v-if, immediately before the label expression. A slot nobody fills renders nothing at all - not even the placeholder a falsy v-if leaves behind - so for every existing checklistbox the label is byte-identical to before. The app icon supplied by cly-app-select brings its own spacing, so the stylesheet rule the wrapper needed is gone too. Caught by the dashboard UI tests in CI, which pass on the base branch and failed here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit 7543ce3)
The granted case for update on alerts called /i/alert/status with no parameters of its own. The handler parses qstring.status, and where that parse is not guarded it throws inside the callback and never replies, so the request hung and the case timed out after 50s rather than failing on an authorization outcome. Each probed endpoint is now given whatever its handler needs to answer, which is what a real caller would send. The assertion is unchanged and still about authorization only. Found by CI on the 24.05 backport, where that parse is unguarded; master guards it with a try/catch and answered 500, so the same case passed there. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit 9062ee6)
…eterministically That case needs a member of its own, and the cleanup suite later asserts that exactly one user remains, so a member left behind fails a test in a different file. The cleanup went through /i/users/delete and ignored its result, which made the whole thing conditional on that call succeeding. The member and any tokens they own are now removed directly, which is what the rest of this file already does for tokens. Found by CI on master: "Verify user deletion should return one user" failed while the token cases themselves passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit db4e2c0)
The render endpoint mints an owner-authority login token and drives a headless dashboard session with it. Gate it with isScopedCredential so a scoped token_permission token, or a legacy app/endpoint-restricted one, cannot have a view rendered with more authority than the token itself carries. api_key callers and unrestricted session tokens are unaffected. This is the same rule already applied to token create, list and delete. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ar2rsawseen
commented
Aug 24, 2026
ar2rsawseen
commented
Aug 24, 2026
…its own token, and close the second factor to a scoped token
Two ways a scoped token still reached more than it was granted.
/o/actions. Its countly-token branch calls authorize.verify_return directly and never
reaches a rights validator, so applyTokenScope never ran for it: only the token's app
restriction was checked, and its permissions were not consulted at all. A token scoped to
another feature on the same app read heatmaps. The branch now asks for the token document
(return_data), keeps it on params.token_data, resolves the owner, bounds that member with
rights.applyTokenScope exactly as rights.js does, and requires a read right for the views
feature before it goes any further. Endpoint scoping stays disabled for permission-carrying
tokens as intended - the point of the review note was that something had to apply the
permissions instead, and now something does.
applyTokenScope is exported for it. The read right check honours the legacy user_of
membership validateRead still honours, and refuses a locked account, so an owner who could
read this through their api_key is not refused here.
/i/two-factor-auth. enable, disable and generate-qr-code go through validateUser, which
bounds what a scoped token may touch per app but does not reject the request - and an
account level route has no app to bound. disable asks for no current code. A token narrowed
to one feature on one app could therefore switch off, or replace, the factor protecting
everything its owner can reach. All three now refuse a scoped credential, the same rule the
token manager and /o/render already apply.
Audited the rest of the validateUser only routes for the same shape: /i/token/create is
already ceilinged by the creating credential, /o/token/check takes the token it reports on
as a parameter, /o/countly_version and getCurrentUserApps mutate nothing. The 24.05 branch
has no generate-qr-code method, so it gets the two it has.
Also: on the platform, both unit suites this PR adds live in test/unit-tests, which the
platform's test:unit glob ('test/unit/*.unit.js' and 'plugins/*/tests/**/*.unit.js') does
not match, so neither has ever run in CI there. Moved to test/unit with the .unit.js
suffix. 25 passing once they do.
The four two-factor-auth cases I added were the only new failures in test-api-core on
this branch. They allowed 403 or 404, on the assumption that a build without the plugin
answers 404. It does not: requestProcessor answers 400 "Invalid path", so the cases
failed everywhere the plugin is not enabled - which is the CI test build.
Widening the allowed set to include 400 would have been the wrong repair. A case that
accepts 400, 403 and 404 passes whether or not the guard exists, which is worse than no
case at all. They now probe once for the route and skip with a reason when it is absent,
and assert exactly 403 when it is there.
Skipping leaves the guard unproven in this build, so the coverage that does not depend on
the plugin being installed is added beside it: a unit suite that reads the plugin source
and requires refuseScopedCredential at each method that mutates the factor, before the
write rather than after it, and requires the global-admin methods to keep using
validateUserForGlobalAdmin. Removing the guard from one arm fails it, checked by doing
exactly that. On 24.05 the generate-qr-code case reports pending, because that branch has
no such method.
For the record on the rest of this branch's test-api-core run: the other three failures
("user permission when app is deleted", "correct admins and users", "should return one
user") are not from this change. The same three fail on #7930, which carries none of it,
and test-api-core passed in the same hour on #7923, #7935, #7941, #7955, #7970, #7894 and
#7871. That is the flaky trio, not a regression.
# Conflicts: # api/utils/rights.js
Member
Author
Conflict resolved —
|
# Conflicts: # api/utils/rights.js # plugins/views/api/api.js # plugins/views/api/parts/viewsUtils.js
The changelog is generated from PR and commit titles later, so an entry written by hand here is duplicated work at best. It is also the single worst file in this wave for conflicts: every merge to the base appends a line, which re-conflicts every open branch that also appends one. Eight of the sixteen conflicts across these security PRs today were this file and nothing else, and two of them came back within the hour. Only the lines this branch added are removed - the file is otherwise the base's, and the change here was purely additive, so nothing else moves.
Member
Author
Merged the base again — #7936 and #7940 had landedThree code conflicts, all the same shape:
Verified after: eslint clean (with Also dropped the CHANGELOG entry from this branch — see the separate note. |
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
Backport of #7963 to
release.24.05.Replaces the token
app/endpointscoping model with explicit CRUD permissions, and makes permission to open a dashboard session an explicit, server-assigned property of the token. See #7963 for the full description of the model.One addition specific to this branch
This backport also takes
hasAdminAccessfrom5e6014496da(SER-2766), which is on later versions but was never backported here. On this branch it treats an app with no permission entry for the member as one they administer, becauseisAdminstarts astrueand is only cleared when an entry exists and is notall.That has to be corrected for the permission model to mean anything here: an intersected member expresses "no access to app B" as the absence of an entry for B, which this branch would otherwise read as administering B. It is also reachable independently of tokens, since
validateUserForWritegates solely onhasAdminAccess.Verified on this branch before and after: a member with access to app A only previously returned
truefromhasAdminAccess(member, B),hasReadRight('core', B, member)andhasDeleteRight('core', B, member); all now returnfalse, while legitimate access to A is unchanged.Verification
Ported by cherry-pick; three conflicts resolved by hand (the
rights.jsimport list, which lacksgetBaseAppFilterhere, and the CHANGELOG and localization sections, which differ from master).The same 21 unit tests pass on this branch. The 5 unrelated failures in
api.utils.common.jsandapi.utils.countly-request.jsare pre-existing onrelease.24.05and fail identically without this change.The API-level suite was exercised on
master(see #7963: 238 passing / 21 failing, the 21 pre-existing and identical on pristine master, with all 16 token cases green). This branch carries the same two commits, including the fix for a hole those tests found - a scoped credential could mint an unrestricted child by omitting thepermissionparameter, since the subset check then had nothing to compare.Note the three token-management endpoints now require a full-permission credential, so a legacy app- or endpoint-restricted token is refused at
/i/token/create,/o/token/listand/i/token/delete. That is deliberate:/o/token/listreturns whole token documents and a document's_idis the token itself.Related
master: security(core): scope auth tokens with explicit CRUD permissions and server-assigned login #7963Supersedes the earlier containment PR #7958, closed in favour of this model.
Reported through the security bug bounty program (received 2026-08-18).
🤖 Generated with Claude Code