Skip to content

Escaped backreferences to prevent XSS - #1138

Open
girishpanchal30 wants to merge 2 commits into
developmentfrom
bugfix/optimole-service/1780
Open

Escaped backreferences to prevent XSS#1138
girishpanchal30 wants to merge 2 commits into
developmentfrom
bugfix/optimole-service/1780

Conversation

@girishpanchal30

Copy link
Copy Markdown
Contributor

Summary:

The 4.2.3 fix for CVE-2026-5217 escaped the srcset descriptor with esc_attr(), but the escaped value is then passed as the replacement argument of preg_replace(), which still expands $0/$1 backreferences and can reinsert a raw double quote to break out of the attribute. Escape $ and \ with addcslashes() right before each preg_replace() call in add_missing_srcset_attributes(), enhance_existing_srcset(), and enhance_existing_sizes().

All Submissions:

Closes https://github.com/Codeinwp/optimole-service/issues/1780

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@pirate-bot

pirate-bot commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Plugin build for 87f97b7 is ready 🛎️!

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Tests do not cover backslash-form backreferences or the slashed-output path targeted by the fix.

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

Pull request overview

Prevents preg_replace() backreference expansion from reintroducing unsafe content into responsive image attributes.

Changes:

  • Escapes replacement-sensitive $ and \ characters.
  • Adds XSS regression tests for affected srcset and sizes paths.
File summaries
File Description
inc/tag_replacer.php Secures attribute replacement strings.
tests/test-srcset.php Adds backreference-injection tests.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced

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

Comment thread tests/test-srcset.php

$tag = '<img src="https://example.com/image.jpg" alt="Test" />';
$missing_srcsets = [
[ 'w' => 800, 'h' => 600, 'd' => 1, 's' => '$0 onload=alert(document.domain) x', 'b' => 0 ],

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The replacement values are correctly escaped immediately before use, with focused regression coverage.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants