From 9fdb5e37b4aa3847e9f88701d89d817edb003811 Mon Sep 17 00:00:00 2001 From: chbndrhnns <7534547+chbndrhnns@users.noreply.github.com> Date: Tue, 18 Aug 2026 19:11:58 +0300 Subject: [PATCH 1/2] fix(services): merge environment on partial service update instead of replacing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PATCH /api/projects/:id/services/:serviceId ran patch.environment through unmaskEnv, which builds its result from the incoming keys alone. A body naming one variable therefore deleted every variable it did not mention, so the documented "send only what changes" (docs/api/services.mdx) silently destroyed the rest of the map. That is worse here than on the endpoint's other collection fields because `environment` is the only one MASKED on read, and env-reveal is deliberately kept off the automation surface: a raw-API or MCP client cannot see what it is about to overwrite and cannot read the values back afterwards. It is the same pairing the project-level PUT /:id/env was removed for ("it could wipe/corrupt masked secrets"); service-env masking then arrived without the merge path. mergeServiceEnv merges onto the stored map with the same absent/null/present triad as mergeAdvanced, plus the sentinel arm only a masked field needs: an absent key is kept, an explicit null removes one, `null` clears the map, `{}` changes nothing, and an echoed •••••••• restores the stored value. unmaskEnv is untouched and keeps its whole-map semantics for the six writers that genuinely own the whole set (create, compose sync, project-crud, migration, deploy) — each rebuilds the map from an upstream spec and must be able to drop a variable the spec no longer declares. `environment` is nullable on update ONLY, for the reason already spelled out for domain/customDomain in the same object literal: absent means "keep", so "clear" needs its own spelling. Create and sync keep Record. Env-tab saves now name their removed keys explicitly via serviceEnvPatch, which lives next to the other dashboard payload builders and is tested there — a client that submits only its current rows can no longer no-op a deletion while reporting success. Fixes #619 --- apps/api/src/lib/secret-env.ts | 48 +++++++ .../src/modules/services/service.schema.ts | 19 +++ .../src/modules/services/service.service.ts | 16 ++- apps/api/test/lib/secret-env.test.ts | 68 ++++++++++ .../services/service-update-env.test.ts | 126 ++++++++++++++++++ .../services/ServiceDetailPanel.tsx | 5 +- apps/dashboard/src/lib/api/services.ts | 9 +- .../src/lib/service-env-payload.test.ts | 58 ++++++++ apps/dashboard/src/lib/service-env-payload.ts | 32 +++++ 9 files changed, 373 insertions(+), 8 deletions(-) create mode 100644 apps/api/test/modules/services/service-update-env.test.ts create mode 100644 apps/dashboard/src/lib/service-env-payload.test.ts create mode 100644 apps/dashboard/src/lib/service-env-payload.ts diff --git a/apps/api/src/lib/secret-env.ts b/apps/api/src/lib/secret-env.ts index 2ccd94f9b..3565af642 100644 --- a/apps/api/src/lib/secret-env.ts +++ b/apps/api/src/lib/secret-env.ts @@ -62,6 +62,12 @@ function maskValue(value: string): string { * Keys absent from `incoming` are absent from the result: a full-map write still * deletes keys the client removed. Callers that only want partial semantics must * pre-merge (this is a whole-map replace with sentinel protection). + * + * This is the right helper when the caller genuinely owns the WHOLE set — create, + * compose sync, and the migration/deploy paths, which each rebuild the map from an + * upstream spec and must be able to drop a variable the spec no longer declares. + * The PATCH endpoint wants `mergeServiceEnv` below instead: a partial body there + * would otherwise delete every variable it merely failed to mention. */ export function unmaskEnv( incoming: Record | null | undefined, @@ -81,6 +87,48 @@ export function unmaskEnv( return out; } +/** + * Merge an incoming compose-service `environment` patch onto what's stored, + * preserving untouched variables and restoring masked secrets (#336, #619). + * + * `environment` is the only field on the PATCH endpoint that is masked on read, + * and reveal is deliberately off the automation surface — so a client cannot see + * what a whole-map replace is about to destroy, and cannot read it back. Omission + * therefore has to mean "keep", and removal has to be explicit. Same triad as + * `mergeAdvanced`, plus the sentinel arm that only a masked field needs: + * + * - key absent from patch → leave the stored value alone + * - key value === null → remove it + * - key value === ENV_MASK → keep the stored value (dropped if not in stored) + * - key value === string → update/insert that key + * - incoming === null → clear the entire environment map + * - incoming === undefined → leave the stored map alone + */ +export function mergeServiceEnv( + stored: Record | null | undefined, + incoming: Record | null | undefined, +): Record { + if (incoming === null) return {}; + if (incoming === undefined || typeof incoming !== "object" || Array.isArray(incoming)) { + return { ...(stored ?? {}) }; + } + const base: Record = { ...(stored ?? {}) }; + for (const [key, value] of Object.entries(incoming)) { + if (value === null) { + delete base[key]; + } else if (isMaskedValue(value)) { + if (stored && Object.hasOwn(stored, key)) { + base[key] = stored[key]; + } else { + delete base[key]; + } + } else if (typeof value === "string") { + base[key] = value; + } + } + return base; +} + /** Whether an env map contains any mask sentinel (i.e. an un-revealed value). */ export function hasMaskedValue( env: Record | null | undefined, diff --git a/apps/api/src/modules/services/service.schema.ts b/apps/api/src/modules/services/service.schema.ts index 26bfc3928..7e8805c05 100644 --- a/apps/api/src/modules/services/service.schema.ts +++ b/apps/api/src/modules/services/service.schema.ts @@ -234,6 +234,25 @@ export const UpdateServiceBody = Type.Object( // spelling of absent. domain: Type.Optional(Type.Union([Type.String({ maxLength: 255 }), Type.Null()])), customDomain: Type.Optional(Type.Union([Type.String({ maxLength: 255 }), Type.Null()])), + // #619: same reasoning as the two above, and for the same reason as `advanced` + // — a keyed map on a PATCH has to be able to say "keep" and "clear" separately. + // It matters more here than anywhere else on this endpoint: `environment` is the + // only field MASKED on read, and reveal is off the automation surface, so a + // client that replaced the whole map could neither see nor recover what it + // destroyed. Nullable ONLY on update — Create/Sync own the whole set and keep + // `Record` (the ratchet in test/modules/services/service-schema-env + // pins that asymmetry). The value type stays UNBOUNDED to match them: capping it + // here alone would make a long value (CA chain, base64 kubeconfig) creatable and + // then un-PATCH-able. + environment: Type.Optional( + Type.Union( + [Type.Record(Type.String(), Type.Union([Type.String(), Type.Null()])), Type.Null()], + { + description: + "MERGED onto the stored map, not replaced: omit a key to keep it, set a key to null to remove it, send null to clear every variable. An empty object changes nothing. Echo the •••••••• sentinel to keep a masked value unchanged.", + }, + ), + ), }, { additionalProperties: false }, ); diff --git a/apps/api/src/modules/services/service.service.ts b/apps/api/src/modules/services/service.service.ts index 9c1cfe293..a9399123f 100644 --- a/apps/api/src/modules/services/service.service.ts +++ b/apps/api/src/modules/services/service.service.ts @@ -15,7 +15,7 @@ import { import { scopedVolumeName, type CommandExecutor } from "@repo/adapters"; import { execInContainer } from "../../lib/agent-exec"; import { encrypt, decrypt } from "../../lib/encryption"; -import { ENV_MASK, hasMaskedValue, maskDriftChanges, maskServiceEnv, unmaskEnv } from "../../lib/secret-env"; +import { ENV_MASK, hasMaskedValue, maskDriftChanges, maskServiceEnv, mergeServiceEnv, unmaskEnv } from "../../lib/secret-env"; import { assertNotControlPlane, assertNotControlPlaneById, @@ -694,13 +694,17 @@ export async function updateService( // When domainType changes, clear the irrelevant domain field. const patch: Record = { ...data }; - // #336: env values are masked on read. Restore any sentinel the client echoed - // back to the stored value so editing an unrelated field never overwrites a - // secret with "••••••••". A real (revealed-and-changed) value passes through. + // #336/#619: env values are masked on read, so a client cannot see what it is + // about to overwrite. Merge onto what's stored rather than replacing: a partial + // body used to delete every variable it merely failed to mention, and because + // reveal is off the automation surface (see service.routes.ts) the caller could + // not read those values back. Sentinels the client echoed restore to the stored + // value, so editing an unrelated field never writes "••••••••" over a secret; a + // revealed-and-changed value passes through; an explicit null removes the key. if ("environment" in patch) { - patch.environment = unmaskEnv( - patch.environment, + patch.environment = mergeServiceEnv( svc.environment as Record | null, + patch.environment, ); } diff --git a/apps/api/test/lib/secret-env.test.ts b/apps/api/test/lib/secret-env.test.ts index 376c41d2c..b650b29c6 100644 --- a/apps/api/test/lib/secret-env.test.ts +++ b/apps/api/test/lib/secret-env.test.ts @@ -10,6 +10,7 @@ import { maskScanService, maskServiceEnv, maskServicesEnv, + mergeServiceEnv, unmaskEnv, } from "../../src/lib/secret-env"; @@ -69,6 +70,73 @@ describe("unmaskEnv", () => { }); }); +/** + * The PATCH counterpart (#619). `unmaskEnv` above deletes by omission, which is + * right for the whole-set writers but destroys a masked value the caller could + * never have read back. Here omission means "keep" and a delete is spelled null. + */ +describe("mergeServiceEnv", () => { + const stored = { API_TOKEN: "real-token", DB_PASSWORD: "hunter2", NODE_ENV: "production" }; + + test("a key absent from the patch keeps its stored value", () => { + expect(mergeServiceEnv(stored, { PORT: "8080" })).toEqual({ ...stored, PORT: "8080" }); + }); + test("a named key is overwritten, the rest survive", () => { + expect(mergeServiceEnv(stored, { NODE_ENV: "staging" })).toEqual({ + ...stored, + NODE_ENV: "staging", + }); + }); + test("null removes just that key", () => { + expect(mergeServiceEnv(stored, { NODE_ENV: null, PORT: "3000" })).toEqual({ + API_TOKEN: "real-token", + DB_PASSWORD: "hunter2", + PORT: "3000", + }); + }); + test("sentinel keeps the stored secret", () => { + expect(mergeServiceEnv(stored, { API_TOKEN: ENV_MASK, NEW: "v" })).toEqual({ + ...stored, + NEW: "v", + }); + }); + test("sentinel with no stored counterpart is dropped (never persists dots)", () => { + expect(mergeServiceEnv(stored, { GHOST: ENV_MASK })).toEqual(stored); + expect(mergeServiceEnv(undefined, { A: ENV_MASK, B: "real" })).toEqual({ B: "real" }); + }); + // #472: maskValue leaves "" alone, so an unset variable reads back as empty and + // has to round-trip as empty — masking it would have the merge restore a value + // the operator had just cleared. + test("an empty value round-trips as empty rather than being treated as a sentinel", () => { + expect(mergeServiceEnv({ ...stored, EMPTY: "" }, { EMPTY: "" })).toEqual({ + ...stored, + EMPTY: "", + }); + expect(mergeServiceEnv(stored, { API_TOKEN: "" })).toEqual({ ...stored, API_TOKEN: "" }); + }); + test("null for the whole map clears it; undefined and {} leave it alone", () => { + expect(mergeServiceEnv(stored, null)).toEqual({}); + expect(mergeServiceEnv(stored, undefined)).toEqual(stored); + expect(mergeServiceEnv(stored, {})).toEqual(stored); + }); + test("a non-string value is dropped rather than written into the map", () => { + expect(mergeServiceEnv(stored, { N: 7 as unknown as string })).toEqual(stored); + }); + // The map is attacker-influenced (any PATCH body key), and the sentinel arm asks + // whether a key exists in `stored` — a prototype-chain hit there would resurrect + // an inherited value under an attacker-chosen name. + test("prototype keys neither pollute nor resolve through the chain", () => { + const merged = mergeServiceEnv(stored, { + ["__proto__"]: "polluted", + constructor: ENV_MASK, + toString: ENV_MASK, + }); + expect(merged).toEqual(stored); + expect(({} as Record).polluted).toBeUndefined(); + expect(Object.prototype.hasOwnProperty.call(merged, "toString")).toBe(false); + }); +}); + describe("helpers", () => { test("isMaskedValue / hasMaskedValue", () => { expect(isMaskedValue(ENV_MASK)).toBe(true); diff --git a/apps/api/test/modules/services/service-update-env.test.ts b/apps/api/test/modules/services/service-update-env.test.ts new file mode 100644 index 000000000..8fddda88f --- /dev/null +++ b/apps/api/test/modules/services/service-update-env.test.ts @@ -0,0 +1,126 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { ENV_MASK } from "@repo/core"; + +const projectRepo = vi.hoisted(() => ({ findById: vi.fn() })); +const serviceRepo = vi.hoisted(() => ({ + findById: vi.fn(), + update: vi.fn(), + listByProject: vi.fn(), +})); + +vi.mock("@repo/db", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + repos: { ...actual.repos, project: projectRepo, service: serviceRepo }, + }; +}); + +import { updateService } from "../../../src/modules/services/service.service"; + +const ctx = { organizationId: "org_1" } as never; +const project = { id: "proj_1", organizationId: "org_1", internalAlias: null }; + +const initialEnv = { + GEMINI_API_KEY: "secret-gemini-key-12345", + TEST_AUTH: "true", + DATABASE_URL: "postgres://user:pass@db:5432/inventar", + MARKET_ENDPOINT: "https://market.inventar.example.com", + BUILD_REVISION: "v1.2.0", +}; + +const row = (over: Record = {}) => ({ + id: "svc_inventar", + projectId: project.id, + name: "inventar", + kind: "compose", + image: "inventar:latest", + environment: { ...initialEnv }, + ports: [], + restart: "unless-stopped", + enabled: true, + exposed: false, + ...over, +}); + +const written = () => serviceRepo.update.mock.calls.at(-1)?.[1] as Record; + +beforeEach(() => { + projectRepo.findById.mockReset().mockResolvedValue(project); + serviceRepo.findById.mockReset().mockResolvedValue(row()); + serviceRepo.update.mockReset().mockResolvedValue(undefined); + serviceRepo.listByProject.mockReset().mockResolvedValue([]); +}); + +describe("updateService — environment partial updates merge rather than replace", () => { + it("preserves untouched environment variables when applying a single-field probe or partial update", async () => { + await updateService(ctx, project.id, "svc_inventar", { + environment: { + PROBE_FIELD: "test", + }, + } as never); + + expect(written().environment).toEqual({ + ...initialEnv, + PROBE_FIELD: "test", + }); + }); + + it("updates existing variable while preserving all other variables", async () => { + await updateService(ctx, project.id, "svc_inventar", { + environment: { + BUILD_REVISION: "v1.2.1", + }, + } as never); + + expect(written().environment).toEqual({ + ...initialEnv, + BUILD_REVISION: "v1.2.1", + }); + }); + + it("deletes a variable when explicitly set to null while preserving other variables", async () => { + await updateService(ctx, project.id, "svc_inventar", { + environment: { + BUILD_REVISION: null, + }, + } as never); + + expect(written().environment).toEqual({ + GEMINI_API_KEY: "secret-gemini-key-12345", + TEST_AUTH: "true", + DATABASE_URL: "postgres://user:pass@db:5432/inventar", + MARKET_ENDPOINT: "https://market.inventar.example.com", + }); + }); + + it("restores masked secret sentinels to their stored values", async () => { + await updateService(ctx, project.id, "svc_inventar", { + environment: { + GEMINI_API_KEY: ENV_MASK, + NEW_KEY: "new_value", + }, + } as never); + + expect(written().environment).toEqual({ + ...initialEnv, + NEW_KEY: "new_value", + }); + }); + + it("clears the entire environment map when explicitly passed null", async () => { + await updateService(ctx, project.id, "svc_inventar", { + environment: null, + } as never); + + expect(written().environment).toEqual({}); + }); + + it("leaves environment unchanged when environment is not mentioned in patch", async () => { + await updateService(ctx, project.id, "svc_inventar", { + restart: "always", + } as never); + + expect(written()).not.toHaveProperty("environment"); + }); +}); diff --git a/apps/dashboard/src/app/(dashboard)/projects/[id]/components/services/ServiceDetailPanel.tsx b/apps/dashboard/src/app/(dashboard)/projects/[id]/components/services/ServiceDetailPanel.tsx index 1dda3d171..1cf2d8229 100644 --- a/apps/dashboard/src/app/(dashboard)/projects/[id]/components/services/ServiceDetailPanel.tsx +++ b/apps/dashboard/src/app/(dashboard)/projects/[id]/components/services/ServiceDetailPanel.tsx @@ -16,6 +16,7 @@ import { } from "@/lib/api/services"; import { deployApi } from "@/lib/api/deploy"; import { formatBytes } from "@/lib/formatBytes"; +import { serviceEnvPatch } from "@/lib/service-env-payload"; import { internalServiceAddress, effectiveServiceAlias, type ComposeAdvanced } from "@repo/core"; import { serviceDisplayUrl } from "@/utils/route-display"; import { @@ -256,7 +257,9 @@ export function ServiceDetailPanel({ setEnvSaving(true); try { const result = await servicesApi.update(projectId, service.id, { - environment: envRecordFromRows(envRows), + // Merge semantics on the API side: name the removed keys as null, or + // deleting a variable here would silently no-op (#619). + environment: serviceEnvPatch(envRows, service.environment), }); if (!result.success) throw new Error(t.projectDetail.services.detail.toast.envSaveFailed); await onRefresh(); diff --git a/apps/dashboard/src/lib/api/services.ts b/apps/dashboard/src/lib/api/services.ts index 6a808af59..5b92dbe5f 100644 --- a/apps/dashboard/src/lib/api/services.ts +++ b/apps/dashboard/src/lib/api/services.ts @@ -175,7 +175,14 @@ export type ServiceInput = { dockerfile?: string; ports?: string[]; dependsOn?: string[]; - environment?: Record; + /** + * Nulls are UPDATE-only — a key set to null removes it, and `null` clears the + * map (#619). Create and sync own the whole set and take `Record`, + * so a null there is rejected by the API validator. Widened on the shared input + * for the same reason `advanced` carries the nullable `ComposeAdvancedPatch`: + * one payload shape for both verbs beats two near-identical types. + */ + environment?: Record | null; volumes?: string[]; command?: string; restart?: string; diff --git a/apps/dashboard/src/lib/service-env-payload.test.ts b/apps/dashboard/src/lib/service-env-payload.test.ts new file mode 100644 index 000000000..50d7505ec --- /dev/null +++ b/apps/dashboard/src/lib/service-env-payload.test.ts @@ -0,0 +1,58 @@ +import { describe, expect, it } from "vitest"; +import { ENV_MASK } from "@repo/core"; +import { serviceEnvPatch } from "./service-env-payload"; + +/** + * The API merges this map (#619), so the client half decides whether a deletion + * happens at all: a payload built from the current rows alone leaves every removed + * variable in place, and the operator gets a success toast for a delete that never + * happened. Every case here is about a key that must come out as `null`. + */ + +const rows = (...pairs: [string, string][]) => pairs.map(([key, value]) => ({ key, value })); + +describe("serviceEnvPatch", () => { + it("nulls a key that was loaded but is no longer in the rows", () => { + expect(serviceEnvPatch(rows(["KEEP", "1"]), { KEEP: ENV_MASK, GONE: ENV_MASK })).toEqual({ + KEEP: "1", + GONE: null, + }); + }); + + it("treats a rename as add-plus-remove", () => { + expect(serviceEnvPatch(rows(["NEW_NAME", "v"]), { OLD_NAME: ENV_MASK })).toEqual({ + NEW_NAME: "v", + OLD_NAME: null, + }); + }); + + it("nulls a key whose row was blanked, and never emits an empty key", () => { + expect(serviceEnvPatch(rows([" ", "v"]), { WAS_HERE: ENV_MASK })).toEqual({ + WAS_HERE: null, + }); + }); + + it("passes the mask sentinel through untouched so the API restores the stored value", () => { + expect(serviceEnvPatch(rows(["SECRET", ENV_MASK]), { SECRET: ENV_MASK })).toEqual({ + SECRET: ENV_MASK, + }); + }); + + it("keeps an empty value as empty rather than dropping the key", () => { + expect(serviceEnvPatch(rows(["BLANK", ""]), { BLANK: "" })).toEqual({ BLANK: "" }); + }); + + it("trims keys and emits nothing to delete when there was no stored map", () => { + expect(serviceEnvPatch(rows([" PADDED ", "v"]))).toEqual({ PADDED: "v" }); + expect(serviceEnvPatch([], null)).toEqual({}); + }); + + // A row that re-adds a key under its original name must not be nulled by the + // deletion pass — `in` is checked against the emitted patch, not the row list. + it("does not null a key that is still present", () => { + expect(serviceEnvPatch(rows(["A", "1"], ["B", "2"]), { A: ENV_MASK, B: ENV_MASK })).toEqual({ + A: "1", + B: "2", + }); + }); +}); diff --git a/apps/dashboard/src/lib/service-env-payload.ts b/apps/dashboard/src/lib/service-env-payload.ts new file mode 100644 index 000000000..7d340b0ba --- /dev/null +++ b/apps/dashboard/src/lib/service-env-payload.ts @@ -0,0 +1,32 @@ +/** + * Build the `environment` value for a service PATCH (#619). + * + * The endpoint MERGES this map onto the stored one, so a key the payload doesn't + * mention is KEPT. An editor that submits only its current rows therefore cannot + * express a deletion — the removed key simply survives. Every key that was there + * when the panel loaded and is gone from the rows now has to be named explicitly + * as `null`. + * + * `originalEnv` is the map the panel loaded. Its VALUES arrive masked (`••••••••`, + * see the api's secret-env.ts) but its KEYS are complete, which is all this needs + * — `maskEnv` iterates `Object.keys` and drops nothing. + */ +export type ServiceEnvRow = { key: string; value: string }; + +export function serviceEnvPatch( + rows: ServiceEnvRow[], + originalEnv?: Record | null, +): Record { + const out: Record = {}; + for (const row of rows) { + // Blanking a row's key is how the editor spells "drop this variable", so an + // empty key contributes nothing and the original it replaced falls through + // to the deletion pass below. + const key = row.key.trim(); + if (key) out[key] = row.value; + } + for (const key of Object.keys(originalEnv ?? {})) { + if (!(key in out)) out[key] = null; + } + return out; +} From 3a2090417271689fc7349c7f00f74058b7cc24fa Mon Sep 17 00:00:00 2001 From: Hydra Date: Tue, 18 Aug 2026 19:12:20 +0300 Subject: [PATCH 2/2] docs(api): state the service PATCH merge contract where clients actually read it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The merge semantics are only useful if a client can discover them, and the two surfaces that mislead are the ones nobody edits by hand: the MCP tool schema and the public API reference. The PATCH route is MCP-exposed and mcp-tools.ts emits UpdateServiceBody verbatim as the tool inputSchema, so an agent saw `environment` as a bare string map behind the description "Update a service's configuration." with destructiveHint: false — nothing in the advertised surface distinguished merge from replace, and after the merge change an agent's natural revocation attempt (omit the key) returns 200 OK while the credential stays injected. The route description and the field's TypeBox description now spell out the absent/null/clear triad, the •••••••• round-trip, and — the part that is easy to get wrong in the other direction — that every OTHER field on this endpoint still replaces wholesale, so ports/volumes/dependsOn/publicEndpoints need the complete list. docs/api/services.mdx gets the same table under "Update a service"; its create-side type stays Record, which is what create accepts. service-schema-env pins both halves of the asymmetry. Moving the nullable field into ComposeFieldsBlock (the obvious dedupe) would teach create and sync to accept nulls that unmaskEnv writes through as dropped keys, and the update override only holds because it is declared after the spread — reordering the object literal reverts it with no other symptom. The value type is also pinned UNBOUNDED: capping it on update alone, as the first cut did at 10000, makes a long value (CA chain, base64 kubeconfig) creatable via POST or /sync and then permanently un-PATCH-able. --- .../src/modules/services/service.routes.ts | 5 +- .../services/service-schema-env.test.ts | 67 +++++++++++++++++++ apps/web/content/docs/api/services.mdx | 20 +++++- 3 files changed, 90 insertions(+), 2 deletions(-) create mode 100644 apps/api/test/modules/services/service-schema-env.test.ts diff --git a/apps/api/src/modules/services/service.routes.ts b/apps/api/src/modules/services/service.routes.ts index 0a3b28ba1..22b74f962 100644 --- a/apps/api/src/modules/services/service.routes.ts +++ b/apps/api/src/modules/services/service.routes.ts @@ -141,7 +141,10 @@ r.patch( { tag: "project:service:write", body: UpdateServiceBody, - mcp: { description: "Update a service's configuration." }, + mcp: { + description: + "Update a service's configuration. Partial: an omitted field is left alone. `environment` and `advanced` are MERGED onto the stored values rather than replacing them — omit a key to keep it, set a key to null to remove it, send null for the whole field to clear it. Env values read back masked as `••••••••`; echo the sentinel to keep one unchanged. Every other field (`ports`, `volumes`, `dependsOn`, `publicEndpoints`, …) REPLACES its stored value wholesale, so send the complete list.", + }, }, cloudProjectProxy, ctrl.update, diff --git a/apps/api/test/modules/services/service-schema-env.test.ts b/apps/api/test/modules/services/service-schema-env.test.ts new file mode 100644 index 000000000..b41dcc125 --- /dev/null +++ b/apps/api/test/modules/services/service-schema-env.test.ts @@ -0,0 +1,67 @@ +import { Value } from "@sinclair/typebox/value"; +import { describe, expect, it } from "vitest"; +import { ENV_MASK } from "@repo/core"; +import { + CreateServiceBody, + SyncServicesBody, + UpdateServiceBody, +} from "../../../src/modules/services/service.schema"; + +/** + * `environment` is nullable on UPDATE only (#619), and that asymmetry is load-bearing + * rather than incidental: on a partial patch, absent means "keep", so a delete needs + * its own spelling — while create and compose sync each rebuild the whole map from an + * upstream spec, where a null key would just be a second way to write "absent". + * + * Both halves are pinned here because both are easy to break by accident. Spreading + * the nullable field into `ComposeFieldsBlock` (the obvious "dedupe") would silently + * teach create and sync to accept nulls that `unmaskEnv` writes straight through as + * dropped keys. And the update override is only in effect because it is declared + * AFTER the spread — reordering the object literal reverts it with no other symptom. + */ + +const checkUpdate = (env: unknown) => Value.Check(UpdateServiceBody, { environment: env }); +const checkCreate = (env: unknown) => Value.Check(CreateServiceBody, { name: "db", environment: env }); +const checkSync = (env: unknown) => + Value.Check(SyncServicesBody, { services: [{ name: "db", environment: env }] }); + +describe("UpdateServiceBody.environment", () => { + it("accepts a per-key null (remove that variable) and a null map (clear everything)", () => { + expect(checkUpdate({ KEEP: "1", DROP: null })).toBe(true); + expect(checkUpdate(null)).toBe(true); + }); + + it("accepts plain strings, the mask sentinel, and an empty map", () => { + expect(checkUpdate({ NODE_ENV: "production" })).toBe(true); + expect(checkUpdate({ API_TOKEN: ENV_MASK })).toBe(true); + expect(checkUpdate({})).toBe(true); + }); + + it("still rejects a non-string, non-null value", () => { + expect(checkUpdate({ PORT: 8080 })).toBe(false); + expect(checkUpdate({ NESTED: { a: 1 } })).toBe(false); + }); + + // The cap on the sibling env surfaces is 10000, but applying it here alone would + // make a long value (CA chain, base64 kubeconfig) creatable via POST or /sync and + // then permanently un-PATCH-able. Update stays as unbounded as they are. + it("does not cap value length more tightly than create and sync do", () => { + const long = "x".repeat(20_000); + expect(checkUpdate({ BUNDLE: long })).toBe(true); + expect(checkCreate({ BUNDLE: long })).toBe(true); + }); +}); + +describe("create and sync stay non-nullable", () => { + it("CreateServiceBody rejects a null env value and a null map", () => { + expect(checkCreate({ DROP: null })).toBe(false); + expect(checkCreate(null)).toBe(false); + expect(checkCreate({ NODE_ENV: "production" })).toBe(true); + }); + + it("SyncServicesBody rejects a null env value and a null map", () => { + expect(checkSync({ DROP: null })).toBe(false); + expect(checkSync(null)).toBe(false); + expect(checkSync({ NODE_ENV: "production" })).toBe(true); + }); +}); diff --git a/apps/web/content/docs/api/services.mdx b/apps/web/content/docs/api/services.mdx index a45eb0135..70c94be3d 100644 --- a/apps/web/content/docs/api/services.mdx +++ b/apps/web/content/docs/api/services.mdx @@ -71,7 +71,7 @@ and optional; the `kind` decides which subset is the source of truth. Unknown ke dockerfile: { type: 'string', description: 'Dockerfile path relative to the build context (max 500).' }, ports: { type: 'string[]', description: 'Port mappings like "8080:80" (max 50 entries, each ≤100 chars).' }, dependsOn: { type: 'string[]', description: 'Names of services this one depends on (max 50).' }, - environment: { type: 'Record', description: 'Compose environment defaults baked into the service.' }, + environment: { type: 'Record', description: 'Compose environment defaults baked into the service. On update this map is merged, not replaced, and accepts nulls — see Update a service.' }, volumes: { type: 'string[]', description: 'Volume mappings (max 50 entries, each ≤500 chars).' }, command: { type: 'string', description: 'Override the container command (max 1000).' }, advanced: { type: 'object', description: 'Extended compose settings. Currently a healthcheck block: { test, interval, timeout, retries, startPeriod, disable }. Unknown keys are rejected.' }, @@ -123,6 +123,24 @@ Every field is optional — send only what changes. Accepts the same compose and **except `kind`**: switching a row's kind is a destructive operation, so delete the service and re-create it under the new kind instead. +Two fields are **merged** onto their stored value instead of replacing it — `environment` and `advanced`, +the endpoint's keyed maps: + +| You send | Result | +| --- | --- | +| the key is absent | the stored value is kept | +| `{"KEY": "value"}` | that one key is set; every other key is untouched | +| `{"KEY": null}` | that one key is removed | +| `{"environment": null}` | every variable is removed | +| `{"environment": {}}` | nothing changes | + +Env values read back masked as `••••••••` (`GET /:serviceId` never returns plaintext). Echo the sentinel to +keep a value unchanged, or send a real string to replace it. Because a masked value can't be read back +outside the dashboard, **omitting a key never deletes it** — a delete has to be spelled `null`. + +Every other field replaces its stored value wholesale, `ports`, `volumes`, `dependsOn` and +`publicEndpoints` included, so send the complete list for those. + ```bash curl -X PATCH https://your-host/api/projects/proj_123/services/svc_456 \ -H "Authorization: Bearer $OPENSHIP_TOKEN" \