feat: add support_copilot app type with OAuth 2.1 JWT auth#3128
feat: add support_copilot app type with OAuth 2.1 JWT auth#3128omar-inkeep merged 2 commits intomainfrom
Conversation
Adds a new support_copilot app type for deploying agents across external tools and support platforms (e.g. browser extensions, helpdesks). The app authenticates end-users via the existing OAuth 2.1 provider using asymmetric JWTs validated via JWKS. Backend (agents-api, agents-core): - New SupportCopilotConfig schema with credentialReferenceIds - tryOAuthSupportCopilotAuth: JWKS-verified JWT flow with azp, tenant, and SpiceDB canUseProjectStrict checks - OAuth JWT path added to manageAuth (priority #3, JWT-only — opaque tokens are not issued by this flow) - GET /manage/tenants/:tenantId/apps for cross-project app discovery (used by the copilot BFF to resolve its app_id per tenant) - COPILOT_OAUTH_CLIENT_ID env var for azp validation UI (agents-manage-ui): - Support Copilot option in New App dropdown (double-gated: PUBLIC_IS_INKEEP_CLOUD_DEPLOYMENT=true AND org has feature:support_copilot entitlement with maxValue > 0) - CredentialMultiSelect component for picking credential references - Default agent and credential pickers in create/update forms - Orange badge for support_copilot in the apps table Tooling: - scripts/setup-oauth-client.ts — creates the OAuth client for the copilot BFF. Supports --print-only for safe prod bootstrap (no .env write). Tests: 15 new unit tests (tryOAuthSupportCopilotAuth, manage OAuth JWT path, tenant apps route). Full agents-api suite: 2362 passed. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: 4f3a044 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This public PR was merged directly in the public repo. The matching monorepo PR was left open for manual follow-up because Matching internal PR: #89 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
TL;DRThis PR introduces a new Key changes
OAuth JWT authentication added to manage and run middlewareBefore: The manage auth middleware supported bypass secret, better-auth sessions, Slack user JWTs, internal service tokens, and a legacy API key exception. The run auth middleware had no After: Both middleware stacks now verify OAuth 2.1 JWT tokens against the platform's own JWKS endpoint ( New
|
There was a problem hiding this comment.
Good feature PR with solid test coverage and well-structured auth. Two items need attention: a security nuance in the manage auth JWT path, and a stale type cast. The rest is clean.
Security note (pre-existing code touched by this PR): The manageBearerAuth OAuth JWT path (step 3, manageAuth.ts:132-155) verifies JWTs via JWKS but only enforces azp when COPILOT_OAUTH_CLIENT_ID is set. When the env var is unset (and the JWKS endpoint is active), any validly-signed JWT with a sub claim passes manage auth. The PR description says "silently rejects any JWT when COPILOT_OAUTH_CLIENT_ID is unset", but the code actually accepts such JWTs (it only rejects when azp is present and mismatches). Consider whether that's desired — a JWT from a future OAuth client could inadvertently gain manage-domain access.
Minor note (outside diff): apps.ts:70 has c.req.query('type') as 'web_client' | 'api' | undefined — the cast misses 'support_copilot' even though the query schema now includes it. Not a runtime bug (string passes through), but the type is stale.
Claude Opus | 𝕏
| try { | ||
| const { payload } = await jwtVerify(token, JWKS); | ||
| const azp = payload.azp as string | undefined; | ||
| if (env.COPILOT_OAUTH_CLIENT_ID && azp !== env.COPILOT_OAUTH_CLIENT_ID) { |
There was a problem hiding this comment.
When COPILOT_OAUTH_CLIENT_ID is unset, this condition is falsy and the JWT passes through unconditionally (as long as it has a sub). This means any JWKS-verified JWT — including ones minted for future OAuth clients — gets manage access.
For defense-in-depth, consider skipping the OAuth JWT path entirely when the env var is unset:
if (!env.COPILOT_OAUTH_CLIENT_ID) {
// No copilot client configured — skip OAuth JWT path
} else {
// existing JWKS verify + azp check
}| c.set('userId', sub); | ||
| if (email) c.set('userEmail', email); | ||
| if (tenantId) c.set('tenantId', tenantId); | ||
| if (azp) c.set('oauthClientId' as any, azp); |
There was a problem hiding this comment.
c.set('oauthClientId' as any, azp) — the as any cast bypasses the Variables type. If oauthClientId is needed by downstream middleware, add it to the Variables type. If it's only for debugging, consider logging it instead.
| throw new HTTPException(401, { message: 'Invalid OAuth client' }); | ||
| } | ||
|
|
||
| if (app.tenantId && jwtTenantId && app.tenantId !== jwtTenantId) { |
There was a problem hiding this comment.
The tenant-mismatch check is skipped when app.tenantId is falsy (null/empty for a global app). Should a support_copilot app ever be global? If not, consider adding an explicit guard: if (!app.tenantId) throw ... to enforce that support_copilot apps are always tenant-scoped.
| apiKey: bearerToken, | ||
| tenantId: app.tenantId || '', | ||
| projectId: app.projectId, | ||
| agentId: requestedAgentId || app.defaultAgentId || '', |
There was a problem hiding this comment.
Falling back to '' for agentId when both requestedAgentId and app.defaultAgentId are absent will produce an execution context with an empty string agent ID. Downstream code likely won't handle this well. Consider rejecting early with a descriptive error.
| fetchApps(tenantId, projectId), | ||
| fetchAgents(tenantId, projectId), | ||
| fetchCredentials(tenantId, projectId), | ||
| fetchEntitlements(tenantId), |
There was a problem hiding this comment.
If fetchEntitlements(tenantId) throws on self-hosted deployments where the entitlements endpoint doesn't exist, the entire apps page shows an error. Consider wrapping with .catch(() => []) since entitlements are only meaningful on cloud.
| const { tenantId } = c.req.valid('param'); | ||
| const page = Number(c.req.query('page')) || 1; | ||
| const limit = Math.min(Number(c.req.query('limit')) || 10, 100); | ||
| const type = c.req.query('type') as 'web_client' | 'api' | 'support_copilot' | undefined; |
There was a problem hiding this comment.
Using c.req.query('type') as ... bypasses the Zod validation already declared in the route schema. Consider using c.req.valid('query') instead so the value is type-safe from the schema.
There was a problem hiding this comment.
PR Review Summary
(6) Total Issues | Risk: Medium
🟠⚠️ Major (2) 🟠⚠️
🟠 1) manageAuth.ts:136 + runAuth.ts:448 OAuth JWT azp validation bypassed when COPILOT_OAUTH_CLIENT_ID is unset
Issue: When env.COPILOT_OAUTH_CLIENT_ID is undefined (not configured), the azp validation check is skipped entirely in both manageAuth.ts and runAuth.ts. The condition env.COPILOT_OAUTH_CLIENT_ID && azp !== env.COPILOT_OAUTH_CLIENT_ID evaluates to false when the env var is unset, meaning any valid JWT signed by the JWKS endpoint (regardless of its azp claim) will be accepted.
Why: The PR description states "the OAuth auth path silently rejects any JWT when COPILOT_OAUTH_CLIENT_ID is unset" as a security feature, but the actual implementation does the opposite — it silently ACCEPTS. This could allow tokens issued to other OAuth clients to authenticate, creating a confused deputy vulnerability in non-production environments or during misconfiguration.
Fix: If the intent is to disable OAuth JWT auth when unconfigured, add an early return before JWT verification:
// In manageAuth.ts, before line 132:
if (!env.COPILOT_OAUTH_CLIENT_ID) {
// OAuth JWT auth disabled - fall through to other auth methods
}
// Or make azp validation mandatory when configured:
if (env.COPILOT_OAUTH_CLIENT_ID && azp !== env.COPILOT_OAUTH_CLIENT_ID) {
throw new HTTPException(401, { message: 'Invalid OAuth client' });
}
if (!env.COPILOT_OAUTH_CLIENT_ID && azp) {
// JWT has azp but we have no expected client - reject
throw new HTTPException(401, { message: 'OAuth not configured' });
}Apply the same pattern in tryOAuthSupportCopilotAuth in runAuth.ts.
Refs:
🟠 2) app-credential-auth.test.ts Missing test for COPILOT_OAUTH_CLIENT_ID unset scenario
Issue: There is no test verifying the behavior when COPILOT_OAUTH_CLIENT_ID env var is not configured. This is the critical security boundary behavior documented in the PR description.
Why: Without this test, a refactor could accidentally change the conditional logic. Given the discrepancy between documented intent ("silently rejects") and actual behavior ("silently accepts"), explicit test coverage is essential to lock in the intended behavior.
Fix: Add test case:
it('should reject OAuth JWT when COPILOT_OAUTH_CLIENT_ID is not configured', async () => {
// Mock env.COPILOT_OAUTH_CLIENT_ID = undefined
// Provide valid JWT with any azp
// Verify 401 response (if intent is to reject)
// OR document and test the accept behavior if intentional
});Refs:
🟡 Minor (3) 🟡
🟡 1) tenantApps.ts:34 Missing commonGetErrorResponses in route definition
Issue: All peer list routes in the manage domain include ...commonGetErrorResponses in their responses object (e.g., apps.ts, credentials.ts, apiKeys.ts, entitlements.ts). This route omits it.
Why: Ensures consistent error response shapes in the OpenAPI spec across all list endpoints.
Fix: Add ...commonGetErrorResponses to the responses object and import from @inkeep/agents-core.
Refs:
🟡 2) manageAuth.ts:20 + runAuth.ts:38 JWKS singleton duplication
Issue: The JWKS object is created at module load time in both manageAuth.ts and runAuth.ts with identical configuration.
Why: This violates DRY and creates two separate caches that refresh independently. Additionally, module initialization now depends on env.INKEEP_AGENTS_API_URL being set at import time.
Fix: Extract JWKS initialization to a shared utility in agents-core (e.g., src/auth/jwks.ts) that exports a lazy-initialized singleton.
Refs:
🟡 3) runAuth.ts:453 Tenant mismatch check allows missing tenantId to bypass validation
Issue: The condition if (app.tenantId && jwtTenantId && app.tenantId !== jwtTenantId) only triggers when BOTH values are truthy. If the JWT is missing the tenantId claim, the check is skipped.
Why: While canUseProjectStrict later validates project access using the app's tenantId, allowing JWTs without explicit tenant claims may be inconsistent with the security model.
Fix: Consider making the tenantId claim required:
if (!jwtTenantId) {
return { authResult: null, failureMessage: 'OAuth JWT missing tenantId claim' };
}Refs:
Inline Comments:
- 🟡 Minor:
apps.ts:70Type cast missing support_copilot - 🟡 Minor:
credential-multi-select.tsx:86Decorative icon missing aria-hidden - 🟡 Minor:
credential-multi-select.tsx:103Selection icon missing aria-hidden - 🟡 Minor:
credential-multi-select.tsx:129Remove button icon missing aria-hidden
💭 Consider (1) 💭
💭 1) manageAuth.ts:134 + runAuth.ts:430 OAuth JWT verification lacks issuer (iss) claim validation
Issue: The jwtVerify calls do not pass an issuer option, meaning JWTs from the JWKS will be accepted regardless of what iss claim they contain.
Why: While the JWKS lookup constrains which keys are trusted, explicit issuer validation is a defense-in-depth measure. The anonymous JWT flow at runAuth.ts:821 does validate issuer: { issuer: 'inkeep' }.
Fix: Add issuer validation for consistency:
await jwtVerify(token, JWKS, { issuer: env.INKEEP_AGENTS_API_URL || 'http://localhost:3002' })Refs:
🚫 REQUEST CHANGES
Summary: The PR implements a well-structured new app type with appropriate SpiceDB authorization and UI gating. However, there's a critical discrepancy between the documented security behavior ("OAuth auth path silently rejects any JWT when COPILOT_OAUTH_CLIENT_ID is unset") and the actual implementation (which accepts any valid JWKS-signed JWT when unconfigured). This needs to be aligned — either fix the code to match the documentation, or update the documentation if the current behavior is intentional. Once this is clarified and the azp validation behavior is locked in with tests, this should be good to merge.
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
runAuth.ts:849-852 |
api app type unhandled in tryAppCredentialAuth |
Pre-existing behavior, not introduced by this PR. API apps use different auth path. |
schemas.ts:1901 |
credentialReferenceIds has no referential integrity validation | Consistent with existing patterns (e.g., defaultAgentId). Schema validates structure, not references. |
setup-oauth-client.ts:121 |
Client secret printed to stdout in --print-only mode | Documented behavior for ops bootstrapping. Script includes appropriate warning. |
Reviewers (7)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-security-iam |
5 | 2 | 1 | 0 | 0 | 0 | 0 |
pr-review-architecture |
6 | 1 | 0 | 0 | 0 | 0 | 2 |
pr-review-tests |
6 | 1 | 0 | 0 | 0 | 0 | 0 |
pr-review-frontend |
4 | 0 | 0 | 0 | 4 | 0 | 0 |
pr-review-consistency |
2 | 1 | 0 | 0 | 1 | 0 | 0 |
pr-review-standards |
2 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-appsec |
3 | 0 | 0 | 0 | 0 | 0 | 1 |
| Total | 28 | 5 | 1 | 0 | 5 | 0 | 3 |
Note: Security-iam and standards both flagged the azp validation issue — consolidated as single Major finding. Architecture flagged JWKS duplication and tenant-level auth boundary. Tests flagged missing coverage for unconfigured env var scenario.
| ) : ( | ||
| <span className="text-muted-foreground">{placeholder}</span> | ||
| )} | ||
| <ChevronsUpDown className="opacity-50 text-muted-foreground" /> |
There was a problem hiding this comment.
🟡 Minor: Decorative icon missing aria-hidden
Issue: The ChevronsUpDown icon is decorative but missing aria-hidden="true".
Why: Screen readers may announce the icon unnecessarily when reading the button.
Fix:
| <ChevronsUpDown className="opacity-50 text-muted-foreground" /> | |
| <ChevronsUpDown className="opacity-50 text-muted-foreground" aria-hidden="true" /> |
| onSelect={() => toggleValue(option.value)} | ||
| > | ||
| {option.label} | ||
| <Check |
There was a problem hiding this comment.
🟡 Minor: Selection indicator icon missing aria-hidden
Issue: The Check icon indicating selection state should be hidden from assistive technology.
Why: The CommandItem component already handles selection state semantics internally; the visual checkmark is decorative.
Fix:
| <Check | |
| <Check | |
| aria-hidden="true" | |
| className={cn( |
| onClick={() => removeValue(id)} | ||
| aria-label={`Remove ${opt?.label ?? id}`} | ||
| > | ||
| <X className="size-3" /> |
There was a problem hiding this comment.
🟡 Minor: Remove button icon missing aria-hidden
Issue: The X icon inside the remove button is decorative and should have aria-hidden="true".
Why: The button already has an accessible aria-label so the icon is purely visual.
Fix:
| <X className="size-3" /> | |
| <X className="size-3" aria-hidden="true" /> |
- Disable OAuth JWT auth entirely when COPILOT_OAUTH_CLIENT_ID is unset (previously the azp check was skipped silently, which could allow JWTs issued to other OAuth clients on the same provider to authenticate) - Require tenantId claim on support_copilot JWTs (was optional before) - Validate issuer (iss) claim in jwtVerify as defense-in-depth - Extract JWKS singleton to shared lazy-initialized util (src/utils/oauthJwks.ts) to remove duplication across middleware - Add commonGetErrorResponses to tenantApps route for consistency - Fix support_copilot missing from type cast in apps list handler - Add aria-hidden to decorative icons in CredentialMultiSelect Tests: new coverage for the unconfigured env var and missing-tenantId paths. Full agents-api suite: 2365 passed. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a re-review scoped to the delta since the prior automated review. The fix commit (4f3a0445 fix(oauth): tighten OAuth JWT auth based on review feedback) addresses all previously raised issues.
Prior Findings — Now Resolved ✅
| Prior Finding | Resolution in Delta |
|---|---|
🟠 OAuth JWT azp validation bypassed when COPILOT_OAUTH_CLIENT_ID unset |
Fixed: Both manageAuth.ts:130 and runAuth.ts:419-424 now guard the entire JWT path with if (env.COPILOT_OAUTH_CLIENT_ID && ...) or early-return when unset |
| 🟠 Missing test coverage for unconfigured env var scenario | Fixed: New tests in both manage-auth-oauth-jwt.test.ts:164-182 and app-credential-auth.test.ts:758-776 verify jwtVerifyMock is NOT called when env var is undefined |
| 🟡 Missing tenantId claim validation in runAuth | Fixed: runAuth.ts:449-452 now explicitly rejects JWTs missing the https://inkeep.com/tenantId claim |
🟡 Missing issuer (iss) validation |
Fixed: Both paths now call jwtVerify(token, getOAuthJwks(), { issuer: getOAuthIssuer() }) |
| 🟡 JWKS singleton duplication between files | Fixed: Extracted to shared oauthJwks.ts with lazy-initialized singleton |
🟡 Type cast missing support_copilot in apps.ts |
Fixed: Line 70 now casts to 'web_client' | 'api' | 'support_copilot' | undefined |
🟡 Missing commonGetErrorResponses in tenantApps.ts |
Fixed: Added at line 44 |
🟡 Accessibility: icons missing aria-hidden |
Fixed: All icons in credential-multi-select.tsx now have aria-hidden attribute |
Delta Quality Assessment
The fix commit is clean and targeted:
- Security: The OAuth JWT auth path is now correctly gated — when
COPILOT_OAUTH_CLIENT_IDis unset, no JWT verification is attempted, preventing confused-deputy attacks from other OAuth clients - Defense-in-depth: Added issuer validation ensures JWTs from external issuers are rejected
- Test coverage: New tests explicitly verify the security boundary (env var unset → no JWT verification)
- Architecture: JWKS singleton extraction eliminates duplication and module-load-time env dependency
✅ APPROVE
Summary: The fix commit comprehensively addresses all prior review findings with appropriate tests. The OAuth JWT security model is now correctly implemented: unconfigured deployments reject all JWTs for support_copilot apps, and configured deployments validate azp, issuer, tenantId, and SpiceDB permissions. Ship it! 🚀
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator (delta) |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta re-review — no new reviewer dispatch needed as prior findings were addressed.
Preview URLsUse these stable preview aliases for testing this PR:
These point to the same Vercel preview deployment as the bot comment, but they stay stable and easier to find. Raw Vercel deployment URLs
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
Ito Test Report ❌15 test cases ran. 1 failed, 14 passed. Across 15 executed test cases, 14 passed and 1 failed: Support Copilot create/edit flows (including zero-credential, deep-link/refresh/back-forward, double-submit, and desktop/mobile viewport behavior), tenant-level discovery/filtering, entitlement/cloud gating, manage OAuth JWT success, and multiple negative run-auth checks (wrong azp, tenant mismatch, denied user) all behaved as expected. The key finding is one high-severity likely bug in /run/api/chat/completions where malformed OAuth bearer tokens missing the tenant claim are not deterministically rejected at auth time and can fall through to a dev/test default context, resulting in a 500 instead of the expected 401/403. ❌ Failed (1)
|
Summary
Adds a new
support_copilotapp type for deploying agents across external tools and support platforms (browser extensions, helpdesks, etc.). End-users authenticate via the existing OAuth 2.1 provider (#3103) using asymmetric JWTs verified via JWKS.What's included
Backend (
agents-api,agents-core):SupportCopilotConfigschema withcredentialReferenceIdstryOAuthSupportCopilotAuth()— JWKS-verified JWT flow withazp, tenant-match, and SpiceDBcanUseProjectStrictchecksmanageAuthat priority done #3 (JWT-only — opaque tokens are not issued by this flow)GET /manage/tenants/:tenantId/apps— cross-project app discovery (used by the copilot BFF to resolve itsapp_idper tenant)COPILOT_OAUTH_CLIENT_IDenv var forazpvalidationUI (
agents-manage-ui):PUBLIC_IS_INKEEP_CLOUD_DEPLOYMENT=trueAND org hasfeature:support_copilotentitlement withmaxValue > 0)CredentialMultiSelectcomponent for picking credential referencessupport_copilotin the apps tableTooling:
scripts/setup-oauth-client.ts— creates the OAuth client for the copilot BFF--print-onlyflag for prod bootstrap (no.envwrite, values go to stdout for copy into secret manager)Gating (per-org opt-in on cloud only)
The Support Copilot option requires both:
PUBLIC_IS_INKEEP_CLOUD_DEPLOYMENT=true(deployment-level flag)org_entitlementwithresource_type = 'feature:support_copilot'andmax_value > 0for that orgTo enable for a customer:
Production setup
pnpm setup-oauth-client --print-only(pointed at prod DB) → copyCOPILOT_OAUTH_CLIENT_ID+COPILOT_OAUTH_CLIENT_SECRETinto secret managerCOPILOT_OAUTH_CLIENT_IDon theagents-apideployment (validatesazpon incoming JWTs)COPILOT_OAUTH_CLIENT_IDandCOPILOT_OAUTH_CLIENT_SECRETon the copilot BFF deployment (needs the secret to exchange auth codes)feature:support_copilotentitlement row for any cloud tenant that should see the dropdown optionTest plan
agents-apitest suite passes (2362 passed, 15 new)support_copilotapp via UI in a cloud-flagged, entitled tenantsupport_copilotapp (credentials + default agent persist)GET /manage/tenants/:tenantId/apps?type=support_copilotreturns cross-project resultspnpm setup-oauth-client --print-onlyagainst prod DBKnown gaps / follow-ups
COPILOT_OAUTH_CLIENT_IDis unset, so an OSS or un-entitled tenant can't actually use asupport_copilotapp even if they create one via curl. Consider adding an API gate if we want stricter defense-in-depth.🤖 Generated with Claude Code