diff --git a/IMPLEMENTATION_PLAN.md b/IMPLEMENTATION_PLAN.md new file mode 100644 index 00000000..29223043 --- /dev/null +++ b/IMPLEMENTATION_PLAN.md @@ -0,0 +1,716 @@ +# ๐Ÿ“‹ GitHub App Authentication Implementation Plan + +## ๐ŸŽฏ Overview + +This document outlines the implementation plan for switching zai-code-bot from Personal Access Token (PAT) authentication to GitHub App authentication. This change enables the bot to post comments as a GitHub App (type 'Bot') instead of as a user, improving security and scalability. + +## ๐Ÿ“… Timeline + +| Phase | Duration | Priority | +| ----------------------------------- | ------------- | -------- | +| Phase 1: Infrastructure Preparation | 1-2 days | โญโญโญ | +| Phase 2: Core Development | 2-3 days | โญโญโญ | +| Phase 3: Integration | 2-3 days | โญโญโญ | +| Phase 4: Testing | 2-3 days | โญโญโญ | +| Phase 5: Deployment | 1 day | โญโญโญ | +| **Total** | **8-12 days** | | + +--- + +## ๐Ÿ—๏ธ Phase 1: Infrastructure Preparation + +### Task 1.1: Create GitHub App + +**Assignee:** DevOps / Backend Engineer +**Duration:** 1 day +**Status:** โฌœ Pending + +#### Steps + +1. Navigate to [GitHub โ†’ Settings โ†’ Developer settings โ†’ GitHub Apps](https://github.com/settings/apps) +2. Click **"New GitHub App"** +3. Fill in: + - **GitHub App name:** `zai-code-bot` + - **Homepage URL:** `https://zai-worker.tokenbel.info` + - **Callback URL:** (leave empty) + - **Description:** `AI-powered code review assistant for pull requests` + +4. **Configure Permissions:** + + | Section | Permission | Reason | + | ------------------- | ------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | + | Repository contents | Read-only | Read files for context | + | Pull requests | Read & Write | Read PR, post comments | + | Issues | Read & Write | Read issue, post comments | + | Comments | Read & Write | Manage comments | + | Repository metadata | Read-only | Get repository info | + | **Collaborators** | **Read-only** | **`GET /repos/{o}/{r}/collaborators/{user}` โ€” the `/zai` authorization gate. Without it GitHub returns 403 "Resource not accessible by integration" and every command fails.** | + +5. **Subscribe to Events:** + - โœ… `Issue comment` + - โœ… `Pull request` + - โœ… `Pull request review comment` + +6. **Generate Private Key:** + - Click **"Generate a private key"** + - Save the `.pem` file securely (it's only shown once!) + - Store in secure password manager (1Password, Vault, etc.) + +7. **Record App ID:** + - Note the **App ID** (e.g., `123456`) from the app settings page + +8. **Install the App:** + - Click **"Install App"** + - Select organizations/accounts for testing + - Note the **Installation ID** for testing + +#### Artifacts + +- `GITHUB_APP_ID` (e.g., `123456`) +- `GITHUB_APP_PRIVATE_KEY` (contents of `.pem` file) +- `TEST_INSTALLATION_ID` (for testing) + +--- + +### Task 1.2: Configure Cloudflare Secrets + +**Assignee:** DevOps +**Duration:** 1 day +**Status:** โฌœ Pending + +#### Steps + +```bash +# Add new secrets to existing Secrets Store +# Store ID: 629e5dd6594845a889e6ddabb26cc009 + +# 1. Add GITHUB_APP_ID +npx wrangler secrets:store create ZAI_GITHUB_APP_ID --store-id 629e5dd6594845a889e6ddabb26cc009 +echo "123456" | npx wrangler secrets:store write ZAI_GITHUB_APP_ID --store-id 629e5dd6594845a889e6ddabb26cc009 + +# 2. Add GITHUB_APP_PRIVATE_KEY +npx wrangler secrets:store create ZAI_GITHUB_APP_PRIVATE_KEY --store-id 629e5dd6594845a889e6ddabb26cc009 +# Paste the contents of .pem file (including -----BEGIN PRIVATE KEY-----) +cat zai-code-bot.pem | npx wrangler secrets:store write ZAI_GITHUB_APP_PRIVATE_KEY --store-id 629e5dd6594845a889e6ddabb26cc009 +``` + +#### Verification + +```bash +# Verify secrets were added +npx wrangler secrets:store list --store-id 629e5dd6594845a889e6ddabb26cc009 +``` + +#### Artifacts + +- Secrets `ZAI_GITHUB_APP_ID` and `ZAI_GITHUB_APP_PRIVATE_KEY` in Cloudflare Secrets Store + +--- + +### Task 1.3: Update Wrangler Configuration + +**Assignee:** Backend Engineer +**Duration:** 1 day +**Status:** โฌœ Pending + +#### Steps + +1. **Update `src/zai-main-worker/wrangler.toml`** + + ```toml + # Add new bindings for GitHub App + [[secrets_store_secrets]] + binding = "GITHUB_APP_ID" + store_id = "629e5dd6594845a889e6ddabb26cc009" + secret_name = "ZAI_GITHUB_APP_ID" + + [[secrets_store_secrets]] + binding = "GITHUB_APP_PRIVATE_KEY" + store_id = "629e5dd6594845a889e6ddabb26cc009" + secret_name = "ZAI_GITHUB_APP_PRIVATE_KEY" + ``` + +2. **Update `src/zai-heavy-worker/wrangler.toml`** + + ```toml + # Add the same bindings + [[secrets_store_secrets]] + binding = "GITHUB_APP_ID" + store_id = "629e5dd6594845a889e6ddabb26cc009" + secret_name = "ZAI_GITHUB_APP_ID" + + [[secrets_store_secrets]] + binding = "GITHUB_APP_PRIVATE_KEY" + store_id = "629e5dd6594845a889e6ddabb26cc009" + secret_name = "ZAI_GITHUB_APP_PRIVATE_KEY" + ``` + +#### Artifacts + +- Updated `wrangler.toml` files + +--- + +### Task 1.4: Create Database Migration + +**Assignee:** Backend Engineer +**Duration:** 1 day +**Status:** โฌœ Pending + +#### Steps + +1. **Create migration file:** `src/zai-main-worker/migrations/0004_add_installation_id.sql` + + ```sql + -- Migration: Add installation_id to jobs table for GitHub App authentication + -- Up: Add installation_id column to jobs table + + ALTER TABLE jobs ADD COLUMN installation_id INTEGER; + + -- Create index for installation_id to speed up queries + CREATE INDEX IF NOT EXISTS idx_jobs_installation_id ON jobs(installation_id); + + -- Add installation_id to webhook_deliveries table for tracking + ALTER TABLE webhook_deliveries ADD COLUMN installation_id INTEGER; + + -- Add installation_id to repositories table for reference + ALTER TABLE repositories ADD COLUMN github_app_installation_id INTEGER; + ``` + +#### Artifacts + +- New migration `0004_add_installation_id.sql` + +--- + +## ๐Ÿ’ป Phase 2: Core Development + +### Task 2.1: Create GitHub App Authentication Module + +**Assignee:** Backend Engineer +**Duration:** 2 days +**Status:** โฌœ Pending + +#### Steps + +1. **Create file:** `src/shared/github-app-auth.js` + + This module should include: + + - **`generateAppJwt(appId, privateKey)`**: Generates a JWT token signed with the app's private key + - **`getInstallationToken(jwt, installationId)`**: Fetches an installation access token from GitHub + - **`AppTokenCache`**: Class for caching installation tokens using KV namespace + - **`createTokenProvider(env)`**: Factory function that handles JWT generation and caching + +2. **Key Implementation Details:** + - JWT should expire in 9 minutes (GitHub recommends max 10 minutes) + - Use Web Crypto API for JWT signing (available in Cloudflare Workers) + - Handle private key normalization (remove headers, whitespace) + - Cache tokens for 5 minutes to avoid generating JWT for every request + +3. **Error Handling:** + + > **โš  SUPERSEDED by [`PAT_REMOVAL_PLAN.md`](./PAT_REMOVAL_PLAN.md):** PAT + > fallback has been removed โ€” GitHub App auth is the only auth path. Auth + > failures are loud (503 in the main worker, classified fail/retry in the + > queue), never silent fallbacks. + - Classified `appAuthError` codes: `app_token_fetch_failed` (retryable), + `app_jwt_rejected`, `app_suspended`, `installation_not_found`, + `app_auth_unconfigured`, `missing_installation_id` (all permanent) + +#### Artifacts + +- `src/shared/github-app-auth.js` +- `src/tests/github-app-auth.test.js` (tests for the module) + +--- + +### Task 2.2: Update GitHubClient + +**Assignee:** Backend Engineer +**Duration:** 1 day +**Status:** โฌœ Pending + +#### Steps + +1. **Update `src/shared/github.js`** + + Modify the constructor to accept an `isApp` option: + + ```javascript + constructor(token, opts = {}) { + this.token = token; + this.baseUrl = GITHUB_API_BASE; + this.userAgent = opts.userAgent || 'zai-code-bot-workers'; + this.isApp = opts.isApp || false; // NEW + } + ``` + +2. **Update the `request` method** to use `Bearer` for App tokens: + + ```javascript + const options = { + method, + headers: { + Authorization: this.isApp ? `Bearer ${this.token}` : `token ${this.token}`, + Accept: 'application/vnd.github+json', + 'X-GitHub-Api-Version': '2022-11-28', + 'User-Agent': this.userAgent, + }, + }; + ``` + +3. **Update JSDoc comments** to reflect support for both PAT and Installation Tokens + +#### Artifacts + +- Updated `src/shared/github.js` + +--- + +## ๐Ÿ”ง Phase 3: Integration + +### Task 3.1: Update Main Worker + +**Assignee:** Backend Engineer +**Duration:** 2 days +**Status:** โฌœ Pending + +#### Steps + +1. **Add import for token provider:** + + ```javascript + import { createTokenProvider } from '../../shared/github-app-auth.js'; + ``` + +2. **Create helper function for GitHub client creation:** + + > **โš  SUPERSEDED by [`PAT_REMOVAL_PLAN.md`](./PAT_REMOVAL_PLAN.md):** the + > PAT fallback below was removed. `createAppGitHubClient` is App-only, + > built lazily on the command path (PR events mint no tokens), and throws + > `status: 503` errors so GitHub redelivers on auth/config failures. + + ```javascript + async function createGitHubClient(env, installationId, logger) { + // Try GitHub App authentication first + if (installationId && env.GITHUB_APP_ID && env.GITHUB_APP_PRIVATE_KEY) { + try { + const tokenProvider = createTokenProvider(env); + const token = await tokenProvider.getInstallationToken(installationId); + logger.info('Using GitHub App authentication', { installationId }); + return { github: new GitHubClient(token, { isApp: true }), isAppAuth: true }; + } catch (appAuthError) { + logger.warn('GitHub App authentication failed, falling back to PAT', { + error: appAuthError.message, + installationId, + }); + } + } + + // Fallback to PAT + const token = await resolveSecretValue(env.GITHUB_TOKEN); + logger.info('Using PAT authentication'); + return { github: new GitHubClient(token), isAppAuth: false }; + } + ``` + +3. **Update `fetch` handler** to: + - Extract `installationId` from webhook payload + - Use the new `createGitHubClient` function + - Pass `installationId` to job creation functions + +4. **Update `createCommandDurableJob`** to include `installationId`: + + ```javascript + async function createCommandDurableJob( + env, + github, + webhookData, + kind, + deliveryId, + installationId, + ) { + // ... existing code ... + const event = { + // ... existing fields ... + installationId: installationId, + }; + return createCommandJob(env.BOT_DB, event, kind); + } + ``` + +5. **Update `createPrContextJob` calls** to pass `installationId` + +#### Artifacts + +- Updated `src/zai-main-worker/src/index.js` + +--- + +### Task 3.2: Update Heavy Worker + +**Assignee:** Backend Engineer +**Duration:** 2 days +**Status:** โฌœ Pending + +#### Steps + +1. **Add import for token provider:** + + ```javascript + import { createTokenProvider } from '../../shared/github-app-auth.js'; + ``` + +2. **Create helper function for queue GitHub client:** + + > **โš  SUPERSEDED by [`PAT_REMOVAL_PLAN.md`](./PAT_REMOVAL_PLAN.md):** no + > PAT fallback โ€” the strict version fails the job for permanent auth + > errors (`missing_installation_id`, `app_auth_unconfigured`, + > `app_jwt_rejected`) and retries only transient mint failures. + + ```javascript + async function createQueueGitHubClient(env, job, logger) { + const installationId = job.installation_id; + + if (installationId && env.GITHUB_APP_ID && env.GITHUB_APP_PRIVATE_KEY) { + try { + const tokenProvider = createTokenProvider(env); + const token = await tokenProvider.getInstallationToken(installationId); + logger.info('Using GitHub App authentication for queue job', { + installationId, + jobId: job.job_id, + }); + return new GitHubClient(token, { isApp: true }); + } catch (appAuthError) { + logger.warn('GitHub App authentication failed in queue, falling back to PAT', { + error: appAuthError.message, + installationId, + jobId: job.job_id, + }); + } + } + + // Fallback to PAT + const token = await resolveSecretValue(env.GITHUB_TOKEN); + logger.info('Using PAT authentication for queue job', { jobId: job.job_id }); + return new GitHubClient(token); + } + ``` + +3. **Update `processQueueMessage`** to use the new client creation function + +#### Artifacts + +- Updated `src/zai-heavy-worker/src/queue.js` + +--- + +### Task 3.3: Update Storage Layer + +**Assignee:** Backend Engineer +**Duration:** 1 day +**Status:** โฌœ Pending + +#### Steps + +1. **Update `src/shared/storage/deliveries.js`** + + - Add `installation_id` to the JOB_BASE query + - Update `createPrJob` to accept and store `installationId` + - Update `createPrContextJob` to accept `installationId` parameter + - Update `createCommandJob` to pass `installationId` to `createPrJob` + +2. **Example changes:** + + ```javascript + const JOB_BASE = ` + SELECT j.job_id, j.delivery_id, j.kind, j.repository_id, j.pr_number, + j.head_sha, p.base_sha, + j.status, j.attempt_count, j.available_at, j.claimed_at, j.lease_expires_at, + j.completed_at, j.last_error_code, j.last_failure_at, j.config_version, + j.installation_id, // NEW + r.owner AS repository_owner, r.name AS repository_name, r.full_name AS repository_full_name, + p.title, p.author_login, p.state, p.closed_by + FROM jobs j + JOIN repositories r ON r.repository_id = j.repository_id + JOIN pull_requests p ON p.repository_id = j.repository_id AND p.pr_number = j.pr_number + `; + + // In createPrJob function: + const installationId = event.installationId || null; + + // In SQL INSERT: + `INSERT INTO jobs + (job_id, delivery_id, kind, repository_id, pr_number, head_sha, installation_id, status, + attempt_count, available_at, config_version, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?, 'queued', 0, ?, 1, ?, ?)`; + ``` + +#### Artifacts + +- Updated `src/shared/storage/deliveries.js` + +--- + +## ๐Ÿงช Phase 4: Testing + +### Task 4.1: Unit Tests + +**Assignee:** QA Engineer / Backend Engineer +**Duration:** 2 days +**Status:** โฌœ Pending + +#### Steps + +1. **Create tests for `github-app-auth.js`:** + - Test JWT generation + - Test Installation Token fetching (mock fetch) + - Test token caching + - Test error handling + +2. **Update existing tests:** + - Update `github.test.js` to test both PAT and App authentication + - Update `index-fetch.test.js` to test new authentication flow + - Update `queue.test.js` to test queue authentication + +3. **Test coverage goals:** + - 100% coverage for new `github-app-auth.js` module + - Maintain existing coverage levels + +#### Artifacts + +- `src/tests/github-app-auth.test.js` +- Updated existing test files + +--- + +### Task 4.2: Integration Testing + +**Assignee:** QA Engineer +**Duration:** 2 days +**Status:** โฌœ Pending + +#### Steps + +1. **Local Testing:** + - Run workers locally with test secrets + - Verify JWT generation + - Verify Installation Token retrieval + +2. **Staging Testing:** + - Deploy to staging environment + - Install GitHub App on test repository + - Verify: + - Comments are posted as the app (`type === 'Bot'`) + - All commands work (`/zai review`, `/zai describe`) + - Authorization works correctly + +3. **Comment Ownership Testing:** + - Verify that comments belong to the bot (`isBotOwnedComment`) + - Test updating existing comments + +#### Artifacts + +- Test report +- Error logs (if any) + +--- + +### Task 4.3: Load Testing + +**Assignee:** DevOps +**Duration:** 1 day +**Status:** โฌœ Pending + +#### Steps + +1. **Token Caching Tests:** + - Verify tokens are not generated for every request + - Check performance with caching enabled + +2. **Concurrent Request Tests:** + - Verify multiple requests don't break authentication + - Check for race conditions + +#### Artifacts + +- Load test results + +--- + +## ๐Ÿš€ Phase 5: Deployment + +### Task 5.1: Deployment Preparation + +**Assignee:** DevOps +**Duration:** 1 day +**Status:** โฌœ Pending + +#### Steps + +1. **Create release branch:** + + ```bash + git checkout -b feature/github-app-auth + git merge main + ``` + +2. **Verify all changes:** + - Ensure all files are updated + - Run all tests + +3. **Update documentation:** + - Update `README.md` with GitHub App setup instructions + - Update `RUNBOOK.md` with new authentication troubleshooting + +--- + +### Task 5.2: Deploy to Production + +**Assignee:** DevOps +**Duration:** 1 day +**Status:** โฌœ Pending + +#### Steps + +1. **Deploy main-worker:** + + ```bash + cd src/zai-main-worker + npx wrangler deploy + ``` + +2. **Deploy heavy-worker:** + + ```bash + cd src/zai-heavy-worker + npx wrangler deploy + ``` + +3. **Apply database migrations:** + + ```bash + npx wrangler d1 execute bot-db --file=../migrations/0004_add_installation_id.sql + ``` + +4. **Configure GitHub App Webhook:** + - Remove old webhook (if any) + - Create new webhook from GitHub App: + - **Payload URL:** `https://zai-worker.tokenbel.info/github/webhook` + - **Content type:** `application/json` + - **Secret:** `ZAI_GITHUB_WEBHOOK_KEY` (same as before) + - **Events:** `Issue comment`, `Pull request`, `Pull request review comment` + +--- + +### Task 5.3: Monitoring and Rollback + +**Assignee:** DevOps +**Duration:** 3 days +**Status:** โฌœ Pending + +#### Steps + +1. **Post-deployment monitoring:** + - Check Cloudflare Workers logs + - Verify comments are posted as the app + - Ensure no authentication errors + +2. **Rollback plan:** + - If something goes wrong: + - Revert to PAT by removing GitHub App usage + - Restore old webhook + - Remove new secrets + +3. **Update status:** + - Close the task after successful deployment + - Document changes + +--- + +## ๐Ÿ“Š Success Metrics + +| Metric | Target Value | Current Value | +| --------------------------- | -------------------- | ------------- | +| Comments from app | 100% | 0% | +| Authentication success rate | 99.9% | 99% (PAT) | +| Token generation time | < 100ms (with cache) | N/A | +| API response time | No change | Baseline | +| Error count | 0 | Current | + +--- + +## ๐ŸŽฏ Risks and Mitigations + +| Risk | Probability | Impact | Mitigation | +| ---------------------------------- | ----------- | -------- | --------------------------------------- | +| JWT generation errors | Low | High | Test locally, use proven libraries | +| Installation Token issues | Medium | High | Fallback to PAT, monitor errors | +| Incompatibility with existing jobs | Medium | Medium | Update database schema, data migration | +| Webhook issues | Low | High | Test in staging, monitor logs | +| Secret leakage | Very Low | Critical | Use Cloudflare Secrets, restrict access | + +--- + +## ๐Ÿ“š Documentation Updates + +### Files to Update + +1. **README.md** + - Add section on GitHub App setup + - Update deployment instructions + +2. **RUNBOOK.md** + - Add troubleshooting for GitHub App authentication + - Describe how to check comment types + +3. **ARCHITECTURE.md** + - Update authentication flow diagrams + - Describe new flow + +--- + +## ๐Ÿ† Acceptance Criteria + +- [ ] GitHub App created and configured +- [ ] Cloudflare Secrets updated +- [ ] GitHub App authentication code written and tested +- [ ] GitHubClient supports both authentication types +- [ ] Main Worker uses GitHub App authentication +- [ ] Heavy Worker uses GitHub App authentication +- [ ] All tests pass +- [ ] Comments are posted as the app (`type === 'Bot'`) +- [ ] Fallback to PAT works when GitHub App config is missing +- [ ] Documentation updated +- [ ] Deployment to production completed +- [ ] Monitoring shows stable operation + +--- + +## ๐Ÿ“ž Contacts + +| Role | Responsible | Contact | +| ---------------- | ----------- | ---------------- | +| Project Lead | [Your Name] | @your-github | +| Backend Engineer | [Name] | @backend-dev | +| DevOps | [Name] | @devops-engineer | +| QA Engineer | [Name] | @qa-engineer | + +--- + +## ๐ŸŽ‰ Conclusion + +This plan provides a **smooth transition** from PAT to GitHub App Authentication with: + +- โœ… Minimal downtime (fallback to PAT) +- โœ… Full backward compatibility +- โœ… Improved security +- โœ… Scalability + +**Next Steps:** + +1. Start with **Phase 1: Infrastructure Preparation** +2. Create GitHub App and obtain App ID + Private Key +3. Add secrets to Cloudflare +4. Proceed to development (Phase 2) diff --git a/PAT_REMOVAL_PLAN.md b/PAT_REMOVAL_PLAN.md new file mode 100644 index 00000000..0dc6f7a7 --- /dev/null +++ b/PAT_REMOVAL_PLAN.md @@ -0,0 +1,380 @@ +# ๐Ÿ”ง GitHub App Cutover โ€” Fix Plan (Issues 1โ€“5 + Full PAT Removal) + +Parent plan: [`IMPLEMENTATION_PLAN.md`](./IMPLEMENTATION_PLAN.md) (GitHub App migration). +This plan fixes review issues **#1โ€“#5** and goes one step further than the parent plan: +**all PAT fallback and PAT logic is removed. GitHub App auth becomes the only auth path.** + +--- + +## 0. Scope + +| Review issue | Resolution in this plan | +| ------------------------------------------------------------------------ | ------------------------------------------------------------------------------------ | +| #1 Failing tests (5) + missing coverage for App-auth paths | WS6 โ€” rewrite/extend test files | +| #2 Secrets read without `resolveSecretValue` (silent auth breakage) | WS1 โ€” provider resolves bindings | +| #3 Missing **Collaborators: read-only** App permission | WS8 โ€” App settings + docs + explicit 403 error | +| #4 Eager token mint on every webhook (PR `synchronize` pays for nothing) | WS4 โ€” lazy client construction | +| #5 `refreshPrComments` hard-wires PAT; `isAuthorized` is PAT-only | WS4 / WS3 โ€” App client + delete dead helper | +| โž• Remove PAT entirely | WS2, WS4, WS5, WS7 โ€” `Bearer`-only client, no fallback, drop `GITHUB_TOKEN` bindings | + +Out of scope (filed separately): migration 0004 dead columns (#8), token cache `expires_at` (#9), docs/OKF updates (#7 โ€” only the parent plan's permission table is touched in WS8), prettier-only churn (#6 โ€” touched files are formatted as part of WS9 hygiene). + +### Design decisions locked in by PAT removal + +1. **App-only auth.** No code path may construct a `GitHubClient` from `GITHUB_TOKEN`. The `GITHUB_TOKEN` Secrets Store binding is removed from both workers; the store entry `ZAI_GITHUB_TOKEN` is deleted only after rollout is confirmed stable (rollback window). +2. **`GitHubClient` always sends `Authorization: Bearer `.** The `isApp` option is deleted. Every token in the system is an installation token. +3. **Auth unavailability is a hard, _visible_ failure.** No silent fallbacks: + - Main worker (webhook, synchronous): respond **503** so GitHub retries the delivery; log a specific error code. + - Heavy worker (queue, durable): classify the failure โ€” transient errors retry, config/permanent errors fail the job with a distinct code (see ยงWS5 table). +4. **Legacy pre-migration jobs** (`installation_id IS NULL`, created before this deploy) can never authenticate. They fail fast with `missing_installation_id` (โ‰ค 3 attempts โ†’ `failed`). Mitigation: deploy during a drained/quiet queue (ยงWS9). +5. **`resolveBotLogin`'s `GET /user` probe is removed.** Installation tokens cannot call `GET /user` (403). `isBotOwnedComment`'s `type === 'Bot'` check covers all App-posted comments; the optional `GITHUB_BOT_LOGIN` var remains the only way to also match legacy PAT-era help comments (`type 'User'`). Review/describe upserts need no change: `comment_publications.github_comment_id` (exact-id match) already recognizes legacy PAT-posted result comments. +6. **Error surface rule (SECURITY.md):** token mint failures log codes + short messages; raw provider bodies stay out of GitHub comments and out of logs (current `err.body` field on mint errors must not be logged โ€” see WS1). + +--- + +## WS1 โ€” `src/shared/github-app-auth.js` (fixes #2, #10-naming) + +### Changes + +1. Import and use `resolveSecretValue` from `./secrets.js` for **both** `GITHUB_APP_ID` and `GITHUB_APP_PRIVATE_KEY`. This handles the three real-world binding shapes (plain string, `{get()}`, `Promise`) documented in `secrets.js` for this exact store (`629e5ddโ€ฆ`). Raw `env.*` reads are forbidden. +2. `createTokenProvider` becomes **async** (secrets must be awaited) and returns a provider with an explicit availability flag: + +```js +import { resolveSecretValue } from './secrets.js'; + +export async function createTokenProvider(env) { + const appId = await resolveSecretValue(env.GITHUB_APP_ID); + const privateKey = await resolveSecretValue(env.GITHUB_APP_PRIVATE_KEY); + const cache = env.BOT_CACHE ? new AppTokenCache(env.BOT_CACHE) : null; + + return { + available: Boolean(appId && privateKey), + async getInstallationToken(installationId) { + if (!appId || !privateKey) { + throw appAuthError( + 'app_auth_unconfigured', + 'GITHUB_APP_ID / GITHUB_APP_PRIVATE_KEY not configured', + { retryable: false }, + ); + } + if (!installationId) { + throw appAuthError('missing_installation_id', 'webhook/job carries no installation id', { + retryable: false, + }); + } + const cached = cache ? await cache.get(installationId) : null; + if (cached) return cached; + + const jwt = await generateAppJwt(appId, privateKey); + const token = await fetchInstallationToken(jwt, installationId); + + if (cache) await cache.set(installationId, token); + return token; + }, + }; +} +``` + +1. Rename module-level `getInstallationToken(jwt, installationId)` โ†’ **`fetchInstallationToken`** (it clashes with the provider method of the same name/arity-1; WS6 tests updated). +2. Add a small `appAuthError(code, message, { retryable, status })` factory. Map HTTP responses from the token endpoint: + +| Mint response / condition | `code` | `retryable` | +| --------------------------------- | ------------------------- | ----------- | +| network error, HTTP 5xx, 429 | `app_token_fetch_failed` | โœ… true | +| 401 (bad JWT / wrong key) | `app_jwt_rejected` | โŒ false | +| 403 (app suspended) | `app_suspended` | โŒ false | +| 404 (installation removed) | `installation_not_found` | โŒ false | +| secrets missing | `app_auth_unconfigured` | โŒ false | +| no installation id on job/webhook | `missing_installation_id` | โŒ false | + +Do **not** attach `err.body` (raw GitHub response) to errors that get logged; keep only status + code (the current code sets `err.body = body` โ€” drop it or mark it non-enumerable). 5. `generateAppJwt` body is unchanged (it is correct). `AppTokenCache` unchanged (5-min KV TTL; `expires_at` handling stays out of scope). + +### Acceptance + +- Unit tests pass with a **real RSA keypair** (see WS6). +- Provider resolves all three binding shapes; `available === false` when either secret is missing/blank. + +--- + +## WS2 โ€” `src/shared/github.js` (PAT removal in the client) + +1. Delete the `isApp` option from the constructor and JSDoc. +2. `Authorization: \`Bearer ${this.token}\`` unconditionally. +3. JSDoc: token param documented as "GitHub App installation access token". + +Tests: `src/tests/github.test.js` line 63 flips from `'token mock-token'` to `'Bearer mock-token'`; any `isApp` option tests are removed/replaced by a single Bearer assertion. + +--- + +## WS3 โ€” `src/shared/auth.js` (fixes #5b, PAT-only helper) + +1. **Delete `isAuthorized(env, owner, repo, username)`** โ€” it builds a PAT client and has no production callers. +2. `authorizeCommenter` stays, plus one hardening for the App world (#3): map **403** to a clear error instead of propagating an opaque one: + +```js +if (error.status === 404) return false; // not a collaborator +if (error.status === 403) { + // Installation token lacks the "Collaborators: read-only" permission. + const e = new Error( + 'app_permission_missing: collaborator check requires Collaborators read-only', + ); + e.code = 'app_permission_missing'; + e.retryable = false; + throw e; +} +throw error; +``` + +1. Update header comment (no more "shared PAT" language). + +Tests: `src/tests/auth.test.js` โ€” remove the `isAuthorized` describe block; add a 403 โ†’ `app_permission_missing` test. + +--- + +## WS4 โ€” `src/zai-main-worker/src/index.js` (fixes #4, #5a, PAT removal) + +### 4.1 Lazy client construction (fix #4) + +- **Delete** the eager `createGitHubClient(env, installationId, logger)` call at line ~152 (right after payload parsing). +- Keep `const installationId = webhookData.installation?.id;` where it is โ€” it is needed by the PR-context path and comment-refresh path. +- Build the client **only on the command path**, after Gate 4 (`parseCommand` succeeded) and before Gate 5 (authorization): + +```js +// --- Gate 4.5: GitHub App client (App-only auth; no PAT fallback) --- +const github = await createAppGitHubClient(env, installationId, logger); +``` + +- The PR-context path (`pull_request` โ†’ `createPrContextJob`) constructs **no** client (it only writes D1 + enqueues). `createCommandDurableJob` reuses the same client for `getPullRequest`. Help/unauthorized/unsupported posts reuse it. +- `createAppGitHubClient` (replaces `createGitHubClient`, drops `isAppAuth` โ€” unused): + +```js +async function createAppGitHubClient(env, installationId, logger) { + const provider = await createTokenProvider(env); + if (!installationId) { + logger.error('Webhook carried no installation id; is the webhook source the GitHub App?', {}); + throw Object.assign(new Error('missing_installation_id'), { status: 503 }); + } + if (!provider.available) { + logger.error('GitHub App auth unconfigured (GITHUB_APP_ID / GITHUB_APP_PRIVATE_KEY)', {}); + throw Object.assign(new Error('app_auth_unconfigured'), { status: 503 }); + } + const token = await provider.getInstallationToken(installationId); + logger.info('Using GitHub App authentication', { installationId }); + return new GitHubClient(token); +} +``` + +- A thrown 503-ish error inside the command path is caught by the existing catch-all โ†’ today that returns 500; **change**: let `createAppGitHubClient` errors map to `503` (GitHub redelivers webhooks on 5xx; 500 also triggers redelivery, but 503 is the accurate semantic). Implement by checking `error.status` in the catch-all or by local try/catch around the client creation returning `json(503, โ€ฆ)`/`Response 503`. Keep messages generic to callers (SECURITY.md). +- A failed mint for a _help_ command fails the same way โ€” acceptable (config error should be loud). + +### 4.2 `refreshPrComments` on App auth (fix #5a) + +```js +async function refreshPrComments(env, plan, installationId, logger) { + // Best-effort: on App-auth failure we skip the mirror (slice is derivative; + // the next gather rewrites it wholesale). Never throw into waitUntil. + try { + const github = await createAppGitHubClient(env, installationId, logger); + return await refreshCommentsSlice({ github, bucket: env.BOT_ARTIFACTS, ...plan }); + } catch (error) { + logger.warn('Comment slice refresh skipped: app auth unavailable', { code: error?.code }); + return null; + } +} +``` + +- Call site: `ctx.waitUntil(refreshPrComments(env, plan, installationId, logger).catch(() => {}))`. +- Restore the doc comment that was deleted in the previous commit (explains the throwaway-client rationale) โ€” updated for App auth. + +### 4.3 `resolveBotLogin` de-PAT (design decision 5) + +```js +/** Optional var matching legacy PAT-era help comments (type 'User'). */ +function resolveBotLogin(env) { + return env?.GITHUB_BOT_LOGIN || null; +} +``` + +- No API call (installation tokens cannot `GET /user`). `postHelp` signature unchanged (`botLogin` still fed to `isBotOwnedComment`). +- Document the one-time cost: if `GITHUB_BOT_LOGIN` is unset, a legacy PAT help comment is not recognized and a fresh help comment is posted once. + +### 4.4 Cleanup in the same file + +- Remove the `## โš ๏ธ` escape-sequence churn in `postUnauthorizedComment` (restore literal emoji) and the `postHelp` find() reformat โ€” keep the diff to real changes (review #6 residue in touched hunks). +- No other reference to `GITHUB_TOKEN` / `resolveSecretValue` remains in this file except the webhook secret (Gate 3). + +--- + +## WS5 โ€” `src/zai-heavy-worker/src/queue.js` (PAT removal, error classification) + +Replace `createQueueGitHubClient` with a strict version: + +```js +async function createQueueGitHubClient(env, job, logger) { + if (!job.installation_id) { + // Pre-migration job: can never authenticate post-PAT-removal. + const e = new Error('missing_installation_id'); + e.retryable = false; + e.code = 'missing_installation_id'; + throw e; + } + const provider = await createTokenProvider(env); + if (!provider.available) { + const e = new Error('app_auth_unconfigured'); + e.retryable = false; + e.code = 'app_auth_unconfigured'; + throw e; + } + const token = await provider.getInstallationToken(job.installation_id); // throws classified appAuthError + logger.info('Using GitHub App authentication for queue job', { + installationId: job.installation_id, + jobId: job.job_id, + }); + return new GitHubClient(token); +} +``` + +- `fetchInstallationToken` errors already carry `retryable`/`code` (WS1); the existing queue catch block honors `error.retryable !== false` โ†’ transient mints (`app_token_fetch_failed`) retry with backoff, permanent ones fail the job (`markJobFailed`) with the cause code in `safeErrorCode(error)` โ†’ logs. +- Remove `resolveSecretValue(env.GITHUB_TOKEN)` import usage from this file (the import can go if unused). +- `src/shared/storage/jobs.js`/`deliveries.js` need **no** change: `claimJob` โ†’ `getJob` already returns `installation_id` via `JOB_BASE`. + +--- + +## WS6 โ€” Tests (fixes #1; keeps CI + coverage thresholds green) + +Thresholds to hold: `src/shared/**` โ‰ฅ 95 lines / 93 branches; `src/zai-main-worker/src/**` โ‰ฅ 90. New code paths must be covered โ€” the matrix below is the definition of done. + +### 6.1 Rewrite `src/tests/github-app-auth.test.js` (4 currently-failing tests) + +Root cause of today's failures: the fabricated `TEST_PRIVATE_KEY` is not valid base64 โ†’ `atob` throws in `generateAppJwt`. Fix with a **real** keypair (no fixtures): + +```js +import { generateKeyPairSync } from 'node:crypto'; + +const { publicKey, privateKey } = generateKeyPairSync('rsa', { modulusLength: 2048 }); +const TEST_PRIVATE_KEY = privateKey.export({ type: 'pkcs8', format: 'pem' }).toString(); +const PUBLIC_KEY_DER = publicKey.export({ type: 'spki', format: 'der' }); +``` + +| Test | Assertion | +| ------------------------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| JWT structure | 3 dot-separated base64url parts | +| JWT header | `{ alg: 'RS256', typ: 'JWT' }` after decode | +| JWT payload | `iss === appId`, `exp - iat === 540`, `iat` โ‰ˆ now | +| **JWT signature** | `crypto.subtle.verify('RSASSA-PKCS1-v1_5', importedSpki, sigBytes, data)` is `true` | +| Key formatting | PEM with `\r\n`, blank lines, surrounding whitespace still works | +| `fetchInstallationToken` | success (token + URL + headers), 404/401/403/5xx/network error mapping (code + retryable flags per WS1 table) | +| `AppTokenCache` | keep the 5 existing passing tests | +| `createTokenProvider` | (a) all three secret binding shapes resolve โ€” string, `{get}`, `Promise`; (b) `available === false` when a secret is missing/blank; (c) cache hit โ†’ no `fetch`; (d) cache miss โ†’ fetch once + `put`; (e) mint error propagates with classification | + +### 6.2 `src/tests/index-fetch.test.js` + +1. **Fix line 578** โ€” the stale assertion. Add `installation: { id: 456 }` to the `prEventPayload`/`prCommentPayload` fixtures and assert `createPrContextJob` was called with `(env.BOT_DB, event, 456)`. +2. `makeEnv()`: replace `GITHUB_TOKEN: 'gh-token'` with `GITHUB_APP_ID`/`GITHUB_APP_PRIVATE_KEY` (via mocked provider โ€” see 3). +3. `vi.mock('../../shared/github-app-auth.js')` (matching the existing `GitHubClient` mock pattern) with a controllable provider: `{ available: true, getInstallationToken: vi.fn().mockResolvedValue('ghs_test') }`. +4. New tests (App-auth coverage for the main worker): + +| Scenario | Expectation | +| ----------------------------------- | -------------------------------------------------------------------------------------------------------------------- | +| Command (`/zai review`) | provider called with `456`; client created; job flow as before | +| **PR `synchronize`** (fix #4) | provider **never** called (no token mint), 202 + job created as today | +| Non-command comment / unknown event | provider never called | +| `installation` absent from payload | 503; no job; error log `missing_installation_id` | +| Provider `available: false` | 503; `app_auth_unconfigured` | +| Mint failure | 503 (no 500); no job | +| Comment-refresh event | refresh path receives `installationId`; when provider fails, refresh is skipped (warn) and the webhook still 200/202 | +| `GITHUB_BOT_LOGIN` unset | help flow works with `botLogin === null` (no `GET /user` call) | + +### 6.3 `src/tests/queue.test.js` + +- Fixture jobs get `installation_id: 456`; env gets the mocked provider (or `GITHUB_APP_ID`/key strings with `fetchInstallationToken` mocked โ€” follow the file's existing mock style). +- New tests: + +| Scenario | Expectation | +| ------------------------------------ | ---------------------------------------------------------------------------- | +| Happy path | handler receives a Bearer client; `markJobSucceeded`; `message.ack()` | +| `installation_id: null` (legacy job) | `markJobFailed` (no retry), `message.ack()`, cause `missing_installation_id` | +| Provider unavailable | fail, `app_auth_unconfigured` | +| Mint 500/network | `markJobRetryable` + `message.retry({delaySeconds})` | +| Mint 401 | fail, `app_jwt_rejected` | + +### 6.4 `src/tests/github.test.js` + +- `Authorization` header: `'Bearer mock-token'` (replace the `'token mock-token'` assertion at line 63); drop `isApp` cases. + +### 6.5 `src/tests/auth.test.js` + +- Delete `isAuthorized` block; add `authorizeCommenter` 403 โ†’ `app_permission_missing` (non-retryable) test. + +--- + +## WS7 โ€” Configuration cleanup (PAT removal in wrangler) + +Both `src/zai-main-worker/wrangler.toml` and `src/zai-heavy-worker/wrangler.toml`: + +1. **Delete** the `[[secrets_store_secrets]]` block binding `GITHUB_TOKEN` (`ZAI_GITHUB_TOKEN`). +2. Update the store manifest comments: auth = GitHub App (`GITHUB_APP_ID`, `GITHUB_APP_PRIVATE_KEY`) + webhook secret + `ZAI_API_KEY`; drop "PAT for result comments" wording. +3. Keep `BOT_CACHE` (repo-config cache + installation-token cache), `BOT_DB`, `BOT_JOBS`, `BOT_ARTIFACTS` untouched. +4. `GITHUB_BOT_LOGIN` remains an optional `[vars]` (main worker only, already commented there). + +> The Secrets Store entry `ZAI_GITHUB_TOKEN` itself is **kept** until WS9 step 6 (rollback window), then deleted. + +--- + +## WS8 โ€” GitHub App permission fix (fixes #3) + +1. **GitHub App settings** (org App โ†’ Permissions & events โ†’ Repository permissions): + - Add **Collaborators โ†’ Read-only** (required by `GET /repos/{owner}/{repo}/collaborators/{user}` with an installation token โ€” the `/zai` authorization gate). + - Confirm existing set: Contents R, Pull requests RW, Issues RW, Pull request comments RW, Metadata R (auto). + - Permission changes require users to **accept the new permissions** on existing installations (org settings โ†’ GitHub Apps) before they take effect โ€” verify post-change. +2. **`IMPLEMENTATION_PLAN.md`** Task 1.1 permission table: add the Collaborators row with the reason above; adjust Task 2/3 snippets that show PAT fallback (mark them superseded by this plan). +3. **Verification** (staging, after WS6 code is deployed to staging): installation token + `GET /collaborators/{user}` for (a) a collaborator โ†’ 204, (b) an outsider โ†’ 404, (c) wrong-permission token โ†’ 403 with `app_permission_missing` in worker logs. + +--- + +## WS9 โ€” Verification, hygiene, rollout + +### 9.1 Local gates (definition of done for the branch) + +```bash +npm test # all green, coverage thresholds hold +npm run format:js # prettier --write (touched files) +npx vitest run src/tests/github-app-auth.test.js src/tests/index-fetch.test.js src/tests/queue.test.js +npm run deploy:main:dry-run && npm run deploy:heavy:dry-run +``` + +### 9.2 Rollout order (production) + +| Step | Action | Why | +| ---- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------- | +| 1 | `npx wrangler d1 migrations apply bot-db --remote` (main worker dir; applies 0004) | New code inserts `installation_id` on every job โ€” schema must exist first. Use migrations ledger, not `d1 execute --file` | +| 2 | Confirm GitHub App state (WS8) + webhook: App-owned webhook at `/github/webhook`, same `ZAI_GITHUB_WEBHOOK_KEY` secret, events `issue_comment`, `pull_request`, `pull_request_review_comment`; App installed on target org/repos | Without installation ids every command 503s | +| 3 | **Drain/quiet window**: confirm queue depth โ‰ˆ 0 (or pause consumer until in-flight pre-migration jobs finish) | Legacy `installation_id NULL` jobs will fail with `missing_installation_id` after cutover (bounded โ‰ค 3 attempts) | +| 4 | `make deploy` (heavy first, then main โ€” existing order) | Consumer ready before producer | +| 5 | Staging checks: `/zai help`, `/zai review`, `/zai describe` as collaborator (comments show `type: Bot`); non-collaborator gets denial comment; PR push produces `pr_context` job | Acceptance | +| 6 | After 24โ€“48h stable: delete `ZAI_GITHUB_TOKEN` from Secrets Store (`wrangler secrets:store delete`) | Closes the rollback window | + +### 9.3 Rollback + +Secrets entry is retained (until step 6), so rollback = `git revert` this branch โ†’ `make deploy` โ†’ re-add the `GITHUB_TOKEN` binding blocks (from this revert). Old code works regardless of whether webhooks arrive via the App or the legacy repo webhook, provided the webhook secret matches. + +### 9.4 Acceptance checklist + +- [x] `npm test` green; coverage thresholds hold (shared โ‰ฅ95/93, main โ‰ฅ90) โ€” 552/552 tests, exit 0 +- [x] No occurrence of `GITHUB_TOKEN` in `src/` outside tests' mock envs (grep gate โ†’ empty) +- [x] No `token` Authorization scheme anywhere (`grep -rn "token \${" src` โ†’ empty) +- [x] PR `synchronize` webhook mints **zero** tokens (test-asserted; plain-issue and ignored-action events too) +- [x] All `appAuthError` codes classified and test-covered +- [ ] Collaborators permission set on the App; staging 204/404 checks pass (manual โ€” WS8.1/8.3) +- [ ] Migration applied before deploy; queue drained at cutover (manual โ€” WS9.2 steps 1โ€“4) +- [ ] `ZAI_GITHUB_TOKEN` deleted after stability window (manual โ€” WS9.2 step 6) + +--- + +## Suggested commit sequence + +1. `refactor(auth): app-only token provider with secret resolution + error classification` (WS1โ€“WS3 + their tests) +2. `refactor(main-worker): lazy App-only GitHub client; de-PAT comment refresh & bot login` (WS4 + index-fetch tests) +3. `refactor(heavy-worker): App-only queue auth with retryable/permanent error mapping` (WS5 + queue tests) +4. `chore(config): drop GITHUB_TOKEN bindings; document App-only secrets` (WS7) +5. `docs: add Collaborators permission; mark PAT fallback superseded` (WS8) diff --git a/src/shared/auth.js b/src/shared/auth.js index 50d2f090..10c9b91d 100644 --- a/src/shared/auth.js +++ b/src/shared/auth.js @@ -5,11 +5,12 @@ * the earlier GitHub Actions bot, which authorized any identifiable user). * Centralizing it here lets both workers apply the same policy and makes a * future policy change a one-file edit. + * + * Auth context: all clients authenticate as the GitHub App installation + * (PAT support removed), so the collaborator check requires the App to hold + * the \"Collaborators: read-only\" repository permission. */ -import { GitHubClient } from './github.js'; -import { resolveSecretValue } from './secrets.js'; - /** * Authorizes a commenter as a repository collaborator. * @param {import('./github.js').GitHubClient} github @@ -25,15 +26,18 @@ export async function authorizeCommenter(github, owner, repo, username) { return true; } catch (error) { if (error.status === 404) return false; + if (error.status === 403) { + // An installation token without the \"Collaborators: read-only\" + // permission gets 403 from GitHub. Surface it as a loud config error โ€” + // misclassifying it as \"not a collaborator\" would silently lock out + // every legitimate /zai command. + const error403 = new Error( + 'app_permission_missing: collaborator check requires Collaborators read-only', + ); + error403.code = 'app_permission_missing'; + error403.retryable = false; + throw error403; + } throw error; } } - -/** - * Convenience factory: build a GitHubClient + run the auth check in one call. - * Useful from the heavy worker, which rebuilds a client per invocation. - */ -export async function isAuthorized(env, owner, repo, username) { - const github = new GitHubClient(await resolveSecretValue(env.GITHUB_TOKEN)); - return authorizeCommenter(github, owner, repo, username); -} diff --git a/src/shared/github-app-auth.js b/src/shared/github-app-auth.js new file mode 100644 index 00000000..8b82e1bf --- /dev/null +++ b/src/shared/github-app-auth.js @@ -0,0 +1,234 @@ +/** + * GitHub App Authentication for Cloudflare Workers. + * Generates JWT and fetches Installation Access Tokens. + * + * This is the ONLY authentication path in the codebase (PAT support removed). + * Secrets must be resolved through `resolveSecretValue` โ€” Secrets Store + * bindings can surface as string | {get()} | Promise, and a raw binding + * stringifies to "[object Object]", silently breaking every token mint. + */ + +import { resolveSecretValue } from './secrets.js'; + +const GITHUB_API_BASE = 'https://api.github.com'; + +/** + * Creates a classified GitHub App authentication error. + * `retryable: false` marks config/permanent failures (queue: fail the job, + * webhook: 503 without pointlessly burning attempts). + * @param {string} code - Stable error code (e.g. 'app_jwt_rejected') + * @param {string} message - Short, log-safe message (no raw provider bodies) + * @param {Object} [opts] + * @param {boolean} [opts.retryable] - default true + * @param {number} [opts.status] - HTTP status from GitHub, when applicable + * @returns {Error} + */ +function appAuthError(code, message, { retryable = true, status } = {}) { + const error = new Error(message); + error.code = code; + error.retryable = retryable; + if (status !== undefined) error.status = status; + return error; +} + +/** + * Encodes bytes to Base64URL format (for JWT parts). + * @param {Uint8Array} bytes + * @returns {string} Base64URL encoded string (no padding) + */ +function encodeBase64UrlBytes(bytes) { + let binary = ''; + for (const byte of bytes) binary += String.fromCharCode(byte); + return btoa(binary).replace(/\+/g, '-').replace(/\//g, '_').replace(/=+$/, ''); +} + +/** + * Generates a JWT token signed with the app's private key. + * @param {string} appId - GitHub App ID + * @param {string} privateKey - PEM-encoded private key + * @returns {Promise} JWT token + */ +export async function generateAppJwt(appId, privateKey) { + const now = Math.floor(Date.now() / 1000); + const payload = { + iat: now, + exp: now + 540, // 9 minutes (GitHub recommends max 10 min) + iss: appId, + }; + + const header = { alg: 'RS256', typ: 'JWT' }; + + const encoder = new TextEncoder(); + const encodedHeader = encodeBase64UrlBytes(encoder.encode(JSON.stringify(header))); + const encodedPayload = encodeBase64UrlBytes(encoder.encode(JSON.stringify(payload))); + + // Normalize private key (remove headers, whitespace) + const normalizedKey = privateKey + .replace(/-----BEGIN PRIVATE KEY-----/g, '') + .replace(/-----END PRIVATE KEY-----/g, '') + .replace(/\s+/g, ''); + + const binaryKey = Uint8Array.from(atob(normalizedKey), (c) => c.charCodeAt(0)); + + const cryptoKey = await crypto.subtle.importKey( + 'pkcs8', + binaryKey, + { name: 'RSASSA-PKCS1-v1_5', hash: 'SHA-256' }, + false, + ['sign'], + ); + + const signature = await crypto.subtle.sign( + 'RSASSA-PKCS1-v1_5', + cryptoKey, + encoder.encode(`${encodedHeader}.${encodedPayload}`), + ); + + const encodedSignature = encodeBase64UrlBytes(new Uint8Array(signature)); + + return `${encodedHeader}.${encodedPayload}.${encodedSignature}`; +} + +/** + * Maps an HTTP status from the installation-token endpoint to a classified + * appAuthError. Only status + code are kept โ€” raw response bodies must not + * leak into logs or GitHub comments (SECURITY.md). + */ +function tokenEndpointError(status) { + if (status === 401) { + return appAuthError('app_jwt_rejected', 'GitHub rejected the app JWT (bad key or app id)', { + retryable: false, + status, + }); + } + if (status === 403) { + return appAuthError('app_suspended', 'GitHub App is suspended or blocked', { + retryable: false, + status, + }); + } + if (status === 404) { + return appAuthError('installation_not_found', 'Installation was removed or inaccessible', { + retryable: false, + status, + }); + } + // 5xx / 429 / anything else unexpected: worth another attempt later. + return appAuthError('app_token_fetch_failed', `GitHub token endpoint returned ${status}`, { + retryable: true, + status, + }); +} + +/** + * Fetches an installation access token from GitHub. + * @param {string} jwt - App JWT token + * @param {string|number} installationId - Installation ID + * @returns {Promise} Installation access token + */ +export async function fetchInstallationToken(jwt, installationId) { + let response; + try { + response = await fetch(`${GITHUB_API_BASE}/app/installations/${installationId}/access_tokens`, { + method: 'POST', + headers: { + Authorization: `Bearer ${jwt}`, + Accept: 'application/vnd.github+json', + 'X-GitHub-Api-Version': '2022-11-28', + 'User-Agent': 'zai-code-bot-workers', + }, + }); + } catch (error) { + // Network-level failure (offline, DNS, timeout): transient. + throw appAuthError('app_token_fetch_failed', `Token endpoint unreachable: ${error.message}`); + } + + if (!response.ok) { + // Drain the body so the connection can be reused, then classify. + await response.text().catch(() => {}); + throw tokenEndpointError(response.status); + } + + const data = await response.json(); + if (typeof data?.token !== 'string' || data.token === '') { + throw appAuthError('app_token_fetch_failed', 'Token endpoint returned no token'); + } + return data.token; +} + +/** + * Caches installation tokens to avoid generating JWT for every request. + * Uses KV namespace for caching (optional optimization). Cache errors are + * swallowed: a cache miss only costs an extra mint, never a failure. + */ +export class AppTokenCache { + constructor(kvNamespace, ttl = 300) { + this.kv = kvNamespace; + this.ttl = ttl; // 5 minutes + } + + async get(installationId) { + if (!this.kv) return null; + try { + const { value } = await this.kv.get(`installation_token:${installationId}`, { + type: 'json', + }); + return value; + } catch { + return null; + } + } + + async set(installationId, token) { + if (!this.kv) return; + try { + await this.kv.put(`installation_token:${installationId}`, JSON.stringify({ value: token }), { + expirationTtl: this.ttl, + }); + } catch { + // Ignore cache errors + } + } +} + +/** + * Creates a token provider that handles JWT generation and caching. + * Resolves both Secrets Store bindings through `resolveSecretValue` (the + * documented happy path is a plain string, but the same store also surfaces + * `{get()}` and Promise shapes depending on the wrangler/workerd version). + * + * @param {Object} env - Environment bindings (GITHUB_APP_ID, GITHUB_APP_PRIVATE_KEY, BOT_CACHE) + * @returns {Promise<{available: boolean, getInstallationToken: function}>} + */ +export async function createTokenProvider(env) { + const appId = await resolveSecretValue(env.GITHUB_APP_ID); + const privateKey = await resolveSecretValue(env.GITHUB_APP_PRIVATE_KEY); + const cache = env.BOT_CACHE ? new AppTokenCache(env.BOT_CACHE) : null; + + return { + available: Boolean(appId && privateKey), + async getInstallationToken(installationId) { + if (!appId || !privateKey) { + throw appAuthError( + 'app_auth_unconfigured', + 'GITHUB_APP_ID / GITHUB_APP_PRIVATE_KEY not configured', + { retryable: false }, + ); + } + if (!installationId) { + throw appAuthError('missing_installation_id', 'webhook/job carries no installation id', { + retryable: false, + }); + } + + const cached = cache ? await cache.get(installationId) : null; + if (cached) return cached; + + const jwt = await generateAppJwt(appId, privateKey); + const token = await fetchInstallationToken(jwt, installationId); + + if (cache) await cache.set(installationId, token); + return token; + }, + }; +} diff --git a/src/shared/github.js b/src/shared/github.js index d5d77c48..a87f077c 100644 --- a/src/shared/github.js +++ b/src/shared/github.js @@ -3,13 +3,15 @@ * * NOTE: webhook signature verification was extracted to `./crypto.js` * (Web Crypto API). This module is pure REST I/O and is shared by both workers. + * Authentication is GitHub App only: every token in the system is an + * installation access token, sent as a Bearer credential. */ const GITHUB_API_BASE = 'https://api.github.com'; export class GitHubClient { /** - * @param {string} token - GitHub Personal Access Token + * @param {string} token - GitHub App installation access token * @param {Object} [opts] * @param {string} [opts.userAgent='zai-code-bot-workers'] */ @@ -33,7 +35,7 @@ export class GitHubClient { const options = { method, headers: { - Authorization: `token ${this.token}`, + Authorization: `Bearer ${this.token}`, Accept: 'application/vnd.github+json', 'X-GitHub-Api-Version': '2022-11-28', 'User-Agent': this.userAgent, diff --git a/src/shared/secrets.js b/src/shared/secrets.js index 49bab9f6..2d18f166 100644 --- a/src/shared/secrets.js +++ b/src/shared/secrets.js @@ -22,8 +22,8 @@ * against the same Secrets Store (629e5ddโ€ฆ). * * USAGE: resolve at the edge, pass a plain string down. - * const token = await resolveSecretValue(env.GITHUB_TOKEN); - * new GitHubClient(token); + * const key = await resolveSecretValue(env.GITHUB_APP_PRIVATE_KEY); + * generateAppJwt(appId, key); */ /** diff --git a/src/shared/storage/deliveries.js b/src/shared/storage/deliveries.js index 2aef3e15..843d0e44 100644 --- a/src/shared/storage/deliveries.js +++ b/src/shared/storage/deliveries.js @@ -6,6 +6,7 @@ const JOB_BASE = ` j.head_sha, p.base_sha, j.status, j.attempt_count, j.available_at, j.claimed_at, j.lease_expires_at, j.completed_at, j.last_error_code, j.last_failure_at, j.config_version, + j.installation_id, r.owner AS repository_owner, r.name AS repository_name, r.full_name AS repository_full_name, p.title, p.author_login, p.state, p.closed_by FROM jobs j @@ -49,6 +50,7 @@ async function createPrJob(db, event, kind, { ownsDelivery }, now = new Date().t const jobId = crypto.randomUUID(); const repositoryId = Number(event.repositoryId); const prNumber = Number(event.prNumber); + const installationId = event.installationId || null; const statements = [ prepare( db, @@ -107,15 +109,16 @@ async function createPrJob(db, event, kind, { ownsDelivery }, now = new Date().t prepare( db, `INSERT INTO jobs - (job_id, delivery_id, kind, repository_id, pr_number, head_sha, status, + (job_id, delivery_id, kind, repository_id, pr_number, head_sha, installation_id, status, attempt_count, available_at, config_version, created_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, 'queued', 0, ?, 1, ?, ?)`, + VALUES (?, ?, ?, ?, ?, ?, ?, 'queued', 0, ?, 1, ?, ?)`, jobId, event.deliveryId, kind, repositoryId, prNumber, event.headSha, + installationId, now, now, now, @@ -152,8 +155,14 @@ async function createPrJob(db, event, kind, { ownsDelivery }, now = new Date().t * Owns the delivery row and is idempotent on (delivery_id, 'pr_context'). * Only created for headSha-producing actions. */ -export function createPrContextJob(db, event, now) { - return createPrJob(db, event, PR_CONTEXT_JOB_KIND, { ownsDelivery: true }, now); +export function createPrContextJob(db, event, installationId, now) { + return createPrJob( + db, + { ...event, installationId }, + PR_CONTEXT_JOB_KIND, + { ownsDelivery: true }, + now, + ); } /** @@ -182,6 +191,7 @@ export function createPrSummaryJob(db, contextJob, now) { title: contextJob.title, authorLogin: contextJob.author_login, state: contextJob.state, + installationId: contextJob.installation_id, }, PR_SUMMARY_JOB_KIND, { ownsDelivery: false }, diff --git a/src/tests/auth.test.js b/src/tests/auth.test.js index 6d6a5c7a..085b42ee 100644 --- a/src/tests/auth.test.js +++ b/src/tests/auth.test.js @@ -1,5 +1,5 @@ -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { authorizeCommenter, isAuthorized } from '../shared/auth.js'; +import { describe, expect, it, vi } from 'vitest'; +import { authorizeCommenter } from '../shared/auth.js'; describe('shared/auth', () => { describe('authorizeCommenter', () => { @@ -15,7 +15,23 @@ describe('shared/auth', () => { expect(await authorizeCommenter(github, 'o', 'r', 'u')).toBe(false); }); - it('rethrows a non-404 status error (e.g. 500)', async () => { + it('maps a 403 to a non-retryable app_permission_missing error', async () => { + // An installation token without "Collaborators: read-only" gets 403 โ€” + // this must surface as a loud config error, never as "not authorized". + const forbidden = Object.assign(new Error('Resource not accessible by integration'), { + status: 403, + }); + const github = { request: vi.fn().mockRejectedValue(forbidden) }; + + const error = await authorizeCommenter(github, 'o', 'r', 'u').catch((e) => e); + + expect(error).toBeInstanceOf(Error); + expect(error.code).toBe('app_permission_missing'); + expect(error.retryable).toBe(false); + expect(error.message).toContain('Collaborators'); + }); + + it('rethrows a non-404/403 status error (e.g. 500)', async () => { const serverError = Object.assign(new Error('boom'), { status: 500 }); const github = { request: vi.fn().mockRejectedValue(serverError) }; await expect(authorizeCommenter(github, 'o', 'r', 'u')).rejects.toBe(serverError); @@ -26,32 +42,4 @@ describe('shared/auth', () => { await expect(authorizeCommenter(github, 'o', 'r', 'u')).rejects.toThrow('network down'); }); }); - - describe('isAuthorized', () => { - let fetchSpy; - beforeEach(() => { - fetchSpy = vi.spyOn(globalThis, 'fetch'); - }); - afterEach(() => vi.restoreAllMocks()); - - it('builds a client from env.GITHUB_TOKEN and authorizes a collaborator', async () => { - fetchSpy.mockResolvedValueOnce(new Response(null, { status: 204 })); - expect(await isAuthorized({ GITHUB_TOKEN: 'tok' }, 'o', 'r', 'u')).toBe(true); - const [url, opts] = fetchSpy.mock.calls[0]; - expect(url).toBe('https://api.github.com/repos/o/r/collaborators/u'); - expect(opts.headers.Authorization).toBe('token tok'); - }); - - it('returns false for a non-collaborator', async () => { - fetchSpy.mockResolvedValueOnce(new Response('nf', { status: 404 })); - expect(await isAuthorized({ GITHUB_TOKEN: 'tok' }, 'o', 'r', 'u')).toBe(false); - }); - - it('resolves a Secrets-Store binding shape (object.get()) for the token', async () => { - fetchSpy.mockResolvedValueOnce(new Response(null, { status: 204 })); - const env = { GITHUB_TOKEN: { get: async () => 'tok-from-binding' } }; - expect(await isAuthorized(env, 'o', 'r', 'u')).toBe(true); - expect(fetchSpy.mock.calls[0][1].headers.Authorization).toBe('token tok-from-binding'); - }); - }); }); diff --git a/src/tests/github-app-auth.test.js b/src/tests/github-app-auth.test.js new file mode 100644 index 00000000..934a32e0 --- /dev/null +++ b/src/tests/github-app-auth.test.js @@ -0,0 +1,363 @@ +/** + * Tests for the GitHub App authentication module. + * + * JWT tests use a REAL RSA keypair generated per run (node:crypto) and verify + * the RS256 signature with Web Crypto โ€” no fixtures, no fabricated base64. + * The token-endpoint and provider tests mock global fetch. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { generateKeyPairSync } from 'node:crypto'; +import { + AppTokenCache, + createTokenProvider, + fetchInstallationToken, + generateAppJwt, +} from '../shared/github-app-auth.js'; + +// --- real keypair ----------------------------------------------------------- + +const { publicKey, privateKey } = generateKeyPairSync('rsa', { modulusLength: 2048 }); +const TEST_PRIVATE_KEY = privateKey.export({ type: 'pkcs8', format: 'pem' }).toString(); +const PUBLIC_KEY_SPKI = publicKey.export({ type: 'spki', format: 'der' }); + +const TEST_APP_ID = '123456'; +const TEST_INSTALLATION_ID = '789012'; + +/** Imports the test public key for crypto.subtle.verify. */ +async function importVerifyKey() { + return crypto.subtle.importKey( + 'spki', + PUBLIC_KEY_SPKI, + { name: 'RSASSA-PKCS1-v1_5', hash: 'SHA-256' }, + false, + ['verify'], + ); +} + +/** Decodes a base64url JWT part to a UTF-8 string. */ +function decodePart(part) { + return Buffer.from(part, 'base64url').toString('utf8'); +} + +/** Builds a mock fetch Response carrying JSON. */ +function jsonResponse(status, body) { + return new Response(status === 204 ? null : JSON.stringify(body), { + status, + headers: { 'content-type': 'application/json' }, + }); +} + +// --------------------------------------------------------------------------- +// generateAppJwt +// --------------------------------------------------------------------------- + +describe('generateAppJwt', () => { + it('generates a JWT with three base64url parts', async () => { + const jwt = await generateAppJwt(TEST_APP_ID, TEST_PRIVATE_KEY); + + expect(typeof jwt).toBe('string'); + const parts = jwt.split('.'); + expect(parts).toHaveLength(3); + for (const part of parts) { + expect(part).toMatch(/^[A-Za-z0-9_-]+$/); + } + }); + + it('signs a verifiable RS256 signature over header.payload', async () => { + const jwt = await generateAppJwt(TEST_APP_ID, TEST_PRIVATE_KEY); + const [header, payload, signature] = jwt.split('.'); + + const verifyKey = await importVerifyKey(); + const valid = await crypto.subtle.verify( + 'RSASSA-PKCS1-v1_5', + verifyKey, + Uint8Array.from(Buffer.from(signature, 'base64url')), + new TextEncoder().encode(`${header}.${payload}`), + ); + expect(valid).toBe(true); + }); + + it('uses the RS256 header and the app id as issuer with a 9-minute lifetime', async () => { + const before = Math.floor(Date.now() / 1000); + const jwt = await generateAppJwt(TEST_APP_ID, TEST_PRIVATE_KEY); + const after = Math.floor(Date.now() / 1000); + + const [header, payload] = jwt.split('.').map(decodePart); + expect(JSON.parse(header)).toEqual({ alg: 'RS256', typ: 'JWT' }); + + const claims = JSON.parse(payload); + expect(claims.iss).toBe(TEST_APP_ID); + expect(claims.exp - claims.iat).toBe(540); + expect(claims.iat).toBeGreaterThanOrEqual(before); + expect(claims.iat).toBeLessThanOrEqual(after); + }); + + it('tolerates PEM keys with CRLF line endings, blank lines, and surrounding whitespace', async () => { + const mangled = `\r\n\r\n ${TEST_PRIVATE_KEY.replace(/\n/g, '\r\n').replace( + /-----END PRIVATE KEY-----/, + '\n\n-----END PRIVATE KEY----- \n', + )}`; + + const jwt = await generateAppJwt(TEST_APP_ID, mangled); + const [header, payload, signature] = jwt.split('.'); + + const verifyKey = await importVerifyKey(); + const valid = await crypto.subtle.verify( + 'RSASSA-PKCS1-v1_5', + verifyKey, + Uint8Array.from(Buffer.from(signature, 'base64url')), + new TextEncoder().encode(`${header}.${payload}`), + ); + expect(valid).toBe(true); + }); +}); + +// --------------------------------------------------------------------------- +// fetchInstallationToken +// --------------------------------------------------------------------------- + +describe('fetchInstallationToken', () => { + let fetchSpy; + beforeEach(() => { + fetchSpy = vi.spyOn(globalThis, 'fetch'); + }); + afterEach(() => vi.restoreAllMocks()); + + it('POSTs the token endpoint with JWT bearer auth and returns the token', async () => { + fetchSpy.mockResolvedValueOnce(jsonResponse(201, { token: 'ghs_1234567890abcdef' })); + + const token = await fetchInstallationToken('test-jwt', TEST_INSTALLATION_ID); + + expect(token).toBe('ghs_1234567890abcdef'); + const [url, opts] = fetchSpy.mock.calls[0]; + expect(url).toBe('https://api.github.com/app/installations/789012/access_tokens'); + expect(opts.method).toBe('POST'); + expect(opts.headers.Authorization).toBe('Bearer test-jwt'); + expect(opts.headers.Accept).toBe('application/vnd.github+json'); + expect(opts.headers['X-GitHub-Api-Version']).toBe('2022-11-28'); + expect(opts.headers['User-Agent']).toBe('zai-code-bot-workers'); + }); + + it.each([ + [401, 'app_jwt_rejected'], + [403, 'app_suspended'], + [404, 'installation_not_found'], + ])('maps HTTP %i to a non-retryable %s error', async (status, code) => { + fetchSpy.mockResolvedValueOnce(jsonResponse(status, { message: 'nope' })); + + const error = await fetchInstallationToken('test-jwt', TEST_INSTALLATION_ID).catch((e) => e); + + expect(error).toBeInstanceOf(Error); + expect(error.code).toBe(code); + expect(error.retryable).toBe(false); + expect(error.status).toBe(status); + // Raw provider bodies must not ride along on the error. + expect(error.body).toBeUndefined(); + expect(error.message).not.toContain('nope'); + }); + + it.each([ + [429, 'app_token_fetch_failed'], + [500, 'app_token_fetch_failed'], + [503, 'app_token_fetch_failed'], + ])('maps HTTP %i to a retryable %s error', async (status, code) => { + fetchSpy.mockResolvedValueOnce(new Response('boom', { status })); + + const error = await fetchInstallationToken('test-jwt', TEST_INSTALLATION_ID).catch((e) => e); + + expect(error.code).toBe(code); + expect(error.retryable).toBe(true); + }); + + it('maps a network failure to a retryable app_token_fetch_failed error', async () => { + fetchSpy.mockRejectedValueOnce(new TypeError('fetch failed')); + + const error = await fetchInstallationToken('test-jwt', TEST_INSTALLATION_ID).catch((e) => e); + + expect(error.code).toBe('app_token_fetch_failed'); + expect(error.retryable).toBe(true); + }); + + it('rejects a 2xx response that carries no token', async () => { + fetchSpy.mockResolvedValueOnce(jsonResponse(201, {})); + + const error = await fetchInstallationToken('test-jwt', TEST_INSTALLATION_ID).catch((e) => e); + + expect(error.code).toBe('app_token_fetch_failed'); + expect(error.retryable).toBe(true); + }); +}); + +// --------------------------------------------------------------------------- +// AppTokenCache +// --------------------------------------------------------------------------- + +describe('AppTokenCache', () => { + let mockKv; + let cache; + + beforeEach(() => { + mockKv = { + get: vi.fn(), + put: vi.fn(), + }; + cache = new AppTokenCache(mockKv, 300); + }); + + it('returns null when the cache is empty', async () => { + mockKv.get.mockResolvedValueOnce({ value: null }); + + expect(await cache.get(TEST_INSTALLATION_ID)).toBeNull(); + }); + + it('returns the cached token when available', async () => { + mockKv.get.mockResolvedValueOnce({ value: 'ghs_cached_token' }); + + expect(await cache.get(TEST_INSTALLATION_ID)).toBe('ghs_cached_token'); + }); + + it('swallows KV read errors as cache misses', async () => { + mockKv.get.mockRejectedValueOnce(new Error('KV error')); + + expect(await cache.get(TEST_INSTALLATION_ID)).toBeNull(); + }); + + it('writes the token with the configured TTL', async () => { + await cache.set(TEST_INSTALLATION_ID, 'ghs_new_token'); + + expect(mockKv.put).toHaveBeenCalledWith( + `installation_token:${TEST_INSTALLATION_ID}`, + JSON.stringify({ value: 'ghs_new_token' }), + { expirationTtl: 300 }, + ); + }); + + it('swallows KV write errors', async () => { + mockKv.put.mockRejectedValueOnce(new Error('KV error')); + + await expect(cache.set(TEST_INSTALLATION_ID, 'ghs_new_token')).resolves.toBeUndefined(); + }); + + it('degrades to no-ops when constructed without a namespace', async () => { + const bare = new AppTokenCache(null); + await expect(bare.get(TEST_INSTALLATION_ID)).resolves.toBeNull(); + await expect(bare.set(TEST_INSTALLATION_ID, 'x')).resolves.toBeUndefined(); + }); +}); + +// --------------------------------------------------------------------------- +// createTokenProvider +// --------------------------------------------------------------------------- + +describe('createTokenProvider', () => { + let fetchSpy; + let mockKv; + + beforeEach(() => { + fetchSpy = vi.spyOn(globalThis, 'fetch'); + mockKv = { get: vi.fn().mockResolvedValue({ value: null }), put: vi.fn() }; + }); + afterEach(() => vi.restoreAllMocks()); + + it.each([ + ['plain strings', { GITHUB_APP_ID: TEST_APP_ID, GITHUB_APP_PRIVATE_KEY: TEST_PRIVATE_KEY }], + [ + 'object {get()} bindings', + { + GITHUB_APP_ID: { get: async () => TEST_APP_ID }, + GITHUB_APP_PRIVATE_KEY: { get: async () => TEST_PRIVATE_KEY }, + }, + ], + [ + 'Promise bindings', + { + GITHUB_APP_ID: Promise.resolve(TEST_APP_ID), + GITHUB_APP_PRIVATE_KEY: Promise.resolve(TEST_PRIVATE_KEY), + }, + ], + ])('resolves %s and reports the provider as available', async (_label, env) => { + const provider = await createTokenProvider(env); + expect(provider.available).toBe(true); + }); + + it.each([ + ['missing app id', { GITHUB_APP_PRIVATE_KEY: TEST_PRIVATE_KEY }], + ['missing private key', { GITHUB_APP_ID: TEST_APP_ID }], + ['blank app id', { GITHUB_APP_ID: ' ', GITHUB_APP_PRIVATE_KEY: TEST_PRIVATE_KEY }], + ['empty bindings', {}], + ])('reports unavailable for %s', async (_label, env) => { + const provider = await createTokenProvider(env); + expect(provider.available).toBe(false); + }); + + it('mints and caches a token on cache miss', async () => { + fetchSpy.mockResolvedValueOnce(jsonResponse(201, { token: 'ghs_fresh' })); + + const provider = await createTokenProvider({ + GITHUB_APP_ID: TEST_APP_ID, + GITHUB_APP_PRIVATE_KEY: TEST_PRIVATE_KEY, + BOT_CACHE: mockKv, + }); + const token = await provider.getInstallationToken(TEST_INSTALLATION_ID); + + expect(token).toBe('ghs_fresh'); + expect(fetchSpy).toHaveBeenCalledOnce(); + expect(mockKv.put).toHaveBeenCalledWith( + `installation_token:${TEST_INSTALLATION_ID}`, + JSON.stringify({ value: 'ghs_fresh' }), + expect.objectContaining({ expirationTtl: 300 }), + ); + }); + + it('serves a cache hit without minting', async () => { + mockKv.get.mockResolvedValueOnce({ value: 'ghs_cached' }); + + const provider = await createTokenProvider({ + GITHUB_APP_ID: TEST_APP_ID, + GITHUB_APP_PRIVATE_KEY: TEST_PRIVATE_KEY, + BOT_CACHE: mockKv, + }); + const token = await provider.getInstallationToken(TEST_INSTALLATION_ID); + + expect(token).toBe('ghs_cached'); + expect(fetchSpy).not.toHaveBeenCalled(); + expect(mockKv.put).not.toHaveBeenCalled(); + }); + + it('propagates a classified mint failure (404 โ†’ installation_not_found)', async () => { + fetchSpy.mockResolvedValueOnce(jsonResponse(404, { message: 'gone' })); + + const provider = await createTokenProvider({ + GITHUB_APP_ID: TEST_APP_ID, + GITHUB_APP_PRIVATE_KEY: TEST_PRIVATE_KEY, + BOT_CACHE: mockKv, + }); + const error = await provider.getInstallationToken(TEST_INSTALLATION_ID).catch((e) => e); + + expect(error.code).toBe('installation_not_found'); + expect(error.retryable).toBe(false); + expect(mockKv.put).not.toHaveBeenCalled(); + }); + + it('throws non-retryable app_auth_unconfigured on mint when secrets are missing', async () => { + const provider = await createTokenProvider({}); + const error = await provider.getInstallationToken(TEST_INSTALLATION_ID).catch((e) => e); + + expect(error.code).toBe('app_auth_unconfigured'); + expect(error.retryable).toBe(false); + expect(fetchSpy).not.toHaveBeenCalled(); + }); + + it('throws non-retryable missing_installation_id when no installation is given', async () => { + const provider = await createTokenProvider({ + GITHUB_APP_ID: TEST_APP_ID, + GITHUB_APP_PRIVATE_KEY: TEST_PRIVATE_KEY, + }); + const error = await provider.getInstallationToken(undefined).catch((e) => e); + + expect(error.code).toBe('missing_installation_id'); + expect(error.retryable).toBe(false); + expect(fetchSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/src/tests/github.test.js b/src/tests/github.test.js index 828d3053..25ce53c7 100644 --- a/src/tests/github.test.js +++ b/src/tests/github.test.js @@ -60,7 +60,7 @@ describe('shared/github (GitHubClient)', () => { fetchSpy.mockResolvedValueOnce(new Response(null, { status: 204 })); await client.request('GET', '/x'); const headers = fetchSpy.mock.calls[0][1].headers; - expect(headers.Authorization).toBe('token mock-token'); + expect(headers.Authorization).toBe('Bearer mock-token'); expect(headers.Accept).toBe('application/vnd.github+json'); expect(headers['X-GitHub-Api-Version']).toBe('2022-11-28'); expect(headers['User-Agent']).toBe('zai-code-bot-workers'); diff --git a/src/tests/index-fetch.test.js b/src/tests/index-fetch.test.js index 05330683..30a47b4e 100644 --- a/src/tests/index-fetch.test.js +++ b/src/tests/index-fetch.test.js @@ -7,13 +7,15 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; // (shared/commands.js), secret resolution (shared/secrets.js), and the pure // event predicates (pr-events.js / comment-events.js). // -// What is MOCKED: GitHubClient, authorizeCommenter, D1 storage (deliveries), +// What is MOCKED: GitHubClient, authorizeCommenter, the App token provider +// (github-app-auth.js โ€” provider + token mint), D1 storage (deliveries), // the queue producer (job-enqueuer), and the R2 slice refreshers โ€” so these // tests pin the routing/gating contract of index.js without network or // bindings. vi.mock('../shared/github.js', () => ({ GitHubClient: vi.fn() })); vi.mock('../shared/auth.js', () => ({ authorizeCommenter: vi.fn() })); +vi.mock('../shared/github-app-auth.js', () => ({ createTokenProvider: vi.fn() })); vi.mock('../shared/storage/deliveries.js', () => ({ createPrContextJob: vi.fn(), createCommandJob: vi.fn(), @@ -29,6 +31,7 @@ vi.mock('../shared/pr-comments.js', () => ({ refreshCommentsSlice: vi.fn() })); vi.mock('../shared/pr-description.js', () => ({ refreshDescriptionSlice: vi.fn() })); import { GitHubClient } from '../shared/github.js'; +import { createTokenProvider } from '../shared/github-app-auth.js'; import { authorizeCommenter } from '../shared/auth.js'; import { createPrContextJob, createCommandJob } from '../shared/storage/deliveries.js'; import { @@ -54,7 +57,8 @@ const SECRET = 'whsec-test'; function makeEnv(overrides = {}) { return { GITHUB_WEBHOOK_SECRET: SECRET, - GITHUB_TOKEN: 'gh-token', + GITHUB_APP_ID: '123456', + GITHUB_APP_PRIVATE_KEY: 'test-app-key', BOT_DB: { __db: true }, BOT_JOBS: { send: vi.fn() }, BOT_ARTIFACTS: { delete: vi.fn(), put: vi.fn(), get: vi.fn() }, @@ -97,6 +101,7 @@ async function signedRequest( function prCommentPayload({ body = '/zai review', action = 'created', onPr = true } = {}) { return { action, + installation: { id: 456 }, repository: { id: 10, name: 'repo', full_name: 'owner/repo', owner: { login: 'owner' } }, issue: { number: 7, @@ -111,6 +116,7 @@ function prCommentPayload({ body = '/zai review', action = 'created', onPr = tru function prEventPayload(action) { return { action, + installation: { id: 456 }, repository: { id: 10, name: 'repo', @@ -152,6 +158,7 @@ function flushWaitUntil(ctx) { let github; let ctx; +let mintToken; beforeEach(() => { vi.clearAllMocks(); @@ -172,6 +179,9 @@ beforeEach(() => { } }, ); + // App token provider: available by default, minting a fixed token. + mintToken = vi.fn().mockResolvedValue('ghs_test'); + createTokenProvider.mockResolvedValue({ available: true, getInstallationToken: mintToken }); authorizeCommenter.mockResolvedValue(true); enqueueJob.mockResolvedValue(true); createCommandJob.mockResolvedValue({ job: { job_id: 'cmd-1' }, created: true }); @@ -356,8 +366,9 @@ describe('command comments', () => { durable: true, }); - // Auth gate ran with a real client for the right user/repo. - expect(GitHubClient).toHaveBeenCalledWith('gh-token'); + // Auth gate ran with an App-authenticated client for the right user/repo. + expect(GitHubClient).toHaveBeenCalledWith('ghs_test'); + expect(mintToken).toHaveBeenCalledWith(456); expect(authorizeCommenter).toHaveBeenCalledWith(github, 'owner', 'repo', 'alice'); // The durable event matches the PR-event job shape. @@ -375,6 +386,7 @@ describe('command comments', () => { title: 'Cloudflare migration', authorLogin: 'pr-author', state: 'open', + installationId: 456, }), 'review', ); @@ -494,17 +506,36 @@ describe('inline commands', () => { expect(github.postComment).toHaveBeenCalledWith('owner', 'repo', 7, formatHelp()); }); - it('updates a PAT-owned help comment via the resolved bot login', async () => { - github.getAuthenticatedUser.mockResolvedValue({ login: 'pat-bot' }); + it('updates a PAT-era help comment only via the configured GITHUB_BOT_LOGIN var', async () => { github.getIssueComments.mockResolvedValue([ { id: 88, body: `help ${HELP_MARKER}`, user: { login: 'pat-bot', type: 'User' } }, ]); - const res = await signedRequest({ body: prCommentPayload({ body: '/zai help' }) }); + const res = await signedRequest( + { body: prCommentPayload({ body: '/zai help' }) }, + makeEnv({ GITHUB_BOT_LOGIN: 'pat-bot' }), + ); expect(res.status).toBe(200); expect(github.updateComment).toHaveBeenCalledWith('owner', 'repo', 88, formatHelp()); expect(github.postComment).not.toHaveBeenCalled(); + // App-only auth: the worker never probes GET /user (installation tokens + // cannot call it); the login comes exclusively from the env var. + expect(github.getAuthenticatedUser).not.toHaveBeenCalled(); + }); + + it('posts a fresh help comment when a PAT-era help comment exists but GITHUB_BOT_LOGIN is unset', async () => { + github.getIssueComments.mockResolvedValue([ + { id: 88, body: `help ${HELP_MARKER}`, user: { login: 'pat-bot', type: 'User' } }, + ]); + + const res = await signedRequest({ body: prCommentPayload({ body: '/zai help' }) }); + + expect(res.status).toBe(200); + // Without the var, a type 'User' comment is not recognized as bot-owned; + // a new help comment is posted once instead of rewriting it. + expect(github.updateComment).not.toHaveBeenCalled(); + expect(github.postComment).toHaveBeenCalledWith('owner', 'repo', 7, formatHelp()); }); it('prefers GITHUB_BOT_LOGIN over the API lookup', async () => { @@ -575,23 +606,27 @@ describe('pull_request events', () => { duplicate: false, }); - expect(createPrContextJob).toHaveBeenCalledWith(env.BOT_DB, { - deliveryId: 'd-1', - action: 'synchronize', - repositoryId: 10, - repository: { - owner: 'owner', - name: 'repo', - fullName: 'owner/repo', - defaultBranch: 'main', + expect(createPrContextJob).toHaveBeenCalledWith( + env.BOT_DB, + { + deliveryId: 'd-1', + action: 'synchronize', + repositoryId: 10, + repository: { + owner: 'owner', + name: 'repo', + fullName: 'owner/repo', + defaultBranch: 'main', + }, + prNumber: 43, + headSha: 'headsha', + baseSha: 'basesha', + title: 'Cloudflare migration', + authorLogin: 'pr-author', + state: 'open', }, - prNumber: 43, - headSha: 'headsha', - baseSha: 'basesha', - title: 'Cloudflare migration', - authorLogin: 'pr-author', - state: 'open', - }); + 456, + ); expect(enqueueJob).toHaveBeenCalledWith(env, 'ctx-1'); expect(authorizeCommenter).not.toHaveBeenCalled(); // PR events bypass the command path }); @@ -644,6 +679,93 @@ describe('pull_request events', () => { }); }); +// --------------------------------------------------------------------------- +// GitHub App authentication (Gate 4.5 โ€” App-only, no PAT fallback) +// --------------------------------------------------------------------------- + +describe('GitHub App authentication', () => { + it('never mints a token for a pull_request synchronize event', async () => { + // Fix for the eager-mint regression: PR-context events only write D1 and + // enqueue; they must not pay a JWT + token-endpoint round trip. + const res = await signedRequest( + { body: prEventPayload('synchronize'), event: 'pull_request' }, + makeEnv(), + ); + + expect(res.status).toBe(202); + expect(createTokenProvider).not.toHaveBeenCalled(); + expect(mintToken).not.toHaveBeenCalled(); + }); + + it('never mints a token for a plain issue comment (no mirror, no command)', async () => { + const res = await signedRequest({ body: prCommentPayload({ body: 'hello', onPr: false }) }); + + expect(res.status).toBe(200); + expect(createTokenProvider).not.toHaveBeenCalled(); + }); + + it('never mints a token for an ignored pull_request action', async () => { + const res = await signedRequest( + { body: prEventPayload('closed'), event: 'pull_request' }, + makeEnv(), + ); + + expect(res.status).toBe(200); + expect(createTokenProvider).not.toHaveBeenCalled(); + }); + + it('returns 503 and creates no job when the payload carries no installation id', async () => { + const payload = prCommentPayload({ body: '/zai review' }); + delete payload.installation; + + const res = await signedRequest({ body: payload }); + + expect(res.status).toBe(503); + expect(authorizeCommenter).not.toHaveBeenCalled(); + expect(createCommandJob).not.toHaveBeenCalled(); + }); + + it('returns 503 when the App credentials are not configured', async () => { + createTokenProvider.mockResolvedValue({ available: false, getInstallationToken: vi.fn() }); + + const res = await signedRequest({ body: prCommentPayload({ body: '/zai review' }) }); + + expect(res.status).toBe(503); + expect(mintToken).not.toHaveBeenCalled(); + expect(createCommandJob).not.toHaveBeenCalled(); + }); + + it('returns 503 (not 500) when the token mint fails', async () => { + // Persistent rejection: a PR comment also feeds the mirror path, which + // consumes (and swallows) the first mint attempt before Gate 4.5 runs. + mintToken.mockRejectedValue( + Object.assign(new Error('GitHub API error: 500'), { + code: 'app_token_fetch_failed', + retryable: true, + }), + ); + + const res = await signedRequest({ body: prCommentPayload({ body: '/zai review' }) }); + await flushWaitUntil(ctx); + + expect(res.status).toBe(503); + expect(authorizeCommenter).not.toHaveBeenCalled(); + expect(createCommandJob).not.toHaveBeenCalled(); + }); + + it('skips the comment mirror when App auth fails but still acks 200', async () => { + mintToken.mockRejectedValueOnce( + Object.assign(new Error('GitHub API error: 500'), { code: 'app_token_fetch_failed' }), + ); + + const res = await signedRequest({ body: prCommentPayload({ body: 'hello' }) }); + await flushWaitUntil(ctx); + + expect(res.status).toBe(200); + expect(refreshCommentsSlice).not.toHaveBeenCalled(); + }); +}); + // --------------------------------------------------------------------------- // Mirror paths: comments slice + description slice refreshes (ctx.waitUntil) // --------------------------------------------------------------------------- diff --git a/src/tests/queue.test.js b/src/tests/queue.test.js index 2312e264..392f7799 100644 --- a/src/tests/queue.test.js +++ b/src/tests/queue.test.js @@ -14,10 +14,14 @@ vi.mock('../shared/storage/jobs.js', () => ({ vi.mock('../zai-heavy-worker/src/handlers/index.js', () => ({ getHeavyHandler: vi.fn(), })); +vi.mock('../shared/github.js', () => ({ GitHubClient: vi.fn() })); +vi.mock('../shared/github-app-auth.js', () => ({ createTokenProvider: vi.fn() })); import { getHeavyHandler } from '../zai-heavy-worker/src/handlers/index.js'; import { processQueueMessage } from '../zai-heavy-worker/src/queue.js'; import { getJob } from '../shared/storage/deliveries.js'; +import { GitHubClient } from '../shared/github.js'; +import { createTokenProvider } from '../shared/github-app-auth.js'; import { claimJob, markJobFailed, @@ -37,6 +41,7 @@ function claimed(attempt_count = 1) { job_id: 'job-1', kind: 'review', attempt_count, + installation_id: 456, repository_owner: 'owner', repository_name: 'repo', pr_number: 1, @@ -44,6 +49,8 @@ function claimed(attempt_count = 1) { } describe('heavy worker queue protocol', () => { + let mintToken; + beforeEach(() => { vi.clearAllMocks(); getHeavyHandler.mockReturnValue(vi.fn().mockResolvedValue({ status: 'success' })); @@ -51,6 +58,16 @@ describe('heavy worker queue protocol', () => { markJobRetryable.mockResolvedValue(undefined); markJobFailed.mockResolvedValue(undefined); markJobSucceeded.mockResolvedValue(undefined); + // App token provider: available by default, minting a fixed token. + mintToken = vi.fn().mockResolvedValue('ghs_test'); + createTokenProvider.mockResolvedValue({ available: true, getInstallationToken: mintToken }); + GitHubClient.mockImplementation( + class { + constructor(token) { + this.token = token; + } + }, + ); }); it('acks malformed messages without touching storage', async () => { @@ -125,7 +142,7 @@ describe('heavy worker queue protocol', () => { ); const msg = message(); - await processQueueMessage(msg, { BOT_DB: {}, GITHUB_TOKEN: 'token' }, logger); + await processQueueMessage(msg, { BOT_DB: {} }, logger); expect(markJobRetryable).toHaveBeenCalledWith({}, 'job-1', 'run-1', 'github_unavailable', 20); expect(msg.retry).toHaveBeenCalledWith({ delaySeconds: 20 }); @@ -149,7 +166,7 @@ describe('heavy worker queue protocol', () => { ); const msg = message(); - await processQueueMessage(msg, { BOT_DB: {}, GITHUB_TOKEN: 'token' }, logger); + await processQueueMessage(msg, { BOT_DB: {} }, logger); expect(markJobRetryable).toHaveBeenCalledWith({}, 'job-1', 'run-1', 'llm_timeout', 20); expect(msg.retry).toHaveBeenCalledWith({ delaySeconds: 20 }); @@ -161,7 +178,7 @@ describe('heavy worker queue protocol', () => { getHeavyHandler.mockReturnValue(vi.fn().mockRejectedValue(new Error('permanent'))); const msg = message(); - await processQueueMessage(msg, { BOT_DB: {}, GITHUB_TOKEN: 'token' }, logger); + await processQueueMessage(msg, { BOT_DB: {} }, logger); expect(markJobFailed).toHaveBeenCalledWith({}, 'job-1', 'run-1', 'operation_failed'); expect(msg.ack).toHaveBeenCalledOnce(); @@ -178,9 +195,130 @@ describe('heavy worker queue protocol', () => { startAnalysisRun.mockRejectedValue(new Error('D1 unavailable')); const msg = message(); - await processQueueMessage(msg, { BOT_DB: {}, GITHUB_TOKEN: 'token' }, logger); + await processQueueMessage(msg, { BOT_DB: {} }, logger); expect(markJobRetryable).toHaveBeenCalledWith({}, 'job-1', null, 'job_failed', 20); expect(msg.retry).toHaveBeenCalledWith({ delaySeconds: 20 }); }); }); + +describe('heavy worker queue GitHub App authentication', () => { + let mintToken; + + beforeEach(() => { + vi.clearAllMocks(); + startAnalysisRun.mockResolvedValue('run-1'); + markJobRetryable.mockResolvedValue(undefined); + markJobFailed.mockResolvedValue(undefined); + markJobSucceeded.mockResolvedValue(undefined); + mintToken = vi.fn().mockResolvedValue('ghs_test'); + createTokenProvider.mockResolvedValue({ available: true, getInstallationToken: mintToken }); + GitHubClient.mockImplementation( + class { + constructor(token) { + this.token = token; + } + }, + ); + }); + + it('authenticates the handler client as the App installation', async () => { + getJob.mockResolvedValue(claimed()); + claimJob.mockResolvedValue(claimed(1)); + const handler = vi.fn().mockResolvedValue({ status: 'success' }); + getHeavyHandler.mockReturnValue(handler); + const msg = message(); + + await processQueueMessage(msg, { BOT_DB: {} }, logger); + + expect(mintToken).toHaveBeenCalledWith(456); + expect(GitHubClient).toHaveBeenCalledWith('ghs_test'); + const handlerArg = handler.mock.calls[0][0]; + expect(handlerArg.github).toBeInstanceOf(Object); + expect(handlerArg.github.token).toBe('ghs_test'); + expect(markJobSucceeded).toHaveBeenCalledWith({}, 'job-1', 'run-1'); + expect(msg.ack).toHaveBeenCalledOnce(); + }); + + it('fails a legacy job without installation_id instead of retrying it', async () => { + const legacy = { ...claimed(), installation_id: null }; + getJob.mockResolvedValue(legacy); + claimJob.mockResolvedValue(legacy); + const handler = vi.fn(); + getHeavyHandler.mockReturnValue(handler); + const msg = message(); + + await processQueueMessage(msg, { BOT_DB: {} }, logger); + + expect(handler).not.toHaveBeenCalled(); + expect(mintToken).not.toHaveBeenCalled(); + expect(markJobFailed).toHaveBeenCalledWith({}, 'job-1', 'run-1', 'operation_failed'); + expect(msg.ack).toHaveBeenCalledOnce(); + expect(msg.retry).not.toHaveBeenCalled(); + expect(logger.error).toHaveBeenCalledWith( + 'Queue job operation_failed', + expect.objectContaining({ causeCode: 'missing_installation_id' }), + ); + }); + + it('fails the job when App credentials are not configured', async () => { + createTokenProvider.mockResolvedValue({ available: false, getInstallationToken: vi.fn() }); + getJob.mockResolvedValue(claimed()); + claimJob.mockResolvedValue(claimed(1)); + const msg = message(); + + await processQueueMessage(msg, { BOT_DB: {} }, logger); + + expect(markJobFailed).toHaveBeenCalledWith({}, 'job-1', 'run-1', 'operation_failed'); + expect(msg.ack).toHaveBeenCalledOnce(); + expect(logger.error).toHaveBeenCalledWith( + 'Queue job operation_failed', + expect.objectContaining({ causeCode: 'app_auth_unconfigured' }), + ); + }); + + it('retries a transient mint failure with backoff', async () => { + mintToken.mockRejectedValue( + Object.assign(new Error('GitHub token endpoint returned 500'), { + code: 'app_token_fetch_failed', + retryable: true, + }), + ); + getJob.mockResolvedValue(claimed()); + claimJob.mockResolvedValue(claimed(1)); + const msg = message(); + + await processQueueMessage(msg, { BOT_DB: {} }, logger); + + expect(markJobRetryable).toHaveBeenCalledWith( + {}, + 'job-1', + 'run-1', + 'app_token_fetch_failed', + 20, + ); + expect(msg.retry).toHaveBeenCalledWith({ delaySeconds: 20 }); + expect(msg.ack).not.toHaveBeenCalled(); + }); + + it('fails the job when the app JWT is rejected (401)', async () => { + mintToken.mockRejectedValue( + Object.assign(new Error('GitHub rejected the app JWT'), { + code: 'app_jwt_rejected', + retryable: false, + }), + ); + getJob.mockResolvedValue(claimed()); + claimJob.mockResolvedValue(claimed(1)); + const msg = message(); + + await processQueueMessage(msg, { BOT_DB: {} }, logger); + + expect(markJobFailed).toHaveBeenCalledWith({}, 'job-1', 'run-1', 'operation_failed'); + expect(msg.ack).toHaveBeenCalledOnce(); + expect(logger.error).toHaveBeenCalledWith( + 'Queue job operation_failed', + expect.objectContaining({ causeCode: 'app_jwt_rejected' }), + ); + }); +}); diff --git a/src/zai-heavy-worker/.dev.vars.example b/src/zai-heavy-worker/.dev.vars.example index dea0e5f1..5ffa2649 100644 --- a/src/zai-heavy-worker/.dev.vars.example +++ b/src/zai-heavy-worker/.dev.vars.example @@ -2,5 +2,8 @@ # Copy to .dev.vars and fill in real values. NEVER commit .dev.vars. # Keys are the `binding` names (= env vars the code reads); in production these # are injected from the Cloudflare Secrets Store (store 629e5dd...). -GITHUB_TOKEN=ghp_local_replace_me +# Auth is GitHub App only (PAT support removed โ€” see PAT_REMOVAL_PLAN.md). +GITHUB_APP_ID=123456 +# Full PEM, newlines escaped as \n on a single line. +GITHUB_APP_PRIVATE_KEY="-----BEGIN PRIVATE KEY-----\n...\n-----END PRIVATE KEY-----" ZAI_API_KEY=local-zai-key diff --git a/src/zai-heavy-worker/src/queue.js b/src/zai-heavy-worker/src/queue.js index 9da903c2..80b270c3 100644 --- a/src/zai-heavy-worker/src/queue.js +++ b/src/zai-heavy-worker/src/queue.js @@ -1,6 +1,6 @@ import { GitHubClient } from '../../shared/github.js'; import { createLogger, generateCorrelationId } from '../../shared/logging.js'; -import { resolveSecretValue } from '../../shared/secrets.js'; +import { createTokenProvider } from '../../shared/github-app-auth.js'; import { getJob } from '../../shared/storage/deliveries.js'; import { safeErrorCode } from '../../shared/storage/database.js'; import { @@ -22,6 +22,44 @@ export async function processQueueBatch(batch, env) { } } +/** + * Creates a GitHub client for queue processing. GitHub App auth is the ONLY + * path (PAT support removed). Config/permanent failures throw with + * `retryable: false` so the queue fails the job instead of burning attempts; + * transient mint failures (network, 5xx) retry with backoff via the + * provider's classified appAuthError. + * @param {Object} env - Environment bindings + * @param {Object} job - Claimed job row (carries installation_id) + * @param {Object} logger - Logger instance + * @returns {Promise} + */ +async function createQueueGitHubClient(env, job, logger) { + if (!job.installation_id) { + // Pre-migration job (created before installation_id was recorded): it + // can never authenticate post-PAT-removal โ€” fail fast, do not retry. + const legacy = new Error('missing_installation_id'); + legacy.code = 'missing_installation_id'; + legacy.retryable = false; + throw legacy; + } + + const provider = await createTokenProvider(env); + if (!provider.available) { + const unconfigured = new Error('app_auth_unconfigured'); + unconfigured.code = 'app_auth_unconfigured'; + unconfigured.retryable = false; + throw unconfigured; + } + + // Throws a classified appAuthError on failure (code + retryable). + const token = await provider.getInstallationToken(job.installation_id); + logger.info('Using GitHub App authentication for queue job', { + installationId: job.installation_id, + jobId: job.job_id, + }); + return new GitHubClient(token); +} + export async function processQueueMessage( message, env, @@ -83,7 +121,10 @@ export async function processQueueMessage( throw unsupported; } runId = await startAnalysisRun(env.BOT_DB, jobId, claimed.attempt_count); - const github = new GitHubClient(await resolveSecretValue(env.GITHUB_TOKEN)); + + // Create GitHub client with App auth if installation_id is available + const github = await createQueueGitHubClient(env, claimed, logger); + await handler({ github, env, db: env.BOT_DB, job: claimed, runId }); await markJobSucceeded(env.BOT_DB, jobId, runId); message.ack(); diff --git a/src/zai-heavy-worker/wrangler.toml b/src/zai-heavy-worker/wrangler.toml index 8eb0fe81..ca468fdd 100644 --- a/src/zai-heavy-worker/wrangler.toml +++ b/src/zai-heavy-worker/wrangler.toml @@ -62,14 +62,23 @@ mode = "smart" # Same store as zai-main-worker (shared). `binding` = env var the code reads; # `secret_name` = key inside the store. # Store id: 629e5dd6594845a889e6ddabb26cc009 -# GITHUB_TOKEN <- secret ZAI_GITHUB_TOKEN (GitHub PAT for result comments) +# GITHUB_APP_ID <- secret ZAI_GITHUB_APP_ID (GitHub App ID; App-only auth) +# GITHUB_APP_PRIVATE_KEY <- secret ZAI_GITHUB_APP_PRIVATE_KEY (GitHub App private key) # ZAI_API_KEY <- secret ZAI_API_KEY (Z.ai key) +# NOTE: the legacy ZAI_GITHUB_TOKEN (PAT) entry is intentionally NOT bound; +# it is kept in the store only for a rollback window and can be deleted once +# the App-only rollout is confirmed stable. [[secrets_store_secrets]] -binding = "GITHUB_TOKEN" +binding = "ZAI_API_KEY" store_id = "629e5dd6594845a889e6ddabb26cc009" -secret_name = "ZAI_GITHUB_TOKEN" +secret_name = "ZAI_API_KEY" [[secrets_store_secrets]] -binding = "ZAI_API_KEY" +binding = "GITHUB_APP_ID" store_id = "629e5dd6594845a889e6ddabb26cc009" -secret_name = "ZAI_API_KEY" +secret_name = "ZAI_GITHUB_APP_ID" + +[[secrets_store_secrets]] +binding = "GITHUB_APP_PRIVATE_KEY" +store_id = "629e5dd6594845a889e6ddabb26cc009" +secret_name = "ZAI_GITHUB_APP_PRIVATE_KEY" diff --git a/src/zai-main-worker/.dev.vars.example b/src/zai-main-worker/.dev.vars.example index afbb5b35..1b1c9866 100644 --- a/src/zai-main-worker/.dev.vars.example +++ b/src/zai-main-worker/.dev.vars.example @@ -2,6 +2,9 @@ # Copy to .dev.vars and fill in real values. NEVER commit .dev.vars. # Keys are the `binding` names (= env vars the code reads); in production these # are injected from the Cloudflare Secrets Store (store 629e5dd...). +# Auth is GitHub App only (PAT support removed โ€” see PAT_REMOVAL_PLAN.md). GITHUB_WEBHOOK_SECRET=local-webhook-secret -GITHUB_TOKEN=ghp_local_replace_me +GITHUB_APP_ID=123456 +# Full PEM, newlines escaped as \n on a single line. +GITHUB_APP_PRIVATE_KEY="-----BEGIN PRIVATE KEY-----\n...\n-----END PRIVATE KEY-----" ZAI_API_KEY=local-zai-key diff --git a/src/zai-main-worker/migrations/0004_add_installation_id.sql b/src/zai-main-worker/migrations/0004_add_installation_id.sql new file mode 100644 index 00000000..a176dc2b --- /dev/null +++ b/src/zai-main-worker/migrations/0004_add_installation_id.sql @@ -0,0 +1,13 @@ +-- Migration: Add installation_id to jobs table for GitHub App authentication +-- Up: Add installation_id column to jobs table + +ALTER TABLE jobs ADD COLUMN installation_id INTEGER; + +-- Create index for installation_id to speed up queries +CREATE INDEX IF NOT EXISTS idx_jobs_installation_id ON jobs(installation_id); + +-- Add installation_id to webhook_deliveries table for tracking +ALTER TABLE webhook_deliveries ADD COLUMN installation_id INTEGER; + +-- Add installation_id to repositories table for reference +ALTER TABLE repositories ADD COLUMN github_app_installation_id INTEGER; diff --git a/src/zai-main-worker/src/index.js b/src/zai-main-worker/src/index.js index 1f8ea5b0..e406b46b 100644 --- a/src/zai-main-worker/src/index.js +++ b/src/zai-main-worker/src/index.js @@ -11,6 +11,7 @@ import { GitHubClient } from '../../shared/github.js'; import { verifyWebhookSignature } from '../../shared/crypto.js'; import { resolveSecretValue } from '../../shared/secrets.js'; +import { createTokenProvider } from '../../shared/github-app-auth.js'; import { parseCommand, isCommand, @@ -66,6 +67,55 @@ function isWebhookPath(request) { return normalized === GITHUB_WEBHOOK_PATH; } +/** + * Creates a GitHub client authenticated as the App installation. GitHub App + * auth is the ONLY path (PAT support removed): missing configuration or a + * missing installation id throws with `status: 503` so the caller can tell + * GitHub to redeliver the webhook instead of failing opaquely. + * @param {Object} env - Environment bindings + * @param {string|number} installationId - GitHub App installation ID from webhook + * @param {Object} logger - Logger instance + * @returns {Promise} + */ +async function createAppGitHubClient(env, installationId, logger) { + const provider = await createTokenProvider(env); + + if (!installationId) { + logger.error('Webhook carried no installation id; is the webhook source the GitHub App?', {}); + throw Object.assign(new Error('missing_installation_id'), { + code: 'missing_installation_id', + status: 503, + }); + } + if (!provider.available) { + logger.error('GitHub App auth unconfigured (GITHUB_APP_ID / GITHUB_APP_PRIVATE_KEY)', {}); + throw Object.assign(new Error('app_auth_unconfigured'), { + code: 'app_auth_unconfigured', + status: 503, + }); + } + + let token; + try { + token = await provider.getInstallationToken(installationId); + } catch (error) { + // Mint failures are server-side auth problems: 503 makes GitHub retry + // the delivery. Log the classified code โ€” never the raw provider body. + logger.error('GitHub App token mint failed', { + code: error?.code, + retryable: error?.retryable, + }); + throw Object.assign(new Error(error?.code || 'app_token_fetch_failed'), { + code: error?.code, + retryable: error?.retryable, + status: 503, + }); + } + + logger.info('Using GitHub App authentication', { installationId }); + return new GitHubClient(token); +} + export default { async fetch(request, env, ctx) { const logger = createLogger(env, 'zai-main-worker'); @@ -114,6 +164,12 @@ export default { repo: webhookData.repository?.full_name, }); + // Installation id from the webhook payload โ€” required for GitHub App + // auth on every path that talks to the GitHub API (command path and the + // comment mirror), and stored on jobs for the heavy worker. No client is + // built here: PR-context events must not pay a token mint. + const installationId = webhookData.installation?.id; + // --- Mirror PR conversation comments into the comments context slice --- // issue_comment created/edited/deleted on a PR trigger a full re-fetch of // the conversation (getPrComments) that overwrites comments.json, so the @@ -124,7 +180,7 @@ export default { if (isPrCommentRefreshEvent(ghEvent, webhookData, env)) { const plan = planCommentsRefresh(webhookData); if (plan) { - ctx.waitUntil(refreshPrComments(env, plan).catch(() => {})); + ctx.waitUntil(refreshPrComments(env, plan, installationId, logger).catch(() => {})); } } @@ -158,7 +214,7 @@ export default { if (!CONTEXT_TRIGGER_ACTIONS.includes(prEvent.action)) { return json(200, { status: 'ignored', action: prEvent.action }); } - const context = await createPrContextJob(env.BOT_DB, prEvent); + const context = await createPrContextJob(env.BOT_DB, prEvent, installationId); try { await enqueueJob(env, context.job.job_id); } catch (error) { @@ -193,8 +249,13 @@ export default { return new Response('OK', { status: 200 }); } + // --- Gate 4.5: GitHub App client (App-only auth; no PAT fallback) --- + // Built only now: everything above (PR-context jobs, mirrors, + // non-command comments) must not pay a token mint. Auth/config errors + // throw with status 503 so GitHub redelivers the webhook. + const github = await createAppGitHubClient(env, installationId, logger); + // --- Gate 5: authorization (collaborator check) --- - const github = new GitHubClient(await resolveSecretValue(env.GITHUB_TOKEN)); const { owner, name, full_name } = repoCoordinates(webhookData.repository); const username = webhookData.comment?.user?.login; @@ -213,7 +274,7 @@ export default { const bucket = classifyCommand(parsed.type); if (bucket === 'help') { - const botLogin = await resolveBotLogin(env, github); + const botLogin = resolveBotLogin(env); await postHelp(github, owner, name, webhookData.issue.number, botLogin); return json(200, { status: 'help', command: parsed.type }); } @@ -229,6 +290,7 @@ export default { webhookData, parsed.type, request.headers.get('x-github-delivery'), + installationId, ); await enqueueJob(env, created.job.job_id); logger.info('Enqueued durable command job', { @@ -259,9 +321,15 @@ export default { } catch (error) { logger.error('Error processing request', { message: error.message, + code: error.code, correlationId, }); - return new Response('Internal Server Error', { status: 500 }); + // Auth/config failures carry status 503 so GitHub redelivers the + // webhook; anything unexpected stays a 500. + const status = error.status === 503 ? 503 : 500; + return new Response(status === 503 ? 'Service Unavailable' : 'Internal Server Error', { + status, + }); } }, @@ -330,7 +398,7 @@ function canRouteDurable(webhookData, env) { * queue consumer then runs the handler with the full {github, env, db, job, * runId} context. */ -async function createCommandDurableJob(env, github, webhookData, kind, deliveryId) { +async function createCommandDurableJob(env, github, webhookData, kind, deliveryId, installationId) { const { owner, name, full_name } = repoCoordinates(webhookData.repository); const prNumber = webhookData.issue.number; const pr = await github.getPullRequest(owner, name, prNumber); @@ -347,6 +415,7 @@ async function createCommandDurableJob(env, github, webhookData, kind, deliveryI title: pr.title || null, authorLogin: pr.user?.login || null, state: pr.state || 'open', + installationId: installationId, }; return createCommandJob(env.BOT_DB, event, kind); } @@ -380,20 +449,14 @@ async function postUnsupported(github, owner, name, issueNumber, commandType) { } /** - * Resolves the login that owns this deployment's GitHub token, so help-comment - * updates can recognize PAT-owned (type 'User') bot comments. Prefers an - * explicit GITHUB_BOT_LOGIN var; otherwise asks the API once (help is rare, - * no caching warranted). Resolving is best-effort: on failure only comments - * from GitHub Apps (type 'Bot') match, which at worst posts a fresh help - * comment instead of updating a PAT-owned one โ€” never rewrites a user's. + * Optional login that owned this deployment's PAT-era comments (type 'User'), + * so help-comment updates can still recognize them. Installation tokens + * cannot call GET /user (403), so this is a pure env read now; when unset, + * only GitHub App comments (type 'Bot') match โ€” at worst a legacy help + * comment is not recognized and a fresh one is posted once, never a user's. */ -async function resolveBotLogin(env, github) { - if (env?.GITHUB_BOT_LOGIN) return env.GITHUB_BOT_LOGIN; - try { - return (await github.getAuthenticatedUser())?.login || null; - } catch { - return null; - } +function resolveBotLogin(env) { + return env?.GITHUB_BOT_LOGIN || null; } async function postHelp(github, owner, name, issueNumber, botLogin = null) { @@ -407,6 +470,7 @@ async function postHelp(github, owner, name, issueNumber, botLogin = null) { isBotOwnedComment(comment, { botLogin }), ) : null; + if (existing?.id) { await github.updateComment(owner, name, existing.id, formatHelp()); } else { @@ -419,11 +483,17 @@ async function postHelp(github, owner, name, issueNumber, botLogin = null) { /** * Full-refresh of the PR `comments` context slice on an issue_comment event. - * Builds a throwaway GitHubClient (this path runs before the command path - * constructs one) and runs the shared refreshCommentsSlice. Errors are - * swallowed by the caller's ctx.waitUntil โ€” the slice is derivative. + * Builds a throwaway App-authenticated GitHubClient and runs the shared + * refreshCommentsSlice. Best-effort: on App-auth failure the mirror is + * skipped โ€” the slice is derivative (the next gather re-captures it from + * scratch) โ€” and errors never escape into ctx.waitUntil. */ -async function refreshPrComments(env, plan) { - const github = new GitHubClient(await resolveSecretValue(env.GITHUB_TOKEN)); - return refreshCommentsSlice({ github, bucket: env.BOT_ARTIFACTS, ...plan }); +async function refreshPrComments(env, plan, installationId, logger) { + try { + const github = await createAppGitHubClient(env, installationId, logger); + return await refreshCommentsSlice({ github, bucket: env.BOT_ARTIFACTS, ...plan }); + } catch (error) { + logger?.warn?.('Comment slice refresh skipped: app auth unavailable', { code: error?.code }); + return null; + } } diff --git a/src/zai-main-worker/wrangler.toml b/src/zai-main-worker/wrangler.toml index b86a80d7..03057dda 100644 --- a/src/zai-main-worker/wrangler.toml +++ b/src/zai-main-worker/wrangler.toml @@ -55,22 +55,31 @@ custom_domain = true # reads (env.) and `secret_name` is the key inside the store. # Store id: 629e5dd6594845a889e6ddabb26cc009 # GITHUB_WEBHOOK_SECRET <- secret ZAI_GITHUB_WEBHOOK_KEY (HMAC-SHA256 webhook secret) -# GITHUB_TOKEN <- secret ZAI_GITHUB_TOKEN (GitHub PAT: repo + read:org) +# GITHUB_APP_ID <- secret ZAI_GITHUB_APP_ID (GitHub App ID; App-only auth) +# GITHUB_APP_PRIVATE_KEY <- secret ZAI_GITHUB_APP_PRIVATE_KEY (GitHub App private key) # ZAI_API_KEY <- secret ZAI_API_KEY (Z.ai key; used once LLM calls land) +# NOTE: the legacy ZAI_GITHUB_TOKEN (PAT) entry is intentionally NOT bound; +# it is kept in the store only for a rollback window and can be deleted once +# the App-only rollout is confirmed stable. [[secrets_store_secrets]] binding = "GITHUB_WEBHOOK_SECRET" store_id = "629e5dd6594845a889e6ddabb26cc009" secret_name = "ZAI_GITHUB_WEBHOOK_KEY" [[secrets_store_secrets]] -binding = "GITHUB_TOKEN" +binding = "ZAI_API_KEY" store_id = "629e5dd6594845a889e6ddabb26cc009" -secret_name = "ZAI_GITHUB_TOKEN" +secret_name = "ZAI_API_KEY" [[secrets_store_secrets]] -binding = "ZAI_API_KEY" +binding = "GITHUB_APP_ID" store_id = "629e5dd6594845a889e6ddabb26cc009" -secret_name = "ZAI_API_KEY" +secret_name = "ZAI_GITHUB_APP_ID" + +[[secrets_store_secrets]] +binding = "GITHUB_APP_PRIVATE_KEY" +store_id = "629e5dd6594845a889e6ddabb26cc009" +secret_name = "ZAI_GITHUB_APP_PRIVATE_KEY" [observability.logs] enabled = true