Add default escape characters to icms_core_DataFilter::addSlashes for backwards compatibility

[Copilot]

User description

icms_core_DataFilter::addSlashes() was calling addcslashes() without default characters, causing it to escape nothing when invoked without the second parameter. This broke SQL queries with single quotes, triggered false Protector SQL injection warnings, and caused unexpected character conversions during save operations.

Changes

• Added DEFAULT_ESCAPE_CHARS constant containing ', ", \, and \0 (NUL byte) to match PHP's addslashes() behavior
• Modified addSlashes() to use the constant when $param is null, providing backwards compatibility while preserving the ability to specify custom character lists
• Updated documentation to clarify default behavior and remove obsolete magic_quotes_gpc references

Example

// Before: No escaping occurred, causing SQL syntax errors
icms_core_DataFilter::addSlashes("O'Brien's content");
// => "O'Brien's content" (unchanged - breaks SQL)

// After: Properly escapes like addslashes()
icms_core_DataFilter::addSlashes("O'Brien's content");
// => "O\'Brien\'s content" (SQL-safe)

// Custom character lists still work
icms_core_DataFilter::addSlashes("test_value", "_");
// => "test\_value"

Original prompt

---

This section details on the original issue you should resolve

<issue_title>Add defaults to icms_core_Datafilter::addSlashes for backwards compatibility</issue_title>
<issue_description>After troubleshooting an upgrade, I discovered the difficulty was caused by icms_core_DataFilter::addSlashes not having any default characters for addcslashes() to produce the same output as addslashes() would - it escapes single quote, double quote, backslash, and NUL.

What happened was Protector would stop the save and say it detected an SQL injection. With Protector disabled, I was able to save, but several records weren't updated, because SQL complained about the syntax. The content being saved had a single quote in it. I also encountered some characters being converted. </issue_description>

Comments on the Issue (you are @copilot in this section)

• Fixes Add defaults to icms_core_Datafilter::addSlashes for backwards compatibility #1674

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

---

PR Type

Bug fix, Enhancement

---

Description

• Add DEFAULT_ESCAPE_CHARS constant matching PHP's addslashes() behavior

• Fix addSlashes() to escape single quotes, double quotes, backslashes, NUL bytes by default

• Improve method documentation and remove obsolete magic_quotes_gpc references

• Maintain backwards compatibility while allowing custom character escaping

---

Diagram Walkthrough

flowchart LR
  A["addSlashes called<br/>without param"] -->|"param === null"| B["Use DEFAULT_ESCAPE_CHARS<br/>constant"]
  B -->|"Contains: ', \", \\, \\0"| C["addcslashes escapes<br/>properly"]
  C -->|"Returns"| D["SQL-safe escaped text"]
  E["addSlashes called<br/>with custom param"] -->|"param provided"| C

Loading

File Walkthrough

Relevant files

Bug fix

DataFilter.php Add default escape characters constant and fix addSlashes method htdocs/libraries/icms/core/DataFilter.php Added DEFAULT_ESCAPE_CHARS class constant containing single quote, double quote, backslash, and NUL byte characters Modified addSlashes() method to use DEFAULT_ESCAPE_CHARS when $param is null Updated method documentation to clarify default behavior and remove obsolete magic_quotes_gpc references Improved code comments explaining backwards compatibility with PHP's addslashes() +20/-4

---

[fiammybe]

@skenow @MekDrop I let a Copilot Agent do its thing, just to have an idea what we can expect from the technology at this time. What do you think? I have the impression it focused too much on the typo ('NUL' instead of 'NULL' I imagine), but how does the rest of the proposal look to you?

[skenow]

The pattern is also what I had come up with - wasn't sure how to test the NUL (not a typo - it's how it's referred to elsewhere) character, though. 1 question - the method signature defines the 2nd parameter as null. Could you just put the default pattern there and not have to do any other defining and testing?

[skenow]

Well, we would have to make other changes to have a non-NULL default for the parameter. I encountered this when just replacing null with the string:

• Fatal error: Default value for parameters with a class type hint can only be NULL

[skenow]

This does solve the issue and preserves the type hinting in the method signature.

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

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Incorrect NUL byte escape Description: The constant uses double-quoted string with escape sequences, but the NUL byte representation '\0' may not be interpreted correctly; it should use "\0" in double quotes or chr(0) to ensure proper NUL byte inclusion. DataFilter.php [57-57] Referred Code const DEFAULT_ESCAPE_CHARS = "'\"\\\0";
Ticket Compliance
🟡	🎫 #1674 🟢 Add default characters to icms_core_DataFilter::addSlashes method to match addslashes() behavior Escape single quote, double quote, backslash, and NUL characters by default Maintain backwards compatibility while allowing custom character lists ⚪ Fix SQL injection false positives from Protector module Fix SQL syntax errors when content contains single quotes
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 Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	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 Learn more about managing compliance generic rules or creating your own custom rules
	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 Learn more about managing compliance generic rules or creating your own custom rules
🔴	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 addSlashes() method does not validate the $text parameter before processing, which could lead to unexpected behavior if non-string types are passed despite the type hint. Referred Code public static function addSlashes(string $text, ?string $param = null) { // Default to escaping the same characters as addslashes() for backwards compatibility if ($param === null) { $param = self::DEFAULT_ESCAPE_CHARS; } return addcslashes($text, $param); Learn more about managing compliance generic rules or creating your own custom rules

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	Deprecate manual escaping for prepared statements While the PR fixes addSlashes, it highlights a larger issue. The project should stop using manual escaping and migrate to prepared statements with parameterized queries to prevent SQL injection vulnerabilities. Examples: Solution Walkthrough: Before: // Code using the manual escaping function class SomeService { public function save($data) { $escaped_data = icms_core_DataFilter::addSlashes($data); $sql = "INSERT INTO some_table (column) VALUES ('" . $escaped_data . "')"; // ... execute raw SQL query } } // The function fixed in the PR class icms_core_DataFilter { public static function addSlashes(string $text, ?string $param = null) { // ... logic to fix escaping return addcslashes($text, $param); } } After: // Code using prepared statements class SomeService { public function save($data) { // Assuming $db is a PDO or similar connection object $stmt = $db->prepare("INSERT INTO some_table (column) VALUES (?)"); $stmt->execute([$data]); } } // The old function is deprecated as it's no longer needed for SQL security class icms_core_DataFilter { /** * @deprecated Use prepared statements for SQL queries instead. */ public static function addSlashes(string $text, ?string $param = null) { // ... } } Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies that manual escaping is an outdated and insecure practice, proposing a migration to prepared statements which is a critical architectural improvement for preventing SQL injection vulnerabilities.	High
Learned best practice	Use null coalescing operator The strict comparison $param === null is correct. However, consider using a null coalescing operator for more concise and idiomatic PHP code when providing default values. htdocs/libraries/icms/core/DataFilter.php [176-178] -if ($param === null) { - $param = self::DEFAULT_ESCAPE_CHARS; -} +$param = $param ?? self::DEFAULT_ESCAPE_CHARS; Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Use strict and consistent comparisons to prevent PHP type juggling and ensure predictable behavior	Low
More