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..2b1ce03 --- /dev/null +++ b/functions/src/__tests__/experiment-id-validation-emulator.test.js @@ -0,0 +1,245 @@ +/** + * @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. + * + * 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; + +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, + body: JSON.stringify(body), + }); + const message = await response.json(); + return { status: response.status, body: message }; +} + +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 (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, + 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); + }); + + 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 new file mode 100644 index 0000000..740bcd8 --- /dev/null +++ b/functions/src/__tests__/experiment-id-validation.test.js @@ -0,0 +1,73 @@ +/** + * @jest-environment node + * + * 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 + * 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 { isValidDocumentId } = require("../../lib/experiment-id.js"); + +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(isValidDocumentId(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(isValidDocumentId(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(isValidDocumentId(value)).toBe(false); + }); + + it("accepts an id exactly at the 1500-byte boundary", () => { + expect(isValidDocumentId("a".repeat(1500))).toBe(true); + }); + + it("accepts a period or double period as part of a longer id, not standing alone", () => { + 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 0f21296..edc2fb5 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,81 @@ 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 with no detail in the body", async (status, statusText) => { + mockFetch.mockResolvedValueOnce({ + status, + statusText, + json: () => Promise.resolve({ errors: [{}] }), + }); + + await expect(osfProvider.listFiles(auth, container)).rejects.toThrow( + `OSF listing failed: ${status} ${statusText}` + ); + }); + + // 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. + 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 +274,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..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,6 +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 { getExperiment } from "./experiment-id.js"; import { ExperimentData, UserData } from './interfaces'; // NO onRequest OPTIONS HERE ANY MORE. apiBase64Handler used to also back a @@ -33,15 +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 1a80bcf..85122b5 100644 --- a/functions/src/api-condition.ts +++ b/functions/src/api-condition.ts @@ -1,9 +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 { getExperiment } from "./experiment-id.js"; import { ExperimentData } from './interfaces'; // Plain handler, dispatched from participant-api.ts alongside @@ -18,15 +18,17 @@ export async function apiConditionHandler(req: Request, res: Response): Promise< 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 23dfddb..1bebe0b 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 { getExperiment } from "./experiment-id.js"; import { ExperimentData, UserData, RequestBody } from './interfaces'; import { apiBase64Handler } from "./api-base64.js"; @@ -185,16 +186,18 @@ export async function apiDataHandler(req: Request, res: Response): Promise if (isValidSessionId(sessionId)) await discardSession(sessionId); }; - 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 7d8c5cf..9bab8cc 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 { 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, @@ -87,16 +88,17 @@ export async function apiFinalizeHandler(req: Request, res: Response): Promise { if (req.method !== "GET") { @@ -32,9 +33,10 @@ export const apiQueueStatus = onRequest({ cors: true }, async (req, res) => { 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; } @@ -42,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 1e9562c..2635b4e 100644 --- a/functions/src/api-session-start.ts +++ b/functions/src/api-session-start.ts @@ -45,10 +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 { getExperiment } from "./experiment-id.js"; import { ExperimentData } from "./interfaces.js"; import { openSession, @@ -100,12 +99,11 @@ export async function apiSessionStartHandler(req: Request, res: Response): Promi 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 7ef03dc..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", @@ -244,8 +258,21 @@ 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 and its own explanation instead (same shape + // as zenodo/gdrive listFiles); collision-cache's rehydrate wraps it in + // CollisionCacheUnavailableError. + if (osfResult.status !== 200) { + 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[] }; + 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") 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);