Skip to content

feat(admin): add privileged access contract and inventory - #9610

Open
pandemicsyn wants to merge 9 commits into
mainfrom
feat/admin-assurance-inventory
Open

pandemicsyn wants to merge 9 commits into
mainfrom
feat/admin-assurance-inventory

Conversation

@pandemicsyn

@pandemicsyn pandemicsyn commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

What changes

Part 1 of the admin lockdown stack (#9610 → #9615 → #9616 → #9618). Groundwork only; no access is removed.

  • Adds a shared privileged-access contract (actor, credential type, action class) that the later PRs build on.
  • Catalogues every admin-capable route, procedure and is_admin check, with tests that fail when a new one is added without being classified.
  • Logs every admin access with how it was authenticated (browser session vs API key) and whether the planned rules would block it.
  • One behaviour change: a request with an empty Authorization header now gets 401 instead of falling back to the session cookie.

Before merge

Nothing. No migrations, env vars or config.

Verification

Automated tests only.

Visual Changes

N/A

@pandemicsyn
pandemicsyn marked this pull request as ready for review November 2, 2026 18:49
@kilo-code-bot

kilo-code-bot Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: 1 Issue Found | Recommendation: Address before merge

Executive Summary

The incremental commit replaces the regex-stripping scan with a state machine and recomputes the two affected counts correctly, but the new scanner has no regex-literal (or template-interpolation) state, so the reviewed-count gate stays blind in files that contain quote-bearing regexes.

Assumptions: [INFERENCE] I did not run the Jest suites; per-file counts were verified by reading each is_admin site, and the scanner's behavior on regex and ${...} spans is derived from the code, not executed. next-auth-options.ts (lines 329, 1308, 1324) and github-apps-router.ts (lines 251, 274, 578) match the updated inventory counts.

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 1
Issue Details (click to expand)

SUGGESTION

File Line Issue
apps/web/src/routers/privileged-inventory.test.ts 48 Scanner has no regex-literal state and treats ${...} as opaque, so quote-bearing regexes desync the scan and hide later is_admin reads
Files Reviewed (2 files)
  • apps/web/src/routers/privileged-inventory.test.ts - 1 issue
  • packages/web-shared/src/lib/admin/privileged-inventory.ts
Previous Review Summaries (2 snapshots, latest commit 79e4f9e)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 79e4f9e)

Status: 1 Issue Found | Recommendation: Address before merge

Executive Summary

The review-fix commit correctly closes the prior batch-role and denied-credential findings, but the new is_admin scanner silently deletes real reads, so the inventory undercounts one file and lost coverage of next-auth-options.ts.

Assumptions: [INFERENCE] I did not run the test suites or typecheck. The incremental range 1d88251..HEAD includes an 800-file merge from main, so this round is scoped to the PR's own commit 79e4f9eae (7 files). The scanner's per-file output is inferred from quote pairing, not executed.

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0
Issue Details (click to expand)

WARNING

File Line Issue
apps/web/src/routers/privileged-inventory.test.ts 403 // stripping runs before string stripping, so URL literals unbalance the quote scan and delete real is_admin reads
Files Reviewed (7 files)
  • apps/web/src/routers/__snapshots__/privileged-inventory.test.ts.snap
  • apps/web/src/routers/privileged-inventory.test.ts - 1 issue
  • packages/web-shared/src/lib/admin/privileged-inventory.ts
  • packages/web-shared/src/lib/user/server.test.ts
  • packages/web-shared/src/lib/user/server.ts
  • packages/web-shared/src/routers/organizations/utils.test.ts
  • packages/web-shared/src/routers/organizations/utils.ts

Fix these issues in Kilo Cloud

Previous review (commit 1d88251)

Status: 4 Issues Found | Recommendation: Address before merge

Executive Summary

The observe-only telemetry is mis-measured on two paths that feed the later cutover decisions: the batch org-access wouldPassWithoutAdmin ignores the caller's required roles, and all denied bearer requests are classified as human_bearer.

Assumptions: [INFERENCE] I did not execute the test suites; findings come from static reading of the checked-out HEAD (1d88251). Worker suites, e2e, and deploy were not run, consistent with the PR description.

