Validate diff input: restrict globs and options, return 400/404 instead of 500

package.json

@@ -72,6 +72,7 @@
         "hosted-git-info": "^10.1.1",
         "libnpmdiff": "^9.0.2",
         "lucide-react": "^0.577.0",
+        "minimatch": "^10.2.6",
         "next": "~16.3.5",
         "npm-package-arg": "^14.0.0",
         "pacote": "^22.0.0",

src/app/[...parts]/_page/NpmDiff/ViewTypeSwitch.tsx

@@ -35,37 +35,35 @@ const ViewTypeButton = forwardRef<
             ...props
         },
         ref,
-    ) => (
-        <Button
-            variant="outline"
-            className={cx(
-                "not-last:rounded-r-none",
-                "not-last:border-r-0",
-                "not-first:rounded-l-none",
-                className!,
-            )}
-            isActive={currentViewType === viewType}
-            asChild
-            {...props}
-            ref={ref}
-        >
-            <Link
-                href={{
-                    pathname,
-                    query: {
-                        ...(searchParams &&
-                            Object.fromEntries(searchParams.entries())),
-                        [DIFF_TYPE_PARAM_NAME]: viewType,
-                    },
-                }}
-                replace
-                shallow
-                prefetch={false}
+    ) => {
+        const params = new URLSearchParams(searchParams?.toString());
+        params.set(DIFF_TYPE_PARAM_NAME, viewType);
+
+        return (
+            <Button
+                variant="outline"
+                className={cx(
+                    "not-last:rounded-r-none",
+                    "not-last:border-r-0",
+                    "not-first:rounded-l-none",
+                    className!,
+                )}
+                isActive={currentViewType === viewType}
+                asChild
+                {...props}
+                ref={ref}
             >
-                {children}
-            </Link>
-        </Button>
-    ),
+                <Link
+                    href={`${pathname ?? ""}?${params}`}
+                    replace
+                    shallow
+                    prefetch={false}
+                >
+                    {children}
+                </Link>
+            </Button>
+        );
+    },
 );
 ViewTypeButton.displayName = "ViewTypeButton";
 

src/app/[...parts]/page.tsx

@@ -2,16 +2,23 @@ import { type Metadata } from "next";
 import { notFound, redirect } from "next/navigation";
 import { type JSX, Suspense } from "react";
 import { type ViewType } from "react-diff-view";
+import Code from "^/components/ui/Code";
 import { createSimplePackageSpec } from "^/lib/createSimplePackageSpec";
 import { DEFAULT_DIFF_FILES_GLOB } from "^/lib/default-diff-files";
-import destination from "^/lib/destination";
-import { parseQuery, type QueryParams } from "^/lib/query";
+import destination, {
+    type Destination,
+    SpecNotFoundError,
+} from "^/lib/destination";
+import {
+    parseDiffOptions,
+    type QueryParams,
+    toSearchString,
+} from "^/lib/query";
 import { simplePackageSpecToString } from "^/lib/SimplePackageSpec";
-import decodeParts from "^/lib/utils/decodeParts";
 import { isCatalogPage } from "^/lib/utils/isCatalogPage";
+import parseParts from "^/lib/utils/parseParts";
 import specsToDiff from "^/lib/utils/specsToDiff";
-import splitParts from "^/lib/utils/splitParts";
-import validateSpecs from "^/lib/utils/validateSpecs";
+import ErrorBox from "./_error/ErrorBox";
 import BundlephobiaDiff from "./_page/BundlephobiaDiff";
 import CatalogPage from "./_page/catalog/CatalogPage";
 import { generateCatalogMetadata } from "./_page/catalog/generateCatalogMetadata";
