Fix avoid file_exists() on oversized strings in File detection to prevent PHP warnings - #267
Fix avoid file_exists() on oversized strings in File detection to prevent PHP warnings#267the-hercules wants to merge 1 commit into
Conversation
…rresponding unit test
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## trunk #267 +/- ##
=========================================
Coverage 86.49% 86.49%
- Complexity 1327 1328 +1
=========================================
Files 68 68
Lines 4295 4295
=========================================
Hits 3715 3715
Misses 580 580
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
felixarntz
left a comment
There was a problem hiding this comment.
@the-hercules Great catch! I think we may be able to find a cleaner solution though.
| // Check if it's a local file path (before base64 check). | ||
| // The length guard avoids calling file_exists() on over-length strings (e.g. base64 data), | ||
| // which would emit a warning containing the entire string. | ||
| if (strlen($file) <= PHP_MAXPATHLEN && file_exists($file) && is_file($file)) { |
There was a problem hiding this comment.
not sure this is the adequate check - can't we instead perform a check for whether the string is possibly a file path? maybe check if it starts with / (since it always needs to be an absolute path in practice)
Not entirely. I came across this because on nginx (Apache untested), with Query Monitor active, image generation fails with: According to Claude Code, this is because Query Monitor takes the full warning text and puts it in an add_filter( 'qm/dispatch/rest', '__return_false' )
// or more specifically
add_filter( 'qm/outputter/headers', fn( $o ) => array_diff_key( $o, [ 'php_errors' => 1 ] ), 999 ); // untested |
Closes #258
Summary
File::detectAndProcessFile()callsfile_exists()while determining whether aninput string is a local path, before the plain-base64 branch is reached. When the
input is a large base64 payload (e.g. an image returned by a provider via
bytesBase64Encoded), the string exceeds the platform's maximum path length andPHP emits:
The warning message embeds the entire input string, so each occurrence writes
~1 MB to the error log. In practice this produced multi-megabyte error logs from
only a handful of image-generation calls. Base64-encoded JPEG data begins with
/9j/, which resembles an absolute path, so the string reachesfile_exists()before detection falls through to the base64 handling that processes it correctly.
Functionally the input was always handled correctly — this is a log-noise issue,
not a data-correctness one.
Change
Guard the filesystem check with a length comparison:
AI Disclosure
Claude Opus 4.8 was used for identification and then verification of correctness of the solution.