Skip to content

Docs/contract runbook dep audit legal routes gallery gate - #1480

Merged
pope-h merged 5 commits into
Shelterflex:mainfrom
Joycejay17:docs/contract-runbook-dep-audit-legal-routes-gallery-gate
Jul 30, 2026
Merged

Docs/contract runbook dep audit legal routes gallery gate#1480
pope-h merged 5 commits into
Shelterflex:mainfrom
Joycejay17:docs/contract-runbook-dep-audit-legal-routes-gallery-gate

Conversation

@Joycejay17

Copy link
Copy Markdown
Contributor

Summary

Four related gaps, all documentation-or-gating work with no behavioural change to product code.

This is a deliberately minimal pass on each. Where an issue asks for a decision that is a maintainer's to make — which legal route is canonical, whether to take a next major-line upgrade — this PR reports what it found and stops rather than choosing.

Closes #1444
Closes #1445
Closes #1446
Closes #1447

Changes

#1444 — Contracts deployment and upgrade runbook

New docs/contracts/RUNBOOK.md. The three existing documents each held part of the picture:

Existing doc Held
docs/contracts/DEPLOYMENT.md Soroban CLI commands, identities, backend env vars
docs/contracts/UPGRADE_PROCESS.md Who may upgrade, PR governance
contracts/deployment/README.md Network config, the idempotent deploy script

None gave the order, and none covered WASM verification, rollback limits, or the testnet/mainnet gate. The runbook covers, in sequence:

  • The testnet/mainnet gate — a table of what differs, and the one rule that matters: mainnet is a maintainer action, never a contributor action, and requires a prior testnet deployment of the same WASM hash.
  • Deployment — build from a clean tree → record the hash before deploying → deploy → verify → init → transfer admin to the multisig → record the deployment. Verification sits before init deliberately.
  • Upgrade — build, testnet-exercise, upload, PR, multisig proposal, timelock delay, execute, re-verify.
  • What the timelock delay is for. Not to catch bugs — tests do that. It exists so an upgrade nobody intended cannot land silently: a compromised key, a mis-encoded proposal, or an approved-wrong-hash all get a window in which somebody notices and cancels.
  • WASM verification — fetch the artifact the network actually holds, hash it, compare to the local hash. With the honest caveat that stellar contract build output can vary across toolchains, so record the rustc and stellar versions alongside the hash rather than expecting a reviewer to trust a bare match.
  • Rollback — a table stating plainly what is recoverable and what is not.
  • PR template mapping — every Contract Upgrade Details field mapped to the step that produces it.

The rollback section is the part I'd most want reviewed for accuracy:

Situation Undoable?
Upgrade proposed, still inside the timelock delay Yes — cancel via multisig. The only cheap reversal.
Upgrade executed, previous WASM still installed Partially — a second upgrade can point back, but goes through the full multisig + timelock cycle again.
Upgrade executed and state migrated No. Reverting code does not revert storage; the old WASM may not read the current state at all.
Wrong init parameters No. init is single-shot. Deploy fresh.
Contract deployed at all No. Pausable or abandonable, never deletable.
Funds moved by a faulty upgrade No.

The three existing docs now cross-link to it. No scripts or contracts were touched.

On validation: the acceptance criteria ask for the runbook to be followed on testnet and corrected. I have not done that — the commands are transcribed from contracts/scripts/, deploy-soroban.sh and the existing docs rather than observed against a live network, and the runbook says so in a "Validation status" section rather than implying otherwise. I've flagged the two steps most likely to need correction on a real run: the exact stellar contract fetch invocation (the CLI surface has moved between versions) and the set_admin entrypoint name, which is not uniform across the suite. Happy to do the testnet pass and correct it in a follow-up if that's preferred over merging it as-is.

#1445 — Dependency audit

New docs/security/dependency-audit-2026-07.md.

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

contracts/ was audited first and is clean:

$ cargo audit
    Loaded 1173 security advisories
    Scanning Cargo.lock for vulnerabilities (235 crate dependencies)
warning: 6 allowed warnings found

The six warnings (derivative, paste, anyhow, rand ×2, spin) are unmaintained/unsound/yanked notices, not CVEs — and all six arrive through soroban-env-host, the host-side test emulator, not the wasm32-unknown-unknown artifact that gets deployed:

derivative v2.2.0 (proc-macro)
└── ark-ec → ark-bls12-381 → soroban-env-host v22.1.3
    └── soroban-ledger-snapshot → soroban-sdk v22.0.10 → (all contracts)

derivative is additionally a proc-macro — compile-time only, emits no runtime code. Exposure to deployed contracts: none. Residual risk is confined to the CI/local test host.

On triage by exposure rather than label — the ordering this produces is close to inverted from the severity ordering. In backend/, both criticals (vitest/@vitest/ui, @redocly/cli) are devDependencies that never ship; the Vitest one additionally requires the UI server to be listening, which npm run test:ci doesn't do. Meanwhile the genuinely urgent items are a high in multer (DoS via deeply nested field names, reachable unauthenticated on any upload endpoint) and a moderate in express/qs (query-string parsing sits in front of every request). Real-exposure ranking:

multerexpress/qsexpress-rate-limitws/ethersmorgan → OpenTelemetry → @redocly/clivitest

frontend/ is dominated by Next.js — roughly 30 of the 66 advisories are next itself, patched in >= 16.2.5. The five middleware/proxy-bypass advisories are the ones that carry real user risk: if anything is gated in middleware.ts, a bypass there is an auth bypass. SSRF in Server Actions and rewrites matters for the same reason on an app that also talks to wallet/RPC infrastructure. Much of the remaining count (postcss, js-yaml, brace-expansion, fast-uri, vite, esbuild) is build tooling inflating the total.

Nothing was upgraded, deliberately. Every available remediation is one of: a next major-line move, an npm audit fix --force resolving outside a stated range (ws under ethers), or a transitive bump under soroban-sdk. Per the issue's own implementation note — 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 touches RPC connectivity. These are maintainer calls. The doc gives a recommended order (next first, then multer, then express, then ws, then dev tooling last) and states the residual risk of doing nothing.

I've therefore left the "fixable vulnerabilities are fixed" criterion unmet on purpose — say the word on which of the five and I'll take them in a follow-up with the full suite verified after each.

Lockfiles: frontend/ carries both package-lock.json (503 KB) and pnpm-lock.yaml (360 KB). CI installs with pnpm install --frozen-lockfile, so the pnpm lockfile is what ships and package-lock.json describes a tree that is never built. This is an auditing hazard specifically — npm audit there reports on the wrong tree, giving both false findings and false silence. Recommend deleting it, not done here because a lockfile change deserves its own PR with a verified install and build. The repository root has the same pair.

Existing tooling: security-scan/ already has a scanner/orchestrator/aggregator structure. Anything durable from this should extend that rather than duplicate it; this doc is a point-in-time snapshot.

#1446 — Duplicate legal routes

The substantive difference between the versions is that there isn't one. Every section of /privacy (59 lines) and /terms (59 lines) is the identical placeholder string:

"This section will be updated before launch. Official legal copy is being prepared by our legal team."

/privacy-policy (165 lines) and /terms-of-service (136 lines) carry real policy text — data collection categories, payment information handling, identity documents, liability limits. So these are not two competing sets of terms; they are scaffolds sitting next to the real documents. That makes the exposure narrower than it first looks, but not zero: the scaffolds were indexable, so a search engine could deliver a user to a page that says the policy doesn't exist yet.

Added robots: { index: false, follow: false } and alternates: { canonical: ... } pointing at the linked routes.

