Skip to content

refactor: replace Bun.serve with Node http.createServer in OAuth handlers #18327

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
thdxr wants to merge 3 commits into
base: dev
Choose a base branch
from
+143 −135
Open

refactor: replace Bun.serve with Node http.createServer in OAuth handlers #18327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
816a5f7
refactor: replace Bun.serve with Node http.createServer in OAuth hand…
thdxr Mar 20, 2026
9f5713e
Merge branch 'dev' into refactor/oauth-node-http
thdxr Mar 20, 2026
a722045
Merge branch 'dev' into refactor/oauth-node-http
Hona Mar 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
161 changes: 81 additions & 80 deletions packages/opencode/src/mcp/oauth-callback.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { createConnection } from "net"
import { createServer } from "http"
import { Log } from "../util/log"
import { OAUTH_CALLBACK_PORT, OAUTH_CALLBACK_PATH } from "./oauth-provider"

Expand Down Expand Up @@ -52,105 +53,105 @@ interface PendingAuth {
}

export namespace McpOAuthCallback {
let server: ReturnType<typeof Bun.serve> | undefined
let server: ReturnType<typeof createServer> | undefined
const pendingAuths = new Map<string, PendingAuth>()
// Reverse index: mcpName → oauthState, so cancelPending(mcpName) can
// find the right entry in pendingAuths (which is keyed by oauthState).
const mcpNameToState = new Map<string, string>()

const CALLBACK_TIMEOUT_MS = 5 * 60 * 1000 // 5 minutes

export async function ensureRunning(): Promise<void> {
if (server) return
function cleanupStateIndex(oauthState: string) {
for (const [name, state] of mcpNameToState) {
if (state === oauthState) {
mcpNameToState.delete(name)
break
}
}
}

const running = await isPortInUse()
if (running) {
log.info("oauth callback server already running on another instance", { port: OAUTH_CALLBACK_PORT })
function handleRequest(req: import("http").IncomingMessage, res: import("http").ServerResponse) {
Copy link
Copy Markdown

@greptile-apps greptile-apps bot Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Inline dynamic import types are unnecessary

import("http").IncomingMessage and import("http").ServerResponse are used as inline type expressions, but http is already imported at line 2. The types can be imported alongside createServer to keep the signature readable.

Suggested change
function handleRequest(req: import("http").IncomingMessage, res: import("http").ServerResponse) {
function handleRequest(req: import("http").IncomingMessage, res: import("http").ServerResponse) {

Change the top-level import to:

import { createServer, type IncomingMessage, type ServerResponse } from "http"

Then the function signature becomes:

function handleRequest(req: IncomingMessage, res: ServerResponse) {

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

const url = new URL(req.url || "/", `http://localhost:${OAUTH_CALLBACK_PORT}`)

if (url.pathname !== OAUTH_CALLBACK_PATH) {
res.writeHead(404)
res.end("Not found")
return
}

server = Bun.serve({
port: OAUTH_CALLBACK_PORT,
fetch(req) {
const url = new URL(req.url)
const code = url.searchParams.get("code")
const state = url.searchParams.get("state")
const error = url.searchParams.get("error")
const errorDescription = url.searchParams.get("error_description")

if (url.pathname !== OAUTH_CALLBACK_PATH) {
return new Response("Not found", { status: 404 })
}
log.info("received oauth callback", { hasCode: !!code, state, error })

const code = url.searchParams.get("code")
const state = url.searchParams.get("state")
const error = url.searchParams.get("error")
const errorDescription = url.searchParams.get("error_description")

log.info("received oauth callback", { hasCode: !!code, state, error })

// Enforce state parameter presence
if (!state) {
const errorMsg = "Missing required state parameter - potential CSRF attack"
log.error("oauth callback missing state parameter", { url: url.toString() })
return new Response(HTML_ERROR(errorMsg), {
status: 400,
headers: { "Content-Type": "text/html" },
})
}
// Enforce state parameter presence
if (!state) {
const errorMsg = "Missing required state parameter - potential CSRF attack"
log.error("oauth callback missing state parameter", { url: url.toString() })
res.writeHead(400, { "Content-Type": "text/html" })
res.end(HTML_ERROR(errorMsg))
return
}

if (error) {
const errorMsg = errorDescription || error
if (pendingAuths.has(state)) {
const pending = pendingAuths.get(state)!
clearTimeout(pending.timeout)
pendingAuths.delete(state)
for (const [name, s] of mcpNameToState) {
if (s === state) {
mcpNameToState.delete(name)
break
}
}
pending.reject(new Error(errorMsg))
}
return new Response(HTML_ERROR(errorMsg), {
headers: { "Content-Type": "text/html" },
})
}
if (error) {
const errorMsg = errorDescription || error
if (pendingAuths.has(state)) {
const pending = pendingAuths.get(state)!
clearTimeout(pending.timeout)
pendingAuths.delete(state)
cleanupStateIndex(state)
pending.reject(new Error(errorMsg))
}
res.writeHead(200, { "Content-Type": "text/html" })
res.end(HTML_ERROR(errorMsg))
return
}

if (!code) {
return new Response(HTML_ERROR("No authorization code provided"), {
status: 400,
headers: { "Content-Type": "text/html" },
})
}
if (!code) {
res.writeHead(400, { "Content-Type": "text/html" })
res.end(HTML_ERROR("No authorization code provided"))
return
}

// Validate state parameter
if (!pendingAuths.has(state)) {
const errorMsg = "Invalid or expired state parameter - potential CSRF attack"
log.error("oauth callback with invalid state", { state, pendingStates: Array.from(pendingAuths.keys()) })
return new Response(HTML_ERROR(errorMsg), {
status: 400,
headers: { "Content-Type": "text/html" },
})
}
// Validate state parameter
if (!pendingAuths.has(state)) {
const errorMsg = "Invalid or expired state parameter - potential CSRF attack"
log.error("oauth callback with invalid state", { state, pendingStates: Array.from(pendingAuths.keys()) })
res.writeHead(400, { "Content-Type": "text/html" })
res.end(HTML_ERROR(errorMsg))
return
}

const pending = pendingAuths.get(state)!
const pending = pendingAuths.get(state)!

clearTimeout(pending.timeout)
pendingAuths.delete(state)
// Clean up reverse index
for (const [name, s] of mcpNameToState) {
if (s === state) {
mcpNameToState.delete(name)
break
}
}
pending.resolve(code)
clearTimeout(pending.timeout)
pendingAuths.delete(state)
cleanupStateIndex(state)
pending.resolve(code)

return new Response(HTML_SUCCESS, {
headers: { "Content-Type": "text/html" },
})
},
})
res.writeHead(200, { "Content-Type": "text/html" })
res.end(HTML_SUCCESS)
}

export async function ensureRunning(): Promise<void> {
if (server) return

const running = await isPortInUse()
if (running) {
log.info("oauth callback server already running on another instance", { port: OAUTH_CALLBACK_PORT })
return
}

log.info("oauth callback server started", { port: OAUTH_CALLBACK_PORT })
server = createServer(handleRequest)
await new Promise<void>((resolve, reject) => {
server!.listen(OAUTH_CALLBACK_PORT, () => {
log.info("oauth callback server started", { port: OAUTH_CALLBACK_PORT })
resolve()
})
server!.on("error", reject)
})
}

export function waitForCallback(oauthState: string, mcpName?: string): Promise<string> {
Expand Down Expand Up @@ -196,7 +197,7 @@ export namespace McpOAuthCallback {

export async function stop(): Promise<void> {
if (server) {
server.stop()
await new Promise<void>((resolve) => server!.close(() => resolve()))
server = undefined
log.info("oauth callback server stopped")
}
Expand Down
117 changes: 62 additions & 55 deletions packages/opencode/src/plugin/codex.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import os from "os"
import { ProviderTransform } from "@/provider/transform"
import { ModelID, ProviderID } from "@/provider/schema"
import { setTimeout as sleep } from "node:timers/promises"
import { createServer } from "http"

const log = Log.create({ service: "plugin.codex" })

Expand Down Expand Up @@ -241,85 +242,91 @@ interface PendingOAuth {
reject: (error: Error) => void
}

let oauthServer: ReturnType<typeof Bun.serve> | undefined
let oauthServer: ReturnType<typeof createServer> | undefined
let pendingOAuth: PendingOAuth | undefined

async function startOAuthServer(): Promise<{ port: number; redirectUri: string }> {
if (oauthServer) {
return { port: OAUTH_PORT, redirectUri: `http://localhost:${OAUTH_PORT}/auth/callback` }
}

oauthServer = Bun.serve({
port: OAUTH_PORT,
fetch(req) {
const url = new URL(req.url)
oauthServer = createServer((req, res) => {
const url = new URL(req.url || "/", `http://localhost:${OAUTH_PORT}`)

if (url.pathname === "/auth/callback") {
const code = url.searchParams.get("code")
const state = url.searchParams.get("state")
const error = url.searchParams.get("error")
const errorDescription = url.searchParams.get("error_description")
if (url.pathname === "/auth/callback") {
const code = url.searchParams.get("code")
const state = url.searchParams.get("state")
const error = url.searchParams.get("error")
const errorDescription = url.searchParams.get("error_description")

if (error) {
const errorMsg = errorDescription || error
pendingOAuth?.reject(new Error(errorMsg))
pendingOAuth = undefined
return new Response(HTML_ERROR(errorMsg), {
headers: { "Content-Type": "text/html" },
})
}

if (!code) {
const errorMsg = "Missing authorization code"
pendingOAuth?.reject(new Error(errorMsg))
pendingOAuth = undefined
return new Response(HTML_ERROR(errorMsg), {
status: 400,
headers: { "Content-Type": "text/html" },
})
}

if (!pendingOAuth || state !== pendingOAuth.state) {
const errorMsg = "Invalid state - potential CSRF attack"
pendingOAuth?.reject(new Error(errorMsg))
pendingOAuth = undefined
return new Response(HTML_ERROR(errorMsg), {
status: 400,
headers: { "Content-Type": "text/html" },
})
}

const current = pendingOAuth
if (error) {
const errorMsg = errorDescription || error
pendingOAuth?.reject(new Error(errorMsg))
pendingOAuth = undefined
res.writeHead(200, { "Content-Type": "text/html" })
res.end(HTML_ERROR(errorMsg))
return
}

exchangeCodeForTokens(code, `http://localhost:${OAUTH_PORT}/auth/callback`, current.pkce)
.then((tokens) => current.resolve(tokens))
.catch((err) => current.reject(err))

return new Response(HTML_SUCCESS, {
headers: { "Content-Type": "text/html" },
})
if (!code) {
const errorMsg = "Missing authorization code"
pendingOAuth?.reject(new Error(errorMsg))
pendingOAuth = undefined
res.writeHead(400, { "Content-Type": "text/html" })
res.end(HTML_ERROR(errorMsg))
return
}

if (url.pathname === "/cancel") {
pendingOAuth?.reject(new Error("Login cancelled"))
if (!pendingOAuth || state !== pendingOAuth.state) {
const errorMsg = "Invalid state - potential CSRF attack"
pendingOAuth?.reject(new Error(errorMsg))
pendingOAuth = undefined
return new Response("Login cancelled", { status: 200 })
res.writeHead(400, { "Content-Type": "text/html" })
res.end(HTML_ERROR(errorMsg))
return
}

return new Response("Not found", { status: 404 })
},
const current = pendingOAuth
pendingOAuth = undefined

exchangeCodeForTokens(code, `http://localhost:${OAUTH_PORT}/auth/callback`, current.pkce)
.then((tokens) => current.resolve(tokens))
.catch((err) => current.reject(err))

res.writeHead(200, { "Content-Type": "text/html" })
res.end(HTML_SUCCESS)
return
}

if (url.pathname === "/cancel") {
pendingOAuth?.reject(new Error("Login cancelled"))
pendingOAuth = undefined
res.writeHead(200)
res.end("Login cancelled")
return
}

res.writeHead(404)
res.end("Not found")
})

await new Promise<void>((resolve, reject) => {
oauthServer!.listen(OAUTH_PORT, () => {
log.info("codex oauth server started", { port: OAUTH_PORT })
resolve()
})
oauthServer!.on("error", reject)
})

log.info("codex oauth server started", { port: OAUTH_PORT })
return { port: OAUTH_PORT, redirectUri: `http://localhost:${OAUTH_PORT}/auth/callback` }
}

function stopOAuthServer() {
if (oauthServer) {
oauthServer.stop()
oauthServer.close(() => {
log.info("codex oauth server stopped")
})
oauthServer = undefined
log.info("codex oauth server stopped")
}
}
Comment on lines 324 to 331
Copy link
Copy Markdown

@greptile-apps greptile-apps bot Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Race condition: port still in use after stopOAuthServer returns

oauthServer is set to undefined synchronously, but server.close() in Node.js is asynchronous — the underlying port is only released once the close callback fires. If startOAuthServer() is called again before that callback fires (e.g. the user immediately re-initiates auth), oauthServer is undefined so the early-return guard is bypassed, and the new listen() call will fail with EADDRINUSE.

By contrast, Bun.serve.stop() released the port synchronously. The sibling McpOAuthCallback.stop() correctly awaits the close before clearing the reference:

await new Promise<void>((resolve) => server!.close(() => resolve()))
server = undefined

stopOAuthServer should follow the same pattern (make it async and await the close) or at minimum only clear oauthServer inside the close callback:

function stopOAuthServer() {
  if (oauthServer) {
    const closing = oauthServer
    oauthServer = undefined
    closing.close(() => {
      log.info("codex oauth server stopped")
    })
  }
}

Note: even the snippet above doesn't fully close the port before returning. If true synchronous-style safety is needed, the function should be made async and awaited at the call site.


Expand Down
Loading