fix: address panel tunnel review followups

Author: skulidropek

packages/api/src/services/panel-cloudflare-tunnel-core.ts

@@ -1,7 +1,3 @@
-import { existsSync, readFileSync } from "node:fs"
-
-import { parseLinuxDefaultGatewayIp } from "./project-port-proxy-core.js"
-
 export type PanelTunnelTarget =
   | {
     readonly ok: true
@@ -145,19 +141,3 @@ export const resolvePanelTunnelTargetUrl = (
     targetUrl: target.toString()
   }
 }
-
-const readDefaultGatewayIp = (): string | null => {
-  try {
-    return parseLinuxDefaultGatewayIp(readFileSync("/proc/net/route", "utf8"))
-  } catch {
-    return null
-  }
-}
-
-export const defaultPanelTunnelLocalhostHost = (): string => {
-  const configured = process.env["DOCKER_GIT_PANEL_TUNNEL_LOCALHOST_HOST"]?.trim()
-  if (configured !== undefined && configured.length > 0) {
-    return configured
-  }
-  return existsSync("/.dockerenv") ? readDefaultGatewayIp() ?? "172.17.0.1" : "127.0.0.1"
-}

packages/api/src/services/panel-cloudflare-tunnel.ts

@@ -1,5 +1,5 @@
 import { spawn, type ChildProcess } from "node:child_process"
-import { mkdirSync, rmSync } from "node:fs"
+import { existsSync, mkdirSync, readFileSync, rmSync } from "node:fs"
 import { join } from "node:path"
 import { randomUUID } from "node:crypto"
 
@@ -8,17 +8,18 @@ import { Duration, Effect } from "effect"
 import type { PanelCloudflareTunnelSession, StartPanelCloudflareTunnelRequest } from "../api/contracts.js"
 import { ApiBadRequestError, ApiInternalError } from "../api/errors.js"
 import {
-  defaultPanelTunnelLocalhostHost,
   parseTryCloudflareUrl,
   resolvePanelTunnelTargetUrl
 } from "./panel-cloudflare-tunnel-core.js"
+import { parseLinuxDefaultGatewayIp } from "./project-port-proxy-core.js"
 
 type PanelCloudflareTunnelRecord = {
   readonly homeDir: string
   logLines: ReadonlyArray<string>
   process: ChildProcess | null
   session: PanelCloudflareTunnelSession
   stderrRemainder: string
+  stopping: boolean
   stdoutRemainder: string
 }
 
@@ -112,11 +113,29 @@ const consumeChunk = (
 const processEnv = (
   homeDir: string
 ): Readonly<Record<string, string | undefined>> => ({
-  ...process.env,
+  PATH: process.env["PATH"],
+  SSL_CERT_DIR: process.env["SSL_CERT_DIR"],
+  SSL_CERT_FILE: process.env["SSL_CERT_FILE"],
   HOME: homeDir,
   NO_COLOR: "1"
 })
 
+const readDefaultGatewayIp = (): string | null => {
+  try {
+    return parseLinuxDefaultGatewayIp(readFileSync("/proc/net/route", "utf8"))
+  } catch {
+    return null
+  }
+}
+
+const defaultPanelTunnelLocalhostHost = (): string => {
+  const configured = process.env["DOCKER_GIT_PANEL_TUNNEL_LOCALHOST_HOST"]?.trim()
+  if (configured !== undefined && configured.length > 0) {
+    return configured
+  }
+  return existsSync("/.dockerenv") ? readDefaultGatewayIp() ?? "172.17.0.1" : "127.0.0.1"
+}
+
 const createStartingRecord = (
   panelUrl: string
 ): PanelCloudflareTunnelRecord => {
@@ -138,6 +157,7 @@ const createStartingRecord = (
       stoppedAt: null
     },
     stderrRemainder: "",
+    stopping: false,
     stdoutRemainder: ""
   }
 }
@@ -150,36 +170,87 @@ const removeTunnelHomeBestEffort = (record: PanelCloudflareTunnelRecord): void =
   }
 }
 
-const stopRecord = (
+const finishStoppedRecord = (
   record: PanelCloudflareTunnelRecord,
   error: string | null = null
 ): PanelCloudflareTunnelSession => {
-  const process = record.process
   record.process = null
+  record.stopping = false
   const session = updateRecord(record, {
     error,
     status: error === null ? "stopped" : "failed",
     stoppedAt: nowIso()
   })
-  if (process !== null && !process.killed) {
-    try {
-      process.kill("SIGTERM")
-    } catch {
-      // process already exited
+  removeTunnelHomeBestEffort(record)
+  return session
+}
+
+const waitForChildClose = (child: ChildProcess): Effect.Effect<void> => {
+  if (child.exitCode !== null || child.signalCode !== null) {
+    return Effect.void
+  }
+
+  return Effect.async((resume) => {
+    let completed = false
+    let killTimer: ReturnType<typeof setTimeout> | null = null
+    const complete = (): void => {
+      if (completed) {
+        return
+      }
+      completed = true
+      child.off("close", complete)
+      child.off("error", complete)
+      child.off("exit", complete)
+      if (killTimer !== null) {
+        clearTimeout(killTimer)
+      }
+      resume(Effect.void)
     }
-    const killTimer = setTimeout(() => {
+
+    child.once("close", complete)
+    child.once("error", complete)
+    child.once("exit", complete)
+    if (!child.killed) {
       try {
-        if (process.exitCode === null && process.signalCode === null) {
-          process.kill("SIGKILL")
+        child.kill("SIGTERM")
+      } catch {
+        complete()
+        return
+      }
+    }
+    killTimer = setTimeout(() => {
+      try {
+        if (child.exitCode === null && child.signalCode === null) {
+          child.kill("SIGKILL")
         }
       } catch {
-        // process already exited
+        complete()
       }
     }, 2_000)
     killTimer.unref()
+  })
+}
+
+const stopRecord = (
+  record: PanelCloudflareTunnelRecord,
+  error: string | null = null
+): Effect.Effect<PanelCloudflareTunnelSession> => {
+  const child = record.process
+  record.process = null
+  record.stopping = true
+  return (child === null ? Effect.void : waitForChildClose(child)).pipe(
+    Effect.map(() => finishStoppedRecord(record, error))
+  )
+}
+
+const runStopRecord = (
+  record: PanelCloudflareTunnelRecord,
+  error: string
+): void => {
+  if (record.stopping || record.session.status === "stopped" || record.session.status === "failed") {
+    return
   }
-  removeTunnelHomeBestEffort(record)
-  return session
+  Effect.runFork(stopRecord(record, error))
 }
 
 const attachProcessHandlers = (
@@ -194,16 +265,16 @@ const attachProcessHandlers = (
     consumeChunk(record, "stderr", chunk)
   })
   child.on("error", (error) => {
-    stopRecord(record, `cloudflared failed to start: ${error.message}`)
+    runStopRecord(record, `cloudflared failed to start: ${error.message}`)
   })
   child.on("close", (exitCode, signal) => {
     flushRemainder(record, "stdout")
     flushRemainder(record, "stderr")
-    if (record.session.status === "stopped" || record.session.status === "failed") {
+    if (record.stopping || record.session.status === "stopped" || record.session.status === "failed") {
       return
     }
     const details = exitCode === null ? `signal ${signal ?? "unknown"}` : `exit code ${exitCode}`
-    stopRecord(record, `cloudflared exited before the tunnel was stopped (${details}).`)
+    runStopRecord(record, `cloudflared exited before the tunnel was stopped (${details}).`)
   })
 }
 
@@ -303,7 +374,7 @@ const waitForRecordId = (
 
     yield* _(preflightPanelTarget(resolved.targetUrl))
     if (currentRecord !== null) {
-      stopRecord(currentRecord)
+      yield* _(stopRecord(currentRecord))
     }
 
     const record = createStartingRecord(resolved.panelUrl)
@@ -324,4 +395,6 @@ export const startPanelCloudflareTunnel = (
   })
 
 export const stopPanelCloudflareTunnel = (): Effect.Effect<PanelCloudflareTunnelSession | null> =>
-  Effect.sync(() => currentRecord === null ? null : stopRecord(currentRecord)).pipe(tunnelRecordLock.withPermits(1))
+  Effect.gen(function*(_) {
+    return currentRecord === null ? null : yield* _(stopRecord(currentRecord))
+  }).pipe(tunnelRecordLock.withPermits(1))

packages/api/tests/panel-cloudflare-tunnel-core.test.ts

@@ -1,4 +1,5 @@
 import { describe, expect, it } from "vitest"
+import * as fc from "fast-check"
 
 import {
   isLocalPanelHostname,
@@ -7,6 +8,58 @@ import {
   resolvePanelTunnelTargetUrl
 } from "../src/services/panel-cloudflare-tunnel-core.js"
 
+const localhostHost = "172.17.0.1"
+const labelCharacters = "abcdefghijklmnopqrstuvwxyz0123456789"
+
+const labelCharacter = (value: number): string =>
+  labelCharacters[value] ?? "a"
+
+const hostnameLabelArbitrary = fc.array(fc.integer({ min: 0, max: 35 }), {
+  maxLength: 24,
+  minLength: 1
+}).map((values) => values.map((value) => labelCharacter(value)).join(""))
+
+const urlSuffixArbitrary = fc.array(fc.integer({ min: 0, max: 35 }), {
+  maxLength: 16,
+  minLength: 0
+}).map((values) => values.length === 0 ? "" : `/share/${values.map((value) => labelCharacter(value)).join("")}?tab=share#logs`)
+
+const publicHostnameArbitrary = fc.oneof(
+  fc.constantFrom("example.com", "public.example.net"),
+  hostnameLabelArbitrary.map((label) => `${label}.example.com`)
+)
+
+const privateIpv4HostnameArbitrary = fc.constantFrom(
+  "10.0.0.5",
+  "100.64.0.1",
+  "169.254.1.1",
+  "172.16.0.1",
+  "172.31.255.254",
+  "192.168.1.2"
+)
+
+const loopbackHostnameArbitrary = fc.constantFrom("0.0.0.0", "127.0.0.1", "localhost")
+
+const tryCloudflareHostnameArbitrary = hostnameLabelArbitrary.map((label) => `${label}.trycloudflare.com`)
+
+const httpUrl = (
+  protocol: "http" | "https",
+  hostname: string,
+  port: number,
+  suffix: string
+): string => `${protocol}://${hostname}:${port}${suffix}`
+
+const originUrl = (value: string): string => {
+  const url = new URL(value)
+  url.pathname = "/"
+  url.search = ""
+  url.hash = ""
+  return url.toString()
+}
+
+const withoutTryCloudflareUrl = (value: string): boolean =>
+  !value.toLowerCase().includes(".trycloudflare.com")
+
 describe("panel Cloudflare tunnel core", () => {
   it("normalizes panel URLs to their origin", () => {
     expect(resolvePanelTunnelTargetUrl("http://192.168.0.206:4174/ports/project", "172.17.0.1")).toEqual({
@@ -59,4 +112,98 @@ describe("panel Cloudflare tunnel core", () => {
       parseTryCloudflareUrl("INF |  https://yellow-field-123.trycloudflare.com                                      |")
     ).toBe("https://yellow-field-123.trycloudflare.com")
   })
+
+  it("normalizes generated private panel URLs to origins", () => {
+    fc.assert(
+      fc.property(
+        fc.constantFrom("http", "https"),
+        privateIpv4HostnameArbitrary,
+        fc.integer({ max: 65_535, min: 1 }),
+        urlSuffixArbitrary,
+        (protocol, hostname, port, suffix) => {
+          const panelUrl = httpUrl(protocol, hostname, port, suffix)
+          const expectedUrl = originUrl(panelUrl)
+
+          expect(resolvePanelTunnelTargetUrl(panelUrl, localhostHost)).toEqual({
+            ok: true,
+            panelUrl: expectedUrl,
+            targetUrl: expectedUrl
+          })
+        }
+      )
+    )
+  })
+
+  it("maps generated loopback panel URLs to the provided localhost host", () => {
+    fc.assert(
+      fc.property(
+        fc.constantFrom("http", "https"),
+        loopbackHostnameArbitrary,
+        fc.integer({ max: 65_535, min: 1 }),
+        urlSuffixArbitrary,
+        (protocol, hostname, port, suffix) => {
+          const panelUrl = httpUrl(protocol, hostname, port, suffix)
+          const expectedTarget = new URL(originUrl(panelUrl))
+          expectedTarget.hostname = localhostHost
+
+          expect(resolvePanelTunnelTargetUrl(panelUrl, localhostHost)).toEqual({
+            ok: true,
+            panelUrl: originUrl(panelUrl),
+            targetUrl: expectedTarget.toString()
+          })
+        }
+      )
+    )
+  })
+
+  it("rejects generated public and trycloudflare hosts", () => {
+    fc.assert(
+      fc.property(
+        fc.constantFrom("http", "https"),
+        publicHostnameArbitrary,
+        fc.integer({ max: 65_535, min: 1 }),
+        urlSuffixArbitrary,
+        (protocol, hostname, port, suffix) => {
+          expect(resolvePanelTunnelTargetUrl(httpUrl(protocol, hostname, port, suffix), localhostHost)).toEqual({
+            ok: false,
+            message: "panelUrl must point to localhost or a private LAN address."
+          })
+        }
+      )
+    )
+
+    fc.assert(
+      fc.property(
+        tryCloudflareHostnameArbitrary,
+        fc.integer({ max: 65_535, min: 1 }),
+        urlSuffixArbitrary,
+        (hostname, port, suffix) => {
+          expect(isTryCloudflareHostname(hostname)).toBe(true)
+          expect(resolvePanelTunnelTargetUrl(httpUrl("https", hostname, port, suffix), localhostHost)).toEqual({
+            ok: false,
+            message: "Open docker-git locally before starting a new Cloudflare tunnel."
+          })
+        }
+      )
+    )
+  })
+
+  it("extracts only generated quick tunnel URLs from noisy cloudflared output", () => {
+    fc.assert(
+      fc.property(
+        fc.string().filter(withoutTryCloudflareUrl),
+        tryCloudflareHostnameArbitrary,
+        fc.string().filter(withoutTryCloudflareUrl),
+        (prefix, hostname, suffix) => {
+          expect(parseTryCloudflareUrl(`${prefix} https://${hostname} ${suffix}`)).toBe(`https://${hostname}`)
+        }
+      )
+    )
+
+    fc.assert(
+      fc.property(fc.string().filter(withoutTryCloudflareUrl), (output) => {
+        expect(parseTryCloudflareUrl(output)).toBeNull()
+      })
+    )
+  })
 })