History

fix(server): preserve inline provider secrets on redacted saves (#10054)

Maxwell Young committed 2 months ago

Commit ac4f1a21 parent 00328922 files · +136 −3Browse files

Unified · Split

2 files
  1. apps/server/src/serverSettings.test.ts +122 −0
  2. apps/server/src/serverSettings.ts +14 −3
apps/server/src/serverSettings.test.ts+122−0
@@ -1059,6 +1059,128 @@ it.layer(NodeServices.layer)("server settings", (it) => { · 1058 unmodified lines
10591059 }).pipe(Effect.provide(makeServerSettingsLayer())),
10601060 );
10611061
1062+ it.effect("keeps the inline value on disk when secret migration fails", () => {
1063+ const cause = new ServerSecretStore.SecretStorePersistError({
1064+ resource: "provider environment secret",
1065+ cause: new Error("Secret storage unavailable"),
1066+ });
1067+ const secretLayer = Layer.effect(
1068+ ServerSecretStore.ServerSecretStore,
1069+ Effect.map(ServerSecretStore.ServerSecretStore, (store) => ({
1070+ ...store,
1071+ set: () => Effect.fail(cause),
1072+ })),
1073+ ).pipe(Layer.provide(ServerSecretStore.layer));
1074+ const settingsLayer = ServerSettingsModule.layer.pipe(
1075+ Layer.provide(secretLayer),
1076+ Layer.provideMerge(Layer.fresh(SqlitePersistenceMemory)),
1077+ Layer.provideMerge(
1078+ Layer.fresh(
1079+ ServerConfig.layerTest(process.cwd(), {
1080+ prefix: "t3code-inline-secret-failure-test-",
1081+ }),
1082+ ),
1083+ ),
1084+ );
1085+ return Effect.gen(function* () {
1086+ const instanceId = ProviderInstanceId.make("codex_personal");
1087+ const service = yield* ServerSettingsModule.ServerSettingsService;
1088+ const config = yield* ServerConfig.ServerConfig;
1089+ const fs = yield* FileSystem.FileSystem;
1090+ const original =
1091+ '{"providerInstances":{"codex_personal":{"driver":"codex","environment":[{"name":"API_TOKEN","value":"inline-test-token","sensitive":true}],"config":{}}}}';
1092+ yield* fs.writeFileString(config.settingsPath, original);
1093+ const error = yield* Effect.flip(
1094+ service.updateSettings({
1095+ providerInstances: {
1096+ [instanceId]: {
1097+ driver: ProviderDriverKind.make("codex"),
1098+ environment: [{ name: "API_TOKEN", value: "", sensitive: true, valueRedacted: true }],
1099+ config: {},
1100+ },
1101+ },
1102+ }),
1103+ );
1104+ assert.equal(error.operation, "write-secret");
1105+ assert.strictEqual(error.cause, cause);
1106+ assert.equal(yield* fs.readFileString(config.settingsPath), original);
1107+ const settings = yield* service.getSettings;
1108+ assert.equal(
1109+ settings.providerInstances[instanceId]?.environment?.[0]?.value,
1110+ "inline-test-token",
1111+ );
1112+ }).pipe(Effect.provide(settingsLayer));
1113+ });
1114+
1115+ for (const { label, variable, expected, duplicate } of [
1116+ {
1117+ label: "preserves an inline secret on a redacted settings save",
1118+ variable: { name: "API_TOKEN", value: "", sensitive: true, valueRedacted: true },
1119+ expected: "inline-test-token",
1120+ },
1121+ {
1122+ label: "preserves the effective last inline secret when names are duplicated",
1123+ variable: { name: "API_TOKEN", value: "", sensitive: true, valueRedacted: true },
1124+ expected: "last-inline-test-token",
1125+ duplicate: true,
1126+ },
1127+ {
1128+ label: "replaces an inline secret with an explicit value",
1129+ variable: { name: "API_TOKEN", value: "replacement-test-token", sensitive: true },
1130+ expected: "replacement-test-token",
1131+ },
1132+ {
1133+ label: "clears an inline secret with an explicit empty value",
1134+ variable: { name: "API_TOKEN", value: "", sensitive: true },
1135+ expected: "",
1136+ },
1137+ ]) {
1138+ it.effect(label, () =>
1139+ Effect.gen(function* () {
1140+ const instanceId = ProviderInstanceId.make("codex_personal");
1141+ const serverSettings = yield* ServerSettingsModule.ServerSettingsService;
1142+ const serverConfig = yield* ServerConfig.ServerConfig;
1143+ const fileSystem = yield* FileSystem.FileSystem;
1144+ yield* fileSystem.writeFileString(
1145+ serverConfig.settingsPath,
1146+ duplicate
1147+ ? '{"providerInstances":{"codex_personal":{"driver":"codex","environment":[{"name":"API_TOKEN","value":"inline-test-token","sensitive":true},{"name":"API_TOKEN","value":"last-inline-test-token","sensitive":true}],"config":{}}}}'
1148+ : '{"providerInstances":{"codex_personal":{"driver":"codex","environment":[{"name":"API_TOKEN","value":"inline-test-token","sensitive":true}],"config":{}}}}',
1149+ );
1150+ const initial = yield* serverSettings.getSettings;
1151+ assert.equal(
1152+ initial.providerInstances[instanceId]?.environment?.[0]?.value,
1153+ "inline-test-token",
1154+ );
1155+
1156+ const next = yield* serverSettings.updateSettings({
1157+ providerInstances: {
1158+ [instanceId]: {
1159+ driver: ProviderDriverKind.make("codex"),
1160+ displayName: "Renamed provider",
1161+ environment: duplicate ? [variable, variable] : [variable],
1162+ config: {},
1163+ },
1164+ },
1165+ });
1166+ assert.equal(next.providerInstances[instanceId]?.environment?.[0]?.value, expected);
1167+ const raw = yield* fileSystem.readFileString(serverConfig.settingsPath);
1168+ assert.notInclude(raw, "inline-test-token");
1169+ assert.notInclude(raw, "replacement-test-token");
1170+
1171+ const reloaded = yield* Effect.gen(function* () {
1172+ const fresh = yield* ServerSettingsModule.ServerSettingsService;
1173+ return yield* fresh.getSettings;
1174+ }).pipe(
1175+ Effect.provide(
1176+ Layer.fresh(ServerSettingsModule.layer).pipe(Layer.provide(ServerSecretStore.layer)),
1177+ ),
1178+ );
1179+ assert.equal(reloaded.providerInstances[instanceId]?.environment?.[0]?.value, expected);
1180+ }).pipe(Effect.provide(makeServerSettingsLayer())),
1181+ );
1182+ }
1183+
10621184 it.effect("stores sensitive provider instance environment values outside settings.json", () =>
10631185 Effect.gen(function* () {
10641186 const serverSettings = yield* ServerSettingsModule.ServerSettingsService;
apps/server/src/serverSettings.ts+14−3
@@ -616,9 +616,20 @@ const make = Effect.gen(function* () { · 615 unmodified lines
616616 }
617617
618618 nextSecretKeys.add(secretName);
619− if (!variable.valueRedacted) {
620− if (variable.value.length > 0) {
621− yield* secretStore.set(secretName, textEncoder.encode(variable.value)).pipe(
619+ // Match the provider environment's last-value-wins behavior for duplicate names.
620+ const previous = variable.valueRedacted
621+ ? current.providerInstances[ProviderInstanceId.make(instanceId)]?.environment?.findLast(
622+ (entry) => entry.name === variable.name,
623+ )
624+ : undefined;
625+ const inlineValue =
626+ previous?.sensitive && !previous.valueRedacted && previous.value.length > 0
627+ ? previous.value
628+ : undefined;
629+ const value = inlineValue ?? variable.value;
630+ if (!variable.valueRedacted || inlineValue !== undefined) {
631+ if (value.length > 0) {
632+ yield* secretStore.set(secretName, textEncoder.encode(value)).pipe(
622633 Effect.mapError(
623634 (cause) =>
624635 new ServerSettingsError({