From 50506a7cc407f26ecddf669cbb0dda0961dfe52c Mon Sep 17 00:00:00 2001 From: Antonio Zaitoun Date: Mon, 3 Aug 2026 00:24:10 +0300 Subject: [PATCH] Address PR #161 Code Quality and Qodo review findings. Wrap symlink stats inside the friendly error handler, allow IPv6 loopback CORS, relax PAT host validation for localhost/IPs/ports, and drop unused imports. Co-authored-by: Cursor --- .../commands/__tests__/auth-command.test.ts | 43 ++++++++++++- src/cli/commands/add-builders.ts | 2 +- src/cli/commands/auth.ts | 63 +++++++++++++++++-- src/cli/utils/wrap/symlink-workspace.ts | 2 +- src/cli/utils/wrap/watch-project.ts | 2 +- src/server/__tests__/cors-origin.test.ts | 40 ++++++++++++ src/server/configure-routes.ts | 1 - src/server/cors-origin.ts | 11 +++- src/server/mcp-meta-routes.ts | 6 +- 9 files changed, 155 insertions(+), 15 deletions(-) create mode 100644 src/server/__tests__/cors-origin.test.ts diff --git a/src/cli/commands/__tests__/auth-command.test.ts b/src/cli/commands/__tests__/auth-command.test.ts index 2635a12..717ee6c 100644 --- a/src/cli/commands/__tests__/auth-command.test.ts +++ b/src/cli/commands/__tests__/auth-command.test.ts @@ -16,7 +16,7 @@ mock.module('../../utils/server-manager', () => ({ restartServer: mock(async () => {}), })); -const { authCommand } = await import('../auth'); +const { authCommand, normalizeProviderHost } = await import('../auth'); function isolateHome(): { restore: () => void } { const home = mkdtempSync(join(tmpdir(), 'capa-auth-home-')); @@ -142,6 +142,25 @@ describe('authCommand', () => { exitSpy.mockRestore(); }); + describe('normalizeProviderHost', () => { + it('accepts DNS hosts, localhost, IPs, and optional ports', () => { + expect(normalizeProviderHost('github.com')).toBe('github.com'); + expect(normalizeProviderHost('git.corp.com:8443')).toBe('git.corp.com:8443'); + expect(normalizeProviderHost('localhost')).toBe('localhost'); + expect(normalizeProviderHost('localhost:3000')).toBe('localhost:3000'); + expect(normalizeProviderHost('192.168.1.10')).toBe('192.168.1.10'); + expect(normalizeProviderHost('192.168.1.10:8080')).toBe('192.168.1.10:8080'); + expect(normalizeProviderHost('::1')).toBe('[::1]'); + expect(normalizeProviderHost('[::1]:8443')).toBe('[::1]:8443'); + }); + + it('rejects schemes and path segments', () => { + expect(normalizeProviderHost('https://github.com')).toBeNull(); + expect(normalizeProviderHost('github.com/org')).toBeNull(); + expect(normalizeProviderHost('')).toBeNull(); + }); + }); + describe('access-token path', () => { const originalFetch = globalThis.fetch; let fetchOk = true; @@ -203,6 +222,28 @@ describe('authCommand', () => { } }); + it('stores a self-hosted token for localhost with a port', async () => { + const { stdout } = await captureOutput(() => + authCommand('localhost:8443', { + accessToken: 'ghe_local', + type: 'github-enterprise', + }), + ); + + expect(stdout).toContain('Authenticated with localhost:8443 using access token'); + + const { loadSettings, getDatabasePath } = await import('../../../shared/config'); + const { CapaDatabase } = await import('../../../db/database'); + const settings = await loadSettings(); + const db = new CapaDatabase(getDatabasePath(settings)); + try { + const stored = db.getGitIntegration('github-enterprise', 'localhost:8443'); + expect(stored?.access_token).toBe('ghe_local'); + } finally { + db.close(); + } + }); + it('requires --type for unknown self-hosted hosts', async () => { const exitSpy = spyOn(process, 'exit').mockImplementation((() => {}) as typeof process.exit); const { stdout } = await captureOutput(() => diff --git a/src/cli/commands/add-builders.ts b/src/cli/commands/add-builders.ts index 17f23ea..bf25ed4 100644 --- a/src/cli/commands/add-builders.ts +++ b/src/cli/commands/add-builders.ts @@ -3,7 +3,7 @@ * Exported for unit tests. */ -import { basename, resolve, relative, join } from 'path'; +import { basename, resolve, relative } from 'path'; import { access } from 'fs/promises'; import { constants } from 'fs'; import { CANONICAL_HOOK_EVENTS, type CanonicalHookEvent, type Hook, type HookSource } from '../../types/hooks'; diff --git a/src/cli/commands/auth.ts b/src/cli/commands/auth.ts index a84b936..7889665 100644 --- a/src/cli/commands/auth.ts +++ b/src/cli/commands/auth.ts @@ -1,3 +1,4 @@ +import { isIP } from 'net'; import { loadSettings, getDatabasePath } from '../../shared/config'; import { CapaDatabase } from '../../db/database'; import { ensureServer } from '../utils/server-manager'; @@ -240,9 +241,12 @@ async function authenticateWithAccessToken( return; } - if (!isValidProvider(provider)) { + const normalizedHost = normalizeProviderHost(provider); + if (!normalizedHost) { error(`Invalid provider: ${provider}`); - error('Provider must be a domain name (e.g., github.com, gitlab.com)'); + error( + 'Provider must be a host (e.g., github.com, localhost:8443, 192.168.1.10). Do not include https://', + ); db.close(); process.exit(1); return; @@ -252,7 +256,7 @@ async function authenticateWithAccessToken( let host: string | undefined; try { - ({ platform, host } = resolveTokenAuthTarget(provider, options.type)); + ({ platform, host } = resolveTokenAuthTarget(normalizedHost, options.type)); } catch (err: unknown) { const message = err instanceof Error ? err.message : String(err); error(message); @@ -261,7 +265,7 @@ async function authenticateWithAccessToken( return; } - const displayHost = host ?? provider; + const displayHost = host ?? normalizedHost; info(`Authenticating with access token: ${displayHost}`); try { @@ -407,6 +411,57 @@ function isValidProvider(provider: string): boolean { return /^[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,}$/i.test(provider); } +/** + * Normalize a PAT/self-hosted host argument to `host` or `host:port`. + * Accepts DNS names, localhost, IPv4/IPv6 literals, and optional ports. + * Rejects schemes and path segments. Returns null when invalid. + */ +export function normalizeProviderHost(provider: string): string | null { + const trimmed = provider.trim(); + if (!trimmed || /\s/.test(trimmed) || trimmed.includes('://') || trimmed.includes('/')) { + return null; + } + + // Bare IPv6 (e.g. ::1) is not a valid URL authority; try the bracketed form first. + const candidates = + isIP(trimmed) === 6 + ? [`[${trimmed}]`] + : [trimmed]; + + for (const candidate of candidates) { + try { + const url = new URL(`https://${candidate}`); + const hostname = stripIpv6Brackets(url.hostname); + if (!hostname) continue; + + const ipVersion = isIP(hostname); + const isLoopback = + hostname === 'localhost' || hostname === '127.0.0.1' || hostname === '::1'; + const isDnsHost = + /^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$/i.test( + hostname, + ); + + if (!isLoopback && ipVersion === 0 && !isDnsHost) { + continue; + } + + const hostPart = ipVersion === 6 ? `[${hostname}]` : hostname; + return url.port ? `${hostPart}:${url.port}` : hostPart; + } catch { + // try next candidate + } + } + + return null; +} + +function stripIpv6Brackets(hostname: string): string { + return hostname.startsWith('[') && hostname.endsWith(']') + ? hostname.slice(1, -1) + : hostname; +} + async function pollForCompletion( db: CapaDatabase, provider: string, diff --git a/src/cli/utils/wrap/symlink-workspace.ts b/src/cli/utils/wrap/symlink-workspace.ts index 5754138..023326c 100644 --- a/src/cli/utils/wrap/symlink-workspace.ts +++ b/src/cli/utils/wrap/symlink-workspace.ts @@ -54,8 +54,8 @@ function symlinkErrorMessage(err: unknown): string { * from `linkPath` to `targetPath`. */ export function createWorkspaceSymlink(targetPath: string, linkPath: string): void { - const targetIsDir = existsSync(targetPath) && statSync(targetPath).isDirectory(); try { + const targetIsDir = existsSync(targetPath) && statSync(targetPath).isDirectory(); if (isWin) { if (targetIsDir) { symlinkSync(targetPath, linkPath, 'junction'); diff --git a/src/cli/utils/wrap/watch-project.ts b/src/cli/utils/wrap/watch-project.ts index 9dde564..f0034b7 100644 --- a/src/cli/utils/wrap/watch-project.ts +++ b/src/cli/utils/wrap/watch-project.ts @@ -1,4 +1,4 @@ -import { watch, type FSWatcher, existsSync, lstatSync, readdirSync } from 'fs'; +import { watch, type FSWatcher, lstatSync, readdirSync } from 'fs'; import { join, resolve, basename } from 'path'; import { createWorkspaceSymlink, diff --git a/src/server/__tests__/cors-origin.test.ts b/src/server/__tests__/cors-origin.test.ts new file mode 100644 index 0000000..71db135 --- /dev/null +++ b/src/server/__tests__/cors-origin.test.ts @@ -0,0 +1,40 @@ +import { describe, it, expect, afterEach } from 'bun:test'; +import { isAllowedOrigin } from '../cors-origin'; + +describe('isAllowedOrigin', () => { + const prev = process.env.CAPA_ALLOWED_ORIGINS; + + afterEach(() => { + if (prev === undefined) delete process.env.CAPA_ALLOWED_ORIGINS; + else process.env.CAPA_ALLOWED_ORIGINS = prev; + }); + + it('allows http localhost and IPv4 loopback', () => { + expect(isAllowedOrigin('http://localhost:5173')).toEqual({ + allowed: true, + origin: 'http://localhost:5173', + }); + expect(isAllowedOrigin('http://127.0.0.1:5912')).toEqual({ + allowed: true, + origin: 'http://127.0.0.1:5912', + }); + }); + + it('allows http IPv6 loopback', () => { + expect(isAllowedOrigin('http://[::1]:5173')).toEqual({ + allowed: true, + origin: 'http://[::1]:5173', + }); + }); + + it('rejects non-loopback origins unless listed in CAPA_ALLOWED_ORIGINS', () => { + delete process.env.CAPA_ALLOWED_ORIGINS; + expect(isAllowedOrigin('http://example.com')).toEqual({ allowed: false }); + + process.env.CAPA_ALLOWED_ORIGINS = 'https://app.example.com'; + expect(isAllowedOrigin('https://app.example.com')).toEqual({ + allowed: true, + origin: 'https://app.example.com', + }); + }); +}); diff --git a/src/server/configure-routes.ts b/src/server/configure-routes.ts index 13a0626..8a40c02 100644 --- a/src/server/configure-routes.ts +++ b/src/server/configure-routes.ts @@ -1,5 +1,4 @@ import type { CapaDatabase } from "../db/database"; -import { parseCapabilitiesFile } from "../shared/capabilities"; import { logger } from "../shared/logger"; import { detectCapabilitiesFile } from "../shared/paths"; import { projectUiUrl } from "../shared/ui-urls"; diff --git a/src/server/cors-origin.ts b/src/server/cors-origin.ts index 47f4f9f..fd2e016 100644 --- a/src/server/cors-origin.ts +++ b/src/server/cors-origin.ts @@ -8,9 +8,12 @@ export function isAllowedOrigin(origin: string | null): { try { const parsed = new URL(origin); + const hostname = stripIpv6Brackets(parsed.hostname); if ( parsed.protocol === "http:" && - (parsed.hostname === "localhost" || parsed.hostname === "127.0.0.1") + (hostname === "localhost" || + hostname === "127.0.0.1" || + hostname === "::1") ) { return { allowed: true, origin }; } @@ -28,3 +31,9 @@ export function isAllowedOrigin(origin: string | null): { return { allowed: false }; } + +function stripIpv6Brackets(hostname: string): string { + return hostname.startsWith("[") && hostname.endsWith("]") + ? hostname.slice(1, -1) + : hostname; +} diff --git a/src/server/mcp-meta-routes.ts b/src/server/mcp-meta-routes.ts index f5b5e9a..9160701 100644 --- a/src/server/mcp-meta-routes.ts +++ b/src/server/mcp-meta-routes.ts @@ -4,11 +4,7 @@ import { detectCapabilitiesFile } from "../shared/paths"; import type { Capabilities } from "../types/capabilities"; import type { CapaMCPServer, ShellToolInfo } from "./mcp-handler"; import type { SessionManager } from "./session-manager"; -import { - resolveSkillContentById, - resolveSkillDescription, - resolveSkillSourceUrl, -} from "./skill-content"; +import { resolveSkillContentById } from "./skill-content"; const JSON_HEADERS = { "Content-Type": "application/json" };