Skip to content

Commit 9610df9

Browse files
ndycodendycode
committed
fix: harden config validation and settings recovery
1 parent b7aea9d commit 9610df9

5 files changed

Lines changed: 284 additions & 50 deletions

File tree

‎lib/config.ts‎

Lines changed: 114 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,11 @@ export const DEFAULT_PLUGIN_CONFIG: PluginConfig = {
199199
preemptiveQuotaMaxDeferralMs: 2 * 60 * 60_000,
200200
};
201201

202+
const PLUGIN_CONFIG_FIELD_SCHEMAS = PluginConfigSchema.shape;
203+
const PLUGIN_CONFIG_KEYS = Object.keys(DEFAULT_PLUGIN_CONFIG) as Array<
204+
keyof PluginConfig
205+
>;
206+
202207
/**
203208
* Return a shallow copy of the default plugin configuration.
204209
*
@@ -238,6 +243,10 @@ export function loadPluginConfig(): PluginConfig {
238243
sourceKind = "file";
239244
}
240245

246+
const normalizedUserConfig = sanitizePluginConfigRecord(userConfig, {
247+
warnOnInvalid: true,
248+
});
249+
241250
const hasFallbackEnvOverride =
242251
process.env.CODEX_AUTH_FALLBACK_UNSUPPORTED_MODEL !== undefined ||
243252
process.env.CODEX_AUTH_FALLBACK_GPT53_TO_GPT52 !== undefined;
@@ -255,16 +264,9 @@ export function loadPluginConfig(): PluginConfig {
255264
}
256265
}
257266

258-
const schemaErrors = getValidationErrors(PluginConfigSchema, userConfig);
259-
if (schemaErrors.length > 0) {
260-
logConfigWarnOnce(
261-
`Plugin config validation warnings: ${schemaErrors.slice(0, 3).join(", ")}`,
262-
);
263-
}
264-
265267
if (
266268
sourceKind === "file" &&
267-
isRecord(userConfig) &&
269+
normalizedUserConfig !== null &&
268270
(process.env.CODEX_MULTI_AUTH_CONFIG_PATH ?? "").trim().length === 0
269271
) {
270272
logConfigWarnOnce(
@@ -274,7 +276,7 @@ export function loadPluginConfig(): PluginConfig {
274276

275277
return {
276278
...DEFAULT_PLUGIN_CONFIG,
277-
...(userConfig as Partial<PluginConfig>),
279+
...(normalizedUserConfig ?? {}),
278280
};
279281
} catch (error) {
280282
const configPath = resolvePluginConfigPath() ?? CONFIG_PATH;
@@ -285,6 +287,62 @@ export function loadPluginConfig(): PluginConfig {
285287
}
286288
}
287289

290+
function sanitizePluginConfigRecord(
291+
data: unknown,
292+
options?: { warnOnInvalid?: boolean },
293+
): Partial<PluginConfig> | null {
294+
if (!isRecord(data)) {
295+
return null;
296+
}
297+
298+
if (options?.warnOnInvalid) {
299+
const schemaErrors = getValidationErrors(PluginConfigSchema, data);
300+
if (schemaErrors.length > 0) {
301+
logConfigWarnOnce(
302+
`Plugin config validation warnings: ${schemaErrors.slice(0, 3).join(", ")}`,
303+
);
304+
}
305+
}
306+
307+
const sanitized: Record<string, unknown> = {};
308+
for (const key of PLUGIN_CONFIG_KEYS) {
309+
const value = data[key];
310+
if (value === undefined) {
311+
continue;
312+
}
313+
const schema = PLUGIN_CONFIG_FIELD_SCHEMAS[key];
314+
const result = schema.safeParse(value);
315+
if (result.success) {
316+
sanitized[String(key)] = result.data;
317+
}
318+
}
319+
320+
return sanitized as Partial<PluginConfig>;
321+
}
322+
323+
function sanitizeStoredPluginConfigRecord(
324+
data: unknown,
325+
): Record<string, unknown> {
326+
if (!isRecord(data)) {
327+
return {};
328+
}
329+
330+
const sanitized: Record<string, unknown> = {};
331+
for (const [key, value] of Object.entries(data)) {
332+
if (!PLUGIN_CONFIG_KEYS.includes(key as keyof PluginConfig)) {
333+
sanitized[key] = value;
334+
continue;
335+
}
336+
const schema = PLUGIN_CONFIG_FIELD_SCHEMAS[key as keyof PluginConfig];
337+
const result = schema.safeParse(value);
338+
if (result.success) {
339+
sanitized[key] = result.data;
340+
}
341+
}
342+
343+
return sanitized;
344+
}
345+
288346
/**
289347
* Remove a leading UTF‑8 byte order mark (BOM) from the given string if present.
290348
*
@@ -443,7 +501,8 @@ function resolveStoredPluginConfigRecord(): {
443501
function sanitizePluginConfigForSave(
444502
config: Partial<PluginConfig>,
445503
): Record<string, unknown> {
446-
const entries = Object.entries(config as Record<string, unknown>);
504+
const normalized = sanitizePluginConfigRecord(config as Record<string, unknown>);
505+
const entries = Object.entries((normalized ?? {}) as Record<string, unknown>);
447506
const sanitized: Record<string, unknown> = {};
448507
for (const [key, value] of entries) {
449508
if (value === undefined) continue;
@@ -478,8 +537,11 @@ export async function savePluginConfig(
478537

479538
if (envPath.length > 0) {
480539
await withConfigSaveLock(envPath, async () => {
540+
const existingConfig = sanitizeStoredPluginConfigRecord(
541+
readConfigRecordFromPath(envPath),
542+
);
481543
const merged = {
482-
...(readConfigRecordFromPath(envPath) ?? {}),
544+
...(existingConfig ?? {}),
483545
...sanitizedPatch,
484546
};
485547
await writeJsonFileAtomicWithRetry(envPath, merged);
@@ -489,11 +551,16 @@ export async function savePluginConfig(
489551

490552
const unifiedPath = getUnifiedSettingsPath();
491553
await withConfigSaveLock(unifiedPath, async () => {
492-
const unifiedConfig = loadUnifiedPluginConfigSync();
554+
const unifiedConfig = sanitizeStoredPluginConfigRecord(
555+
loadUnifiedPluginConfigSync(),
556+
);
493557
const legacyPath = unifiedConfig ? null : resolvePluginConfigPath();
558+
const legacyConfig = legacyPath
559+
? sanitizeStoredPluginConfigRecord(readConfigRecordFromPath(legacyPath))
560+
: null;
494561
const merged = {
495562
...(unifiedConfig ??
496-
(legacyPath ? readConfigRecordFromPath(legacyPath) : null) ??
563+
legacyConfig ??
497564
{}),
498565
...sanitizedPatch,
499566
};
@@ -510,12 +577,30 @@ export async function savePluginConfig(
510577
*/
511578
function parseBooleanEnv(value: string | undefined): boolean | undefined {
512579
if (value === undefined) return undefined;
513-
return value === "1";
580+
const normalized = value.trim().toLowerCase();
581+
if (normalized.length === 0) return undefined;
582+
if (
583+
normalized === "1" ||
584+
normalized === "true" ||
585+
normalized === "yes"
586+
) {
587+
return true;
588+
}
589+
if (
590+
normalized === "0" ||
591+
normalized === "false" ||
592+
normalized === "no"
593+
) {
594+
return false;
595+
}
596+
return undefined;
514597
}
515598

516599
function parseNumberEnv(value: string | undefined): number | undefined {
517600
if (value === undefined) return undefined;
518-
const parsed = Number(value);
601+
const trimmed = value.trim();
602+
if (trimmed.length === 0) return undefined;
603+
const parsed = Number(trimmed);
519604
if (!Number.isFinite(parsed)) return undefined;
520605
return parsed;
521606
}
@@ -544,7 +629,13 @@ function resolveBooleanSetting(
544629
configValue: boolean | undefined,
545630
defaultValue: boolean,
546631
): boolean {
547-
const envValue = parseBooleanEnv(process.env[envName]);
632+
const rawEnvValue = process.env[envName];
633+
const envValue = parseBooleanEnv(rawEnvValue);
634+
if (rawEnvValue !== undefined && envValue === undefined) {
635+
logConfigWarnOnce(
636+
`Ignoring invalid boolean env ${envName}=${JSON.stringify(rawEnvValue)}. Expected 0/1, true/false, or yes/no.`,
637+
);
638+
}
548639
if (envValue !== undefined) return envValue;
549640
return configValue ?? defaultValue;
550641
}
@@ -566,7 +657,13 @@ function resolveNumberSetting(
566657
defaultValue: number,
567658
options?: { min?: number; max?: number },
568659
): number {
569-
const envValue = parseNumberEnv(process.env[envName]);
660+
const rawEnvValue = process.env[envName];
661+
const envValue = parseNumberEnv(rawEnvValue);
662+
if (rawEnvValue !== undefined && envValue === undefined) {
663+
logConfigWarnOnce(
664+
`Ignoring invalid numeric env ${envName}=${JSON.stringify(rawEnvValue)}. Expected a finite number.`,
665+
);
666+
}
570667
const candidate = envValue ?? configValue ?? defaultValue;
571668
const min = options?.min ?? Number.NEGATIVE_INFINITY;
572669
const max = options?.max ?? Number.POSITIVE_INFINITY;

‎lib/unified-settings.ts‎

Lines changed: 92 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import {
2+
copyFileSync,
23
existsSync,
34
mkdirSync,
45
renameSync,
@@ -16,6 +17,7 @@ type JsonRecord = Record<string, unknown>;
1617
export const UNIFIED_SETTINGS_VERSION = 1 as const;
1718

1819
const UNIFIED_SETTINGS_PATH = join(getCodexMultiAuthDir(), "settings.json");
20+
const UNIFIED_SETTINGS_BACKUP_PATH = `${UNIFIED_SETTINGS_PATH}.bak`;
1921
const RETRYABLE_FS_CODES = new Set(["EBUSY", "EPERM"]);
2022
let settingsWriteQueue: Promise<void> = Promise.resolve();
2123

@@ -45,6 +47,64 @@ function cloneRecord(value: unknown): JsonRecord | null {
4547
return { ...value };
4648
}
4749

50+
function parseSettingsRecord(content: string): JsonRecord {
51+
const parsed = cloneRecord(JSON.parse(content));
52+
if (!parsed) {
53+
throw new Error("Unified settings must contain a JSON object at the root.");
54+
}
55+
return parsed;
56+
}
57+
58+
function readSettingsRecordSyncFromPath(filePath: string): JsonRecord | null {
59+
if (!existsSync(filePath)) {
60+
return null;
61+
}
62+
return parseSettingsRecord(readFileSync(filePath, "utf8"));
63+
}
64+
65+
async function readSettingsRecordAsyncFromPath(
66+
filePath: string,
67+
): Promise<JsonRecord | null> {
68+
if (!existsSync(filePath)) {
69+
return null;
70+
}
71+
return parseSettingsRecord(await fs.readFile(filePath, "utf8"));
72+
}
73+
74+
function trySnapshotUnifiedSettingsBackupSync(): void {
75+
if (!existsSync(UNIFIED_SETTINGS_PATH)) {
76+
return;
77+
}
78+
for (let attempt = 0; attempt < 5; attempt += 1) {
79+
try {
80+
copyFileSync(UNIFIED_SETTINGS_PATH, UNIFIED_SETTINGS_BACKUP_PATH);
81+
return;
82+
} catch (error) {
83+
if (!isRetryableFsError(error) || attempt >= 4) {
84+
return;
85+
}
86+
Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 10 * 2 ** attempt);
87+
}
88+
}
89+
}
90+
91+
async function trySnapshotUnifiedSettingsBackupAsync(): Promise<void> {
92+
if (!existsSync(UNIFIED_SETTINGS_PATH)) {
93+
return;
94+
}
95+
for (let attempt = 0; attempt < 5; attempt += 1) {
96+
try {
97+
await fs.copyFile(UNIFIED_SETTINGS_PATH, UNIFIED_SETTINGS_BACKUP_PATH);
98+
return;
99+
} catch (error) {
100+
if (!isRetryableFsError(error) || attempt >= 4) {
101+
return;
102+
}
103+
await sleep(10 * 2 ** attempt);
104+
}
105+
}
106+
}
107+
48108
/**
49109
* Reads and parses the unified settings JSON file from disk.
50110
*
@@ -56,16 +116,22 @@ function cloneRecord(value: unknown): JsonRecord | null {
56116
* - Sensitive data: this function performs no token or secret redaction; any sensitive values present in the file are returned as-is and callers are responsible for redaction before logging or external exposure.
57117
*/
58118
function readSettingsRecordSync(): JsonRecord | null {
59-
if (!existsSync(UNIFIED_SETTINGS_PATH)) {
60-
return null;
119+
try {
120+
const primaryRecord = readSettingsRecordSyncFromPath(UNIFIED_SETTINGS_PATH);
121+
if (primaryRecord) {
122+
return primaryRecord;
123+
}
124+
} catch (error) {
125+
const backupRecord = readSettingsRecordSyncFromPath(
126+
UNIFIED_SETTINGS_BACKUP_PATH,
127+
);
128+
if (backupRecord) {
129+
return backupRecord;
130+
}
131+
throw error;
61132
}
62133

63-
const raw = readFileSync(UNIFIED_SETTINGS_PATH, "utf8");
64-
const parsed = cloneRecord(JSON.parse(raw));
65-
if (!parsed) {
66-
throw new Error("Unified settings must contain a JSON object at the root.");
67-
}
68-
return parsed;
134+
return readSettingsRecordSyncFromPath(UNIFIED_SETTINGS_BACKUP_PATH);
69135
}
70136

71137
/**
@@ -76,16 +142,24 @@ function readSettingsRecordSync(): JsonRecord | null {
76142
* @returns The parsed settings record as an object clone, or `null` if unavailable or invalid.
77143
*/
78144
async function readSettingsRecordAsync(): Promise<JsonRecord | null> {
79-
if (!existsSync(UNIFIED_SETTINGS_PATH)) {
80-
return null;
145+
try {
146+
const primaryRecord = await readSettingsRecordAsyncFromPath(
147+
UNIFIED_SETTINGS_PATH,
148+
);
149+
if (primaryRecord) {
150+
return primaryRecord;
151+
}
152+
} catch (error) {
153+
const backupRecord = await readSettingsRecordAsyncFromPath(
154+
UNIFIED_SETTINGS_BACKUP_PATH,
155+
);
156+
if (backupRecord) {
157+
return backupRecord;
158+
}
159+
throw error;
81160
}
82161

83-
const raw = await fs.readFile(UNIFIED_SETTINGS_PATH, "utf8");
84-
const parsed = cloneRecord(JSON.parse(raw));
85-
if (!parsed) {
86-
throw new Error("Unified settings must contain a JSON object at the root.");
87-
}
88-
return parsed;
162+
return readSettingsRecordAsyncFromPath(UNIFIED_SETTINGS_BACKUP_PATH);
89163
}
90164

91165
/**
@@ -125,6 +199,7 @@ function writeSettingsRecordSync(record: JsonRecord): void {
125199
const payload = normalizeForWrite(record);
126200
const data = `${JSON.stringify(payload, null, 2)}\n`;
127201
const tempPath = `${UNIFIED_SETTINGS_PATH}.${process.pid}.${Date.now()}.tmp`;
202+
trySnapshotUnifiedSettingsBackupSync();
128203
writeFileSync(tempPath, data, "utf8");
129204
let moved = false;
130205
try {
@@ -176,6 +251,7 @@ async function writeSettingsRecordAsync(record: JsonRecord): Promise<void> {
176251
const payload = normalizeForWrite(record);
177252
const data = `${JSON.stringify(payload, null, 2)}\n`;
178253
const tempPath = `${UNIFIED_SETTINGS_PATH}.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2, 8)}.tmp`;
254+
await trySnapshotUnifiedSettingsBackupAsync();
179255
await fs.writeFile(tempPath, data, "utf8");
180256
let moved = false;
181257
try {

‎test/config-save.test.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -135,11 +135,11 @@ describe("plugin config save paths", () => {
135135
parallelProbingMaxConcurrency: 7,
136136
});
137137

138-
const loaded = loadPluginConfig();
139-
expect(loaded.codexMode).toBe(false);
140-
expect(loaded.parallelProbing).toBe(true);
141-
expect(loaded.parallelProbingMaxConcurrency).toBe(7);
142-
});
138+
const loaded = loadPluginConfig();
139+
expect(loaded.codexMode).toBe(false);
140+
expect(loaded.parallelProbing).toBe(true);
141+
expect(loaded.parallelProbingMaxConcurrency).toBe(2);
142+
});
143143

144144
it("resolves parallel probing settings and clamps concurrency", async () => {
145145
const { getParallelProbing, getParallelProbingMaxConcurrency } =

0 commit comments

Comments
 (0)