Fix: Add null/undefined validation before `.trim()` calls to prevent TypeError

[vzegnameta]

Description

The pdf-analyzer service crashes with TypeError: Cannot read properties of undefined (reading 'trim') when requests contain malformed or missing pdf_source parameters.

Error Logs

[/analyze] Error processing PDF: TypeError: Cannot read properties of undefined (reading 'trim')
    at validateLocalPath (file:///app/dist/pdf-utils.js:37:33)
    at readPdfBytes (file:///app/dist/pdf-utils.js:90:5)
    at analyzePdf (file:///app/dist/service.js:248:11)

Root Cause

The pdf-utils.ts file calls .trim() on string parameters without first validating that they are non-null and of type string. When a request contains an undefined or null pdf_source, the code crashes with an unhelpful TypeError instead of a descriptive validation error.

Solution

Add null/undefined checks before calling .trim() in the following functions in src/pdf-utils.ts:

Changes Required

1. isGeminiFileUri() function

export function isGeminiFileUri(source: string): boolean {
  if (source == null || typeof source !== "string") {
    return false;
  }
  return source.startsWith(GEMINI_FILE_URI_PREFIX);
}

2. isUrl() function

export function isUrl(source: string): boolean {
  if (source == null || typeof source !== "string") {
    return false;
  }
  if (isGeminiFileUri(source)) {
    return false;
  }
  try {
    const url = new URL(source);
    return url.protocol === "http:" || url.protocol === "https:";
  } catch {
    return false;
  }
}

3. validateLocalPath() function

export function validateLocalPath(pdfPath: string): void {
  if (pdfPath == null) {
    throw new Error("PDF path is required but was null or undefined");
  }

  if (typeof pdfPath !== "string") {
    throw new Error(`PDF path must be a string, got ${typeof pdfPath}`);
  }

  const trimmedPath = pdfPath.trim();

  if (trimmedPath === "") {
    throw new Error("PDF path cannot be empty");
  }

  if (!path.isAbsolute(trimmedPath)) {
    throw new Error(`PDF path must be absolute: ${trimmedPath}`);
  }

  if (!fs.existsSync(trimmedPath)) {
    throw new Error(`PDF file not found: ${trimmedPath}`);
  }

  const stats = fs.statSync(trimmedPath);
  if (stats.isDirectory()) {
    throw new Error(`Path is a directory, not a file: ${trimmedPath}`);
  }

  if (!trimmedPath.toLowerCase().endsWith(".pdf")) {
    throw new Error(`File is not a PDF: ${trimmedPath}`);
  }
}

4. fetchPdfFromUrl() function

export async function fetchPdfFromUrl(url: string): Promise<Buffer> {
  if (url == null) {
    throw new Error("URL is required but was null or undefined");
  }

  if (typeof url !== "string") {
    throw new Error(`URL must be a string, got ${typeof url}`);
  }

  const trimmedUrl = url.trim();
  if (trimmedUrl === "") {
    throw new Error("URL cannot be empty");
  }

  const controller = new AbortController();
  const timeoutId = setTimeout(() => controller.abort(), FETCH_TIMEOUT_MS);

  let response: Response;
  try {
    response = await fetch(trimmedUrl, { signal: controller.signal });
  } catch (error) {
    if (error instanceof Error && error.name === "AbortError") {
      throw new Error(`Request timed out after ${FETCH_TIMEOUT_MS / 1000}s`);
    }
    const message = error instanceof Error ? error.message : "Unknown error";
    throw new Error(`Failed to fetch URL: ${message}`);
  } finally {
    clearTimeout(timeoutId);
  }

  if (!response.ok) {
    throw new Error(`HTTP ${response.status}: ${response.statusText}`);
  }

  const contentType = response.headers.get("content-type");
  if (
    contentType &&
    !contentType.includes("application/pdf") &&
    !contentType.includes("octet-stream")
  ) {
    throw new Error(`URL does not point to a PDF file. Content-Type: ${contentType}`);
  }

  const arrayBuffer = await response.arrayBuffer();
  return Buffer.from(arrayBuffer);
}

5. readPdfBytes() function

export function readPdfBytes(pdfPath: string): Buffer {
  if (pdfPath == null) {
    throw new Error("PDF path is required but was null or undefined");
  }

  if (typeof pdfPath !== "string") {
    throw new Error(`PDF path must be a string, got ${typeof pdfPath}`);
  }

  validateLocalPath(pdfPath);
  return fs.readFileSync(pdfPath.trim());
}

Test Updates

Update the test in src/service.test.ts to reflect the new error message:

// Before
it("trims whitespace from paths", () => {
  expect(() => validateLocalPath("   ")).toThrow("must be absolute");
  expect(() => validateLocalPath("  /nonexistent.pdf  ")).toThrow("not found");
});

// After
it("trims whitespace from paths", () => {
  expect(() => validateLocalPath("   ")).toThrow("cannot be empty");
  expect(() => validateLocalPath("  /nonexistent.pdf  ")).toThrow("not found");
});

Benefits

1. Better error messages: Instead of cryptic TypeError: Cannot read properties of undefined, users get descriptive errors like "PDF path is required but was null or undefined"
2. Graceful handling: isUrl() and isGeminiFileUri() return false for invalid inputs instead of crashing
3. Defensive programming: Validates inputs at function boundaries
4. Easier debugging: Clear error messages help identify the source of malformed requests

Impact

This fix prevents service crashes and provides better error messages for API consumers when they send malformed requests.

Testing

All existing tests pass with these changes (except one unrelated test that fails due to missing fixture file in the test environment).

[valentinozegna]

After investigating, I think the proposed fix patches the symptom rather than the root cause, and I want to flag that before this lands. Posting the alternative I went with in #43.

What was actually happening

The [/analyze] log prefix in the stack trace narrows the bug to the HTTP REST endpoint specifically. Comparing the two transports:

• MCP path (src/server.ts): registers analyze_pdf with a zod inputSchema. The MCP SDK validates against that schema before invoking the handler, so pdf_source is guaranteed to be a non-empty string (or non-empty string[]) by the time analyzePdf runs.
• HTTP path (src/transports/http.ts:39, before the fix):

  const { pdf_source, queries } = body;
  if (!pdf_source || !queries || !Array.isArray(queries) || queries.length === 0) {
    res.writeHead(400, ...);
    return;
  }

  Hand-rolled, and weaker than zod. !pdf_source only rejects falsy values (undefined, null, "", 0, false). It accepts:
  • numbers (!42 === false)
  • plain objects (!{} === false)
  • boxed strings, e.g. new String("...") from a misbehaving client
  • non-string entries inside an array

Any of those reach analyzePdf, run through classifySource, fall into the { kind: "path", path: <non-string> } branch, and ultimately hit validateLocalPath's .trim() call. Hence the crash.

Why the proposed null guards in pdf-utils.ts aren't the right fix

The five functions named in the issue (isGeminiFileUri, isUrl, validateLocalPath, fetchPdfFromUrl, readPdfBytes) all sit behind the boundary. Their parameter types say string, and they're only ever called from internal code that has a typed PdfSource union. Adding if (x == null || typeof x !== "string") to each one:

1. Validates the same value 3+ times along one call path.
2. Pushes boundary concerns into the pure core, against the codebase's "validate at boundaries" principle.
3. Lets the actual bypass at transports/http.ts:39 keep existing, so the next field added without a guard reproduces the same class of bug.
4. Doesn't catch the cases I listed above for the HTTP entry — e.g., pdf_source = 123 would still pass the !pdf_source check, and the new typeof !== "string" guard would fire, but only after duplicating the work zod was already supposed to do.

What #43 does instead

• Replaces the hand-rolled HTTP check with AnalyzePdfInputSchema.safeParse(body), returning a 400 with zod's path-aware error details.
• Hoists the field defs into a shared AnalyzePdfInputShape and points both server.ts (MCP) and http.ts (REST) at it, so the two transports stay in sync going forward.
• Leaves src/pdf-utils.ts untouched — its callers can no longer pass non-strings.
• Adds 5 regression tests in transports/http.test.ts covering missing pdf_source, non-string pdf_source, empty queries, empty-string queries, and non-JSON bodies.

Net effect: stricter validation, single source of truth, better error messages for clients (zod path + reason vs. a single English sentence), no defensive code scattered through internal helpers.

If there's a reason to prefer the original approach I'm missing — e.g., another caller path I didn't trace — happy to revisit.