Not removed or redirected. The issue reserves the canonical decision for a maintainer, and deletion is not reversible from a contributor's side. The noindex + canonical stops the ambiguity mattering to search engines while that decision is pending. If you confirm /privacy-policy and /terms-of-service are canonical, the redirects are a two-line next.config.mjs change I'm happy to add here.

/cookies: checked, and it is not orphaned. It matches the short-page shape (59 lines, all placeholder) but it is linked from components/CookieConsentBanner.tsx:27 and has no longer-form duplicate — so it is the canonical cookie policy. Its content being placeholder is a content gap, not a routing one. It gets a self-referencing canonical and no noindex.

Canonical metadata could not be added to /privacy-policy and /terms-of-service themselves: both are "use client" components, which cannot export metadata. That needs a layout.tsx per route and belongs with the metadata issue.

#1447 — Design system gallery

app/design-system/page.tsx now calls notFound() unless NODE_ENV === "development", plus robots: { index: false, follow: false }.

Gated on NODE_ENV rather than FeatureFlagProvider, which the issue offered as an option. frontend/lib/featureFlags.ts starts from static defaults and hydrates from GET /api/config/feature-flags after mount — it is a client-side provider. A flag would still ship the route and its markup to production and only hide it after hydration, which is a visual gate rather than a real one. A server-component NODE_ENV check makes it a genuine 404.

Chose this over deleting the route because the gallery is the only thing that type-checks the component library end to end, and it still renders during next build — so that incidental check survives. /design-system moves from a static route to ƒ (server-rendered on demand) in the build output, which is expected.

How to test

Frontend CI (both green locally):

$ pnpm run lint
✖ 4 problems (0 errors, 4 warnings)     # all pre-existing, unrelated files

$ pnpm run build
✓ Compiled successfully in 7.2s

Build output confirms the gating: ├ ƒ /design-system, ├ ○ /privacy, ├ ○ /terms.

Contracts CI: no Rust changed in this branch — only markdown under docs/contracts/ and contracts/. cargo fmt --all -- --check passes. clippy --workspace --all-targets --all-features and test --workspace are unaffected by markdown-only edits.

Backend CI: untouched by this branch. No dependency, endpoint or openapi.yml changes.

Manual checks:

  • /design-system returns 404 in a production build, renders normally under next dev.
  • View source on /privacy and /terms shows <meta name="robots" content="noindex, nofollow"> and <link rel="canonical" href="/privacy-policy"> / /terms-of-service.
  • Links from the app still resolve to /privacy-policy and /terms-of-service; CookieConsentBanner still resolves to /cookies.

Security Considerations

The audit doc names unpatched advisories in a public repo. That's the point of the issue, and all of them are already public in the GitHub and RustSec advisory databases; nothing in the doc reveals an exposure an attacker couldn't derive from the lockfiles.

Note on commit 5

8c701865 removes PR_DESCRIPTION_#1316.md and PR_DESCRIPTION_#1335_#1336.md from the repo root — leftover working notes from merged PRs, unreferenced anywhere. Unrelated to the four issues; drop that commit if you'd rather keep them.

Checklist

  • I linked an issue
  • I tested locally
  • I did not commit secrets
  • I updated docs if needed
  • Code follows the project's style guidelines
  • CI checks pass
  • If UI changes: I included before/after screenshots — n/a, the only UI change makes a page unreachable

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.
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.
/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.
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.
PR_DESCRIPTION_#1316.md and PR_DESCRIPTION_#1335_#1336.md are leftover
working notes from merged PRs and are not referenced anywhere.
@vercel

vercel Bot commented Jul 30, 2026

Copy link
Copy Markdown

@Joycejay17 is attempting to deploy a commit to the pope-h's projects Team on Vercel.

A member of the Team first needs to authorize it.

@drips-wave

drips-wave Bot commented Jul 30, 2026

Copy link
Copy Markdown

@Joycejay17 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@pope-h
pope-h merged commit e50e6a5 into Shelterflex:main Jul 30, 2026
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants