From 5970e70e2641e7d77b6f77d9624cd6990a1b81ba Mon Sep 17 00:00:00 2001
From: kaitranntt <kaitran.ntt@gmail.com>
Date: Tue, 6 Jan 2026 13:00:50 -0500
Subject: [PATCH] fix(cliproxy): harden nickname validation and race condition
 handling

- Trim nickname in API route for consistency with CLI
- Block URL-unsafe chars (%, /, &, ?, #) in nickname
- Block reserved patterns (kiro-N, ghcp-N) used by auto-discovery
- Add reload-merge pattern in discovery to reduce race condition
---
 src/cliproxy/account-manager.ts               | 35 +++++++++++++++++--
 src/web-server/routes/cliproxy-auth-routes.ts |  4 ++-
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/src/cliproxy/account-manager.ts b/src/cliproxy/account-manager.ts
index 2fced28c..393b3c0a 100644
--- a/src/cliproxy/account-manager.ts
+++ b/src/cliproxy/account-manager.ts
@@ -95,7 +95,7 @@ export function generateNickname(email?: string): string {
 
 /**
  * Validate nickname
- * Rules: 1-50 chars, any non-whitespace allowed (permissive)
+ * Rules: 1-50 chars, no whitespace, URL-safe, no reserved patterns
  * @returns null if valid, error message if invalid
  */
 export function validateNickname(nickname: string): string | null {
@@ -108,6 +108,14 @@ export function validateNickname(nickname: string): string | null {
   if (/\s/.test(nickname)) {
     return 'Nickname cannot contain whitespace';
   }
+  // Block URL-unsafe chars that break routing
+  if (/[%\/&?#]/.test(nickname)) {
+    return 'Nickname cannot contain special URL characters (%, /, &, ?, #)';
+  }
+  // Block reserved patterns used by auto-discovery (kiro-1, ghcp-2, etc.)
+  if (/^(kiro|ghcp)-\d+$/i.test(nickname)) {
+    return 'Nickname cannot match reserved pattern (kiro-N, ghcp-N)';
+  }
   return null;
 }
 
@@ -597,7 +605,30 @@ export function discoverExistingAccounts(): void {
     }
   }
 
-  saveAccountsRegistry(registry);
+  // Reload-merge pattern: reduce race condition with concurrent OAuth registration
+  // Reload fresh registry and merge discovered accounts (fresh registry wins on conflicts)
+  const freshRegistry = loadAccountsRegistry();
+  for (const [providerName, discovered] of Object.entries(registry.providers)) {
+    if (!discovered) continue;
+    const prov = providerName as CLIProxyProvider;
+    if (!freshRegistry.providers[prov]) {
+      freshRegistry.providers[prov] = discovered;
+    } else {
+      // Merge accounts, preferring fresh registry's existing entries
+      const freshProviderAccounts = freshRegistry.providers[prov];
+      if (!freshProviderAccounts) continue;
+      for (const [id, meta] of Object.entries(discovered.accounts)) {
+        if (!freshProviderAccounts.accounts[id]) {
+          freshProviderAccounts.accounts[id] = meta;
+          // Set default if none exists
+          if (!freshProviderAccounts.default || freshProviderAccounts.default === 'default') {
+            freshProviderAccounts.default = id;
+          }
+        }
+      }
+    }
+  }
+  saveAccountsRegistry(freshRegistry);
 }
 
 /**
diff --git a/src/web-server/routes/cliproxy-auth-routes.ts b/src/web-server/routes/cliproxy-auth-routes.ts
index 306b30ac..e7570a8f 100644
--- a/src/web-server/routes/cliproxy-auth-routes.ts
+++ b/src/web-server/routes/cliproxy-auth-routes.ts
@@ -277,7 +277,9 @@ router.delete('/accounts/:provider/:accountId', (req: Request, res: Response): v
  */
 router.post('/:provider/start', async (req: Request, res: Response): Promise<void> => {
   const { provider } = req.params;
-  const { nickname, noIncognito: noIncognitoBody } = req.body;
+  const { nickname: nicknameRaw, noIncognito: noIncognitoBody } = req.body;
+  // Trim nickname for consistency with CLI (oauth-handler.ts trims input)
+  const nickname = typeof nicknameRaw === 'string' ? nicknameRaw.trim() : nicknameRaw;
 
   // Validate provider
   if (!validProviders.includes(provider as CLIProxyProvider)) {