Fix. Scanner. Cloud Analysis. Prevent infinite status polling.

inc/spbc-settings.php

@@ -1284,8 +1284,8 @@ function spbc_field_scanner__prepare_data__analysis_log(&$table)
                     $analysis_comment = __('Processing: new, preparing to queueing..', 'security-malware-firewall');
                     break;
                 case 'ERROR':
-                    $pscan_status = '<span class="spbcRed">' . __('Checked', 'security-malware-firewall') . '</span>';
-                    $analysis_comment = '<span class="spbcRed">' . __('Files cause errors on execution.', 'security-malware-firewall') . '</span>';
+                    $pscan_status = '<span class="spbcRed">' . __('Error', 'security-malware-firewall') . '</span>';
+                    $analysis_comment = '<span class="spbcRed">' . __('Something went wrong. Please contact support team.', 'security-malware-firewall') . '</span>';
                     break;
                 case 'IN_SCANER':
                     $pscan_status = __('Queued for inspection', 'security-malware-firewall');
@@ -1335,13 +1335,21 @@ function spbc_field_scanner__prepare_data__analysis_log(&$table)
             }
 
             if ( !is_null($row->pscan_estimated_execution_time) ) {
-                $estimated_execution_time = $row->pscan_estimated_execution_time . ' ' . __('second(s)', 'security-malware-firewall');
+                if ( $row->pscan_estimated_execution_time === '0' ) {
+                    $estimated_execution_time = __('Not available', 'security-malware-firewall');
+                } else {
+                    $estimated_execution_time = $row->pscan_estimated_execution_time . ' ' . __('second(s)', 'security-malware-firewall');
+                }
             } else {
                 $estimated_execution_time = $row->pscan_processing_status === 'DONE' ? 'Done' : 'Wait for assessing';
             }
 
             // Filter actions for approved files
-            if ( in_array($row->pscan_status, array('SAFE','DANGEROUS')) || $curr_time - $row->last_sent < 500 ) {
+            if (
+                    $row->pscan_processing_status === 'ERROR' ||
+                    in_array($row->pscan_status, array('SAFE','DANGEROUS')) ||
+                    $curr_time - $row->last_sent < 500
+            ) {
                 unset($row->actions['check_analysis_status']);
             }
 

lib/CleantalkSP/SpbctWP/Scanner/ScannerActions/CloudAnalysisActions.php

@@ -7,14 +7,19 @@
 
 class CloudAnalysisActions
 {
+    /**
+     * Maximum polling period for checking file analysis status (in seconds)
+     * @var int
+     */
+    private const MAX_POLLING_PERIOD_SECONDS = DAY_IN_SECONDS; // 24 hours
     /**
      * tested
      * @param array $response API Response
-     * @param bool $await_estimated_data Do await estimated data set on undone files check
+     * @param bool $polling_limit_reached Whether the polling limit has been reached
      * @return mixed API Response
      * @throws \Exception if validation failed
      */
-    public static function validateAnalysisStatusResponse($response)
+    public static function validateAnalysisStatusResponse($response, $polling_limit_reached = false)
     {
         // Check if API error
         if ( !empty($response['error']) ) {
@@ -56,18 +61,11 @@ public static function validateAnalysisStatusResponse($response)
             }
         }
 
+        $eta_required = !in_array($response['processing_status'], array('DONE', 'ERROR', 'UNKNOWN')) && !$polling_limit_reached;
+
         //estimated time validation
-        if ( $response['processing_status'] !== 'DONE' ) {
-            if ( ! isset($response['estimated_execution_time'])) {
-                throw new \Exception('response provided no estimated scan time');
-            }
-            //todo remove on business decision
-            //if ( ! isset($response['number_of_files'])) {
-            //  throw new Exception('response provided no number of estimated files');
-            //}
-            //if ( ! isset($response['number_of_files_scanned'])) {
-            //  throw new Exception('response provided no number of already scanned files');
-            //}
+        if ( $eta_required && ! isset($response['estimated_execution_time']) ) {
+            throw new \Exception('response provided no estimated scan time');
         }
 
         return $response;
@@ -87,6 +85,11 @@ public static function isStatusUpdaterExcludedFile(array $file_info)
             throw new \Exception('skipped');
         }
 
+        //skip errored files
+        if (isset($file_info['pscan_processing_status']) && $file_info['pscan_processing_status'] === 'ERROR') {
+            throw new \Exception('skipped');
+        }
+
         // skip not queued files
         $pscan_pending_queue = isset($file_info['pscan_pending_queue']) && $file_info['pscan_pending_queue'] == '1';
         if ($pscan_pending_queue) {
@@ -239,12 +242,13 @@ public static function checkFilesAnalysisStatus($file_ids_input = '')
                 continue;
             }
 
+            $polling_limit_reached = self::isPollingLimitReached($file_info);
+
             // Perform API call
             $api_response = SpbcAPI::method__security_pscan_status($spbc->settings['spbc_key'], $file_info['pscan_file_id']);
-
             // Validate API response
             try {
-                $api_response = CloudAnalysisActions::validateAnalysisStatusResponse($api_response);
+                $api_response = CloudAnalysisActions::validateAnalysisStatusResponse($api_response, $polling_limit_reached);
             } catch ( \Exception $exception ) {
                 $out['error_detail'][] = array(
                     'file_path' => $file_info['path'],
@@ -255,7 +259,7 @@ public static function checkFilesAnalysisStatus($file_ids_input = '')
                 continue;
             }
 
-            $update_query = self::filesAnalysisStatusUpdateQuery($api_response, $file_info);
+            $update_query = self::filesAnalysisStatusUpdateQuery($api_response, $file_info, $polling_limit_reached);
             $update_result = $wpdb->query($update_query);
 
             if ( $update_result === false ) {
@@ -279,7 +283,6 @@ public static function checkFilesAnalysisStatus($file_ids_input = '')
 
         // Fill counters
         $out['counters'] = $counters;
-
         return $out;
     }
 
@@ -289,17 +292,17 @@ public static function checkFilesAnalysisStatus($file_ids_input = '')
      * @param array $file_info
      * @return string
      */
-    private static function filesAnalysisStatusUpdateQuery($api_response, $file_info)
+    private static function filesAnalysisStatusUpdateQuery($api_response, $file_info, $polling_limit_reached = false)
     {
         global $wpdb;
 
-        if ( $api_response['processing_status'] !== 'DONE' ) {
+        if ($api_response['processing_status'] !== 'DONE' ) {
             return $wpdb->prepare(
                 'UPDATE ' . SPBC_TBL_SCAN_FILES
                 . ' SET pscan_pending_queue = 0, pscan_processing_status  = %s, pscan_estimated_execution_time  = %s'
                 . ' WHERE pscan_file_id = %s',
-                $api_response['processing_status'],
-                $api_response['estimated_execution_time'],
+                $polling_limit_reached ? 'ERROR' : $api_response['processing_status'],
+                $api_response['estimated_execution_time'] ?? null,
                 $file_info['pscan_file_id']
             );
         }
@@ -315,7 +318,7 @@ private static function filesAnalysisStatusUpdateQuery($api_response, $file_info
                 . ' status = "APPROVED_BY_CLOUD",'
                 . ' pscan_estimated_execution_time = NULL'
                 . ' WHERE pscan_file_id = %s',
-                isset($api_response['file_balls']) ? $api_response['file_balls'] : '{SAFE:0}',
+                $api_response['file_balls'] ?? '{SAFE:0}',
                 $file_info['pscan_file_id']
             );
         }
@@ -332,8 +335,20 @@ private static function filesAnalysisStatusUpdateQuery($api_response, $file_info
             . ' pscan_estimated_execution_time = NULL'
             . ' WHERE pscan_file_id = %s',
             $api_response['file_status'],
-            isset($api_response['file_balls']) ? $api_response['file_balls'] : '{DANGEROUS:0}',
+            $api_response['file_balls'] ?? '{DANGEROUS:0}',
             $file_info['pscan_file_id']
         );
     }
+
+    /**
+     * Check if the polling limit has been reached for a file.
+     * @param array $file_info
+     * @return bool
+     */
+    private static function isPollingLimitReached(array $file_info)
+    {
+        //limit the time of waiting for the analysis result to avoid infinite waiting
+        return isset($file_info['last_sent']) &&
+            (current_time('timestamp') - $file_info['last_sent']) >= self::MAX_POLLING_PERIOD_SECONDS;
+    }
 }

security-malware-firewall.php

@@ -1797,7 +1797,7 @@ function spbc_scanner_update_pscan_files_status()
     $undone_files_list = $wpdb->get_results(
         'SELECT fast_hash'
         . ' FROM ' . SPBC_TBL_SCAN_FILES
-        . ' WHERE pscan_processing_status <> "DONE" AND pscan_processing_status IS NOT NULL',
+        . ' WHERE pscan_processing_status NOT IN ("DONE", "ERROR") AND pscan_processing_status IS NOT NULL',
         ARRAY_A
     );
 

tests/Inc/ScannerTest.php

@@ -102,6 +102,25 @@ public function testSpbcScannerPscanUpdateCheckExclusions()
         $result = CloudAnalysisActions::isStatusUpdaterExcludedFile(['pscan_processing_status' => 'NEW']);
     }
 
+    /**
+     * ERROR-marked files must be excluded from the pscan status updater,
+     * otherwise cron keeps polling the cloud API for them forever
+     * (they never turn into DONE, so the file is never skipped by any other check).
+     *
+     * @test
+     */
+    public function testSpbcScannerPscanUpdateCheckExclusionsErrorStatusIsSkipped()
+    {
+        try {
+            CloudAnalysisActions::isStatusUpdaterExcludedFile(
+                ['pscan_processing_status' => 'ERROR', 'pscan_pending_queue' => 0]
+            );
+            $this->fail('Expected exception not thrown for ERROR status');
+        } catch (\Exception $e) {
+            $this->assertEquals('skipped', $e->getMessage());
+        }
+    }
+
     /**
      * function spbc_scanner_validate_pscan_status_response($response)
      * @test
@@ -122,6 +141,182 @@ public function testSpbcScannerValidatePscanStatusResponse()
         }
     }
 
+    /**
+     * ERROR responses must be accepted even without "estimated_execution_time",
+     * since the cloud does not report an ETA for a failed analysis.
+     *
+     * @test
+     */
+    public function testSpbcScannerValidatePscanStatusResponseErrorWithoutEstimatedTime()
+    {
+        $response = ['processing_status' => 'ERROR'];
+
+        $result = CloudAnalysisActions::validateAnalysisStatusResponse($response);
+
+        $this->assertEquals($response, $result);
+    }
+
+    /**
+     * Regression guard: statuses that are still "in progress" (e.g. NEW)
+     * must keep requiring "estimated_execution_time".
+     *
+     * @test
+     */
+    public function testSpbcScannerValidatePscanStatusResponseInProgressStillRequiresEstimatedTime()
+    {
+        $this->expectException(\Exception::class);
+        $this->expectExceptionMessage('response provided no estimated scan time');
+
+        CloudAnalysisActions::validateAnalysisStatusResponse(['processing_status' => 'NEW']);
+    }
+
+    /**
+     * Once the polling limit is reached the file is about to be crunched to ERROR anyway,
+     * so a missing "estimated_execution_time" must no longer abort the update.
+     *
+     * @test
+     */
+    public function testSpbcScannerValidatePscanStatusResponseSkipsEtaCheckWhenPollingLimitReached()
+    {
+        $response = ['processing_status' => 'NEW'];
+
+        $result = CloudAnalysisActions::validateAnalysisStatusResponse($response, true);
+
+        $this->assertEquals($response, $result);
+    }
+
+    /**
+     * UNKNOWN is a state the cloud reports without any ETA, so it must not require one.
+     *
+     * @test
+     */
+    public function testSpbcScannerValidatePscanStatusResponseUnknownDoesNotRequireEta()
+    {
+        $response = ['processing_status' => 'UNKNOWN'];
+
+        $result = CloudAnalysisActions::validateAnalysisStatusResponse($response);
+
+        $this->assertEquals($response, $result);
+    }
+
+    /**
+     * Regression guard for the update query builder: an ERROR api response
+     * without "estimated_execution_time" must not trigger an "Undefined array key" notice
+     * (would be converted to an exception by the current phpunit configuration)
+     * and must still produce a valid UPDATE query.
+     *
+     * @test
+     */
+    public function testSpbcScannerFilesAnalysisStatusUpdateQueryHandlesMissingEstimatedTimeOnError()
+    {
+        $method = new \ReflectionMethod(CloudAnalysisActions::class, 'filesAnalysisStatusUpdateQuery');
+        $method->setAccessible(true);
+
+        $api_response = ['processing_status' => 'ERROR'];
+        $file_info = ['pscan_file_id' => '123'];
+
+        $query = $method->invoke(null, $api_response, $file_info);
+
+        $this->assertIsString($query);
+        $this->assertStringContainsString('pscan_processing_status  = \'ERROR\'', $query);
+        $this->assertStringContainsString('pscan_file_id = \'123\'', $query);
+    }
+
+    /**
+     * A file that has been awaiting the analysis result for longer than the polling limit
+     * must be written to the DB as ERROR, so the status updater stops polling it.
+     *
+     * @test
+     */
+    public function testSpbcScannerIsPollingLimitReachedDetectsStaleFile()
+    {
+        $method = new \ReflectionMethod(CloudAnalysisActions::class, 'isPollingLimitReached');
+        $method->setAccessible(true);
+
+        $stale = current_time('timestamp') - DAY_IN_SECONDS - 60;
+        $fresh = current_time('timestamp') - HOUR_IN_SECONDS;
+
+        $this->assertTrue($method->invoke(null, ['last_sent' => $stale]));
+        $this->assertFalse($method->invoke(null, ['last_sent' => $fresh]));
+    }
+
+    /**
+     * "last_sent" is stored in WP local time, so the comparison must use local time too.
+     * Using time() instead of current_time('timestamp') shifts the limit by the GMT offset:
+     * with a negative offset files expire too early, with a positive one they expire too late.
+     *
+     * @test
+     */
+    public function testSpbcScannerIsPollingLimitReachedIsTimezoneSafe()
+    {
+        $original_offset = get_option('gmt_offset');
+        $method = new \ReflectionMethod(CloudAnalysisActions::class, 'isPollingLimitReached');
+        $method->setAccessible(true);
+
+        try {
+            // negative offset: time() runs ahead of local time, a fresh file must not expire early
+            update_option('gmt_offset', -5);
+            $sent_20h_ago = current_time('timestamp') - (20 * HOUR_IN_SECONDS);
+            $this->assertFalse(
+                $method->invoke(null, ['last_sent' => $sent_20h_ago]),
+                'A file sent 20 local hours ago must not be expired yet'
+            );
+
+            // positive offset: time() lags behind local time, a stale file must still expire
+            update_option('gmt_offset', 5);
+            $sent_25h_ago = current_time('timestamp') - (25 * HOUR_IN_SECONDS);
+            $this->assertTrue(
+                $method->invoke(null, ['last_sent' => $sent_25h_ago]),
+                'A file sent 25 local hours ago must be expired'
+            );
+        } finally {
+            update_option('gmt_offset', $original_offset);
+        }
+    }
+
+    /**
+     * Reaching the polling limit must crunch any non-final status to ERROR in the update query.
+     *
+     * @test
+     */
+    public function testSpbcScannerFilesAnalysisStatusUpdateQueryCrunchesStatusOnPollingLimit()
+    {
+        $method = new \ReflectionMethod(CloudAnalysisActions::class, 'filesAnalysisStatusUpdateQuery');
+        $method->setAccessible(true);
+
+        $api_response = ['processing_status' => 'UNKNOWN', 'estimated_execution_time' => 10];
+        $file_info = ['pscan_file_id' => '789'];
+
+        $crunched = $method->invoke(null, $api_response, $file_info, true);
+        $this->assertStringContainsString('pscan_processing_status  = \'ERROR\'', $crunched);
+        $this->assertStringNotContainsString('\'UNKNOWN\'', $crunched);
+
+        $not_crunched = $method->invoke(null, $api_response, $file_info, false);
+        $this->assertStringContainsString('pscan_processing_status  = \'UNKNOWN\'', $not_crunched);
+    }
+
+    /**
+     * The polling limit must never override a finished analysis:
+     * a DONE response has to keep its verdict-based query.
+     *
+     * @test
+     */
+    public function testSpbcScannerFilesAnalysisStatusUpdateQueryKeepsDoneVerdictOnPollingLimit()
+    {
+        $method = new \ReflectionMethod(CloudAnalysisActions::class, 'filesAnalysisStatusUpdateQuery');
+        $method->setAccessible(true);
+
+        $query = $method->invoke(
+            null,
+            ['processing_status' => 'DONE', 'file_status' => 'SAFE'],
+            ['pscan_file_id' => '790'],
+            true
+        );
+
+        $this->assertStringContainsString('status = "APPROVED_BY_CLOUD"', $query);
+        $this->assertStringNotContainsString('\'ERROR\'', $query);
+    }
+
     /**
      * function spbc_scanner_get_files_by_category($category, $count = false)
      * @test