Skip to content

Fix medium and low-severity bugs from the code audit - #88

Open
m-murphy wants to merge 35 commits into
mainfrom
fix/audit-medium
Open

m-murphy wants to merge 35 commits into
mainfrom
fix/audit-medium

Conversation

@m-murphy

@m-murphy m-murphy commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

Follow-up to #87: the medium and low findings from the code audit, plus a schema bug found while adding tests. One commit per fix; each fix has a regression test that fails without it (the activity refactor's test pins unchanged output instead). The last 10 commits address a review of this PR; see Review follow-ups.

API

  • Errors: UNIQUE and FOREIGN KEY violations return 409 with a readable message instead of a 500 with raw SQLite text.
  • Auth:
    • Login matches email before username, and usernames can't contain @.
    • Login and /auth/switch take as long to reject a missing account as a wrong password.
  • CSV export:
    • Cells containing the chosen delimiter (;, tab) are quoted.
    • Data exports neutralize formula-like text cells (=, +, @, and - unless the whole cell is a number, so -131 is unchanged but -2 in SubjID gets a '). Import templates are unaffected.
  • Derivation CSV:
    • Positions are normalized, and static wells checked, during validation.
    • A blank reduce_parent_quantity no longer overrides the shared setting.
    • Numbers that don't parse are row errors.
    • Paper rows check that the box or bag exists.
    • Headers are case-insensitive, and rows of only commas are skipped.
  • Plate scanner CSV: quoted fields are parsed.
  • Stats:
    • An unknown study returns empty stats, not whole-database totals.
    • The location filter includes static wells and papers.
    • No huge IN lists: a large location dropped from ~60 s to under 1 s on the production copy.
    • created_to includes the whole day (also in exports and the specimen list).
  • Export:
    • A subject listed for several visits keeps every date, and subjects listed without a date are included.
    • Specimen IDs are chunked.
  • Studies: summary and delete use subqueries, so studies past SQLite's ~65k parameter limit work.
  • Search: % and _ in search text match literally.
  • qPCR: target replacement is atomic and rejects duplicate names.
  • Control definitions: PATCH can't change controlType, and checks name and composition uniqueness.
  • Activity: /activity/recent is capped at 50 rows and built with three joined queries instead of up to seven per row.
  • Sheets:
    • Paper moves resolve sheets by name plus an optional fromParentName / toParentName; an ambiguous target is reported.
    • Nothing resolves a sheet by name alone any more: /collections/check no longer accepts type sheet (no client sent it).
    • Migration 006: sheet names are unique per box (bags are exempt for legacy DBS data).
    • Moving a sheet into a box that already has one with that name returns 409.
  • Locations: creating a root without parentId, or moving a root under a parent, no longer trips the storage-type CHECK. Moving a location to the root without a storage type is now a 400.
  • Control batches: new POST /blood-controls/batches/create-many-with-specimens creates all of the wizard's densities in one transaction. A conflict returns 409, other failures 400.
  • qPCR results tables (migration 007): qpcr_run, qpcr_well_result and qpcr_amplification_data were never created by initial_schema.sql, so results upload failed on fresh installs. They're created IF NOT EXISTS; existing databases are unchanged.
  • Integrity script:
    • Opens the database read-only, so it no longer migrates backups; accepts --db=<path>.
    • Checks tag links (uses rowid) and qPCR wells.

Web

  • Sessions:
    • A 401 from a data request signs the user out and redirects to login.
    • The query cache is cleared when the user changes.
  • Cache refresh: saving settings refreshes every query, and qPCR well edits refresh the cached experiment.
  • Stale responses: fixed races in qPCR well details and the Locations collection search.
  • CSV parsing: bulk import, the import preview, and scanner CSVs use the contract's quote-aware parser.
  • Dates: date-only values (collection, derivation, production dates) display on their own day in every time zone, and "today" defaults use the local date. Web tests run in America/Los_Angeles so this stays caught.
  • Container edit: clearing a comment is saved.
  • Add Specimens to Collection:
    • A scanner's Enter no longer submits.
    • Partly filled rows are reported.
    • Per-row server errors are shown.
  • Control batch wizard: uses the atomic multi-batch endpoint and shows the server's error.
  • URL filters: /specimens and /studies keep search and filters in the URL, so Clear Filters works and Back undoes a sort or filter change (search typing replaces the entry).
  • Other:
    • Backspace no longer navigates away while the Add Container modal is open.
    • Large qPCR result uploads are base64-encoded in chunks.

Review follow-ups

  • CSV formula neutralization spares only whole numbers; -1+cmd|'/C calc'!A0 was exported unescaped.
  • Sheets can't be resolved by name alone (see Sheets).
  • The remaining date-only displays use local calendar dates.
  • Back undoes /studies sort and filter changes.
  • The batch wizard keeps conflicts as 409, and its messages say "control batch" (CONTEXT.md).
  • Regression tests for the qPCR well and Locations search races, the integrity script's tag/qPCR checks, and settings userId validation.
  • CollectionSpecimenEntry.test.tsx follows the web test README.
  • Shared helpers replace duplicated code: parseCsv(..., { skipBlankRows }), shiftCalendarDate, the composition finder's excludeId, and a named study specimen subquery.
  • .gitignore no longer hides new files in packages/api/src/scripts.

Deploy notes

  • Migrations 006 and 007 run at startup; both applied cleanly to a copy of the production database.
  • Migration 006 aborts startup if any box holds two sheets with the same name (the app then won't start until the duplicates are renamed or the backup is restored). The production copy has none. To check a database before deploying:
    SELECT box_id, name, COUNT(*) FROM sheet WHERE box_id IS NOT NULL GROUP BY box_id, name HAVING COUNT(*) > 1;
    Take a backup first either way.
  • API behaviour changes:
    • 409 instead of 500 for constraint conflicts.
    • Usernames with @ are rejected (none exist today).
    • created_to is inclusive of the whole day.
    • Activity limit is capped at 50.
    • Sheet mappings accept fromParentName / toParentName.
    • New 400s: moving a location to the root without a storage type; PUT /settings/:key with a userId that isn't a positive integer or null; /collections/check with type sheet.
    • Saving settings marks every cached query stale (settings feed many unrelated queries).

Test plan

  • bun run ci:verify
  • Web: 909/909
  • Contract: 84/84
  • API: 1043/1043
  • Migrations 006 and 007 on a copy of the production database
  • No lint errors on changed lines

🤖 Generated with Claude Code

m-murphy and others added 30 commits September 25, 2026 09:26
UNIQUE and FOREIGN KEY violations (a duplicate study title, re-using a
deleted account's email) returned 500 with the raw SQLite message. They
now return 409 with a readable message; the SQLite text is only included
outside production.

Login matched email or username with one query and took the first row,
so a username equal to someone's email could shadow them. Email is now
matched first, and usernames cannot contain "@". Admin settings writes
validate the target userId.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
escapeCsvCell only quoted cells containing a comma, so ";" and tab
exports split any value containing the delimiter into extra columns.

Data exports (API exports and the collection table download) now prefix
text cells starting with =, +, @, tab, or CR with "'" so a spreadsheet
does not run them. A leading "-" is treated the same way unless it starts
a number or a phrase, so values like "-131" export unchanged. Import
templates are not affected.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Positions were compared unnormalized, so "A2" passed validation over a
  stored A02 and "A3"/"A03" were not flagged as duplicates; the import
  then failed and rolled back. Positions are normalized and checked
  with the shared occupancy rule (static wells included).
- A blank reduce_parent_quantity cell overrode a shared "false" setting.
- Quantities that are not numbers ("2,5", "3x") were silently dropped;
  they are now row errors.
- Paper rows passed validation if any paper existed anywhere, then
  failed on import when the named box or bag was missing.
- Headers match case-insensitively, and rows with no values (Excel's
  trailing ",,,") are skipped.
- The source specimen type warning compares against the first row whose
  parent resolves, not only row 0.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
parsePlateCSV split lines on raw commas, so a scanner export with quoted
fields produced headers like "Position" (with quotes) that never matched
the scanner configuration, and every row was dropped. It now uses the
shared contract CSV parser.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- An unknown study code returned totals for the whole database.
- The location filter only counted micronix and cryovial tubes; static
  wells and papers on sheets in boxes or bags were left out (a DBS
  location reported 0 of its 2,873 papers).
- Location and tag filters sent their full container id lists as IN
  parameters inside every chunk, which could exceed SQLite's limit and
  made a large location take about 60 s. They are now matched with
  subqueries and in-memory sets (under 1 s on the production copy).
- A bare created_to date (YYYY-MM-DD) compared "<=" against timestamps
  and left out everything created on that day, in stats, exports, and
  the specimen list.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Multi-study and single-study exports stored one date filter per subject,
so a manifest listing a subject for two visits exported only the last
one. Once any subject in a study had a date, subjects listed without a
date matched nothing. Each subject now keeps a list of date filters, and
subjects listed without a date match all of their dates (still honoring
the global date range).

Exact-date tolerance is computed in UTC, and the single-study container
query chunks specimen ids to stay under SQLite's parameter limit.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…limit.

- Replacing qPCR targets deleted the old ones before inserting the new
  ones outside a transaction; two targets with the same name (two blank
  names both become varATS) hit the unique index and left the
  experiment with no targets. Duplicates are now rejected up front and
  the replace runs in one transaction.
- Control definition PATCH accepted controlType, which could move a
  definition out of every blood route, and skipped the name and
  composition uniqueness checks that create applies.
- /activity/recent took any limit, including -1 (no limit in SQLite),
  with several queries per row. It is now clamped to 1-50.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
check-integrity opened the file through openOperationalDatabase, which
runs schema migrations, so pointing it at a backup upgraded the backup.
It now opens the file read-only (DATABASE_PATH or --db=<path>).

GROUP_CONCAT(<table>.id) failed on storage_container_tag, which has no
id column, and the error was skipped, so orphaned tag links were never
reported. Ids now come from rowid. qPCR well references are checked too.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Paper moves resolved a sheet by name alone and took the first match
anywhere, but default names like "Sheet-1" repeat in every box: a paper
in box BB's Sheet-1 failed with "No target collection mapping", or moved
into another box's sheet. Source sheets now match every sheet with that
name (optionally narrowed by fromParentName), and the target must be a
single sheet (narrowed by toParentName), otherwise the move reports the
ambiguity.

UNIQUE(name, box_id, bag_id) never fired because one of box_id / bag_id
is always NULL. Migration 006 adds a unique (box_id, name) index; bags
are exempt because legacy DBS bags hold many same-named sheets. Moving
sheets into a box that already has a sheet with that name returns 409.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Creating a root location without an explicit parentId stored a NULL
storage type (the check was parentId === null), and moving a root under
a parent without sending storageTypeId kept it; both violated the
location CHECK and returned 500. Moving a child to the root now requires
a storage type up front.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The batch wizard created one batch per density with separate requests.
If the third of four failed, the first two stayed, and a retry created
them again under new suggested names. The error shown was Axios's
"Request failed with status code 400" rather than the server's message.

A new POST /blood-controls/batches/create-many-with-specimens creates
every batch in one transaction; the wizard sends all densities in one
request and shows the server's error.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…hange.

When a session expired, every request returned 401 but the user stayed
set, so AuthGuard never redirected and each page showed "Could not
load" with a Retry that kept failing. A 401 from any non-auth endpoint
now clears the user, which sends them to login.

The query cache also survived logout, login, and user switch, so the
next user briefly saw the previous user's personal export
configurations and settings. It is cleared whenever the user changes.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Saving settings only invalidated the settings query, so the Export,
  qPCR, and move pages kept old export and scanner configurations for
  up to 5 minutes. A save now marks every query stale.
- qPCR well content-type edits updated only local state; revisiting the
  page within 5 minutes showed the old wells.
- Clicking a qPCR well and quickly another could show the first well's
  container in the second well's panel.
- A slow Locations collection search response could overwrite newer
  results or reopen the dropdown after the box was cleared.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The bulk import parser, the import preview, and the scanner plate parser
split lines on raw commas, so a quoted comment like "low vol, hemolyzed"
shifted every later column, and quoted scanner exports kept the quote
characters. They now use the contract's RFC 4180 parser, like the
derivation and control batch imports.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
new Date('2024-01-15') is UTC midnight, so collection and derivation
dates rendered a day early in the Americas. "Today" defaults used
toISOString(), which is tomorrow's date after about 5pm Pacific, and
that date was saved as the derivation or production date.

Bare YYYY-MM-DD values are now parsed as local calendar dates, and
today's date comes from the local clock.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ContainerEditModal sent `formData.comment || undefined`, so an emptied
comment was dropped from the JSON body, the server changed nothing, and
the modal closed as if it had saved. It now sends '', which the API
stores as NULL.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…rid.

In Add Specimens to Collection, a barcode scanner's trailing Enter
submitted the form, rows filled so far were created, half-filled rows
were skipped silently, and a 400 showed only "Validation failed".
Enter in a field no longer submits, partly filled positions are listed
as errors, and per-row server errors are shown against their positions.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…oads.

- /specimens read its filters from the URL only on mount, so "Clear
  Filters" (and back/forward) changed the URL but not the table. Filters
  are now derived from the URL.
- The Backspace "back to specimens" hotkey fired while the Add Container
  modal was open if focus was on a button.
- qPCR result uploads encoded the file with String.fromCharCode(...bytes),
  which throws on files over a few hundred KB; encoding is now chunked.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The study summary and study delete passed every subject, specimen, and
container id as bound parameters, which fails once a study passes
SQLite's variable limit (about 65k here). They now select through
subqueries on study_id.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Login and /auth/switch skipped bcrypt when the account did not exist,
so response time revealed which emails, usernames, and user ids are
real. Both now compare against a throwaway hash in that case.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Search queries built LIKE '%<text>%' from raw user input, so "_" and
"%" acted as wildcards: searching "3D7_0" also matched "3D7X0.05".
Search, specimen list, location, and error-log filters now escape the
text with ESCAPE '\'.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ueries.

Each recent specimen ran up to 3 follow-up queries and each container
up to 7. The feed now uses three joined queries in total (specimens,
studies, containers) and produces the same labels, which a new test
pins.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The Studies page kept search, sort, and type/lead filters in component
state, so the "Clear Filters" command (which clears the URL) did
nothing, filtered views could not be linked, and back/forward lost
them. They now live in the URL (q, sort, dir, type, lead), with defaults
left out; each control updates the URL in a single change.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Adds resetUserContextForTesting() for the provider's once-per-load
cache, and tests that switching users clears cached queries and that a
401 from a data request signs the user out.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
qpcr_run, qpcr_well_result, and qpcr_amplification_data are in the
Drizzle schema and in databases created before the SQL snapshot, but
initial_schema.sql never created them, so uploading qPCR results failed
with "no such table" on any new install. Migration 007 creates them
with IF NOT EXISTS (existing databases are unchanged), and the snapshot
now includes them.

Adds a route test for results upload that covers amplification rows
without a target name, which found this.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The check skipped any text starting with "-" and a digit, so
"-1+cmd|'/C calc'!A0" exported unescaped. Now a leading "-" is spared
only when the whole cell is a number ("-131", "-0.5"); "-2 in SubjID"
gets the ' prefix like other formula-shaped text.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
resolveCollectionByName returned the first sheet with a name anywhere,
which the "resolve by parent" decision ruled out. Sheets now resolve
only through resolveSheetsByName with their box or bag, and
/collections/check no longer accepts type 'sheet' (no client sent it).
resolveContainerByPosition is narrowed to plates and cryovial boxes,
the only types it ever matched.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Collection, derivation, and production dates on the container,
specimen, control batch, control definition, and blood control pages,
the study stats date range, and the qPCR list still parsed YYYY-MM-DD
as UTC midnight, so they showed the previous day in the Americas. They
now go through parseDisplayDate.

Web tests now run in America/Los_Angeles, so date tests fail on this
bug wherever CI runs; the new SpecimenDetail test fails on the old code.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Every URL update replaced the history entry, so Back left the page
instead of undoing a filter. Sort, type, lead, and clear now push an
entry; search keystrokes still replace so typing doesn't flood history.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
createBatchesWithSpecimens wrapped every failure in a ValidationError,
so a UNIQUE violation or ConflictError came back 400. Those now stay
409 with the readable conflict message; other failures stay 400. The
message reads "Control batch N (...): ... No control batches were
created.", per CONTEXT.md, as do the wizard's own fallbacks.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
m-murphy and others added 5 commits October 5, 2026 22:05
5a3ceb1 fixed both without tests. Each new test fails with the
stale-response guard removed: a slow reply for well A01 must not
replace well B01's container, and a slow "PL-1" collection search must
not replace "PL-12" results. The three qPCR well edits share one
applyUpdatedWells helper instead of three copies.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…userId.

938057f and 6de04c6 shipped without tests. The integrity test plants an
orphaned tag link (no id column) and qPCR well, and fails if ids come
from .id instead of rowid; checkDatabaseIntegrity is exported for it.
The settings test sends userId "abc", -1, and 1.5, each now a 400.

.gitignore's blanket scripts/ rule also hid packages/api/src/scripts
(its tracked files were force-added), so it is now an exception like
the other script folders.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…E asks.

Use the render helper, createMockedDomainModule, and user-event. The
scanner test now checks that Enter starts no submit (no half-filled-row
error), and still fails with the Enter guard removed.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- parseCsv takes skipBlankRows instead of four callers pasting the same
  filter.
- shiftCalendarDate (lib/datetime.ts) replaces the two inline Date.UTC
  day shifts in the stats upper bound and export tolerance window.
- The "Create batch with specimens" comment is back on its route.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Control definition PATCH calls findBloodControlDefinitionByComposition
  with a new excludeId option instead of re-creating its query; a test
  checks a definition doesn't conflict with itself.
- Study delete names the specimen subquery once instead of inlining it
  twice.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant