apply deterministic refactor of regex patterns to counter ReDoS

[fiammybe]

User description

Similar to what is in the syntax highlighter. In the future we might put this somewhere in an IPF class

---

PR Type

Bug fix, Enhancement

---

Description

• Replace vulnerable regex patterns with deterministic string-based parsing

• Mitigate ReDoS (Regular Expression Denial of Service) attacks

• Implement replaceTagDeterministic() method for safe tag replacement

• Apply refactor to adsense and customtag preload plugins

---

Diagram Walkthrough

flowchart LR
  A["Vulnerable preg_replace_callback<br/>with regex patterns"] -->|"Replace with"| B["Deterministic string parsing<br/>using strpos/substr"]
  B -->|"Applied to"| C["adsense.php<br/>customtag.php"]
  C -->|"Result"| D["ReDoS vulnerability<br/>mitigated"]

Loading

File Walkthrough

Relevant files

Bug fix

adsense.php Replace regex with deterministic tag replacement                  htdocs/plugins/preloads/adsense.php Replaced two preg_replace_callback() calls with replaceTagDeterministic() method in eventAfterPreviewTarea() and eventAfterDisplayTarea() Added new private static method replaceTagDeterministic() that uses string functions (strpos, substr) instead of regex Method iteratively finds and replaces [adsense]...[/adsense] tags by invoking callback with matches array +32/-4    customtag.php Replace regex with deterministic tag replacement                  htdocs/plugins/preloads/customtag.php Replaced two preg_replace_callback() calls with replaceTagDeterministic() method in eventBeforePreviewTarea() and eventBeforeDisplayTarea() Added new private static method replaceTagDeterministic() using string functions for safe tag parsing Method handles [customtag]...[/customtag] tags with callback invocation matching preg_replace_callback behavior +32/-4

---

