Follow-up: GET /api/settings leaks the settings path and stack on an unreadable settings file

[justin808]

Context

Found while reviewing PR #157 (issue #128, GET /api/attention). That pull request's independent review caught the same defect on /api/doctor, where the route was newly added, and fixed it there. GET /api/settings has carried the identical pattern since before this batch, so it is out of that PR's scope and is filed here instead.

Problem

GET /api/settings calls readDashboardSettings with no error handling:

app.get("/api/settings", async (_req, res) => {
  res.json(await readDashboardSettings(persistedSettingsPath, { targetRepos: config.targetRepos }));
});

readDashboardSettings returns its fallback only for ENOENT; for a settings file that exists but is unparseable, holds zero valid targets, or cannot be read (EACCES), it throws Could not read dashboard settings at <absolute path>: <parser message> (src/server/settings.ts:31-37). Express 5 forwards a rejected async handler to its default error handler, which answers 500 with an HTML page carrying that message, the absolute settings path, and server stack frames.

Unlike /api/doctor, this route has no loopback check, so it is served to every host in ALLOWED_HOSTS. On a dashboard reachable from anything other than localhost, a broken or unreadable settings file discloses a filesystem path and internal stack frames to any client that can reach the port. The failure is also unhelpful to the operator: the settings page shows a generic fetch error rather than a usable message.

Proposed fix

Mirror what PR #157 did for the doctor route: keep the endpoint answering, or answer a generic error, but never the raw message.

app.get("/api/settings", async (_req, res) => {
  try {
    res.json(await readDashboardSettings(persistedSettingsPath, { targetRepos: config.targetRepos }));
  } catch {
    res.status(500).json({ error: "Dashboard settings could not be read." });
  }
});

Falling back to { targetRepos: config.targetRepos } is the other option, but it is worse here than for the doctor: the settings page would silently show configured defaults as if they were saved values, and a subsequent PUT would overwrite the unreadable file. Prefer the explicit generic 500 so the operator learns the file is broken without learning where it lives.

Consider the same treatment for PUT /api/settings (src/server/app.ts:183), which can throw from writeDashboardSettings for the same class of reasons.

Test

In src/server/app.test.ts, alongside the doctor case added by PR #157 (still answers a report when the saved settings cannot be read):

it("does not leak the settings path when the saved settings cannot be read", async () => {
  await writeFile(settingsPath(stateRoot), "{ this is not json", "utf8");
  const response = await request(app).get("/api/settings");
  expect(response.status).toBe(500);
  expect(JSON.stringify(response.body)).not.toContain(stateRoot);
  expect(response.text).not.toContain("Could not read dashboard settings");
  expect(response.text).not.toContain("src/server/settings.ts");
});

Evidence

• src/server/app.ts:179-180 — unguarded readDashboardSettings in the GET /api/settings handler, with no loopback check on the route.
• src/server/settings.ts:31-37 — the non-ENOENT throw whose message embeds the absolute path.
• PR Serve GET /api/attention and report the attention read scope from the doctor #157's round-0 independent review verified the behavior by running the app against a settings file containing { this is not json: /api/doctor and /api/settings both answered 500 with the absolute path and stack in an HTML body, while /api/attention answered its generic {"error":"Attention payload could not be built."} with no leak.
• express 5.2.1 is the installed version; it forwards rejected async handler promises to the default error handler.

Source: PR #157, batch acd-attention-1a-20260906, tracker #134.