Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ on:
pull_request:
branches: [main]

permissions:
contents: read

jobs:
verify:
runs-on: ubuntu-latest
Expand Down
76 changes: 76 additions & 0 deletions packages/cli/src/auth/wellknown.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import z from "zod"

export namespace WellKnownAuth {
const LOOPBACK_HOSTS = new Set(["localhost", "127.0.0.1", "[::1]"])

export const Document = z.object({
auth: z.object({
command: z.array(z.string().min(1)).min(1),
env: z.string().min(1),
}),
})
export type Command = z.infer<typeof Document>["auth"]

// The provider dictates a local command, so plain http is only allowed for local development.
export function endpoint(providerURL: string): string {
let url: URL
try {
url = new URL(providerURL)
} catch (cause) {
throw new Error("Invalid auth provider URL. Supply an absolute https URL.", { cause })
}
const loopback = url.protocol === "http:" && LOOPBACK_HOSTS.has(url.hostname)
if (url.protocol !== "https:" && !loopback) {
throw new Error("Auth provider URL must use https (except http on loopback hosts).")
}
url.search = ""
url.hash = ""
url.pathname = `${url.pathname.replace(/\/+$/, "")}/.well-known/aictrl`
return url.toString()
}

export function display(value: string | readonly string[]): string {
return JSON.stringify(value).replace(
/[\u007f-\u009f\u061c\u200e\u200f\u2028-\u202e\u2066-\u2069]/g,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 display() misses zero-width/invisible code points.

--- a/packages/cli/src/auth/wellknown.ts
+++ b/packages/cli/src/auth/wellknown.ts
@@ -31,7 +31,9 @@
   export function display(value: string | readonly string[]): string {
     return JSON.stringify(value).replace(
-      /[\u007f-\u009f\u061c\u200e\u200f\u2028-\u202e\u2066-\u2069]/g,
+      /[\u007f-\u009f\u00ad\u061c\u070f\u180e\u200b-\u200f\u2028-\u202e\u2060-\u2069]/g,
       (character) => `\\u${character.charCodeAt(0).toString(16).padStart(4, "0")}`,
     )
   }
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #215, packages/cli/src/auth/wellknown.ts:34):

Problem: display() misses zero-width/invisible code points
Detail: The anti-spoofing escaper covers U+007F–U+009F, U+061C, U+200E/F, U+2028–U+202E and U+2066–U+2069, but not U+200B–U+200D (ZWSP/ZWNJ/ZWJ), U+00AD (soft hyphen), U+070F, U+180E or U+2060–U+2064, which render invisibly in terminals. This prompt is the only approval gate before the provider-controlled command array is spawned, so a malicious provider can embed these characters in auth.command or the URL to make the displayed command misrepresent what will actually execute.
Suggested fix: Widen the regex to also escape the invisible code points: /[\u007f-\u009f\u00ad\u061c\u070f\u180e\u200b-\u200f\u2028-\u202e\u2060-\u2069]/g, or invert the policy and escape everything outside a safe printable allowlist.

Suggested patch:
--- a/packages/cli/src/auth/wellknown.ts
+++ b/packages/cli/src/auth/wellknown.ts
@@ -31,7 +31,9 @@
   export function display(value: string | readonly string[]): string {
     return JSON.stringify(value).replace(
-      /[\u007f-\u009f\u061c\u200e\u200f\u2028-\u202e\u2066-\u2069]/g,
+      /[\u007f-\u009f\u00ad\u061c\u070f\u180e\u200b-\u200f\u2028-\u202e\u2060-\u2069]/g,
       (character) => `\\u${character.charCodeAt(0).toString(16).padStart(4, "0")}`,
     )
   }

Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters

The anti-spoofing escaper covers U+007F–U+009F, U+061C, U+200E/F, U+2028–U+202E and U+2066–U+2069, but not U+200B–U+200D (ZWSP/ZWNJ/ZWJ), U+00AD (soft hyphen), U+070F, U+180E or U+2060–U+2064, which render invisibly in terminals. This prompt is the only approval gate before the provider-controlled command array is spawned, so a malicious provider can embed these characters in auth.command or the URL to make the displayed command misrepresent what will actually execute.

  export function display(value: string | readonly string[]): string {
    return JSON.stringify(value).replace(
      /[\u007f-\u009f\u061c\u200e\u200f\u2028-\u202e\u2066-\u2069]/g,
      (character) => `\\u${character.charCodeAt(0).toString(16).padStart(4, "0")}`,
    )
  }

(character) => `\\u${character.charCodeAt(0).toString(16).padStart(4, "0")}`,
)
}

export async function fetchCommand(providerURL: string): Promise<Command> {
const target = endpoint(providerURL)
let response: Response
try {
response = await fetch(target, { redirect: "error" })

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Well-known fetch has no timeout; login can hang forever.

🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #215, packages/cli/src/auth/wellknown.ts:43):

Problem: Well-known fetch has no timeout; login can hang forever
Detail: fetchCommand awaits fetch(target, { redirect: "error" }) with no AbortSignal/timeout, so a provider that accepts the connection but never responds (or a blackholing middlebox) blocks `aictrl auth login --url ...` indefinitely with no error message. The redirect and HTTP-error paths are handled, but the stalled-connection case is not. Repro: run `aictrl auth login --url https://slow.example.com` where the server accepts TCP but never responds → the interactive login hangs until the user manually Ctrl-C's.
Suggested fix: Pass AbortSignal.timeout (e.g. AbortSignal.timeout(10_000)) in the fetch init and mention the timeout in the existing "Failed to fetch auth provider config" error so the cause is surfaced to the user.

Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters

fetchCommand awaits fetch(target, { redirect: "error" }) with no AbortSignal/timeout, so a provider that accepts the connection but never responds (or a blackholing middlebox) blocks aictrl auth login --url ... indefinitely with no error message. The redirect and HTTP-error paths are handled, but the stalled-connection case is not. Repro: run aictrl auth login --url https://slow.example.com where the server accepts TCP but never responds → the interactive login hangs until the user manually Ctrl-C's.

  export async function fetchCommand(providerURL: string): Promise<Command> {
    const target = endpoint(providerURL)
    let response: Response
    try {
      response = await fetch(target, { redirect: "error" })
    } catch (cause) {

} catch (cause) {
throw new Error(
"Failed to fetch auth provider config. Check connectivity and use a direct URL without redirects.",
{
cause,
},
)
}
if (!response.ok) {
throw new Error(`Failed to fetch auth provider config (HTTP ${response.status}). Check the provider URL.`, {
cause: response,
})
}
let document: unknown
try {
document = await response.json()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Well-known fetch buffers unbounded response body.

🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #215, packages/cli/src/auth/wellknown.ts:59):

Problem: Well-known fetch buffers unbounded response body
Detail: The well-known document is read via `await response.json()` with no Content-Type check, Content-Length check, or streamed-size cap, and this runs before the user approves anything. A hostile or misbehaving provider (including an allowed loopback http one) can return an arbitrarily large body and exhaust CLI memory before schema validation ever runs. Standard hardening for fetching untrusted auth config is to cap the body size.
Suggested fix: Check Content-Type is application/json when present, and read the body as a capped stream (abort past e.g. 64 KiB) before JSON parsing, mapping the failure to the existing actionable error style.

Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters

The well-known document is read via await response.json() with no Content-Type check, Content-Length check, or streamed-size cap, and this runs before the user approves anything. A hostile or misbehaving provider (including an allowed loopback http one) can return an arbitrarily large body and exhaust CLI memory before schema validation ever runs. Standard hardening for fetching untrusted auth config is to cap the body size.

    let document: unknown
    try {
      document = await response.json()
    } catch (cause) {
      throw new Error("Auth provider config is not valid JSON. Contact the provider to fix its well-known document.", {

} catch (cause) {
throw new Error("Auth provider config is not valid JSON. Contact the provider to fix its well-known document.", {
cause,
})
}
try {
return Document.parse(document).auth
} catch (cause) {
throw new Error(
"Auth provider config must supply auth.command as a nonempty string array and auth.env as a string.",
{
cause,
},
)
}
}
}
19 changes: 14 additions & 5 deletions packages/cli/src/cli/cmd/auth.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Auth } from "../../auth"
import { WellKnownAuth } from "../../auth/wellknown"
import { cmd } from "./cmd"
import * as prompts from "@clack/prompts"
import { UI } from "../ui"
Expand Down Expand Up @@ -263,9 +264,17 @@ export const AuthLoginCommand = cmd({
UI.empty()
prompts.intro("Add credential")
if (args.url) {
const wellknown = await fetch(`${args.url}/.well-known/aictrl`).then((x) => x.json() as any)
prompts.log.info(`Running \`${wellknown.auth.command.join(" ")}\``)
const proc = Process.spawn(wellknown.auth.command, {
const wellknown = await WellKnownAuth.fetchCommand(args.url)
const approved = await prompts.confirm({
message: `${WellKnownAuth.display(args.url)} wants to run ${WellKnownAuth.display(wellknown.command)} to get a token. Run it?`,
initialValue: false,
})
if (prompts.isCancel(approved)) throw new UI.CancelledError()
if (!approved) {
prompts.outro("Cancelled")
return
}
const proc = Process.spawn(wellknown.command, {
stdout: "pipe",
})
if (!proc.stdout) {
Expand All @@ -281,10 +290,10 @@ export const AuthLoginCommand = cmd({
}
await Auth.set(args.url, {
type: "wellknown",
key: wellknown.auth.env,
key: wellknown.env,
token: token.trim(),
})
prompts.log.success("Logged into " + args.url)
prompts.log.success("Logged into " + WellKnownAuth.display(args.url))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚪ Success log prints JSON-quoted provider URL.

🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #215, packages/cli/src/cli/cmd/auth.ts:296):

Problem: Success log prints JSON-quoted provider URL
Detail: WellKnownAuth.display() returns JSON.stringify(value), so for a scalar string it wraps the URL in double quotes; the success line now renders as `Logged into "https://auth.example.com"`. Neighbouring messages in this file interpolate raw values (e.g. "Go to: " + authorize.url), and the JSON quoting only adds value in the confirm prompt where it delimits argv elements. The success line should escape control characters without adding JSON quoting.
Suggested fix: Add an escape-only variant for scalar strings (e.g. displayScalar(url) that JSON.stringify's then strips the outer quotes), so the success message keeps the pre-PR `Logged into https://…` shape while still escaping control characters.

Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters

WellKnownAuth.display() returns JSON.stringify(value), so for a scalar string it wraps the URL in double quotes; the success line now renders as Logged into "https://auth.example.com". Neighbouring messages in this file interpolate raw values (e.g. "Go to: " + authorize.url), and the JSON quoting only adds value in the confirm prompt where it delimits argv elements. The success line should escape control characters without adding JSON quoting.

          await Auth.set(args.url, {
            type: "wellknown",
            key: wellknown.env,
            token: token.trim(),
          })
          prompts.log.success("Logged into " + WellKnownAuth.display(args.url))
          prompts.outro("Done")

prompts.outro("Done")
return
}
Expand Down
11 changes: 2 additions & 9 deletions packages/cli/src/plugin/codex.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,15 @@ interface PkceCodes {
}

async function generatePKCE(): Promise<PkceCodes> {
const verifier = generateRandomString(43)
// RFC 7636: base64url-encode 32 random bytes to get an unbiased 43-character verifier.
const verifier = base64UrlEncode(crypto.getRandomValues(new Uint8Array(32)).buffer)
const encoder = new TextEncoder()
const data = encoder.encode(verifier)
const hash = await crypto.subtle.digest("SHA-256", data)
const challenge = base64UrlEncode(hash)
return { verifier, challenge }
}

function generateRandomString(length: number): string {
const chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~"
const bytes = crypto.getRandomValues(new Uint8Array(length))
return Array.from(bytes)
.map((b) => chars[b % chars.length])
.join("")
}

function base64UrlEncode(buffer: ArrayBuffer): string {
const bytes = new Uint8Array(buffer)
const binary = String.fromCharCode(...bytes)
Expand Down
131 changes: 131 additions & 0 deletions packages/cli/test/auth/wellknown.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
import { test, expect } from "bun:test"
import { WellKnownAuth } from "../../src/auth/wellknown"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚪ New test file skips describe() wrapper convention.

🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #215, packages/cli/test/auth/wellknown.test.ts:3-5):

Problem: New test file skips describe() wrapper convention
Detail: Sibling test files under packages/cli/test (e.g. test/plugin/codex.test.ts, test/cli/plugin-auth-picker.test.ts) import `describe` from "bun:test" and group all cases under a describe("<unit>", ...) block, but the new wellknown.test.ts declares bare top-level test() calls with no describe wrapper. Grouping under e.g. describe("WellKnownAuth", ...) keeps test output and file organization consistent with the rest of packages/cli/test.
Suggested fix: Import describe from "bun:test" and wrap all cases in describe("WellKnownAuth", () => { ... }) (nested describes for endpoint/display/fetchCommand if desired).

Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters

Sibling test files under packages/cli/test (e.g. test/plugin/codex.test.ts, test/cli/plugin-auth-picker.test.ts) import describe from "bun:test" and group all cases under a describe("", ...) block, but the new wellknown.test.ts declares bare top-level test() calls with no describe wrapper. Grouping under e.g. describe("WellKnownAuth", ...) keeps test output and file organization consistent with the rest of packages/cli/test.

import { test, expect } from "bun:test"
import { WellKnownAuth } from "../../src/auth/wellknown"

test("builds the well-known endpoint for https providers", () => {
  expect(WellKnownAuth.endpoint("https://auth.example.com")).toBe("https://auth.example.com/.well-known/aictrl")
})

test("builds the well-known endpoint for https providers", () => {
expect(WellKnownAuth.endpoint("https://auth.example.com")).toBe("https://auth.example.com/.well-known/aictrl")
})

test("allows plain http only for loopback hosts", () => {
expect(WellKnownAuth.endpoint("http://localhost:4096")).toBe("http://localhost:4096/.well-known/aictrl")
expect(WellKnownAuth.endpoint("http://127.0.0.1:4096")).toBe("http://127.0.0.1:4096/.well-known/aictrl")
expect(WellKnownAuth.endpoint("http://[::1]:4096")).toBe("http://[::1]:4096/.well-known/aictrl")
})

test("rejects insecure or non-http provider URLs", () => {
expect(() => WellKnownAuth.endpoint("http://auth.example.com")).toThrow("must use https")
expect(() => WellKnownAuth.endpoint("file:///etc/passwd")).toThrow("must use https")
expect(() => WellKnownAuth.endpoint("auth.example.com")).toThrow()
})

test("accepts a well-formed auth command", () => {
const parsed = WellKnownAuth.Document.parse({ auth: { command: ["cli", "token"], env: "TOKEN" }, config: {} })
expect(parsed.auth).toEqual({ command: ["cli", "token"], env: "TOKEN" })
})

test("rejects malformed auth commands", () => {
expect(() => WellKnownAuth.Document.parse({ auth: { command: "cli token", env: "TOKEN" } })).toThrow()
expect(() => WellKnownAuth.Document.parse({ auth: { command: [], env: "TOKEN" } })).toThrow()
expect(() => WellKnownAuth.Document.parse({ auth: { command: ["cli"] } })).toThrow()
expect(() => WellKnownAuth.Document.parse({})).toThrow()
})

test("appends the endpoint to the base path without query, fragment, or trailing slashes", () => {
for (const suffix of ["", "/", "///", "?tenant=example", "/?tenant=example#login"]) {
expect(WellKnownAuth.endpoint(`https://auth.example.com/base${suffix}`)).toBe(
"https://auth.example.com/base/.well-known/aictrl",
)
}
})

test("malformed provider URLs have a minimal actionable message and retain the cause", () => {
try {
WellKnownAuth.endpoint("invalid-owned-url\nPRIVATE_DOCUMENT_MARKER")
throw new Error("Expected an invalid URL error")
} catch (error) {
expect(error).toBeInstanceOf(Error)
expect((error as Error).message).toContain("Supply an absolute https URL")
expect((error as Error).message).not.toContain("PRIVATE_DOCUMENT_MARKER")
expect((error as Error).cause).toBeInstanceOf(Error)
}
})

test("terminal display escapes controls while preserving exact values and argument boundaries", () => {
const command = ["owned-cli", "two words", 'quote" and `literal`', "\x1b[2J\x07\r\n\t\0\x7f\x85\u2028\u2029\u202e"]
const rendered = WellKnownAuth.display(command)
expect(JSON.parse(rendered)).toEqual(command)
expect(rendered).not.toMatch(/[\x00-\x1f\x7f-\x9f\u2028\u2029\u202e]/)
expect(rendered).toContain('"two words"')
expect(rendered).toContain("\\u001b")
expect(rendered).toContain("\\u202e")
const provider = "https://auth.example.com/?hint=\x1b[2J\n\u2066"
expect(JSON.parse(WellKnownAuth.display(provider))).toBe(provider)
expect(WellKnownAuth.display(provider)).not.toMatch(/[\x00-\x1f\u2066]/)
})

test("fetches the canonical endpoint and returns only the validated auth command", async () => {
const paths: string[] = []
const server = Bun.serve({
hostname: "127.0.0.1",
port: 0,
fetch(request) {
paths.push(new URL(request.url).pathname)
return Response.json({ auth: { command: ["owned-cli", "two words"], env: "OWNED_TOKEN" } })
},
})
try {
expect(await WellKnownAuth.fetchCommand(`${server.url}base///?tenant=example#login`)).toEqual({
command: ["owned-cli", "two words"],
env: "OWNED_TOKEN",
})
expect(paths).toEqual(["/base/.well-known/aictrl"])
} finally {
await server.stop(true)
}
})

test("never follows an auth document redirect to another owned loopback server", async () => {
let followed = 0
const target = Bun.serve({
hostname: "127.0.0.1",
port: 0,
fetch() {
followed++
return Response.json({ auth: { command: ["owned-cli"], env: "OWNED_TOKEN" } })
},
})
const redirect = Bun.serve({
hostname: "127.0.0.1",
port: 0,
fetch: () => Response.redirect(target.url),
})
try {
await expect(WellKnownAuth.fetchCommand(redirect.url.toString())).rejects.toThrow("without redirects")
expect(followed).toBe(0)
} finally {
await Promise.all([redirect.stop(true), target.stop(true)])
}
})

for (const [name, response, message] of [
["HTTP error", () => new Response("PRIVATE_DOCUMENT_MARKER", { status: 503 }), "HTTP 503"],
["invalid JSON", () => new Response("PRIVATE_DOCUMENT_MARKER"), "not valid JSON"],
["invalid schema", () => Response.json({ auth: { command: "PRIVATE_DOCUMENT_MARKER" } }), "auth.command"],
] as const) {
test(`${name} produces an actionable error with cause without echoing the document`, async () => {
const server = Bun.serve({ hostname: "127.0.0.1", port: 0, fetch: response })
try {
let failure: Error | undefined
try {
await WellKnownAuth.fetchCommand(server.url.toString())
} catch (error) {
failure = error as Error
}
expect(failure).toBeInstanceOf(Error)
expect(failure!.message).toContain(message)
expect(failure!.message).not.toContain("PRIVATE_DOCUMENT_MARKER")
expect(failure!.cause).toBeDefined()
} finally {
await server.stop(true)
}
})
}
Loading