Upd. ContactEncoder. Imrove area-label processing.

[svfcode]

task https://app.doboard.com/1/task/55549

[Copilot]

🟡 Changes recommended

It introduces a likely regression in attribute exclusions (missing <option value=...> handling) and has a security hardening gap in the OpenSSL randomness fallback that should be addressed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates the ContactsEncoder’s handling of aria-label placeholders (to avoid predictable placeholder collisions) and extends attribute-based exclusion logic to better prevent unwanted encoding inside certain HTML attributes, with accompanying PHPUnit coverage.

Changes:

• Reworked aria-label hiding/restoration to use cryptographically-random placeholders instead of predictable counters.
• Expanded and made configurable the “attribute exclusions” detection logic (including support for runtime additions).
• Added PHPUnit tests for aria-label behavior and for attribute exclusion matching (e.g., data-mask, placeholder formats).

File summaries

File	Description
ContactsEncoder.php	Uses secure random placeholders for aria-label protection and restores placeholders more safely.
Helper/ContactsEncoderHelper.php	Adds runtime-configurable attribute exclusion rules and improves matching logic.
tests/ContactsEncoder/TestContactsEncoderAriaLabel.php	Adds tests covering aria-label preservation and placeholder collision/XSS scenarios.
tests/ContactsEncoder/TestContactsEncoderAttributeExclusions.php	Adds tests covering attribute exclusion detection and customization APIs.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The new aria-label tests can be order-dependent due to the ContactsEncoder singleton, and aria-label protection isn’t re-applied before phone encoding when both email and phone encoding are enabled.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

tests/ContactsEncoder/TestContactsEncoderAriaLabel.php:35

• createEncoder() returns a singleton instance without resetting it; because ContactsEncoder::getInstance() only initializes once, this test can become order-dependent (params from a previous test run may be reused). Reset the singleton before returning the encoder (as done in the other test class) and/or drop it in tearDown().

This issue also appears on line 70 of the same file.
tests/ContactsEncoder/TestContactsEncoderAriaLabel.php:58

• assertNotRegExp() is deprecated/removed in newer PHPUnit versions and is inconsistent with the next test which already uses preg_match(); using a preg_match-based assertion here avoids PHPUnit-version coupling.

tests/ContactsEncoder/TestContactsEncoderAriaLabel.php:73

• This test class doesn't drop the ContactsEncoder singleton after running; adding tearDown() prevents cross-test pollution (other tests may call getInstance() with different Params and unexpectedly reuse this instance).

        $this->assertTrue(strpos($result, 'ct_temp_aria_0') !== false);
        $this->assertFalse((bool) preg_match('/>\s*aria-label\s*=/', $result));
    }
}

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

modifyGlobalEmails()/modifyGlobalPhoneNumbers() can crash when called directly because $this->temp_content may be null during helper checks.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

ContactsEncoder.php:353

• modifyGlobalPhoneNumbers() uses $this->temp_content inside the callback but does not initialize it when the method is called directly, which can crash helper checks (e.g. strpos() on null in isInsideScriptTag()) and makes exclusion detection unreliable. Assign $this->temp_content = $content after optional aria-label protection.

    public function modifyGlobalPhoneNumbers($content)
    {
        $owns_aria_protection = empty($this->aria_placeholders);
        if ( $owns_aria_protection ) {
            $content = $this->handleAriaLabelContent($content);
        }

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The changes are well-scoped, add test coverage for the new behavior, and the remaining feedback is minor/documentation-level with a small best-practice tweak.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

Helper/ContactsEncoderHelper.php:240

• The hasAttributeExclusions() docblock still describes both parameters as “email”, but this helper is used for phone matches as well and $temp_content is the full HTML/content. Updating this avoids misleading documentation for consumers.

ContactsEncoder.php:866

• isSecureAriaLabelPlaceholderAvailable() currently only checks for function existence; on some setups openssl_random_pseudo_bytes() may exist but never provide cryptographically-strong bytes ($crypto_strong=false), making this return true even though placeholders cannot be generated. Using getSecureRandomBytes() here keeps the check accurate and avoids running the aria-label replacement pass when it can’t produce placeholders.

    private function isSecureAriaLabelPlaceholderAvailable()
    {
        return function_exists('random_bytes') || function_exists('openssl_random_pseudo_bytes');
    }

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite