diff --git a/mobile/src/tasks/github-project-reference.test.ts b/mobile/src/tasks/github-project-reference.test.ts index d083d96ccc7..591725819e8 100644 --- a/mobile/src/tasks/github-project-reference.test.ts +++ b/mobile/src/tasks/github-project-reference.test.ts @@ -17,6 +17,17 @@ describe('parseGitHubProjectInput', () => { }) }) + it('accepts Enterprise Managed User owners (`_` suffix)', () => { + expect(parseGitHubProjectInput('octocat_acme/1')).toEqual({ owner: 'octocat_acme', number: 1 }) + expect(parseGitHubProjectInput('https://github.com/users/octocat_acme/projects/1')).toEqual({ + owner: 'octocat_acme', + number: 1, + host: 'github.com' + }) + expect(parseGitHubProjectInput('_acme/1')).toBeNull() + expect(parseGitHubProjectInput('https://github.com/orgs/_acme/projects/1')).toBeNull() + }) + it('accepts github.com user Project URLs', () => { expect(parseGitHubProjectInput('http://github.com/users/octocat/projects/3')).toEqual({ owner: 'octocat', diff --git a/mobile/src/tasks/github-project-reference.ts b/mobile/src/tasks/github-project-reference.ts index 5ed7ef22ead..64761b972e4 100644 --- a/mobile/src/tasks/github-project-reference.ts +++ b/mobile/src/tasks/github-project-reference.ts @@ -1,3 +1,7 @@ +import { + GITHUB_OWNER_NUMBER_SHORTHAND_RE, + GITHUB_OWNER_SLUG_RE +} from '../../../src/shared/github/owner-slug' import type { GitHubProjectIdentity } from '../../../src/shared/github/project-identity' export type GitHubProjectOwnerType = GitHubProjectIdentity['ownerType'] @@ -29,7 +33,7 @@ export type ParsedGitHubProjectInput = { viewNumber?: number } -const OWNER_RE = /^[A-Za-z0-9][A-Za-z0-9-]*$/ +const OWNER_RE = GITHUB_OWNER_SLUG_RE function positiveInteger(value: string | undefined): number | null { if (!value || !/^\d+$/.test(value)) { @@ -41,7 +45,7 @@ function positiveInteger(value: string | undefined): number | null { export function parseGitHubProjectInput(input: string): ParsedGitHubProjectInput | null { const trimmed = input.trim() - const short = /^([A-Za-z0-9][A-Za-z0-9-]*)\/(\d+)$/.exec(trimmed) + const short = GITHUB_OWNER_NUMBER_SHORTHAND_RE.exec(trimmed) if (short) { const number = positiveInteger(short[2]) return number ? { owner: short[1]!, number } : null diff --git a/src/main/git/git-username.test.ts b/src/main/git/git-username.test.ts new file mode 100644 index 00000000000..687f27b97b0 --- /dev/null +++ b/src/main/git/git-username.test.ts @@ -0,0 +1,37 @@ +import { describe, expect, it } from 'vitest' + +import { isPlausibleHostedLogin } from './git-username' + +describe('isPlausibleHostedLogin', () => { + it('accepts ordinary GitHub logins', () => { + expect(isPlausibleHostedLogin('octocat')).toBe(true) + expect(isPlausibleHostedLogin('mona-lisa')).toBe(true) + expect(isPlausibleHostedLogin('a')).toBe(true) + expect(isPlausibleHostedLogin('a'.repeat(39))).toBe(true) + }) + + it('accepts Enterprise Managed User logins, which carry a _shortcode suffix', () => { + expect(isPlausibleHostedLogin('octocat_acme')).toBe(true) + expect(isPlausibleHostedLogin('mona-lisa_acme')).toBe(true) + expect(isPlausibleHostedLogin(`${'a'.repeat(34)}_acme`)).toBe(true) + }) + + it('still rejects leading or trailing separators, double hyphens and non-tokens', () => { + expect(isPlausibleHostedLogin('_acme')).toBe(false) + expect(isPlausibleHostedLogin('octocat_')).toBe(false) + expect(isPlausibleHostedLogin('-octocat')).toBe(false) + expect(isPlausibleHostedLogin('octocat-')).toBe(false) + expect(isPlausibleHostedLogin('octo--cat')).toBe(false) + expect(isPlausibleHostedLogin('{"message":"API rate limit exceeded"}')).toBe(false) + expect(isPlausibleHostedLogin('a'.repeat(40))).toBe(false) + for (const login of [ + 'octocat_acme/branch', + 'octocat_acme\\branch', + 'octocat_acme\nother', + 'octocat_acme.lock', + 'octocat_acme other' + ]) { + expect(isPlausibleHostedLogin(login)).toBe(false) + } + }) +}) diff --git a/src/main/git/git-username.ts b/src/main/git/git-username.ts index 88c7c5603c2..6f32fd59451 100644 --- a/src/main/git/git-username.ts +++ b/src/main/git/git-username.ts @@ -36,10 +36,10 @@ export function normalizeGitUsername(value: string): string { * (rate-limit 403 still prints JSON on stdout) so they never become branch names. */ export function isPlausibleHostedLogin(value: string): boolean { - // GitHub usernames: 1–39 chars, alphanumerics and single hyphens, no leading/trailing hyphen. + // Preserve GitHub's length/separator limits while allowing the EMU _shortcode suffix. return ( /^[A-Za-z0-9]$/.test(value) || - (/^[A-Za-z0-9][A-Za-z0-9-]{0,37}[A-Za-z0-9]$/.test(value) && !value.includes('--')) + (/^[A-Za-z0-9][A-Za-z0-9_-]{0,37}[A-Za-z0-9]$/.test(value) && !value.includes('--')) ) } @@ -158,7 +158,7 @@ function parseGhAuthStatusLogin(output: string): string { let currentLogin = '' let firstLogin = '' for (const line of output.split('\n')) { - const login = line.match(/Logged in to github\.com account\s+([A-Za-z0-9-]+)/)?.[1] + const login = line.match(/Logged in to github\.com account\s+([A-Za-z0-9][A-Za-z0-9_-]*)/)?.[1] if (login) { currentLogin = login if (!firstLogin) { diff --git a/src/main/git/repo-username.test.ts b/src/main/git/repo-username.test.ts index 9add077c9f8..617cf6d219e 100644 --- a/src/main/git/repo-username.test.ts +++ b/src/main/git/repo-username.test.ts @@ -167,15 +167,19 @@ describe('resolveLocalGitUsername', () => { await expect(resolveLocalGitUsername('/repo')).resolves.toBe('gh-demo') }) - it('uses GitHub CLI login for GitHub remotes instead of repo-local author identity', async () => { - originRemoteUrl = 'https://github.com/stablyai/orca.git' - gitConfig['user.email'] = 'demo@example.com' - gitConfig['user.name'] = 'Demo User' - ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'gh-demo\n', stderr: '' }) + it.each(['gh-demo', 'octocat_acme'])( + 'uses GitHub login %s instead of repo-local author identity', + async (login) => { + originRemoteUrl = 'https://github.com/stablyai/orca.git' + gitConfig['user.email'] = 'demo@example.com' + gitConfig['user.name'] = 'Demo User' + ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: `${login}\n`, stderr: '' }) - await expect(resolveLocalGitUsername('/repo')).resolves.toBe('gh-demo') - expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) - }) + await expect(resolveLocalGitUsername('/repo')).resolves.toBe(login) + await expect(resolveLocalGitUsername('/other-repo')).resolves.toBe(login) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) + } + ) it('uses GitHub CLI login for a single GitHub remote not named origin', async () => { remoteUrls.upstream = 'https://github.com/stablyai/orca.git' @@ -302,19 +306,22 @@ describe('resolveLocalGitUsername', () => { }) }) - it('uses auth status fallback after fast GitHub CLI API failure', async () => { - originRemoteUrl = 'https://github.com/stablyai/orca.git' - ghExecFileAsyncMock - .mockRejectedValueOnce(makeExecError('gh api unavailable')) - .mockResolvedValueOnce({ - stdout: '', - stderr: - 'github.com\n ✓ Logged in to github.com account demo-user\n - Active account: true\n' - }) + it.each(['demo-user', 'octocat_acme'])( + 'preserves the full login %s on the auth-status fallback', + async (login) => { + originRemoteUrl = 'https://github.com/stablyai/orca.git' + ghExecFileAsyncMock + .mockRejectedValueOnce(makeExecError('gh api unavailable')) + .mockResolvedValueOnce({ + stdout: '', + stderr: `github.com\n ✓ Logged in to github.com account ${login} (keyring)\n - Active account: true\n` + }) - await expect(resolveLocalGitUsername('/repo')).resolves.toBe('demo-user') - expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) - }) + await expect(resolveLocalGitUsername('/repo')).resolves.toBe(login) + await expect(resolveLocalGitUsername('/other-repo')).resolves.toBe(login) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) + } + ) it('settles within the wall even when the gh child never exits', async () => { vi.useFakeTimers() @@ -329,25 +336,27 @@ describe('resolveLocalGitUsername', () => { expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) }) - it('picks the active account from a multi-account auth status output', async () => { - // Why: each account block prints its login line BEFORE its - // "Active account" marker; a cross-block regex would capture the next - // block's login instead of the active one. - originRemoteUrl = 'https://github.com/stablyai/orca.git' - ghExecFileAsyncMock - .mockRejectedValueOnce(makeExecError('gh api unavailable')) - .mockResolvedValueOnce({ - stdout: '', - stderr: [ - 'github.com', - ' ✓ Logged in to github.com account active-user (keyring)', - ' - Active account: true', - ' - Git operations protocol: https', - ' ✓ Logged in to github.com account inactive-user (keyring)', - ' - Active account: false' - ].join('\n') - }) + it.each([ + { active: 'active-user', inactive: 'inactive-user', activeFirst: true }, + { active: 'octocat_acme', inactive: 'ordinary-user', activeFirst: true }, + { active: 'octocat_acme', inactive: 'ordinary-user', activeFirst: false }, + { active: 'ordinary-user', inactive: 'octocat_acme', activeFirst: false } + ])( + 'selects $active with activeFirst=$activeFirst from multiple accounts', + async ({ active, inactive, activeFirst }) => { + originRemoteUrl = 'https://github.com/stablyai/orca.git' + const accounts = [ + ` ✓ Logged in to github.com account ${active} (keyring)\n - Active account: true`, + ` ✓ Logged in to github.com account ${inactive} (keyring)\n - Active account: false` + ] + if (!activeFirst) { + accounts.reverse() + } + ghExecFileAsyncMock + .mockRejectedValueOnce(makeExecError('gh api unavailable')) + .mockResolvedValueOnce({ stdout: '', stderr: ['github.com', ...accounts].join('\n') }) - await expect(resolveLocalGitUsername('/repo')).resolves.toBe('active-user') - }) + await expect(resolveLocalGitUsername('/repo')).resolves.toBe(active) + } + ) }) diff --git a/src/main/github/github-api-repository-validation.test.ts b/src/main/github/github-api-repository-validation.test.ts new file mode 100644 index 00000000000..81e395a8bdb --- /dev/null +++ b/src/main/github/github-api-repository-validation.test.ts @@ -0,0 +1,25 @@ +// Why: owner/repo overrides become authenticated REST paths, so the slug gate +// must keep rejecting path-shaped input while accepting every real login shape — +// including Enterprise Managed User logins, which end in `_`. +import { describe, expect, it } from 'vitest' +import { isValidGitHubApiRepository } from './github-api-repository-validation' + +describe('isValidGitHubApiRepository', () => { + it('accepts plain and Enterprise Managed User owners', () => { + expect(isValidGitHubApiRepository({ owner: 'acme', repo: 'orca' })).toBe(true) + expect(isValidGitHubApiRepository({ owner: 'octocat_acme', repo: 'level5' })).toBe(true) + }) + + it('rejects leading underscore, hyphen, dot, and path-shaped owners', () => { + expect(isValidGitHubApiRepository({ owner: '_acme', repo: 'orca' })).toBe(false) + expect(isValidGitHubApiRepository({ owner: '-acme', repo: 'orca' })).toBe(false) + expect(isValidGitHubApiRepository({ owner: '.acme', repo: 'orca' })).toBe(false) + expect(isValidGitHubApiRepository({ owner: 'a/b', repo: 'orca' })).toBe(false) + }) + + it('rejects reserved and path-shaped repos', () => { + expect(isValidGitHubApiRepository({ owner: 'acme', repo: '.' })).toBe(false) + expect(isValidGitHubApiRepository({ owner: 'acme', repo: '..' })).toBe(false) + expect(isValidGitHubApiRepository({ owner: 'acme', repo: 'a/b' })).toBe(false) + }) +}) diff --git a/src/main/github/github-api-repository-validation.ts b/src/main/github/github-api-repository-validation.ts index e70197288cd..b687a0ebdee 100644 --- a/src/main/github/github-api-repository-validation.ts +++ b/src/main/github/github-api-repository-validation.ts @@ -1,3 +1,4 @@ +import { GITHUB_OWNER_SLUG_RE } from '../../shared/github/owner-slug' import type { GitHubOwnerRepo } from '../../shared/github/pull-request-types' export type GitHubApiRepositoryResolution = @@ -7,7 +8,7 @@ export type GitHubApiRepositoryResolution = | (() => Promise) // Why: renderer/RPC overrides reach authenticated REST paths. -const OWNER_SLUG_RE = /^[A-Za-z0-9][A-Za-z0-9-]*$/ +const OWNER_SLUG_RE = GITHUB_OWNER_SLUG_RE const REPOSITORY_SLUG_RE = /^[A-Za-z0-9._-]+$/ export function isValidGitHubApiRepository(repository: GitHubOwnerRepo): boolean { diff --git a/src/main/github/github-api-repository.test.ts b/src/main/github/github-api-repository.test.ts index ad09e2d38ec..b9fc0f159f2 100644 --- a/src/main/github/github-api-repository.test.ts +++ b/src/main/github/github-api-repository.test.ts @@ -136,17 +136,20 @@ describe('resolveGitHubRepoExecution', () => { expect(isGitHubHostAuthenticatedMock).not.toHaveBeenCalled() }) - it('normalizes github.com without spending an auth inventory probe', async () => { - await expect( - resolveGitHubApiRepository('/repo', { - owner: 'acme', - repo: 'widgets', - host: ' GitHub.COM ' - }) - ).resolves.toEqual({ owner: 'acme', repo: 'widgets', host: 'github.com' }) + it.each(['acme', 'octocat_acme'])( + 'normalizes github.com for %s without an auth inventory probe', + async (owner) => { + await expect( + resolveGitHubApiRepository('/repo', { + owner, + repo: 'widgets', + host: ' GitHub.COM ' + }) + ).resolves.toEqual({ owner, repo: 'widgets', host: 'github.com' }) - expect(isGitHubHostAuthenticatedMock).not.toHaveBeenCalled() - }) + expect(isGitHubHostAuthenticatedMock).not.toHaveBeenCalled() + } + ) it('backfills the origin host for a host-less caller-specific resolver', async () => { const ownerRepo = { owner: 'upstream', repo: 'widgets' } diff --git a/src/main/github/project-view-host-auth.test.ts b/src/main/github/project-view-host-auth.test.ts index f243c06ed45..7377b8e6063 100644 --- a/src/main/github/project-view-host-auth.test.ts +++ b/src/main/github/project-view-host-auth.test.ts @@ -93,33 +93,113 @@ describe('project view host authentication boundary', () => { expect(hostAuthenticatedMock).not.toHaveBeenCalled() }) - it('uses a pasted github.com URL instead of the ambient Enterprise host', async () => { - ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { - const query = args.find((arg) => arg.startsWith('query=')) ?? '' - return query.includes('projectV2') - ? { - stdout: JSON.stringify({ - data: { organization: { projectV2: { id: 'PVT_7', title: 'Roadmap' } } } - }), - stderr: '' - } - : { - stdout: JSON.stringify({ data: { organization: { login: 'acme' } } }), - stderr: '' - } - }) + it.each([ + { owner: 'acme-co', path: 'orgs', root: 'organization', ownerType: 'organization' }, + { owner: 'octocat', path: 'users', root: 'user', ownerType: 'user' }, + { owner: 'octocat_acme', path: 'users', root: 'user', ownerType: 'user' } + ])( + 'resolves $owner on github.com instead of the ambient Enterprise host', + async ({ owner, path, root, ownerType }) => { + ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { + const query = args.find((arg) => arg.startsWith('query=')) ?? '' + return query.includes('projectV2') + ? { + stdout: JSON.stringify({ + data: { [root]: { projectV2: { id: 'PVT_7', title: 'Roadmap' } } } + }), + stderr: '' + } + : { + stdout: JSON.stringify({ data: { [root]: { login: owner } } }), + stderr: '' + } + }) + await expect( + resolveProjectRef({ + input: `https://github.com/${path}/${owner}/projects/7/views/2`, + host: 'github.corp.example' + }) + ).resolves.toEqual({ + ok: true, + host: 'github.com', + owner, + ownerType, + number: 7, + viewNumber: 2, + title: 'Roadmap' + }) + + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) + expect( + ghExecFileAsyncMock.mock.calls.every(([, options]) => options.host === 'github.com') + ).toBe(true) + expect(hostAuthenticatedMock).not.toHaveBeenCalled() + for (const [args] of ghExecFileAsyncMock.mock.calls) { + expect(args).toContain(`owner=${owner}`) + expect(args).toContainEqual(expect.stringContaining(`${root}(login:$owner)`)) + } + } + ) + + it.each(['octocat', 'octocat_acme'])( + 'resolves user shorthand %s after an organization miss', + async (owner) => { + ghExecFileAsyncMock + .mockResolvedValueOnce({ stdout: '{"data":{"organization":null}}', stderr: '' }) + .mockResolvedValueOnce({ + stdout: JSON.stringify({ data: { user: { login: owner } } }), + stderr: '' + }) + .mockResolvedValueOnce({ + stdout: '{"data":{"user":{"projectV2":{"id":"PVT_7","title":"Roadmap"}}}}', + stderr: '' + }) + + await expect(resolveProjectRef({ input: `${owner}/7` })).resolves.toEqual({ + ok: true, + owner, + ownerType: 'user', + number: 7, + title: 'Roadmap', + host: 'github.com' + }) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(3) + const queries = ghExecFileAsyncMock.mock.calls.map(([args]) => + args.find((arg: string) => arg.startsWith('query=')) + ) + expect(queries[0]).toContain('organization(login:$owner)') + expect(queries[1]).toContain('user(login:$owner)') + expect(queries[2]).toContain('user(login:$owner)') + for (const [args, options] of ghExecFileAsyncMock.mock.calls) { + expect(args).toContain(`owner=${owner}`) + expect(options.host).toBe('github.com') + } + expect(hostAuthenticatedMock).not.toHaveBeenCalled() + } + ) + + it('rejects an unconfigured host even for a valid EMU owner', async () => { + hostAuthenticatedMock.mockResolvedValue(false) await expect( resolveProjectRef({ - input: 'https://github.com/orgs/acme/projects/7', - host: 'github.corp.example' + input: 'https://unconfigured.example/users/octocat_acme/projects/7', + host: 'unconfigured.example' }) - ).resolves.toMatchObject({ ok: true, host: 'github.com' }) - - expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) - expect( - ghExecFileAsyncMock.mock.calls.every(([, options]) => options.host === 'github.com') - ).toBe(true) - expect(hostAuthenticatedMock).not.toHaveBeenCalled() + ).resolves.toMatchObject({ ok: false, error: { type: 'auth_required' } }) + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + expect(acquireMock).not.toHaveBeenCalled() }) + + it.each(['_acme/7', 'https://github.com/users/a%2Fb/projects/7'])( + 'rejects malformed owner input %s before requesting GitHub', + async (input) => { + await expect(resolveProjectRef({ input })).resolves.toMatchObject({ + ok: false, + error: { type: 'validation_error' } + }) + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + expect(acquireMock).not.toHaveBeenCalled() + } + ) }) diff --git a/src/main/github/project-view.test.ts b/src/main/github/project-view.test.ts index 48486ae4a9e..cfa45726760 100644 --- a/src/main/github/project-view.test.ts +++ b/src/main/github/project-view.test.ts @@ -3,8 +3,8 @@ // resolve host" partially overlaps "could not resolve to a"), // (b) repo slug validation must accept names with leading underscore // (GitHub allows them, e.g. `_internal`), -// (c) owner slug validation must reject `.`/`_` (GitHub disallows them in -// usernames/orgs), +// (c) owner slug validation must reject `.` and a leading `_`/`-`, but accept +// the `_` suffix GitHub appends to Enterprise Managed User logins, // (d) parseProjectPaste shorthand owner-only alphabet matches the renderer, // (e) project owner/capability caches stay bounded in long sessions. import { beforeEach, describe, expect, it } from 'vitest' @@ -90,12 +90,13 @@ describe('isValidOwnerSlug', () => { expect(isValidOwnerSlug('user1')).toBe(true) }) - it('rejects underscore (GitHub disallows it in usernames/orgs)', () => { - expect(isValidOwnerSlug('_acme')).toBe(false) - expect(isValidOwnerSlug('acme_co')).toBe(false) + it('accepts Enterprise Managed User logins (GitHub appends `_`)', () => { + expect(isValidOwnerSlug('octocat_acme')).toBe(true) + expect(isValidOwnerSlug('acme_co')).toBe(true) }) - it('rejects leading hyphen and dot', () => { + it('rejects leading underscore, hyphen and dot', () => { + expect(isValidOwnerSlug('_acme')).toBe(false) expect(isValidOwnerSlug('-acme')).toBe(false) expect(isValidOwnerSlug('.acme')).toBe(false) }) @@ -138,10 +139,25 @@ describe('parseProjectPaste', () => { expect(parseProjectPaste('acme/42')).toEqual({ kind: 'bare', owner: 'acme', number: 42 }) }) - it('rejects shorthand with underscore in owner (renderer parity)', () => { - // Why: the renderer's parser uses `[A-Za-z0-9][A-Za-z0-9-]*` for owner - // (matches OWNER_SLUG_RE). Both sides must reject the same inputs. - expect(parseProjectPaste('co_op/45')).toBeNull() + it('accepts shorthand with an Enterprise Managed User owner (renderer parity)', () => { + // Why: the renderer's parser uses `[A-Za-z0-9][A-Za-z0-9_-]*` for owner + // (matches OWNER_SLUG_RE). Both sides must accept and reject the same inputs. + expect(parseProjectPaste('octocat_acme/1')).toEqual({ + kind: 'bare', + owner: 'octocat_acme', + number: 1 + }) + expect(parseProjectPaste('_acme/45')).toBeNull() + }) + + it('parses a user URL with an Enterprise Managed User owner', () => { + expect(parseProjectPaste('https://github.com/users/octocat_acme/projects/1/views/1')).toEqual({ + kind: 'user', + owner: 'octocat_acme', + number: 1, + host: 'github.com', + viewNumber: 1 + }) }) it('parses org URL with view number', () => { @@ -164,7 +180,8 @@ describe('parseProjectPaste', () => { }) it('rejects URLs whose owner has invalid characters', () => { - expect(parseProjectPaste('https://github.com/orgs/co_op/projects/1')).toBeNull() + expect(parseProjectPaste('https://github.com/orgs/_acme/projects/1')).toBeNull() + expect(parseProjectPaste('https://github.com/orgs/.acme/projects/1')).toBeNull() }) it('accepts enterprise-host URLs only when that host is provided (GHES)', () => { diff --git a/src/main/github/project-view/internals.ts b/src/main/github/project-view/internals.ts index 3bea5b5584d..d6088552c82 100644 --- a/src/main/github/project-view/internals.ts +++ b/src/main/github/project-view/internals.ts @@ -3,6 +3,7 @@ // every gh call through the runner gives us transient-5xx retry, WSL path // translation, and a single hook point for future quota tracking. import { acquire, release } from '../gh-utils' +import { isGitHubOwnerSlug } from '../../../shared/github/owner-slug' import { extractExecError, ghExecFileAsync } from '../../git/runner' import { repositoryRateLimitGuard, @@ -60,17 +61,16 @@ export async function projectHostAuthenticationError( // ─── Slug validation ────────────────────────────────────────────────── -// Why: GitHub usernames/org logins disallow `_`, `.`, leading `-`. Repo names -// are looser — they allow leading `_`, `.`, `-` (`.` and `..` reserved). We -// validate each separately so untrusted Project row data (`nameWithOwner`) -// can't become an arbitrary REST path while still accepting realistic repo -// names like `_internal` or `.github`. -const OWNER_SLUG_RE = /^[A-Za-z0-9][A-Za-z0-9-]*$/ +// Why: the owner check lives in shared/github/owner-slug (main, renderer and +// mobile all parse it). Repo names are looser — they allow leading `_`, `.`, `-` +// (`.` and `..` reserved). We validate each separately so untrusted Project row +// data (`nameWithOwner`) can't become an arbitrary REST path while still +// accepting realistic repo names like `_internal` or `.github`. const REPO_SLUG_RE = /^[A-Za-z0-9._-]+$/ const REPO_SLUG_RESERVED = new Set(['.', '..']) export function isValidOwnerSlug(value: unknown): value is string { - return typeof value === 'string' && value.length > 0 && OWNER_SLUG_RE.test(value) + return isGitHubOwnerSlug(value) } export function isValidRepoSlug(value: unknown): value is string { diff --git a/src/main/github/project-view/project-view-reference.ts b/src/main/github/project-view/project-view-reference.ts index 54542a3e720..831967e0ecf 100644 --- a/src/main/github/project-view/project-view-reference.ts +++ b/src/main/github/project-view/project-view-reference.ts @@ -1,4 +1,5 @@ import type { ResolveProjectRefArgs } from '../../../shared/github/project-request-types' +import { GITHUB_OWNER_NUMBER_SHORTHAND_RE } from '../../../shared/github/owner-slug' import type { GitHubProjectViewError, ResolveProjectRefResult @@ -66,9 +67,7 @@ export function parseProjectPaste(input: string, host?: string): ParsedPaste | n } catch { // Shorthand parsing below remains available for non-URL input. } - // owner/number shorthand — owner alphabet matches OWNER_SLUG_RE. - const shortRe = /^([A-Za-z0-9][A-Za-z0-9-]*)\/(\d+)$/ - const sm = trimmed.match(shortRe) + const sm = trimmed.match(GITHUB_OWNER_NUMBER_SHORTHAND_RE) if (sm) { const number = Number.parseInt(sm[2], 10) if (!Number.isInteger(number) || number < 1) { diff --git a/src/renderer/src/components/github-project/project-picker-input.test.ts b/src/renderer/src/components/github-project/project-picker-input.test.ts index 68ae3eb89eb..52cfcc5b947 100644 --- a/src/renderer/src/components/github-project/project-picker-input.test.ts +++ b/src/renderer/src/components/github-project/project-picker-input.test.ts @@ -17,6 +17,16 @@ describe('ProjectPicker project input', () => { expect(parseProjectInput('acme/7')).toEqual({ owner: 'acme', number: 7 }) }) + it('accepts Enterprise Managed User owners (`_` suffix) in URLs and shorthand', () => { + expect(parseProjectInput('https://github.com/users/octocat_acme/projects/1/views/1')).toEqual({ + owner: 'octocat_acme', + number: 1, + host: 'github.com', + viewNumber: 1 + }) + expect(parseProjectInput('octocat_acme/1')).toEqual({ owner: 'octocat_acme', number: 1 }) + }) + it('browses and reports auth errors against the active project host', () => { expect(getProjectPickerBrowseHost({ host: ' GitHub.Corp.Example:8443 ' })).toBe( 'github.corp.example:8443' @@ -30,7 +40,8 @@ describe('ProjectPicker project input', () => { 'https://github.corp.example/orgs/acme/projects/7evil', 'https://github.corp.example/orgs/acme/projects/7/views/2evil', 'https://github.corp.example/orgs/acme/projects/7/files', - 'https://github.corp.example/orgs/co_op/projects/7' + 'https://github.corp.example/orgs/_acme/projects/7', + '_acme/7' ]) { expect(parseProjectInput(input)).toBeNull() } diff --git a/src/renderer/src/components/github-project/project-picker-input.ts b/src/renderer/src/components/github-project/project-picker-input.ts index d670e814e8b..e6cd4457f8d 100644 --- a/src/renderer/src/components/github-project/project-picker-input.ts +++ b/src/renderer/src/components/github-project/project-picker-input.ts @@ -1,3 +1,7 @@ +import { + GITHUB_OWNER_NUMBER_SHORTHAND_RE, + GITHUB_OWNER_SLUG_RE +} from '../../../../shared/github/owner-slug' import { isGitHubProjectRefInputTooLarge } from '../../../../shared/github/project-ref-input' export function parseProjectInput( @@ -7,7 +11,7 @@ export function parseProjectInput( if (!trimmed || isGitHubProjectRefInputTooLarge(trimmed)) { return null } - const short = /^([A-Za-z0-9][A-Za-z0-9-]*)\/(\d+)$/.exec(trimmed) + const short = GITHUB_OWNER_NUMBER_SHORTHAND_RE.exec(trimmed) if (short) { const number = Number(short[2]) return Number.isSafeInteger(number) && number > 0 ? { owner: short[1], number } : null @@ -26,7 +30,7 @@ export function parseProjectInput( const hasView = parts.length === 6 && parts[4] === 'views' if ( (parts[0] === 'orgs' || parts[0] === 'users') && - /^[A-Za-z0-9][A-Za-z0-9-]*$/.test(parts[1] ?? '') && + GITHUB_OWNER_SLUG_RE.test(parts[1] ?? '') && parts[2] === 'projects' && (parts.length === 4 || hasView) ) { diff --git a/src/shared/github/owner-slug.test.ts b/src/shared/github/owner-slug.test.ts new file mode 100644 index 00000000000..d3a899d4d23 --- /dev/null +++ b/src/shared/github/owner-slug.test.ts @@ -0,0 +1,45 @@ +import { describe, expect, it } from 'vitest' + +import { + GITHUB_OWNER_NUMBER_SHORTHAND_RE, + GITHUB_OWNER_SLUG_RE, + isGitHubOwnerSlug +} from './owner-slug' + +describe('GitHub owner slug alphabet', () => { + it('accepts plain logins and Enterprise Managed User logins', () => { + for (const owner of ['acme', 'acme-co', 'user1', 'octocat_acme', 'acme_co']) { + expect(GITHUB_OWNER_SLUG_RE.test(owner)).toBe(true) + expect(isGitHubOwnerSlug(owner)).toBe(true) + } + }) + + it('rejects a leading underscore, hyphen or dot, and path-shaped values', () => { + for (const owner of [ + '_acme', + '-acme', + '.acme', + 'a/b', + 'a.b', + 'a_b/c', + 'a_b\\c', + 'a_b%2Fc', + 'a_b?c', + 'a_b#c', + '' + ]) { + expect(GITHUB_OWNER_SLUG_RE.test(owner)).toBe(false) + expect(isGitHubOwnerSlug(owner)).toBe(false) + } + expect(isGitHubOwnerSlug(123)).toBe(false) + }) + + it('shorthand captures the same owner alphabet plus a number', () => { + expect(GITHUB_OWNER_NUMBER_SHORTHAND_RE.exec('octocat_acme/12')?.slice(1)).toEqual([ + 'octocat_acme', + '12' + ]) + expect(GITHUB_OWNER_NUMBER_SHORTHAND_RE.test('_acme/12')).toBe(false) + expect(GITHUB_OWNER_NUMBER_SHORTHAND_RE.test('acme/x')).toBe(false) + }) +}) diff --git a/src/shared/github/owner-slug.ts b/src/shared/github/owner-slug.ts new file mode 100644 index 00000000000..a06827c8d99 --- /dev/null +++ b/src/shared/github/owner-slug.ts @@ -0,0 +1,14 @@ +// Why: GitHub owner logins are alphanumerics and single hyphens — plus, for +// Enterprise Managed Users, the `_` GitHub itself appends — so `_` is +// valid after the first character and never leading. One source for main, renderer +// and mobile: three drifting copies is how EMU logins got rejected (#20449). +const OWNER_SLUG_SOURCE = '[A-Za-z0-9][A-Za-z0-9_-]*' + +export const GITHUB_OWNER_SLUG_RE = new RegExp(`^${OWNER_SLUG_SOURCE}$`) + +/** `owner/123` shorthand — match[1] is the owner, match[2] the number. */ +export const GITHUB_OWNER_NUMBER_SHORTHAND_RE = new RegExp(`^(${OWNER_SLUG_SOURCE})\\/(\\d+)$`) + +export function isGitHubOwnerSlug(value: unknown): value is string { + return typeof value === 'string' && GITHUB_OWNER_SLUG_RE.test(value) +}