feat(scraper): enable external script fetching in sandbox

arabold · arabold · commit 88b7e7a430b8 · 2025-04-17T09:21:11.000-07:00
Introduces a fetchScriptContent callback to executeJsInSandbox, allowing the HtmlJsExecutorMiddleware to securely fetch external scripts using the context's fetcher before executing them in the Node.js vm sandbox.

Updates relevant types, strategies, and tests.
diff --git a/src/scraper/middleware/components/HtmlJsExecutorMiddleware.test.ts b/src/scraper/middleware/components/HtmlJsExecutorMiddleware.test.ts
@@ -1,7 +1,19 @@
 import type { DOMWindow } from "jsdom";
-import { type Mock, beforeEach, describe, expect, it, vi } from "vitest";
+import {
+  type Mock,
+  type MockedObject,
+  beforeEach,
+  describe,
+  expect,
+  it,
+  vi,
+} from "vitest";
+import type { ContentFetcher, RawContent } from "../../fetcher/types";
 import { executeJsInSandbox } from "../../utils/sandbox";
-import type { SandboxExecutionResult } from "../../utils/sandbox";
+import type {
+  SandboxExecutionOptions,
+  SandboxExecutionResult,
+} from "../../utils/sandbox";
 import type { ContentProcessingContext } from "../types";
 import { HtmlJsExecutorMiddleware } from "./HtmlJsExecutorMiddleware";
 
@@ -15,10 +27,17 @@ describe("HtmlJsExecutorMiddleware", () => {
   let mockContext: ContentProcessingContext;
   let mockNext: Mock;
   let mockSandboxResult: SandboxExecutionResult;
+  let mockFetcher: MockedObject<ContentFetcher>;
 
   beforeEach(() => {
     vi.resetAllMocks();
 
+    // Create a mock fetcher
+    mockFetcher = vi.mocked<ContentFetcher>({
+      canFetch: vi.fn(),
+      fetch: vi.fn(),
+    });
+
     mockContext = {
       source: "http://example.com",
       content: "", // Will be set in tests
@@ -31,8 +50,10 @@ describe("HtmlJsExecutorMiddleware", () => {
         url: "http://example.com", // Can reuse context.source
         library: "test-lib",
         version: "1.0.0",
+        signal: undefined, // Initialize signal
         // Add other optional ScraperOptions properties if needed for specific tests
       },
+      fetcher: mockFetcher,
       // dom property might be added by the middleware
     };
     mockNext = vi.fn().mockResolvedValue(undefined);
@@ -53,10 +74,14 @@ describe("HtmlJsExecutorMiddleware", () => {
     await middleware.process(mockContext, mockNext);
 
     expect(executeJsInSandbox).toHaveBeenCalledOnce();
-    expect(executeJsInSandbox).toHaveBeenCalledWith({
-      html: "<p>Initial</p><script></script>",
-      url: "http://example.com",
-    });
+    // Verify fetchScriptContent is passed as a function
+    expect(executeJsInSandbox).toHaveBeenCalledWith(
+      expect.objectContaining({
+        html: "<p>Initial</p><script></script>",
+        url: "http://example.com",
+        fetchScriptContent: expect.any(Function),
+      }),
+    );
   });
 
   it("should update context.content with finalHtml from sandbox result", async () => {
@@ -130,10 +155,14 @@ describe("HtmlJsExecutorMiddleware", () => {
     await middleware.process(mockContext, mockNext);
 
     expect(executeJsInSandbox).toHaveBeenCalledOnce();
-    expect(executeJsInSandbox).toHaveBeenCalledWith({
-      html: initialHtml,
-      url: "http://example.com",
-    });
+    // Updated assertion to expect fetchScriptContent
+    expect(executeJsInSandbox).toHaveBeenCalledWith(
+      expect.objectContaining({
+        html: initialHtml,
+        url: "http://example.com",
+        fetchScriptContent: expect.any(Function),
+      }),
+    );
     expect(mockNext).toHaveBeenCalledOnce();
   });
 
@@ -152,4 +181,112 @@ describe("HtmlJsExecutorMiddleware", () => {
     );
     expect(mockNext).not.toHaveBeenCalled(); // Should not proceed if middleware itself fails
   });
+
+  // --- Tests for fetchScriptContent callback logic ---
+
+  it("fetchScriptContent callback should use context.fetcher to fetch script", async () => {
+    mockContext.content = "<p>Initial</p><script src='ext.js'></script>";
+    const middleware = new HtmlJsExecutorMiddleware();
+    const mockScriptContent = "console.log('fetched');";
+    const mockRawContent: RawContent = {
+      content: Buffer.from(mockScriptContent),
+      mimeType: "application/javascript",
+      source: "http://example.com/ext.js",
+    };
+    mockFetcher.fetch.mockResolvedValue(mockRawContent);
+
+    await middleware.process(mockContext, mockNext);
+
+    // Get the options passed to the sandbox mock
+    const sandboxOptions = (executeJsInSandbox as Mock).mock
+      .calls[0][0] as SandboxExecutionOptions;
+    expect(sandboxOptions.fetchScriptContent).toBeDefined();
+
+    // Invoke the callback to test its behavior
+    const fetchedContent = await sandboxOptions.fetchScriptContent!(
+      "http://example.com/ext.js",
+    );
+
+    expect(mockFetcher.fetch).toHaveBeenCalledWith("http://example.com/ext.js", {
+      signal: undefined,
+      followRedirects: true,
+    });
+    expect(fetchedContent).toBe(mockScriptContent);
+    expect(mockContext.errors).toHaveLength(0); // No errors expected during fetch
+  });
+
+  it("fetchScriptContent callback should handle fetcher errors", async () => {
+    mockContext.content = "<p>Initial</p><script src='ext.js'></script>";
+    const middleware = new HtmlJsExecutorMiddleware();
+    const fetchError = new Error("Network Failed");
+    mockFetcher.fetch.mockRejectedValue(fetchError);
+
+    await middleware.process(mockContext, mockNext);
+
+    const sandboxOptions = (executeJsInSandbox as Mock).mock
+      .calls[0][0] as SandboxExecutionOptions;
+    const fetchedContent = await sandboxOptions.fetchScriptContent!(
+      "http://example.com/ext.js",
+    );
+
+    expect(mockFetcher.fetch).toHaveBeenCalledWith("http://example.com/ext.js", {
+      signal: undefined,
+      followRedirects: true,
+    });
+    expect(fetchedContent).toBeNull();
+    expect(mockContext.errors).toHaveLength(1);
+    expect(mockContext.errors[0].message).toContain(
+      "Failed to fetch external script http://example.com/ext.js: Network Failed",
+    );
+    expect(mockContext.errors[0].cause).toBe(fetchError);
+  });
+
+  it("fetchScriptContent callback should handle non-JS MIME types", async () => {
+    mockContext.content = "<p>Initial</p><script src='style.css'></script>";
+    const middleware = new HtmlJsExecutorMiddleware();
+    const mockRawContent: RawContent = {
+      content: "body { color: red; }",
+      mimeType: "text/css", // Incorrect MIME type
+      source: "http://example.com/style.css",
+    };
+    mockFetcher.fetch.mockResolvedValue(mockRawContent);
+
+    await middleware.process(mockContext, mockNext);
+
+    const sandboxOptions = (executeJsInSandbox as Mock).mock
+      .calls[0][0] as SandboxExecutionOptions;
+    const fetchedContent = await sandboxOptions.fetchScriptContent!(
+      "http://example.com/style.css",
+    );
+
+    expect(mockFetcher.fetch).toHaveBeenCalledWith("http://example.com/style.css", {
+      signal: undefined,
+      followRedirects: true,
+    });
+    expect(fetchedContent).toBeNull();
+    expect(mockContext.errors).toHaveLength(1);
+    expect(mockContext.errors[0].message).toContain(
+      "Skipping execution of external script http://example.com/style.css due to unexpected MIME type: text/css",
+    );
+  });
+
+  it("fetchScriptContent callback should handle missing fetcher in context", async () => {
+    mockContext.content = "<p>Initial</p><script src='ext.js'></script>";
+    mockContext.fetcher = undefined; // Remove fetcher for this test
+    const middleware = new HtmlJsExecutorMiddleware();
+
+    await middleware.process(mockContext, mockNext);
+
+    const sandboxOptions = (executeJsInSandbox as Mock).mock
+      .calls[0][0] as SandboxExecutionOptions;
+    const fetchedContent = await sandboxOptions.fetchScriptContent!(
+      "http://example.com/ext.js",
+    );
+
+    expect(mockFetcher.fetch).not.toHaveBeenCalled(); // Fetcher should not be called
+    expect(fetchedContent).toBeNull();
+    expect(mockContext.errors).toHaveLength(0); // Only logs a warning, doesn't add error
+    // We can't easily verify logger.warn was called without mocking it again here,
+    // but the null return and lack of fetch call imply the check worked.
+  });
 });
diff --git a/src/scraper/middleware/components/HtmlJsExecutorMiddleware.ts b/src/scraper/middleware/components/HtmlJsExecutorMiddleware.ts
@@ -1,5 +1,6 @@
 import { logger } from "../../../utils/logger";
-import { executeJsInSandbox } from "../../utils/sandbox"; // Updated import path
+import type { FetchOptions, RawContent } from "../../fetcher/types";
+import { executeJsInSandbox } from "../../utils/sandbox";
 import type { ContentProcessingContext, ContentProcessorMiddleware } from "../types";
 
 /**
@@ -32,10 +33,97 @@ export class HtmlJsExecutorMiddleware implements ContentProcessorMiddleware {
         `Executing JavaScript in sandbox for HTML content from ${context.source}`,
       );
 
+      // Define the callback for fetching external scripts
+      const fetchScriptContentCallback = async (
+        scriptUrl: string,
+      ): Promise<string | null> => {
+        if (!context.fetcher) {
+          logger.warn(
+            `No fetcher available in context to fetch external script: ${scriptUrl}`,
+          );
+          return null;
+        }
+        try {
+          logger.debug(`Fetching external script via context fetcher: ${scriptUrl}`);
+          // Pass relevant options, especially the signal for cancellation
+          const fetchOptions: FetchOptions = {
+            signal: context.options?.signal, // Pass signal from context if available
+            followRedirects: true, // Generally want to follow redirects for scripts
+            // timeout: context.options?.fetchTimeout // Add if timeout is configurable at context level
+          };
+          const rawContent: RawContent = await context.fetcher.fetch(
+            scriptUrl,
+            fetchOptions,
+          );
+
+          // Optional: Check MIME type to be reasonably sure it's JavaScript
+          const allowedMimeTypes = [
+            "application/javascript",
+            "text/javascript",
+            "application/x-javascript",
+          ];
+          // Allow common JS types or be lenient if type is generic/unknown
+          const mimeTypeLower = rawContent.mimeType.toLowerCase();
+          if (
+            !allowedMimeTypes.includes(mimeTypeLower) &&
+            !["application/octet-stream", "unknown/unknown", ""].includes(mimeTypeLower) // Allow empty MIME type as well
+          ) {
+            logger.warn(
+              `Skipping execution of external script ${scriptUrl} due to unexpected MIME type: ${rawContent.mimeType}`,
+            );
+            context.errors.push(
+              new Error(
+                `Skipping execution of external script ${scriptUrl} due to unexpected MIME type: ${rawContent.mimeType}`,
+              ),
+            );
+            return null;
+          }
+
+          // Convert content to string using provided encoding or default to utf-8
+          const contentBuffer = Buffer.isBuffer(rawContent.content)
+            ? rawContent.content
+            : Buffer.from(rawContent.content);
+
+          // Validate encoding before using it
+          const validEncodings: BufferEncoding[] = [
+            "ascii",
+            "utf8",
+            "utf-8",
+            "utf16le",
+            "ucs2",
+            "ucs-2",
+            "base64",
+            "base64url",
+            "latin1",
+            "binary",
+            "hex",
+          ];
+          const encoding =
+            rawContent.encoding &&
+            validEncodings.includes(rawContent.encoding.toLowerCase() as BufferEncoding)
+              ? (rawContent.encoding.toLowerCase() as BufferEncoding)
+              : "utf-8";
+
+          return contentBuffer.toString(encoding);
+        } catch (fetchError) {
+          // fetcher.fetch is expected to throw on error (e.g., 404, network error)
+          const message =
+            fetchError instanceof Error ? fetchError.message : String(fetchError);
+          logger.warn(`Failed to fetch external script ${scriptUrl}: ${message}`); // Use warn for fetch failures like 404
+          context.errors.push(
+            new Error(`Failed to fetch external script ${scriptUrl}: ${message}`, {
+              cause: fetchError,
+            }),
+          );
+          return null; // Indicate failure to the sandbox runner
+        }
+      };
+
       // TODO: Plumb timeout options from context.options if available
       const sandboxOptions = {
         html: initialHtml,
         url: context.source,
+        fetchScriptContent: fetchScriptContentCallback, // Pass the callback
         // timeout: context.options?.scriptTimeout // Example for future enhancement
       };
 
diff --git a/src/scraper/middleware/types.ts b/src/scraper/middleware/types.ts
@@ -1,4 +1,5 @@
 import type { DOMWindow } from "jsdom";
+import type { ContentFetcher } from "../fetcher/types";
 import type { ScraperOptions } from "../types";
 
 /**
@@ -22,6 +23,9 @@ export interface ContentProcessingContext {
 
   /** Optional JSDOM window object for HTML processing. */
   dom?: DOMWindow;
+
+  /** Optional fetcher instance for resolving resources relative to the source. */
+  fetcher?: ContentFetcher;
 }
 
 /**
diff --git a/src/scraper/strategies/WebScraperStrategy.ts b/src/scraper/strategies/WebScraperStrategy.ts
@@ -91,7 +91,8 @@ export class WebScraperStrategy extends BaseScraperStrategy {
         metadata: {},
         links: [],
         errors: [],
-        options: options, // Pass the full options object
+        options,
+        fetcher: this.httpFetcher,
       };
 
       let pipeline: ContentProcessingPipeline;
diff --git a/src/scraper/types.ts b/src/scraper/types.ts
@@ -39,6 +39,8 @@ export interface ScraperOptions {
   ignoreErrors?: boolean;
   /** CSS selectors for elements to exclude during HTML processing */
   excludeSelectors?: string[];
+  /** Optional AbortSignal for cancellation */
+  signal?: AbortSignal;
 }
 
 /**
diff --git a/src/scraper/utils/sandbox.test.ts b/src/scraper/utils/sandbox.test.ts
diff --git a/src/scraper/utils/sandbox.ts b/src/scraper/utils/sandbox.ts

Original file line number	Diff line number	Diff line change
`@@ -39,6 +39,8 @@ export interface ScraperOptions {`
`39`	`39`	`ignoreErrors?: boolean;`
`40`	`40`	`/** CSS selectors for elements to exclude during HTML processing */`
`41`	`41`	`excludeSelectors?: string[];`
	`42`	`+ /** Optional AbortSignal for cancellation */`
	`43`	`+ signal?: AbortSignal;`
`42`	`44`	`}`
`43`	`45`
`44`	`46`	`/**`