Pr 500 qr code rebased - #558
Merged
BastiOfBerlin merged 4 commits intoAug 13, 2026
Merged
Conversation
- Add internationalization (i18n) to QrCodeScanner component using next-intl - Extract hardcoded UI strings: "Start Camera", "Stop Scanning", camera error messages - Follow existing translation conventions used in other components - Fix ShareQrCodeDialog to support multiple instances on same page - Replace hardcoded id="qr-code-svg" with per-instance unique id via useId() - Update handleDownload to reference dynamic id instead of document.getElementById() - Ensures no DOM id conflicts when multiple dialogs are rendered (cherry picked from commit d19590f)
Regenerated on npm 11 against current main instead of replaying the original lockfile diff, which was cut from d3b151e in January and no longer applies.
Formatting only, no behaviour change. Kept separate from the authored commit so that one stays byte-identical to the contribution.
Layered on top of spliit-app#500, from a fork that had arrived at a QR code independently. - Parse the pasted or scanned URL with the URL API instead of building a RegExp from window.location.origin, which interpolates it unescaped. Resolving against the current origin keeps spliit-app#500's support for relative /groups/<id> links from a scanned code, while an absolute link retains its own origin and so still fails the same-origin check — the previous relative-URL fallback matched any host. - Let the scanner be retried after a permission denial. The start button rendered only while hasPermission was null, so a dismissed browser prompt left the error message as the only remaining UI until the popover was closed and reopened. - Use marginSize instead of includeMargin, deprecated in qrcode.react 4, and give the QR an explicit title and colours so it stays legible when the dialog is rendered against a dark theme. - Add titles to the copy and share buttons, which had none.
This was referenced Aug 13, 2026
BastiOfBerlin
added a commit
that referenced
this pull request
Aug 13, 2026
…560) # security: harden AI extraction, CSV export, and the migration script Part of the series in #553. Five independent fixes, none of which changes behaviour for a correctly-configured instance. Grouped into one PR because they are all small and all fall out of the same audit; happy to split any of them out if you'd rather review them separately. Nothing here is being publicly disclosed anywhere; if you'd prefer to take any of it privately instead, say so and I'll close this and re-send. ## 1. AI extraction was reachable with the feature flags off `extractExpenseInformationFromImage` and `extractCategoryFromTitle` are server actions, so each has its own callable endpoint. `ENABLE_RECEIPT_EXTRACT` and `ENABLE_CATEGORY_EXTRACT` only ever hid the buttons that call them. An instance that deliberately leaves the AI features off — because the operator does not want to pay for them, or has no key configured — still had them callable by anyone who can reach the app, spending the operator's OpenAI credit. Both actions now check `getRuntimeFeatureFlags()` before doing any work. ## 2. Receipt extraction forwarded any caller-supplied URL to OpenAI The image URL is a plain string parameter of the action. Since the action is directly callable (see above), an arbitrary URL could be passed and OpenAI would fetch it: an SSRF primitive using someone else's egress, plus unmetered API spend on images the app never uploaded. New `isAllowedUploadUrl` restricts extraction to `http(s)` URLs on the app's own configured S3 host. It derives that host exactly the way `next.config.mjs` derives the image `remotePatterns` — custom `S3_UPLOAD_ENDPOINT` if set, otherwise the `bucket.s3.region.amazonaws.com` form — so the trusted-host set stays consistent between the two. With no S3 configured the allow-list is empty and nothing passes, which is the right answer: without uploads there are no receipts to extract. Unit-tested, including suffix look-alikes (`minio.example.com.evil.com`), non-`http(s)` schemes, port insensitivity, and the unconfigured case. ## 3. The receipt action could return `NaN` The model is asked for a plain number but nothing guarantees it obliges; `Number()` on anything else yields `NaN`, which flowed into the expense form. It now reports `null`, which the dialog already renders as "unknown" — the existing truthiness check on `receiptInfo.amount` handles both, so this is a tightening rather than a behaviour change. ## 4. CSV formula injection (CWE-1236) Expense titles, category names and participant names are user-controlled and were written to the export verbatim. A cell beginning with `=`, `+`, `-`, `@`, tab or CR is executed as a formula by Excel, LibreOffice and Google Sheets when the file is opened. So a group member can title an expense `=HYPERLINK(...)` or a `WEBSERVICE` call and have it execute in another member's spreadsheet — the export is the delivery mechanism, and the person who opens it is not the person who wrote the text. Such cells are now prefixed with a single quote, the standard neutralisation. ## 5. The CSV route created its own `PrismaClient` Every other data path uses the `@/lib/prisma` singleton; this route constructed a second client at module scope, opening a second connection pool. On a long-running container that is a slow leak against the database's connection limit. Now uses the singleton, matching the JSON export route right next to it. ## Two smaller items in the same spirit - **`src/scripts/migrate.ts` disabled TLS verification process-wide.** It set `NODE_TLS_REJECT_UNAUTHORIZED='0'` at module scope, which turns off certificate validation for *every* outbound TLS connection the process makes, not just the legacy database one it was meant for. The script now connects with validated TLS; a self-signed legacy database can supply its CA through a new `OLD_POSTGRES_CA_CERT` env var. (This is the one-off v1→v2 import script, so the blast radius is small, but it is also the script most likely to be run with production credentials in the environment.) - **`ci.yml` had no `permissions:` block**, so it ran with the repository's default token scopes. The checks only read the repository. `cd.yml` already declares per-job permissions, so this just brings CI in line. ## Verification `npm ci --ignore-scripts`, `npx prisma generate`, `check-types`, `lint`, `check-formatting` all clean against current `main` (lint: 16 warnings, all pre-existing). `uploaded-image-url.test.ts` adds 14 cases. ## Not included The regex-injection fix in `add-group-by-url-button.tsx` was part of the same audit, but it already went upstream with #558 — it was needed there to keep the QR scanner's relative-URL support from accepting any host. Co-authored-by: Claude <noreply@anthropic.com>
BastiOfBerlin
added a commit
that referenced
this pull request
Aug 13, 2026
# deps: routine updates Part of the series in #553. Two commits, three changed import lines, no behaviour change intended. ## This turned out to be much smaller than I advertised in #553 I had this down as "folds nine merged Dependabot PRs from my fork". That framing was wrong, and the reason is worth stating because it also shrinks the major-upgrades PR later in the series. **`main`'s lockfile is already current for almost everything.** `npm ci` installs what the lockfile pins, carets float, and the lockfile has been regenerated recently (#556, #558). So: | declared in `package.json` | actually installed | |---|---| | `@radix-ui/react-select: ^2.3.0` | 2.3.7 | | `@tanstack/react-query: ^5.59.15` | 5.101.4 | | `@trpc/server: ^11.0.0-rc.586` | **11.18.0** — stable, not the rc | | `dayjs: ^1.11.10` | 1.11.21 | | `next: ^16.0.7` | 16.3.1 | | `prettier: ^3.0.3` | 3.9.6 | Nothing there needs upgrading. My fork's versions of those lines were *behind* what you already ship. So this PR is not the batch of routine bumps I described — it's the handful the caret ranges were actually blocking, plus a cleanup. ## 1. The six that were genuinely stale Each of these needed a major-range change to move at all: - **`content-disposition` 0.5.4 → 2.0.1.** v2 exports `create` instead of a default, so the CSV and JSON export routes import `{ create as contentDisposition }`. It ships its own types, so `@types/content-disposition` goes. - **`negotiator` 0.6.4 → 1.0.0** - **`@formatjs/intl-localematcher` 0.5.10 → 0.8.13** - **`next-themes` 0.2.1 → 0.4.6.** `ThemeProviderProps` moved to the package root; the `next-themes/dist/types` deep import no longer resolves. - **`dotenv` 16.6.1 → 17.4.2** - **`@total-typescript/ts-reset` 0.5.1 → 0.6.1** **The first three belong together.** `next-intl` 4.13 already depends on `@formatjs/intl-localematcher@^0.8` and `negotiator@^1`, so pinning the root at 0.5 and 0.6 forced npm to carry nested duplicate copies of both. Raising the root deduplicates them. Also dropped **`uuid` and `@types/uuid`** — nothing imports uuid. `next-s3-upload` pulls its own copy, which is now hoisted rather than nested. Net: **930 packages, down from 934.** ## 2. Realigning the declared ranges — provably a no-op 43 dependencies declare a floor far below what is installed. `tailwindcss: "^3"` resolving to 3.4.19; `"@types/node": "^20"` to 20.19.43. The manifest describes a tree nobody runs. That costs two things: a fresh `npm install` without the lockfile may legally pick something years older than CI has ever exercised, and reading `package.json` — the normal way to answer "what are we on" — currently gives the wrong answer for a third of the tree. Each range is raised to `^<installed>`, so no major boundary is crossed and nothing is upgraded; the installed version already satisfied the old range by definition. **I verified this rather than asserting it.** Regenerating the lockfile from scratch with the old floors and with the new ones produces *identical* resolutions for all 930 packages — I diffed every entry. The only lockfile change in that commit is the root entry, which mirrors `package.json`. It's a separate commit precisely so you can drop it if you'd rather not carry the churn. Commit 1 stands alone without it. ## Deliberately left behind `prisma`/`@prisma/client` at 6, `zod` at 3, `typescript` at 5, `jest` at 29, `tailwindcss` at 3, `lucide-react` at 0.501, `openai` at 4, `@hookform/resolvers` at 3, `@types/react` at 18. Those have API surface and get their own PR. One of them is worth flagging now, though: **`react` is on 19.2.8 while `@types/react` is on 18.3.31.** That mismatch is pre-existing on `main`, not something this PR introduces, and it type-checks today — but it's the kind of thing that produces confusing errors, so I'll fix it in the major-upgrades PR unless you'd rather have it sooner. ## On the audit warning `npm audit` reports 2 moderate advisories against `uuid` (GHSA-w5hq-g745-h8pq), reached through `next-s3-upload`. **This PR does not fix that and does not make it worse** — it's transitive, `next-s3-upload` pins its own range, and there's no patched version in that range. Dropping our direct `uuid` doesn't help because it was never the vulnerable path. Same advisory count before and after. ## Verification Against `f8ccc7a`, Node 24 / npm 11: `npm ci --ignore-scripts`, `npx prisma generate`, `check-types`, `lint` (16 warnings / 0 errors, all pre-existing), `check-formatting`, `npm test` — 7 suites, 101 tests. Also ran a full `npm run build`, since `tsc` won't catch runtime breakage from the Radix or `next-intl` resolution changes. All 25 routes compile. Not verified: the two export routes and the theme toggle in a browser. The `content-disposition` and `next-themes` changes are the only ones that touch running code, and both are worth a click — a CSV export with a non-ASCII group name, and a dark/light toggle. --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat: QR codes for sharing and joining groups
Picks up #500 by @theo-engels, rebased onto current
mainand with afew fixes layered on top. Their commit is unchanged and still under their
name — everything I added is in a separate commit at the end.
Closes #500.
What it does
Two things, both from the original PR:
QrCodebutton in the share popover opensa dialog with the group's URL as a QR, the Spliit logo embedded in the middle,
and a button to download it as a PNG.
second mode that opens the camera and accepts a scanned group link.
Why it needed a rebase
The PR conflicted, but only on
package-lock.json— its lockfile commit(
3fabe84) was cut fromd3b151eback in January and no longer applies.I regenerated the lockfile against current
mainon npm 11 rather thanreplaying that diff. It produces the same 17-line/11-line shape, and after #556
I'd rather not replay a six-month-old lockfile diff onto
mainagain.package.jsonand the source files rebased cleanly.Commits
ee4f467-x, byte-identicala849a7fpackage-lock.jsonfor the two new dependencies3195ad4622e0e0Please merge this with a merge commit or a rebase rather than a squash —
squashing collapses their commit and mine into one authorship.
What I changed on top
The relative-URL fallback accepted any host
This is the one worth a close look. #500 extends the "add by URL" parser so a
scanned code can carry a relative link:
The second pattern is unanchored and has no origin check, so it also matches
absolute URLs on any host —
https://evil.example/groups/abcscans as avalid group link. The first pattern has a separate problem that predates this
PR: it interpolates
window.location.origininto aRegExpunescaped.Both go away with the URL API:
Resolving against the current origin is what keeps the relative-link case
working — a relative path inherits our origin and passes, while an absolute URL
keeps its own and fails the check.
pathnamealso drops the query string, sothe
?ref=shareon shared links no longer needs its own character class.The scanner couldn't be retried after a denied permission
The start button rendered only while
hasPermission === null:So dismissing the browser's camera prompt — easy to do by accident — flipped it
to
falseand left the error message as the only remaining UI. The only wayback was to close and reopen the popover. It now renders whenever the scanner
isn't running, and
startScanningresets the flag so a stale error doesn't situnderneath a fresh attempt.
Smaller things
includeMarginis deprecated in qrcode.react 4; switched tomarginSize.bgColor/fgColorand atitle. It renders over athemed surface, so it shouldn't inherit one, and the title gives screen
readers something to announce.
titleattributes to the copy and share buttons in the share popover,which had none. New
en-USstrings only, so Weblate picks them up normally.New dependencies
qrcode.reacthtml5-qrcodehtml5-qrcodeis not small, and it's worth deciding deliberately. It's importedby
qr-code-scanner.tsx, a client component reachable from the groups list, soNext will split it into that route's chunk rather than the shared bundle — but
anyone landing on the groups page pays for it once the chunk loads. If that's
not a trade you want, a
next/dynamicimport behind the scan-mode toggle woulddefer it to the moment someone actually taps "Scan QR", and I'm happy to add
that here.
Verification
npm ci --ignore-scripts,npx prisma generate,npm run check-types,npm run lint,npm run check-formatting— all clean against currentmain.Lint reports 16 warnings, all of them pre-existing.
Not yet exercised on a physical device: the camera path needs a real phone, and
the PNG download path needs a browser check on Safari in particular
(
canvas.toDataURLon an SVG-sourced image is the fiddly part). Worth someoneconfirming both before this goes out.