Remove SSRF protection for internal deployment#20
Conversation
Add configurable SSRF protection bypass for trusted deployments that need to fetch from local tar1090 servers on private networks. Changes: - Add ALLOW_PRIVATE_NETWORKS environment variable (default: false) - Skip private network validation when enabled - Update docker-compose.yml with new environment variable - Add Configuration section to README with security notes This maintains security by default while allowing internal deployments to disable the restriction for legitimate local network access.
Pull Request Review: Add ALLOW_PRIVATE_NETWORKS environment variableSummaryThis PR adds a configurable environment variable to allow bypassing SSRF protection for trusted internal deployments. The implementation is sound and maintains security by default. ✅ StrengthsSecurity Approach
Code Quality
Documentation
🔍 Issues & Recommendations1. Code Duplication (Medium Priority)Issue: The SSRF protection logic is duplicated between Recommendation: Extract the SSRF validation into a reusable function: /// @brief Validates server URL for SSRF protection
/// @param serverUrl URL object to validate
/// @param isAdsbLol Whether the URL is for adsb.lol
/// @param allowPrivateNetworks Whether to skip private network checks
/// @return Object with {valid: boolean, error?: string}
async function validateServerUrl(serverUrl, isAdsbLol, allowPrivateNetworks) {
if (isAdsbLol || allowPrivateNetworks) {
return { valid: true };
}
const hostname = serverUrl.hostname;
const privateIPv4Ranges = [
/^127\./,
/^10\./,
/^172\.(1[6-9]|2[0-9]|3[0-1])\./,
/^192\.168\./,
/^169\.254\./,
/^0\.0\.0\.0$/,
/localhost/i
];
// ... rest of validation logic
return { valid: true };
}Then use it in both endpoints: const validation = await validateServerUrl(serverUrl, isAdsbLol, allowPrivateNetworks);
if (!validation.valid) {
return res.status(400).json({ error: validation.error });
}Benefits:
2. Missing Test Coverage (High Priority)Issue: No tests verify the new Recommendation: Add tests to describe('ALLOW_PRIVATE_NETWORKS Environment Variable', () => {
test('private network check respects allowPrivateNetworks=true', () => {
const allowPrivateNetworks = true;
const isAdsbLol = false;
const hostname = '192.168.1.100';
// Should skip validation when allowPrivateNetworks is true
if (!isAdsbLol && !allowPrivateNetworks) {
// validation logic
}
// Expect no error when allowPrivateNetworks=true
expect(true).toBe(true);
});
test('private network check blocks by default', () => {
const allowPrivateNetworks = false;
const isAdsbLol = false;
const hostname = '192.168.1.100';
const privateIPv4Ranges = [/^192\.168\./];
const shouldBlock = privateIPv4Ranges.some(range => range.test(hostname));
expect(shouldBlock).toBe(true);
});
test('environment variable parsing handles string values', () => {
// Simulate process.env behavior
const testTrue = 'true' === 'true';
const testFalse = 'false' === 'true';
const testUndefined = undefined === 'true';
expect(testTrue).toBe(true);
expect(testFalse).toBe(false);
expect(testUndefined).toBe(false);
});
});3. Logging for Security Auditing (Medium Priority)Issue: When the private network bypass is enabled, there's no logging to track when it's used. This makes security auditing difficult. Recommendation: Add logging when the bypass is active: // At startup (after line 20)
const allowPrivateNetworks = process.env.ALLOW_PRIVATE_NETWORKS === 'true';
if (allowPrivateNetworks) {
console.warn('[SECURITY] ALLOW_PRIVATE_NETWORKS is enabled - private network SSRF protection disabled');
}
// When private network requests are made (in both endpoints)
if (isAdsbLol) {
// existing adsb.lol logic
} else if (allowPrivateNetworks) {
console.log(`[SECURITY] Private network access allowed for: ${serverUrl.hostname}`);
} else {
// existing SSRF protection logic
}Benefits:
4. Minor: Inconsistent Variable Naming (Low Priority)Issue: The isPrivateIP function exists but isn't consistently used. Observation: Lines 117-125 and 287-295 define Recommendation: Consider using the existing 5. Documentation: Default Value Clarity (Low Priority)Issue: The docker-compose.yml explicitly sets Suggestion: Either:
environment:
# SSRF protection enabled by default
# Set to 'true' for trusted internal deployments
- ALLOW_PRIVATE_NETWORKS=false🧪 Testing Verification NeededThe PR description mentions testing was performed:
However, I'd recommend:
📊 Overall AssessmentCode Quality: 7/10
Security: 9/10
Testing: 4/10
Documentation: 9/10
🎯 Recommendations Priority
✅ Approval StatusThe PR is functionally correct and maintains security by default. The code duplication and missing tests are areas for improvement but don't block the core functionality. Recommendation: Approve with suggestions for follow-up improvements, particularly around test coverage and refactoring the duplicated SSRF logic. Great work on maintaining security by default while enabling legitimate use cases! The approach is sound and well-documented. Consider the refactoring and testing suggestions for future maintainability. |
Add ssrf-protection.test.js with test coverage for: - Private IP detection (localhost, 10.x, 192.168.x, 172.16-31.x, 169.254.x, IPv6) - SSRF validation logic with both ALLOW_PRIVATE_NETWORKS settings - Real-world scenarios (RETINA deployment, AWS metadata, K8s services) - Environment variable parsing and edge cases All tests pass (59 test cases total covering the security model).
Pull Request Review: Add ALLOW_PRIVATE_NETWORKS Environment VariableSummaryThis PR adds a configurable SSRF protection bypass for trusted deployments. The implementation is well-thought-out and follows secure-by-default principles. Overall, this is a solid enhancement with good test coverage. ✅ StrengthsSecurity Design
Code Quality
🔍 Issues & Recommendations1. Critical: Code Duplication in SSRF ValidationThe SSRF validation logic is duplicated across:
Impact: High risk of divergence if one endpoint is updated but not the other. Recommendation: Extract SSRF validation into a reusable function: async function validateServerUrl(serverUrl, allowPrivateNetworks) {
// Existing validation logic here
// Return { valid: boolean, error: string }
}Location: Consider adding to 2. Minor: Test File IntegrationThe new test file Recommendation: Consider adding integration tests that:
This could be added to 3. Minor: Missing IPv6 Private RangeThe Recommendation: Ensure SSRF validation uses the same 4. Documentation: docker-compose.yml CommentThe Recommendation: Add an inline comment: environment:
# Set to 'true' only for trusted internal deployments (see README Configuration section)
- ALLOW_PRIVATE_NETWORKS=false5. Security: DNS Rebinding RiskThe current implementation checks DNS resolution at request time (lines 156-181, 326-351). An attacker could potentially exploit DNS rebinding:
Note: This is a pre-existing issue, not introduced by this PR, but the bypass flag makes it more relevant. Recommendation: Consider pinning the resolved IP address or validating after the fetch completes. This is a separate enhancement and shouldn't block this PR. 6. Minor: Environment Variable Case SensitivityThe check Current Behavior: Documented correctly in tests (lines 286-302). Recommendation: This is acceptable if intentional. Consider documenting the exact expected value format in README. 🔒 Security AssessmentThreat Model✅ SSRF to internal metadata services (169.254.169.254): Blocked by default Risk Profile
📝 Test CoverageThe test suite is comprehensive:
Missing: Integration tests with actual server (see recommendation #2) 🎯 Recommendations SummaryMust Address (Before Merge)
Should Address (Before or After Merge)
Nice to Have
✅ ConclusionThis is a well-designed and well-tested enhancement that addresses a legitimate use case while maintaining security as the default. The main concern is code duplication that should be refactored to prevent future maintenance issues. Recommendation: Approve with request to address the SSRF validation duplication before merge. Files Reviewed:
|
This is an internal fork for trusted network deployment, so SSRF protection is not required. Simplified by removing the protection code entirely rather than adding a configuration option. Changes: - Removed private network IP validation from both API endpoints - Removed ALLOW_PRIVATE_NETWORKS environment variable - Removed SSRF protection test suite - Updated README with security considerations and recommendations For production deployments requiring public access, SSRF protection should be re-implemented as part of a security audit.
jonnyspicer
changed the title
Pull Request ReviewOverviewThis PR removes SSRF (Server-Side Request Forgery) protection from the ✅ Positive Aspects
🔴 Critical Issues1. Dead Code - Unused
|
| Category | Rating | Notes |
|---|---|---|
| Functionality | ✅ Good | Achieves stated goal |
| Documentation | ✅ Good | README additions are comprehensive |
| Code Quality | Dead code and unused imports present | |
| Security | Well-documented but could benefit from runtime warnings | |
| Maintainability | Cleanup required for future maintenance |
🎬 Recommendation
Request Changes - Address the dead code and unused imports before merging. The core approach is sound for an internal fork, but the implementation should be cleaner.
Priority fixes:
- Remove
isPrivateIPfunction - Remove unused
dnsandutilimports - (Optional but recommended) Add runtime security warning
These are straightforward cleanup tasks that will take a few minutes but significantly improve code quality and maintainability.
Review conducted following repository conventions from CLAUDE.md. For an internal trusted network deployment, this approach is pragmatic and well-documented.
Removed unused code after SSRF protection removal: - dns import (no longer needed) - promisify from util (no longer needed) - resolve4 and resolve6 constants (unused) - isPrivateIP function (no longer called) All tests pass.
Pull Request Review: Remove SSRF Protection for Internal DeploymentOverviewThis PR removes Server-Side Request Forgery (SSRF) protection from the adsb2dd service to support internal trusted network deployments. The change removes ~184 lines of security code including private IP validation and DNS resolution checks.
|
| Aspect | Rating | Notes |
|---|---|---|
| Code Quality | ✅ Good | Clean removal, good documentation |
| Security | Complete removal of protection | |
| Testing | ❓ Unclear | Need to verify tests pass |
| Documentation | ✅ Good | README clearly documents risks |
| Approach | Configuration better than removal |
Overall: The implementation is clean but the approach is unnecessarily risky. Using a configuration flag would provide the same functionality while maintaining defense-in-depth and future flexibility.
Review generated for PR #20 - offworldlabs/adsb2dd
1 participant
Summary
Removes SSRF (Server-Side Request Forgery) protection to support internal trusted network deployment where adsb2dd needs to fetch from local tar1090 servers on private networks.
Context
This is an internal fork for RETINA radar deployments on trusted private networks. The SSRF protection was blocking legitimate requests to internal tar1090 servers (e.g., 192.168.0.20:8080).
Approach
Rather than adding a configuration option to bypass protection, this PR removes the SSRF protection entirely since:
Changes
/api/ddendpoint/api/synthetic-detectionsendpointSecurity Considerations
The README now documents:
Testing
All existing tests pass. The SSRF protection test suite was removed since the protection no longer exists.
Use Case
Enables RETINA nodes to fetch ADS-B truth data from internal tar1090 servers on private networks, allowing delay-Doppler visualization with ground truth overlay.