Harden policy-human authorization and remove /tmp import bootstrap - #15
Harden policy-human authorization and remove /tmp import bootstrap#15juguelio wants to merge 1 commit into
Conversation
Two findings from a security review of the repo, plus diagnostics for the error path that made the World ID sandbox hard to debug. 1. Wallet-session bypass when issuing a policy-human authorization. The SIWE session check was skipped whenever the deployment was not production, so a staging/testnet deployment trusted `body.walletAddress` as the policy holder. The World ID signal only proves a human bound a proof to that address, not that they control it, so a caller could obtain a signing-key-backed EIP-712 authorization naming an arbitrary holder. Bounded (redeeming on-chain still needs that wallet, and it burns the caller's own nullifier), but it stops being bounded if the staging signing key is ever the production one. CWE-639. The session is now required unconditionally. The browser sandbox still needs to work — outside World App, MiniKit Wallet Auth is unavailable so no SIWE session can ever exist — so the escape hatch is preserved but inverted: it is never opened implicitly (not by NODE_ENV, not by the World ID environment), only by setting RISKA_ALLOW_UNSIGNED_BROWSER_WALLET=true on purpose, and it is ignored outright on a production build so a stray env var cannot re-open the bypass next to the real signing key. NOTE: this changes behaviour. A testnet/staging deployment that relied on the implicit bypass must now set RISKA_ALLOW_UNSIGNED_BROWSER_WALLET=true, which is why it is called out here rather than buried. 2. scripts/build_whitepaper_pdf.py prepended /tmp/codex_pdf_build to sys.path before importing reportlab. /tmp is world-writable, so on a shared or CI host another user could plant a module there and execute code as whoever runs the build, with access to the .env secrets in that process. Removed; it now uses the normally installed reportlab. CWE-426. Also logs, on the "proof of human is missing or is not bound" path, the expected identifier and protocol version, the expected vs received signal hashes, and the received identifiers. That single message currently conflates three distinct failures — wrong protocol version, signal mismatch, and an identity that already opened a policy — which is what made this expensive to diagnose. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Someone is attempting to deploy a commit to the p2pdevmedia's projects Team on Vercel. A member of the Team first needs to authorize it. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ba57cdb7e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const allowsUnsignedBrowserWallet = | ||
| process.env.RISKA_ALLOW_UNSIGNED_BROWSER_WALLET === "true" && | ||
| process.env.NODE_ENV !== "production"; |
There was a problem hiding this comment.
Because this gates the escape hatch on NODE_ENV, the TEST browser-wallet flow remains unusable in any built staging/testnet deployment where NODE_ENV is production. The browser-wallet path in WalletAuth only calls connectWallet(...) and never creates the HTTP-only riska_wallet_session, so even with RISKA_ALLOW_UNSIGNED_BROWSER_WALLET=true this endpoint returns wallet_session_required instead of allowing the World ID simulator/reviewer-demo path. Please key the bypass off the requested deployment, or another dedicated app environment, rather than the Next build mode.
Useful? React with 👍 / 👎.
|
Cerrado a pedido. La corrección quedó committeada en fbfa17e; no se mergea a main. |
2 participants
Dos hallazgos de una revisión de seguridad del repo, más instrumentación para el camino de error que hizo difícil debuggear el sandbox de World ID.
1. Bypass de sesión de wallet al emitir la autorización · CWE-639
En
verify-policy-human-handler.ts, el chequeo de sesión SIWE se saltea cuando el deployment no es producción:Con eso, un deployment de staging/testnet confía en
body.walletAddresscomo titular de la póliza. El signal de World ID solo prueba que un humano ligó su prueba a esa dirección, no que la controle — así que un llamante puede obtener una autorización EIP-712 firmada con la clave del protocolo nombrando a un titular arbitrario.El impacto está acotado (canjearla on-chain igual requiere controlar esa wallet, y quema el nullifier propio), pero deja de estarlo si la clave de firma de staging llegara a ser la de producción.
Ahora la sesión es obligatoria siempre. El sandbox de navegador sigue necesitando funcionar —fuera de World App no hay MiniKit Wallet Auth, así que nunca puede existir una sesión SIWE— así que mantuve la salida de emergencia pero invertida:
NODE_ENV, ni por el entorno de World ID. Esa apertura implícita era el bug.RISKA_ALLOW_UNSIGNED_BROWSER_WALLET=true.2. Ruta
/tmpinyectada en el import de Python · CWE-426scripts/build_whitepaper_pdf.pyhacía:antes de importar
reportlab./tmpes de escritura pública y esa ruta no es específica del usuario, así que en un host compartido o de CI otro usuario puede dejar ahí unreportlab/__init__.pymalicioso y ejecutar código con los permisos de quien corre el build — incluyendo acceso a los secretos del.envcargados en ese proceso. Al estar en el índice 0, tiene precedencia sobre el paquete legítimo.Eliminado; ahora usa el
reportlabinstalado normalmente.3. Diagnóstico del camino de error
En la rama de "World ID proof of human is missing or is not bound to the connected wallet" ahora se loguea el identifier y el protocolo esperados, el signal hash esperado vs. los recibidos, y los identifiers que volvieron.
Ese mensaje hoy mezcla tres fallas distintas —protocolo equivocado, signal que no coincide, e identidad que ya abrió póliza— y fue lo que más caro salió al diagnosticar el sandbox. Si preferís esto en un PR aparte, lo separo sin problema.
Relacionado: #14 (fixes del sandbox del simulador). Son independientes y se pueden mergear en cualquier orden.