Skip to content

fix(api): return public-safe errors from the tunnel and MITM routes - #11872

Merged
diegosouzapw merged 2 commits into
cheaper-inference:release/v3.8.51from
pacocartones:fix/oss051-tunnel-error-sanitization
Aug 28, 2026
Merged

diegosouzapw merged 2 commits into
cheaper-inference:release/v3.8.51from
pacocartones:fix/oss051-tunnel-error-sanitization

Conversation

@pacocartones

Copy link
Copy Markdown
Contributor

Summary

Two independent defects on the same surface — src/app/api/tunnels/** and src/app/api/settings/mitm/route.ts.

1. Fourteen catch blocks returned a raw error.message. All fourteen answered with { error: error instanceof Error ? error.message : "<fallback>" }. Those errors come from cloudflared / tailscale / tailscaled / ngrok child processes and from filesystem I/O under the operator's home directory, so the body disclosed host layout, binary install locations, the OS account name and — for Tailscale — live tskey-* credentials. Hard Rule #12 forbids putting a raw err.message in an HTTP body.

File Lines
tunnels/cloudflared/route.ts 32, 70
tunnels/ngrok/route.ts 29, 69
tunnels/tailscale/route.ts 17
tunnels/tailscale/check/route.ts 17
tunnels/tailscale/enable/route.ts 20
tunnels/tailscale/disable/route.ts 20
tunnels/tailscale/login/route.ts 20
tunnels/tailscale/install/route.ts 35 (SSE error event)
tunnels/tailscale/start-daemon/route.ts 23
settings/mitm/route.ts 183, 239, 278

Routing these through the existing sanitizeErrorMessage() does not close it. That helper only rewrites tokens that look like an absolute path ending in a source extension — SOURCE_EXT = ["ts","tsx","js","jsx","mjs","cjs"] in open-sse/utils/error.ts:25 — so every leak shape these routes actually produce survives it verbatim. Measured against the current tip:

IN : ENOENT: no such file or directory, open '/home/operator/.omniroute/data/tunnels.json'
OUT: ENOENT: no such file or directory, open '/home/operator/.omniroute/data/tunnels.json'

IN : spawn /usr/local/bin/cloudflared ENOENT
OUT: spawn /usr/local/bin/cloudflared ENOENT

IN : tailscale up failed: invalid key tskey-auth-kMn3Qz7RtY-9fVbXsPq2LdWc
OUT: tailscale up failed: invalid key tskey-auth-kMn3Qz7RtY-9fVbXsPq2LdWc

IN : Command failed: /opt/omniroute/bin/tailscaled --state=/var/lib/tailscale/tailscaled.state
OUT: Command failed: /opt/omniroute/bin/tailscaled --state=/var/lib/tailscale/tailscaled.state

So this PR adds src/lib/api/publicSafeTunnelError.ts, which returns a route-owned literal plus a coarse machine-readable reason, and logs the real error server-side where the operator can still read it. It never tries to scrub the upstream text — it does not echo it at all. A test pins the sanitizeErrorMessage gap above, so if the shared sanitizer ever grows to cover these shapes the test fails and tells you to simplify this module away.

2. validateBody() has no response field, so three routes returned undefined. validateBody() returns { success, error } (src/shared/validation/helpers.ts:23-26); validatedJsonBody() is the sibling helper that carries a ready-made response. Three call sites did return validation.response anyway — tunnels/cloudflared/route.ts:53, tunnels/ngrok/route.ts:50, and tunnels/tailscale/routeUtils.ts:37 (whose five POST callers then do if ("response" in parsed) return parsed.response, and the key is present, holding undefined). A body that fails validation therefore returned undefined from the handler and Next answered with a framework 500 instead of a 400. They now build the 400 explicitly with formatValidationMessage().

Reachability — stated precisely, and it is not uniform. /api/tunnels/ngrok (both methods), /api/tunnels/tailscale and /api/tunnels/tailscale/check are not in LOCAL_ONLY_API_PREFIXES, and GET /api/tunnels/cloudflared is explicitly exempted through LOCAL_ONLY_API_GET_EXEMPTIONS (src/server/authz/routeGuard.ts:223-226, added by #11531 so remote dashboards keep read-only status). Those bodies can reach a non-loopback caller. Everything else here — the five Tailscale process routes, POST /api/tunnels/cloudflared and all of /api/settings/mitm — is loopback-gated by #11531 and by the MITM entry, and is covered for one consistent contract rather than because it is remotely reachable.

The response shape stays backward compatible: error is still a string, which is what EndpointPageClient.tsx reads (new Error(data?.error || fallback)); reason is added alongside it.

Related Issues

Validation

  • Change type: other — API error contract on existing routes
  • Focused tests and category gates from the golden path
  • npm run lint
  • Reconciled with the current active release base; focused checks rerun afterward
  • Production-code changes include a new or updated automated test in this PR

Rebased onto release/v3.8.51 @ 0ad03316 and every check below re-run on that base.

Focused suite (new file):

$ node --import tsx/esm ... --test tests/unit/tunnel-routes-error-sanitization.test.ts
✔ sanitizeErrorMessage alone leaves every tunnel leak shape intact
✔ toPublicSafeTunnelError never echoes the upstream message
✔ toPublicSafeTunnelError logs the real error server-side
✔ classifyTunnelError maps causes without echoing them
✔ GET /api/tunnels/ngrok does not leak a host path in its 500 body
✔ POST /api/tunnels/ngrok answers 400 (not a framework 500) on an invalid body
✔ POST /api/tunnels/cloudflared answers 400 (not a framework 500) on an invalid body
✔ POST /api/tunnels/tailscale/enable answers 400 (not a framework 500) on an out-of-range port
✔ no tunnel or MITM route echoes a raw error.message any more
ℹ tests 9   ℹ pass 9   ℹ fail 0
exit 0

Red-before-green — same suite with only the route changes reverted (new module kept so the file still imports):

ℹ tests 9   ℹ pass 4   ℹ fail 5
✖ GET /api/tunnels/ngrok does not leak a host path in its 500 body
  AssertionError: body leaked the state path:
  {"error":"ENOENT: no such file or directory, open '/home/operator/.omniroute/data/tunnels.json'"}
✖ POST /api/tunnels/ngrok answers 400 (not a framework 500) on an invalid body
  AssertionError: handler must return a Response, not undefined
✖ POST /api/tunnels/cloudflared answers 400 (not a framework 500) on an invalid body
  AssertionError: handler must return a Response, not undefined
✖ POST /api/tunnels/tailscale/enable answers 400 (not a framework 500) on an out-of-range port
  AssertionError: handler must return a Response, not undefined
✖ no tunnel or MITM route echoes a raw error.message any more
  AssertionError: Hard Rule #12: these lines put a raw error.message in a response body
exit 1

Zone suite — every tunnel, MITM, route-guard and error-sanitization test (62 files):

ℹ tests 332   ℹ pass 332   ℹ fail 0
exit 0

Dashboard consumer of these routes:

$ npx vitest run --config vitest.config.ts "src/app/(dashboard)/dashboard/endpoint/__tests__/EndpointPageClient.test.tsx"
Test Files  1 passed (1)
     Tests  4 passed (4)
exit 0

Gates:

$ npm run typecheck:core
> tsc --pretty false -p tsconfig.typecheck-core.json
exit 0   (no diagnostics)

$ npm run check:changelog-integrity
[changelog-integrity] OK — no base bullets lost vs origin/release/v3.8.51.
exit 0

$ npm run check:route-validation:t06
[t06:route-validation] PASS - 688 route files scanned, all request.json() usages are validated.
exit 0

$ npm run lint
There are suppressions left that do not occur anymore. Consider re-running the command with `--prune-suppressions`.
exit 2

Inherited lint state, declared up front. npm run lint reports zero diagnostics for this branch — the exit 2 comes only from the stale-suppression check. The identical command on the pristine base (release/v3.8.51, no changes applied) produces the same single line and the same exit 2, so it is not introduced here. This PR intentionally does not touch config/quality/eslint-suppressions.json. Same observation as PR #11705.

Tests Added Or Updated

  • tests/unit/tunnel-routes-error-sanitization.test.ts — added, 9 tests.

Coverage Notes

  • Production code changed: src/lib/api/publicSafeTunnelError.ts (new) and eleven route files under src/app/api/tunnels/** plus src/app/api/settings/mitm/route.ts.
  • src/lib/api/publicSafeTunnelError.ts is fully covered: toPublicSafeTunnelError against all five real leak shapes (asserting the secret appears nowhere in the serialized body, not just in error), the server-side log call, and every classifyTunnelError branch including the unknown default.
  • Route wiring is covered three ways rather than by the helper alone: an end-to-end 500 on GET /api/tunnels/ngrok — one of the routes that is not loopback-gated — driven through the real handler via the globalThis.__ngrokListener seam getNgrokTunnelStatus() already reads; end-to-end 400s on the three validateBody call sites; and a source sweep over src/app/api/tunnels/** and src/app/api/settings/mitm/** that fails if any line reintroduces the error instanceof Error ? error.message pattern, which covers the routes that cannot be driven hermetically (the five Tailscale process routes and the MITM CA flow both shell out).
  • No coverage moved down in any touched file.

Reviewer Notes

  • The same validateBody().response bug exists in two more routes outside this PR's scope, and I left them alone rather than widen the diff: src/app/api/settings/cache-config/route.ts:88 and src/app/api/settings/models-dev/route.ts:68. Both do return validation.response on a validateBody() result, so both return undefined and produce a framework 500 where a 400 was intended. Happy to fold them in here or open a separate PR — your call.
  • Why the type checker never caught defect 2. validation is narrowed to { success: false; error }, so validation.response is a genuine TS error. It does not surface because tsconfig.typecheck-core.json is an explicit 27-file allowlist that contains no src/app/api route, and next.config.mjs:321 sets typescript.ignoreBuildErrors: true, so the production build does not typecheck either. Open PR ci: add API route TypeScript regression gate #11705 adds exactly the missing gate and freezes a baseline of 304 pre-existing src/app/api diagnostics — these three are very likely among them. If ci: add API route TypeScript regression gate #11705 lands first, this PR should shrink that baseline rather than conflict with it.
  • Blind spot: npm run check:route-validation:t06 passes identically before and after this fix. scripts/check/check-route-validation.mjs is satisfied by the presence of validateBody( or .safeParse( anywhere in a route file; it never checks that the validated value is the one the handler uses, nor that the failure branch returns a real Response. All three broken call sites call validateBody(...) and so pass the gate while returning undefined. Separately, its REQUEST_JSON_REGEX is /request\.json\s*\(/, which keys on the literal identifier request — six route files under src/app/api call req.json( and are skipped entirely.
  • Classification is deliberately coarse. classifyTunnelError reads the upstream message only to pick one of six fixed labels; the message itself never reaches the client. If you would rather the routes emit no reason at all, dropping it is a one-line change and the response stays backward compatible either way.
  • No migrations, no feature flags. Manual validation worth doing if you want it: stop cloudflared, hit GET /api/tunnels/cloudflared and confirm the body is the fixed sentence while the real cause is in the server log.

Two independent defects on the same surface.

1. Fourteen catch blocks in src/app/api/tunnels/** and
   src/app/api/settings/mitm/route.ts answered with
   `{ error: error instanceof Error ? error.message : "<fallback>" }`. Those
   errors come from cloudflared / tailscale / tailscaled / ngrok child processes
   and from filesystem I/O under the operator's home directory, so the body
   disclosed host layout, binary install paths, the OS account name and — for
   Tailscale — live tskey-* credentials. Hard Rule cheaper-inference#12 forbids this.

   sanitizeErrorMessage() does not close it: it only rewrites tokens that look
   like an absolute path ending in a source extension (SOURCE_EXT in
   open-sse/utils/error.ts), so .json state paths, extension-less binary paths
   and tskey-* keys survive it verbatim. Add
   src/lib/api/publicSafeTunnelError.ts, which returns a route-owned literal
   plus a coarse machine-readable `reason` and logs the real error server-side.

   Reachability is not uniform: /api/tunnels/ngrok, /api/tunnels/tailscale and
   /api/tunnels/tailscale/check are not in LOCAL_ONLY_API_PREFIXES, and
   GET /api/tunnels/cloudflared is exempted via LOCAL_ONLY_API_GET_EXEMPTIONS
   (cheaper-inference#11531), so those bodies can reach a non-loopback caller. The rest are
   loopback-gated and are covered for one consistent contract.

2. validateBody() returns { success, error } and has no `response` field —
   validatedJsonBody() is the helper that has one. Three call sites did
   `return validation.response`, so a failed body validation returned undefined
   and Next answered with a framework 500 instead of a 400. They now build the
   400 explicitly, keeping the { error: string } shape the dashboard reads.

The response shape stays backward compatible: `error` is still a string, with
`reason` added alongside it.
@diegosouzapw
diegosouzapw merged commit dff3f8b into cheaper-inference:release/v3.8.51 Aug 28, 2026
7 of 16 checks passed
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…heaper-inference#11872)

Closes a real Hard Rule cheaper-inference#12 violation — 14 catch blocks across the tunnel/MITM routes echoed a raw err.message, which for Tailscale could leak a live tskey-* credential and always disclosed host layout / OS account name. Routes all 14 sites through a new toPublicSafeTunnelError() classifier, verified by a dedicated regression suite (14/14 passing) asserting no route echoes a raw error.message. Thanks for the security fix!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants