Skip to content

Conversation

@pujitm
Copy link
Member

@pujitm pujitm commented Jun 19, 2025

Summary by CodeRabbit

  • New Features

    • Introduced a comprehensive Nginx control script for Unraid OS, enabling advanced server management, SSL certificate handling, and dynamic configuration based on system state. fix: remote access via connect config in rc.nginx webgui#2269
    • Added a utility function to safely execute code with error handling support.
  • Improvements

    • Enhanced logging across remote access, WAN access, and settings services for improved traceability.
    • Added initialization and cleanup hooks to remote access and UPnP services for better lifecycle management.
    • Optimized configuration persistence by batching rapid changes for more efficient updates.
    • Refined URL resolution logic for improved configuration retrieval and error handling.
    • Broadened pattern matching for domain keys in Nginx state parsing.
    • Updated remote access settings to reload the network stack after changes.
    • Simplified remote access and WAN port configuration logic for clarity and accuracy.
    • Improved port mapping logic with explicit error handling in UPnP service.
    • Updated UI and form controls for remote access settings to reflect SSL requirements and access type restrictions.
  • Configuration

    • Updated the default path for module configuration files.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jun 19, 2025

Walkthrough

This update introduces a new Nginx control script for Unraid OS, enhances logging and lifecycle management in several remote access and configuration services, and adjusts configuration persistence logic to use buffered event handling. Minor default path changes and log improvements are also included across various modules.

Changes

File(s) Change Summary
api/src/environment.ts Changed default fallback for PATHS_CONFIG_MODULES environment variable to a new directory.
packages/unraid-api-plugin-connect/src/service/dynamic-remote-access.service.ts Implements OnApplicationBootstrap, adds initialization logic, stricter config typing, and verbose logging.
packages/unraid-api-plugin-connect/src/service/upnp.service.ts Adds onModuleDestroy lifecycle hook to clean up UPnP mappings on module destruction; refactors port mapping logic.
packages/unraid-api-plugin-connect/src/event-handler/wan-access.handler.ts Adds a logger and logs when WAN access is enabled or disabled; removes OnModuleDestroy implementation.
packages/unraid-api-plugin-connect/src/service/static-remote-access.service.ts Updates log statement at start of beginRemoteAccess for clarity and removes redundant dynamic access type check.
packages/unraid-api-plugin-connect/src/service/upnp-remote-access.service.ts Adds verbose log at start of begin() method; updates port retrieval logic and config saving after lease creation.
packages/unraid-api-plugin-connect/src/service/config.persistence.ts Switches from debounceTime to bufferTime for config changes, updates constructor typing, error logging.
packages/unraid-api-plugin-connect/src/service/url-resolver.service.ts Stricter config typing, refactors port retrieval and FQDN URL processing, adds consolidated error handling.
api/src/unraid-api/graph/resolvers/settings/settings.resolver.ts Updates verbose logging after settings update, removes previous restart log.
api/src/store/state-parsers/nginx.ts Broadens regex for nginx FQDN keys.
plugin/source/dynamix.unraid.net/etc/rc.d/rc.nginx Adds comprehensive Nginx control script for Unraid OS with SSL, config generation, and process management.
packages/unraid-api-plugin-connect/src/service/connect-settings.service.ts Injects NetworkService, updates WAN port setting logic, reloads network stack after remote access update, refines UI schema and control states.
packages/unraid-api-plugin-connect/src/test/url-resolver.service.test.ts Updates test mock typing for stricter config service generic parameter.
packages/unraid-shared/src/util/processing.ts Adds makeSafeRunner utility function for safe execution of functions with error handling.

Sequence Diagram(s)

sequenceDiagram
    participant Module as Module
    participant DynamicService as DynamicRemoteAccessService
    participant Config as ConfigService
    participant Logger as Logger

    Module->>DynamicService: onApplicationBootstrap()
    DynamicService->>Config: get('connect.wanAccessEnabled')
    Config-->>DynamicService: return wanAccessEnabled
    alt WAN Access Enabled
        DynamicService->>Config: get('connect.dynamicRemoteAccess.type')
        Config-->>DynamicService: return type
        DynamicService->>DynamicService: setType(type)
        DynamicService->>Logger: log("Set dynamic remote access type")
    end
Loading
sequenceDiagram
    participant UpnpService as UpnpService
    participant Module as Module

    Module->>UpnpService: onModuleDestroy()
    UpnpService->>UpnpService: disableUpnp()
Loading
sequenceDiagram
    participant ConfigService as ConfigService
    participant Persister as ConnectConfigPersister

    ConfigService-->>Persister: emits changes (buffered 25ms)
    Persister->>Persister: Check for 'connect.config' changes
    alt Any found
        Persister->>Persister: persist()
    end
Loading

Possibly related PRs

Suggested reviewers

  • elibosley

Poem

In Unraid’s halls a script appears,
Nginx commands for admins’ cheers.
Logs now verbose, with lifecycle flair,
Configs buffered with utmost care.
Remote access wakes on every start,
While UPnP cleans up its part.
Code refined—let’s raise a toast!
For plugins stable, safe, and robust.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e527f49 and 981bf84.

📒 Files selected for processing (3)
  • packages/unraid-api-plugin-connect/src/service/dynamic-remote-access.service.ts (6 hunks)
  • packages/unraid-api-plugin-connect/src/service/static-remote-access.service.ts (1 hunks)
  • packages/unraid-api-plugin-connect/src/service/upnp-remote-access.service.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/unraid-api-plugin-connect/src/service/static-remote-access.service.ts
  • packages/unraid-api-plugin-connect/src/service/upnp-remote-access.service.ts
  • packages/unraid-api-plugin-connect/src/service/dynamic-remote-access.service.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: Build API
  • GitHub Check: Build Unraid UI Library (Webcomponent Version)
  • GitHub Check: Build Web App
  • GitHub Check: Test API
  • GitHub Check: Analyze (javascript-typescript)
✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@pujitm pujitm force-pushed the fix/remote-access branch from dc14457 to 424c466 Compare June 19, 2025 18:50
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (3)
plugin/source/dynamix.unraid.net/etc/rc.d/rc.nginx (3)

19-19: Remove unused variable

The variable CALLER is defined but never used in the script.

-CALLER="nginx"

808-813: Improve error detection pattern

The grep pattern for detecting bind errors is too broad and could match unrelated log entries.

-      if  tail -10 $SYSLOG | grep -qm1 'Address already in use'; then
+      if  tail -10 $SYSLOG | grep -qm1 'nginx.*bind.*Address already in use'; then

827-833: Add process existence check before pkill

Using pkill unconditionally could have unintended effects if the process doesn't exist.

 nginx_renew(){
   # stop unconditionally
-  pkill --ns $$ -f $NGINX
+  if nginx_running; then
+    pkill --ns $$ -f $NGINX
+    nginx_waitfor_shutdown
+  fi
   # rebuild configuration
   build_ssl
   # start unconditionally
   $NGINX -c $CONF 2>/dev/null
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 424c466 and 278332b.

📒 Files selected for processing (1)
  • plugin/source/dynamix.unraid.net/etc/rc.d/rc.nginx (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: Build Web App
  • GitHub Check: Build Unraid UI Library (Webcomponent Version)
  • GitHub Check: Build API
  • GitHub Check: Test API
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
plugin/source/dynamix.unraid.net/etc/rc.d/rc.nginx (2)

81-95: Well-structured remote access check

Good implementation with proper checks for file existence, command availability, and null values.


341-341: Fix nginx http2 directive syntax

The http2 directive cannot be specified inline with the listen directive in modern nginx versions.

-$(listen $PORTSSL ssl http2)
+$(listen $PORTSSL ssl)
+    http2 on;

Likely an incorrect or invalid review comment.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

♻️ Duplicate comments (2)
plugin/source/dynamix.unraid.net/etc/rc.d/rc.nginx (2)

38-40: NGINX_CUSTOMFA still lacks default value handling.

The variable NGINX_CUSTOMFA is used in Content-Security-Policy headers at lines 136, 148, 188, 204, 218, 264, 286, 309, and 343 but might be undefined if the defaults file doesn't exist or doesn't define it.

Add a default value after loading defaults:

 # Load defaults
 # Defines NGINX_CUSTOMFA for custom Content-Security-Policy frame-ancestors url
 [[ -r $DEFAULTS ]] && . $DEFAULTS
+# Ensure NGINX_CUSTOMFA has a default value
+[[ -z $NGINX_CUSTOMFA ]] && NGINX_CUSTOMFA=""

592-594: curl commands still lack timeout protection.

The curl commands for fetching WAN IPs lack timeouts, which could cause the script to hang if the remote servers are unresponsive.

-      WANIP=$(curl https://wanip4.unraid.net/ 2>/dev/null)
-      WANIP6=$(curl https://wanip6.unraid.net/ 2>/dev/null)
+      WANIP=$(curl --connect-timeout 5 --max-time 10 https://wanip4.unraid.net/ 2>/dev/null)
+      WANIP6=$(curl --connect-timeout 5 --max-time 10 https://wanip6.unraid.net/ 2>/dev/null)
🧹 Nitpick comments (2)
plugin/source/dynamix.unraid.net/etc/rc.d/rc.nginx (2)

758-758: pkill command may affect unintended processes.

The pkill --ns $$ -f $NGINX command could potentially kill other nginx processes outside the intended namespace if process isolation isn't perfect.

Add more specific targeting:

-    pkill --ns $$ -f $NGINX
+    # More targeted process termination
+    if [[ -s $PID ]]; then
+      local pid=$(cat $PID 2>/dev/null)
+      [[ -n $pid && $pid =~ ^[0-9]+$ ]] && kill -KILL $pid 2>/dev/null
+    fi
+    pkill --ns $$ -f "^$NGINX"

808-813: Hardcoded tail and grep pattern may miss edge cases.

The log parsing logic using tail -10 $SYSLOG | grep -qm1 'Address already in use' is fragile and may miss relevant error messages or catch false positives.

Consider more robust error detection:

-      if  tail -10 $SYSLOG | grep -qm1 'Address already in use'; then
+      # Check nginx-specific error logs for binding issues
+      if tail -20 $SYSLOG | grep -q "nginx.*Address already in use\|nginx.*bind.*failed"; then
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 767541b and 7b830a5.

📒 Files selected for processing (3)
  • api/src/store/state-parsers/nginx.ts (2 hunks)
  • packages/unraid-api-plugin-connect/src/service/dynamic-remote-access.service.ts (6 hunks)
  • plugin/source/dynamix.unraid.net/etc/rc.d/rc.nginx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/unraid-api-plugin-connect/src/service/dynamic-remote-access.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
`api/**/*`: Use pnpm ONLY for package management. Always run scripts from api/package.json unless requested. Test suite is VITEST, do not use jest. Run tests with: pnpm --filter ./...

api/**/*: Use pnpm ONLY for package management.
Always run scripts from api/package.json unless requested.
Test suite is VITEST, do not use jest.
Run tests with: pnpm --filter ./api test.

  • api/src/store/state-parsers/nginx.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: Test API
  • GitHub Check: Build Web App
  • GitHub Check: Build API
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
api/src/store/state-parsers/nginx.ts (1)

5-6: LGTM - Regex enhancement improves key matching flexibility.

The addition of optional separators [_-]? broadens the pattern to match variations like "nginx_fqdn" and "nginx-fqdn6", which aligns with the enhanced nginx configuration management described in the summary.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7b830a5 and 72bdae5.

📒 Files selected for processing (2)
  • api/src/store/state-parsers/nginx.ts (1 hunks)
  • packages/unraid-api-plugin-connect/src/service/url-resolver.service.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • api/src/store/state-parsers/nginx.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: Build Web App
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
packages/unraid-api-plugin-connect/src/service/url-resolver.service.ts (3)

118-118: LGTM! Stricter typing improves type safety.

The addition of the true generic parameter makes ConfigService more strict, which aligns with the broader pattern across the codebase.


262-262: LGTM! Cleaner configuration access pattern.

Direct use of configService.getOrThrow is more straightforward than destructuring nested store structures and follows the improved configuration access patterns seen across the PR.


369-369: LGTM! Enhanced error logging improves debugging.

Adding the warning log for error instances provides better visibility into FQDN processing failures, which complements the existing generic error logging.

@pujitm pujitm force-pushed the fix/remote-access branch from 72bdae5 to ee10c15 Compare June 23, 2025 15:36
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/unraid-api-plugin-connect/src/service/connect-settings.service.ts (1)

255-255: Good addition: Network stack reload ensures changes take effect.

Reloading the network stack after remote access configuration changes is a solid operational practice. The async call is properly awaited.

Consider adding error handling around the network service call if network failures shouldn't block the remote access update:

-        await this.networkService.reloadNetworkStack();
+        try {
+            await this.networkService.reloadNetworkStack();
+        } catch (error) {
+            this.logger.warn('Failed to reload network stack after remote access update', error);
+        }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ee10c15 and 74ad1bc.

📒 Files selected for processing (4)
  • packages/unraid-api-plugin-connect/src/service/connect-settings.service.ts (4 hunks)
  • packages/unraid-api-plugin-connect/src/service/upnp-remote-access.service.ts (1 hunks)
  • packages/unraid-api-plugin-connect/src/service/upnp.service.ts (2 hunks)
  • packages/unraid-api-plugin-connect/src/service/url-resolver.service.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/unraid-api-plugin-connect/src/service/upnp-remote-access.service.ts
  • packages/unraid-api-plugin-connect/src/service/upnp.service.ts
  • packages/unraid-api-plugin-connect/src/service/url-resolver.service.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: Build Web App
  • GitHub Check: Test API
  • GitHub Check: Build API
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
packages/unraid-api-plugin-connect/src/service/connect-settings.service.ts (2)

29-29: LGTM: Clean service import addition.

The NetworkService import follows standard patterns and is properly placed with other service imports.


44-45: LGTM: Proper dependency injection.

The NetworkService dependency is correctly injected following NestJS patterns with proper typing and readonly modifier.

@pujitm pujitm marked this pull request as ready for review June 24, 2025 15:30
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/unraid-shared/src/util/processing.ts (1)

16-16: Fix documentation example context issue.

The example uses this.logger without a proper context, which could confuse developers.

-   } else {
-     this.logger.warn(error, 'Uncaught error in network resolver');
-   }
+   } else {
+     console.warn('Uncaught error:', error);
+   }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1d2be77 and 1186525.

📒 Files selected for processing (2)
  • packages/unraid-api-plugin-connect/src/service/url-resolver.service.ts (4 hunks)
  • packages/unraid-shared/src/util/processing.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/unraid-api-plugin-connect/src/service/url-resolver.service.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Build Web App
  • GitHub Check: Build Unraid UI Library (Webcomponent Version)
  • GitHub Check: Build API
  • GitHub Check: Test API
  • GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
packages/unraid-shared/src/util/processing.ts (1)

1-33: Well-designed utility for centralized error handling.

The makeSafeRunner function provides a clean abstraction for safe function execution with centralized error handling. The implementation follows good TypeScript practices with proper use of generics and comprehensive documentation.

pujitm added 2 commits June 24, 2025 13:44
upnp cleanup, which was the original intent of this handler, is handled
by its service
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (6)
plugin/source/dynamix.unraid.net/etc/rc.d/rc.nginx (6)

39-39: NGINX_CUSTOMFA needs a default value.

The variable is loaded from defaults but used later without ensuring it has a value, which can cause issues in Content-Security-Policy headers.

Add a default value after loading defaults:

 # Load defaults
 # Defines NGINX_CUSTOMFA for custom Content-Security-Policy frame-ancestors url
 [[ -r $DEFAULTS ]] && . $DEFAULTS
+# Ensure NGINX_CUSTOMFA has a default value
+[[ -z $NGINX_CUSTOMFA ]] && NGINX_CUSTOMFA=""

56-59: Certificate deletion logic is overly broad and potentially unsafe.

The pattern *\.unraid\.net could match valid certificates unintentionally, creating security risks.

Consider more specific validation:

 if [[ -f $CERTPATH ]]; then
   TMPCERTNAME=$(openssl x509 -noout -subject -nameopt multiline -in $CERTPATH | sed -n 's/ *commonName *= //p')
-  [[ $TMPCERTNAME == *\.unraid\.net ]] && rm $CERTPATH
+  # Only delete legacy unraid.net certificates, not valid ones
+  if [[ $TMPCERTNAME == *\.unraid\.net ]]; then
+    # Additional validation could be added here
+    log "Removing legacy unraid.net certificate: $TMPCERTNAME"
+    rm $CERTPATH
+  fi
 fi

573-575: Hardcoded temporary file paths create security and concurrency risks.

Using fixed paths /tmp/key.pem and /tmp/cert.pem can cause race conditions and security issues with multiple instances.

Use secure temporary files:

-    openssl req -x509 -nodes -days 3650 -newkey rsa:2048 -sha512 -keyout /tmp/key.pem -out /tmp/cert.pem -subj "$openssl_subject" -extensions SAN -config <(cat /etc/ssl/openssl.cnf; printf "[SAN]\nsubjectAltName=${openssl_altname}") &>/dev/null
-    cat /tmp/cert.pem /tmp/key.pem >$SELFCERTPATH
-    rm -f /tmp/cert.pem /tmp/key.pem
+    # Create secure temporary files
+    TMPKEY=$(mktemp)
+    TMPCERT=$(mktemp)
+    openssl req -x509 -nodes -days 3650 -newkey rsa:2048 -sha512 -keyout "$TMPKEY" -out "$TMPCERT" -subj "$openssl_subject" -extensions SAN -config <(cat /etc/ssl/openssl.cnf; printf "[SAN]\nsubjectAltName=${openssl_altname}") &>/dev/null
+    cat "$TMPCERT" "$TMPKEY" >$SELFCERTPATH
+    rm -f "$TMPCERT" "$TMPKEY"

591-593: WAN IP fetching lacks timeout protection.

The curl commands could hang indefinitely if the remote servers are unresponsive.

Add timeouts:

-      WANIP=$(curl https://wanip4.unraid.net/ 2>/dev/null)
-      WANIP6=$(curl https://wanip6.unraid.net/ 2>/dev/null)
+      WANIP=$(curl --connect-timeout 5 --max-time 10 https://wanip4.unraid.net/ 2>/dev/null)
+      WANIP6=$(curl --connect-timeout 5 --max-time 10 https://wanip6.unraid.net/ 2>/dev/null)

700-703: nginx_running function has potential race condition.

The function doesn't validate PID file contents or handle cases where the process check fails.

Improve robustness:

 nginx_running(){
   sleep 0.1
-  [[ -s $PID && -n "$(cat $PID)" && -d "/proc/$(cat $PID)" ]] && return 0 || return 1
+  if [[ -s $PID ]]; then
+    local pid=$(cat $PID 2>/dev/null)
+    if [[ -n $pid && $pid =~ ^[0-9]+$ && -d "/proc/$pid" ]]; then
+      return 0
+    fi
+  fi
+  return 1
 }

868-908: Command-line interface lacks input validation.

The script doesn't validate that exactly one argument is provided, which could cause unexpected behavior.

Add input validation:

+# Validate command line arguments
+if [[ $# -ne 1 ]]; then
+  echo "Usage: $0 check|start|stop|term|restart|reload|renew|update|port|upgrade|rotate|status"
+  exit 1
+fi
+
 case "$1" in
🧹 Nitpick comments (1)
plugin/source/dynamix.unraid.net/etc/rc.d/rc.nginx (1)

541-691: Consider breaking down the build_ssl function.

This 150-line function handles multiple responsibilities and would benefit from decomposition for maintainability.

Consider extracting smaller functions like:

  • generate_self_signed_cert()
  • handle_ca_certificate()
  • handle_tailscale_certificate()
  • write_nginx_config()
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1186525 and e527f49.

📒 Files selected for processing (2)
  • packages/unraid-api-plugin-connect/src/event-handler/wan-access.handler.ts (1 hunks)
  • plugin/source/dynamix.unraid.net/etc/rc.d/rc.nginx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/unraid-api-plugin-connect/src/event-handler/wan-access.handler.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: Test API
  • GitHub Check: Build Web App
  • GitHub Check: Build API
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Cloudflare Pages

@pujitm pujitm merged commit 7bc583b into main Jun 24, 2025
10 checks passed
@pujitm pujitm deleted the fix/remote-access branch June 24, 2025 19:10
@github-actions
Copy link
Contributor

This plugin has been deployed to Cloudflare R2 and is available for testing.
Download it at this URL:

https://preview.dl.unraid.net/unraid-api/tag/PR1422/dynamix.unraid.net.plg

pujitm pushed a commit that referenced this pull request Jul 8, 2025
🤖 I have created a release *beep* *boop*
---


## [4.9.0](v4.8.0...v4.9.0)
(2025-07-08)


### Features

* add graphql resource for API plugins
([#1420](#1420))
([642a220](642a220))
* add management page for API keys
([#1408](#1408))
([0788756](0788756))
* add rclone ([#1362](#1362))
([5517e75](5517e75))
* API key management
([#1407](#1407))
([d37dc3b](d37dc3b))
* api plugin management via CLI
([#1416](#1416))
([3dcbfbe](3dcbfbe))
* build out docker components
([#1427](#1427))
([711cc9a](711cc9a))
* docker and info resolver issues
([#1423](#1423))
([9901039](9901039))
* fix shading in UPC to be less severe
([#1438](#1438))
([b7c2407](b7c2407))
* info resolver cleanup
([#1425](#1425))
([1b279bb](1b279bb))
* initial codeql setup
([#1390](#1390))
([2ade7eb](2ade7eb))
* initialize claude code in codebse
([#1418](#1418))
([b6c4ee6](b6c4ee6))
* move api key fetching to use api key service
([#1439](#1439))
([86bea56](86bea56))
* move to cron v4 ([#1428](#1428))
([b8035c2](b8035c2))
* move to iframe for changelog
([#1388](#1388))
([fcd6fbc](fcd6fbc))
* native slackware package
([#1381](#1381))
([4f63b4c](4f63b4c))
* send active unraid theme to docs
([#1400](#1400))
([f71943b](f71943b))
* slightly better watch mode
([#1398](#1398))
([881f1e0](881f1e0))
* upgrade nuxt-custom-elements
([#1461](#1461))
([345e83b](345e83b))
* use bigint instead of long
([#1403](#1403))
([574d572](574d572))


### Bug Fixes

* activation indicator removed
([5edfd82](5edfd82))
* alignment of settings on ManagementAccess settings page
([#1421](#1421))
([70c790f](70c790f))
* allow rclone to fail to initialize
([#1453](#1453))
([7c6f02a](7c6f02a))
* always download 7.1 versioned files for patching
([edc0d15](edc0d15))
* api `pnpm type-check`
([#1442](#1442))
([3122bdb](3122bdb))
* **api:** connect config `email` validation
([#1454](#1454))
([b9a1b9b](b9a1b9b))
* backport
unraid/webgui[#2269](https://github.com/unraid/api/issues/2269) rc.nginx
update ([#1436](#1436))
([a7ef06e](a7ef06e))
* bigint
([e54d27a](e54d27a))
* config migration from `myservers.cfg`
([#1440](#1440))
([c4c9984](c4c9984))
* **connect:** fatal race-condition in websocket disposal
([#1462](#1462))
([0ec0de9](0ec0de9))
* **connect:** mothership connection
([#1464](#1464))
([7be8bc8](7be8bc8))
* console hidden
([9b85e00](9b85e00))
* debounce is too long
([#1426](#1426))
([f12d231](f12d231))
* delete legacy connect keys and ensure description
([22fe91c](22fe91c))
* **deps:** pin dependencies
([#1465](#1465))
([ba75a40](ba75a40))
* **deps:** pin dependencies
([#1470](#1470))
([412b329](412b329))
* **deps:** storybook v9
([#1476](#1476))
([45bb49b](45bb49b))
* **deps:** update all non-major dependencies
([#1366](#1366))
([291ee47](291ee47))
* **deps:** update all non-major dependencies
([#1379](#1379))
([8f70326](8f70326))
* **deps:** update all non-major dependencies
([#1389](#1389))
([cb43f95](cb43f95))
* **deps:** update all non-major dependencies
([#1399](#1399))
([68df344](68df344))
* **deps:** update dependency @types/diff to v8
([#1393](#1393))
([00da27d](00da27d))
* **deps:** update dependency cache-manager to v7
([#1413](#1413))
([9492c2a](9492c2a))
* **deps:** update dependency commander to v14
([#1394](#1394))
([106ea09](106ea09))
* **deps:** update dependency diff to v8
([#1386](#1386))
([e580f64](e580f64))
* **deps:** update dependency dotenv to v17
([#1474](#1474))
([d613bfa](d613bfa))
* **deps:** update dependency lucide-vue-next to ^0.509.0
([#1383](#1383))
([469333a](469333a))
* **deps:** update dependency marked to v16
([#1444](#1444))
([453a5b2](453a5b2))
* **deps:** update dependency shadcn-vue to v2
([#1302](#1302))
([26ecf77](26ecf77))
* **deps:** update dependency vue-sonner to v2
([#1401](#1401))
([53ca414](53ca414))
* disable file changes on Unraid 7.2
([#1382](#1382))
([02de89d](02de89d))
* do not start API with doinst.sh
([7d88b33](7d88b33))
* do not uninstall fully on 7.2
([#1484](#1484))
([2263881](2263881))
* drop console with terser
([a87d455](a87d455))
* error logs from `cloud` query when connect is not installed
([#1450](#1450))
([719f460](719f460))
* flash backup integration with Unraid Connect config
([#1448](#1448))
([038c582](038c582))
* header padding regression
([#1477](#1477))
([e791cc6](e791cc6))
* incorrect state merging in redux store
([#1437](#1437))
([17b7428](17b7428))
* lanip copy button not present
([#1459](#1459))
([a280786](a280786))
* move to bigint scalar
([b625227](b625227))
* node_modules dir removed on plugin update
([#1406](#1406))
([7b005cb](7b005cb))
* omit Connect actions in UPC when plugin is not installed
([#1417](#1417))
([8c8a527](8c8a527))
* parsing of `ssoEnabled` in state.php
([#1455](#1455))
([f542c8e](f542c8e))
* pin ranges ([#1460](#1460))
([f88400e](f88400e))
* pr plugin promotion workflow
([#1456](#1456))
([13bd9bb](13bd9bb))
* proper fallback if missing paths config modules
([7067e9e](7067e9e))
* rc.unraid-api now cleans up older dependencies
([#1404](#1404))
([83076bb](83076bb))
* remote access lifecycle during boot & shutdown
([#1422](#1422))
([7bc583b](7bc583b))
* sign out correctly on error
([#1452](#1452))
([d08fc94](d08fc94))
* simplify usb listing
([#1402](#1402))
([5355115](5355115))
* theme issues when sent from graph
([#1424](#1424))
([75ad838](75ad838))
* **ui:** notifications positioning regression
([#1445](#1445))
([f73e5e0](f73e5e0))
* use some instead of every for connect detection
([9ce2fee](9ce2fee))


### Reverts

* revert package.json dependency updates from commit 711cc9a for api and
packages/*
([94420e4](94420e4))

---
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants