diff --git a/src/app/s/[id]/download/preparing/PreparingDownload.tsx b/src/app/s/[id]/download/preparing/PreparingDownload.tsx index 4ee4c55..cc613a6 100644 --- a/src/app/s/[id]/download/preparing/PreparingDownload.tsx +++ b/src/app/s/[id]/download/preparing/PreparingDownload.tsx @@ -5,7 +5,7 @@ import { useEffect, useState } from 'react'; import styles from './PreparingDownload.module.scss'; -import { pollOperation, type OperationProgress } from '@/lib/search/operationPoll'; +import { operationPath, pollOperation, type OperationProgress } from '@/lib/search/operationPoll'; import { artifactHref, pinDownloadedVersion } from '@/lib/search/searchClient'; import { renderedFile } from '@/lib/search/shareDownload'; import type { FileRendering } from '@/lib/search/snapshot'; @@ -36,7 +36,9 @@ const PreparingDownload = ({ const bundle = preparing?.rendering.bundle; useEffect(() => { - if (!operationId || !format || bundle === undefined) { + const pollPath = operationPath(operationId); + + if (!operationId || !pollPath || !format || bundle === undefined) { return; } @@ -48,7 +50,7 @@ const PreparingDownload = ({ { operation_id: operationId, status: 'pending', - poll: `/api/datasets/operations/${operationId}`, + poll: pollPath, }, { signal: controller.signal, diff --git a/src/lib/search/operationPoll.ts b/src/lib/search/operationPoll.ts index 52e7c57..4cc72ac 100644 --- a/src/lib/search/operationPoll.ts +++ b/src/lib/search/operationPoll.ts @@ -13,11 +13,13 @@ * * This module has no DOM/Node dependency, so `node --test` can import it * directly. It takes no base URL and reads no env: a caller supplies - * `fetchImpl`, a thin wrapper that resolves the envelope's own relative - * `poll` path against wherever it needs to go (see searchClient.ts's - * `mintDataset`, which polls through the site's own same-origin proxy, - * and snapshot.ts's `captureSnapshot`, which polls the Datasets API directly, since - * a done poll needs no principal header, unlike the `pin` call next to it). + * `fetchImpl`, a thin wrapper that resolves the relative `poll` path + * against wherever it needs to go (see searchClient.ts's `mintDataset`, + * which polls through the site's own same-origin proxy). A server-side + * caller that polls the Datasets API directly (snapshot.ts's + * `captureSnapshot`, shareDownload.ts's `awaitOperation`) ignores that path + * and fetches `operationPath` of the checked operation id on every poll, + * because a path a response names is untrusted. */ import type { @@ -32,6 +34,16 @@ import { datasetsFetch } from './datasetsFetch.ts'; export type { OperationEnvelope, OperationProgress }; +/** The shape of an operation id dfo-apis mints. */ +export const OPERATION_SHAPE = /^op_[a-f0-9]+$/; + +/** The Datasets API path that reads operation `id`, or undefined when `id` + * is not an operation id. */ +export const operationPath = (id: unknown): string | undefined => + typeof id === 'string' && OPERATION_SHAPE.test(id) + ? `/api/datasets/operations/${id}` + : undefined; + export type PollResponse = { ok: boolean; status: number; json: () => Promise }; export type PollFetch = (url: string, signal?: AbortSignal) => Promise; diff --git a/src/lib/search/shareDownload.ts b/src/lib/search/shareDownload.ts index d09534b..c9174c3 100644 --- a/src/lib/search/shareDownload.ts +++ b/src/lib/search/shareDownload.ts @@ -34,7 +34,12 @@ import type { OperationResource, ProblemDetails } from '@developmentfinance/clie import { datasetsFetch } from './datasetsFetch.ts'; import { DEFAULT_DOWNLOAD_CHOICES } from './downloadOptions.ts'; -import { OPERATION_DEADLINE, pollOperation } from './operationPoll.ts'; +import { + OPERATION_DEADLINE, + OPERATION_SHAPE, + operationPath, + pollOperation, +} from './operationPoll.ts'; import { artifactHref, DATASET_FORMATS } from './searchClient.ts'; import type { SearchShare } from './searchShare'; import { MINT_DEFAULT_RENDERING, renderingArtifact, type FileRendering } from './snapshot.ts'; @@ -60,7 +65,6 @@ type Query = { get(name: string): string | null; has(name: string): boolean }; const FORMAT_IDS: readonly string[] = DATASET_FORMATS.map((format) => format.id); const PART_SHAPE = /^\d+$/; -export const OPERATION_SHAPE = /^op_[a-f0-9]+$/; const refused = (status: 400 | 404, message: string): ShareDownload => ({ kind: 'refused', @@ -421,8 +425,9 @@ export const serveLiveDownload = async ({ if (res.status === 202) { const operation = (body as Partial | null)?.operation; + const pollPath = operationPath(operation?.operation_id); - if (operation && OPERATION_SHAPE.test(operation.operation_id)) { + if (operation && pollPath) { return html ? { kind: 'redirect', @@ -435,6 +440,7 @@ export const serveLiveDownload = async ({ } : await awaitOperation({ operation, + pollPath, target, base, pin, @@ -473,6 +479,7 @@ export const serveLiveDownload = async ({ * file of the rendering asked for. */ const awaitOperation = async ({ operation, + pollPath, target, base, pin, @@ -484,6 +491,8 @@ const awaitOperation = async ({ log, }: { operation: OperationResource['operation']; + /** `operationPath` of the operation's checked id. */ + pollPath: string; target: Extract; base: string; pin: PinServed; @@ -496,7 +505,7 @@ const awaitOperation = async ({ }): Promise => { // Every poll reads the operation by its checked id. The poll path a // response names is ignored. - const pollUrl = `${base}/api/datasets/operations/${operation.operation_id}`; + const pollUrl = `${base}${pollPath}`; let manifest: VersionLinkDocument | null; try { diff --git a/src/lib/search/snapshot.ts b/src/lib/search/snapshot.ts index 79cba93..88c41e3 100644 --- a/src/lib/search/snapshot.ts +++ b/src/lib/search/snapshot.ts @@ -22,7 +22,7 @@ import type { } from '@developmentfinance/client/wire'; import { datasetsFetch } from './datasetsFetch.ts'; -import { pollOperation } from './operationPoll.ts'; +import { operationPath, pollOperation } from './operationPoll.ts'; import { SPEC_SHAPE, VERSION_SHAPE } from './versionLink.ts'; /** The fields of a Datasets manifest this module reads. */ @@ -206,19 +206,33 @@ export const captureSnapshot = async ({ } // An over-budget mint answers 202 with an operation envelope; a done - // poll needs no principal. - const manifest = ( - minted.status === 202 - ? await pollOperation(minted.body.operation, { - fetchImpl: (url) => - fetchImpl(`${base}${url}`, { - cache: 'no-store', - signal: AbortSignal.timeout(DOOR_TIMEOUT_MS), - }), - deadlineMs: pollDeadlineMs, - }) - : minted.body - ) as (Partial & Partial>) | null; + // poll needs no principal. Every poll reads the operation by its + // checked id, and the poll path a response names is ignored. + let manifest = minted.body as + (Partial & Partial>) | null; + + if (minted.status === 202) { + const operation = minted.body?.operation; + const pollPath = operationPath(operation?.operation_id); + + if (!pollPath) { + throw new Error(`mint ${minted.status}`, { + cause: new Error('the mint named no valid operation id'), + }); + } + + manifest = await pollOperation( + { ...operation, poll: pollPath }, + { + fetchImpl: () => + fetchImpl(`${base}${pollPath}`, { + cache: 'no-store', + signal: AbortSignal.timeout(DOOR_TIMEOUT_MS), + }), + deadlineMs: pollDeadlineMs, + }, + ); + } if (typeof manifest?.content_version !== 'string') { throw new Error(manifest?.code ?? `mint ${minted.status}`); diff --git a/src/lib/search/test/snapshot.test.ts b/src/lib/search/test/snapshot.test.ts index 00bd91f..c5df01a 100644 --- a/src/lib/search/test/snapshot.test.ts +++ b/src/lib/search/test/snapshot.test.ts @@ -160,6 +160,92 @@ test('captureSnapshot fails before minting when no principal is configured', asy assert.equal(calls.length, 0); }); +// A Datasets API double whose mint answers 202 for `operationId` and whose +// every envelope names a hostile poll path: another surface, a `..` +// segment, then an absolute URL. The operation's own path answers pending +// twice, then done. Records every URL requested. +const asyncMint = (operationId: unknown) => { + const hostile = [ + '/api/graph/x', + '/api/datasets/operations/../../presentation/shares/x', + 'https://evil.example/api/datasets/operations/op_1f', + ]; + const urls: string[] = []; + let polls = 0; + const fetchImpl = (async (url: string) => { + urls.push(url); + const answer = (status: number, body: unknown) => + ({ ok: status < 300, status, json: async () => body }) as Response; + const envelope = (poll: string) => ({ + operation_id: operationId, + status: 'pending', + poll, + retry_after_ms: 0, + }); + + if (url === 'https://api/api/datasets') { + return answer(202, { operation: envelope(hostile[0]!) }); + } + + if (url === 'https://api/api/datasets/operations/op_1f') { + polls += 1; + + return polls < hostile.length + ? answer(200, { operation: envelope(hostile[polls]!) }) + : answer(200, { + operation: { operation_id: operationId, status: 'done' }, + manifest: { + content_version: 'cv_abc', + definition: { spec_id: 'spec_crs' }, + artifacts, + }, + }); + } + + return url.endsWith('/pin') ? answer(200, {}) : answer(404, { code: 'not_found' }); + }) as typeof fetch; + + return { urls, fetchImpl }; +}; + +// Slow (about 750 ms), because pollOperation floors each retry_after_ms at 250 ms. +test('captureSnapshot polls the operation by its id and never the poll path a response names', async () => { + const { urls, fetchImpl } = asyncMint('op_1f'); + + assert.equal( + ( + await captureSnapshot({ + base: 'https://api', + specId: 'spec_crs', + definition: { spec_id: 'spec_crs' }, + principal: 'site', + pollDeadlineMs: 5_000, + fetchImpl, + }) + ).artifactDigest, + 'sha256:csv', + ); + assert.deepEqual(urls, [ + 'https://api/api/datasets', + ...Array(3).fill('https://api/api/datasets/operations/op_1f'), + 'https://api/api/datasets/spec_crs/cv_abc/pin', + ]); +}); + +for (const [name, operationId] of [ + ['a path in the id', 'op_1f/../../graph'], + ['upper-case hex', 'OP_1F'], + ['a number', 42], + ['no id', undefined], +] as const) { + test(`captureSnapshot refuses an operation with ${name} before any poll`, async () => { + const { urls, fetchImpl } = asyncMint(operationId); + + await assert.rejects(capture(fetchImpl, 'site'), { message: 'mint 202' }); + assert.deepEqual(urls, ['https://api/api/datasets']); + }); +} + const part = (specId: string, status = 'definite') => ({ status, ...(status === 'definite' && { spec: { spec_id: specId } }),