Upload completion returns 500 when an optional file group is omitted #1648

Activity

1. joshunrau

   added

   BugType: existing behavior is wrongType: existing behavior is wrong

   Priority: LowNice to have, cosmetic or speculativeNice to have, cosmetic or speculative

   Difficulty: LowIsolated change, about 2 hours or lessIsolated change, about 2 hours or less

   Area: APIapps/api (NestJS backend, Prisma models, CASL permissions)apps/api (NestJS backend, Prisma models, CASL permissions)

   Good First IssueWell-scoped, low-risk and self-contained; a good entry point for a new contributorWell-scoped, low-risk and self-contained; a good entry point for a new contributor

   on Oct 5, 2026
2. joshunrau commented on Oct 26, 2026

   

   joshunrau

   on Oct 26, 2026

   CollaboratorAuthor

   More actions

   Still a bug on main (d86740e), but the steps in the issue cannot run, and the suggested fix would not cover the input that does reproduce it. $$FileGroup has declared min: z.int().positive() (packages/schemas/src/instrument/instrument.file.ts:17) since 21695b3 added the FILE kind. So a { min: 0, max: 2 } group gets a 422 "Instrument validation failed" from POST /v1/instruments (apps/api/src/instruments/instruments.service.ts:102-107), and the test at apps/api/src/instrument-records/files/__tests__/files.service.test.ts:322 mocks an instrument that cannot be stored.

   The 500 is still reachable with a valid instrument. basename is a bare z.string() (instrument.file.ts:14), and uploads is a plain object, so a missing key falls through to Object.prototype:

   1. Upload a FILE instrument with one group { basename: 'constructor', count: { min: 1, max: 1 } }.
   2. Create a record and post { "uploads": {} } to upload-complete.

   uploads.constructor is Object. Its length of 1 passes the count check at files.service.ts:177, and the loop at :183 throws TypeError: uploadedFiles is not iterable. Any other function member of Object.prototype whose length falls in the group's count range does the same, such as hasOwnProperty, isPrototypeOf or __defineGetter__.

   Fix

   ?? [] does not catch this, because the inherited function is not nullish. Read own keys only, and drop the !:

   const uploadsByBasename = new Map(Object.entries(uploads));
   // in the loop
   const uploadedFiles = uploadsByBasename.get(fileGroup.basename) ?? [];

   noUncheckedIndexedAccess is on, so an Object.hasOwn ternary would still need the !. Both sides of the ?? are already covered: a present key by :244, a missing one by :295.

   • Unit: replace the :322 test with a constructor group { min: 1, max: 1 } and { uploads: {} }. Expect the 400 Invalid file count for file group 'constructor': expected 1 file(s), but got '0' and no instrumentRecordModel.update call.
   • e2e: no seeded instrument has such a group, and testing/ has no bundler. Read the ARBITRARY_SINGLE_FILE bundle the way readSeededBundle does (testing/src/specs/authorization.spec.ts:278-284). Replace basename:"file" with basename:"constructor", and name:"ARBITRARY_SINGLE_FILE" with a name built from uniqueId, because the id is a hash of name and edition. POST /v1/instruments it, then follow record-file-deletion.spec.ts with { uploads: {} } and assert a 400.

   The renderer has the same flaw. packages/react-core/src/components/FileInstrumentContent/Dropzone.tsx:52 reads store.errors[basename] from errors: {}. For a group named after any Object.prototype member, toString included, issues is the inherited value and issues.map throws on first render. store.ts:31-32 (errors[basename] ??= []) would also throw on a submit with the wrong count. #1706 is already editing $$FileGroup, and rejecting those basenames there would cover the renderer too. The API lookup should not depend on that.

   Order with #1706

   Land this first. If #1706 allows min: 0, the original steps start to reproduce. That PR should then also reject uploads keys that name no group, because with an optional group a misspelled key completes the record with no files. If #1706 keeps positive(), the min: 0 fixtures at files.service.test.ts:146, :210 and :307 describe instruments the schema rejects, and should be changed or removed.

   Related: the client-supplied location

   setUploadComplete stores each file's location.basename and location.index exactly as sent (files.service.ts:110-112). Nothing checks that the basename matches the group the file is listed under. Nothing checks that the index is one of the range(count.max) slots presigned at :88, or that indexes are unique. $FileLocation.index accepts any z.int() (packages/schemas/src/storage/storage.ts:6), and InstrumentRecordFile indexes only recordId (apps/api/prisma/schema.prisma:206). On ARBITRARY_SINGLE_FILE, "file": [{ "location": { "basename": "file", "index": 7 }, … }] returns 201 and stores a row whose download key file_7 was never presigned. A location basename that names no group stores a row that find never returns (:57). This should be its own issue, and it should land before #1742. The fix suggested there writes storageKey in setUploadComplete, and the only location available there is this client-supplied one.

   _{Triage note written with Claude Code. Code references point at main @ d86740e.}