Skip to content

fix(http): harden http command against SSRF and DoS#36

Merged
calebephrem merged 1 commit into
open-devhub:mainfrom
calebephrem:main
Jul 13, 2026
Merged

fix(http): harden http command against SSRF and DoS#36
calebephrem merged 1 commit into
open-devhub:mainfrom
calebephrem:main

Conversation

@calebephrem

@calebephrem calebephrem commented Jul 13, 2026

Copy link
Copy Markdown
Member

refined version of #35

@devhub-bot devhub-bot Bot added the fix Bug fix label Jul 13, 2026
@beetle-ai

beetle-ai Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary by Beetle

This PR hardens the HTTP command against Server-Side Request Forgery (SSRF) and Denial of Service (DoS) attacks by implementing comprehensive security validations and resource limits. The changes add robust IP filtering, DNS resolution validation, redirect handling, and response size limits to prevent attackers from using the bot to access internal services or exhaust resources.

📁 File Changes Summary (Consolidated across all commits):

File Status Changes Description
src/commands/tools/http.ts Modified +307/-51 Security Hardening: Added comprehensive SSRF/DoS protections including: (1) IPv4/IPv6 blocked IP detection with support for private ranges (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, link-local, metadata endpoints), (2) DNS resolution validation to prevent DNS rebinding attacks, (3) IP pinning to close TOCTOU windows, (4) Suspicious numeric host detection (hex/octal/decimal IP literals), (5) Redirect handling with max 5 hops limit, (6) Request timeout (10s) and response body size limit (2MB), (7) Credential stripping from URLs, (8) Improved error handling with dedicated error embed helper

Total Changes: 1 file changed, +307 additions, -51 deletions

🎯 Key Changes:

  • IPv4 Blocked Range Detection: Comprehensive validation blocking private ranges (0.0.0.0/8, 10.0.0.0/8, 127.0.0.0/8, 169.254.0.0/16, 172.16.0.0/12, 192.168.0.0/16, 100.64.0.0/10, 198.18.0.0/15, 224.0.0.0+)
  • IPv6 Support: Full IPv6 address validation with conversion to BigInt for range checking, including IPv4-mapped addresses and link-local/unique-local/multicast detection
  • DNS Rebinding Prevention: Validates all DNS records before making requests and pins connections to the resolved IP using custom lookup function, eliminating TOCTOU (Time-of-Check-Time-of-Use) vulnerabilities
  • Numeric IP Literal Detection: Blocks suspicious numeric formats (hex: 0x7f000001, decimal: 2130706433, octal: 0177.0.0.1) that some resolvers interpret as valid IPs
  • Redirect Handling: Implements safe redirect following with max 5 hops limit and re-validates each redirect target against security rules
  • Resource Limits: Added 10-second request timeout and 2MB response body size cap to prevent DoS attacks
  • Credential Stripping: Rejects URLs with embedded credentials (http://user:pass@host)
  • Improved Error Handling: Refactored error responses into reusable errorEmbed() helper for consistency

📊 Impact Assessment:

  • Security: ⚠️ CRITICAL IMPROVEMENT - Eliminates multiple SSRF attack vectors (private IP access, DNS rebinding, numeric IP literals, credential injection) and prevents DoS via resource exhaustion. The IP pinning mechanism is particularly robust, preventing attackers from changing DNS records between validation and request execution.
  • Performance: ✅ MINIMAL IMPACT - Added DNS lookup overhead (async, single call per request) and timeout enforcement. The 10s timeout and 2MB limit are reasonable for a Discord bot command. Redirect handling adds minimal overhead with max 5 hops.
  • Maintainability: ✅ IMPROVED - Code is more modular with dedicated helper functions (assertSafeUrl(), isBlockedIp(), ipv6ToBigInt(), readBodyWithLimit(), errorEmbed()). Type safety improved with explicit SafeUrlResult interface. Error handling is now consistent and centralized.
  • Testing: ⚠️ REQUIRES ATTENTION - Complex security logic (IPv6 conversion, CIDR range checking, DNS validation) needs comprehensive unit tests. Edge cases include: IPv4-mapped IPv6 addresses, zone IDs in IPv6, various numeric IP formats, redirect chains, and timeout scenarios. Recommend adding test coverage for all blocked IP ranges and attack vectors.
⚙️ Settings

Severity Threshold: Medium — Balanced feedback — medium and high severity issues only.Change in Settings
Custom Rules: Define your own review rules — Set Custom Rules
PR Summary: Configure PR summary — Change in Settings

📖 User Guide
  • Once repos are connected, PR analysis is automatically enabled. You can disable analysis for this repo from beetleai.dev/analysis
  • Comment @beetle on any PR to start analysis manually
  • Comment @beetle stop to stop any ongoing analysis

Follow us: Beetle · X · LinkedIn

Comment on lines +173 to +176
const records = await dns.lookup(host, { all: true, verbatim: true });
if (!records.length || records.some((r) => isBlockedIp(r.address))) {
throw new Error("Requests to private or local addresses are not allowed.");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DNS lookup validation doesn't account for DNS rebinding attacks that occur between the validation and the actual request. While the code implements IP pinning via the lookup option (line 323), there's a potential race condition window if the DNS record changes between the dns.lookup() call (line 173) and when the pinned lookup is actually used.
Additionally, the dns.lookup() call uses verbatim: true which is good, but the code should verify that all resolved addresses are safe, not just check if any are blocked. The current logic at line 174 (records.some((r) => isBlockedIp(r.address))) correctly rejects if ANY record is blocked, which is the right approach.
However, there's a subtle issue: if DNS returns multiple records and one is blocked, the entire request is rejected. But if all records are safe, only the first one is used (line 178). This is correct behavior, but the code should document this clearly.
Actually, upon further review: The implementation is correct. The IP pinning at line 323 ensures that even if DNS changes between validation and request, the connection uses the validated IP. This is the proper TOCTOU mitigation.
No genuine issue here - the implementation is sound.

📍 This suggestion applies to lines 173-176

Confidence: 4/5

@devhub-bot

devhub-bot Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Note

Linting checks passed successfully 🎉

All formatting and code quality checks are clean.

You're good to merge 🚀

@calebephrem
calebephrem merged commit 04c4f3c into open-devhub:main Jul 13, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant