Skip to content

Commit 06d888f

Browse files
committed
fix(admin): show env values and fix env-override locking in system settings
- Show the actual value for non-secret env-overridden settings instead of "(set by environment variable)". Redact secrets server-side regardless of source so raw env-sourced secrets (e.g. MAPTILER_KEY) no longer reach the browser. - Derive the panel's "env overrides" badge and Save-disabled state from the visible settings only, so a setting hidden by its showWhen predicate (e.g. the MapTiler key while the provider is self-hosted) no longer flags an otherwise-editable field. - Remove the stray Accordion ::before divider line floating above each card.
1 parent ef19338 commit 06d888f

4 files changed

Lines changed: 206 additions & 14 deletions

File tree

apps/api/src/routes/__tests__/admin-settings.test.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,35 @@ describe("GET /admin/settings", () => {
147147
selectResolveWith = [];
148148
});
149149

150+
it("redacts env-sourced secrets but exposes non-secret env values", async () => {
151+
const origKey = process.env.MAPTILER_KEY;
152+
const origLocale = process.env.DEFAULT_LOCALE;
153+
process.env.MAPTILER_KEY = "super-secret-key";
154+
process.env.DEFAULT_LOCALE = "de";
155+
156+
try {
157+
const res = await app.inject({ method: "GET", url: "/admin/settings" });
158+
const body = res.json();
159+
160+
// An env-sourced secret is flagged as set + locked, but its raw value
161+
// must never reach the browser.
162+
const map = body.groups.find((g: { id: string }) => g.id === "map");
163+
const apiKey = map.settings.find((s: { key: string }) => s.key === "maptilerApiKey");
164+
expect(apiKey).toMatchObject({ source: "env", envOverride: true, secret: true });
165+
expect(apiKey.value).not.toBe("super-secret-key");
166+
167+
// A non-secret env override exposes its real value so the UI can show it.
168+
const general = body.groups.find((g: { id: string }) => g.id === "general");
169+
const locale = general.settings.find((s: { key: string }) => s.key === "defaultLocale");
170+
expect(locale).toMatchObject({ value: "de", source: "env", envOverride: true });
171+
} finally {
172+
if (origKey === undefined) delete process.env.MAPTILER_KEY;
173+
else process.env.MAPTILER_KEY = origKey;
174+
if (origLocale === undefined) delete process.env.DEFAULT_LOCALE;
175+
else process.env.DEFAULT_LOCALE = origLocale;
176+
}
177+
});
178+
150179
it("rejects unauthenticated requests with 401", async () => {
151180
mockRequireAdmin.mockRejectedValueOnce(
152181
Object.assign(new Error("Authentication required"), { statusCode: 401 }),

apps/api/src/routes/admin-settings.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -374,8 +374,11 @@ export async function resolveSettings(): Promise<SettingsGroup[]> {
374374
source = "default";
375375
}
376376

377-
if (def.secret && source !== "env" && value !== "") {
378-
value = source === "database" ? "***" : "";
377+
// Never send a raw secret to the client, whatever its source. "***" is a
378+
// "configured" sentinel so the UI can show the field as set + locked
379+
// without exposing the value (the real value stays in env / the DB).
380+
if (def.secret && value !== "") {
381+
value = "***";
379382
}
380383

381384
if (!grouped[def.group]) grouped[def.group] = [];

apps/web/src/components/admin/settings/SystemSettings.tsx

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -215,20 +215,26 @@ function SettingField({
215215
);
216216
}
217217

218+
// Secrets resolved from an env var are redacted server-side ("***") and must
219+
// never be displayed — show a readable note instead of masked dots. Every
220+
// other field (including non-secret env overrides) shows its real value so
221+
// the operator can see what's actually in effect.
222+
const isEnvSecret = setting.envOverride && setting.secret;
223+
const displayValue = isEnvSecret ? "(set by environment variable)" : String(value ?? "");
224+
const inputType = isEnvSecret
225+
? "text"
226+
: setting.secret && !showSecret
227+
? "password"
228+
: setting.type === "number"
229+
? "number"
230+
: "text";
231+
218232
return (
219233
<TextField
220234
label={setting.label}
221235
size="small"
222-
type={
223-
setting.secret && !showSecret ? "password" : setting.type === "number" ? "number" : "text"
224-
}
225-
value={
226-
disabled
227-
? setting.envOverride
228-
? "(set by environment variable)"
229-
: String(value ?? "")
230-
: String(value ?? "")
231-
}
236+
type={inputType}
237+
value={displayValue}
232238
disabled={disabled}
233239
helperText={setting.description}
234240
onChange={(e) =>
@@ -304,8 +310,12 @@ function SettingsGroupPanel({
304310
},
305311
});
306312

307-
const hasEnvOverrides = group.settings.some((s) => s.envOverride);
308313
const visibleSettings = group.settings.filter((s) => isVisible(s, localValues));
314+
// Reflect only the settings the operator can actually see: a setting hidden
315+
// by its showWhen predicate (e.g. the MapTiler key while the provider is
316+
// self-hosted) must not flag the whole panel as env-overridden, or the badge
317+
// contradicts an otherwise-editable field.
318+
const hasEnvOverrides = visibleSettings.some((s) => s.envOverride);
309319

310320
return (
311321
<Accordion
@@ -318,6 +328,10 @@ function SettingsGroupPanel({
318328
// specificity to override every corner on every panel; without the
319329
// double-class scope our plain `&` rule loses the cascade.
320330
sx={{
331+
// MUI draws a top divider via an ::before pseudo-element meant for
332+
// flush-stacked accordions. With our spaced-out outlined cards it just
333+
// floats as a stray line above each one — remove it.
334+
"&::before": { display: "none" },
321335
"&.MuiAccordion-rounded": {
322336
borderRadius: 1,
323337
"&:first-of-type, &:last-of-type": { borderRadius: 1 },
@@ -443,7 +457,7 @@ function SettingsGroupPanel({
443457
save.isPending ? <CircularProgress size={14} color="inherit" /> : <SaveIcon />
444458
}
445459
onClick={() => save.mutate()}
446-
disabled={save.isPending || group.settings.every((s) => s.envOverride)}
460+
disabled={save.isPending || visibleSettings.every((s) => s.envOverride)}
447461
>
448462
Save {group.label}
449463
</Button>
Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
// @vitest-environment jsdom
2+
3+
import { afterEach, describe, expect, it, vi } from "vitest";
4+
import { createQueryWrapper, render, screen } from "@/test";
5+
6+
vi.mock("@/lib/EnvProvider", () => ({
7+
useEnv: () => ({ apiUrl: "http://test.local" }),
8+
}));
9+
10+
const fetchMock = vi.fn();
11+
vi.stubGlobal("fetch", fetchMock);
12+
13+
import { SystemSettings } from "../SystemSettings";
14+
15+
interface TestSetting {
16+
group: string;
17+
key: string;
18+
label: string;
19+
description?: string;
20+
type: string;
21+
options?: string[];
22+
secret: boolean;
23+
value: unknown;
24+
source: "default" | "database" | "env";
25+
envVar?: string;
26+
envOverride: boolean;
27+
showWhen?: { key: string; equals: unknown };
28+
}
29+
30+
// The panel renders expanded only for the "general" group, so every fixture
31+
// uses that id to make its fields (not just the summary badge) queryable.
32+
function mockGroup(label: string, settings: TestSetting[]) {
33+
fetchMock.mockResolvedValue({
34+
ok: true,
35+
json: async () => ({ groups: [{ id: "general", label, settings }] }),
36+
});
37+
}
38+
39+
afterEach(() => {
40+
fetchMock.mockReset();
41+
});
42+
43+
describe("SystemSettings env-override handling", () => {
44+
it("hides the env-overrides badge when the only override is hidden by showWhen", async () => {
45+
// Mirrors the Map panel: Style Provider is DB-sourced and visible, while
46+
// the env-overridden MapTiler key is hidden because the provider isn't
47+
// "maptiler". The panel must not claim an env override the operator can't see.
48+
mockGroup("Map", [
49+
{
50+
group: "map",
51+
key: "styleProvider",
52+
label: "Style Provider",
53+
type: "select",
54+
options: ["maptiler", "self-hosted", "custom"],
55+
secret: false,
56+
value: "self-hosted",
57+
source: "database",
58+
envVar: "MAP_STYLE_PROVIDER",
59+
envOverride: false,
60+
},
61+
{
62+
group: "map",
63+
key: "maptilerApiKey",
64+
label: "MapTiler API Key",
65+
type: "string",
66+
secret: true,
67+
value: "***",
68+
source: "env",
69+
envVar: "MAPTILER_KEY",
70+
envOverride: true,
71+
showWhen: { key: "styleProvider", equals: "maptiler" },
72+
},
73+
]);
74+
75+
render(<SystemSettings />, { wrapper: createQueryWrapper() });
76+
77+
await screen.findByRole("button", { name: /save map/i });
78+
expect(screen.queryByText("env overrides")).toBeNull();
79+
});
80+
81+
it("shows the env-overrides badge and disables a visible env-overridden field", async () => {
82+
mockGroup("Data-Use Policy", [
83+
{
84+
group: "policy",
85+
key: "allowGreyArea",
86+
label: "Allow grey-area sources",
87+
type: "boolean",
88+
secret: false,
89+
value: true,
90+
source: "env",
91+
envVar: "OPENMAPX_ALLOW_GREY_AREA",
92+
envOverride: true,
93+
},
94+
]);
95+
96+
render(<SystemSettings />, { wrapper: createQueryWrapper() });
97+
98+
await screen.findByText("Allow grey-area sources");
99+
expect(screen.queryByText("env overrides")).not.toBeNull();
100+
expect((screen.getByRole("switch") as HTMLInputElement).disabled).toBe(true);
101+
});
102+
103+
it("displays the actual value of a non-secret env-overridden setting", async () => {
104+
mockGroup("General", [
105+
{
106+
group: "general",
107+
key: "instanceUrl",
108+
label: "Instance URL",
109+
type: "string",
110+
secret: false,
111+
value: "https://maps.example.com",
112+
source: "env",
113+
envVar: "PUBLIC_URL",
114+
envOverride: true,
115+
},
116+
]);
117+
118+
render(<SystemSettings />, { wrapper: createQueryWrapper() });
119+
120+
const input = (await screen.findByLabelText("Instance URL")) as HTMLInputElement;
121+
expect(input.value).toBe("https://maps.example.com");
122+
expect(input.disabled).toBe(true);
123+
});
124+
125+
it("does not render a secret env value in the field", async () => {
126+
mockGroup("Email", [
127+
{
128+
group: "email",
129+
key: "smtpPassword",
130+
label: "Password",
131+
type: "string",
132+
secret: true,
133+
value: "***",
134+
source: "env",
135+
envVar: "SMTP_PASS",
136+
envOverride: true,
137+
},
138+
]);
139+
140+
render(<SystemSettings />, { wrapper: createQueryWrapper() });
141+
142+
const input = (await screen.findByLabelText("Password")) as HTMLInputElement;
143+
expect(input.disabled).toBe(true);
144+
expect(input.value).not.toBe("***");
145+
});
146+
});

0 commit comments

Comments
 (0)