78 lines
6.1 KiB
Markdown
78 lines
6.1 KiB
Markdown
# SmartProxy Module Problems
|
|
|
|
Based on test analysis, the following potential issues have been identified in the SmartProxy module:
|
|
|
|
## 1. HttpProxy Route Configuration Issue
|
|
**Location**: `ts/proxies/http-proxy/http-proxy.ts:380`
|
|
**Problem**: The HttpProxy is trying to read the 'type' property of an undefined object when updating route configurations.
|
|
**Evidence**: `test.http-forwarding-fix.ts` fails with:
|
|
```
|
|
TypeError: Cannot read properties of undefined (reading 'type')
|
|
at HttpProxy.updateRouteConfigs (/mnt/data/lossless/push.rocks/smartproxy/ts/proxies/http-proxy/http-proxy.ts:380:24)
|
|
```
|
|
**Impact**: Routes with `useHttpProxy` configuration may not work properly.
|
|
|
|
## 2. Connection Forwarding Issues
|
|
**Problem**: Basic TCP forwarding appears to not be working correctly after the simplification to just 'forward' and 'socket-handler' action types.
|
|
**Evidence**: Multiple forwarding tests timeout waiting for data to be forwarded:
|
|
- `test.forwarding-fix-verification.ts` - times out waiting for forwarded data
|
|
- `test.connection-forwarding.ts` - times out on SNI-based forwarding
|
|
**Impact**: The 'forward' action type may not be properly forwarding connections to target servers.
|
|
|
|
## 3. Missing Certificate Manager Methods
|
|
**Problem**: Tests expect `provisionAllCertificates` method on certificate manager but it may not exist or may not be properly initialized.
|
|
**Evidence**: Multiple tests fail with "this.certManager.provisionAllCertificates is not a function"
|
|
**Impact**: Certificate provisioning may not work as expected.
|
|
|
|
## 4. Route Update Mechanism
|
|
**Problem**: The route update mechanism may have issues preserving certificate manager callbacks and other state.
|
|
**Evidence**: Tests specifically designed to verify callback preservation after route updates.
|
|
**Impact**: Dynamic route updates might break certificate management functionality.
|
|
|
|
## 5. Route-Specific Security Not Fully Implemented
|
|
**Problem**: While the route definitions support security configurations (ipAllowList, ipBlockList, authentication), these are not being enforced at the route level.
|
|
**Evidence**:
|
|
- SecurityManager has methods like `isIPAuthorized` for route-specific security
|
|
- Route connection handler only checks global IP validation, not route-specific security rules
|
|
- No evidence of route.action.security being checked when handling connections
|
|
**Impact**: Route-specific security rules defined in configuration are not enforced, potentially allowing unauthorized access.
|
|
**Status**: ✅ FIXED - Route-specific IP allow/block lists are now enforced when a route is matched. Authentication is logged as not enforceable for non-terminated connections.
|
|
**Additional Fix**: Removed security checks from route matching logic - security is now properly enforced AFTER a route is matched, not during matching.
|
|
|
|
## 6. Security Property Location Consolidation
|
|
**Problem**: Security was defined in two places - route.security and route.action.security - causing confusion.
|
|
**Status**: ✅ FIXED - Consolidated to only route.security. Removed action.security from types and updated all references.
|
|
|
|
## Recommendations
|
|
|
|
1. **Verify Forward Action Implementation**: Check that the 'forward' action type properly establishes bidirectional data flow between client and target server. ✅ FIXED - Basic forwarding now works correctly.
|
|
|
|
2. **Fix HttpProxy Route Handling**: Ensure that route objects passed to HttpProxy.updateRouteConfigs have the expected structure with all required properties. ✅ FIXED - Routes now preserve their structure.
|
|
|
|
3. **Review Certificate Manager API**: Ensure all expected methods exist and are properly documented.
|
|
|
|
4. **Add Integration Tests**: Many unit tests are testing internal implementation details. Consider adding more integration tests that test the public API.
|
|
|
|
5. **Implement Route-Specific Security**: Add security checks when a route is matched to enforce route-specific IP allow/block lists and authentication rules. ✅ FIXED - IP allow/block lists are now enforced at the route level.
|
|
|
|
6. **Fix TLS Detection Logic**: The connection handler was treating all connections as TLS. This has been partially fixed but needs proper testing for all TLS modes.
|
|
|
|
## 7. HTTP Domain Matching Issue
|
|
**Problem**: Routes with domain restrictions fail to match HTTP connections because domain information is only available after HTTP headers are received, but route matching happens immediately upon connection.
|
|
**Evidence**:
|
|
- `test.http-port8080-forwarding.ts` - "No route found for connection on port 8080" despite having a matching route
|
|
- HTTP connections provide domain info via the Host header, which arrives after the initial TCP connection
|
|
- Route matching in `handleInitialData` happens before HTTP headers are parsed
|
|
**Impact**: HTTP routes with domain restrictions cannot be matched, forcing users to remove domain restrictions for HTTP routes.
|
|
**Root Cause**: For non-TLS connections, SmartProxy attempts to match routes immediately, but the domain information needed for matching is only available after parsing HTTP headers.
|
|
**Status**: ✅ FIXED - Added skipDomainCheck parameter to route matching for HTTP proxy ports. When a port is configured with useHttpProxy and the connection is not TLS, domain validation is skipped at the initial route matching stage, allowing the HttpProxy to handle domain-based routing after headers are received.
|
|
|
|
## 8. HttpProxy Plain HTTP Forwarding Issue
|
|
**Problem**: HttpProxy is an HTTPS server but SmartProxy forwards plain HTTP connections to it via `useHttpProxy` configuration.
|
|
**Evidence**:
|
|
- `test.http-port8080-forwarding.ts` - Connection immediately closed after forwarding to HttpProxy
|
|
- HttpProxy is created with `http2.createSecureServer` expecting TLS connections
|
|
- SmartProxy forwards raw HTTP data to HttpProxy's HTTPS port
|
|
**Impact**: Plain HTTP connections cannot be handled by HttpProxy, despite `useHttpProxy` configuration suggesting this should work.
|
|
**Root Cause**: Design mismatch - HttpProxy is designed for HTTPS/TLS termination, not plain HTTP forwarding.
|
|
**Status**: Documented. The `useHttpProxy` configuration should only be used for ports that receive TLS connections requiring termination. For plain HTTP forwarding, use direct forwarding without HttpProxy. |