-
Notifications
You must be signed in to change notification settings - Fork 751
feat(sdk): support renewable model authentication #720
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import { existsSync } from "node:fs"; | ||
| import { readFile } from "node:fs/promises"; | ||
| import { homedir } from "node:os"; | ||
| import { join } from "node:path"; | ||
| import { join, resolve } from "node:path"; | ||
| import { | ||
| Codex, | ||
| type CodexOptions, | ||
|
|
@@ -9,13 +10,16 @@ import { | |
| type TurnOptions, | ||
| } from "@openai/codex-sdk"; | ||
| import { z } from "incur"; | ||
| import { parse, stringify } from "smol-toml"; | ||
| import type { CodexSecuritySurface } from "./api.js"; | ||
| import { accountStatus } from "./auth.js"; | ||
| import { accountStatus, environmentEntry, openAiApiKey } from "./auth.js"; | ||
| import { | ||
| mergedCodexConfig, | ||
| scanModelConfiguration, | ||
| scanModelProvider, | ||
| type CodexSecurityConfig, | ||
| type JsonObject, | ||
| type JsonValue, | ||
| } from "./config.js"; | ||
| import { CodexSecurityError } from "./errors.js"; | ||
| import { | ||
|
|
@@ -32,6 +36,8 @@ import { | |
| type CodexSecurityThreadSource, | ||
| } from "./thread-source.js"; | ||
|
|
||
| export { environmentEntry } from "./auth.js"; | ||
|
|
||
| type Finding = { occurrenceId: string } & Record<string, unknown>; | ||
| type ReadOnlyCodexThreadSource = Extract< | ||
| CodexSecurityThreadSource, | ||
|
|
@@ -182,6 +188,11 @@ export async function runReadOnlyCodex( | |
| new Codex({ | ||
| codexPathOverride: executablePathForSpawn(command!.command), | ||
| env: environment, | ||
| configOverrides: await commandAuthConfigOverrides( | ||
| environment!, | ||
| config, | ||
| options.signal, | ||
| ), | ||
| config: { | ||
| ...config, | ||
| mcp_servers: await disabledMcpServers( | ||
|
|
@@ -412,16 +423,18 @@ export async function comparisonEnvironment( | |
| (entry): entry is [string, string] => entry[1] !== undefined, | ||
| ), | ||
| ); | ||
| if (await hasConfiguredCommandAuth(environment, signal)) { | ||
| for (const key of Object.keys(environment)) { | ||
| if (["OPENAI_API_KEY", "CODEX_API_KEY"].includes(key.toUpperCase())) { | ||
| delete environment[key]; | ||
|
Comment on lines
+426
to
+429
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a user runs CLI match/compare in an untrusted checkout with a selected relative SECURITY.md reference: SECURITY.md:L63-L68 Dismiss this finding Reply with Valid reasons: What each reason means
Example: Useful? React with 👍 / 👎. |
||
| } | ||
| } | ||
| return environment; | ||
| } | ||
| if (environmentEntry(environment, "CODEX_SECURITY_SCAN_ID") !== undefined) { | ||
| return environment; | ||
| } | ||
| if ( | ||
| Object.entries(environment).some( | ||
| ([name, value]) => | ||
| ["OPENAI_API_KEY", "CODEX_API_KEY"].includes(name.toUpperCase()) && | ||
| value.trim().length > 0, | ||
| ) | ||
| ) { | ||
| if (openAiApiKey(environment)) { | ||
| return environment; | ||
| } | ||
| const credentialHome = codexSecurityCredentialHome(source); | ||
|
|
@@ -460,16 +473,94 @@ export async function comparisonEnvironment( | |
| return environment; | ||
| } | ||
|
|
||
| export function environmentEntry( | ||
| async function hasConfiguredCommandAuth( | ||
| environment: Record<string, string>, | ||
| requested: string, | ||
| ): string | undefined { | ||
| const exact = environment[requested]; | ||
| if (exact !== undefined || process.platform !== "win32") return exact; | ||
| const upper = requested.toUpperCase(); | ||
| return Object.entries(environment).find( | ||
| ([name]) => name.toUpperCase() === upper, | ||
| )?.[1]; | ||
| signal?: AbortSignal, | ||
| ): Promise<boolean> { | ||
| const config = await readCodexHomeConfig(environment, signal); | ||
| if (config === undefined) return false; | ||
| const selected = scanModelProvider(config); | ||
| if (typeof selected !== "string") return false; | ||
| const providers = config["model_providers"] as JsonObject | undefined; | ||
| const provider = providers?.[selected] as JsonObject | undefined; | ||
| // Codex validates the auth table. Do not replace an explicitly configured | ||
| // command provider with another login even if its configuration is invalid. | ||
| return provider?.["auth"] !== undefined; | ||
| } | ||
|
|
||
| async function commandAuthConfigOverrides( | ||
| environment: Record<string, string>, | ||
| overrides: JsonObject | undefined, | ||
| signal?: AbortSignal, | ||
| ): Promise<string[]> { | ||
| const config = await readCodexHomeConfig(environment, signal); | ||
| if (config === undefined) return []; | ||
| const home = resolve( | ||
| expandHome( | ||
| environmentEntry(environment, "CODEX_HOME")!.trim(), | ||
| environment, | ||
| ), | ||
| ); | ||
| const providers = config["model_providers"] as JsonObject | undefined; | ||
| const providerOverrides = { | ||
| ...(overrides?.["model_providers"] as JsonObject | undefined), | ||
| }; | ||
| let changed = false; | ||
| for (const [name, provider] of Object.entries(providers ?? {})) { | ||
| const auth = (provider as JsonObject)?.["auth"] as JsonObject | undefined; | ||
| const overrideAuth = ( | ||
| providerOverrides?.[name] as JsonObject | undefined | ||
| )?.["auth"] as JsonObject | undefined; | ||
| if ( | ||
| typeof auth !== "object" || | ||
| auth === null || | ||
| Array.isArray(auth) || | ||
| auth["cwd"] !== undefined || | ||
| overrideAuth?.["cwd"] !== undefined | ||
| ) | ||
| continue; | ||
| // The SDK inherits the caller's cwd. Resolve implicit auth helpers from | ||
| // the supplied home instead of allowing the checkout to supply them. | ||
| providerOverrides[name] = { | ||
| ...(providerOverrides[name] as JsonObject | undefined), | ||
| auth: { ...overrideAuth, cwd: home }, | ||
| }; | ||
| changed = true; | ||
| } | ||
| // Override the table as a value: CLI dotted paths cannot quote provider IDs. | ||
| return changed ? [`model_providers=${inlineToml(providerOverrides)}`] : []; | ||
| } | ||
|
|
||
| function inlineToml(value: JsonValue): string { | ||
| if (Array.isArray(value)) return `[${value.map(inlineToml).join(",")}]`; | ||
| if (value !== null && typeof value === "object") { | ||
| return `{${Object.entries(value) | ||
| .map(([key, item]) => `${JSON.stringify(key)}=${inlineToml(item)}`) | ||
| .join(",")}}`; | ||
| } | ||
| return stringify({ value }).slice("value = ".length).trim(); | ||
| } | ||
|
|
||
| async function readCodexHomeConfig( | ||
| environment: Record<string, string>, | ||
| signal?: AbortSignal, | ||
| ): Promise<JsonObject | undefined> { | ||
| const home = environmentEntry(environment, "CODEX_HOME")?.trim(); | ||
| if (!home) return undefined; | ||
| try { | ||
| return parse( | ||
| await readFile(join(expandHome(home, environment), "config.toml"), { | ||
| encoding: "utf8", | ||
| signal, | ||
| }), | ||
| ) as JsonObject; | ||
| } catch (error) { | ||
| signal?.throwIfAborted(); | ||
| if ((error as NodeJS.ErrnoException).code === "ENOENT") return undefined; | ||
| throw new CodexSecurityError( | ||
| "Could not read the configured Codex provider.", | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| function validateComparison( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| export { OpenAiFindingEmbedder } from "./embeddings.js"; | ||
| export type { | ||
| FindingEmbedder, | ||
| OpenAiFindingEmbedderOptions, | ||
| } from "./embeddings.js"; | ||
| export { startFindingsServer } from "./server.js"; | ||
| export type { FindingsServerOptions } from "./server.js"; | ||
| export { SqliteFindingsStore } from "./sqlite-store.js"; | ||
| export type { FindingEmbedding, FindingsStore } from "./storage.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the selected provider uses an
auth.commandthat resolves relative to the process CWD, a malicious scanned checkout can supply that executable. This branch preserves the command-authCODEX_HOME;deduplicateScanthen launches the Codex app server with the saved target ascwd, so the helper runs outside the read-only filesystem profile. A local reproduction wrote to a sibling directory despite read-only mode. Resolve the helper outside the target, or run the auth-bearing parent from a private directory.SECURITY.md reference: SECURITY.md:L63-L68
Dismiss this finding
Reply with
@codex security dismiss <reason> [context].Valid reasons:
false-positive,duplicate,out-of-scope,compensating-control,risk-accepted, orother.What each reason means
false-positive— Not a vulnerabilityduplicate— Already tracked elsewhereout-of-scope— Outside this review's scopecompensating-control— Mitigated by another controlrisk-accepted— Risk intentionally acceptedother— Another reason; context requiredExample:
@codex security dismiss duplicate Already flagged by another reviewUseful? React with 👍 / 👎.