Share the bounded read, the symlink walk and the file name rule in the host

src/ipc/handlers/loginFailureReason.ts

@@ -19,11 +19,10 @@
  * into it. `code` and `name` are both public and writable, so a value that
  * merely looks like a Node enum member proves nothing about where it came
  * from: they are read as lookup keys into the tables below and never copied
- * into the answer. An HTTP status is parsed out of the message as a number
- * and then used the same way, because the message is written from the
- * response and a status the caller can influence is no safer than a field the
- * thrower can set. A key that is not in a table maps to that table's
- * catch-all token.
+ * into the answer. The HTTP status a refused response carries is used the same
+ * way, because it comes from the response and a status the caller can
+ * influence is no safer than a field the thrower can set. A key that is not in
+ * a table maps to that table's catch-all token.
  *
  * The other half of the answer is where the error came from. The handler's
  * catch wraps two very different things: the network round trip and the
@@ -82,23 +81,18 @@ export const STORAGE_MESSAGES = new Map<string, string>([
   ["A system password store is required for account storage", "no-system-password-store"]
 ])
 
-/** `Network request failed with status 503`. The digits are read as a number, never carried over as text. */
-const STATUS_MESSAGE = /^Network request failed with status (\d{3}|unknown)$/
-
 /**
  * The HTTP statuses the auth service actually answers with, each mapped to
  * the token that names it.
  *
- * Not `http-status-${status}`: `network.ts` writes that message from the
- * response's own status line, and `assertString` accepts `503` as a password,
- * so a login with that password and an outage on the other end used to put
- * the password in the log. The status is parsed into a number here and then
- * treated exactly like `code`, as a lookup key whose value never reaches the
- * answer. A status outside the table degrades to its range, which still
- * separates "the service rejected us" from "the service is broken", and
- * anything that is not a 4xx or 5xx (a redirect this transport does not
- * follow, or the literal `unknown` when the response had no status line at
- * all) is `http-other`.
+ * Not `http-status-${status}`: the status comes from the response's own status
+ * line, and `assertString` accepts `503` as a password, so a login with that
+ * password and an outage on the other end used to put the password in the log.
+ * The status is treated exactly like `code`, as a lookup key whose value never
+ * reaches the answer. A status outside the table degrades to its range, which
+ * still separates "the service rejected us" from "the service is broken", and
+ * anything that is not a 4xx or 5xx (a redirect this transport does not follow,
+ * or no status line at all) is `http-other`.
  */
 export const HTTP_STATUSES = new Map<number, string>([
   [400, "http-bad-request"],
@@ -193,17 +187,21 @@ export const ERROR_NAMES = new Map<string, string>([
   ["AbortError", "unclassified-AbortError"]
 ])
 
-/** Names an HTTP failure without ever formatting the status back into a string. */
-function httpReason(status: string | undefined): string {
-  const code = Number(status) // `unknown` and a missing group are both NaN, and every comparison below is false for NaN.
-  const named = HTTP_STATUSES.get(code)
+/** Names an HTTP failure without ever formatting the status back into a string. Every comparison is false for NaN, so a response with no status line lands on `http-other`. */
+function httpReason(status: number): string {
+  const named = HTTP_STATUSES.get(status)
   if (named) return named
-  if (code >= 400 && code < 500) return "http-4xx"
-  if (code >= 500 && code < 600) return "http-5xx"
+  if (status >= 400 && status < 500) return "http-4xx"
+  if (status >= 500 && status < 600) return "http-5xx"
 
   return "http-other"
 }
 
+/** The status a refusal carries, as a lookup key only: `network.ts` sets `statusCode` on every non-2xx it throws (`BoundedResponseError`), including when the response had no status line and `Number` reads it as NaN. Structural rather than `instanceof`, so this module stays Electron-free. */
+function statusOf(error: Error): number | undefined {
+  return "statusCode" in error ? Number((error as { statusCode?: unknown }).statusCode) : undefined
+}
+
 /** The `code` an error carries, as a lookup key only: a non-string is no key at all. */
 function codeOf(error: Error): string {
   const code: unknown = (error as { code?: unknown }).code
@@ -228,8 +226,8 @@ export function loginFailureReason(error: unknown): string {
   const known = NETWORK_MESSAGES.get(error.message)
   if (known) return known
 
-  const status = STATUS_MESSAGE.exec(error.message)
-  if (status) return httpReason(status[1])
+  const status = statusOf(error)
+  if (status !== undefined) return httpReason(status)
 
   const code = codeOf(error)
   if (code) return NETWORK_CODES.get(code) ?? "network-other"

src/ipc/network.ts

@@ -58,6 +58,64 @@ export class BoundedResponseError extends Error {
   }
 }
 
+/** What {@link collectBounded} reads off a response. Electron's `net` and Node's `http(s)` both answer this shape. */
+type BoundedResponse = NodeJS.EventEmitter & {
+  statusCode?: number
+  headers: Record<string, string | string[] | undefined>
+}
+
+/**
+ * Reads one response into `chunks` under `maxBytes`, then settles `finish`.
+ *
+ * The ceiling is checked twice: against the declared `Content-Length` before a
+ * byte is read, and against the running total as the chunks arrive, so a lying
+ * length costs the cap and not the heap. A non-2xx is a refusal rather than a
+ * body, carrying its status and headers for the callers that classify one (see
+ * {@link BoundedResponseError}); every other caller keeps matching on the
+ * message. Both transports below read through this, so no cap, status rule or
+ * refusal text can be tightened on one side only.
+ *
+ * @param cancel How this transport drops the exchange: `request.abort` for
+ * Electron's `net`, `request.destroy` for Node's `http(s)`.
+ * @param finish The caller's settle-once function, given the error on a refusal.
+ */
+function collectBounded(response: BoundedResponse, maxBytes: number, chunks: Buffer[], cancel: () => void, finish: (error?: Error) => void): void {
+  const contentLengthHeader = response.headers["content-length"]
+  const contentLength = Number(Array.isArray(contentLengthHeader) ? contentLengthHeader[0] : contentLengthHeader)
+
+  const refuse = (error: Error): void => {
+    cancel()
+    finish(error)
+  }
+
+  if (Number.isFinite(contentLength) && contentLength > maxBytes) {
+    refuse(new Error("Network response is too large"))
+    return
+  }
+
+  if (response.statusCode === undefined || response.statusCode < 200 || response.statusCode >= 300) {
+    refuse(new BoundedResponseError(`Network request failed with status ${response.statusCode ?? "unknown"}`, response.statusCode, response.headers))
+    return
+  }
+
+  let responseBytes = 0
+
+  response.on("data", (chunk: Buffer | string) => {
+    const chunkBuffer = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk)
+    responseBytes += chunkBuffer.length
+
+    if (responseBytes > maxBytes) {
+      refuse(new Error("Network response is too large"))
+      return
+    }
+
+    chunks.push(chunkBuffer)
+  })
+  response.on("end", () => finish())
+  response.on("aborted", () => finish(new Error("Network response was aborted")))
+  response.on("error", (error: Error) => finish(error))
+}
+
 export function requestBoundedText(url: URL, options: BoundedRequestOptions = {}): Promise<string> {
   return requestBoundedBuffer(url, options).then((bytes) => bytes.toString("utf8"))
 }
@@ -74,7 +132,6 @@ export function requestBoundedBuffer(url: URL, options: BoundedRequestOptions =
 
   return new Promise((resolve, reject) => {
     let settled = false
-    let responseBytes = 0
     const chunks: Buffer[] = []
     const request = net.request({
       url: url.toString(),
@@ -100,39 +157,7 @@ export function requestBoundedBuffer(url: URL, options: BoundedRequestOptions =
       }
     }
 
-    request.on("response", (response) => {
-      const contentLengthHeader = response.headers["content-length"]
-      const contentLengthValue = Array.isArray(contentLengthHeader) ? contentLengthHeader[0] : contentLengthHeader
-      const contentLength = Number(contentLengthValue)
-
-      if (Number.isFinite(contentLength) && contentLength > maxBytes) {
-        request.abort()
-        finish(new Error("Network response is too large"))
-        return
-      }
-
-      if (response.statusCode === undefined || response.statusCode < 200 || response.statusCode >= 300) {
-        request.abort()
-        finish(new BoundedResponseError(`Network request failed with status ${response.statusCode ?? "unknown"}`, response.statusCode, response.headers))
-        return
-      }
-
-      response.on("data", (chunk: Buffer | string) => {
-        const chunkBuffer = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk)
-        responseBytes += chunkBuffer.length
-
-        if (responseBytes > maxBytes) {
-          request.abort()
-          finish(new Error("Network response is too large"))
-          return
-        }
-
-        chunks.push(chunkBuffer)
-      })
-      response.on("end", () => finish())
-      response.on("aborted", () => finish(new Error("Network response was aborted")))
-      response.on("error", (error) => finish(error))
-    })
+    request.on("response", (response) => collectBounded(response, maxBytes, chunks, () => request.abort(), finish))
 
     request.on("error", (error) => finish(error))
     request.on("login", (_authInfo, callback) => callback())
@@ -159,11 +184,11 @@ export function requestBoundedBuffer(url: URL, options: BoundedRequestOptions =
  * multi-gigabyte file download needs a streamed-to-disk write and a
  * socket-inactivity timeout, not the wall-clock timeout this function keeps.
  *
- * Every guarantee `requestBoundedText` carries is preserved here and no
- * caller of `requestBoundedText` is affected, since that function is
- * untouched:
- *  - the response is capped at `maxBytes`, checked against `Content-Length`
- *    up front and against the running streamed total as chunks arrive;
+ * Every guarantee `requestBoundedText` carries is preserved here, because the
+ * reading itself is the same code: both transports hand their response to
+ * {@link collectBounded}, so the cap, the status rule and the refusal text
+ * cannot be tightened on one side only. What stays split is what genuinely
+ * differs, the transport and how it is cancelled. On top of that:
  *  - the whole exchange is bounded by `REQUEST_TIMEOUT_MS`, the same
  *    wall-clock timeout `requestBoundedText` uses (not `request.setTimeout`,
  *    which only measures socket inactivity and would let a slow-trickling
@@ -208,7 +233,6 @@ export function requestBoundedTextViaNode(url: URL, options: BoundedRequestOptio
 
   return new Promise((resolve, reject) => {
     let settled = false
-    let responseBytes = 0
     const chunks: Buffer[] = []
     // Whichever request is in flight right now (the CONNECT, or the real one), so the
     // one timeout below can abort it without knowing which phase it landed in.
@@ -231,38 +255,7 @@ export function requestBoundedTextViaNode(url: URL, options: BoundedRequestOptio
       finish(new Error("Network request timed out"))
     }, timeoutMs)
 
-    function onResponse(response: IncomingMessage): void {
-      const contentLengthHeader = response.headers["content-length"]
-      const contentLength = Number(contentLengthHeader)
-
-      if (Number.isFinite(contentLength) && contentLength > maxBytes) {
-        abortInFlight()
-        finish(new Error("Network response is too large"))
-        return
-      }
-
-      if (response.statusCode === undefined || response.statusCode < 200 || response.statusCode >= 300) {
-        abortInFlight()
-        finish(new Error(`Network request failed with status ${response.statusCode ?? "unknown"}`))
-        return
-      }
-
-      response.on("data", (chunk: Buffer | string) => {
-        const chunkBuffer = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk)
-        responseBytes += chunkBuffer.length
-
-        if (responseBytes > maxBytes) {
-          abortInFlight()
-          finish(new Error("Network response is too large"))
-          return
-        }
-
-        chunks.push(chunkBuffer)
-      })
-      response.on("end", () => finish())
-      response.on("aborted", () => finish(new Error("Network response was aborted")))
-      response.on("error", (error) => finish(error))
-    }
+    const onResponse = (response: IncomingMessage): void => collectBounded(response, maxBytes, chunks, () => abortInFlight(), finish)
 
     // A tunneled request is always sent with node:http's own `request`, `agent` and all,
     // never `https.request`: the agent already hands back a socket doing TLS on its own

src/ipc/pathPolicy.ts

@@ -5,7 +5,7 @@ import { basename, dirname, join, resolve } from "node:path"
 import { getConfig } from "@src/config/configManager"
 import { MODS_FOLDER_NAME } from "@domain/mods/folder"
 import type { PathGrant } from "@src/ipc/validation"
-import { assertNonRootPath, comparablePath, isPathGranted, isRestoreWorkspaceName } from "@src/ipc/validation"
+import { assertNonRootPath, assertNoSymlinkComponents, comparablePath, isPathGranted, isRestoreWorkspaceName } from "@src/ipc/validation"
 
 type ApprovedPath = PathGrant & {
   expiresAt: number
@@ -153,27 +153,6 @@ function getProtectedPaths(config: ConfigType): string[] {
   return [app.getPath("userData"), app.getPath("appData"), app.getPath("home"), app.getAppPath(), ...getConfiguredFolders(config), ...getLauncherFolders()]
 }
 
-function assertNoSymlinkComponents(pathValue: string): void {
-  let current = resolve(pathValue)
-  let parent = dirname(current)
-
-  while (!fse.existsSync(current)) {
-    if (parent === current) break
-    current = parent
-    parent = dirname(current)
-  }
-
-  while (current !== parent) {
-    const stats = fse.lstatSync(current)
-    if (stats.isSymbolicLink()) throw new TypeError("Symbolic links are not allowed for managed paths")
-    current = parent
-    parent = dirname(current)
-  }
-
-  const rootStats = fse.lstatSync(current)
-  if (rootStats.isSymbolicLink()) throw new TypeError("Symbolic links are not allowed for managed paths")
-}
-
 export async function assertManagedPath(value: unknown, name = "path", options: PathPolicyOptions = {}): Promise<string> {
   const pathValue = resolve(assertNonRootPath(value, name))
   const config = await getConfig()
@@ -183,7 +162,7 @@ export async function assertManagedPath(value: unknown, name = "path", options:
   if (!isConfiguredPath && !isApprovedPath && !isRestoreWorkspace) throw new TypeError(`Unmanaged ${name}`)
 
   if (!options.allowMissing && !fse.existsSync(pathValue)) throw new TypeError(`Missing ${name}`)
-  if (!options.allowSymlinks) assertNoSymlinkComponents(pathValue)
+  if (!options.allowSymlinks) assertNoSymlinkComponents(pathValue, "Symbolic links are not allowed for managed paths")
   return pathValue
 }
 

src/ipc/validation.ts

@@ -1,4 +1,5 @@
-import { isAbsolute, relative, resolve, sep } from "node:path"
+import { existsSync, lstatSync } from "node:fs"
+import { dirname, isAbsolute, relative, resolve, sep } from "node:path"
 import { fileURLToPath } from "node:url"
 
 import semver from "semver"
@@ -171,6 +172,40 @@ export function isPathWithin(root: string, candidate: string, allowRoot = true):
   return (allowRoot && relativePath === "") || (relativePath !== "" && relativePath !== ".." && !relativePath.startsWith(`..${sep}`) && !isAbsolute(relativePath))
 }
 
+/**
+ * Refuses a path any of whose existing ancestors is a symbolic link.
+ *
+ * The walk starts at the deepest component that exists, because a caller may
+ * name a file it is about to create, and climbs to the filesystem root, which
+ * is checked too. A link anywhere on that chain means the path the caller
+ * vetted and the path the filesystem opens are not the same path.
+ *
+ * Both sides of the host call this one: the path policy before it hands a
+ * managed path to a handler, and the extraction workers around every write
+ * they make. Two copies is how one side gets tightened and the other left
+ * stale, the same reason the archive size ceilings moved here in #362.
+ *
+ * @param message Wording for the refusal, so the path policy keeps its own.
+ */
+export function assertNoSymlinkComponents(pathValue: string, message = "Symbolic links are not allowed"): void {
+  let current = resolve(pathValue)
+  let parent = dirname(current)
+
+  while (!existsSync(current)) {
+    if (parent === current) return
+    current = parent
+    parent = dirname(current)
+  }
+
+  while (current !== parent) {
+    if (lstatSync(current).isSymbolicLink()) throw new TypeError(message)
+    current = parent
+    parent = dirname(current)
+  }
+
+  if (lstatSync(current).isSymbolicLink()) throw new TypeError(message)
+}
+
 /**
  * One entry of a path allow list.
  *