diff --git a/apps/api/src/handlers/github/__tests__/index.test.ts b/apps/api/src/handlers/github/__tests__/index.test.ts index cb417104b..52105dcb6 100644 --- a/apps/api/src/handlers/github/__tests__/index.test.ts +++ b/apps/api/src/handlers/github/__tests__/index.test.ts @@ -19,6 +19,8 @@ const { mockUpdateTaskPrStatus, mockUpsertGitHubPullRequestFactFromWebhook, mockRecordPrStatusChangeInTaskHistory, + mockIsFromKnownInstallation, + mockVerify, mockVerifyAndReceive, webhooksConstructorParams, } = vi.hoisted(() => ({ @@ -47,6 +49,8 @@ const { mockUpdateTaskPrStatus: vi.fn(), mockUpsertGitHubPullRequestFactFromWebhook: vi.fn(), mockRecordPrStatusChangeInTaskHistory: vi.fn(), + mockIsFromKnownInstallation: vi.fn(), + mockVerify: vi.fn(), mockVerifyAndReceive: vi.fn(), webhooksConstructorParams: [] as unknown[], })); @@ -70,6 +74,10 @@ vi.mock('@octokit/webhooks', () => ({ onError() {} + verify(payload: string, signature: string) { + return mockVerify(payload, signature); + } + verifyAndReceive({ id, name, @@ -112,6 +120,10 @@ vi.mock('../handleInstallationCreated', () => ({ handleInstallationCreated: mockHandleInstallationCreated, })); +vi.mock('../isFromKnownInstallation', () => ({ + isFromKnownInstallation: mockIsFromKnownInstallation, +})); + vi.mock('../handlePrComment', () => ({ handlePrComment: mockHandlePrComment, })); @@ -177,11 +189,15 @@ describe('github webhook router', () => { mockResolveDeploymentEnvVar.mockReset(); mockUpdateTaskPrStatus.mockReset(); mockUpsertGitHubPullRequestFactFromWebhook.mockReset(); + mockIsFromKnownInstallation.mockReset(); + mockVerify.mockReset(); mockVerifyAndReceive.mockReset(); mockIsRepoSkipped.mockReturnValue(false); mockResolveConfiguredGitHubAppSlug.mockResolvedValue('roomote'); mockResolveDeploymentEnvVar.mockResolvedValue('test-secret'); + mockIsFromKnownInstallation.mockResolvedValue(true); + mockVerify.mockResolvedValue(true); mockHandlePrComment.mockResolvedValue({ status: 'ok' }); mockRecordWebhook.mockImplementation( async ( @@ -340,6 +356,51 @@ describe('github webhook router', () => { ).toBeLessThan(mockVerifyAndReceive.mock.invocationCallOrder[0]!); }); + it('returns 401 without an installation lookup when the signature is invalid', async () => { + mockVerify.mockResolvedValue(false); + + const response = await app.request('http://localhost/api/webhooks/github', { + method: 'POST', + headers: { + 'x-github-delivery': 'delivery-bad-signature', + 'x-github-event': 'push', + 'x-hub-signature-256': 'sha256=forged', + }, + body: JSON.stringify({ ref: 'refs/heads/main' }), + }); + + expect(response.status).toBe(401); + expect(mockIsFromKnownInstallation).not.toHaveBeenCalled(); + expect(mockVerifyAndReceive).not.toHaveBeenCalled(); + expect(mockRecordWebhook).not.toHaveBeenCalled(); + }); + + it('drops deliveries from unknown installations before recording or handling', async () => { + mockIsFromKnownInstallation.mockResolvedValue(false); + + const payload = JSON.stringify({ + ref: 'refs/heads/main', + installation: { id: 999 }, + }); + + const response = await app.request('http://localhost/api/webhooks/github', { + method: 'POST', + headers: { + 'x-github-delivery': 'delivery-unknown-installation', + 'x-github-event': 'push', + 'x-hub-signature-256': 'sha256=test', + }, + body: payload, + }); + + expect(response.status).toBe(200); + expect(await response.json()).toEqual({ message: 'unknown_installation' }); + expect(mockIsFromKnownInstallation).toHaveBeenCalledWith('push', payload); + expect(mockVerifyAndReceive).not.toHaveBeenCalled(); + expect(mockRecordWebhook).not.toHaveBeenCalled(); + expect(mockResolveConfiguredGitHubAppSlug).not.toHaveBeenCalled(); + }); + it('returns 401 without processing when no webhook secret is configured', async () => { mockResolveDeploymentEnvVar.mockResolvedValue(null); diff --git a/apps/api/src/handlers/github/__tests__/isFromKnownInstallation.test.ts b/apps/api/src/handlers/github/__tests__/isFromKnownInstallation.test.ts new file mode 100644 index 000000000..b67111a9e --- /dev/null +++ b/apps/api/src/handlers/github/__tests__/isFromKnownInstallation.test.ts @@ -0,0 +1,128 @@ +// pnpm --filter @roomote/api test github/__tests__/isFromKnownInstallation.test.ts + +const { mockFindFirst, mockFindPendingFirst } = vi.hoisted(() => ({ + mockFindFirst: vi.fn(), + mockFindPendingFirst: vi.fn(), +})); + +vi.mock('@roomote/db/server', () => ({ + db: { + query: { + githubInstallations: { + findFirst: mockFindFirst, + }, + githubPendingInstallations: { + findFirst: mockFindPendingFirst, + }, + }, + }, + githubInstallations: { + installationId: 'githubInstallations.installationId', + }, + githubPendingInstallations: { + appId: 'githubPendingInstallations.appId', + }, + eq: vi.fn((left: unknown, right: unknown) => [left, right]), +})); + +import { isFromKnownInstallation } from '../isFromKnownInstallation'; + +describe('isFromKnownInstallation', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('allows installation.created when the account has a pending installation', async () => { + mockFindPendingFirst.mockResolvedValue({ id: 'pending-row' }); + + const payload = JSON.stringify({ + action: 'created', + installation: { id: 123, account: { id: 555 } }, + }); + + await expect( + isFromKnownInstallation('installation', payload), + ).resolves.toBe(true); + expect(mockFindFirst).not.toHaveBeenCalled(); + }); + + it('rejects installation.created when no pending installation matches the account', async () => { + mockFindPendingFirst.mockResolvedValue(undefined); + + const payload = JSON.stringify({ + action: 'created', + installation: { id: 123, account: { id: 555 } }, + }); + + await expect( + isFromKnownInstallation('installation', payload), + ).resolves.toBe(false); + }); + + it('rejects installation.created without an installation account', async () => { + const payload = JSON.stringify({ + action: 'created', + installation: { id: 123 }, + }); + + await expect( + isFromKnownInstallation('installation', payload), + ).resolves.toBe(false); + expect(mockFindPendingFirst).not.toHaveBeenCalled(); + }); + + it('allows events from a known installation', async () => { + mockFindFirst.mockResolvedValue({ id: 'installation-row' }); + + const payload = JSON.stringify({ + action: 'opened', + installation: { id: 456 }, + }); + + await expect( + isFromKnownInstallation('pull_request', payload), + ).resolves.toBe(true); + }); + + it('rejects events from an unknown installation', async () => { + mockFindFirst.mockResolvedValue(undefined); + + const payload = JSON.stringify({ + action: 'opened', + installation: { id: 789 }, + }); + + await expect( + isFromKnownInstallation('pull_request', payload), + ).resolves.toBe(false); + }); + + it('rejects non-created installation events from an unknown installation', async () => { + mockFindFirst.mockResolvedValue(undefined); + + const payload = JSON.stringify({ + action: 'deleted', + installation: { id: 789 }, + }); + + await expect( + isFromKnownInstallation('installation', payload), + ).resolves.toBe(false); + expect(mockFindPendingFirst).not.toHaveBeenCalled(); + }); + + it('allows signed events without an installation reference', async () => { + const payload = JSON.stringify({ zen: 'Keep it logically awesome.' }); + + await expect(isFromKnownInstallation('ping', payload)).resolves.toBe(true); + expect(mockFindFirst).not.toHaveBeenCalled(); + }); + + it('defers malformed payloads to signature verification', async () => { + await expect( + isFromKnownInstallation('pull_request', 'not-json'), + ).resolves.toBe(true); + expect(mockFindFirst).not.toHaveBeenCalled(); + expect(mockFindPendingFirst).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/api/src/handlers/github/index.ts b/apps/api/src/handlers/github/index.ts index 518cea278..ae49ceac7 100644 --- a/apps/api/src/handlers/github/index.ts +++ b/apps/api/src/handlers/github/index.ts @@ -35,6 +35,7 @@ import { handlePushConflictCheck } from './handlePushConflictCheck'; import { handleWorkflowRunCompleted } from './handleWorkflowRunCompleted'; // Utilities: +import { isFromKnownInstallation } from './isFromKnownInstallation'; import { recordWebhook } from './recordWebhook'; /** @@ -487,6 +488,23 @@ github.post('/', async (c) => { const payload = await c.req.text(); + // Verify the signature before the installation lookup so unsigned junk + // cannot trigger database reads. + if (!(await webhooks.verify(payload, signature))) { + return c.json({ error: 'invalid_signature' }, { status: 401 }); + } + + // The app is public, so any account can install it and produce validly + // signed deliveries; drop events from installations this deployment does + // not know before they are recorded or handled. + if (!(await isFromKnownInstallation(name, payload))) { + apiLogger.debug( + `[GitHub] ignoring webhook ${id} (${name}) from unknown installation`, + ); + + return c.json({ message: 'unknown_installation' }); + } + // The event handlers classify logins synchronously (mention detection, // bot-identity checks); refresh the configured app slug first so an app // created through the /setup flow is recognized as ourselves. diff --git a/apps/api/src/handlers/github/isFromKnownInstallation.ts b/apps/api/src/handlers/github/isFromKnownInstallation.ts new file mode 100644 index 000000000..415cf6770 --- /dev/null +++ b/apps/api/src/handlers/github/isFromKnownInstallation.ts @@ -0,0 +1,79 @@ +import { + db, + eq, + githubInstallations, + githubPendingInstallations, +} from '@roomote/db/server'; + +/** + * A public GitHub App can be installed by any account, so a validly signed + * webhook delivery is not proof of a relationship with this deployment. + * Events must reference an installation this deployment has already synced + * before they are recorded or handled. `installation` creation events are + * instead matched against pending installations - the same check their + * handler enforces - so installs nobody requested through this deployment + * are dropped before they are persisted. + */ +export async function isFromKnownInstallation( + eventName: string, + rawPayload: string, +): Promise { + let action: string | undefined; + let installationId: number | undefined; + let accountId: number | undefined; + + try { + const parsed: unknown = JSON.parse(rawPayload); + + if (typeof parsed !== 'object' || parsed === null) { + return true; + } + + const payload = parsed as { + action?: unknown; + installation?: { id?: unknown; account?: { id?: unknown } | null } | null; + }; + + action = typeof payload.action === 'string' ? payload.action : undefined; + installationId = + typeof payload.installation?.id === 'number' + ? payload.installation.id + : undefined; + accountId = + typeof payload.installation?.account?.id === 'number' + ? payload.installation.account.id + : undefined; + } catch { + // Defer malformed payloads to verifyAndReceive's own error handling. + return true; + } + + if (eventName === 'installation' && action === 'created') { + if (accountId === undefined) { + return false; + } + + // The pending row's appId column stores the account id of the requested + // installation target; see finishCreateGitHubInstallationCommand. + const pendingInstallation = + await db.query.githubPendingInstallations.findFirst({ + where: eq(githubPendingInstallations.appId, accountId), + columns: { id: true }, + }); + + return pendingInstallation !== undefined; + } + + if (installationId === undefined) { + // Signed events without an installation reference (for example `ping`) + // come from GitHub for this app itself. + return true; + } + + const installation = await db.query.githubInstallations.findFirst({ + where: eq(githubInstallations.installationId, installationId), + columns: { id: true }, + }); + + return installation !== undefined; +} diff --git a/apps/docs/providers/source-control/github.mdx b/apps/docs/providers/source-control/github.mdx index 43e4552d4..2118327b4 100644 --- a/apps/docs/providers/source-control/github.mdx +++ b/apps/docs/providers/source-control/github.mdx @@ -16,7 +16,14 @@ Open `/setup`, choose GitHub, and click **Create GitHub App**. Roomote sends GitHub a manifest with the callback, webhook, permissions, and events preconfigured. After GitHub redirects back, Roomote saves the generated app ID, app slug, OAuth client credentials, webhook secret, and private key, then sends -you to install the app on repositories. +you to install the app on repositories. You pick the account or organization +to install on during that install step. + +By default the app is created on your personal GitHub account and marked as +installable on any account, so you can install it on any organization you +belong to. If your organization should own the app instead, click **Show +advanced config** and enter the organization name before clicking **Create +GitHub App**. Use the manual steps only when you already have a GitHub App or want to manage these values in deployment environment variables yourself. @@ -55,6 +62,11 @@ these values in deployment environment variables yourself. openssl rand -hex 32 ``` +11. Under **Where can this GitHub App be installed?**, choose **Any account** + if you want to install the app somewhere other than the account that owns + it. **Only on this account** also works when the owning account is where + you will install it. + ## Permissions and events Grant these repository permissions: diff --git a/apps/web/src/app/(onboarding)/setup/GitHubSourceControlConfig.tsx b/apps/web/src/app/(onboarding)/setup/GitHubSourceControlConfig.tsx index 1353c50d4..a75e73056 100644 --- a/apps/web/src/app/(onboarding)/setup/GitHubSourceControlConfig.tsx +++ b/apps/web/src/app/(onboarding)/setup/GitHubSourceControlConfig.tsx @@ -21,6 +21,7 @@ export function GitHubSourceControlConfig({ onManualValues: () => void; }) { const [githubOrganization, setGithubOrganization] = useState(''); + const [showAdvancedConfig, setShowAdvancedConfig] = useState(false); const [manifestForm, setManifestForm] = useState<{ postTarget: string; values: { manifest: string }; @@ -54,26 +55,48 @@ export function GitHubSourceControlConfig({

Roomote can create it for you automatically or you can enter values manually if you already have an app or want to do each step yourself. + You'll pick the account or organization to install it on during + the GitHub install step.

-
- - setGithubOrganization(event.target.value)} - placeholder="your-organization" - disabled={createGitHubAppManifest.isPending || manifestForm !== null} - data-1p-ignore - /> -

- The app can only be installed on the account that owns it. Enter an - organization name to create the app there, or leave this blank to - create it on your personal GitHub account. -

+
+
+ +
+ {showAdvancedConfig ? ( + <> +
+ + setGithubOrganization(event.target.value)} + placeholder="your-organization" + disabled={ + createGitHubAppManifest.isPending || manifestForm !== null + } + data-1p-ignore + /> +
+

+ By default the app is created on your personal GitHub account and + can be installed on any organization you belong to. Enter an + organization name if the organization should own the app instead. +

+ + ) : null}
{manifestForm ? ( diff --git a/apps/web/src/app/(onboarding)/setup/StepSourceControlConfig.client.test.tsx b/apps/web/src/app/(onboarding)/setup/StepSourceControlConfig.client.test.tsx index ce376c822..690b7346f 100644 --- a/apps/web/src/app/(onboarding)/setup/StepSourceControlConfig.client.test.tsx +++ b/apps/web/src/app/(onboarding)/setup/StepSourceControlConfig.client.test.tsx @@ -176,7 +176,10 @@ describe('StepSourceControlConfig', () => { screen.getByRole('button', { name: 'Enter values manually' }), ).toBeInTheDocument(); expect( - screen.getByLabelText('GitHub organization (optional)'), + screen.queryByLabelText('GitHub organization'), + ).not.toBeInTheDocument(); + expect( + screen.getByRole('button', { name: 'Show advanced config' }), ).toBeInTheDocument(); expect(screen.queryByText('GitHub App ID')).not.toBeInTheDocument(); }); @@ -207,7 +210,10 @@ describe('StepSourceControlConfig', () => { />, ); - fireEvent.change(screen.getByLabelText('GitHub organization (optional)'), { + fireEvent.click( + screen.getByRole('button', { name: 'Show advanced config' }), + ); + fireEvent.change(screen.getByLabelText('GitHub organization'), { target: { value: ' example-org ' }, }); fireEvent.click(screen.getByRole('button', { name: 'Create GitHub App' })); diff --git a/apps/web/src/components/settings/SourceControl.tsx b/apps/web/src/components/settings/SourceControl.tsx index dca2996df..2d1c51fd0 100644 --- a/apps/web/src/components/settings/SourceControl.tsx +++ b/apps/web/src/components/settings/SourceControl.tsx @@ -734,6 +734,7 @@ function GitHubAppSettingsSetup({ redirectTarget: string; }) { const [githubOrganization, setGithubOrganization] = useState(''); + const [showAdvancedConfig, setShowAdvancedConfig] = useState(false); const [manifestForm, setManifestForm] = useState<{ postTarget: string; values: { manifest: string }; @@ -766,25 +767,47 @@ function GitHubAppSettingsSetup({

Self-hosted Roomote needs its own GitHub App. Create one automatically, or enter values manually if you already have an app. + You'll pick the account or organization to install it on during + the GitHub install step.

-
- - setGithubOrganization(event.target.value)} - placeholder="your-organization" - disabled={createGitHubAppManifest.isPending || manifestForm !== null} - data-1p-ignore - /> -

- The app can only be installed on the account that owns it. Enter an - organization name to create the app there, or leave this blank to - create it on your personal GitHub account. -

+
+
+ +
+ {showAdvancedConfig ? ( + <> +
+ + setGithubOrganization(event.target.value)} + placeholder="your-organization" + disabled={ + createGitHubAppManifest.isPending || manifestForm !== null + } + data-1p-ignore + /> +
+

+ By default the app is created on your personal GitHub account and + can be installed on any organization you belong to. Enter an + organization name if the organization should own the app instead. +

+ + ) : null}
{manifestForm ? (
{ ], request_oauth_on_install: true, setup_on_update: true, - public: false, + public: true, }); }); diff --git a/apps/web/src/trpc/commands/github/mutations.ts b/apps/web/src/trpc/commands/github/mutations.ts index 52ee80907..c893dc46f 100644 --- a/apps/web/src/trpc/commands/github/mutations.ts +++ b/apps/web/src/trpc/commands/github/mutations.ts @@ -250,7 +250,10 @@ function buildGitHubAppManifest(): GitHubAppManifest { default_events: [...GITHUB_APP_DEFAULT_EVENTS], request_oauth_on_install: true, setup_on_update: true, - public: false, + // Public so the app can be installed on any account via GitHub's install + // picker; a private app can only be installed on the account that owns + // it, which would force asking for the organization before creation. + public: true, }; }