Skip to content

Commit 59597ac

Browse files
d-csTrigger.dev RepoOps
d-cs
authored and
Trigger.dev RepoOps
committed
fix(webapp): preserve Vercel values when empty writes are enabled
Keep blank Vercel imports from overwriting existing values when the default-off empty environment variable feature is enabled. Mono-RevId: 46b3866fb5682306bce50a89af04a21178cd0214
1 parent 39d66e0 commit 59597ac

4 files changed

Lines changed: 168 additions & 29 deletions

File tree

‎.server-changes/fix-vercel-empty-env-imports.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
area: webapp
3+
type: fix
4+
---
5+
6+
Keep blank Vercel imports from overwriting existing values when the default-off empty environment variable feature is enabled.

‎apps/webapp/app/models/vercelIntegration.server.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1395,6 +1395,11 @@ export class VercelIntegrationRepository {
13951395
}
13961396

13971397
const varsToSync = mergedEnvVars.filter((envVar) => {
1398+
// Keep the flag-off path unchanged. When enabled, discard blanks
1399+
// after merging so a blank project override cannot select a shared fallback.
1400+
if (allowEmptyValues && envVar.value.trim() === "") {
1401+
return false;
1402+
}
13981403
if (envVar.isSecret) {
13991404
return false;
14001405
}

‎apps/webapp/app/v3/environmentVariables/emptyEnvironmentValuesRollout.test.ts‎

Lines changed: 152 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -127,14 +127,18 @@ async function fixture(prisma: PrismaClient) {
127127
describe("empty env rollout through production boundaries", () => {
128128
for (const emptyValue of ["", " \t "]) {
129129
postgresTest(
130-
`Vercel pull respects off/on/isolation/rollback for ${JSON.stringify(emptyValue)}`,
130+
`Vercel pull skips blanks regardless of the direct-write flag for ${JSON.stringify(emptyValue)}`,
131131
async ({ prisma }) => {
132132
const operations: string[] = [];
133+
let rejectGlobalFlagRead = false;
133134
const observed = prisma.$extends({
134135
query: {
135136
$allModels: {
136137
$allOperations: async ({ model, operation, args, query }) => {
137138
operations.push(`${model}.${operation}`);
139+
if (rejectGlobalFlagRead && model === "FeatureFlag" && operation === "findFirst") {
140+
throw new Error("flag lookup unavailable");
141+
}
138142
return query(args);
139143
},
140144
},
@@ -168,37 +172,51 @@ describe("empty env rollout through production boundaries", () => {
168172
await f.repository.create(f.project.id, {
169173
override: true,
170174
environmentIds: [f.environment.id],
171-
variables: [{ key: "PLAIN", value: "original" }],
175+
variables: [
176+
{ key: "PLAIN", value: "original" },
177+
{ key: "SENSITIVE", value: "original-secret" },
178+
],
172179
lastUpdatedBy: { type: "user", userId: f.user.id },
173180
});
181+
// The existing global flag lookup must still fail before provider reads.
182+
rejectGlobalFlagRead = true;
183+
expect((await pull()).isErr()).toBe(true);
184+
expect(external.calls).toEqual([]);
185+
rejectGlobalFlagRead = false;
174186
operations.length = 0;
175187
const off = await pull();
176188
expect(off.isOk()).toBe(true);
177189
if (off.isOk()) expect(off.value.errors).toEqual([]);
178190
expect(await f.read()).toEqual({
179191
PLAIN: "original",
192+
SENSITIVE: "original-secret",
180193
ENCRYPTED: "encrypted-shared-fallback",
181194
COLLISION: "shared-fallback",
182195
ORDINARY: "updated",
183196
});
184197
expect(operations.filter((op) => op === "FeatureFlag.findFirst")).toHaveLength(1);
198+
// Set a value distinct from the provider fallback. An enabled blank project
199+
// override must be skipped after merging, not replaced with a shared value.
200+
await f.repository.create(f.project.id, {
201+
override: true,
202+
environmentIds: [f.environment.id],
203+
variables: [{ key: "COLLISION", value: "keep-collision" }],
204+
});
185205
await f.setFlag(true);
186206
external.calls.length = 0;
187207
const on = await pull();
188208
expect(on.isOk()).toBe(true);
189209
if (on.isOk()) expect(on.value.errors).toEqual([]);
190210
expect(await f.read()).toEqual({
191-
PLAIN: emptyValue,
192-
ENCRYPTED: emptyValue,
193-
COLLISION: emptyValue,
194-
SHARED: emptyValue,
211+
PLAIN: "original",
212+
SENSITIVE: "original-secret",
213+
ENCRYPTED: "encrypted-shared-fallback",
214+
COLLISION: "keep-collision",
195215
ORDINARY: "updated",
196216
});
197217
expect(external.calls.filter((p) => p === "/v1/env")).toHaveLength(1);
198218
expect(external.calls.filter((p) => p.endsWith("/encrypted"))).toHaveLength(1);
199-
expect(external.calls.some((p) => p.endsWith("/shared") || p.endsWith("/sensitive"))).toBe(
200-
false
201-
);
219+
expect(external.calls.some((p) => p.endsWith("/sensitive"))).toBe(false);
202220
operations.length = 0;
203221
const repeat = await pull();
204222
expect(repeat.isOk() && repeat.value.syncedCount).toBe(0);
@@ -214,29 +232,28 @@ describe("empty env rollout through production boundaries", () => {
214232
ENCRYPTED: "encrypted-shared-fallback",
215233
ORDINARY: "updated",
216234
});
235+
// An explicit false override keeps the original off-path fallback policy
236+
// and bypasses the global flag lookup, even when that lookup would fail.
237+
await f.setFlag(false);
238+
rejectGlobalFlagRead = true;
239+
operations.length = 0;
240+
expect((await pull()).isOk()).toBe(true);
241+
expect((await f.read()).COLLISION).toBe("shared-fallback");
242+
expect(operations.filter((op) => op === "FeatureFlag.findFirst")).toHaveLength(0);
243+
rejectGlobalFlagRead = false;
217244
external.deny();
218245
const failed = await pull();
219246
expect(failed.isOk() && failed.value.errors.length).toBeGreaterThan(0);
220-
expect((await f.read()).PLAIN).toBe(emptyValue);
247+
expect((await f.read()).PLAIN).toBe("original");
221248
await f.setFlag(false);
222249
const stored = await f.read();
223-
expect(stored.PLAIN).toBe(emptyValue);
224-
const child = spawnSync(
225-
process.execPath,
226-
[
227-
"-e",
228-
'process.stdout.write(JSON.stringify({present:Object.hasOwn(process.env,"PLAIN"),empty:process.env.PLAIN===""}))',
229-
],
230-
{ env: { ...process.env, ...stored }, encoding: "utf8" }
231-
);
232-
expect(child.status).toBe(0);
233-
expect(JSON.parse(child.stdout)).toEqual({ present: true, empty: emptyValue === "" });
250+
expect(stored.PLAIN).toBe("original");
234251
}
235252
);
236253
}
237254

238255
postgresTest(
239-
"REST update and billing-shaped import enforce the current organization flag",
256+
"direct REST writes allow empty values when enabled while Vercel imports preserve existing values",
240257
async ({ prisma }) => {
241258
const f = await fixture(prisma);
242259
const params = { projectRef: f.project.externalRef, slug: "prod", name: "REST_EMPTY" };
@@ -252,7 +269,12 @@ describe("empty env rollout through production boundaries", () => {
252269
await f.repository.create(f.project.id, {
253270
override: true,
254271
environmentIds: [f.environment.id],
255-
variables: [{ key: "REST_EMPTY", value: "original" }],
272+
variables: [
273+
{ key: "REST_EMPTY", value: "original" },
274+
{ key: "BILLING_EMPTY", value: "original" },
275+
{ key: "BILLING_WHITESPACE", value: "original" },
276+
{ key: "SENSITIVE", value: "original-secret" },
277+
],
256278
});
257279
const capability = async () => {
258280
const response = await readCapability({ params, context: {}, request: request("GET") });
@@ -265,24 +287,94 @@ describe("empty env rollout through production boundaries", () => {
265287
const off = await update();
266288
expect(off?.status).toBe(400);
267289
expect((await f.read()).REST_EMPTY).toBe("original");
290+
const protectedKeys = ["BILLING_EMPTY", "BILLING_WHITESPACE", "SENSITIVE"];
291+
const snapshotProtectedValues = async () => {
292+
const rows = await prisma.environmentVariableValue.findMany({
293+
where: {
294+
environmentId: f.environment.id,
295+
variable: { OR: protectedKeys.map((key) => ({ key })) },
296+
},
297+
include: { valueReference: true },
298+
orderBy: { id: "asc" },
299+
});
300+
const secrets = await prisma.secretStore.findMany({
301+
where: { OR: rows.map((row) => ({ key: row.valueReference!.key })) },
302+
orderBy: { key: "asc" },
303+
});
304+
return { rows, secrets };
305+
};
306+
const beforeSync = await snapshotProtectedValues();
268307
const billingBody = {
269-
variables: { BILLING_EMPTY: "", BILLING_NORMAL: "updated" },
308+
variables: {
309+
BILLING_EMPTY: "",
310+
BILLING_WHITESPACE: " \t ",
311+
SENSITIVE: "",
312+
NEW_EMPTY: "",
313+
BILLING_NORMAL: "updated",
314+
},
270315
override: true,
271316
source: { type: "integration", integration: "vercel" },
272317
};
273318
const sync = () =>
274319
importVariables({ params, context: {}, request: request("POST", billingBody) });
275320
expect((await sync()).status).toBe(200);
276-
expect((await f.read()).BILLING_EMPTY).toBeUndefined();
321+
expect((await f.read()).BILLING_EMPTY).toBe("original");
277322
await f.setFlag(true);
278323
expect((await capability()).allowEmptyEnvironmentVariableValues).toBe(true);
279324
expect((await update())?.status).toBe(200);
280-
expect((await sync()).status).toBe(200);
325+
// Old billing may still send blanks; retries/concurrent requests must not
326+
// alter stored ciphertext, versions, row IDs, timestamps or provenance.
327+
for (const result of await Promise.all([sync(), sync(), sync()])) {
328+
expect(result.status).toBe(200);
329+
}
330+
expect(await snapshotProtectedValues()).toEqual(beforeSync);
281331
const response = await readVariable({ params, context: {}, request: request("GET") });
282332
expect(response.status).toBe(200);
283333
expect(await response.json()).toMatchObject({ name: "REST_EMPTY", value: "" });
284-
expect((await f.read()).BILLING_EMPTY).toBe("");
334+
expect(await f.read()).toMatchObject({
335+
REST_EMPTY: "",
336+
BILLING_EMPTY: "original",
337+
BILLING_WHITESPACE: "original",
338+
SENSITIVE: "original-secret",
339+
BILLING_NORMAL: "updated",
340+
});
341+
expect((await f.read()).NEW_EMPTY).toBeUndefined();
342+
const blankOnly = await importVariables({
343+
params,
344+
context: {},
345+
request: request("POST", {
346+
...billingBody,
347+
variables: { BILLING_EMPTY: "", NEW_EMPTY: "" },
348+
}),
349+
});
350+
expect(blankOnly.status).toBe(200);
351+
expect(await snapshotProtectedValues()).toEqual(beforeSync);
352+
expect((await f.read()).BILLING_EMPTY).toBe("original");
353+
expect((await f.read()).NEW_EMPTY).toBeUndefined();
354+
const directImport = await importVariables({
355+
params,
356+
context: {},
357+
request: request("POST", {
358+
variables: { DIRECT_IMPORT_EMPTY: "" },
359+
override: true,
360+
}),
361+
});
362+
expect(directImport.status).toBe(200);
363+
expect((await f.read()).DIRECT_IMPORT_EMPTY).toBe("");
364+
const stored = await f.read();
365+
const child = spawnSync(
366+
process.execPath,
367+
[
368+
"-e",
369+
'process.stdout.write(JSON.stringify({present:Object.hasOwn(process.env,"REST_EMPTY"),empty:process.env.REST_EMPTY===""}))',
370+
],
371+
{ env: { ...process.env, ...stored }, encoding: "utf8" }
372+
);
373+
expect(child.status).toBe(0);
374+
expect(JSON.parse(child.stdout)).toEqual({ present: true, empty: true });
285375
await f.setFlag(false);
376+
expect((await sync()).status).toBe(200);
377+
expect(await snapshotProtectedValues()).toEqual(beforeSync);
286378
expect((await update())?.status).toBe(400);
287379
const rolledBack = await capability();
288380
expect(rolledBack.allowEmptyEnvironmentVariableValues).toBe(false);
@@ -291,3 +383,36 @@ describe("empty env rollout through production boundaries", () => {
291383
}
292384
);
293385
});
386+
387+
postgresTest(
388+
"Vercel blank imports preserve the existing flag-read failure boundary",
389+
async ({ prisma }) => {
390+
let fail = false;
391+
const observed = prisma.$extends({
392+
query: {
393+
featureFlag: {
394+
findFirst: async ({ args, query }) => {
395+
if (fail) throw new Error("flag lookup unavailable");
396+
return query(args);
397+
},
398+
},
399+
},
400+
}) as unknown as PrismaClient;
401+
const f = await fixture(observed);
402+
await f.repository.create(f.project.id, {
403+
override: true,
404+
environmentIds: [f.environment.id],
405+
variables: [{ key: "PRESERVE", value: "original" }],
406+
});
407+
fail = true;
408+
await expect(
409+
f.repository.create(f.project.id, {
410+
override: true,
411+
environmentIds: [f.environment.id],
412+
variables: [{ key: "PRESERVE", value: "" }],
413+
lastUpdatedBy: { type: "integration", integration: "vercel" },
414+
})
415+
).rejects.toThrow("flag lookup unavailable");
416+
expect((await f.read()).PRESERVE).toBe("original");
417+
}
418+
);

‎apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -119,10 +119,13 @@ export class EnvironmentVariablesRepository implements Repository {
119119
values = values.filter((v) => v.key.trim() !== "");
120120
if (
121121
values.some((v) => v.value.trim() === "") &&
122-
!(await emptyEnvironmentVariableValuesEnabled(
122+
// Resolve the existing flag first, preserving its reads and failure behavior.
123+
(!(await emptyEnvironmentVariableValuesEnabled(
123124
project.organization.featureFlags,
124125
this.prismaClient
125-
))
126+
)) ||
127+
(options.lastUpdatedBy?.type === "integration" &&
128+
options.lastUpdatedBy.integration === "vercel"))
126129
) {
127130
values = values.filter((v) => v.value.trim() !== "");
128131
// Automated sync must remain a no-op when every supplied value is disabled.

0 commit comments

Comments
 (0)