From 97033116a1a89f117187fef6b48b9271a8bd4809 Mon Sep 17 00:00:00 2001 From: Josh de Leeuw Date: Tue, 22 Sep 2026 08:30:08 -0400 Subject: [PATCH 1/5] Point the test site's code samples at the test site Both deploy workflows set NEXT_PUBLIC_BASE_URL, but nothing read it, so samples copied from datapipe-test.web.app fell through to the packages' built-in pipe.jspsych.org and wrote to production. The API reference printed the production origin too. lib/base-url.js now reads it, with production as the fallback. The API reference uses it, and on any non-production build every sample passes the deployment explicitly (base_url for the extension, baseURL for datapipe-client). Production samples are unchanged. Fixes #246 Co-Authored-By: Claude Opus 5 --- __tests__/base-url.test.js | 39 +++++++++++++++++++++++ __tests__/extension-snippet.test.js | 7 ++++ components/dashboard/CodeHints.js | 24 +++++++++----- components/dashboard/extension-snippet.js | 9 ++++-- components/docs/ApiPrimitives.js | 5 ++- lib/base-url.js | 35 ++++++++++++++++++++ 6 files changed, 108 insertions(+), 11 deletions(-) create mode 100644 __tests__/base-url.test.js create mode 100644 lib/base-url.js diff --git a/__tests__/base-url.test.js b/__tests__/base-url.test.js new file mode 100644 index 0000000..f91adf6 --- /dev/null +++ b/__tests__/base-url.test.js @@ -0,0 +1,39 @@ +/** + * @jest-environment node + * + * The deployment URL shown in the API reference and threaded into the code + * samples (#246). The test site used to hand out samples that wrote to + * production, because nothing read NEXT_PUBLIC_BASE_URL. + */ + +import { + PRODUCTION_BASE_URL, + resolveBaseURL, + sampleBaseURL, +} from "../lib/base-url"; + +describe("resolveBaseURL", () => { + it("uses the configured deployment", () => { + expect(resolveBaseURL("https://datapipe-test.web.app")).toBe("https://datapipe-test.web.app"); + }); + + it("drops trailing slashes so `${base}/api/...` never doubles one", () => { + expect(resolveBaseURL("https://datapipe-test.web.app//")).toBe("https://datapipe-test.web.app"); + }); + + it("falls back to production when unset or blank", () => { + expect(resolveBaseURL(undefined)).toBe(PRODUCTION_BASE_URL); + expect(resolveBaseURL("")).toBe(PRODUCTION_BASE_URL); + expect(resolveBaseURL(" / ")).toBe(PRODUCTION_BASE_URL); + }); +}); + +describe("sampleBaseURL", () => { + it("adds no override on production, where the packages' default is right", () => { + expect(sampleBaseURL(PRODUCTION_BASE_URL)).toBeNull(); + }); + + it("names every other deployment explicitly", () => { + expect(sampleBaseURL("https://datapipe-test.web.app")).toBe("https://datapipe-test.web.app"); + }); +}); diff --git a/__tests__/extension-snippet.test.js b/__tests__/extension-snippet.test.js index fc4ddf8..61cff14 100644 --- a/__tests__/extension-snippet.test.js +++ b/__tests__/extension-snippet.test.js @@ -71,6 +71,13 @@ describe("the jsPsych code sample", () => { expect(calls.run).toHaveLength(1); }); + it("passes base_url to the extension when the build is not production", async () => { + const { calls } = await execute(extensionSnippet("EXP123", "https://datapipe-test.web.app")); + expect(calls.initialize).toEqual([ + { experiment_id: "EXP123", filename: "p42.csv", base_url: "https://datapipe-test.web.app" }, + ]); + }); + it("appends no save trial: the extension owns the submission", async () => { const { calls } = await execute(code); expect(calls.run[0]).toHaveLength(0); diff --git a/components/dashboard/CodeHints.js b/components/dashboard/CodeHints.js index ed0ad20..b7bd3cf 100644 --- a/components/dashboard/CodeHints.js +++ b/components/dashboard/CodeHints.js @@ -13,6 +13,14 @@ import { ChevronDown } from "lucide-react"; import CodeBlock from "../CodeBlock"; import { extensionSnippet } from "./extension-snippet"; import { EXTENSION_PIPE_SCRIPT, DATAPIPE_CLIENT_SCRIPT } from "./script-tags"; +import { SAMPLE_BASE_URL } from "../../lib/base-url"; + +// Off production, every sample names this build's deployment. The published +// packages default to pipe.jspsych.org, so a sample copied from the test site +// without an override sends its data to production (#246). +const EXTENSION_OPTIONS = SAMPLE_BASE_URL ? `, { base_url: "${SAMPLE_BASE_URL}" }` : ""; +const CLIENT_OPTION = SAMPLE_BASE_URL ? `\n baseURL: "${SAMPLE_BASE_URL}",` : ""; +const CLIENT_INLINE_OPTION = SAMPLE_BASE_URL ? `, baseURL: "${SAMPLE_BASE_URL}"` : ""; export default function CodeHints({ expId }) { const [language, setLanguage] = useState("jsPsych v8"); @@ -92,7 +100,7 @@ export default function CodeHints({ expId }) { {EXTENSION_PIPE_SCRIPT} - {extensionSnippet(expId)} + {extensionSnippet(expId, SAMPLE_BASE_URL)} Each trial is sent as it happens, so a participant who closes the tab partway through does not take all of their data with them: their completed trials arrive as a separate file ending in .partial.json, and do not count toward your session limit. A participant who finishes produces one ordinary file. @@ -117,7 +125,7 @@ export default function CodeHints({ expId }) { recording_duration: 15000, on_finish: async function(data){ const filename = \`\${subject_id}_\${jsPsych.getProgress().current_trial_global}_audio.webm\`; - await jsPsychExtensionPipe.saveBase64Data("${expId}", filename, data.response); + await jsPsychExtensionPipe.saveBase64Data("${expId}", filename, data.response${EXTENSION_OPTIONS}); data.response = filename; } };`} @@ -140,7 +148,7 @@ export default function CodeHints({ expId }) { async function createExperiment(){ let condition; try { - condition = await jsPsychExtensionPipe.getCondition("${expId}"); + condition = await jsPsychExtensionPipe.getCondition("${expId}"${EXTENSION_OPTIONS}); } catch (error) { document.body.innerHTML = "

The experiment could not be started.

"; throw error; @@ -181,7 +189,7 @@ export default function CodeHints({ expId }) { const result = await DataPipe.saveData({ experiment_id: "${expId}", filename: "UNIQUE_FILENAME.csv", - data: dataAsString, + data: dataAsString,${CLIENT_OPTION} }); if (!result.ok) { @@ -209,7 +217,7 @@ export default function CodeHints({ expId }) { const filename = "UNIQUE_FILENAME.csv"; const session = DataPipe.createSession({ experiment_id: "${expId}", - filename: filename, + filename: filename,${CLIENT_OPTION} }); // ...after each trial: @@ -220,7 +228,7 @@ export default function CodeHints({ expId }) { experiment_id: "${expId}", filename: filename, data: dataAsString, - session: session, + session: session,${CLIENT_OPTION} }); await session.close({ submitted: result.ok });`} @@ -242,7 +250,7 @@ export default function CodeHints({ expId }) { const result = await DataPipe.saveBase64Data({ experiment_id: "${expId}", filename: "UNIQUE_FILENAME.webm", - data: base64DataString, + data: base64DataString,${CLIENT_OPTION} });`} @@ -259,7 +267,7 @@ export default function CodeHints({ expId }) { {` let condition; try { - condition = await DataPipe.getCondition({ experiment_id: "${expId}" }); + condition = await DataPipe.getCondition({ experiment_id: "${expId}"${CLIENT_INLINE_OPTION} }); } catch (error) { document.body.innerHTML = "

The experiment could not be started.

"; throw error; diff --git a/components/dashboard/extension-snippet.js b/components/dashboard/extension-snippet.js index 0f02d13..1a69d67 100644 --- a/components/dashboard/extension-snippet.js +++ b/components/dashboard/extension-snippet.js @@ -17,16 +17,21 @@ // which time the declaration has run. The test covers this, because a reader // copying the sample is likely to "simplify" it back into a string. // +// `baseURL` is set only on builds that are not production (lib/base-url.js), +// where the extension's built-in default would send the data to the wrong +// deployment. +// // Flush-left on purpose: CodeBlock strips the first line's indentation from // every line, and there is none here to strip. -export function extensionSnippet(expId) { +export function extensionSnippet(expId, baseURL = null) { + const baseURLParam = baseURL ? `,\n base_url: "${baseURL}"` : ""; return `const jsPsych = initJsPsych({ extensions: [ { type: jsPsychExtensionPipe, params: { experiment_id: "${expId}", - filename: () => \`\${subject_id}.csv\` + filename: () => \`\${subject_id}.csv\`${baseURLParam} } } ] diff --git a/components/docs/ApiPrimitives.js b/components/docs/ApiPrimitives.js index f2c2bae..649e8ca 100644 --- a/components/docs/ApiPrimitives.js +++ b/components/docs/ApiPrimitives.js @@ -1,4 +1,5 @@ import { Stack, Heading, Text, Table, Badge, Code } from "@chakra-ui/react"; +import { BASE_URL } from "../../lib/base-url"; /** * ApiPrimitives @@ -12,8 +13,10 @@ import { Stack, Heading, Text, Table, Badge, Code } from "@chakra-ui/react"; * * Package D (docs IA plan §4) reuses these unchanged for /docs/api's moved * API reference content. + * + * BASE_URL is the deployment this build serves (lib/base-url.js), so the test + * site's reference names the test site rather than production. */ -export const BASE_URL = "https://pipe.jspsych.org"; export function EndpointHeading({ method, path, children }) { return ( diff --git a/lib/base-url.js b/lib/base-url.js new file mode 100644 index 0000000..0640bb2 --- /dev/null +++ b/lib/base-url.js @@ -0,0 +1,35 @@ +// The DataPipe deployment this build serves, for the URLs shown in the API +// reference and threaded into the copy-paste code samples. +// +// Both deploy workflows set NEXT_PUBLIC_BASE_URL (firebase-deploy.yml -> +// https://pipe.jspsych.org, firebase-deploy-test.yml -> +// https://datapipe-test.web.app). Before this module nothing read it, so the +// test deployment handed testers samples that fell through to +// datapipe-client's built-in production URL: copy one, run it, and the data +// landed in the live service, against real researchers' experiments. +// +// Next.js inlines NEXT_PUBLIC_* at build time, so this must stay a literal +// `process.env.NEXT_PUBLIC_BASE_URL` reference (no dynamic lookup), and the +// value is fixed per build, not per request. The production URL is the +// fallback: an unconfigured build (a self-host that never set the var) is +// describing the hosted service. + +export const PRODUCTION_BASE_URL = "https://pipe.jspsych.org"; + +export function resolveBaseURL(configured) { + let url = (configured || "").trim(); + while (url.endsWith("/")) url = url.slice(0, -1); + return url || PRODUCTION_BASE_URL; +} + +export const BASE_URL = resolveBaseURL(process.env.NEXT_PUBLIC_BASE_URL); + +// The URL the code samples must pass explicitly, or null when the published +// packages' own default already points at this deployment. Production samples +// stay free of an option researchers would only copy around; every other +// build names its deployment, because the npm packages cannot know it. +export function sampleBaseURL(baseURL) { + return baseURL === PRODUCTION_BASE_URL ? null : baseURL; +} + +export const SAMPLE_BASE_URL = sampleBaseURL(BASE_URL); From f2f5f2f0f38f82d42a81c5efe5db5548f45ca683 Mon Sep 17 00:00:00 2001 From: Josh de Leeuw Date: Fri, 25 Sep 2026 12:06:56 -0400 Subject: [PATCH 2/5] Return clear errors instead of crashes on three production failure paths - Reject experiment IDs Firestore can't store (e.g. an unfilled "__DATAPIPE_STUDY1_ID__" placeholder) with the normal not-found response. Firestore throws on these instead of missing, so /api/data answered with a 500. - Log the provider error when createexperiment returns a 502, so the failure is visible in Cloud Logging and not only in the browser. - Check the HTTP status in OSF listFiles. An OSF error response crashed with "Cannot read properties of undefined (reading 'filter')", which hid OSF's real status and failed uploads permanently through the collision cache. Co-Authored-By: Claude Opus 5.5 --- .../experiment-id-validation-emulator.test.js | 74 +++++++++++++++++++ .../experiment-id-validation.test.js | 68 +++++++++++++++++ functions/src/__tests__/providers-osf.test.js | 44 +++++++++++ functions/src/api-base64.ts | 8 ++ functions/src/api-condition.ts | 8 ++ functions/src/api-data.ts | 8 ++ functions/src/api-finalize.ts | 7 ++ functions/src/api-queue-status.ts | 7 ++ functions/src/api-session-start.ts | 8 ++ functions/src/create-experiment.ts | 3 + functions/src/experiment-id.ts | 42 +++++++++++ functions/src/providers/osf.ts | 15 +++- 12 files changed, 290 insertions(+), 2 deletions(-) create mode 100644 functions/src/__tests__/experiment-id-validation-emulator.test.js create mode 100644 functions/src/__tests__/experiment-id-validation.test.js create mode 100644 functions/src/experiment-id.ts diff --git a/functions/src/__tests__/experiment-id-validation-emulator.test.js b/functions/src/__tests__/experiment-id-validation-emulator.test.js new file mode 100644 index 0000000..6fbd0df --- /dev/null +++ b/functions/src/__tests__/experiment-id-validation-emulator.test.js @@ -0,0 +1,74 @@ +/** + * @jest-environment node + * + * Regression coverage for the reserved/invalid-experimentID 500 bug. + * + * Production logs showed a participant site POSTing + * experimentID: "__DATAPIPE_STUDY1_ID__" -- an unfilled template placeholder + * -- to /api/data. db.collection("experiments").doc(experimentID).get() + * throws "3 INVALID_ARGUMENT: Resource id ... is invalid because it is + * reserved" for that shape, and the throw escaped as an unhandled 500 instead + * of the ordinary 400 EXPERIMENT_NOT_FOUND a nonexistent-but-valid id already + * gets. + * + * isValidExperimentId() (experiment-id.ts) now gates every public, + * unauthenticated endpoint that looks an experiment up by a client-supplied + * id before the Firestore call that would otherwise throw. This suite proves + * the three data-submission endpoints that dispatch straight from a request + * body (api/data, api/base64, api/condition) answer with the normal + * not-found response instead of a 500. + */ + +import MESSAGES from "../api-messages"; +import { fnUrl } from "./helpers/fn-url.js"; + +process.env.FIRESTORE_EMULATOR_HOST = "localhost:8080"; + +// Exactly the placeholder observed in production logs -- also exactly +// Firestore's reserved /^__.*__$/ shape. +const RESERVED_ID = "__DATAPIPE_STUDY1_ID__"; + +async function postJSON(url, body) { + const response = await fetch(url, { + method: "POST", + headers: { "Content-Type": "application/json", Accept: "*/*" }, + body: JSON.stringify(body), + }); + const message = await response.json(); + return { status: response.status, body: message }; +} + +jest.setTimeout(30000); + +describe("reserved/invalid experimentID does not 500", () => { + it("POST /api/data returns 400 EXPERIMENT_NOT_FOUND, not a 500", async () => { + const { status, body } = await postJSON(fnUrl("/api/data"), { + experimentID: RESERVED_ID, + filename: "data.csv", + data: "trial_type\nhtml-keyboard-response\n", + }); + + expect(status).toBe(400); + expect(body).toEqual(MESSAGES.EXPERIMENT_NOT_FOUND); + }); + + it("POST /api/base64 returns 400 EXPERIMENT_NOT_FOUND, not a 500", async () => { + const { status, body } = await postJSON(fnUrl("/api/base64"), { + experimentID: RESERVED_ID, + filename: "image.png", + data: "data:image/png;base64,aGVsbG8=", + }); + + expect(status).toBe(400); + expect(body).toEqual(MESSAGES.EXPERIMENT_NOT_FOUND); + }); + + it("POST /api/condition returns 400 EXPERIMENT_NOT_FOUND, not a 500", async () => { + const { status, body } = await postJSON(fnUrl("/api/condition"), { + experimentID: RESERVED_ID, + }); + + expect(status).toBe(400); + expect(body).toEqual(MESSAGES.EXPERIMENT_NOT_FOUND); + }); +}); diff --git a/functions/src/__tests__/experiment-id-validation.test.js b/functions/src/__tests__/experiment-id-validation.test.js new file mode 100644 index 0000000..60c30c6 --- /dev/null +++ b/functions/src/__tests__/experiment-id-validation.test.js @@ -0,0 +1,68 @@ +/** + * @jest-environment node + * + * isValidExperimentId (experiment-id.ts) -- the gate between a client- + * supplied experimentID and db.collection("experiments").doc(experimentID). + * + * The bug this guards against: Firestore does not treat an invalid document + * id as a lookup miss, it THROWS synchronously out of doc()/get() for a + * handful of specific shapes -- most commonly a participant site that never + * filled in a template placeholder, e.g. "__DATAPIPE_STUDY1_ID__", which + * matches Firestore's own reserved __...__ pattern. That throw was escaping + * every public endpoint that looked an experiment up by id as an unhandled + * 500, instead of the ordinary 400 EXPERIMENT_NOT_FOUND a nonexistent id + * already gets. + * + * Unlike isValidSessionId (staging.ts), this is deliberately NOT pinned to + * the nanoid alphabet create-experiment.ts mints new ids from -- older + * experiment ids may use other formats, so this only has to reject what + * Firestore itself would reject. + */ + +const { isValidExperimentId } = require("../../lib/experiment-id.js"); + +describe("isValidExperimentId", () => { + it.each([ + ["a 12-char nanoid-style id, as create-experiment.ts mints", "aB3xY9kLm2Qz"], + ["an id with mixed alphanumeric, hyphen and underscore characters", "abc-DEF_123"], + ])("accepts %s", (_label, value) => { + expect(isValidExperimentId(value)).toBe(true); + }); + + it.each([ + ["the empty string", ""], + ["a reserved __...__ id (the unfilled-template-placeholder case)", "__DATAPIPE_STUDY1_ID__"], + ["a reserved __...__ id with nothing in between", "____"], + ["an id containing a forward slash", "abc/def"], + ["a leading slash", "/abc"], + ["exactly a single period", "."], + ["exactly a double period", ".."], + ["longer than 1500 bytes in UTF-8", "a".repeat(1501)], + // A multi-byte character pushes this over 1500 BYTES despite being under + // 1500 JS string characters -- proves the check is byte-length, not + // string-length. + ["over 1500 bytes via multi-byte characters, under 1500 JS characters", "é".repeat(751)], + ])("rejects %s", (_label, value) => { + expect(isValidExperimentId(value)).toBe(false); + }); + + it.each([ + ["null", null], + ["undefined", undefined], + ["a number", 123], + ["a plain object", {}], + ["an array", ["a"]], + ["a boolean", true], + ])("rejects %s (not a string)", (_label, value) => { + expect(isValidExperimentId(value)).toBe(false); + }); + + it("accepts an id exactly at the 1500-byte boundary", () => { + expect(isValidExperimentId("a".repeat(1500))).toBe(true); + }); + + it("accepts a period or double period as part of a longer id, not standing alone", () => { + expect(isValidExperimentId("a.b")).toBe(true); + expect(isValidExperimentId("a..b")).toBe(true); + }); +}); diff --git a/functions/src/__tests__/providers-osf.test.js b/functions/src/__tests__/providers-osf.test.js index 0f21296..2928f0f 100644 --- a/functions/src/__tests__/providers-osf.test.js +++ b/functions/src/__tests__/providers-osf.test.js @@ -152,6 +152,8 @@ describe("osfProvider.listFiles", () => { it("returns name/id pairs and filters out folder entries", async () => { mockFetch.mockResolvedValueOnce({ + status: 200, + statusText: "OK", json: () => Promise.resolve({ data: [ @@ -174,6 +176,46 @@ describe("osfProvider.listFiles", () => { expect.objectContaining({ method: "GET" }) ); }); + + // Regression coverage: a non-OK response (403/404/410/429/...) has no + // `data` field -- OSF returns a JSON:API error object instead -- so this + // must be checked before the body is parsed. Left unchecked, + // folder["data"].filter throws an opaque "Cannot read properties of + // undefined (reading 'filter')" instead of naming OSF's real status, which + // is exactly what reached production through the collision cache as + // "Collision cache rehydration failed: ... Cannot read properties of + // undefined (reading 'filter')" with no clue it was really an OSF 403. + it.each([ + [403, "Forbidden"], + [404, "Not Found"], + [410, "Gone"], + [429, "Too Many Requests"], + ])("throws with the status and statusText on a %i response", async (status, statusText) => { + mockFetch.mockResolvedValueOnce({ + status, + statusText, + json: () => Promise.resolve({ errors: [{ detail: "nope" }] }), + }); + + await expect(osfProvider.listFiles(auth, container)).rejects.toThrow( + `OSF listing failed: ${status} ${statusText}` + ); + }); + + // Same failure class as above, but with no status code to blame: a 200 + // whose body doesn't have the expected `data` array. Must not crash inside + // the filter/map with an unhelpful TypeError. + it("throws a clear error on a 200 response with no `data` array", async () => { + mockFetch.mockResolvedValueOnce({ + status: 200, + statusText: "OK", + json: () => Promise.resolve({ unexpected: "shape" }), + }); + + await expect(osfProvider.listFiles(auth, container)).rejects.toThrow( + "OSF listing failed: response body had no file list" + ); + }); }); // Real experiment documents store osfFilesLink as the osfstorage root's @@ -197,6 +239,8 @@ describe("osfProvider file URLs against a real osfstorage upload link", () => { it("updateFile addresses a listFiles id without doubling the provider segment", async () => { mockFetch.mockResolvedValueOnce({ + status: 200, + statusText: "OK", json: () => Promise.resolve({ data: [ diff --git a/functions/src/api-base64.ts b/functions/src/api-base64.ts index 9b35c11..4319f3f 100644 --- a/functions/src/api-base64.ts +++ b/functions/src/api-base64.ts @@ -13,6 +13,7 @@ import { getProviderForExperiment, claimNameFor } from "./providers/index.js"; import { WriteResult, ResolvedAuth } from "./providers/types.js"; import { claimFilename, claimFilenameWithoutCredentials, confirmClaim, CollisionCacheUnavailableError } from "./collision-cache.js"; import { isCompactionInFlight, COMPACTION_HOLD_REASON } from "./compaction-gate.js"; +import { isValidExperimentId } from "./experiment-id.js"; import { ExperimentData, UserData } from './interfaces'; // NO onRequest OPTIONS HERE ANY MORE. apiBase64Handler used to also back a @@ -33,6 +34,13 @@ export async function apiBase64Handler(req: Request, res: Response): Promise = db.collection("experiments").doc(experimentID); const exp_doc: DocumentSnapshot = await exp_doc_ref.get(); diff --git a/functions/src/api-condition.ts b/functions/src/api-condition.ts index 1a80bcf..d541b23 100644 --- a/functions/src/api-condition.ts +++ b/functions/src/api-condition.ts @@ -4,6 +4,7 @@ import { DocumentReference, DocumentData, DocumentSnapshot } from "firebase-admi import { db } from "./app.js"; import writeLog from "./write-log.js"; import MESSAGES from "./api-messages.js"; +import { isValidExperimentId } from "./experiment-id.js"; import { ExperimentData } from './interfaces'; // Plain handler, dispatched from participant-api.ts alongside @@ -18,6 +19,13 @@ export async function apiConditionHandler(req: Request, res: Response): Promise< return; } + // Firestore throws (not misses) on reserved ids like "__X__"; see experiment-id.ts. + // No writeLog: logs/{experimentID} would throw the same way. + if (!isValidExperimentId(experimentID)) { + res.status(400).json(MESSAGES.EXPERIMENT_NOT_FOUND); + return; + } + const exp_doc_ref: DocumentReference = db.collection("experiments").doc(experimentID); const exp_doc: DocumentSnapshot = await exp_doc_ref.get(); diff --git a/functions/src/api-data.ts b/functions/src/api-data.ts index 23dfddb..b2a51e9 100644 --- a/functions/src/api-data.ts +++ b/functions/src/api-data.ts @@ -21,6 +21,7 @@ import { WriteResult, ResolvedAuth } from "./providers/types.js"; import { claimFilename, claimFilenameWithoutCredentials, confirmClaim, CollisionCacheUnavailableError } from "./collision-cache.js"; import { isCompactionInFlight, COMPACTION_HOLD_REASON } from "./compaction-gate.js"; import { discardSession, isValidSessionId } from "./staging.js"; +import { isValidExperimentId } from "./experiment-id.js"; import { ExperimentData, UserData, RequestBody } from './interfaces'; import { apiBase64Handler } from "./api-base64.js"; @@ -185,6 +186,13 @@ export async function apiDataHandler(req: Request, res: Response): Promise if (isValidSessionId(sessionId)) await discardSession(sessionId); }; + // Firestore throws (not misses) on reserved ids like "__X__"; see experiment-id.ts. + // No writeLog: logs/{experimentID} would throw the same way. + if (!isValidExperimentId(experimentID)) { + res.status(400).json(MESSAGES.EXPERIMENT_NOT_FOUND); + return; + } + const exp_doc_ref: DocumentReference = db.collection("experiments").doc(experimentID); const exp_doc: DocumentSnapshot = await exp_doc_ref.get(); diff --git a/functions/src/api-finalize.ts b/functions/src/api-finalize.ts index 7d8c5cf..ec03320 100644 --- a/functions/src/api-finalize.ts +++ b/functions/src/api-finalize.ts @@ -48,6 +48,7 @@ import { db, functions } from "./app.js"; import { finalizeExperiment } from "./finalization.js"; import { ExperimentData, FinalizationState } from "./interfaces.js"; import { requireUser } from "./require-user.js"; +import { isValidExperimentId } from "./experiment-id.js"; // Task-queue functions cap at 1800s (see module header). finalizeExperiment // streams rather than buffers (docs/finalization-spec.md's "why streaming, @@ -87,6 +88,12 @@ export async function apiFinalizeHandler(req: Request, res: Response): Promise { if (req.method !== "GET") { @@ -32,6 +33,12 @@ export const apiQueueStatus = onRequest({ cors: true }, async (req, res) => { return; } + // Firestore throws (not misses) on reserved ids like "__X__"; see experiment-id.ts. + if (!isValidExperimentId(experimentID)) { + res.status(403).json({ error: "Access denied" }); + return; + } + // Verify the user owns this experiment const expDoc = await db.doc(`experiments/${experimentID}`).get(); if (!expDoc.exists || expDoc.data()?.owner !== uid) { diff --git a/functions/src/api-session-start.ts b/functions/src/api-session-start.ts index 1e9562c..8476b38 100644 --- a/functions/src/api-session-start.ts +++ b/functions/src/api-session-start.ts @@ -49,6 +49,7 @@ import { DocumentSnapshot } from "firebase-admin/firestore"; import { db } from "./app.js"; import writeLog from "./write-log.js"; import MESSAGES from "./api-messages.js"; +import { isValidExperimentId } from "./experiment-id.js"; import { ExperimentData } from "./interfaces.js"; import { openSession, @@ -100,6 +101,13 @@ export async function apiSessionStartHandler(req: Request, res: Response): Promi return; } + // Firestore throws (not misses) on reserved ids like "__X__"; see experiment-id.ts. + // No writeLog: logs/{experimentID} would throw the same way. + if (!isValidExperimentId(experimentID)) { + res.status(400).json(MESSAGES.EXPERIMENT_NOT_FOUND); + return; + } + const exp_doc: DocumentSnapshot = await db .collection("experiments") .doc(experimentID) diff --git a/functions/src/create-experiment.ts b/functions/src/create-experiment.ts index 18e6ca7..642a86c 100644 --- a/functions/src/create-experiment.ts +++ b/functions/src/create-experiment.ts @@ -177,6 +177,9 @@ export async function createExperimentHandler(req: Request, res: Response): Prom providerContainer = await storageProvider.createDataContainer(auth, containerInput); } catch (e) { const detail = e instanceof Error ? e.message : "Unknown error"; + // Otherwise this failure leaves no server-side trace: the 502 below is + // the only record, and it goes to the browser, not Cloud Logging. + console.error(`Error creating storage container for provider ${provider}:`, detail); res.status(502).json({ error: "Failed to create storage container", detail }); return; } diff --git a/functions/src/experiment-id.ts b/functions/src/experiment-id.ts new file mode 100644 index 0000000..d76e31a --- /dev/null +++ b/functions/src/experiment-id.ts @@ -0,0 +1,42 @@ +// Whether a client-supplied experimentID is even a value Firestore will +// accept as a document id -- checked BEFORE it is ever spliced into +// db.collection("experiments").doc(experimentID). Firestore does not treat an +// invalid id as a lookup miss: it THROWS synchronously for a handful of +// specific shapes ("3 INVALID_ARGUMENT: Resource id ... is invalid because it +// is reserved"), and that throw was reaching participant-facing endpoints +// unhandled as a 500. The commonest real case is a participant site that +// never filled in a template placeholder -- e.g. "__DATAPIPE_STUDY1_ID__" -- +// which happens to be exactly the reserved __...__ shape below. +// +// Kept in its own module rather than folded into staging.ts: it has nothing +// to do with the RTDB staging tier that file owns, it just happens to be the +// same kind of gate as isValidSessionId there -- the one check a +// client-controlled string has to clear before it is trusted as a Firestore +// document id. +// +// Deliberately NOT restricted to the nanoid alphabet create-experiment.ts +// mints new ids from: older experiment ids may use other formats, and the +// only thing this function has to guarantee is that whatever it accepts, a +// Firestore doc() call will accept too. (Contrast isValidSessionId in +// staging.ts, which DOES pin the exact format the server mints, because a +// sessionId is never anything but server-generated.) +// +// Firestore's own restrictions on a document id (see +// https://firebase.google.com/docs/firestore/quotas#collections_documents): +// - must be a non-empty string +// - may not contain "/" +// - may not be exactly "." or ".." +// - may not match /^__.*__$/ (reserved for Firestore's own use) +// - may not exceed 1500 bytes in UTF-8 +const RESERVED_ID_PATTERN = /^__.*__$/; +const MAX_ID_BYTES = 1500; + +/** Whether `value` is a string Firestore will accept as a document id. */ +export function isValidExperimentId(value: unknown): value is string { + if (typeof value !== "string" || value.length === 0) return false; + if (value.includes("/")) return false; + if (value === "." || value === "..") return false; + if (RESERVED_ID_PATTERN.test(value)) return false; + if (Buffer.byteLength(value, "utf-8") > MAX_ID_BYTES) return false; + return true; +} diff --git a/functions/src/providers/osf.ts b/functions/src/providers/osf.ts index 7ef03dc..9e00fba 100644 --- a/functions/src/providers/osf.ts +++ b/functions/src/providers/osf.ts @@ -244,8 +244,19 @@ export const osfProvider: StorageProvider = { }, }); - const folder = (await osfResult.json()) as { data: OSFFile[] }; - const listOfFiles: OSFFile[] = folder["data"]; + // An error response has no `data`, so the filter below would throw an opaque + // TypeError. Throw OSF's status instead (same shape as zenodo/gdrive listFiles); + // collision-cache's rehydrate wraps it in CollisionCacheUnavailableError. + if (osfResult.status !== 200) { + throw new Error(`OSF listing failed: ${osfResult.status} ${osfResult.statusText}`); + } + + const folder = (await osfResult.json()) as { data?: OSFFile[] }; + const listOfFiles = folder["data"]; + + if (!Array.isArray(listOfFiles)) { + throw new Error("OSF listing failed: response body had no file list"); + } return listOfFiles .filter((file) => file.attributes.kind === "file") From 8018c38cf299ddda25fac5705440362cf41b068b Mon Sep 17 00:00:00 2001 From: Josh de Leeuw Date: Fri, 25 Sep 2026 13:45:46 -0400 Subject: [PATCH 3/5] Route experiment lookups through one helper and tighten error logs getExperiment() returns null for a missing experiment and for an id Firestore would reject outright, so every endpoint that looks an experiment up by a client-supplied id keeps a single not-found branch and none can reach the doc() call that throws. This replaces the guard copied into six handlers and extends the fix to clear-errors and ensure-derived-paths, which still returned 500s. - api-queue-status checks its `download` queue-entry id the same way (404 instead of a 500 on "__x__" or "a/b"). - writeLog skips ids Firestore would reject, rather than every caller avoiding it on a comment that said it would throw (it never did). - create-experiment logs the Error object and the uid on a 502. - OSF listFiles errors carry OSF's own errors[0].detail, falling back to statusText. - Emulator coverage now includes session start, finalize, clear-errors, ensure-derived-paths and queue-status (experimentID and download). Co-Authored-By: Claude Opus 5.5 --- .../experiment-id-validation-emulator.test.js | 191 +++++++++++++++++- .../experiment-id-validation.test.js | 25 ++- functions/src/__tests__/providers-osf.test.js | 39 +++- functions/src/api-base64.ts | 18 +- functions/src/api-condition.ts | 18 +- functions/src/api-data.ts | 17 +- functions/src/api-finalize.ts | 17 +- functions/src/api-queue-status.ts | 23 ++- functions/src/api-session-start.ts | 20 +- functions/src/clear-errors.ts | 12 +- functions/src/create-experiment.ts | 8 +- functions/src/ensure-derived-paths.ts | 11 +- functions/src/experiment-id.ts | 24 ++- functions/src/providers/osf.ts | 22 +- functions/src/write-log.ts | 6 + 15 files changed, 336 insertions(+), 115 deletions(-) diff --git a/functions/src/__tests__/experiment-id-validation-emulator.test.js b/functions/src/__tests__/experiment-id-validation-emulator.test.js index 6fbd0df..2b1ce03 100644 --- a/functions/src/__tests__/experiment-id-validation-emulator.test.js +++ b/functions/src/__tests__/experiment-id-validation-emulator.test.js @@ -11,36 +11,98 @@ * of the ordinary 400 EXPERIMENT_NOT_FOUND a nonexistent-but-valid id already * gets. * - * isValidExperimentId() (experiment-id.ts) now gates every public, - * unauthenticated endpoint that looks an experiment up by a client-supplied - * id before the Firestore call that would otherwise throw. This suite proves - * the three data-submission endpoints that dispatch straight from a request - * body (api/data, api/base64, api/condition) answer with the normal - * not-found response instead of a 500. + * getExperiment() (experiment-id.ts) -- built on isValidDocumentId() -- now + * gates every endpoint that looks an experiment up by a client-supplied id + * before the Firestore call that would otherwise throw, so each endpoint + * folds the reserved-id case into the not-found branch it already had. This + * suite covers all of them: + * - the unauthenticated, participant-facing endpoints that answer 400 + * EXPERIMENT_NOT_FOUND for both a reserved id and a nonexistent one: + * /api/data, /api/base64, /api/condition, /api/session. + * - the authenticated, researcher-facing dashboard endpoints that answer + * 403 Access denied for a reserved id, a nonexistent id, and someone + * else's experiment alike (the same convention each of them already used + * for "doesn't exist" vs. "not yours"): /api/finalize, /api/clearerrors, + * /api/ensurederivedpaths, and /api/queuestatus's own experimentID + * parameter. + * - /api/queuestatus's separate `download` query parameter, which is + * checked with the same isValidDocumentId() gate but answers its own + * shape (404 "Queue entry not found") since a queue entry, not the + * experiment, is what a reserved or slash-containing id there would + * otherwise 500 trying to look up. */ +import { initializeApp, getApp } from "firebase-admin/app"; +import { getFirestore } from "firebase-admin/firestore"; +import { randomUUID } from "crypto"; import MESSAGES from "../api-messages"; import { fnUrl } from "./helpers/fn-url.js"; process.env.FIRESTORE_EMULATOR_HOST = "localhost:8080"; +const config = { projectId: "datapipe-test" }; +const AUTH_EMULATOR_SIGNUP_URL = + "http://localhost:9099/identitytoolkit.googleapis.com/v1/accounts:signUp?key=fake"; + // Exactly the placeholder observed in production logs -- also exactly // Firestore's reserved /^__.*__$/ shape. const RESERVED_ID = "__DATAPIPE_STUDY1_ID__"; +// A shorter reserved id, used for api-queue-status's `download` param below +// -- any /^__.*__$/ shape triggers the same Firestore throw, so this only +// needs to be reserved, not the exact production placeholder. +const RESERVED_DOWNLOAD_ID = "__x__"; + +jest.setTimeout(30000); + +let db; -async function postJSON(url, body) { +beforeAll(() => { + let app; + try { + app = getApp("experiment-id-validation-test"); + } catch { + app = initializeApp(config, "experiment-id-validation-test"); + } + db = getFirestore(app); +}); + +async function signUpEmulatorUser() { + const email = `experiment-id-validation-${randomUUID()}@example.test`; + const res = await fetch(AUTH_EMULATOR_SIGNUP_URL, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ email, password: "Password123!", returnSecureToken: true }), + }); + const body = await res.json(); + if (!res.ok) { + throw new Error(`Auth emulator signUp failed (${res.status}): ${JSON.stringify(body)}`); + } + return { uid: body.localId, idToken: body.idToken }; +} + +async function postJSON(url, body, idToken) { + const headers = { "Content-Type": "application/json", Accept: "*/*" }; + if (idToken !== undefined) { + headers.Authorization = `Bearer ${idToken}`; + } const response = await fetch(url, { method: "POST", - headers: { "Content-Type": "application/json", Accept: "*/*" }, + headers, body: JSON.stringify(body), }); const message = await response.json(); return { status: response.status, body: message }; } -jest.setTimeout(30000); +async function getJSON(url, idToken) { + const response = await fetch(url, { + headers: idToken !== undefined ? { Authorization: `Bearer ${idToken}` } : {}, + }); + const message = await response.json(); + return { status: response.status, body: message }; +} -describe("reserved/invalid experimentID does not 500", () => { +describe("reserved/invalid experimentID does not 500 (unauthenticated participant endpoints)", () => { it("POST /api/data returns 400 EXPERIMENT_NOT_FOUND, not a 500", async () => { const { status, body } = await postJSON(fnUrl("/api/data"), { experimentID: RESERVED_ID, @@ -71,4 +133,113 @@ describe("reserved/invalid experimentID does not 500", () => { expect(status).toBe(400); expect(body).toEqual(MESSAGES.EXPERIMENT_NOT_FOUND); }); + + it("POST /api/session returns 400 EXPERIMENT_NOT_FOUND, not a 500", async () => { + const { status, body } = await postJSON(fnUrl("/api/session"), { + experimentID: RESERVED_ID, + }); + + expect(status).toBe(400); + expect(body).toEqual(MESSAGES.EXPERIMENT_NOT_FOUND); + }); +}); + +describe("reserved/invalid experimentID does not 500 (authenticated dashboard endpoints)", () => { + it("POST /api/finalize returns 403 Access denied, not a 500", async () => { + const { idToken } = await signUpEmulatorUser(); + const { status, body } = await postJSON( + fnUrl("/api/finalize"), + { experimentID: RESERVED_ID }, + idToken + ); + + expect(status).toBe(403); + expect(body).toEqual({ error: "Access denied" }); + }); + + it("POST /api/clearerrors returns 403 Access denied, not a 500", async () => { + const { idToken } = await signUpEmulatorUser(); + const { status, body } = await postJSON( + fnUrl("/api/clearerrors"), + { experimentID: RESERVED_ID }, + idToken + ); + + expect(status).toBe(403); + expect(body).toEqual({ error: "Access denied" }); + }); + + it("POST /api/ensurederivedpaths returns 403 Access denied, not a 500", async () => { + const { idToken } = await signUpEmulatorUser(); + const { status, body } = await postJSON( + fnUrl("/api/ensurederivedpaths"), + { experimentID: RESERVED_ID }, + idToken + ); + + expect(status).toBe(403); + expect(body).toEqual({ error: "Access denied" }); + }); + + it("GET /api/queuestatus?experimentID=__X__ returns 403 Access denied, not a 500", async () => { + const { idToken } = await signUpEmulatorUser(); + const { status, body } = await getJSON( + `${fnUrl("/api/queuestatus")}?experimentID=${RESERVED_ID}`, + idToken + ); + + expect(status).toBe(403); + expect(body).toEqual({ error: "Access denied" }); + }); +}); + +describe("api-queue-status: reserved/slash-containing `download` id does not 500", () => { + const createdExperimentIds = []; + + afterEach(async () => { + if (createdExperimentIds.length === 0) return; + const batch = db.batch(); + for (const experimentID of createdExperimentIds) { + batch.delete(db.collection("experiments").doc(experimentID)); + } + await batch.commit(); + createdExperimentIds.length = 0; + }); + + async function seedOwnedExperiment(uid) { + const experimentID = `queue-status-reserved-download-${randomUUID()}`; + createdExperimentIds.push(experimentID); + await db.collection("experiments").doc(experimentID).set({ + owner: uid, + active: true, + storageProvider: "gdrive", + }); + return experimentID; + } + + it("returns 404 Queue entry not found for a reserved __x__ download id, on an experiment the caller owns", async () => { + const { uid, idToken } = await signUpEmulatorUser(); + const experimentID = await seedOwnedExperiment(uid); + + const { status, body } = await getJSON( + `${fnUrl("/api/queuestatus")}?experimentID=${experimentID}&download=${RESERVED_DOWNLOAD_ID}`, + idToken + ); + + expect(status).toBe(404); + expect(body).toEqual({ error: "Queue entry not found" }); + }); + + it("returns 404 Queue entry not found for a slash-containing download id (a%2Fb), on an experiment the caller owns", async () => { + const { uid, idToken } = await signUpEmulatorUser(); + const experimentID = await seedOwnedExperiment(uid); + + const { status, body } = await getJSON( + `${fnUrl("/api/queuestatus")}?experimentID=${experimentID}&download=${encodeURIComponent("a/b")}`, + idToken + ); + + expect(status).toBe(404); + expect(body).toEqual({ error: "Queue entry not found" }); + }); }); diff --git a/functions/src/__tests__/experiment-id-validation.test.js b/functions/src/__tests__/experiment-id-validation.test.js index 60c30c6..740bcd8 100644 --- a/functions/src/__tests__/experiment-id-validation.test.js +++ b/functions/src/__tests__/experiment-id-validation.test.js @@ -1,8 +1,13 @@ /** * @jest-environment node * - * isValidExperimentId (experiment-id.ts) -- the gate between a client- - * supplied experimentID and db.collection("experiments").doc(experimentID). + * isValidDocumentId (experiment-id.ts) -- the gate between a client-supplied + * id and db.collection(...).doc(id). Named for what it actually checks (any + * Firestore document id), not just the experiments collection: api-queue- + * status.ts runs its `download` queue-entry id through the same check, and + * write-log.ts its logs/{experimentID} id. It was renamed from + * isValidExperimentId to isValidDocumentId to reflect that; the logic itself + * is unchanged. * * The bug this guards against: Firestore does not treat an invalid document * id as a lookup miss, it THROWS synchronously out of doc()/get() for a @@ -19,14 +24,14 @@ * Firestore itself would reject. */ -const { isValidExperimentId } = require("../../lib/experiment-id.js"); +const { isValidDocumentId } = require("../../lib/experiment-id.js"); -describe("isValidExperimentId", () => { +describe("isValidDocumentId", () => { it.each([ ["a 12-char nanoid-style id, as create-experiment.ts mints", "aB3xY9kLm2Qz"], ["an id with mixed alphanumeric, hyphen and underscore characters", "abc-DEF_123"], ])("accepts %s", (_label, value) => { - expect(isValidExperimentId(value)).toBe(true); + expect(isValidDocumentId(value)).toBe(true); }); it.each([ @@ -43,7 +48,7 @@ describe("isValidExperimentId", () => { // string-length. ["over 1500 bytes via multi-byte characters, under 1500 JS characters", "é".repeat(751)], ])("rejects %s", (_label, value) => { - expect(isValidExperimentId(value)).toBe(false); + expect(isValidDocumentId(value)).toBe(false); }); it.each([ @@ -54,15 +59,15 @@ describe("isValidExperimentId", () => { ["an array", ["a"]], ["a boolean", true], ])("rejects %s (not a string)", (_label, value) => { - expect(isValidExperimentId(value)).toBe(false); + expect(isValidDocumentId(value)).toBe(false); }); it("accepts an id exactly at the 1500-byte boundary", () => { - expect(isValidExperimentId("a".repeat(1500))).toBe(true); + expect(isValidDocumentId("a".repeat(1500))).toBe(true); }); it("accepts a period or double period as part of a longer id, not standing alone", () => { - expect(isValidExperimentId("a.b")).toBe(true); - expect(isValidExperimentId("a..b")).toBe(true); + expect(isValidDocumentId("a.b")).toBe(true); + expect(isValidDocumentId("a..b")).toBe(true); }); }); diff --git a/functions/src/__tests__/providers-osf.test.js b/functions/src/__tests__/providers-osf.test.js index 2928f0f..edc2fb5 100644 --- a/functions/src/__tests__/providers-osf.test.js +++ b/functions/src/__tests__/providers-osf.test.js @@ -190,11 +190,11 @@ describe("osfProvider.listFiles", () => { [404, "Not Found"], [410, "Gone"], [429, "Too Many Requests"], - ])("throws with the status and statusText on a %i response", async (status, statusText) => { + ])("throws with the status and statusText on a %i response with no detail in the body", async (status, statusText) => { mockFetch.mockResolvedValueOnce({ status, statusText, - json: () => Promise.resolve({ errors: [{ detail: "nope" }] }), + json: () => Promise.resolve({ errors: [{}] }), }); await expect(osfProvider.listFiles(auth, container)).rejects.toThrow( @@ -202,6 +202,41 @@ describe("osfProvider.listFiles", () => { ); }); + // OSF's JSON:API error body carries its own explanation in errors[0].detail + // -- surfaced in preference to the generic statusText whenever it's present, + // since "403 Forbidden" alone gives a researcher nothing actionable while + // OSF's own detail says exactly what went wrong. + it("throws with OSF's own errors[0].detail on a 403 response, not just the statusText", async () => { + mockFetch.mockResolvedValueOnce({ + status: 403, + statusText: "Forbidden", + json: () => + Promise.resolve({ + errors: [{ detail: "You do not have permission to perform this action." }], + }), + }); + + await expect(osfProvider.listFiles(auth, container)).rejects.toThrow( + "OSF listing failed: 403 You do not have permission to perform this action." + ); + }); + + // Some error responses (a proxy timeout page, an HTML 5xx) are not JSON at + // all -- .json() rejects instead of resolving with a body that merely lacks + // `errors`. Must fall back to statusText rather than let that rejection + // propagate as an unrelated, confusing failure. + it("falls back to statusText when the error body is not JSON", async () => { + mockFetch.mockResolvedValueOnce({ + status: 502, + statusText: "Bad Gateway", + json: () => Promise.reject(new SyntaxError("Unexpected token < in JSON at position 0")), + }); + + await expect(osfProvider.listFiles(auth, container)).rejects.toThrow( + "OSF listing failed: 502 Bad Gateway" + ); + }); + // Same failure class as above, but with no status code to blame: a 200 // whose body doesn't have the expected `data` array. Must not crash inside // the filter/map with an unhelpful TypeError. diff --git a/functions/src/api-base64.ts b/functions/src/api-base64.ts index 4319f3f..c8ca477 100644 --- a/functions/src/api-base64.ts +++ b/functions/src/api-base64.ts @@ -1,7 +1,6 @@ import type { Request } from "firebase-functions/v2/https"; import type { Response } from "express"; import { randomUUID } from "crypto"; -import { DocumentReference, DocumentData, DocumentSnapshot } from "firebase-admin/firestore"; import { db } from "./app.js"; import writeLog from "./write-log.js"; import isBase64 from "is-base64"; @@ -13,7 +12,7 @@ import { getProviderForExperiment, claimNameFor } from "./providers/index.js"; import { WriteResult, ResolvedAuth } from "./providers/types.js"; import { claimFilename, claimFilenameWithoutCredentials, confirmClaim, CollisionCacheUnavailableError } from "./collision-cache.js"; import { isCompactionInFlight, COMPACTION_HOLD_REASON } from "./compaction-gate.js"; -import { isValidExperimentId } from "./experiment-id.js"; +import { getExperiment } from "./experiment-id.js"; import { ExperimentData, UserData } from './interfaces'; // NO onRequest OPTIONS HERE ANY MORE. apiBase64Handler used to also back a @@ -34,22 +33,17 @@ export async function apiBase64Handler(req: Request, res: Response): Promise = db.collection("experiments").doc(experimentID); - const exp_doc: DocumentSnapshot = await exp_doc_ref.get(); + // null for a missing experiment AND for an id Firestore would reject + // outright (e.g. an unfilled "__X__" placeholder); see experiment-id.ts. + const exp_doc = await getExperiment(experimentID); - if (!exp_doc.exists) { + if (!exp_doc) { res.status(400).json(MESSAGES.EXPERIMENT_NOT_FOUND); await writeLog(experimentID, "logError", MESSAGES.EXPERIMENT_NOT_FOUND); return; } + const exp_doc_ref = exp_doc.ref; const exp_data: ExperimentData = exp_doc.data() as ExperimentData; if (!exp_data) { diff --git a/functions/src/api-condition.ts b/functions/src/api-condition.ts index d541b23..85122b5 100644 --- a/functions/src/api-condition.ts +++ b/functions/src/api-condition.ts @@ -1,10 +1,9 @@ import type { Request } from "firebase-functions/v2/https"; import type { Response } from "express"; -import { DocumentReference, DocumentData, DocumentSnapshot } from "firebase-admin/firestore"; import { db } from "./app.js"; import writeLog from "./write-log.js"; import MESSAGES from "./api-messages.js"; -import { isValidExperimentId } from "./experiment-id.js"; +import { getExperiment } from "./experiment-id.js"; import { ExperimentData } from './interfaces'; // Plain handler, dispatched from participant-api.ts alongside @@ -19,22 +18,17 @@ export async function apiConditionHandler(req: Request, res: Response): Promise< return; } - // Firestore throws (not misses) on reserved ids like "__X__"; see experiment-id.ts. - // No writeLog: logs/{experimentID} would throw the same way. - if (!isValidExperimentId(experimentID)) { - res.status(400).json(MESSAGES.EXPERIMENT_NOT_FOUND); - return; - } - - const exp_doc_ref: DocumentReference = db.collection("experiments").doc(experimentID); - const exp_doc: DocumentSnapshot = await exp_doc_ref.get(); + // null for a missing experiment AND for an id Firestore would reject + // outright (e.g. an unfilled "__X__" placeholder); see experiment-id.ts. + const exp_doc = await getExperiment(experimentID); - if (!exp_doc.exists) { + if (!exp_doc) { res.status(400).json(MESSAGES.EXPERIMENT_NOT_FOUND); await writeLog(experimentID, "logError", MESSAGES.EXPERIMENT_NOT_FOUND); return; } + const exp_doc_ref = exp_doc.ref; const exp_data: ExperimentData = exp_doc.data() as ExperimentData; if (!exp_data) { diff --git a/functions/src/api-data.ts b/functions/src/api-data.ts index b2a51e9..1bebe0b 100644 --- a/functions/src/api-data.ts +++ b/functions/src/api-data.ts @@ -21,7 +21,7 @@ import { WriteResult, ResolvedAuth } from "./providers/types.js"; import { claimFilename, claimFilenameWithoutCredentials, confirmClaim, CollisionCacheUnavailableError } from "./collision-cache.js"; import { isCompactionInFlight, COMPACTION_HOLD_REASON } from "./compaction-gate.js"; import { discardSession, isValidSessionId } from "./staging.js"; -import { isValidExperimentId } from "./experiment-id.js"; +import { getExperiment } from "./experiment-id.js"; import { ExperimentData, UserData, RequestBody } from './interfaces'; import { apiBase64Handler } from "./api-base64.js"; @@ -186,23 +186,18 @@ export async function apiDataHandler(req: Request, res: Response): Promise if (isValidSessionId(sessionId)) await discardSession(sessionId); }; - // Firestore throws (not misses) on reserved ids like "__X__"; see experiment-id.ts. - // No writeLog: logs/{experimentID} would throw the same way. - if (!isValidExperimentId(experimentID)) { - res.status(400).json(MESSAGES.EXPERIMENT_NOT_FOUND); - return; - } - - const exp_doc_ref: DocumentReference = db.collection("experiments").doc(experimentID); - const exp_doc: DocumentSnapshot = await exp_doc_ref.get(); + // null for a missing experiment AND for an id Firestore would reject + // outright (e.g. an unfilled "__X__" placeholder); see experiment-id.ts. + const exp_doc = await getExperiment(experimentID); - if (!exp_doc.exists) { + if (!exp_doc) { res.status(400).json(MESSAGES.EXPERIMENT_NOT_FOUND); await writeLog(experimentID, "logError", MESSAGES.EXPERIMENT_NOT_FOUND); return; } + const exp_doc_ref = exp_doc.ref; const exp_data: ExperimentData = exp_doc.data() as ExperimentData; if (!exp_data) { diff --git a/functions/src/api-finalize.ts b/functions/src/api-finalize.ts index ec03320..9bab8cc 100644 --- a/functions/src/api-finalize.ts +++ b/functions/src/api-finalize.ts @@ -48,7 +48,7 @@ import { db, functions } from "./app.js"; import { finalizeExperiment } from "./finalization.js"; import { ExperimentData, FinalizationState } from "./interfaces.js"; import { requireUser } from "./require-user.js"; -import { isValidExperimentId } from "./experiment-id.js"; +import { getExperiment } from "./experiment-id.js"; // Task-queue functions cap at 1800s (see module header). finalizeExperiment // streams rather than buffers (docs/finalization-spec.md's "why streaming, @@ -88,22 +88,17 @@ export async function apiFinalizeHandler(req: Request, res: Response): Promise { if (req.method !== "GET") { @@ -33,15 +33,10 @@ export const apiQueueStatus = onRequest({ cors: true }, async (req, res) => { return; } - // Firestore throws (not misses) on reserved ids like "__X__"; see experiment-id.ts. - if (!isValidExperimentId(experimentID)) { - res.status(403).json({ error: "Access denied" }); - return; - } - - // Verify the user owns this experiment - const expDoc = await db.doc(`experiments/${experimentID}`).get(); - if (!expDoc.exists || expDoc.data()?.owner !== uid) { + // Verify the user owns this experiment. getExperiment's null also covers an + // id Firestore would reject outright, which would otherwise throw as a 500. + const expDoc = await getExperiment(experimentID); + if (!expDoc || expDoc.data()?.owner !== uid) { res.status(403).json({ error: "Access denied" }); return; } @@ -49,7 +44,13 @@ export const apiQueueStatus = onRequest({ cors: true }, async (req, res) => { const download = req.query.download as string | undefined; if (download) { - // Return a signed download URL for a specific queue entry + // Return a signed download URL for a specific queue entry. The id is + // checked first for the same reason as experimentID: doc() throws on a + // reserved "__X__" id, and a "/" would make this a collection path. + if (!isValidDocumentId(download)) { + res.status(404).json({ error: "Queue entry not found" }); + return; + } const queueDoc = await db.doc(`uploadQueue/${download}`).get(); if (!queueDoc.exists || queueDoc.data()?.experimentID !== experimentID) { res.status(404).json({ error: "Queue entry not found" }); diff --git a/functions/src/api-session-start.ts b/functions/src/api-session-start.ts index 8476b38..2635b4e 100644 --- a/functions/src/api-session-start.ts +++ b/functions/src/api-session-start.ts @@ -45,11 +45,9 @@ import type { Request } from "firebase-functions/v2/https"; import type { Response } from "express"; -import { DocumentSnapshot } from "firebase-admin/firestore"; -import { db } from "./app.js"; import writeLog from "./write-log.js"; import MESSAGES from "./api-messages.js"; -import { isValidExperimentId } from "./experiment-id.js"; +import { getExperiment } from "./experiment-id.js"; import { ExperimentData } from "./interfaces.js"; import { openSession, @@ -101,19 +99,11 @@ export async function apiSessionStartHandler(req: Request, res: Response): Promi return; } - // Firestore throws (not misses) on reserved ids like "__X__"; see experiment-id.ts. - // No writeLog: logs/{experimentID} would throw the same way. - if (!isValidExperimentId(experimentID)) { - res.status(400).json(MESSAGES.EXPERIMENT_NOT_FOUND); - return; - } - - const exp_doc: DocumentSnapshot = await db - .collection("experiments") - .doc(experimentID) - .get(); + // null for a missing experiment AND for an id Firestore would reject + // outright (e.g. an unfilled "__X__" placeholder); see experiment-id.ts. + const exp_doc = await getExperiment(experimentID); - if (!exp_doc.exists) { + if (!exp_doc) { res.status(400).json(MESSAGES.EXPERIMENT_NOT_FOUND); await writeLog(experimentID, "logError", MESSAGES.EXPERIMENT_NOT_FOUND); return; diff --git a/functions/src/clear-errors.ts b/functions/src/clear-errors.ts index dc61462..e7caec8 100644 --- a/functions/src/clear-errors.ts +++ b/functions/src/clear-errors.ts @@ -29,10 +29,7 @@ import { FieldValue } from "firebase-admin/firestore"; import { db } from "./app.js"; import MESSAGES from "./api-messages.js"; import { requireUser } from "./require-user.js"; - -function experimentRef(experimentID: string) { - return db.collection("experiments").doc(experimentID); -} +import { getExperiment } from "./experiment-id.js"; function logsRef(experimentID: string) { return db.collection("logs").doc(experimentID); @@ -57,12 +54,13 @@ export async function clearErrorsHandler(req: Request, res: Response): Promise MAX_ID_BYTES) return false; return true; } + +/** + * experiments/{experimentID}, or null when there is no such experiment -- + * including when `experimentID` is not an id Firestore would accept at all. + * Every endpoint that looks an experiment up by a client-supplied id goes + * through this, so none of them can reach the doc() call that throws, and + * each keeps the single not-found branch it already had. + */ +export async function getExperiment(experimentID: unknown): Promise { + if (!isValidDocumentId(experimentID)) return null; + const snap = await db.collection("experiments").doc(experimentID).get(); + return snap.exists ? snap : null; +} diff --git a/functions/src/providers/osf.ts b/functions/src/providers/osf.ts index 9e00fba..c629bad 100644 --- a/functions/src/providers/osf.ts +++ b/functions/src/providers/osf.ts @@ -57,6 +57,20 @@ function mapStatus(errorCode: number | null): ProviderErrorCode { } } +// OSF's own explanation of an error response: the JSON:API body's +// errors[0].detail, e.g. "You do not have permission to perform this action." +// Empty when the body is not JSON or has no detail -- the caller falls back to +// statusText, which is itself empty behind some proxies and over HTTP/2. +async function osfErrorDetail(response: { json: () => Promise }): Promise { + try { + const body = (await response.json()) as { errors?: { detail?: unknown }[] }; + const detail = body?.errors?.[0]?.detail; + return typeof detail === "string" ? detail : ""; + } catch { + return ""; + } +} + export const osfProvider: StorageProvider = { id: "osf", authMethod: "oauth2", @@ -245,10 +259,12 @@ export const osfProvider: StorageProvider = { }); // An error response has no `data`, so the filter below would throw an opaque - // TypeError. Throw OSF's status instead (same shape as zenodo/gdrive listFiles); - // collision-cache's rehydrate wraps it in CollisionCacheUnavailableError. + // TypeError. Throw OSF's status and its own explanation instead (same shape + // as zenodo/gdrive listFiles); collision-cache's rehydrate wraps it in + // CollisionCacheUnavailableError. if (osfResult.status !== 200) { - throw new Error(`OSF listing failed: ${osfResult.status} ${osfResult.statusText}`); + const reason = (await osfErrorDetail(osfResult)) || osfResult.statusText; + throw new Error(`OSF listing failed: ${osfResult.status} ${reason}`.trim()); } const folder = (await osfResult.json()) as { data?: OSFFile[] }; diff --git a/functions/src/write-log.ts b/functions/src/write-log.ts index 5ee8cf5..be68092 100644 --- a/functions/src/write-log.ts +++ b/functions/src/write-log.ts @@ -1,6 +1,7 @@ import { db } from "./app.js"; import { FieldValue, Timestamp } from "firebase-admin/firestore"; import { StorageProviderId } from "./providers/types.js"; +import { isValidDocumentId } from "./experiment-id.js"; /** * logs/{experimentID} — the per-experiment activity record. @@ -144,6 +145,11 @@ export default async function writeLog( error?: object, context?: LogContext ): Promise { + // A client-supplied id Firestore would reject (an unfilled "__X__" + // placeholder, say) can never have a log document. doc() would throw into + // the catch below and print an error per request; skip it quietly instead. + if (!isValidDocumentId(experimentID)) return false; + try { const log_doc_ref = db.collection("logs").doc(experimentID); From a7a91f000d270c46e5bccb03a05dc01cada8a187 Mon Sep 17 00:00:00 2001 From: Josh de Leeuw Date: Mon, 28 Sep 2026 14:36:39 -0400 Subject: [PATCH 4/5] Log why a Dataverse server rejected a token at connect validateStaticToken collapsed every non-200 into "invalid", and the connect endpoint turns that into "Invalid API token" without logging anything. A real 401, a firewall 403, and an outage 5xx looked the same after the fact, which left #278 undiagnosable. Log the server, status, and a truncated, token-scrubbed body so the next failure says why. Co-Authored-By: Claude Opus 5.5 --- .../src/__tests__/providers-dataverse.test.js | 21 +++++++++++++++++++ functions/src/providers/dataverse.ts | 16 +++++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/functions/src/__tests__/providers-dataverse.test.js b/functions/src/__tests__/providers-dataverse.test.js index f9d0284..5c9e268 100644 --- a/functions/src/__tests__/providers-dataverse.test.js +++ b/functions/src/__tests__/providers-dataverse.test.js @@ -1351,4 +1351,25 @@ describe("9. validateStaticToken", () => { expect(result).toBe(false); }); + + it("logs the status and body of a non-200, with the token scrubbed", async () => { + const warn = jest.spyOn(console, "warn").mockImplementation(() => {}); + mockFetch.mockResolvedValueOnce( + mockResponse({ + status: 403, + statusText: "Forbidden", + textBody: '{"status":"ERROR","message":"Bad api key test-token"}', + }) + ); + + await dataverseProvider.validateStaticToken(auth); + + expect(warn).toHaveBeenCalledTimes(1); + const [message] = warn.mock.calls[0]; + expect(message).toContain("403"); + expect(message).toContain(SERVER_URL); + expect(message).toContain("Bad api key [redacted]"); + expect(message).not.toContain("test-token"); + warn.mockRestore(); + }); }); diff --git a/functions/src/providers/dataverse.ts b/functions/src/providers/dataverse.ts index 7e2bac1..1e9d2ba 100644 --- a/functions/src/providers/dataverse.ts +++ b/functions/src/providers/dataverse.ts @@ -423,7 +423,21 @@ export const dataverseProvider: StorageProvider = { }); // Never throw on a non-200 -- a bad/expired token is simply "not valid", // not an exceptional condition. - return response.status === 200; + if (response.status === 200) return true; + + // Log what the installation actually said. The caller collapses every + // non-200 into "Invalid API token", so without this a real 401, a WAF + // 403, and an outage 5xx are indistinguishable after the fact. The body + // is truncated (a WAF block page can be large) and scrubbed of the token + // in case an installation echoes the key back in its error message. + let body = ""; + try { + body = (await response.text()).split(auth.token).join("[redacted]").slice(0, 300); + } catch { + // Body is diagnostic only; an unreadable one still leaves the status. + } + console.warn(`dataverse validateStaticToken: ${serverUrl}/api/users/:me returned ${response.status}: ${body}`); + return false; }, // Reads the token's expiry from the one endpoint that reports it, GET From 0d31a443fb6a9f077c2b988da0365775d40cdb7e Mon Sep 17 00:00:00 2001 From: Josh de Leeuw Date: Wed, 30 Sep 2026 09:55:57 -0400 Subject: [PATCH 5/5] Cap the Dataverse token-check body read and scrub tokens from create logs validateStaticToken read the whole non-200 body with response.text() from a researcher-chosen server, so a hostile or slow-dripping installation could exhaust dashboardapi's shared memory. It now reads at most 4KB for at most 5s, then drops the connection, and flattens the logged body to one line. createExperimentHandler logged the raw provider Error, but a Dataverse installation can echo the API key in its error text. The logged stack is now scrubbed with the same redaction validateStaticToken uses. Co-Authored-By: Claude Opus 5.5 --- .../create-experiment-log-redaction.test.js | 58 ++++++++++++++++++ .../src/__tests__/providers-dataverse.test.js | 61 +++++++++++++++++++ functions/src/create-experiment.ts | 17 ++++-- functions/src/providers/dataverse.ts | 50 ++++++++++++--- functions/src/redact.ts | 7 +++ 5 files changed, 180 insertions(+), 13 deletions(-) create mode 100644 functions/src/__tests__/create-experiment-log-redaction.test.js create mode 100644 functions/src/redact.ts diff --git a/functions/src/__tests__/create-experiment-log-redaction.test.js b/functions/src/__tests__/create-experiment-log-redaction.test.js new file mode 100644 index 0000000..8d639c7 --- /dev/null +++ b/functions/src/__tests__/create-experiment-log-redaction.test.js @@ -0,0 +1,58 @@ +/** + * @jest-environment node + * + * createExperimentHandler logs a failed createDataContainer to Cloud Logging. + * Auth rides in request headers, but a provider can still echo the token in + * its error text (a Dataverse installation answering "Bad api key "), so + * the logged stack must be scrubbed. The 502 detail still goes back to the + * researcher unchanged: it is their own token, on their own request. + */ + +const TOKEN = "dv-secret-token-1234"; + +jest.mock("../../lib/app.js", () => ({ + db: { doc: () => ({ get: async () => ({ data: () => ({}) }) }) }, +})); +jest.mock("../../lib/connect-provider.js", () => ({ + verifyOwnership: async () => ({ ok: true }), +})); +jest.mock("../../lib/resolve-token.js", () => ({ + __esModule: true, + default: async () => ({ success: true, token: TOKEN, serverUrl: "https://dataverse.mock.test" }), +})); +jest.mock("../../lib/providers/index.js", () => ({ + listProviders: () => ["dataverse"], + getProvider: () => ({ + containerInput: [], + createDataContainer: async () => { + throw new Error(`Dataverse dataset creation failed: 401 Bad api key ${TOKEN}`); + }, + }), +})); + +const { createExperimentHandler } = require("../../lib/create-experiment.js"); + +function mockRes() { + const res = {}; + res.status = jest.fn(() => res); + res.json = jest.fn(() => res); + return res; +} + +it("scrubs the provider token from the logged error", async () => { + const error = jest.spyOn(console, "error").mockImplementation(() => {}); + const res = mockRes(); + + await createExperimentHandler( + { method: "POST", body: { provider: "dataverse", title: "t", uid: "u1", idToken: "x" } }, + res + ); + + expect(res.status).toHaveBeenCalledWith(502); + expect(error).toHaveBeenCalledTimes(1); + const logged = JSON.stringify(error.mock.calls[0]); + expect(logged).not.toContain(TOKEN); + expect(logged).toContain("Bad api key [redacted]"); + expect(logged).toContain("create-experiment"); // the stack survives + error.mockRestore(); +}); diff --git a/functions/src/__tests__/providers-dataverse.test.js b/functions/src/__tests__/providers-dataverse.test.js index 5c9e268..3f6ae89 100644 --- a/functions/src/__tests__/providers-dataverse.test.js +++ b/functions/src/__tests__/providers-dataverse.test.js @@ -9,6 +9,8 @@ // and the docblock + node-fetch mock convention is kept for consistency with // every other adapter suite (see commit 0664bd5/313abbf/9008f67). +import { Readable } from "stream"; + const mockFetch = jest.fn(); jest.mock("node-fetch", () => ({ @@ -30,6 +32,7 @@ function mockResponse({ status, statusText, jsonBody, textBody }) { statusText, json: () => Promise.resolve(jsonBody), text: () => Promise.resolve(textBody), + body: textBody === undefined ? null : Readable.from([Buffer.from(textBody)]), }; } @@ -1372,4 +1375,62 @@ describe("9. validateStaticToken", () => { expect(message).not.toContain("test-token"); warn.mockRestore(); }); + + it("reads only a bounded prefix of a huge body and drops the connection", async () => { + const warn = jest.spyOn(console, "warn").mockImplementation(() => {}); + let pulled = 0; + // An endless 64KB-chunk body: text() would never finish buffering it. + const body = new Readable({ + read() { + pulled += 1; + this.push(Buffer.alloc(64 * 1024, "x")); + }, + }); + mockFetch.mockResolvedValueOnce({ status: 403, statusText: "Forbidden", body }); + + const result = await dataverseProvider.validateStaticToken(auth); + + expect(result).toBe(false); + expect(body.destroyed).toBe(true); + expect(pulled).toBeLessThan(5); + const [message] = warn.mock.calls[0]; + expect(message.length).toBeLessThan(500); + warn.mockRestore(); + }); + + it("gives up on a body that stalls, keeping what arrived", async () => { + jest.useFakeTimers(); + const warn = jest.spyOn(console, "warn").mockImplementation(() => {}); + const body = new Readable({ read() {} }); + body.push("partial block page"); + mockFetch.mockResolvedValueOnce({ status: 503, statusText: "Service Unavailable", body }); + + const pending = dataverseProvider.validateStaticToken(auth); + await jest.advanceTimersByTimeAsync(5000); + const result = await pending; + + expect(result).toBe(false); + expect(body.destroyed).toBe(true); + expect(warn.mock.calls[0][0]).toContain("503: partial block page"); + warn.mockRestore(); + jest.useRealTimers(); + }); + + it("flattens a multi-line body onto one log line", async () => { + const warn = jest.spyOn(console, "warn").mockImplementation(() => {}); + mockFetch.mockResolvedValueOnce( + mockResponse({ + status: 403, + statusText: "Forbidden", + textBody: "\n \n Request blocked\n \n", + }) + ); + + await dataverseProvider.validateStaticToken(auth); + + const [message] = warn.mock.calls[0]; + expect(message).not.toContain("\n"); + expect(message).toContain(" Request blocked "); + warn.mockRestore(); + }); }); diff --git a/functions/src/create-experiment.ts b/functions/src/create-experiment.ts index 781bf92..f2ad68c 100644 --- a/functions/src/create-experiment.ts +++ b/functions/src/create-experiment.ts @@ -25,6 +25,7 @@ import { customAlphabet } from "nanoid"; import { db } from "./app.js"; import { verifyOwnership } from "./connect-provider.js"; import resolveToken from "./resolve-token.js"; +import { redactSecret } from "./redact.js"; import { getProvider, listProviders } from "./providers/index.js"; import { ContainerRef, StorageProviderId, ResolvedAuth } from "./providers/types.js"; import { ExperimentData, UserData } from "./interfaces.js"; @@ -180,10 +181,18 @@ export async function createExperimentHandler(req: Request, res: Response): Prom // Otherwise this failure leaves no server-side trace: the 502 below is // the only record, and it goes to the browser, not Cloud Logging. The // Error itself, not just its message, so the stack and any network - // `code`/`cause` (ECONNRESET, ETIMEDOUT) survive; uid ties it to a - // user's report. Provider errors carry no token -- auth rides in the - // request headers, never the URL or the message. - console.error(`Error creating storage container for provider ${provider}, user ${uid}:`, e); + // `code` (ECONNRESET, ETIMEDOUT) survive; uid ties it to a user's + // report. Auth rides in request headers, never the URL, but a provider + // can still echo the token in its error text (a Dataverse installation + // answering "Bad api key "), so the stack is scrubbed before it + // reaches Cloud Logging. + const trace = e instanceof Error ? (e.stack ?? e.message) : String(e); + const code = (e as { code?: unknown } | null)?.code; + console.error( + `Error creating storage container for provider ${provider}, user ${uid}:`, + redactSecret(trace, auth.token), + ...(code !== undefined ? [{ code }] : []) + ); res.status(502).json({ error: "Failed to create storage container", detail }); return; } diff --git a/functions/src/providers/dataverse.ts b/functions/src/providers/dataverse.ts index 1e9d2ba..08c68ed 100644 --- a/functions/src/providers/dataverse.ts +++ b/functions/src/providers/dataverse.ts @@ -1,6 +1,8 @@ -import fetch from "node-fetch"; +import fetch, { type Response } from "node-fetch"; +import type { Readable } from "stream"; import { randomBytes } from "crypto"; import { decrypt } from "../crypto-utils.js"; +import { redactSecret } from "../redact.js"; import { UserData } from "../interfaces.js"; import { isAllowedServerUrl } from "./server-url.js"; import { @@ -55,6 +57,39 @@ function quoteHeaderParam(value: string): string { return `"${sanitized}"`; } +// validateStaticToken's server URL is researcher-chosen, so its error body is +// untrusted: read at most this much, for at most this long, then drop the +// connection. response.text() would buffer a hostile multi-hundred-MB (or +// slow-drip) body into dashboardapi's shared memory. The margin over the 300 +// characters logged leaves room for a token-sized redaction. +const VALIDATE_BODY_MAX_BYTES = 4096; +const VALIDATE_BODY_TIMEOUT_MS = 5000; + +// Never throws: the body is diagnostic only, and an unreadable one still +// leaves the status. +async function readBodyPrefix(response: Response, maxBytes: number, timeoutMs: number): Promise { + const stream = response.body as Readable | null; + if (!stream) return ""; + const chunks: Buffer[] = []; + let total = 0; + let timer: NodeJS.Timeout | undefined; + const timedOut = new Promise((resolve) => { + timer = setTimeout(resolve, timeoutMs); + }); + const read = (async () => { + for await (const chunk of stream) { + const buf = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk as Uint8Array); + chunks.push(buf); + total += buf.length; + if (total >= maxBytes) break; + } + })().catch(() => {}); + await Promise.race([read, timedOut]); + clearTimeout(timer); + stream.destroy(); + return Buffer.concat(chunks).subarray(0, maxBytes).toString("utf8"); +} + function authHeaders(auth: ResolvedAuth): Record { return { "X-Dataverse-key": auth.token }; } @@ -428,14 +463,11 @@ export const dataverseProvider: StorageProvider = { // Log what the installation actually said. The caller collapses every // non-200 into "Invalid API token", so without this a real 401, a WAF // 403, and an outage 5xx are indistinguishable after the fact. The body - // is truncated (a WAF block page can be large) and scrubbed of the token - // in case an installation echoes the key back in its error message. - let body = ""; - try { - body = (await response.text()).split(auth.token).join("[redacted]").slice(0, 300); - } catch { - // Body is diagnostic only; an unreadable one still leaves the status. - } + // is scrubbed of the token in case an installation echoes the key back in + // its error message, and flattened to one line so a multi-line block page + // stays in one log entry. + const prefix = await readBodyPrefix(response, VALIDATE_BODY_MAX_BYTES, VALIDATE_BODY_TIMEOUT_MS); + const body = redactSecret(prefix, auth.token).replace(/\s+/g, " ").trim().slice(0, 300); console.warn(`dataverse validateStaticToken: ${serverUrl}/api/users/:me returned ${response.status}: ${body}`); return false; }, diff --git a/functions/src/redact.ts b/functions/src/redact.ts new file mode 100644 index 0000000..50a2d28 --- /dev/null +++ b/functions/src/redact.ts @@ -0,0 +1,7 @@ +// Replaces every occurrence of `secret` in `text` before it is logged. An +// empty secret is a no-op: "abc".split("") would otherwise redact between +// every character. +export function redactSecret(text: string, secret: string | undefined): string { + if (!secret) return text; + return text.split(secret).join("[redacted]"); +}