feat: add GitHub integration endpoints for token exchange and installation checks#162
feat: add GitHub integration endpoints for token exchange and installation checks#162catrielmuller wants to merge 3 commits intomainfrom
Conversation
…ation checks - Add POST /api/integrations/github/exchange-token for OIDC token exchange - Add POST /api/integrations/github/exchange-token-with-pat for PAT token exchange - Add GET /api/integrations/github/check-installation for CLI polling - Implement OIDC JWT verification using GitHub's JWKS endpoint - Add findGitHubIntegrationByAccountLogin() helper function - Support both production (OIDC) and development (PAT) authentication flows
![kiloconnect[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| export async function GET(request: NextRequest) { | ||
| try { | ||
| const owner = request.nextUrl.searchParams.get('owner'); |
There was a problem hiding this comment.
WARNING: Unauthenticated endpoint can be used to enumerate GitHub App installations
This GET route accepts an arbitrary owner and returns whether an active integration exists. If this is reachable from the browser, it can leak which orgs/users have the GitHub App installed (useful for targeted abuse). Consider requiring an authenticated Kilo session and/or restricting to internal callers, plus rate limiting.
| return NextResponse.json({ error: 'Missing owner or repo in request body' }, { status: 400 }); | ||
| } | ||
|
|
||
| const octokit = new Octokit({ auth: pat }); |
There was a problem hiding this comment.
CRITICAL: PAT-based token exchange needs stronger guardrails
This endpoint lets any caller who can present any GitHub PAT (with repo access) mint a GitHub App installation token for the owner. That is a large privilege escalation surface if the route is accessible outside a tightly controlled environment. Strongly consider enforcing Kilo-side auth + authorization, restricting to server-to-server usage, and/or limiting which repos/owners/workflows are allowed to exchange.
| try { | ||
| await octokit.rest.repos.get({ owner, repo }); | ||
| } catch (error) { | ||
| console.error('PAT validation failed:', error); |
There was a problem hiding this comment.
WARNING: Logging raw Octokit errors may leak sensitive request details
console.error(..., error) can include rich error objects (request metadata, headers) depending on the thrown type. Since this code handles PATs, prefer logging a sanitized subset (e.g., status + message) and rely on Sentry scrubbing for the rest.
|
|
||
| const payload = await verifyGitHubOIDCToken(oidcToken, 'kilo-github-action'); | ||
|
|
||
| const repositoryOwner = payload.repository_owner; |
There was a problem hiding this comment.
WARNING: OIDC payload is used without runtime validation/normalization
payload.repository_owner is assumed to be a valid login string and is used directly for the DB lookup. If the claim is missing/unexpected or differs in casing, this will either throw later or incorrectly return 404. Consider validating required claims (e.g., repository_owner, repository) and normalizing login casing before calling the DB query.
| audience: expectedAudience, | ||
| }); | ||
|
|
||
| return payload as GitHubOIDCTokenPayload; |
There was a problem hiding this comment.
WARNING: Unsafe cast of JWT payload can hide missing/incorrect claims
jwtVerify() returns an untyped payload. Casting with as GitHubOIDCTokenPayload skips runtime validation, so downstream code can assume properties exist when they do not. Consider validating with a schema (e.g., Zod) and returning a typed, validated object.
| .where( | ||
| and( | ||
| eq(platform_integrations.platform, PLATFORM.GITHUB), | ||
| eq(platform_integrations.platform_account_login, accountLogin), |
There was a problem hiding this comment.
WARNING: GitHub logins are case-insensitive; exact-match lookup may fail
platform_account_login comparisons are case-sensitive in many DB collations. If logins are stored with different casing than the incoming accountLogin (e.g., from OIDC repository_owner), this can produce false negatives. Consider normalizing to lowercase at write time and query time (or using a case-insensitive comparison).
Code Review SummaryStatus: 11 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (7 files)
|
- Fix case-insensitive account lookup in findGitHubIntegrationByAccountLogin - Add repository_id and repository_visibility to OIDC token payload - Add repository-scoped token generation to generateGitHubInstallationToken - Validate OIDC repository_owner_id matches stored platform_account_id - Validate OIDC repository is in installation scope (for selected repos) - Scope OIDC tokens to requesting repository only - Verify PAT has write permissions (not just read) before token exchange - Scope PAT tokens to validated repository only (fixes privilege escalation) - Add audit logging for all token exchanges Fixes high-severity privilege escalation where read-only PAT with access to one repo could obtain write access to all repos in the installation.
Security Improvements AddedAfter a comprehensive security review, I've added critical security hardening to all three GitHub integration endpoints: 🔒 Security FixesHigh Severity - Privilege Escalation FixedPAT Endpoint (): Fixed a privilege escalation vulnerability where a PAT with read access to a single repository could be exchanged for an installation token with write access to all repositories in the installation. Mitigations:
Medium Severity - OIDC Validation HardenedOIDC Endpoint (): Added multiple validation layers to prevent edge cases and potential abuse. Improvements:
Low Severity - Case Sensitivity FixedDatabase Query: Fixed case-sensitive comparison on case-insensitive GitHub usernames/org names. Fix:
📊 Changes Summary5 files modified, +72 lines added:
✅ Backward CompatibilityAll changes are backward compatible:
🧪 Testing Recommendations
|
| const payload = await verifyGitHubOIDCToken(oidcToken, 'kilo-github-action'); | ||
|
|
||
| const repositoryOwner = payload.repository_owner; | ||
| const repositoryOwnerId = payload.repository_owner_id; |
There was a problem hiding this comment.
WARNING: repository_owner_id type mismatch can cause false "owner mismatch" rejections
GitHub OIDC claims like repository_owner_id are typically numeric in the JWT payload; after jose parsing, this may be a number while integration.platform_account_id is stored as a string. The strict inequality check can then fail even when the IDs represent the same value, returning 403 unexpectedly.
| const repositoryOwnerId = payload.repository_owner_id; | |
| const repositoryOwnerId = String(payload.repository_owner_id); |
GitHub OIDC claims may be parsed as numbers by jose, but platform_account_id is stored as text in the database. Convert repositoryOwnerId to string to prevent false 403 rejections due to type mismatch (e.g., 12345 !== '12345').
Type Mismatch FixFixed a type comparison issue in the OIDC owner ID validation: Problem: GitHub OIDC Solution: Normalize Commit: e184c96 |
| console.error('GitHub installation check failed:', error); | ||
| Sentry.captureException(error); | ||
| return NextResponse.json( | ||
| { error: error instanceof Error ? error.message : 'Internal server error' }, |
There was a problem hiding this comment.
WARNING: Returning raw exception messages can leak internals
This response exposes error.message to the caller, which may include sensitive details (upstream errors, stack context, etc.). Prefer a generic error message in the HTTP response and rely on Sentry/server logs for diagnostics.
| { error: error instanceof Error ? error.message : 'Internal server error' }, | |
| { error: 'Internal server error' }, |
| const { token } = await generateGitHubInstallationToken( | ||
| integration.platform_installation_id, | ||
| integration.github_app_type || 'standard', | ||
| [`${owner}/${repo}`] |
There was a problem hiding this comment.
WARNING: Scope installation token using canonical repository name
repositoryNames is built from user-provided owner/repo. Using the canonical name from GitHub (repoData.full_name) avoids casing/canonicalization mismatches and reduces the chance the installation-token request fails unexpectedly.
| [`${owner}/${repo}`] | |
| [repoData.full_name] |
| Sentry.captureException(error); | ||
|
|
||
| return NextResponse.json( | ||
| { error: error instanceof Error ? error.message : 'Internal server error' }, |
There was a problem hiding this comment.
WARNING: Returning raw exception messages can leak internals
This 500 response exposes error.message to the caller. Prefer a generic message in the response and keep details in Sentry/server logs.
| { error: error instanceof Error ? error.message : 'Internal server error' }, | |
| { error: 'Internal server error' }, |
| } | ||
|
|
||
| return NextResponse.json( | ||
| { error: error instanceof Error ? error.message : 'Internal server error' }, |
There was a problem hiding this comment.
WARNING: Returning raw exception messages can leak internals
This 500 response exposes error.message to the caller. Prefer a generic message in the response and keep details in Sentry/server logs.
| { error: error instanceof Error ? error.message : 'Internal server error' }, | |
| { error: 'Internal server error' }, |
| import { findGitHubIntegrationByAccountLogin } from '@/lib/integrations/db/platform-integrations'; | ||
| import * as Sentry from '@sentry/nextjs'; | ||
|
|
||
| export async function GET(request: NextRequest) { |
There was a problem hiding this comment.
Can you give a little more info on:
- Why we need to check the installation in the first place
- Why we need to check by owner?
Shouldn't an authenticated user be required here? And then we can simply query their own `platform_integrations?
There was a problem hiding this comment.
@jrf0110 It's to support the kilo github install and kilo github run commands of the CLI, so the CLI needs an endpoint to check if the installation ends to continue with the generation of the .github/workflow files.
2 participants
Summary
Implements three new GitHub integration endpoints to support the Kilo GitHub Action and CLI.
New Endpoints
Implementation Details
OIDC Token Exchange (Production)
https://token.actions.githubusercontent.com) and audience (kilo-github-action)PAT Token Exchange (Development)
Installation Check (CLI)
{ installation: true/false }based on database lookupkilo github installcommandFiles Changed
src/app/api/integrations/github/exchange-token/route.ts(new) - 48 linessrc/app/api/integrations/github/exchange-token-with-pat/route.ts(new) - 64 linessrc/app/api/integrations/github/check-installation/route.ts(new) - 24 linessrc/lib/integrations/platforms/github/oidc.ts(new) - 47 linessrc/lib/integrations/db/platform-integrations.ts(modified) - addedfindGitHubIntegrationByAccountLogin()GitHub Action Migration Required
The GitHub Action in
/home/catrielmuller/Dev/Kilo-Org/kilo/github/index.tsneeds URL updates:Old URLs:
https://api.kilo.ai/exchange_github_app_tokenhttps://api.kilo.ai/exchange_github_app_token_with_patNew URLs:
https://api.kilo.ai/api/integrations/github/exchange-tokenhttps://api.kilo.ai/api/integrations/github/exchange-token-with-patTesting
OIDC Endpoint
Requires GitHub Actions workflow with
id-token: writepermission.PAT Endpoint
Installation Check
curl "https://api.kilo.ai/api/integrations/github/check-installation?owner=kilo-org"Dependencies
jose- JWT verification and JWKS handling (already in lockfile)@octokit/rest- GitHub API client (existing)