Skip to content

Commit 5c45958

Browse files
westonruterwestonruter
committed
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 WordPress#12632. Follow-up to r34258, r52837, r62813, r62815. Props mukesh27, westonruter. See #65670. Fixes #65686. git-svn-id: https://develop.svn.wordpress.org/trunk@62978 602fd350-edb4-49c9-b593-d223f7449a82
1 parent 6693ea1 commit 5c45958

4 files changed

Lines changed: 345 additions & 6 deletions

File tree

‎src/wp-admin/includes/media.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3425,9 +3425,9 @@ function attachment_submitbox_metadata() {
34253425

34263426
$file_size = false;
34273427

3428-
if ( isset( $meta['filesize'] ) ) {
3429-
$file_size = $meta['filesize'];
3430-
} elseif ( file_exists( $file ) ) {
3428+
if ( isset( $meta['filesize'] ) && is_numeric( $meta['filesize'] ) && (int) $meta['filesize'] > 0 ) {
3429+
$file_size = (int) $meta['filesize'];
3430+
} elseif ( is_string( $file ) && '' !== $file && is_readable( $file ) ) {
34313431
$file_size = wp_filesize( $file );
34323432
}
34333433

‎src/wp-includes/media.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4716,9 +4716,9 @@ function wp_prepare_attachment_for_js( $attachment ) {
47164716

47174717
$attached_file = get_attached_file( $attachment->ID );
47184718

4719-
if ( isset( $meta['filesize'] ) ) {
4720-
$bytes = $meta['filesize'];
4721-
} elseif ( file_exists( $attached_file ) ) {
4719+
if ( isset( $meta['filesize'] ) && is_numeric( $meta['filesize'] ) && (int) $meta['filesize'] > 0 ) {
4720+
$bytes = (int) $meta['filesize'];
4721+
} elseif ( is_string( $attached_file ) && '' !== $attached_file && is_readable( $attached_file ) ) {
47224722
$bytes = wp_filesize( $attached_file );
47234723
} else {
47244724
$bytes = '';

‎tests/phpunit/tests/admin/includesMedia.php‎

Lines changed: 177 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,177 @@
1+
<?php
2+
3+
/**
4+
* @group media
5+
* @group admin
6+
*/
7+
class Tests_Admin_IncludesMedia extends WP_UnitTestCase {
8+
9+
public function set_up() {
10+
parent::set_up();
11+
12+
require_once ABSPATH . 'wp-admin/includes/media.php';
13+
}
14+
15+
/**
16+
* Tests that a `filesize` stored in the attachment metadata is normalized to a positive integer.
17+
*
18+
* When the stored value cannot be normalized, it should be treated as missing so that the
19+
* filesystem fallback runs instead.
20+
*
21+
* @ticket 65686
22+
*
23+
* @covers ::attachment_submitbox_metadata
24+
*
25+
* @dataProvider data_attachment_submitbox_metadata_filesize
26+
*
27+
* @param mixed $filesize The `filesize` value stored in the attachment metadata.
28+
* @param int<0, max>|null $expected The expected file size in bytes, or null if none should be displayed.
29+
*/
30+
public function test_attachment_submitbox_metadata_filesize( $filesize, ?int $expected ) {
31+
$id = self::factory()->attachment->create_object(
32+
array(
33+
'file' => 'test-image.jpg',
34+
'post_title' => 'Attachment Title',
35+
'post_parent' => 0,
36+
'post_mime_type' => 'image/jpeg',
37+
)
38+
);
39+
$this->assertIsInt( $id );
40+
41+
wp_update_attachment_metadata(
42+
$id,
43+
array(
44+
'width' => 50,
45+
'height' => 50,
46+
'file' => 'test-image.jpg',
47+
'filesize' => $filesize,
48+
)
49+
);
50+
51+
$GLOBALS['post'] = get_post( $id );
52+
53+
$output = get_echo( 'attachment_submitbox_metadata' );
54+
55+
if ( null === $expected ) {
56+
$this->assertStringNotContainsString( 'misc-pub-filesize', $output, 'The file size should not have been displayed.' );
57+
} else {
58+
$this->assertStringContainsString( size_format( $expected ), $output, 'The displayed file size did not match the normalized file size.' );
59+
}
60+
}
61+
62+
/**
63+
* Data provider.
64+
*
65+
* @return array<non-falsy-string, array{ filesize: mixed, expected: int<0, max>|null }>
66+
*/
67+
public function data_attachment_submitbox_metadata_filesize(): array {
68+
return array(
69+
'an integer' => array(
70+
'filesize' => 12345,
71+
'expected' => 12345,
72+
),
73+
'a numeric string' => array(
74+
'filesize' => '12345',
75+
'expected' => 12345,
76+
),
77+
'a float' => array(
78+
'filesize' => 12345.6,
79+
'expected' => 12345,
80+
),
81+
'a float as a string' => array(
82+
'filesize' => '12345.6',
83+
'expected' => 12345,
84+
),
85+
'an exponential string' => array(
86+
'filesize' => '1e3',
87+
'expected' => 1000,
88+
),
89+
'a value smaller than a byte' => array(
90+
'filesize' => 0.5,
91+
'expected' => null,
92+
),
93+
'zero' => array(
94+
'filesize' => 0,
95+
'expected' => null,
96+
),
97+
'a negative integer' => array(
98+
'filesize' => -12345,
99+
'expected' => null,
100+
),
101+
'an empty string' => array(
102+
'filesize' => '',
103+
'expected' => null,
104+
),
105+
'a non-numeric string' => array(
106+
'filesize' => 'not-a-number',
107+
'expected' => null,
108+
),
109+
'an array' => array(
110+
'filesize' => array( 12345 ),
111+
'expected' => null,
112+
),
113+
'null' => array(
114+
'filesize' => null,
115+
'expected' => null,
116+
),
117+
'false' => array(
118+
'filesize' => false,
119+
'expected' => null,
120+
),
121+
'true' => array(
122+
'filesize' => true,
123+
'expected' => null,
124+
),
125+
);
126+
}
127+
128+
/**
129+
* Tests that an unusable `filesize` in the attachment metadata falls back to the size of the file.
130+
*
131+
* @ticket 65686
132+
*
133+
* @covers ::attachment_submitbox_metadata
134+
*
135+
* @dataProvider data_attachment_submitbox_metadata_filesize_falls_back_to_the_file
136+
*
137+
* @param mixed $filesize The `filesize` value stored in the attachment metadata.
138+
*/
139+
public function test_attachment_submitbox_metadata_filesize_falls_back_to_the_file( $filesize ) {
140+
$id = self::factory()->attachment->create_upload_object( DIR_TESTDATA . '/images/canola.jpg' );
141+
$this->assertIsInt( $id );
142+
$file = get_attached_file( $id );
143+
$this->assertIsString( $file );
144+
145+
$meta = wp_get_attachment_metadata( $id );
146+
$this->assertIsArray( $meta );
147+
$meta['filesize'] = $filesize;
148+
wp_update_attachment_metadata( $id, $meta );
149+
150+
$GLOBALS['post'] = get_post( $id );
151+
152+
$output = get_echo( 'attachment_submitbox_metadata' );
153+
154+
$filesize = wp_filesize( $file );
155+
$this->assertIsInt( $filesize );
156+
$this->assertStringContainsString( size_format( $filesize ), $output );
157+
}
158+
159+
/**
160+
* Data provider.
161+
*
162+
* @return array<non-falsy-string, array{ filesize: mixed }>
163+
*/
164+
public function data_attachment_submitbox_metadata_filesize_falls_back_to_the_file(): array {
165+
return array(
166+
'a value smaller than a byte' => array( 'filesize' => 0.5 ),
167+
'zero' => array( 'filesize' => 0 ),
168+
'a negative integer' => array( 'filesize' => -12345 ),
169+
'an empty string' => array( 'filesize' => '' ),
170+
'a non-numeric string' => array( 'filesize' => 'not-a-number' ),
171+
'an array' => array( 'filesize' => array( 12345 ) ),
172+
'null' => array( 'filesize' => null ),
173+
'false' => array( 'filesize' => false ),
174+
'true' => array( 'filesize' => true ),
175+
);
176+
}
177+
}

‎tests/phpunit/tests/media.php‎

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -667,6 +667,168 @@ public function test_wp_prepare_attachment_for_js_without_image_sizes() {
667667
$this->assertArrayHasKey( 'sizes', $prepped );
668668
}
669669

670+
/**
671+
* Tests that a `filesize` stored in the attachment metadata is normalized to a positive integer.
672+
*
673+
* When the stored value cannot be normalized, it should be treated as missing so that the
674+
* filesystem fallback runs instead.
675+
*
676+
* @ticket 65686
677+
*
678+
* @dataProvider data_wp_prepare_attachment_for_js_filesize
679+
*
680+
* @param mixed $filesize The `filesize` value stored in the attachment metadata.
681+
* @param int<0, max>|null $expected The expected `filesizeInBytes` value, or null if it should not be set.
682+
*/
683+
public function test_wp_prepare_attachment_for_js_filesize( $filesize, ?int $expected ) {
684+
$id = self::factory()->attachment->create_object(
685+
array(
686+
'file' => 'test-image.jpg',
687+
'post_title' => 'Attachment Title',
688+
'post_parent' => 0,
689+
'post_mime_type' => 'image/jpeg',
690+
)
691+
);
692+
$this->assertIsInt( $id );
693+
694+
wp_update_attachment_metadata(
695+
$id,
696+
array(
697+
'width' => 50,
698+
'height' => 50,
699+
'file' => 'test-image.jpg',
700+
'filesize' => $filesize,
701+
)
702+
);
703+
704+
$post = get_post( $id );
705+
$this->assertInstanceOf( WP_Post::class, $post );
706+
$prepped = wp_prepare_attachment_for_js( $post );
707+
$this->assertIsArray( $prepped );
708+
709+
if ( null === $expected ) {
710+
$this->assertArrayNotHasKey( 'filesizeInBytes', $prepped, 'The filesize should not have been set.' );
711+
$this->assertArrayNotHasKey( 'filesizeHumanReadable', $prepped, 'The human readable filesize should not have been set.' );
712+
} else {
713+
$this->assertSame( $expected, $prepped['filesizeInBytes'], 'The filesize was not normalized to an integer.' );
714+
$this->assertSame( size_format( $expected ), $prepped['filesizeHumanReadable'], 'The human readable filesize did not match the normalized filesize.' );
715+
}
716+
}
717+
718+
/**
719+
* Data provider.
720+
*
721+
* @return array<non-falsy-string, array{ filesize: mixed, expected: int<0, max>|null }>
722+
*/
723+
public function data_wp_prepare_attachment_for_js_filesize(): array {
724+
return array(
725+
'an integer' => array(
726+
'filesize' => 12345,
727+
'expected' => 12345,
728+
),
729+
'a numeric string' => array(
730+
'filesize' => '12345',
731+
'expected' => 12345,
732+
),
733+
'a float' => array(
734+
'filesize' => 12345.6,
735+
'expected' => 12345,
736+
),
737+
'a float as a string' => array(
738+
'filesize' => '12345.6',
739+
'expected' => 12345,
740+
),
741+
'an exponential string' => array(
742+
'filesize' => '1e3',
743+
'expected' => 1000,
744+
),
745+
'a value smaller than a byte' => array(
746+
'filesize' => 0.5,
747+
'expected' => null,
748+
),
749+
'zero' => array(
750+
'filesize' => 0,
751+
'expected' => null,
752+
),
753+
'a negative integer' => array(
754+
'filesize' => -12345,
755+
'expected' => null,
756+
),
757+
'an empty string' => array(
758+
'filesize' => '',
759+
'expected' => null,
760+
),
761+
'a non-numeric string' => array(
762+
'filesize' => 'not-a-number',
763+
'expected' => null,
764+
),
765+
'an array' => array(
766+
'filesize' => array( 12345 ),
767+
'expected' => null,
768+
),
769+
'null' => array(
770+
'filesize' => null,
771+
'expected' => null,
772+
),
773+
'false' => array(
774+
'filesize' => false,
775+
'expected' => null,
776+
),
777+
'true' => array(
778+
'filesize' => true,
779+
'expected' => null,
780+
),
781+
);
782+
}
783+
784+
/**
785+
* Tests that an unusable `filesize` in the attachment metadata falls back to the size of the file.
786+
*
787+
* @ticket 65686
788+
*
789+
* @dataProvider data_wp_prepare_attachment_for_js_filesize_falls_back_to_the_file
790+
*
791+
* @param mixed $filesize The `filesize` value stored in the attachment metadata.
792+
*/
793+
public function test_wp_prepare_attachment_for_js_filesize_falls_back_to_the_file( $filesize ) {
794+
$id = self::factory()->attachment->create_upload_object( DIR_TESTDATA . '/images/canola.jpg' );
795+
$this->assertIsInt( $id );
796+
$post = get_post( $id );
797+
$this->assertInstanceOf( WP_Post::class, $post );
798+
$file = get_attached_file( $id );
799+
$this->assertIsString( $file );
800+
801+
$meta = wp_get_attachment_metadata( $id );
802+
$this->assertIsArray( $meta );
803+
$meta['filesize'] = $filesize;
804+
wp_update_attachment_metadata( $id, $meta );
805+
806+
$prepped = wp_prepare_attachment_for_js( $post );
807+
$this->assertIsArray( $prepped );
808+
$this->assertArrayHasKey( 'filesizeInBytes', $prepped );
809+
810+
$this->assertSame( wp_filesize( $file ), $prepped['filesizeInBytes'] );
811+
}
812+
813+
/**
814+
* Data provider.
815+
*
816+
* @return array<non-falsy-string, array{ filesize: mixed }>
817+
*/
818+
public function data_wp_prepare_attachment_for_js_filesize_falls_back_to_the_file(): array {
819+
return array(
820+
'a value smaller than a byte' => array( 'filesize' => 0.5 ),
821+
'zero' => array( 'filesize' => 0 ),
822+
'a negative integer' => array( 'filesize' => -12345 ),
823+
'an empty string' => array( 'filesize' => '' ),
824+
'a non-numeric string' => array( 'filesize' => 'not-a-number' ),
825+
'an array' => array( 'filesize' => array( 12345 ) ),
826+
'null' => array( 'filesize' => null ),
827+
'false' => array( 'filesize' => false ),
828+
'true' => array( 'filesize' => true ),
829+
);
830+
}
831+
670832
/**
671833
* @ticket 19067
672834
* @expectedDeprecated wp_convert_bytes_to_hr

0 commit comments

Comments
 (0)