Skip to content

fix: resolve 4 critical/high bugs found in security review#168

Closed
HEYALT wants to merge 1 commit intoBigBodyCobain:mainfrom
HEYALT:fix/critical-bugs-164-165-166-167
Closed

fix: resolve 4 critical/high bugs found in security review#168
HEYALT wants to merge 1 commit intoBigBodyCobain:mainfrom
HEYALT:fix/critical-bugs-164-165-166-167

Conversation

@HEYALT
Copy link
Copy Markdown

@HEYALT HEYALT commented May 7, 2026

Summary

Fixes 4 bugs found during security code review.

Changes

Fix #164 — Dead code in decrypt_wormhole_dm_envelope (backend/routers/wormhole.py)

  • Removed duplicate decrypt_wormhole_dm_envelope definition (was defined twice)
  • Removed immediate return that made ~80 lines of MLS guard logic, format negotiation, and transport tier checks unreachable
  • Removed duplicate Pydantic model class definitions
  • Removed unused import main as _m

Fix #165 — HMAC secret leaked in API response (backend/routers/admin.py)

  • api_reset_all_agent_credentials no longer returns the raw OPENCLAW_HMAC_SECRET
  • Replaced with a boolean hmac_regenerated: true flag

Fix #166 — Request body consumed by HMAC middleware (backend/auth.py)

  • _verify_openclaw_hmac() now caches body_bytes on request._body after reading
  • Route handlers on /api/ai/* can now access the body after HMAC verification

Fix #167 — CSP nonce not wired into script-src (frontend/src/middleware.ts)

  • Production CSP now uses 'nonce-${nonce}' instead of 'unsafe-inline'
  • Dev mode retains 'unsafe-inline' 'unsafe-eval' for Next.js HMR
  • Fixed unused _noncenonce parameter

Closes #164, Closes #165, Closes #166, Closes #167

- Fix BigBodyCobain#164: Remove dead code and duplicate function definitions in
  decrypt_wormhole_dm_envelope. The MLS guard logic, format negotiation,
  and transport tier checks now execute instead of being bypassed by
  an immediate return statement.

- Fix BigBodyCobain#165: Stop leaking the raw OPENCLAW_HMAC_SECRET in the API
  response from api_reset_all_agent_credentials. Return a boolean
  confirmation flag instead.

- Fix BigBodyCobain#166: Cache the request body on request._body after HMAC
  verification in _verify_openclaw_hmac() so downstream route
  handlers on /api/ai/* can still read the body.

- Fix BigBodyCobain#167: Wire the CSP nonce into script-src in production mode,
  replacing unsafe-inline with nonce-based restrictions. Dev mode
  retains unsafe-inline for Next.js HMR compatibility.
@BigBodyCobain
Copy link
Copy Markdown
Owner

Thanks for putting this together. I reviewed the patch and agree that several of the issues are legitimate, but I do not want to merge this PR as-is because two parts have regression risk.

The admin.py HMAC response fix looks directionally right: the reset endpoint should not return the raw regenerated secret.

The auth.py request-body caching change also looks low risk, even if Starlette usually caches request.body() internally.

The two risky parts are:

  1. backend/routers/wormhole.py

The duplicate/dead-code issue is real, but the PR changes behavior by replacing the current delegation path with an older inline decrypt implementation. The current delegated main.decrypt_wormhole_dm_envelope() includes newer wormhole behavior like alias unwrapping, alias-binding updates, local DH secret handling, identity alias handling, and legacy dm1 gating. Activating the older inline copy could regress Wormhole/Infonet messaging.

A safer fix would remove the duplicate/dead definitions while keeping the router delegated to the current main.py implementation, or port the full current main.py behavior exactly.

  1. frontend/src/middleware.ts

The CSP nonce issue is valid, but switching production script-src from unsafe-inline to nonce-only is risky until every Next.js inline bootstrap script receives the nonce. We previously had production hydration/blank-shell failures around this area, so I do not want to merge that without a browser-verified production build.

I am open to a revised PR that splits this into safer commits:

  • Fix HMAC reset response.
  • Optionally keep the request body cache change.
  • Clean up wormhole dead code without changing the active decrypt behavior.
  • Leave CSP nonce enforcement for a separate PR with production browser verification.

Thanks again. The review was useful; I just want to avoid shipping a security cleanup that accidentally breaks Wormhole/Infonet or first paint.

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