From 9df6a3f5ebb0319aa7e3fe3be6159d39ec6f587f Mon Sep 17 00:00:00 2001 From: Pujit Mehrotra Date: Mon, 25 Aug 2025 13:21:48 -0400 Subject: [PATCH] fix(connect): clear `wanport` upon disabling remote access (#1624) Resolve #1615 -- lingering wanport caused issue with LAN Access via Connect, because those URL's are constructed using `wanport`, but since WAN access would be disabled, NGINX would not listen on the port. ## Summary by CodeRabbit * **Bug Fixes** * WAN access settings now correctly gate related options; UPnP only enables when WAN access is Always. * Static WAN port is applied only when eligible and is cleared when WAN access is disabled to avoid unintended overrides. * Dynamic remote access migration uses sanitized URLs to prevent propagation of user-provided addressing. * **Chores** * Minor ordering and formatting adjustments to reflect updated precedence and clarify behavior. --- .../src/__test__/url-resolver.service.test.ts | 113 +++++++++++++++++- .../src/network/url-resolver.service.ts | 22 +++- .../connect-settings.service.ts | 24 ++-- 3 files changed, 145 insertions(+), 14 deletions(-) diff --git a/packages/unraid-api-plugin-connect/src/__test__/url-resolver.service.test.ts b/packages/unraid-api-plugin-connect/src/__test__/url-resolver.service.test.ts index 6e91190ee..9b1d5b67d 100644 --- a/packages/unraid-api-plugin-connect/src/__test__/url-resolver.service.test.ts +++ b/packages/unraid-api-plugin-connect/src/__test__/url-resolver.service.test.ts @@ -28,7 +28,6 @@ describe('UrlResolverService', () => { describe('getServerIps', () => { it('should return empty arrays when store is not loaded', () => { (mockConfigService.get as Mock).mockReturnValue(null); - (mockConfigService.getOrThrow as Mock).mockReturnValue(null); const result = service.getServerIps(); @@ -144,7 +143,7 @@ describe('UrlResolverService', () => { nginx: { defaultUrl: 'https://default.unraid.net', lanIp: '192.168.1.1', - lanIp6: '2001:db8::1', + lanIp6: 'ipv6.unraid.local', lanName: 'unraid.local', lanMdns: 'unraid.local', sslEnabled: testCase.sslEnabled, @@ -205,8 +204,9 @@ describe('UrlResolverService', () => { }, }; - (mockConfigService.get as Mock).mockReturnValue(mockStore); - (mockConfigService.getOrThrow as Mock).mockReturnValue(443); + (mockConfigService.get as Mock) + .mockReturnValueOnce(mockStore) + .mockReturnValueOnce(443); const result = service.getServerIps(); @@ -259,6 +259,106 @@ describe('UrlResolverService', () => { expect(wanFqdnUrl).toBeDefined(); expect(wanFqdnUrl?.ipv4?.toString()).toBe('https://wan.unraid.net/'); }); + it('should handle invalid WAN port values gracefully', () => { + const testCases = [ + { port: null, description: 'null port' }, + { port: undefined, description: 'undefined port' }, + { port: '', description: 'empty string port' }, + { port: 'invalid', description: 'non-numeric port' }, + { port: 0, description: 'zero port' }, + { port: -1, description: 'negative port' }, + { port: 65536, description: 'port above valid range' }, + { port: 1.5, description: 'non-integer port' }, + ]; + + testCases.forEach(({ port, description }) => { + const mockStore = { + emhttp: { + nginx: { + defaultUrl: 'https://default.unraid.net', + lanIp: '192.168.1.1', + lanIp6: 'ipv6.unraid.local', + lanName: 'unraid.local', + lanMdns: 'unraid.local', + sslEnabled: true, + sslMode: 'yes', + httpPort: 80, + httpsPort: 443, + fqdnUrls: [ + { + interface: 'WAN', + id: null, + fqdn: 'wan.unraid.net', + isIpv6: false, + }, + ], + }, + }, + }; + + (mockConfigService.get as Mock) + .mockReturnValueOnce(mockStore) + .mockReturnValueOnce(port); + + const result = service.getServerIps(); + + // Should fallback to nginx.httpsPort (443) for WAN FQDN URLs + const wanFqdnUrl = result.urls.find( + (url) => url.type === URL_TYPE.WAN && url.name === 'FQDN WAN' + ); + expect(wanFqdnUrl).toBeDefined(); + expect(wanFqdnUrl?.ipv4?.toString()).toBe('https://wan.unraid.net/'); + expect(result.errors).toHaveLength(0); + }); + }); + + it('should use valid WAN port when provided', () => { + const testCases = [ + { port: 1, expected: 'https://wan.unraid.net:1/' }, + { port: 8080, expected: 'https://wan.unraid.net:8080/' }, + { port: 65535, expected: 'https://wan.unraid.net:65535/' }, + { port: '3000', expected: 'https://wan.unraid.net:3000/' }, // string that parses to valid number + ]; + + testCases.forEach(({ port, expected }) => { + const mockStore = { + emhttp: { + nginx: { + defaultUrl: 'https://default.unraid.net', + lanIp: '192.168.1.1', + lanIp6: 'ipv6.unraid.local', + lanName: 'unraid.local', + lanMdns: 'unraid.local', + sslEnabled: true, + sslMode: 'yes', + httpPort: 80, + httpsPort: 443, + fqdnUrls: [ + { + interface: 'WAN', + id: null, + fqdn: 'wan.unraid.net', + isIpv6: false, + }, + ], + }, + }, + }; + + (mockConfigService.get as Mock) + .mockReturnValueOnce(mockStore) + .mockReturnValueOnce(port); + + const result = service.getServerIps(); + + const wanFqdnUrl = result.urls.find( + (url) => url.type === URL_TYPE.WAN && url.name === 'FQDN WAN' + ); + expect(wanFqdnUrl).toBeDefined(); + expect(wanFqdnUrl?.ipv4?.toString()).toBe(expected); + expect(result.errors).toHaveLength(0); + }); + }); }); describe('getRemoteAccessUrl', () => { @@ -287,8 +387,9 @@ describe('UrlResolverService', () => { }, }; - (mockConfigService.get as Mock).mockReturnValue(mockStore); - (mockConfigService.getOrThrow as Mock).mockReturnValue(443); + (mockConfigService.get as Mock) + .mockReturnValueOnce(mockStore) + .mockReturnValueOnce(443); const result = service.getRemoteAccessUrl(); diff --git a/packages/unraid-api-plugin-connect/src/network/url-resolver.service.ts b/packages/unraid-api-plugin-connect/src/network/url-resolver.service.ts index cc4d11657..cc28f6947 100644 --- a/packages/unraid-api-plugin-connect/src/network/url-resolver.service.ts +++ b/packages/unraid-api-plugin-connect/src/network/url-resolver.service.ts @@ -241,6 +241,25 @@ export class UrlResolverService { }, []); } + /** + * Validates and sanitizes a WAN port value. + * + * @param rawPort - The raw port value from configuration + * @returns A valid port number between 1-65535, or undefined if invalid + */ + private validateWanPort(rawPort: unknown): number | undefined { + if (rawPort == null || rawPort === '') { + return undefined; + } + + const port = Number(rawPort); + if (!Number.isInteger(port) || port < 1 || port > 65535) { + return undefined; + } + + return port; + } + /** * Resolves all available server access URLs from the nginx configuration. * This is the main method of the service that aggregates all possible access URLs. @@ -260,7 +279,8 @@ export class UrlResolverService { } const { nginx } = store.emhttp; - const wanport = this.configService.getOrThrow('connect.config.wanport', { infer: true }); + const rawWanPort = this.configService.get('connect.config.wanport', { infer: true }); + const wanport = this.validateWanPort(rawWanPort); if (!nginx || Object.keys(nginx).length === 0) { return { urls: [], errors: [new Error('Nginx Not Loaded')] }; diff --git a/packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts b/packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts index 5eb1ef514..0b5daee9e 100644 --- a/packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts +++ b/packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts @@ -216,17 +216,28 @@ export class ConnectSettingsService { input.forwardType ); - this.configService.set('connect.config.wanaccess', input.accessType === WAN_ACCESS_TYPE.ALWAYS); - if (input.forwardType === WAN_FORWARD_TYPE.STATIC) { + // Currently, Dynamic Remote Access (WAN_ACCESS_TYPE.DYNAMIC) is not enabled, + // so we treat it as disabled for this condition. + const wanaccessEnabled = input.accessType === WAN_ACCESS_TYPE.ALWAYS; + + this.configService.set( + 'connect.config.upnpEnabled', + wanaccessEnabled && input.forwardType === WAN_FORWARD_TYPE.UPNP + ); + + if (wanaccessEnabled && input.forwardType === WAN_FORWARD_TYPE.STATIC) { this.configService.set('connect.config.wanport', input.port); // when forwarding with upnp, the upnp service will clear & set the wanport as necessary } - this.configService.set( - 'connect.config.upnpEnabled', - input.forwardType === WAN_FORWARD_TYPE.UPNP - ); + + this.configService.set('connect.config.wanaccess', wanaccessEnabled); + // do the wanaccess port-override last; it should have the highest precedence + if (!wanaccessEnabled) { + this.configService.set('connect.config.wanport', null); + } // Use the dynamic remote access service to handle the transition + // currently disabled; this call ensures correct migration behavior. await this.remoteAccess.enableDynamicRemoteAccess({ type: dynamicRemoteAccessType, allowedUrl: { @@ -238,7 +249,6 @@ export class ConnectSettingsService { }); await this.networkService.reloadNetworkStack(); - return true; }