fix: [AI-7520] harden CLI gateway loopback (PR review majors)

saravmajestic · claude · saravmajestic · commit e46fdd48b44e · 2026-07-16T12:53:13.000+05:30
Address sahrizvi's review on #1001: - Register the pending flow in a state-keyed Map synchronously in authorize() BEFORE opening the browser, and have callback() await that promise — an already-signed-in user's instant redirect is no longer dropped as CSRF, and concurrent /auth flows no longer clobber each other - Bind the callback server to 127.0.0.1 (was all-interfaces / LAN-reachable) - Validate `state` BEFORE honoring the `?error=` branch, so an unauthenticated request can't cancel an in-progress sign-in - HTML-escape reflected values in the callback error page (localhost XSS) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
diff --git a/packages/opencode/src/altimate/plugin/altimate.ts b/packages/opencode/src/altimate/plugin/altimate.ts
@@ -5,8 +5,8 @@ import open from "open"
 import { AltimateApi } from "../api/client"
 
 // Loopback port the CLI listens on for the browser to deliver the gateway
-// credential after Google sign-in. Must match the redirect the web authorize
-// page posts back to. 7317 is otherwise unused in this codebase.
+// credential after sign-in. Must match the redirect the web authorize page posts
+// back to. 7317 is otherwise unused in this codebase.
 const CALLBACK_PORT = 7317
 
 // Web app that hosts the signup/login (authorize) page. Overridable for
@@ -15,14 +15,23 @@ const DEFAULT_WEB_URL = "https://app.myaltimate.com"
 // Fallback gateway API base if the callback omits one.
 const DEFAULT_API_URL = "https://api.myaltimate.com"
 
