Skip to content

Security: 5 vulnerability fixes (SSRF, exec bypass, chunked OOM, env UB, eviction)#430

Closed
devatsecure wants to merge 5 commits intoRightNow-AI:mainfrom
devatsecure:pr/security-fixes
Closed

Security: 5 vulnerability fixes (SSRF, exec bypass, chunked OOM, env UB, eviction)#430
devatsecure wants to merge 5 commits intoRightNow-AI:mainfrom
devatsecure:pr/security-fixes

Conversation

@devatsecure
Copy link
Copy Markdown

Summary

  • SSRF fail-open bypass: DNS resolution failure was silently allowing requests. Now fails closed. Also strips URL userinfo (http://user@localhost) and uses url::Url for proper parsing. Unified into single ssrf.rs module shared by web_fetch.rs and host_functions.rs.
  • Exec allowlist shell substitution bypass: $(), backticks, and <()/>() were invisible to extract_all_commands(). Now rejected in allowlist mode with clear error directing users to full mode.
  • Chunked response unbounded memory: Only Content-Length header was checked, absent for chunked transfer encoding. Now also checks bytes().len() after download.
  • ENV_MUTEX inconsistency: 4 set_var/remove_var call sites lacked the ENV_MUTEX guard, risking UB in async context. All wrapped.
  • Delivery receipt eviction drift: Single-bucket eviction could leave total above MAX_RECEIPTS. Now iterates all DashMap buckets.

Test plan

  • All 5 fixes have dedicated unit tests (17 SSRF tests, 5 exec allowlist tests, 1 eviction test)
  • cargo build --workspace --lib passes
  • cargo test --workspace passes (1,767+ tests)
  • cargo clippy --workspace --all-targets -- -D warnings zero warnings

🤖 Generated with Claude Code

devatsecure and others added 5 commits March 8, 2026 13:02
…ipping

- Extract shared ssrf.rs module used by web_fetch.rs and host_functions.rs
- Use url::Url for proper parsing (handles userinfo, IPv6, edge cases)
- Fail CLOSED on DNS resolution failure (was silently allowing)
- Strip userinfo from URLs before hostname extraction
- Unified blocklist across both code paths (was inconsistent)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reject $(), backticks, and <()/>() in allowlist mode since these
embed commands invisible to static command extraction. Full mode
is unaffected. Clear error message directs users to full mode.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Previously only checked Content-Length header, which is absent for
chunked transfer encoding. Now also checks actual body size after
download via bytes(). Prevents memory exhaustion from large chunked
responses.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Four call sites were using set_var/remove_var without the ENV_MUTEX
guard, creating potential UB in multi-threaded async context.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Single-bucket eviction could leave total above MAX_RECEIPTS when
the picked bucket had fewer entries than needed. Now iterates across
all buckets until total is within bounds.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@jaberjaber23 jaberjaber23 added the under-review PR is under review label Mar 11, 2026
@jaberjaber23
Copy link
Copy Markdown
Member

thanks for the security audit! we're reviewing each fix individually against our existing security hardening. will keep you posted

@jaberjaber23
Copy link
Copy Markdown
Member

Important security improvements but has merge conflicts across multiple files. Please rebase onto main and we'll merge promptly.

@devatsecure
Copy link
Copy Markdown
Author

These changes were already merged directly into main. Closing as the rebase produces an empty diff — all 5 commits are in main. No resubmit needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

under-review PR is under review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants