Backend review: security, correctness and performance fixes

.claude/skills/pre-pr-review/SKILL.md

@@ -0,0 +1,41 @@
+---
+name: pre-pr-review
+description: Review a branch before its pull request is opened or updated. Use whenever a PR or merge request is about to be created, pushed or marked ready, or the user asks "can we open the PR", "make a PR", "push this", even when they do not mention a review.
+---
+
+# Before a pull request
+
+No pull request is opened, and no branch is pushed for one, until these steps
+pass. A step that fails is fixed on the branch, not noted for later.
+
+1. **Checks.** `npm run typecheck`, `npm run lint`, `npm run format:check`,
+   `npm run test:cov`, `npm run build`, and `npm run test:e2e` against a
+   database of its own (`docker run -d --name vrt-e2e-pr -e POSTGRES_PASSWORD=pg
+   -p 55499:5432 postgres:15.3-alpine3.18`, then with
+   `DATABASE_URL=postgresql://postgres:pg@localhost:55499/postgres`:
+   `npx prisma migrate deploy`, `npx prisma migrate diff --from-config-datasource
+   --to-schema prisma/schema.prisma --exit-code`, `npm run test:e2e`; remove it
+   with `docker rm -fv vrt-e2e-pr`). If the branch touches the Dockerfiles,
+   compose files, the seed or auth, also bring the stack up with
+   `docker compose up --build`, check `/health`, run `npm run test:acceptance`
+   (and `test:ldap` / `test:oidc` with their compose overlays when auth
+   changed), then `docker compose down --volumes`. If `dinov2/` changed, run
+   `python -m unittest test_patches` there.
+2. **Code review.** Run the `code-review` skill on the branch against its base
+   (`master` unless the PR targets another branch), at `high` effort or above
+   for a branch of more than a few commits. Fix every confirmed finding on the
+   branch; for a finding left as is, say why in the PR description.
+3. **Comments.** Go through the diff's comments with the rule in `AGENTS.md`
+   (Code conventions, "Comments"): delete any that restate the code, narrate
+   history, point at review notes or ids that are not in the repo, or would
+   not change what a reader does. Keep only non-obvious whys, one line each.
+   Test names follow the same rule.
+4. **Plan status.** If the branch works through a plan in `docs/`, tick the
+   done tasks there and record follow-ups.
+5. **Commits.** Author is the repo's configured git identity; no
+   `Co-Authored-By` or "Generated with" lines.
+
+Then open the PR. Its description lists what changed by area, how it was
+verified (the commands above and their results), what needs attention when
+deploying (migrations, new or changed environment variables, behaviour
+changes), and what is deliberately left for later.

.env

@@ -1,4 +1,4 @@
-JWT_SECRET='jwtPrivateKey'
+JWT_SECRET='vrt-local-development-only-jwt-secret'
 JWT_LIFE_TIME=1d
 
 BODY_PARSER_JSON_LIMIT="5mb"
@@ -30,6 +30,15 @@ AWS_S3_BUCKET_NAME=
 
 # Multi-instance socket.io fan-out (optional). Unset = in-memory adapter.
 #REDIS_URL=redis://localhost:6379
+# How long the first Redis connection may take before the API exits instead of waiting (default 30000).
+
+# Log levels, comma separated; unset logs everything. Read from the process environment, not from this file.
+#LOG_LEVEL=log,warn,error,fatal
+
+# S3 request timeouts in milliseconds (STATIC_SERVICE=s3)
+
+
+#REGISTRATION_ENABLED=true
 
 #LDAP_ENABLED=true
 #LDAP_TLS_NO_VERIFY=false
@@ -67,3 +76,9 @@ OLLAMA_BASE_URL=http://localhost:11434
 # DINOv2 service for the rendering-noise filter's "dinov2Check" (see dinov2/README.md)
 #DINOV2_URL=http://localhost:8000
 #DINOV2_TIMEOUT_MS=60000