+// Escape reflected values before interpolating them into the callback HTML — the
+// error text originates from the URL query string, so it must not be trusted.
+function escapeHtml(s: string): string {
+  return s.replace(
+    /[&<>"']/g,
+    (c) => ({ "&": "&amp;", "<": "&lt;", ">": "&gt;", '"': "&quot;", "'": "&#39;" })[c] as string,
+  )
+}
+
 const HTML_SUCCESS = `<!doctype html><meta charset="utf-8"><title>Altimate Code</title>
 <body style="font-family:system-ui;text-align:center;padding:64px">
 <h2>Signed in ✓</h2><p>You can return to your terminal.</p>
 <script>setTimeout(()=>window.close(),1500)</script></body>`
 
 const HTML_ERROR = (msg: string) => `<!doctype html><meta charset="utf-8"><title>Altimate Code</title>
 <body style="font-family:system-ui;text-align:center;padding:64px">
-<h2>Connection failed</h2><p>${msg}</p><p>Please return to your terminal and try again.</p></body>`
+<h2>Connection failed</h2><p>${escapeHtml(msg)}</p><p>Please return to your terminal and try again.</p></body>`
 
 interface CallbackResult {
   api_url: string
@@ -31,13 +40,16 @@ interface CallbackResult {
 }
 
 interface Pending {
-  state: string
   resolve: (creds: CallbackResult) => void
   reject: (err: Error) => void
 }
 
 let server: ReturnType<typeof createServer> | undefined
-let pending: Pending | undefined
+// Pending flows keyed by the unguessable `state`. Registered synchronously in
+// authorize() BEFORE the browser opens, so an instant redirect (an already
+// signed-in user) is matched instead of dropped; keying by state also lets two
+// concurrent /auth flows coexist without clobbering each other.
+const pending = new Map<string, Pending>()
 
 async function startCallbackServer(): Promise<void> {
   if (server) return
@@ -54,22 +66,20 @@ async function startCallbackServer(): Promise<void> {
       res.end(body)
     }
 
-    const error = url.searchParams.get("error")
-    if (error) {
-      pending?.reject(new Error(error))
-      pending = undefined
-      html(200, HTML_ERROR(error))
+    // Validate `state` FIRST — before honoring `error` — so a request without a
+    // known state can neither cancel an in-progress flow nor deliver anything.
+    const state = url.searchParams.get("state")
+    const entry = state ? pending.get(state) : undefined
+    if (!state || !entry) {
+      html(400, HTML_ERROR("Invalid or unknown sign-in state"))
       return
     }
+    pending.delete(state)
 
-    const state = url.searchParams.get("state")
-    // Bind the callback to the unguessable state the CLI generated — rejects a
-    // stray/malicious local request that didn't originate from our browser tab.
-    if (!pending || !state || state !== pending.state) {
-      const msg = "Invalid state — possible CSRF"
-      pending?.reject(new Error(msg))
-      pending = undefined
-      html(400, HTML_ERROR(msg))
+    const error = url.searchParams.get("error")
+    if (error) {
+      entry.reject(new Error(error))
+      html(200, HTML_ERROR(error))
       return
     }
 
@@ -78,20 +88,19 @@ async function startCallbackServer(): Promise<void> {
     const apiUrl = url.searchParams.get("url") || DEFAULT_API_URL
     if (!apiKey || !instance) {
       const msg = "Missing credential in callback"
-      pending.reject(new Error(msg))
-      pending = undefined
+      entry.reject(new Error(msg))
       html(400, HTML_ERROR(msg))
       return
     }
 
-    const current = pending
-    pending = undefined
-    current.resolve({ api_url: apiUrl, instance, api_key: apiKey })
+    entry.resolve({ api_url: apiUrl, instance, api_key: apiKey })
     html(200, HTML_SUCCESS)
   })
 
   await new Promise<void>((resolve, reject) => {
-    server!.listen(CALLBACK_PORT, () => resolve())
+    // Bind to loopback only — the credential/abort endpoints must not be reachable
+    // from the LAN.
+    server!.listen(CALLBACK_PORT, "127.0.0.1", () => resolve())
     server!.on("error", reject)
   })
 }
@@ -103,16 +112,15 @@ function stopCallbackServer() {
   }
 }
 
-function waitForCallback(state: string, timeoutMs = 5 * 60 * 1000): Promise<CallbackResult> {
+// Register a pending flow keyed by `state` and return its promise. Called
+// synchronously in authorize() before the browser opens; the server handler
+// resolves/rejects it by state, so a fast redirect is never lost.
+function registerPending(state: string, timeoutMs = 5 * 60 * 1000): Promise<CallbackResult> {
   return new Promise<CallbackResult>((resolve, reject) => {
     const timeout = setTimeout(() => {
-      if (pending) {
-        pending = undefined
-        reject(new Error("Timed out waiting for browser sign-in"))
-      }
+      if (pending.delete(state)) reject(new Error("Timed out waiting for browser sign-in"))
     }, timeoutMs)
-    pending = {
-      state,
+    pending.set(state, {
       resolve: (creds) => {
         clearTimeout(timeout)
         resolve(creds)
@@ -121,7 +129,7 @@ function waitForCallback(state: string, timeoutMs = 5 * 60 * 1000): Promise<Call
         clearTimeout(timeout)
         reject(err)
       },
-    }
+    })
   })
 }
 
@@ -134,10 +142,11 @@ export async function AltimateAuthPlugin(_input: PluginInput): Promise<Hooks> {
           type: "oauth",
           label: "Altimate LLM Gateway",
           async authorize() {
-            // Bind the port BEFORE opening the browser so the credential can
-            // only be delivered to this process.
             const state = randomBytes(16).toString("hex")
             await startCallbackServer()
+            // Register the pending flow BEFORE opening the browser so an instant
+            // redirect can be matched by state rather than dropped as CSRF.
+            const result = registerPending(state)
 
             const webUrl = (process.env.ALTIMATE_WEB_URL || DEFAULT_WEB_URL).replace(/\/+$/, "")
             const redirect = `http://localhost:${CALLBACK_PORT}/callback`
@@ -156,7 +165,7 @@ export async function AltimateAuthPlugin(_input: PluginInput): Promise<Hooks> {
               method: "auto",
               async callback() {
                 try {
-                  const creds = await waitForCallback(state)
+                  const creds = await result
                   // Persist to ~/.altimate/altimate.json — the provider loader
                   // reads this first (it carries the instance/tenant + api_url
                   // the generic auth.json store can't).
@@ -169,7 +178,8 @@ export async function AltimateAuthPlugin(_input: PluginInput): Promise<Hooks> {
                 } catch {
                   return { type: "failed" }
                 } finally {
-                  stopCallbackServer()
+                  // Keep the shared server up while another flow is still waiting.
+                  if (pending.size === 0) stopCallbackServer()
                 }
               },
             }
