🔒 Security: Fix Critical Vulnerabilities (RCE, Path Traversal, SSRF) #404
Conversation
- VULN-001: Fix RCE in PDF convert-to-image by using execFile instead of exec - VULN-003: Fix path traversal in git-helper by sanitizing slug input - VULN-004: Add SSRF protection to AI provider proxy with IP validation Security Impact: - Prevents command injection via malicious PDF filenames (CVSS 9.8) - Blocks directory traversal attacks in git sync feature (CVSS 8.6) - Prevents SSRF attacks against internal services (CVSS 8.6)
Includes formal bug reports with: - CVSS scores and attack vector analysis - Step-by-step reproduction instructions - Proof of concept outlines - Impact assessments - Recommended fixes
There was a problem hiding this comment.
Pull request overview
This PR addresses three critical and high-severity security vulnerabilities identified during a security audit: Remote Code Execution via command injection in PDF conversion, path traversal in Git sync operations, and Server-Side Request Forgery in the AI provider proxy.
Key changes:
- Replaced unsafe
exec()withexecFile()in PDF conversion to prevent command injection - Added input sanitization for git repository slugs to prevent path traversal attacks
- Implemented DNS resolution checks and private IP blocking to prevent SSRF attacks
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| workflow/packages/blocks/community/pdf/src/lib/actions/convert-to-image.ts | Replaces exec() with execFile() using array-based arguments to prevent command injection |
| workflow/packages/backend/api/src/app/ee/project-release/git-sync/git-helper.ts | Adds sanitizeSlug() function to strip path traversal sequences from user input |
| workflow/packages/backend/api/src/app/ai/ai-provider-proxy.ts | Implements DNS resolution and private IP range validation to prevent SSRF |
| BUG_REPORT_VULN-001_RCE_PDF_Command_Injection.md | Documentation of the RCE vulnerability and fix approach |
| BUG_REPORT_VULN-002_Auth_Bypass_Webhook.md | Documentation of authentication bypass vulnerability (not fixed in this PR) |
| BUG_REPORT_VULN-003_Path_Traversal_Git_Sync.md | Documentation of path traversal vulnerability and fix approach |
| BUG_REPORT_VULN-004_SSRF_AI_Provider_Proxy.md | Documentation of SSRF vulnerability and fix approach |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async function isPrivateIP(hostname: string): Promise<boolean> { | ||
| try { | ||
| const addresses = await dns.resolve4(hostname) | ||
| for (const ip of addresses) { | ||
| if (PRIVATE_IP_RANGES.some(pattern => pattern.test(ip))) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } catch { | ||
| // If DNS resolution fails, block the request for safety | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
The isPrivateIP function only checks IPv4 addresses via dns.resolve4(), but does not check IPv6 addresses. Attackers could bypass SSRF protection by using IPv6 addresses. Add dns.resolve6() checks to validate IPv6 addresses against the IPv6 patterns already defined in PRIVATE_IP_RANGES.
| /^::1$/, // IPv6 loopback | ||
| /^fe80:/, // IPv6 link-local | ||
| /^fc00:/, // IPv6 unique local | ||
| /^fd/, // IPv6 unique local |
There was a problem hiding this comment.
The IPv6 unique local pattern /^fd/ is incomplete and will match any address starting with 'fd' regardless of what follows. It should be /^fd[0-9a-f]{2}:/i to properly match the fd00::/8 range.
| /^fd/, // IPv6 unique local | |
| /^fd[0-9a-f]{2}:/i, // IPv6 unique local |
| const sanitized = slug | ||
| .replace(/\.\./g, '') // Remove parent directory references | ||
| .replace(/[/\\]/g, '') // Remove path separators | ||
| .replace(/[^a-zA-Z0-9_-]/g, '_'); // Replace other invalid chars with underscore |
There was a problem hiding this comment.
The sanitization removes '..' sequences but doesn't prevent multiple consecutive dots that could still be problematic in some contexts. After removing '..', there could be sequences like '....' that become '..' again. Consider using a whitelist approach or applying the replacement iteratively until no more matches are found, or use a single regex that only allows the valid characters: slug.replace(/[^a-zA-Z0-9_-]/g, '_').
1 participant
Summary
This PR addresses 3 critical/high severity security vulnerabilities discovered during a security audit:
Vulnerabilities Fixed
Changes
1. VULN-001: PDF Convert-to-Image RCE Fix
workflow/packages/blocks/community/pdf/src/lib/actions/convert-to-image.tsexec()with string interpolation allowing command injectionexecFile()and array-based arguments2. VULN-003: Git Sync Path Traversal Fix
workflow/packages/backend/api/src/app/ee/project-release/git-sync/git-helper.tsgitRepo.slugused directly inpath.join()without validationsanitizeSlug()function to strip traversal sequences3. VULN-004: AI Provider Proxy SSRF Fix
workflow/packages/backend/api/src/app/ai/ai-provider-proxy.tsTesting
Impact
These fixes prevent:
Submitted as part of AIxBlock Bug Bounty Program