security(packages/core): harden auth middleware against insecure transport and injection (#38)

* chore: fix command export default issue

* security(packages/core): harden auth middleware against insecure transport and injection

Enforce HTTPS on OAuth endpoint URLs per RFC 8252 §8.3, with a
loopback exemption for local redirect flows. Escape cmd.exe
metacharacters in URLs on Windows to prevent command injection.
Remove redundant existsSync check in loadFromPath to eliminate
a TOCTOU race condition.

Co-Authored-By: Claude <noreply@anthropic.com>

* refactor(packages/core): move isSecureAuthUrl before openBrowser

Group URL validation exports together for better readability.

Co-Authored-By: Claude <noreply@anthropic.com>

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: zrosenbauer

.changeset/auth-security-hardening.md

@@ -0,0 +1,11 @@
+---
+'@kidd-cli/core': patch
+---
+
+Harden auth middleware against insecure transport and injection
+
+Enforce HTTPS on OAuth endpoint URLs (authUrl, tokenUrl, deviceAuthUrl) per RFC 8252 §8.3, allowing HTTP only for loopback addresses used during local redirect flows. Resolvers now return null for non-secure URLs.
+
+Escape `cmd.exe` metacharacters (`&`, `|`, `<`, `>`, `^`) in URLs passed to `cmd /c start` on Windows to prevent command injection via query strings.
+
+Remove redundant `existsSync` check in `loadFromPath` to eliminate a TOCTOU race condition, matching the pattern already used in the dotenv resolver.

packages/core/src/lib/store/create-store.ts

@@ -49,13 +49,12 @@ export function createStore<TData = unknown>(options: StoreOptions<TData>): File
    * @returns The file content, or null if the file does not exist or cannot be read.
    */
   function loadFromPath(filePath: string): string | null {
-    if (!existsSync(filePath)) {
-      return null
-    }
     const [error, content] = attempt(() => readFileSync(filePath, 'utf8'))
+
     if (error) {
       return null
     }
+
     return content
   }
 

packages/core/src/middleware/auth/oauth-server.test.ts

@@ -8,12 +8,22 @@ vi.mock(import('node:child_process'), () => ({
   execFile: vi.fn().mockReturnValue({ on: vi.fn() }),
 }))
 
+vi.mock(import('node:os'), async (importOriginal) => {
+  const original = await importOriginal()
+  return {
+    ...original,
+    platform: vi.fn().mockReturnValue(original.platform()),
+  }
+})
+
 import { execFile } from 'node:child_process'
+import { platform } from 'node:os'
 
 import {
   createDeferred,
   createTimeout,
   destroyServer,
+  isSecureAuthUrl,
   openBrowser,
   sendSuccessPage,
   startLocalServer,
@@ -136,6 +146,36 @@ describe('sendSuccessPage()', () => {
   })
 })
 
