From b0ee44018b3e3682818cf74f1bb3c7f7d13e5a7e Mon Sep 17 00:00:00 2001 From: Joycejay17 Date: Thu, 30 Jul 2026 06:42:04 +0100 Subject: [PATCH 1/5] docs(contracts): add end-to-end deployment and upgrade runbook The repository documented the pieces of a deployment across three files - CLI commands in docs/contracts/DEPLOYMENT.md, governance in UPGRADE_PROCESS.md, script configuration in contracts/deployment/README.md - but nothing gave the ordered procedure that ties them together. Adds docs/contracts/RUNBOOK.md covering, in order: the testnet/mainnet gate, deploy (build, hash, deploy, verify, init, transfer admin to multisig, record), upgrade (including what the timelock delay is actually for), WASM hash verification against the on-chain artifact, and a rollback table that states plainly which situations are recoverable and which are not. Maps each PR template Contract Upgrade Details field to the step it comes from. Cross-links the three existing documents to it. No scripts or contracts are changed. --- contracts/DEPLOYMENT.md | 6 +- contracts/deployment/README.md | 6 + docs/contracts/DEPLOYMENT.md | 8 +- docs/contracts/RUNBOOK.md | 315 ++++++++++++++++++++++++++++++ docs/contracts/UPGRADE_PROCESS.md | 4 + 5 files changed, 336 insertions(+), 3 deletions(-) create mode 100644 docs/contracts/RUNBOOK.md diff --git a/contracts/DEPLOYMENT.md b/contracts/DEPLOYMENT.md index 90fe1daea..8f772fb96 100644 --- a/contracts/DEPLOYMENT.md +++ b/contracts/DEPLOYMENT.md @@ -1,7 +1,9 @@ # Moved -Canonical deployment runbook: +Canonical deployment docs: -- `docs/contracts/DEPLOYMENT.md` +- `docs/contracts/RUNBOOK.md` — ordered deploy/upgrade procedure (start here) +- `docs/contracts/DEPLOYMENT.md` — Soroban CLI command reference +- `docs/contracts/UPGRADE_PROCESS.md` — upgrade governance This file is kept only as a compatibility pointer. diff --git a/contracts/deployment/README.md b/contracts/deployment/README.md index 95fb4b86a..c9ab44d66 100644 --- a/contracts/deployment/README.md +++ b/contracts/deployment/README.md @@ -2,6 +2,12 @@ > **EVM + Soroban** deployment scripts for the Shelterflex contract suite. +> **Doing a real deployment?** This README documents the scripts and their +> configuration. The ordered procedure around them — WASM hash verification, +> transferring admin authority to the multisig, the timelock, rollback limits, +> and the testnet/mainnet gate — is in +> [`docs/contracts/RUNBOOK.md`](../../docs/contracts/RUNBOOK.md). + --- ## Overview diff --git a/docs/contracts/DEPLOYMENT.md b/docs/contracts/DEPLOYMENT.md index e9230c2d3..6d8c7beed 100644 --- a/docs/contracts/DEPLOYMENT.md +++ b/docs/contracts/DEPLOYMENT.md @@ -1,6 +1,12 @@ # Contracts Deployment & IDs (Soroban CLI) -This runbook is the single source of truth for deploying the core contracts on Soroban testnet and wiring the resulting IDs into backend environment variables. +This document is the command reference for deploying the core contracts on Soroban testnet and wiring the resulting IDs into backend environment variables. + +> **Performing a deployment or an upgrade?** Start with +> [`RUNBOOK.md`](./RUNBOOK.md) — the ordered end-to-end procedure covering WASM +> verification, the multisig handover, the timelock, rollback limits and the +> testnet/mainnet gate. This file gives you the commands; the runbook gives you +> the order and the checks. It covers: diff --git a/docs/contracts/RUNBOOK.md b/docs/contracts/RUNBOOK.md new file mode 100644 index 000000000..7005b53c1 --- /dev/null +++ b/docs/contracts/RUNBOOK.md @@ -0,0 +1,315 @@ +# Contract Deployment & Upgrade Runbook + +**Status:** documentation only. This runbook describes the procedure using the +scripts and contracts that already exist — it does not introduce or change any +of them. + +The repository already documents the *pieces*: + +| Document | Covers | +| ----------------------------------------------------- | --------------------------------------------- | +| [`DEPLOYMENT.md`](./DEPLOYMENT.md) | Soroban CLI commands, identities, backend env | +| [`UPGRADE_PROCESS.md`](./UPGRADE_PROCESS.md) | Who may upgrade, PR governance | +| [`../../contracts/deployment/README.md`](../../contracts/deployment/README.md) | Multi-network config and the idempotent deploy script | + +This file is the **ordered procedure** that ties them together. A deployment is +performed rarely and cannot be undone, which is exactly the combination in which +a step gets skipped. Follow the steps in order and do not skip Step 3. + +--- + +## 0. The testnet/mainnet gate + +There is one gate, and it is not automated: + +> **Every contract ships to testnet first. Mainnet deployment is a maintainer +> action, never a contributor action.** + +| | Testnet | Mainnet | +| --- | --- | --- | +| Who runs it | Any contributor | Maintainers only | +| Config | `contracts/deployment/config/testnet.json` | `contracts/deployment/config/mainnet.json` | +| Network passphrase | `Test SDF Network ; September 2015` | `Public Global Stellar Network ; September 2015` | +| Funding | Friendbot | Real XLM | +| Admin authority | May be a single deployer key while iterating | **Must** be the multisig before the contract holds value | +| Prerequisite | none | A testnet deployment of the *same WASM hash*, exercised and reviewed | + +Contributor PRs describing a deployment should tick **Testnet** in the PR +template. If you believe a mainnet deployment is warranted, say so in the PR and +stop — do not deploy. + +--- + +## 1. Deploying a new contract + +### Step 1 — Build from a clean tree + +```bash +cd contracts +git status --porcelain # must be empty; you cannot verify a dirty build +cargo fmt --all -- --check +cargo clippy --workspace --all-targets --all-features +cargo test --workspace +bash scripts/build-wasm.sh # or: stellar contract build --out-dir contracts/artifacts +``` + +Record the commit SHA you built from. Every later step refers back to it. + +### Step 2 — Record the WASM hash *before* deploying + +```bash +sha256sum contracts/artifacts/.wasm +``` + +Write this value down now. Computing it after deployment defeats the point of +the check in Step 3 — you would be comparing the deployed artifact against +itself. + +### Step 3 — Deploy + +```bash +export STELLAR_SECRET_KEY="S..." +export STELLAR_DEPLOYER_ADDRESS="G..." + +stellar contract deploy \ + --wasm contracts/artifacts/.wasm \ + --source-account shelter_admin \ + --network testnet +``` + +Or, for the full suite, the idempotent script — it skips anything already +recorded in `deployed/{network}.json`: + +```bash +bash contracts/deployment/scripts/deploy-soroban.sh --network testnet +``` + +Record the returned contract ID (`C...`) and the deploy transaction hash. + +### Step 4 — Verify the deployed WASM matches what you built + +**This is the step most often skipped and the one that matters most.** See +[§3 Verification](#3-verification--proving-the-deployment-is-what-you-reviewed). +Do it before initializing, not after. + +### Step 5 — Initialize + +```bash +stellar contract invoke \ + --id "$CONTRACT_ID" --source-account shelter_admin --network testnet --send=yes \ + -- init --admin "$ADMIN_ADDR" ... +``` + +Initialization is normally **single-shot** — a second `init` fails with +`already initialized`. If you initialize with the wrong admin address, the +contract is not recoverable by re-initializing; deploy a fresh instance. + +For per-contract `init` signatures see [`DEPLOYMENT.md`](./DEPLOYMENT.md). + +### Step 6 — Transfer admin authority to the multisig + +A contract that will hold user funds must not remain under a single deployer +key. Hand the admin role to the `multisig_admin` contract (or a Stellar multisig +account, per [`UPGRADE_PROCESS.md`](./UPGRADE_PROCESS.md)): + +```bash +stellar contract invoke \ + --id "$CONTRACT_ID" --source-account shelter_admin --network testnet --send=yes \ + -- set_admin --new_admin "$MULTISIG_ADDR" +``` + +Then **confirm the transfer landed** by reading the admin back and by verifying +the old key can no longer perform an admin action: + +```bash +stellar contract invoke --id "$CONTRACT_ID" --network testnet -- get_admin +``` + +If the read does not return the multisig address, stop. A contract you believe +is under multisig control but is not is worse than one you know is under a +single key. + +### Step 7 — Record the deployment + +Confirm the contract ID landed in `contracts/deployment/deployed/{network}.json` +(the deploy script writes it; a manual `stellar contract deploy` does not — add +the entry yourself). See [§5](#5-where-deployments-are-tracked). + +--- + +## 2. Upgrading an existing contract + +Contributors may **propose** an upgrade. Only maintainers holding multisig keys +may **execute** one. + +1. **Build and hash the new WASM.** Steps 1–2 above, from a clean tree, on the + commit under review. +2. **Deploy the new WASM to testnet** and exercise it against the existing test + suite and the manual checklist. An upgrade proposal without a testnet run + behind it is not reviewable. +3. **Install the WASM on the target network.** `stellar contract upload` returns + the WASM hash the upgrade will point at. Verify it equals the hash from + step 1. +4. **Open a PR** using `.github/PULL_REQUEST_TEMPLATE.md` and fill in + *Contract Upgrade Details* completely — see [§6](#6-filling-in-the-pr-template) + for where each value comes from. +5. **Maintainer proposes the upgrade through the multisig.** The proposal names + the contract ID and the new WASM hash. +6. **The timelock delay elapses.** The `timelock` contract holds the proposal + for a fixed waiting period (24–48h) before it becomes executable. + + The delay is not a formality and it is not there to catch bugs — tests do + that. It exists so that **an upgrade nobody intended cannot land silently**. + If a multisig key is compromised, or a proposal is mis-encoded, or a + maintainer approves the wrong hash, the waiting period is the window in which + somebody notices and cancels. It converts an instant, irreversible action + into one with a review period. + + A proposal can be cancelled at any point *during* the delay. That is the last + moment at which an upgrade is cheap to undo. +7. **Multisig executes** after the delay. +8. **Verify the upgrade** — re-run §3 against the live contract and confirm the + deployed hash is the new one. + +--- + +## 3. Verification — proving the deployment is what you reviewed + +Deploying and verifying are different acts. A successful deploy transaction +proves *something* was installed; it does not prove it was the artifact that was +built, reviewed, and tested. + +Fetch the WASM the network actually holds and hash it: + +```bash +# 1. The hash you built (Step 2) +LOCAL_HASH=$(sha256sum contracts/artifacts/.wasm | cut -d' ' -f1) + +# 2. The WASM the network is serving for that contract ID +stellar contract fetch --id "$CONTRACT_ID" --network testnet > /tmp/onchain.wasm +ONCHAIN_HASH=$(sha256sum /tmp/onchain.wasm | cut -d' ' -f1) + +# 3. They must be identical +[ "$LOCAL_HASH" = "$ONCHAIN_HASH" ] && echo "MATCH" || echo "MISMATCH — STOP" +``` + +A mismatch means one of: you deployed a stale artifact, you built from a +different commit, or you are looking at the wrong contract ID. Do not +initialize, do not transfer admin, and do not proceed. Establish which of the +three it is first. + +Also confirm, and state the result in the PR: + +- [ ] Local WASM hash matches the on-chain WASM hash. +- [ ] The build came from a clean tree at a named commit SHA. +- [ ] `get_admin` returns the multisig address (for contracts holding value). +- [ ] `cargo test --workspace` passes at that commit. +- [ ] The deploy transaction is visible on the explorer for the intended network. + +Reproducibility caveat, stated honestly: `stellar contract build` output can +differ across toolchain versions and build environments, so a hash computed on +another machine may not match yours even when the source is identical. Record +the Rust and `stellar` CLI versions alongside the hash so a reviewer can +reproduce the comparison rather than having to trust it. + +--- + +## 4. Rollback — what can and cannot be undone + +Be honest with yourself about this before you deploy, not after. + +| Situation | Can it be undone? | +| --- | --- | +| Upgrade proposed, still inside the timelock delay | **Yes.** Cancel the proposal through the multisig. This is the only cheap reversal. | +| Upgrade executed, previous WASM still installed on-network | **Partially.** A *second* upgrade can point the contract back at the previous WASM hash — but it is a new upgrade and goes through the full multisig + timelock cycle again. | +| Upgrade executed and state migrated to a new layout | **No.** Reverting the code does not revert the storage. If a migration ran, the old WASM may not be able to read the current state at all. | +| Contract deployed with wrong `init` parameters | **No.** `init` is single-shot. Deploy a fresh instance and repoint whatever referenced the old one. | +| Contract deployed at all | **No.** The contract ID and its history are permanent on-chain. It can be paused or abandoned, never deleted. | +| Funds moved by a faulty upgrade | **No.** Nothing in this repository can reverse a settled transaction. | + +**If a faulty upgrade is discovered after execution:** + +1. **Pause first, diagnose second.** Contracts using `soroban_pausable` expose a + pause entrypoint — use it to stop further damage before you understand the + cause. A paused contract is recoverable; an actively-draining one may not be. +2. Notify maintainers and record the contract ID, the bad WASM hash, and the + execution transaction. +3. Decide between rolling forward (a fix, through the normal timelock cycle) and + rolling back to the previous WASM hash. Rolling forward is usually correct — + the previous WASM may not understand the current state. +4. If state is corrupted, neither option helps. Treat it as an incident and + follow [`../disaster-recovery-runbook.md`](../disaster-recovery-runbook.md). + +The timelock delay is the only genuine undo in this list. Everything after +execution is mitigation. + +--- + +## 5. Where deployments are tracked + +`contracts/deployment/deployed/{network}.json`, one file per network, is the +record of what is deployed where. It is what makes +`deploy-soroban.sh` idempotent: a contract with an ID already recorded is +skipped on re-run. + +- The deploy script writes entries automatically. +- A manual `stellar contract deploy` does **not** — add the entry by hand, in + the same PR as the deployment, or the next script run will deploy a duplicate. +- To intentionally redeploy a contract, remove its entry first: + + ```bash + jq 'del(.rent_wallet)' contracts/deployment/deployed/testnet.json > tmp.json \ + && mv tmp.json contracts/deployment/deployed/testnet.json + ``` + +The `deployed/` directory currently contains only `.gitkeep` — no network file +has been committed yet, so there is no committed record of any deployment. The +first deployment recorded through a PR establishes it. + +Backend environment variables that consume these IDs +(`SOROBAN_CONTRACT_ID`, `SOROBAN_STAKING_POOL_ID`, …) are listed in +[`DEPLOYMENT.md`](./DEPLOYMENT.md#backend-env-vars). Update them in the same +change as the deployment record, or the backend will keep talking to the old +contract. + +--- + +## 6. Filling in the PR template + +`.github/PULL_REQUEST_TEMPLATE.md` requires a *Contract Upgrade Details* +section. Each field maps to a step above: + +| Template field | Where the value comes from | +| --- | --- | +| **Network** (Testnet / Mainnet) | §0. Contributors tick Testnet. | +| **Contract ID** (`C...`) | Output of `stellar contract deploy`, Step 3. | +| **WASM Hash** (`sha256:...`) | `sha256sum` from Step 2, **confirmed against on-chain** in §3. | +| **Deployer Public Key** (`G...`) | `stellar keys address ` / `$STELLAR_DEPLOYER_ADDRESS`. | +| **Deploy Transaction** | Explorer link for the transaction from Step 3. Use the testnet explorer for testnet. | +| **Admin/upgrade authority is a multisig** | Tick only after §1 Step 6 and after `get_admin` read back the multisig address. | +| **Maintainer has reviewed and approved** | Maintainer ticks this, not the contributor. | +| **Upgrade transaction ready for signature (XDR)** | §2 step 4 — the unsigned upgrade transaction. | +| **New contract deployed successfully** | Step 3 plus the §3 hash match. Do not tick on the deploy alone. | +| **All existing tests pass** | `cargo test --workspace` at the built commit. | +| **Manual testing checklist** | What you exercised on testnet, named specifically. | +| **No breaking changes** | Interface diff against the previous WASM; list them if there are any. | + +Alongside the hash, include the toolchain versions you built with +(`rustc --version`, `stellar --version`) so a reviewer can reproduce it. + +--- + +## Validation status + +The procedure above is assembled from the existing scripts, configs and docs in +this repository and from the fields the PR template requires. **It has not yet +been executed end to end against testnet.** The commands are transcribed from +`contracts/scripts/`, `contracts/deployment/scripts/deploy-soroban.sh` and +[`DEPLOYMENT.md`](./DEPLOYMENT.md) rather than observed, so treat step outputs as +expected rather than confirmed. + +A testnet dry run is the natural follow-up, and any step that does not match +should be corrected here. Sections most likely to need adjustment on a real run: +the exact `stellar contract fetch` invocation in §3 (CLI surface has moved +between versions), and the per-contract `set_admin` entrypoint name in §1 +Step 6, which is not uniform across the suite. diff --git a/docs/contracts/UPGRADE_PROCESS.md b/docs/contracts/UPGRADE_PROCESS.md index b8cc8348e..5696a0b3a 100644 --- a/docs/contracts/UPGRADE_PROCESS.md +++ b/docs/contracts/UPGRADE_PROCESS.md @@ -1,5 +1,9 @@ # Contract Upgrade Process (Soroban) +> **Governance summary.** For the ordered procedure — including WASM hash +> verification, what the timelock delay is for, and what can and cannot be +> rolled back — see [`RUNBOOK.md`](./RUNBOOK.md). + ## Overview - Contributors can **deploy** new contract instances. - Only the **multisig admin** (maintainers) can **upgrade** existing contracts. From 4838a648f33b729746c74052997c12940ce11a2e Mon Sep 17 00:00:00 2001 From: Joycejay17 Date: Thu, 30 Jul 2026 06:42:04 +0100 Subject: [PATCH 2/5] docs(security): dependency audit across all three workspaces Runs cargo audit, npm audit and pnpm audit and triages every finding by actual exposure rather than by severity label. contracts/ was audited first since a vulnerable crate compiled into on-chain code cannot be patched after deployment: zero vulnerabilities, six informational warnings that all arrive via soroban-env-host - the host-side test emulator, not the wasm32 artifact - so none reach deployed code. backend/ and frontend/ findings are ranked by reachability. The two critical advisories in both are dev-only test tooling and are the least urgent items on the list; the most urgent are a high in multer and a moderate in express that both sit in the request path. Frontend risk is concentrated in the ~30 next advisories, notably the middleware/proxy bypasses. No dependencies are changed. Every available remediation is a next major-line move, an npm audit fix --force outside a stated range, or a bump under soroban-sdk - maintainer calls, with a recommended order and the residual risk of doing nothing stated. Also records that frontend/ carries both package-lock.json and pnpm-lock.yaml while CI installs with pnpm, so npm audit there reports on a tree that never ships. --- docs/security/dependency-audit-2026-07.md | 191 ++++++++++++++++++++++ 1 file changed, 191 insertions(+) create mode 100644 docs/security/dependency-audit-2026-07.md diff --git a/docs/security/dependency-audit-2026-07.md b/docs/security/dependency-audit-2026-07.md new file mode 100644 index 000000000..dd24903b9 --- /dev/null +++ b/docs/security/dependency-audit-2026-07.md @@ -0,0 +1,191 @@ +# Dependency Vulnerability Audit — July 2026 + +Point-in-time audit of all three dependency trees. **No dependencies were +changed in this pass** — see [Why nothing was upgraded here](#why-nothing-was-upgraded-here). + +| Workspace | Tool | Result | +| ------------ | ------------- | ------ | +| `contracts/` | `cargo audit` | **0 vulnerabilities**, 6 informational warnings | +| `backend/` | `npm audit` | 59 (6 critical, 18 high, 34 moderate, 1 low) | +| `frontend/` | `pnpm audit` | 66 (1 critical, 32 high, 28 moderate, 5 low) | + +Advisory databases move; re-run before acting on this. + +--- + +## 1. contracts/ — priority workspace + +A vulnerable crate compiled into on-chain code cannot be patched after +deployment, so this workspace was audited first. + +``` +$ cargo audit + Loaded 1173 security advisories + Scanning Cargo.lock for vulnerabilities (235 crate dependencies) +warning: 6 allowed warnings found +``` + +**Zero vulnerabilities.** The six warnings are unmaintained / unsound / yanked +notices, not CVEs: + +| Crate | ID | Class | +| --- | --- | --- | +| `derivative` 2.2.0 | RUSTSEC-2024-0388 | unmaintained | +| `paste` 1.0.15 | RUSTSEC-2024-0436 | unmaintained | +| `anyhow` 1.0.102 | RUSTSEC-2026-0190 | unsound (`Error::downcast_mut()`) | +| `rand` 0.8.5 | RUSTSEC-2026-0097 | unsound (custom logger + `rand::rng()`) | +| `rand` 0.9.2 | RUSTSEC-2026-0097 | unsound | +| `spin` 0.9.8 | — | yanked | + +### Triage: none of these reach on-chain code + +All six arrive through `soroban-env-host` (via `soroban-sdk` → +`soroban-ledger-snapshot`), which is the **host-side test environment** — the +emulator `cargo test --workspace` runs against. It is not compiled into the +`wasm32-unknown-unknown` artifact that gets deployed. + +``` +derivative v2.2.0 (proc-macro) +└── ark-ec v0.4.2 + └── ark-bls12-381 v0.4.0 + └── soroban-env-host v22.1.3 + └── soroban-ledger-snapshot v22.0.10 + └── soroban-sdk v22.0.10 → (all contracts) +``` + +`derivative` is additionally a proc-macro — it runs at compile time and emits no +runtime code at all. + +**Exposure: none for deployed contracts.** The residual risk is confined to the +local/CI test host. `anyhow`'s unsoundness needs a `downcast_mut()` call the +contracts do not make; `rand`'s needs a custom logger the test harness does not +install. + +**Action: none required.** These clear when `soroban-sdk` bumps its own +dependencies. Pinning them ourselves would mean overriding the SDK's transitive +versions — more risk than the warnings carry. + +--- + +## 2. backend/ — payments and personal data + +``` +$ npm audit +59 vulnerabilities (1 low, 34 moderate, 18 high, 6 critical) +``` + +Direct dependencies with advisories, triaged by **actual exposure** rather than +by severity label: + +| Severity | Package | Ships to prod? | Real exposure | +| --- | --- | --- | --- | +| CRITICAL | `vitest`, `@vitest/ui` | **No** — devDependency | Arbitrary file read/exec *only while the Vitest UI server is listening*. Never runs in production, never in `npm run test:ci`. **Lowest real urgency on this list despite the label.** | +| CRITICAL | `@redocly/cli` | **No** — devDependency | Via OpenTelemetry transitives. Used by `npm run openapi:validate` in CI only. Not a runtime path. | +| HIGH | `multer` | **Yes** | DoS via deeply nested field names and via incomplete cleanup of aborted uploads. This is a **request path** on any upload endpoint — reachable by an unauthenticated client. **Highest real priority in this workspace.** | +| HIGH | `ws` | **Yes** (via `ethers`) | Uninitialized memory disclosure + memory-exhaustion DoS. Reachable if the backend opens outbound websockets to an RPC provider. Memory disclosure is the concerning half. | +| HIGH | `@opentelemetry/*` | Yes | Telemetry path, not request-handling. Lower exposure than the label. | +| MODERATE | `express` (via `qs`) | **Yes** | Query-string parsing sits in front of **every** request. Moderate label, broad reach — treat above the OpenTelemetry highs. | +| MODERATE | `morgan` | Yes | Log forging via unneutralized control characters in `:remote-user`. Affects log integrity, not the service. Matters here because logs are the audit trail for payment activity. | +| MODERATE | `express-rate-limit` (via `ip-address`) | **Yes** | Rate limiting is a control the payment endpoints rely on; a parsing flaw there is a bypass risk, not just a crash risk. | +| MODERATE | `resend` (via `svix`) | Yes | Outbound email. Exposure depends on webhook-signature verification usage. | +| MODERATE | `ethers` (via `ws`) | Yes | Same `ws` root cause. | + +**Ranking by real exposure**, which differs sharply from the severity ordering: +`multer` → `express`/`qs` → `express-rate-limit` → `ws`/`ethers` → `morgan` → +OpenTelemetry → `@redocly/cli` → `vitest`/`@vitest/ui`. + +The two loudest findings (critical, in dev-only tooling) are the two least +urgent. The most urgent is a `high` and a `moderate` sitting in the request path. + +`npm audit` reports fixes available for all of the above; `ws` needs +`--force` because the resolution falls outside `ethers`' stated range. + +--- + +## 3. frontend/ — wallet interaction and auth tokens + +``` +$ pnpm audit +66 vulnerabilities found +Severity: 5 low | 28 moderate | 32 high | 1 critical +``` + +Dominated by **`next` — roughly 30 of the 66 advisories are Next.js itself**, +across middleware/proxy bypass, SSRF in Server Actions and rewrites, cache +poisoning, and image-optimizer DoS. Patched in `>= 16.2.5`. + +| Severity | Package | Ships? | Real exposure | +| --- | --- | --- | --- | +| HIGH | `next` (middleware / proxy bypass, ×5) | **Yes** | The one class that matters most here. If any auth or route protection is enforced in middleware, a bypass is an auth bypass. Warrants checking whether this app gates anything in `middleware.ts`. | +| HIGH | `next` (SSRF in Server Actions / rewrites) | **Yes** | Server-side request forgery from a Next.js app that also talks to a wallet/RPC backend is a genuine pivot. | +| MODERATE | `next` (null origin bypasses Server Actions CSRF) | **Yes** | CSRF on an authenticated session. | +| HIGH | `sharp` (libvips CVEs) | Yes | Image processing. Exposure depends on whether user-supplied images are processed server-side. | +| MODERATE | `next-intl` (open redirect, prototype pollution) | **Yes** | In active use — `next.config.mjs` wires `createNextIntlPlugin`. Open redirect on a login flow is a phishing primitive. | +| HIGH | `postcss`, `js-yaml`, `brace-expansion`, `fast-uri` | Build-time | Toolchain, not shipped runtime. Real urgency well below the label. | +| CRITICAL | `vitest` | **No** — dev | Same reasoning as the backend. Not urgent. | +| HIGH | `vite`, MODERATE `esbuild` | **No** — dev server | `esbuild` "any website can send requests to the dev server" needs a running dev server. Not a production exposure. | +| HIGH | `lodash` (`_.template` code injection) | Depends | Only exploitable if `_.template` is called on attacker input — worth confirming rather than assuming. | + +The `next` findings are the ones that carry actual user risk. Most of the rest +of the count is build tooling inflating the total. + +### Lockfile state — `frontend/` carries two lockfiles + +`frontend/package-lock.json` (503 KB) sits alongside `frontend/pnpm-lock.yaml` +(360 KB). CI installs with `pnpm install --frozen-lockfile`, so +**`pnpm-lock.yaml` is what ships** and `package-lock.json` describes a tree that +is never built. + +This is an auditing hazard specifically: `npm audit` in `frontend/` reads +`package-lock.json` and reports on the wrong tree — findings that do not apply, +and worse, silence about ones that do. Anyone auditing this workspace must use +`pnpm audit`. + +Recommend deleting `frontend/package-lock.json`. Not done here — it is a +lockfile change and belongs in its own PR where a full install and build can be +verified against it. (Note the repository root also has both a +`package-lock.json` and a `pnpm-lock.yaml`; same question, separate scope.) + +--- + +## Existing tooling — `security-scan/` + +`security-scan/` already exists with a scanner/orchestrator/aggregator +structure. Anything durable from this audit should extend that rather than +duplicate it, so results land in the same report pipeline. This document is a +point-in-time snapshot, not a replacement for it. + +--- + +## Why nothing was upgraded here + +Every remediation available in the two Node workspaces is either a +`next` major-line move, an `npm audit fix --force` that resolves outside a +dependency's stated range (`ws` under `ethers`), or a transitive bump under +`soroban-sdk`. + +Per the issue's own guidance — a dependency upgrade that silently changes +behaviour in a payment path is worse than the advisory it resolved. `next` +`16.0.x → 16.2.5` touches middleware, Server Actions and caching in an app whose +auth and wallet flows depend on exactly those. `ws` under `ethers` outside its +stated range touches RPC connectivity. + +These are maintainer calls, not contributor calls. Recommended order if the +maintainers want them taken: + +1. **`next` → `>= 16.2.5`** — clears ~30 advisories including the middleware + bypasses. Highest value, needs a full manual pass over auth and wallet flows. +2. **`multer`** — request-path DoS, small and self-contained. +3. **`express` / `express-rate-limit`** — patch-level, low risk. +4. **`ws` / `ethers`** — needs `--force`; verify RPC connectivity after. +5. **Dev tooling** (`vitest`, `vite`, `@redocly/cli`) — no production exposure; + batch whenever convenient. + +### Residual risk if nothing is done + +- Frontend: middleware/proxy bypass and Server Action SSRF remain live against + an app handling wallet interaction and auth tokens. **This is the largest + single item in this report.** +- Backend: unauthenticated upload DoS via `multer`; memory disclosure via `ws` + on outbound RPC websockets. +- Contracts: none. The deployed WASM is unaffected by all six warnings. From ca9acd9a00227150f427beeb7d2bfdd2e4808291 Mon Sep 17 00:00:00 2001 From: Joycejay17 Date: Thu, 30 Jul 2026 06:42:04 +0100 Subject: [PATCH 3/5] fix(legal): stop the duplicate privacy and terms routes being indexable /privacy and /terms are unlinked duplicates of /privacy-policy and /terms-of-service. Both are indexable, so a search engine can currently deliver a user to a version the product does not treat as current - and which of the two binds that user is genuinely ambiguous. The substantive difference is that there isn't one: every section of /privacy and /terms is the same placeholder sentence, while the linked routes carry real policy text. They are scaffolds, not an alternative set of terms. Adds noindex and points canonical at the linked routes, so the ambiguity stops mattering to search engines while a maintainer decides which route is canonical. Deliberately does not delete or redirect - that is the maintainer decision the issue reserves. /cookies matches the short-page shape but is linked from CookieConsentBanner and has no duplicate, so it is canonical and only gets a self-referencing canonical URL. --- frontend/app/cookies/page.tsx | 7 +++++++ frontend/app/privacy/page.tsx | 11 +++++++++++ frontend/app/terms/page.tsx | 11 +++++++++++ 3 files changed, 29 insertions(+) diff --git a/frontend/app/cookies/page.tsx b/frontend/app/cookies/page.tsx index f87d7de46..8f3b26b93 100644 --- a/frontend/app/cookies/page.tsx +++ b/frontend/app/cookies/page.tsx @@ -3,10 +3,17 @@ import { LegalPage } from "@/components/legal/LegalPage"; export const dynamic = "force-static"; +/** + * Canonical cookie policy. Unlike `/privacy` and `/terms`, this route has no + * longer-form duplicate and is linked from `components/CookieConsentBanner.tsx`, + * so it is not orphaned — its content is placeholder, but the route is the + * real one. + */ export const metadata: Metadata = { title: "Cookie Policy — Shelterflex", description: "Understand how Shelterflex uses cookies and similar technologies. Official legal copy will be updated before launch.", + alternates: { canonical: "/cookies" }, }; const PLACEHOLDER = diff --git a/frontend/app/privacy/page.tsx b/frontend/app/privacy/page.tsx index 82dbd845e..d8cf9dd3a 100644 --- a/frontend/app/privacy/page.tsx +++ b/frontend/app/privacy/page.tsx @@ -3,10 +3,21 @@ import { LegalPage } from "@/components/legal/LegalPage"; export const dynamic = "force-static"; +/** + * Non-canonical duplicate of `/privacy-policy`. + * + * Every section below is placeholder text, while `/privacy-policy` carries the + * substantive policy and is the route the app links to. Until a maintainer + * confirms which route is canonical (see #1446), this one is excluded from + * indexing and points its canonical URL at `/privacy-policy` so search engines + * cannot deliver a user to the placeholder version. + */ export const metadata: Metadata = { title: "Privacy Policy — Shelterflex", description: "Learn how Shelterflex collects, uses, and protects your personal data. Official legal copy will be updated before launch.", + alternates: { canonical: "/privacy-policy" }, + robots: { index: false, follow: false }, }; const PLACEHOLDER = diff --git a/frontend/app/terms/page.tsx b/frontend/app/terms/page.tsx index 0fe566eaa..829a0b251 100644 --- a/frontend/app/terms/page.tsx +++ b/frontend/app/terms/page.tsx @@ -3,10 +3,21 @@ import { LegalPage } from "@/components/legal/LegalPage"; export const dynamic = "force-static"; +/** + * Non-canonical duplicate of `/terms-of-service`. + * + * Every section below is placeholder text, while `/terms-of-service` carries + * the substantive terms and is the route the app links to. Until a maintainer + * confirms which route is canonical (see #1446), this one is excluded from + * indexing and points its canonical URL at `/terms-of-service` so search + * engines cannot deliver a user to the placeholder version. + */ export const metadata: Metadata = { title: "Terms of Service — Shelterflex", description: "Read the Shelterflex Terms of Service. Official legal copy will be updated before launch.", + alternates: { canonical: "/terms-of-service" }, + robots: { index: false, follow: false }, }; const PLACEHOLDER = From 2a827a39cda47a49f89fd1e8082d0bbc05e4ed94 Mon Sep 17 00:00:00 2001 From: Joycejay17 Date: Thu, 30 Jul 2026 06:42:04 +0100 Subject: [PATCH 4/5] fix(frontend): keep the design system gallery out of production app/design-system is an internal component gallery with no auth and no environment gating, reachable by anyone at /design-system in production and indexable there. Returns notFound() outside development. Gated on NODE_ENV rather than FeatureFlagProvider because that provider resolves client-side from the backend, so a flag would still ship the route and its markup and only hide it after hydration - a server-side check makes it a real 404. The page still renders during next build, so it keeps type-checking against the component library. Adds noindex as well. --- frontend/app/design-system/page.tsx | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/frontend/app/design-system/page.tsx b/frontend/app/design-system/page.tsx index 721555fdf..960b9256f 100644 --- a/frontend/app/design-system/page.tsx +++ b/frontend/app/design-system/page.tsx @@ -1,3 +1,5 @@ +import type { Metadata } from "next" +import { notFound } from "next/navigation" import { Button } from "@/components/ui/button" import { Card, @@ -10,6 +12,25 @@ import { import { Input } from "@/components/ui/input" import { ThemeToggle } from "@/components/theme-toggle" +/** + * Internal component gallery — a development artefact, not a user-facing page. + * + * Gated on NODE_ENV rather than a feature flag: FeatureFlagProvider resolves + * client-side from the backend, so a flag would still ship the route and its + * markup to production and only hide it after hydration. Checking NODE_ENV in + * the server component keeps the route reachable in `next dev` while making it + * a genuine 404 in a production build. + * + * The gallery still renders during `next build`, so it continues to type-check + * against the component library. + */ +const isDevelopment = process.env.NODE_ENV === "development" + +export const metadata: Metadata = { + title: "Design System — Shelterflex", + robots: { index: false, follow: false }, +} + const tokenRows = [ { name: "Primary", token: "--primary", sampleClass: "bg-primary" }, { name: "Secondary", token: "--secondary", sampleClass: "bg-secondary" }, @@ -18,6 +39,10 @@ const tokenRows = [ ] export default function DesignSystemPage() { + if (!isDevelopment) { + notFound() + } + return (
From 8c7018654711598d7c1742b922c99e8b53165f02 Mon Sep 17 00:00:00 2001 From: Joycejay17 Date: Thu, 30 Jul 2026 06:42:04 +0100 Subject: [PATCH 5/5] chore: remove stale PR description scratch files PR_DESCRIPTION_#1316.md and PR_DESCRIPTION_#1335_#1336.md are leftover working notes from merged PRs and are not referenced anywhere. --- PR_DESCRIPTION_#1316.md | 19 --------- PR_DESCRIPTION_#1335_#1336.md | 72 ----------------------------------- 2 files changed, 91 deletions(-) delete mode 100644 PR_DESCRIPTION_#1316.md delete mode 100644 PR_DESCRIPTION_#1335_#1336.md diff --git a/PR_DESCRIPTION_#1316.md b/PR_DESCRIPTION_#1316.md deleted file mode 100644 index 7412c456b..000000000 --- a/PR_DESCRIPTION_#1316.md +++ /dev/null @@ -1,19 +0,0 @@ -## Summary - -Remove the backend health widget from the public site header so public visitors no longer see an operator diagnostic in the marketing UI, and relocate the component to the admin health page as an operator-facing surface. - -## Changes - -- Removed the backend health compact widget from the public header across the marketing/site shell. -- Kept the component available on the admin health page so operators can still access it in an appropriate surface. -- Gated the health polling so it only runs when the health widget is actually visible/eligible, preventing anonymous public page loads from issuing health checks. -- Added a regression test covering the public header to ensure the widget does not render there again. - -## Checklist - -- [x] The public header no longer renders a backend health indicator. -- [x] Anonymous/public page loads no longer trigger the health request from the header. -- [x] The health component remains available on the admin health surface. -- [x] Frontend lint and build pass locally. - -Closes #1316 diff --git a/PR_DESCRIPTION_#1335_#1336.md b/PR_DESCRIPTION_#1335_#1336.md deleted file mode 100644 index 8e3568c9b..000000000 --- a/PR_DESCRIPTION_#1335_#1336.md +++ /dev/null @@ -1,72 +0,0 @@ -# Title -Contracts: test coverage for allowlist_registry, schema_registry, soroban_access_control, soroban_pausable - -# Body - -## Summary - -Adds coverage to the four lowest-tested crates in the `contracts/` workspace: - -| Crate | Before | After | -|---|---|---| -| `allowlist_registry` | 6 | 24 | -| `schema_registry` | 6 | 20 | -| `soroban_access_control` | 7 | 14 | -| `soroban_pausable` | 8 | 16 | - -No contract logic was changed — this PR is tests only, per the "out of scope: changing contract behaviour" note on both issues. A few places where the existing behavior looked ambiguous or potentially unintended are documented below rather than silently asserted as correct. - -Closes #1335 -Closes #1336 - -## What's covered - -### `allowlist_registry` (#1335) -- Authorization: `remove` and `bulk_add` rejected for non-admin callers (`add` already had coverage) -- Double-initialization rejected; `add`/`remove`/`bulk_add` all fail predictably before initialization; read-only queries (`is_member`, `get_entry`, `member_count`) return safe defaults instead of panicking before init -- Duplicate `remove` (removing an already-removed entry) fails on the second call -- Expiry boundary: `expires_at == now` rejected, `now + 1` accepted; `bulk_add` skips already-expired entries in a batch without erroring -- `member_count` and `get_entry` correctly exclude/reject expired entries -- `add`, `remove`, and `bulk_add` events asserted for topic and payload content (not just "an event fired") - -### `schema_registry` (#1335) -- Authorization: `register_transition` rejected for non-admin callers -- Double-initialization panics as expected; `register_transition` fails predictably pre-init -- `register_transition` rejects `source == target` -- `execute_migration` version-mismatch path covered, including the `migration_rejected` event -- `execute_migration` success path covered, including the `migration_executed` event and the resulting version bump -- Idempotency (`AlreadyExecuted`) already had coverage; added monotonic `migration_id` incrementing across multiple migrations -- `get_receipt` (absent → present) and `verify_migration` (true / wrong-target / nonexistent-id) covered -- Invariant enforcement exercised properly: a *registered* downgrade transition and a major-version bump that doesn't reset minor to 0 both correctly fail `dry_run` / `execute_migration` with `InvariantViolation`. (The pre-existing "downgrade blocked" test only exercised an *unregistered* pair, i.e. `UnsupportedTransition` — it never actually reached the invariant-checking code path.) - -### `soroban_access_control` (#1336) -- Authorization: `set_operator` rejected for non-admin callers (only `admin_only_operation`/`admin_or_operator_operation` had coverage before) -- Double-initialization panics; calling a privileged function before init panics predictably; calling with zero mocked authorizations (not just the wrong signer) fails, proving `require_auth()` is actually wired up -- `set_operator` overwrite: setting a new operator revokes the previous operator's privileges and grants the new one's, in the same test -- `admin_or_operator_operation` denies a caller when no operator has ever been set (not just when a *different* operator exists) -- Unauthorized-access event asserted for topic/payload content, exercised via a second operation (`set_operator`) to confirm the operation name in the event payload is correct per-call, not hardcoded - -### `soroban_pausable` (#1336) -- Double-initialization panics; calling `pause` before init panics predictably; `is_paused()` before init safely defaults to `false` -- `pause`/`unpause` with zero mocked authorizations fail, proving `require_auth()` is actually enforced (not just the wrong-signer case, which already had coverage) -- Multi-cycle pause/unpause loop (3 iterations) proving `guarded_operation` toggles correctly every cycle, not just once -- `pause` and `unpause` events asserted for topic content (previously emitted but never asserted) - -## Ambiguous / possibly-unintended behavior found (flagging per issue instructions, not fixed here) - -1. **`allowlist_registry::add` silently overwrites an existing entry.** The `Error::AlreadyExists` variant is defined but never returned anywhere in the contract — re-adding an already-registered address just updates its label/expiry instead of erroring. Documented as current behavior in `test_add_duplicate_overwrites_existing_entry`. Is overwrite the intended semantics, or should this reject like the unused error variant suggests? - -2. **`allowlist_registry::initialize` has no authorization check at all.** It takes no `caller` argument and never calls `require_auth()` on the `admin` it's given — any account can call it once, for any admin address, with zero authorizations mocked (see `test_initialize_succeeds_without_any_mocked_auth`). This is presumably fine if bootstrap security relies on deploy-time control, but flagging since every other privileged path in this crate does enforce auth. - -3. **`schema_registry::execute_migration` has no admin check.** It calls `caller.require_auth()` but never compares `caller` to the registry admin, unlike `register_transition` (which goes through `require_admin`). Any address able to sign can execute an already-registered migration (`test_execute_migration_allows_non_admin_caller` documents this as current behavior). Given migrations mutate contract-wide schema state, this looks more like a genuine authorization gap than intended design — recommend a follow-up issue if confirmed. - -4. **`schema_registry::register_transition` silently overwrites an existing (source, target) entry** rather than rejecting a duplicate registration. Documented in `test_register_transition_overwrites_existing_entry`. Similar question to #1 above — intended, or should re-registration be rejected? - -5. **`soroban_access_control` doesn't implement role granting/revocation or admin transfer.** Issue #1336's scope for this crate asks for coverage of "role granting and revocation" and "admin transfer, including loss of the previous admin's authority." The actual crate surface is two generic authorization-check helpers (`require_admin_permission`, `require_admin_or_operator_permission`) plus a minimal test harness contract with `init`/`set_operator`/two gated operations — there is no grant/revoke or transfer-admin function anywhere in this crate to test. I did not add that functionality to the harness contract since that would be adding new behavior rather than testing existing behavior (out of scope per the issue). Flagging for maintainer confirmation: does this functionality live in a different crate, or does the issue scope need adjusting for what this crate actually does? Separately: since there's no admin-transfer path, the "contract cannot be left without an admin" invariant holds trivially — admin is immutable once set at init. - -6. **`soroban_pausable`'s test harness has exactly one pause-gated operation** (`guarded_operation`). The issue asks to "enumerate operations gated by pause rather than testing one representative case" — there is only one to enumerate. Flagging in case maintainers want the harness contract expanded with more representative gated/ungated operations; I left it as-is to avoid changing contract behavior beyond what the issue authorized. - -## Test plan -- [x] `cd contracts && cargo fmt --all -- --check` -- [x] `cargo clippy --workspace --all-targets --all-features` -- [x] `cargo test --workspace` — all tests pass, including the 47 new tests across the four crates (0 failures, 0 pre-existing test regressions)