Render service, from #777's review: RENDER_FILES checked, contained and keyed

tools/render/README.md

@@ -273,7 +273,17 @@ docker run ... -v /path/to/release-files:/files:ro -e RENDER_FILES=/files websit
 
 A file missing from the folder is not reported as such by the page: the render
 waits out `RENDER_TIMEOUT`. So check the folder holds every file before a bulk
-run. `/health` says which source is in use (`files`).
+run. The service refuses to start if `RENDER_FILES` is not a directory, and
+`/health` says which source is in use (`files`). A link inside the folder is
+followed only if it stays inside it.
+
+Only requests to the download site are answered from the folder; a deployment
+whose profile takes its assets from the site's own host (`assetsFromHost`,
+`downloadBase`) is not redirected. The page also asks the download site for
+icons and structure films, which the render page does not draw; from a folder
+without them they are 404s. Figures cached from the download site and from a
+folder have different cache keys, so switching `RENDER_FILES` on or off never
+serves one for the other.
 
 Query parameters: `token`, `scale`, `subpathways=false`, and for GIF `delay`
 (ms per frame) and `maxSize` (longest side). All of them are part of the cache

tools/render/local-files.spec.ts

@@ -1,4 +1,4 @@
-import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises';
+import { mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises';
 import { tmpdir } from 'node:os';
 import path from 'node:path';
 import { afterAll, beforeAll, describe, expect, it } from 'vitest';
@@ -12,6 +12,11 @@ beforeAll(async () => {
   await writeFile(path.join(root, '98', 'diagram', 'R-HSA-1.json'), '{"nodes":[]}');
   await writeFile(path.join(root, '98', 'ehld', 'R-HSA-2.svg'), '<svg/>');
   await writeFile(path.join(root, 'outside.txt'), 'not under 98');
+  // A sibling whose name starts with the folder's: `98x` is not inside `98`.
+  await mkdir(path.join(root, '98x'));
+  await writeFile(path.join(root, '98x', 'f.txt'), 'a sibling');
+  // A link inside the folder to a file outside it.
+  await symlink(path.join(root, 'outside.txt'), path.join(root, '98', 'link.txt'));
 });
 afterAll(() => rm(root, { recursive: true, force: true }));
 
@@ -47,6 +52,8 @@ describe('the download site from a local folder', () => {
       // An encoded slash survives the URL's own normalising and decodes to a
       // step out of the folder, to a file that exists.
       '/%2e%2e%2foutside.txt',
+      '/%2e%2e%2f98x%2ff.txt',
+      '/link.txt',
       '/%E0%A4%A',
     ]) {
       expect((await localAnswer(path.join(root, '98'), at(escape))).status, escape).toBe(404);

tools/render/local-files.ts

@@ -9,9 +9,10 @@
  * until the release is published. So when a folder is given, every request the
  * page makes to the download site is answered from it, laid out as the site is
  * (`<folder>/98/diagram/R-HSA-69620.json`), and a file the folder lacks is a
- * 404 -- never a request to the site.
+ * 404 -- never a request to the site. A link inside the folder is followed only
+ * if it stays inside it.
  */
-import { readFile } from 'node:fs/promises';
+import { readFile, realpath } from 'node:fs/promises';
 import path from 'node:path';
 
 /** The download site, as the site's profiles name it (environments.ts, `S3`). */
@@ -37,7 +38,12 @@ export async function localAnswer(
   url: URL
 ): Promise<{ status: number; contentType: string; body: Buffer | string }> {
   const missing = { status: 404, contentType: 'text/plain', body: 'not in the local files' };
-  const base = path.resolve(root);
+  let base: string;
+  try {
+    base = await realpath(root);
+  } catch {
+    return missing;
+  }
   let relative: string;
   try {
     relative = decodeURIComponent(url.pathname);
@@ -47,6 +53,8 @@ export async function localAnswer(
   const file = path.resolve(base, `.${relative}`);
   if (!file.startsWith(base + path.sep)) return missing;
   try {
+    // Where the file really is: a link out of the folder is no file of it.
+    if (!(await realpath(file)).startsWith(base + path.sep)) return missing;
     const body = await readFile(file);
     const contentType = TYPES[path.extname(file).toLowerCase()] ?? 'application/octet-stream';
     return { status: 200, contentType, body };

tools/render/render-core.spec.ts

@@ -1,4 +1,7 @@
-import type { Page } from '@playwright/test';
+import type { Page, Route } from '@playwright/test';
+import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises';
+import { tmpdir } from 'node:os';
+import path from 'node:path';
 import { describe, expect, it } from 'vitest';
 import { render } from './render-core.ts';
 
@@ -79,38 +82,67 @@ describe('a figure of another species', () => {
 describe("the release's files", () => {
   // A stand-in page that records its routes and stops at the first navigation.
   function routed() {
-    const routes: { matcher: unknown; handler: unknown }[] = [];
+    const routes: { matcher: (url: URL) => boolean; handler: (route: Route) => Promise<void> }[] =
+      [];
+    let seen: typeof routes = [];
     const page = {
       on() {},
       off() {},
-      async route(matcher: unknown, handler: unknown) {
+      async route(matcher: (url: URL) => boolean, handler: (route: Route) => Promise<void>) {
         routes.push({ matcher, handler });
       },
       async unroute(matcher: unknown, handler: unknown) {
         const i = routes.findIndex((r) => r.matcher === matcher && r.handler === handler);
         if (i >= 0) routes.splice(i, 1);
       },
       async goto() {
-        seen = routes.length;
+        seen = [...routes];
         throw new Error('stop here');
       },
     };
-    let seen = 0;
     return { page, routes, seen: () => seen };
   }
 
   it('are read from the folder while the page draws, and the route removed after', async () => {
-    const { page, routes, seen } = routed();
-    await render(stand(page), { base: 'http://site', pathway: 'R-HSA-1', files: '/files' }).catch(
-      () => undefined
-    );
-    expect(seen()).toBe(1);
-    expect(routes).toHaveLength(0);
+    const root = await mkdtemp(path.join(tmpdir(), 'render-core-files-'));
+    try {
+      await mkdir(path.join(root, '98', 'diagram'), { recursive: true });
+      await writeFile(path.join(root, '98', 'diagram', 'R-HSA-1.json'), '{"x":1}');
+      const { page, routes, seen } = routed();
+      await render(stand(page), { base: 'http://site', pathway: 'R-HSA-1', files: root }).catch(
+        () => undefined
+      );
+      const [only] = seen();
+      expect(seen()).toHaveLength(1);
+      expect(only?.matcher(new URL('https://download.reactome.org/98/diagram/R-HSA-1.json'))).toBe(
+        true
+      );
+      expect(only?.matcher(new URL('http://site/ContentService/data/database/version'))).toBe(
+        false
+      );
+      const fulfilled: unknown[] = [];
+      const route = (url: string) =>
+        ({
+          request: () => ({ url: () => url }),
+          fulfill: async (answer: unknown) => void fulfilled.push(answer),
+          continue: async () => void fulfilled.push('continued'),
+        }) as unknown as Route;
+      await only?.handler(route('https://download.reactome.org/98/diagram/R-HSA-1.json'));
+      await only?.handler(route('https://download.reactome.org/98/diagram/R-HSA-2.json'));
+      expect(fulfilled).toMatchObject([
+        { status: 200, contentType: 'application/json' },
+        { status: 404 },
+      ]);
+      expect(String((fulfilled[0] as { body: Buffer }).body)).toBe('{"x":1}');
+      expect(routes).toHaveLength(0);
+    } finally {
+      await rm(root, { recursive: true, force: true });
+    }
   });
 
   it('are left to the download site when no folder is given', async () => {
     const { page, seen } = routed();
     await render(stand(page), { base: 'http://site', pathway: 'R-HSA-1' }).catch(() => undefined);
-    expect(seen()).toBe(0);
+    expect(seen()).toHaveLength(0);
   });
 });

tools/render/render-core.ts

@@ -186,12 +186,12 @@ export async function render(
   };
   page.on('pageerror', onPageError);
   page.on('console', onConsole);
-  const fromFiles = async (route: Route) => {
-    if (!files) return route.continue();
-    const { status, contentType, body } = await localAnswer(files, new URL(route.request().url()));
+  const fromFiles = async (route: Route, root: string) => {
+    const { status, contentType, body } = await localAnswer(root, new URL(route.request().url()));
     return route.fulfill({ status, contentType, body });
   };
-  if (files) await page.route(isDownload, fromFiles);
+  const handler = files ? (route: Route) => fromFiles(route, files) : undefined;
+  if (handler) await page.route(isDownload, handler);
 
   try {
     await page.goto(
@@ -270,7 +270,7 @@ export async function render(
   } finally {
     page.off('pageerror', onPageError);
     page.off('console', onConsole);
-    if (files) await page.unroute(isDownload, fromFiles);
+    if (handler) await page.unroute(isDownload, handler);
   }
 }
 

tools/render/service.ts

@@ -33,6 +33,9 @@
  *   RENDER_CONCURRENCY  simultaneous renders, default 2
  *   RENDER_QUEUE        pending renders before 503, default 8
  *   RENDER_CACHE_MAX    bytes of cache to keep, default 2 GB (0 disables)
+ *   RENDER_TIMEOUT      ms before a render is abandoned, default 45000
+ *   RENDER_FILES        a folder laid out as the download site, read instead of it
+ *                       (local-files.ts); unset, the page fetches from the site
  *
  * Query parameters: token, scale, subpathways=false, dark=true, select=<stId>,
  * highlight=<stId>,<stId>...,
@@ -54,7 +57,7 @@ import {
 } from './params.ts';
 import { chromium, type Browser } from '@playwright/test';
 import { createHash } from 'node:crypto';
-import { readFileSync, readdirSync, realpathSync } from 'node:fs';
+import { readFileSync, readdirSync, realpathSync, statSync } from 'node:fs';
 import { fileURLToPath } from 'node:url';
 import {
   mkdir,
@@ -108,6 +111,12 @@ const CACHE_KEY = process.env['RENDER_CACHE_KEY'] || 'v12';
 // for the page to read instead of the site (local-files.ts). Unset, it fetches
 // them from the site, as a reader's browser does.
 const FILES = process.env['RENDER_FILES'] || undefined;
+// A mistyped folder would otherwise fail every render after the full timeout,
+// with /health saying the files are local.
+if (FILES && !statSync(FILES, { throwIfNoEntry: false })?.isDirectory()) {
+  console.error(`RENDER_FILES is not a directory: ${FILES}`);
+  process.exit(2);
+}
 const CONCURRENCY = Number(process.env['RENDER_CONCURRENCY'] || 2);
 // Generous next to a real render, which is 3-8s, but far short of the two
 // minutes a page that never becomes ready would otherwise hold a browser for.
@@ -278,6 +287,10 @@ function cacheKey({
         highlight,
         // Only when set, so every figure cached before it keeps its key.
         ...(species ? [species] : []),
+        // Where the page's files came from: a figure drawn from the download
+        // site is not served as one drawn from a release's own files, nor the
+        // reverse. Only when set, as above.
+        ...(FILES ? ['files'] : []),
       ])
     )
     .digest('hex');