fix(security): Fix DOMPDF RCE and customer email sanitization#4568
fix(security): Fix DOMPDF RCE and customer email sanitization#4568jekkos wants to merge 4 commits into
Conversation
- Disable isPhpEnabled in DOMPDF to prevent RCE via embedded PHP in HTML - Disable isRemoteEnabled to prevent SSRF attacks - Add email validation and sanitization in CSV import (FILTER_SANITIZE_EMAIL, FILTER_VALIDATE_EMAIL) - Reject invalid email formats during customer import
📝 WalkthroughWalkthroughThe pull request introduces three independent security hardening measures: email validation and sanitization for customer CSV imports with full test coverage, disabling of remote resources and PHP execution in PDF generation, and escaping of email addresses in table helper output to prevent XSS injection. ChangesSecurity Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Email columns in bootstrap tables had escaping disabled (line 52) and mailto() function doesn't escape its parameters. This fix escapes email addresses before passing to mailto() in: - get_person_data_row() (employees) - get_customer_data_row() (customers) - get_supplier_data_row() (suppliers) Attack vector: Malicious email via CSV import renders XSS in table view.
Tests cover: - Valid email acceptance - Invalid email rejection with row-specific error - XSS payload sanitization in email field - Mixed valid/invalid email handling - Email with special characters sanitization Verifies fixes for customer email import vulnerability.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/Controllers/CustomersCsvImportTest.php (1)
82-82: 💤 Low value
assertJSONExactcouples to the exact lang string.This asserts the literal
'Customers imported successfully'. Iflang('Customers.csv_import_success')differs or changes, the test breaks for non-behavioral reasons. Consider assertingsuccess === true(as the other tests do) rather than the exact message text.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Controllers/CustomersCsvImportTest.php` at line 82, The test in CustomersCsvImportTest is asserting the exact message string via assertJSONExact(['success' => true, 'message' => 'Customers imported successfully']), which couples the spec to a literal lang string; change the assertion to only verify success === true (e.g. use assertJSON or assertJSONFragment to check ['success' => true]) and remove the exact message check so the test no longer depends on lang('Customers.csv_import_success') text.app/Controllers/Customers.php (1)
425-425: 💤 Low valueInconsistent failure entries and hardcoded message.
Other failure paths push a bare integer (
$failCodes[] = $i;at Lines 468 and 474), while this path pushes a descriptive string. The user-facing message is also hardcoded in English unlike the surroundinglang()usage. Consider routing this throughlang()and keeping the failCodes element type consistent for a uniformcsv_import_partially_failedmessage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Controllers/Customers.php` at line 425, The current failure branch pushes a descriptive string into $failCodes while other branches push the row index integer and the user-facing text is hardcoded; change the line that currently does $failCodes[] = 'Row ' . $i . ': Invalid email format' to push the integer index ($failCodes[] = $i;) so $failCodes stays consistently an array of integers, and move the human-readable message into a language entry (e.g. add a key like 'customers.invalid_email' to your lang files) and ensure the code that composes the final csv_import_partially_failed message uses lang('...') to format "Row X: {message}" when rendering failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Controllers/Customers.php`:
- Around line 422-428: The current email validation treats empty emails as
invalid because filter_var('' , FILTER_VALIDATE_EMAIL) returns false; update the
check in the import loop around the $email variable so you only validate when an
address is present (e.g. trim and lower-case $email, then if $email is not empty
and !filter_var($email, FILTER_VALIDATE_EMAIL) push the 'Invalid email format'
into $failCodes and continue). Keep the existing increment of $i and the rest of
the flow (postSave() still handles optional emails).
In `@tests/Controllers/CustomersCsvImportTest.php`:
- Around line 17-20: The test class CustomersCsvImportTest is at risk of
cross-test DB contamination because protected $refresh = false prevents
DatabaseTestTrait::regressDatabase() from resetting state; update the test to
set protected $refresh = true in the class declaration so the DB is rolled back
before each test, or alternatively implement a tearDown that truncates the
customers table (or calls DatabaseTestTrait::tearDownDatabase()) or wrap tests
exercising /customers/importCsvFile in a transaction to ensure imported rows
don't persist across methods; reference the CustomersCsvImportTest class, the
$refresh property, DatabaseTestTrait::regressDatabase(),
DatabaseTestTrait::tearDownDatabase(), and hasInDatabase() when making the fix.
---
Nitpick comments:
In `@app/Controllers/Customers.php`:
- Line 425: The current failure branch pushes a descriptive string into
$failCodes while other branches push the row index integer and the user-facing
text is hardcoded; change the line that currently does $failCodes[] = 'Row ' .
$i . ': Invalid email format' to push the integer index ($failCodes[] = $i;) so
$failCodes stays consistently an array of integers, and move the human-readable
message into a language entry (e.g. add a key like 'customers.invalid_email' to
your lang files) and ensure the code that composes the final
csv_import_partially_failed message uses lang('...') to format "Row X:
{message}" when rendering failures.
In `@tests/Controllers/CustomersCsvImportTest.php`:
- Line 82: The test in CustomersCsvImportTest is asserting the exact message
string via assertJSONExact(['success' => true, 'message' => 'Customers imported
successfully']), which couples the spec to a literal lang string; change the
assertion to only verify success === true (e.g. use assertJSON or
assertJSONFragment to check ['success' => true]) and remove the exact message
check so the test no longer depends on lang('Customers.csv_import_success')
text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ccd7be3-87fc-492e-bd95-056b4357b849
📒 Files selected for processing (4)
app/Controllers/Customers.phpapp/Helpers/dompdf_helper.phpapp/Helpers/tabular_helper.phptests/Controllers/CustomersCsvImportTest.php
| $email = filter_var(strtolower($data[4]), FILTER_SANITIZE_EMAIL); | ||
|
|
||
| if (!filter_var($email, FILTER_VALIDATE_EMAIL)) { | ||
| $failCodes[] = 'Row ' . $i . ': Invalid email format'; | ||
| $i++; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Empty/optional emails are now rejected, breaking imports of customers without an email.
filter_var('', FILTER_VALIDATE_EMAIL) returns false, so any row with a blank email column will now fail with "Invalid email format" and be skipped. postSave() does not treat email as required, so customers without an email were previously importable. This is a behavioral regression for legitimate rows that omit the email.
Validate only when an email is actually present:
🐛 Proposed fix to allow empty emails while still rejecting malformed ones
- $email = filter_var(strtolower($data[4]), FILTER_SANITIZE_EMAIL);
-
- if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
- $failCodes[] = 'Row ' . $i . ': Invalid email format';
- $i++;
- continue;
- }
-
+ $email = filter_var(strtolower($data[4]), FILTER_SANITIZE_EMAIL);
+
+ if ($email !== '' && !filter_var($email, FILTER_VALIDATE_EMAIL)) {
+ $failCodes[] = 'Row ' . $i . ': Invalid email format';
+ $i++;
+ continue;
+ }
+If email is intended to be mandatory for CSV imports, please confirm and ignore this.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $email = filter_var(strtolower($data[4]), FILTER_SANITIZE_EMAIL); | |
| if (!filter_var($email, FILTER_VALIDATE_EMAIL)) { | |
| $failCodes[] = 'Row ' . $i . ': Invalid email format'; | |
| $i++; | |
| continue; | |
| } | |
| $email = filter_var(strtolower($data[4]), FILTER_SANITIZE_EMAIL); | |
| if ($email !== '' && !filter_var($email, FILTER_VALIDATE_EMAIL)) { | |
| $failCodes[] = 'Row ' . $i . ': Invalid email format'; | |
| $i++; | |
| continue; | |
| } | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Controllers/Customers.php` around lines 422 - 428, The current email
validation treats empty emails as invalid because filter_var('' ,
FILTER_VALIDATE_EMAIL) returns false; update the check in the import loop around
the $email variable so you only validate when an address is present (e.g. trim
and lower-case $email, then if $email is not empty and !filter_var($email,
FILTER_VALIDATE_EMAIL) push the 'Invalid email format' into $failCodes and
continue). Keep the existing increment of $i and the rest of the flow
(postSave() still handles optional emails).
| protected $migrate = true; | ||
| protected $migrateOnce = true; | ||
| protected $refresh = false; | ||
| protected $namespace = null; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CodeIgniter 4 DatabaseTestTrait $refresh false transaction rollback between tests behavior
💡 Result:
In CodeIgniter 4, the DatabaseTestTrait's $refresh property and the automatic transaction rollback mechanism are distinct features that serve different purposes in your test suite [1][2]. The $refresh property (when set to true) controls whether the database migrations are completely reset (rolled back to version 0 and re-run) before the test class begins [1][2]. Setting $refresh to false simply skips this heavy-duty reset process [1]. It does not, by itself, disable or enable database transactions [1][3]. Regarding transactions, the DatabaseTestTrait does not automatically wrap every test method in a database transaction for the purpose of rollback [4]. Instead, database isolation is primarily managed through either: 1. Manual Test Transactions: You can use the database transaction system in "test mode" by calling $this->db->transStart(true) at the beginning of your test, which ensures that all queries within that block are automatically rolled back, regardless of their success [5]. 2. Cleanup via Assertions: If you use the hasInDatabase method provided by the trait, CodeIgniter tracks these specific insertions and automatically removes them after the test has finished, providing a clean state for that specific data [4]. If you set $refresh to false, your database state will persist across tests within the same class (or across different classes if migrations were already run) [1][6]. If you need a clean database for every test method while $refresh is false, you must manually manage your transactions or explicitly clear the affected tables in your tearDown method [4][5]. In summary, setting $refresh to false does not trigger or bypass automatic transaction rollbacks; it only skips the full migration reset [1][2]. Transactions must still be managed explicitly using the database connection methods if you want to ensure data isolation during your tests [5].
Citations:
- 1: https://codeigniter4.github.io/userguide/testing/database.html
- 2: https://codeigniter.com/user_guide/testing/database.html
- 3: https://codeigniter4.github.io/api/classes/CodeIgniter-Test-CIUnitTestCase.html
- 4: https://codeigniter4.github.io/api/classes/CodeIgniter-Test-DatabaseTestTrait.html
- 5: https://codeigniter4.github.io/userguide/database/transactions.html
- 6: feat: make migration/seed settings flexible on database testing codeigniter4/CodeIgniter4#3993
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List file (sanity)
ls -la tests/Controllers/CustomersCsvImportTest.php
# Show relevant parts of the test file (around the properties + setup/teardown + failing assertions)
sed -n '1,120p' tests/Controllers/CustomersCsvImportTest.php
sed -n '120,260p' tests/Controllers/CustomersCsvImportTest.php
# Search within this file for email/assert logic and any hasInDatabase usage
rg -n "refresh|migrateOnce|tearDown|transStart|hasInDatabase|check_email_exists|example\.com|assertNotNull|assertJSONExact|mailchimp|addOrUpdateMember" tests/Controllers/CustomersCsvImportTest.php || true
# Find how this test base class is defined (where DatabaseTestTrait comes from)
rg -n "class CustomersCsvImportTest|extends |use .*DatabaseTestTrait" tests/Controllers/CustomersCsvImportTest.php || true
# Identify parent test case (if local) and whether it uses DatabaseTestTrait defaults
# (Try to find the referenced parent class name(s) and locate their definitions)
rg -n "namespace|use |extends " tests/Controllers/CustomersCsvImportTest.phpRepository: opensourcepos/opensourcepos
Length of output: 10923
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate DatabaseTestTrait source in vendor (or in installed CodeIgniter package)
fd "DatabaseTestTrait.php" -t f . || true
fd "DatabaseTestTrait" -t f vendor . || true
# Show trait file (small enough) and any related transaction/refresh logic
files=$(fd "DatabaseTestTrait.php" -t f vendor . || true)
echo "Found: $files"
for f in $files; do
echo "==== $f ===="
wc -l "$f"
sed -n '1,200p' "$f"
sed -n '200,400p' "$f" || true
done
# Also locate CIUnitTestCase base class used here
fd "CIUnitTestCase.php" -t f vendor . || true
base=$(fd "CIUnitTestCase.php" -t f vendor . | head -n 1 || true)
if [ -n "$base" ]; then
echo "==== $base ===="
wc -l "$base"
sed -n '1,220p' "$base"
fi
# Search within vendor for $refresh or transStart usage in test traits
rg -n "\\$refresh|transStart|rollback|hasInDatabase|DatabaseTestTrait" vendor/codeigniter* vendor/ | head -n 200 || trueRepository: opensourcepos/opensourcepos
Length of output: 143
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo top:"
ls -ლა
echo "Searching for DatabaseTestTrait.php + CIUnitTestCase.php in-repo (incl. vendor if present)..."
find . -name 'DatabaseTestTrait.php' 2>/dev/null | head -n 50
find . -name 'CIUnitTestCase.php' 2>/dev/null | head -n 50
echo "Searching for the string 'class DatabaseTestTrait'..."
rg -n "class DatabaseTestTrait" . 2>/dev/null | head -n 20 || true
echo "Searching for '$refresh' usage in test traits..."
rg -n "\\$refresh" . 2>/dev/null | head -n 50 || trueRepository: opensourcepos/opensourcepos
Length of output: 163
🌐 Web query:
CodeIgniter 4 DatabaseTestTrait teardown transaction rollback $refresh migrateOnce implementation
💡 Result:
In CodeIgniter 4’s testing database helpers, $refresh does NOT imply a per-test “teardown transaction rollback” in the DatabaseTestTrait itself. Instead, the trait’s documented behavior is: when $refresh is true, “all migrations are rolled back to version 0” before the test runs, and the trait provides regressDatabase to do that/reset work manually if needed [1][2]. On the other hand, rollback of a database transaction at the end of the test is something CodeIgniter documents as being handled via the CIUnitTestCase/DatabaseTestTrait lifecycle methods (setUp/tearDown) and “special functionality executed during the setUp and tearDown phases” (meaning you should call parent::tearDown to keep the framework cleanup running) [1]. The API docs list tearDownDatabase and other internal helpers on DatabaseTestTrait, but they don’t document a specific “transaction rollback” detail in that public docs snippet [3]. Implementation detail you can rely on from the official docs: 1) Set protected $refresh = true to force the database to be completely refreshed (migrations rolled back to version 0) before each test [1][2]. 2) If you override tearDown in your test class, you must call parent::tearDown or you will “lose much of the functionality described here” [1]. 3) The trait exposes migrateDatabase (called during setUp) and regressDatabase (called during $refresh) as the hooks that affect how migrations/seeding state is prepared/reset [4][1][2]. 4) $migrateOnce controls whether migrations run only once before the first test, otherwise migrations run before each test [4][1]. So, the “$refresh migrateOnce teardown transaction rollback” behavior is effectively: - $refresh: reset migrations/seeds state by rolling back migrations to 0 (via regressDatabase) before the test [4][1][2]. - $migrateOnce: if true, run migrations only once before the first test (rather than every test) [4][1]. - teardown: ensure parent::tearDown is called so the trait’s cleanup (via its tearDown* helpers) still runs [1]. If what you specifically need is the exact line(s) in the CodeIgniter 4 source where DatabaseTestTrait begins/rolls back a transaction during teardown, that isn’t shown in the API snippets available from the docs pages above; you’d need to inspect the DatabaseTestTrait PHP source file in the CodeIgniter repository to confirm the precise rollback call and whether it’s conditional on $refresh/$migrateOnce. The official docs pages only state the migration rollback-to-0 behavior for $refresh and the requirement to call parent::tearDown for the framework cleanup to occur [4][1].
Citations:
- 1: https://codeigniter.com/user_guide/testing/database.html
- 2: https://michalsn.github.io/CodeIgniter4/testing/database.html
- 3: https://codeigniter4.github.io/api/classes/CodeIgniter-Test-DatabaseTestTrait.html
- 4: https://codeigniter4.github.io/userguide/testing/database.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la
test -d vendor && echo "vendor exists" || echo "vendor missing"
find . -maxdepth 4 -type d -name vendor 2>/dev/null
# If vendor exists, locate the actual DatabaseTestTrait + CIUnitTestCase implementations
if [ -d vendor ]; then
fd "DatabaseTestTrait.php" -t f vendor | head -n 20
fd "CIUnitTestCase.php" -t f vendor | head -n 20
db_trait=$(fd "DatabaseTestTrait.php" -t f vendor | head -n 1 || true)
ci_unit=$(fd "CIUnitTestCase.php" -t f vendor | head -n 1 || true)
echo "DatabaseTestTrait file: $db_trait"
echo "CIUnitTestCase file: $ci_unit"
if [ -n "${db_trait:-}" ]; then
sed -n '1,240p' "$db_trait"
sed -n '240,520p' "$db_trait"
fi
if [ -n "${ci_unit:-}" ]; then
sed -n '1,260p' "$ci_unit"
fi
echo "Search for transaction/rollback/migrateDatabase/regressDatabase in DatabaseTestTrait..."
rg -n "transStart|transRollback|rollback|revert|migrateDatabase|regressDatabase|hasInDatabase|refresh" "$db_trait" || true
fi
# Also inspect CIUnitTestCase for any per-test transaction wrapping logic
if [ -d vendor ] && [ -n "${ci_unit:-}" ]; then
rg -n "transStart|transRollback|rollback|DatabaseTestTrait|hasInDatabase" "$ci_unit" || true
fiRepository: opensourcepos/opensourcepos
Length of output: 3307
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for DatabaseTestTrait definition in vendor..."
rg -n "trait DatabaseTestTrait" vendor | head -n 20 || true
rg -n "class CIUnitTestCase" vendor | head -n 20 || true
rg -n "namespace CodeIgniter\\\\Test" vendor | head -n 20 || true
echo "Searching for filenames containing DatabaseTestTrait/CIUnitTestCase..."
find vendor -type f \( -iname "*databasetesttrait*.php" -o -iname "*ciunittestcase*.php" \) | head -n 50
echo "Searching for regressDatabase/migrateDatabase/hasInDatabase helpers in vendor..."
rg -n "regressDatabase|migrateDatabase|hasInDatabase" vendor | head -n 50 || trueRepository: opensourcepos/opensourcepos
Length of output: 404
🏁 Script executed:
#!/bin/bash
set -euo pipefail
db_trait="vendor/codeigniter4/framework/system/Test/DatabaseTestTrait.php"
ci_unit="vendor/codeigniter4/framework/system/Test/CIUnitTestCase.php"
wc -l "$db_trait" "$ci_unit"
sed -n '1,260p' "$db_trait"
sed -n '260,620p' "$db_trait"
sed -n '1,260p' "$ci_unit"
sed -n '260,620p' "$ci_unit"
echo "Search within DatabaseTestTrait.php for refresh/transactions/teardown hooks:"
rg -n "\\$refresh|transStart|transRollback|transCommit|rollback|regressDatabase|migrateDatabase|hasInDatabase|tearDown" "$db_trait"Repository: opensourcepos/opensourcepos
Length of output: 24278
Fix potential cross-test DB contamination in CustomersCsvImportTest ($refresh = false)
With protected $refresh = false (lines 17-20), DatabaseTestTrait skips regressDatabase() before each test, and tearDownDatabase() only clears rows added via hasInDatabase() (via $insertCache). Since these tests exercise /customers/importCsvFile directly and don’t use hasInDatabase(), inserted customers can persist across test methods—so any import logic that treats existing emails as duplicates can make later assertions flaky. Set $refresh = true or explicitly clear/transaction-wrap the affected tables in tearDown (or ensure imported emails remain unique after sanitization).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/Controllers/CustomersCsvImportTest.php` around lines 17 - 20, The test
class CustomersCsvImportTest is at risk of cross-test DB contamination because
protected $refresh = false prevents DatabaseTestTrait::regressDatabase() from
resetting state; update the test to set protected $refresh = true in the class
declaration so the DB is rolled back before each test, or alternatively
implement a tearDown that truncates the customers table (or calls
DatabaseTestTrait::tearDownDatabase()) or wrap tests exercising
/customers/importCsvFile in a transaction to ensure imported rows don't persist
across methods; reference the CustomersCsvImportTest class, the $refresh
property, DatabaseTestTrait::regressDatabase(),
DatabaseTestTrait::tearDownDatabase(), and hasInDatabase() when making the fix.
|
❌ Staging deployment failed Check the workflow logs for details. |
2 participants
Summary
Vulnerability Details
1. DOMPDF RCE (Critical)
Location:
app/Helpers/dompdf_helper.phpIssue: DOMPDF was configured with
isPhpEnabled => trueandisRemoteEnabled => true, allowing:Attack Vector: Malicious actors could inject PHP code into customer data fields (names, addresses, comments). When PDFs are generated, this code would execute on the server.
Fix: Set both options to
false:isPhpEnabled => false- Prevents PHP injection attacksisRemoteEnabled => false- Prevents SSRF attacks2. Customer Email Import Sanitization (Medium-High)
Location:
app/Controllers/Customers.php:postImportCsvFile()Issue: CSV import only lowercased email addresses without validation or sanitization, allowing:
<script>alert(1)</script>@example.com)Fix: Added proper email handling:
filter_var($email, FILTER_SANITIZE_EMAIL)- Removes dangerous charactersfilter_var($email, FILTER_VALIDATE_EMAIL)- Rejects invalid email formats3. XSS in Bootstrap Table Email Rendering (Medium-High)
Location:
app/Helpers/tabular_helper.phpIssue: Email columns in bootstrap tables had escaping disabled (line 52), and
mailto()function doesn't escape its parameters:Attack Vector:
<script>alert(1)</script>@example.com<a href="mailto:<script>alert(1)</script>@example.com">...</a>Fix: Escape email addresses before passing to
mailto():Applies to:
get_person_data_row()- Employee emailsget_customer_data_row()- Customer emailsget_supplier_data_row()- Supplier emailsTesting
Security Comments
Comments added to DOMPDF configuration to prevent future developers from inadvertently re-enabling these dangerous options.
Summary by CodeRabbit
New Features
Bug Fixes