gabriel / musehub public
14-application-security.md markdown
99 lines 6.0 KB
Raw
sha256:a507bd1b3380da1a4a55da9d4e9166d200a8ec4101b9ce3d9ebe9e6aa73d6d36 docs+fix: resolve Section 14 app-security gaps with real co… Sonnet 5 minor ⚠ breaking 8 hours ago

Section 14 — Application Security

Companion to musehub-production-readiness-checklist.md.

Update (2026-09-07) — remaining gaps reviewed with real code inspection

Every item this section previously left as "not reviewed" was actually checked against the code this pass, not just assumed.

SSRF review — resolved, and it's solid

musehub/security/ssrf.py is a dedicated, well-built module: HTTPS-only enforcement, a comprehensive blocked-range list (RFC-1918, loopback, link-local — including the AWS metadata endpoint 169.254.169.254, carrier-grade NAT, IPv6 equivalents), and a two-layer design — check_url_safe() for fast sync validation at model/field-parse time, validate_outbound_url() for full async DNS-resolution checking (blocks DNS-rebinding) immediately before delivery. Confirmed both are actually wired in, not just defined: musehub/models/musehub.py calls check_url_safe on webhook URL fields, musehub/services/musehub_webhook_dispatcher.py calls validate_outbound_url before every delivery attempt. (The original doc guessed this lived in musehub/worker.py — it doesn't; that file is the unrelated background job processor. The real logic is in the webhook dispatcher.)

CSRF review — resolved, genuinely N/A

Grepped the entire codebase for set_cookie/Response.cookie/session-cookie patterns — zero matches. No cookie-based sessions exist anywhere, so the classic CSRF attack vector (browsers automatically attaching cookies to cross-site requests) doesn't apply. This isn't a "likely low risk" hedge anymore — it's confirmed there's nothing for CSRF to exploit.

WebSocket/SSE authentication — resolved, correctly implemented

The MCP endpoint (musehub/api/routes/mcp.py) and the social-feed SSE endpoint (musehub/api/routes/musehub/social.py) both use optional_signed_request/optional_token at the connection level — deliberately allowing anonymous connections for read-only/discovery access — but enforce a required fresh MSign signature deeper in the call for privileged actions (confirmed by reading the actual dispatch logic, not just the dependency declaration). TokenClaims and optional_token in musehub/auth/dependencies.py are just re-exported aliases for MSignContext/optional_signed_request from the real implementation in musehub/auth/request_signing.py — one auth system throughout, not two parallel ones.

MSign replay protection — real finding: it's freshness-only, not true replay prevention

Read musehub/auth/request_signing.py closely. The ts field check is:

if abs(server_ts - ts) > REPLAY_WINDOW_SECONDS:  # default 30s
    raise HTTPException(401, ...)

This bounds how long a captured signed request stays valid — it does not track whether a given (handle, ts, sig) has already been used. Compare against the codebase's own separate nonce mechanism (musehub/services/musehub_auth.py's auth-challenge flow, and MPay's nonce_hex with a DB UniqueConstraint) — both of those genuinely prevent replay via single-use enforcement. Per-request MSign signing has no equivalent. The module's own docstring calling this "replay protection" overstates what it does; it's a freshness/skew check.

Practical severity is real but narrow: exploiting this requires an attacker to capture a validly-signed request in transit within its 30-second window (network position, compromised proxy/logging, or — relevant here, since Cloudflare terminates TLS at the edge and re-encrypts to origin — anyone with edge-layer visibility) and the replayed action needs to have a non-idempotent side effect for the replay to matter. GET requests replaying harmlessly; a captured POST/DELETE replayed within the window would re-execute.

Not fixed in this pass — the real fix (a short-lived (handle, sig) dedup cache, e.g. a Postgres table or Redis key with TTL matching REPLAY_WINDOW_SECONDS, checked on every authenticated request) is genuine engineering work with real performance implications (a write on every authenticated API call, and needs to work correctly across the blue/green instance topology). Flagging precisely rather than either overstating the current risk or quietly leaving the docstring's inaccurate claim uncorrected.

Account/data deletion — a mechanism exists

Confirmed soft-delete via deleted_at on identities (musehub/api/routes/musehub/users.py, consistent with the deleted_at.is_(None) filter already seen in the auth path). Not deeply audited for completeness (e.g. whether all related data — repos, issues, comments — cascades correctly), just confirmed the mechanism isn't entirely absent.

Still open — genuinely needs Gabriel or a dedicated pass, not code-checkable from here

  • [ ] Uploaded content scanning (malware/content scanning beyond size/structure limits)
  • [ ] Data classification and retention policy — organizational decision, not a code question
  • [ ] Correct the misleading "replay protection" docstring language in request_signing.py, and decide whether the dedup-cache fix is worth building before or after launch
  • [ ] Formal OWASP-oriented pass — this section and #161 together cover most of the Top 10 categories already; a dedicated closer-to-launch pass (Section 17) should build on these findings rather than starting cold

What's already solid — confirmed by code review (from the original pass, still true)

  • No SQL injection surface (parameterized queries throughout)
  • Path traversal explicitly rejected in the coordination module
  • Archive/decompression bomb protection (mpack_max_decompressed_bytes, 4 GB cap)
  • Debug mode off by default; /docs//redoc//_debug/memory all correctly gated
  • No passwords stored anywhere (MSign is signature-based)
  • Rate limiting on auth-adjacent endpoints
  • CORS fails closed
  • Security headers and CSP set (X-Frame-Options, X-Content-Type-Options, CSP, HSTS)
  • Thorough, deliberate upload/quota limits (size, commit count, object count, per-user/per-repo quotas, daily upload cap)
File History 1 commit
sha256:a507bd1b3380da1a4a55da9d4e9166d200a8ec4101b9ce3d9ebe9e6aa73d6d36 docs+fix: resolve Section 14 app-security gaps with real co… Sonnet 5 minor 8 hours ago