Skip to content

Pr 500 qr code rebased - #558

Merged
BastiOfBerlin merged 4 commits into
spliit-app:mainfrom
BastiOfBerlin:pr-500-qr-code-rebased
Aug 13, 2026
Merged

Pr 500 qr code rebased#558
BastiOfBerlin merged 4 commits into
spliit-app:mainfrom
BastiOfBerlin:pr-500-qr-code-rebased

Conversation

@BastiOfBerlin

Copy link
Copy Markdown
Collaborator

feat: QR codes for sharing and joining groups

Picks up #500 by @theo-engels, rebased onto current main and with a
few 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:

  • Share a group as a QR code. A QrCode button in the share popover opens
    a 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.
  • Join a group by scanning one. On narrow viewports, "Add by URL" gains a
    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 from d3b151e back in January and no longer applies.

I regenerated the lockfile against current main on npm 11 rather than
replaying 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 main again.

package.json and the source files rebased cleanly.

Commits

ee4f467 @theo-engels' original commit, cherry-picked with -x, byte-identical
a849a7f regenerate package-lock.json for the two new dependencies
3195ad4 apply the repo's Prettier config to the three touched files
622e0e0 the fixes below

Please 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:

const [, groupId] =
  urlToProcess.match(new RegExp(`${window.location.origin}/groups/([^/]+)`)) ??
  urlToProcess.match(/\/groups\/([^/?]+)/) ?? // Also match relative URLs from QR
  []

The second pattern is unanchored and has no origin check, so it also matches
absolute URLs on any host — https://evil.example/groups/abc scans as a
valid group link. The first pattern has a separate problem that predates this
PR: it interpolates window.location.origin into a RegExp unescaped.

Both go away with the URL API:

const parsed = new URL(urlToProcess, window.location.origin)
if (parsed.origin === window.location.origin) {
  groupId = parsed.pathname.match(/^\/groups\/([^/]+)/)?.[1]
}

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. pathname also drops the query string, so
the ?ref=share on 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:

{!isScanning && hasPermission === null && ( <Button > )}

So dismissing the browser's camera prompt — easy to do by accident — flipped it
to false and left the error message as the only remaining UI. The only way
back was to close and reopen the popover. It now renders whenever the scanner
isn't running, and startScanning resets the flag so a stale error doesn't sit
underneath a fresh attempt.

Smaller things

  • includeMargin is deprecated in qrcode.react 4; switched to marginSize.
  • Gave the QR explicit bgColor/fgColor and a title. It renders over a
    themed surface, so it shouldn't inherit one, and the title gives screen
    readers something to announce.
  • Added title attributes to the copy and share buttons in the share popover,
    which had none. New en-US strings only, so Weblate picks them up normally.

New dependencies

qrcode.react ISC 45 kB unminified, rendering only
html5-qrcode Apache-2.0 375 kB minified, unpacked 3.4 MB

html5-qrcode is not small, and it's worth deciding deliberately. It's imported
by qr-code-scanner.tsx, a client component reachable from the groups list, so
Next 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/dynamic import behind the scan-mode toggle would
defer 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 current main.
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.toDataURL on an SVG-sourced image is the fiddly part). Worth someone
confirming both before this goes out.

theo-engels and others added 4 commits August 13, 2026 18:31
- 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.
@BastiOfBerlin
BastiOfBerlin merged commit 7aa38e0 into spliit-app:main Aug 13, 2026
1 check passed
@BastiOfBerlin
BastiOfBerlin deleted the pr-500-qr-code-rebased branch August 13, 2026 18:50
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants