chore(auth): address PR review feedback from CodeRabbit and jokob-sk

- Reverted unrelated changes to docker-compose.yml, .env, and server/__main__.py to prevent scope creep.
- Added regex sanitization to X-Forwarded-For IP parsing and LDAP identifiers to prevent log injection.
- Validated API_TOKEN before authenticating sessions in front/index.php.
- Delegated hardcoded local admin fallback to LocalProvider.is_fallback_allowed().
- Made FAILED_LOGINS thresholds configurable via UI settings.
- Increased Docker UI test reliability by replacing sleeps with ldapwhoami polling and dynamic container resolution.
- Moved test_ldap.py to test/plugins/test_ldap_ui_config.py for centralized testing structure.

Author: adamoutler

.env

@@ -8,22 +8,6 @@ LOGS_LOCATION=/path/to/docker_logs
 
 PORT=20211
 
-# LDAP Authentication Settings
-# LDAP_ENABLED=true
-# LDAP_SERVER=ldap.example.com
-# LDAP_PORT=389
-# LDAP_USE_SSL=false
-# LDAP_USE_START_TLS=false
-# LDAP_TLS_VERIFY_CERT=true
-# LDAP_CA_CERT_PATH=/etc/ssl/certs/ca-certificates.crt
-# LDAP_DISABLE_LOCAL_ADMIN=false
-# LDAP_DIRECT_BIND_FORMAT=uid={username},ou=users,dc=example,dc=com
-# LDAP_BIND_DN=cn=readonly,dc=example,dc=com
-# LDAP_BIND_PASSWORD=CHANGE_ME
-# LDAP_BASE_DN=ou=users,dc=example,dc=com
-# LDAP_USER_FILTER=(uid={username})
-# LDAP_USERNAME_ATTRIBUTE=uid
-
 #DEVELOPMENT VARIABLES
 
 DEV_LOCATION=/path/to/local/source/code

.github/workflows/run-all-tests.yml

@@ -66,6 +66,7 @@ jobs:
           if [ "${{ github.event.inputs.run_api }}" == "true" ]; then PATHS="$PATHS test/api_endpoints/ test/server/"; fi
           if [ "${{ github.event.inputs.run_backend }}" == "true" ]; then PATHS="$PATHS test/backend/ test/db/"; fi
           if [ "${{ github.event.inputs.run_docker_env }}" == "true" ]; then PATHS="$PATHS test/docker_tests/"; fi
+          if [ "${{ github.event.inputs.run_ldap }}" == "true" ]; then PATHS="$PATHS test/server/test_ldap_provider.py test/server/test_ldap_provider_env.py test/ui/test_ui_ldap_login.py test/docker_tests/test_ldap_ui.py"; fi
           if [ "${{ github.event.inputs.run_ui }}" == "true" ]; then PATHS="$PATHS test/ui/"; fi
           if [ "${{ github.event.inputs.run_plugins }}" == "true" ]; then PATHS="$PATHS test/plugins/"; fi
 
@@ -98,5 +99,4 @@ jobs:
         if: always()
         run: |
           docker stop netalertx-test-container || true
-          docker rm netalertx-test-container || true  docker stop netalertx-test-container || true
           docker rm netalertx-test-container || true

PR_REVIEW_REPORT.md

@@ -0,0 +1,30 @@
+# PR Review Resolution Report
+
+## Reverts (Scope Creep)
+1. `docker-compose.yml` and `.env`: Reverted to `origin/main` to remove unrelated changes and avoid warning about missing variables or obsolete Compose version syntax.
+2. `server/__main__.py`: Reverted `is_idle_this_loop` logic and idle notification to match `origin/main`.
+3. `server/plugin.py`: Verified that it matches `origin/main` (no unrelated `sample_json` or `_run_test_cmd` changes exist).
+
+
+## api_server_start.py Issues
+1. Re-added `localhost` to the CORS and trusted_proxies list to avoid breaking integrations like homepage dashboards.
+2. Made rate-limiting constants (`FAILED_LOGINS` and `MAX_FAILED_ATTEMPTS`) dynamic via `AUTH_MAX_FAILED_ATTEMPTS` and `AUTH_LOCKOUT_TIME`.
+3. Added strict sanitization of `client_ip` from `X-Forwarded-For` using regex to mitigate log injection vectors.
+
+## Flaky UI Tests
+1. Replaced `time.sleep(2)` with a `WebDriverWait` in `test/ui/test_ui_ldap_login.py` as requested by CodeRabbit.
+
+## Docker Test Reliability
+1. Replaced hardcoded container names in `test/docker_tests/test_ldap_ui.py` with dynamic resolution using `docker ps` filters to support Compose V2 correctly.
+2. Removed hardcoded 10-second sleeps and added a bounded polling loop checking LDAP readiness via `ldapwhoami`.
+3. Added strict timeouts to `subprocess.run` commands during Docker test setup to prevent CI hangs.
+
+## Auth Logic Safety
+1. Moved `$_SESSION['login'] = 1` assignment in `front/index.php` to *after* the `API_TOKEN` is successfully resolved to prevent partially authenticated sessions without tokens.
+
+## LDAP Provider Fixes
+1. Updated PHP frontend `LDAP_enabled` check in `security.php` to accept `yes` and `on` in addition to `true` and `1`, matching the backend behavior.
+2. Fixed `server/auth/ldap_provider.py` to strictly check the boolean return of `conn.search()` and fail properly if the LDAP search operation fails, as pointed out by CodeRabbit.
+
+## Code Organization
+1. Moved `test_ldap.py` out of the frontend plugins directory into `test/plugins/test_ldap_ui_config.py` and updated the plugin's `config.json` to reference the new path.

docker-compose.yml

@@ -67,21 +67,6 @@ services:
       GRAPHQL_PORT: ${GRAPHQL_PORT:-20212}                      # GraphQL API port
       ALWAYS_FRESH_INSTALL: ${ALWAYS_FRESH_INSTALL:-false}      # Set to true to reset your config and database on each container start
       NETALERTX_DEBUG: ${NETALERTX_DEBUG:-0}                    # 0=kill all services and restart if any dies. 1 keeps running dead services.
-      # LDAP Authentication Variables
-      LDAP_ENABLED: ${LDAP_ENABLED:-}
-      LDAP_SERVER: ${LDAP_SERVER:-}                             # LDAP server hostname or IP
-      LDAP_PORT: ${LDAP_PORT:-389}
-      LDAP_USE_SSL: ${LDAP_USE_SSL:-false}
-      LDAP_USE_START_TLS: ${LDAP_USE_START_TLS:-false}
-      LDAP_TLS_VERIFY_CERT: ${LDAP_TLS_VERIFY_CERT:-true}
-      LDAP_CA_CERT_PATH: ${LDAP_CA_CERT_PATH:-}
-      LDAP_DISABLE_LOCAL_ADMIN: ${LDAP_DISABLE_LOCAL_ADMIN:-false}
-      LDAP_DIRECT_BIND_FORMAT: ${LDAP_DIRECT_BIND_FORMAT:-}
-      LDAP_BIND_DN: ${LDAP_BIND_DN:-}
-      LDAP_BIND_PASSWORD: ${LDAP_BIND_PASSWORD:-}               # Service account password for LDAP
-      LDAP_BASE_DN: ${LDAP_BASE_DN:-}
-      LDAP_USER_FILTER: ${LDAP_USER_FILTER:-(uid={username})}
-      LDAP_USERNAME_ATTRIBUTE: ${LDAP_USERNAME_ATTRIBUTE:-uid}
 
     # Resource limits to prevent resource exhaustion
     mem_limit: 2048m            # Maximum memory usage

front/index.php

@@ -4,6 +4,7 @@
 <?php
 
 require_once $_SERVER['DOCUMENT_ROOT'].'/php/server/db.php';
+require_once $_SERVER['DOCUMENT_ROOT'].'/php/server/util.php';
 require_once $_SERVER['DOCUMENT_ROOT'].'/php/templates/language/lang.php';
 require_once $_SERVER['DOCUMENT_ROOT'].'/php/templates/security.php';
 
@@ -37,16 +38,17 @@
 } else {
     $ldap_enabled_line = getConfigLine('/^LDAP_enabled.*=/', $configLines);
     if ($ldap_enabled_line !== null && isset($ldap_enabled_line[1])) {
-        $ldap_enabled = strtolower(trim($ldap_enabled_line[1])) === 'true';
+        $ldap_enabled_value = strtolower(trim($ldap_enabled_line[1]));
+        $ldap_enabled = $ldap_enabled_value === 'true' || $ldap_enabled_value === '1';
     }
 }
 
 /**
  * Derive the Python API port from the GRAPHQL_PORT setting in app.conf.
- * Falls back to 20211 (the default) when not set.
+ * Falls back to 20212 (the default) when not set.
  */
 $gql_line = getConfigLine('/^GRAPHQL_PORT.*=/', $configLines);
-$graphql_port = 20211;
+$graphql_port = 20212;
 if ($gql_line !== null && isset($gql_line[1])) {
     $parsed_port = (int) preg_replace('/[^0-9]/', '', $gql_line[1]);
     if ($parsed_port >= 1 && $parsed_port <= 65535) {
@@ -125,12 +127,21 @@ function is_authenticated(): bool {
 
 function login_user(): void {
     global $nax_Password, $api_token;
+
+    $resolved_api_token = !empty($api_token)
+        ? $api_token
+        : (function_exists('getSettingValue') ? getSettingValue('API_TOKEN') : '');
+
+    if (empty($resolved_api_token)) {
+        throw new RuntimeException('API_TOKEN is not configured');
+    }
+
     $_SESSION['login'] = 1;
     session_regenerate_id(true);
     $_SESSION['csrf_token'] = bin2hex(random_bytes(32));
 
     // Set remember-me cookie with HMAC (not raw password hash)
-    $cookie_value = hash_hmac('sha256', $nax_Password, $api_token);
+    $cookie_value = hash_hmac('sha256', $nax_Password, $resolved_api_token);
     setcookie(COOKIE_SAVE_LOGIN_NAME, $cookie_value, [
         'expires'  => time() + 3600 * 24 * 7,
         'path'     => '/',
@@ -174,6 +185,14 @@ function logout_user(): void {
     if ($ldap_enabled) {
         // LDAP path: delegate credential validation to the Python API.
         // The API token is required so only server-side callers can reach the endpoint.
+        $resolved_api_token = !empty($api_token)
+            ? $api_token
+            : (function_exists('getSettingValue') ? getSettingValue('API_TOKEN') : '');
+
+        if (empty($resolved_api_token)) {
+            throw new RuntimeException('API_TOKEN is not configured');
+        }
+
         $ldap_payload = json_encode([
             'username' => isset($_POST['loginusername']) ? trim($_POST['loginusername']) : '',
             'password' => $_POST['loginpassword'],
@@ -182,7 +201,7 @@ function logout_user(): void {
             'http' => [
                 'method'        => 'POST',
                 'header'        => "Content-Type: application/json\r\n"
-                                 . "Authorization: Bearer " . $api_token . "\r\n"
+                                 . "Authorization: Bearer " . $resolved_api_token . "\r\n"
                                  . "X-Forwarded-For: " . ($_SERVER['REMOTE_ADDR'] ?? '127.0.0.1') . "\r\n",
                 'content'       => $ldap_payload,
                 'timeout'       => 5,

front/php/server/query_graphql.php

@@ -12,6 +12,9 @@
 // Helper function to get GraphQL URL (you can replace this with environment variables)
 function getGraphQLUrl() {
     $port = getSettingValue("GRAPHQL_PORT");  // Port for the GraphQL server
+    if (empty($port) || !is_numeric($port)) {
+        $port = 20211;
+    }
     return "0.0.0.0:$port/graphql";  // Full URL to the GraphQL endpoint
 }
 

front/php/templates/auth.php

@@ -29,12 +29,26 @@
 }
 
 $config_file_lines = file($config_file);
-$config_file_lines_pw = array_values(preg_grep('/^SETPWD_password.*=/', $config_file_lines));
-$password_line = explode("'", $config_file_lines_pw[0]);
+$config_file_lines_pw = array_values(preg_grep('/^SETPWD_password\s*=/', $config_file_lines ?: []));
+if (empty($config_file_lines_pw) || substr_count($config_file_lines_pw[0], "'") < 2) {
+    http_response_code(401);
+    echo 'Unauthorized 401';
+    exit;
+}
+$password_line = explode("'", $config_file_lines_pw[0], 3);
 $nax_Password = $password_line[1];
 
-$config_file_lines_token = array_values(preg_grep('/^API_TOKEN.*=/', $config_file_lines));
-$api_token = !empty($config_file_lines_token) ? explode("'", $config_file_lines_token[0])[1] : '';
+$config_file_lines_token = array_values(preg_grep('/^API_TOKEN\s*=/', $config_file_lines ?: []));
+$token_line = !empty($config_file_lines_token) ? explode("'", $config_file_lines_token[0], 3) : [];
+$api_token = $token_line[1] ?? '';
+if (empty($api_token)) {
+    $api_token = getenv('API_TOKEN') ?: '';
+}
+if ($api_token === '') {
+    http_response_code(401);
+    echo 'Unauthorized 401';
+    exit;
+}
 
 $expected_cookie = hash_hmac('sha256', $nax_Password, $api_token);
 if (isset($_COOKIE[$CookieSaveLoginName]) && hash_equals($expected_cookie, $_COOKIE[$CookieSaveLoginName])) {

front/php/templates/security.php

@@ -75,23 +75,28 @@ function redirect($url) {
 $configLines = file(CONFIG_PATH);
 
 // Handle web protection and password
-$nax_WebProtection = strtolower(trim(getConfigLine('/^SETPWD_enable_password.*=/', $configLines)[1] ?? 'false'));
+$nax_WebProtection = strtolower(trim(getConfigValue('/^SETPWD_enable_password\s*=/', $configLines)));
 
 $ldap_enabled = false;
 $env_ldap = getenv('LDAP_ENABLED');
 if ($env_ldap === false) $env_ldap = getenv('LDAP_enabled');
 
 if ($env_ldap !== false && $env_ldap !== '') {
-    $ldap_enabled = strtolower(trim($env_ldap)) === 'true' || trim($env_ldap) === '1';
+    $env_val = strtolower(trim($env_ldap));
+    $ldap_enabled = in_array($env_val, ['true', '1', 'yes', 'on']);
 } else {
-    $ldap_enabled = strtolower(trim(getConfigLine('/^LDAP_enabled.*=/', $configLines)[1] ?? 'false')) === 'true';
+    $val = strtolower(trim(getConfigValue('/^LDAP_enabled\s*=/', $configLines)));
+    $ldap_enabled = in_array($val, ['true', '1', 'yes', 'on']);
 }
 
 if ($ldap_enabled) {
     $nax_WebProtection = 'true';
 }
-$nax_Password = getConfigValue('/^SETPWD_password.*=/', $configLines);
-$api_token = getConfigValue('/^API_TOKEN.*=/', $configLines, "'");
+$nax_Password = getConfigValue('/^SETPWD_password\s*=/', $configLines);
+$api_token = getConfigValue('/^API_TOKEN\s*=/', $configLines, "'");
+if (empty($api_token)) {
+    $api_token = getenv('API_TOKEN') ?: '';
+}
 
 $expectedToken = 'Bearer ' . $api_token;
 

front/plugins/auth_ldap/config.json

@@ -202,7 +202,7 @@
       "description": [
         {
           "language_code": "en_us",
-          "string": "Upgrade a plain-text LDAP connection to TLS using the STARTTLS command.  Typically used with port 389.  Mutually exclusive with LDAPS."
+          "string": "Upgrade a plain-text LDAP connection to TLS using the STARTTLS command. Typically used with port 389. Mutually exclusive with LDAPS.<br><br><b>Recommended: set to true (or use LDAP_USE_SSL=true with port 636) before production.</b>"
         }
       ]
     },
@@ -549,7 +549,7 @@
         ]
       },
       "maxLength": 512,
-      "default_value": "python3 /app/front/plugins/auth_ldap/test_ldap.py --test-user={LDAP_test_username}",
+      "default_value": "python3 /app/test/plugins/test_ldap_ui_config.py --test-user={LDAP_test_username}",
       "options": [],
       "localized": [
         "name",

scripts/run_tests_in_docker_environment.sh

@@ -80,7 +80,7 @@ docker exec netalertx-test-container /bin/bash -c " \
 # --- 9. Execute Tests ---
 echo "--- Executing tests inside the container ---"
 docker exec netalertx-test-container /bin/bash -c " \
-    cd /workspaces/NetAlertX && pytest -m 'not (docker or compose or feature_complete)' --cache-clear -o cache_dir=/tmp/.pytest_cache; \
+    cd /workspaces/NetAlertX && pytest test/ -m 'not (docker or compose or feature_complete)' --cache-clear -o cache_dir=/tmp/.pytest_cache; \
 "
 
 # --- 10. Final Teardown ---