@@ -32,9 +39,9 @@ export async function generateMetadata({
     params,
 }: DiffPageProps): Promise<Metadata> {
     const { parts } = await params;
-    const specs = splitParts(decodeParts(parts));
+    const specs = parseParts(parts, { decode: true });
 
-    if (!validateSpecs(specs)) {
+    if (specs == null) {
         notFound();
     }
 
@@ -55,37 +62,51 @@ const DiffPageInner = async ({
     searchParams,
 }: DiffPageProps): Promise<JSX.Element> => {
     const { parts } = await params;
-    const { diffFiles, ...optionsQuery } = await searchParams;
+    const query = await searchParams;
+    const { diffFiles, ...optionsQuery } = query;
 
-    const specsOrVersions = splitParts(decodeParts(parts));
+    const specsOrVersions = parseParts(parts, { decode: true });
 
-    if (!validateSpecs(specsOrVersions)) {
+    if (specsOrVersions == null) {
         notFound();
     }
 
     if (isCatalogPage(specsOrVersions)) {
         return <CatalogPage specs={specsOrVersions} />;
     }
 
-    const { redirect: redirectTarget, canonicalSpecs } =
-        await destination(specsOrVersions);
+    const parsedOptions = parseDiffOptions({
+        // If no diffFiles is passed, use the default.
+        // This is done here, since we don't want a fall back in the API
+        diffFiles: diffFiles ?? DEFAULT_DIFF_FILES_GLOB,
+        ...optionsQuery,
+    });
 
-    if (redirectTarget !== false) {
-        const specsStr = specsToDiff(canonicalSpecs);
-        const searchStr = Object.entries(await searchParams)
-            .map(([key, value]) => `${key}=${value}`)
-            .join("&");
-
-        redirect(
-            `/${specsStr}` + (searchStr?.length > 0 ? `?${searchStr}` : ""),
+    if (!parsedOptions.ok) {
+        return (
+            <ErrorBox className="flex flex-col items-start gap-2 self-center">
+                <h3>Invalid options</h3>
+                <Code className="max-w-2xl">{parsedOptions.message}</Code>
+            </ErrorBox>
         );
+    }
+
+    let target: Destination;
+    try {
+        target = await destination(specsOrVersions);
+    } catch (e) {
+        if (e instanceof SpecNotFoundError) {
+            notFound();
+        }
+        throw e;
+    }
+
+    const { redirect: redirectTarget, canonicalSpecs } = target;
+
+    if (redirectTarget !== false) {
+        redirect(`/${specsToDiff(canonicalSpecs)}${toSearchString(query)}`);
     } else {
-        const options = parseQuery({
-            // If no diffFiles is passed, use the default.
-            // This is done here, since we don't want a fall back in the API
-            diffFiles: diffFiles ?? DEFAULT_DIFF_FILES_GLOB,
-            ...optionsQuery,
-        });
+        const { options } = parsedOptions;
 
         const [a, b] = canonicalSpecs.map((spec) =>
             createSimplePackageSpec(spec),

src/app/api/[...parts]/route.ts

@@ -1,11 +1,13 @@
 import { type NextRequest, NextResponse } from "next/server";
-import destination from "^/lib/destination";
+import destination, {
+    type Destination,
+    SpecNotFoundError,
+} from "^/lib/destination";
 import npmDiff from "^/lib/npmDiff";
-import { parseQuery } from "^/lib/query";
+import { fromSearchParams, parseDiffOptions } from "^/lib/query";
 import { defaultPageCachingHeaders } from "^/lib/utils/headers";
+import parseParts from "^/lib/utils/parseParts";
 import specsToDiff from "^/lib/utils/specsToDiff";
-import splitParts from "^/lib/utils/splitParts";
-import validateSpecs from "^/lib/utils/validateSpecs";
 
 export const maxDuration = 60;
 
@@ -21,19 +23,33 @@ export interface DiffApiContext {
 export async function GET(req: NextRequest, { params }: DiffApiContext) {
     const { parts } = await params;
     const { searchParams } = new URL(req.url);
-    const options = Object.fromEntries(searchParams);
 
-    const specsOrVersions = splitParts(parts);
+    const specsOrVersions = parseParts(parts);
 
-    if (!validateSpecs(specsOrVersions)) {
+    if (specsOrVersions == null) {
         return NextResponse.json("Invalid package spec", { status: 400 });
     }
 
-    const { redirect: red, canonicalSpecs } =
-        await destination(specsOrVersions);
+    const parsedOptions = parseDiffOptions(fromSearchParams(searchParams));
+
+    if (!parsedOptions.ok) {
+        return NextResponse.json(parsedOptions.message, { status: 400 });
+    }
+
+    let target: Destination;
+    try {
+        target = await destination(specsOrVersions);
+    } catch (e) {
+        if (e instanceof SpecNotFoundError) {
+            return NextResponse.json(e.message, { status: 404 });
+        }
+        throw e;
+    }
+
+    const { redirect: red, canonicalSpecs } = target;
 
     if (red === false) {
-        const result = await npmDiff(canonicalSpecs, parseQuery(options));
+        const result = await npmDiff(canonicalSpecs, parsedOptions.options);
 
         if (result.ok) {
             return new NextResponse(result.diff, {
@@ -47,8 +63,8 @@ export async function GET(req: NextRequest, { params }: DiffApiContext) {
     } else {
         const newUrl = new URL(`/api/${specsToDiff(canonicalSpecs)}`, req.url);
 
-        Array.from(searchParams).forEach(([key, value]) => {
-            newUrl.searchParams.set(key, value);
+        searchParams.forEach((value, key) => {
+            newUrl.searchParams.append(key, value);
         });
 
         return NextResponse.redirect(

src/app/page.client.tsx

@@ -6,6 +6,7 @@ import { useBoolean } from "react-use";
 import Stack from "^/components/ui/Stack";
 import { type AutocompleteSuggestion } from "^/lib/autocomplete";
 import { DEFAULT_DIFF_FILES_GLOB } from "^/lib/default-diff-files";
+import { toSearchString } from "^/lib/query/searchParams";
 import ExamplesList from "./_page/ExamplesList";
 import MainForm from "./_page/MainForm/MainForm";
 import OptionsForm from "./_page/OptionsForm";
@@ -47,11 +48,7 @@ const IndexPageClient: FunctionComponent<LandingProps> = ({
     const goToDiff = (a: string | undefined, b: string | undefined): void => {
         setLoading(true);
 
-        router.push(
-            `/${a}...${b}?${Object.entries(query)
-                .map(([k, v]) => `${k}=${v}`)
-                .join("&")}`,
-        );
+        router.push(`/${a}...${b}${toSearchString(query)}`);
     };
 
     return (

src/lib/destination/SpecNotFoundError.ts

@@ -0,0 +1,6 @@
+export default class SpecNotFoundError extends Error {
+    constructor(spec: string) {
+        super(`Could not find "${spec}"`);
+        this.name = "SpecNotFoundError";
+    }
+}

src/lib/destination/canonicalSpec.test.ts

@@ -1,6 +1,7 @@
 import { manifest, resolve } from "pacote";
 import type pacote from "pacote";
 import canonicalSpec from "./canonicalSpec";
+import SpecNotFoundError from "./SpecNotFoundError";
 
 // Increase the timeout for long running tests
 jest.setTimeout(15_000);
@@ -199,4 +200,12 @@ describe("canonicalSpec", () => {
             expect(() => t("file:example")).rejects.toThrow(expectedError),
         ]);
     });
+
+    it("Throws SpecNotFoundError for a package missing from the registry", () =>
+        expect(() =>
+            t("npm-diff-app-does-not-exist-9f3a2c@latest"),
+        ).rejects.toThrow(SpecNotFoundError));
+
+    it("Throws SpecNotFoundError for a range with no matching version", () =>
+        expect(() => t("chalk@^999.0.0")).rejects.toThrow(SpecNotFoundError));
 });

src/lib/destination/canonicalSpec.ts

@@ -1,6 +1,8 @@
 import { cacheLife } from "next/cache";
 import npa, { type AliasResult } from "npm-package-arg";
 import { manifest, resolve } from "pacote";
+import { hasErrorCode } from "^/lib/utils/hasErrorCode";
+import SpecNotFoundError from "./SpecNotFoundError";
 
 const hashFinder = /(?:\#.*)?$/;
 
@@ -58,14 +60,34 @@ async function handleNpaResult(result: npa.Result): Promise<string> {
  * - https://github.com/npm/npm-package-arg#result-object
  * - https://docs.npmjs.com/cli/v7/commands/npm-install
  */
-async function canonicalSpec(spec: string): Promise<string> {
+async function cachedCanonicalSpec(spec: string): Promise<string | null> {
     "use cache";
 
-    cacheLife("hours");
+    try {
+        const canonical = await handleNpaResult(npa(spec));
+
+        cacheLife("hours");
+
+        return canonical;
+    } catch (e) {
+        if (hasErrorCode(e, "E404") || hasErrorCode(e, "ETARGET")) {
+            cacheLife("minutes");
+
+            return null;
+        }
+
+        throw e;
+    }
+}
 
-    const result = npa(spec);
+async function canonicalSpec(spec: string): Promise<string> {
+    const canonical = await cachedCanonicalSpec(spec);
+
+    if (canonical == null) {
+        throw new SpecNotFoundError(spec);
+    }
 
-    return handleNpaResult(result);
+    return canonical;
 }
 
 export default canonicalSpec;

src/lib/destination/index.ts

@@ -1,5 +1,6 @@
 import arrayEquals from "^/lib/utils/arrayEquals";
 import canonicalSpec from "./canonicalSpec";
+import SpecNotFoundError from "./SpecNotFoundError";
 import versionsToSpecs from "./versionsToSpecs";
 
 export type Redirect = false | "temporary" | "permanent";
@@ -52,3 +53,4 @@ async function destination(
 }
 
 export default destination;
+export { SpecNotFoundError };

src/lib/query/index.ts

@@ -1,5 +1,6 @@
-import parseQuery from "./parseQuery";
+import parseDiffOptions from "./parseDiffOptions";
 import type QueryParams from "./QueryParams";
+import { fromSearchParams, toSearchString } from "./searchParams";
 
 export type { QueryParams };
-export { parseQuery };
+export { fromSearchParams, parseDiffOptions, toSearchString };