Skip to content

Commit 6b0dbea

Browse files
ndycodendycode
committed
fix: harden settings backup and env warnings
1 parent addf0ae commit 6b0dbea

3 files changed

Lines changed: 99 additions & 32 deletions

File tree

‎lib/config.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -645,7 +645,7 @@ function resolveBooleanSetting(
645645
envValue === undefined
646646
) {
647647
logConfigWarnOnce(
648-
`Ignoring invalid boolean env ${envName}=${JSON.stringify(rawEnvValue)}. Expected 0/1, true/false, or yes/no.`,
648+
`Ignoring invalid boolean env ${envName}. Expected 0/1, true/false, or yes/no.`,
649649
);
650650
}
651651
if (envValue !== undefined) return envValue;
@@ -677,7 +677,7 @@ function resolveNumberSetting(
677677
envValue === undefined
678678
) {
679679
logConfigWarnOnce(
680-
`Ignoring invalid numeric env ${envName}=${JSON.stringify(rawEnvValue)}. Expected a finite number.`,
680+
`Ignoring invalid numeric env ${envName}. Expected a finite number.`,
681681
);
682682
}
683683
const candidate = envValue ?? configValue ?? defaultValue;

‎lib/unified-settings.ts‎

Lines changed: 61 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,11 @@ const UNIFIED_SETTINGS_BACKUP_PATH = `${UNIFIED_SETTINGS_PATH}.bak`;
2121
const RETRYABLE_FS_CODES = new Set(["EBUSY", "EPERM"]);
2222
const TRANSIENT_READ_FS_CODES = new Set(["EBUSY", "EAGAIN"]);
2323
let settingsWriteQueue: Promise<void> = Promise.resolve();
24-
let backupDerivedReadPending = false;
24+
25+
type SettingsReadResult = {
26+
record: JsonRecord | null;
27+
usedBackup: boolean;
28+
};
2529

2630
function isRetryableFsError(error: unknown): boolean {
2731
const code = (error as NodeJS.ErrnoException | undefined)?.code;
@@ -137,8 +141,10 @@ function shouldFallbackToSettingsBackup(
137141
* When the current in-memory state was recovered from backup, snapshotting is
138142
* skipped so a corrupt primary cannot overwrite the only known-good backup.
139143
*/
140-
function trySnapshotUnifiedSettingsBackupSync(): void {
141-
if (backupDerivedReadPending) {
144+
function trySnapshotUnifiedSettingsBackupSync(options?: {
145+
skipBecauseBackupRead?: boolean;
146+
}): void {
147+
if (options?.skipBecauseBackupRead) {
142148
return;
143149
}
144150
if (!existsSync(UNIFIED_SETTINGS_PATH)) {
@@ -160,8 +166,10 @@ function trySnapshotUnifiedSettingsBackupSync(): void {
160166
/**
161167
* Async variant of `trySnapshotUnifiedSettingsBackupSync`.
162168
*/
163-
async function trySnapshotUnifiedSettingsBackupAsync(): Promise<void> {
164-
if (backupDerivedReadPending) {
169+
async function trySnapshotUnifiedSettingsBackupAsync(options?: {
170+
skipBecauseBackupRead?: boolean;
171+
}): Promise<void> {
172+
if (options?.skipBecauseBackupRead) {
165173
return;
166174
}
167175
if (!existsSync(UNIFIED_SETTINGS_PATH)) {
@@ -190,26 +198,29 @@ async function trySnapshotUnifiedSettingsBackupAsync(): Promise<void> {
190198
* - Windows: file locking on Windows may cause reads to fail; in those cases this function returns `null`.
191199
* - 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.
192200
*/
193-
function readSettingsRecordSync(): JsonRecord | null {
201+
function readSettingsRecordSyncInternal(): SettingsReadResult {
194202
const primaryExists = existsSync(UNIFIED_SETTINGS_PATH);
195203
try {
196204
const primaryRecord = readSettingsRecordSyncFromPath(UNIFIED_SETTINGS_PATH);
197205
if (primaryRecord) {
198-
return primaryRecord;
206+
return { record: primaryRecord, usedBackup: false };
199207
}
200208
} catch (error) {
201209
if (!shouldFallbackToSettingsBackup(primaryExists, error)) {
202210
throw error;
203211
}
204212
const backupRecord = readSettingsBackupSync();
205213
if (backupRecord) {
206-
backupDerivedReadPending = true;
207-
return backupRecord;
214+
return { record: backupRecord, usedBackup: true };
208215
}
209216
throw error;
210217
}
211218

212-
return null;
219+
return { record: null, usedBackup: false };
220+
}
221+
222+
function readSettingsRecordSync(): JsonRecord | null {
223+
return readSettingsRecordSyncInternal().record;
213224
}
214225

215226
/**
@@ -219,28 +230,31 @@ function readSettingsRecordSync(): JsonRecord | null {
219230
*
220231
* @returns The parsed settings record as an object clone, or `null` if unavailable or invalid.
221232
*/
222-
async function readSettingsRecordAsync(): Promise<JsonRecord | null> {
233+
async function readSettingsRecordAsyncInternal(): Promise<SettingsReadResult> {
223234
const primaryExists = existsSync(UNIFIED_SETTINGS_PATH);
224235
try {
225236
const primaryRecord = await readSettingsRecordAsyncFromPath(
226237
UNIFIED_SETTINGS_PATH,
227238
);
228239
if (primaryRecord) {
229-
return primaryRecord;
240+
return { record: primaryRecord, usedBackup: false };
230241
}
231242
} catch (error) {
232243
if (!shouldFallbackToSettingsBackup(primaryExists, error)) {
233244
throw error;
234245
}
235246
const backupRecord = await readSettingsBackupAsync();
236247
if (backupRecord) {
237-
backupDerivedReadPending = true;
238-
return backupRecord;
248+
return { record: backupRecord, usedBackup: true };
239249
}
240250
throw error;
241251
}
242252

243-
return null;
253+
return { record: null, usedBackup: false };
254+
}
255+
256+
async function readSettingsRecordAsync(): Promise<JsonRecord | null> {
257+
return (await readSettingsRecordAsyncInternal()).record;
244258
}
245259

246260
/**
@@ -275,20 +289,24 @@ function normalizeForWrite(record: JsonRecord): JsonRecord {
275289
*
276290
* @param record - The settings object to persist; it will be normalized to include the unified settings version.
277291
*/
278-
function writeSettingsRecordSync(record: JsonRecord): void {
292+
function writeSettingsRecordSync(
293+
record: JsonRecord,
294+
options?: { skipBackupSnapshot?: boolean },
295+
): void {
279296
mkdirSync(getCodexMultiAuthDir(), { recursive: true });
280297
const payload = normalizeForWrite(record);
281298
const data = `${JSON.stringify(payload, null, 2)}\n`;
282299
const tempPath = `${UNIFIED_SETTINGS_PATH}.${process.pid}.${Date.now()}.tmp`;
283-
trySnapshotUnifiedSettingsBackupSync();
300+
trySnapshotUnifiedSettingsBackupSync({
301+
skipBecauseBackupRead: options?.skipBackupSnapshot,
302+
});
284303
writeFileSync(tempPath, data, "utf8");
285304
let moved = false;
286305
try {
287306
for (let attempt = 0; attempt < 5; attempt += 1) {
288307
try {
289308
renameSync(tempPath, UNIFIED_SETTINGS_PATH);
290309
moved = true;
291-
backupDerivedReadPending = false;
292310
return;
293311
} catch (error) {
294312
if (!isRetryableFsError(error) || attempt >= 4) {
@@ -328,20 +346,24 @@ function writeSettingsRecordSync(record: JsonRecord): void {
328346
*
329347
* @param record - The settings object to persist; it will be normalized (version set)
330348
*/
331-
async function writeSettingsRecordAsync(record: JsonRecord): Promise<void> {
349+
async function writeSettingsRecordAsync(
350+
record: JsonRecord,
351+
options?: { skipBackupSnapshot?: boolean },
352+
): Promise<void> {
332353
await fs.mkdir(getCodexMultiAuthDir(), { recursive: true });
333354
const payload = normalizeForWrite(record);
334355
const data = `${JSON.stringify(payload, null, 2)}\n`;
335356
const tempPath = `${UNIFIED_SETTINGS_PATH}.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2, 8)}.tmp`;
336-
await trySnapshotUnifiedSettingsBackupAsync();
357+
await trySnapshotUnifiedSettingsBackupAsync({
358+
skipBecauseBackupRead: options?.skipBackupSnapshot,
359+
});
337360
await fs.writeFile(tempPath, data, "utf8");
338361
let moved = false;
339362
try {
340363
for (let attempt = 0; attempt < 5; attempt += 1) {
341364
try {
342365
await fs.rename(tempPath, UNIFIED_SETTINGS_PATH);
343366
moved = true;
344-
backupDerivedReadPending = false;
345367
return;
346368
} catch (error) {
347369
if (!isRetryableFsError(error) || attempt >= 4) {
@@ -413,9 +435,12 @@ export function loadUnifiedPluginConfigSync(): JsonRecord | null {
413435
* @param pluginConfig - Key/value map representing plugin configuration to persist
414436
*/
415437
export function saveUnifiedPluginConfigSync(pluginConfig: JsonRecord): void {
416-
const record = readSettingsRecordSync() ?? {};
417-
record.pluginConfig = { ...pluginConfig };
418-
writeSettingsRecordSync(record);
438+
const { record, usedBackup } = readSettingsRecordSyncInternal();
439+
const nextRecord = record ?? {};
440+
nextRecord.pluginConfig = { ...pluginConfig };
441+
writeSettingsRecordSync(nextRecord, {
442+
skipBackupSnapshot: usedBackup,
443+
});
419444
}
420445

421446
/**
@@ -430,9 +455,12 @@ export function saveUnifiedPluginConfigSync(pluginConfig: JsonRecord): void {
430455
*/
431456
export async function saveUnifiedPluginConfig(pluginConfig: JsonRecord): Promise<void> {
432457
await enqueueSettingsWrite(async () => {
433-
const record = await readSettingsRecordAsync() ?? {};
434-
record.pluginConfig = { ...pluginConfig };
435-
await writeSettingsRecordAsync(record);
458+
const { record, usedBackup } = await readSettingsRecordAsyncInternal();
459+
const nextRecord = record ?? {};
460+
nextRecord.pluginConfig = { ...pluginConfig };
461+
await writeSettingsRecordAsync(nextRecord, {
462+
skipBackupSnapshot: usedBackup,
463+
});
436464
});
437465
}
438466

@@ -473,8 +501,11 @@ export async function saveUnifiedDashboardSettings(
473501
dashboardDisplaySettings: JsonRecord,
474502
): Promise<void> {
475503
await enqueueSettingsWrite(async () => {
476-
const record = await readSettingsRecordAsync() ?? {};
477-
record.dashboardDisplaySettings = { ...dashboardDisplaySettings };
478-
await writeSettingsRecordAsync(record);
504+
const { record, usedBackup } = await readSettingsRecordAsyncInternal();
505+
const nextRecord = record ?? {};
506+
nextRecord.dashboardDisplaySettings = { ...dashboardDisplaySettings };
507+
await writeSettingsRecordAsync(nextRecord, {
508+
skipBackupSnapshot: usedBackup,
509+
});
479510
});
480511
}

‎test/config-save.test.ts‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,42 @@ describe("plugin config save paths", () => {
190190
}
191191
});
192192

193+
it("redacts raw values from invalid numeric env override warnings", async () => {
194+
process.env.CODEX_AUTH_PARALLEL_PROBING_MAX_CONCURRENCY = "secret-value";
195+
vi.resetModules();
196+
const logWarnMock = vi.fn();
197+
198+
vi.doMock("../lib/logger.js", async () => {
199+
const actual =
200+
await vi.importActual<typeof import("../lib/logger.js")>(
201+
"../lib/logger.js",
202+
);
203+
return {
204+
...actual,
205+
logWarn: logWarnMock,
206+
};
207+
});
208+
209+
try {
210+
const { getParallelProbingMaxConcurrency } = await import(
211+
"../lib/config.js"
212+
);
213+
expect(
214+
getParallelProbingMaxConcurrency({ parallelProbingMaxConcurrency: 4 }),
215+
).toBe(4);
216+
expect(logWarnMock).toHaveBeenCalledWith(
217+
expect.stringContaining(
218+
"Ignoring invalid numeric env CODEX_AUTH_PARALLEL_PROBING_MAX_CONCURRENCY.",
219+
),
220+
);
221+
expect(logWarnMock).not.toHaveBeenCalledWith(
222+
expect.stringContaining("secret-value"),
223+
);
224+
} finally {
225+
vi.doUnmock("../lib/logger.js");
226+
}
227+
});
228+
193229
it("normalizes fallback chain and drops invalid entries", async () => {
194230
const { getUnsupportedCodexFallbackChain } =
195231
await import("../lib/config.js");

0 commit comments

Comments
 (0)