Security + performance audit fixes (19 fixed, 2 partial, 3 deferred)#249
Merged
Conversation
Deep security + performance audit of frontend, gateway/API, infra, MCP, and connector. 19 findings fixed, 2 partial, 3 deferred. All fixes were adversarially re-reviewed; the review's corrections are folded in. Security - approval: gate resolve_permission on membership/admin OR bot-owner and purge stale approval_delegations + RESPOND grants on member removal (#1) - agent_bridge: require bot channel membership in handle_acp_event_frame before any store write / event-log / broadcast (#3) - ratelimit: bound eviction (retain live windows + hard-clear fallback) (#12) - email: never log one-time codes when an email provider is configured (#13) - CSP: add default/script/style/img/font/connect/media-src; allow https images and blob media (#17) - connector: cap self-update downloads, monotonic anti-downgrade guard, bound the loopback pre-auth request body (413) (#14, #24, #15) - workspace: escape LIKE wildcards; files: normalize content_type to a safe allowlist (#22, #23) Performance - auth: run all bcrypt hash/verify on spawn_blocking, incl. admin seed (#2) - db: bounded unread/mention scan (99+), partial index for pending permissions, batched Fleet policy resolution (#6, #7, #5) - gateway: TTL cache for per-trace bot rules, presence roster cache (#8, #9) - frontend: plain-text streaming render + memoized MarkdownRenderer, stable channelFiles memo, offline stream-finalize guard (#10, #11) - connector: Value::take / mem::take to drop redundant deep clones (#20, #21) Deferred (documented): #4 Redis-backed cross-replica limiter (interim: replicaCount=1), #16 httpOnly-cookie session token, #18 Arc fan-out. Verified offline: server cargo check + 172/172 lib tests; connector cargo check + 98/98 tests; mcp-server cargo check; frontend typecheck (changed files clean) + 66/66 vitest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cargo fmt --check on the agent packages requires the (mem::take, clone) tuple from the #21 fix to wrap across lines. Formatting only, no behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes from a deep security + performance audit across all six components (frontend, gateway/API, infra, MCP, connector). The findings were produced by a multi-agent audit, each adversarially verified, and every fix here was then re-reviewed by a second adversarial pass — which caught real bugs in the first-pass fixes (see Review corrections below) that are folded in.
19 fixed · 2 partial · 3 deferred.
Verification (offline)
cargo check✅ · 172/172 lib testscargo check✅ · 98/98 testscargo check✅PdfViewer.tsxpdfjs errors remain, untouched)Findings addressed
Security
resolve_permissiongated on membership/admin OR bot-owner; staleapproval_delegations+ RESPOND grants purged on member removal — closes the "removed delegate keeps approver rights" hole at the source.handle_acp_event_framenow requires bot channel membership before any store write / event-log / broadcast.default/script/style/img/font/connect/media-src), withhttps:images andblob:media allowed so avatars/audio still load.content_typenormalized to a safe allowlist.Performance
spawn_blocking(incl. admin seed) — no more reactor stalls on login.99+), partial index for pending-permission scans, batched Fleet policy resolution (removes the O(pending) N+1).MarkdownRenderer(no per-frame full re-parse), stablechannelFilesmemo, offline stream-finalize guard.Value::take/mem::takedrop redundant deep clones on the hot streaming path.Review corrections (first-pass fixes that were wrong, now fixed)
retain-only removed the memory bound under an all-live-key floodmedia-src) and external avatars (nohttps:img)media-src 'self' blob:+https:ensure_memberblocked legitimate non-member bot ownersmember/admin OR bot-ownerseed.rsBehavior changes to note
99+); field names/types unchanged.approval_audit).Deferred (with rationale)
replicaCount: 1.Arc-based fan-out — cross-cutting; excluded to keep this PR's edits non-overlapping.Adds migration
0049_messages_pending_permission_index.sql.🤖 Generated with Claude Code