Overview

Severity Count
CRITICAL 0
WARNING 2
SUGGESTION 2
Issue Details (click to expand)

WARNING

File Line Issue
packages/web-shared/src/routers/organizations/utils.ts 293 Batch wouldPassWithoutAdmin uses "has any membership" instead of the caller's required roles, so prior-is_admin-only accesses are undercounted
packages/web-shared/src/lib/user/server.ts 228 Denied bearer requests derive credential from failure fields that authError never populates, so delegated-workload, service-assertion, and scoped Cloud Agent denials are all logged as human_bearer

SUGGESTION

File Line Issue
apps/web/src/routers/privileged-inventory.test.ts 403 occurrencesIn counts is_admin in comments and object literals, so the reviewed-count gate can pass without an actual read being reviewed
apps/web/src/routers/privileged-inventory.test.ts 15 BROAD_WILDCARDS lists codingPlans.* while the inventory route is codingPlans.admin*, and omits the broad debug.* entry, so the "swallowed by wildcard" check cannot fire for either
Files Reviewed (36 files)
  • apps/web/src/app/(app)/organizations/[id]/integrations/linear/reinstall/route.ts
  • apps/web/src/app/(app)/organizations/[id]/integrations/slack/reinstall/route.ts
  • apps/web/src/app/api/auto-routing/mode/route.ts
  • apps/web/src/app/api/claw/openclaw-export/route.ts
  • apps/web/src/app/api/cloud-agent/sessions/stream-ticket/route.ts
  • apps/web/src/app/api/integrations/google/callback/route.ts
  • apps/web/src/app/api/integrations/google/connect/route.ts
  • apps/web/src/app/api/integrations/google/disconnect/route.ts
  • apps/web/src/app/api/organizations/[id]/defaults/route.ts
  • apps/web/src/app/api/organizations/[id]/guardrails/mcp-connections/route.ts
  • apps/web/src/app/api/organizations/[id]/guardrails/route.ts
  • apps/web/src/app/api/organizations/[id]/modes/route.ts
  • apps/web/src/app/api/organizations/[id]/user-tokens/route.auth.test.ts
  • apps/web/src/app/api/organizations/[id]/user-tokens/route.ts
  • apps/web/src/app/payments/topup/route.ts
  • apps/web/src/lib/auth/cloud-agent-authority.test.ts
  • apps/web/src/lib/auth/resource-delegation.ts
  • apps/web/src/lib/organizations/organization-auth.test.ts
  • apps/web/src/lib/organizations/organization-auth.ts
  • apps/web/src/lib/trpc/admin-procedures.ts
  • apps/web/src/proxy.ts
  • apps/web/src/routers/__snapshots__/privileged-inventory.test.ts.snap
  • apps/web/src/routers/privileged-inventory.test.ts - 2 issues
  • apps/web/src/routers/trpc-provenance.test.ts
  • packages/web-shared/src/lib/admin/admin-access-log.test.ts
  • packages/web-shared/src/lib/admin/admin-access-log.ts
  • packages/web-shared/src/lib/admin/privileged-access.test.ts
  • packages/web-shared/src/lib/admin/privileged-access.ts
  • packages/web-shared/src/lib/admin/privileged-inventory.ts
  • packages/web-shared/src/lib/trpc/init.test.ts
  • packages/web-shared/src/lib/trpc/init.ts
  • packages/web-shared/src/lib/trpc/route-handler.ts
  • packages/web-shared/src/lib/user/server.test.ts
  • packages/web-shared/src/lib/user/server.ts - 1 issue
  • packages/web-shared/src/routers/organizations/utils.test.ts
  • packages/web-shared/src/routers/organizations/utils.ts - 1 issue

Reviewed by deepseek-flash-latest · Input: 89.4K · Output: 25.1K · Cached: 1.3M

Review guidance: REVIEW.md from base branch main

@pandemicsyn
pandemicsyn added this pull request to stack #9714 November 3, 2026 17:37
…inventory

# Conflicts:
#	packages/web-shared/src/lib/user/server.ts
#	packages/web-shared/src/routers/organizations/utils.ts

This branch has not been deployed

No deployments
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