+#DATABASE_URL=postgresql://postgres:postgres@localhost:5432/vrt_db_dev # required; docker-compose.yml builds it from POSTGRES_*
+#MAX_IMAGE_PIXELS=100000000 # largest image in pixels (width x height); 0 turns the limit off
+#MULTIPART_FILE_SIZE_LIMIT=104857600 # largest multipart upload in bytes (100 MiB)
+#DIFF_WORKERS_COUNT= # diff worker threads; unset: CPUs minus 1, between 1 and 8
+#DIFF_QUEUE_LIMIT=256 # diff jobs waiting for a worker; when full the API answers 503
+#DIFF_WORKER_MAX_HEAP_MB=512 # heap limit of each diff worker, MB

.github/dependabot.yml

@@ -46,17 +46,28 @@ updates:
           - "*"
 
   - package-ecosystem: docker
-    # base images of the api (Dockerfile) and the migration image (prisma/Dockerfile)
+    # base images of the api (Dockerfile), the migration image (prisma/Dockerfile) and dinov2/Dockerfile
     directories:
       - "/"
       - "/prisma"
+      - "/dinov2"
     schedule:
       interval: weekly
     groups:
       docker:
         patterns:
           - "*"
 
+  - package-ecosystem: pip
+    # the DINOv2 service image installs dinov2/requirements.txt
+    directory: "/dinov2"
+    schedule:
+      interval: weekly
+    groups:
+      pip:
+        patterns:
+          - "*"
+
   - package-ecosystem: github-actions
     directory: "/"
     schedule:

.github/workflows/docker.yml

@@ -1,41 +1,23 @@
 name: Docker images
 
-# Builds visualregressiontracker/api, visualregressiontracker/migration and
-# visualregressiontracker/dinov2 for linux/amd64 and linux/arm64 (Apple
-# Silicon, Graviton), so docker compose runs them natively instead of under
-# emulation.
-# - pushing an api-vX.Y.Z, migration-vX.Y.Z or dinov2-vX.Y.Z tag publishes that image as X.Y.Z
-# - "Run workflow" with an existing tag (re)publishes it, e.g. to add arm64 to a released version
-# - pull requests that touch the images only build them, without pushing
-# Publishing needs the DOCKERHUB_USERNAME and DOCKERHUB_TOKEN repository secrets.
-
 on:
   push:
     tags:
       - 'api-v*'
       - 'migration-v*'
       - 'dinov2-v*'
-  pull_request:
-    branches: [master]
-    paths:
-      - Dockerfile
-      - prisma/Dockerfile
-      - prisma/schema.prisma
-      - dinov2/**
-      - .github/workflows/docker.yml
   workflow_dispatch:
     inputs:
       tag:
         description: Existing tag to publish, e.g. api-v5.7.0, migration-v5.5.0 or dinov2-v1.0.0
         required: true
 
-# GITHUB_TOKEN is only used to check out the code; image pushes use the Docker Hub secrets
 permissions:
   contents: read
 
 jobs:
   docker:
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-24.04
     strategy:
       fail-fast: false
       matrix:
@@ -54,24 +36,22 @@ jobs:
           TAG: ${{ inputs.tag || github.ref_name }}
           PREFIX: ${{ matrix.image }}-v
         run: |
-          if [ "$GITHUB_EVENT_NAME" = "pull_request" ]; then
-            echo "build=true" >> "$GITHUB_OUTPUT"
-            echo "push=false" >> "$GITHUB_OUTPUT"
-          elif [ "${TAG#"$PREFIX"}" != "$TAG" ]; then
-            echo "build=true" >> "$GITHUB_OUTPUT"
-            echo "push=true" >> "$GITHUB_OUTPUT"
-            echo "version=${TAG#"$PREFIX"}" >> "$GITHUB_OUTPUT"
+          if [ "${TAG#"$PREFIX"}" != "$TAG" ]; then
+            {
+              echo "build=true"
+              echo "push=true"
+              echo "version=${TAG#"$PREFIX"}"
+            } >> "$GITHUB_OUTPUT"
           else
             echo "Tag $TAG is not a ${{ matrix.image }} release, skipping"
             echo "build=false" >> "$GITHUB_OUTPUT"
           fi
 
-      - uses: actions/checkout@v7
+      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
         if: steps.version.outputs.build == 'true'
         with:
           # a manual run checks out the tag itself, never a branch with the same name
           ref: ${{ inputs.tag && format('refs/tags/{0}', inputs.tag) || github.ref }}
-          # keep the token out of .git/config, which is part of the build context
           persist-credentials: false
 
       - name: Set up QEMU
@@ -96,6 +76,6 @@ jobs:
           context: ${{ matrix.context }}
           platforms: linux/amd64,linux/arm64
           push: ${{ steps.version.outputs.push == 'true' }}
-          tags: visualregressiontracker/${{ matrix.image }}:${{ steps.version.outputs.version || 'ci' }}
+          tags: visualregressiontracker/${{ matrix.image }}:${{ steps.version.outputs.version }}
           cache-from: type=gha,scope=${{ matrix.image }}
           cache-to: type=gha,mode=max,scope=${{ matrix.image }}

.github/workflows/workflow.yml

@@ -6,7 +6,6 @@ on:
   pull_request:
     branches: [master]
 
-# a new push to a pull request supersedes its running checks; master runs to completion
 concurrency:
   group: ${{ github.workflow }}-${{ github.ref }}
   cancel-in-progress: ${{ github.event_name == 'pull_request' }}
@@ -16,15 +15,14 @@ permissions:
 
 jobs:
   checks:
-    # https://github.com/actions/runner-images#available-images
     runs-on: ubuntu-24.04
     timeout-minutes: 15
 
     steps:
-      - uses: actions/checkout@v7
+      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
 
       - name: Setup Node.js environment
-        uses: actions/setup-node@v7
+        uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0
         with:
           node-version: '24'
           cache: npm
@@ -35,6 +33,10 @@ jobs:
       - name: Install npm dependencies
         run: npm ci
 
+      - name: Install prisma npm dependencies
+        working-directory: prisma
+        run: npm ci
+
       - name: Static analysis
         run: npm run lint
 
@@ -55,7 +57,6 @@ jobs:
       fail-fast: false
       matrix:
         include:
-          # acceptance tests call the api container, e2e tests run the app in-process against its database
           - suite: e2e
             compose-file: docker-compose.yml
             test: npm run test:acceptance && npm run test:e2e
@@ -66,14 +67,14 @@ jobs:
             compose-file: docker-compose.yml:docker-compose.oidc.yml
             test: npm run test:oidc
     env:
-      # read by every docker compose command below
       COMPOSE_FILE: ${{ matrix.compose-file }}
+      COMPOSE_PROJECT_NAME: vrt
 
     steps:
-      - uses: actions/checkout@v7
+      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
 
       - name: Setup Node.js environment
-        uses: actions/setup-node@v7
+        uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0
         with:
           node-version: '24'
           cache: npm
@@ -84,12 +85,32 @@ jobs:
       - name: Install npm dependencies
         run: npm ci
 
-      - name: Build and run containers
-        run: docker compose up --build -d
+      - name: Set up Docker Buildx
+        uses: docker/setup-buildx-action@f87e5991a6d7451dcb8d9637bfbc97413f497069 # v4.4.1
+
+      - name: Build api image
+        uses: docker/build-push-action@c3c9e263c25d99ce0380d002d59b67737d91b0dc # v7.4.0
+        with:
+          context: .
+          load: true
+          tags: vrt-api:latest
+          cache-from: type=gha,scope=ci-api
+          cache-to: type=gha,mode=max,scope=ci-api
+
+      - name: Build migration image
+        uses: docker/build-push-action@c3c9e263c25d99ce0380d002d59b67737d91b0dc # v7.4.0
+        with:
+          context: prisma
+          load: true
+          tags: vrt-migration:latest
+          cache-from: type=gha,scope=ci-migration
+          cache-to: type=gha,mode=max,scope=ci-migration
+
+      - name: Run containers
+        run: docker compose up --no-build -d
 
       - name: Wait for migrations and the api
         run: |
-          # the migration container applies the migrations, seeds the default user and exits
           status=$(docker wait vrt_migration)
           if [ "$status" != 0 ]; then
             echo "::error::migration container exited with $status"
@@ -113,12 +134,28 @@ jobs:
     # See: https://github.com/Visual-Regression-Tracker/backend/issues/273
     runs-on: ubuntu-24.04
     timeout-minutes: 10
+    services:
+      postgres:
+        image: postgres:15.3-alpine3.18
+        env:
+          POSTGRES_USER: postgres
+          POSTGRES_PASSWORD: postgres
+          POSTGRES_DB: vrt_migrations
+        ports:
+          - 5432:5432
+        options: >-
+          --health-cmd "pg_isready -U postgres -d vrt_migrations"
+          --health-interval 5s
+          --health-timeout 5s
+          --health-retries 20
+    env:
+      DATABASE_URL: postgresql://postgres:postgres@localhost:5432/vrt_migrations
 
     steps:
-      - uses: actions/checkout@v7
+      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
 
       - name: Setup Node.js environment
-        uses: actions/setup-node@v7
+        uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0
         with:
           node-version: '24'
           cache: npm
@@ -130,28 +167,31 @@ jobs:
         working-directory: prisma
         run: npm ci
 
-      - name: Static analysis
+      - name: Unit tests
         working-directory: prisma
         run: npm run test
 
+      - name: Migrations match the schema
+        working-directory: prisma
+        run: |
+          npx prisma migrate deploy
+          npx prisma migrate diff --from-config-datasource --to-schema schema.prisma --exit-code
+
   test-dinov2:
     runs-on: ubuntu-24.04
     timeout-minutes: 10
 
     steps:
-      - uses: actions/checkout@v7
+      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
 
-      # the logic is tested with a stand-in encoder: numpy only, no model;
-      # the runner's own Python 3.12, in a venv (system pip is locked down)
       - name: Unit tests
         working-directory: dinov2
         run: |
           python3 -m venv .venv
           .venv/bin/pip install numpy==2.5.4
           .venv/bin/python -m unittest test_patches
 
-  # Branch protection requires a check named "build": it passes only when every job above did,
-  # so the required check keeps covering the whole pipeline after the split.
+  # branch protection requires a check named "build"
   build:
     if: ${{ !cancelled() }}
     needs: [checks, integration, test-migrations, test-dinov2]

.gitignore

@@ -40,3 +40,7 @@ lerna-debug.log*
 
 # TypeScript incremental build info
 *.tsbuildinfo
+
+# Claude Code
+.claude/settings.local.json
+.claude/worktrees/

AGENTS.md

@@ -55,7 +55,15 @@ Run typecheck, lint, format check and the unit tests before calling work done.
   Fields older clients do not send must be `@IsOptional()`, or saving breaks for them.
 - Format only the files you changed (`npx prettier --write <files>`), not whole
   directories: prettier also rewrites untouched files, markdown included.
-- Comments explain *why*, in the surrounding style; match existing naming.
+- Comments: by default, none. Write one only for a non-obvious _why_ that a
+  reader would otherwise get wrong, and keep it to one line. Delete on sight:
+  comments that restate the code or its names, narrate past bugs or earlier
+  versions (that goes in the commit message), explain what a well-named
+  function already says, point at review notes or ticket ids that are not in
+  the repo, or run for several lines without changing what a reader would do.
+  No TODOs or "known issue" notes (open an issue). Tests follow the same rule:
+  the test name says what is checked. Match existing naming. Before a pull
+  request, see the `pre-pr-review` skill.
 
 ## Services beside the API