Skip to content
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 31 additions & 2 deletions frontend/apps/app/app/projects/new/page.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getInstallations } from '@liam-hq/github'
import { getInstallationsForUsername } from '@liam-hq/github'
import { redirect } from 'next/navigation'
import { ProjectNewPage } from '../../../components/ProjectNewPage'
import { getOrganizationId } from '../../../features/organizations/services/getOrganizationId'
Expand Down Expand Up @@ -31,7 +31,36 @@ export default async function NewProjectPage() {
redirect(urlgen('login'))
}

const { installations } = await getInstallations(data.session)
// Derive GitHub username from Supabase user metadata (GitHub provider) without using `any`.
// Supabase types `user.user_metadata` and `identity_data` as `any`, so we first
// treat them as `unknown` and then narrow with a custom type guard.
const isRecord = (value: unknown): value is Record<string, unknown> =>
typeof value === 'object' && value !== null

const usernameFromIdentities = (() => {
const identities = Array.isArray(user.identities) ? user.identities : []
const githubIdentity = identities.find(
(identity) =>
identity &&
typeof identity.provider === 'string' &&
identity.provider === 'github',
)
const identityData = githubIdentity?.identity_data as unknown
if (isRecord(identityData)) {
const userNameField = identityData['user_name']
if (typeof userNameField === 'string') return userNameField
}
return undefined
})()

const githubLogin = usernameFromIdentities

if (!githubLogin) {
console.error('GitHub login not found on user metadata')
redirect(urlgen('login'))
}

const { installations } = await getInstallationsForUsername(githubLogin)

return (
<ProjectNewPage
Expand Down
67 changes: 66 additions & 1 deletion frontend/internal-packages/github/src/api.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
import { createAppAuth } from '@octokit/auth-app'
import { Octokit } from '@octokit/rest'
import { err, type Result, type ResultAsync } from 'neverthrow'
import type { GitHubContentItem } from './types'
import type { GitHubContentItem, Installation } from './types'

const createOctokit = async (installationId: number) => {
const octokit = new Octokit({
Expand All @@ -21,6 +21,71 @@ const createOctokit = async (installationId: number) => {
return octokit
}

const createAppOctokit = async () => {
const octokit = new Octokit({
authStrategy: createAppAuth,
auth: {
appId: process.env['GITHUB_APP_ID'],
privateKey: process.env['GITHUB_PRIVATE_KEY']?.replace(/\\n/g, '\n'),
},
})

return octokit
}

export const getInstallationsForUsername = async (
username: string,
): Promise<{ installations: Installation[] }> => {
const appOctokit = await createAppOctokit()
const _normalizedUsername = username.toLowerCase()

const allInstallations = (await appOctokit.paginate(
appOctokit.request,
'GET /app/installations',
)) as Installation[]

// const normalizedUsername = username.toLowerCase()

// const matchedInstallations: Installation[] = []

// for (const installation of allInstallations) {
// const account = installation.account as {
// type?: string
// login?: string
// } | null
// const accountLogin = account?.login
// const accountType = account?.type

// if (!accountLogin || !accountType) continue

// if (accountType === 'User') {
// if (accountLogin.toLowerCase() === normalizedUsername) {
// matchedInstallations.push(installation)
// }
// continue
// }

// if (accountType === 'Organization') {
// // Authenticate as the installation to check membership for the user directly
// const installationOctokit = await createOctokit(installation.id)
// const membershipResult = await fromPromise(
// installationOctokit.request('GET /orgs/{org}/members/{username}', {
// org: accountLogin,
// username,
// }),
// )

// // If the request succeeds, the user is a member
// if (membershipResult.isOk()) {
// matchedInstallations.push(installation)
// }
// // Errors (e.g., 404, permission) are treated as non-membership
// }
// }

return { installations: allInstallations }
}
Comment on lines 36 to 88
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Filtering currently disabled — every installation leaks through

Right now getInstallationsForUsername just returns allInstallations and never applies any username/org membership filtering (the entire block is commented out). That defeats the purpose of this PR and exposes installations the caller shouldn’t see. Please reinstate the filtering logic and run the org membership checks with bounded concurrency (e.g., Promise.allSettled) so we don’t regress latency while still returning only the installations tied to the requesting user.

-  const appOctokit = await createAppOctokit()
-  const _normalizedUsername = username.toLowerCase()
-
-  const allInstallations = (await appOctokit.paginate(
-    appOctokit.request,
-    'GET /app/installations',
-  )) as Installation[]
-
-  return { installations: allInstallations }
+  const appOctokit = await createAppOctokit()
+  const normalizedUsername = username.trim().toLowerCase()
+
+  const allInstallations = (await appOctokit.paginate(
+    appOctokit.request,
+    'GET /app/installations',
+  )) as Installation[]
+
+  const checks = await Promise.allSettled(
+    allInstallations.map(async (installation) => {
+      const account = installation.account as {
+        type?: string
+        login?: string
+      } | null
+      const accountLogin = account?.login
+      const accountType = account?.type
+
+      if (!accountLogin || !accountType) return null
+
+      if (accountType === 'User') {
+        return accountLogin.toLowerCase() === normalizedUsername
+          ? installation
+          : null
+      }
+
+      if (accountType === 'Organization') {
+        const installationOctokit = await createOctokit(installation.id)
+        const membershipResult = await fromPromise(
+          installationOctokit.request('GET /orgs/{org}/members/{username}', {
+            org: accountLogin,
+            username,
+          }),
+        )
+        return membershipResult.isOk() ? installation : null
+      }
+
+      return null
+    }),
+  )
+
+  const matchedInstallations = checks.reduce<Installation[]>(
+    (acc, result) => {
+      if (result.status === 'fulfilled' && result.value) {
+        acc.push(result.value)
+      }
+      return acc
+    },
+    [],
+  )
+
+  return { installations: matchedInstallations }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const getInstallationsForUsername = async (
username: string,
): Promise<{ installations: Installation[] }> => {
const appOctokit = await createAppOctokit()
const _normalizedUsername = username.toLowerCase()
const allInstallations = (await appOctokit.paginate(
appOctokit.request,
'GET /app/installations',
)) as Installation[]
// const normalizedUsername = username.toLowerCase()
// const matchedInstallations: Installation[] = []
// for (const installation of allInstallations) {
// const account = installation.account as {
// type?: string
// login?: string
// } | null
// const accountLogin = account?.login
// const accountType = account?.type
// if (!accountLogin || !accountType) continue
// if (accountType === 'User') {
// if (accountLogin.toLowerCase() === normalizedUsername) {
// matchedInstallations.push(installation)
// }
// continue
// }
// if (accountType === 'Organization') {
// // Authenticate as the installation to check membership for the user directly
// const installationOctokit = await createOctokit(installation.id)
// const membershipResult = await fromPromise(
// installationOctokit.request('GET /orgs/{org}/members/{username}', {
// org: accountLogin,
// username,
// }),
// )
// // If the request succeeds, the user is a member
// if (membershipResult.isOk()) {
// matchedInstallations.push(installation)
// }
// // Errors (e.g., 404, permission) are treated as non-membership
// }
// }
return { installations: allInstallations }
}
export const getInstallationsForUsername = async (
username: string,
): Promise<{ installations: Installation[] }> => {
const appOctokit = await createAppOctokit()
const normalizedUsername = username.trim().toLowerCase()
const allInstallations = (await appOctokit.paginate(
appOctokit.request,
'GET /app/installations',
)) as Installation[]
// const normalizedUsername = username.toLowerCase()
// const matchedInstallations: Installation[] = []
// for (const installation of allInstallations) {
// const account = installation.account as {
// type?: string
// login?: string
// } | null
// const accountLogin = account?.login
// const accountType = account?.type
//
// if (!accountLogin || !accountType) continue
//
// if (accountType === 'User') {
// if (accountLogin.toLowerCase() === normalizedUsername) {
// matchedInstallations.push(installation)
// }
// continue
// }
//
// if (accountType === 'Organization') {
// // Authenticate as the installation to check membership for the user directly
// const installationOctokit = await createOctokit(installation.id)
// const membershipResult = await fromPromise(
// installationOctokit.request('GET /orgs/{org}/members/{username}', {
// org: accountLogin,
// username,
// }),
// )
//
// // If the request succeeds, the user is a member
// if (membershipResult.isOk()) {
// matchedInstallations.push(installation)
// }
// // Errors (e.g., 404, permission) are treated as non-membership
// }
// }
const checks = await Promise.allSettled(
allInstallations.map(async (installation) => {
const account = installation.account as {
type?: string
login?: string
} | null
const accountLogin = account?.login
const accountType = account?.type
if (!accountLogin || !accountType) return null
if (accountType === 'User') {
return accountLogin.toLowerCase() === normalizedUsername
? installation
: null
}
if (accountType === 'Organization') {
const installationOctokit = await createOctokit(installation.id)
const membershipResult = await fromPromise(
installationOctokit.request('GET /orgs/{org}/members/{username}', {
org: accountLogin,
username,
}),
)
return membershipResult.isOk() ? installation : null
}
return null
}),
)
const matchedInstallations = checks.reduce<Installation[]>(
(acc, result) => {
if (result.status === 'fulfilled' && result.value) {
acc.push(result.value)
}
return acc
},
[],
)
return { installations: matchedInstallations }
}


export const getPullRequestDetails = async (
installationId: number,
owner: string,
Expand Down
Loading