Make WordPress Core

Opened 3 weeks ago

Closed 3 weeks ago

Last modified 7 days ago

#65670 closed defect (bug) (fixed)

WP_REST_Attachments_Controller::get_attachment_filesize() fatals on non-integer filesize metadata

Reported by: apermo Owned by: westonruter
Priority: normal Milestone: 7.1
Component: REST API Version: 7.0
Severity: normal Keywords: has-patch has-unit-tests
Cc: Focuses:

Description (last modified by westonruter)

Serving an attachment via the REST API can throw a fatal error:

TypeError: WP_REST_Attachments_Controller::get_attachment_filesize():
Return value must be of type ?int, string returned
  .../class-wp-rest-attachments-controller.php:2373 (get_attachment_filesize)
  .../class-wp-rest-attachments-controller.php:1431 (prepare_item_for_response)
  .../class-wp-rest-posts-controller.php:669 (get_item)

get_attachment_filesize() (introduced in 7.0.0) returns the stored filesize value from attachment metadata verbatim:

$meta = wp_get_attachment_metadata( $attachment_id );
if ( isset( $meta['filesize'] ) ) {
    return $meta['filesize'];
}

Attachment metadata is untyped postmeta, so filesize is not guaranteed to be an integer. The file does not declare strict_types, so a numeric string is coerced and survives — but a non-numeric string violates the method's ?int return type and throws a fatal TypeError.

Real-world trigger: plugins populate filesize from remote storage API responses. For example, WP Media Folder (JoomUnited) stores the Google Drive fileSize field, which the Drive API returns as a string; Dropbox and OneDrive addons do the same from their API payloads.

The filesize REST schema declares 'type' => 'integer', so the intent is clearly an integer. Core should normalize the untyped meta value rather than fatal.

Steps to reproduce:

  1. Create an attachment.
  2. Store non-numeric metadata: wp_update_attachment_metadata( $id, array( 'filesize' => 'corrupt' ) );
  3. GET /wp/v2/media/<id> -> fatal TypeError.

Proposed fix: only return the stored value when numeric (cast to int), otherwise fall through to recompute the size from the file via wp_filesize(). Patch + PHPUnit coverage attached (GitHub PR).

Change History (9)

This ticket was mentioned in PR #12611 on WordPress/wordpress-develop by @apermo.


3 weeks ago
#1

  • Keywords has-patch has-unit-tests added

## Trac ticket

https://core.trac.wordpress.org/ticket/65670

## Problem

WP_REST_Attachments_Controller::get_attachment_filesize() (new in 7.0.0) returns the filesize value from attachment metadata verbatim against a strict ?int return type. Attachment metadata is untyped postmeta, so a non-numeric string value throws a fatal TypeError when the media endpoint is requested:

TypeError: WP_REST_Attachments_Controller::get_attachment_filesize():
Return value must be of type ?int, string returned

A numeric string only survives because the file has no strict_types declaration (coercion). A non-numeric one fatals.

Plugins populate filesize from remote storage API responses — e.g. the Google Drive fileSize field (returned as a string) via WP Media Folder, and the equivalent Dropbox/OneDrive addons — so this occurs on real sites.

## Fix

Only return the stored value when it is numeric, cast to int; otherwise fall through to recompute the size from the file via wp_filesize() (which always casts to int), returning null if unavailable. This matches the 'type' => 'integer' REST schema.

## Testing

Two regression tests added to tests/phpunit/tests/rest-api/rest-attachments-controller.php:

  • numeric-string filesize meta → normalized to int in the response;
  • non-numeric filesize meta → no fatal, falls back to the real file size.

The non-numeric test reproduces the reported TypeError against unpatched trunk (red), and passes with the fix (green).

## Disclosure

Claude (Anthropic) assisted in diagnosing the root cause and drafting the patch and tests. I reviewed the change, verified the red/green test behavior locally, and take responsibility for it.

@apermo commented on PR #12611:


3 weeks ago
#2

@westonruter would you be able to review this? It fixes a fatal TypeError in get_attachment_filesize() (new in 7.0.0) when filesize attachment metadata is stored as a non-numeric string — which happens on sites using plugins that populate it from remote storage APIs (e.g. the Google Drive fileSize string via WP Media Folder). Trac: https://core.trac.wordpress.org/ticket/65670

@apermo commented on PR #12611:


3 weeks ago
#3

@westonruter Would you be so kind to have a look at it? Thanks :)

#4 @westonruter
3 weeks ago

  • Milestone Awaiting Review7.1
  • Owner set to westonruter
  • Status newreviewing

#5 @westonruter
3 weeks ago

In 62813:

Code Quality: Ensure wp_filesize() always returns a non-negative-int.

A negative file size is clearly impossible, and the return value of 0 is already documented as being the error case.

  • Values filtered by pre_wp_filesize and wp_filesize are cast to int if they are numeric.
  • Non-numeric values returned by the wp_filesize filter are discarded in favor of zero.
  • Negative values filtered by the pre_wp_filesize filter are treated the same as null (and do not short-circuit).
  • Negative values returned by the wp_filesize filter are clamped to be at least zero.

Developed as part of https://github.com/WordPress/wordpress-develop/pull/12611.
Follow-up to r52837, r52932.

Props westonruter, apermo.
See #65670, #64898.

#6 follow-up: @westonruter
3 weeks ago

  • Description modified (diff)

@apermo Aside: please ensure ticket descriptions get submitted in Trac WikiFormatting instead of Markdown.

#7 @westonruter
3 weeks ago

  • Resolutionfixed
  • Status reviewingclosed

In 62815:

REST API: Normalize non-integer attachment filesize metadata.

  • The return value of the WP_REST_Attachments_Controller::get_attachment_filesize() method is narrowed from int|null to non-negative-int|null.
  • The stored filesize attachment metadata is only returned if it is a numeric value and is greater than zero; it is cast to an int, preventing a TypeError if a non-numeric string was stored in metadata.
  • The filesize property of the attachments endpoint adds null as a possible value in addition to integer.

Developed in https://github.com/WordPress/wordpress-develop/pull/12611.
Follow-up to r62813, r61703.

Props apermo, westonruter, xate, mukesh27.
Fixes #65670.

#8 in reply to: ↑ 6 @apermo
3 weeks ago

Replying to westonruter:

@apermo Aside: please ensure ticket descriptions get submitted in Trac WikiFormatting instead of Markdown.

My bad, missed out on that one, will pay closer attention next time. Thank you for the quick and great work :)

#9 @westonruter
7 days ago

In 62978:

Media: Normalize non-numeric attachment filesize metadata.

The stored filesize attachment metadata was read without validation in wp_prepare_attachment_for_js() and attachment_submitbox_metadata(). Attachment metadata is untyped, and the filesize key is commonly written by offloading plugins from a remote storage API response, so it can arrive as a numeric string, or be empty, non-numeric, or negative when the remote lookup fails. A numeric string was passed through verbatim, making filesizeInBytes a string in the media modal while the wp_filesize() branch of the very same conditional yielded an int; a non-numeric value such as 'unknown' was truthy and suppressed the fallback entirely, so size_format() returned false and the file size rendered empty even when the real file was readable.

Both call sites now only trust the stored value when it is numeric and casts to an integer greater than zero, and otherwise recompute the size with wp_filesize(). The fallback condition also replaces file_exists() with is_readable() guarded on a non-empty string, since get_attached_file() can be filtered to return a non-string. PHPUnit coverage is added for both functions.

Developed in https://github.com/WordPress/wordpress-develop/pull/12632.
Follow-up to r34258, r52837, r62813, r62815.

Props mukesh27, westonruter.
See #65670.
Fixes #65686.

Note: See TracTickets for help on using tickets.