[qodo-free-for-open-source-projects]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Infinite loop risk Description: The replaceTagDeterministic function modifies $text in-place during iteration, which can cause the $pos pointer to become misaligned if the replacement string contains the opening tag pattern, potentially leading to infinite loops or skipped matches. adsense.php [92-110] Referred Code private static function replaceTagDeterministic(string $text, string $tag, $callback): string { $open = '[' . $tag . ']'; $close = '[/' . $tag . ']'; $pos = 0; while (($start = strpos($text, $open, $pos)) !== false) { $innerStart = $start + strlen($open); $end = strpos($text, $close, $innerStart); if ($end === false) { break; } $inner = substr($text, $innerStart, $end - $innerStart); $full = substr($text, $start, ($end + strlen($close)) - $start); $replacement = call_user_func($callback, array($full, $inner)); $text = substr($text, 0, $start) . $replacement . substr($text, $end + strlen($close)); $pos = $start + strlen($replacement); } return $text; }
	Infinite loop risk duplicate Description: The replaceTagDeterministic function modifies $text in-place during iteration, which can cause the $pos pointer to become misaligned if the replacement string contains the opening tag pattern, potentially leading to infinite loops or skipped matches. customtag.php [91-109] Referred Code private static function replaceTagDeterministic(string $text, string $tag, $callback): string { $open = '[' . $tag . ']'; $close = '[/' . $tag . ']'; $pos = 0; while (($start = strpos($text, $open, $pos)) !== false) { $innerStart = $start + strlen($open); $end = strpos($text, $close, $innerStart); if ($end === false) { break; } $inner = substr($text, $innerStart, $end - $innerStart); $full = substr($text, $start, ($end + strlen($close)) - $start); $replacement = call_user_func($callback, array($full, $inner)); $text = substr($text, 0, $start) . $replacement . substr($text, $end + strlen($close)); $pos = $start + strlen($replacement); } return $text; }
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Missing error handling: The replaceTagDeterministic method does not validate input parameters or handle potential errors from call_user_func callback execution. Referred Code private static function replaceTagDeterministic(string $text, string $tag, $callback): string { $open = '[' . $tag . ']'; $close = '[/' . $tag . ']'; $pos = 0; while (($start = strpos($text, $open, $pos)) !== false) { $innerStart = $start + strlen($open); $end = strpos($text, $close, $innerStart); if ($end === false) { break; } $inner = substr($text, $innerStart, $end - $innerStart); $full = substr($text, $start, ($end + strlen($close)) - $start); $replacement = call_user_func($callback, array($full, $inner)); $text = substr($text, 0, $start) . $replacement . substr($text, $end + strlen($close)); $pos = $start + strlen($replacement); } return $text; }
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Missing input validation: The replaceTagDeterministic method does not validate the $text parameter or sanitize the $tag parameter before string operations. Referred Code private static function replaceTagDeterministic(string $text, string $tag, $callback): string { $open = '[' . $tag . ']'; $close = '[/' . $tag . ']'; $pos = 0; while (($start = strpos($text, $open, $pos)) !== false) { $innerStart = $start + strlen($open); $end = strpos($text, $close, $innerStart); if ($end === false) { break; } $inner = substr($text, $innerStart, $end - $innerStart); $full = substr($text, $start, ($end + strlen($close)) - $start); $replacement = call_user_func($callback, array($full, $inner)); $text = substr($text, 0, $start) . $replacement . substr($text, $end + strlen($close)); $pos = $start + strlen($replacement); } return $text; }
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-free-for-open-source-projects]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Extract duplicated logic into a reusable utility The replaceTagDeterministic method is duplicated in two classes, IcmsPreloadAdsense and IcmsPreloadCustomtag. This logic should be extracted to a shared location like a utility class or trait to improve maintainability. Examples: htdocs/plugins/preloads/adsense.php [83-110] /** * Deterministically replace [tag]...[/tag] occurrences by invoking a preg-style callback. * Builds a $matches array like preg_replace_callback would: [0] full match, [1] inner. * * @param string $text * @param string $tag e.g. 'adsense' * @param callable $callback e.g. 'icms_sanitizeAdsenses_callback' * @return string */ private static function replaceTagDeterministic(string $text, string $tag, $callback): string ... (clipped 18 lines) htdocs/plugins/preloads/customtag.php [82-109] /** * Deterministically replace [tag]...[/tag] occurrences by invoking a preg-style callback. * Builds a $matches array like preg_replace_callback would: [0] full match, [1] inner. * * @param string $text * @param string $tag e.g. 'customtag' * @param callable $callback e.g. 'icms_sanitizeCustomtags_callback' * @return string */ private static function replaceTagDeterministic(string $text, string $tag, $callback): string ... (clipped 18 lines) Solution Walkthrough: Before: // In IcmsPreloadAdsense class class IcmsPreloadAdsense extends icms_preload_Item { // ... private static function replaceTagDeterministic(string $text, ...): string { // ... implementation ... } } // In IcmsPreloadCustomtag class class IcmsPreloadCustomtag extends icms_preload_Item { // ... private static function replaceTagDeterministic(string $text, ...): string { // ... identical implementation ... } } After: // In a new utility class/trait trait TagReplacer { private static function replaceTagDeterministic(string $text, ...): string { // ... implementation ... } } // In IcmsPreloadAdsense class class IcmsPreloadAdsense extends icms_preload_Item { use TagReplacer; // ... no local implementation of replaceTagDeterministic } // In IcmsPreloadCustomtag class class IcmsPreloadCustomtag extends icms_preload_Item { use TagReplacer; // ... no local implementation of replaceTagDeterministic } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies significant code duplication of the replaceTagDeterministic method introduced in the PR, which impacts future maintainability.	Medium
Learned best practice	Validate callback before invocation Add validation to ensure the callback is callable before invoking it with call_user_func. This prevents runtime errors if an invalid callback is passed. htdocs/plugins/preloads/adsense.php [92-110] private static function replaceTagDeterministic(string $text, string $tag, $callback): string { + if (!is_callable($callback)) { + return $text; + } $open = '[' . $tag . ']'; $close = '[/' . $tag . ']'; $pos = 0; while (($start = strpos($text, $open, $pos)) !== false) { $innerStart = $start + strlen($open); $end = strpos($text, $close, $innerStart); if ($end === false) { break; } $inner = substr($text, $innerStart, $end - $innerStart); $full = substr($text, $start, ($end + strlen($close)) - $start); $replacement = call_user_func($callback, array($full, $inner)); ... } return $text; } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Validate and sanitize callback function parameters before use to prevent potential security issues and runtime errors.	Low
General	✅ Use substr_replace for cleaner code Suggestion Impact: The suggestion was directly implemented. The code was changed from using substr concatenation to substr_replace exactly as suggested. code diff: - $text = substr($text, 0, $start) . $replacement . substr($text, $end + strlen($close)); + $text = substr_replace($text, $replacement, $start, ($end + strlen($close)) - $start); Replace the manual string concatenation using substr with a single call to substr_replace for improved code clarity and efficiency. htdocs/plugins/preloads/adsense.php [106] -$text = substr($text, 0, $start) . $replacement . substr($text, $end + strlen($close)); +$text = substr_replace($text, $replacement, $start, ($end + strlen($close)) - $start); [Suggestion processed] Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies an opportunity to use substr_replace for a more concise and readable string replacement, which is a good practice.	Low
Update

[skenow]

Could we turn this into a common callable method to reduce the number of duplicate instances of this method?

[fiammybe]

you mean put it in a trait for re-use? That might be a good idea. And better than shove it into common.php or functions.php :-)

[fiammybe]

Could we turn this into a common callable method to reduce the number of duplicate instances of this method?

Can we postpone this (but create a ticket for it so we don't forget) until after we have composer-based autoloading? That will allow for a cleaner solution with either a trait, or a utility class.

In the meantime, let's integrate this fix already in the 2.0.3 and get that version going again.