Repository navigation
Enhanced srcset handling - #1219
Conversation
There was a problem hiding this comment.
🟢 Approved
This change is approved. The note asks you to confirm that open issue #1112 is out of scope.
Validation details
- Files reviewed: 5/5 changed files. The row about open issue #1112 (one-viewport slot width extrapolation) stays unchecked.
- PHPUnit 9.6.22 on PHP 8.3.36 passed all 11 added and modified tests at HEAD [R3].
- On the base tree, each added or modified test failed or errored [R5].
Test_Dimension_Replacementalso passed with lazyload enabled [R2].- The profiler script emits only width descriptors, so the new descriptor check drops no real data.
- A security pass found no remaining injection path in the changed code.
Untested areas
- Open issue #1112 (one-viewport slot width extrapolation) was not checked. This change does not touch that logic.
- No browser check of the rendered image markup was run.
🤖 Automated review · run code-review-agent_6ae97be70ee424.90642072.
🤖 Review agent — review posted ✅ on 2b2c1bd8 · approved · 0 findings · 17 min
Run code-review-agent_6ae97be70ee424.90642072 · trail
| foreach ( $missing_srcsets as $srcset_data ) { | ||
| // Validate required fields | ||
| if ( ! isset( $srcset_data['w'] ) || ! isset( $srcset_data['h'] ) || ! isset( $srcset_data['s'] ) ) { | ||
| if ( ! isset( $srcset_data['w'] ) || ! isset( $srcset_data['h'] ) || ! self::is_srcset_descriptor( $srcset_data['s'] ?? null ) ) { |
There was a problem hiding this comment.
note The new check accepts every <number>w descriptor the profiler stores. Widths that the profiler extrapolates from one viewport still pass. They can still overstate slot widths in the generated sizes value, as open issue #1112 describes. Please confirm that this change is not meant to fix that issue. A separate fix for it means this change is safe. If this change was meant to close it, the fix is incomplete.
There was a problem hiding this comment.
Confirmed out of scope. This PR fixes optimole-service#1904 (stored XSS). It only rejects srcset descriptors that aren't plain <int>w and changes where decoding/width/height are inserted. #1112 (single-viewport width extrapolation in srcset-detector.js _generateResponsiveSizes()) is a sizing-accuracy issue in the profiler script, which this change doesn't touch. It stays open for a separate fix.
|
🎉 This PR is included in version 4.2.19 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
pirate-bot
added
the
released
All Submissions:
Changes proposed in this Pull Request:
Strengthened image tag attribute handling and
srcsetvalidation to prevent unintended attribute injection and ensure only valid width descriptors are processed. Added a utility method for safely injecting image attributes, tightened descriptor validation, and introduced comprehensive tests covering edge cases and security scenarios.Closes https://github.com/Codeinwp/optimole-service/issues/1904
Other information: