feat: Plan 9 PR C — control-plane writes + agent-builder view - #28
Merged
Conversation
CP: - PUT /v1/policies/active (policy:admin): create-or-replace the active policy and emit a policy.replace sync_event in one transaction so edges pull the new policy over SSE instead of serving a stale snapshot. Pure parsePolicyUpdate validator (+unit tests) for threshold/action/ sample-rate/fingerprint bounds matching the migration CHECKs. - API-key CRUD: GET/POST /v1/api-keys, DELETE /v1/api-keys/:id. Secret (vrl_ + 64 hex) returned once; HMAC hash + prefix persisted; soft-revoke. Scope allowlist validation. Cross-tenant delete is a 404 (no leak). - principals list returns derived assurance_level (verified_key iff a non-revoked key has control_verified_at); principal detail includes read-only issuer attrs (trust_weight/verified_at/is_bootstrap) via LEFT JOIN. principals query params (entity_kind/limit/offset) made optional so agent-builder can list owned principals. - integration coverage: policy PUT (create/update/sync event/authz), api-key CRUD (mint/list/revoke/isolation), agent-builder reads (assurance_level, issuer attrs). Dashboard: - agent-builder view (/agent-builder): owned-principals master list with assurance badges, selected-principal detail with keys + assurance, in/out attestation feed, network score-history line chart, read-only issuer relationship card. Tenant + principal id embedded in query keys. - vitest: scoreSeries transform, PrincipalList/KeyList/AttestationFeed, agentBuilderQueryKeys. Verification: CP tsc clean, 143 unit + 40 integration pass; dashboard typecheck clean, 45 vitest pass, vite build succeeds. Out of scope (PR D): billing (Stripe checkout/portal/webhook) + admin (tenants/bootstrap/graph health/issuer queue). No new migrations.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (24)
Comment |
PR Summary by QodoPlan 9 PR C: policy writes + API-key CRUD + agent-builder view
AI Description
Diagram
High-Level Assessment
Files changed (24)
|
Code Review by Qodo
1. Unscoped API-key CRUD
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+9
to
+11
| const router = Router(); | ||
| router.use(authMiddleware); | ||
|
|
There was a problem hiding this comment.
2. Unscoped api-key crud 🐞 Bug ⛨ Security
The new /v1/api-keys GET/POST/DELETE routes only require authentication and a tenantId, so any authenticated API key (even with empty scopes) can mint new keys with elevated scopes (including '*') and revoke other active keys in the same tenant.
Agent Prompt
### Issue description
`/v1/api-keys` endpoints are authenticated but not authorized. Because `parseScopes()` allows powerful scopes like `policy:admin` and `*`, any authenticated API key can mint a stronger key and/or revoke other keys within the tenant.
### Issue Context
- The router applies `authMiddleware` but never applies `requireScope()` (or any role/scope check).
- `requireScope()` exists and already enforces API-key scopes (including `*`).
### Fix Focus Areas
- control-plane/src/routes/apiKeys.ts[9-89]
- control-plane/src/middleware/requireScope.ts[4-36]
### What to change
1. Add an explicit authorization gate for API-key management (e.g. `requireScope('policy:admin')`) to **GET**, **POST**, and **DELETE** routes (or `router.use(requireScope('policy:admin'))` after `authMiddleware`).
2. Prevent privilege escalation by ensuring minted scopes are not more permissive than the caller’s own scopes unless the caller is platform staff/admin (OIDC) or already has `*`.
3. Add an integration test proving a low-scope/empty-scope key gets `403` for POST/DELETE (and for GET if intended).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+40
to
+43
| router.put( | ||
| '/active', | ||
| requireScope('policy:admin'), | ||
| defineHandler({ |
There was a problem hiding this comment.
3. Cross-tenant role bypass 🐞 Bug ⛨ Security
PUT /v1/policies/active uses requireScope('policy:admin'), but requireScope() authorizes OIDC users
if ANY membership role is admin/staff, not the active tenant role; a user who is admin in tenant A
and only a member in tenant B can still update tenant B’s policy when selecting tenant B via
X-Tenant-Id.
Agent Prompt
### Issue description
`requireScope()` currently checks `req.user.roles` across *all* memberships and treats any `admin`/`staff` membership as sufficient. For tenant-scoped writes like `PUT /v1/policies/active`, this allows privilege escalation when a user belongs to multiple tenants with different roles.
### Issue Context
- `authenticateOidc()` sets `req.user.tenantId` from the *active* membership selected via `X-Tenant-Id`, but also sets `req.user.roles` to all membership roles.
- `requireScope()` checks `req.user.roles.some(role in {staff,admin})` rather than checking the role for `req.user.tenantId`.
### Fix Focus Areas
- control-plane/src/middleware/requireScope.ts[18-35]
- control-plane/src/middleware/auth.ts[126-145]
- control-plane/src/routes/policies.ts[40-49]
### What to change
1. Update `requireScope()` to authorize OIDC users based on the **active membership role** (`req.user.role`) or by looking up the role for `req.user.tenantId`, rather than any role across `req.user.roles`.
2. Add an integration test with one OIDC user who is `admin` in tenant A and `member` in tenant B proving `PUT /v1/policies/active` for tenant B returns `403`.
3. Ensure other tenant-scoped endpoints using `requireScope()` follow the same active-tenant semantics.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+60
to
+63
| `SELECT p.*, i.trust_weight::float AS trust_weight, i.verified_at, i.is_bootstrap | ||
| FROM principals p | ||
| LEFT JOIN issuers i ON i.principal_id = p.id | ||
| WHERE p.id = $1`, |
There was a problem hiding this comment.
4. Principal detail fields leak 🐞 Bug ≡ Correctness
getPrincipal() SELECTs issuer columns into the row and then spreads the row into the returned object, so GET /v1/principals/:id includes unexpected top-level trust_weight/verified_at/is_bootstrap fields in addition to the nested issuer object.
Agent Prompt
### Issue description
`principalRepository.getPrincipal()` adds issuer columns via a LEFT JOIN and then spreads the entire SQL row into the returned principal object. This causes the HTTP response to include unintended top-level fields (`trust_weight`, `verified_at`, `is_bootstrap`) in addition to `issuer`.
### Issue Context
`GET /v1/principals/:id` returns the repository object directly, so any extra properties on the returned object become part of the public API payload.
### Fix Focus Areas
- control-plane/src/domains/principal/principalRepository.ts[58-78]
- control-plane/src/routes/principals.ts[45-50]
### What to change
1. Destructure the joined columns out of the row before spreading, e.g.:
- `const { trust_weight, verified_at, is_bootstrap, ...p } = row;`
- return `{ ...p, issuer: trust_weight == null ? null : { trust_weight, verified_at, is_bootstrap } }`
2. Add/extend a test asserting those three fields are absent from the top-level response for both issuer and non-issuer principals.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
Plan 9 PR C — control-plane writes + agent-builder view
@coderabbitai review
Implements the PR C slice of
docs/superpowers/plans/2026-08-01-plan-9-dashboard.md(PR A #26, PR B #27 merged).Control plane — writes
PUT /v1/policies/active(policy:admin/ tenant admin role): create-or-replaces the active policy and emits apolicy.replacesync event in one transaction (mirrorsscoreWriter'sappendEventWithClientpattern) so edges pull the new policy over SSE instead of serving a stale snapshot. PureparsePolicyUpdatevalidator (+unit tests) enforces the migration CHECKs (threshold 0-100 int, action enums,allow_sample_rate0-1, fingerprint arrays).GET/POST /v1/api-keys,DELETE /v1/api-keys/:id. Secret (vrl_+ 64 hex) returned exactly once; HMAC-SHA256 hash + prefix persisted; soft-revoke (revoked_at). Scope allowlist (attest:read|write,admin:read,policy:read|admin,*). Cross-tenant delete is a 404 (no info leak).assurance_level(verified_keyiff a non-revoked key hascontrol_verified_at); principal detail includes read-only issuer attrs (trust_weight/verified_at/is_bootstrap) via LEFT JOIN. Madeentity_kind/limit/offsetquery params optional so the agent-builder can list owned principals (latent strict-validation fix).Dashboard — agent-builder view (
/agent-builder)Master-detail: owned-principals list with assurance badges → selected principal shows keys + assurance, in/out attestation feed, network score-history line chart (recharts), and a read-only issuer relationship card. Composed from existing
/v1/principals*,/v1/attestations,/v1/scores/:id/historyendpoints. Active tenant + selected principal id embedded in query keys.Verification
tsc --noEmitclean; 143 unit + 40 integration tests pass (new integration suite covers policy PUT create/update/sync-event/authz, api-key CRUD/isolation, agent-builder reads).vite buildsucceeds.Out of scope (PR D): billing (Stripe checkout/portal/webhook) + admin (tenants/bootstrap/graph health/issuer queue). No new migrations.
Note on attestation feed authz
GET /v1/attestationsrequiresattest:read(API key) or tenant admin/staff role (OIDC), unchanged. For owned principals the caller's tenant owns the issuer/subject, so participants-visibility attestations are visible to that tenant. The view degrades gracefully to an error panel when the caller lacks the scope; refining read authz for plain tenant members is out of scope here.