Fix crash when using user mention or accessing invalid change requests (#3051)

Author: SamyPesse

packages/gitbook-v2/src/lib/context.ts

@@ -13,7 +13,13 @@ import type {
     SiteStructure,
     Space,
 } from '@gitbook/api';
-import { type GitBookDataFetcher, createDataFetcher, throwIfDataError } from '@v2/lib/data';
+import {
+    type GitBookDataFetcher,
+    createDataFetcher,
+    getDataOrNull,
+    throwIfDataError,
+} from '@v2/lib/data';
+import { notFound } from 'next/navigation';
 import { assert } from 'ts-essentials';
 import { GITBOOK_URL } from './env';
 import { type ImageResizer, createImageResizer } from './images';
@@ -277,7 +283,7 @@ export async function fetchSpaceContextByIds(
             })
         ),
         ids.changeRequest
-            ? throwIfDataError(
+            ? getDataOrNull(
                   dataFetcher.getChangeRequest({
                       spaceId: ids.space,
                       changeRequestId: ids.changeRequest,
@@ -286,17 +292,30 @@ export async function fetchSpaceContextByIds(
             : null,
     ]);
 
-    const revisionId = changeRequest?.revision ?? ids.revision ?? space.revision;
+    if (ids.changeRequest && !changeRequest) {
+        // When trying to render a change request with an invalid / non-existing ID,
+        // we should return a 404.
+        notFound();
+    }
+
+    const revisionId = ids.revision ?? changeRequest?.revision ?? space.revision;
 
-    const pages = await throwIfDataError(
+    const pages = await getDataOrNull(
         dataFetcher.getRevisionPages({
             spaceId: ids.space,
             revisionId,
             // We only care about the Git metadata when the Git sync is enabled,
             // otherwise we can optimize performance by not fetching it
             metadata: !!space.gitSync,
-        })
+        }),
+
+        // When trying to render a revision with an invalid / non-existing ID,
+        // we should handle gracefully the 404 and throw notFound.
+        ids.revision ? [404] : undefined
     );
+    if (!pages) {
+        notFound();
+    }
 
     return {
         ...baseContext,

packages/gitbook-v2/src/lib/data/api.ts

@@ -166,12 +166,9 @@ export function createDataFetcher(
                 })
             );
         },
-        //
-        // API that are not tied to the token
-        // where the data is the same for all users
-        //
+
         getUserById(userId) {
-            return trace('getUserById', () => getUserById({ apiToken: null }, { userId }));
+            return trace('getUserById', () => getUserById(input, { userId }));
         },
     };
 }

packages/gitbook/e2e/internal.spec.ts

@@ -28,6 +28,7 @@ import {
     headerLinks,
     runTestCases,
     waitForCookiesDialog,
+    waitForNotFound,
 } from './util';
 
 const testCases: TestsCase[] = [
@@ -308,6 +309,18 @@ const testCases: TestsCase[] = [
                 url: '~/revisions/S55pwsEr5UVoroaOiWnP/blocks/headings',
                 run: waitForCookiesDialog,
             },
+            {
+                name: 'Invalid revision',
+                url: '~/revisions/idnotfound/blocks/headings',
+                run: waitForNotFound,
+                screenshot: false,
+            },
+            {
+                name: 'Invalid change request',
+                url: '~/changes/idnotfound/blocks/headings',
+                run: waitForNotFound,
+                screenshot: false,
+            },
         ],
     },
     {

packages/gitbook/e2e/util.ts

@@ -16,7 +16,7 @@ import {
     type CustomizationThemedColor,
     type SiteCustomizationSettings,
 } from '@gitbook/api';
-import { type BrowserContext, type Page, expect, test } from '@playwright/test';
+import { type BrowserContext, type Page, type Response, expect, test } from '@playwright/test';
 import deepMerge from 'deepmerge';
 import rison from 'rison';
 import type { DeepPartial } from 'ts-essentials';
@@ -33,7 +33,7 @@ export interface Test {
     /**
      * Test to run
      */
-    run?: (page: Page) => Promise<unknown>;
+    run?: (page: Page, response: Response | null) => Promise<unknown>;
     /**
      * Whether the test should be fullscreened during testing.
      */
@@ -138,6 +138,11 @@ export async function waitForCookiesDialog(page: Page) {
     await expect(dialog).toBeVisible();
 }
 
+export async function waitForNotFound(_page: Page, response: Response | null) {
+    expect(response).not.toBeNull();
+    expect(response?.status()).toBe(404);
+}
+
 /**
  * Transform test cases into Playwright tests and run it.
  */
@@ -183,9 +188,9 @@ export function runTestCases(testCases: TestsCase[]) {
                         }
                     });
 
-                    await page.goto(url);
+                    const response = await page.goto(url);
                     if (testEntry.run) {
-                        await testEntry.run(page);
+                        await testEntry.run(page, response);
                     }
                     const screenshotOptions = testEntry.screenshot;
                     if (screenshotOptions !== false) {