refactor(http): separate retry and redirect phases

Author: jdalton

src/http-request/download.mts

@@ -105,23 +105,29 @@ export async function httpDownload(
   // passed) and the inner total===0 + interval-throttle branches fire
   // only on real network downloads, not the unit test mocks.
   /* c8 ignore start */
-  let progressCallback:
+  function createProgressCallback():
     | ((downloaded: number, total: number) => void)
-    | undefined
-  if (onProgress) {
-    progressCallback = onProgress
-  } else if (logger) {
-    let lastPercent = 0
-    progressCallback = (downloaded: number, total: number) => {
-      const percent = total === 0 ? 0 : MathFloor((downloaded / total) * 100)
-      if (percent >= lastPercent + progressInterval) {
-        logger.log(
-          `  Progress: ${percent}% (${(downloaded / 1024 / 1024).toFixed(1)} MB / ${(total / 1024 / 1024).toFixed(1)} MB)`,
-        )
-        lastPercent = percent
+    | undefined {
+    let progressCallback:
+      | ((downloaded: number, total: number) => void)
+      | undefined
+    if (onProgress) {
+      progressCallback = onProgress
+    } else if (logger) {
+      let lastPercent = 0
+      progressCallback = (downloaded: number, total: number) => {
+        const percent = total === 0 ? 0 : MathFloor((downloaded / total) * 100)
+        if (percent >= lastPercent + progressInterval) {
+          logger.log(
+            `  Progress: ${percent}% (${(downloaded / 1024 / 1024).toFixed(1)} MB / ${(total / 1024 / 1024).toFixed(1)} MB)`,
+          )
+          lastPercent = percent
+        }
       }
     }
+    return progressCallback
   }
+  const progressCallback = createProgressCallback()
   /* c8 ignore stop */
 
   // Download to a temp file first, then atomically rename to destination.
@@ -137,6 +143,47 @@ export async function httpDownload(
     await safeDelete(tempPath)
   }
 
+  async function verifyDownloadedFile(
+    result: Awaited<ReturnType<typeof httpDownloadAttempt>>,
+  ): Promise<void> {
+    // Both digests were computed over the response chunks before they reached
+    // the destination stream, so verification does not reread the temp file.
+    if (sha256) {
+      const expectedHash = sha256.toLowerCase()
+
+      // Use constant-time comparison to prevent timing attacks.
+      if (
+        result.sha256.length !== expectedHash.length ||
+        !crypto.timingSafeEqual(
+          BufferFrom!(result.sha256),
+          Buffer.from(expectedHash),
+        )
+      ) {
+        await safeDelete(tempPath)
+        throw new ErrorCtor(
+          `Checksum verification failed for ${url}\n` +
+            `Expected: ${expectedHash}\n` +
+            `Computed: ${result.sha256}`,
+        )
+      }
+    }
+    if (
+      integrity &&
+      (result.integrity.length !== integrity.length ||
+        !crypto.timingSafeEqual(
+          BufferFrom!(result.integrity),
+          Buffer.from(integrity),
+        ))
+    ) {
+      await safeDelete(tempPath)
+      throw new ErrorCtor(
+        `Integrity verification failed for ${url}\n` +
+          `Expected: ${integrity}\n` +
+          `Computed: ${result.integrity}`,
+      )
+    }
+  }
+
   // Retry logic with exponential backoff
   let lastError: Error | undefined
   for (let attempt = 0; attempt <= retries; attempt++) {
@@ -151,42 +198,7 @@ export async function httpDownload(
         timeout,
       })
 
-      // Both digests were computed over the response chunks before they reached
-      // the destination stream, so verification does not reread the temp file.
-      if (sha256) {
-        const expectedHash = sha256.toLowerCase()
-
-        // Use constant-time comparison to prevent timing attacks.
-        if (
-          result.sha256.length !== expectedHash.length ||
-          !crypto.timingSafeEqual(
-            BufferFrom!(result.sha256),
-            Buffer.from(expectedHash),
-          )
-        ) {
-          await safeDelete(tempPath)
-          throw new ErrorCtor(
-            `Checksum verification failed for ${url}\n` +
-              `Expected: ${expectedHash}\n` +
-              `Computed: ${result.sha256}`,
-          )
-        }
-      }
-      if (
-        integrity &&
-        (result.integrity.length !== integrity.length ||
-          !crypto.timingSafeEqual(
-            BufferFrom!(result.integrity),
-            Buffer.from(integrity),
-          ))
-      ) {
-        await safeDelete(tempPath)
-        throw new ErrorCtor(
-          `Integrity verification failed for ${url}\n` +
-            `Expected: ${integrity}\n` +
-            `Computed: ${result.integrity}`,
-        )
-      }
+      await verifyDownloadedFile(result)
 
       // Download succeeded - atomically rename temp file to destination.
       // This overwrites any existing file at destPath.

src/http-request/request-attempt.mts

@@ -159,6 +159,100 @@ export async function httpRequestAttempt(
       }
     }
 
+    function handleRedirect(res: IncomingResponse, location: string): void {
+      // Drain the redirect response body to free the socket.
+      res.resume()
+
+      emitResponse({
+        headers: res.headers,
+        status: res.statusCode,
+        statusText: res.statusMessage,
+      })
+
+      if (maxRedirects <= 0) {
+        // Hook already emitted above — reject directly to avoid double-fire.
+        settled = true
+        reject(
+          new ErrorCtor(
+            `Too many redirects (exceeded maximum: ${maxRedirects})`,
+          ),
+        )
+        return
+      }
+
+      // Resolve the Location header against the current url whether it
+      // is absolute or relative — the URL constructor ignores the base
+      // when the first argument already parses as an absolute URL. A
+      // scheme check on `.protocol` (not `startsWith('http')`, which
+      // also accepts `httpfoo:`) validates the result.
+      const redirectParsed = new URLCtor(location, url)
+      if (
+        redirectParsed.protocol !== 'http:' &&
+        redirectParsed.protocol !== 'https:'
+      ) {
+        // Hook already emitted above — reject directly to avoid double-fire.
+        settled = true
+        reject(
+          new ErrorCtor(
+            `Redirect Location has an unsupported scheme: ${location}`,
+          ),
+        )
+        return
+      }
+      const redirectUrl = redirectParsed.toString()
+
+      if (isHttps && redirectParsed.protocol !== 'https:') {
+        // Hook already emitted above — reject directly to avoid double-fire.
+        settled = true
+        reject(
+          new ErrorCtor(
+            `Redirect from HTTPS to HTTP is not allowed: ${redirectUrl}`,
+          ),
+        )
+        return
+      }
+
+      // Strip auth/session headers on cross-origin redirects to prevent
+      // leaking credentials to third-party hosts (e.g., GitHub -> S3).
+      let redirectHeaders = headers
+      if (new URLCtor(url).origin !== redirectParsed.origin) {
+        redirectHeaders = {
+          __proto__: null,
+        } as unknown as typeof headers
+        const stripped = new Set([
+          'authorization',
+          'cookie',
+          'proxy-authenticate',
+          'proxy-authorization',
+        ])
+        for (const key of ObjectKeys(headers)) {
+          if (!stripped.has(key.toLowerCase())) {
+            ;(redirectHeaders as Record<string, unknown>)[key] = (
+              headers as Record<string, unknown>
+            )[key]
+          }
+        }
+      }
+
+      // Redirect chaining — Promise adoption handles the inner result.
+      settled = true
+      resolve(
+        httpRequestAttempt(redirectUrl, {
+          body,
+          ca,
+          followRedirects,
+          headers: redirectHeaders,
+          hooks,
+          maxRedirects: maxRedirects - 1,
+          maxResponseSize,
+          method,
+          stream,
+          timeout,
+        }),
+      )
+      return
+    }
+
     /* c8 ignore start - External HTTP/HTTPS request */
     const request = httpModule.request(
       requestOptions,
@@ -177,96 +271,7 @@ export async function httpRequestAttempt(
             res.statusCode < 400 &&
             res.headers.location
           ) {
-            // Drain the redirect response body to free the socket.
-            res.resume()
-
-            emitResponse({
-              headers: res.headers,
-              status: res.statusCode,
-              statusText: res.statusMessage,
-            })
-
-            if (maxRedirects <= 0) {
-              // Hook already emitted above — reject directly to avoid double-fire.
-              settled = true
-              reject(
-                new ErrorCtor(
-                  `Too many redirects (exceeded maximum: ${maxRedirects})`,
-                ),
-              )
-              return
-            }
-
-            // Resolve the Location header against the current url whether it
-            // is absolute or relative — the URL constructor ignores the base
-            // when the first argument already parses as an absolute URL. A
-            // scheme check on `.protocol` (not `startsWith('http')`, which
-            // also accepts `httpfoo:`) validates the result.
-            const redirectParsed = new URLCtor(res.headers.location, url)
-            if (
-              redirectParsed.protocol !== 'http:' &&
-              redirectParsed.protocol !== 'https:'
-            ) {
-              // Hook already emitted above — reject directly to avoid double-fire.
-              settled = true
-              reject(
-                new ErrorCtor(
-                  `Redirect Location has an unsupported scheme: ${res.headers.location}`,
-                ),
-              )
-              return
-            }
-            const redirectUrl = redirectParsed.toString()
-
-            if (isHttps && redirectParsed.protocol !== 'https:') {
-              // Hook already emitted above — reject directly to avoid double-fire.
-              settled = true
-              reject(
-                new ErrorCtor(
-                  `Redirect from HTTPS to HTTP is not allowed: ${redirectUrl}`,
-                ),
-              )
-              return
-            }
-
-            // Strip auth/session headers on cross-origin redirects to prevent
-            // leaking credentials to third-party hosts (e.g., GitHub -> S3).
-            let redirectHeaders = headers
-            if (new URLCtor(url).origin !== redirectParsed.origin) {
-              redirectHeaders = {
-                __proto__: null,
-              } as unknown as typeof headers
-              const stripped = new Set([
-                'authorization',
-                'cookie',
-                'proxy-authenticate',
-                'proxy-authorization',
-              ])
-              for (const key of ObjectKeys(headers)) {
-                if (!stripped.has(key.toLowerCase())) {
-                  ;(redirectHeaders as Record<string, unknown>)[key] = (
-                    headers as Record<string, unknown>
-                  )[key]
-                }
-              }
-            }
-
-            // Redirect chaining — Promise adoption handles the inner result.
-            settled = true
-            resolve(
-              httpRequestAttempt(redirectUrl, {
-                body,
-                ca,
-                followRedirects,
-                headers: redirectHeaders,
-                hooks,
-                maxRedirects: maxRedirects - 1,
-                maxResponseSize,
-                method,
-                stream,
-                timeout,
-              }),
-            )
+            handleRedirect(res, res.headers.location)
             return
           }