Skip to content

Commit 5008cd3

Browse files
vytisbulkeviciusvytisbulkevicius
authored
Merge pull request #1077 from Codeinwp/bugfix/optimole-service/1703
Add CSRF protection and input validation
2 parents aa0b9a9 + bf1c06b commit 5008cd3

3 files changed

Lines changed: 266 additions & 4 deletions

File tree

‎assets/js/single-attachment.js‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ jQuery(document).ready(function($) {
125125
function uploadFile() {
126126
var formData = new FormData();
127127
formData.append("action", "optml_replace_file");
128+
formData.append("optml_replace_nonce", OMAttachmentEdit.nonce);
128129
formData.append("attachment_id", OMAttachmentEdit.attachmentId);
129130
formData.append("file", $("#optml-replace-file-field")[0].files[0]);
130131

@@ -138,11 +139,12 @@ jQuery(document).ready(function($) {
138139
contentType: false,
139140
success: function(response) {
140141
$(".optml-svg-loader").hide();
141-
if(response.success) {
142+
if (response && response.success) {
142143
window.location.reload();
143144
} else {
145+
var msg = (response && (response.data || response.message)) || OMAttachmentEdit.i18n.replaceFileError;
144146
$(".optml-replace-file-error").removeClass("hidden");
145-
$(".optml-replace-file-error").text(response.message);
147+
$(".optml-replace-file-error").text(msg);
146148
}
147149
},
148150
error: function(response) {

‎inc/media_rename/attachment_edit.php‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@ public function enqueue_scripts( $hook ) {
9292
'maxFileSize' => $max_file_size,
9393
'attachmentId' => $id,
9494
'mimeType' => $mime_type,
95+
'nonce' => wp_create_nonce( 'optml_replace_media_nonce' ),
9596
'i18n' => [
9697
'maxFileSizeError' => $max_file_size_error,
9798
'replaceFileError' => __( 'Error replacing file', 'optimole-wp' ),
@@ -329,7 +330,19 @@ public function save_attachment_filename( $post_id ) {
329330
* Replace the file
330331
*/
331332
public function replace_file() {
332-
$id = (int) sanitize_text_field( $_POST['attachment_id'] );
333+
if ( ! check_ajax_referer( 'optml_replace_media_nonce', 'optml_replace_nonce', false ) ) {
334+
wp_send_json_error( __( 'Security check failed', 'optimole-wp' ) );
335+
}
336+
337+
$id = absint( $_POST['attachment_id'] ?? 0 );
338+
339+
if ( ! $id ) {
340+
wp_send_json_error( __( 'Invalid attachment ID', 'optimole-wp' ) );
341+
}
342+
343+
if ( get_post_type( $id ) !== 'attachment' ) {
344+
wp_send_json_error( __( 'Invalid attachment ID', 'optimole-wp' ) );
345+
}
333346

334347
if ( ! current_user_can( 'edit_post', $id ) ) {
335348
wp_send_json_error( __( 'You are not allowed to replace this file', 'optimole-wp' ) );
@@ -339,6 +352,17 @@ public function replace_file() {
339352
wp_send_json_error( __( 'No file uploaded', 'optimole-wp' ) );
340353
}
341354

355+
$file_info = wp_check_filetype_and_ext( $_FILES['file']['tmp_name'], $_FILES['file']['name'] );
356+
357+
if ( empty( $file_info['type'] ) ) {
358+
wp_send_json_error( __( 'Could not determine uploaded file type', 'optimole-wp' ) );
359+
}
360+
361+
$original_mime = get_post_mime_type( $id );
362+
if ( $file_info['type'] !== $original_mime ) {
363+
wp_send_json_error( __( 'The uploaded file type does not match the original file type.', 'optimole-wp' ) );
364+
}
365+
342366
$replacer = new Optml_Attachment_Replace( $id, $_FILES['file'] );
343367

344368
$replaced = $replacer->replace();

‎tests/media_rename/test-attachment-edit.php‎

Lines changed: 237 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,21 +5,65 @@
55

66
/**
77
* Class Test_Attachment_Edit.
8+
*
9+
* Extends WP_Ajax_UnitTestCase so that:
10+
* - DOING_AJAX is true, ensuring wp_send_json_* calls wp_die() rather than die()
11+
* - wp_die() is overridden to throw WPAjaxDieContinueException / WPAjaxDieStopException
12+
* after storing the response in $this->_last_response
13+
* - _handleAjax() dispatches wp_ajax_* actions and captures JSON output cleanly
814
*/
9-
class Test_Attachment_Edit extends WP_UnitTestCase {
15+
class Test_Attachment_Edit extends WP_Ajax_UnitTestCase {
1016
/**
1117
* Test instance
1218
*
1319
* @var Optml_Attachment_Edit
1420
*/
1521
private $instance;
1622

23+
/**
24+
* A real JPEG attachment (file on disk) shared across replace_file tests.
25+
*
26+
* @var int
27+
*/
28+
private static $jpeg_attachment_id;
29+
30+
/**
31+
* Create a real JPEG attachment once for the whole class.
32+
*/
33+
public static function wpSetUpBeforeClass( WP_UnitTest_Factory $factory ) {
34+
self::$jpeg_attachment_id = $factory->attachment->create_upload_object(
35+
OPTML_PATH . 'tests/assets/sample-test.jpg'
36+
);
37+
}
38+
39+
/**
40+
* Clean up the shared attachment after all tests in this class run.
41+
*/
42+
public static function tear_down_after_class() {
43+
wp_delete_post( self::$jpeg_attachment_id, true );
44+
parent::tear_down_after_class();
45+
}
46+
1747
/**
1848
* Setup test
1949
*/
2050
public function setUp(): void {
2151
parent::setUp();
52+
wp_set_current_user( 1 );
53+
2254
$this->instance = new Optml_Attachment_Edit();
55+
56+
// Register the wp_ajax_optml_replace_file action so _handleAjax() can
57+
// invoke replace_file() through the normal WordPress AJAX dispatch path.
58+
$this->instance->init();
59+
}
60+
61+
/**
62+
* Reset $_FILES after each test (WP_Ajax_UnitTestCase resets $_POST/$_GET).
63+
*/
64+
public function tearDown(): void {
65+
$_FILES = [];
66+
parent::tearDown();
2367
}
2468

2569
/**
@@ -49,4 +93,196 @@ public function test_prepare_attachment_filename() {
4993

5094
$this->assertEquals( 'test-file', get_post_meta( $attachment->ID, '_optml_pending_rename', true ) );
5195
}
96+
97+
/**
98+
* Dispatch the optml_replace_file AJAX action with the given POST data and
99+
* $_FILES, catch the wp_die() exception, and return the decoded JSON.
100+
*
101+
* _handleAjax() starts its own output buffer and the die handler stores
102+
* the captured JSON in $this->_last_response before throwing, so no manual
103+
* ob_start() is needed here.
104+
*
105+
* @param array $post Contents for $_POST (nonce and attachment_id).
106+
* @param array $files Contents for $_FILES['file'], or empty to omit.
107+
* @return array Decoded JSON response array, or empty array.
108+
*/
109+
private function call_replace_file( array $post, array $files = [] ): array {
110+
$_POST = $post;
111+
$_FILES = $files;
112+
113+
try {
114+
$this->_handleAjax( 'optml_replace_file' );
115+
} catch ( WPAjaxDieContinueException $e ) {
116+
// Normal path: wp_send_json_* echoed JSON then called wp_die().
117+
} catch ( WPAjaxDieStopException $e ) {
118+
// Fallback: wp_die() called with a plain string (e.g. '-1' from a
119+
// nonce failure when $die = true). Treat as a failed response.
120+
}
121+
122+
return ! empty( $this->_last_response )
123+
? ( json_decode( $this->_last_response, true ) ?? [] )
124+
: [];
125+
}
126+
127+
/**
128+
* A request with no nonce must be rejected with a security error.
129+
*/
130+
public function test_replace_file_no_nonce() {
131+
$response = $this->call_replace_file( [ 'attachment_id' => (string) self::$jpeg_attachment_id ] );
132+
133+
$this->assertFalse( $response['success'] );
134+
$this->assertStringContainsString( 'Security check', $response['data'] );
135+
}
136+
137+
/**
138+
* A request with a syntactically valid but wrong nonce must be rejected.
139+
*/
140+
public function test_replace_file_bad_nonce() {
141+
$response = $this->call_replace_file( [
142+
'attachment_id' => (string) self::$jpeg_attachment_id,
143+
'optml_replace_nonce' => 'not_a_real_nonce',
144+
] );
145+
146+
$this->assertFalse( $response['success'] );
147+
$this->assertStringContainsString( 'Security check', $response['data'] );
148+
}
149+
150+
/**
151+
* attachment_id = 0 must be rejected before any file or capability check.
152+
*/
153+
public function test_replace_file_zero_id() {
154+
$response = $this->call_replace_file( [
155+
'attachment_id' => '0',
156+
'optml_replace_nonce' => wp_create_nonce( 'optml_replace_media_nonce' ),
157+
] );
158+
159+
$this->assertFalse( $response['success'] );
160+
$this->assertStringContainsString( 'Invalid attachment ID', $response['data'] );
161+
}
162+
163+
/**
164+
* An ID that belongs to a non-attachment post type must be rejected.
165+
*/
166+
public function test_replace_file_non_attachment_post() {
167+
$post_id = self::factory()->post->create( [ 'post_type' => 'post' ] );
168+
169+
$response = $this->call_replace_file( [
170+
'attachment_id' => (string) $post_id,
171+
'optml_replace_nonce' => wp_create_nonce( 'optml_replace_media_nonce' ),
172+
] );
173+
174+
wp_delete_post( $post_id, true );
175+
176+
$this->assertFalse( $response['success'] );
177+
$this->assertStringContainsString( 'Invalid attachment ID', $response['data'] );
178+
}
179+
180+
/**
181+
* A request with no $_FILES['file'] entry must be rejected.
182+
*/
183+
public function test_replace_file_no_file_uploaded() {
184+
$response = $this->call_replace_file( [
185+
'attachment_id' => (string) self::$jpeg_attachment_id,
186+
'optml_replace_nonce' => wp_create_nonce( 'optml_replace_media_nonce' ),
187+
] );
188+
189+
$this->assertFalse( $response['success'] );
190+
$this->assertStringContainsString( 'No file uploaded', $response['data'] );
191+
}
192+
193+
/**
194+
* A binary blob named .jpg whose content is not a real image must be
195+
* rejected: wp_check_filetype_and_ext() uses getimagesize() for image/*
196+
* types and returns an empty type when the magic bytes do not match.
197+
*/
198+
public function test_replace_file_unrecognizable_mime() {
199+
$tmp = tempnam( sys_get_temp_dir(), 'optml_test_' );
200+
file_put_contents( $tmp, 'this is not image data @@##$$%%' );
201+
202+
$response = $this->call_replace_file(
203+
[
204+
'attachment_id' => (string) self::$jpeg_attachment_id,
205+
'optml_replace_nonce' => wp_create_nonce( 'optml_replace_media_nonce' ),
206+
],
207+
[
208+
'file' => [
209+
'name' => 'fake.jpg',
210+
'type' => 'image/jpeg',
211+
'tmp_name' => $tmp,
212+
'error' => 0,
213+
'size' => filesize( $tmp ),
214+
],
215+
]
216+
);
217+
218+
@unlink( $tmp );
219+
220+
$this->assertFalse( $response['success'] );
221+
$this->assertStringContainsString( 'determine', $response['data'] );
222+
}
223+
224+
/**
225+
* Uploading a file whose real MIME type differs from the original attachment
226+
* (SVG against a JPEG attachment) must be rejected by the MIME-match check.
227+
*/
228+
public function test_replace_file_mime_mismatch() {
229+
$response = $this->call_replace_file(
230+
[
231+
'attachment_id' => (string) self::$jpeg_attachment_id,
232+
'optml_replace_nonce' => wp_create_nonce( 'optml_replace_media_nonce' ),
233+
],
234+
[
235+
'file' => [
236+
'name' => 'sample.svg',
237+
'type' => 'image/svg+xml',
238+
'tmp_name' => OPTML_PATH . 'tests/assets/sample.svg',
239+
'error' => 0,
240+
'size' => filesize( OPTML_PATH . 'tests/assets/sample.svg' ),
241+
],
242+
]
243+
);
244+
245+
$this->assertFalse( $response['success'] );
246+
$this->assertStringContainsString( 'does not match', $response['data'] );
247+
}
248+
249+
/**
250+
* A valid nonce, valid attachment ID, and a replacement whose MIME type
251+
* matches the original must pass all validation and succeed.
252+
*
253+
* A fresh attachment is created so the shared fixture is not consumed by
254+
* the actual file-move that Optml_Attachment_Replace performs.
255+
*/
256+
public function test_replace_file_valid_jpeg_replacement() {
257+
$attachment_id = self::factory()->attachment->create_upload_object(
258+
OPTML_PATH . 'tests/assets/sample-test.jpg'
259+
);
260+
261+
// Optml_Attachment_Replace moves (not copies) the tmp file.
262+
$tmp = tempnam( sys_get_temp_dir(), 'optml_repl_' ) . '.jpg';
263+
copy( OPTML_PATH . 'tests/assets/small-1.jpg', $tmp );
264+
265+
$response = $this->call_replace_file(
266+
[
267+
'attachment_id' => (string) $attachment_id,
268+
'optml_replace_nonce' => wp_create_nonce( 'optml_replace_media_nonce' ),
269+
],
270+
[
271+
'file' => [
272+
'name' => 'small-1.jpg',
273+
'type' => 'image/jpeg',
274+
'tmp_name' => $tmp,
275+
'error' => 0,
276+
'size' => filesize( $tmp ),
277+
],
278+
]
279+
);
280+
281+
if ( file_exists( $tmp ) ) {
282+
@unlink( $tmp );
283+
}
284+
wp_delete_post( $attachment_id, true );
285+
286+
$this->assertTrue( $response['success'] );
287+
}
52288
}

0 commit comments

Comments
 (0)