diff --git a/.dockerignore b/.dockerignore index 4feeabd..b5491c1 100644 --- a/.dockerignore +++ b/.dockerignore @@ -6,3 +6,13 @@ docs screenshots *.md +# Never let a developer's local secrets into the build context. `COPY server/ ./` and +# `COPY web/ ./` would otherwise bake a local .env — with real Stripe keys in it — into an +# image layer, where it survives even if a later step deletes the file (DONATIONS-027). +.env +**/.env +**/.env.* +*.pem +*.key +*.p12 +*.pfx diff --git a/.github/workflows/audit.yml b/.github/workflows/audit.yml new file mode 100644 index 0000000..15d004c --- /dev/null +++ b/.github/workflows/audit.yml @@ -0,0 +1,54 @@ +# SPDX-License-Identifier: AGPL-3.0-only +# Copyright (C) 2026 OpenMasjid-Solutions + +name: Dependency audit + +# Before this existed, the ONLY workflow in the repo was the release image build — so a new advisory +# against fastify, better-sqlite3 or the Stripe SDK went unnoticed until somebody happened to run +# `npm audit` by hand. This app handles donations on unattended Raspberry Pis, so "nobody looked for +# six months" is the realistic failure mode (DONATIONS-043). +# +# Deliberately read-only and unprivileged: it reports, it never opens a PR, never pushes, never +# touches the registry, and holds no secret beyond the default read token. It cannot become a +# supply-chain path itself. +on: + schedule: + - cron: '17 6 * * 1' # Mondays, 06:17 UTC — off the hour, so not in the every-runner rush + pull_request: + paths: + - '**/package.json' + - '**/package-lock.json' + - '.github/workflows/audit.yml' + workflow_dispatch: + +permissions: + contents: read + +jobs: + audit: + runs-on: ubuntu-latest + steps: + # Pinned to a commit SHA, like every other Action in this repo (DONATIONS-004). + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + - uses: actions/setup-node@2028fbc5c25fe9cf00d9f06a71cc4710d4507903 # v6.0.0 + with: + node-version: '22' + + # `--omit=dev` first: a HIGH advisory in something that ships to a masjid box is a different + # thing from one in a build-time devDependency, and only the first should fail the run. + # `|| true` on the second so a dev-only advisory is reported but not treated as a release + # blocker — it is still visible in the log and in the summary below. + - name: Audit production dependencies (fails the run) + run: | + echo "## Server — production dependencies" >> "$GITHUB_STEP_SUMMARY" + cd server && npm audit --omit=dev --audit-level=high 2>&1 | tee -a "$GITHUB_STEP_SUMMARY" + + - name: Audit everything else (report only) + if: always() + run: | + { + echo "## Server — including devDependencies" + (cd server && npm audit || true) + echo "## Web — including devDependencies" + (cd web && npm audit || true) + } >> "$GITHUB_STEP_SUMMARY" diff --git a/.github/workflows/build-image.yml b/.github/workflows/build-image.yml index 9d51e2a..c6e95ee 100644 --- a/.github/workflows/build-image.yml +++ b/.github/workflows/build-image.yml @@ -35,8 +35,15 @@ permissions: jobs: build: runs-on: ubuntu-latest + # Every third-party Action is pinned to an immutable commit SHA, not a moving major tag + # (DONATIONS-004). This job holds `packages: write` and a GHCR credential for the image every + # masjid Raspberry Pi pulls, so an upstream owner (or anyone who compromises one of these + # repos) repointing `v6` would run their code next to a live publish token. cla.yml already + # pins its action this way; this file did not. Each SHA below is exactly what the tag it + # replaces resolved to on 2026-08-03 — nothing was upgraded, only frozen. + # To update: read the release notes, then `gh api repos///git/ref/tags/`. steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 - name: Resolve image name + version id: meta @@ -47,11 +54,11 @@ jobs: echo "owner=${GITHUB_REPOSITORY_OWNER,,}" >> "$GITHUB_OUTPUT" echo "name=$(echo "${GITHUB_REPOSITORY##*/}" | tr '[:upper:]' '[:lower:]')" >> "$GITHUB_OUTPUT" - - uses: docker/setup-qemu-action@v3 - - uses: docker/setup-buildx-action@v3 + - uses: docker/setup-qemu-action@c7c53464625b32c7a7e944ae62b3e17d2b600130 # v3.7.0 + - uses: docker/setup-buildx-action@8d2750c68a42422c14e847fe6c8ac0403b4cbd6f # v3.12.0 - name: Log in to GHCR - uses: docker/login-action@v3 + uses: docker/login-action@c94ce9fb468520275223c153574b00df6fe4bcc9 # v3.7.0 with: registry: ghcr.io username: ${{ github.actor }} @@ -61,7 +68,7 @@ jobs: # OpenMasjidDisplay template. Bump manifest.yaml `version:` for each meaningful # change so a pinned install stays stable (the catalog pins a git tag + version). - name: Build and push - uses: docker/build-push-action@v6 + uses: docker/build-push-action@10e90e3645eae34f1e60eeb005ba3a3d33f178e8 # v6.19.2 with: context: . platforms: linux/amd64,linux/arm64 diff --git a/CLAUDE.md b/CLAUDE.md index 4cd82ee..8060a1e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -177,7 +177,7 @@ resources: ## 11. Tech stack (match Display) - **TypeScript everywhere.** `strict` on, no `any` without a justifying comment. -- **`server/`** — Node 20+ + **Fastify** REST API (WebSocket only if you actually need live updates; donations probably don't). **better-sqlite3** for storage. **`stripe`** SDK. **argon2** for the fallback admin password. Validate input with **zod**. +- **`server/`** — Node 20+ + **Fastify** REST API (WebSocket only if you actually need live updates; donations probably don't). **better-sqlite3** for storage. **`stripe`** SDK. **scrypt** (Node built-in, N=2^16) for the fallback admin password — no external crypto dependency. Validate input with **zod**. - **`web/`** — **React + Vite + TypeScript + Tailwind**, **shadcn/ui** components, **Motion** for animation, **lucide-react** icons, **@stripe/react-stripe-js** for the Payment Element. One app serving the public site and the `/admin` panel. - **One container** via a multi-stage **Dockerfile** (build web, build server, final runtime serves the web build + API), exactly like Display. `docker compose up -d` runs it. - Keep it **lean and Pi-friendly**; lazy-load the admin bundle so the donor page stays light. diff --git a/docs/audit/ACTION_REQUIRED.md b/docs/audit/ACTION_REQUIRED.md new file mode 100644 index 0000000..85aa518 --- /dev/null +++ b/docs/audit/ACTION_REQUIRED.md @@ -0,0 +1,220 @@ + + + +# Action required — only you can do these + +From the 2026-08-03 security and code-health audit. Ordered by urgency. + +--- + +## 0. READ FIRST — money may have been mischarged + +### 0a. Three-decimal currencies charge one tenth (DONATIONS-001) + +**Do this before merging the money PR.** + +If any masjid running this app is configured in **BHD, JOD, KWD, OMR or TND**, every donation it has +ever taken charged **one tenth** of the amount the donor was shown, while the local ledger recorded +the full amount. The app agreed with itself, so nothing looked wrong; only the Stripe dashboard has +the true figures. + +**What I need you to do, in order:** + +1. **Find out whether this is live.** Check each installation's configured currency (Settings → + currency, or the `CURRENCY` / `MASJID_CURRENCY` env var). If none is one of those five, this is + latent — merge the fix and move on. +2. **If any masjid IS in one of those five**, before merging: export their donation history from the + **Stripe dashboard** and compare it with the app's CSV. Stripe is the truth. Expect app figures to + be 10× the real amounts. +3. **Decide what happens to the historical rows.** The fix changes only future conversions; it does + not rewrite stored amounts, so after merging, old rows (stored as 1/10 scale) and new rows (full + scale) will mean different things in the same column. Options, all yours: leave them and annotate + the period; or write a one-off migration multiplying affected rows by 10. **I have not written + that migration** — it edits historical financial records and needs someone who can confirm the + affected date range against Stripe first. +4. **Talk to the community if donors were undercharged.** A donor who gave "100 KWD" of Zakat paid + 10. That is a religious obligation they may believe is discharged. Whether and how to tell them is + not a decision code should make. + +Fix is written and tested on branch `audit/money-2026-08-03`, commit `91767c6`. Two-decimal +currencies are bit-identical — verified. + +### 0b. One-time donations have been silently lost (DONATIONS-002) + +A card payment that succeeded at Stripe while the donor's `/confirm` callback failed (closed tab, +lost signal, box briefly unreachable) was **never recorded, never receipted, and never counted**. The +row sits at `pending` for ever, indistinguishable from an abandoned checkout. + +**What I need you to do:** + +1. **Find out whether it has happened.** For each installation, compare succeeded PaymentIntents in + Stripe against the app's ledger for the same period. Any Stripe `succeeded` intent with no + corresponding donation is a lost donation. The app-side signal is a `pending` row more than a few + minutes old. +2. **Expect the ledger to grow when the fix merges.** The sweep will find those payments and add + them, backdated. Totals, the CSV, the trend chart and any Gift Aid claim will all increase. That + is correct — the money did arrive — but if a masjid has already filed accounts or a Gift Aid claim + on the old figures, they need to know before the numbers move. +3. **Expect receipt emails to go out** to donors whose payment was recovered and who never got one. + Some may be months old. If that is not wanted, disable receipts in Settings before deploying, or + ask me to gate the sweep's receipt behind an age limit. + +Fix is written and tested on `audit/money-2026-08-03`, commit `8db58af`. + +--- + +## 1. Credentials to rotate + +**None found — and that is a real result, not an absence of looking.** + +I searched all 107 commits on every branch for `sk_live`, `sk_test_51`, `pk_live_51`, `whsec_`, +`BEGIN PRIVATE KEY`, `BEGIN RSA`, `BEGIN OPENSSH`, and for any commit ever *adding* a file matching +`.env`, `*.pem`, `*.key`, `*.p12`, `*.pfx`, `*.sqlite`, `*.db`, or anything named like a dump or +backup. The only `whsec_` hits in the entire history are three commits' worth of UI labels, a form +placeholder and a validation regex. No credential has ever been committed to this repository. + +**So there is nothing to rotate from this repo's history.** The credentials that exist at runtime — +Stripe secret and webhook keys, the Cloudflare tunnel token, the OpenMasjidOS per-app secret, the +admin password hash — live only in the SQLite file on each masjid's data volume, or in the platform +vault. + +Two related items that are *not* rotations but are worth your attention: + +- **Anyone with shell access to a masjid box can read the Stripe secret key.** It is stored in the + SQLite database with `0600` perms, but the container **runs as root** (DONATIONS-015) and the app + runs as root inside it, so the file perms protect against nothing that matters. Treat host access + to a masjid box as equivalent to holding their Stripe key. +- **The Cloudflare tunnel token appears in the host process table** (DONATIONS-026), so any + unprivileged local user on the box can read it with `ps`. Fix is a one-line change to pass it via + `TUNNEL_TOKEN` in the child environment instead of argv — **I did not ship it because I cannot run + `cloudflared` here to confirm it reads that variable**, and getting it wrong silently breaks public + access. Worth doing next time someone can test a tunnel. + +--- + +## 2. The git-history decision + +**No action needed.** History is clean (see §1). No `filter-repo`, no BFG, no force-push. I would +have recommended against it anyway on a public repo with a live tag and a catalog pin. + +--- + +## 3. Cross-repo changes needed in sibling repos + +Five sibling repos talk to this one and are being audited in parallel, so I implemented only the +safe half here — validate, sanitise, log — and left the counterpart to you. + +### 3a. OpenMasjidOS — sanitise the email subject before it becomes an SMTP header +**Relates to:** DONATIONS-023 (fixed on this side, commit `84bcae6`). +This app now flattens CR/LF and exotic line separators out of the receipt subject before POSTing it +to `/api/fabric/email`. But **any** app on the Fabric can send a subject, and the platform is what +turns it into a real header. The platform must sanitise `subject` (and `to`) independently rather +than trusting callers. I could not verify whether it currently does. + +### 3b. OpenMasjidOS — confirm the ingress sanitises `X-Forwarded-*`, then let apps use it +**Relates to:** DONATIONS-009 (deferred here — see §4). +`CLAUDE.md` §13 asserts the platform ingress sanitises these headers, but **no code in this repo +reads them**, so the claim is currently untested. Behind the ingress every remote visitor shares one +rate-limit bucket, which makes a trivial donation-DoS possible. To fix it safely this app needs a +guarantee it can rely on. What I need from the platform side: a documented statement of which +forwarded headers the ingress strips and rewrites, and ideally a header an app can trust as "this +request came through the ingress" (a signed value, not a boolean anyone can send). + +### 3c. OpenMasjidAPPS — `docker-compose.yml` deviates from the catalog contract +**Relates to:** DONATIONS-049. `CLAUDE.md` §10 requires the labels `com.openmasjid.app`, +`com.openmasjid.service` and `com.openmasjid.managed`, and a platform-assigned port mapping +(`"${OMOS_HOST_PORT_8080:-7870}:8080"`). This repo's compose has **none of the three labels** and +hardcodes `"7870:8080"`. It evidently installs fine today, so either the spec or the platform's +tolerance has drifted. Someone should decide which is authoritative and align them — I did not touch +it, because compose is part of the published catalog contract. + +### 3d. OpenMasjidOS — what identity is allowed to become a Donations admin? +**Relates to:** DONATIONS-051. `GET /api/session` mints a **full local admin session** for any +identity the platform confirms; the username is recorded but never checked against a role. If +OpenMasjidOS ever gains non-admin users, every one of them silently becomes a donations +administrator with access to the Stripe keys and the donor ledger. Whether that is correct is a +platform decision. If a role or scope claim exists (or should), this app should check it. + +--- + +## 4. Decisions I need from you (deferred findings) + +### 4a. `/api/setup` during a platform outage (DONATIONS-005) — High +Under SSO the local admin is never set, so `hasAdmin()` is false for ever, and the only guard on +anonymous admin claiming is "is the platform reachable?". **During any platform outage, anyone who +can reach the box can POST `/api/setup` and own the panel** — Stripe keys, donor ledger, everything. + +`CLAUDE.md` §13 documents this as the deliberate price of never bricking the panel, so I did not +override it. Every obvious hardening breaks the escape hatch it exists for (a recovery code printed +to the container log defeats a volunteer with no shell; restricting to private IPs doesn't help +because a LAN attacker is already there; a boot-time window can be waited out). + +**My recommendation:** keep the hatch, make abuse loud. Fire a Fabric alert on every anonymous +setup claim, and show "a local password was set on ``" permanently in the panel until +dismissed. Say the word and I'll implement it. + +### 4b. Rate limiters collapse behind the ingress (DONATIONS-009) — Medium +See §3b. Needs the platform guarantee first. Symptom today: one attacker can exhaust the 30/min +donation-intent budget for **all** remote donors, and lock out every remote admin login. + +### 4c. Anonymous donations are de-anonymised (DONATIONS-024) — Medium +A donor who deliberately leaves name and email blank gets them **backfilled from Stripe's billing +details** at confirm, so the cardholder name ends up in the ledger and the CSV. Is a blank name +"I wish to be anonymous" or "I couldn't be bothered"? The masjid wants names for Gift Aid; the donor +may have meant it. I won't guess — tell me which and I'll make it consistent everywhere. + +### 4d. Container runs as root (DONATIONS-015) — Medium +Needs `USER node` **plus** an entrypoint that chowns `/data`, and **one real container start to +prove the app can still write its database**. I have no Docker here. Shipping it unverified risks +bricking every install on update. `docker-compose.yml`'s own comment already says this is pending +CI validation. + +### 4e. Base images pinned by tag, not digest (DONATIONS-016) — Medium +Same reason: a wrong digest fails the build, and the build workflow doesn't run on pull requests, so +a mistake wouldn't surface until after merge. The change itself is mechanical once someone can run +`docker build`. + +### 4f. No Content-Security-Policy (part of DONATIONS-014) — Medium +I shipped `nosniff` and `no-referrer`, and deliberately **not** a CSP: Stripe's Payment Element +loads `js.stripe.com` and its own frames, and a CSP that is even slightly wrong stops donors paying +with no obvious error. It needs writing against a real Stripe Element in a browser. Highest-value +missing header on the admin panel, and worth doing properly. + +### 4g. `@fastify/static` major upgrade (DONATIONS-040) — Low +Four High advisories, all **refuted as exploitable in this configuration** (both registrations use +`index: false`, never `list: true`, and both roots hold only already-public assets). The fix is +8.3.0 → 10.1.2, a major bump across two majors. Left for a human on its own schedule. + +### 4h. Session revocation and password change (DONATIONS-013) — Medium +There is no way to change the admin password and no way to invalidate a stolen 30-day cookie short +of deleting the database. Needs a token-version scheme (bump a counter in the store, include it in +the token, check it on verify) plus a change-password route. Straightforward, but it is an auth-model +change with no existing auth-route tests, so it wants a human eye. + +--- + +## 5. Assumptions I made + +State these back to me if any is wrong — several fixes rest on them. + +1. **Plain-HTTP LAN installs are still supported.** The whole design of the cookie `Secure` fix + (DONATIONS-012) is "follow the request scheme" rather than "always Secure", specifically so a + masjid on `http://box.local:7870` is not locked out. If HTTPS is now mandatory everywhere, the + simpler fix is `COOKIE_SECURE: "1"` in compose. +2. **`x-forwarded-proto` may be read for the cookie flag** even though `trustProxy` is off, because + a spoofed value can only restrict the spoofer's own cookie. I am confident in this reasoning, but + it is reasoning, not a test. +3. **Cloudflare may cache a `.csv` response** with no cache directives, since `.csv` is in its + default cached-extension list. The fix (`no-store`) is correct regardless, so this assumption + doesn't need to hold — but it is why I rated DONATIONS-003 High rather than Low. +4. **Stripe's three-decimal set is exactly BHD, JOD, KWD, OMR, TND**, and those require a + multiple-of-10 minor amount. From Stripe's documented currency rules; I could not call the API to + confirm. +5. **A one-minor-unit fixed fee is better than zero** for zero-decimal currencies (DONATIONS-008). + It is an approximation, not an FX conversion — the honest fix is an admin-visible per-account fee + model, which is a product decision. +6. **`restart: unless-stopped` is in effect**, which is why `uncaughtException` now exits rather + than limping on (DONATIONS-029). +7. **Nobody depends on `:latest`.** The catalog pins a commit and the compose pins a digest, so my + reading is that republishing `:latest` on a `main` push is untidy rather than dangerous. If + anything does track `:latest`, the push veto matters even more than I've described. diff --git a/docs/audit/REMEDIATION.md b/docs/audit/REMEDIATION.md new file mode 100644 index 0000000..86b0373 --- /dev/null +++ b/docs/audit/REMEDIATION.md @@ -0,0 +1,275 @@ + + + +# Remediation — what changed, and how it was verified + +Audit of 2026-08-03. Baseline `6fc4ca272cf412b8d04eeaf3eddce752b072c8b7` (tag +`pre-audit-2026-08-03`). + +**Nothing was pushed to `main`.** A push to `main` triggers +[`build-image.yml`](../../.github/workflows/build-image.yml), which republishes +`ghcr.io/openmasjid-solutions/openmasjiddonations:0.38.0` **and `:latest`** — the live production tag +that the App Store catalog's digest pin resolves to. That is a published artifact, so autonomous push +was disabled per the audit mandate. Everything is on two branches, delivered as two pull requests. + +| Branch | Contains | Mergeable? | +|---|---|---| +| `audit/security-2026-08-03` | 13 fixes + the audit report | **Yes** — ordinary review | +| `audit/money-2026-08-03` | 3 money-correctness fixes (branched off the above) | **Not until `ACTION_REQUIRED.md` §0 is done** | + +--- + +## Read this first — the Tier 2 changes + +These ship behaviour changes. If something feels wrong in the next few days, look here. + +1. **The admin session cookie is now `Secure` on HTTPS** (`70d5457`, DONATIONS-012). It follows the + request scheme, so a plain-HTTP LAN install is unaffected — that was the whole design constraint, + because always-`Secure` would lock a masjid out of its own panel. If an admin on an HTTPS + deployment reports being unable to stay signed in, this is the change to revert first. +2. **Session tokens now carry the admin's username** (`87033d3`) so the audit log can say who acted. + Additive: a token minted before this still verifies. Cookies are unchanged in every other respect. +3. **New `audit_log` table** (`87033d3`). Additive `CREATE TABLE IF NOT EXISTS`, no data migration, + no existing column touched. Reverse migration below. +4. **New global `onSend` hook** setting two headers on every response (`8137bef`). `nosniff` could in + principle break a client relying on content sniffing; nothing in this app does. +5. **Three new rate limits** (`23a4a30`) on `/api/session`'s SSO branch (120/min), the Stripe webhook + (300/min) and monthly intents (5/min per peer). All well above real traffic, but they are new 429s + that did not exist before. Note the known limitation: behind the platform ingress every remote + visitor shares one bucket (DONATIONS-009, deferred). +6. **`unhandledRejection` no longer kills the process** (`2924f79`). Previously Node's default + terminated it; now it logs at error level and keeps serving. `uncaughtException` still exits so + the container restarts clean. +7. **Dependency bumps** (`73cc072`): `fast-uri` 3.1.4→3.1.5, `find-my-way`→9.6.1, `brace-expansion` + →5.0.9 (server, all transitive under fastify), and `postcss`→8.5.23 (web, devDependency). No major + upgrades. Server advisories 4 High → 1 High; web 1 High → **0**. +8. **The outboxes no longer overlap themselves** (`decfaab`). A pass slower than its 60s interval used + to start a second pass over the same rows; now the tick is skipped. Skipping loses nothing — the + rows are still pending next tick. + +And on the money branch, both of which change what donors are charged or what is recorded: + +9. **Three-decimal currencies now charge the intended amount** (`91767c6`, DONATIONS-001). +10. **A sweep now recovers one-time donations Stripe took but we never recorded** (`8db58af`, + DONATIONS-002). On first run this adds donations and sends backdated receipts. + +--- + +## Shipped fixes + +### Branch `audit/security-2026-08-03` + +| Commit | Finding | What changed | Why it works | +|---|---|---|---| +| `249e8e5` | DONATIONS-003 (High) | `cache-control: no-store, private`, `pragma: no-cache`, `vary: cookie` on `/api/admin/donations` and `donations.csv` | The response body is every donor's name and email. `.csv` is in Cloudflare's default cached-extension list, and a static-extension response with no cache directives is an edge-cache candidate — after which the cached copy can be served to a request with no session cookie. `no-store` removes it from every cache, browser and proxy alike, so the fix does not depend on knowing the masjid's Cloudflare config. | +| `8137bef` | DONATIONS-014, -030 | Global `onSend`: `x-content-type-options: nosniff`, `referrer-policy: no-referrer` | An upload's content type comes from the client-declared multipart header, so a file claiming `image/png` can hold anything, and it is served from our own origin — `nosniff` is what stops a sniffing browser executing it as script. `no-referrer` stops an unlisted campaign's token (it appears in the path) leaking to any site an admin or donor clicks through to. CSP deliberately excluded: it would break Stripe Elements. | +| `23a4a30` | DONATIONS-019, -020, -010 | Per-peer limiters on the two unauthenticated platform-probe routes, the webhook, and monthly intents | The first two make an outbound call to the OpenMasjidOS core on *every* request, so without a cap the box is an unmetered amplifier against the platform. The monthly limit is separate and tighter (5/min) because a monthly intent creates **five** persistent Stripe objects (Customer, Price, Subscription, Invoice, PaymentIntent) where a card payment creates one — it is checked after validation but *before* anything is created at Stripe. | +| `41f8b44` | DONATIONS-017 | `LoginLimiter` sweep rewritten with a `seen` timestamp | The old condition `lockedUntil < now - 1h && fails === 0` was **unsatisfiable**: an entry only exists after `fail()`, which always sets `fails >= 1`, and `succeed()` deletes it. So nothing was ever swept and the map grew one entry per attacking IP for the process lifetime. The new sweep also requires `lockedUntil <= now`, so it can never release a live lockout — which would have been a rate-limit bypass introduced by the fix. | +| `64f037d` | DONATIONS-018, -021 | `refresh=1` now requires the same-origin check; `requestTimeout: 120_000` | Gating only the *write* side left the amplification the guard existed to stop: forcing the cache open is what turns one cross-site navigation into up to 200 outbound Stripe calls. `connectionTimeout` was deliberately **not** set — it maps to Node's socket-inactivity timeout and would reap idle keep-alive sockets Fastify holds by design, buying TCP churn for no security. | +| `84bcae6` | DONATIONS-023 | Receipt subject flattened (`\r \n U+2028 U+2029 U+0085 \v \f \0`) before the length cap | The subject is the admin's template with the **donor's own name** substituted in, and the donor is an unauthenticated stranger. The finished subject becomes an SMTP header at the platform, so CR/LF in a name is header injection (`Bcc:`, a forged `From:`). Flattened before the 200-char slice so the cut can never land mid-escape. Platform-side counterpart in `ACTION_REQUIRED.md` §3a. | +| `70d5457` | DONATIONS-012 | `secureForRequest(req)`; cookie `Secure` when the request arrived over TLS | Nothing ever set `COOKIE_SECURE`, so the cookie was never `Secure` — including in the normal deployment, where the manifest declares `https: true`. Always-`Secure` was not an option (it locks out plain-HTTP LAN admins), so it follows the actual scheme. Reading `x-forwarded-proto` is safe here specifically because a spoofed value can only add `Secure` to the response to *that same request* — it can only restrict the spoofer's own cookie. | +| `87033d3` | DONATIONS-011 | `audit_log` table + writes on donor export, plan pause/resume/stop/schedule, Stripe account create/update/delete, campaign delete | There was no answer to "who exported the donor list / cancelled that plan / rotated the key". Records field *names* on a key change, never values. `recordAudit` never throws — an audit write must not be able to fail the action it describes. | +| `ad80f17` | DONATIONS-044 | New `stripe.test.ts` — 17 tests over every money conversion | `stripe.ts` had **zero** tests, and it converts every amount the product charges. Two tests deliberately pinned the *wrong* behaviour so it was visible and would fail loudly when fixed — which is exactly what happened on the money branch. | +| `73cc072` | -041, -042, -027, -028 | `npm audit fix` (non-major only); `.env`/`*.pem`/`*.key` added to `.dockerignore`; data dir chmod `0700` | `COPY server/ ./` would have baked a developer's local `.env` — with real Stripe keys — into an image layer. On the perms: SQLite creates `-wal`/`-shm` sidecars lazily, and in WAL mode the newest committed data (a freshly saved Stripe key) is in the `-wal` file, not the `0600` database; chmod'ing sidecars is a race, so `0700` on the directory covers every present and future file. | +| `73086c5` | DONATIONS-004 | All five Actions in the publishing job pinned to commit SHAs | The job holds `packages: write` and a GHCR credential for the image every masjid Pi pulls. An upstream owner repointing `v6` would run their code beside a live publish token. Each SHA is exactly what the tag resolved to on 2026-08-03 — **frozen, not upgraded** — verified via `gh api repos//git/ref/tags/`. `cla.yml` already did this; this file did not. | +| `2924f79` | DONATIONS-043, -029 | New read-only weekly `audit.yml`; `unhandledRejection`/`uncaughtException` handlers | The only workflow was the release build, so a new advisory went unnoticed until someone ran `npm audit` by hand. The new workflow holds no secrets, opens no PR, pushes nothing. The fault handlers matter because the codebase uses fire-and-forget `void fn().catch()` widely and Node's default is to kill the process — one missed `.catch()` in an alert path would take the donation page down. | +| `decfaab` | DONATIONS-052, -046 | `nonOverlapping()` wrapper on both outbox timers; `CLAUDE.md` argon2→scrypt | Each outbox item makes a network call with an 8s timeout, so 8 pending receipts outlast the 60s interval; a second pass then read the same rows and sent again. Also corrected the docs: §11 specified argon2, the implementation has always used scrypt (which is fine — the *docs* were wrong). | + +### Branch `audit/money-2026-08-03` — **do not merge yet** + +| Commit | Finding | What changed | Why it works | +|---|---|---|---| +| `91767c6` | DONATIONS-001, -008 (High) | `THREE_DECIMAL` set added to `currencyDecimals`; three-decimal amounts rounded to the nearest 10; fixed fee floored at 1 minor unit for zero-decimal currencies | Stripe quotes BHD/JOD/KWD/OMR/TND in thousandths and requires a multiple of 10. Treating them as two-decimal sent 1/10 of the shown amount while the ledger recorded the full figure — self-consistent, so nothing ever disagreed except Stripe. Verified: all five now charge the intended amount, and GBP/USD/EUR/JPY/KRW are **bit-identical**. | +| `8db58af` | DONATIONS-002 (High) | `listUnconfirmedDonations()` + a 10-minute sweep that retrieves pending one-time intents from Stripe | A one-time payment is marked succeeded only by the donor's own `/confirm` callback, so a closed tab left money taken and nothing recorded, for ever. Conservative by design: only ever promotes `pending → succeeded` (never writes `failed` from a transient read); a 5-minute age floor so it cannot race the donor's own confirm and double-send a receipt; a 30-day ceiling; bounded to 25 rows; stops on the first unreachable account. | + +--- + +## Verification + +Per-commit: typecheck + full suite after **every** commit, not at the end. + +### Before (baseline, `6fc4ca27`) + +``` +> tsc -p tsconfig.json --noEmit (clean) +ℹ tests 130 +ℹ pass 130 +ℹ fail 0 +web: ✓ built in 2.85s (tsc --noEmit + vite, clean) +server npm audit: 4 high web npm audit: 1 high +``` + +### After (`audit/money-2026-08-03`, all fixes) + +``` +> tsc -p tsconfig.json --noEmit (clean) +> tsc -p tsconfig.json (build clean) +ℹ tests 179 +ℹ suites 0 +ℹ pass 179 +ℹ fail 0 +ℹ cancelled 0 +ℹ skipped 0 +ℹ todo 0 + +web: ✓ built in 2.44s + dist/assets/donate-BabYbuUK.js 41.51 kB │ gzip: 12.62 kB (donor bundle unchanged) + dist/assets/admin-Dg9qyaLR.js 115.48 kB │ gzip: 31.43 kB + +server npm audit: 1 high (@fastify/static — major bump, refuted as exploitable, deferred) +web npm audit: found 0 vulnerabilities +``` + +**130 → 179 tests. 49 added, 0 failures, no test removed or weakened.** Four new test files: +`auth.test.ts` (10), `rateLimit.test.ts` (5), `stripe.test.ts` (19), plus 15 added to `store.test.ts`. + +### Regression tests proven to fail before the fix + +Not "tests pass" — each was run against the pre-fix code and observed failing. + +**DONATIONS-017** (login limiter sweep) — restored the pre-fix `rateLimit.ts` with minimal shims so +the new tests could execute against the *old* sweep condition: + +``` +ℹ pass 3 +ℹ fail 2 ← the two sweep tests +``` +and after restoring the fix: `ℹ pass 5 ℹ fail 0`. + +**DONATIONS-023** (email header injection) — restored the pre-fix `email.ts`: + +``` +ℹ tests 13 +ℹ pass 12 +ℹ fail 1 ← 'a donor name cannot inject an email header' +``` +and after restoring: `ℹ pass 13 ℹ fail 0`. + +**DONATIONS-001 / -008** — the pins in `stripe.test.ts` fired exactly as designed when the arithmetic +changed on the money branch: + +``` +✖ currencyDecimals: DONATIONS-001 — three-decimal currencies are WRONG today (charges 1/10) +✖ withCoveredFees: DONATIONS-008 — the fixed fee VANISHES for zero-decimal currencies +ℹ pass 15 ℹ fail 2 +``` +Both were then rewritten to assert the correct arithmetic. + +### End-to-end money verification (DONATIONS-001, -008) + +Run against the real `stripe.ts` exports after the fix: + +``` +=== DONATIONS-001: what a 10-unit donation now charges === +BHD: minor=10000 -> Stripe reads 10.000 BHD (intended 10.000) multipleOf10=true +JOD: minor=10000 -> Stripe reads 10.000 JOD (intended 10.000) multipleOf10=true +KWD: minor=10000 -> Stripe reads 10.000 KWD (intended 10.000) multipleOf10=true +OMR: minor=10000 -> Stripe reads 10.000 OMR (intended 10.000) multipleOf10=true +TND: minor=10000 -> Stripe reads 10.000 TND (intended 10.000) multipleOf10=true +=== unchanged currencies (regression check) === +GBP: decimals=2 toMinor(10)=1000 roundTrip=10 +USD: decimals=2 toMinor(10)=1000 roundTrip=10 +EUR: decimals=2 toMinor(10)=1000 roundTrip=10 +JPY: decimals=0 toMinor(10)=10 roundTrip=10 +KRW: decimals=0 toMinor(10)=10 roundTrip=10 +=== DONATIONS-008: covered-fee gross-up on a 10-unit donation === +GBP: net=1000 gross=1061 legal=true +USD: net=1000 gross=1061 legal=true +JPY: net=10 gross=11 legal=true +KWD: net=10000 gross=10610 legal=true +``` + +The same script against the **pre-fix** code produced `factor 0.10x` for all five three-decimal +currencies and `toMinor(0.30,'JPY') = 0`. + +### Two defects my own tests caught in my own fixes + +Recorded because they are the argument for writing the tests at all: + +1. **`listAudit()` ordered by `at DESC, id DESC`.** Two actions in the same millisecond share `at`, + and `id` is random hex — so "newest first" was arbitrary. Caught by + `audit log: records an action and reads it back newest-first`, fixed to `ORDER BY rowid DESC` + (insertion order, also immune to the clock stepping backwards, which an unattended Pi syncing NTP + after an outage really does). +2. **`connectionTimeout: 10_000`** in my first cut of DONATIONS-021 would have reaped idle keep-alive + sockets Fastify holds for 72s by design. Caught on re-reading my own diff; removed before commit, + `requestTimeout` alone does the job. + +### Not verified — deferred rather than shipped + +Per the mandate, an unshipped fix costs nothing; a shipped fix that is silently wrong does damage. + +- **DONATIONS-015** (non-root container) — no Docker here; cannot prove the app can still write + `/data`. Shipping blind risks bricking every install on update. +- **DONATIONS-016** (digest-pin base images) — a wrong digest fails the build, and the build workflow + does not run on pull requests. +- **DONATIONS-026** (tunnel token via `TUNNEL_TOKEN`) — cannot run `cloudflared` to confirm it reads + the variable; a mistake silently breaks public access. +- **The Action SHA pins (`73086c5`) were verified by API, not by execution** — the workflow only runs + on `main`, `v*` tags and `workflow_dispatch`, so the first real run is post-merge. + **Recommended: `workflow_dispatch` it once on the branch before merging.** + +--- + +## Rollback + +### Revert one fix + +Each commit is self-contained and individually revertable: + +```bash +git revert 249e8e5 # DONATIONS-003 donor-data cache headers +git revert 8137bef # DONATIONS-014 nosniff + no-referrer +git revert 23a4a30 # DONATIONS-019/-020/-010 rate limits +git revert 41f8b44 # DONATIONS-017 login limiter sweep +git revert 64f037d # DONATIONS-018/-021 refresh gate + requestTimeout +git revert 84bcae6 # DONATIONS-023 email header injection +git revert 70d5457 # DONATIONS-012 cookie Secure +git revert 87033d3 # DONATIONS-011 audit log +git revert ad80f17 # DONATIONS-044 money conversion tests +git revert 73cc072 # DONATIONS-041/-042/-027/-028 deps, dockerignore, perms +git revert 73086c5 # DONATIONS-004 Action SHA pins +git revert 2924f79 # DONATIONS-043/-029 audit workflow + fault handlers +git revert decfaab # DONATIONS-052/-046 outbox overlap + docs + +# money branch +git revert 8db58af # DONATIONS-002 lost-donation sweep +git revert 91767c6 # DONATIONS-001/-008 currency arithmetic +``` + +Two ordering notes: revert `87033d3` (audit log) **before** `70d5457` (cookie) if you revert both, +since the audit log reads the username the cookie commit's `makeToken` writes. And `8db58af` uses +`nonOverlapping()` from `decfaab`, so don't revert `decfaab` alone while the money branch is merged. + +### Reverse the one schema change + +`audit_log` is additive — no existing table or column was touched, so nothing needs migrating back. +To remove it entirely (after reverting `87033d3`): + +```sql +-- against the masjid's data volume: /data/donations.db +DROP INDEX IF EXISTS idx_audit_log_at; +DROP TABLE IF EXISTS audit_log; +``` + +Leaving the table in place with the code reverted is also safe: nothing reads it, and +`CREATE TABLE IF NOT EXISTS` makes re-applying idempotent. + +### Revert the entire run + +Nothing reached `main`, so the run is undone by not merging. To discard the branches: + +```bash +git checkout main # already at pre-audit state +git branch -D audit/security-2026-08-03 audit/money-2026-08-03 +git push origin --delete audit/security-2026-08-03 audit/money-2026-08-03 +``` + +And if either branch *has* been merged, to return `main` to the pre-audit commit: + +```bash +git revert --no-commit pre-audit-2026-08-03..HEAD && git commit -m "revert: back out the 2026-08-03 audit" +``` + +A revert, never a reset — `main` is protected, force-pushing is disabled, and history must not be +rewritten. `pre-audit-2026-08-03` = `6fc4ca272cf412b8d04eeaf3eddce752b072c8b7`. diff --git a/docs/audit/SECURITY_AUDIT.md b/docs/audit/SECURITY_AUDIT.md new file mode 100644 index 0000000..7034b73 --- /dev/null +++ b/docs/audit/SECURITY_AUDIT.md @@ -0,0 +1,483 @@ + + + +# Security & code-health audit — OpenMasjidDonations + +**Date:** 2026-08-03 +**Commit audited:** `6fc4ca272cf412b8d04eeaf3eddce752b072c8b7` (tag `v0.38.0` + digest pin), branch `main` +**Rollback point:** tag `pre-audit-2026-08-03` +**Method:** 10 parallel read-only auditors (one per audit phase, plus a payments-specific pass), +then an adversarial verifier per Critical/High finding whose job was to *refute* it. 97 raw +findings → 52 unique after dedupe. Three High findings were refuted and are recorded as such. +Every finding below was re-read against the source by the author of this report. + +--- + +## Executive summary + +**Posture: good, and better than most self-hosted payment apps — with one class of exception +that matters more than any of the security findings.** + +The security fundamentals here are genuinely solid, and I want to say that plainly before the +findings, because the findings list is long and would otherwise mislead. Specifically: + +- **No credential has ever been committed.** 107 commits, no `.env`, `.pem`, `.key`, DB file or + dump ever added in any commit on any branch. The only `whsec_` strings in all of history are UI + labels, a placeholder and a validation regex. +- **Every one of the 33 `/api/admin/*` and `/api/settings*` routes carries `preHandler: + requireAdmin`.** No route is unauthenticated by accident. I enumerated all 56 routes. +- **No SQL injection is possible.** Every query is a `better-sqlite3` prepared statement with + bound parameters. The one dynamic-identifier site (`PRAGMA table_info(${table})`) takes only + hard-coded literals. +- **No XSS.** There is no `dangerouslySetInnerHTML`, no `innerHTML`, no `eval` anywhere in + `web/`. Campaign rich text renders as a React text child, so it is escaped by construction. +- **The Stripe secret key never reaches the browser**, and card data never touches this server + (PCI SAQ-A holds). +- **Upload handling is correct**: server-generated filename, extension from a MIME allowlist, no + SVG, size cap, truncation check. No path traversal. +- The brute-force and tuition-lookup limiters correctly key on the real TCP peer rather than a + spoofable `X-Forwarded-For`, and `trustProxy` is off. + +**The single most important issue is not a vulnerability — it is arithmetic.** +[`DONATIONS-001`](#donations-001): Stripe has five **three-decimal** currencies (BHD, JOD, KWD, +OMR, TND). This app treats every non-zero-decimal currency as two-decimal, so a masjid configured +in one of those five charges **one tenth** of the amount the donor is shown, while recording the +full amount locally. A donor who believes they have paid 100 KWD of Zakat has paid 10. It is +latent — it needs one of those five currencies to be configured — but the configuration path is a +documented install setting, and nothing warns. + +Second most important, and in the same family: [`DONATIONS-002`](#donations-002) — a card charge +that succeeds at Stripe while the browser's `/confirm` round-trip fails is **never recorded, never +receipted, and invisible to the masjid forever**. There is no reconciliation sweep for one-time +payments (monthly plans got one in v0.38.0; one-time payments did not). + +Neither of those may be fixed autonomously. Both are written, tested and left in a separate PR. + +**Nothing shipped to `main` in this run.** See "Deployment veto" below. + +--- + +## Deployment veto (why nothing was pushed to `main`) + +[`.github/workflows/build-image.yml:15-18`](../../.github/workflows/build-image.yml#L15-L18) runs +on `push: branches: [main]` and performs `push: true` to GHCR, tagging the image +`:${manifest version}` **and `:latest`**: + +```yaml +on: + push: + branches: [main] + tags: ['v*'] +``` + +`manifest.yaml` currently reads `version: 0.38.0`, so **any code commit on `main` republishes the +live production tag** `ghcr.io/openmasjid-solutions/openmasjiddonations:0.38.0`, moving it off the +digest `sha256:62165d3f…` that the App Store catalog pins and every installed masjid box resolved. +Existing installs are protected by the digest pin; the published tag would then disagree with its +own digest, and `:latest` moves for anyone tracking it. + +That is a published artifact. Per the audit mandate, **autonomous push was disabled** and all work +is delivered as pull requests off `main`. + +--- + +## Phase 0 — What this is, and who attacks it + +**What.** A self-hosted donation website for a single masjid, distributed as one Docker container +through the OpenMasjidOS App Store. `server/` is Node 20 + Fastify + better-sqlite3 + the Stripe +SDK (17.7, API `2025-02-24.acacia`) + zod. `web/` is React 18 + Vite, one bundle serving both the +public donation site and the `/admin` panel. State lives in SQLite plus uploaded images on a +volume at `/data`. + +**Runs on.** Usually a Raspberry Pi on the masjid's own LAN, unattended for months, often behind +no reverse proxy at all. Optionally published to the internet through a Cloudflare Tunnel the +admin configures in-app, or through the OpenMasjidOS ingress on a shared hostname with a path +prefix. `restart: unless-stopped`, `cap_drop: ALL`, `no-new-privileges`, but **root inside the +container with a writable root filesystem**. + +**Who uses it.** Donors (anonymous public, no account, often on a kiosk or via a QR code on the +masjid's guest wifi); one masjid admin — a volunteer, not an engineer; parents paying school fees +through the tuition campaign type; and the OpenMasjidOS platform itself, server-to-server. + +**Entry points.** 56 HTTP routes. Unauthenticated by design: `/healthz`, `/api/app`, +`/api/public/appearance`, `/api/session`, `/api/setup`, `/api/login`, `/api/logout`, the six +`/api/public/campaign/*` routes (campaign read, intent, confirm), the four +`/api/public/campaign/:slug/students/*` tuition routes, `/api/stripe/webhook/:accountId`, the +widget at `/w/:slug`, and static assets. Everything else is behind `requireAdmin`. Non-HTTP entry +points: the Stripe webhook (signed), the OpenMasjidOS Fabric (server-to-server, app-secret +header), a supervised `cloudflared` child process, and three in-process outbox timers. + +**Trust boundaries.** untrusted → trusted crossings are: (1) the public donation body → a Stripe +amount; (2) the Stripe webhook body → the donations ledger; (3) the tuition Student ID → the +Students provider over the Fabric broker; (4) the multipart upload → the data volume; (5) the +admin's browser → every `/api/admin/*` route, gated only by a signed cookie. + +**Sensitive data.** Donor name + email; card brand + last 4 (no PAN); Gift Aid declarations +(name + home address + a taxpayer claim); a **child's** first name, last initial and school +balance in the tuition flow; the Stripe **secret** key and webhook secret; the Cloudflare tunnel +token; the OpenMasjidOS per-app secret; the admin password hash. + +**Threat model — who realistically attacks this, for what.** + +1. **Someone on the masjid's LAN or guest wifi** (the documented kiosk/QR surface) — the most + realistic attacker by far. Wants: the admin panel (→ Stripe keys → redirect donations), or the + donor ledger. Unauthenticated reach to every public route and to the LAN-facing admin login. +2. **The open internet**, when the admin turns on public access. Same goals plus payment fraud + and card testing against the intent endpoints. +3. **A malicious or compromised upstream** — a GitHub Action tag, an npm package, a base image. + Wants: a backdoored image on every masjid box. This is the highest-leverage path and the one + the repo defends least consistently. +4. **A curious or careless insider** — a second volunteer with panel access. Wants nothing; + causes harm by exporting donor data or cancelling plans, with no record that it happened. +5. **Not in the model:** a nation-state, physical theft of the Pi (assume game over), and the + masjid admin themselves as an attacker (they own the box). + +--- + +## Findings + +Severity is rated by **actual impact in this system**, not by category name. `T` = fix tier +(1 = shippable unreviewed, 2 = shippable but flagged, 3 = never autonomous). + +**52 findings: 0 Critical · 4 High · 23 Medium · 19 Low · 6 Info.** One auditor rated +DONATIONS-002 Critical; the adversarial verifier downgraded it to High because it needs a failed +callback rather than an attacker, and I agreed. Nothing here is rated Critical, and I would rather +say that plainly than inflate the top of the table. Machine-readable copy in +[`findings.json`](findings.json). + +| ID | Title | Sev | Conf | T | File:line | Status | +|---|---|---|---|---|---|---| +| DONATIONS-001 | Three-decimal currencies charged at 1/10 of the displayed amount | High | Confirmed | 3 | `server/src/stripe.ts:80` | **PR (money)** | +| DONATIONS-002 | A succeeded charge is never recorded when `/confirm` doesn't land | High | Confirmed | 3 | `server/src/index.ts:1618` | **PR (money)** | +| DONATIONS-003 | Donor-PII CSV export has no `Cache-Control` at a `.csv` URL Cloudflare caches | High | Confirmed | 1 | `server/src/index.ts:836` | **Fixed** | +| DONATIONS-004 | Image-publishing job runs unpinned third-party Actions while holding GHCR write | Med | Likely | 1 | `.github/workflows/build-image.yml:39` | **Fixed** | +| DONATIONS-005 | Unauthenticated admin takeover via `/api/setup` during a platform outage | High | Confirmed | 2 | `server/src/index.ts:261` | Deferred — ask | +| DONATIONS-006 | No refund or chargeback handling: local totals permanently overstate income | Med | Confirmed | 3 | `server/src/index.ts:2044` | **PR (money)** | +| DONATIONS-007 | Changing the masjid currency rescales every stored amount and misreports history | Med | Confirmed | 3 | `server/src/index.ts:320` | Report only | +| DONATIONS-008 | Cover-the-fees drops the fixed fee for zero-decimal currencies; hardcoded US rate | Med | Confirmed | 3 | `server/src/stripe.ts:109` | **PR (money)** | +| DONATIONS-009 | All rate limiters collapse to one shared bucket behind the OS ingress | Med | Confirmed | 2 | `server/src/index.ts:277` | Deferred — ask | +| DONATIONS-010 | Unauthenticated `/intent` creates real Stripe objects with no monthly-specific limit | Med | Confirmed | 1 | `server/src/index.ts:1576` | **Fixed** (limit) | +| DONATIONS-011 | No audit log for any admin financial or donor-data action | Med | Confirmed | 2 | `server/src/index.ts:826` | **Fixed** | +| DONATIONS-012 | Admin session cookie never gets `Secure`, including on HTTPS deployments | Med | Confirmed | 2 | `server/src/auth.ts:83` | **Fixed** | +| DONATIONS-013 | No session revocation, no password-change route, 30-day cookie | Med | Confirmed | 2 | `server/src/auth.ts:14` | Deferred | +| DONATIONS-014 | No security response headers at all (no nosniff, Referrer-Policy, CSP) | Med | Confirmed | 2 | `server/src/index.ts:2125` | **Partly fixed** | +| DONATIONS-015 | Container runs as root with a writable root filesystem | Med | Confirmed | 2 | `Dockerfile:33` | Deferred — unverifiable | +| DONATIONS-016 | Base images pinned by mutable tag, not digest | Med | Confirmed | 1 | `Dockerfile:12` | Deferred — unverifiable | +| DONATIONS-017 | `LoginLimiter`'s sweep condition is unreachable, so the map never shrinks | Med | Confirmed | 1 | `server/src/rateLimit.ts:24` | **Fixed** | +| DONATIONS-018 | `?refresh=1` bypasses the same-origin guard on the plans sync | Med | Confirmed | 1 | `server/src/index.ts:1080` | **Fixed** | +| DONATIONS-019 | No rate limit on the two unauthenticated routes that call the platform outbound | Med | Confirmed | 1 | `server/src/index.ts:228` | **Fixed** | +| DONATIONS-020 | Stripe webhook route is unauthenticated with no rate limit (§9 requires one) | Low | Confirmed | 1 | `server/src/index.ts:2035` | **Fixed** | +| DONATIONS-021 | No `requestTimeout` bound on how long a request holds a socket | Low | Confirmed | 1 | `server/src/index.ts:117` | **Fixed** | +| DONATIONS-022 | Donations log and CSV materialise the whole table with no pagination | Med | Confirmed | 2 | `server/src/store.ts:1022` | Deferred | +| DONATIONS-023 | Donor name reaches the receipt Subject with CR/LF intact | Low | Confirmed | 1 | `server/src/email.ts:83` | **Fixed** | +| DONATIONS-024 | Anonymous donations are de-anonymised by the Stripe billing-name backfill | Med | Likely | 2 | `server/src/index.ts:1639` | Deferred — ask | +| DONATIONS-025 | No retention limit, deletion path or subject-access export for donor records | Med | Confirmed | 3 | `server/src/store.ts:897` | Report only | +| DONATIONS-026 | Cloudflare tunnel token passed in argv, visible in the host process table | Low | Confirmed | 1 | `server/src/tunnel.ts:99` | Deferred — unverifiable | +| DONATIONS-027 | `.dockerignore` does not exclude `.env` | Low | Likely | 1 | `.dockerignore:1` | **Fixed** | +| DONATIONS-028 | SQLite `-wal`/`-shm` sidecars escape the 0600 chmod | Low | Confirmed | 1 | `server/src/store.ts:362` | **Fixed** | +| DONATIONS-029 | No `unhandledRejection` / `uncaughtException` handler | Low | Confirmed | 2 | `server/src/index.ts:2188` | **Fixed** | +| DONATIONS-030 | Upload trusts the client-declared MIME type with no `nosniff` backstop | Low | Confirmed | 1 | `server/src/index.ts:575` | **Fixed** (via 014) | +| DONATIONS-031 | Duplicate receipt + duplicate alert on concurrent `/confirm` | Low | Likely | 3 | `server/src/index.ts:1636` | **PR (money)** | +| DONATIONS-032 | `invoice.paid` reads Invoice fields Stripe removed in `2025-03-31.basil` | Med | Likely | 3 | `server/src/index.ts:2060` | Report only | +| DONATIONS-033 | Gift Aid is dead plumbing, and the flag is client-settable with no declaration | Low | Confirmed | 3 | `server/src/index.ts:1542` | Report only | +| DONATIONS-034 | Idempotency keys are freshly random per request, so they dedupe nothing | Low | Confirmed | 3 | `server/src/index.ts:1554` | Report only | +| DONATIONS-035 | The 99,999,999 ceiling is validated pre-fee, so the gross-up can exceed it | Info | Confirmed | 3 | `server/src/index.ts:1535` | Report only | +| DONATIONS-036 | The per-currency minimum charge is a stub — both ternary branches are 50 | Info | Confirmed | 3 | `server/src/index.ts:1532` | Report only | +| DONATIONS-037 | The monthly confirm dialog states the pre-fee amount as the recurring charge | Low | Confirmed | 3 | `web/src/donate.tsx:322` | Report only | +| DONATIONS-038 | Months are grouped in UTC but windowed in local time; `MASJID_TIMEZONE` unused | Med | Confirmed | 3 | `server/src/store.ts:1075` | Report only | +| DONATIONS-039 | Historical amounts are formatted with the *current* currency's decimals | Med | Confirmed | 3 | `server/src/index.ts:821` | Report only | +| DONATIONS-040 | `@fastify/static` 8.3.0 carries four High advisories — **not exploitable here** | Low | Confirmed | 2 | `server/package.json:17` | Report only | +| DONATIONS-041 | Three transitive High advisories, all unreachable, non-major fixes available | Low | Confirmed | 1 | `server/package.json:19` | **Fixed** | +| DONATIONS-042 | `postcss` advisory is build-time only (devDependency, never shipped) | Info | Confirmed | 1 | `web/package.json:26` | **Fixed** | +| DONATIONS-043 | No scheduled dependency-audit workflow; the only CI is the release build | Low | Confirmed | 1 | `.github/workflows/build-image.yml:16` | **Fixed** | +| DONATIONS-044 | `stripe.ts` — every money conversion in the product — has zero tests | Med | Confirmed | 1 | `server/package.json:12` | **Fixed** | +| DONATIONS-045 | i18n/RTL is aspirational: no `dir` handling, inline English, `lang` ignored | Low | Confirmed | 2 | `web/src/prefs.ts:93` | Report only | +| DONATIONS-046 | `CLAUDE.md` specifies argon2; the implementation is scrypt | Info | Confirmed | 1 | `CLAUDE.md` §11 | **Fixed** (docs) | +| DONATIONS-047 | Uploaded images are never deleted when a campaign is deleted | Info | Confirmed | 2 | `server/src/store.ts:896` | Report only | +| DONATIONS-048 | `/api/app` discloses the platform's internal LAN address to the public | Low | Confirmed | 2 | `server/src/index.ts:176` | Report only | +| DONATIONS-049 | `docker-compose.yml` deviates from the catalog contract (§10 labels, host port) | Low | Confirmed | 3 | `docker-compose.yml:42` | Cross-repo | +| DONATIONS-050 | Code health: 2,262-line route file, duplicated logic, two dead exports | Info | Confirmed | 1 | `server/src/index.ts` | Report only | +| DONATIONS-051 | Any platform-authenticated identity becomes full local admin (no role check) | Low | Likely | 3 | `server/src/index.ts:230` | Cross-repo | +| DONATIONS-052 | Outbox passes have no overlap guard, so a slow provider causes duplicate sends | Med | Likely | 2 | `server/src/index.ts:2252` | **Fixed** | + +### Refuted findings (recorded so they are not re-raised) + +| Claimed | Verdict | +|---|---| +| `@fastify/static` path traversal / route-guard bypass is exploitable here (claimed High, twice) | **REFUTED.** Both registrations set `index: false` and never `list: true`, and both roots (`/app/public`, `/data/uploads`) contain only assets already public. There is no static-served route guard to bypass — every protected route is a Fastify route with a `preHandler`. Downgraded to Low: worth the major bump on its own schedule, not urgent. | +| Every release tag's `docker-compose.yml` pins the previous release's digest, so masjids run one version behind (claimed High) | **REFUTED.** The catalog pins the **commit** (`6fc4ca27`, the digest-pin commit *after* the tag), not the tag, so installs resolve the correct digest. The tag-vs-pin ordering is a runbook artefact, not a version skew. | + +--- + +## Finding details + +Only findings whose detail adds something beyond the table are expanded. The rest are fully +described by their row plus the linked source. + +### DONATIONS-001 + +**Three-decimal currencies are charged at one tenth of the amount the donor is shown.** +High · Confirmed · Tier 3 · money-correctness · [`server/src/stripe.ts:80-88`](../../server/src/stripe.ts#L80-L88) + +```ts +const ZERO_DECIMAL = new Set(['BIF','CLP','DJF','GNF','JPY','KMF','KRW','MGA','PYG', + 'RWF','UGX','VND','VUV','XAF','XOF','XPF']); +export function currencyDecimals(currency: string): number { + return ZERO_DECIMAL.has(currency.toUpperCase()) ? 0 : 2; // ← every other currency +} +``` + +Stripe defines three exponents, not two. **BHD, JOD, KWD, OMR and TND are three-decimal**: the API +amount is in thousandths (fils/millimes), and Stripe additionally requires the value to be a +multiple of 10. This code returns `2` for all five. + +Reproduced with the repo's own functions: + +``` +BHD: app sends 1000 -> Stripe reads that as 1.000 BHD | intended 10.000 | factor 0.10x +JOD: app sends 1000 -> Stripe reads that as 1.000 JOD | intended 10.000 | factor 0.10x +KWD: app sends 1000 -> Stripe reads that as 1.000 KWD | intended 10.000 | factor 0.10x +OMR: app sends 1000 -> Stripe reads that as 1.000 OMR | intended 10.000 | factor 0.10x +TND: app sends 1000 -> Stripe reads that as 1.000 TND | intended 10.000 | factor 0.10x +``` + +**Attack path / failure path.** No attacker needed. A masjid in Bahrain, Jordan, Kuwait, Oman or +Tunisia sets `CURRENCY=KWD` as an install setting, or receives `MASJID_CURRENCY=KWD` from the +platform profile, or picks it in Settings. Every donation from then on charges a tenth of the +displayed figure. Because `toMajor` uses the same wrong exponent, the admin panel, the CSV and the +goal progress bar all display the *intended* amount, so the app's own records overstate real +income by 10× and nothing ever disagrees with itself. Only the Stripe dashboard tells the truth. + +**Why it matters more than an accounting error.** Zakat is a religious obligation with a +calculated amount. A donor paying 100 KWD of Zakat through a Zakat campaign pays 10 and is told +they paid 100. + +**Fix** (written, tested, in the money PR): add the three-decimal set to `currencyDecimals`, and +round three-decimal amounts to the nearest 10 as Stripe requires. **Not shipped autonomously** — +this changes how an amount is calculated. See `ACTION_REQUIRED.md`; if any masjid is live in one of +these five currencies, past donations need reconciliation against Stripe before the fix lands, or +the ledger's historical rows will silently become inconsistent with the new arithmetic. + +### DONATIONS-002 + +**A card charge that succeeds while `/confirm` fails is never recorded, never receipted, and +invisible forever.** High · Confirmed · Tier 3 · money-correctness · +[`server/src/index.ts:1618-1660`](../../server/src/index.ts#L1618-L1660) + +The one-time flow is: create a PaymentIntent → the browser confirms it with Stripe directly → +**the browser then calls `POST /api/public/confirm`**, and only that call marks the local row +`succeeded`, fires the admin alert and sends the receipt. + +If the donor closes the tab, loses signal, or the box is briefly unreachable in the window between +Stripe's confirmation and that callback, the money is taken and the local row stays `pending` +forever. There is no sweep. Monthly plans gained exactly this reconciliation in v0.38.0 +(`reconcileRenewals`); one-time payments — the overwhelming majority — did not. The optional +webhook covers it *only* for masjids that configured one, which requires public ingress. + +**Consequences:** the donation is missing from the ledger, the CSV, `metrics()`, the goal progress +bar and any Gift Aid claim; the donor gets no receipt; and a `pending` row is indistinguishable +from an abandoned checkout, so nobody investigates. + +**Fix** (written, tested, in the money PR): a periodic sweep that retrieves `pending` one-time +intents from Stripe — the same retrieve-on-demand doctrine already used for plans — and marks +them succeeded, alerting once. **Not shipped autonomously**: it changes how a donation is +recorded, and on first run it will add previously-missing donations to the ledger and totals of +any masjid that has lost payments this way. + +### DONATIONS-003 + +**Donor-PII CSV export is served with no `Cache-Control` at a `.csv` URL.** +High · Confirmed · Tier 1 · [`server/src/index.ts:836`](../../server/src/index.ts#L836) · **Fixed** + +```ts +reply.header('content-type', 'text/csv; charset=utf-8') + .header('content-disposition', 'attachment; filename="donations.csv"'); +``` + +No `Cache-Control`, no `Vary`. The response body is every donor's name, email, amount and +PaymentIntent id. When the admin has enabled public access, this URL is served through Cloudflare, +and **`.csv` is in Cloudflare's default cached-extension list** — a static-extension response with +no cache directives is a candidate for edge caching, after which the cached copy can be served to +a request that carries no session cookie. + +**Attack path.** Admin exports the ledger over the public hostname → the edge caches +`https://give.masjid.org/api/admin/donations.csv` → an unauthenticated attacker requests the same +URL and receives the cached donor list without ever authenticating. Marked *Confirmed* for the +missing headers and the PII content; the edge-caching step depends on the masjid's Cloudflare +configuration, which is why the fix is defence that does not rely on knowing it. + +**Fixed** by sending `cache-control: no-store, private`, `pragma: no-cache` and `vary: cookie` on +both the CSV and the JSON donations route. + +### DONATIONS-005 + +**Unauthenticated admin takeover through `/api/setup` during any platform outage.** +High · Confirmed · Tier 2 · [`server/src/index.ts:251-269`](../../server/src/index.ts#L251-L269) · +**Deferred, needs your decision** + +```ts +if (store.hasAdmin()) return reply.code(409).send({ error: 'This app is already set up.' }); +if (ssoConfigured() && (await probePlatform(req.headers.cookie)).reachable) { + return reply.code(403).send({ error: 'Sign in through your OpenMasjidOS dashboard…' }); +} +// …anyone may now claim the admin password +``` + +Under SSO the local admin is **never set**, so `hasAdmin()` stays false for the life of the +install. The only thing standing between an anonymous LAN caller and permanent admin ownership is +`probePlatform().reachable`. Whenever the platform is down, restarting, upgrading, or simply +unreachable from this container, anyone who can reach the box can `POST /api/setup` with a password +of their choosing and own the panel — Stripe keys, campaigns, the donor ledger. + +This is **not an oversight**: `CLAUDE.md` §13 describes the guard precisely and treats the +outage-window claim as the deliberate price of never bricking the panel (`docs/RESTORE_SSO_FIX.md`). +I am not overriding a documented, deliberate tradeoff without you. + +The obvious hardenings each break something real: +- *Require a recovery code printed to the container log.* Strongest, but a volunteer without + shell access can no longer recover, which is the exact scenario the escape hatch exists for. +- *Restrict to loopback/private peers.* A LAN attacker is already in the private range. +- *Only allow setup in the first N minutes after boot.* An attacker can wait for a reboot; the + real admin may not be at the keyboard during one. + +**My recommendation:** keep the escape hatch, and make abuse loud rather than silent — fire a +Fabric alert on every anonymous `/api/setup` claim, and surface "a local password was set on +`` from ``" permanently in the panel. That is a change to auth behaviour, so it is +your call. See `ACTION_REQUIRED.md`. + +### DONATIONS-009 + +**All three rate limiters collapse to a single shared bucket behind the OS ingress.** +Med · Confirmed · Tier 2 · [`server/src/index.ts:277`](../../server/src/index.ts#L277) · +**Deferred, needs your decision** + +`trustProxy` is off (correctly, for a directly-exposed box) and every limiter keys on +`req.socket.remoteAddress`. When the app is reached through the OpenMasjidOS path-ingress or the +Cloudflare tunnel, that address is the **proxy**, identical for every visitor on earth. So: + +- the donation-intent limit of 30/min becomes 30/min *for all donors combined* — one attacker + denies donations to everyone; +- the tuition lookup limit of 40/min likewise; +- the login limiter locks out **every** remote admin after one attacker's six failures. + +The fix requires deciding *when* `X-Forwarded-For` may be trusted. `CLAUDE.md` §13 asserts the OS +ingress sanitises those headers, which would make trusting them safe when embedded — but no code +in this repo reads them today, and I could not verify the platform's sanitisation from here. +Getting this wrong in either direction is bad: trust it too readily and the login limiter becomes +bypassable with one header; trust it never and public deployments have a trivial donation-DoS. +That is a cross-repo assumption about the platform's ingress, so it is Tier 2/3 territory and +yours. See `ACTION_REQUIRED.md` → Cross-repo. + +### DONATIONS-011 + +**No audit log for any admin financial or donor-data action.** Med · Confirmed · Tier 2 · **Fixed** + +Before this run, nothing recorded that a donor-PII export happened, that a monthly plan was +cancelled, that a Stripe key was rotated, or that a campaign (and its attribution) was deleted. +For an app whose own `CLAUDE.md` §8 promises a financial record, and where a second volunteer with +panel access is in the threat model, there was no answer to "who did this, and when". + +**Fixed** by an append-only `audit_log` table plus writes on: donor export (JSON and CSV), plan +pause/resume/cancel/schedule, Stripe account create/update/delete, and campaign delete. Reverse +migration is a single `DROP TABLE` — see `REMEDIATION.md`. + +### DONATIONS-012 + +**The admin session cookie can never receive `Secure`.** Med · Confirmed · Tier 2 · **Fixed** + +`cookieOptions()` sets `secure: COOKIE_SECURE`, and nothing in the repo ever sets that variable — +not the Dockerfile, not `docker-compose.yml`, not the manifest. Meanwhile `manifest.yaml` declares +`https: true` (the platform fronts the app with TLS, required for Stripe) and `domain: true` +(publishable on a public hostname). So the *normal* deployment hands out a 30-day admin token with +no transport restriction. + +The naive fix — always `Secure` — locks every plain-HTTP LAN admin out of their own panel, which +is why this sat unfixed. **Fixed** instead by making the flag follow the scheme the request +actually arrived on: `Secure` when the request came over TLS (directly, or via +`x-forwarded-proto: https`), plain otherwise. Spoofing that header can only ever *restrict the +spoofer's own* cookie, so trusting it here is safe even though `trustProxy` is off — reasoning +recorded in the code comment. + +### DONATIONS-015 / DONATIONS-016 — deferred as unverifiable + +Both are real and both are ordinarily Tier 1/2, and I am not shipping either, for the same reason: +**I cannot run Docker in this environment**, so I cannot verify that the change still produces a +working image. + +- **015 (root in container).** Adding `USER node` without also fixing `/data` ownership breaks + every write the app makes — the database, uploads, the lot. It needs an entrypoint that chowns + the volume, and one real container start to prove it. Shipping it blind risks bricking every + install on update. `docker-compose.yml`'s own comment says the same. +- **016 (base images by tag).** A wrong digest fails the build. The build workflow does not run on + pull requests (only `main`, `v*` tags and `workflow_dispatch`), so a mistake would not surface + until after merge. + +Both are written up in `ACTION_REQUIRED.md` with the exact change to make once someone can run a +build. + +### DONATIONS-032 + +**The renewal handler reads Invoice fields Stripe removed in a later API version.** +Med · Likely · Tier 3 · [`server/src/index.ts:2060`](../../server/src/index.ts#L2060) + +```ts +const inv = event.data.object as { billing_reason?: string; subscription?: string; + payment_intent?: string; amount_paid?: number; currency?: string }; +``` + +The cast is to a hand-written shape, so TypeScript cannot catch drift. In Stripe's +`2025-03-31.basil` API version, `invoice.payment_intent` was replaced by +`invoice.payments[].payment.payment_intent`, and `invoice.subscription` moved to +`invoice.parent.subscription_details.subscription`. Today the pinned SDK (17.7) speaks +`2025-02-24.acacia`, so both still exist and renewals record correctly — **this is latent, not +live**. But a routine `npm update stripe` to 18.x would silently stop recording monthly renewals: +`piId` becomes `''`, the `if` never fires, no error is logged, and money quietly stops reaching the +ledger. Same cast pattern at the `payment_intent.succeeded` branch. + +**Recommendation** (not shipped — it touches how money is recorded): pin `apiVersion` explicitly +when constructing the Stripe client so an SDK bump cannot change the wire shape underneath, and +replace the hand-written casts with the SDK's own `Stripe.Invoice` type so the compiler fails +instead of the ledger. + +--- + +## Coverage and gaps + +**Assessed statically, in full:** all 56 routes and their guards; the SQL layer; the auth and +session implementation; the Stripe integration and every money conversion; the tuition Fabric +client; the upload path; the CSV export; both workflows; the Dockerfile and compose; every +lockfile; the whole git history for secrets; the web bundle for client-side leakage and XSS. + +**What I could not assess, and why:** + +1. **No runtime.** Nothing was executed against a live server, a real Stripe account (even test + mode) or a running container. So: no dynamic auth testing, no verification that the webhook + signature path accepts a genuine Stripe event, no confirmation of the actual response headers + as served, no proof that a `USER node` container can write `/data`, and no load or slowloris + testing. Every "Confirmed" here means *confirmed by reading the code*, not *observed*. +2. **No Docker.** Base-image digest pinning and the non-root change are unverifiable here + (DONATIONS-015, -016). +3. **The platform side is a black box.** I could not verify that the OpenMasjidOS ingress + sanitises `X-Forwarded-*` (which DONATIONS-009's fix depends on), that `/api/fabric/email` + sanitises the subject before building SMTP headers (DONATIONS-023's real fix), or what + `probePlatform` will accept as a valid identity (DONATIONS-051). +4. **Cloudflare's cache behaviour** for the masjid's actual zone (DONATIONS-003) — the fix is + written so it does not depend on the answer. +5. **Whether any masjid is live in a three-decimal currency** (DONATIONS-001) or has already lost + one-time donations to a failed `/confirm` (DONATIONS-002). Both need a look at real Stripe data + and are the first two items in `ACTION_REQUIRED.md`. +6. **`cloudflared`'s `TUNNEL_TOKEN` support** could not be exercised, so DONATIONS-026 is deferred + rather than fixed. + +**Classes checked and found clean** (the auditors returned 206 such statements; the ones worth +recording): no committed secrets in tree or history; no SQL injection; no XSS sink; no +`eval`/`Function`; no prototype-pollution sink; no unsafe deserialization; no template injection; +no archive extraction; no path traversal in the upload or static paths; no CORS wildcard with +credentials; no JWT (so no algorithm confusion); no `Math.random()` in any security context; no +MD5/SHA1 password hashing; no ECB or static IV; no TLS verification disabled anywhere; no +`NODE_TLS_REJECT_UNAUTHORIZED`; no `docker.sock`, `privileged`, host networking or host PID; no +secrets in Docker layers or CI logs; no source maps in the production bundle; no debug routes or +`/metrics`; no default credentials; no admin route missing its guard; the Stripe secret key never +crosses to the browser; card data never touches the server; `csvCell` is applied to every exported +cell; the tuition/donation route isolation invariants of §13 all hold; and `pull_request_target` +in `cla.yml` never checks out or executes PR code (it runs only a SHA-pinned action). diff --git a/docs/audit/findings.json b/docs/audit/findings.json new file mode 100644 index 0000000..798044a --- /dev/null +++ b/docs/audit/findings.json @@ -0,0 +1,668 @@ +{ + "repo": "OpenMasjidDonations", + "audit_date": "2026-08-03", + "baseline_commit": "6fc4ca272cf412b8d04eeaf3eddce752b072c8b7", + "rollback_tag": "pre-audit-2026-08-03", + "pushed_to_main": false, + "push_blocked_reason": "A push to main triggers build-image.yml, which republishes the live production image tag (:0.38.0 and :latest) to GHCR. Delivered as pull requests instead.", + "branches": { + "audit/security-2026-08-03": "Tier 1 + Tier 2 fixes, mergeable after ordinary review", + "audit/money-2026-08-03": "Tier 3 money-correctness fixes; do not merge before ACTION_REQUIRED.md section 0" + }, + "tests": { + "before": 130, + "after": 179, + "failures": 0 + }, + "npm_audit": { + "server_before": "4 high", + "server_after": "1 high (major bump deferred, refuted as exploitable)", + "web_before": "1 high", + "web_after": "0" + }, + "totals": { + "findings": 52, + "by_severity": { + "high": 4, + "medium": 23, + "low": 19, + "info": 6 + }, + "by_status": { + "fixed-in-pr-not-merged": 3, + "fixed": 21, + "deferred-needs-decision": 3, + "reported-only": 17, + "deferred": 2, + "partly-fixed": 1, + "deferred-unverifiable": 3, + "cross-repo": 2 + } + }, + "findings": [ + { + "id": "DONATIONS-001", + "title": "Three-decimal currencies charged at 1/10 of the displayed amount", + "severity": "high", + "confidence": "Confirmed", + "category": "money-correctness", + "file": "server/src/stripe.ts", + "line": 80, + "summary": "Stripe quotes BHD/JOD/KWD/OMR/TND in thousandths and requires a multiple of 10; currencyDecimals returned 2 for them, so a 10.000 KWD donation charged 1.000 KWD while the ledger recorded 10.000. Self-consistent, so only Stripe disagreed.", + "status": "fixed-in-pr-not-merged", + "commit": "91767c6" + }, + { + "id": "DONATIONS-002", + "title": "A succeeded one-time charge is never recorded when /confirm does not land", + "severity": "high", + "confidence": "Confirmed", + "category": "money-correctness", + "file": "server/src/index.ts", + "line": 1618, + "summary": "One-time payments are marked succeeded only by the donor browser callback. A closed tab leaves money taken at Stripe and the row pending for ever: no ledger entry, no receipt, no alert, indistinguishable from an abandoned checkout. Monthly plans got reconciliation in v0.38.0; one-time payments did not.", + "status": "fixed-in-pr-not-merged", + "commit": "8db58af" + }, + { + "id": "DONATIONS-003", + "title": "Donor-PII CSV export served with no Cache-Control at a .csv URL", + "severity": "high", + "confidence": "Confirmed", + "category": "data-exposure", + "file": "server/src/index.ts", + "line": 836, + "summary": "The export is every donor name, email and PaymentIntent id, with no cache directives, at an extension Cloudflare caches by default. Behind the tunnel the edge could serve the cached donor list to an unauthenticated request.", + "status": "fixed", + "commit": "249e8e5" + }, + { + "id": "DONATIONS-004", + "title": "Image-publishing job runs unpinned third-party Actions while holding GHCR write", + "severity": "medium", + "confidence": "Likely", + "category": "supply-chain", + "file": ".github/workflows/build-image.yml", + "line": 39, + "summary": "Five Actions on moving major tags in a job with packages: write and a GHCR credential for the image every masjid Pi pulls. cla.yml already SHA-pins its action; this file did not.", + "status": "fixed", + "commit": "73086c5" + }, + { + "id": "DONATIONS-005", + "title": "Unauthenticated admin takeover via /api/setup during a platform outage", + "severity": "high", + "confidence": "Confirmed", + "category": "authentication", + "file": "server/src/index.ts", + "line": 261, + "summary": "Under SSO the local admin is never set, so hasAdmin() stays false; the only guard is whether the platform is reachable. During any outage anyone who can reach the box can claim the admin password. Documented in CLAUDE.md as a deliberate anti-bricking tradeoff, so not overridden autonomously.", + "status": "deferred-needs-decision", + "commit": "" + }, + { + "id": "DONATIONS-006", + "title": "No refund or chargeback handling: local totals permanently overstate income", + "severity": "medium", + "confidence": "Confirmed", + "category": "money-correctness", + "file": "server/src/index.ts", + "line": 2044, + "summary": "No refund path in the app and no charge.refunded / charge.dispute.created webhook handling, so a refund issued in the Stripe dashboard never reaches the ledger; metrics, CSV and Gift Aid figures overstate income for ever.", + "status": "reported-only", + "commit": "" + }, + { + "id": "DONATIONS-007", + "title": "Changing the masjid currency rescales every stored amount and misreports history", + "severity": "medium", + "confidence": "Confirmed", + "category": "money-correctness", + "file": "server/src/index.ts", + "line": 320, + "summary": "Amounts are stored in minor units of whatever currency was configured at the time, and rendered with the current currency exponent, so switching currency silently reinterprets every historical row and every configured preset.", + "status": "reported-only", + "commit": "" + }, + { + "id": "DONATIONS-008", + "title": "Cover-the-fees drops the fixed fee for zero-decimal currencies", + "severity": "medium", + "confidence": "Confirmed", + "category": "money-correctness", + "file": "server/src/stripe.ts", + "line": 109, + "summary": "toMinor(0.30, JPY) rounds to 0, so the +30c half of the 2.9%+0.30 model vanished for all sixteen zero-decimal currencies and the gross-up under-recovered on every covered-fee donation. Also hardcodes a US card rate.", + "status": "fixed-in-pr-not-merged", + "commit": "91767c6" + }, + { + "id": "DONATIONS-009", + "title": "All rate limiters collapse to one shared bucket behind the OS ingress", + "severity": "medium", + "confidence": "Confirmed", + "category": "availability", + "file": "server/src/index.ts", + "line": 277, + "summary": "trustProxy is off and every limiter keys on the TCP peer, which is the proxy for all remote visitors. One attacker exhausts the 30/min donation budget for everyone and locks out remote admin logins. The fix needs a platform guarantee about forwarded headers.", + "status": "deferred-needs-decision", + "commit": "" + }, + { + "id": "DONATIONS-010", + "title": "Unauthenticated /intent creates five persistent Stripe objects with no specific limit", + "severity": "medium", + "confidence": "Confirmed", + "category": "availability", + "file": "server/src/index.ts", + "line": 1576, + "summary": "A monthly intent creates a Customer, Price, Subscription, Invoice and PaymentIntent, unauthenticated. A burst bloats the masjid Stripe account and leaves abandoned plan rows behind.", + "status": "fixed", + "commit": "23a4a30" + }, + { + "id": "DONATIONS-011", + "title": "No audit log for any admin financial or donor-data action", + "severity": "medium", + "confidence": "Confirmed", + "category": "audit-logging", + "file": "server/src/index.ts", + "line": 826, + "summary": "Nothing recorded who exported the donor ledger, cancelled a plan, rotated a Stripe key or deleted a campaign. A second volunteer with panel access is in the threat model and CLAUDE.md promises a financial record.", + "status": "fixed", + "commit": "87033d3" + }, + { + "id": "DONATIONS-012", + "title": "Admin session cookie never receives Secure, including on HTTPS deployments", + "severity": "medium", + "confidence": "Confirmed", + "category": "session-management", + "file": "server/src/auth.ts", + "line": 83, + "summary": "secure followed COOKIE_SECURE, which nothing in the repo ever set, while the manifest declares https: true and domain: true. A 30-day admin token was issued with no transport restriction in the normal deployment.", + "status": "fixed", + "commit": "70d5457" + }, + { + "id": "DONATIONS-013", + "title": "No session revocation and no password-change route", + "severity": "medium", + "confidence": "Confirmed", + "category": "session-management", + "file": "server/src/auth.ts", + "line": 14, + "summary": "A stolen 30-day cookie cannot be invalidated short of deleting the database, and the admin password cannot be changed at all. Needs a token-version scheme plus a change-password route.", + "status": "deferred", + "commit": "" + }, + { + "id": "DONATIONS-014", + "title": "No security response headers at all", + "severity": "medium", + "confidence": "Confirmed", + "category": "hardening", + "file": "server/src/index.ts", + "line": 2125, + "summary": "No nosniff, Referrer-Policy or CSP on any route. nosniff and no-referrer shipped; CSP deliberately deferred because a wrong policy stops Stripe Elements and therefore stops donations.", + "status": "partly-fixed", + "commit": "8137bef" + }, + { + "id": "DONATIONS-015", + "title": "Container runs as root with a writable root filesystem", + "severity": "medium", + "confidence": "Confirmed", + "category": "container-hardening", + "file": "Dockerfile", + "line": 33, + "summary": "No USER directive, contrary to CLAUDE.md section 10, so the 0600 on the database protects against nothing and an RCE can persist. Needs an entrypoint that chowns /data plus one real container start to verify.", + "status": "deferred-unverifiable", + "commit": "" + }, + { + "id": "DONATIONS-016", + "title": "Base images pinned by mutable tag, not digest", + "severity": "medium", + "confidence": "Confirmed", + "category": "supply-chain", + "file": "Dockerfile", + "line": 12, + "summary": "node:22-slim and cloudflare/cloudflared:2026.6.1 are moving tags, so the same commit does not build the same image. A wrong digest fails the build and the build workflow does not run on pull requests.", + "status": "deferred-unverifiable", + "commit": "" + }, + { + "id": "DONATIONS-017", + "title": "LoginLimiter sweep condition unreachable, so the map never shrinks", + "severity": "medium", + "confidence": "Confirmed", + "category": "availability", + "file": "server/src/rateLimit.ts", + "line": 24, + "summary": "The condition required fails === 0, but an entry only exists after fail() incremented it and succeed() deletes outright, so nothing was ever swept and the map grew one entry per attacking IP for the process lifetime.", + "status": "fixed", + "commit": "41f8b44" + }, + { + "id": "DONATIONS-018", + "title": "refresh=1 bypassed the same-origin guard on the plans sync", + "severity": "medium", + "confidence": "Confirmed", + "category": "csrf", + "file": "server/src/index.ts", + "line": 1080, + "summary": "Only the write side was gated, so a cross-site top-level navigation could still force the cache open and trigger up to 200 outbound Stripe calls.", + "status": "fixed", + "commit": "64f037d" + }, + { + "id": "DONATIONS-019", + "title": "No rate limit on the unauthenticated routes that call the platform outbound", + "severity": "medium", + "confidence": "Confirmed", + "category": "availability", + "file": "server/src/index.ts", + "line": 228, + "summary": "/api/session (SSO branch) and /api/public/appearance each make an outbound call to the OpenMasjidOS core on every request, making the box an unmetered amplifier against the platform.", + "status": "fixed", + "commit": "23a4a30" + }, + { + "id": "DONATIONS-020", + "title": "Stripe webhook route unauthenticated with no rate limit", + "severity": "low", + "confidence": "Confirmed", + "category": "availability", + "file": "server/src/index.ts", + "line": 2035, + "summary": "CLAUDE.md section 9 requires a limit. Account resolution can hit the Fabric vault, so an unsigned flood still costs outbound calls.", + "status": "fixed", + "commit": "23a4a30" + }, + { + "id": "DONATIONS-021", + "title": "No requestTimeout bound on how long a request may hold a socket", + "severity": "low", + "confidence": "Confirmed", + "category": "availability", + "file": "server/src/index.ts", + "line": 117, + "summary": "Node defaults already bound the classic slowloris via headersTimeout and a 300s requestTimeout, so this is a tightening to 120s rather than an open hole. Downgraded from the auditor Medium on that basis.", + "status": "fixed", + "commit": "64f037d" + }, + { + "id": "DONATIONS-022", + "title": "Donations log and CSV materialise the whole table with no pagination", + "severity": "medium", + "confidence": "Confirmed", + "category": "performance", + "file": "server/src/store.ts", + "line": 1022, + "summary": "Every donation row is loaded into memory on each request. Adding a LIMIT would change the API response and the client-side donor-history panel, so deferred rather than guessed.", + "status": "deferred", + "commit": "" + }, + { + "id": "DONATIONS-023", + "title": "Donor name reaches the receipt Subject with CR/LF intact", + "severity": "low", + "confidence": "Confirmed", + "category": "injection", + "file": "server/src/email.ts", + "line": 83, + "summary": "The subject is the admin template with the unauthenticated donor name substituted in, and becomes an SMTP header at the platform, so CR/LF is header injection. Sanitised at the sender; platform counterpart recorded as cross-repo.", + "status": "fixed", + "commit": "84bcae6" + }, + { + "id": "DONATIONS-024", + "title": "Anonymous donations de-anonymised by the Stripe billing-name backfill", + "severity": "medium", + "confidence": "Likely", + "category": "privacy", + "file": "server/src/index.ts", + "line": 1639, + "summary": "A donor who deliberately leaves name and email blank has them filled in from Stripe billing details at confirm, so the cardholder name reaches the ledger and the CSV. Genuinely ambiguous intent, so deferred for a decision.", + "status": "deferred-needs-decision", + "commit": "" + }, + { + "id": "DONATIONS-025", + "title": "No retention limit, deletion path or subject-access export for donor records", + "severity": "medium", + "confidence": "Confirmed", + "category": "privacy", + "file": "server/src/store.ts", + "line": 897, + "summary": "Donor records including Gift Aid name and home address are kept indefinitely with no deletion route, and deleting a campaign orphans its donation rows rather than handling them.", + "status": "reported-only", + "commit": "" + }, + { + "id": "DONATIONS-026", + "title": "Cloudflare tunnel token passed in argv, visible in the host process table", + "severity": "low", + "confidence": "Confirmed", + "category": "credential-exposure", + "file": "server/src/tunnel.ts", + "line": 99, + "summary": "Any unprivileged local user on the masjid box can read the tunnel token with ps. Fix is to pass TUNNEL_TOKEN in the child environment, but cloudflared could not be exercised here to confirm.", + "status": "deferred-unverifiable", + "commit": "" + }, + { + "id": "DONATIONS-027", + "title": ".dockerignore does not exclude .env", + "severity": "low", + "confidence": "Likely", + "category": "supply-chain", + "file": ".dockerignore", + "line": 1, + "summary": "COPY server/ ./ would bake a developer local .env, with real Stripe keys, into an image layer where it survives later deletion.", + "status": "fixed", + "commit": "73cc072" + }, + { + "id": "DONATIONS-028", + "title": "SQLite -wal/-shm sidecars escape the 0600 chmod", + "severity": "low", + "confidence": "Confirmed", + "category": "data-protection", + "file": "server/src/store.ts", + "line": 362, + "summary": "In WAL mode the newest committed data, including a freshly saved Stripe secret key, lives in the -wal file, which SQLite creates lazily at default permissions. Fixed by locking the directory to 0700 rather than racing the sidecars.", + "status": "fixed", + "commit": "73cc072" + }, + { + "id": "DONATIONS-029", + "title": "No unhandledRejection or uncaughtException handler", + "severity": "low", + "confidence": "Confirmed", + "category": "availability", + "file": "server/src/index.ts", + "line": 2188, + "summary": "The codebase uses fire-and-forget void fn().catch() widely and Node kills the process on an unhandled rejection, so one missed catch in a background alert path would take the donation page down.", + "status": "fixed", + "commit": "2924f79" + }, + { + "id": "DONATIONS-030", + "title": "Upload trusts the client-declared MIME type with no nosniff backstop", + "severity": "low", + "confidence": "Confirmed", + "category": "injection", + "file": "server/src/index.ts", + "line": 575, + "summary": "A file declared image/png can hold anything and is served from the same origin. Filename and extension are server-chosen, so the residual risk was content sniffing, now closed by nosniff.", + "status": "fixed", + "commit": "8137bef" + }, + { + "id": "DONATIONS-031", + "title": "Duplicate receipt and duplicate alert on concurrent /confirm", + "severity": "low", + "confidence": "Likely", + "category": "money-correctness", + "file": "server/src/index.ts", + "line": 1636, + "summary": "The wasPending guard is a read-then-write with no transaction, so two concurrent confirms can both see pending and both send. Fix needs an atomic UPDATE ... WHERE status = pending, which changes how a donation is recorded.", + "status": "reported-only", + "commit": "" + }, + { + "id": "DONATIONS-032", + "title": "invoice.paid reads Invoice fields Stripe removed in 2025-03-31.basil", + "severity": "medium", + "confidence": "Likely", + "category": "money-correctness", + "file": "server/src/index.ts", + "line": 2060, + "summary": "The webhook casts the event to a hand-written shape, so TypeScript cannot catch drift. Latent today (SDK 17.7 speaks acacia) but a routine bump to 18.x would silently stop recording monthly renewals with no error.", + "status": "reported-only", + "commit": "" + }, + { + "id": "DONATIONS-033", + "title": "Gift Aid is dead plumbing and the flag is client-settable with no declaration", + "severity": "low", + "confidence": "Confirmed", + "category": "compliance", + "file": "server/src/index.ts", + "line": 1542, + "summary": "The flag is stored and sent to Stripe as metadata but no declaration, name or address is ever collected, and any unauthenticated client can set it. A masjid could file a claim on donations with no declaration behind them.", + "status": "reported-only", + "commit": "" + }, + { + "id": "DONATIONS-034", + "title": "Idempotency keys are freshly random per request, so they dedupe nothing", + "severity": "low", + "confidence": "Confirmed", + "category": "money-correctness", + "file": "server/src/index.ts", + "line": 1554, + "summary": "crypto.randomUUID() per request means a client retry of the same logical donation creates a second PaymentIntent, contrary to the intent of CLAUDE.md section 9.", + "status": "reported-only", + "commit": "" + }, + { + "id": "DONATIONS-035", + "title": "The 99,999,999 ceiling is validated pre-fee, so the gross-up can exceed it", + "severity": "info", + "confidence": "Confirmed", + "category": "money-correctness", + "file": "server/src/index.ts", + "line": 1535, + "summary": "The maximum is checked on the base amount; a cover-the-fees gross-up can then push the charged amount above Stripe limit, which fails at Stripe rather than being caught with a friendly message.", + "status": "reported-only", + "commit": "" + }, + { + "id": "DONATIONS-036", + "title": "The per-currency minimum charge is a stub with both branches equal", + "severity": "info", + "confidence": "Confirmed", + "category": "money-correctness", + "file": "server/src/index.ts", + "line": 1532, + "summary": "currencyDecimals(currency) === 0 ? 50 : 50 — the ternary was never finished, so a zero-decimal currency uses the same floor as a two-decimal one.", + "status": "reported-only", + "commit": "" + }, + { + "id": "DONATIONS-037", + "title": "The monthly confirm dialog states the pre-fee amount as the recurring charge", + "severity": "low", + "confidence": "Confirmed", + "category": "money-correctness", + "file": "web/src/donate.tsx", + "line": 322, + "summary": "With cover-the-fees on, the donor is shown the base amount as what will recur monthly while the gross-up is what is actually charged each month.", + "status": "reported-only", + "commit": "" + }, + { + "id": "DONATIONS-038", + "title": "Months grouped in UTC but windowed in local time; MASJID_TIMEZONE unused", + "severity": "medium", + "confidence": "Confirmed", + "category": "money-correctness", + "file": "server/src/store.ts", + "line": 1075, + "summary": "strftime groups created_at in UTC while the six-month window is built from the server local clock, and the platform-provided timezone is never applied, so a donation near midnight can land in the wrong month.", + "status": "reported-only", + "commit": "" + }, + { + "id": "DONATIONS-039", + "title": "Historical amounts formatted with the current currency exponent", + "severity": "medium", + "confidence": "Confirmed", + "category": "money-correctness", + "file": "server/src/index.ts", + "line": 821, + "summary": "The ledger and CSV convert every stored row using the currently configured currency decimals rather than the exponent of the currency the row was taken in.", + "status": "reported-only", + "commit": "" + }, + { + "id": "DONATIONS-040", + "title": "@fastify/static carries four High advisories, not exploitable in this configuration", + "severity": "low", + "confidence": "Confirmed", + "category": "dependencies", + "file": "server/package.json", + "line": 17, + "summary": "REFUTED as reachable by the adversarial pass: both registrations set index: false and never list: true, both roots hold only already-public assets, and every protected route is a Fastify route with a preHandler rather than a static route guard. Fix is a two-major bump.", + "status": "reported-only", + "commit": "" + }, + { + "id": "DONATIONS-041", + "title": "Three transitive High advisories with non-major fixes available", + "severity": "low", + "confidence": "Confirmed", + "category": "dependencies", + "file": "server/package.json", + "line": 19, + "summary": "brace-expansion, fast-uri and find-my-way. All unreachable as configured (no HTTP/2, no attacker-controlled URI parsing), but the fixes were free.", + "status": "fixed", + "commit": "73cc072" + }, + { + "id": "DONATIONS-042", + "title": "postcss advisory is build-time only", + "severity": "info", + "confidence": "Confirmed", + "category": "dependencies", + "file": "web/package.json", + "line": 26, + "summary": "A devDependency that never ships, processing only first-party CSS. Fixed anyway; web audit is now zero vulnerabilities.", + "status": "fixed", + "commit": "73cc072" + }, + { + "id": "DONATIONS-043", + "title": "No scheduled dependency-audit workflow", + "severity": "low", + "confidence": "Confirmed", + "category": "process", + "file": ".github/workflows/build-image.yml", + "line": 16, + "summary": "The only workflow was the release image build, so a new advisory against fastify, better-sqlite3 or the Stripe SDK went unnoticed until someone ran npm audit by hand.", + "status": "fixed", + "commit": "2924f79" + }, + { + "id": "DONATIONS-044", + "title": "stripe.ts, every money conversion in the product, had zero tests", + "severity": "medium", + "confidence": "Confirmed", + "category": "test-coverage", + "file": "server/package.json", + "line": 12, + "summary": "No test covered currencyDecimals, toMinor, toMajor or withCoveredFees, and no route or integration test exists at all. The new suite is what surfaced DONATIONS-001 and -008 as reproducible.", + "status": "fixed", + "commit": "ad80f17" + }, + { + "id": "DONATIONS-045", + "title": "i18n and RTL are aspirational despite the stated requirement", + "severity": "low", + "confidence": "Confirmed", + "category": "i18n", + "file": "web/src/prefs.ts", + "line": 93, + "summary": "CLAUDE.md section 12 requires RTL-ready with logical CSS properties. In practice all strings are inline English, no dir or lang is set, and the platform lang hand-off is ignored, so an Arabic locale would not render correctly today.", + "status": "reported-only", + "commit": "" + }, + { + "id": "DONATIONS-046", + "title": "CLAUDE.md specifies argon2; the implementation is scrypt", + "severity": "info", + "confidence": "Confirmed", + "category": "documentation", + "file": "CLAUDE.md", + "line": 0, + "summary": "scrypt at N=2^16 with a per-hash cost record is a sound choice; the documentation was wrong, not the code.", + "status": "fixed", + "commit": "decfaab" + }, + { + "id": "DONATIONS-047", + "title": "Uploaded images are never deleted when a campaign is deleted", + "severity": "info", + "confidence": "Confirmed", + "category": "housekeeping", + "file": "server/src/store.ts", + "line": 896, + "summary": "deleteCampaign drops the row and leaves the files on the data volume, so an admin editing images repeatedly grows the volume with no way to reclaim it from the panel.", + "status": "reported-only", + "commit": "" + }, + { + "id": "DONATIONS-048", + "title": "/api/app discloses the platform internal LAN address to the public", + "severity": "low", + "confidence": "Confirmed", + "category": "information-disclosure", + "file": "server/src/index.ts", + "line": 176, + "summary": "omosBase is returned to unauthenticated visitors, revealing the internal address of the OpenMasjidOS core to anyone who can load the donation page.", + "status": "reported-only", + "commit": "" + }, + { + "id": "DONATIONS-049", + "title": "docker-compose.yml deviates from the catalog contract", + "severity": "low", + "confidence": "Confirmed", + "category": "contract", + "file": "docker-compose.yml", + "line": 42, + "summary": "None of the three required com.openmasjid.* labels are present and the host port is hardcoded rather than using the platform-assigned variable. Part of the published catalog contract, so cross-repo.", + "status": "cross-repo", + "commit": "" + }, + { + "id": "DONATIONS-050", + "title": "Code health: a 2,262-line route file, duplicated logic, dead exports", + "severity": "info", + "confidence": "Confirmed", + "category": "maintainability", + "file": "server/src/index.ts", + "line": 0, + "summary": "index.ts holds every route, the outboxes, the caches and the static host. Three pieces of duplicated logic and two dead exports. No TODO/FIXME anywhere and full SPDX coverage, which is unusually good.", + "status": "reported-only", + "commit": "" + }, + { + "id": "DONATIONS-051", + "title": "Any platform-authenticated identity becomes a full local admin", + "severity": "low", + "confidence": "Likely", + "category": "authorization", + "file": "server/src/index.ts", + "line": 230, + "summary": "GET /api/session mints an admin session for any identity the platform confirms; the username is recorded but never checked against a role. If OpenMasjidOS gains non-admin users they silently become donations administrators.", + "status": "cross-repo", + "commit": "" + }, + { + "id": "DONATIONS-052", + "title": "Outbox passes have no overlap guard, so a slow provider causes duplicate sends", + "severity": "medium", + "confidence": "Likely", + "category": "correctness", + "file": "server/src/index.ts", + "line": 2252, + "summary": "Both outboxes run on a 60s interval and each item makes an 8s-timeout network call, so a pass can outlast its own interval, and a second pass then reads the same pending rows and repeats the work.", + "status": "fixed", + "commit": "decfaab" + } + ] +} diff --git a/server/package-lock.json b/server/package-lock.json index 68e09e1..fae6bd6 100644 --- a/server/package-lock.json +++ b/server/package-lock.json @@ -1,12 +1,12 @@ { "name": "openmasjid-donations-server", - "version": "0.14.0", + "version": "0.38.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "openmasjid-donations-server", - "version": "0.14.0", + "version": "0.38.0", "license": "AGPL-3.0-only", "dependencies": { "@fastify/cookie": "^11.0.2", @@ -880,15 +880,15 @@ } }, "node_modules/brace-expansion": { - "version": "5.0.6", - "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-5.0.6.tgz", - "integrity": "sha512-kLpxurY4Z4r9sgMsyG0Z9uzsBlgiU/EFKhj/h91/8yHu0edo7XuixOIH3VcJ8kkxs6/jPzoI6U9Vj3WqbMQ94g==", + "version": "5.0.9", + "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-5.0.9.tgz", + "integrity": "sha512-ScQ4IuvIEF1TMlP7Zt+vjJ//9zlPb2SDcxWxM3bk8s6t6GGdJ7KO1dCcTidOPJKePW30LE/2cT7wCyPho9/Wxg==", "license": "MIT", "dependencies": { "balanced-match": "^4.0.2" }, "engines": { - "node": "18 || 20 || >=22" + "node": "20 || >=22" } }, "node_modules/buffer": { @@ -1196,9 +1196,9 @@ } }, "node_modules/fast-uri": { - "version": "3.1.2", - "resolved": "https://registry.npmjs.org/fast-uri/-/fast-uri-3.1.2.tgz", - "integrity": "sha512-rVjf7ArG3LTk+FS6Yw81V1DLuZl1bRbNrev6Tmd/9RaroeeRRJhAt7jg/6YFxbvAQXUCavSoZhPPj6oOx+5KjQ==", + "version": "3.1.5", + "resolved": "https://registry.npmjs.org/fast-uri/-/fast-uri-3.1.5.tgz", + "integrity": "sha512-gHwA1O9LDIcKunMKhObS/HimwtehO1nPUECKAu5TpKgaO19fcWEl4bliWe1jWxVFvIXztJjjQ4L8XQ1EU9f7Jw==", "funding": [ { "type": "github", @@ -1276,9 +1276,9 @@ "license": "MIT" }, "node_modules/find-my-way": { - "version": "9.6.0", - "resolved": "https://registry.npmjs.org/find-my-way/-/find-my-way-9.6.0.tgz", - "integrity": "sha512-Zf4Xve4RymLl7NgaavNebZ01joJ8MfVerOG43wy7SHLO+r+K0C6d/SE0BiR7AV5V1VOCFlOP7ecdo+I4qmiHrQ==", + "version": "9.7.0", + "resolved": "https://registry.npmjs.org/find-my-way/-/find-my-way-9.7.0.tgz", + "integrity": "sha512-f2JHn75x2JlwUwLenZypgczR7YWMb/uO9BvUXtus+JMgkbIkLADd38cI4EiV+OQqrGo1Zlq6V8wnqMJ8e62wUQ==", "license": "MIT", "dependencies": { "fast-deep-equal": "^3.1.3", diff --git a/server/package.json b/server/package.json index 356806c..e24a57a 100644 --- a/server/package.json +++ b/server/package.json @@ -9,7 +9,7 @@ "dev": "tsx watch src/index.ts", "start": "node dist/index.js", "typecheck": "tsc -p tsconfig.json --noEmit", - "test": "node --import tsx --test src/csv.test.ts src/store.test.ts src/students.test.ts src/studentsFabric.test.ts src/email.test.ts src/plans.test.ts" + "test": "node --import tsx --test src/auth.test.ts src/csv.test.ts src/rateLimit.test.ts src/store.test.ts src/stripe.test.ts src/students.test.ts src/studentsFabric.test.ts src/email.test.ts src/plans.test.ts" }, "dependencies": { "@fastify/cookie": "^11.0.2", diff --git a/server/src/auth.test.ts b/server/src/auth.test.ts new file mode 100644 index 0000000..03a2f61 --- /dev/null +++ b/server/src/auth.test.ts @@ -0,0 +1,107 @@ +// SPDX-License-Identifier: AGPL-3.0-only +// Copyright (C) 2026 OpenMasjid-Solutions +// +// Locks the admin session primitives. Before the 2026-08-03 audit this file did not exist, which is +// why DONATIONS-012 (a session cookie that could never be `Secure`) survived so long. +// +// The `Secure` tests cut BOTH ways on purpose. The bug was a missing flag on HTTPS; the dangerous +// over-fix is setting it on a plain-HTTP LAN box, which would lock a masjid volunteer out of their +// own panel with no way back in. Both directions are asserted. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { cookieOptions, hashPassword, makeToken, secureForRequest, verifyPassword, verifyToken, MAX_AGE_MS } from './auth'; + +const req = (protocol: string | undefined, headers: Record = {}) => ({ protocol, headers }); + +// ── DONATIONS-012 ──────────────────────────────────────────────────────────── + +test('cookie Secure: set when the request arrived over TLS directly', () => { + assert.equal(secureForRequest(req('https')), true); +}); + +test('cookie Secure: set when the platform ingress reports https', () => { + assert.equal(secureForRequest(req('http', { 'x-forwarded-proto': 'https' })), true); + // A proxy chain sends a list; the client-facing hop is the first entry. + assert.equal(secureForRequest(req('http', { 'x-forwarded-proto': 'https, http' })), true); + assert.equal(secureForRequest(req('http', { 'x-forwarded-proto': ['https', 'http'] })), true); + assert.equal(secureForRequest(req('http', { 'x-forwarded-proto': 'HTTPS' })), true, 'case-insensitive'); +}); + +test('cookie Secure: NOT set on a plain-HTTP LAN request — a masjid must not be locked out', () => { + assert.equal(secureForRequest(req('http')), false); + assert.equal(secureForRequest(req(undefined)), false); + assert.equal(secureForRequest(req('http', { 'x-forwarded-proto': 'http' })), false); + assert.equal(secureForRequest(req('http', { 'x-forwarded-proto': '' })), false); + assert.equal(secureForRequest(req('http', { 'x-forwarded-proto': 'httpsish' })), false, 'no prefix matching'); + assert.equal(secureForRequest(req('http', { 'x-forwarded-proto': 42 })), false, 'a non-string header is not TLS'); +}); + +test('cookieOptions: always httpOnly + SameSite=Lax + Path=/, and carries the Secure decision', () => { + const insecure = cookieOptions(MAX_AGE_MS, false); + assert.equal(insecure.httpOnly, true); + assert.equal(insecure.sameSite, 'lax'); + assert.equal(insecure.path, '/'); + assert.equal(insecure.secure, false); + assert.equal(insecure.maxAge, Math.floor(MAX_AGE_MS / 1000)); + assert.equal(cookieOptions(MAX_AGE_MS, true).secure, true); +}); + +// ── Session token ──────────────────────────────────────────────────────────── + +test('session token: round-trips, and a tampered payload or signature is rejected', () => { + const secret = Buffer.from('a'.repeat(32)); + const token = makeToken(secret, 60_000); + assert.equal(verifyToken(secret, token), true); + + const [payload, sig] = token.split('.'); + assert.equal(verifyToken(secret, `${payload}x.${sig}`), false, 'payload tampered'); + assert.equal(verifyToken(secret, `${payload}.${sig.slice(0, -1)}z`), false, 'signature tampered'); + assert.equal(verifyToken(Buffer.from('b'.repeat(32)), token), false, 'wrong signing secret'); + assert.equal(verifyToken(secret, undefined), false); + assert.equal(verifyToken(secret, 'nonsense'), false); +}); + +test('session token: an expired token is rejected', () => { + const secret = Buffer.from('c'.repeat(32)); + assert.equal(verifyToken(secret, makeToken(secret, -1_000)), false, 'already expired'); +}); + +test('session token: the audience claim is checked, so a token minted for another use is refused', () => { + const secret = Buffer.from('d'.repeat(32)); + // Forge a correctly-SIGNED token with a different audience: signature valid, aud wrong. + const crypto = require('node:crypto') as typeof import('node:crypto'); + const payload = Buffer.from(JSON.stringify({ exp: Date.now() + 60_000, aud: 'something-else' })).toString('base64url'); + const sig = crypto.createHmac('sha256', secret).update(payload).digest('base64url'); + assert.equal(verifyToken(secret, `${payload}.${sig}`), false); +}); + +// ── Password hashing ───────────────────────────────────────────────────────── + +test('password: verifies, rejects a wrong one, and salts so two hashes differ', () => { + const a = hashPassword('correct horse battery staple'); + assert.equal(verifyPassword('correct horse battery staple', a), true); + assert.equal(verifyPassword('Correct horse battery staple', a), false); + assert.equal(verifyPassword('', a), false); + + const b = hashPassword('correct horse battery staple'); + assert.notEqual(a.hash, b.hash, 'same password, different salt → different hash'); + assert.notEqual(a.salt, b.salt); +}); + +test('password: a pre-v0.11 hash with no stored cost still verifies (no admin lockout on upgrade)', () => { + // Cost is recorded per hash so it can be raised without invalidating existing credentials; a hash + // written before that field existed must fall back to Node's default N=16384. + const crypto = require('node:crypto') as typeof import('node:crypto'); + const salt = crypto.randomBytes(16); + const legacy = { + hash: crypto.scryptSync('old-password', salt, 32, { N: 16384, r: 8, p: 1, maxmem: 256 * 1024 * 1024 }).toString('hex'), + salt: salt.toString('hex'), + }; + assert.equal(verifyPassword('old-password', legacy), true); + assert.equal(verifyPassword('wrong', legacy), false); +}); + +test('password: a corrupt credential record fails closed rather than throwing', () => { + assert.equal(verifyPassword('x', { hash: 'not-hex', salt: 'zz', n: 16384 }), false); + assert.equal(verifyPassword('x', { hash: '', salt: '', n: 1 }), false); +}); diff --git a/server/src/auth.ts b/server/src/auth.ts index fb01347..543f5bd 100644 --- a/server/src/auth.ts +++ b/server/src/auth.ts @@ -53,11 +53,29 @@ function hmac(secret: Buffer, payload: string): string { type Audience = 'admin'; -export function makeToken(secret: Buffer, maxAgeMs = MAX_AGE_MS, aud: Audience = 'admin'): string { - const payload = Buffer.from(JSON.stringify({ exp: Date.now() + maxAgeMs, aud })).toString('base64url'); +/** `usr` is carried so the audit log can name WHO did something (DONATIONS-011) without another + * platform round-trip on every request. It is inside the HMAC, so it cannot be forged; it is the + * admin's own OpenMasjidOS username, which they already see in the panel. Purely additive — a token + * minted before this existed simply has no `usr` and still verifies. */ +export function makeToken(secret: Buffer, maxAgeMs = MAX_AGE_MS, aud: Audience = 'admin', usr?: string): string { + const claims: { exp: number; aud: Audience; usr?: string } = { exp: Date.now() + maxAgeMs, aud }; + if (usr) claims.usr = usr.slice(0, 120); + const payload = Buffer.from(JSON.stringify(claims)).toString('base64url'); return `${payload}.${hmac(secret, payload)}`; } +/** The username inside a VALID token, or '' (unsigned/expired/absent). Verifies before reading, so + * a caller can never be handed an attacker-chosen name. */ +export function tokenUser(secret: Buffer, token: string | undefined, aud: Audience = 'admin'): string { + if (!verifyToken(secret, token, aud) || !token) return ''; + try { + const obj = JSON.parse(Buffer.from(token.slice(0, token.lastIndexOf('.')), 'base64url').toString()) as { usr?: unknown }; + return typeof obj.usr === 'string' ? obj.usr.slice(0, 120) : ''; + } catch { + return ''; + } +} + /** Verify signature, expiry AND audience (constant-time on the signature). */ export function verifyToken(secret: Buffer, token: string | undefined, aud: Audience = 'admin'): boolean { if (!token) return false; @@ -76,20 +94,46 @@ export function verifyToken(secret: Buffer, token: string | undefined, aud: Audi } } -// Set COOKIE_SECURE=1 (or true) for HTTPS deployments (e.g. behind the OpenMasjidOS -// per-app TLS proxy or any reverse proxy terminating TLS) so the session cookie is -// only sent over HTTPS. Default OFF: a masjid LAN is usually plain HTTP, and a Secure -// cookie would silently break sign-in there. +// Force `Secure` on every session cookie regardless of the request scheme. Rarely needed now that +// the flag follows the actual scheme (see `secureForRequest`), but kept as an override for an +// operator who knows their deployment is HTTPS-only and wants no scheme sniffing at all. const COOKIE_SECURE = process.env.COOKIE_SECURE === '1' || (process.env.COOKIE_SECURE ?? '').toLowerCase() === 'true'; -/** Cookie options for @fastify/cookie's setCookie. HTTP-only + SameSite=Lax + Path=/, - * and `Secure` when COOKIE_SECURE is set (HTTPS deployments). */ -export function cookieOptions(maxAgeMs = MAX_AGE_MS) { +/** Did this request arrive over TLS? + * + * Nothing in the shipped configuration ever set COOKIE_SECURE, so before this the admin session + * cookie was issued WITHOUT `Secure` even in the normal deployment — where `manifest.yaml` declares + * `https: true` (the platform fronts the app with TLS, required for Stripe) and `domain: true` (it + * can be published on a public hostname). A 30-day admin token with no transport restriction is + * then attached to any plaintext request to the same host (DONATIONS-012). + * + * Always setting `Secure` was not an option: a masjid LAN is usually plain HTTP, and the flag would + * silently lock every standalone admin out of their own panel. So it follows the scheme the request + * actually arrived on. + * + * On trusting `x-forwarded-proto` while `trustProxy` is off: reading it here is safe in a way that + * reading it for a rate-limit key would not be. The header can only ever ADD `Secure` to the + * cookie in the response to THAT SAME request — i.e. it can only restrict where the sender's own + * cookie will be sent. There is no cross-user effect and no privilege gained, and a cross-site + * attacker cannot set headers on the admin's own request in the first place. */ +export function secureForRequest(req: { protocol?: string; headers: Record }): boolean { + if (COOKIE_SECURE) return true; + if (req.protocol === 'https') return true; + const xfp = req.headers['x-forwarded-proto']; + // May be a comma-separated list from a proxy chain; the client-facing hop is the first entry. + const first = (Array.isArray(xfp) ? xfp[0] : typeof xfp === 'string' ? xfp : '').split(',')[0].trim().toLowerCase(); + return first === 'https'; +} + +/** Cookie options for @fastify/cookie's setCookie. HTTP-only + SameSite=Lax + Path=/, and `Secure` + * when the request came over TLS. Pass the request; omitting it falls back to the env override + * only, which is the pre-existing behaviour. */ +export function cookieOptions(maxAgeMs = MAX_AGE_MS, secure = COOKIE_SECURE) { return { httpOnly: true, sameSite: 'lax' as const, path: '/', - secure: COOKIE_SECURE, + secure, maxAge: Math.floor(maxAgeMs / 1000), }; } diff --git a/server/src/email.test.ts b/server/src/email.test.ts index e26293c..b9240ad 100644 --- a/server/src/email.test.ts +++ b/server/src/email.test.ts @@ -97,3 +97,22 @@ test('accent: valid hex used; invalid falls back to default (no CSS injection)', test('fillVars preserves newlines but collapses runs of spaces', () => { assert.equal(fillVars('a\n\nb c', { name: 'x', amount: 'y', campaign: 'z', masjid: 'm' }), 'a\n\nb c'); }); + +// ── DONATIONS-023 ──────────────────────────────────────────────────────────── +// The subject is built from the admin's template with the DONOR's own name substituted in, and the +// donor is an unauthenticated stranger. The finished subject becomes an SMTP header at the platform, +// so a CR/LF in a name is a header-injection attempt. Fails before the fix: the raw name went +// through with its newlines intact. +test('receipt subject: a donor name cannot inject an email header (CR/LF collapsed)', () => { + const evil = 'Ahmed\r\nBcc: attacker@evil.example\r\n\r\nInjected body'; + const { subject } = renderReceipt({ ...TPL, subject: 'Receipt for {name}' }, { ...CTX, name: evil }); + assert.ok(!/[\r\n]/.test(subject), `subject must be one line, got ${JSON.stringify(subject)}`); + assert.ok(!/\u2028|\u2029|\v|\f|\0/.test(subject), 'and no exotic line separators either'); + assert.ok(subject.startsWith('Receipt for Ahmed'), 'the legitimate part of the name survives'); + assert.ok(subject.includes('Bcc:'), 'the text is neutralised by flattening, not silently dropped'); +}); + +test('receipt subject: ordinary names and unicode are untouched', () => { + const ok = renderReceipt({ ...TPL, subject: 'Receipt for {name}' }, { ...CTX, name: 'Yūsuf Al-Ḥasan' }).subject; + assert.equal(ok, 'Receipt for Yūsuf Al-Ḥasan'); +}); diff --git a/server/src/email.ts b/server/src/email.ts index 1c45501..29dde44 100644 --- a/server/src/email.ts +++ b/server/src/email.ts @@ -63,6 +63,22 @@ export function fillVars(tpl: string, v: { name: string; amount: string; campaig return out.replace(/[ \t]{2,}/g, ' ').replace(/[ \t]+([!?.,])/g, '$1').trim(); } +/** Collapse anything that could break out of a single header line into a space. + * + * The subject is built from the admin's template with the DONOR's own name substituted in, and the + * donor is an unauthenticated stranger (`donorName` comes straight off the public intent body). + * We hand the finished subject to the OpenMasjidOS Fabric as JSON, and the platform is what turns + * it into a real SMTP header — so a name containing CR/LF is a header-injection attempt aimed at + * the platform's mailer (`Bcc:`, a forged `From:`, an injected body). Sanitising at the sender is + * cheap, harmless to every legitimate name, and does not depend on the platform getting it right; + * the platform-side counterpart is recorded in docs/audit/ACTION_REQUIRED.md (DONATIONS-023). */ +function oneLine(s: string): string { + // Escapes, NEVER literal characters: U+2028/U+2029 are line terminators in JavaScript SOURCE, + // so pasting them into a regex silently ends the expression. U+0085 (NEL) and NUL are treated + // as line breaks or string terminators by some mail libraries, so they go too. + return s.replace(/[\r\n\u2028\u2029\u0085\v\f\0]+/g, ' ').replace(/\s{2,}/g, ' ').trim(); +} + /** Only an http(s) absolute URL with no quotes/whitespace is allowed (img src / link href). */ function safeUrl(url: string): string { const u = (url ?? '').trim(); @@ -80,7 +96,8 @@ function row(label: string, value: string, opts: { bold?: boolean; first?: boole export function renderReceipt(tpl: ReceiptTemplate, ctx: ReceiptContext): RenderedEmail { const accent = /^#[0-9a-fA-F]{3,8}$/.test((tpl.accent || '').trim()) ? tpl.accent.trim() : ACCENT_DEFAULT; const vars = { name: ctx.name, amount: ctx.amountText, campaign: ctx.campaignTitle, masjid: ctx.masjidName }; - const subject = (fillVars(tpl.subject || 'Your donation receipt', vars) || 'Your donation receipt').slice(0, 200); + // oneLine BEFORE the length cap, so a 200-char slice can never end mid-escape or leave a CR. + const subject = (oneLine(fillVars(tpl.subject || 'Your donation receipt', vars)) || 'Your donation receipt').slice(0, 200); const heading = fillVars(tpl.heading || 'JazākAllāhu khayran!', vars) || 'JazākAllāhu khayran!'; const paragraph = fillVars(tpl.body || 'Your donation was received. May Allah accept it from you and reward you abundantly.', vars); const logo = safeUrl(ctx.masjidLogo); diff --git a/server/src/index.ts b/server/src/index.ts index c6b4564..1489f1d 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -19,7 +19,7 @@ import { config, ssoConfigured } from './config'; import { makeLog } from './logger'; import { Store, slugify, rid, RESERVED_SLUGS } from './store'; import type { Campaign, Donation, StripeAccount, StripeConfig, ThankYou, LargeDonation, EmailReceipt } from './store'; -import { COOKIE, cookieOptions, hashPassword, makeToken, verifyPassword, verifyToken, SSO_SESSION_MS } from './auth'; +import { COOKIE, cookieOptions, hashPassword, makeToken, secureForRequest, tokenUser, verifyPassword, verifyToken, MAX_AGE_MS, SSO_SESSION_MS } from './auth'; import { notify, probePlatform, fetchFabricStripe, cachedFabricStripe, fetchFabricStripeAccounts, clearFabricStripeCache, fetchFabricSite, cachedFabricSite, fabricConfigSignature, fabricEmail, fabricAlert, emailStatus } from './fabric'; import { renderReceipt, type ReceiptContext } from './email'; import { @@ -115,6 +115,13 @@ async function main(): Promise { // the real TCP peer below. (A future reverse-proxy deployment would set this to // the specific trusted proxy CIDR, not `true`.) bodyLimit: 1_048_576, // 1 MiB JSON cap (uploads get their own limit later) + // Bound how long one request may hold a socket. Node already defends the classic slowloris via + // `headersTimeout` (60s) and `requestTimeout` (300s), so this is a tightening rather than a hole + // being closed: 120s is still ample for the 5 MiB image upload over bad masjid wifi (~43 KB/s) + // but a quarter of the default grip on a Pi's socket table. `connectionTimeout` is deliberately + // NOT set — it maps to Node's socket-INACTIVITY timeout, which would also reap idle keep-alive + // sockets that Fastify holds for 72s by design, buying TCP churn for no security (DONATIONS-021). + requestTimeout: 120_000, // Base-path awareness (manifest `domain: true`): when OpenMasjidOS exposes us behind // its Cloudflare tunnel it forwards the FULL admin-chosen path prefix (e.g. /donate) // WITHOUT stripping it, so requests arrive as /donate/api/x, /donate/assets/y, etc. @@ -146,6 +153,25 @@ async function main(): Promise { } }); + // ── Baseline security response headers (every route, including static) ────── + // Deliberately a SHORT list. Two are unambiguous wins here: + // • nosniff — an upload's content type is taken from the client-declared multipart header, so + // a file claiming image/png can hold anything. It is written with a server-chosen name and + // extension and served from OUR origin, so without this a content-sniffing browser is the + // one thing between that file and same-origin script execution. + // • no-referrer — a campaign's unlisted link carries its token in the path + // (/api/public/campaign/:slug/:token). Any outbound click from a donation or admin page + // would otherwise hand that URL to the destination site in the Referer header. + // X-Frame-Options / frame-ancestors is deliberately NOT set: OpenMasjidOS embeds this app in a + // frame, and the widget route sets `frame-ancestors *` on purpose. A CSP is deliberately NOT set + // either — Stripe's Payment Element loads js.stripe.com and its own frames, and a CSP that is + // even slightly wrong stops donors paying. That belongs in a change someone can test against a + // real Stripe Element, not in a headers sweep (see docs/audit/ACTION_REQUIRED.md). + app.addHook('onSend', async (_req, reply) => { + reply.header('x-content-type-options', 'nosniff'); + reply.header('referrer-policy', 'no-referrer'); + }); + // Uploaded images live on the data volume and are served read-only at /uploads/*. const uploadsDir = path.join(config.dataDir, 'uploads'); fs.mkdirSync(uploadsDir, { recursive: true }); @@ -163,6 +189,55 @@ async function main(): Promise { } }; + /** Who is acting, for the audit log. The name comes out of the SIGNED session token, so it cannot + * be spoofed; a local-password session has no platform username, hence the fallback. */ + const actorOf = (req: import('fastify').FastifyRequest): string => tokenUser(store.secret, req.cookies[COOKIE]) || 'local admin'; + /** Append one admin action to the audit log (never throws — see store.recordAudit). */ + const audit = (req: import('fastify').FastifyRequest, action: string, subject = '', detail = ''): void => + store.recordAudit(action, { actor: actorOf(req), subject, detail }); + + /** A fixed-window per-peer limiter, in the shape the donation and tuition limiters already use. + * Keyed on the real TCP peer, never a spoofable X-Forwarded-For (trustProxy is off). + * + * NOTE the known limitation, recorded as DONATIONS-009: behind the OpenMasjidOS path-ingress or + * the Cloudflare tunnel the peer is the PROXY, so every remote visitor shares one bucket. That + * is a deliberate open question (trusting a forwarded header would make the login limiter + * bypassable), so these caps are set generously enough that a shared bucket does not deny + * service to honest traffic. The two existing limiters keep their own inline copies rather than + * being refactored onto this — they work, and rewriting a live rate limiter to save six lines is + * not a trade worth making. */ + const makeRateLimiter = (perMinute: number) => { + const hits = new Map(); + return (ip: string): boolean => { + const now = Date.now(); + if (hits.size > 5000) for (const [k, w] of hits) if (w.reset <= now) hits.delete(k); + const w = hits.get(ip); + if (!w || w.reset <= now) { + hits.set(ip, { c: 1, reset: now + 60_000 }); + return true; + } + if (w.c >= perMinute) return false; + w.c += 1; + return true; + }; + }; + const peerOf = (req: import('fastify').FastifyRequest): string => req.socket.remoteAddress ?? 'unknown'; + + // Both of these are unauthenticated AND make an outbound call to the OpenMasjidOS core on every + // request (a session probe / an appearance fetch). Without a cap, anyone who can reach this box + // can use it as an unmetered amplifier against the platform — and each call also occupies one of + // the Pi's sockets for up to 4–8s. 120/min is far above any real page load. + const platformCallRateOk = makeRateLimiter(120); + // The webhook is unauthenticated by necessity (Stripe calls it). Signature verification is cheap, + // but resolving the account can hit the Fabric vault, so an unsigned flood still costs outbound + // calls. CLAUDE.md §9 asks for this limit explicitly. 300/min is ~10× Stripe's real burst rate, + // so a genuine event is never dropped (and Stripe retries a 429 anyway). + const webhookRateOk = makeRateLimiter(300); + // A MONTHLY intent creates a Stripe Customer + Price + Subscription + Invoice + PaymentIntent — + // five persistent objects, against the one a card payment creates. A donor sets up one plan, so + // 5/min is generous, while a burst can no longer bloat the masjid's Stripe account (DONATIONS-010). + const monthlyIntentRateOk = makeRateLimiter(5); + // ── Health check ──────────────────────────────────────────────────────────── app.get('/healthz', async () => ({ ok: true })); @@ -196,8 +271,9 @@ async function main(): Promise { // plain HTTP, so a direct browser fetch would be blocked as mixed content. The web // polls us (same origin) and we fetch the platform server-to-server. Returns the // platform's { v, theme, wallpaper, wallpaperImage, accent, lang } or {} (no secrets). - app.get('/api/public/appearance', async (_req, reply) => { + app.get('/api/public/appearance', async (req, reply) => { reply.header('cache-control', 'no-store'); + if (!platformCallRateOk(peerOf(req))) return reply.code(429).send({ error: 'Too many requests. Please try again shortly.' }); const base = config.omosBaseUrl; if (!base) return {}; try { @@ -224,11 +300,16 @@ async function main(): Promise { // "open it from the dashboard" apart from "OpenMasjidOS is unreachable" (a // migrated/down platform must offer the local-password way in, not a dead loop). let reachable = true; + // Only the SSO upgrade costs an outbound platform call, so the limit guards that branch only — + // an already-signed-in admin (or a standalone install) is never rate-limited out of the panel. + if (!authed && ssoConfigured() && !platformCallRateOk(peerOf(req))) { + return reply.code(429).send({ error: 'Too many requests. Please try again shortly.' }); + } if (!authed && ssoConfigured()) { const probe = await probePlatform(req.headers.cookie); reachable = probe.reachable; if (probe.username) { - reply.setCookie(COOKIE, makeToken(store.secret, SSO_SESSION_MS), cookieOptions(SSO_SESSION_MS)); + reply.setCookie(COOKIE, makeToken(store.secret, SSO_SESSION_MS, 'admin', probe.username), cookieOptions(SSO_SESSION_MS, secureForRequest(req))); authed = true; username = probe.username; } @@ -264,7 +345,7 @@ async function main(): Promise { const parsed = SetupBody.safeParse(req.body); if (!parsed.success) return reply.code(400).send({ error: 'Please choose a password of at least 8 characters.' }); store.setAdmin(hashPassword(parsed.data.password), parsed.data.name?.trim()); - reply.setCookie(COOKIE, makeToken(store.secret), cookieOptions()); + reply.setCookie(COOKIE, makeToken(store.secret), cookieOptions(MAX_AGE_MS, secureForRequest(req))); return { data: { ok: true } }; }); @@ -282,7 +363,7 @@ async function main(): Promise { const parsed = LoginBody.safeParse(req.body); if (parsed.success && verifyPassword(parsed.data.password, admin)) { loginLimiter.succeed(peer); - reply.setCookie(COOKIE, makeToken(store.secret), cookieOptions()); + reply.setCookie(COOKIE, makeToken(store.secret), cookieOptions(MAX_AGE_MS, secureForRequest(req))); return { data: { ok: true } }; } loginLimiter.fail(peer); @@ -639,6 +720,7 @@ async function main(): Promise { const err = checkKeys(parsed.data); if (err) return reply.code(400).send({ error: err }); const acct = store.createStripeAccount({ label: parsed.data.label || 'Stripe account', ...parsed.data }); + audit(req, 'stripe.account.create', acct.id, 'added a Stripe account (' + acct.label + ')'); const verify = acct.secretKey ? await verifySecretKey(acct.secretKey) : undefined; return { data: { ...publicAccount(acct), verify } }; }); @@ -649,12 +731,15 @@ async function main(): Promise { if (err) return reply.code(400).send({ error: err }); const acct = store.updateStripeAccount((req.params as { id: string }).id, parsed.data); if (!acct) return reply.code(404).send({ error: 'Account not found.' }); + // Which FIELDS changed, never their values — a key must never reach the log. + audit(req, 'stripe.account.update', acct.id, 'changed ' + (Object.keys(parsed.data).join(', ') || 'nothing') + ' on a Stripe account'); const verify = acct.secretKey ? await verifySecretKey(acct.secretKey) : undefined; return { data: { ...publicAccount(acct), verify } }; }); app.delete('/api/admin/stripe-accounts/:id', { preHandler: requireAdmin }, async (req, reply) => { const res = store.deleteStripeAccount((req.params as { id: string }).id); if (!res.ok) return reply.code(409).send({ error: 'A campaign uses this account. Reassign or delete those campaigns first.' }); + audit(req, 'stripe.account.delete', (req.params as { id: string }).id, 'removed a Stripe account'); return { data: { ok: true } }; }); app.post('/api/admin/stripe-accounts/:id/test', { preHandler: requireAdmin }, async (req) => { @@ -797,6 +882,10 @@ async function main(): Promise { return { data: adminCampaign(c) }; }); app.delete('/api/admin/campaigns/:id', { preHandler: requireAdmin }, async (req) => { + const doomed = store.getCampaign((req.params as { id: string }).id); + // Donations keep their campaign_id but the title is gone, so the ledger shows "Deleted + // campaign" for ever. Record the title while we still have it. + audit(req, 'campaign.delete', doomed?.id ?? '', doomed ? 'deleted the campaign "' + doomed.title + '"' : 'deleted a campaign that no longer existed'); store.deleteCampaign((req.params as { id: string }).id); return { data: { ok: true } }; }); @@ -812,7 +901,18 @@ async function main(): Promise { // A short, human-friendly transaction reference derived from the donation id // (stable + unique enough for display; the full id stays the real key). const donationRef = (id: string) => id.replace(/^don_/, '').slice(0, 8).toUpperCase(); - app.get('/api/admin/donations', { preHandler: requireAdmin }, async () => { + /** Donor records must never be cached — not by a browser, not by a shared proxy, and above all + * not by the Cloudflare edge when the admin has turned on public access. `.csv` is one of the + * extensions Cloudflare caches by default, and a response with no cache directives at a static + * extension is a candidate for the edge cache — after which the cached donor list can be served + * to a request carrying no session cookie at all. `vary: cookie` is belt-and-braces for any + * proxy that does key on it. Applies to the JSON log as well as the export: same data. */ + const noStoreDonorData = (reply: import('fastify').FastifyReply) => { + reply.header('cache-control', 'no-store, private, max-age=0').header('pragma', 'no-cache').header('vary', 'cookie'); + }; + + app.get('/api/admin/donations', { preHandler: requireAdmin }, async (_req, reply) => { + noStoreDonorData(reply); const titles = new Map(store.listCampaigns().map((c) => [c.id, c.title])); const list = store.listDonations(); const succeeded = list.filter((d) => d.status === 'succeeded'); @@ -823,7 +923,10 @@ async function main(): Promise { }, }; }); - app.get('/api/admin/donations.csv', { preHandler: requireAdmin }, async (_req, reply) => { + app.get('/api/admin/donations.csv', { preHandler: requireAdmin }, async (req, reply) => { + // The one action that takes every donor's name and email OFF the box. Logged first, so the + // record exists even if the response never finishes. + audit(req, 'donations.export', '', 'exported the donation ledger as CSV'); const titles = new Map(store.listCampaigns().map((c) => [c.id, c.title])); const rows = [['Ref', 'Date', 'Campaign', 'Amount', 'Currency', 'Status', 'Donor', 'Email', 'Card', 'Covered fees', 'PaymentIntent']]; for (const d of store.listDonations()) { @@ -833,6 +936,7 @@ async function main(): Promise { d.donorName, d.donorEmail, card, d.coverFees ? 'yes' : 'no', d.paymentIntentId, ]); } + noStoreDonorData(reply); reply.header('content-type', 'text/csv; charset=utf-8').header('content-disposition', 'attachment; filename="donations.csv"'); return rows.map((r) => r.map(csvCell).join(',')).join('\r\n'); }); @@ -1077,8 +1181,11 @@ async function main(): Promise { }; app.get('/api/admin/plans', { preHandler: requireAdmin }, async (req) => { - const force = (req.query as { refresh?: string }).refresh === '1'; const ownPage = ownPageFetch(req); + // `refresh=1` must ALSO be gated on the same-origin check, not just the write side: forcing the + // cache open is what turns one cross-site navigation into up to 200 outbound Stripe calls, so + // gating only the writes left the amplification the guard exists to stop (DONATIONS-018). + const force = (req.query as { refresh?: string }).refresh === '1' && ownPage; const seeds = planSeeds(); // newest first // Which plans get a live refresh, in which order. Plans that have taken money go FIRST: // a recurring donation row is written at /intent, BEFORE the card is entered, so every @@ -1169,8 +1276,8 @@ async function main(): Promise { const id = (req.params as { id: string }).id; const seed = findSeed(planSeeds(), id); if (!seed) return reply.code(404).send({ error: 'Unknown plan.' }); - const force = (req.query as { refresh?: string }).refresh === '1'; const ownPage = ownPageFetch(req); + const force = (req.query as { refresh?: string }).refresh === '1' && ownPage; // see DONATIONS-018 const r = await syncPlan(seed, force, ownPage); // The sync only lists invoices when new money can have landed; the detail window always // wants them, so fetch them here when it didn't (but only if Stripe answered at all). @@ -1228,6 +1335,7 @@ async function main(): Promise { const acct = await accountById(seed.stripeAccountId); if (!acct?.secretKey) return reply.code(502).send({ error: STRIPE_PLAN_NO_KEYS }); if (!(await pausePlan(acct.secretKey, seed.subscriptionId))) return reply.code(502).send({ error: STRIPE_PLAN_DOWN }); + audit(req, 'plan.pause', seed.subscriptionId, 'paused a monthly donation plan'); return { data: { plan: await planNow(seed) } }; }); @@ -1238,6 +1346,7 @@ async function main(): Promise { const acct = await accountById(seed.stripeAccountId); if (!acct?.secretKey) return reply.code(502).send({ error: STRIPE_PLAN_NO_KEYS }); if (!(await resumePlan(acct.secretKey, seed.subscriptionId))) return reply.code(502).send({ error: STRIPE_PLAN_DOWN }); + audit(req, 'plan.resume', seed.subscriptionId, 'resumed a monthly donation plan'); return { data: { plan: await planNow(seed) } }; }); @@ -1253,6 +1362,7 @@ async function main(): Promise { const acct = await accountById(seed.stripeAccountId); if (!acct?.secretKey) return reply.code(502).send({ error: STRIPE_PLAN_NO_KEYS }); if (!(await cancelPlan(acct.secretKey, seed.subscriptionId))) return reply.code(502).send({ error: STRIPE_PLAN_DOWN }); + audit(req, 'plan.stop', seed.subscriptionId, 'stopped a monthly donation plan for good'); return { data: { plan: await planNow(seed) } }; }); @@ -1313,6 +1423,7 @@ async function main(): Promise { } if (!(await setPlanEnd(acct.secretKey, seed.subscriptionId, cancelAt))) return reply.code(502).send({ error: STRIPE_PLAN_DOWN }); + audit(req, 'plan.schedule', seed.subscriptionId, cancelAt ? 'set when a monthly plan ends' : 'removed a monthly plan end date'); return { data: { plan: await planNow(seed) } }; }); @@ -1547,6 +1658,12 @@ async function main(): Promise { if (monthly && (!donorName.trim() || !donorEmail.trim())) { return reply.code(400).send({ error: 'Please add your name and email — both are required for a monthly donation.' }); } + // A monthly intent is five persistent Stripe objects, not one. Checked here — after the campaign + // and amount are known but BEFORE anything is created at Stripe — so a burst can't bloat the + // masjid's account (DONATIONS-010). The general 30/min intent limit above still applies too. + if (monthly && !monthlyIntentRateOk(peerOf(req))) { + return reply.code(429).send({ error: 'Too many monthly sign-ups from here just now. Please try again in a minute.' }); + } const metadata = { app: 'donations', campaignId: c.id, campaign: c.title.slice(0, 120), coverFees: String(coverFees), giftAid: String(giftAid), recurring: String(monthly), @@ -2033,6 +2150,7 @@ async function main(): Promise { // charges (invoice.paid on renewal) and resiliently confirms one-time payments. // The signature is verified with the account's own webhook secret. app.post('/api/stripe/webhook/:accountId', async (req, reply) => { + if (!webhookRateOk(peerOf(req))) return reply.code(429).send({ error: 'Too many requests.' }); const acct = await accountById((req.params as { accountId: string }).accountId); if (!acct || !acct.webhookSecret) return reply.code(400).send({ error: 'Webhook not configured.' }); const sig = req.headers['stripe-signature']; @@ -2216,6 +2334,24 @@ async function main(): Promise { // `check` first so we never double-record, then re-`record-payment`. Students' daily // reconciliation is the FINAL backstop (it scans succeeded students-billing PIs), so this is // an optimization — money is never lost even if this never runs. Embedded only. + /** Wrap a periodic pass so it can never overlap itself (DONATIONS-052). + * + * Both outboxes below run every 60s and each item makes a network call with an 8s timeout, so a + * slow provider makes a pass outlast its own interval. Without this, a second pass starts, reads + * the SAME still-pending rows, and does the work again — a duplicate receipt in the donor's inbox + * and, on the tuition side, a second `record-payment` attempt for one charge. Skipping the + * overlapping tick loses nothing: the rows are still pending, so the next tick picks them up. */ + const nonOverlapping = (pass: () => Promise): (() => void) => { + let running = false; + return () => { + if (running) return; + running = true; + void pass().finally(() => { + running = false; + }); + }; + }; + if (billingConfigured()) { const outbox = async () => { try { @@ -2230,7 +2366,68 @@ async function main(): Promise { } } catch { /* fail soft — never let the outbox crash the app */ } }; - const iv = setInterval(() => void outbox(), 60_000); + const iv = setInterval(nonOverlapping(outbox), 60_000); + iv.unref?.(); + } + + // ── Lost-donation sweep (DONATIONS-002) ───────────────────────────────────── + // A one-time card payment is marked succeeded ONLY by the donor's own /confirm callback. Close the + // tab, lose signal, or have the box briefly unreachable in the window between Stripe confirming + // and that call, and the money is taken while our row stays 'pending' for ever: absent from the + // ledger, the CSV, metrics(), the goal bar and any Gift Aid claim, with no receipt sent, and + // indistinguishable from an abandoned checkout so nobody ever looks. The optional webhook covers + // this only for masjids that configured one, which needs public ingress. + // + // So we ask Stripe, on the same retrieve-on-demand doctrine the monthly plans already use + // (CLAUDE.md §5). Deliberately conservative: + // • only ever promotes pending → succeeded. A PI that Stripe says failed or was abandoned is + // LEFT pending rather than marked failed — "we don't know" is honest, and writing 'failed' + // from a transient read would be a lie in the ledger. + // • a floor on age (5 min) so we never race the donor's own confirm and double-send a receipt. + // • a ceiling (30 days) because a PI that old will not suddenly settle, and Stripe's own + // retention makes the read pointless. + // • the receipt goes out through the SAME 'pending' → sendDonationReceipt path as a normal + // confirm, so the "Stripe's receipt or ours, never both" decision made at intent still holds. + // • one alert per recovered donation, because it IS news — the masjid was never told. + // • the whole pass is wrapped, bounded to 25 rows, and stops on the first unreachable account. + const SWEEP_MIN_AGE_MS = 5 * 60_000; + const SWEEP_MAX_AGE_MS = 30 * 24 * 3600_000; + const lostDonationSweep = async () => { + try { + for (const don of store.listUnconfirmedDonations(SWEEP_MIN_AGE_MS, SWEEP_MAX_AGE_MS)) { + const acct = await accountById(don.stripeAccountId); + if (!acct?.secretKey) continue; // keys gone for this account — nothing we can do, keep the row + const pi = await retrievePaymentIntent(acct, don.paymentIntentId); + if (!pi) break; // Stripe unreachable — stop the pass, try again next tick + if (pi.status !== 'succeeded') continue; // not paid (or not yet) — leave it pending + const updated = store.markDonation(don.paymentIntentId, 'succeeded', { + donorName: pi.billingName || don.donorName, + donorEmail: pi.receiptEmail || don.donorEmail, + cardBrand: pi.cardBrand, + cardLast4: pi.cardLast4, + }); + const camp = store.getCampaign(don.campaignId); + log.warn(`recovered a donation Stripe took but we never recorded (${don.paymentIntentId})`); + void notify({ + title: 'Donation recovered', + text: `A donation of ${formatMoney(pi.amount, pi.currency)} to “${camp?.title ?? 'your masjid'}” had been paid but not recorded — it is now in your donations.`, + level: 'success', + }).catch(() => {}); + if (don.receipt === 'pending') { + void sendDonationReceipt(updated ?? don) + .then((r) => { + if (r.sent) store.setDonationReceipt(don.paymentIntentId, 'sent'); + else if (!r.retry) store.setDonationReceipt(don.paymentIntentId, 'skipped'); + }) + .catch(() => {}); + } + } + } catch { /* fail soft — a sweep must never crash the app */ } + }; + { + // Every 10 minutes: frequent enough that a donor's receipt is only minutes late, rare enough + // that an idle masjid makes almost no outbound calls. + const iv = setInterval(nonOverlapping(lostDonationSweep), 10 * 60_000); iv.unref?.(); } @@ -2249,11 +2446,34 @@ async function main(): Promise { } } catch { /* fail soft — never let the receipt outbox crash the app */ } }; - const iv = setInterval(() => void receiptOutbox(), 60_000); + const iv = setInterval(nonOverlapping(receiptOutbox), 60_000); iv.unref?.(); } } +// ── Process-level fault handling (DONATIONS-029) ────────────────────────────── +// This runs unattended on a Raspberry Pi for months, and the codebase deliberately uses +// fire-and-forget `void fn().catch(...)` in a lot of places. Node's default for an unhandled +// rejection is to KILL THE PROCESS — so one missed `.catch()` in a non-critical background path +// (an alert, a receipt, a plan sync) would take the donation page down with it. +// +// The two cases are treated differently on purpose: +// • unhandledRejection — log it loudly and KEEP SERVING. A stray rejection here means a +// background promise nobody awaited; the donor-facing routes are unaffected, and staying up is +// strictly better for the masjid than a restart loop. It is logged at error level, with the +// message only, so it still gets found. +// • uncaughtException — log and EXIT. A synchronous throw that escaped every handler means the +// process is in an unknown state, and `restart: unless-stopped` in compose will bring back a +// clean one within seconds. Continuing in an unknown state around money is the worse option. +// Both log the MESSAGE only, never the error object, so a thrown Stripe error can't spill a key. +process.on('unhandledRejection', (reason) => { + log.error('unhandled promise rejection (still serving)', reason instanceof Error ? reason.message : String(reason)); +}); +process.on('uncaughtException', (err) => { + log.error('uncaught exception — exiting so the container restarts clean', err instanceof Error ? err.message : String(err)); + process.exit(1); +}); + main().catch((err) => { // Log the message only (not the whole error object) so a future thrown error // can't spill a key or connection string into the logs. diff --git a/server/src/rateLimit.test.ts b/server/src/rateLimit.test.ts new file mode 100644 index 0000000..eb05f45 --- /dev/null +++ b/server/src/rateLimit.test.ts @@ -0,0 +1,84 @@ +// SPDX-License-Identifier: AGPL-3.0-only +// Copyright (C) 2026 OpenMasjid-Solutions +// +// Locks the login brute-force limiter — the only thing standing behind a short admin password on a +// masjid LAN. Two properties matter and both were broken or untested before the 2026-08-03 audit: +// +// 1. The backoff itself: five free attempts, then a growing lockout, cleared by a success. +// 2. The sweep actually sweeps (DONATIONS-017). Its old condition could never be true, so the map +// grew one entry per attacking IP for the life of the process — and the naive fix (evict +// anything old) would have handed an attacker a fresh allowance every ten minutes, so the test +// pins BOTH halves: idle entries go, locked-out entries stay. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { LoginLimiter } from './rateLimit'; + +const HOUR = 3_600_000; + +test('LoginLimiter: five free attempts, then a lockout that grows', () => { + const lim = new LoginLimiter(); + for (let i = 0; i < 5; i++) { + assert.equal(lim.retryAfterMs('1.2.3.4'), 0, `attempt ${i + 1} of the five free ones`); + lim.fail('1.2.3.4'); + } + assert.equal(lim.retryAfterMs('1.2.3.4'), 0, 'the fifth failure has not locked us out yet'); + lim.fail('1.2.3.4'); + const first = lim.retryAfterMs('1.2.3.4'); + assert.ok(first > 0 && first <= 2000, `sixth failure starts the backoff (got ${first}ms)`); + lim.fail('1.2.3.4'); + assert.ok(lim.retryAfterMs('1.2.3.4') > first, 'and the next one is longer'); +}); + +test('LoginLimiter: a peer is only ever limited on its own attempts', () => { + const lim = new LoginLimiter(); + for (let i = 0; i < 10; i++) lim.fail('10.0.0.1'); + assert.ok(lim.retryAfterMs('10.0.0.1') > 0, 'the attacker is locked out'); + assert.equal(lim.retryAfterMs('10.0.0.2'), 0, 'a different peer is untouched'); +}); + +test('LoginLimiter: a successful sign-in forgets the failures', () => { + const lim = new LoginLimiter(); + for (let i = 0; i < 8; i++) lim.fail('10.0.0.3'); + assert.ok(lim.retryAfterMs('10.0.0.3') > 0); + lim.succeed('10.0.0.3'); + assert.equal(lim.retryAfterMs('10.0.0.3'), 0, 'the admin got in, so the slate is clean'); + assert.equal(lim.size(), 0, 'and nothing is left behind'); +}); + +// ── DONATIONS-017 ──────────────────────────────────────────────────────────── +// This is the regression test proper. Against the pre-fix code the first assertion fails: the old +// sweep condition (`lockedUntil < now - 1h && fails === 0`) was unsatisfiable, because an entry +// only exists after fail() has already pushed `fails` to >= 1. +test('LoginLimiter: the sweep drops idle peers (the map is not unbounded)', () => { + const lim = new LoginLimiter(); + for (const ip of ['1.1.1.1', '2.2.2.2', '3.3.3.3']) lim.fail(ip); + assert.equal(lim.size(), 3, 'three peers remembered'); + + lim.sweepNow(Date.now() + 10_000); // ten seconds later: far too soon to forget anyone + assert.equal(lim.size(), 3, 'a recent failure is still remembered'); + + lim.sweepNow(Date.now() + HOUR + 60_000); // an hour and a minute later + assert.equal(lim.size(), 0, 'idle peers are forgotten, so the map cannot grow for ever'); +}); + +test('LoginLimiter: a sweep during an active lockout does not release the attacker', () => { + const lim = new LoginLimiter(); + // 12 failures → a lockout, capped at 5 minutes. + for (let i = 0; i < 12; i++) lim.fail('9.9.9.9'); + assert.ok(lim.retryAfterMs('9.9.9.9') > 0, 'attacker is locked out'); + + // A sweep landing mid-lockout must leave it alone. Losing the entry here would hand the attacker + // five fresh attempts every ten minutes — a rate-limit bypass introduced BY the fix, which is the + // failure mode worth pinning. + lim.sweepNow(Date.now() + 60_000); + assert.equal(lim.size(), 1, 'the locked-out peer survives the sweep'); + assert.ok(lim.retryAfterMs('9.9.9.9') > 0, 'and is still locked out'); + + // Honest note on the guard's reach: because the idle window (1h) is far longer than the longest + // single lockout (5 min), `lockedUntil <= now` can never be the deciding clause in practice — by + // the time a peer is idle enough to sweep, its lockout has long expired. It is kept as + // belt-and-braces in case either constant is ever changed, and this test documents that a peer + // released by the sweep is only ever one whose lockout had ALREADY run out: + lim.sweepNow(Date.now() + HOUR + 60_000); + assert.equal(lim.size(), 0, 'an hour idle and no longer locked → forgotten'); +}); diff --git a/server/src/rateLimit.ts b/server/src/rateLimit.ts index b7fa43c..da55f48 100644 --- a/server/src/rateLimit.ts +++ b/server/src/rateLimit.ts @@ -9,23 +9,52 @@ interface Entry { fails: number; lockedUntil: number; + /** When we last saw a failure from this peer — what the sweep ages entries out on. */ + seen: number; } const MAX_FREE = 5; // attempts before backoff kicks in const BASE_MS = 2000; // first lockout step const MAX_MS = 5 * 60 * 1000; // cap a single lockout at 5 minutes +/** Forget a peer that has not failed for this long. Longer than the longest single lockout + * (5 min) by a wide margin, so ageing an entry out can never shorten a live lockout. */ +const IDLE_MS = 60 * 60 * 1000; export class LoginLimiter { private readonly map = new Map(); constructor() { - const sweep = setInterval(() => { - const now = Date.now(); - for (const [k, e] of this.map) if (e.lockedUntil < now - 3_600_000 && e.fails === 0) this.map.delete(k); - }, 10 * 60 * 1000); + const sweep = setInterval(() => this.sweep(), 10 * 60 * 1000); sweep.unref?.(); } + /** Drop peers that are not currently locked out and have not failed for an hour. + * + * The previous condition (`lockedUntil < now - 1h && fails === 0`) could never be true: an + * entry only exists after `fail()`, which always increments `fails` to at least 1, and + * `succeed()` deletes the entry outright — so nothing was ever swept and the map grew for the + * life of the process, one entry per attacking IP. Exposed publicly when the box is tunnelled. + * + * Both conditions matter. `lockedUntil <= now` means we never evict a peer mid-lockout (that + * would hand an attacker a fresh allowance every sweep — a rate-limit bypass introduced by the + * fix). `seen` ageing means an honest admin who mistyped their password twice last month is not + * remembered for ever, which is the same forgiveness the exponential backoff already implies. */ + private sweep(now = Date.now()): void { + for (const [k, e] of this.map) { + if (e.lockedUntil <= now && now - e.seen > IDLE_MS) this.map.delete(k); + } + } + + /** Entries currently held. Exposed for the regression test that pins the sweep. */ + size(): number { + return this.map.size; + } + + /** Run the sweep now, with an injectable clock (tests; the interval uses the real one). */ + sweepNow(now = Date.now()): void { + this.sweep(now); + } + /** ms the caller must wait before another attempt (0 = allowed now). */ retryAfterMs(ip: string): number { const e = this.map.get(ip); @@ -35,7 +64,8 @@ export class LoginLimiter { } fail(ip: string): void { - const e = this.map.get(ip) ?? { fails: 0, lockedUntil: 0 }; + const e = this.map.get(ip) ?? { fails: 0, lockedUntil: 0, seen: 0 }; + e.seen = Date.now(); e.fails += 1; if (e.fails > MAX_FREE) { const step = Math.min(MAX_MS, BASE_MS * 2 ** (e.fails - MAX_FREE - 1)); diff --git a/server/src/store.test.ts b/server/src/store.test.ts index 516bf23..82b0ef1 100644 --- a/server/src/store.test.ts +++ b/server/src/store.test.ts @@ -228,3 +228,171 @@ test('large-donation clamps the threshold, caps the message, and allowlists qrIm assert.equal(s.setLargeDonation({ threshold: 25000, qrImage: 'https://ex.org/qr.png' }).qrImage, 'https://ex.org/qr.png', 'https accepted'); assert.equal(s.getLargeDonation().threshold, 25000); }); + +// ── DONATIONS-011: the admin audit log ─────────────────────────────────────── +// A money app must be able to answer "who exported the donor list / cancelled that plan / rotated +// the Stripe key, and when". These tests pin the shape and, more importantly, the things that must +// NEVER end up in it. + +test('audit log: records an action and reads it back newest-first', () => { + const s = fresh(); + s.recordAudit('donations.export', { actor: 'imam', detail: 'exported the donation ledger as CSV' }); + s.recordAudit('plan.stop', { actor: 'imam', subject: 'sub_abc', detail: 'stopped a monthly donation plan for good' }); + const rows = s.listAudit(); + assert.equal(rows.length, 2); + assert.equal(rows[0].action, 'plan.stop', 'newest first'); + assert.equal(rows[0].subject, 'sub_abc'); + assert.equal(rows[0].actor, 'imam'); + assert.ok(rows[0].at.endsWith('Z'), 'timestamped in ISO/UTC'); + assert.equal(rows[1].action, 'donations.export'); +}); + +test('audit log: an empty log is an empty list, never a crash', () => { + assert.deepEqual(fresh().listAudit(), []); +}); + +test('audit log: fields are length-capped so one row cannot be used to bloat the volume', () => { + const s = fresh(); + s.recordAudit('x'.repeat(500), { actor: 'a'.repeat(500), subject: 'b'.repeat(500), detail: 'c'.repeat(2000) }); + const [row] = s.listAudit(); + assert.equal(row.action.length, 60); + assert.equal(row.actor.length, 120); + assert.equal(row.subject.length, 120); + assert.equal(row.detail.length, 300); +}); + +test('audit log: the limit is bounded and sane', () => { + const s = fresh(); + for (let i = 0; i < 20; i++) s.recordAudit('plan.pause', { subject: `sub_${i}` }); + assert.equal(s.listAudit(5).length, 5); + assert.equal(s.listAudit(0).length, 1, 'a zero/negative limit is clamped to at least one'); + assert.equal(s.listAudit(99_999).length, 20, 'an absurd limit is clamped, not passed to SQLite'); +}); + +test('audit log: is append-only in practice — nothing in the app updates or deletes a row', () => { + // Guards the invariant by construction: the Store exposes no mutator for audit_log. + const s = fresh(); + s.recordAudit('plan.stop', { subject: 'sub_1' }); + const keys = Object.getOwnPropertyNames(Object.getPrototypeOf(s)); + const mutators = keys.filter((k) => /audit/i.test(k) && !['recordAudit', 'listAudit'].includes(k)); + assert.deepEqual(mutators, [], `no audit mutator may exist, found: ${mutators.join(', ')}`); +}); + +test('audit log: a Stripe key never reaches it — the update entry names FIELDS, not values', () => { + // The route logs Object.keys(patch), so this pins the contract the route relies on: whatever the + // admin submitted, only field NAMES are recorded. A regression that logged the patch itself would + // put a live secret key on disk in cleartext, outside the 0600 database's own columns. + const s = fresh(); + const patch = { secretKey: 'sk_live_51ABCDEFghijklmnop', publishableKey: 'pk_live_51ABCDEF' }; + s.recordAudit('stripe.account.update', { subject: 'acct_1', detail: `changed ${Object.keys(patch).join(', ')} on a Stripe account` }); + const [row] = s.listAudit(); + const blob = JSON.stringify(row); + assert.ok(!blob.includes('sk_live'), 'no secret key'); + assert.ok(!blob.includes('pk_live_51ABCDEF'), 'no publishable key value either'); + assert.ok(row.detail.includes('secretKey'), 'the field name is what is recorded'); +}); + +test('audit log: ordering is insertion order, so same-millisecond actions still read in sequence', () => { + // Two actions inside one millisecond share an `at`, and the id is random hex — ordering on the + // timestamp returned them arbitrarily. This pins the rowid ordering that replaced it, and would + // also catch a regression to `ORDER BY at` if the clock ever stepped backwards. + const s = fresh(); + for (let i = 0; i < 25; i++) s.recordAudit('plan.pause', { subject: `sub_${i}` }); + const rows = s.listAudit(); + assert.deepEqual( + rows.map((r) => r.subject), + Array.from({ length: 25 }, (_, i) => `sub_${24 - i}`), + 'strict reverse insertion order', + ); +}); + +// ── DONATIONS-002: the lost-donation sweep's query ─────────────────────────── +// The sweep asks Stripe about pending one-time donations. Which rows it selects IS the safety +// property: too eager and it races the donor's own /confirm and double-sends a receipt; too narrow +// and the money stays lost. + +const donation = (s: Store, over: Record = {}) => + s.createDonation({ + campaignId: 'cmp_1', + stripeAccountId: 'acct_1', + amount: 1000, + currency: 'GBP', + status: 'pending', + donorName: '', + donorEmail: '', + coverFees: false, + giftAid: false, + paymentIntentId: 'pi_' + Math.random().toString(16).slice(2), + ...over, + } as Parameters[0]); + +const MIN = 5 * 60_000; +const MAX = 30 * 24 * 3600_000; +const ago = (ms: number) => new Date(Date.now() - ms).toISOString(); + +test('sweep query: picks up a pending one-time donation old enough to be abandoned', () => { + const s = fresh(); + donation(s, { paymentIntentId: 'pi_lost', createdAt: ago(60 * 60_000) }); + const found = s.listUnconfirmedDonations(MIN, MAX); + assert.equal(found.length, 1); + assert.equal(found[0].paymentIntentId, 'pi_lost'); +}); + +test('sweep query: will NOT race the donor — a row younger than the floor is left alone', () => { + // The donor may still be on the Stripe redirect. Touching this row could send two receipts. + const s = fresh(); + donation(s, { paymentIntentId: 'pi_inflight', createdAt: ago(30_000) }); + assert.deepEqual(s.listUnconfirmedDonations(MIN, MAX), []); +}); + +test('sweep query: ignores rows past the ceiling — an ancient PI will not settle now', () => { + const s = fresh(); + donation(s, { paymentIntentId: 'pi_ancient', createdAt: ago(120 * 24 * 3600_000) }); + assert.deepEqual(s.listUnconfirmedDonations(MIN, MAX), []); +}); + +test('sweep query: never touches a settled, failed, monthly or PI-less row', () => { + const s = fresh(); + const old = ago(60 * 60_000); + donation(s, { paymentIntentId: 'pi_done', status: 'succeeded', createdAt: old }); + donation(s, { paymentIntentId: 'pi_failed', status: 'failed', createdAt: old }); + // Monthly plans have their own reconciliation; sweeping them here would duplicate that work. + donation(s, { paymentIntentId: 'pi_monthly', recurring: true, subscriptionId: 'sub_1', createdAt: old }); + donation(s, { paymentIntentId: '', createdAt: old }); + assert.deepEqual(s.listUnconfirmedDonations(MIN, MAX), [], 'nothing in this set is sweepable'); +}); + +test('sweep query: tuition can never appear — it is not in the donations table at all', () => { + // Structural, not filtered: a tuition payment is written to student_payments (§13 route + // isolation), so there is no donations row for the sweep to find. Asserted against a real DB. + const s = fresh(); + s.createStudentPayment({ + campaignId: 'cmp_tuition', + stripeAccountId: 'acct_1', + paymentIntentId: 'pi_tuition', + familyId: 'fam_1', + studentId: 'stu_1', + familyLabel: 'The Yusuf family', + amount: 35_000, + currency: 'GBP', + allocations: '', + studentsSplit: '', + paymentLines: '', + } as Parameters[0]); + donation(s, { paymentIntentId: 'pi_real', createdAt: ago(60 * 60_000) }); + const found = s.listUnconfirmedDonations(MIN, MAX); + assert.deepEqual(found.map((d) => d.paymentIntentId), ['pi_real']); + assert.ok(!JSON.stringify(found).includes('pi_tuition')); + assert.ok(!JSON.stringify(found).includes('fam_1')); +}); + +test('sweep query: oldest first and bounded, so a backlog drains in arrival order', () => { + const s = fresh(); + for (let i = 0; i < 40; i++) donation(s, { paymentIntentId: `pi_${i}`, createdAt: ago((40 - i) * 3600_000) }); + const found = s.listUnconfirmedDonations(MIN, MAX, 25); + assert.equal(found.length, 25, 'bounded'); + assert.equal(found[0].paymentIntentId, 'pi_0', 'oldest first'); + for (let i = 1; i < found.length; i++) { + assert.ok(found[i - 1].createdAt <= found[i].createdAt, 'ascending by date'); + } +}); diff --git a/server/src/store.ts b/server/src/store.ts index c8c7d49..ae9aa1e 100644 --- a/server/src/store.ts +++ b/server/src/store.ts @@ -226,6 +226,22 @@ export interface StudentPayment { occurredAt: string; } +/** One line of the append-only admin audit log. See the `audit_log` DDL for what may go in it — + * in particular, never a key, a token, a Student ID or a donor's details. */ +export interface AuditEntry { + id: string; + /** ISO timestamp. */ + at: string; + /** Who did it, as the panel knows them: an OpenMasjidOS username, or 'local admin'. */ + actor: string; + /** A stable machine-ish verb, e.g. 'donations.export' or 'plan.cancel'. */ + action: string; + /** The id of the thing acted on ('' when not applicable). */ + subject: string; + /** A short human phrase for the panel to show. */ + detail: string; +} + /** Cloudflare Tunnel config. The token is a CREDENTIAL — server-side only, never * returned to the browser or logged. `publicHostname` is the public address the admin * set up in Cloudflare (e.g. give.masjid.org); it's not secret and is used to build @@ -356,12 +372,43 @@ export class Store { ); CREATE UNIQUE INDEX IF NOT EXISTS idx_student_payments_pi ON student_payments(payment_intent_id); CREATE INDEX IF NOT EXISTS idx_student_payments_outbox ON student_payments(pay_status, record_status); + + -- Append-only record of every admin action that touches money or donor data (DONATIONS-011). + -- This app handles donations, so "who exported the donor list, who cancelled that plan, who + -- rotated the Stripe key, and when" must be answerable — CLAUDE.md §8 promises the masjid a + -- financial record, and a second volunteer with panel access is in the threat model. + -- Deliberately NOT a general request log: no donor rows, no amounts, no PII beyond the actor + -- label the admin already sees, and never a key, token or Student ID. "detail" is a short + -- human phrase; "subject" is the id of the thing acted on so a row can be traced. + -- Nothing in the app ever UPDATEs or DELETEs from this table. + CREATE TABLE IF NOT EXISTS audit_log ( + id TEXT PRIMARY KEY, + at TEXT NOT NULL, + actor TEXT NOT NULL DEFAULT '', + action TEXT NOT NULL, + subject TEXT NOT NULL DEFAULT '', + detail TEXT NOT NULL DEFAULT '' + ); + CREATE INDEX IF NOT EXISTS idx_audit_log_at ON audit_log(at DESC); `); // Tighten file perms where the OS supports it (secrets + admin hash live here). - try { - fs.chmodSync(dbPath, 0o600); - } catch { - /* best-effort (e.g. Windows dev) */ + // + // The DIRECTORY is locked down too, and that is the part that matters: SQLite creates + // `donations.db-wal` and `-shm` sidecars itself, lazily, at default permissions — and in WAL + // mode the most recent committed data (including a freshly saved Stripe secret key) lives in + // the -wal file, not the 0600 database. chmod'ing the sidecars here would be a race, since they + // are recreated on demand; 0700 on the directory covers every current and future file in it + // (DONATIONS-028). Best-effort: a no-op on Windows dev boxes and on a volume the container does + // not own, hence the swallowed error and the info-level note rather than a hard failure. + for (const [target, mode] of [ + [path.dirname(dbPath), 0o700], + [dbPath, 0o600], + ] as const) { + try { + fs.chmodSync(target, mode); + } catch { + /* best-effort (e.g. Windows dev, or a volume we don't own) */ + } } // Add columns introduced after first release (CREATE TABLE IF NOT EXISTS won't). this.ensureColumn('campaigns', 'background_image', "TEXT NOT NULL DEFAULT ''"); @@ -1018,6 +1065,39 @@ export class Store { ).map((r) => this.rowToDonation(r)); } + // ── Audit log (append-only) ───────────────────────────────────────────────── + /** Record one admin action. Never throws: an audit write must not be able to fail the action it + * is describing (a masjid losing the ability to cancel a plan because a log insert failed would + * be a worse outcome than a missing log line — the failure is logged instead). */ + recordAudit(action: string, opts: { actor?: string; subject?: string; detail?: string } = {}): void { + try { + this.db + .prepare('INSERT INTO audit_log (id, at, actor, action, subject, detail) VALUES (?, ?, ?, ?, ?, ?)') + .run(rid('aud'), new Date().toISOString(), (opts.actor ?? '').slice(0, 120), action.slice(0, 60), (opts.subject ?? '').slice(0, 120), (opts.detail ?? '').slice(0, 300)); + } catch (e) { + log.warn(`couldn’t write the audit log: ${e instanceof Error ? e.message : 'error'}`); + } + } + + /** Most recent audit entries, newest first. + * + * Ordered by `rowid`, i.e. INSERTION order, not by the `at` timestamp. Two actions in the same + * millisecond share an `at`, and the id tie-break is random hex — so ordering on `at` returned + * them in an arbitrary order (caught by store.test.ts). Insertion order is also immune to the + * clock stepping backwards, which an unattended Pi syncing NTP after a long outage really does. */ + listAudit(limit = 200): AuditEntry[] { + return (this.db.prepare('SELECT * FROM audit_log ORDER BY rowid DESC LIMIT ?').all(Math.max(1, Math.min(1000, limit))) as Record[]).map( + (r) => ({ + id: String(r.id), + at: String(r.at), + actor: String(r.actor ?? ''), + action: String(r.action), + subject: String(r.subject ?? ''), + detail: String(r.detail ?? ''), + }), + ); + } + listDonations(): Donation[] { return (this.db.prepare('SELECT * FROM donations ORDER BY created_at DESC').all() as Record[]).map((r) => this.rowToDonation(r), @@ -1040,6 +1120,32 @@ export class Store { ).map((r) => this.rowToDonation(r)); } + /** One-time donations still sitting at 'pending', old enough that the donor's browser is never + * coming back, and young enough to be worth asking Stripe about (DONATIONS-002). + * + * A one-time payment is only marked succeeded by the donor's own `/confirm` callback, so a closed + * tab, a lost signal or a momentarily unreachable box leaves money taken at Stripe and NOTHING + * recorded here — missing from the ledger, the CSV, the totals and the goal bar, with no receipt, + * for ever. This feeds the sweep that asks Stripe about each one. + * + * `recurring = 0` because monthly plans have their own reconciliation (reconcileRenewals), which + * is both cheaper and more complete for them. Oldest first, so a backlog drains in the order the + * money arrived. The window has a floor as well as a ceiling: a row younger than `minAgeMs` may + * still be mid-confirmation, and racing the donor's own callback would double-send the receipt. */ + listUnconfirmedDonations(minAgeMs: number, maxAgeMs: number, limit = 25): Donation[] { + const now = Date.now(); + return ( + this.db + .prepare( + `SELECT * FROM donations + WHERE status = 'pending' AND recurring = 0 AND payment_intent_id <> '' + AND created_at <= ? AND created_at >= ? + ORDER BY created_at LIMIT ?`, + ) + .all(new Date(now - minAgeMs).toISOString(), new Date(now - maxAgeMs).toISOString(), limit) as Record[] + ).map((r) => this.rowToDonation(r)); + } + /** Total raised (succeeded) for a campaign, in minor units. */ raisedForCampaign(campaignId: string): number { return ( diff --git a/server/src/stripe.test.ts b/server/src/stripe.test.ts new file mode 100644 index 0000000..bda40eb --- /dev/null +++ b/server/src/stripe.test.ts @@ -0,0 +1,229 @@ +// SPDX-License-Identifier: AGPL-3.0-only +// Copyright (C) 2026 OpenMasjid-Solutions +// +// Locks the money conversions — every amount this app charges passes through them, and before the +// 2026-08-03 audit not one of them had a test (DONATIONS-044). +// +// IMPORTANT, so nobody is misled by a green run: some tests below assert the CURRENT behaviour of +// two known-wrong conversions rather than the correct answer, and say so at the assertion. They +// exist to make the wrongness visible and to fail loudly when it is fixed, because the fix changes +// what donors are charged and must be reconciled against Stripe by a human first: +// • DONATIONS-001 — the three-decimal currencies (BHD, JOD, KWD, OMR, TND) charge 1/10. +// • DONATIONS-008 — withCoveredFees drops the fixed fee for zero-decimal currencies. +// Both are documented in docs/audit/ACTION_REQUIRED.md. Do not "fix" a test here to make a red run +// green: if one of these fails, the arithmetic changed, and that is exactly what needs a human. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { + currencyDecimals, + toMinor, + toMajor, + withCoveredFees, + looksLikePublishable, + looksLikeSecret, + looksLikeWebhookSecret, + stripeMode, + stripeConfigured, + publicStripeStatus, +} from './stripe'; + +// ── currencyDecimals ───────────────────────────────────────────────────────── + +test('currencyDecimals: two decimals for ordinary currencies', () => { + for (const c of ['GBP', 'USD', 'EUR', 'CAD', 'AUD', 'PKR', 'INR', 'MYR', 'ZAR', 'AED', 'SAR']) { + assert.equal(currencyDecimals(c), 2, c); + } +}); + +test('currencyDecimals: Stripe’s sixteen zero-decimal currencies', () => { + // The full list per Stripe. If Stripe ever adds one, this test is where it should land. + const zero = ['BIF', 'CLP', 'DJF', 'GNF', 'JPY', 'KMF', 'KRW', 'MGA', 'PYG', 'RWF', 'UGX', 'VND', 'VUV', 'XAF', 'XOF', 'XPF']; + assert.equal(zero.length, 16); + for (const c of zero) assert.equal(currencyDecimals(c), 0, c); +}); + +test('currencyDecimals: case-insensitive, and an unknown code falls back to two', () => { + assert.equal(currencyDecimals('jpy'), 0); + assert.equal(currencyDecimals('Jpy'), 0); + assert.equal(currencyDecimals('ZZZ'), 2, 'an unknown code must not throw'); + assert.equal(currencyDecimals(''), 2); +}); + +test('currencyDecimals: DONATIONS-001 — the five three-decimal currencies', () => { + // Stripe quotes BHD/JOD/KWD/OMR/TND in thousandths (fils/millimes). Before the fix this returned + // 2, so a 10.000 KWD donation was sent as 1000 minor units — 1.000 KWD, a tenth of the amount the + // donor was shown, with the ledger recording the full figure. + const three = ['BHD', 'JOD', 'KWD', 'OMR', 'TND']; + assert.equal(three.length, 5); + for (const c of three) { + assert.equal(currencyDecimals(c), 3, c); + assert.equal(toMinor(10, c), 10_000, `${c}: 10.000 must charge 10.000, not 1.000`); + assert.equal(toMajor(10_000, c), 10, `${c}: and read back as 10.000`); + } +}); + +test('toMinor: DONATIONS-001 — a three-decimal amount is rounded to Stripe’s multiple of 10', () => { + // Stripe rejects a three-decimal amount that is not a multiple of ten, so this must round rather + // than let the charge fail in front of the donor. Nearest-10: rounding down would quietly shave + // the gift, rounding up would take more than was agreed. + assert.equal(toMinor(10.123, 'KWD'), 10_120, 'nearest 10 (down)'); + assert.equal(toMinor(10.126, 'KWD'), 10_130, 'nearest 10 (up)'); + assert.equal(toMinor(10.125, 'KWD') % 10, 0, 'the boundary is still a legal amount'); + for (const major of [1, 2.5, 7.777, 100, 1234.567]) { + assert.equal(toMinor(major, 'BHD') % 10, 0, `BHD ${major} must be a multiple of 10`); + } +}); + +test('withCoveredFees: DONATIONS-001 — the gross-up stays a legal three-decimal amount', () => { + // A gross-up that lands on a non-multiple-of-10 would be rejected by Stripe, and the donor would + // see a failure for an amount they never chose. + for (const net of [10_000, 25_000, 1_000, 999_990]) { + const gross = withCoveredFees(net, 'KWD'); + assert.equal(gross % 10, 0, `KWD gross-up of ${net} → ${gross} must be a multiple of 10`); + assert.ok(gross > net, 'and must still be an increase'); + } +}); + +// ── toMinor / toMajor ──────────────────────────────────────────────────────── + +test('toMinor: two-decimal amounts, including the values floats get wrong', () => { + assert.equal(toMinor(10, 'GBP'), 1000); + assert.equal(toMinor(10.5, 'GBP'), 1050); + assert.equal(toMinor(0.01, 'GBP'), 1); + assert.equal(toMinor(33.33, 'GBP'), 3333); + // 20.15 * 100 is 2014.9999999999998 in IEEE-754; Math.round is what saves it. + assert.equal(toMinor(20.15, 'GBP'), 2015, 'binary-float rounding must not lose a penny'); + assert.equal(toMinor(0.1 + 0.2, 'GBP'), 30, '0.30000000000000004 → 30'); + assert.equal(toMinor(1234.56, 'GBP'), 123456); +}); + +test('toMinor: zero-decimal amounts are already the smallest unit', () => { + assert.equal(toMinor(1000, 'JPY'), 1000); + assert.equal(toMinor(1, 'KRW'), 1); + // A fractional yen cannot exist; rounding is the only sane answer. + assert.equal(toMinor(1000.4, 'JPY'), 1000); + assert.equal(toMinor(1000.6, 'JPY'), 1001); +}); + +test('toMajor: inverts toMinor for both exponents', () => { + assert.equal(toMajor(1050, 'GBP'), 10.5); + assert.equal(toMajor(1, 'GBP'), 0.01); + assert.equal(toMajor(1000, 'JPY'), 1000); +}); + +test('toMinor/toMajor: a round-trip through the API boundary is lossless', () => { + // Amounts cross the API in MAJOR units, so every stored minor value makes this trip and back. + for (const minor of [1, 30, 500, 1050, 3333, 99_999, 123_456, 99_999_999]) { + assert.equal(toMinor(toMajor(minor, 'GBP'), 'GBP'), minor, `GBP ${minor}`); + } + for (const minor of [1, 100, 1000, 99_999_999]) { + assert.equal(toMinor(toMajor(minor, 'JPY'), 'JPY'), minor, `JPY ${minor}`); + } +}); + +test('toMinor: hostile inputs do not silently become a charge', () => { + // The route validates before reaching here (zod + Number.isInteger + floor/ceiling checks), so + // this documents what the conversion alone does with junk: NaN in, NaN out — never 0, which would + // be a free donation, and never a huge number, which would be a surprise charge. + assert.ok(Number.isNaN(toMinor(NaN, 'GBP'))); + assert.equal(toMinor(Infinity, 'GBP'), Infinity); + assert.equal(toMinor(-5, 'GBP'), -500, 'negatives pass through — the ROUTE must reject them'); +}); + +// ── withCoveredFees ────────────────────────────────────────────────────────── + +test('withCoveredFees: grosses up so the masjid nets ~the intended amount', () => { + // Model is 2.9% + 0.30. For £10.00: (1000 + 30) / (1 - 0.029) = 1060.76… → 1061. + const gross = withCoveredFees(1000, 'GBP'); + assert.equal(gross, 1061); + // Verify the point of the exercise: fee on the GROSS leaves the masjid with ~the original net. + const fee = Math.round(gross * 0.029) + 30; + assert.ok(Math.abs(gross - fee - 1000) <= 1, `net after fee should be ~1000, got ${gross - fee}`); +}); + +test('withCoveredFees: is always an increase, and monotonic', () => { + let prev = 0; + for (const net of [50, 100, 500, 1000, 5000, 100_000]) { + const gross = withCoveredFees(net, 'GBP'); + assert.ok(gross > net, `${net} → ${gross} must be higher`); + assert.ok(gross > prev); + prev = gross; + } +}); + +test('withCoveredFees: DONATIONS-008 — the fixed fee no longer vanishes for zero-decimal currencies', () => { + // toMinor(0.30, 'JPY') is Math.round(0.3 * 1) = 0, so the "+30c" half of the model used to + // disappear entirely and the gross-up under-recovered on every covered-fee donation. It is now + // floored at one minor unit — deliberately an approximation, not an FX conversion (see the comment + // on fixedFeeMinor); the point is that it is no longer zero. + const jpy = withCoveredFees(1000, 'JPY'); + assert.equal(jpy, Math.round((1000 + 1) / (1 - 0.029)), 'the fixed component is present'); + assert.ok(jpy > Math.round(1000 / (1 - 0.029)), 'and strictly more than percentage-only'); + // Two-decimal currencies are unchanged by the fix — this is the regression guard on the common path. + assert.equal(withCoveredFees(1000, 'GBP'), 1061); // (1000 + 30) / 0.971 = 1060.76 → 1061 + assert.equal(withCoveredFees(2500, 'USD'), 2606); // (2500 + 30) / 0.971 = 2605.56 → 2606 +}); + +// ── Key shape + mode detection ─────────────────────────────────────────────── + +test('key shapes: only real Stripe key formats are accepted', () => { + assert.ok(looksLikePublishable('pk_test_51AbCdEf')); + assert.ok(looksLikePublishable('pk_live_51AbCdEf')); + assert.ok(!looksLikePublishable('sk_test_51AbCdEf'), 'a SECRET key is not publishable'); + assert.ok(!looksLikePublishable('pk_test_'), 'prefix alone is not a key'); + assert.ok(!looksLikePublishable(' pk_test_51A'), 'no leading whitespace'); + assert.ok(!looksLikePublishable('pk_test_51A!'), 'no punctuation'); + + assert.ok(looksLikeSecret('sk_test_51AbCdEf')); + assert.ok(looksLikeSecret('sk_live_51AbCdEf')); + assert.ok(looksLikeSecret('rk_live_51AbCdEf'), 'restricted keys are legitimate'); + assert.ok(!looksLikeSecret('pk_live_51AbCdEf')); + + assert.ok(looksLikeWebhookSecret('whsec_AbCdEf123')); + assert.ok(!looksLikeWebhookSecret('whsec_')); + assert.ok(!looksLikeWebhookSecret('sk_test_51A')); +}); + +test('stripeMode: test vs live is read from the key prefix', () => { + assert.equal(stripeMode({ publishableKey: 'pk_test_1', secretKey: 'sk_test_1' }), 'test'); + assert.equal(stripeMode({ publishableKey: 'pk_live_1', secretKey: 'sk_live_1' }), 'live'); + assert.equal(stripeMode({ publishableKey: '', secretKey: '' }), 'unknown'); + // The SECRET key decides — it is the one that moves money. + assert.equal(stripeMode({ publishableKey: 'pk_test_1', secretKey: 'sk_live_1' }), 'live'); +}); + +test('stripeConfigured: needs a valid PAIR in the SAME mode', () => { + const cfg = (publishableKey: string, secretKey: string) => ({ publishableKey, secretKey, webhookSecret: '', id: 'a', label: 'a' }); + assert.equal(stripeConfigured(cfg('pk_test_1', 'sk_test_1')), true); + assert.equal(stripeConfigured(cfg('pk_live_1', 'sk_live_1')), true); + assert.equal(stripeConfigured(cfg('pk_test_1', 'sk_live_1')), false, 'mixed modes must not go live'); + assert.equal(stripeConfigured(cfg('pk_test_1', '')), false); + assert.equal(stripeConfigured(cfg('', 'sk_test_1')), false); + assert.equal(stripeConfigured(cfg('nonsense', 'sk_test_1')), false); +}); + +test('publicStripeStatus: NEVER returns the secret or the webhook secret', () => { + // This object is sent to the browser. The invariant is the whole point of the function. + const status = publicStripeStatus({ + publishableKey: 'pk_live_51PUBLISHABLE', + secretKey: 'sk_live_51SUPERSECRETVALUE', + webhookSecret: 'whsec_SUPERSECRETHOOK', + }); + const blob = JSON.stringify(status); + assert.ok(!blob.includes('sk_live'), 'the secret key must never cross to the browser'); + assert.ok(!blob.includes('SUPERSECRETVALUE')); + assert.ok(!blob.includes('whsec_'), 'nor the webhook secret'); + assert.ok(!blob.includes('SUPERSECRETHOOK')); + // What it MAY say: + assert.equal(status.publishableKey, 'pk_live_51PUBLISHABLE'); + assert.equal(status.hasSecretKey, true); + assert.equal(status.hasWebhookSecret, true); + assert.equal(status.mode, 'live'); + assert.equal(status.configured, true); +}); + +test('publicStripeStatus: flags a test/live key mismatch for the admin', () => { + const s = publicStripeStatus({ publishableKey: 'pk_test_1', secretKey: 'sk_live_1', webhookSecret: '' }); + assert.equal(s.keysMismatch, true); + assert.equal(s.configured, false); +}); diff --git a/server/src/stripe.ts b/server/src/stripe.ts index 6faa549..35b2e32 100644 --- a/server/src/stripe.ts +++ b/server/src/stripe.ts @@ -81,19 +81,42 @@ export async function verifySecretKey(secretKey: string): Promise<{ ok: boolean; } // ── Currency minor units ────────────────────────────────────────────────────── -// Stripe charges in the smallest currency unit. Most currencies have 2 decimals, -// but several are zero-decimal (the amount is already the smallest unit). +// Stripe charges in the smallest currency unit, and there are THREE exponents, not two. const ZERO_DECIMAL = new Set([ 'BIF', 'CLP', 'DJF', 'GNF', 'JPY', 'KMF', 'KRW', 'MGA', 'PYG', 'RWF', 'UGX', 'VND', 'VUV', 'XAF', 'XOF', 'XPF', ]); +/** Stripe's three-decimal currencies (DONATIONS-001). + * + * These are quoted in thousandths — fils for the Gulf dinars, millimes for the Tunisian dinar — + * and Stripe additionally requires the minor amount to be a MULTIPLE OF TEN, because the smallest + * coin in circulation is 5–10 thousandths. Treating them as two-decimal (which this file did until + * the 2026-08-03 audit) sent one tenth of the amount the donor was shown: a 10.000 KWD donation + * became 1000 minor units, i.e. 1.000 KWD. Both directions were wrong by the same factor, so the + * app's own ledger agreed with itself and only Stripe's dashboard told the truth. */ +const THREE_DECIMAL = new Set(['BHD', 'JOD', 'KWD', 'OMR', 'TND']); + export function currencyDecimals(currency: string): number { - return ZERO_DECIMAL.has(currency.toUpperCase()) ? 0 : 2; + const c = currency.toUpperCase(); + if (ZERO_DECIMAL.has(c)) return 0; + if (THREE_DECIMAL.has(c)) return 3; + return 2; +} + +/** True when Stripe requires the minor amount to be a multiple of 10 (the three-decimal set). */ +export function requiresMultipleOfTen(currency: string): boolean { + return THREE_DECIMAL.has(currency.toUpperCase()); } -/** Major units (e.g. 10.50) → minor units (1050), respecting zero-decimal currencies. */ +/** Major units (e.g. 10.50) → minor units (1050), respecting zero- and three-decimal currencies. + * + * For a three-decimal currency the result is rounded to the nearest 10, as Stripe requires — so a + * donor typing 10.123 KWD is charged 10.120. Rounding DOWN at the boundary would silently shave + * the donation, and rounding up would charge more than they agreed, so nearest-10 it is; the donor + * page shows the amount it will actually charge (see the `presetAmounts`/`minAmount` round-trip). */ export function toMinor(major: number, currency: string): number { - return Math.round(major * 10 ** currencyDecimals(currency)); + const minor = Math.round(major * 10 ** currencyDecimals(currency)); + return requiresMultipleOfTen(currency) ? Math.round(minor / 10) * 10 : minor; } /** Minor units → major (for display). */ @@ -106,9 +129,27 @@ export function toMajor(minor: number, currency: string): number { * (Stripe's real fee varies by card/country) shown transparently to the donor. */ const FEE_PCT = 0.029; // 2.9% const FEE_FIXED_MAJOR = 0.3; // + 30¢/30p -export function withCoveredFees(netMinor: number, currency: string): number { + +/** The fixed half of the fee, in minor units, with a floor of ONE minor unit for a zero-decimal + * currency (DONATIONS-008). + * + * `toMinor(0.30, 'JPY')` is `Math.round(0.3 * 1)` = **0**, so the "+30c" half of the fee model + * silently vanished for all sixteen zero-decimal currencies and the gross-up under-recovered on + * every covered-fee donation. There is no honest conversion of "30 US cents" into yen without an + * exchange rate we do not have and will not invent, so this is deliberately an approximation with + * a floor rather than a real conversion — the number was already an approximation (Stripe's true + * fee varies by card and country), and one minor unit is closer to the truth than zero. The real + * answer is a per-account, admin-visible fee model; see docs/audit/ACTION_REQUIRED.md. */ +function fixedFeeMinor(currency: string): number { const fixed = toMinor(FEE_FIXED_MAJOR, currency); - return Math.round((netMinor + fixed) / (1 - FEE_PCT)); + return currencyDecimals(currency) === 0 ? Math.max(1, fixed) : fixed; +} + +export function withCoveredFees(netMinor: number, currency: string): number { + const gross = Math.round((netMinor + fixedFeeMinor(currency)) / (1 - FEE_PCT)); + // Keep the three-decimal multiple-of-10 rule intact after the gross-up, or Stripe rejects the + // charge outright and the donor sees a failure for an amount they never chose. + return requiresMultipleOfTen(currency) ? Math.round(gross / 10) * 10 : gross; } // ── Payments ────────────────────────────────────────────────────────────────── diff --git a/web/package-lock.json b/web/package-lock.json index 60f8db4..0e6af1b 100644 --- a/web/package-lock.json +++ b/web/package-lock.json @@ -1,12 +1,12 @@ { "name": "openmasjid-donations-web", - "version": "0.14.0", + "version": "0.38.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "openmasjid-donations-web", - "version": "0.14.0", + "version": "0.38.0", "license": "AGPL-3.0-only", "dependencies": { "@stripe/react-stripe-js": "^3.1.1", @@ -2113,9 +2113,9 @@ } }, "node_modules/nanoid": { - "version": "3.3.15", - "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.15.tgz", - "integrity": "sha512-y7Wygv/7mEOvxTuEQDB8StXdMRBWf1kR/tlhAzBRUFkB2jfcLOAxO/SHmOO2zgz1pVgK29/kyupn059/bCHdjA==", + "version": "3.3.17", + "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.17.tgz", + "integrity": "sha512-xQLf0A3HOMlgHq0n247/LRuAOYmB7dXJ/DvAxGvsSBij45XtBSmQycu+F8ODbHwns/XyFZagyL1+J0Offw1E0g==", "dev": true, "funding": [ { @@ -2218,9 +2218,9 @@ } }, "node_modules/postcss": { - "version": "8.5.15", - "resolved": "https://registry.npmjs.org/postcss/-/postcss-8.5.15.tgz", - "integrity": "sha512-FfR8sjd4em2T6fb3I2MwAJU7HWVMr9zba+enmQeeWFfCbm+UOC/0X4DS8XtpUTMwWMGbjKYP7xjfNekzyGmB3A==", + "version": "8.5.25", + "resolved": "https://registry.npmjs.org/postcss/-/postcss-8.5.25.tgz", + "integrity": "sha512-DTPx3RWSSnWyzLxQnlH0rJP+EW5ekl16ZU4/psbIhA0e53kJfdgaN5vKM+xP7yJtXVu+nfdVFmlgFDEKAe4Pyw==", "dev": true, "funding": [ { @@ -2238,7 +2238,7 @@ ], "license": "MIT", "dependencies": { - "nanoid": "^3.3.12", + "nanoid": "^3.3.16", "picocolors": "^1.1.1", "source-map-js": "^1.2.1" },