fix(connect): clear wanport upon disabling remote access
#1624
Conversation
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.
|
Caution Review failedThe pull request is closed. WalkthroughDerives a boolean WAN flag from accessType to gate WAN behaviors, conditionally sets/clears upnpEnabled and wanport, sanitizes allowedUrl for dynamic remote access migration, and validates/falls back WAN port usage when resolving FQDN URLs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant API as ConnectSettingsService
participant Store as SettingsStore
participant Migr as DynamicRemoteAccessMigration
User->>API: updateSettings(input)
API->>API: wanaccessEnabled = (input.accessType == ALWAYS)
API->>API: upnpEnabled = wanaccessEnabled && forwardType==UPNP
API->>API: wanport = wanaccessEnabled && forwardType==STATIC ? input.wanport : null
API->>Store: save({ wanaccess: wanaccessEnabled, upnpEnabled, wanport, ... })
Note right of API: If WAN disabled, ensure wanport cleared
API->>Migr: migrate({ allowedUrl: { type: WAN, ipv4: null, ipv6: null, name: null }})
Migr-->>API: ok
API-->>User: result
sequenceDiagram
autonumber
participant Resolver as UrlResolverService
participant Store as SettingsStore
participant Nginx as NginxConfig
Resolver->>Store: get url/interface and rawWanPort
Resolver->>Resolver: wanport = validateWanPort(rawWanPort)
alt interface == LAN
Resolver->>Nginx: use nginx.httpsPort as portToUse
else
alt wanport valid
Resolver->>Resolver: use wanport as portToUse
else
Resolver->>Nginx: fallback to nginx.httpsPort as portToUse
end
end
Resolver->>Resolver: call getUrlForField(..., portSsl=Number(portToUse))
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Claude finished @pujitm's task —— View job Code Review: Critical Issues AnalysisTodo List
Critical Issue Found: Incorrect Port Handling🚨 CRITICAL BUG in The removal of LAN vs WAN port selection logic introduced a bug with null handling: Problematic change: // Before (correct)
const portToUse = urlType === URL_TYPE.LAN ? nginx.httpsPort : wanport || nginx.httpsPort;
portSsl: Number(portToUse)
// After (incorrect)
portSsl: Number(wanport || nginx.httpsPort)Issue: When The new validation method Other Changes Look Good ✅
Fix needed: Use the new |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts (3)
219-222: Good guard: derive a single source of truth for WAN enablement.Computing
wanaccessEnabledfrominput.accessType === WAN_ACCESS_TYPE.ALWAYSmakes the downstream conditionals straightforward and prevents accidental WAN-side effects when access is disabled.Small naming nit: consider
wanAccessEnabled(camel-case “Access”) for consistency with common TS style.
228-232: Consider proactively nulling wanport when switching to UPNP.When moving from STATIC to UPNP while WAN remains enabled,
wanportis currently left untouched until the UPNP service updates it. If any consumer readswanporteagerly (even whileupnpEnabledis true), it could observe a stale static port during that brief window.Optional: clear
wanportimmediately when selecting UPNP; the UPNP service can then repopulate it.Apply this localized tweak:
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 } +else if (wanaccessEnabled && input.forwardType === WAN_FORWARD_TYPE.UPNP) { + // Avoid transient reuse of a stale static port while UPNP mapping is being (re)applied + this.configService.set('connect.config.wanport', null); +}
130-137: Unreachable check for DYNAMIC access type (now out of band).The guard that throws when
settings.accessType === WAN_ACCESS_TYPE.DYNAMIC && settings.forwardType === WAN_FORWARD_TYPE.STATICappears unreachable given the current UI schema only exposes DISABLED/ALWAYS. If you intend to keep it for defensive API use, add a short comment noting it’s a server-side safeguard; otherwise, consider removing it to reduce confusion.- if ( - !nginx.sslEnabled && - settings.accessType === WAN_ACCESS_TYPE.DYNAMIC && - settings.forwardType === WAN_FORWARD_TYPE.STATIC - ) { - throw new GraphQLError( - 'SSL must be provisioned and enabled for dynamic access and static port forwarding.' - ); - } + // Server-side safeguard kept for potential non-UI callers using legacy DYNAMIC semantics. + // Remove if/when DYNAMIC is fully retired end-to-end. + if ( + !nginx.sslEnabled && + settings.accessType === WAN_ACCESS_TYPE.DYNAMIC && + settings.forwardType === WAN_FORWARD_TYPE.STATIC + ) { + throw new GraphQLError( + 'SSL must be provisioned and enabled for dynamic access and static port forwarding.' + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts
🔇 Additional comments (3)
packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts (3)
223-226: Correct precedence: gate UPNP strictly on WAN being enabled.
upnpEnablednow depends on both WAN being enabled and the selected forward type being UPNP. This avoids leaving UPNP toggled on when WAN access is disabled, aligning with the PR objective.
239-249: Safer migration call: sanitize allowedUrl to WAN with null IPs.Passing a WAN-typed
allowedUrlwith all address fields null avoids persisting or trusting user-provided URL details during the migration path. This makes the transition idempotent and reduces surprises while Dynamic Remote Access remains disabled.
233-237: Fix confirmed: null wanport is treated as “unset” by all consumersThe only runtime consumer of
connect.config.wanportis the URL resolver, which uses a truthiness fallback (wanport || nginx.httpsPort) and thus treatsnull(and0) as “use default” rather than merely checking key presence. No other code paths gate on the mere existence ofwanport.• packages/unraid-api-plugin-connect/src/network/url-resolver.service.ts (line 263–268):
const wanport = this.configService.getOrThrow('connect.config.wanport', { infer: true }); … portSsl: Number(wanport || nginx.httpsPort)
• No other call sites perform presence-only checks onwanport(verified via ripgrep across the repo).With this, the cleared
wanportwhen disabling WAN access will correctly revert to default LAN behavior.
pujitm
force-pushed
the
fix/local-access-fails
branch
from
50d0fdd to
06be9be
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/unraid-api-plugin-connect/src/network/url-resolver.service.ts (1)
262-266: Refactor WAN port lookup to be optional and sanitize its valueThe
getOrThrowcall forconnect.config.wanportwill indeed throw once the key is cleared (when remote access is disabled), breakinggetServerIps(). We need to:
- Replace the strict lookup with an optional
getcall.- Coerce the raw value to an integer.
- Validate it falls within the [1–65535] range, or treat it as “undefined” if not.
- Update the tests that currently mock
getOrThrowto instead mockget.Apply this diff in
packages/unraid-api-plugin-connect/src/network/url-resolver.service.tsaround line 263:- const { nginx } = store.emhttp; - const wanport = this.configService.getOrThrow('connect.config.wanport', { infer: true }); + const { nginx } = store.emhttp; + // Optional WAN port: may be cleared when remote access is disabled + const wanportRaw = this.configService.get('connect.config.wanport', { infer: true }) as + | string + | number + | undefined + | null; + const parsedWanport = + typeof wanportRaw === 'number' + ? wanportRaw + : Number.parseInt((wanportRaw ?? '').toString().trim(), 10); + const wanport = + Number.isInteger(parsedWanport) && parsedWanport > 0 && parsedWanport <= 65535 + ? parsedWanport + : undefined;• Update
packages/unraid-api-plugin-connect/src/__test__/url-resolver.service.test.ts:
– Change instances of(mockConfigService.getOrThrow as Mock)to(mockConfigService.get as Mock)and adjust return values to cover both valid ports and undefined/null cases.
– Add tests for whenwanportis missing or invalid, ensuringgetServerIps()still returns only LAN URLs without throwing.
🧹 Nitpick comments (1)
packages/unraid-api-plugin-connect/src/network/url-resolver.service.ts (1)
334-338: Harden port handling for FQDNs: validate/coerce port and avoid accidental NaN; optionally gate WAN entries when remote access is disabled.
- Ensure the port you pass to getUrlForField is a validated integer within [1..65535] and already numeric to avoid passing NaN/invalid values.
- Optional: If nginx.wanAccessEnabled is false, consider skipping WAN FQDN entries altogether so getRemoteAccessUrl() doesn’t surface a WAN URL when remote access is off.
Apply this diff to keep the port numeric and validated, and to skip WAN entries when disabled:
const urlType = this.getUrlTypeFromFqdn(fqdnUrl.interface); - const portToUse = urlType === URL_TYPE.LAN ? nginx.httpsPort : wanport || nginx.httpsPort; + // Skip WAN entries entirely if WAN access is disabled (optional but safer) + if (urlType === URL_TYPE.WAN && !nginx.wanAccessEnabled) { + return; + } + // Prefer validated WAN port for non-LAN; always use nginx.httpsPort for LAN + const portToUse = + urlType === URL_TYPE.LAN + ? nginx.httpsPort + : (sanitizedWanport ?? nginx.httpsPort); const fqdnUrlToUse = this.getUrlForField({ url: fqdnUrl.fqdn, - portSsl: Number(portToUse), + // portToUse is already a number and validated + portSsl: portToUse, });Note: sanitizedWanport is introduced in the retrieval block below (see separate comment). If you prefer not to introduce a new variable, compute and validate locally, but ensure bounds-checking and avoid Number(undefined).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/unraid-api-plugin-connect/src/network/url-resolver.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
packages/unraid-api-plugin-connect/src/network/url-resolver.service.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
packages/unraid-api-plugin-connect/src/network/url-resolver.service.ts
🔇 Additional comments (1)
packages/unraid-api-plugin-connect/src/network/url-resolver.service.ts (1)
334-334: Good fix: LAN FQDNs now reliably use nginx.httpsPort (443).Prioritizing nginx.httpsPort for LAN interface FQDNs directly addresses the bug where stale WAN ports leaked into LAN URLs after disabling remote access. This aligns with the PR objective.
This reverts commit 06be9be.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
🤖 I have created a release *beep* *boop* --- ## [4.16.0](v4.15.1...v4.16.0) (2025-08-27) ### Features * add `parityCheckStatus` field to `array` query ([#1611](#1611)) ([c508366](c508366)) * generated UI API key management + OAuth-like API Key Flows ([#1609](#1609)) ([674323f](674323f)) ### Bug Fixes * **connect:** clear `wanport` upon disabling remote access ([#1624](#1624)) ([9df6a3f](9df6a3f)) * **connect:** valid LAN FQDN while remote access is enabled ([#1625](#1625)) ([aa58888](aa58888)) * correctly parse periods in share names from ini file ([#1629](#1629)) ([7d67a40](7d67a40)) * **rc.unraid-api:** remove profile sourcing ([#1622](#1622)) ([6947b5d](6947b5d)) * remove unused api key calls ([#1628](#1628)) ([9cd0d6a](9cd0d6a)) * retry VMs init for up to 2 min ([#1612](#1612)) ([b2e7801](b2e7801)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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
Tests
Chores