From 41f2f5ac6cb308f147625793015b3a2ac3983741 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 25 Oct 2026 16:45:19 +0100 Subject: [PATCH 01/13] fix(db): leave a Postgres 16 volume alone while the database runs on it Going back to Postgres 16 after a move that stopped half-way left the moving marker, and the next start emptied the live 16 database and started a second server on its data directory. db-upgrade now waits for the database service and does nothing unless it runs Postgres 18. Using 16 again after a finished move and then updating started the app on the move-day copy without a word. The marker now records the old database's last checkpoint, and a later start refuses when it moved on; the guide says how to move the current database instead. The disk check leaves room for the first pre-migration dump, and the move check seeds the old database with v1.45's own migrations. --- docker-compose.yml | 37 +++++++++++-- docs/ops/postgres-major-upgrade.md | 27 +++++++++- scripts/compose-db-upgrade-check.sh | 54 +++++++++++++++++-- .../compose-db-upgrade-guard.test.ts | 2 + 4 files changed, 112 insertions(+), 8 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index 292340e3a..ad87ed191 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -680,6 +680,10 @@ services: # # On every start it decides in a second and exits: # - no `pgdata` data (a new install): nothing to do; + # - the database service runs Postgres 16 (after going back): nothing to + # do, the live 16 volume is left alone; + # - 16 was used again after an earlier move: refuses, so the app does not + # start on the move-day copy; # - Postgres 18 already holds the database: nothing to do; # - otherwise it starts Postgres 16 inside this container on the old # volume, streams a dump into the new database in one transaction, @@ -737,14 +741,35 @@ services: log "no Postgres 16 database to move, nothing to do" exit 0 fi + # The database service must be the new major. After going back to 16 the + # service runs on this very volume: touching it then would wipe the live data. + i=0 + until NEWVER=$$(new "SHOW server_version_num" 2>/dev/null); do + i=$$((i + 1)) + [ "$$i" -lt 60 ] || { log "the database service did not answer for a minute"; exit 1; } + sleep 1 + done + if [ "$$NEWVER" -lt 180000 ]; then + log "the database service runs Postgres $$((NEWVER / 10000)), not 18: nothing to move, the old volume is left alone" + exit 0 + fi + checkpoint() { pg_controldata "$$OLD" | awk -F': *' '/^Latest checkpoint location/ { print $$2 }'; } USER_TABLES="FROM pg_tables WHERE schemaname NOT IN ('pg_catalog', 'information_schema')" TABLES=$$(new "SELECT count(*) $$USER_TABLES") if [ -e "$$MARK" ]; then + MOVED_ON=$$(sed -n 1p "$$MARK") + MOVED_CHECKPOINT=$$(sed -n 2p "$$MARK") + if [ -n "$$MOVED_CHECKPOINT" ] && [ "$$(checkpoint)" != "$$MOVED_CHECKPOINT" ]; then + log "the Postgres 16 database changed after it was moved on $$MOVED_ON: it was used again after going back to 16." + log "Postgres 18 still holds the state of that day, so it is not started on. To move the current 16 database," + log "see docs/ops/postgres-major-upgrade.md, \"Updating again after going back\"." + exit 1 + fi if [ "$$TABLES" != 0 ]; then - log "the Postgres 16 database was moved on $$(cat "$$MARK"), nothing to do" + log "the Postgres 16 database was moved on $$MOVED_ON, nothing to do" exit 0 fi - log "the Postgres 16 database was moved on $$(cat "$$MARK"), but the Postgres 18 database is empty now." + log "the Postgres 16 database was moved on $$MOVED_ON, but the Postgres 18 database is empty now." log "Not importing the old state again by itself. See docs/ops/postgres-major-upgrade.md, \"Starting over\"." exit 1 fi @@ -763,8 +788,9 @@ services: fi NEED=$$(du -sk "$$OLD" | cut -f1) FREE=$$(df -Pk /var/lib/postgresql/next | awk 'NR == 2 { print $$4 }') - if [ "$$FREE" -le "$$NEED" ]; then - log "the move needs about $$((NEED / 1024)) MiB free for the new database, the disk has $$((FREE / 1024)) MiB." + # Room for the copy, and for the dump the app takes before its first migration. + if [ "$$FREE" -le $$((NEED * 3 / 2)) ]; then + log "the move needs about $$((NEED * 3 / 2 / 1024)) MiB free for the new database and its first dump, the disk has $$((FREE / 1024)) MiB." log "Nothing was changed. Free some space and start the stack again." exit 1 fi @@ -798,6 +824,9 @@ services: vacuumdb -h db -U healthlog -d healthlog --analyze-in-stages --quiet kill -INT "$$PID" wait "$$PID" || true + # The marker keeps the old database's last checkpoint: a later start can tell + # whether 16 was used again after the move. + { date -u +%Y-%m-%dT%H:%M:%SZ; checkpoint; } > "$$MOVING" mv "$$MOVING" "$$MARK" log "done. The old volume stays as it was, as a way back; see docs/ops/postgres-major-upgrade.md" diff --git a/docs/ops/postgres-major-upgrade.md b/docs/ops/postgres-major-upgrade.md index e856dc33f..670316b99 100644 --- a/docs/ops/postgres-major-upgrade.md +++ b/docs/ops/postgres-major-upgrade.md @@ -17,7 +17,9 @@ Postgres 18 database is up and before the app, and it exits in a second when there is nothing to move. When the old `pgdata` volume holds a Postgres 16 database and the new one is empty, it: -1. checks there is room on the disk for a second copy; +1. checks that the database service runs Postgres 18, and that there is room + on the disk for a second copy and for the dump the app takes before its + first migration; 2. starts Postgres 16 inside its own container on the old volume, reachable only from inside that container; 3. streams a dump into Postgres 18 in a single transaction; @@ -55,6 +57,29 @@ HEALTHLOG_IMAGE_REF=:v1.45.0 docker compose up -d ``` Anything written after the move lives only in the Postgres 18 volume. +`db-upgrade` can stay in the file: it sees that the database service runs +Postgres 16 and leaves that volume alone. This also holds after a move that +stopped half-way. + +## Updating again after going back + +If you used Postgres 16 again after going back, the Postgres 18 volume holds +the state of the day of the move, not what you wrote since. `db-upgrade` +notices that the old database changed after the move and refuses to start on +the old copy. To move the current 16 database, remove the old copy and the +marker, then start again: + +```bash +docker compose down +docker volume rm _postgres-data +docker compose run --rm --entrypoint rm db-upgrade \ + /var/lib/postgresql/data/HEALTHLOG_MOVED_TO_POSTGRES_18 +docker compose up -d +``` + +`docker volume ls` shows the volume's full name. Removing it deletes what was +written on Postgres 18 before you went back; take a dump of it first if any +of that matters. ## Afterwards diff --git a/scripts/compose-db-upgrade-check.sh b/scripts/compose-db-upgrade-check.sh index ad49e8b39..b22f3dd02 100755 --- a/scripts/compose-db-upgrade-check.sh +++ b/scripts/compose-db-upgrade-check.sh @@ -11,7 +11,11 @@ # empty the new database and run the move again. # 5. Empty the new volume: it must refuse to import the old state again, # and move it once more after the marker is removed as the guide says. -# 6. A new install with no old volume: nothing to do. +# 6. Going back to 16 after a move that did not finish: db-upgrade leaves +# the live 16 database alone. +# 7. Using 16 again after a move, then updating: refused, not the old copy. +# 8. Updating again as the guide says: the current 16 database moves. +# 9. A new install with no old volume: nothing to do. # # Needs Docker and the repo's node_modules (for the migrations). Run it from # anywhere in the repo: `bash scripts/compose-db-upgrade-check.sh`. @@ -85,7 +89,7 @@ wait_for_port() { migrate_old() { local started=$SECONDS until DATABASE_URL="postgresql://healthlog:$POSTGRES_PASSWORD@127.0.0.1:$PORT/healthlog?schema=public" \ - node "$ROOT/node_modules/prisma/build/index.js" migrate deploy --schema "$ROOT/prisma/schema.prisma" >"$WORK/migrate.log" 2>&1; do + node "$ROOT/node_modules/prisma/build/index.js" migrate deploy --schema "$WORK/old-prisma/prisma/schema.prisma" >"$WORK/migrate.log" 2>&1; do if [ $((SECONDS - started)) -ge 30 ] || ! grep -q "P1001" "$WORK/migrate.log"; then cat "$WORK/migrate.log" >&2 return 1 @@ -94,6 +98,18 @@ migrate_old() { done } +# The v1.45 schema, migrated with v1.45's own migrations. +mkdir -p "$WORK/old-prisma" +git -C "$ROOT" archive "$OLD_REF" prisma | tar -x -C "$WORK/old-prisma" +# The current compose file with its database pointed back at Postgres 16, the +# way docs/ops/postgres-major-upgrade.md "Going back" describes it. +sed -e 's|image: postgres:18\.[0-9]*-alpine|image: postgres:16.15-alpine|' \ + -e 's|- postgres-data:/var/lib/postgresql$|- pgdata:/var/lib/postgresql/data|' \ + "$ROOT/docker-compose.yml" >"$WORK/back.yml" +grep -q 'image: postgres:16.15-alpine' "$WORK/back.yml" && grep -q -- '- pgdata:/var/lib/postgresql/data' "$WORK/back.yml" || + { echo "FAIL: could not derive the going-back compose file" >&2; exit 1; } +back() { docker compose -p "$PROJECT" -f "$WORK/back.yml" "$@"; } + down_all step "a v1.45 database on Postgres 16" old up -d --wait db >"$WORK/up.log" 2>&1 || { cat "$WORK/up.log" >&2; fail "the v1.45 database did not start"; } @@ -157,10 +173,42 @@ CODE=$(run_upgrade) [ "$CODE" = 0 ] || fail "db-upgrade exited $CODE after the marker was removed" [ "$(probe docker exec healthlog-db)" = "$PROBE" ] || fail "the probe rows differ after starting over" +step "going back to 16 after a move that did not finish leaves 16 alone" +new down >/dev/null 2>&1 +docker volume rm "${PROJECT}_postgres-data" >/dev/null +docker run --rm -v "${PROJECT}_pgdata:/old" postgres:16.15-alpine \ + mv /old/HEALTHLOG_MOVED_TO_POSTGRES_18 /old/HEALTHLOG_MOVING_TO_POSTGRES_18 +back up -d db-upgrade >/dev/null 2>&1 || true +CODE=$(docker wait healthlog-db-upgrade) +upgrade_log +[ "$CODE" = 0 ] || fail "db-upgrade exited $CODE beside a Postgres 16 database service" +upgrade_log | grep -q 'not 18: nothing to move' || fail "db-upgrade did not recognise the Postgres 16 service" +[ "$(probe docker exec healthlog-db)" = "$PROBE" ] || fail "the live Postgres 16 data changed beside db-upgrade" + +step "using 16 again after a move, then updating, is refused instead of serving the old copy" +back down >/dev/null 2>&1 +CODE=$(run_upgrade) +[ "$CODE" = 0 ] || fail "the move after going back exited $CODE" +back up -d db >/dev/null 2>&1 +back exec -T db psql -U healthlog -d healthlog -v ON_ERROR_STOP=1 -qc \ + "INSERT INTO check_probe.rows VALUES (50001, 'written on 16 after the move')" +back down >/dev/null 2>&1 +CODE=$(run_upgrade) +[ "$CODE" = 1 ] || fail "db-upgrade exited $CODE after 16 was used again, expected 1" +upgrade_log | grep -q 'changed after it was moved' || fail "the refusal did not say that 16 changed" + +step "updating again as the guide says moves the current 16 database" +new down >/dev/null 2>&1 +docker volume rm "${PROJECT}_postgres-data" >/dev/null +new run --rm --entrypoint rm db-upgrade /var/lib/postgresql/data/HEALTHLOG_MOVED_TO_POSTGRES_18 >/dev/null 2>&1 +CODE=$(run_upgrade) +[ "$CODE" = 0 ] || fail "db-upgrade exited $CODE when moving the current 16 database" +[ "$(sql "SELECT count(*) FROM check_probe.rows")" = 50001 ] || fail "the row written on 16 did not arrive" + step "a new install: nothing to do" new down -v >/dev/null 2>&1 CODE=$(run_upgrade) [ "$CODE" = 0 ] || fail "db-upgrade exited $CODE on a new install" upgrade_log | tail -1 | grep -q 'no Postgres 16 database to move' || fail "the new install did not say there was nothing to move" -echo "OK: the Postgres 16 -> 18 move behaves in all six cases" +echo "OK: the Postgres 16 -> 18 move behaves in all nine cases" diff --git a/src/__tests__/compose-db-upgrade-guard.test.ts b/src/__tests__/compose-db-upgrade-guard.test.ts index 1ab117a85..2109088d9 100644 --- a/src/__tests__/compose-db-upgrade-guard.test.ts +++ b/src/__tests__/compose-db-upgrade-guard.test.ts @@ -77,6 +77,8 @@ describe("the Postgres 16 -> 18 move in docker-compose.yml", () => { // The checks run before the restore, in this order. const order = [ "PG_VERSION", + // Never touch the old volume while the database service runs on it. + 'if [ "$$NEWVER" -lt 180000 ]', "$$MARK", "$$MOVING", 'elif [ "$$TABLES" != 0 ]', From e8c96eea883e1f837c02a2f1ff6ec63656b5f651 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 25 Oct 2026 16:47:00 +0100 Subject: [PATCH 02/13] docs: say what deletion keeps and for how long, and drop stale claims The reproductive-health page promised that deleting all data or the account erases at once. Both wait for the grace window now, the nightly dumps keep a record until they age out, and the old pgdata volume is a full copy until it is removed; the page says so and how to make deletion final. The README, the getting-started note on majors, the move guide and the Unraid and Portainer templates catch up with Postgres 18, the backups volume and the recovery kit. The demo seed's spread bound leaves room for the seed's own one-in-150 spread. --- README.md | 2 +- docs/ops/postgres-major-upgrade.md | 6 ++-- docs/self-hosting/getting-started.md | 4 ++- docs/self-hosting/portainer/templates.json | 4 +-- docs/self-hosting/reproductive-health-data.md | 32 +++++++++++++------ docs/self-hosting/unraid/healthlog.xml | 4 +-- tests/integration/demo-seed-shape.test.ts | 6 ++-- 7 files changed, 38 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index edf748bbb..4c880ab87 100644 --- a/README.md +++ b/README.md @@ -87,7 +87,7 @@ curl -s http://localhost:3000/api/version # returns version + buildSha + built `SESSION_COOKIE_SECURE=false` is the one setting plain-HTTP self-hosts must set: without it the session cookie carries the `Secure` flag, the browser drops it over HTTP, and login silently fails. Leave it unset (or `true`) only when a TLS reverse proxy serves HTTPS — and then set `NEXT_PUBLIC_APP_URL` and `APP_URL` to the public URL first (see the [self-hosting guide](https://docs.healthlog.dev/getting-started/self-host-in-10-minutes/)). -Persistent state lives in exactly one named volume (`postgres-data`, the Postgres data); the app container is stateless, so backing up the database backs up everything. An install from v1.45 or earlier moves its Postgres 16 database into it on the first start ([details](docs/ops/postgres-major-upgrade.md)). +Persistent state lives in the database volume (`postgres-data`) and in `backups`, which holds the nightly database dumps; the app container is stateless. An install from v1.45 or earlier moves its Postgres 16 database from `pgdata` into the new volume on the first start ([details](docs/ops/postgres-major-upgrade.md)); recovering a lost server is in [disaster-recovery.md](docs/ops/disaster-recovery.md). NAS and homelab users have ready-made paths: [Unraid Community Applications](docs/self-hosting/unraid.md), [Portainer app templates](docs/self-hosting/portainer/README.md) and [TrueNAS](docs/self-hosting/truenas.md) (the Community train app is pending catalog review). diff --git a/docs/ops/postgres-major-upgrade.md b/docs/ops/postgres-major-upgrade.md index 670316b99..ff1c236da 100644 --- a/docs/ops/postgres-major-upgrade.md +++ b/docs/ops/postgres-major-upgrade.md @@ -36,8 +36,10 @@ If any step fails, the new database stays empty, the app does not start, and the log says what happened. A move cut short by a restart or a full disk is run again from scratch on the next `docker compose up -d`. -The old `pgdata` volume is read and never changed, apart from the marker -file. That is the way back. +The old `pgdata` volume keeps the database as it was. Postgres 16 runs on it +during the move, so it writes a checkpoint there (and recovers first after an +unclean stop), and the marker file is added; no data in it changes. That is +the way back. ## Going back diff --git a/docs/self-hosting/getting-started.md b/docs/self-hosting/getting-started.md index 0cd793b9a..859616446 100644 --- a/docs/self-hosting/getting-started.md +++ b/docs/self-hosting/getting-started.md @@ -433,7 +433,9 @@ Keep the dump until the new version has run for a few days, and keep your encryption key with it: a dump without the key cannot be read. The database image is a separate matter: a new Postgres minor is a change of -the `image:` line, a new major (16 to 18) is a dump and restore, described in +the `image:` line. The move from 16 to 18 runs by itself on the first start +of the compose file from v1.46; a database you run yourself moves by dump +and restore. Both are in [`docs/ops/postgres-major-upgrade.md`](../ops/postgres-major-upgrade.md). The start log says what is about to happen before the migrations run: diff --git a/docs/self-hosting/portainer/templates.json b/docs/self-hosting/portainer/templates.json index bfa70a388..c0cebc054 100644 --- a/docs/self-hosting/portainer/templates.json +++ b/docs/self-hosting/portainer/templates.json @@ -5,7 +5,7 @@ "type": 3, "title": "HealthLog", "name": "healthlog", - "description": "Self-hosted personal health tracker (PWA + native iOS client): weight, blood pressure, pulse, blood glucose, body composition, sleep, mood, menstrual cycle, and medications on one encrypted timeline. AES-256-GCM at rest, no vendor cloud, no telemetry. Ships app + PostgreSQL 16 as one stack.", + "description": "Self-hosted personal health tracker (PWA + native iOS client): weight, blood pressure, pulse, blood glucose, body composition, sleep, mood, menstrual cycle, and medications on one encrypted timeline. AES-256-GCM at rest, no vendor cloud, no telemetry. Ships app + PostgreSQL 18 as one stack.", "note": "Generate each secret with: openssl rand -hex 32 (a 64-char hex string). For plain-HTTP / LAN access set SESSION_COOKIE_SECURE to false, or login fails. Set NEXT_PUBLIC_APP_URL and APP_URL to how you reach the app. Full guide: https://github.com/MBombeck/HealthLog/blob/main/docs/self-hosting/getting-started.md", "categories": ["Tools", "Other"], "platform": "linux", @@ -33,7 +33,7 @@ { "name": "HEALTHLOG_PLATFORM", "label": "Platform hint", - "description": "Opens the admin's encryption key backup step on the Portainer instructions.", + "description": "Opens the admin's recovery kit on the Portainer instructions.", "default": "portainer" }, { diff --git a/docs/self-hosting/reproductive-health-data.md b/docs/self-hosting/reproductive-health-data.md index 314b65aa2..1fcaec46a 100644 --- a/docs/self-hosting/reproductive-health-data.md +++ b/docs/self-hosting/reproductive-health-data.md @@ -95,19 +95,31 @@ different things depending on which button you press. two actions below, the row stays. A start that was folded into a later cycle stays for as long as that cycle does, because deleting the later cycle gives it back. -- **Settings → Advanced → Delete all data** removes the rows outright, tombstones +- **Settings → Advanced → Delete all data** removes the rows, tombstones included. Everything the account owns goes except what you sign in - with; the confirmation dialog lists it. -- **Deleting the account** removes the account row, and the database - cascades every record with it. + with; the confirmation dialog lists it. Before it does, it keeps a copy + of the account on the server for the deletion grace window (14 days by + default), so a mistaken wipe can be restored; the copy is erased when + the window ends. +- **Deleting the account** locks it at once and erases it, with every + record, when the grace window ends. Until then an admin can undo it. -Two consequences follow, and they matter for the seizure threat model +`ACCOUNT_DELETION_GRACE_DAYS=0` makes both immediate again, without the +copy and without an undo. + +Three consequences follow, and they matter for the seizure threat model above. A host imaged while a per-entry deletion is the only deletion you -have performed still holds that entry. And a backup taken before any of -these actions still holds whatever it captured, because a backup is a -copy of a moment. If your reason for deleting is that the data must not -be recoverable from the machine, use one of the two whole-record actions -and rotate your backups. +have performed still holds that entry; so does a host imaged inside the +grace window. A backup taken before any of these actions still holds +whatever it captured, because a backup is a copy of a moment: the nightly +database dumps keep a deleted record until they age out (`DB_BACKUP_KEEP` +nights on the server, and your bucket's lifecycle rule for `database/`). +And after the move to Postgres 18 the old `pgdata` volume is a full copy +of the database from that day until you remove it. If your reason for +deleting is that the data must not be recoverable from the machine, set +the grace window to 0 or wait it out, remove the old volume, and let the +dumps age out or delete them +([backup-restore.md](../ops/backup-restore.md)). ## Practical hardening diff --git a/docs/self-hosting/unraid/healthlog.xml b/docs/self-hosting/unraid/healthlog.xml index c68c70c58..895340e41 100644 --- a/docs/self-hosting/unraid/healthlog.xml +++ b/docs/self-hosting/unraid/healthlog.xml @@ -46,14 +46,14 @@ 3000 - + false - unraid + unraid enforce diff --git a/tests/integration/demo-seed-shape.test.ts b/tests/integration/demo-seed-shape.test.ts index d35aa6742..d7557b8bd 100644 --- a/tests/integration/demo-seed-shape.test.ts +++ b/tests/integration/demo-seed-shape.test.ts @@ -105,7 +105,9 @@ describe("demo seed, short-window shape", () => { it("leaves the median of the window in the settled band", () => { // The usual range is the median ± 3 MAD of the last 30 days. A stretch // that fills half the window moves that median onto the stretch and - // widens the band to something like 87-181 mmHg. + // widens the band to something like 87-181 mmHg. The seed's own spread + // reaches about ±22 in one run of 150, so the bound sits at 24, still far + // below what a stretch that long produces. const sorted = [...systolic].sort((a, b) => a - b); const median = sorted[Math.floor(sorted.length / 2)]; expect(median).toBeLessThanOrEqual(124); @@ -113,7 +115,7 @@ describe("demo seed, short-window shape", () => { .map((v) => Math.abs(v - median)) .sort((a, b) => a - b); const mad = deviations[Math.floor(deviations.length / 2)]; - expect(3 * 1.4826 * mad).toBeLessThanOrEqual(20); + expect(3 * 1.4826 * mad).toBeLessThanOrEqual(24); }); it("raises ramipril on the day the stretch turns", () => { From 933be858c5e2f17070992dc242458e9d02661075 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 25 Oct 2026 16:53:31 +0100 Subject: [PATCH 03/13] fix(images): size a GIF by its largest frame, not only its logical screen The header check before a thumbnail decode read only the GIF logical screen. A decoder sizes its buffer from the frame's image descriptor, so a file with a small screen and a huge frame passed the pixel cap and allocated the full frame. Walk every block, read each image descriptor, size the image by the largest of screen and frames, and refuse a file that ends inside a header or descriptor, carries an unknown block, or has no frame. --- src/lib/__tests__/gif-fixture.ts | 35 ++++++++++ src/lib/__tests__/image-dimensions.test.ts | 56 ++++++++++++++- src/lib/documents/__tests__/thumbnail.test.ts | 27 +++++++ src/lib/image-dimensions.ts | 70 +++++++++++++++++-- 4 files changed, 181 insertions(+), 7 deletions(-) create mode 100644 src/lib/__tests__/gif-fixture.ts diff --git a/src/lib/__tests__/gif-fixture.ts b/src/lib/__tests__/gif-fixture.ts new file mode 100644 index 000000000..c166e5e7a --- /dev/null +++ b/src/lib/__tests__/gif-fixture.ts @@ -0,0 +1,35 @@ +import { Buffer } from "node:buffer"; + +const u16 = (n: number) => [n & 0xff, (n >> 8) & 0xff]; + +/** + * A GIF with the given logical screen and frames. Each frame carries one + * byte of image data, which is enough for a header walk and for a decoder. + */ +export function gif( + screen: { width: number; height: number; colourTable?: boolean }, + frames: Array<{ + width: number; + height: number; + left?: number; + top?: number; + extension?: boolean; + }>, +): Buffer { + const out: number[] = [...Buffer.from("GIF89a", "latin1")]; + out.push(...u16(screen.width), ...u16(screen.height)); + out.push(screen.colourTable ? 0x80 : 0x00, 0, 0); + if (screen.colourTable) out.push(0, 0, 0, 255, 255, 255); + for (const frame of frames) { + if (frame.extension) { + // A graphic control extension. + out.push(0x21, 0xf9, 4, 0, 0, 0, 0, 0); + } + out.push(0x2c, ...u16(frame.left ?? 0), ...u16(frame.top ?? 0)); + out.push(...u16(frame.width), ...u16(frame.height), 0x80); + out.push(0, 0, 0, 255, 255, 255); // local colour table, two entries + out.push(2, 2, 0x44, 0x01, 0); // LZW size, one sub-block, terminator + } + out.push(0x3b); + return Buffer.from(out); +} diff --git a/src/lib/__tests__/image-dimensions.test.ts b/src/lib/__tests__/image-dimensions.test.ts index 9145b1304..76cdc39ce 100644 --- a/src/lib/__tests__/image-dimensions.test.ts +++ b/src/lib/__tests__/image-dimensions.test.ts @@ -4,6 +4,7 @@ import { createCanvas } from "@napi-rs/canvas"; import { describe, expect, it } from "vitest"; import { readImageDimensions } from "../image-dimensions"; +import { gif } from "./gif-fixture"; /** * The header walk both the avatar upload and the document thumbnail trust to @@ -36,9 +37,58 @@ describe("readImageDimensions", () => { ); }); - it("reads a GIF's logical screen", () => { - const gif = Buffer.from("GIF89a\x25\x00\x17\x00\x00\x00\x00", "latin1"); - expect(readImageDimensions(gif, "image/gif")).toEqual(SIZE); + it("reads a GIF's logical screen when its frames fit inside it", () => { + expect( + readImageDimensions( + gif({ width: 37, height: 23 }, [{ width: 37, height: 23 }]), + "image/gif", + ), + ).toEqual(SIZE); + }); + + it("sizes a GIF by its largest frame, not only its logical screen", () => { + // The decoder allocates for the frame: 10 x 10 on the screen, 20000 x + // 20000 in the descriptor was a 1.6 GB decode behind a 100-pixel check. + expect( + readImageDimensions( + gif({ width: 10, height: 10 }, [{ width: 20_000, height: 20_000 }]), + "image/gif", + ), + ).toEqual({ width: 20_000, height: 20_000 }); + // A later frame, an offset, a global colour table and an extension in + // between are all walked. + expect( + readImageDimensions( + gif({ width: 10, height: 10, colourTable: true }, [ + { width: 4, height: 4 }, + { left: 60_000, top: 5, width: 5_000, height: 3, extension: true }, + ]), + "image/gif", + ), + ).toEqual({ width: 65_000, height: 10 }); + }); + + it("refuses a GIF it cannot walk to the end of a frame descriptor", () => { + const whole = gif({ width: 10, height: 10 }, [{ width: 10, height: 10 }]); + // Logical screen only: no frame, no size. + expect(readImageDimensions(whole.subarray(0, 13), "image/gif")).toBeNull(); + // Cut inside the image descriptor. + expect(readImageDimensions(whole.subarray(0, 18), "image/gif")).toBeNull(); + // A block no decoder knows. + const unknown = Buffer.from(whole); + unknown[13] = 0x99; + expect(readImageDimensions(unknown, "image/gif")).toBeNull(); + // A zero-sized frame. + expect( + readImageDimensions( + gif({ width: 10, height: 10 }, [{ width: 0, height: 10 }]), + "image/gif", + ), + ).toBeNull(); + // Not a GIF signature a decoder accepts. + const signature = Buffer.from(whole); + signature.write("GIF80a", 0, "latin1"); + expect(readImageDimensions(signature, "image/gif")).toBeNull(); }); it("skips fill bytes, RSTn and TEM before the frame header", () => { diff --git a/src/lib/documents/__tests__/thumbnail.test.ts b/src/lib/documents/__tests__/thumbnail.test.ts index 68fc8cc6d..7620df07e 100644 --- a/src/lib/documents/__tests__/thumbnail.test.ts +++ b/src/lib/documents/__tests__/thumbnail.test.ts @@ -19,6 +19,7 @@ vi.mock("@napi-rs/canvas", async (importActual) => { }; }); +import { gif } from "../../__tests__/gif-fixture"; import { generateThumbnail, THUMB_LONG_EDGE } from "../thumbnail"; /** @@ -214,6 +215,32 @@ describe("generateThumbnail", () => { expect(result.ok).toBe(true); }); + it("refuses a GIF whose frame is far larger than its logical screen, before decoding", async () => { + loadImageMock.mockClear(); + // A 10 x 10 screen around a 20000 x 20000 frame: the decoder sizes its + // buffer from the frame. + const bomb = gif({ width: 10, height: 10 }, [ + { width: 20_000, height: 20_000 }, + ]); + expect(bomb.length).toBeLessThan(64); + await expect(generateThumbnail(bomb, "image/gif")).resolves.toEqual({ + ok: false, + reason: "pixel-cap", + }); + expect(loadImageMock).not.toHaveBeenCalled(); + }); + + it("still previews an ordinary GIF", async () => { + const result = await generateThumbnail( + gif({ width: 1, height: 1 }, [{ width: 1, height: 1 }]), + "image/gif", + ); + expect(result).toMatchObject({ + ok: true, + thumbnail: { width: 1, height: 1 }, + }); + }); + it("refuses an image whose size cannot be read instead of decoding it", async () => { loadImageMock.mockClear(); // Start of image, then straight into a scan: no frame header to read. diff --git a/src/lib/image-dimensions.ts b/src/lib/image-dimensions.ts index 64346a025..806c5f6b1 100644 --- a/src/lib/image-dimensions.ts +++ b/src/lib/image-dimensions.ts @@ -64,12 +64,74 @@ function readPng(bytes: Buffer): ImageDimensions | null { return { width: bytes.readUInt32BE(16), height: bytes.readUInt32BE(20) }; } +/** + * The area a GIF decoder has to allocate: the logical screen, grown to cover + * every frame's image descriptor. + * + * The logical screen alone is not that size. A decoder sizes its buffer from + * the frame it decodes, so a file declaring a 10 x 10 screen and a + * 20000 x 20000 frame passed a screen-only check and allocated 1.6 GB. Every + * block is walked (GIF89a, section 17 onwards): extensions and image data are + * skipped sub-block by sub-block, each image descriptor is read, and anything + * that is not a block a decoder accepts makes the file unreadable. The walk + * is linear in the file's length. + * + * A file that ends inside a frame's image data is still read up to there, + * since decoders draw what they got; one that ends inside a header or a + * descriptor, or has no frame at all, has no size. + */ function readGif(bytes: Buffer): ImageDimensions | null { - // "GIF87a" / "GIF89a", then the logical screen: width, height (LE). - if (bytes.length < 10 || bytes.toString("ascii", 0, 4) !== "GIF8") { - return null; + // "GIF87a" / "GIF89a", then the logical screen descriptor (7 bytes). + if (bytes.length < 13) return null; + const signature = bytes.toString("ascii", 0, 6); + if (signature !== "GIF87a" && signature !== "GIF89a") return null; + let width = bytes.readUInt16LE(6); + let height = bytes.readUInt16LE(8); + const screenFlags = bytes[10]!; + let offset = 13; + if (screenFlags & 0x80) offset += 3 << ((screenFlags & 0x07) + 1); + + /** Past a run of data sub-blocks; `bytes.length` when the file ends in it. */ + const skipSubBlocks = (from: number): number => { + let at = from; + while (at < bytes.length) { + const size = bytes[at]!; + at += 1; + if (size === 0) return at; + at += size; + } + return bytes.length; + }; + + let frames = 0; + while (offset < bytes.length) { + const introducer = bytes[offset]!; + if (introducer === 0x3b) break; // trailer + if (introducer === 0x21) { + // Extension: introducer, label, then data sub-blocks. + if (offset + 2 > bytes.length) return null; + offset = skipSubBlocks(offset + 2); + continue; + } + if (introducer !== 0x2c) return null; + // Image descriptor: left, top, width, height (LE), flags. + if (offset + 10 > bytes.length) return null; + const left = bytes.readUInt16LE(offset + 1); + const top = bytes.readUInt16LE(offset + 3); + const frameWidth = bytes.readUInt16LE(offset + 5); + const frameHeight = bytes.readUInt16LE(offset + 7); + const frameFlags = bytes[offset + 9]!; + if (frameWidth === 0 || frameHeight === 0) return null; + width = Math.max(width, left + frameWidth); + height = Math.max(height, top + frameHeight); + frames += 1; + offset += 10; + if (frameFlags & 0x80) offset += 3 << ((frameFlags & 0x07) + 1); + // The LZW minimum code size, then the image data sub-blocks. + if (offset >= bytes.length) break; + offset = skipSubBlocks(offset + 1); } - return { width: bytes.readUInt16LE(6), height: bytes.readUInt16LE(8) }; + return frames > 0 ? { width, height } : null; } /** From 328268d894cc9dc039560dcda5beca81ea5b7648 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 25 Oct 2026 17:05:54 +0100 Subject: [PATCH 04/13] build(deps): bump the production-weekly group and codeql-action @aws-sdk/client-s3 and lib-storage 3.1155.0, @napi-rs/canvas 1.0.13, @tanstack/react-virtual 3.14.22, jose 6.2.13, lucide-react 1.61.0, nodemailer 10.2.1, and github/codeql-action 4.39.1. --- .github/workflows/codeql.yml | 4 +- .github/workflows/scorecard.yml | 2 +- .github/workflows/trivy.yml | 4 +- package.json | 14 +- pnpm-lock.yaml | 405 ++++++++++++++++---------------- 5 files changed, 219 insertions(+), 210 deletions(-) diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index b48a1a95b..d31f6aecc 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -60,7 +60,7 @@ jobs: # minutes and can produce nothing actionable, because nobody edits # it and the fix for anything found there is `pnpm prisma generate`. - name: Initialise CodeQL - uses: github/codeql-action/init@6f2ac81f5cd81fe351a197ba0b00d91d03cd2aee # v4.39.0 + uses: github/codeql-action/init@9b3254085494b55a0f7c4c2b3d2d52cc712ce370 # v4.39.1 with: languages: javascript-typescript build-mode: none @@ -75,6 +75,6 @@ jobs: - public - name: Analyse - uses: github/codeql-action/analyze@6f2ac81f5cd81fe351a197ba0b00d91d03cd2aee # v4.39.0 + uses: github/codeql-action/analyze@9b3254085494b55a0f7c4c2b3d2d52cc712ce370 # v4.39.1 with: category: "/language:javascript-typescript" diff --git a/.github/workflows/scorecard.yml b/.github/workflows/scorecard.yml index 448ce985d..e8e5e9174 100644 --- a/.github/workflows/scorecard.yml +++ b/.github/workflows/scorecard.yml @@ -54,7 +54,7 @@ jobs: publish_results: true - name: Upload results - uses: github/codeql-action/upload-sarif@6f2ac81f5cd81fe351a197ba0b00d91d03cd2aee # v4.39.0 + uses: github/codeql-action/upload-sarif@9b3254085494b55a0f7c4c2b3d2d52cc712ce370 # v4.39.1 with: sarif_file: scorecard-results.sarif category: scorecard diff --git a/.github/workflows/trivy.yml b/.github/workflows/trivy.yml index dbe96f860..5005de7d5 100644 --- a/.github/workflows/trivy.yml +++ b/.github/workflows/trivy.yml @@ -88,7 +88,7 @@ jobs: skip-dirs: src/generated - name: Upload results - uses: github/codeql-action/upload-sarif@6f2ac81f5cd81fe351a197ba0b00d91d03cd2aee # v4.39.0 + uses: github/codeql-action/upload-sarif@9b3254085494b55a0f7c4c2b3d2d52cc712ce370 # v4.39.1 with: sarif_file: trivy-filesystem.sarif category: trivy-filesystem @@ -145,7 +145,7 @@ jobs: exit-code: "0" - name: Upload results - uses: github/codeql-action/upload-sarif@6f2ac81f5cd81fe351a197ba0b00d91d03cd2aee # v4.39.0 + uses: github/codeql-action/upload-sarif@9b3254085494b55a0f7c4c2b3d2d52cc712ce370 # v4.39.1 with: sarif_file: trivy-published-image.sarif category: trivy-published-image diff --git a/package.json b/package.json index 2eed08de7..53c67faa9 100644 --- a/package.json +++ b/package.json @@ -54,13 +54,13 @@ "mcp:stdio": "npx tsx src/cli/mcp-stdio.ts" }, "dependencies": { - "@aws-sdk/client-s3": "^3.1153.0", - "@aws-sdk/lib-storage": "3.1153.0", + "@aws-sdk/client-s3": "^3.1156.0", + "@aws-sdk/lib-storage": "3.1155.0", "@dnd-kit/core": "^6.3.1", "@dnd-kit/sortable": "^10.0.0", "@dnd-kit/utilities": "^3.2.2", "@modelcontextprotocol/sdk": "^1.32.1", - "@napi-rs/canvas": "1.0.11", + "@napi-rs/canvas": "1.0.13", "@node-rs/argon2": "^2.2.2", "@parse/node-apn": "^8.1.0", "@prisma/adapter-pg": "^7.10.1", @@ -68,20 +68,20 @@ "@simplewebauthn/browser": "^14.0.0", "@simplewebauthn/server": "^14.0.3", "@tanstack/react-query": "5.104.5", - "@tanstack/react-virtual": "^3.14.21", + "@tanstack/react-virtual": "^3.14.22", "@types/sax": "^1.2.7", "class-variance-authority": "^0.7.1", "clsx": "^2.1.1", "date-fns": "^4.4.0", "dotenv": "^18.0.8", "fflate": "^0.8.3", - "jose": "^6.2.12", + "jose": "^6.2.13", "jspdf": "^4.2.1", "jspdf-autotable": "^5.0.9", - "lucide-react": "^1.59.0", + "lucide-react": "^1.61.0", "mmdb-lib": "^3.0.3", "next": "16.4.2", - "nodemailer": "10.1.7", + "nodemailer": "10.2.1", "otpauth": "^9.5.2", "p-limit": "^7.3.3", "pg": "^8.23.1", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 914dac854..f42434e67 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -45,11 +45,11 @@ importers: .: dependencies: '@aws-sdk/client-s3': - specifier: ^3.1153.0 - version: 3.1153.0 + specifier: ^3.1156.0 + version: 3.1156.0 '@aws-sdk/lib-storage': - specifier: 3.1153.0 - version: 3.1153.0(@aws-sdk/client-s3@3.1153.0) + specifier: 3.1155.0 + version: 3.1155.0(@aws-sdk/client-s3@3.1156.0) '@dnd-kit/core': specifier: ^6.3.1 version: 6.3.1(react-dom@19.3.1(react@19.3.1))(react@19.3.1) @@ -63,8 +63,8 @@ importers: specifier: ^1.32.1 version: 1.32.1(@cfworker/json-schema@4.1.1)(supports-color@7.2.0)(zod@4.6.7) '@napi-rs/canvas': - specifier: 1.0.11 - version: 1.0.11 + specifier: 1.0.13 + version: 1.0.13 '@node-rs/argon2': specifier: ^2.2.2 version: 2.2.2 @@ -87,8 +87,8 @@ importers: specifier: 5.104.5 version: 5.104.5(@types/react@19.3.0)(react@19.3.1) '@tanstack/react-virtual': - specifier: ^3.14.21 - version: 3.14.21(react-dom@19.3.1(react@19.3.1))(react@19.3.1) + specifier: ^3.14.22 + version: 3.14.22(react-dom@19.3.1(react@19.3.1))(react@19.3.1) '@types/sax': specifier: ^1.2.7 version: 1.2.7 @@ -108,8 +108,8 @@ importers: specifier: ^0.8.3 version: 0.8.3 jose: - specifier: ^6.2.12 - version: 6.2.12 + specifier: ^6.2.13 + version: 6.2.13 jspdf: specifier: ^4.2.1 version: 4.2.1 @@ -117,8 +117,8 @@ importers: specifier: ^5.0.9 version: 5.0.9(jspdf@4.2.1) lucide-react: - specifier: ^1.59.0 - version: 1.59.0(@types/react@19.3.0)(react@19.3.1) + specifier: ^1.61.0 + version: 1.61.0(@types/react@19.3.0)(react@19.3.1) mmdb-lib: specifier: ^3.0.3 version: 3.0.3 @@ -126,8 +126,8 @@ importers: specifier: 16.4.2 version: 16.4.2(@babel/core@7.29.7(supports-color@7.2.0))(@opentelemetry/api@1.9.1)(@playwright/test@1.64.1)(@types/node@26.6.5)(react-dom@19.3.1(react@19.3.1))(react@19.3.1) nodemailer: - specifier: 10.1.7 - version: 10.1.7 + specifier: 10.2.1 + version: 10.2.1 otpauth: specifier: ^9.5.2 version: 9.5.2 @@ -301,74 +301,74 @@ packages: resolution: {integrity: sha512-UrcABB+4bUrFABwbluTIBErXwvbsU/V7TZWfmbgJfbkwiBuziS9gxdODUyuiecfdGQ85jglMW6juS3+z5TsKLw==} engines: {node: '>=10'} - '@aws-sdk/checksums@3.1001.4': - resolution: {integrity: sha512-H8BiG/2DnfwydVxD1uDPLa1s1sfhRnVTbD2OwzfTr3WlJzgbIzICDXxBTlTMjbWNNCe4VS9X55fco+JI34L9kw==} + '@aws-sdk/checksums@3.1001.5': + resolution: {integrity: sha512-6erto9nK2OZO/woEgmFIaPh5gChgdNd7nL6jgAf8TAXz3/QHC8Pan2TFe9GYp3IxOIKCCGWPylSjtOHKfNzQlw==} engines: {node: '>=20.0.0'} - '@aws-sdk/client-s3@3.1153.0': - resolution: {integrity: sha512-h0VScQZPfBApg1Qi4AedsOUSMZ7m758kZIg0JO0B5HtwSzQhB7Tgh6h1JoGEXELgrWADPusYMpMBDmrRAEx2yw==} + '@aws-sdk/client-s3@3.1156.0': + resolution: {integrity: sha512-Z2PqQqKMDVX56tNZ6ggmjoI5o/8+4+8ArmjLwE5ktYsEzs1BrvklBT1ZzI+s0Vcf3wfPguoEMD7EkMswmEta/A==} engines: {node: '>=20.0.0'} - '@aws-sdk/core@3.978.4': - resolution: {integrity: sha512-JvGp6y9sPX8FSs2K/EmFMZcUfdkHxqGE45+oxwGC75OIh50j2e4cbx7zcR3BwNs2DSGqJdBja5h6Vy89iIDmRQ==} + '@aws-sdk/core@3.978.5': + resolution: {integrity: sha512-ruI0AWqcrfL20XDS6SwZIJFPOBZ6N0ADa9nLzWlBhxk1aoUjbUX3JkTR9vEmk7o35XpAHi68ahZ4T6RE2U9XyA==} engines: {node: '>=20.0.0'} - '@aws-sdk/credential-provider-env@3.972.75': - resolution: {integrity: sha512-nc8Epg98PcYoKNx0VFlwtmZSCP+tuJ1FLWqpslET/gp4h/WYuULSJqtxppM6ywKKDdzNJPvLD7ceRfmxm3Qdxw==} + '@aws-sdk/credential-provider-env@3.972.76': + resolution: {integrity: sha512-YxmL1Ar7bV9ZkJvtv+lj+MF6le76NnZnbmgySyZuxpZaCv0/1lZ3KxKy6RIj1h9ui2hWnVjZxbmTMeO0y/NrQA==} engines: {node: '>=20.0.0'} - '@aws-sdk/credential-provider-http@3.972.77': - resolution: {integrity: sha512-KkYFdIQwuw9IOHfEzK78slxmVWuhcm1w6YgE3V3WVxAeRRj9q5J9jlwzpY/oszWxd4SD96FTFkDdRGBdMFF2Mw==} + '@aws-sdk/credential-provider-http@3.972.78': + resolution: {integrity: sha512-3VHknDRngoebsaqyT1iR75b5cBT2zfEtODK6yYmYltljkK1HEAQLs2FEd1IpWqAeOYdMGaG9wikngWLTufY7RA==} engines: {node: '>=20.0.0'} - '@aws-sdk/credential-provider-ini@3.973.20': - resolution: {integrity: sha512-OSl2R/0zLkoeKEIy6oP2tu/bsSdY+DGgro2eOffMD2CcP8POv7rS14FY5Jzc5bjnqB1G9wv/aM1akHC2mdlhZw==} + '@aws-sdk/credential-provider-ini@3.973.21': + resolution: {integrity: sha512-aLx9hbCX6mGAyv4ZZd8UagJ4RASHVJw6BFDSIyuKmLkqNL6H47EjBPK6qaJ4B5TDV6mc+vMa0SJFYyo4mkvVQg==} engines: {node: '>=20.0.0'} - '@aws-sdk/credential-provider-login@3.972.82': - resolution: {integrity: sha512-8jyDPtf1WPxeQ/5xlNpyxAD6P7YKwkYeNk8B2vFEDwkPWDvb7c3tQJG5/jbNWbF2yTMIH9tDTueBkdfovYIAPw==} + '@aws-sdk/credential-provider-login@3.972.83': + resolution: {integrity: sha512-9bJU6FjRacF4FK/QHf8Dyxhur7B1grRj8C1Yh4IkpoFW2exQflPdCv0VrbemLW9ZctLNle+AE7TETs9KNvuLYg==} engines: {node: '>=20.0.0'} - '@aws-sdk/credential-provider-node@3.972.87': - resolution: {integrity: sha512-TKj2C0x4IMPSSOAlmeryX8fCPvvPS2pFJz2ikV9IGqyh6Ehsg4leMalCTVfV2rvMTrlGAe7X7nK0HZ3P0KZR9g==} + '@aws-sdk/credential-provider-node@3.972.88': + resolution: {integrity: sha512-AG0KID5qhQPzrFkh5b0Cws1Amytvzo0L0Xj6hMgJXEwD6ZXAqninDhSNAWJiN5zU4YVVZnuidnqwPwYJ04IamA==} engines: {node: '>=20.0.0'} - '@aws-sdk/credential-provider-process@3.972.75': - resolution: {integrity: sha512-ZNm8T/HJNhpS8MikDVAsym8QyUSUuOUGiCkmI3TTya7G6xhOgfRLd2iK91HhQQyvRM36eo6Xm845hHVyfToA2w==} + '@aws-sdk/credential-provider-process@3.972.76': + resolution: {integrity: sha512-pdhKFTXv1zPYhh92MTlp0jVbLWvUEV9v+rziuEkoskGGQ2iJskxHTO38nA8UAbT/N4IM3n8OZJ9rIY5IBO15/A==} engines: {node: '>=20.0.0'} - '@aws-sdk/credential-provider-sso@3.973.19': - resolution: {integrity: sha512-qHGDAGesyWtRw3BapyFkYSUzVNrC1CWPS6+S2lf/MLMEkWB2gR3Bl6imVHy1nhImXKS+gcuUDzbu39BnsHJc1Q==} + '@aws-sdk/credential-provider-sso@3.973.20': + resolution: {integrity: sha512-DlxrAB3MnjNLx/da7b89W+3DyJQYso/q7Ns15MsST2IV2nkAYQOYTUCaffNSjgrpMP99Dm2mV2jxzcbyF94ZfA==} engines: {node: '>=20.0.0'} - '@aws-sdk/credential-provider-web-identity@3.972.81': - resolution: {integrity: sha512-nHkgkMIC6NHgqfI6UAQq5cCS4bUkNxkT+r9fXCq/X4IKfX/DHGZ8UM3T4b78OXpWK5OlzEzN4kxwyax0W/MUCA==} + '@aws-sdk/credential-provider-web-identity@3.972.82': + resolution: {integrity: sha512-LUo46IqPTJ4xKBsrLuoaxiv/D2GTBCcFDMD1y3jKIqPR/p7Rw14RJFZ0iRWftqZtyLPQX9JMGnSi3mBu4y/pNA==} engines: {node: '>=20.0.0'} - '@aws-sdk/lib-storage@3.1153.0': - resolution: {integrity: sha512-zshL4tYVwTXfEOQ0iqNsXUPq/0aoZwOHrAsqlBmGcAij1+8qc79rm9LpzBuEV7KWiSMetZ+89n7mSYgcJKFgyA==} + '@aws-sdk/lib-storage@3.1155.0': + resolution: {integrity: sha512-ThvepKZILBOGCO1rsRsuAWps6hwfMfFVVhRpNrxup+o0q/Hyi50IaDSGZYxrZMPbWH/8pG2LFld//OYDmyr48A==} engines: {node: '>=20.0.0'} peerDependencies: - '@aws-sdk/client-s3': ^3.1153.0 + '@aws-sdk/client-s3': ^3.1155.0 - '@aws-sdk/middleware-sdk-s3@3.972.80': - resolution: {integrity: sha512-zX3lXN/9XOUAuGpxgt3wXwZrXhTjQYEgRsqa856PbZWpOtleUWmCMbhlkLfusPuhW24cOC/8JYR8hnRalrrIkQ==} + '@aws-sdk/middleware-sdk-s3@3.972.81': + resolution: {integrity: sha512-jfanv1pV0sJFZw6/DPiNjvK7B478god0zOum5yIWo+/+1Gy+CSXWpGBWx+3nBJyDvjcu/2kUMr4ADUp9BHH34Q==} engines: {node: '>=20.0.0'} - '@aws-sdk/nested-clients@3.997.49': - resolution: {integrity: sha512-zjqIqzTYLRKhT7TPJDUreK9vG9PjhmnfLCaTaqArkRT7j+Ogl0tuXB7MpSd8rlRfRT+xTOeyNFWaOfyNcf51hg==} + '@aws-sdk/nested-clients@3.997.50': + resolution: {integrity: sha512-nKb8qu4QsnpZTq/6JIXc72mzuzfh7BK4sUQkpQznPec2z4W7Ob2xYkcM4i5e1yGTBnbJVeFv/0sf3jcz85hclA==} engines: {node: '>=20.0.0'} - '@aws-sdk/signature-v4-multi-region@3.996.48': - resolution: {integrity: sha512-cFDXfEbXaGwbnpvR/DyqfxfjxpjXVRVthQLgkwvcfDS5u3+8G9WSj1X1Ag/pTRg/QOeOno1Tq3ycEdrKsCUcOA==} + '@aws-sdk/signature-v4-multi-region@3.996.49': + resolution: {integrity: sha512-nvuAZNRrRRVCiARRKsYlQ0VnXiEQiq7vXPH+UA+LSo+kN3Djl9eo/V2r0udkw85PnsNshmv2Em1clld5M3j67w==} engines: {node: '>=20.0.0'} - '@aws-sdk/token-providers@3.1153.0': - resolution: {integrity: sha512-6bKJ2OfLboxhzeJ3hBDeB8L6emCYf9DfdrjBAharLrY42J1RV/6jA6G8jvfNghzID8Yb40BdSY7jQMtWx9Ot0Q==} + '@aws-sdk/token-providers@3.1155.0': + resolution: {integrity: sha512-iIcQnYLhWuq9qOLQbIWFrxF3PF0svVeZvHnUS4DUnO4zUFmFHIyGyjqrntXG+e/5PUgfavV0pYfDIKHxTzN2LA==} engines: {node: '>=20.0.0'} - '@aws-sdk/types@3.974.6': - resolution: {integrity: sha512-v/clNZzZnDxGyvpHMOGpJKVXFAExJzUNAAjaWGdcx8QAcXLGwTaOkw33p5SHAi0YAioK32xB3hWwOekRVfmfKg==} + '@aws-sdk/types@3.975.0': + resolution: {integrity: sha512-WQlhs6juByzWOzj5fwxOx0mPJaEqMcG/sDpl3Id61B3XhKE63xY+0QWcnG+sWR/sCfprmreyUo1lM7IKZmHMzw==} engines: {node: '>=20.0.0'} '@aws-sdk/xml-builder@3.972.41': @@ -1237,79 +1237,79 @@ packages: resolution: {integrity: sha512-VVPPgHyQ6ShqnrmDWuxjmUIsO9gWyOZFmuOfLd9LfBGQJwZfy0gvv9pbHSJuoFNIYC7ZDX9aoFwowjcdSC4E8w==} engines: {node: '>=18'} - '@napi-rs/canvas-android-arm64@1.0.11': - resolution: {integrity: sha512-dtJS4FQEfH9OwSYLtMrEnj7T8DzBbJc/97LI/OkiWOYpW/iW5t6R/gcxVrUXS8l45gCVpwMufgucRe/pe0ISqA==} + '@napi-rs/canvas-android-arm64@1.0.13': + resolution: {integrity: sha512-trn7AaDzQ+yeeA0ezhkC7cB1ZDPtp4UYk0EvB5F8qtYL9jEVWtVrU30WOpI52DszoTxaTsCgf5PP4ZVaOlOIHQ==} engines: {node: '>= 10'} cpu: [arm64] os: [android] - '@napi-rs/canvas-darwin-arm64@1.0.11': - resolution: {integrity: sha512-J/JylWoJTG8OQqZaiqly4IODyAxNofUlZo0wtSBJtHk6AdymbdIkPynAM1JvQaZpPaG5h+msk9QuK8y4i1ooqw==} + '@napi-rs/canvas-darwin-arm64@1.0.13': + resolution: {integrity: sha512-6Y2djEetL8copxfNK6hg7oig/xqTwOchv19hBOm+zBNdKc/02Z3kjZN5wrLzv/l5O04/bAQS6I/AlQpwBlXgPg==} engines: {node: '>= 10'} cpu: [arm64] os: [darwin] - '@napi-rs/canvas-darwin-x64@1.0.11': - resolution: {integrity: sha512-nlecCk3xy06eAGC7krZtzNDm9UNTHKuHQYK8ZYegdjzgSL8anGt/eLJZm86OWaNgZkwQwvDqfSmG8Nqe6MPZNQ==} + '@napi-rs/canvas-darwin-x64@1.0.13': + resolution: {integrity: sha512-OZoPwwu4TMMg8GTCFBrg8UFExwQTU/55i3uTC/izDat6PHE+ro9dimyQoOboFf91+NY7Pnn70ScR8f8I8+eZXw==} engines: {node: '>= 10'} cpu: [x64] os: [darwin] - '@napi-rs/canvas-linux-arm-gnueabihf@1.0.11': - resolution: {integrity: sha512-DKImmG+sTPolE0kGPsYPBxTtOjb3INALp32O2lW2CfSr0bAzSEKKuu+GrQbtilClyoPOahUErS3OrznFDraoWA==} + '@napi-rs/canvas-linux-arm-gnueabihf@1.0.13': + resolution: {integrity: sha512-dZSYaKROuswU2eUWG5aBKndNVe68elo1YX93xtRax7q658tj8rYE+HwduovWJgs6qwv1r1/CLR60nDeBbyjvyg==} engines: {node: '>= 10'} cpu: [arm] os: [linux] - '@napi-rs/canvas-linux-arm64-gnu@1.0.11': - resolution: {integrity: sha512-55sykACGENj5j1rhj+81AO8JbR9HYgGjRZ7/H+pNr4VE1DvINKYNMTcutk8ZmWxk276jAQvUKJ3+Kdj9GXU3lQ==} + '@napi-rs/canvas-linux-arm64-gnu@1.0.13': + resolution: {integrity: sha512-W6FzYE8ADHI93In1vi80KBL6btDtAMuj50Ch5ptkWS8xuntyzbR4h5JDUmXoakxZn2yMInksudTEqVSWoFqFGg==} engines: {node: '>= 10'} cpu: [arm64] os: [linux] libc: [glibc] - '@napi-rs/canvas-linux-arm64-musl@1.0.11': - resolution: {integrity: sha512-6Jz6jys17yXydO+WdJgu7JhoB4K7/w9sWHByzzTmLjQghUx3TDMWvLv+UcCdnWhg4oprET67EMymnrETdeGNAg==} + '@napi-rs/canvas-linux-arm64-musl@1.0.13': + resolution: {integrity: sha512-+6fQ+rR49hkAa4voh2stLIwQ+4t+gmoseM8Bc53/DlMXb73IKymDPWK9A/NtWeqTW4ijWppYuyuxS2hPI8tzNA==} engines: {node: '>= 10'} cpu: [arm64] os: [linux] libc: [musl] - '@napi-rs/canvas-linux-riscv64-gnu@1.0.11': - resolution: {integrity: sha512-lf2XeEebWiUYVOVEjLXxF7dfXrBBSEOBn9AqBh39VYXrPt5cDqI+1t58IFSjn8npqkg5jIOs8TQbjKABbFW1gQ==} + '@napi-rs/canvas-linux-riscv64-gnu@1.0.13': + resolution: {integrity: sha512-17XEMO3t9RW/TtHtSFf2AkaSWuCWdzbPeidz7zrOFawHxoLSJCnpTI1hN5aUvrZNnkNNmt/vZVJCh0ryUaik3A==} engines: {node: '>= 10'} cpu: [riscv64] os: [linux] libc: [glibc] - '@napi-rs/canvas-linux-x64-gnu@1.0.11': - resolution: {integrity: sha512-iDapw7nFSE5tsmQxcH2+KotJhyfVPmHd6vUOQN/Em+8WNbkZJwJ/rDowlJMFnMl0SYSDSfgKCNUGi6Sn5++Smg==} + '@napi-rs/canvas-linux-x64-gnu@1.0.13': + resolution: {integrity: sha512-lc48IwknVHBsNzXn9Xs5OkUsRX38Ml3Z1aOybQ51mMEzZckm8+4A0qROlyYHEoBPzr1kL3IU8ZUAqDb7wtdX8w==} engines: {node: '>= 10'} cpu: [x64] os: [linux] libc: [glibc] - '@napi-rs/canvas-linux-x64-musl@1.0.11': - resolution: {integrity: sha512-26q26AyITo3CGZX4HWFIMgBMA1lu3YN07gxzBm4tW6Hglq/OVyTP50kDrmWuKybCX6dh3m8Ly+c7YhI+sqCgCQ==} + '@napi-rs/canvas-linux-x64-musl@1.0.13': + resolution: {integrity: sha512-bGY1r26S2bs8UNFJcPWcCSNdCFbKep5PATsvtgtC9A5u6SRgr2/jJE/RlhHL5m9ZpixNDhVkLY/5h16Kx5V9YA==} engines: {node: '>= 10'} cpu: [x64] os: [linux] libc: [musl] - '@napi-rs/canvas-win32-arm64-msvc@1.0.11': - resolution: {integrity: sha512-cUO96IdsfvIj2S+2PNcDSkTcLhjPj8pIOEWRV1Kj70MOP4wnE9zSk7N4C1EKHlFGRcgdR5AkbnBZj0dfHK9nbQ==} + '@napi-rs/canvas-win32-arm64-msvc@1.0.13': + resolution: {integrity: sha512-2vRPBZoaIvJrUkDhZTE2Hn0YrnpJRCWVM3EJACoBKjuWI6WgPxGtUoRMsxRviCsyks8bKTSFdyw14CellnG2qA==} engines: {node: '>= 10'} cpu: [arm64] os: [win32] - '@napi-rs/canvas-win32-x64-msvc@1.0.11': - resolution: {integrity: sha512-DFYp/ZQRdKunI/LPc0UkRXG1vlNjJKOJf8O8Ctb8+wm2Nt2ELg9Eamc31D2VoakkfGo8L+z0Iejz6WtFbQwyGw==} + '@napi-rs/canvas-win32-x64-msvc@1.0.13': + resolution: {integrity: sha512-DUlW3lZPz63kYsf8mxgyyZuevhlH0T3/BGP9l+NazhB0F1+gL6D4hqZdnhZpw24DlWmTpATDcyYSZlJOi+DdfQ==} engines: {node: '>= 10'} cpu: [x64] os: [win32] - '@napi-rs/canvas@1.0.11': - resolution: {integrity: sha512-vt5tog1cr+QKadIddGe+3LrcX0EmjEd0izF717Uwf92mBmgYfq2XRmL2r4bZU40spB69exxhrBn9I91HYEr/7w==} + '@napi-rs/canvas@1.0.13': + resolution: {integrity: sha512-b87ugZpQ5ViwqVBkjervxpwa0GmJPsKHLBNoZN6GpjWeHzTbDUGryriq+2DYW2XbB+xCqSIINQysC0/I87nPbw==} engines: {node: '>= 10'} '@napi-rs/wasm-runtime@1.2.5': @@ -2796,6 +2796,10 @@ packages: resolution: {integrity: sha512-3hJiwG1F5t1SoXqp/yOCmYXziyx7ZVqF63S5W6YU201e/qwVaHPex+E/3vvyY9a3ORq/b+BydeNZpP2saQ9X2g==} engines: {node: '>=18.0.0'} + '@smithy/core@3.36.2': + resolution: {integrity: sha512-oRw0rB9QSUiCZfEs23G5F4Af+b2rQt1lItIICWu5IejFNd+uPM8SWqiKLMpQ6bX1Q7slsLbjuoE2niErFtkVPQ==} + engines: {node: '>=18.0.0'} + '@smithy/credential-provider-imds@4.5.2': resolution: {integrity: sha512-A9uSdn72ozbRUSit0eib0TW7nXuNPlaeM0zcGkJ+nE6tFcSDbnmtwoxbTCFBukVQcszDAyvsd7+rTduPTXpygg==} engines: {node: '>=18.0.0'} @@ -2952,14 +2956,14 @@ packages: '@types/react': optional: true - '@tanstack/react-virtual@3.14.21': - resolution: {integrity: sha512-wWE/ZJDNNDp18JHWy7n5PQooqerHePVkEEu6Op0twKQ2InF9DPhYMQptQY0syDEviMd3/r8D0OUvKJibDGm9HQ==} + '@tanstack/react-virtual@3.14.22': + resolution: {integrity: sha512-zNiB9IF3prySTfAUrj9tPHjD/lonBYkfxrPP4v4ITUx+qwHajJR5cY/xirjvwc+KWkur10GX3TpsbKh7JkD8Lw==} peerDependencies: react: ^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0 react-dom: ^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0 - '@tanstack/virtual-core@3.19.6': - resolution: {integrity: sha512-CsTW892z23gy7IyxYTTorVu1uZWB+0N349rSMgKz4rFLPpLza7F7TpcCKeI5iUAwYQCt8QUlOU67J+KXCxtpGw==} + '@tanstack/virtual-core@3.19.7': + resolution: {integrity: sha512-gxy5SvH0mFRJ0YujtrflB/1HAu9Wn1KGvL0bjZfQkYVVMsIpQiwcf1mR06/ViN4NjllUb4HNCjqE9FGwgTvMCQ==} '@tesseract.js-data/deu@1.0.0': resolution: {integrity: sha512-3ter1p2gFAiCd5CMC+PodZ9YY2UDomkKj+ytlKeeZqOUmhLx1SDnYPf5zog0g5MBbxYveW+8omI3PUflqHfZ4Q==} @@ -5040,8 +5044,8 @@ packages: resolution: {integrity: sha512-AC/7JofJvZGrrneWNaEnJeOLUx+JlGt7tNa0wZiRPT4MY1wmfKjt2+6O2p2uz2+skll8OZZmJMNqeke7kKbNgQ==} hasBin: true - jose@6.2.12: - resolution: {integrity: sha512-9NiFmJEex0sy2Dk58j2UGBSHgUs2ypF9eZSu4L6vjOX3Dp96Sw1F3uL+H+D1sx02jZZdzUT0HgvCy59CuvXcWw==} + jose@6.2.13: + resolution: {integrity: sha512-7TIg10ZE1x82YediN+lcrPNvGMKge9yFHQD+Qho/b8y2adfugU3AsvvHfxV0fzyFllT6l9xCjf4RWY9g/Z9PJQ==} js-md4@0.3.2: resolution: {integrity: sha512-/GDnfQYsltsjRswQhN9fhv3EMw2sCpUdrdxyWDOUK7eyD++r3gRhzgiQgc/x4MAv2i1iuQ4lxO5mvqM3vj4bwA==} @@ -5420,8 +5424,8 @@ packages: resolution: {integrity: sha512-5J9ysMYUpYIg9RF2vJpy9SinEmSviFSe0GyPpCQ4L5QSkLAgeLXlTAOu2ZwWUU5m+0SBl6gUU1R1ZQB3aKypfA==} engines: {bun: '>=1.0.0', deno: '>=1.30.0', node: '>=8.0.0'} - lucide-react@1.59.0: - resolution: {integrity: sha512-5qgMyyCPU6OvnKpLS2t693+ZtiqA8oaSweWoUows0BLFKFf6V75m4+CptpmohZHpD5IuOVIBXHBBiv2vpLJmtw==} + lucide-react@1.61.0: + resolution: {integrity: sha512-v2SQPPPNjnr+BlhL9Spuzmo5yR+hLkftfuE4E3jaxdNPgSH0hJKPcLJBP7yAta87jQNPsSERS59PJjCJ/CorBA==} peerDependencies: '@types/react': '*' react: ^16.5.1 || ^17.0.0 || ^18.0.0 || ^19.0.0 @@ -5633,8 +5637,8 @@ packages: resolution: {integrity: sha512-BdiKAgbfRlvfMq/XzO/AF9NPmaA8a+YpLAyAFTVFBj3gARrAIfWvNq7Q7NE4se3KgFu0KkrvQMxTS0XVnsGzHg==} engines: {node: '>=18'} - nodemailer@10.1.7: - resolution: {integrity: sha512-OQa6FDln5fLttmqSbjDiRYvFDOFNrBqfiOwHPlEbURil3b+vPB1v8xpi+Sp46drzNDlsTnNsS502vthmz56r4w==} + nodemailer@10.2.1: + resolution: {integrity: sha512-ZrDNsPFoDMwefNs7sJwrYiEJ/CG4ClJWDZv05iNJsEBWa8GRDry1QJHTJxsuTAY9eDDKjX4k+q3SxslXJOao7w==} engines: {node: '>=20.0.0'} non-error@0.1.0: @@ -7268,170 +7272,170 @@ snapshots: '@alloc/quick-lru@5.2.0': {} - '@aws-sdk/checksums@3.1001.4': + '@aws-sdk/checksums@3.1001.5': dependencies: - '@aws-sdk/core': 3.978.4 - '@aws-sdk/types': 3.974.6 - '@smithy/core': 3.36.0 + '@aws-sdk/core': 3.978.5 + '@aws-sdk/types': 3.975.0 + '@smithy/core': 3.36.2 '@smithy/types': 4.19.0 tslib: 2.8.1 - '@aws-sdk/client-s3@3.1153.0': + '@aws-sdk/client-s3@3.1156.0': dependencies: - '@aws-sdk/checksums': 3.1001.4 - '@aws-sdk/core': 3.978.4 - '@aws-sdk/credential-provider-node': 3.972.87 - '@aws-sdk/middleware-sdk-s3': 3.972.80 - '@aws-sdk/signature-v4-multi-region': 3.996.48 - '@aws-sdk/types': 3.974.6 - '@smithy/core': 3.36.0 + '@aws-sdk/checksums': 3.1001.5 + '@aws-sdk/core': 3.978.5 + '@aws-sdk/credential-provider-node': 3.972.88 + '@aws-sdk/middleware-sdk-s3': 3.972.81 + '@aws-sdk/signature-v4-multi-region': 3.996.49 + '@aws-sdk/types': 3.975.0 + '@smithy/core': 3.36.2 '@smithy/fetch-http-handler': 5.8.0 '@smithy/node-http-handler': 4.12.1 '@smithy/types': 4.19.0 tslib: 2.8.1 - '@aws-sdk/core@3.978.4': + '@aws-sdk/core@3.978.5': dependencies: - '@aws-sdk/types': 3.974.6 + '@aws-sdk/types': 3.975.0 '@aws-sdk/xml-builder': 3.972.41 '@aws/lambda-invoke-store': 0.3.0 - '@smithy/core': 3.36.0 + '@smithy/core': 3.36.2 '@smithy/signature-v4': 5.7.4 '@smithy/types': 4.19.0 bowser: 2.14.1 tslib: 2.8.1 - '@aws-sdk/credential-provider-env@3.972.75': + '@aws-sdk/credential-provider-env@3.972.76': dependencies: - '@aws-sdk/core': 3.978.4 - '@aws-sdk/types': 3.974.6 - '@smithy/core': 3.36.0 + '@aws-sdk/core': 3.978.5 + '@aws-sdk/types': 3.975.0 + '@smithy/core': 3.36.2 '@smithy/types': 4.19.0 tslib: 2.8.1 - '@aws-sdk/credential-provider-http@3.972.77': + '@aws-sdk/credential-provider-http@3.972.78': dependencies: - '@aws-sdk/core': 3.978.4 - '@aws-sdk/types': 3.974.6 - '@smithy/core': 3.36.0 + '@aws-sdk/core': 3.978.5 + '@aws-sdk/types': 3.975.0 + '@smithy/core': 3.36.2 '@smithy/fetch-http-handler': 5.8.0 '@smithy/node-http-handler': 4.12.1 '@smithy/types': 4.19.0 tslib: 2.8.1 - '@aws-sdk/credential-provider-ini@3.973.20': + '@aws-sdk/credential-provider-ini@3.973.21': dependencies: - '@aws-sdk/core': 3.978.4 - '@aws-sdk/credential-provider-env': 3.972.75 - '@aws-sdk/credential-provider-http': 3.972.77 - '@aws-sdk/credential-provider-login': 3.972.82 - '@aws-sdk/credential-provider-process': 3.972.75 - '@aws-sdk/credential-provider-sso': 3.973.19 - '@aws-sdk/credential-provider-web-identity': 3.972.81 - '@aws-sdk/nested-clients': 3.997.49 - '@aws-sdk/types': 3.974.6 - '@smithy/core': 3.36.0 + '@aws-sdk/core': 3.978.5 + '@aws-sdk/credential-provider-env': 3.972.76 + '@aws-sdk/credential-provider-http': 3.972.78 + '@aws-sdk/credential-provider-login': 3.972.83 + '@aws-sdk/credential-provider-process': 3.972.76 + '@aws-sdk/credential-provider-sso': 3.973.20 + '@aws-sdk/credential-provider-web-identity': 3.972.82 + '@aws-sdk/nested-clients': 3.997.50 + '@aws-sdk/types': 3.975.0 + '@smithy/core': 3.36.2 '@smithy/credential-provider-imds': 4.5.2 '@smithy/types': 4.19.0 tslib: 2.8.1 - '@aws-sdk/credential-provider-login@3.972.82': + '@aws-sdk/credential-provider-login@3.972.83': dependencies: - '@aws-sdk/core': 3.978.4 - '@aws-sdk/nested-clients': 3.997.49 - '@aws-sdk/types': 3.974.6 - '@smithy/core': 3.36.0 + '@aws-sdk/core': 3.978.5 + '@aws-sdk/nested-clients': 3.997.50 + '@aws-sdk/types': 3.975.0 + '@smithy/core': 3.36.2 '@smithy/types': 4.19.0 tslib: 2.8.1 - '@aws-sdk/credential-provider-node@3.972.87': + '@aws-sdk/credential-provider-node@3.972.88': dependencies: - '@aws-sdk/credential-provider-env': 3.972.75 - '@aws-sdk/credential-provider-http': 3.972.77 - '@aws-sdk/credential-provider-ini': 3.973.20 - '@aws-sdk/credential-provider-process': 3.972.75 - '@aws-sdk/credential-provider-sso': 3.973.19 - '@aws-sdk/credential-provider-web-identity': 3.972.81 - '@aws-sdk/types': 3.974.6 - '@smithy/core': 3.36.0 + '@aws-sdk/credential-provider-env': 3.972.76 + '@aws-sdk/credential-provider-http': 3.972.78 + '@aws-sdk/credential-provider-ini': 3.973.21 + '@aws-sdk/credential-provider-process': 3.972.76 + '@aws-sdk/credential-provider-sso': 3.973.20 + '@aws-sdk/credential-provider-web-identity': 3.972.82 + '@aws-sdk/types': 3.975.0 + '@smithy/core': 3.36.2 '@smithy/credential-provider-imds': 4.5.2 '@smithy/types': 4.19.0 tslib: 2.8.1 - '@aws-sdk/credential-provider-process@3.972.75': + '@aws-sdk/credential-provider-process@3.972.76': dependencies: - '@aws-sdk/core': 3.978.4 - '@aws-sdk/types': 3.974.6 - '@smithy/core': 3.36.0 + '@aws-sdk/core': 3.978.5 + '@aws-sdk/types': 3.975.0 + '@smithy/core': 3.36.2 '@smithy/types': 4.19.0 tslib: 2.8.1 - '@aws-sdk/credential-provider-sso@3.973.19': + '@aws-sdk/credential-provider-sso@3.973.20': dependencies: - '@aws-sdk/core': 3.978.4 - '@aws-sdk/nested-clients': 3.997.49 - '@aws-sdk/token-providers': 3.1153.0 - '@aws-sdk/types': 3.974.6 - '@smithy/core': 3.36.0 + '@aws-sdk/core': 3.978.5 + '@aws-sdk/nested-clients': 3.997.50 + '@aws-sdk/token-providers': 3.1155.0 + '@aws-sdk/types': 3.975.0 + '@smithy/core': 3.36.2 '@smithy/types': 4.19.0 tslib: 2.8.1 - '@aws-sdk/credential-provider-web-identity@3.972.81': + '@aws-sdk/credential-provider-web-identity@3.972.82': dependencies: - '@aws-sdk/core': 3.978.4 - '@aws-sdk/nested-clients': 3.997.49 - '@aws-sdk/types': 3.974.6 - '@smithy/core': 3.36.0 + '@aws-sdk/core': 3.978.5 + '@aws-sdk/nested-clients': 3.997.50 + '@aws-sdk/types': 3.975.0 + '@smithy/core': 3.36.2 '@smithy/types': 4.19.0 tslib: 2.8.1 - '@aws-sdk/lib-storage@3.1153.0(@aws-sdk/client-s3@3.1153.0)': + '@aws-sdk/lib-storage@3.1155.0(@aws-sdk/client-s3@3.1156.0)': dependencies: - '@aws-sdk/client-s3': 3.1153.0 - '@smithy/core': 3.36.0 + '@aws-sdk/client-s3': 3.1156.0 + '@smithy/core': 3.36.2 '@smithy/types': 4.19.0 buffer: 5.6.0 events: 3.3.0 stream-browserify: 3.0.0 tslib: 2.8.1 - '@aws-sdk/middleware-sdk-s3@3.972.80': + '@aws-sdk/middleware-sdk-s3@3.972.81': dependencies: - '@aws-sdk/core': 3.978.4 - '@aws-sdk/signature-v4-multi-region': 3.996.48 - '@aws-sdk/types': 3.974.6 - '@smithy/core': 3.36.0 + '@aws-sdk/core': 3.978.5 + '@aws-sdk/signature-v4-multi-region': 3.996.49 + '@aws-sdk/types': 3.975.0 + '@smithy/core': 3.36.2 '@smithy/types': 4.19.0 tslib: 2.8.1 - '@aws-sdk/nested-clients@3.997.49': + '@aws-sdk/nested-clients@3.997.50': dependencies: - '@aws-sdk/core': 3.978.4 - '@aws-sdk/signature-v4-multi-region': 3.996.48 - '@aws-sdk/types': 3.974.6 - '@smithy/core': 3.36.0 + '@aws-sdk/core': 3.978.5 + '@aws-sdk/signature-v4-multi-region': 3.996.49 + '@aws-sdk/types': 3.975.0 + '@smithy/core': 3.36.2 '@smithy/fetch-http-handler': 5.8.0 '@smithy/node-http-handler': 4.12.1 '@smithy/types': 4.19.0 tslib: 2.8.1 - '@aws-sdk/signature-v4-multi-region@3.996.48': + '@aws-sdk/signature-v4-multi-region@3.996.49': dependencies: - '@aws-sdk/types': 3.974.6 + '@aws-sdk/types': 3.975.0 '@smithy/signature-v4': 5.7.4 '@smithy/types': 4.19.0 tslib: 2.8.1 - '@aws-sdk/token-providers@3.1153.0': + '@aws-sdk/token-providers@3.1155.0': dependencies: - '@aws-sdk/core': 3.978.4 - '@aws-sdk/nested-clients': 3.997.49 - '@aws-sdk/types': 3.974.6 - '@smithy/core': 3.36.0 + '@aws-sdk/core': 3.978.5 + '@aws-sdk/nested-clients': 3.997.50 + '@aws-sdk/types': 3.975.0 + '@smithy/core': 3.36.2 '@smithy/types': 4.19.0 tslib: 2.8.1 - '@aws-sdk/types@3.974.6': + '@aws-sdk/types@3.975.0': dependencies: '@smithy/types': 4.19.0 tslib: 2.8.1 @@ -8368,7 +8372,7 @@ snapshots: express: 5.2.1(supports-color@7.2.0) express-rate-limit: 8.6.2(express@5.2.1(supports-color@7.2.0))(supports-color@7.2.0) hono: 4.13.13 - jose: 6.2.12 + jose: 6.2.13 json-schema-typed: 8.0.2 pkce-challenge: 5.0.1 raw-body: 3.0.2 @@ -8389,52 +8393,52 @@ snapshots: strict-event-emitter: 0.5.1 optional: true - '@napi-rs/canvas-android-arm64@1.0.11': + '@napi-rs/canvas-android-arm64@1.0.13': optional: true - '@napi-rs/canvas-darwin-arm64@1.0.11': + '@napi-rs/canvas-darwin-arm64@1.0.13': optional: true - '@napi-rs/canvas-darwin-x64@1.0.11': + '@napi-rs/canvas-darwin-x64@1.0.13': optional: true - '@napi-rs/canvas-linux-arm-gnueabihf@1.0.11': + '@napi-rs/canvas-linux-arm-gnueabihf@1.0.13': optional: true - '@napi-rs/canvas-linux-arm64-gnu@1.0.11': + '@napi-rs/canvas-linux-arm64-gnu@1.0.13': optional: true - '@napi-rs/canvas-linux-arm64-musl@1.0.11': + '@napi-rs/canvas-linux-arm64-musl@1.0.13': optional: true - '@napi-rs/canvas-linux-riscv64-gnu@1.0.11': + '@napi-rs/canvas-linux-riscv64-gnu@1.0.13': optional: true - '@napi-rs/canvas-linux-x64-gnu@1.0.11': + '@napi-rs/canvas-linux-x64-gnu@1.0.13': optional: true - '@napi-rs/canvas-linux-x64-musl@1.0.11': + '@napi-rs/canvas-linux-x64-musl@1.0.13': optional: true - '@napi-rs/canvas-win32-arm64-msvc@1.0.11': + '@napi-rs/canvas-win32-arm64-msvc@1.0.13': optional: true - '@napi-rs/canvas-win32-x64-msvc@1.0.11': + '@napi-rs/canvas-win32-x64-msvc@1.0.13': optional: true - '@napi-rs/canvas@1.0.11': + '@napi-rs/canvas@1.0.13': optionalDependencies: - '@napi-rs/canvas-android-arm64': 1.0.11 - '@napi-rs/canvas-darwin-arm64': 1.0.11 - '@napi-rs/canvas-darwin-x64': 1.0.11 - '@napi-rs/canvas-linux-arm-gnueabihf': 1.0.11 - '@napi-rs/canvas-linux-arm64-gnu': 1.0.11 - '@napi-rs/canvas-linux-arm64-musl': 1.0.11 - '@napi-rs/canvas-linux-riscv64-gnu': 1.0.11 - '@napi-rs/canvas-linux-x64-gnu': 1.0.11 - '@napi-rs/canvas-linux-x64-musl': 1.0.11 - '@napi-rs/canvas-win32-arm64-msvc': 1.0.11 - '@napi-rs/canvas-win32-x64-msvc': 1.0.11 + '@napi-rs/canvas-android-arm64': 1.0.13 + '@napi-rs/canvas-darwin-arm64': 1.0.13 + '@napi-rs/canvas-darwin-x64': 1.0.13 + '@napi-rs/canvas-linux-arm-gnueabihf': 1.0.13 + '@napi-rs/canvas-linux-arm64-gnu': 1.0.13 + '@napi-rs/canvas-linux-arm64-musl': 1.0.13 + '@napi-rs/canvas-linux-riscv64-gnu': 1.0.13 + '@napi-rs/canvas-linux-x64-gnu': 1.0.13 + '@napi-rs/canvas-linux-x64-musl': 1.0.13 + '@napi-rs/canvas-win32-arm64-msvc': 1.0.13 + '@napi-rs/canvas-win32-x64-msvc': 1.0.13 '@napi-rs/wasm-runtime@1.2.5(@emnapi/core@1.10.0)(@emnapi/runtime@1.10.0)': dependencies: @@ -9892,6 +9896,11 @@ snapshots: '@smithy/types': 4.19.0 tslib: 2.8.1 + '@smithy/core@3.36.2': + dependencies: + '@smithy/types': 4.19.0 + tslib: 2.8.1 + '@smithy/credential-provider-imds@4.5.2': dependencies: '@smithy/core': 3.36.0 @@ -10072,13 +10081,13 @@ snapshots: optionalDependencies: '@types/react': 19.3.0 - '@tanstack/react-virtual@3.14.21(react-dom@19.3.1(react@19.3.1))(react@19.3.1)': + '@tanstack/react-virtual@3.14.22(react-dom@19.3.1(react@19.3.1))(react@19.3.1)': dependencies: - '@tanstack/virtual-core': 3.19.6 + '@tanstack/virtual-core': 3.19.7 react: 19.3.1 react-dom: 19.3.1(react@19.3.1) - '@tanstack/virtual-core@3.19.6': {} + '@tanstack/virtual-core@3.19.7': {} '@tesseract.js-data/deu@1.0.0': {} @@ -12354,7 +12363,7 @@ snapshots: jiti@2.7.0: {} - jose@6.2.12: {} + jose@6.2.13: {} js-md4@0.3.2: {} @@ -12677,7 +12686,7 @@ snapshots: lru.min@1.1.5: {} - lucide-react@1.59.0(@types/react@19.3.0)(react@19.3.1): + lucide-react@1.61.0(@types/react@19.3.0)(react@19.3.1): dependencies: react: 19.3.1 optionalDependencies: @@ -12868,7 +12877,7 @@ snapshots: node-releases@2.0.58: {} - nodemailer@10.1.7: {} + nodemailer@10.2.1: {} non-error@0.1.0: {} @@ -13113,7 +13122,7 @@ snapshots: pdfjs-dist@6.4.299: optionalDependencies: - '@napi-rs/canvas': 1.0.11 + '@napi-rs/canvas': 1.0.13 perfect-debounce@2.1.0: {} From 8783182b1e78a69a23bdc3491afd375b297183ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 25 Oct 2026 17:06:17 +0100 Subject: [PATCH 05/13] build(deps): keep @aws-sdk/client-s3 on the same release as lib-storage --- package.json | 2 +- pnpm-lock.yaml | 33 ++++++++++++--------------------- 2 files changed, 13 insertions(+), 22 deletions(-) diff --git a/package.json b/package.json index 53c67faa9..e372d5129 100644 --- a/package.json +++ b/package.json @@ -54,7 +54,7 @@ "mcp:stdio": "npx tsx src/cli/mcp-stdio.ts" }, "dependencies": { - "@aws-sdk/client-s3": "^3.1156.0", + "@aws-sdk/client-s3": "^3.1155.0", "@aws-sdk/lib-storage": "3.1155.0", "@dnd-kit/core": "^6.3.1", "@dnd-kit/sortable": "^10.0.0", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index f42434e67..81124e808 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -45,11 +45,11 @@ importers: .: dependencies: '@aws-sdk/client-s3': - specifier: ^3.1156.0 - version: 3.1156.0 + specifier: ^3.1155.0 + version: 3.1155.0 '@aws-sdk/lib-storage': specifier: 3.1155.0 - version: 3.1155.0(@aws-sdk/client-s3@3.1156.0) + version: 3.1155.0(@aws-sdk/client-s3@3.1155.0) '@dnd-kit/core': specifier: ^6.3.1 version: 6.3.1(react-dom@19.3.1(react@19.3.1))(react@19.3.1) @@ -305,8 +305,8 @@ packages: resolution: {integrity: sha512-6erto9nK2OZO/woEgmFIaPh5gChgdNd7nL6jgAf8TAXz3/QHC8Pan2TFe9GYp3IxOIKCCGWPylSjtOHKfNzQlw==} engines: {node: '>=20.0.0'} - '@aws-sdk/client-s3@3.1156.0': - resolution: {integrity: sha512-Z2PqQqKMDVX56tNZ6ggmjoI5o/8+4+8ArmjLwE5ktYsEzs1BrvklBT1ZzI+s0Vcf3wfPguoEMD7EkMswmEta/A==} + '@aws-sdk/client-s3@3.1155.0': + resolution: {integrity: sha512-l3uMeIlzWVgj/RTJTh5sKYKtQz1la3cvWLUPn1cwFH4UZpcqTH66SgJHCeB2FOE9bEAjOdbmCyPKt9aw867Fng==} engines: {node: '>=20.0.0'} '@aws-sdk/core@3.978.5': @@ -2792,10 +2792,6 @@ packages: resolution: {integrity: sha512-tlqY9xq5ukxTUZBmoOp+m61cqwQD5pHJtFY3Mn8CA8ps6yghLH/Hw8UPdqg4OLmFW3IFlcXnQNmo/dh8HzXYIQ==} engines: {node: '>=18'} - '@smithy/core@3.36.0': - resolution: {integrity: sha512-3hJiwG1F5t1SoXqp/yOCmYXziyx7ZVqF63S5W6YU201e/qwVaHPex+E/3vvyY9a3ORq/b+BydeNZpP2saQ9X2g==} - engines: {node: '>=18.0.0'} - '@smithy/core@3.36.2': resolution: {integrity: sha512-oRw0rB9QSUiCZfEs23G5F4Af+b2rQt1lItIICWu5IejFNd+uPM8SWqiKLMpQ6bX1Q7slsLbjuoE2niErFtkVPQ==} engines: {node: '>=18.0.0'} @@ -7280,7 +7276,7 @@ snapshots: '@smithy/types': 4.19.0 tslib: 2.8.1 - '@aws-sdk/client-s3@3.1156.0': + '@aws-sdk/client-s3@3.1155.0': dependencies: '@aws-sdk/checksums': 3.1001.5 '@aws-sdk/core': 3.978.5 @@ -7389,9 +7385,9 @@ snapshots: '@smithy/types': 4.19.0 tslib: 2.8.1 - '@aws-sdk/lib-storage@3.1155.0(@aws-sdk/client-s3@3.1156.0)': + '@aws-sdk/lib-storage@3.1155.0(@aws-sdk/client-s3@3.1155.0)': dependencies: - '@aws-sdk/client-s3': 3.1156.0 + '@aws-sdk/client-s3': 3.1155.0 '@smithy/core': 3.36.2 '@smithy/types': 4.19.0 buffer: 5.6.0 @@ -9891,11 +9887,6 @@ snapshots: '@sindresorhus/merge-streams@4.0.0': {} - '@smithy/core@3.36.0': - dependencies: - '@smithy/types': 4.19.0 - tslib: 2.8.1 - '@smithy/core@3.36.2': dependencies: '@smithy/types': 4.19.0 @@ -9903,25 +9894,25 @@ snapshots: '@smithy/credential-provider-imds@4.5.2': dependencies: - '@smithy/core': 3.36.0 + '@smithy/core': 3.36.2 '@smithy/types': 4.19.0 tslib: 2.8.1 '@smithy/fetch-http-handler@5.8.0': dependencies: - '@smithy/core': 3.36.0 + '@smithy/core': 3.36.2 '@smithy/types': 4.19.0 tslib: 2.8.1 '@smithy/node-http-handler@4.12.1': dependencies: - '@smithy/core': 3.36.0 + '@smithy/core': 3.36.2 '@smithy/types': 4.19.0 tslib: 2.8.1 '@smithy/signature-v4@5.7.4': dependencies: - '@smithy/core': 3.36.0 + '@smithy/core': 3.36.2 '@smithy/types': 4.19.0 tslib: 2.8.1 From 4d970561faa8890e6135024da1415d41451caefd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 25 Oct 2026 17:06:52 +0100 Subject: [PATCH 06/13] fix(documents): decode thumbnails in the reader child, share it fairly, judge exits by cause Three defects in the bounded document reader. Image uploads were decoded into their thumbnails in the server process. A header check is all that stood between a decoder and an allocation the file chooses, and the GIF check showed how that fails. The image is now decoded in the same child process PDFs run in, under its memory ceiling and time limit; the header check stays in front of it. The reader's one slot went to whoever asked first, and a waiting item waited without end, so a few heavy PDFs from one account held everyone's lab scans behind them. The slot now goes to a read a person is waiting on before background work, to accounts in turn, to a few waiting items per account, and an item that waits past its lane's deadline is answered busy, which callers treat as retryable. Any child that ended without an answer counted as a document past its limits, so a kernel kill caused by the container's overall memory marked a good PDF as permanently failed. Only a bound the reader measured (the sampled memory, the time limit, the child's heap cap) is final now; any other end is run once more and then answered interrupted, which the thumbnail job retries within its ordinary attempts. --- next.config.ts | 8 +- .../documents/__tests__/image-engine.test.ts | 54 +++++ .../local-extract-resource-limit.test.ts | 16 +- .../documents/__tests__/pdf-isolation.test.ts | 118 +++++++++- .../documents/__tests__/reader-queue.test.ts | 138 ++++++++++++ src/lib/documents/__tests__/thumbnail.test.ts | 25 ++- src/lib/documents/image-engine.mjs | 81 +++++++ src/lib/documents/local-extract.ts | 15 +- src/lib/documents/pdf-child.mjs | 19 +- src/lib/documents/pdf-isolation.ts | 207 +++++++++++++----- src/lib/documents/pdfjs.ts | 26 ++- src/lib/documents/rasterize-pdf.ts | 22 +- src/lib/documents/reader-queue.ts | 163 ++++++++++++++ src/lib/documents/thumbnail.ts | 130 ++++++++--- .../jobs/__tests__/document-thumbnail.test.ts | 43 ++++ src/lib/jobs/document-thumbnail-backfill.ts | 12 +- src/lib/jobs/document-thumbnail.ts | 17 +- 17 files changed, 972 insertions(+), 122 deletions(-) create mode 100644 src/lib/documents/__tests__/image-engine.test.ts create mode 100644 src/lib/documents/__tests__/reader-queue.test.ts create mode 100644 src/lib/documents/image-engine.mjs create mode 100644 src/lib/documents/reader-queue.ts diff --git a/next.config.ts b/next.config.ts index 3a9b959db..013e81c7a 100644 --- a/next.config.ts +++ b/next.config.ts @@ -302,11 +302,13 @@ const nextConfig: NextConfig = { outputFileTracingIncludes: { "*": [ "./src/lib/ai/prompts/safety-contracts.*.yaml", - // PDFs are read and rendered in a child process with a memory ceiling - // and a time limit (`src/lib/documents/pdf-isolation.ts`). The child is - // started from these two files by path, which the tracer cannot follow. + // PDFs are read and rendered, and uploaded images decoded into their + // thumbnails, in a child process with a memory ceiling and a time limit + // (`src/lib/documents/pdf-isolation.ts`). The child is started from + // these files by path, which the tracer cannot follow. "./src/lib/documents/pdf-child.mjs", "./src/lib/documents/pdf-engine.mjs", + "./src/lib/documents/image-engine.mjs", // Document AI local text extraction and rasterization both run the // app's one pdfjs-dist (`src/lib/documents/pdfjs.ts`) server-side; the // text read is the provider-free fallback that keeps content search diff --git a/src/lib/documents/__tests__/image-engine.test.ts b/src/lib/documents/__tests__/image-engine.test.ts new file mode 100644 index 000000000..95237b1a8 --- /dev/null +++ b/src/lib/documents/__tests__/image-engine.test.ts @@ -0,0 +1,54 @@ +import { Buffer } from "node:buffer"; + +import { createCanvas } from "@napi-rs/canvas"; +import { describe, expect, it, vi } from "vitest"; + +import { renderImageThumbnail } from "../image-engine.mjs"; + +/** + * What runs in the reader's child for an image upload: decode, check the + * decoded size once more, draw onto a fresh JPEG. The header walk in the + * server is a reading of the file; the decoder has the final word, so the + * decoded size is checked before a canvas is drawn from it. + */ + +const OPTIONS = { longEdge: 320, quality: 70, maxPixels: 60_000_000 }; + +function png(width: number, height: number): Buffer { + const canvas = createCanvas(width, height); + const ctx = canvas.getContext("2d"); + ctx.fillStyle = "#3366cc"; + ctx.fillRect(0, 0, width, height); + return canvas.toBuffer("image/png"); +} + +describe("renderImageThumbnail", () => { + it("draws a decoded image onto a JPEG no longer than the long edge", async () => { + const outcome = await renderImageThumbnail(png(1200, 800), OPTIONS); + expect(outcome).toMatchObject({ ok: true, width: 320, height: 213 }); + if (!outcome.ok) return; + expect(Buffer.from(outcome.jpeg).subarray(0, 2).toString("hex")).toBe( + "ffd8", + ); + }); + + it("refuses a decoded image larger than the header said, before drawing it", async () => { + const createCanvasSpy = vi.fn(); + const outcome = await renderImageThumbnail( + Buffer.from("anything"), + OPTIONS, + { + loadImage: async () => ({ width: 30_000, height: 30_000 }), + createCanvas: createCanvasSpy, + }, + ); + expect(outcome).toEqual({ ok: false, reason: "pixel-cap" }); + expect(createCanvasSpy).not.toHaveBeenCalled(); + }); + + it("throws what the decoder threw", async () => { + await expect( + renderImageThumbnail(Buffer.from("not an image"), OPTIONS), + ).rejects.toThrow(); + }); +}); diff --git a/src/lib/documents/__tests__/local-extract-resource-limit.test.ts b/src/lib/documents/__tests__/local-extract-resource-limit.test.ts index 22663188f..05fb4f0d4 100644 --- a/src/lib/documents/__tests__/local-extract-resource-limit.test.ts +++ b/src/lib/documents/__tests__/local-extract-resource-limit.test.ts @@ -19,11 +19,11 @@ vi.mock("../pdfjs", async (importOriginal) => ({ readPdfText, })); -import { PdfResourceLimitError } from "../pdfjs"; +import { PdfReaderUnavailableError, PdfResourceLimitError } from "../pdfjs"; import { extractPdfText, localExtractText } from "../local-extract"; describe("a PDF past the reader's limits", () => { - it.each(["memory", "time", "exit"] as const)( + it.each(["memory", "time"] as const)( "answers resource-limit for the %s bound, without the error log a broken file gets", async (limit) => { readPdfText.mockRejectedValueOnce(new PdfResourceLimitError(limit)); @@ -42,6 +42,18 @@ describe("a PDF past the reader's limits", () => { }, ); + it.each(["busy", "interrupted"] as const)( + "passes a reader that was %s up to the job instead of recording an outcome", + async (kind) => { + readPdfText.mockRejectedValueOnce(new PdfReaderUnavailableError(kind)); + logCaught.mockClear(); + await expect( + extractPdfText(Buffer.from("%PDF-1.7 large")), + ).rejects.toBeInstanceOf(PdfReaderUnavailableError); + expect(logCaught).not.toHaveBeenCalled(); + }, + ); + it("still answers error for anything else the reader throws", async () => { const broken = new Error("Invalid PDF structure"); broken.name = "InvalidPDFException"; diff --git a/src/lib/documents/__tests__/pdf-isolation.test.ts b/src/lib/documents/__tests__/pdf-isolation.test.ts index 28ba074a5..4bf197d77 100644 --- a/src/lib/documents/__tests__/pdf-isolation.test.ts +++ b/src/lib/documents/__tests__/pdf-isolation.test.ts @@ -1,5 +1,6 @@ import { Buffer } from "node:buffer"; -import { readFileSync } from "node:fs"; +import { mkdtempSync, readFileSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; import { join } from "node:path"; import { createDeflate } from "node:zlib"; @@ -8,7 +9,15 @@ import { describe, expect, it, vi } from "vitest"; const { annotate } = vi.hoisted(() => ({ annotate: vi.fn() })); vi.mock("@/lib/logging/context", () => ({ annotate })); -import { runPdfChild } from "../pdf-isolation"; +import { gif } from "../../__tests__/gif-fixture"; +import { + MAX_QUEUED_PER_OWNER, + runOnce, + runPdfChild, + withPdfWork, + type PdfChildRequest, + type PdfChildResult, +} from "../pdf-isolation"; import { readPdfText } from "../pdfjs"; import { rasterizePdf } from "../rasterize-pdf"; import { generateThumbnail } from "../thumbnail"; @@ -187,3 +196,108 @@ describe("PDF reading runs in a bounded child process", () => { expect(result).toMatchObject({ ok: false, reason: "error" }); }); }); + +describe("an image upload is decoded in the bounded child", () => { + it("ends the child, not the server, when a decoder allocates past the ceiling", async () => { + // The header walk refuses this file; here it goes to the decoder + // directly, as a decoder surprise the walk did not foresee would. + const bomb = gif({ width: 10, height: 10 }, [ + { width: 20_000, height: 20_000 }, + ]); + const before = process.memoryUsage().rss; + const result = await runPdfChild( + { + op: "image-thumbnail", + data: bomb, + options: { longEdge: 320, quality: 70, maxPixels: 1e12 }, + }, + { timeoutMs: 60_000, memoryMb: 300 }, + ); + expect(result).toEqual({ ok: false, reason: "limit", limit: "memory" }); + expect(process.memoryUsage().rss - before).toBeLessThan(200 * 1024 ** 2); + }, 90_000); +}); + +describe("a child that ends without an answer", () => { + const dir = mkdtempSync(join(tmpdir(), "reader-child-")); + const script = (name: string, body: string) => { + const path = join(dir, name); + writeFileSync(path, body); + return path; + }; + const request = { op: "text", data: Buffer.from("x"), maxPages: 1 } as const; + + it("is not a final verdict when nothing this module measured ended it", async () => { + // A kill from outside: the kernel's out-of-memory killer acting for the + // whole container looks like this to the parent. + const killed = script( + "killed-child", + 'process.kill(process.pid, "SIGKILL");\n', + ); + await expect( + runOnce(killed, request, { timeoutMs: 10_000 }), + ).resolves.toEqual({ ok: false, reason: "interrupted" }); + }); + + it("is the document's doing when V8 reached the heap cap the child was given", async () => { + const heap = script( + "heap-child", + 'process.stderr.write("FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory\\n", () => process.exit(134));\n', + ); + await expect( + runOnce(heap, request, { timeoutMs: 10_000 }), + ).resolves.toEqual({ ok: false, reason: "limit", limit: "memory" }); + }); + + it("is run once more, and answered interrupted when it ends that way again", async () => { + const interrupted = vi.fn( + async (): Promise> => ({ + ok: false, + reason: "interrupted", + }), + ); + await expect( + runPdfChild(request, { timeoutMs: 1_000, run: interrupted }), + ).resolves.toEqual({ ok: false, reason: "interrupted" }); + expect(interrupted).toHaveBeenCalledTimes(2); + + const recovers = vi + .fn<() => Promise>>() + .mockResolvedValueOnce({ ok: false, reason: "interrupted" }) + .mockResolvedValueOnce({ ok: true, value: "text" }); + await expect( + runPdfChild(request, { timeoutMs: 1_000, run: recovers }), + ).resolves.toEqual({ ok: true, value: "text" }); + expect(recovers).toHaveBeenCalledTimes(2); + }); +}); + +describe("the reader's queue", () => { + it("answers one account's work past its waiting cap busy, at once", async () => { + const request = { + op: "text", + data: Buffer.from("x"), + maxPages: 1, + } as const; + let open: () => void = () => {}; + const gate = new Promise((resolve) => (open = resolve)); + const run = vi.fn(async (): Promise> => { + await gate; + return { ok: true, value: "text" }; + }); + const asA = () => + withPdfWork({ owner: "account-a", lane: "interactive" }, () => + runPdfChild(request, { timeoutMs: 1_000, run }), + ); + + const running = asA(); + const waiting = Array.from({ length: MAX_QUEUED_PER_OWNER }, asA); + await expect(asA()).resolves.toMatchObject({ ok: false, reason: "busy" }); + + open(); + await expect(running).resolves.toMatchObject({ ok: true }); + for (const item of waiting) { + await expect(item).resolves.toMatchObject({ ok: true }); + } + }); +}); diff --git a/src/lib/documents/__tests__/reader-queue.test.ts b/src/lib/documents/__tests__/reader-queue.test.ts new file mode 100644 index 000000000..ea4f96c7c --- /dev/null +++ b/src/lib/documents/__tests__/reader-queue.test.ts @@ -0,0 +1,138 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { createReaderQueue, type ReaderTicket } from "../reader-queue"; + +/** + * The document reader has one slot. It used to go to whoever asked first, + * and a waiting item waited without end: a few heavy PDFs from one account + * held every other account's lab scan behind them. These pin the order the + * slot is handed out in, the cap on what one account may queue, and the + * deadline a waiting item gets. + */ + +const WAIT = { interactive: 1_000, background: 5_000 }; + +function queue(maxQueuedPerOwner = 10) { + return createReaderQueue({ slots: 1, maxQueuedPerOwner, waitMs: WAIT }); +} + +/** Acquire, and note the order in which tickets are granted. */ +function recorder() { + const order: string[] = []; + const tickets = new Map(); + const track = (label: string, pending: Promise) => + pending.then((ticket) => { + if (ticket.ok) order.push(label); + tickets.set(label, ticket); + return ticket; + }); + const release = (label: string) => { + const ticket = tickets.get(label); + if (ticket?.ok) ticket.release(); + }; + return { order, track, release }; +} + +const flush = () => new Promise((resolve) => setImmediate(resolve)); + +describe("the reader queue", () => { + it("takes accounts in turn instead of first come, first served", async () => { + const q = queue(); + const r = recorder(); + void r.track("a1", q.acquire("a", "background")); + void r.track("a2", q.acquire("a", "background")); + void r.track("a3", q.acquire("a", "background")); + void r.track("b1", q.acquire("b", "background")); + void r.track("c1", q.acquire("c", "background")); + await flush(); + for (const label of ["a1", "a2", "b1", "c1", "a3"]) { + r.release(label); + await flush(); + } + // a1 held the slot. Of the waiting items a2 came first, then b and c + // each get their turn before a's third item comes round. + expect(r.order).toEqual(["a1", "a2", "b1", "c1", "a3"]); + }); + + it("serves a read a person is waiting on before background work", async () => { + const q = queue(); + const r = recorder(); + void r.track("bg-running", q.acquire("a", "background")); + void r.track("bg-a", q.acquire("a", "background")); + void r.track("bg-b", q.acquire("b", "background")); + void r.track("lab-scan", q.acquire("c", "interactive")); + await flush(); + r.release("bg-running"); + await flush(); + expect(r.order).toEqual(["bg-running", "lab-scan"]); + }); + + it("refuses an account's item past its waiting cap at once, without touching others", async () => { + const q = queue(2); + const r = recorder(); + await r.track("a-running", q.acquire("a", "interactive")); + void r.track("a-1", q.acquire("a", "interactive")); + void r.track("a-2", q.acquire("a", "background")); + await expect(q.acquire("a", "interactive")).resolves.toMatchObject({ + ok: false, + reason: "queue-full", + }); + // Another account still gets in line, and its turn comes before the + // capped account's second waiting item. + void r.track("b-1", q.acquire("b", "interactive")); + expect(q.snapshot().waiting).toEqual({ interactive: 2, background: 1 }); + for (const label of ["a-running", "a-1"]) { + r.release(label); + await flush(); + } + expect(r.order).toEqual(["a-running", "a-1", "b-1"]); + r.release("b-1"); + await flush(); + expect(r.order.at(-1)).toBe("a-2"); + r.release("a-2"); + }); + + describe("with a deadline", () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + afterEach(() => { + vi.useRealTimers(); + }); + + it("answers a waiting item busy when its lane's deadline passes, and keeps the slot for the next", async () => { + const q = queue(); + const held = await q.acquire("a", "background"); + const waiting = q.acquire("b", "interactive"); + const later = q.acquire("c", "background"); + + await vi.advanceTimersByTimeAsync(WAIT.interactive + 1); + await expect(waiting).resolves.toEqual({ + ok: false, + reason: "wait-expired", + retryAfterMs: WAIT.interactive, + }); + expect(q.snapshot()).toEqual({ + running: 1, + waiting: { interactive: 0, background: 1 }, + }); + + // The expired item left the queue: releasing hands the slot to the + // next one in line, not to nobody. + if (held.ok) held.release(); + await expect(later).resolves.toMatchObject({ ok: true }); + }); + + it("releases a slot once, however often release is called", async () => { + const q = queue(); + const first = await q.acquire("a", "background"); + if (!first.ok) throw new Error("expected the slot"); + first.release(); + first.release(); + expect(q.snapshot().running).toBe(0); + const a = await q.acquire("a", "background"); + expect(a.ok).toBe(true); + expect(q.snapshot().running).toBe(1); + }); + }); +}); diff --git a/src/lib/documents/__tests__/thumbnail.test.ts b/src/lib/documents/__tests__/thumbnail.test.ts index 7620df07e..6a28f1aa4 100644 --- a/src/lib/documents/__tests__/thumbnail.test.ts +++ b/src/lib/documents/__tests__/thumbnail.test.ts @@ -5,10 +5,9 @@ import { describe, expect, it, vi } from "vitest"; vi.mock("@/lib/logging/context", () => ({ annotate: vi.fn() })); -// Partial mock of the native canvas module: `loadImage` delegates to the real -// decoder by default (so every real-fixture test below is unaffected) and can -// be overridden per-test via `loadImageMock` to simulate a decoded image whose -// dimensions the header sniff could not have known. +// Partial mock of the native canvas module in this process: `loadImage` +// delegates to the real decoder and records every call, so a test can tell +// whether an upload was decoded here rather than in the reader's child. const { loadImageMock } = vi.hoisted(() => ({ loadImageMock: vi.fn() })); vi.mock("@napi-rs/canvas", async (importActual) => { const actual = await importActual(); @@ -166,14 +165,16 @@ describe("generateThumbnail", () => { }); }); - it("refuses an oversize image the header sniff missed (post-decode cap)", async () => { - // A readable header declaring a small image whose decoder is forced to - // return 30000×30000 = 900 MP: only the post-decode backstop can catch - // it. - loadImageMock.mockResolvedValueOnce({ width: 30_000, height: 30_000 }); - await expect( - generateThumbnail(sourceImage(16, 16, "image/jpeg"), "image/jpeg"), - ).resolves.toEqual({ ok: false, reason: "pixel-cap" }); + it("decodes an uploaded image in the reader's child process, not in this one", async () => { + // The decoder allocates what the file makes it allocate; in the child + // that ends the child. Nothing of an image upload is decoded here. + loadImageMock.mockClear(); + const result = await generateThumbnail( + sourceImage(640, 480, "image/png"), + "image/png", + ); + expect(result.ok).toBe(true); + expect(loadImageMock).not.toHaveBeenCalled(); }); /** diff --git a/src/lib/documents/image-engine.mjs b/src/lib/documents/image-engine.mjs new file mode 100644 index 000000000..1012ea2dd --- /dev/null +++ b/src/lib/documents/image-engine.mjs @@ -0,0 +1,81 @@ +// @ts-check +/** + * Decoding an uploaded image into the vault's preview thumbnail. + * + * The server never runs this in its own process. An image decoder allocates + * the whole bitmap before anything can look at it, and the size it allocates + * is whatever the file declares in a place the decoder chooses: a GIF frame + * descriptor once reached a 1.6 GB decode behind a header check that only + * read the logical screen. The header check (`../image-dimensions.ts`) still + * refuses what it can see before a child is started; this module is what + * runs in that child (`./pdf-child.mjs`, under the memory ceiling and time + * limit of `./pdf-isolation.ts`), so a decoder surprise ends the child and + * not the server. + * + * Plain JavaScript for the same reason as `./pdf-engine.mjs`: the child runs + * it with plain Node from the checkout and the standalone image alike. + */ +import { Buffer } from "node:buffer"; + +/** + * @typedef {object} ImageThumbnailOptions + * @property {number} longEdge The preview's longest side; never upscaled. + * @property {number} quality JPEG quality, 0 to 100. + * @property {number} maxPixels The most pixels a decoded source may have. + */ + +/** + * @typedef {{ ok: true; jpeg: Uint8Array; width: number; height: number } + * | { ok: false; reason: "pixel-cap" | "empty-render" }} ImageThumbnailOutcome + */ + +/** + * The slice of `@napi-rs/canvas` used here; the suites pass a stand-in. + * + * @typedef {object} CanvasModule + * @property {(source: Buffer) => Promise<{ width: number; height: number }>} loadImage + * @property {(width: number, height: number) => { + * getContext(type: "2d"): { drawImage(image: unknown, dx: number, dy: number, dw: number, dh: number): void }; + * toBuffer(mime: "image/jpeg", quality?: number): Buffer; + * }} createCanvas + */ + +/** + * Decode an image and draw it onto a fresh JPEG no longer than + * `options.longEdge` on its long side. The re-encode carries none of the + * source's metadata. Throws what the decoder threw. + * + * @param {Uint8Array} data + * @param {ImageThumbnailOptions} options + * @param {CanvasModule} [canvasModule] + * @returns {Promise} + */ +export async function renderImageThumbnail(data, options, canvasModule) { + const napi = + canvasModule ?? + /** @type {CanvasModule} */ ( + /** @type {unknown} */ (await import("@napi-rs/canvas")) + ); + const image = await napi.loadImage( + Buffer.from(data.buffer, data.byteOffset, data.byteLength), + ); + // The decoded size again, before a canvas of that size is drawn from: the + // header check is a reading of the file, the decoder had the final word. + if (image.width * image.height > options.maxPixels) { + return { ok: false, reason: "pixel-cap" }; + } + if (image.width <= 0 || image.height <= 0) { + return { ok: false, reason: "empty-render" }; + } + const scale = Math.min( + 1, + options.longEdge / Math.max(image.width, image.height), + ); + const width = Math.max(1, Math.round(image.width * scale)); + const height = Math.max(1, Math.round(image.height * scale)); + const canvas = napi.createCanvas(width, height); + canvas.getContext("2d").drawImage(image, 0, 0, width, height); + const jpeg = canvas.toBuffer("image/jpeg", options.quality); + if (jpeg.byteLength === 0) return { ok: false, reason: "empty-render" }; + return { ok: true, jpeg: new Uint8Array(jpeg), width, height }; +} diff --git a/src/lib/documents/local-extract.ts b/src/lib/documents/local-extract.ts index 8469dc7c6..9507e89a9 100644 --- a/src/lib/documents/local-extract.ts +++ b/src/lib/documents/local-extract.ts @@ -32,7 +32,11 @@ import { Buffer } from "node:buffer"; import { annotate } from "@/lib/logging/context"; import { logCaught } from "@/lib/logging/signal"; -import { PdfResourceLimitError, readPdfText } from "./pdfjs"; +import { + PdfReaderUnavailableError, + PdfResourceLimitError, + readPdfText, +} from "./pdfjs"; /** * Minimum joined text length for a PDF to count as a real text-layer document. @@ -70,9 +74,11 @@ export type LocalExtractResult = /** * Extract a text-layer PDF's embedded text server-side. Returns `empty` for a * scanned/no-text-layer PDF, `resource-limit` for one past the reader's memory - * or time bound, and `error` for a malformed/encrypted one — never - * throws, so a bad document can never abort the caller (the upload/backfill job - * just leaves it un-indexed). An `error` leaves a `warn` line naming what + * or time bound, and `error` for a malformed/encrypted one, so a bad document + * can never abort the caller (the upload/backfill job just leaves it + * un-indexed). The one throw is `PdfReaderUnavailableError`: the reader was + * busy or its process ended for no measured reason, which says nothing about + * the document, so the job runs again instead of recording an outcome. An `error` leaves a `warn` line naming what * pdf.js threw, because the outcome alone does not say why a document stayed * out of the index. */ @@ -84,6 +90,7 @@ export async function extractPdfText( // Bound the page walk so a pathological many-page PDF costs no more. text = (await readPdfText(buffer, LOCAL_MAX_PAGES)).trim(); } catch (err) { + if (err instanceof PdfReaderUnavailableError) throw err; if (err instanceof PdfResourceLimitError) { // Not a fault of the file's bytes being unreadable: they are readable, // only not within the bounds this server gives one document. diff --git a/src/lib/documents/pdf-child.mjs b/src/lib/documents/pdf-child.mjs index d143ad503..28b4f9429 100644 --- a/src/lib/documents/pdf-child.mjs +++ b/src/lib/documents/pdf-child.mjs @@ -1,9 +1,10 @@ // @ts-check /** - * The process a PDF is read and rendered in. `./pdf-isolation.ts` starts it - * for one request, hands it the bytes over the IPC channel, and takes back - * either the result or the fact that it died. It holds no secrets: it is - * started with an almost empty environment and never opens the database. + * The process a PDF is read and rendered in, and an uploaded image is decoded + * into its thumbnail in. `./pdf-isolation.ts` starts it for one request, + * hands it the bytes over the IPC channel, and takes back either the result + * or the fact that it died. It holds no secrets: it is started with an almost + * empty environment and never opens the database. * * On Linux it volunteers itself to the kernel's out-of-memory killer first, * so when a document outruns the memory check between two samples and the @@ -12,6 +13,7 @@ */ import { writeFileSync } from "node:fs"; +import { renderImageThumbnail } from "./image-engine.mjs"; import { readTextLayer, renderPdfPages } from "./pdf-engine.mjs"; try { @@ -22,7 +24,8 @@ try { /** * @typedef {{ op: "text"; data: Uint8Array; maxPages: number } - * | { op: "render"; data: Uint8Array; options: import("./pdf-engine.mjs").RenderOptions }} PdfChildRequest + * | { op: "render"; data: Uint8Array; options: import("./pdf-engine.mjs").RenderOptions } + * | { op: "image-thumbnail"; data: Uint8Array; options: import("./image-engine.mjs").ImageThumbnailOptions }} PdfChildRequest */ /** @@ -52,6 +55,12 @@ async function handle(request) { document: await renderPdfPages(data, request.options), }; } + if (request.op === "image-thumbnail") { + return { + ok: true, + thumbnail: await renderImageThumbnail(data, request.options), + }; + } return failure(new Error("unknown request")); } catch (err) { return failure(err); diff --git a/src/lib/documents/pdf-isolation.ts b/src/lib/documents/pdf-isolation.ts index 5149f9b3e..c0a2ea8dc 100644 --- a/src/lib/documents/pdf-isolation.ts +++ b/src/lib/documents/pdf-isolation.ts @@ -1,5 +1,6 @@ /** - * Runs pdf.js in a child process with a memory ceiling and a time limit. + * Runs pdf.js, and the image decoder behind the vault's thumbnails, in a child + * process with a memory ceiling and a time limit. * * A PDF's cost is set by whoever wrote it. A 400 KB file can declare an image * of twenty thousand pixels square, and pdf.js allocated about 7 GB decoding @@ -20,20 +21,34 @@ * - it starts with an almost empty environment, so it holds no key, token * or database address; * - one runs at a time (`MAX_PARALLEL_CHILDREN`), so a burst of uploads - * cannot multiply the ceiling. + * cannot multiply the ceiling. Who runs next is decided by + * `./reader-queue.ts`: a read a person is waiting on before background + * work, accounts in turn, a few waiting items per account, and a + * deadline after which a waiting item is answered `busy`. * - * A document that crosses a bound fails with `limit` and is not something a - * retry will fix; callers record it as a final failure. + * A document that crosses a bound this module measured (the sampled memory, + * the time limit, or the child's own heap cap) fails with `limit` and is not + * something a retry will fix; callers record it as a final failure. A child + * that is gone without an answer and without a measured cause is not that: + * the kernel may have taken it because the container as a whole ran short, + * the server's own memory included. It is run once more, and a second such + * exit answers `interrupted`, which callers treat as retryable. * * This module logs nothing and imports nothing from the app, so the * end-to-end suite can read report text through `./pdfjs.ts` as well. */ +import { AsyncLocalStorage } from "node:async_hooks"; import { execFile, fork, type ChildProcess } from "node:child_process"; import { existsSync } from "node:fs"; import { readFile } from "node:fs/promises"; import { join } from "node:path"; +import type { + ImageThumbnailOptions, + ImageThumbnailOutcome, +} from "./image-engine.mjs"; import type { RenderOptions, RenderedDocument } from "./pdf-engine.mjs"; +import { createReaderQueue, type ReaderLane } from "./reader-queue"; /** Resident memory a child may reach before it is killed. */ export const PDF_CHILD_MEMORY_MB = 640; @@ -61,18 +76,31 @@ const CHILD_PATH = ["src", "lib", "documents", "pdf-child.mjs"]; export type PdfChildRequest = | { op: "text"; data: Uint8Array; maxPages: number } - | { op: "render"; data: Uint8Array; options: RenderOptions }; + | { op: "render"; data: Uint8Array; options: RenderOptions } + | { op: "image-thumbnail"; data: Uint8Array; options: ImageThumbnailOptions }; type PdfChildValue = R extends { op: "text" } ? string - : RenderedDocument; + : R extends { op: "render" } + ? RenderedDocument + : ImageThumbnailOutcome; /** Why a child gave no result. */ export type PdfChildFailure = /** The document crossed the memory ceiling or the time limit. */ - | { ok: false; reason: "limit"; limit: "memory" | "time" | "exit" } - /** pdf.js refused the document: malformed, encrypted, unreadable. */ - | { ok: false; reason: "error"; name: string; message: string }; + | { ok: false; reason: "limit"; limit: "memory" | "time" } + /** pdf.js or the decoder refused the input: malformed, encrypted, unreadable. */ + | { ok: false; reason: "error"; name: string; message: string } + /** + * The child ended without an answer and without a cause this module + * measured, twice. Not the document's verdict; try again later. + */ + | { ok: false; reason: "interrupted" } + /** + * The reader was taken for longer than the wait this work may spend, or + * this account already has as much waiting as it may. Try again later. + */ + | { ok: false; reason: "busy"; retryAfterMs: number }; export type PdfChildResult = { ok: true; value: PdfChildValue } | PdfChildFailure; @@ -81,27 +109,67 @@ export interface PdfChildLimits { timeoutMs: number; /** Overrides `PDF_CHILD_MEMORY_MB`; the suites use a small one. */ memoryMb?: number; + /** Replaces the child start; the suites use it to stand in for a child. */ + run?: ( + script: string, + request: PdfChildRequest, + limits: PdfChildLimits, + ) => Promise>; } -// ── Concurrency ──────────────────────────────────────────────────────────── +// ── Who runs next ────────────────────────────────────────────────────────── -let running = 0; -const waiting: Array<() => void> = []; +/** Items one account may have waiting for the reader, across both lanes. */ +export const MAX_QUEUED_PER_OWNER = 4; -async function acquireSlot(): Promise { - if (running < MAX_PARALLEL_CHILDREN) { - running++; - return; - } - await new Promise((resolve) => waiting.push(resolve)); +/** + * How long an item may wait for the reader. A person waiting on a read gets + * an answer within a minute; background work can wait longer, and its job is + * retried when it does not get a turn. + */ +export const READER_WAIT_MS: Record = { + interactive: 60_000, + background: 5 * 60_000, +}; + +const queue = createReaderQueue({ + slots: MAX_PARALLEL_CHILDREN, + maxQueuedPerOwner: MAX_QUEUED_PER_OWNER, + waitMs: READER_WAIT_MS, +}); + +/** Whose work this is and whether a person is waiting on it. */ +export interface PdfWork { + /** The account the work is done for; the queue takes accounts in turn. */ + owner: string; + lane: ReaderLane; } -function releaseSlot(): void { - const next = waiting.shift(); - if (next) next(); - else running--; +const workContext = new AsyncLocalStorage(); + +/** + * Run `fn` with every reader call inside it counted as `work`. The entry + * points set it: the background AI run worker (interactive) and the document + * jobs (background). A call outside any of them is background work of an + * unattributed owner. + */ +export function withPdfWork( + work: PdfWork, + fn: () => Promise, +): Promise { + return workContext.run(work, fn); } +const UNATTRIBUTED: PdfWork = { owner: "unattributed", lane: "background" }; + +/** The work the reader would count a call made here as. */ +export function currentPdfWork(): PdfWork { + return workContext.getStore() ?? UNATTRIBUTED; +} + +/** How many times a child that vanished without a measured cause is run. */ +const MAX_ATTEMPTS = 2; + // ── Resident memory of another process ───────────────────────────────────── /** The child's resident set in MB, or null where it cannot be read. */ @@ -132,6 +200,7 @@ interface ChildReply { ok: boolean; text?: string; document?: RenderedDocument; + thumbnail?: ImageThumbnailOutcome; error?: { name: string; message: string }; } @@ -150,9 +219,36 @@ function decodeDocument(document: RenderedDocument): RenderedDocument { }; } +function decodeValue( + request: R, + reply: ChildReply, +): PdfChildValue { + if (request.op === "text") return reply.text as PdfChildValue; + if (request.op === "render") { + return decodeDocument( + reply.document as RenderedDocument, + ) as PdfChildValue; + } + const thumbnail = reply.thumbnail as ImageThumbnailOutcome; + return ( + thumbnail.ok + ? { + ...thumbnail, + jpeg: Buffer.from( + thumbnail.jpeg.buffer, + thumbnail.jpeg.byteOffset, + thumbnail.jpeg.byteLength, + ), + } + : thumbnail + ) as PdfChildValue; +} + /** - * Read or render one PDF in a child process. Never throws: a document that - * crosses a bound answers `limit`, one pdf.js refuses answers `error`. + * Read or render one PDF, or decode one image, in a child process. Never + * throws: a document that crosses a bound answers `limit`, one the reader + * refuses answers `error`, a reader that is taken answers `busy`, and a + * child that twice ended for no reason it could see answers `interrupted`. */ export async function runPdfChild( request: R, @@ -168,15 +264,26 @@ export async function runPdfChild( }; } - await acquireSlot(); + const work = currentPdfWork(); + const ticket = await queue.acquire(work.owner, work.lane); + if (!ticket.ok) { + return { ok: false, reason: "busy", retryAfterMs: ticket.retryAfterMs }; + } try { - return await runOnce(script, request, limits); + for (let attempt = 1; ; attempt++) { + const result = limits.run + ? ((await limits.run(script, request, limits)) as PdfChildResult) + : await runOnce(script, request, limits); + if (result.ok || result.reason !== "interrupted") return result; + if (attempt >= MAX_ATTEMPTS) return result; + } } finally { - releaseSlot(); + ticket.release(); } } -function runOnce( +/** Start one child for `request` and wait for its answer or its end. */ +export function runOnce( script: string, request: R, limits: PdfChildLimits, @@ -244,11 +351,7 @@ function runOnce( child.on("message", (raw) => { const reply = raw as ChildReply; if (reply.ok) { - const value = - request.op === "text" - ? reply.text - : decodeDocument(reply.document as RenderedDocument); - finish({ ok: true, value: value as PdfChildValue }); + finish({ ok: true, value: decodeValue(request, reply) }); } else { finish({ ok: false, @@ -259,13 +362,10 @@ function runOnce( } }); - child.on("error", (err) => { - finish({ - ok: false, - reason: "error", - name: err.name, - message: err.message.slice(0, 300), - }); + // The process could not be started or signalled. What pdf.js or the + // decoder thought of the document arrives as a reply, never here. + child.on("error", () => { + finish({ ok: false, reason: "interrupted" }); }); child.on("close", () => { @@ -273,21 +373,24 @@ function runOnce( finish({ ok: false, reason: "limit", limit: killedFor }); return; } - // Gone without an answer: V8 hit its heap ceiling, the kernel took it - // for memory, or it crashed in native code. Any of them is the - // document's doing. - const heap = /heap limit|heap out of memory/i.test(stderrTail); - finish({ ok: false, reason: "limit", limit: heap ? "memory" : "exit" }); + // Gone without an answer. V8 reaching the heap cap this module gave + // the child is the document's doing, whatever else the host runs. Any + // other end (the kernel's out-of-memory killer, which also acts when + // the server itself grew, a signal from outside, a native crash) says + // nothing certain about the document, so it is not a final verdict. + if (/heap limit/i.test(stderrTail)) { + finish({ ok: false, reason: "limit", limit: "memory" }); + return; + } + finish({ ok: false, reason: "interrupted" }); }); child.send(request, (err) => { - if (err) { - finish({ - ok: false, - reason: "error", - name: err.name, - message: err.message.slice(0, 300), - }); + // The bytes never reached the child, mostly because it was already + // gone. That says nothing about the document either: end the child and + // let its close answer `interrupted`. + if (err && child.exitCode === null && child.signalCode === null) { + child.kill("SIGKILL"); } }); }); diff --git a/src/lib/documents/pdfjs.ts b/src/lib/documents/pdfjs.ts index e106ff129..7a55c75ef 100644 --- a/src/lib/documents/pdfjs.ts +++ b/src/lib/documents/pdfjs.ts @@ -21,16 +21,33 @@ export const PDF_TEXT_TIMEOUT_MS = 60_000; /** A document that crossed the memory ceiling or the time limit. */ export class PdfResourceLimitError extends Error { override name = "PdfResourceLimitError"; - constructor(readonly limit: "memory" | "time" | "exit") { + constructor(readonly limit: "memory" | "time") { super(`the PDF crossed the reader's ${limit} limit`); } } +/** + * The reader could not take the document now: it was busy past the wait this + * work may spend, or its process ended for a reason that says nothing about + * the document. Not a verdict on the file; the work is tried again later. + */ +export class PdfReaderUnavailableError extends Error { + override name = "PdfReaderUnavailableError"; + constructor(readonly kind: "busy" | "interrupted") { + super( + kind === "busy" + ? "the document reader is busy" + : "the document reader stopped before it answered", + ); + } +} + /** * The text layer of the first `maxPages` pages, one page after another. * Throws what pdf.js threw (a malformed file, a password, a page that will - * not parse) under its own name, or `PdfResourceLimitError` when the - * document crossed a bound. + * not parse) under its own name, `PdfResourceLimitError` when the document + * crossed a bound, or `PdfReaderUnavailableError` when the reader could not + * take it now. */ export async function readPdfText( data: Uint8Array, @@ -43,6 +60,9 @@ export async function readPdfText( ); if (result.ok) return result.value; if (result.reason === "limit") throw new PdfResourceLimitError(result.limit); + if (result.reason === "busy" || result.reason === "interrupted") { + throw new PdfReaderUnavailableError(result.reason); + } const err = new Error(result.message); err.name = result.name; throw err; diff --git a/src/lib/documents/rasterize-pdf.ts b/src/lib/documents/rasterize-pdf.ts index e410ad6e6..632bbb8dd 100644 --- a/src/lib/documents/rasterize-pdf.ts +++ b/src/lib/documents/rasterize-pdf.ts @@ -107,7 +107,20 @@ export type RasterResult = */ texts?: string[]; } - | { ok: false; reason: "unsupported" | "render-failed" | "resource-limit" }; + | { + ok: false; + reason: + | "unsupported" + | "render-failed" + | "resource-limit" + /** + * Not an answer about this document: the reader was busy past the + * wait this work may spend, or its process ended for no reason it + * measured. The caller tries again later. + */ + | "busy" + | "interrupted"; + }; export { pageText, @@ -166,6 +179,13 @@ export async function rasterizePdf( ); if (!result.ok) { + if (result.reason === "busy" || result.reason === "interrupted") { + annotate({ + action: { name: "documents.rasterize.deferred" }, + meta: { reason: result.reason }, + }); + return { ok: false, reason: result.reason }; + } if (result.reason === "limit") { annotate({ action: { name: "documents.rasterize.failed" }, diff --git a/src/lib/documents/reader-queue.ts b/src/lib/documents/reader-queue.ts new file mode 100644 index 000000000..6c05de942 --- /dev/null +++ b/src/lib/documents/reader-queue.ts @@ -0,0 +1,163 @@ +/** + * Who gets the document reader next. + * + * The reader (`./pdf-isolation.ts`) runs one child process at a time, because + * the server and one child at its memory ceiling are what fit the default + * container. It used to hand that one slot out first come, first served, + * with no limit on how long anyone waited: a few heavy PDFs from one account, + * each allowed up to a minute, put every other person's lab scan behind them + * for as long as the uploads kept coming. + * + * Now the slot goes: + * + * - to interactive work first: a read a person started and is waiting on + * (a lab scan, a medication label, a document read) is served before + * background work (thumbnails, indexing, summaries) that nobody watches; + * - round robin across accounts within each lane, so one account's queue + * takes one turn and then waits behind everyone else's next item; + * - to at most `maxQueuedPerOwner` waiting items per account; one more is + * refused at once rather than queued; + * - within a deadline per lane: an item still waiting when it passes leaves + * the queue with a retryable "busy" answer instead of waiting on. + * + * Nothing here knows what a PDF is, so the order can be tested on its own. + */ + +export type ReaderLane = "interactive" | "background"; + +/** Interactive before background. */ +const LANE_ORDER: readonly ReaderLane[] = ["interactive", "background"]; + +export interface ReaderQueueOptions { + /** Work running at once. */ + slots: number; + /** Items one owner may have waiting, across both lanes. */ + maxQueuedPerOwner: number; + /** How long an item may wait in each lane before it is answered busy. */ + waitMs: Record; +} + +/** The slot, or why there is none. `release` must be called exactly once. */ +export type ReaderTicket = + | { ok: true; release: () => void } + | { ok: false; reason: "queue-full" | "wait-expired"; retryAfterMs: number }; + +interface Waiter { + owner: string; + grant: () => void; + timer: ReturnType; +} + +export interface ReaderQueue { + acquire(owner: string, lane: ReaderLane): Promise; + /** Items running and waiting, for the suites and diagnostics. */ + snapshot(): { running: number; waiting: Record }; +} + +export function createReaderQueue(options: ReaderQueueOptions): ReaderQueue { + let running = 0; + // Per lane, one FIFO per owner. A Map iterates in insertion order, so the + // first key is the owner whose turn it is; after a turn the owner moves to + // the back. + const lanes: Record> = { + interactive: new Map(), + background: new Map(), + }; + + const waitingFor = (owner: string): number => + LANE_ORDER.reduce( + (sum, lane) => sum + (lanes[lane].get(owner)?.length ?? 0), + 0, + ); + + const remove = (lane: ReaderLane, waiter: Waiter): void => { + const queue = lanes[lane].get(waiter.owner); + if (!queue) return; + const at = queue.indexOf(waiter); + if (at !== -1) queue.splice(at, 1); + if (queue.length === 0) lanes[lane].delete(waiter.owner); + }; + + const next = (): Waiter | null => { + for (const lane of LANE_ORDER) { + const owners = lanes[lane]; + const first = owners.keys().next(); + if (first.done) continue; + const owner = first.value; + const queue = owners.get(owner)!; + const waiter = queue.shift()!; + owners.delete(owner); + if (queue.length > 0) owners.set(owner, queue); + return waiter; + } + return null; + }; + + const release = (): void => { + const waiter = next(); + if (waiter) { + clearTimeout(waiter.timer); + waiter.grant(); + } else { + running--; + } + }; + + const ticket = (): ReaderTicket => { + let released = false; + return { + ok: true, + release: () => { + if (released) return; + released = true; + release(); + }, + }; + }; + + return { + acquire(owner, lane) { + if (running < options.slots) { + running++; + return Promise.resolve(ticket()); + } + if (waitingFor(owner) >= options.maxQueuedPerOwner) { + return Promise.resolve({ + ok: false, + reason: "queue-full", + retryAfterMs: options.waitMs[lane], + }); + } + return new Promise((resolve) => { + const waiter: Waiter = { + owner, + // The slot passes straight from the releasing item to this one; + // `running` stays as it is. + grant: () => resolve(ticket()), + timer: setTimeout(() => { + remove(lane, waiter); + resolve({ + ok: false, + reason: "wait-expired", + retryAfterMs: options.waitMs[lane], + }); + }, options.waitMs[lane]), + }; + const queue = lanes[lane].get(owner); + if (queue) queue.push(waiter); + else lanes[lane].set(owner, [waiter]); + }); + }, + snapshot() { + const count = (lane: ReaderLane) => + [...lanes[lane].values()].reduce((sum, q) => sum + q.length, 0); + return { + running, + waiting: { + interactive: count("interactive"), + background: count("background"), + }, + }; + }, + }; +} diff --git a/src/lib/documents/thumbnail.ts b/src/lib/documents/thumbnail.ts index 7cc504e42..cb6d4c6ed 100644 --- a/src/lib/documents/thumbnail.ts +++ b/src/lib/documents/thumbnail.ts @@ -6,6 +6,12 @@ * rasterised (via `rasterizePdf`, bounded to one page) then downscaled; every * other MIME type has no preview (the card keeps its kind icon). * + * Both untrusted decodes happen in the reader's child process + * (`./pdf-isolation.ts`) under its memory ceiling and time limit: the image + * through `./image-engine.mjs`, the PDF page through `./pdf-engine.mjs`. An + * image's declared size is still checked here first, so a bomb the header + * shows is refused without starting a child. + * * `@napi-rs/canvas` + pdfjs are pulled LAZILY via runtime `import()` — pdfjs's * module top-level references the browser-only `DOMMatrix` global, so a STATIC * import would make the Turbopack server chunk evaluate that reference at @@ -24,12 +30,14 @@ * binary, or any render error resolves to `{ ok: false }`, and the caller * leaves the document without a thumbnail (the card falls back to its kind * icon). This preserves the "a bad document never aborts an upload/backfill" - * contract. + * contract. `reader-busy` and `interrupted` are the two answers that say + * nothing about the document; the job tries those again. */ import { Buffer } from "node:buffer"; import { nativeCanvasSupported } from "./native-canvas-support"; +import { runPdfChild } from "@/lib/documents/pdf-isolation"; import { rasterizePdf } from "@/lib/documents/rasterize-pdf"; import { readImageDimensions, @@ -96,6 +104,10 @@ export type ThumbnailSkipReason = | "raster-failed" | "resource-limit" | "empty-render" + /** The reader was busy; the job is retried. */ + | "reader-busy" + /** The reader's process ended for no measured reason; tried again later. */ + | "interrupted" | "error"; /** Best-effort generation outcome — never an exception. */ @@ -152,6 +164,76 @@ function drawScaledJpeg( return { jpeg, width, height }; } +/** How long decoding one image into its preview may take. */ +export const IMAGE_THUMBNAIL_TIMEOUT_MS = 30_000; + +/** + * Decode and downscale an image in the reader's child process, under its + * memory ceiling and time limit. The decoder allocates whatever the file + * makes it allocate; in the child that ends the child, not the server. + */ +async function imageThumbnail(bytes: Buffer): Promise { + const result = await runPdfChild( + { + op: "image-thumbnail", + data: bytes, + options: { + longEdge: THUMB_LONG_EDGE, + quality: THUMB_QUALITY, + maxPixels: MAX_SOURCE_PIXELS, + }, + }, + { timeoutMs: IMAGE_THUMBNAIL_TIMEOUT_MS }, + ); + if (!result.ok) { + if (result.reason === "busy") return { ok: false, reason: "reader-busy" }; + if (result.reason === "interrupted") { + return { ok: false, reason: "interrupted" }; + } + if (result.reason === "limit") { + annotate({ + action: { name: "documents.thumbnail.rejected" }, + meta: { + source: "image", + reason: "resource_limit", + limit: result.limit, + }, + }); + return { ok: false, reason: "resource-limit" }; + } + annotate({ + action: { name: "documents.thumbnail.failed" }, + meta: { + mimeType: "image", + reason: result.name, + message: result.message.slice(0, 300), + }, + }); + return { ok: false, reason: "error" }; + } + const outcome = result.value; + if (!outcome.ok) { + if (outcome.reason === "pixel-cap") { + // The decoder found a larger image than the header walk did. + annotate({ + action: { name: "documents.thumbnail.rejected" }, + meta: { source: "image", reason: "pixel_cap_post_decode" }, + }); + } + return { ok: false, reason: outcome.reason }; + } + const thumb: Thumbnail = { + jpeg: Buffer.from(outcome.jpeg), + width: outcome.width, + height: outcome.height, + }; + annotate({ + action: { name: "documents.thumbnail.ok" }, + meta: { source: "image", width: thumb.width, height: thumb.height }, + }); + return { ok: true, thumbnail: thumb }; +} + /** * Generate a small JPEG preview for a stored document, or `{ ok: false }` when * the type is unsupported or rendering fails. Never throws. @@ -173,15 +255,10 @@ export async function generateThumbnail( }); return { ok: false, reason: "native-canvas-unsupported" }; } - // Lazy import: keeps @napi-rs/canvas (+ transitively pdfjs on the PDF path) - // out of the eager server chunk. Resolved on first generation, then cached. - const napi = - (await import("@napi-rs/canvas")) as unknown as NapiCanvasModule; - if (THUMBNAILABLE_IMAGE_MIMES.has(mimeType)) { - // Refuse a decompression bomb BEFORE `loadImage` allocates the bitmap, - // and refuse an image whose size cannot be read: that one would reach - // the decoder with no ceiling at all. + // Refuse a decompression bomb before a decoder is started at all, and + // refuse an image whose size cannot be read: that one would reach the + // decoder with no ceiling but the child's. const pixels = sniffSourcePixels(bytes, mimeType); if (pixels === null) { annotate({ @@ -197,28 +274,7 @@ export async function generateThumbnail( }); return { ok: false, reason: "pixel-cap" }; } - const image = await napi.loadImage(bytes); - // Post-decode backstop: should a decoder ever read a larger frame than - // the header walk did, the decoded size is checked again before the - // bitmap is drawn. - if (image.width * image.height > MAX_SOURCE_PIXELS) { - annotate({ - action: { name: "documents.thumbnail.rejected" }, - meta: { - source: "image", - reason: "pixel_cap_post_decode", - pixels: image.width * image.height, - }, - }); - return { ok: false, reason: "pixel-cap" }; - } - const thumb = drawScaledJpeg(napi, image); - if (!thumb) return { ok: false, reason: "empty-render" }; - annotate({ - action: { name: "documents.thumbnail.ok" }, - meta: { source: "image", width: thumb.width, height: thumb.height }, - }); - return { ok: true, thumbnail: thumb }; + return await imageThumbnail(bytes); } if (mimeType === "application/pdf") { @@ -229,9 +285,21 @@ export async function generateThumbnail( // bytes will again, so the job records it as final. return { ok: false, reason: "resource-limit" }; } + if (!raster.ok && raster.reason === "busy") { + return { ok: false, reason: "reader-busy" }; + } + if (!raster.ok && raster.reason === "interrupted") { + return { ok: false, reason: "interrupted" }; + } if (!raster.ok || raster.images.length === 0) { return { ok: false, reason: "raster-failed" }; } + // The page is a JPEG the child rendered, no longer than + // `RASTER_TARGET_LONG_EDGE` on its long side: the one decode left in + // this process is of the server's own output at a bounded size. + // Lazy import: keeps @napi-rs/canvas out of the eager server chunk. + const napi = + (await import("@napi-rs/canvas")) as unknown as NapiCanvasModule; const pageJpeg = Buffer.from(raster.images[0]!.dataBase64, "base64"); const image = await napi.loadImage(pageJpeg); const thumb = drawScaledJpeg(napi, image); diff --git a/src/lib/jobs/__tests__/document-thumbnail.test.ts b/src/lib/jobs/__tests__/document-thumbnail.test.ts index 4ae6ef3e3..c2b6e918f 100644 --- a/src/lib/jobs/__tests__/document-thumbnail.test.ts +++ b/src/lib/jobs/__tests__/document-thumbnail.test.ts @@ -177,4 +177,47 @@ describe("thumbnailRetryDue", () => { thumbnailRetryDue({ reason: "error", attempts: 3, failedAt: at }, before), ).toBe(false); }); + + it("gives a renderer that ended for no measured reason the ordinary attempts, not a final verdict", async () => { + const { thumbnailRetryDue } = + await import("../document-thumbnail-backfill"); + expect( + thumbnailRetryDue( + { reason: "interrupted", attempts: 1, failedAt: at }, + before, + ), + ).toBe(true); + expect( + thumbnailRetryDue( + { reason: "interrupted", attempts: 3, failedAt: at }, + before, + ), + ).toBe(false); + expect( + thumbnailRetryDue( + { reason: "resource-limit", attempts: 1, failedAt: at }, + before, + ), + ).toBe(false); + }); +}); + +describe("runDocumentThumbnail when the reader cannot take the document now", () => { + it("throws for a busy reader so the job runs again, and records nothing", async () => { + generateThumbnail.mockResolvedValue({ ok: false, reason: "reader-busy" }); + await expect( + runDocumentThumbnail({ userId: "user-1", documentId: "doc-1" }), + ).rejects.toMatchObject({ name: "PdfReaderUnavailableError" }); + expect(failureUpsert).not.toHaveBeenCalled(); + }); + + it("records an interrupted render as a failure the backfill tries again", async () => { + generateThumbnail.mockResolvedValue({ ok: false, reason: "interrupted" }); + await runDocumentThumbnail({ userId: "user-1", documentId: "doc-1" }); + expect(failureUpsert).toHaveBeenCalledWith( + expect.objectContaining({ + create: expect.objectContaining({ reason: "interrupted" }), + }), + ); + }); }); diff --git a/src/lib/jobs/document-thumbnail-backfill.ts b/src/lib/jobs/document-thumbnail-backfill.ts index 4da644497..f0c87426e 100644 --- a/src/lib/jobs/document-thumbnail-backfill.ts +++ b/src/lib/jobs/document-thumbnail-backfill.ts @@ -51,9 +51,15 @@ const THUMBNAILABLE_MIMES = [ /** * Render refusals that the same bytes will always get again: the first page * renders blank, the render comes out empty, the image is past the pixel cap - * or its size cannot be read, the PDF crossed the renderer's memory or time bound. One attempt settles - * them; queueing a document that exhausts the renderer again at every boot - * would only repeat the exhaustion. + * or its size cannot be read, the document crossed a bound the renderer + * measured (its memory ceiling, its time limit, its heap cap). One attempt + * settles them; queueing a document that exhausts the renderer again at + * every boot would only repeat the exhaustion. + * + * `interrupted` is deliberately not here: a renderer that ended without a + * measured cause may have been taken by the kernel because the whole + * container ran short, which says nothing about the document. It gets the + * ordinary bounded attempts. */ const FINAL_THUMBNAIL_FAILURES = [ "raster-failed", diff --git a/src/lib/jobs/document-thumbnail.ts b/src/lib/jobs/document-thumbnail.ts index 4195820ee..0ead396c8 100644 --- a/src/lib/jobs/document-thumbnail.ts +++ b/src/lib/jobs/document-thumbnail.ts @@ -29,6 +29,8 @@ import { decryptDocumentContent, encryptThumbnail, } from "@/lib/documents/store"; +import { withPdfWork } from "@/lib/documents/pdf-isolation"; +import { PdfReaderUnavailableError } from "@/lib/documents/pdfjs"; import { generateThumbnail } from "@/lib/documents/thumbnail"; import { getGlobalBoss } from "@/lib/jobs/boss-instance"; import { withBackgroundEvent } from "@/lib/logging/background"; @@ -55,10 +57,12 @@ export async function runDocumentThumbnail( if (!userId || !documentId) return; await withBackgroundEvent("job.document_thumbnail", async (evt) => { evt.addMeta("document_id", documentId); - await renderThumbnail(userId, documentId, (reason) => { - evt.addMeta("thumbnail_missing_reason", reason); - evt.elevateLevel("warn"); - }); + await withPdfWork({ owner: userId, lane: "background" }, () => + renderThumbnail(userId, documentId, (reason) => { + evt.addMeta("thumbnail_missing_reason", reason); + evt.elevateLevel("warn"); + }), + ); }); } @@ -107,6 +111,11 @@ async function renderThumbnail( } const result = await generateThumbnail(bytes, document.mimeType); + if (!result.ok && result.reason === "reader-busy") { + // The reader was taken by other work for longer than a background item + // waits. Nothing is wrong with the document: pg-boss runs the job again. + throw new PdfReaderUnavailableError("busy"); + } if (!result.ok) { annotate({ action: { name: "documents.thumbnail.run" }, From d3257a1b6b721f73dab637e9eb4dff9803c821d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 25 Oct 2026 17:06:57 +0100 Subject: [PATCH 07/13] fix(documents): tell people when the document reader is busy, and attribute its work A read a person started now runs as interactive work of their account, and the document jobs as background work of theirs, so the reader's queue can order and cap them. When the reader is busy or stopped before it answered, a lab scan, label read or document read fails with aiRuns.readerBusy (503, nothing read or charged) instead of a misleading unreadable-file error, and the index and summary jobs throw so the queue runs them again rather than recording an outcome about the document. --- docs/api/openapi.yaml | 17 ++++++------ messages/de.json | 6 ++++- messages/en.json | 6 ++++- messages/es.json | 6 ++++- messages/fr.json | 6 ++++- messages/it.json | 6 ++++- messages/ko.json | 6 ++++- messages/nl.json | 6 ++++- messages/pl.json | 6 ++++- messages/pt-BR.json | 6 ++++- .../documents/use-document-assist.ts | 2 ++ src/components/labs/ocr-review-dialog.tsx | 2 ++ src/lib/documents/ai-route-support.ts | 12 +++++++-- src/lib/documents/ai-runs/extract-run.ts | 5 +++- src/lib/documents/ai-runs/index-run.ts | 3 +++ src/lib/documents/ai-runs/run-support.ts | 16 +++++++++++- src/lib/documents/ai-runs/types.ts | 1 + src/lib/documents/extract.ts | 4 +-- src/lib/documents/index-document.ts | 7 +++++ src/lib/documents/ocr-plan.ts | 14 ++++++++-- .../jobs/__tests__/document-ai-run.test.ts | 26 +++++++++++++++++++ src/lib/jobs/document-ai-run.ts | 16 +++++++++++- .../jobs/document-content-index-backfill.ts | 6 ++++- src/lib/jobs/document-index.ts | 13 +++++----- src/lib/jobs/document-summary.ts | 23 +++++++++++++++- src/lib/labs/ocr-run.ts | 5 ++++ .../medications/ingredients/label-extract.ts | 8 ++++++ src/lib/openapi/error-codes.ts | 1 + src/lib/openapi/routes/ai-runs.ts | 2 +- 29 files changed, 202 insertions(+), 35 deletions(-) diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 02f8a3095..780128da3 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -38529,13 +38529,13 @@ components: account.deletion.not_pending; activity — activity.calendar.future_date, activity.calendar.too_old; ai — ai.budget.exceeded, ai.provider.none, ai.record.notPermitted, ai.unavailable, ai.usage.aboveCeiling, ai.usage.defaultAboveCeiling, ai.usage.invalid, ai.usage.levelTooLow, ai.usage.rateLimited; ai_provider - — ai_provider.invalid, ai_provider.no_fields; aiRuns — aiRuns.failed, aiRuns.notFound, aiRuns.timedOut, - aiRuns.workerUnavailable; allergy — allergy.invalid; analytics — analytics.invalid_query; anamnesis — - anamnesis.auditC.invalid, anamnesis.auditC.notFound, anamnesis.auditC.rateLimited, - anamnesis.fact.conflict, anamnesis.fact.currentExists, anamnesis.fact.invalidValue, - anamnesis.lifeConfirm.invalid, anamnesis.lifeSections.invalid; auth — auth.account.deletion_pending, - auth.admin.required, auth.already_authenticated, auth.mfa.code_invalid, auth.missing, - auth.refresh.invalid, auth.refresh.reuse, auth.refresh.revoked, auth.reproof.failed, + — ai_provider.invalid, ai_provider.no_fields; aiRuns — aiRuns.failed, aiRuns.notFound, + aiRuns.readerBusy, aiRuns.timedOut, aiRuns.workerUnavailable; allergy — allergy.invalid; analytics — + analytics.invalid_query; anamnesis — anamnesis.auditC.invalid, anamnesis.auditC.notFound, + anamnesis.auditC.rateLimited, anamnesis.fact.conflict, anamnesis.fact.currentExists, + anamnesis.fact.invalidValue, anamnesis.lifeConfirm.invalid, anamnesis.lifeSections.invalid; auth — + auth.account.deletion_pending, auth.admin.required, auth.already_authenticated, auth.mfa.code_invalid, + auth.missing, auth.refresh.invalid, auth.refresh.reuse, auth.refresh.revoked, auth.reproof.failed, auth.reproof.required, auth.reproof.too_weak, auth.scope.insufficient, auth.session.insecure_transport, auth.session.unavailable, auth.stepup.required, auth.token.expired, auth.token.invalid; backup — backup.foreign_reference, backup.key.missing, backup.payload.undecryptable, backup.restore.active, @@ -75840,7 +75840,8 @@ components: description: "Present once `status` is `FAILED`: the `meta.errorCode`, message and HTTP status the queuing route would have answered with, or one of the run's own codes: `aiRuns.workerUnavailable` (no worker took the run within fifteen minutes), `aiRuns.timedOut` (the read outlived the calls the AI response time allows), - `aiRuns.failed` (anything else)." + `aiRuns.readerBusy` (the server's document reader was busy past the wait a read gets, or stopped before it + answered; nothing was read or charged, try again in a minute), `aiRuns.failed` (anything else)." createdAt: type: string startedAt: diff --git a/messages/de.json b/messages/de.json index 96a78eff5..1ee556a07 100644 --- a/messages/de.json +++ b/messages/de.json @@ -175,6 +175,9 @@ "failed": "Das Etikett konnte nicht gelesen werden. Versuch ein schärferes Foto." } } + }, + "aiRuns": { + "readerBusy": "Der Dokumentleser ist gerade mit anderen Aufgaben beschäftigt. Versuch es in einer Minute noch einmal." } }, "doctorReport": { @@ -13258,7 +13261,8 @@ "backgroundScan": "Wird im Hintergrund gelesen. Mit einem langsamen Modell kann das ein paar Minuten dauern.", "waitingForWorker": "Warte auf den Hintergrunddienst. Ändert sich das nicht, läuft der Worker auf dem Server vielleicht nicht.", "errorWorkerUnavailable": "Der Hintergrunddienst ist gerade nicht erreichbar. Versuch es gleich noch einmal oder bitte die Person, die den Server betreibt, ihn zu prüfen.", - "errorTimedOut": "Das Lesen hat länger gedauert, als deine KI-Antwortzeit erlaubt. Versuch es noch einmal oder erhöhe die Antwortzeit in den KI-Einstellungen." + "errorTimedOut": "Das Lesen hat länger gedauert, als deine KI-Antwortzeit erlaubt. Versuch es noch einmal oder erhöhe die Antwortzeit in den KI-Einstellungen.", + "errorReaderBusy": "Der Dokumentleser ist gerade mit anderen Aufgaben beschäftigt. Versuch es in einer Minute noch einmal." }, "symptoms": { "add": "Neues Symptom", diff --git a/messages/en.json b/messages/en.json index bb4cf34c4..c3ab4fd71 100644 --- a/messages/en.json +++ b/messages/en.json @@ -175,6 +175,9 @@ "failed": "The label could not be read. Try a sharper photo." } } + }, + "aiRuns": { + "readerBusy": "The document reader is busy with other work. Try again in a minute." } }, "doctorReport": { @@ -13258,7 +13261,8 @@ "backgroundScan": "Reading in the background. With a slow model this can take a few minutes.", "waitingForWorker": "Waiting for the background worker. If this does not change, the server's worker may not be running.", "errorWorkerUnavailable": "The background worker is not available right now. Try again in a moment, or ask the server operator to check it.", - "errorTimedOut": "Reading took longer than your AI response time allows. Try again, or raise the response time in the AI settings." + "errorTimedOut": "Reading took longer than your AI response time allows. Try again, or raise the response time in the AI settings.", + "errorReaderBusy": "The document reader is busy with other work. Try again in a minute." }, "symptoms": { "add": "New symptom", diff --git a/messages/es.json b/messages/es.json index 5c9056ae2..11c5c0dfd 100644 --- a/messages/es.json +++ b/messages/es.json @@ -175,6 +175,9 @@ "failed": "No se pudo leer la etiqueta. Prueba con una foto más nítida." } } + }, + "aiRuns": { + "readerBusy": "El lector de documentos está ocupado con otras tareas. Inténtalo de nuevo en un minuto." } }, "doctorReport": { @@ -13258,7 +13261,8 @@ "backgroundScan": "Leyendo en segundo plano. Con un modelo lento puede tardar unos minutos.", "waitingForWorker": "Esperando al proceso en segundo plano. Si no cambia, puede que el worker del servidor no esté en marcha.", "errorWorkerUnavailable": "El proceso en segundo plano no está disponible ahora mismo. Inténtalo de nuevo en un momento o pide a quien administra el servidor que lo revise.", - "errorTimedOut": "La lectura tardó más de lo que permite tu tiempo de respuesta de IA. Inténtalo de nuevo o aumenta el tiempo de respuesta en los ajustes de IA." + "errorTimedOut": "La lectura tardó más de lo que permite tu tiempo de respuesta de IA. Inténtalo de nuevo o aumenta el tiempo de respuesta en los ajustes de IA.", + "errorReaderBusy": "El lector de documentos está ocupado con otras tareas. Inténtalo de nuevo en un minuto." }, "symptoms": { "add": "Nuevo síntoma", diff --git a/messages/fr.json b/messages/fr.json index ddc754baf..865cc6f52 100644 --- a/messages/fr.json +++ b/messages/fr.json @@ -175,6 +175,9 @@ "failed": "L’étiquette n’a pas pu être lue. Essayez une photo plus nette." } } + }, + "aiRuns": { + "readerBusy": "Le lecteur de documents est occupé par d'autres tâches. Réessayez dans une minute." } }, "doctorReport": { @@ -13258,7 +13261,8 @@ "backgroundScan": "Lecture en arrière-plan. Avec un modèle lent, cela peut prendre quelques minutes.", "waitingForWorker": "En attente du traitement en arrière-plan. Si rien ne change, le worker du serveur n'est peut-être pas lancé.", "errorWorkerUnavailable": "Le traitement en arrière-plan n'est pas disponible pour le moment. Réessayez dans un instant ou demandez à la personne qui gère le serveur de le vérifier.", - "errorTimedOut": "La lecture a pris plus de temps que votre délai de réponse IA ne le permet. Réessayez ou augmentez ce délai dans les réglages IA." + "errorTimedOut": "La lecture a pris plus de temps que votre délai de réponse IA ne le permet. Réessayez ou augmentez ce délai dans les réglages IA.", + "errorReaderBusy": "Le lecteur de documents est occupé par d'autres tâches. Réessayez dans une minute." }, "symptoms": { "add": "Nouveau symptôme", diff --git a/messages/it.json b/messages/it.json index fe6450c8d..f7daf94b8 100644 --- a/messages/it.json +++ b/messages/it.json @@ -175,6 +175,9 @@ "failed": "Non è stato possibile leggere l'etichetta. Prova con una foto più nitida." } } + }, + "aiRuns": { + "readerBusy": "Il lettore di documenti è occupato con altre attività. Riprova tra un minuto." } }, "doctorReport": { @@ -13258,7 +13261,8 @@ "backgroundScan": "Lettura in background. Con un modello lento può richiedere qualche minuto.", "waitingForWorker": "In attesa del processo in background. Se non cambia, il worker del server potrebbe non essere attivo.", "errorWorkerUnavailable": "Il processo in background non è disponibile al momento. Riprova tra poco o chiedi a chi gestisce il server di controllarlo.", - "errorTimedOut": "La lettura ha richiesto più tempo di quanto consenta il tuo tempo di risposta dell'IA. Riprova o aumenta il tempo di risposta nelle impostazioni IA." + "errorTimedOut": "La lettura ha richiesto più tempo di quanto consenta il tuo tempo di risposta dell'IA. Riprova o aumenta il tempo di risposta nelle impostazioni IA.", + "errorReaderBusy": "Il lettore di documenti è occupato con altre attività. Riprova tra un minuto." }, "symptoms": { "add": "Nuovo sintomo", diff --git a/messages/ko.json b/messages/ko.json index 2ef2164d3..f702d430c 100644 --- a/messages/ko.json +++ b/messages/ko.json @@ -175,6 +175,9 @@ "failed": "라벨을 읽지 못했어요. 더 선명한 사진으로 다시 시도하세요." } } + }, + "aiRuns": { + "readerBusy": "문서 판독기가 다른 작업으로 바빠요. 1분 후에 다시 시도하세요." } }, "doctorReport": { @@ -13258,7 +13261,8 @@ "backgroundScan": "백그라운드에서 읽고 있어요. 느린 모델이면 몇 분 걸릴 수 있어요.", "waitingForWorker": "백그라운드 작업자를 기다리고 있어요. 계속 그대로라면 서버의 작업자가 실행 중이 아닐 수 있어요.", "errorWorkerUnavailable": "지금은 백그라운드 작업자를 사용할 수 없어요. 잠시 후 다시 시도하거나 서버 운영자에게 확인을 요청하세요.", - "errorTimedOut": "AI 응답 시간 설정보다 읽기가 오래 걸렸어요. 다시 시도하거나 AI 설정에서 응답 시간을 늘려 주세요." + "errorTimedOut": "AI 응답 시간 설정보다 읽기가 오래 걸렸어요. 다시 시도하거나 AI 설정에서 응답 시간을 늘려 주세요.", + "errorReaderBusy": "문서 판독기가 다른 작업으로 바빠요. 1분 후에 다시 시도하세요." }, "symptoms": { "add": "새 증상", diff --git a/messages/nl.json b/messages/nl.json index 9bd5560ec..33897f904 100644 --- a/messages/nl.json +++ b/messages/nl.json @@ -175,6 +175,9 @@ "failed": "Het etiket kon niet worden gelezen. Probeer een scherpere foto." } } + }, + "aiRuns": { + "readerBusy": "De documentlezer is met ander werk bezig. Probeer het over een minuut opnieuw." } }, "doctorReport": { @@ -13258,7 +13261,8 @@ "backgroundScan": "Wordt op de achtergrond gelezen. Met een traag model kan dit een paar minuten duren.", "waitingForWorker": "Wachten op het achtergrondproces. Als dit niet verandert, draait het achtergrondproces van de server mogelijk niet.", "errorWorkerUnavailable": "Het achtergrondproces is op dit moment niet beschikbaar. Probeer het zo opnieuw, of vraag de beheerder van de server om het te controleren.", - "errorTimedOut": "Het lezen duurde langer dan je ingestelde AI-responstijd toestaat. Probeer het opnieuw, of verhoog de responstijd in de AI-instellingen." + "errorTimedOut": "Het lezen duurde langer dan je ingestelde AI-responstijd toestaat. Probeer het opnieuw, of verhoog de responstijd in de AI-instellingen.", + "errorReaderBusy": "De documentlezer is met ander werk bezig. Probeer het over een minuut opnieuw." }, "symptoms": { "add": "Nieuw symptoom", diff --git a/messages/pl.json b/messages/pl.json index 2a1673bcd..1c33d3938 100644 --- a/messages/pl.json +++ b/messages/pl.json @@ -175,6 +175,9 @@ "failed": "Nie udało się odczytać etykiety. Spróbuj ostrzejszego zdjęcia." } } + }, + "aiRuns": { + "readerBusy": "Czytnik dokumentów jest zajęty innymi zadaniami. Spróbuj ponownie za minutę." } }, "doctorReport": { @@ -13258,7 +13261,8 @@ "backgroundScan": "Odczyt w tle. Przy wolnym modelu może to potrwać kilka minut.", "waitingForWorker": "Oczekiwanie na proces w tle. Jeśli nic się nie zmienia, worker na serwerze może nie działać.", "errorWorkerUnavailable": "Proces w tle jest teraz niedostępny. Spróbuj ponownie za chwilę albo poproś administratora serwera o sprawdzenie go.", - "errorTimedOut": "Odczyt trwał dłużej, niż pozwala ustawiony czas odpowiedzi AI. Spróbuj ponownie albo wydłuż czas odpowiedzi w ustawieniach AI." + "errorTimedOut": "Odczyt trwał dłużej, niż pozwala ustawiony czas odpowiedzi AI. Spróbuj ponownie albo wydłuż czas odpowiedzi w ustawieniach AI.", + "errorReaderBusy": "Czytnik dokumentów jest zajęty innymi zadaniami. Spróbuj ponownie za minutę." }, "symptoms": { "add": "Nowy objaw", diff --git a/messages/pt-BR.json b/messages/pt-BR.json index cc011e70a..60606bfc6 100644 --- a/messages/pt-BR.json +++ b/messages/pt-BR.json @@ -175,6 +175,9 @@ "failed": "Não foi possível ler o rótulo. Tente uma foto mais nítida." } } + }, + "aiRuns": { + "readerBusy": "O leitor de documentos está ocupado com outras tarefas. Tente novamente em um minuto." } }, "doctorReport": { @@ -13258,7 +13261,8 @@ "backgroundScan": "Lendo em segundo plano. Com um modelo lento, isso pode levar alguns minutos.", "waitingForWorker": "Aguardando o worker em segundo plano. Se isso não mudar, o worker do servidor pode não estar em execução.", "errorWorkerUnavailable": "O worker em segundo plano não está disponível agora. Tente novamente em instantes, ou peça ao operador do servidor para verificá-lo.", - "errorTimedOut": "A leitura demorou mais do que o seu tempo de resposta de IA permite. Tente novamente, ou aumente o tempo de resposta nas configurações de IA." + "errorTimedOut": "A leitura demorou mais do que o seu tempo de resposta de IA permite. Tente novamente, ou aumente o tempo de resposta nas configurações de IA.", + "errorReaderBusy": "O leitor de documentos está ocupado com outras tarefas. Tente novamente em um minuto." }, "symptoms": { "add": "Novo sintoma", diff --git a/src/components/documents/use-document-assist.ts b/src/components/documents/use-document-assist.ts index c47fb646f..999590bca 100644 --- a/src/components/documents/use-document-assist.ts +++ b/src/components/documents/use-document-assist.ts @@ -228,6 +228,8 @@ export function documentAiErrorKey(err: unknown): string { return "aiRuns.errorWorkerUnavailable"; case "aiRuns.timedOut": return "aiRuns.errorTimedOut"; + case "aiRuns.readerBusy": + return "aiRuns.errorReaderBusy"; case "consent.ai.required": return "documents.assist.errorConsent"; case "ai.provider.none": diff --git a/src/components/labs/ocr-review-dialog.tsx b/src/components/labs/ocr-review-dialog.tsx index e98317b24..f2310f561 100644 --- a/src/components/labs/ocr-review-dialog.tsx +++ b/src/components/labs/ocr-review-dialog.tsx @@ -117,6 +117,8 @@ export function extractErrorMessage( return t("aiRuns.errorWorkerUnavailable"); case "aiRuns.timedOut": return t("aiRuns.errorTimedOut"); + case "aiRuns.readerBusy": + return t("aiRuns.errorReaderBusy"); default: break; } diff --git a/src/lib/documents/ai-route-support.ts b/src/lib/documents/ai-route-support.ts index 5a903e484..9f89710f8 100644 --- a/src/lib/documents/ai-route-support.ts +++ b/src/lib/documents/ai-route-support.ts @@ -154,9 +154,14 @@ export type VisionInput = // Refs #776 — `rasterFailed` (this PDF would not render on a raster- // capable build) is distinct from `pdfNeedsAnthropic` (this build / // provider cannot take a PDF at all): they need different fixes, so - // they must not share a label. + // they must not share a label. `readerBusy` is neither: the reader + // could not take the document now, and the work is tried again. reason: - "decryptFailed" | "fileType" | "pdfNeedsAnthropic" | "rasterFailed"; + | "decryptFailed" + | "fileType" + | "pdfNeedsAnthropic" + | "rasterFailed" + | "readerBusy"; }; /** @@ -211,6 +216,9 @@ export async function prepareVisionInput( if (raster.ok) { return { ok: true, images: raster.images, documents: [] }; } + if (raster.reason === "busy" || raster.reason === "interrupted") { + return { ok: false, reason: "readerBusy" }; + } // Refs #776 — a build that cannot rasterize at all is a capability gap a // PDF-native (Anthropic) provider would cover; a render failure on a // capable build is a property of THIS document. Report them apart. diff --git a/src/lib/documents/ai-runs/extract-run.ts b/src/lib/documents/ai-runs/extract-run.ts index c968e0d54..3d34a1768 100644 --- a/src/lib/documents/ai-runs/extract-run.ts +++ b/src/lib/documents/ai-runs/extract-run.ts @@ -52,6 +52,7 @@ import { dateOnlyAtNoonUtc } from "@/lib/tz/date-only"; import { refusalAsFailure } from "./http"; import { meteredReadBudget } from "./metered-budget"; import { + readerBusyFailure, runFailure, unreadableDocument, type DocumentTextPick, @@ -306,7 +307,9 @@ export async function executeDocumentExtract(args: { source = { mode: "text", text: input.text }; } const plan = await planInboundReads(source, schedule); - if (!plan.ok) return refused(pdfUnreadable()); + if (!plan.ok) { + return refused(plan.busy ? readerBusyFailure() : pdfUnreadable()); + } await schedule.arm(plan.args.chunks.length * OCR_EXTRACT_MODEL_CALLS); let result: InboundExtractionResult; diff --git a/src/lib/documents/ai-runs/index-run.ts b/src/lib/documents/ai-runs/index-run.ts index 64cf2251c..c04c6ec43 100644 --- a/src/lib/documents/ai-runs/index-run.ts +++ b/src/lib/documents/ai-runs/index-run.ts @@ -32,6 +32,7 @@ import { recordIndexAttempt } from "@/lib/documents/index-document"; import type { requireDocumentVisionProvider } from "@/lib/documents/provider-order"; import { annotate } from "@/lib/logging/context"; +import { readerBusyFailure } from "./run-support"; import { settleAiRunBudget } from "./store"; import type { AiRunBudget, @@ -157,6 +158,8 @@ export async function executeDocumentIndex(args: { // gone. await settleAiRunBudget(userId, budget, 0, null); await refundDocumentAiSlot(userId); + // A busy reader says nothing about the document: no attempt is recorded. + if (vision.reason === "readerBusy") return readerBusyFailure(); if (vision.reason === "pdfNeedsAnthropic") { await recordIndexAttempt(userId, document.id, { indexed: false, diff --git a/src/lib/documents/ai-runs/run-support.ts b/src/lib/documents/ai-runs/run-support.ts index 340f86b10..0aa291830 100644 --- a/src/lib/documents/ai-runs/run-support.ts +++ b/src/lib/documents/ai-runs/run-support.ts @@ -13,7 +13,7 @@ import type { } from "@/lib/documents/provider-order"; import type { VisionInput } from "@/lib/documents/ai-route-support"; -import type { AiRunOutcome } from "./types"; +import { AI_RUN_ERROR_CODES, type AiRunOutcome } from "./types"; export type DocumentVisionPick = Awaited< ReturnType @@ -44,6 +44,19 @@ export function unreadableDocument(parseError: boolean): AiRunOutcome { }); } +/** + * The document reader was busy with other work for longer than a person's + * read waits, or its process ended for no reason it measured. Nothing was + * read and nothing is charged; asking again in a minute is the answer. + */ +export function readerBusyFailure(): AiRunOutcome { + return runFailure({ + status: 503, + message: "The document reader is busy. Try again in a minute.", + errorCode: AI_RUN_ERROR_CODES.readerBusy, + }); +} + /** * A stored original that could not become a provider input, as the summary * and suggest routes have always answered it. @@ -59,6 +72,7 @@ export function visionPreparationFailure( errorCode: "documents.inbound.pdfNeedsAnthropic", }); } + if (reason === "readerBusy") return readerBusyFailure(); if (reason === "fileType") { return runFailure({ status: 422, diff --git a/src/lib/documents/ai-runs/types.ts b/src/lib/documents/ai-runs/types.ts index 69d22ddd6..79155281a 100644 --- a/src/lib/documents/ai-runs/types.ts +++ b/src/lib/documents/ai-runs/types.ts @@ -74,6 +74,7 @@ export const AI_RUN_ERROR_CODES = { workerUnavailable: "aiRuns.workerUnavailable", timedOut: "aiRuns.timedOut", failed: "aiRuns.failed", + readerBusy: "aiRuns.readerBusy", } as const; /** How long a queued run waits for a worker before it is failed. */ diff --git a/src/lib/documents/extract.ts b/src/lib/documents/extract.ts index 361faa61a..a1660adf5 100644 --- a/src/lib/documents/extract.ts +++ b/src/lib/documents/extract.ts @@ -484,7 +484,7 @@ export function planInboundTextReads( export async function planInboundReads( source: InboundReadSource, schedule: OcrSchedule, -): Promise<({ ok: true } & InboundReads) | { ok: false }> { +): Promise<({ ok: true } & InboundReads) | { ok: false; busy?: true }> { if (source.mode === "text") { return { ok: true, ...planInboundTextReads(source.text, schedule) }; } @@ -494,7 +494,7 @@ export async function planInboundReads( source.pdfSupported, schedule, ); - if (!plan.ok) return { ok: false }; + if (!plan.ok) return plan; return { ok: true, args: { mode: "vision", chunks: plan.chunks, totalPages: plan.totalPages }, diff --git a/src/lib/documents/index-document.ts b/src/lib/documents/index-document.ts index 69ae2ff41..cd9f6e259 100644 --- a/src/lib/documents/index-document.ts +++ b/src/lib/documents/index-document.ts @@ -58,6 +58,7 @@ import { import { transcribeDocument } from "@/lib/documents/describe"; import { documentAutoReadEnabled } from "@/lib/documents/document-settings"; import { localExtractText } from "@/lib/documents/local-extract"; +import { PdfReaderUnavailableError } from "@/lib/documents/pdfjs"; import { resolveDocumentVisionProvider } from "@/lib/documents/provider-order"; import { decryptDocumentContent } from "@/lib/documents/store"; import { prisma } from "@/lib/db"; @@ -238,6 +239,12 @@ async function tryProviderIndex( if (vision.reason === "rasterFailed") { return { outcome: null, note: "raster-failed" }; } + if (vision.reason === "readerBusy") { + // The reader could not take the document now. Falling back to the + // local text layer would settle for the lesser read for good; the job + // runs again instead. + throw new PdfReaderUnavailableError("busy"); + } if (vision.reason === "pdfNeedsAnthropic") { return { outcome: null, note: "pdf-needs-anthropic" }; } diff --git a/src/lib/documents/ocr-plan.ts b/src/lib/documents/ocr-plan.ts index f46242133..b23a108f1 100644 --- a/src/lib/documents/ocr-plan.ts +++ b/src/lib/documents/ocr-plan.ts @@ -170,7 +170,12 @@ export type OcrVisionPlan = /** Set when the PDF is longer than the page cap: only the first pages are read. */ pageCoverage?: { read: number; total: number }; } - | { ok: false }; + /** + * `busy`: the reader could not take the PDF now (busy past the wait a read + * gets, or its process ended for no measured reason). Not a verdict on the + * file; the person asks again. + */ + | { ok: false; busy?: true }; /** * Plan the reads of an image or a PDF. Every PDF is rendered first: a report @@ -226,7 +231,12 @@ export async function planVisionReads( : {}), }; } - if (!pdfSupported) return { ok: false }; + if (!pdfSupported) { + return !raster.ok && + (raster.reason === "busy" || raster.reason === "interrupted") + ? { ok: false, busy: true } + : { ok: false }; + } return { ok: true, chunks: [ diff --git a/src/lib/jobs/__tests__/document-ai-run.test.ts b/src/lib/jobs/__tests__/document-ai-run.test.ts index a4cd54d32..7de034a2d 100644 --- a/src/lib/jobs/__tests__/document-ai-run.test.ts +++ b/src/lib/jobs/__tests__/document-ai-run.test.ts @@ -285,6 +285,32 @@ describe("runDocumentAiRun", () => { ); }); + it("reads as interactive work of the run's owner, so the document reader serves it first", async () => { + let seen: unknown = null; + executeDocumentIndex.mockImplementation(async () => { + const { currentPdfWork } = await import("@/lib/documents/pdf-isolation"); + seen = currentPdfWork(); + return { ok: true, data: {} }; + }); + await runDocumentAiRun("run1", NOW + 2_700_000, now); + expect(seen).toEqual({ owner: "u1", lane: "interactive" }); + }); + + it("answers a busy document reader with a retryable code and hands the reservation back", async () => { + const { PdfReaderUnavailableError } = await import("@/lib/documents/pdfjs"); + executeDocumentIndex.mockRejectedValue( + new PdfReaderUnavailableError("busy"), + ); + expect(await runDocumentAiRun("run1", NOW + 2_700_000, now)).toBe("failed"); + expect(settleAiRunBudget).toHaveBeenCalledWith("u1", budget, 0, null); + expect(failAiRun).toHaveBeenCalledWith( + "run1", + expect.objectContaining({ status: 503, errorCode: "aiRuns.readerBusy" }), + "RUNNING", + new Date(NOW), + ); + }); + it("fails a run whose document is gone and hands the reservation back", async () => { loadOwnedDocument.mockResolvedValue(null); expect(await runDocumentAiRun("run1", NOW + 2_700_000, now)).toBe("failed"); diff --git a/src/lib/jobs/document-ai-run.ts b/src/lib/jobs/document-ai-run.ts index 4df270a6a..69949a2b0 100644 --- a/src/lib/jobs/document-ai-run.ts +++ b/src/lib/jobs/document-ai-run.ts @@ -42,6 +42,8 @@ import { loadOwnedDocument } from "@/lib/documents/ai-route-support"; import { loadDocumentChatText } from "@/lib/documents/content-index"; import { refusalAsFailure } from "@/lib/documents/ai-runs/http"; import { executeDocumentExtract } from "@/lib/documents/ai-runs/extract-run"; +import { withPdfWork } from "@/lib/documents/pdf-isolation"; +import { PdfReaderUnavailableError } from "@/lib/documents/pdfjs"; import { DOCUMENT_INDEX_MODEL_CALLS, executeDocumentIndex, @@ -614,7 +616,12 @@ export async function runDocumentAiRun( let outcome: AiRunOutcome; try { - outcome = await RUN_DISPATCH[run.kind](run, deadlineAt, now); + // A person asked for this read and is waiting on it: the document + // reader serves it ahead of background work, in turn with other people. + outcome = await withPdfWork( + { owner: run.userId, lane: "interactive" }, + () => RUN_DISPATCH[run.kind](run, deadlineAt, now), + ); } catch (err) { // Everything that throws here threw before the body took over the // reservation, so it goes back here. @@ -622,6 +629,13 @@ export async function runDocumentAiRun( const failure: AiRunFailure = (err instanceof RunRefused ? err.failure : null) ?? + (err instanceof PdfReaderUnavailableError + ? { + status: 503, + errorCode: AI_RUN_ERROR_CODES.readerBusy, + message: "The document reader is busy. Try again in a minute.", + } + : null) ?? refusalAsFailure(err) ?? { status: 500, errorCode: AI_RUN_ERROR_CODES.failed, diff --git a/src/lib/jobs/document-content-index-backfill.ts b/src/lib/jobs/document-content-index-backfill.ts index 45fc3d078..35a062761 100644 --- a/src/lib/jobs/document-content-index-backfill.ts +++ b/src/lib/jobs/document-content-index-backfill.ts @@ -55,6 +55,7 @@ import { upsertContentIndex, } from "@/lib/documents/content-index"; import { transcribeDocument } from "@/lib/documents/describe"; +import { withPdfWork } from "@/lib/documents/pdf-isolation"; import { getGlobalBoss } from "@/lib/jobs/boss-instance"; import { resolveDocumentVisionProvider } from "@/lib/documents/provider-order"; import { annotate } from "@/lib/logging/context"; @@ -320,7 +321,10 @@ export async function runContentIndexBackfillForUser( skipped += 1; continue; } - const vision = await prepareVisionInput(document, pick.pdfSupported); + const vision = await withPdfWork( + { owner: userId, lane: "background" }, + () => prepareVisionInput(document, pick.pdfSupported), + ); if (!vision.ok) { // Not vision-indexable after all (undecryptable, unreadable bytes, // or a PDF the rasteriser could not render) — refund and move on so diff --git a/src/lib/jobs/document-index.ts b/src/lib/jobs/document-index.ts index 710e3552c..ca5d027e9 100644 --- a/src/lib/jobs/document-index.ts +++ b/src/lib/jobs/document-index.ts @@ -15,6 +15,7 @@ import { STATUS_MODEL_CALLS } from "@/lib/ai/ai-budgets"; import { maybeAutoStageLabFacts } from "@/lib/documents/auto-stage-labs"; import { indexDocumentContent } from "@/lib/documents/index-document"; +import { withPdfWork } from "@/lib/documents/pdf-isolation"; import type { OcrSchedule } from "@/lib/documents/ocr-plan"; import { ocrReadCallMs } from "@/lib/documents/ocr-read"; import { prisma } from "@/lib/db"; @@ -50,9 +51,9 @@ export async function runDocumentIndex( const { userId, documentId } = payload; if (!userId || !documentId) return; const localOnly = payload.localOnly === true; - const outcome = await indexDocumentContent(userId, documentId, { - localOnly, - }); + const outcome = await withPdfWork({ owner: userId, lane: "background" }, () => + indexDocumentContent(userId, documentId, { localOnly }), + ); annotate({ action: { name: "documents.autoIndex.run" }, meta: outcome.indexed @@ -72,9 +73,9 @@ export async function runDocumentIndex( options.deadlineAt === undefined ? undefined : await stagingSchedule(userId, options.deadlineAt, options.now); - await maybeAutoStageLabFacts(userId, documentId, { schedule }).catch( - caughtAs("documents.auto_index.lab_staging_failed"), - ); + await withPdfWork({ owner: userId, lane: "background" }, () => + maybeAutoStageLabFacts(userId, documentId, { schedule }), + ).catch(caughtAs("documents.auto_index.lab_staging_failed")); } } diff --git a/src/lib/jobs/document-summary.ts b/src/lib/jobs/document-summary.ts index a1a9bbb84..9b421da34 100644 --- a/src/lib/jobs/document-summary.ts +++ b/src/lib/jobs/document-summary.ts @@ -43,6 +43,8 @@ import type { Locale } from "@/lib/i18n/config"; import { resolveJobLocale } from "@/lib/i18n/job-locale"; import { documentAutoReadEnabled } from "@/lib/documents/document-settings"; import { resolveDocumentVisionProvider } from "@/lib/documents/provider-order"; +import { withPdfWork } from "@/lib/documents/pdf-isolation"; +import { PdfReaderUnavailableError } from "@/lib/documents/pdfjs"; import { encryptDocumentSummary } from "@/lib/documents/store"; import { prisma } from "@/lib/db"; import { getGlobalBoss } from "@/lib/jobs/boss-instance"; @@ -81,6 +83,25 @@ export interface DocumentSummaryPayload { enqueuedAt?: string; } +/** + * The stored original as provider input, read as this account's background + * work. A busy reader is not a property of the document: the job throws, and + * pg-boss runs it again. + */ +async function prepareSummaryVision( + userId: string, + document: Parameters[0], + pick: { pdfSupported: boolean }, +): Promise> { + const vision = await withPdfWork({ owner: userId, lane: "background" }, () => + prepareVisionInput(document, pick.pdfSupported), + ); + if (!vision.ok && vision.reason === "readerBusy") { + throw new PdfReaderUnavailableError("busy"); + } + return vision; +} + /** * Generate + persist ONE document's background summary (owner-scoped). Never * throws for an expected outcome — every gate that fails resolves to a tagged @@ -210,7 +231,7 @@ export async function runDocumentSummaryJob( const document = await loadOwnedDocument(userId, documentId); if (!document) return; - const vision = await prepareVisionInput(document, pick.pdfSupported); + const vision = await prepareSummaryVision(userId, document, pick); if (!vision.ok) { // Cannot read this file with the picked provider (decrypt miss, unsupported // type, or a PDF needing an Anthropic provider). Graceful no-op. diff --git a/src/lib/labs/ocr-run.ts b/src/lib/labs/ocr-run.ts index 75b668be5..b122cd55b 100644 --- a/src/lib/labs/ocr-run.ts +++ b/src/lib/labs/ocr-run.ts @@ -20,6 +20,7 @@ import { AI_BUDGETS } from "@/lib/ai/ai-budgets"; import { refusalAsFailure } from "@/lib/documents/ai-runs/http"; import type { ProviderChainType } from "@/lib/ai/provider-chain"; import { meteredReadBudget } from "@/lib/documents/ai-runs/metered-budget"; +import { readerBusyFailure } from "@/lib/documents/ai-runs/run-support"; import { AI_RUN_ERROR_CODES, type AiRunBudget, @@ -221,6 +222,10 @@ export async function executeOcrExtraction(args: { input.pick.pdfSupported, schedule, ); + if (!plan.ok && plan.busy) { + await settle(false); + return readerBusyFailure(); + } if (!plan.ok) { annotate({ action: { name: "labs.ocr.fileRejected" }, diff --git a/src/lib/medications/ingredients/label-extract.ts b/src/lib/medications/ingredients/label-extract.ts index 14430b10c..2379f5afd 100644 --- a/src/lib/medications/ingredients/label-extract.ts +++ b/src/lib/medications/ingredients/label-extract.ts @@ -29,6 +29,7 @@ import { type AIProvider, type CompletionParams, } from "@/lib/ai/types"; +import { readerBusyFailure } from "@/lib/documents/ai-runs/run-support"; import { settleAiRunBudget } from "@/lib/documents/ai-runs/store"; import type { AiRunBudget, @@ -232,6 +233,13 @@ export async function executeSupplementLabelExtraction(args: { ]; } else { const raster = await rasterizePdf(bytes); + if ( + !raster.ok && + (raster.reason === "busy" || raster.reason === "interrupted") + ) { + await settleAiRunBudget(userId, budget, 0, null); + return readerBusyFailure(); + } if (!raster.ok) { await settleAiRunBudget(userId, budget, 0, null); return failure({ diff --git a/src/lib/openapi/error-codes.ts b/src/lib/openapi/error-codes.ts index 2b49765fd..a36ad0286 100644 --- a/src/lib/openapi/error-codes.ts +++ b/src/lib/openapi/error-codes.ts @@ -57,6 +57,7 @@ export const ERROR_CODE_CATALOGUE: Readonly> = aiRuns: [ "aiRuns.failed", "aiRuns.notFound", + "aiRuns.readerBusy", "aiRuns.timedOut", "aiRuns.workerUnavailable", ], diff --git a/src/lib/openapi/routes/ai-runs.ts b/src/lib/openapi/routes/ai-runs.ts index 1be07ade9..8be799378 100644 --- a/src/lib/openapi/routes/ai-runs.ts +++ b/src/lib/openapi/routes/ai-runs.ts @@ -64,7 +64,7 @@ const aiRun = z }) .nullable() .describe( - "Present once `status` is `FAILED`: the `meta.errorCode`, message and HTTP status the queuing route would have answered with, or one of the run's own codes: `aiRuns.workerUnavailable` (no worker took the run within fifteen minutes), `aiRuns.timedOut` (the read outlived the calls the AI response time allows), `aiRuns.failed` (anything else).", + "Present once `status` is `FAILED`: the `meta.errorCode`, message and HTTP status the queuing route would have answered with, or one of the run's own codes: `aiRuns.workerUnavailable` (no worker took the run within fifteen minutes), `aiRuns.timedOut` (the read outlived the calls the AI response time allows), `aiRuns.readerBusy` (the server's document reader was busy past the wait a read gets, or stopped before it answered; nothing was read or charged, try again in a minute), `aiRuns.failed` (anything else).", ), createdAt: z.string(), startedAt: z.string().nullable(), From 5a875d284079f7938ebdf5e4ed2182fa6fb9216f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 25 Oct 2026 17:11:55 +0100 Subject: [PATCH 08/13] fix(auth): issue a Secure session cookie under the __Host- prefix A cookie named healthlog_session can be set by any host under the same registrable domain, so a page on a sibling subdomain could plant a session of its choosing in a visitor's browser. Wherever the session cookie is Secure it is now issued as __Host-healthlog_session, which a browser accepts only from this exact host. Plain-HTTP installs, which cannot hold a __Host- cookie, keep the old name. Nobody is signed out: the old name is still read after the new one, a session found under it is re-issued under the new name and the old cookie expired, and a browser holding the new cookie is never answered from the old one. Every reader and writer goes through one module, which the proxy, the native-caller check, the native handoff and the OIDC callback use; the session-surface guard follows the new reader. --- docs/api/README.md | 9 +- docs/api/openapi.yaml | 4 +- docs/ops/measurement-maintenance.md | 4 +- .../proxy-onboarding-redirect.test.ts | 10 ++ src/__tests__/session-surface-guard.test.ts | 9 +- .../native/complete/__tests__/route.test.ts | 1 - src/app/api/auth/native/complete/route.ts | 18 +-- .../oidc/callback/__tests__/route.test.ts | 1 - src/app/api/auth/oidc/callback/route.ts | 10 +- src/lib/auth/__tests__/native-client.test.ts | 11 ++ src/lib/auth/__tests__/session.test.ts | 129 ++++++++++++++-- src/lib/auth/native-client.ts | 14 +- src/lib/auth/session-cookie.ts | 139 ++++++++++++++++++ src/lib/auth/session.ts | 53 ++++--- src/lib/openapi/routes/index.ts | 3 +- src/proxy.ts | 4 +- .../parity/oidc-callback.parity.test.ts | 6 +- tests/kit/call-route.ts | 6 +- tests/kit/factories.ts | 5 +- 19 files changed, 348 insertions(+), 88 deletions(-) create mode 100644 src/lib/auth/session-cookie.ts diff --git a/docs/api/README.md b/docs/api/README.md index 507e31faf..ad6ddfaaf 100644 --- a/docs/api/README.md +++ b/docs/api/README.md @@ -82,9 +82,12 @@ before treating a warning as a regression. v1.30.17. The path still lists (`GET`) and revokes (`DELETE /api/tokens/{id}`) every token above, whichever surface issued it. - - `sessionCookie` — HttpOnly `healthlog_session` cookie set by the - password and passkey login flows. Used by the web app and any iOS - client running inside a web view. + - `sessionCookie` — HttpOnly session cookie set by the password and + passkey login flows. Used by the web app and any iOS client running + inside a web view. It is named `__Host-healthlog_session` wherever it is + `Secure` (every HTTPS install), so no other host under the same domain + can set it; a plain-HTTP install keeps `healthlog_session`. A cookie + under the old name is still read and is re-issued under the new one. - **`components.schemas`** — DTOs for the documented routes plus the underlying Prisma models (e.g. `Measurement`, `Medication`, `MedicationIntakeEvent`, `MoodEntry`, `User`, `DashboardLayout`, diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 780128da3..a0c7d6a89 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -94543,4 +94543,6 @@ components: type: apiKey in: cookie name: healthlog_session - description: Browser session cookie — set by /api/auth/login. + description: Browser session cookie — set by /api/auth/login. Named `__Host-healthlog_session` wherever the cookie is + `Secure` (every HTTPS install); a plain-HTTP install, which cannot hold a `__Host-` cookie, keeps + `healthlog_session`. diff --git a/docs/ops/measurement-maintenance.md b/docs/ops/measurement-maintenance.md index c43caa52d..e0bce3094 100644 --- a/docs/ops/measurement-maintenance.md +++ b/docs/ops/measurement-maintenance.md @@ -107,10 +107,12 @@ In the admin area, or with a signed-in admin session: ```sh curl -X POST https:///api/admin/maintenance/measurements \ -H 'content-type: application/json' \ - --cookie 'healthlog_session=' \ + --cookie '__Host-healthlog_session=' \ -d '{"vacuum": true, "reindex": true}' ``` +On a plain-HTTP install the cookie is named `healthlog_session`. + The answer is `202` with `{ "enqueued": true }`, or `enqueued: false` when a run is already queued or running. A Bearer token is refused: the route is cookie-only, like every admin route. Without a background worker the route diff --git a/src/__tests__/proxy-onboarding-redirect.test.ts b/src/__tests__/proxy-onboarding-redirect.test.ts index 7047d1a4c..ee609e893 100644 --- a/src/__tests__/proxy-onboarding-redirect.test.ts +++ b/src/__tests__/proxy-onboarding-redirect.test.ts @@ -75,6 +75,16 @@ describe("proxy.ts onboarding redirect (v1.4.22 C4)", () => { expect(res.headers.get("location")).toBeNull(); }); + it("counts the __Host- session cookie as a session at the page gate", () => { + const res = proxy( + makeRequest("/insights", { "__Host-healthlog_session": "sess-1" }), + ); + expect(res.status).toBe(200); + expect(res.headers.get("location")).toBeNull(); + const anonymous = proxy(makeRequest("/insights")); + expect(anonymous.headers.get("location")).toMatch(/\/auth\/login$/); + }); + it("passes through when hl_onboarding cookie is absent (completed users)", () => { const res = proxy(makeRequest("/", { healthlog_session: "sess-1" })); expect(res.status).toBe(200); diff --git a/src/__tests__/session-surface-guard.test.ts b/src/__tests__/session-surface-guard.test.ts index c97358181..7467f1473 100644 --- a/src/__tests__/session-surface-guard.test.ts +++ b/src/__tests__/session-surface-guard.test.ts @@ -190,10 +190,13 @@ describe("S1 — the direct session-resolution set is frozen", () => { /\b(getSession|getUnswitchedSession|getSessionUserLocale|validateSessionFromCookieValue|validateSessionWithCreatedAt|loadOnboardingFlowState)\s*\(/; /** - * Reaching for the cookie by name is the way around the helpers. Both the - * exported constant and the raw literal count. + * Reaching for the cookie by name is the way around the helpers. The + * reader (`readSessionCookie`, which tries both names), the exported names + * and either raw literal all count; the `__Host-` name joined the old one + * when a `Secure` session cookie started being issued under it. */ - const RAW_SESSION_COOKIE = /\bSESSION_COOKIE_NAME\b|"healthlog_session"/; + const RAW_SESSION_COOKIE = + /\b(readSessionCookie|SESSION_COOKIE_NAMES?|HOST_SESSION_COOKIE|LEGACY_SESSION_COOKIE)\b|"(__Host-)?healthlog_session"/; it("no file under src/app derives a session outside requireAuth unlisted", () => { const resolvers = appFilesMatching(SESSION_DERIVING_HELPERS); diff --git a/src/app/api/auth/native/complete/__tests__/route.test.ts b/src/app/api/auth/native/complete/__tests__/route.test.ts index e82eeed96..2fb1e6066 100644 --- a/src/app/api/auth/native/complete/__tests__/route.test.ts +++ b/src/app/api/auth/native/complete/__tests__/route.test.ts @@ -20,7 +20,6 @@ vi.mock("@/lib/rate-limit", () => ({ const validateSessionWithCreatedAt = vi.fn(); vi.mock("@/lib/auth/session", () => ({ validateSessionWithCreatedAt: (v: unknown) => validateSessionWithCreatedAt(v), - SESSION_COOKIE_NAME: "healthlog_session", })); // Keep the real shared core (buildHandoffCallbackUrl is used by diff --git a/src/app/api/auth/native/complete/route.ts b/src/app/api/auth/native/complete/route.ts index d2f19d5ce..8f1b40633 100644 --- a/src/app/api/auth/native/complete/route.ts +++ b/src/app/api/auth/native/complete/route.ts @@ -29,11 +29,11 @@ import { annotate } from "@/lib/logging/context"; import { checkAuthSurfaceRateLimit } from "@/lib/rate-limit"; import { prisma } from "@/lib/db"; import { auditLog } from "@/lib/auth/audit"; -import { shouldEmitSecureCookie } from "@/lib/auth/secure-cookie"; +import { validateSessionWithCreatedAt } from "@/lib/auth/session"; import { - validateSessionWithCreatedAt, - SESSION_COOKIE_NAME, -} from "@/lib/auth/session"; + expireSessionCookies, + readSessionCookie, +} from "@/lib/auth/session-cookie"; import { mintNativeHandoff } from "@/lib/auth/native-handoff"; import { NATIVE_HANDOFF_STATE_COOKIE, @@ -95,7 +95,7 @@ export const GET = apiHandler(async (req: NextRequest) => { // (row + expiry), never "a cookie is present". Projects `createdAt` so the // freshness comparison below is structurally always available. const authed = await validateSessionWithCreatedAt( - req.cookies.get(SESSION_COOKIE_NAME)?.value, + readSessionCookie(req.cookies)?.value, ); if (!authed) { annotate({ meta: { reason: "no_session" } }); @@ -140,13 +140,7 @@ export const GET = apiHandler(async (req: NextRequest) => { await prisma.session .delete({ where: { id: authed.session.id } }) .catch(() => {}); - response.cookies.set(SESSION_COOKIE_NAME, "", { - httpOnly: true, - secure: shouldEmitSecureCookie(), - sameSite: "lax", - maxAge: 0, - path: "/", - }); + expireSessionCookies(response.cookies); return response; }); diff --git a/src/app/api/auth/oidc/callback/__tests__/route.test.ts b/src/app/api/auth/oidc/callback/__tests__/route.test.ts index a01fa9212..161f0e687 100644 --- a/src/app/api/auth/oidc/callback/__tests__/route.test.ts +++ b/src/app/api/auth/oidc/callback/__tests__/route.test.ts @@ -42,7 +42,6 @@ vi.mock("@/lib/auth/audit", () => ({ auditLog: vi.fn() })); vi.mock("@/lib/auth/session", () => ({ createSession: vi.fn(), validateSessionFromCookieValue: vi.fn(), - SESSION_COOKIE_NAME: "healthlog_session", })); vi.mock("@/lib/auth/login-alert", () => ({ recordSignInDevice: vi.fn() })); vi.mock("@/lib/auth/mfa/challenge", () => ({ diff --git a/src/app/api/auth/oidc/callback/route.ts b/src/app/api/auth/oidc/callback/route.ts index 4b7b83ac8..bfe808080 100644 --- a/src/app/api/auth/oidc/callback/route.ts +++ b/src/app/api/auth/oidc/callback/route.ts @@ -3,10 +3,8 @@ import { NextRequest, NextResponse } from "next/server"; import { apiHandler } from "@/lib/api-handler"; import { annotate, getEvent } from "@/lib/logging/context"; import { checkAuthSurfaceRateLimit } from "@/lib/rate-limit"; -import { - validateSessionFromCookieValue, - SESSION_COOKIE_NAME, -} from "@/lib/auth/session"; +import { validateSessionFromCookieValue } from "@/lib/auth/session"; +import { readSessionCookie } from "@/lib/auth/session-cookie"; import { discoverOidcMetadata, exchangeCodeForTokens, @@ -225,7 +223,7 @@ export const GET = apiHandler(async (req: NextRequest) => { // session gains nothing here but a redirect to a page they could already // reach — no code redemption, no session mint, no identity change. const existingSession = await validateSessionFromCookieValue( - req.cookies.get(SESSION_COOKIE_NAME)?.value, + readSessionCookie(req.cookies)?.value, ); if (existingSession) { annotate({ meta: { reason: "duplicate_callback_authenticated" } }); @@ -258,7 +256,7 @@ export const GET = apiHandler(async (req: NextRequest) => { : null; if (isClaim) { const liveSession = await validateSessionFromCookieValue( - req.cookies.get(SESSION_COOKIE_NAME)?.value, + readSessionCookie(req.cookies)?.value, ); if (liveSession) { annotate({ meta: { reason: "claim_signed_in" } }); diff --git a/src/lib/auth/__tests__/native-client.test.ts b/src/lib/auth/__tests__/native-client.test.ts index 2accfde8b..c503bf9f7 100644 --- a/src/lib/auth/__tests__/native-client.test.ts +++ b/src/lib/auth/__tests__/native-client.test.ts @@ -124,6 +124,17 @@ describe("isCookielessNativeCaller (M-3 refresh-token issuance gate)", () => { ).toBe(false); }); + it("false when the session cookie arrives under its __Host- name", () => { + expect( + isCookielessNativeCaller( + h({ + "x-client-type": "native", + cookie: "foo=bar; __Host-healthlog_session=abc123", + }), + ), + ).toBe(false); + }); + it("true for a native caller whose cookie header lacks the session cookie", () => { expect( isCookielessNativeCaller( diff --git a/src/lib/auth/__tests__/session.test.ts b/src/lib/auth/__tests__/session.test.ts index 65d80fd7f..ecf516f9a 100644 --- a/src/lib/auth/__tests__/session.test.ts +++ b/src/lib/auth/__tests__/session.test.ts @@ -1,7 +1,9 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; const cookieState = vi.hoisted(() => ({ sessionId: undefined as string | undefined, + /** The session cookie under the `__Host-` name, when the browser holds one. */ + hostSessionId: undefined as string | undefined, delete: vi.fn(), set: vi.fn(), })); @@ -71,10 +73,15 @@ vi.mock("@/lib/auth/hmac", () => ({ vi.mock("next/headers", () => ({ cookies: vi.fn(async () => ({ - get: (name: string) => - name === "healthlog_session" && cookieState.sessionId - ? { value: cookieState.sessionId } - : undefined, + get: (name: string) => { + if (name === "healthlog_session" && cookieState.sessionId) { + return { value: cookieState.sessionId }; + } + if (name === "__Host-healthlog_session" && cookieState.hostSessionId) { + return { value: cookieState.hostSessionId }; + } + return undefined; + }, set: cookieState.set, delete: cookieState.delete, })), @@ -98,6 +105,16 @@ const UNKNOWN_SECRET = `hls_${"c".repeat(64)}`; const THIRTY_DAYS_MS = 30 * 24 * 60 * 60 * 1000; +/** Both session cookie names were expired, the `__Host-` one as `Secure`. */ +function expectSessionCookiesExpired() { + expect(cookieState.delete).toHaveBeenCalledWith("healthlog_session"); + expect(cookieState.set).toHaveBeenCalledWith( + "__Host-healthlog_session", + "", + expect.objectContaining({ maxAge: 0, path: "/", secure: true }), + ); +} + function seed(row: Partial & { id: string }): SessionRow { const full: SessionRow = { userId: "user-1", @@ -115,6 +132,7 @@ beforeEach(() => { vi.clearAllMocks(); store.sessions = []; cookieState.sessionId = "sess-expired"; + cookieState.hostSessionId = undefined; // `clearAllMocks` drops recorded calls but keeps implementations, so a // rejection staged by one test would otherwise leak into the next. Re-arm // the write mocks with a benign default each time. @@ -203,6 +221,97 @@ describe("getSession — cookie resolution", () => { }); }); +describe("the session cookie's name on a Secure install", () => { + // A cookie named `healthlog_session` can be planted by any host under the + // same registrable domain; one named `__Host-…` only by this host, and only + // when Secure, on Path=/, without Domain. + beforeEach(() => { + vi.stubEnv("SESSION_COOKIE_SECURE", "true"); + }); + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it("issues a new session as __Host-healthlog_session, Secure, on /, without Domain", async () => { + cookieState.sessionId = undefined; + await createSession("user-1", false); + const issued = cookieState.set.mock.calls.find( + (c) => c[0] === "__Host-healthlog_session", + ); + expect(issued?.[1]).toMatch(/^hls_[0-9a-f]{64}$/); + expect(issued?.[2]).toMatchObject({ secure: true, path: "/" }); + expect(issued?.[2]).not.toHaveProperty("domain"); + expect( + cookieState.set.mock.calls.some((c) => c[0] === "healthlog_session"), + ).toBe(false); + }); + + it("keeps a session found under the old name, and re-issues it under the new one", async () => { + seed({ id: "sess_cuid_1", tokenHash: `hash:${SECRET}` }); + cookieState.sessionId = SECRET; + + const result = await getSession(); + + expect(result?.session.id).toBe("sess_cuid_1"); + expect(cookieState.set).toHaveBeenCalledWith( + "__Host-healthlog_session", + SECRET, + expect.objectContaining({ secure: true, path: "/" }), + ); + expect(cookieState.delete).toHaveBeenCalledWith("healthlog_session"); + }); + + it("answers from the __Host- cookie when a planted old-name cookie rides along", async () => { + seed({ id: "sess_mine", tokenHash: `hash:${SECRET}` }); + seed({ + id: "sess_planted", + userId: "attacker", + tokenHash: `hash:${FORGED_SECRET}`, + }); + cookieState.hostSessionId = SECRET; + cookieState.sessionId = FORGED_SECRET; + + const result = await getSession(); + + expect(result?.session.id).toBe("sess_mine"); + }); + + it("logs out both names", async () => { + seed({ id: "sess_cuid_1", tokenHash: `hash:${SECRET}` }); + cookieState.hostSessionId = SECRET; + cookieState.sessionId = undefined; + await destroySession(); + expect(prisma.session.delete).toHaveBeenCalledWith({ + where: { id: "sess_cuid_1" }, + }); + expectSessionCookiesExpired(); + }); +}); + +describe("the session cookie's name on a plain-HTTP install", () => { + beforeEach(() => { + vi.stubEnv("SESSION_COOKIE_SECURE", "false"); + }); + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it("keeps the old name, which a browser accepts without Secure", async () => { + cookieState.sessionId = undefined; + await createSession("user-1", false); + expect( + cookieState.set.mock.calls.some( + (c) => c[0] === "healthlog_session" && c[2]?.secure === false, + ), + ).toBe(true); + expect( + cookieState.set.mock.calls.some( + (c) => c[0] === "__Host-healthlog_session", + ), + ).toBe(false); + }); +}); + describe("getSession — sliding expiry", () => { it("extends a modern session and re-emits the same secret", async () => { seed({ @@ -257,7 +366,7 @@ describe("getSession", () => { expect(prisma.session.deleteMany).toHaveBeenCalledWith({ where: { id: "sess-expired" }, }); - expect(cookieState.delete).toHaveBeenCalledWith("healthlog_session"); + expectSessionCookiesExpired(); expect(cookieState.delete).toHaveBeenCalledWith("hl_onboarding"); }); }); @@ -385,7 +494,7 @@ describe("a session lookup that fails is not a missing session", () => { cookieState.sessionId = SECRET; failLookup(); await expect(destroySession()).resolves.toBeUndefined(); - expect(cookieState.delete).toHaveBeenCalledWith("healthlog_session"); + expectSessionCookiesExpired(); }); }); @@ -401,7 +510,7 @@ describe("destroySession", () => { expect(prisma.session.delete).toHaveBeenCalledWith({ where: { id: "sess_cuid_1" }, }); - expect(cookieState.delete).toHaveBeenCalledWith("healthlog_session"); + expectSessionCookiesExpired(); }); it("logs a legacy cookie out too", async () => { @@ -427,7 +536,7 @@ describe("destroySession", () => { expect(prisma.session.delete).toHaveBeenCalledWith({ where: { id: "sess-gone" }, }); - expect(cookieState.delete).toHaveBeenCalledWith("healthlog_session"); + expectSessionCookiesExpired(); expect(cookieState.delete).toHaveBeenCalledWith("hl_onboarding"); }); @@ -441,7 +550,7 @@ describe("destroySession", () => { // A non-P2025 delete failure is recorded on the wide event, not thrown, // so logout never leaves the client authenticated with the cookie intact. await expect(destroySession()).resolves.toBeUndefined(); - expect(cookieState.delete).toHaveBeenCalledWith("healthlog_session"); + expectSessionCookiesExpired(); expect(cookieState.delete).toHaveBeenCalledWith("hl_onboarding"); }); }); diff --git a/src/lib/auth/native-client.ts b/src/lib/auth/native-client.ts index c1df65ffc..9953a4cb2 100644 --- a/src/lib/auth/native-client.ts +++ b/src/lib/auth/native-client.ts @@ -16,6 +16,7 @@ * - `X-Client-Type: native` always forces native policy. * - `X-Client-Type: web` always forces web policy. */ +import { cookieHeaderCarriesSession } from "./session-cookie"; export type ClientPolicy = "web" | "native"; @@ -95,13 +96,6 @@ export function resolveTokenPolicy(headers: Headers): TokenPolicyDecision { }; } -/** - * Name of the browser session cookie (mirrors `SESSION_COOKIE` in - * `src/lib/auth/session.ts`). A request that round-trips this cookie is a - * browser, not a genuine cookie-less native caller. - */ -const SESSION_COOKIE_NAME = "healthlog_session"; - /** * True only for a genuinely cookie-less native caller — the gate for * *issuing* a 60-day refresh token (M-3 hardening). @@ -110,7 +104,7 @@ const SESSION_COOKIE_NAME = "healthlog_session"; * a browser context (DOM/XSS-reachable), even when that browser spoofs * `X-Client-Type: native`. Two browser tells suppress issuance: * - a `Mozilla/`-bearing User-Agent (every real browser sends one), and - * - an inbound `healthlog_session` cookie (a browser round-trips its + * - an inbound session cookie, under either of its names (a browser round-trips its * session cookie; the native client authenticates by Bearer and never * holds one). * @@ -127,9 +121,7 @@ export function isCookielessNativeCaller(headers: Headers): boolean { // Belt-and-braces: a request that presents the session cookie is a // browser round-trip regardless of UA. Native callers never hold one. - const cookie = headers.get("cookie") ?? ""; - if (cookie.split(/;\s*/).some((c) => c.startsWith(`${SESSION_COOKIE_NAME}=`))) - return false; + if (cookieHeaderCarriesSession(headers.get("cookie"))) return false; return true; } diff --git a/src/lib/auth/session-cookie.ts b/src/lib/auth/session-cookie.ts new file mode 100644 index 000000000..49a648b0b --- /dev/null +++ b/src/lib/auth/session-cookie.ts @@ -0,0 +1,139 @@ +/** + * The browser session cookie's name, and the one way to read and clear it. + * + * A cookie named `healthlog_session` can be set by any host under the same + * registrable domain: a page on `other.example.com` may write one with + * `Domain=example.com`, and the browser sends it to `health.example.com` + * alongside, or instead of, the server's own. That lets a sibling subdomain + * plant a session of its choosing in a visitor's browser. A cookie whose name + * starts with `__Host-` is accepted by the browser only when it is `Secure`, + * has `Path=/` and carries no `Domain`, so only this exact host can have set + * it. + * + * So wherever the session cookie is `Secure` (`shouldEmitSecureCookie()`), + * it is issued as `__Host-healthlog_session`. A plain-HTTP install cannot + * hold a `__Host-` cookie and keeps the old name. + * + * The transition signs nobody out: the old name is still read, after the new + * one, and `getSession()` re-issues a session found under the old name under + * the new one and expires the old cookie. A browser that holds the new + * cookie is never answered from the old one, so once a person has been seen + * once after the upgrade, a planted old-name cookie no longer reaches them. + * + * Nothing here imports the database, so the proxy can use it. + */ +import { shouldEmitSecureCookie } from "./secure-cookie"; + +/** The name every release before this one used, and plain-HTTP installs keep. */ +export const LEGACY_SESSION_COOKIE = "healthlog_session"; + +/** The name a `Secure` session cookie is issued under. */ +export const HOST_SESSION_COOKIE = "__Host-healthlog_session"; + +/** The names a session cookie is read under, in the order they are tried. */ +export const SESSION_COOKIE_NAMES = [ + HOST_SESSION_COOKIE, + LEGACY_SESSION_COOKIE, +] as const; + +export type SessionCookieName = (typeof SESSION_COOKIE_NAMES)[number]; + +/** The name a session cookie issued now is set under. */ +export function sessionCookieName(): SessionCookieName { + return shouldEmitSecureCookie() ? HOST_SESSION_COOKIE : LEGACY_SESSION_COOKIE; +} + +interface CookieReader { + get(name: string): { value: string } | undefined; +} + +interface CookieStore extends CookieReader, CookieWriter { + delete(name: string): unknown; +} + +interface CookieWriter { + set( + name: string, + value: string, + options: { + httpOnly: boolean; + secure: boolean; + sameSite: "lax"; + maxAge: number; + path: string; + }, + ): unknown; +} + +/** The session cookie a request carries, under whichever name it came. */ +export function readSessionCookie( + jar: CookieReader, +): { name: SessionCookieName; value: string } | null { + for (const name of SESSION_COOKIE_NAMES) { + const value = jar.get(name)?.value; + if (value) return { name, value }; + } + return null; +} + +/** Whether a raw `Cookie` header carries a session cookie under either name. */ +export function cookieHeaderCarriesSession(header: string | null): boolean { + if (!header) return false; + return header + .split(/;\s*/) + .some((pair) => + SESSION_COOKIE_NAMES.some((name) => pair.startsWith(`${name}=`)), + ); +} + +/** The attributes the session cookie is issued with. */ +export function sessionCookieOptions(maxAgeSeconds: number) { + return { + httpOnly: true, + // A `__Host-` cookie is refused by the browser without `Secure`; the + // name and the flag come from the same switch. + secure: shouldEmitSecureCookie(), + // OAuth callbacks (e.g. Withings) arrive via top-level cross-site + // redirect. Lax keeps CSRF protection for unsafe methods while allowing + // this flow. + sameSite: "lax" as const, + maxAge: maxAgeSeconds, + path: "/", + }; +} + +/** + * Set the session cookie under the name this install issues, and expire a + * cookie under the old name the request still carried, so a browser holds + * one session cookie, not two. + */ +export function issueSessionCookie( + store: CookieStore, + value: string, + maxAgeSeconds: number, +): void { + const name = sessionCookieName(); + store.set(name, value, sessionCookieOptions(maxAgeSeconds)); + if (name !== LEGACY_SESSION_COOKIE && store.get(LEGACY_SESSION_COOKIE)) { + store.delete(LEGACY_SESSION_COOKIE); + } +} + +/** + * Expire the session cookie under both names. The `__Host-` one is + * overwritten with `Secure` and `Path=/`, without which the browser ignores + * the overwrite and the cookie stays; the old name is deleted as it always + * was. + */ +export function expireSessionCookies( + jar: CookieWriter & { delete(name: string): unknown }, +): void { + jar.set(HOST_SESSION_COOKIE, "", { + httpOnly: true, + secure: true, + sameSite: "lax", + maxAge: 0, + path: "/", + }); + jar.delete(LEGACY_SESSION_COOKIE); +} diff --git a/src/lib/auth/session.ts b/src/lib/auth/session.ts index 7224dee51..3350c5ea8 100644 --- a/src/lib/auth/session.ts +++ b/src/lib/auth/session.ts @@ -11,14 +11,15 @@ import { SessionStoreUnavailableError } from "@/lib/api-errors"; import { credentialRevocations } from "@/lib/auth/credential-revocations"; import { locales, type Locale } from "@/lib/i18n/config"; import { LOCALE_COOKIE, setLocaleCookie } from "@/lib/i18n/locale-cookie"; +import { + expireSessionCookies, + issueSessionCookie, + readSessionCookie, + sessionCookieName, +} from "@/lib/auth/session-cookie"; -const SESSION_COOKIE = "healthlog_session"; const SESSION_MAX_AGE_MS = 30 * 24 * 60 * 60 * 1000; // 30 days -/** Public name of the browser session cookie, for routes that read it off a - * `NextRequest` directly (rather than through `getSession()`'s `cookies()`). */ -export const SESSION_COOKIE_NAME = SESSION_COOKIE; - /** * Marks a cookie value as a v1.30.32 session secret. Doubles as the branch * between the two resolution paths below, so a cookie is only ever looked up @@ -244,7 +245,7 @@ export async function setOnboardingPendingCookie( secure: shouldEmitSecureCookie(), // v1.4.22 — Strict (not Lax) because // no cross-site redirect flow ever depends on this cookie. The - // sibling `healthlog_session` cookie stays Lax because the + // sibling session cookie stays Lax because the // Withings OAuth callback arrives via top-level cross-site // redirect; the onboarding hint has no equivalent flow. sameSite: "strict", @@ -291,15 +292,7 @@ export async function createSession( }); const cookieStore = await cookies(); - cookieStore.set(SESSION_COOKIE, secret, { - httpOnly: true, - secure: shouldEmitSecureCookie(), - // OAuth callbacks (e.g. Withings) arrive via top-level cross-site redirect. - // Lax keeps CSRF protection for unsafe methods while allowing this flow. - sameSite: "lax", - maxAge: SESSION_MAX_AGE_MS / 1000, - path: "/", - }); + issueSessionCookie(cookieStore, secret, SESSION_MAX_AGE_MS / 1000); // Anchor the onboarding cookie alongside the session cookie so a // future auth surface added without remembering the helper can never @@ -329,8 +322,9 @@ export async function getSession(): Promise<{ } const cookieStore = await cookies(); - const cookieValue = cookieStore.get(SESSION_COOKIE)?.value; - if (!cookieValue) return null; + const carried = readSessionCookie(cookieStore); + if (!carried) return null; + const cookieValue = carried.value; const session = await findSessionByCookie(cookieValue); @@ -340,7 +334,7 @@ export async function getSession(): Promise<{ .deleteMany({ where: { id: session.id } }) .catch(() => {}); } - cookieStore.delete(SESSION_COOKIE); + expireSessionCookies(cookieStore); cookieStore.delete(ONBOARDING_COOKIE); return null; } @@ -364,13 +358,16 @@ export async function getSession(): Promise<{ where: { id: session.id }, data: { expiresAt: newExpiry }, }); - cookieStore.set(SESSION_COOKIE, cookieValue, { - httpOnly: true, - secure: shouldEmitSecureCookie(), - sameSite: "lax", - maxAge: SESSION_MAX_AGE_MS / 1000, - path: "/", - }); + issueSessionCookie(cookieStore, cookieValue, SESSION_MAX_AGE_MS / 1000); + } else if (carried.name !== sessionCookieName()) { + // Issued before the `__Host-` name: re-issued under it, so the upgrade + // signs nobody out. A server-component render cannot write cookies; the + // next route request does. + try { + issueSessionCookie(cookieStore, cookieValue, SESSION_MAX_AGE_MS / 1000); + } catch { + // Read-only cookie context. + } } // v1.23 — sliding "last seen" stamp for the active-session list. Throttled @@ -501,7 +498,7 @@ export async function getSession(): Promise<{ */ export async function getSessionUserLocale(): Promise { const cookieStore = await cookies(); - const cookieValue = cookieStore.get(SESSION_COOKIE)?.value; + const cookieValue = readSessionCookie(cookieStore)?.value; if (!cookieValue) return null; // Same two-path resolution as `getSession`, read-only: this runs in the root @@ -514,7 +511,7 @@ export async function getSessionUserLocale(): Promise { export async function destroySession(): Promise { const cookieStore = await cookies(); - const cookieValue = cookieStore.get(SESSION_COOKIE)?.value; + const cookieValue = readSessionCookie(cookieStore)?.value; if (cookieValue) { // Resolve the cookie to its row before deleting — the cookie is a secret, // not the primary key, so `delete({ where: { id: cookieValue } })` would @@ -547,7 +544,7 @@ export async function destroySession(): Promise { }); } } - cookieStore.delete(SESSION_COOKIE); + expireSessionCookies(cookieStore); cookieStore.delete(ONBOARDING_COOKIE); } diff --git a/src/lib/openapi/routes/index.ts b/src/lib/openapi/routes/index.ts index 31da5dec6..cea0de0a0 100644 --- a/src/lib/openapi/routes/index.ts +++ b/src/lib/openapi/routes/index.ts @@ -288,7 +288,8 @@ export const openApiComponents: NonNullable = { type: "apiKey", in: "cookie", name: "healthlog_session", - description: "Browser session cookie — set by /api/auth/login.", + description: + "Browser session cookie — set by /api/auth/login. Named `__Host-healthlog_session` wherever the cookie is `Secure` (every HTTPS install); a plain-HTTP install, which cannot hold a `__Host-` cookie, keeps `healthlog_session`.", }, }, }; diff --git a/src/proxy.ts b/src/proxy.ts index d5227364c..8cc2b7f8f 100644 --- a/src/proxy.ts +++ b/src/proxy.ts @@ -1,5 +1,6 @@ import { NextResponse, type NextRequest } from "next/server"; import { shouldRunWeb } from "@/lib/process-type"; +import { readSessionCookie } from "@/lib/auth/session-cookie"; import { findRetiredRoute, retiredRouteEnvelope, @@ -560,8 +561,7 @@ export function proxy(request: NextRequest) { // can never be session-gated without also being uncacheable. const isSessionGatedPage = !isApiRoute && !isStaticFile && !isPublic; if (isSessionGatedPage) { - const hasSession = request.cookies.has("healthlog_session"); - if (!hasSession) { + if (!readSessionCookie(request.cookies)) { return applyBaselineSecurityHeaders( NextResponse.redirect(new URL("/auth/login", request.url)), ); diff --git a/tests/integration/routes/parity/oidc-callback.parity.test.ts b/tests/integration/routes/parity/oidc-callback.parity.test.ts index 3f4145bbf..661d78770 100644 --- a/tests/integration/routes/parity/oidc-callback.parity.test.ts +++ b/tests/integration/routes/parity/oidc-callback.parity.test.ts @@ -22,7 +22,7 @@ import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; import { GET } from "@/app/api/auth/oidc/callback/route"; import { encrypt } from "@/lib/crypto"; import { OIDC_STATE_COOKIE } from "@/lib/auth/oidc-cookie"; -import { SESSION_COOKIE_NAME } from "@/lib/auth/session"; +import { sessionCookieName } from "@/lib/auth/session-cookie"; import { anonymous, callRoute } from "../../../kit/call-route"; import { makeSession, makeUser } from "../../../kit/factories"; @@ -312,7 +312,7 @@ describe("GET /api/auth/oidc/callback parity", () => { const session = await makeSession(first); await step("12-duplicate-with-session", { state: null, - cookies: { [SESSION_COOKIE_NAME]: session.cookie }, + cookies: { [sessionCookieName()]: session.cookie }, }); }); @@ -382,7 +382,7 @@ describe("GET /api/auth/oidc/callback parity", () => { identity("sub-claimer", "claimer@example.test", true); await step("23-claim-signed-in", { state: { intent: "claim", claimBinding: "binding-1" }, - cookies: { [SESSION_COOKIE_NAME]: session.cookie }, + cookies: { [sessionCookieName()]: session.cookie }, }); await step("24-claim-no-cookie", { state: { intent: "claim", claimBinding: "binding-1" }, diff --git a/tests/kit/call-route.ts b/tests/kit/call-route.ts index b81e2bd74..26c180ec1 100644 --- a/tests/kit/call-route.ts +++ b/tests/kit/call-route.ts @@ -25,7 +25,7 @@ import { NextRequest } from "next/server"; import type { z } from "zod"; import { openApiComponents, openApiPaths } from "@/lib/openapi/routes"; -import { SESSION_COOKIE_NAME } from "@/lib/auth/session"; +import { sessionCookieName } from "@/lib/auth/session-cookie"; import { cookieJar, headerJar } from "../integration/mock-next-headers"; import type { BearerFixture, SessionFixture } from "./factories"; @@ -122,7 +122,7 @@ export async function callRoute( headers.set("authorization", `Bearer ${options.auth.token}`); } if (options.auth.kind === "session") { - headers.set("cookie", `${SESSION_COOKIE_NAME}=${options.auth.cookie}`); + headers.set("cookie", `${sessionCookieName()}=${options.auth.cookie}`); } // `next/headers` is served from the shared jars; fill them for this call @@ -133,7 +133,7 @@ export async function callRoute( const savedHeaders = new Map(headerJar); cookieJar.clear(); if (options.auth.kind === "session") { - cookieJar.set(SESSION_COOKIE_NAME, options.auth.cookie); + cookieJar.set(sessionCookieName(), options.auth.cookie); } headerJar.delete("authorization"); headers.forEach((value, name) => headerJar.set(name.toLowerCase(), value)); diff --git a/tests/kit/factories.ts b/tests/kit/factories.ts index 1510314f6..ff7bd564f 100644 --- a/tests/kit/factories.ts +++ b/tests/kit/factories.ts @@ -27,7 +27,8 @@ import type { Workout, } from "@/generated/prisma/client"; import { prisma } from "@/lib/db"; -import { createSession, SESSION_COOKIE_NAME } from "@/lib/auth/session"; +import { createSession } from "@/lib/auth/session"; +import { sessionCookieName } from "@/lib/auth/session-cookie"; import { issueApiToken } from "@/lib/auth/issue-token"; import { cookieJar } from "../integration/mock-next-headers"; @@ -91,7 +92,7 @@ export async function makeSession( "healthlog-kit", options.mfaVerifiedAt ?? null, ); - const cookie = cookieJar.get(SESSION_COOKIE_NAME); + const cookie = cookieJar.get(sessionCookieName()); cookieJar.clear(); for (const [name, value] of before) cookieJar.set(name, value); if (!cookie) { From 0b830e6b0294764fe004ea5dffc27339a75aa818 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 25 Oct 2026 17:12:00 +0100 Subject: [PATCH 09/13] fix(http): exempt only a Bearer request without a session cookie from the cross-site check The cross-site write check let through any request that carried an Authorization header, while the caller is resolved from the session cookie before any header. A browser attaches Basic or Negotiate credentials on its own once a proxy or host asked for them, so a cookie-authenticated write from a sibling subdomain could pass. Only Authorization: Bearer is exempt now, and only when no session cookie rides along; a request carrying both is checked as the cookie request it resolves to. The share-link passphrase POST under /api/c/ is exempt by path: its credential is the token in the path and the passphrase in the body, and from the shared view's no-referrer page a browser without Sec-Fetch-Site sends Origin: null, which was refused. --- .../__tests__/api-handler-cross-site.test.ts | 59 +++++++++++++++++++ src/lib/http/cross-site-guard.ts | 31 +++++++--- 2 files changed, 83 insertions(+), 7 deletions(-) diff --git a/src/lib/__tests__/api-handler-cross-site.test.ts b/src/lib/__tests__/api-handler-cross-site.test.ts index 1ee042d8e..c8b8737f4 100644 --- a/src/lib/__tests__/api-handler-cross-site.test.ts +++ b/src/lib/__tests__/api-handler-cross-site.test.ts @@ -114,6 +114,65 @@ describe("apiHandler and a state change from another site", () => { ); }); + it("refuses a browser-attached Basic or Negotiate header riding a session cookie", async () => { + // The browser adds these on its own once a proxy or the host asked for + // them, and the caller is resolved from the cookie first: the header is + // not what the request is authenticated by. + for (const authorization of ["Basic dXNlcjpwYXNz", "Negotiate YIIGhg"]) { + const response = await post({ + "content-type": "application/json", + authorization, + "sec-fetch-site": "same-site", + origin: "https://evil.example.com", + cookie: "healthlog_session=s", + }); + expect(response.status).toBe(403); + } + expect(handler).not.toHaveBeenCalled(); + }); + + it("checks a Bearer request that also carries a session cookie as the cookie request it resolves to", async () => { + for (const cookie of [ + "healthlog_session=s", + "theme=dark; __Host-healthlog_session=s", + ]) { + const response = await post({ + "content-type": "application/json", + authorization: "Bearer hlk_x", + "sec-fetch-site": "cross-site", + cookie, + }); + expect(response.status).toBe(403); + } + expect(handler).not.toHaveBeenCalled(); + // From the app's own page the same pair passes. + const own = await post({ + "content-type": "application/json", + authorization: "Bearer hlk_x", + "sec-fetch-site": "same-origin", + cookie: "healthlog_session=s", + }); + expect(own.status).toBe(200); + }); + + it("lets the share-link passphrase POST through from a no-referrer page", async () => { + // The shared view is served with `Referrer-Policy: no-referrer`; a + // browser without Sec-Fetch-Site then sends `Origin: null`. The token in + // the path and the passphrase in the body are the credential. + const response = await route( + new NextRequest(`${ORIGIN}/api/c/hls_token/unlock`, { + method: "POST", + headers: { + host: "health.example.com", + "content-type": "application/json", + origin: "null", + }, + body: '{"value":1}', + }), + ); + expect(response.status).toBe(200); + }); + it("leaves reads alone", async () => { const response = await route( new NextRequest(`${ORIGIN}/api/measurements`, { diff --git a/src/lib/http/cross-site-guard.ts b/src/lib/http/cross-site-guard.ts index 1dbf72bd1..ee1e4a7bf 100644 --- a/src/lib/http/cross-site-guard.ts +++ b/src/lib/http/cross-site-guard.ts @@ -25,17 +25,28 @@ * - A request with neither header is not from a browser page (the phone * app, a script, a webhook sender) and passes; it carries no cookie a * page could have lent it. - * - A request carrying `Authorization` passes: a page cannot attach that - * header to another origin without a CORS preflight, which this server - * never answers. + * - A request carrying `Authorization: Bearer` and no session cookie + * passes: a page cannot attach that header to another origin without a + * CORS preflight, which this server never answers, and without a session + * cookie the token is what the request is authenticated by. Any other + * scheme does not count: a browser attaches `Basic` or `Negotiate` + * credentials on its own when a proxy or the host asked for them, and + * the caller is resolved from the session cookie before any header + * (`src/lib/auth/guards/caller.ts`), so a request with both is a cookie + * request and is checked as one. * * Exempt by path: the surfaces that authenticate with a secret in a header * or the URL rather than the session cookie, and that a third party's client - * may legitimately call from a browser (a push client's action button). The - * MCP OAuth endpoints, which must accept cross-origin requests by their - * specification, do not run through `apiHandler` at all. + * may legitimately call from a browser (a push client's action button), and + * the share-link passphrase check under `/api/c/`, whose credential is the + * token in the path plus the passphrase in the body. A shared-record page is + * served with `Referrer-Policy: no-referrer`, and a browser without + * `Sec-Fetch-Site` then sends `Origin: null` on its POST, which no host + * check can admit. The MCP OAuth endpoints, which must accept cross-origin + * requests by their specification, do not run through `apiHandler` at all. */ import { apiError } from "@/lib/api-response"; +import { cookieHeaderCarriesSession } from "@/lib/auth/session-cookie"; const SAFE_METHODS = new Set(["GET", "HEAD", "OPTIONS"]); @@ -51,6 +62,7 @@ export const CROSS_SITE_EXEMPT_PREFIXES = [ "/api/internal/deploy-webhook", "/api/notifications/action/", "/api/monitoring/csp-report", + "/api/c/", ] as const; function hostOf(value: string | null | undefined): string | null { @@ -92,7 +104,12 @@ export function crossSiteWriteReason( if (CROSS_SITE_EXEMPT_PREFIXES.some((p) => pathname.startsWith(p))) { return null; } - if (request.headers.has("authorization")) return null; + if ( + request.headers.get("authorization")?.startsWith("Bearer ") && + !cookieHeaderCarriesSession(request.headers.get("cookie")) + ) { + return null; + } const site = request.headers.get("sec-fetch-site")?.trim().toLowerCase(); if (site) { From d87718407e348f9e160e3475c74b512c8a67944d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 25 Oct 2026 17:15:36 +0100 Subject: [PATCH 10/13] fix(http): let operators name their proxy, and survive a bad TRUST_PROXY_HOPS Forwarding headers were believed from any connection on a private address. Behind Docker Desktop, OrbStack and docker-proxy every connection to a published port arrives from the gateway, a private address, so a caller reaching the port directly could again choose the client address that rate limits, the audit log and sign-in notices record. TRUSTED_PROXY_CIDRS (comma-separated CIDRs) now names the proxies: when set, only those peers may report a client address. The private-range default stays when it is unset, the boot log states the rule in force in one line, and the reverse-proxy guide says what to set for a proxy on the same host, on a Docker network, or on a public address (which, unlisted, puts all clients in one bucket). An invalid TRUST_PROXY_HOPS threw on every request that resolved the client address, so every route answered 500, the health probe included. It now resolves no address, warns once, and is named at boot. --- .env.example | 6 + .env.production.example | 17 ++ docker-compose.yml | 7 + docs/self-hosting/reverse-proxy.md | 63 ++++++- scripts/env-manifest.json | 5 + src/instrumentation.ts | 13 ++ .../api-handler-invalid-proxy-hops.test.ts | 52 ++++++ src/lib/__tests__/get-client-ip.test.ts | 118 +++++++++++- src/lib/api-response.ts | 83 ++++++--- src/lib/http/proxy-trust.ts | 175 ++++++++++++++++++ 10 files changed, 501 insertions(+), 38 deletions(-) create mode 100644 src/lib/__tests__/api-handler-invalid-proxy-hops.test.ts create mode 100644 src/lib/http/proxy-trust.ts diff --git a/.env.example b/.env.example index 8ba3be7d0..462459b44 100644 --- a/.env.example +++ b/.env.example @@ -234,6 +234,12 @@ API_TOKEN_HMAC_KEY="REPLACE-WITH-OUTPUT-OF-openssl-rand-hex-32" # refuse the headers, log one warning, and share one rate-limit bucket across # all anonymous callers. # TRUST_PROXY_HOPS="1" +# The addresses the reverse proxy connects from (comma-separated CIDRs). Set, +# only those connections may report the client's address; unset, any private +# address may, which behind Docker Desktop, OrbStack or docker-proxy includes +# every caller of the published port. Same host: 127.0.0.1/32,::1/128; Docker +# network: the proxy's subnet. See docs/self-hosting/reverse-proxy.md. +# TRUSTED_PROXY_CIDRS="" # TRUST_CF_CONNECTING_IP="" diff --git a/.env.production.example b/.env.production.example index 81356bfb9..4c88afe04 100644 --- a/.env.production.example +++ b/.env.production.example @@ -565,6 +565,23 @@ API_TOKEN_HMAC_KEY="" # anonymous callers into one rate-limit bucket. # TRUST_PROXY_HOPS="1" # +# The addresses your reverse proxy connects from, as comma-separated CIDRs +# (IPv4 or IPv6; a bare address is one host). When set, only those +# connections may report the client's address; every other connection is +# its own client. When unset, any private address may, which is the old +# behaviour and is NOT safe behind Docker Desktop, OrbStack or docker-proxy: +# there every caller of the published port arrives from the gateway, a +# private address, and could name its own client address. Set it whenever a +# reverse proxy is in front: +# proxy on the same host (proxying to 127.0.0.1:3000): 127.0.0.1/32,::1/128 +# proxy container on a Docker network: that network's subnet (docker +# network inspect ), e.g. 172.18.0.0/16 +# proxy on another machine with a public address: that address, /32 +# A proxy on a public address that is not listed is not believed: every +# request through it shares the proxy's address and one rate-limit bucket. +# The boot log names the rule in force in one [proxy-trust] line. +# TRUSTED_PROXY_CIDRS="127.0.0.1/32,::1/128" +# # Behind Cloudflare the real visitor IP arrives in the `cf-connecting-ip` # header; the X-Forwarded-For chain otherwise yields the Cloudflare edge IP, # so the audit-log overview stores the CF datacentre address and the geo diff --git a/docker-compose.yml b/docker-compose.yml index c73d135f1..cbf7e5aab 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -108,7 +108,14 @@ x-healthlog-env: &healthlog_env # collapses every anonymous caller into one rate-limit bucket. Listed here # so an operator value in .env reaches the container (the `environment:` # block is a whitelist). + # TRUSTED_PROXY_CIDRS names the addresses the reverse proxy connects from + # (comma-separated CIDRs). Set, only those connections may report a client + # address; unset, any private address may, which behind Docker Desktop, + # OrbStack or docker-proxy includes every caller of the published port. + # Same host: 127.0.0.1/32,::1/128. Docker network: the proxy's subnet. + # See docs/self-hosting/reverse-proxy.md. TRUST_PROXY_HOPS: "${TRUST_PROXY_HOPS:-}" + TRUSTED_PROXY_CIDRS: "${TRUSTED_PROXY_CIDRS:-}" TRUST_CF_CONNECTING_IP: "${TRUST_CF_CONNECTING_IP:-}" IP_GEO_LOOKUP_URL: "${IP_GEO_LOOKUP_URL:-}" IP_GEO_LOOKUP_DISABLED: "${IP_GEO_LOOKUP_DISABLED:-}" diff --git a/docs/self-hosting/reverse-proxy.md b/docs/self-hosting/reverse-proxy.md index 5dee21854..3cceebaac 100644 --- a/docs/self-hosting/reverse-proxy.md +++ b/docs/self-hosting/reverse-proxy.md @@ -21,13 +21,14 @@ tailnet with no extra container. See | `NEXT_PUBLIC_APP_URL` | Every public-facing absolute URL the UI emits | Must match the hostname the proxy serves | | `APP_URL` | Server-side absolute URLs from background jobs | Mirror of the above; both must match | | `TRUST_PROXY_HOPS` | `X-Forwarded-For` chain trust | Defaults to `1` — one trusted hop | +| `TRUSTED_PROXY_CIDRS` | Which connections may report a client address | Unset: any private address; see below | | `TRUST_CF_CONNECTING_IP` | Reading the visitor address Cloudflare sends | Off unless `1`; see the warning below | Set the URL pair to the public origin (`https://your-instance.example.com`) and restart the `app` container. The image reads both at startup; no rebuild needed. -All four variables are on the shipped `docker-compose.yml` +All five variables are on the shipped `docker-compose.yml` `environment:` whitelist, so a value in `.env` reaches the container without editing the compose file. (Before v1.37.33 the whitelist was missing `TRUST_PROXY_HOPS`; on those versions add @@ -68,13 +69,56 @@ The reserved and multicast blocks are in the list because no host on the internet can open a connection from them, so a peer there sits on a network you run. A request that reaches HealthLog straight from a public address carries headers its sender wrote, so HealthLog ignores them and -uses the connection's own address, and says so once in the log. A proxy on -the same host or Docker network connects from a private address, so the -usual setups above need nothing. If your proxy reaches HealthLog over a -public address, put it on a private network with HealthLog instead. The log +uses the connection's own address, and says so once in the log. The log line also shows up when the published port is open to the internet beside the proxy; bind it to the proxy's network or to `127.0.0.1` then. +That list is the default when `TRUSTED_PROXY_CIDRS` is unset, and it is +only as good as the network in front of the port. Read the next section +before relying on it. + +### `TRUSTED_PROXY_CIDRS` — name the proxy + +Set `TRUSTED_PROXY_CIDRS` to the addresses your reverse proxy connects +from, as comma-separated CIDRs (IPv4 or IPv6; a bare address counts as a +single host). Once it is set, only a connection from inside those ranges +may report the client's address. Every other connection is its own client, +whatever its address, private ones included, and the log says so once. + +Why it matters: some setups make every caller look like a private address. +Docker Desktop, OrbStack, and Docker's `docker-proxy` (the userland proxy +behind a published port such as `3000:3000`) forward each connection from +the gateway address of the Docker network, which is private. On those +hosts, someone who can reach the published port directly, from the LAN or +from the internet if the port is open, passes the private-address check +and can again choose the address that every per-IP rate limit, the audit +log and the sign-in notices see, a new one per request if they like. +Naming your proxy closes that. The default port binding in +`docker-compose.yml` stays as it is (LAN installs rely on it); this +variable is the fix. + +What to set: + +| Where the proxy runs | `TRUSTED_PROXY_CIDRS` | +| ------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------ | +| On the same host, proxying to `127.0.0.1:3000` (bare Nginx, Caddy) | `127.0.0.1/32,::1/128` | +| In a container on a Docker network with HealthLog (Traefik, NPM, Coolify) | that network's subnet, e.g. `172.18.0.0/16` (`docker network inspect ` shows it under `IPAM`) | +| On another machine that connects from a public address | that machine's address, e.g. `198.51.100.7/32` | +| No proxy, or `tailscale serve` only | leave it unset | + +At boot the app logs one `[proxy-trust]` line naming the rule in force: +which connections it reads forwarding headers from and how many hops deep. +A value that is not a CIDR is logged as an error, and until it is fixed no +connection may report a client address. + +**A proxy that connects from a public address.** Without +`TRUSTED_PROXY_CIDRS`, such a proxy is not believed: every request through +it resolves to the proxy's own address, so all your users share one +rate-limit bucket and one person mistyping a password can hold sign-in back +for everyone. Releases before v1.46 believed any peer, so this shows up after +the upgrade. Set `TRUSTED_PROXY_CIDRS` to the proxy's address (or put the proxy +on a private network with HealthLog) and restart. + ### `TRUST_CF_CONNECTING_IP` — only when the origin takes Cloudflare only With `TRUST_CF_CONNECTING_IP=1` HealthLog takes the visitor's address from @@ -283,6 +327,15 @@ The one-shot stderr warning `[getClientIp] TRUST_PROXY_HOPS=N but X-Forwarded-For carried M entries` confirms a mismatch. Adjust the env var down to the actual chain length and restart. +If every request logs the same client address, the proxy's own, the proxy +is not one HealthLog believes: the `[getClientIp]` warning about a +connection "from a public address" or "from outside TRUSTED_PROXY_CIDRS" +names the case. Add the proxy's address to `TRUSTED_PROXY_CIDRS`. + +A `TRUST_PROXY_HOPS` value that is not a whole number resolves no client +address at all (requests keep working) and is named in the `[proxy-trust]` +boot line. + ## Running over plain HTTP on a LAN (no reverse proxy) The session cookie carries the `Secure` flag by default under diff --git a/scripts/env-manifest.json b/scripts/env-manifest.json index 75339c7fd..51477220b 100644 --- a/scripts/env-manifest.json +++ b/scripts/env-manifest.json @@ -432,6 +432,11 @@ "purpose": "Trusted reverse-proxy hop count; the client IP is read that many entries from the right of X-Forwarded-For. Forwarding headers are read only from a peer whose own address is not public (loopback, private, CGNAT, unique-local, link-local, reserved, multicast).", "example": "1" }, + { + "name": "TRUSTED_PROXY_CIDRS", + "purpose": "Comma-separated CIDRs (IPv4 or IPv6; a bare address is one host) the reverse proxy connects from. Set, only a connection from inside them may report the client address in X-Forwarded-For or X-Real-IP; every other connection is its own client. Unset, any non-public address may, which behind Docker Desktop, OrbStack or docker-proxy includes every caller of a published port. Same host: 127.0.0.1/32,::1/128; Docker network: the proxy's subnet. An entry that is not a CIDR lets no connection report an address and is named at boot.", + "example": "127.0.0.1/32,::1/128" + }, { "name": "TRUST_CF_CONNECTING_IP", "purpose": "\"1\" takes the client IP from Cloudflare's CF-Connecting-IP header, from every connection and before the peer check. Safe only when the origin accepts connections from Cloudflare alone (a Tunnel, or a firewall admitting only Cloudflare's ranges); otherwise any caller forges the address rate limits and the audit log record. Off by default.", diff --git a/src/instrumentation.ts b/src/instrumentation.ts index e7cd50243..96bf1f6c9 100644 --- a/src/instrumentation.ts +++ b/src/instrumentation.ts @@ -15,6 +15,18 @@ async function registerOfflineGeoNoticeAtBoot(): Promise { } } +/** + * Which connections may report a client address (TRUSTED_PROXY_CIDRS, + * TRUST_PROXY_HOPS), said once, with a bad value named. + */ +async function logProxyTrustAtBootSafely(): Promise { + try { + (await import("@/lib/http/proxy-trust")).logProxyTrustAtBoot(); + } catch { + // A log line must never break boot. + } +} + export async function register() { // Only start the worker on the Node.js server runtime (not Edge, not build) if (process.env.NEXT_RUNTIME === "nodejs") { @@ -54,6 +66,7 @@ export async function register() { } await registerOfflineGeoNoticeAtBoot(); + await logProxyTrustAtBootSafely(); // Does the configured encryption key open this database? A process whose // key does not refuses every API request with one clear 503 and starts diff --git a/src/lib/__tests__/api-handler-invalid-proxy-hops.test.ts b/src/lib/__tests__/api-handler-invalid-proxy-hops.test.ts new file mode 100644 index 000000000..b051dc4cf --- /dev/null +++ b/src/lib/__tests__/api-handler-invalid-proxy-hops.test.ts @@ -0,0 +1,52 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +vi.mock("@/lib/db", () => ({ prisma: {} })); +vi.mock("@/lib/auth/session", () => ({ getSession: vi.fn() })); +vi.mock("@/lib/auth/hmac", () => ({ hashToken: vi.fn() })); +vi.mock("@/lib/auth/audit", () => ({ + auditLog: vi.fn().mockResolvedValue(undefined), +})); +vi.mock("@/lib/logging/transports", () => ({ emitIfSampled: vi.fn() })); +vi.mock("next/headers", () => ({ + headers: vi.fn(async () => ({ get: () => null })), + cookies: vi.fn(async () => ({ + get: () => undefined, + set: () => {}, + delete: () => {}, + })), +})); + +import { NextRequest } from "next/server"; +import { apiHandler } from "@/lib/api-handler"; +import { getClientIp } from "@/lib/api-response"; + +/** + * A typo in `TRUST_PROXY_HOPS` used to make every request that asked for the + * client address throw, and `apiHandler` asks for it on every request to + * record it: every route answered 500, the health probe included, so the + * container looked dead for a configuration slip. It now resolves no address + * and the boot log names the value. + */ +afterEach(() => { + vi.unstubAllEnvs(); + vi.restoreAllMocks(); +}); + +describe("apiHandler with an invalid TRUST_PROXY_HOPS", () => { + it("answers the route instead of 500, with no client address", async () => { + vi.stubEnv("TRUST_PROXY_HOPS", "two"); + vi.spyOn(console, "warn").mockImplementation(() => {}); + const seen: Array = []; + const route = apiHandler(async (request: NextRequest) => { + seen.push(getClientIp(request)); + return Response.json({ data: { status: "ok" }, error: null }); + }); + const response = await route( + new NextRequest("https://health.example.com/api/health", { + headers: { "x-forwarded-for": "203.0.113.4" }, + }), + ); + expect(response.status).toBe(200); + expect(seen).toEqual([null]); + }); +}); diff --git a/src/lib/__tests__/get-client-ip.test.ts b/src/lib/__tests__/get-client-ip.test.ts index 6ef8182b0..9d57d4c25 100644 --- a/src/lib/__tests__/get-client-ip.test.ts +++ b/src/lib/__tests__/get-client-ip.test.ts @@ -10,16 +10,22 @@ import { import { getClientIp, getClientIpOrTrustWarning, + _resetInvalidHopsWarningForTests, + _resetPublicPeerForwardingWarningForTests, _resetTrustViolationWarningForTests, _resetUnreadProxyHeadersWarningForTests, } from "../api-response"; +import { describeProxyTrust } from "../http/proxy-trust"; const ORIGINAL_HOPS = process.env.TRUST_PROXY_HOPS; const ORIGINAL_CF_FLAG = process.env.TRUST_CF_CONNECTING_IP; +const ORIGINAL_CIDRS = process.env.TRUSTED_PROXY_CIDRS; beforeEach(() => { delete process.env.TRUST_PROXY_HOPS; delete process.env.TRUST_CF_CONNECTING_IP; + delete process.env.TRUSTED_PROXY_CIDRS; + _resetPublicPeerForwardingWarningForTests(); _resetTrustViolationWarningForTests(); _resetUnreadProxyHeadersWarningForTests(); }); @@ -29,6 +35,8 @@ afterEach(() => { else process.env.TRUST_PROXY_HOPS = ORIGINAL_HOPS; if (ORIGINAL_CF_FLAG === undefined) delete process.env.TRUST_CF_CONNECTING_IP; else process.env.TRUST_CF_CONNECTING_IP = ORIGINAL_CF_FLAG; + if (ORIGINAL_CIDRS === undefined) delete process.env.TRUSTED_PROXY_CIDRS; + else process.env.TRUSTED_PROXY_CIDRS = ORIGINAL_CIDRS; }); function makeRequest(headers: Record): Request { @@ -64,11 +72,23 @@ describe("getClientIp trusted-proxy semantics (V3 audit)", () => { expect(ip).toBeNull(); }); - it("throws at boot when TRUST_PROXY_HOPS is unparseable (fixes review L-1)", () => { + it("resolves no address for an unparseable TRUST_PROXY_HOPS instead of failing the request", () => { + // It used to throw on every request that asked for the client address, + // so every route answered 500, the health probe included. Reading the + // headers anyway would silently trust a chain the operator never + // described (review L-1), so: no address, a trust violation, one warning. process.env.TRUST_PROXY_HOPS = "garbage"; - expect(() => - getClientIp(makeRequest({ "x-forwarded-for": "5.6.7.8" })), - ).toThrow(/TRUST_PROXY_HOPS/); + _resetInvalidHopsWarningForTests(); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + onTestFinished(() => warn.mockRestore()); + const request = makeRequest({ "x-forwarded-for": "5.6.7.8" }); + expect(getClientIpOrTrustWarning(request)).toEqual({ + ip: null, + trustViolation: true, + }); + expect(getClientIp(request)).toBeNull(); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0]![0]).toMatch(/TRUST_PROXY_HOPS="garbage"/); }); it.each(["", " "])( @@ -480,3 +500,93 @@ describe("getClientIp against the connection's own peer", () => { ).toBeNull(); }); }); + +/** + * Behind Docker Desktop, OrbStack or `docker-proxy`, every connection to a + * published port arrives from the gateway, a private address, so the + * private-range default believed a direct caller's forwarding headers again. + * `TRUSTED_PROXY_CIDRS` names the proxies; no other peer may forward. + */ +describe("getClientIp with TRUSTED_PROXY_CIDRS", () => { + const stamped = (peer: string, headers: Record = {}) => + makeRequest({ ...headers, "x-healthlog-peer-address": peer }); + + it("reads forwarding headers only from a peer inside the list", () => { + process.env.TRUSTED_PROXY_CIDRS = "172.18.0.0/16, 127.0.0.1"; + expect( + getClientIp(stamped("172.18.0.5", { "x-forwarded-for": "203.0.113.5" })), + ).toBe("203.0.113.5"); + expect( + getClientIp(stamped("127.0.0.1", { "x-real-ip": "203.0.113.6" })), + ).toBe("203.0.113.6"); + }); + + it("uses the gateway's own address for a direct caller from a private network outside the list", () => { + process.env.TRUSTED_PROXY_CIDRS = "172.18.0.0/16"; + vi.spyOn(console, "warn").mockImplementation(() => {}); + const forged = ["203.0.113.1", "203.0.113.2"].map((ip) => + getClientIp( + stamped("192.168.65.1", { "x-forwarded-for": ip, "x-real-ip": ip }), + ), + ); + expect(forged).toEqual(["192.168.65.1", "192.168.65.1"]); + vi.restoreAllMocks(); + }); + + it("believes a proxy on a public address once it is listed", () => { + process.env.TRUSTED_PROXY_CIDRS = "198.51.100.7/32"; + expect( + getClientIp( + stamped("198.51.100.7", { "x-forwarded-for": "203.0.113.9" }), + ), + ).toBe("203.0.113.9"); + }); + + it("takes IPv6 ranges and bare addresses", () => { + process.env.TRUSTED_PROXY_CIDRS = "fd00::/8,2001:db8::1"; + expect( + getClientIp(stamped("fd12::3", { "x-forwarded-for": "203.0.113.1" })), + ).toBe("203.0.113.1"); + expect( + getClientIp(stamped("2001:db8::1", { "x-forwarded-for": "203.0.113.2" })), + ).toBe("203.0.113.2"); + expect( + getClientIp(stamped("2001:db8::2", { "x-forwarded-for": "203.0.113.3" })), + ).toBe("2001:db8::2"); + }); + + it("lets no peer forward when an entry is not a CIDR", () => { + process.env.TRUSTED_PROXY_CIDRS = "172.18.0.0/16, not-a-range"; + vi.spyOn(console, "warn").mockImplementation(() => {}); + expect( + getClientIp(stamped("172.18.0.5", { "x-forwarded-for": "203.0.113.5" })), + ).toBe("172.18.0.5"); + vi.restoreAllMocks(); + }); +}); + +describe("the boot line naming the proxy rule", () => { + it("names the private-range default, and what it lets in behind a port forwarder", () => { + expect(describeProxyTrust()).toMatchObject({ + level: "info", + message: expect.stringMatching(/TRUSTED_PROXY_CIDRS unset.*docker-proxy/), + }); + }); + + it("names the list when one is set", () => { + process.env.TRUSTED_PROXY_CIDRS = "127.0.0.1/32"; + expect(describeProxyTrust()).toMatchObject({ + level: "info", + message: expect.stringContaining("only connections from 127.0.0.1/32"), + }); + }); + + it("names a bad TRUST_PROXY_HOPS and a bad CIDR as errors", () => { + process.env.TRUST_PROXY_HOPS = "one"; + process.env.TRUSTED_PROXY_CIDRS = "10.0.0.0/33"; + const line = describeProxyTrust(); + expect(line.level).toBe("error"); + expect(line.message).toContain('TRUST_PROXY_HOPS="one"'); + expect(line.message).toContain('"10.0.0.0/33"'); + }); +}); diff --git a/src/lib/api-response.ts b/src/lib/api-response.ts index f7f8f058c..afc629b15 100644 --- a/src/lib/api-response.ts +++ b/src/lib/api-response.ts @@ -7,7 +7,11 @@ import { peerAddressStampInstalled, readPeerAddress, } from "@/lib/http/peer-address"; -import { isPublicIp } from "@/lib/validations/notifications"; +import { + parseTrustProxyHops, + parseTrustedProxyCidrs, + peerMayForward, +} from "@/lib/http/proxy-trust"; export function apiSuccess(data: T, status = 200) { return NextResponse.json({ data, error: null }, { status }); @@ -508,13 +512,16 @@ export async function safeJson( * observed when the request arrived. * - "N" (>1) → trust N hops; read the Nth-from-rightmost XFF entry. * - * Forwarding headers are read only from a connection whose peer could be a - * proxy: loopback, a private or unique-local network, CGNAT (Tailscale). A - * connection from a public address reached the server directly, so whatever - * forwarding headers it sends are its own, and its own address is the client - * IP. The peer comes from `@/lib/http/peer-address`. Without that check, the - * default of one trusted hop let any direct caller name a fresh address per - * request and step around every per-IP limit. + * Forwarding headers are read only from a connection whose peer may be a + * proxy (`peerMayForward` in `@/lib/http/proxy-trust`): one inside + * `TRUSTED_PROXY_CIDRS` when that is set, otherwise any loopback, private, + * unique-local or CGNAT (Tailscale) address. Any other connection reached the + * server directly, so whatever forwarding headers it sends are its own, and + * its own address is the client IP. The peer comes from + * `@/lib/http/peer-address`. Without that check, the default of one trusted + * hop let any direct caller name a fresh address per request and step around + * every per-IP limit. An invalid `TRUST_PROXY_HOPS` resolves no address + * (and is named at boot) instead of failing the request. * * Cloudflare opt-in (`TRUST_CF_CONNECTING_IP=1`): when the env flag is * set, the helper prefers the `cf-connecting-ip` header before walking @@ -561,22 +568,24 @@ function readCfConnectingIp(request: Request): string | null { return looksLikeIp(trimmed) ? trimmed : null; } -function parseTrustProxyHops(raw: string | undefined): number { - // Empty is unset. The shipped docker-compose.yml passes - // `TRUST_PROXY_HOPS: "${TRUST_PROXY_HOPS:-}"`, which is an empty string for - // every operator who never set it; reading that as invalid threw on every - // request that asked for the client IP. - const trimmed = raw?.trim() ?? ""; - if (trimmed === "") return 1; - if (!/^\d+$/.test(trimmed)) { - // Reject explicitly-invalid values so an operator typo doesn't silently - // switch a real-proxy deployment to "no XFF trust" mode (review - // finding L-1: TRUST_PROXY_HOPS=garbage degraded to hops=0 silently). - throw new Error( - `TRUST_PROXY_HOPS must be a non-negative integer, got: ${JSON.stringify(raw)}`, - ); - } - return parseInt(trimmed, 10); +/** + * Once per process: `TRUST_PROXY_HOPS` is not a number. Every request that + * asks for the client address used to throw on it, so every route answered + * 500, `/api/health` included. It resolves no address instead, and says so. + */ +let invalidHopsWarned = false; + +function warnInvalidHopsOnce(raw: string | undefined): void { + if (invalidHopsWarned) return; + invalidHopsWarned = true; + console.warn( + `[getClientIp] TRUST_PROXY_HOPS=${JSON.stringify(raw)} is not a non-negative integer; no client address is read from forwarding headers until it is fixed, and every anonymous caller shares one rate-limit bucket.`, + ); +} + +/** Test-only reset for {@link warnInvalidHopsOnce}. */ +export function _resetInvalidHopsWarningForTests(): void { + invalidHopsWarned = false; } /** @@ -646,12 +655,23 @@ let publicPeerForwardingWarned = false; function warnPublicPeerForwardingOnce(): void { if (publicPeerForwardingWarned) return; publicPeerForwardingWarned = true; + const listed = + parseTrustedProxyCidrs(process.env.TRUSTED_PROXY_CIDRS).kind !== "unset"; console.warn( - "[getClientIp] a connection from a public address sent X-Forwarded-For or X-Real-IP; those headers are ignored and the connection's own address is used. " + - "If HealthLog is meant to be reached only through a reverse proxy, the published port is open beyond it.", + listed + ? "[getClientIp] a connection from outside TRUSTED_PROXY_CIDRS sent X-Forwarded-For or X-Real-IP; those headers are ignored and the connection's own address is used. " + + "If it was your proxy, add its address to TRUSTED_PROXY_CIDRS; otherwise the published port is reachable beyond the proxy." + : "[getClientIp] a connection from a public address sent X-Forwarded-For or X-Real-IP; those headers are ignored and the connection's own address is used. " + + "If HealthLog is meant to be reached only through a reverse proxy, the published port is open beyond it. " + + "If the proxy connects from a public address, set TRUSTED_PROXY_CIDRS to it.", ); } +/** Test-only reset for {@link warnPublicPeerForwardingOnce}. */ +export function _resetPublicPeerForwardingWarningForTests(): void { + publicPeerForwardingWarned = false; +} + /** * Tagged return shape so a caller can apply a tighter universal * rate-limit when the trust chain is misconfigured. F-6 (mobile security @@ -681,6 +701,10 @@ export function getClientIpOrTrustWarning(request: Request): { if (cfIp) return { ip: cfIp, trustViolation: false }; const hops = parseTrustProxyHops(process.env.TRUST_PROXY_HOPS); + if (hops === null) { + warnInvalidHopsOnce(process.env.TRUST_PROXY_HOPS); + return { ip: null, trustViolation: true }; + } // The connection's own peer, stamped by the server for every request it // receives. `undefined` only for a request the server did not receive (a @@ -693,9 +717,10 @@ export function getClientIpOrTrustWarning(request: Request): { if (peer !== undefined) { // No proxy declared: the peer is the client. if (hops === 0) return { ip: peer, trustViolation: false }; - // A peer on a public address reached the server directly; its - // forwarding headers are its own choice, not a proxy's report. - if (isPublicIp(peer)) { + // A peer that is not a proxy (outside TRUSTED_PROXY_CIDRS when it is + // set, on a public address when it is not) reached the server directly; + // its forwarding headers are its own choice, not a proxy's report. + if (!peerMayForward(peer)) { if ( request.headers.has("x-real-ip") || request.headers.get("x-forwarded-for")?.trim() !== peer diff --git a/src/lib/http/proxy-trust.ts b/src/lib/http/proxy-trust.ts new file mode 100644 index 000000000..d7cc9128d --- /dev/null +++ b/src/lib/http/proxy-trust.ts @@ -0,0 +1,175 @@ +/** + * Which connections may tell the server who the client is. + * + * A reverse proxy reports the client's address in `X-Forwarded-For` or + * `X-Real-IP`. Anyone else who sends those headers is choosing the address + * every per-IP rate limit, the audit log and the sign-in notices see, so the + * client-IP resolver (`getClientIpOrTrustWarning` in `@/lib/api-response`) + * reads them only from a peer this module says may forward. + * + * Two rules: + * + * - `TRUSTED_PROXY_CIDRS` set (comma-separated CIDRs, IPv4 or IPv6): only + * a peer inside one of them may forward. Every other connection is its + * own client, whatever address it comes from. + * - unset: any peer whose address is not public (loopback, private, + * CGNAT, unique-local, link-local) may forward. This is the long-standing + * default and stays for compatibility, but it is only as good as the + * network: behind Docker Desktop, OrbStack or `docker-proxy` every + * connection to a published port arrives from the gateway address, a + * private one, so a caller that reaches the port directly is believed + * again. Setting the list closes that. + * + * `TRUST_PROXY_HOPS` says how many proxies the `X-Forwarded-For` chain + * passed through. A value that is not a non-negative integer used to throw + * on every request that asked for the client address, `/api/health` + * included; it now resolves no address and is named once at boot. + * + * Both are read per call, so the suites can change them; the parsed CIDR + * list is cached by its raw value. + */ +import { BlockList, isIP } from "node:net"; + +import { isPublicIp } from "@/lib/validations/notifications"; + +// ── TRUST_PROXY_HOPS ────────────────────────────────────────────────────── + +/** + * The configured hop count, `1` when unset or empty, or `null` when the + * value is not a non-negative integer. + */ +export function parseTrustProxyHops(raw: string | undefined): number | null { + // Empty is unset. The shipped docker-compose.yml passes + // `TRUST_PROXY_HOPS: "${TRUST_PROXY_HOPS:-}"`, which is an empty string for + // every operator who never set it. + const trimmed = raw?.trim() ?? ""; + if (trimmed === "") return 1; + if (!/^\d+$/.test(trimmed)) return null; + return parseInt(trimmed, 10); +} + +// ── TRUSTED_PROXY_CIDRS ─────────────────────────────────────────────────── + +export type TrustedProxyList = + /** Unset: the private-address default applies. */ + | { kind: "unset" } + | { kind: "list"; entries: string[]; blockList: BlockList } + /** Set, but an entry is not a CIDR. No peer may forward. */ + | { kind: "invalid"; invalid: string[] }; + +/** `10.0.0.0/8`, `127.0.0.1`, `fd00::/8`; a bare address is a /32 or /128. */ +function parseCidr( + entry: string, +): { address: string; prefix: number; family: "ipv4" | "ipv6" } | null { + const [address = "", prefixText, extra] = entry.split("/"); + if (extra !== undefined) return null; + const version = isIP(address); + if (version === 0) return null; + const max = version === 4 ? 32 : 128; + if (prefixText !== undefined && !/^\d{1,3}$/.test(prefixText)) return null; + const prefix = prefixText === undefined ? max : Number(prefixText); + if (prefix > max) return null; + return { address, prefix, family: version === 4 ? "ipv4" : "ipv6" }; +} + +let cached: { raw: string; list: TrustedProxyList } | null = null; + +export function parseTrustedProxyCidrs( + raw: string | undefined, +): TrustedProxyList { + const value = raw?.trim() ?? ""; + if (cached && cached.raw === value) return cached.list; + let list: TrustedProxyList; + if (value === "") { + list = { kind: "unset" }; + } else { + const entries = value + .split(",") + .map((entry) => entry.trim()) + .filter((entry) => entry.length > 0); + const blockList = new BlockList(); + const invalid: string[] = []; + for (const entry of entries) { + const cidr = parseCidr(entry); + if (!cidr) { + invalid.push(entry); + continue; + } + blockList.addSubnet(cidr.address, cidr.prefix, cidr.family); + } + list = + invalid.length > 0 || entries.length === 0 + ? { kind: "invalid", invalid: invalid.length > 0 ? invalid : [value] } + : { kind: "list", entries, blockList }; + } + cached = { raw: value, list }; + return list; +} + +/** Whether a connection from `peer` may report the client's address. */ +export function peerMayForward(peer: string): boolean { + const list = parseTrustedProxyCidrs(process.env.TRUSTED_PROXY_CIDRS); + if (list.kind === "unset") return !isPublicIp(peer); + if (list.kind === "invalid") return false; + const family = isIP(peer) === 6 ? "ipv6" : "ipv4"; + return list.blockList.check(peer, family); +} + +// ── The boot line ───────────────────────────────────────────────────────── + +/** + * One line naming the rule in force, for the boot log: which peers may + * forward, how many hops are read, and what is wrong with a value that + * cannot be used. + */ +export function describeProxyTrust(): { + level: "info" | "error"; + message: string; +} { + const hopsRaw = process.env.TRUST_PROXY_HOPS; + const hops = parseTrustProxyHops(hopsRaw); + const list = parseTrustedProxyCidrs(process.env.TRUSTED_PROXY_CIDRS); + const problems: string[] = []; + if (hops === null) { + problems.push( + `TRUST_PROXY_HOPS=${JSON.stringify(hopsRaw)} is not a non-negative integer, so no client address is read from forwarding headers`, + ); + } + if (list.kind === "invalid") { + problems.push( + `TRUSTED_PROXY_CIDRS has entries that are not CIDRs (${list.invalid.map((e) => JSON.stringify(e)).join(", ")}), so no connection may report a client address`, + ); + } + const peers = + list.kind === "list" + ? `only connections from ${list.entries.join(", ")} (TRUSTED_PROXY_CIDRS)` + : list.kind === "unset" + ? "connections from any loopback, private, CGNAT, unique-local or link-local address (TRUSTED_PROXY_CIDRS unset; behind Docker Desktop, OrbStack or docker-proxy that includes every caller of a published port)" + : "no connection"; + if (problems.length > 0) { + return { + level: "error", + message: `[proxy-trust] ${problems.join("; ")}. Fix the value; see docs/self-hosting/reverse-proxy.md.`, + }; + } + return { + level: "info", + message: + hops === 0 + ? "[proxy-trust] TRUST_PROXY_HOPS=0: forwarding headers are never read; the client address is the connection's own." + : `[proxy-trust] forwarding headers are read from ${peers}, ${hops} hop${hops === 1 ? "" : "s"} deep (TRUST_PROXY_HOPS).`, + }; +} + +/** + * Log the rule in force once, at boot (`src/instrumentation.ts`). The + * logger is a parameter, as in the readiness summary: the production build + * strips direct `console.log` calls. + */ +export function logProxyTrustAtBoot( + log: Pick = console, +): void { + const { level, message } = describeProxyTrust(); + if (level === "error") log.error(message); + else log.log(message); +} From 70df2009d933f38cb80fed72d2f1882c58b5d130 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 25 Oct 2026 17:16:47 +0100 Subject: [PATCH 11/13] fix(deploy-webhook): keep DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP's meaning, add a signature-only flag DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP had been changed to mean signed requests only, so an operator who set it for a sender that adds a timestamp but cannot sign would have had every deploy notification refused after the upgrade. It means the secret header plus a fresh timestamp again. Signature-only, the setting that stops a replay, is DEPLOY_WEBHOOK_REQUIRE_SIGNATURE, on the compose whitelist, in the env manifest and examples, and in the deploy guide. --- .env.example | 6 +++- .env.production.example | 12 +++++-- docker-compose.yml | 14 +++++--- docs/ops/deploy.md | 17 +++++++--- scripts/env-manifest.json | 7 +++- .../deploy-webhook/__tests__/route.test.ts | 30 +++++++++++++++-- src/app/api/internal/deploy-webhook/route.ts | 33 +++++++++++++------ 7 files changed, 93 insertions(+), 26 deletions(-) diff --git a/.env.example b/.env.example index 462459b44..c0efd6658 100644 --- a/.env.example +++ b/.env.example @@ -376,8 +376,12 @@ API_TOKEN_HMAC_KEY="REPLACE-WITH-OUTPUT-OF-openssl-rand-hex-32" # Configure the SAME value in Coolify's Notifications → Webhook # custom-headers panel. See docs/ops/deploy.md. # DEPLOY_WEBHOOK_SECRET="" -# "true" accepts signed requests only; leave unset for Coolify's stock sender. +# "true" also requires a fresh X-Deploy-Webhook-Timestamp on a secret-header +# request; leave unset for Coolify's stock sender, which sends none. # DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP="" +# "true" accepts signed requests only (the setting that stops a replay); +# leave unset for Coolify's stock sender. +# DEPLOY_WEBHOOK_REQUIRE_SIGNATURE="" # ----------------------------------------------------------------------------- diff --git a/.env.production.example b/.env.production.example index 4c88afe04..dc6d226c1 100644 --- a/.env.production.example +++ b/.env.production.example @@ -295,11 +295,17 @@ API_TOKEN_HMAC_KEY="" # Optional: deployment-dashboard URL (e.g. the Coolify UI) linked in # deploy-failure notifications. When unset the notification omits the link. # DEPLOY_LOGS_URL="" +# Optional: require an X-Deploy-Webhook-Timestamp within +/- 5 minutes on a +# request that authenticates with the secret header. Keeps a stale capture +# out, not a fresh forgery by someone who saw the secret. Leave unset for +# Coolify's stock sender, which sends no timestamp. +# DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP="false" # Optional replay protection: accept only signed webhook calls # (X-Deploy-Webhook-Signature, an HMAC of the timestamp and body under the -# secret, with a timestamp within +/- 5 minutes). Leave unset for Coolify's -# stock sender, which can only send the secret header; see docs/ops/deploy.md. -# DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP="false" +# secret, with a timestamp within +/- 5 minutes); the secret header alone is +# refused. Leave unset for Coolify's stock sender, which can only send the +# secret header; see docs/ops/deploy.md. +# DEPLOY_WEBHOOK_REQUIRE_SIGNATURE="false" # ----------------------------------------------------------------------------- diff --git a/docker-compose.yml b/docker-compose.yml index cbf7e5aab..0a6ecc172 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -311,14 +311,18 @@ x-healthlog-env: &healthlog_env # X-Deploy-Webhook-Secret header (what Coolify's stock sender can do; no # replay protection) or with X-Deploy-Webhook-Signature: sha256=." under the secret>, the timestamp - # within 5 minutes. DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP=true accepts signed - # requests only. DEPLOY_LOGS_URL is the deployment-dashboard URL (e.g. the - # Coolify UI) linked in deploy-failure notifications. Listed here so - # operator values in .env reach the container (the `environment:` block is - # a whitelist). See docs/ops/deploy.md. + # within 5 minutes. DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP=true also requires a + # fresh X-Deploy-Webhook-Timestamp on a secret-header request; + # DEPLOY_WEBHOOK_REQUIRE_SIGNATURE=true accepts signed requests only, the + # one setting that stops a replay. DEPLOY_LOGS_URL is the + # deployment-dashboard URL (e.g. the Coolify UI) linked in deploy-failure + # notifications. Listed here so operator values in .env reach the + # container (the `environment:` block is a whitelist). See + # docs/ops/deploy.md. DEPLOY_WEBHOOK_SECRET: "${DEPLOY_WEBHOOK_SECRET:-}" DEPLOY_LOGS_URL: "${DEPLOY_LOGS_URL:-}" DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP: "${DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP:-}" + DEPLOY_WEBHOOK_REQUIRE_SIGNATURE: "${DEPLOY_WEBHOOK_REQUIRE_SIGNATURE:-}" # TLS leaf SPKI-change alarm baseline (optional). The native client # pins the served TLS leaf; this is the known-good pin set the monitor # alarms against on a rotation. Comma-separated for the dual-pin diff --git a/docs/ops/deploy.md b/docs/ops/deploy.md index 65593cd4b..c841da4fe 100644 --- a/docs/ops/deploy.md +++ b/docs/ops/deploy.md @@ -540,8 +540,17 @@ low-friction default. **5 minutes** of server time. A signed request sends no secret, and a captured one is useless once the window has passed. -Set `DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP=true` to accept signed requests only. -Use it when the sender, or a small relay in front of the app, can sign: +Two settings tighten this: + +- `DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP=true` requires a fresh + `X-Deploy-Webhook-Timestamp` on a request that uses the secret header; one + without it is refused with `401 {"status":"stale"}`. This is what the + setting has always meant. It keeps an old capture out, but not someone who + saw the secret and sends a new request with a new timestamp. +- `DEPLOY_WEBHOOK_REQUIRE_SIGNATURE=true` accepts signed requests only; the + secret header alone is refused with 401. This is the one that stops a + replay. Use it when the sender, or a small relay in front of the app, can + sign: ```sh TS=$(date +%s) @@ -555,5 +564,5 @@ curl -X POST https:///api/internal/deploy-webhook \ -d "$BODY" ``` -Before 1.46 the strict setting accepted the secret header plus a fresh -timestamp. Such a sender now gets 401 in strict mode and has to sign. +Both may be set; a signed request satisfies both. An operator who set the +timestamp flag before 1.46 keeps exactly the behaviour they had. diff --git a/scripts/env-manifest.json b/scripts/env-manifest.json index 51477220b..70546fd43 100644 --- a/scripts/env-manifest.json +++ b/scripts/env-manifest.json @@ -268,7 +268,12 @@ }, { "name": "DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP", - "purpose": "\"true\" (or \"1\") accepts only signed deploy-webhook requests (X-Deploy-Webhook-Signature with a fresh X-Deploy-Webhook-Timestamp); the secret header alone is refused with 401. Leave unset for Coolify's stock sender, which can only send the secret header.", + "purpose": "\"true\" (or \"1\") requires a fresh X-Deploy-Webhook-Timestamp (within 5 minutes) on a deploy-webhook request that authenticates with the secret header; one without it is refused with 401. Keeps a stale capture out, not a fresh forgery by someone who saw the secret. Leave unset for Coolify's stock sender, which sends no timestamp.", + "example": "true" + }, + { + "name": "DEPLOY_WEBHOOK_REQUIRE_SIGNATURE", + "purpose": "\"true\" (or \"1\") accepts only signed deploy-webhook requests (X-Deploy-Webhook-Signature with a fresh X-Deploy-Webhook-Timestamp); the secret header alone is refused with 401. The setting that stops a replay. Leave unset for Coolify's stock sender, which can only send the secret header.", "example": "true" } ] diff --git a/src/app/api/internal/deploy-webhook/__tests__/route.test.ts b/src/app/api/internal/deploy-webhook/__tests__/route.test.ts index 86c18de62..407bac693 100644 --- a/src/app/api/internal/deploy-webhook/__tests__/route.test.ts +++ b/src/app/api/internal/deploy-webhook/__tests__/route.test.ts @@ -444,12 +444,18 @@ describe("POST /api/internal/deploy-webhook (timestamp replay protection)", () = */ describe("POST /api/internal/deploy-webhook (signed requests)", () => { const ORIGINAL_REQUIRE = process.env.DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP; + const ORIGINAL_SIGNATURE = process.env.DEPLOY_WEBHOOK_REQUIRE_SIGNATURE; afterEach(() => { if (ORIGINAL_REQUIRE === undefined) { delete process.env.DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP; } else { process.env.DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP = ORIGINAL_REQUIRE; } + if (ORIGINAL_SIGNATURE === undefined) { + delete process.env.DEPLOY_WEBHOOK_REQUIRE_SIGNATURE; + } else { + process.env.DEPLOY_WEBHOOK_REQUIRE_SIGNATURE = ORIGINAL_SIGNATURE; + } }); async function signed( @@ -491,8 +497,8 @@ describe("POST /api/internal/deploy-webhook (signed requests)", () => { expect(auditLog).not.toHaveBeenCalled(); }); - it("in strict mode accepts only a signed request, not the bare secret with a fresh timestamp", async () => { - process.env.DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP = "true"; + it("with DEPLOY_WEBHOOK_REQUIRE_SIGNATURE accepts only a signed request, not the bare secret with a fresh timestamp", async () => { + process.env.DEPLOY_WEBHOOK_REQUIRE_SIGNATURE = "true"; const unsigned = jsonRequest( { status: "success" }, { @@ -503,4 +509,24 @@ describe("POST /api/internal/deploy-webhook (signed requests)", () => { expect((await POST(unsigned)).status).toBe(401); expect((await POST(await signed('{"status":"success"}'))).status).toBe(200); }); + + it("keeps DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP meaning the secret plus a fresh timestamp", async () => { + // What the flag meant before 1.46, and what an operator who set it with + // a sender that timestamps but cannot sign still gets. + process.env.DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP = "true"; + const fresh = jsonRequest( + { status: "success" }, + { + "x-deploy-webhook-secret": "test-secret", + "x-deploy-webhook-timestamp": String(Math.floor(Date.now() / 1000)), + }, + ); + expect((await POST(fresh)).status).toBe(200); + const bare = jsonRequest( + { status: "success" }, + { "x-deploy-webhook-secret": "test-secret" }, + ); + expect((await POST(bare)).status).toBe(401); + expect((await POST(await signed('{"status":"success"}'))).status).toBe(200); + }); }); diff --git a/src/app/api/internal/deploy-webhook/route.ts b/src/app/api/internal/deploy-webhook/route.ts index 9c09600e8..ef5aaef34 100644 --- a/src/app/api/internal/deploy-webhook/route.ts +++ b/src/app/api/internal/deploy-webhook/route.ts @@ -110,9 +110,15 @@ function normalizePayload(payload: CoolifyDeployPayload): NormalizedEvent { * - Default: a signed request is verified as above; an unsigned one * authenticates with the secret header, as Coolify's stock sender does, * and a timestamp it carries must still be fresh. - * - `DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP=true`: only a signed request is - * accepted. The name predates the signature; requiring a timestamp - * without one would not make a replay any harder. + * - `DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP=true`: what it has always meant, the + * secret header plus a fresh timestamp; an unsigned request without one + * is refused. It keeps a stale capture out, not a fresh forgery by + * someone who saw the secret. A signed request is accepted as above. + * - `DEPLOY_WEBHOOK_REQUIRE_SIGNATURE=true`: only a signed request is + * accepted; the secret header alone is refused. This is the setting + * that stops a replay. It has its own name so that an operator who set + * the timestamp flag, with a sender that adds a timestamp but cannot + * sign, keeps the behaviour they asked for. * * See docs/ops/deploy.md ("Deploy-status webhook") for the operator * guidance. @@ -132,7 +138,11 @@ function checkTimestampFreshness( now: number = Date.now(), ): { ok: boolean; reason?: string } { const raw = request.headers.get("x-deploy-webhook-timestamp"); - if (raw === null || raw === "") return { ok: true }; + if (raw === null || raw === "") { + return flagSet("DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP") + ? { ok: false, reason: "timestamp_missing" } + : { ok: true }; + } const ts = parseTimestamp(raw); if (ts === null) return { ok: false, reason: "timestamp_invalid" }; if (Math.abs(now - ts) > TIMESTAMP_WINDOW_MS) { @@ -141,11 +151,11 @@ function checkTimestampFreshness( return { ok: true }; } -function strictMode(): boolean { - return ( - process.env.DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP === "true" || - process.env.DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP === "1" - ); +function flagSet( + name: "DEPLOY_WEBHOOK_REQUIRE_TIMESTAMP" | "DEPLOY_WEBHOOK_REQUIRE_SIGNATURE", +): boolean { + const value = process.env[name]?.trim().toLowerCase(); + return value === "true" || value === "1"; } /** @@ -296,7 +306,10 @@ export const POST = apiHandler(async (request: NextRequest) => { getEvent()?.setAuth({ auth_method: "webhook_secret" }); annotate({ meta: { deploy_webhook_auth: "signature" } }); } else { - if (strictMode() || !hasValidSecret(request)) { + if ( + flagSet("DEPLOY_WEBHOOK_REQUIRE_SIGNATURE") || + !hasValidSecret(request) + ) { return NextResponse.json({ status: "unauthorized" }, { status: 401 }); } // Freshness AFTER the secret check: an unauthenticated caller learns From 698312863d16466eb44d036133c67eb7543e412f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 25 Oct 2026 17:18:45 +0100 Subject: [PATCH 12/13] test(labs): check the labs gate per read, and only the labs module's The guard asked whether a file that reads the lab table mentions the gate anywhere, so one ungated read in a file with a gated one passed, and whenModuleOn for any module counted as the labs gate. It now walks the syntax tree: each read must sit in a function that asks the labs module, or in a private helper whose every caller in the file does, or be listed by file and function with its reason. whenModuleOn counts only with "labs". --- .../labs-module-ai-gate-guard.test.ts | 273 +++++++++++++++--- 1 file changed, 228 insertions(+), 45 deletions(-) diff --git a/src/__tests__/labs-module-ai-gate-guard.test.ts b/src/__tests__/labs-module-ai-gate-guard.test.ts index 61a762c82..d798e69ef 100644 --- a/src/__tests__/labs-module-ai-gate-guard.test.ts +++ b/src/__tests__/labs-module-ai-gate-guard.test.ts @@ -14,49 +14,197 @@ * * ## What it proves and what it does not * - * Every module under the paths that build model input (`src/lib/ai`, - * `src/lib/insights`, `src/lib/jobs`, `src/lib/mcp`) that reads the lab - * table either asks the labs module itself (`isModuleEnabled(…, "labs")`, - * the `correlation:LAB_DRAWS` surface, or the MCP `whenModuleOn` wrapper) or - * is listed below with the reason its read cannot reach a prompt. A new - * reader fails here until it does one or the other. The data routes are not - * in scope: a module decides what a model is given, not what the owner can - * read (`CLAUDE.md`, "An AI capability gates model work"). + * Every lab read under the paths that build model input (`src/lib/ai`, + * `src/lib/insights`, `src/lib/jobs`, `src/lib/mcp`) either sits in a + * function that asks the labs module itself (`isModuleEnabled(…, "labs")`, + * the `correlation:LAB_DRAWS` surface, or the MCP `whenModuleOn(…, "labs")` + * wrapper), or sits in a private helper of its file whose every caller in + * that file does, or is listed below by file and function with the reason + * its rows cannot reach a prompt. A new reader fails here until it does one + * of those. The data routes are not in scope: a module decides what a model + * is given, not what the owner can read (`CLAUDE.md`, "An AI capability + * gates model work"). * - * It is grep-shaped: it sees a read, not where the rows go, and a reader in - * another directory is outside it. The behaviour is pinned per path by the - * module-off cases in the suites of the readers themselves. + * The check is per read, not per file. It used to ask whether a file that + * reads the table mentions the gate anywhere, so one ungated read in a file + * with a gated one passed, and `whenModuleOn` for any module counted as the + * labs gate. It still sees a read and the function around it, not where the + * rows go: a gate in the function is taken to cover the read, and a reader + * in another directory is outside it. The behaviour is pinned per path by + * the module-off cases in the suites of the readers themselves. */ +import ts from "typescript"; import { describe, expect, it } from "vitest"; import { readSourceFolder, stripComments } from "./helpers/source-files"; const ROOTS = ["src/lib/ai", "src/lib/insights", "src/lib/jobs", "src/lib/mcp"]; -/** A read of the lab table: the Prisma delegate or raw SQL naming it. */ -const LAB_READ = - /\.labResult\s*\.\s*(?:findMany|findFirst|findUnique|count|aggregate|groupBy)\s*\(|\blab_results\b/; +const READ_METHODS = new Set([ + "findMany", + "findFirst", + "findUnique", + "count", + "aggregate", + "groupBy", +]); + +/** Raw SQL naming the lab table. */ +const LAB_TABLE = /\blab_results\b/; /** What counts as asking the labs module. */ const LABS_GATE = - /isModuleEnabled\(\s*[\w.]+\s*,\s*"labs"\s*\)|"correlation:LAB_DRAWS"|whenModuleOn\(/; + /isModuleEnabled\(\s*[\w.]+\s*,\s*"labs"\s*\)|"correlation:LAB_DRAWS"|whenModuleOn\(\s*[\w.]+\s*,\s*"labs"/; -/** Readers whose rows never reach a model, each with the reason. */ +/** + * Reads whose rows never reach a model, by `file#top-level name`, each with + * the reason. + */ const NOT_MODEL_INPUT: Record = { - "src/lib/jobs/lab-biomarker-backfill.ts": + "src/lib/jobs/lab-biomarker-backfill.ts#runLabBiomarkerBackfillForUser": "links stored rows to their biomarker; no model is involved", - "src/lib/jobs/soft-delete-retention.ts": + "src/lib/jobs/lab-biomarker-backfill.ts#enqueueBootTimeLabBiomarkerBackfill": + "finds the accounts whose rows need linking; no model is involved", + "src/lib/jobs/soft-delete-retention.ts#SOFT_DELETE_RETENTION": "purges rows past the trash window; no model is involved", - "src/lib/insights/correlation-channel-series.ts": - "`fetchLabDraws`: both callers gate it, the Coach on correlation:LAB_DRAWS and the correlations data route on the same surface", - "src/lib/mcp/rich-reads.ts": - "`getLabHistory`: reached only through the MCP get_labs tool, which runs behind whenModuleOn", - "src/lib/ai/coach/tools/availability.ts": + "src/lib/insights/correlation-channel-series.ts#fetchLabDraws": + "both callers gate it, the Coach on correlation:LAB_DRAWS and the correlations data route on the same surface", + "src/lib/mcp/rich-reads.ts#getLabHistory": + 'reached only through the MCP get_labs tool, which runs behind whenModuleOn(ctx, "labs")', + "src/lib/ai/coach/tools/availability.ts#readTableBounds": "counts lab rows for the Coach's empty-read answer; the only lab subject comes from get_labs, which answers module_disabled first", + "src/lib/ai/coach/tools/availability.ts#holdsRowsBefore": + "whether lab rows predate the lookback, for the same empty-read answer and the same get_labs subject", }; -function labReaders(): Array<{ path: string; gated: boolean }> { - const out: Array<{ path: string; gated: boolean }> = []; +type FunctionNode = ts.SignatureDeclaration & { body?: ts.Node }; + +function isReadCall(node: ts.Node): boolean { + if (!ts.isCallExpression(node)) return false; + const callee = node.expression; + if (!ts.isPropertyAccessExpression(callee)) return false; + if (!READ_METHODS.has(callee.name.text)) return false; + const target = callee.expression; + return ( + ts.isPropertyAccessExpression(target) && target.name.text === "labResult" + ); +} + +function isTableLiteral(node: ts.Node): boolean { + return ( + (ts.isStringLiteral(node) || + ts.isNoSubstitutionTemplateLiteral(node) || + ts.isTemplateHead(node) || + ts.isTemplateMiddle(node) || + ts.isTemplateTail(node)) && + LAB_TABLE.test(node.text) + ); +} + +function enclosingFunctions(node: ts.Node): FunctionNode[] { + const out: FunctionNode[] = []; + for (let at = node.parent; at; at = at.parent) { + if (ts.isFunctionLike(at)) out.push(at as FunctionNode); + } + return out; +} + +/** The statement directly under the file that holds `node`. */ +function topLevelStatement(node: ts.Node): ts.Statement | null { + let at: ts.Node = node; + while (at.parent && !ts.isSourceFile(at.parent)) at = at.parent; + return at.parent ? (at as ts.Statement) : null; +} + +function declaredName(statement: ts.Statement): string | null { + if (ts.isFunctionDeclaration(statement)) return statement.name?.text ?? null; + if (ts.isVariableStatement(statement)) { + const [first] = statement.declarationList.declarations; + return first && ts.isIdentifier(first.name) ? first.name.text : null; + } + return null; +} + +function isExported(statement: ts.Statement): boolean { + return ( + ts.canHaveModifiers(statement) && + (ts.getModifiers(statement) ?? []).some( + (m) => m.kind === ts.SyntaxKind.ExportKeyword, + ) + ); +} + +/** + * Whether the read at `node` is behind the labs gate: a function around it + * asks, or it sits in a private top-level helper every one of whose + * references in the file is itself behind the gate. + */ +function gated( + file: ts.SourceFile, + node: ts.Node, + seen: Set = new Set(), +): boolean { + for (const fn of enclosingFunctions(node)) { + if (LABS_GATE.test(stripComments(fn.getText(file)))) return true; + } + const statement = topLevelStatement(node); + const name = statement ? declaredName(statement) : null; + if (!statement || !name || isExported(statement) || seen.has(name)) { + return false; + } + seen.add(name); + const references: ts.Node[] = []; + const visit = (at: ts.Node) => { + if ( + ts.isIdentifier(at) && + at.text === name && + topLevelStatement(at) !== statement + ) { + references.push(at); + } + ts.forEachChild(at, visit); + }; + visit(file); + return ( + references.length > 0 && + references.every((ref) => gated(file, ref, new Set(seen))) + ); +} + +interface LabRead { + /** `file#top-level name`. */ + site: string; + line: number; + gated: boolean; +} + +function readsIn(path: string, source: string): LabRead[] { + const file = ts.createSourceFile( + path, + source, + ts.ScriptTarget.Latest, + true, + path.endsWith(".tsx") ? ts.ScriptKind.TSX : ts.ScriptKind.TS, + ); + const out: LabRead[] = []; + const visit = (node: ts.Node) => { + if (isReadCall(node) || isTableLiteral(node)) { + const statement = topLevelStatement(node); + const name = (statement && declaredName(statement)) ?? "(top level)"; + out.push({ + site: `${path}#${name}`, + line: file.getLineAndCharacterOfPosition(node.getStart(file)).line + 1, + gated: gated(file, node), + }); + } + ts.forEachChild(node, visit); + }; + visit(file); + return out; +} + +function labReads(): LabRead[] { + const out: LabRead[] = []; for (const root of ROOTS) { for (const [rel, source] of readSourceFolder(root, { floor: 20, @@ -64,49 +212,84 @@ function labReaders(): Array<{ path: string; gated: boolean }> { !r.split("/").includes("__tests__") && !/\.(?:test|spec)\.tsx?$/.test(r), })) { - const code = stripComments(source); - if (!LAB_READ.test(code)) continue; - out.push({ path: `${root}/${rel}`, gated: LABS_GATE.test(code) }); + out.push(...readsIn(`${root}/${rel}`, source)); } } - return out.sort((a, b) => a.path.localeCompare(b.path)); + return out.sort((a, b) => a.site.localeCompare(b.site) || a.line - b.line); } describe("labs reach a model only with the labs module on", () => { - const readers = labReaders(); + const reads = labReads(); - it("finds the readers it exists to check", () => { + it("finds the reads it exists to check", () => { // An empty match set would pass every assertion below vacuously. - expect(readers.length).toBeGreaterThanOrEqual(8); - expect(readers.filter((r) => r.gated).length).toBeGreaterThanOrEqual(6); + expect(reads.length).toBeGreaterThanOrEqual(14); + expect(reads.filter((r) => r.gated).length).toBeGreaterThanOrEqual(8); }); - it("every reader asks the labs module or is listed as not model input", () => { - const open = readers - .filter((r) => !r.gated && !(r.path in NOT_MODEL_INPUT)) - .map((r) => r.path); + it("every read asks the labs module or is listed as not model input", () => { + const open = reads + .filter((r) => !r.gated && !(r.site in NOT_MODEL_INPUT)) + .map((r) => `${r.site}:${r.line}`); expect(open).toEqual([]); }); - it("lists no reader that has since gained the gate or gone", () => { - const paths = new Map(readers.map((r) => [r.path, r.gated])); + it("lists no site that has since gained the gate or gone", () => { const stale = Object.keys(NOT_MODEL_INPUT).filter( - (path) => paths.get(path) !== false, + (site) => !reads.some((r) => r.site === site && !r.gated), ); expect(stale).toEqual([]); }); - it("the matchers catch what they exist to catch", () => { - expect(LAB_READ.test("await prisma.labResult.findMany({")).toBe(true); - expect(LAB_READ.test("tx.labResult\n .count({")).toBe(true); - expect(LAB_READ.test("SELECT 1 FROM lab_results")).toBe(true); - expect(LAB_READ.test("prisma.labResultDocumentLink.findMany(")).toBe(false); + it("judges each read on its own, not the file it is in", () => { + const source = ` + export async function gatedRead(userId: string) { + if (!(await isModuleEnabled(userId, "labs"))) return null; + return prisma.labResult.findMany({ where: { userId } }); + } + export async function openRead(userId: string) { + return prisma.labResult.count({ where: { userId } }); + } + async function helper(userId: string) { + return prisma.labResult.findFirst({ where: { userId } }); + } + export async function callsHelperGated(userId: string) { + if (!(await isModuleEnabled(userId, "labs"))) return null; + return helper(userId); + } + async function helperTwo(userId: string) { + return prisma.$queryRaw\`SELECT 1 FROM lab_results\`; + } + export async function callsHelperTwo(userId: string) { + return helperTwo(userId); + } + export function medsOnly(ctx: Ctx) { + return whenModuleOn(ctx, "medications", "t", () => + prisma.labResult.findMany({}), + ); + } + `; + const verdicts = readsIn("fixture.ts", source).map( + (r) => `${r.site}:${r.gated}`, + ); + expect(verdicts).toEqual([ + "fixture.ts#gatedRead:true", + "fixture.ts#openRead:false", + "fixture.ts#helper:true", + "fixture.ts#helperTwo:false", + "fixture.ts#medsOnly:false", + ]); + }); + + it("the gate matcher takes the labs module only", () => { expect(LABS_GATE.test('await isModuleEnabled(userId, "labs")')).toBe(true); expect(LABS_GATE.test('await isModuleEnabled(ctx.userId, "labs")')).toBe( true, ); + expect(LABS_GATE.test('whenModuleOn(ctx, "labs", "get_labs"')).toBe(true); expect(LABS_GATE.test('isModuleEnabled(userId, "medications")')).toBe( false, ); + expect(LABS_GATE.test('whenModuleOn(ctx, "environment", "x"')).toBe(false); }); }); From f63d5d5819977ff9ff442f3581e3eb56460c4529 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 25 Oct 2026 18:02:45 +0100 Subject: [PATCH 13/13] fix(ai-runs): do not count time in the document reader against a run's deadline A background AI run's deadline is armed in model time plus a fixed margin. Time spent waiting in the document reader's queue, and the read itself, came out of that margin, so a person's lab scan or document read could be timed out by the reaper because other work held the reader. When the reader grants the slot it now moves the run's deadline by the wait plus the most the read may take, and gives back the unused part when the read ends, never past the job's own expiry and only while the run is running. The interactive queue wait stays within the margin, which covers the run until the slot is granted. --- .../documents/__tests__/pdf-isolation.test.ts | 74 +++++++++++++++++++ src/lib/documents/pdf-isolation.ts | 28 +++++++ .../jobs/__tests__/document-ai-run.test.ts | 2 +- src/lib/jobs/document-ai-run.ts | 34 ++++++++- tests/integration/document-ai-runs.test.ts | 62 ++++++++++++++++ 5 files changed, 198 insertions(+), 2 deletions(-) diff --git a/src/lib/documents/__tests__/pdf-isolation.test.ts b/src/lib/documents/__tests__/pdf-isolation.test.ts index 4bf197d77..84fed27f2 100644 --- a/src/lib/documents/__tests__/pdf-isolation.test.ts +++ b/src/lib/documents/__tests__/pdf-isolation.test.ts @@ -12,6 +12,7 @@ vi.mock("@/lib/logging/context", () => ({ annotate })); import { gif } from "../../__tests__/gif-fixture"; import { MAX_QUEUED_PER_OWNER, + READER_WAIT_MS, runOnce, runPdfChild, withPdfWork, @@ -301,3 +302,76 @@ describe("the reader's queue", () => { } }); }); + +describe("the caller's clock while it waits for the reader", () => { + const request = { op: "text", data: Buffer.from("x"), maxPages: 1 } as const; + + it("moves by the wait plus the read's allowance when the slot is granted, then gives back what the read did not use", async () => { + let open: () => void = () => {}; + const gate = new Promise((resolve) => (open = resolve)); + const holder = vi.fn(async (): Promise> => { + await gate; + return { ok: true, value: "first" }; + }); + const quick = vi.fn(async (): Promise> => ({ + ok: true, + value: "second", + })); + const moves: number[] = []; + const first = withPdfWork({ owner: "a", lane: "interactive" }, () => + runPdfChild(request, { timeoutMs: 1_000, run: holder }), + ); + const second = withPdfWork( + { + owner: "b", + lane: "interactive", + extendDeadline: async (ms) => { + moves.push(ms); + }, + }, + () => runPdfChild(request, { timeoutMs: 1_000, run: quick }), + ); + await new Promise((resolve) => setTimeout(resolve, 150)); + open(); + await expect(first).resolves.toMatchObject({ ok: true }); + await expect(second).resolves.toMatchObject({ ok: true, value: "second" }); + + // Granted after about 150 ms of waiting, with two attempts' allowance. + expect(moves).toHaveLength(2); + expect(moves[0]!).toBeGreaterThanOrEqual(2_000 + 140); + expect(moves[0]!).toBeLessThan(2_000 + 1_000); + // The quick read gives back nearly all of its allowance. + expect(moves[1]!).toBeLessThan(0); + expect(moves[1]!).toBeGreaterThan(-2_000 - 1); + // Net: the wait and the read's own time, nothing more. + expect(moves[0]! + moves[1]!).toBeGreaterThanOrEqual(140); + expect(moves[0]! + moves[1]!).toBeLessThan(1_000); + }); + + it("reads on when moving the clock fails", async () => { + const run = vi.fn(async (): Promise> => ({ + ok: true, + value: "text", + })); + await expect( + withPdfWork( + { + owner: "c", + lane: "interactive", + extendDeadline: async () => { + throw new Error("database gone"); + }, + }, + () => runPdfChild(request, { timeoutMs: 1_000, run }), + ), + ).resolves.toEqual({ ok: true, value: "text" }); + }); + + it("waits no longer for the reader than a run's margin can absorb before the slot moves its deadline", async () => { + // The deadline moves when the slot is granted; until then the run's + // armed margin is all that stands between a waiting read and the + // reaper. + const { AI_RUN_MARGIN_MS } = await import("../ai-runs/types"); + expect(READER_WAIT_MS.interactive).toBeLessThanOrEqual(AI_RUN_MARGIN_MS); + }); +}); diff --git a/src/lib/documents/pdf-isolation.ts b/src/lib/documents/pdf-isolation.ts index c0a2ea8dc..74167c6b7 100644 --- a/src/lib/documents/pdf-isolation.ts +++ b/src/lib/documents/pdf-isolation.ts @@ -143,6 +143,16 @@ export interface PdfWork { /** The account the work is done for; the queue takes accounts in turn. */ owner: string; lane: ReaderLane; + /** + * Moves the caller's own clock by `ms`. A background AI run has a deadline + * measured in model calls, and time spent waiting for the reader or + * reading is not model time: when the slot is granted the clock moves by + * the wait plus the most the read may take, and when the read ends it moves + * back by the part of that allowance the read did not use. So the run's + * model-time budget starts once the reader is done with it, however long + * the queue was. Failures are the caller's to swallow; the read goes on. + */ + extendDeadline?: (ms: number) => Promise; } const workContext = new AsyncLocalStorage(); @@ -265,10 +275,15 @@ export async function runPdfChild( } const work = currentPdfWork(); + const queuedAt = Date.now(); const ticket = await queue.acquire(work.owner, work.lane); if (!ticket.ok) { return { ok: false, reason: "busy", retryAfterMs: ticket.retryAfterMs }; } + const grantedAt = Date.now(); + // The most this read may take: every attempt to its time limit. + const allowance = limits.timeoutMs * MAX_ATTEMPTS; + await moveCallerClock(work, grantedAt - queuedAt + allowance); try { for (let attempt = 1; ; attempt++) { const result = limits.run @@ -279,6 +294,19 @@ export async function runPdfChild( } } finally { ticket.release(); + await moveCallerClock( + work, + -Math.max(0, allowance - (Date.now() - grantedAt)), + ); + } +} + +async function moveCallerClock(work: PdfWork, ms: number): Promise { + if (!work.extendDeadline || ms === 0) return; + try { + await work.extendDeadline(ms); + } catch { + // The caller's clock is the caller's; the read is not failed for it. } } diff --git a/src/lib/jobs/__tests__/document-ai-run.test.ts b/src/lib/jobs/__tests__/document-ai-run.test.ts index 7de034a2d..bd933e31e 100644 --- a/src/lib/jobs/__tests__/document-ai-run.test.ts +++ b/src/lib/jobs/__tests__/document-ai-run.test.ts @@ -293,7 +293,7 @@ describe("runDocumentAiRun", () => { return { ok: true, data: {} }; }); await runDocumentAiRun("run1", NOW + 2_700_000, now); - expect(seen).toEqual({ owner: "u1", lane: "interactive" }); + expect(seen).toMatchObject({ owner: "u1", lane: "interactive" }); }); it("answers a busy document reader with a retryable code and hands the reservation back", async () => { diff --git a/src/lib/jobs/document-ai-run.ts b/src/lib/jobs/document-ai-run.ts index 69949a2b0..0c5bd62ab 100644 --- a/src/lib/jobs/document-ai-run.ts +++ b/src/lib/jobs/document-ai-run.ts @@ -187,6 +187,34 @@ async function armDeadline( }); } +/** + * Move a running run's deadline by `ms`, never past the job's own expiry. + * The document reader calls it (`PdfWork.extendDeadline`): the deadline is + * model time, and a wait in the reader's queue or the read itself is not, so + * a person's read is not timed out because other work held the reader. + */ +export async function moveRunDeadline( + runId: string, + ms: number, + deadlineAt: number | undefined, +): Promise { + const delta = Math.round(ms); + if (deadlineAt === undefined) { + await prisma.$executeRaw` + UPDATE document_ai_runs + SET expires_at = expires_at + (${delta}::double precision * interval '1 millisecond') + WHERE id = ${runId} AND status = 'RUNNING'`; + return; + } + await prisma.$executeRaw` + UPDATE document_ai_runs + SET expires_at = LEAST( + expires_at + (${delta}::double precision * interval '1 millisecond'), + ${new Date(deadlineAt)} + ) + WHERE id = ${runId} AND status = 'RUNNING'`; +} + async function runDocumentIndex( run: ClaimedAiRun, deadlineAt: number | undefined, @@ -619,7 +647,11 @@ export async function runDocumentAiRun( // A person asked for this read and is waiting on it: the document // reader serves it ahead of background work, in turn with other people. outcome = await withPdfWork( - { owner: run.userId, lane: "interactive" }, + { + owner: run.userId, + lane: "interactive", + extendDeadline: (ms) => moveRunDeadline(run.id, ms, deadlineAt), + }, () => RUN_DISPATCH[run.kind](run, deadlineAt, now), ); } catch (err) { diff --git a/tests/integration/document-ai-runs.test.ts b/tests/integration/document-ai-runs.test.ts index 68a3a922c..177493de2 100644 --- a/tests/integration/document-ai-runs.test.ts +++ b/tests/integration/document-ai-runs.test.ts @@ -540,6 +540,68 @@ describe("the reaper", () => { }); }); +/** + * A run's deadline is model time. The document reader moves it by the time + * a read waited for the reader and took, so a person's lab scan is not + * timed out because other work held the reader; never past the job's own + * expiry, and only while the run is running. + */ +describe("the deadline while the document reader works", () => { + async function runningRun(expiresAt: Date) { + const user = await makeUser(); + await grantConsent(user.id); + await signIn(user.id); + const queued = await postLabScanText("Glucose 95 mg/dL"); + const { runId } = queued.body.data as { runId: string }; + await getPrismaClient().documentAiRun.update({ + where: { id: runId }, + data: { status: "RUNNING", expiresAt }, + }); + return runId; + } + const expiry = async (runId: string) => + ( + await getPrismaClient().documentAiRun.findUniqueOrThrow({ + where: { id: runId }, + }) + ).expiresAt.getTime(); + + it("moves a running run's deadline by the reader's time, so the reaper leaves it alone", async () => { + const { moveRunDeadline } = await import("@/lib/jobs/document-ai-run"); + const { reapAiRuns } = await import("@/lib/documents/ai-runs/store"); + const armed = new Date(Date.now() + 5_000); + const runId = await runningRun(armed); + + // The slot is granted after a 60 s wait, with 70 s allowed for the read. + await moveRunDeadline(runId, 130_000, undefined); + expect(await expiry(runId)).toBe(armed.getTime() + 130_000); + // Without the move the reaper would have taken it a minute later. + expect( + (await reapAiRuns(new Date(armed.getTime() + 60_000))).timedOut, + ).toBe(0); + + // The read took 10 s of its 70: the unused 60 s go back. + await moveRunDeadline(runId, -60_000, undefined); + expect(await expiry(runId)).toBe(armed.getTime() + 70_000); + }); + + it("never moves it past the job's own expiry, and leaves a run that is not running alone", async () => { + const { moveRunDeadline } = await import("@/lib/jobs/document-ai-run"); + const armed = new Date(Date.now() + 5_000); + const runId = await runningRun(armed); + const jobEnds = armed.getTime() + 20_000; + await moveRunDeadline(runId, 130_000, jobEnds); + expect(await expiry(runId)).toBe(jobEnds); + + await getPrismaClient().documentAiRun.update({ + where: { id: runId }, + data: { status: "FAILED" }, + }); + await moveRunDeadline(runId, 130_000, undefined); + expect(await expiry(runId)).toBe(jobEnds); + }); +}); + describe("a summary in the background", () => { it("marks a stored summary PENDING while queued and READY when it lands", async () => { const user = await makeUser();