harden failure bundle artifact downloads

Author: merlinsantiago982-cmd

src/lib/bundle.test.ts

@@ -8,7 +8,7 @@
  * the full http+fetch path is wired against MSW).
  */
 
-import { mkdtempSync } from 'node:fs';
+import { existsSync, mkdtempSync, readFileSync } from 'node:fs';
 import { tmpdir } from 'node:os';
 import { join } from 'node:path';
 import { describe, expect, it } from 'vitest';
@@ -19,10 +19,12 @@ import {
   buildMeta,
   pickCodeExtension,
   resolveBundleDir,
+  STREAM_URL_MAX_BYTES,
   STREAM_URL_MAX_RETRIES,
   streamUrlToFile,
   stepFilenamePrefix,
   type AssertContextIntegrityOptions,
+  writeBundle,
 } from './bundle.js';
 import type { CliFailureContext } from '../commands/test.js';
 
@@ -548,6 +550,15 @@ describe('stepFilenamePrefix', () => {
     expect(stepFilenamePrefix(100)).toBe('100');
     expect(stepFilenamePrefix(999)).toBe('999');
   });
+
+  it('rejects non-numeric runtime values before they become path segments', () => {
+    expect(() => stepFilenamePrefix('../escape' as unknown as number)).toThrowError(
+      expect.objectContaining({
+        code: 'VALIDATION_ERROR',
+        details: expect.objectContaining({ field: 'steps[].stepIndex' }),
+      }),
+    );
+  });
 });
 
 describe('buildMeta', () => {
@@ -667,6 +678,49 @@ describe('streamUrlToFile retry', () => {
     expect(calls).toBe(1);
   });
 
+  it('rejects local artifact URLs before fetch is called', async () => {
+    let calls = 0;
+    const fetchImpl = async () => {
+      calls++;
+      return new Response('private', { status: 200 });
+    };
+    await expect(
+      streamUrlToFile(
+        'http://127.0.0.1:8080/private',
+        '/tmp/will-not-be-written',
+        fetchImpl as typeof globalThis.fetch,
+        { sleep: noSleep },
+      ),
+    ).rejects.toMatchObject({
+      code: 'VALIDATION_ERROR',
+      details: expect.objectContaining({ field: 'artifact-url' }),
+    });
+    expect(calls).toBe(0);
+  });
+
+  it('rejects oversized artifacts from Content-Length without retrying', async () => {
+    let calls = 0;
+    const fetchImpl = async () => {
+      calls++;
+      return new Response('too large', {
+        status: 200,
+        headers: { 'content-length': String(STREAM_URL_MAX_BYTES + 1) },
+      });
+    };
+    await expect(
+      streamUrlToFile(
+        'https://example.com/huge',
+        '/tmp/will-not-be-written',
+        fetchImpl as typeof globalThis.fetch,
+        { sleep: noSleep },
+      ),
+    ).rejects.toMatchObject({
+      code: 'PAYLOAD_TOO_LARGE',
+      details: expect.objectContaining({ maxBytes: STREAM_URL_MAX_BYTES }),
+    });
+    expect(calls).toBe(1);
+  });
+
   it('sleeps between retries', async () => {
     const sleepDelays: number[] = [];
     const fetchImpl = async () => {
@@ -689,3 +743,46 @@ describe('streamUrlToFile retry', () => {
     expect(sleepDelays.every(d => d > 0)).toBe(true);
   });
 });
+
+describe('writeBundle artifact filename safety', () => {
+  it('sanitizes evidence kind before using it in a downloaded sidecar filename', async () => {
+    const dir = mkdtempSync(join(tmpdir(), 'bundle-path-test-'));
+    const ctx: CliFailureContext = {
+      ...baseCtx,
+      result: { ...baseCtx.result, videoUrl: null },
+      steps: [
+        {
+          ...baseCtx.steps[2]!,
+          screenshotUrl: null,
+          htmlSnapshotUrl: null,
+        },
+      ],
+      failure: {
+        ...baseCtx.failure,
+        evidence: [
+          {
+            kind: '../../escape' as unknown as 'log',
+            stepIndex: 5,
+            url: 'https://signed.example.com/escape.txt',
+            summary: 'malicious kind',
+          },
+        ],
+      },
+    };
+    const fetchImpl = async () => new Response('sidecar', { status: 200 });
+
+    const result = await writeBundle(ctx, {
+      dir,
+      failedOnly: false,
+      fetchImpl: fetchImpl as typeof globalThis.fetch,
+    });
+
+    expect(result.files).toContain('steps/05-escape-0.txt');
+    expect(existsSync(join(dir, 'steps', '05-escape-0.txt'))).toBe(true);
+    expect(existsSync(join(dir, 'escape-0.txt'))).toBe(false);
+    const evidence = JSON.parse(readFileSync(join(dir, 'steps', '05-evidence.json'), 'utf8')) as [
+      { path: string },
+    ];
+    expect(evidence[0].path).toBe('steps/05-escape-0.txt');
+  });
+});

src/lib/bundle.ts

@@ -34,14 +34,15 @@
  */
 
 import { mkdir, mkdtemp, readdir, rename, rm, stat, unlink, writeFile } from 'node:fs/promises';
-import type { Writable } from 'node:stream';
+import { Readable, Transform, type TransformCallback, type Writable } from 'node:stream';
 import { pipeline } from 'node:stream/promises';
 import type { ReadableStream as NodeReadableStream } from 'node:stream/web';
 import { createWriteStream } from 'node:fs';
 import { dirname, isAbsolute, join, resolve } from 'node:path';
 import type { CliFailureContext, CliTestStep } from '../commands/test.js';
-import { ApiError, TransportError } from './errors.js';
+import { ApiError, localValidationError, TransportError } from './errors.js';
 import type { FetchImpl } from './http.js';
+import { assertNotLocal } from './target-url.js';
 
 /** Schema version stamped into `meta.json`. Bumps with the contract. */
 export const BUNDLE_SCHEMA_VERSION = 'cli-v1' as const;
@@ -50,6 +51,10 @@ export const BUNDLE_SCHEMA_VERSION = 'cli-v1' as const;
 export const STREAM_URL_MAX_RETRIES = 3;
 /** Delay between retries for transient transport failures (ms). */
 export const STREAM_URL_RETRY_DELAY_MS = 1000;
+/** Hard deadline for a single artifact download attempt (ms). */
+export const STREAM_URL_TIMEOUT_MS = 30_000;
+/** Max bytes accepted from one presigned artifact URL. Keeps videos bounded. */
+export const STREAM_URL_MAX_BYTES = 200 * 1024 * 1024;
 
 /** Default radius around the failed step when `--failed-only` is set. */
 export const FAILED_ONLY_RADIUS = 1;
@@ -350,6 +355,14 @@ export function pickCodeExtension(language: string, framework: string): string {
  * naturally without breaking existing fixtures.
  */
 export function stepFilenamePrefix(stepIndex: number): string {
+  if (!Number.isSafeInteger(stepIndex) || stepIndex < 0) {
+    throw localValidationError(
+      'steps[].stepIndex',
+      'must be a non-negative integer',
+      undefined,
+      'field',
+    );
+  }
   return stepIndex >= 100 ? String(stepIndex).padStart(3, '0') : String(stepIndex).padStart(2, '0');
 }
 
@@ -659,8 +672,9 @@ async function writeStepArtifacts(
             path: `steps/${prefix}-snapshot.html`,
           };
         }
+        const kindSlug = safeFilenameSegment(entry.kind);
         const ext = sidecarExtension(entry.kind);
-        const filename = `${prefix}-${entry.kind}-${i}.${ext}`;
+        const filename = `${prefix}-${kindSlug}-${i}.${ext}`;
         await streamUrlToFile(entry.url, join(stepsTmpDir, filename), fetchImpl);
         filesWritten.push(`steps/${filename}`);
         return {
@@ -680,7 +694,7 @@ async function writeStepArtifacts(
   }
 }
 
-function sidecarExtension(kind: 'log' | 'network' | 'console' | 'screenshot' | 'snapshot'): string {
+function sidecarExtension(kind: unknown): string {
   if (kind === 'network' || kind === 'console') return 'json';
   if (kind === 'screenshot') return 'png';
   if (kind === 'snapshot') return 'html';
@@ -690,6 +704,15 @@ function sidecarExtension(kind: 'log' | 'network' | 'console' | 'screenshot' | '
   return 'txt';
 }
 
+function safeFilenameSegment(value: unknown): string {
+  const raw = typeof value === 'string' ? value : '';
+  const cleaned = raw
+    .replace(/[^A-Za-z0-9_-]+/g, '-')
+    .replace(/^-+|-+$/g, '')
+    .slice(0, 64);
+  return cleaned.length > 0 ? cleaned : 'artifact';
+}
+
 /**
  * Stream a presigned URL into a file with bounded retry on transport
  * failures. 4xx (presigned URL expired or unauthorized) is NOT
@@ -711,12 +734,18 @@ export async function streamUrlToFile(
   fetchImpl: FetchImpl,
   deps?: { sleep?: (ms: number) => Promise<void> },
 ): Promise<void> {
+  validateArtifactUrl(url);
   const sleepFn = deps?.sleep ?? ((ms: number) => new Promise<void>(r => setTimeout(r, ms)));
   for (let attempt = 1; attempt <= STREAM_URL_MAX_RETRIES; attempt++) {
+    const controller = new AbortController();
+    const timeout = setTimeout(() => controller.abort(), STREAM_URL_TIMEOUT_MS);
     let response: Response;
     try {
-      response = await fetchImpl(url);
+      response = await fetchImpl(url, { signal: controller.signal });
+      assertContentLengthWithinLimit(response, url);
     } catch (err) {
+      clearTimeout(timeout);
+      if (err instanceof ApiError) throw err;
       const message = err instanceof Error ? err.message : String(err);
       if (attempt < STREAM_URL_MAX_RETRIES) {
         await sleepFn(STREAM_URL_RETRY_DELAY_MS);
@@ -725,6 +754,7 @@ export async function streamUrlToFile(
       throw new TransportError(`Failed to download presigned URL ${url}: ${message}`);
     }
     if (!response.ok) {
+      clearTimeout(timeout);
       // Non-2xx: the URL itself is bad (expired, unauthorized, not found).
       // Retrying the same URL won't help — surface immediately.
       throw ApiError.fromEnvelope({
@@ -746,9 +776,13 @@ export async function streamUrlToFile(
       // a tolerable amount of memory.
       try {
         const buffer = Buffer.from(await response.arrayBuffer());
+        assertBytesWithinLimit(buffer.byteLength, url);
         await writeFile(filePath, buffer);
+        clearTimeout(timeout);
         return;
       } catch (err) {
+        clearTimeout(timeout);
+        if (err instanceof ApiError) throw err;
         const message = err instanceof Error ? err.message : String(err);
         if (attempt < STREAM_URL_MAX_RETRIES) {
           await sleepFn(STREAM_URL_RETRY_DELAY_MS);
@@ -766,11 +800,13 @@ export async function streamUrlToFile(
     const fileSink = createWriteStream(filePath);
     try {
       const webBody = response.body as unknown as NodeReadableStream<Uint8Array>;
-      const { Readable } = await import('node:stream');
       const nodeStream = Readable.fromWeb(webBody);
-      await pipeline(nodeStream, fileSink as unknown as Writable);
+      await pipeline(nodeStream, artifactByteLimit(url), fileSink as unknown as Writable);
+      clearTimeout(timeout);
       return;
     } catch (err) {
+      clearTimeout(timeout);
+      if (err instanceof ApiError) throw err;
       const message = err instanceof Error ? err.message : String(err);
       if (attempt < STREAM_URL_MAX_RETRIES) {
         await sleepFn(STREAM_URL_RETRY_DELAY_MS);
@@ -781,6 +817,70 @@ export async function streamUrlToFile(
   }
 }
 
+function validateArtifactUrl(url: string): void {
+  try {
+    assertNotLocal(url);
+  } catch (err) {
+    if (err instanceof ApiError) {
+      throw ApiError.fromEnvelope({
+        error: {
+          code: 'VALIDATION_ERROR',
+          message: 'Failure bundle artifact URL is invalid.',
+          nextAction:
+            'Re-run `testsprite test failure get`. Report the requestId to support if the bundle keeps returning a private or invalid artifact URL.',
+          requestId: 'local',
+          details: { ...err.details, field: 'artifact-url', url },
+        },
+      });
+    }
+    throw err;
+  }
+}
+
+function assertContentLengthWithinLimit(response: Response, url: string): void {
+  const raw = response.headers.get('content-length');
+  if (!raw) return;
+  const parsed = Number(raw);
+  if (!Number.isFinite(parsed) || parsed < 0) return;
+  assertBytesWithinLimit(parsed, url);
+}
+
+function artifactByteLimit(url: string): Transform {
+  let bytes = 0;
+  return new Transform({
+    transform(
+      chunk: Buffer | Uint8Array | string,
+      encoding: BufferEncoding,
+      cb: TransformCallback,
+    ) {
+      const length =
+        typeof chunk === 'string' ? Buffer.byteLength(chunk, encoding) : chunk.byteLength;
+      bytes += length;
+      try {
+        assertBytesWithinLimit(bytes, url);
+      } catch (err) {
+        cb(err as Error);
+        return;
+      }
+      cb(null, chunk);
+    },
+  });
+}
+
+function assertBytesWithinLimit(bytes: number, url: string): void {
+  if (bytes <= STREAM_URL_MAX_BYTES) return;
+  throw ApiError.fromEnvelope({
+    error: {
+      code: 'PAYLOAD_TOO_LARGE',
+      message: 'Failure bundle artifact is too large.',
+      nextAction:
+        'Re-run `testsprite test failure get`. Report the requestId to support if the bundle keeps returning an oversized artifact.',
+      requestId: 'local',
+      details: { url, bytes, maxBytes: STREAM_URL_MAX_BYTES },
+    },
+  });
+}
+
 function isPresignedUrl(value: string): boolean {
   return value.startsWith('https://');
 }