Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens ContactsEncoder’s aria-label placeholder handling to prevent attacker-controlled placeholder collisions during the “hide/restore aria-label” round-trip, and adds regression tests covering both normal aria-label preservation and a CVE-style markup breakout/XSS payload.
Changes:
- Replace sequential
ct_temp_aria_Nplaceholders with unguessable%%APBCT_ARIA_<random>%%placeholders. - Restore aria-label placeholders only at the first occurrence to avoid rewriting planted tokens.
- Add PHPUnit coverage for aria-label email preservation and for the reported CVE-2026-77830 payload patterns.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/ApbctWP/ContactsEncoder/TestContactsEncoder.php | Adds regression tests for aria-label preservation and for preventing token-collision / markup-breakout behavior. |
| lib/Cleantalk/Common/ContactsEncoder/ContactsEncoder.php | Switches aria-label placeholder generation to random, and updates restoration logic to be collision-resistant. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (27.14%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## fix #866 +/- ##
============================================
+ Coverage 30.02% 30.05% +0.02%
- Complexity 6541 6544 +3
============================================
Files 289 289
Lines 26247 26262 +15
============================================
+ Hits 7880 7892 +12
- Misses 18367 18370 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
tests/ApbctWP/ContactsEncoder/TestContactsEncoder.php:253
- The regex
/>aria-label=/only detects a breakout when there is no whitespace/newline between>andaria-labeland whenaria-labelhas no whitespace around=. Using a whitespace-tolerant pattern makes the regression test stronger for markup injection cases.
This issue also appears on line 267 of the same file.
$result = $this->contacts_encoder->modifyContent($payload);
$this->assertStringContainsString('ct_temp_aria_0', $result);
$this->assertNotRegExp('/>aria-label=/', $result);
}
tests/ApbctWP/ContactsEncoder/TestContactsEncoder.php:269
- Same as above:
preg_match('/>aria-label=/')can miss injected> aria-label =variants. Consider making the detection whitespace-tolerant so the test fails on more real-world breakout formats.
$planted_token_preserved = strpos($result, 'ct_temp_aria_0') !== false;
$breakout_injected = (bool) preg_match('/>aria-label=/', $result);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
lib/Cleantalk/Common/ContactsEncoder/ContactsEncoder.php:897
handleAriaLabelContent()(andrestoreAriaLabelPlaceholder()) assume$contentis always a string, but callers pass the result ofpreg_replace_callback(), which can benullon PCRE errors (e.g., backtrack/recursion limit). Passingnullintostrpos()/substr()can raise a TypeError and break rendering. Consider coercing$contentto string at the start ofhandleAriaLabelContent()to avoid fatals during restore.
private function handleAriaLabelContent($content, $reverse = false)
{
if ( !$reverse ) {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
lib/Cleantalk/Common/ContactsEncoder/ContactsEncoder.php:259
- When both email and phone encoding are enabled, aria-label placeholders are restored at the end of modifyGlobalEmails(), before modifyGlobalPhoneNumbers() runs. That means phone-number encoding can still rewrite phone numbers inside aria-label, which contradicts the intent of hiding/restoring aria-label around encoding passes.
// modify content to prevent aria-label replaces by hiding it
if ( $this->do_encode_emails || $this->do_encode_phones ) {
$content = $this->handleAriaLabelContent($content);
}
tests/ApbctWP/ContactsEncoder/TestContactsEncoder.php:232
- Test coverage currently verifies aria-label preservation for emails, but not for phone numbers. Since modifyContent() now also applies aria-label placeholder handling when phone encoding is enabled, add a test case asserting that an aria-label containing a phone number is preserved when phone encoding (and especially email+phone encoding together) is enabled.
public function testModifyContentPreservesAriaLabelWithEmail()
{
$email = 'info@example.com';
$content = '<button aria-label="Contact us at ' . $email . '">Click</button>';
task https://app.doboard.com/1/task/55549