Repository navigation
feat(auth): audit-log the is_admin bypasses on non-admin procedures - #5126
Merged
Merged
Conversation
#5124 gated admin_access on adminOnly:true and adminProcedure, which covers the /admin console but misses the is_admin escape hatch on regular procedures — the path a Kilo employee actually takes to read a customer organization's data. Those paths emitted nothing. Adds kind:"kilo_admin_elevation" alongside the existing kind:"admin_guard", plus reason/target, and funnels the bypasses through elevateViaKiloAdmin/recordKiloAdminElevation so the choke point is structural rather than something each site has to remember.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Executive SummaryIncremental review of the async conversion and REST-attribution fallback in the elevation audit helpers found no new issues — all call sites are awaited and the never-throw guarantee is preserved; one previously flagged sink error-handling warning and one null-attribution suggestion remain open from the prior review. Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (9 files in incremental diff)
Carried from previous review (unchanged file, verified still present): Fix these issues in Kilo Cloud Previous Review Summary (commit 4b5f116)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 4b5f116)Status: 3 Issues Found | Recommendation: Address before merge Executive SummaryThe audit-logging elevation mechanism is well-structured and covers the intended bypasses, but there is a minor robustness issue in the sink error handling and two observability gaps worth noting. Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (16 files)
Reviewed by kimi-k3 · Input: 56.3K · Output: 12.3K · Cached: 464.4K Review guidance: REVIEW.md from base branch |
`recordKiloAdminElevation` hard-coded `surface: 'trpc'` and read request
metadata only from a tRPC context. But `ensureOrganizationAccess`,
`getOrganizationsAccessRoles`, and `ensureOrganizationAccessAndFetchOrg` are
also called from REST route handlers that hand-roll a `{ user }` context —
`/api/cloud-agent/sessions/stream-ticket`, `/api/auto-routing/settings`,
`/api/code-indexing/enabled`, the integration connect/callback handlers, and
the org integration reinstall routes.
An admin elevation on those paths was therefore emitted as a tRPC request with
null route/IP and, worse, `authVia: 'session'` — a fabricated value on the one
field documented as the token-compromise discriminator. A stolen admin bearer
token reading an arbitrary customer organization over REST looked like a
web-console session.
Make the two elevation emitters async and dispatch on the presence of
`ctx.trpcPath`: tRPC contexts are attributed from the context as before,
anything else delegates to the existing `recordKiloAdminElevationForRequest`,
which derives surface/authVia/route/IP from the request headers. Keeping one
emitter name means a future `is_admin` bypass is attributed correctly on either
transport by default, rather than depending on the author picking the right
variant.
`tokenSource` stays null for REST callers, which pass a bare `{ user }`;
`authVia: 'token'` still fires, and null is already a documented value for that
field. Noted inline so it is not mistaken for an oversight.
emilieschario
approved these changes
Aug 7, 2026
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.
Follow-up to #5124.
Why
#5124 gates
admin_accessonadminOnly: trueandadminProcedure. That covers the/adminconsole, but Kilo's highest-value admin capability — an employee reading an arbitrary customer organization's data — flows through theis_adminescape hatch on regular, non-admin procedures. Those paths touch neither choke point, so they emitted nothing.The risk isn't just the missing events, it's the false confidence: an Axiom detection built on
admin_accesswould look complete while the most sensitive paths stayed invisible. This closes that.How
Every event now carries two orthogonal dimensions instead of one, so
event:"admin_access"still returns the whole picture and can then be split:surface—rest|trpc(unchanged, the transport)kind—admin_guard(feat(auth): audit-log all admin access (choke-point, IP-independent) #5124's two choke points) |kilo_admin_elevation(this PR), withreasonandtargetRather than sprinkling emit calls, the bypasses funnel through
elevateViaKiloAdmin/recordKiloAdminElevation, so the choke-point property is structural instead of relying on everyone remembering to log.elevateViaKiloAdminreturns the elevated value, which makes the grant and its audit record a single expression. TheensureOrganizationAccessfamily was the natural starting point — it already centralises the org-access sites, and covers the 19baseProcedureorg procedures behind it.route/methodfor emits inside a resolver come from a newbaseProceduremiddleware that publishes the procedure path onto the context; resolvers only receivectx, not the middleware'spath/type, so without this the events couldn't name the procedure they came from.For the wasteland/gastown token routes the event is emitted at mint time, since the elevation is exercised in the worker where this app emits nothing at all.
Two emits are placed before their DB lookup so an admin probing a non-existent organization is still attributed, rather than vanishing into the 404.
#5124's properties are preserved: audit logging still never denies a legitimate admin action (all three entry points are non-throwing), identity remains the primary key with
iprecorded but not depended on, and no token, header, cookie or secret is ever included.Notes for review
admin_guardfromadminProcedureand akilo_admin_elevationfromensureOrganizationAccess. Both are true; deduping would need per-request state.resolveScopedConfig's personal branch is deliberately not treated as an elevation. It already requiresowner_id === ctx.user.id, sois_adminonly gates a staff-only feature on the caller's own config — no customer data is reached. Emitting there would add false positives to the elevation stream.kiloUserId+ timestamp within the token lifetime, not exact.generateApiTokenhas nojti; adding one only pays off once the downstream workers log it, which is cross-repo.ctxinstead ofuserId+isGlobalAdmin. That's what makes the bypass auditable in one place, and it removes theisGlobalAdmin: ctx.user.is_adminrepetition across 12 call sites that made an unaudited bypass easy to add.admin-access-log.tsnow states the coverage limit explicitly and names the funnel new bypasses must use, so the next reader isn't misled by the feat(auth): audit-log all admin access (choke-point, IP-independent) #5124 title.