+describe('isSecureAuthUrl()', () => {
+  it('should accept HTTPS URLs', () => {
+    expect(isSecureAuthUrl('https://auth.example.com/authorize')).toBeTruthy()
+  })
+
+  it('should reject HTTP URLs on non-loopback hosts', () => {
+    expect(isSecureAuthUrl('http://auth.example.com/authorize')).toBeFalsy()
+  })
+
+  it('should accept HTTP on 127.0.0.1', () => {
+    expect(isSecureAuthUrl('http://127.0.0.1:8080/callback')).toBeTruthy()
+  })
+
+  it('should accept HTTP on localhost', () => {
+    expect(isSecureAuthUrl('http://localhost:3000/callback')).toBeTruthy()
+  })
+
+  it('should accept HTTP on [::1]', () => {
+    expect(isSecureAuthUrl('http://[::1]:8080/callback')).toBeTruthy()
+  })
+
+  it('should reject non-HTTP schemes', () => {
+    expect(isSecureAuthUrl('ftp://auth.example.com/authorize')).toBeFalsy()
+  })
+
+  it('should reject invalid URLs', () => {
+    expect(isSecureAuthUrl('not a url')).toBeFalsy()
+  })
+})
+
 describe('openBrowser()', () => {
   afterEach(() => {
     vi.clearAllMocks()
@@ -148,6 +188,17 @@ describe('openBrowser()', () => {
     const [, args] = vi.mocked(execFile).mock.calls[0]
     expect(args).toContain('https://example.com')
   })
+
+  it('should escape cmd metacharacters in URLs on Windows', () => {
+    vi.mocked(platform).mockReturnValue('win32')
+
+    openBrowser('https://example.com/auth?a=1&b=2')
+
+    expect(vi.mocked(execFile)).toHaveBeenCalled()
+    const [command, args] = vi.mocked(execFile).mock.calls[0]
+    expect(command).toBe('cmd')
+    expect(args).toContain('https://example.com/auth?a=1^&b=2')
+  })
 })
 
 describe('startLocalServer()', () => {

packages/core/src/middleware/auth/oauth-server.ts

@@ -160,6 +160,33 @@ export function sendSuccessPage(res: ServerResponse): void {
   res.end(CLOSE_PAGE_HTML)
 }
 
+/**
+ * Check whether a URL is safe for use as an OAuth endpoint.
+ *
+ * Requires HTTPS for all URLs except loopback addresses, where
+ * HTTP is permitted per RFC 8252 §8.3 (native app redirect URIs).
+ *
+ * @param url - The URL string to validate.
+ * @returns True when the URL uses HTTPS or HTTP on a loopback address.
+ */
+export function isSecureAuthUrl(url: string): boolean {
+  try {
+    const parsed = new URL(url)
+
+    if (parsed.protocol === 'https:') {
+      return true
+    }
+
+    if (parsed.protocol !== 'http:') {
+      return false
+    }
+
+    return isLoopbackHost(parsed.hostname)
+  } catch {
+    return false
+  }
+}
+
 /**
  * Open a URL in the user's default browser using a platform-specific command.
  *
@@ -180,7 +207,7 @@ export function openBrowser(url: string): void {
 
   const { command, args } = match(platform())
     .with('darwin', () => ({ args: [url], command: 'open' }))
-    .with('win32', () => ({ args: ['/c', 'start', '', url], command: 'cmd' }))
+    .with('win32', () => ({ args: ['/c', 'start', '', escapeCmdMeta(url)], command: 'cmd' }))
     .otherwise(() => ({ args: [url], command: 'xdg-open' }))
 
   const child = execFile(command, args)
@@ -256,3 +283,32 @@ function isHttpUrl(url: string): boolean {
     return false
   }
 }
+
+/**
+ * Check whether a hostname is a loopback address.
+ *
+ * RFC 8252 §8.3 permits HTTP for loopback interfaces during
+ * native app authorization flows.
+ *
+ * @private
+ * @param hostname - The hostname to check.
+ * @returns True when the hostname is a loopback address.
+ */
+function isLoopbackHost(hostname: string): boolean {
+  return hostname === '127.0.0.1' || hostname === '[::1]' || hostname === 'localhost'
+}
+
+/**
+ * Escape `cmd.exe` metacharacters in a URL string.
+ *
+ * Characters like `&`, `|`, `<`, `>`, and `^` are interpreted as
+ * command separators or redirectors by `cmd.exe`. Prefixing each
+ * with `^` neutralises the special meaning.
+ *
+ * @private
+ * @param url - The URL to escape.
+ * @returns The escaped URL string.
+ */
+function escapeCmdMeta(url: string): string {
+  return url.replaceAll(/[&|<>^]/g, '^$&')
+}

packages/core/src/middleware/auth/strategies/device-code.test.ts

@@ -57,6 +57,28 @@ describe('resolveFromDeviceCode()', () => {
     vi.clearAllMocks()
   })
 
+  it('should return null when deviceAuthUrl uses HTTP', async () => {
+    const prompts = createMockPrompts()
+
+    const result = await resolveFromDeviceCode({
+      ...createDefaultOptions(prompts),
+      deviceAuthUrl: 'http://auth.example.com/device/code',
+    })
+
+    expect(result).toBeNull()
+  })
+
+  it('should return null when tokenUrl uses HTTP', async () => {
+    const prompts = createMockPrompts()
+
+    const result = await resolveFromDeviceCode({
+      ...createDefaultOptions(prompts),
+      tokenUrl: 'http://auth.example.com/token',
+    })
+
+    expect(result).toBeNull()
+  })
+
   it('should return null when device auth request fails', async () => {
     vi.spyOn(globalThis, 'fetch').mockRejectedValue(new Error('network error'))
 

packages/core/src/middleware/auth/strategies/device-code.ts

@@ -13,7 +13,7 @@ import { match } from 'ts-pattern'
 import type { Prompts } from '@/context/types.js'
 
 import { createBearerCredential, postFormEncoded } from '../credential.js'
-import { openBrowser } from '../oauth-server.js'
+import { isSecureAuthUrl, openBrowser } from '../oauth-server.js'
 import type { AuthCredential } from '../types.js'
 
 /**
@@ -47,6 +47,14 @@ export async function resolveFromDeviceCode(options: {
   readonly prompts: Prompts
   readonly openBrowserOnStart?: boolean
 }): Promise<AuthCredential | null> {
+  if (!isSecureAuthUrl(options.deviceAuthUrl)) {
+    return null
+  }
+
+  if (!isSecureAuthUrl(options.tokenUrl)) {
+    return null
+  }
+
   const deadline = Date.now() + options.timeout
   const signal = AbortSignal.timeout(options.timeout)
 

packages/core/src/middleware/auth/strategies/oauth.test.ts

@@ -114,6 +114,34 @@ describe('resolveFromOAuth()', () => {
     vi.restoreAllMocks()
   })
 
+  it('should return null when authUrl uses HTTP', async () => {
+    const result = await resolveFromOAuth({
+      authUrl: 'http://auth.example.com/authorize',
+      callbackPath: '/callback',
+      clientId: 'test-client',
+      port: 0,
+      scopes: [],
+      timeout: 5000,
+      tokenUrl: 'https://auth.example.com/token',
+    })
+
+    expect(result).toBeNull()
+  })
+
+  it('should return null when tokenUrl uses HTTP', async () => {
+    const result = await resolveFromOAuth({
+      authUrl: 'https://auth.example.com/authorize',
+      callbackPath: '/callback',
+      clientId: 'test-client',
+      port: 0,
+      scopes: [],
+      timeout: 5000,
+      tokenUrl: 'http://auth.example.com/token',
+    })
+
+    expect(result).toBeNull()
+  })
+
   it('should return null when the timeout fires before a code arrives', async () => {
     const fetchSpy = vi.spyOn(globalThis, 'fetch')
 

packages/core/src/middleware/auth/strategies/oauth.ts

@@ -16,6 +16,7 @@ import {
   createDeferred,
   createTimeout,
   destroyServer,
+  isSecureAuthUrl,
   openBrowser,
   sendSuccessPage,
   startLocalServer,
@@ -44,6 +45,14 @@ export async function resolveFromOAuth(options: {
   readonly callbackPath: string
   readonly timeout: number
 }): Promise<AuthCredential | null> {
+  if (!isSecureAuthUrl(options.authUrl)) {
+    return null
+  }
+
+  if (!isSecureAuthUrl(options.tokenUrl)) {
+    return null
+  }
+
   const codeVerifier = generateCodeVerifier()
   const codeChallenge = deriveCodeChallenge(codeVerifier)
   const state = randomBytes(32).toString('hex')