From b2f67363a1617090ac0c4fe82dab0acb768ff047 Mon Sep 17 00:00:00 2001 From: Shish Date: Tue, 9 Jan 2024 21:01:10 +0000 Subject: [PATCH] [core] roll back database transaction when upload fails - fixes #1007 --- core/imageboard/event.php | 4 ---- core/imageboard/misc.php | 4 ++++ ext/danbooru_api/main.php | 6 ++++-- ext/image_hash_ban/test.php | 7 ++----- ext/ouroboros_api/main.php | 12 ++++++------ ext/replace_file/main.php | 2 +- ext/res_limit/test.php | 24 +++++++++--------------- ext/upload/main.php | 21 +++++++++++++-------- ext/upload/test.php | 15 ++++++--------- 9 files changed, 45 insertions(+), 50 deletions(-) diff --git a/core/imageboard/event.php b/core/imageboard/event.php index f8797b0d..5011dc1c 100644 --- a/core/imageboard/event.php +++ b/core/imageboard/event.php @@ -23,10 +23,6 @@ class ImageAdditionEvent extends Event } } -class ImageAdditionException extends SCoreException -{ -} - /** * An image is being deleted. */ diff --git a/core/imageboard/misc.php b/core/imageboard/misc.php index d8cfaba6..f9fdf18a 100644 --- a/core/imageboard/misc.php +++ b/core/imageboard/misc.php @@ -16,6 +16,7 @@ namespace Shimmie2; */ function add_dir(string $base, ?array $extra_tags = []): array { + global $database; $results = []; foreach (list_files($base) as $full_path) { @@ -24,6 +25,7 @@ function add_dir(string $base, ?array $extra_tags = []): array $tags = array_merge(path_to_tags($short_path), $extra_tags); try { + $database->execute("SAVEPOINT upload"); $dae = send_event(new DataUploadEvent($full_path, [ 'filename' => pathinfo($filename, PATHINFO_BASENAME), 'tags' => $tags, @@ -32,7 +34,9 @@ function add_dir(string $base, ?array $extra_tags = []): array foreach($dae->images as $image) { $results[] = new UploadSuccess($filename, $image->id); } + $database->execute("RELEASE SAVEPOINT upload"); } catch (UploadException $ex) { + $database->execute("ROLLBACK TO SAVEPOINT upload"); $results[] = new UploadError($filename, $ex->getMessage()); } } diff --git a/ext/danbooru_api/main.php b/ext/danbooru_api/main.php index 6cdd6ce3..28bf058e 100644 --- a/ext/danbooru_api/main.php +++ b/ext/danbooru_api/main.php @@ -264,7 +264,7 @@ class DanbooruApi extends Extension */ private function api_add_post(): void { - global $user, $page; + global $database, $user, $page; $danboorup_kludge = 1; // danboorup for firefox makes broken links out of location: /path // Check first if a login was supplied, if it wasn't check if the user is logged in via cookie @@ -342,6 +342,7 @@ class DanbooruApi extends Extension //log_debug("danbooru_api", "upload($filename): fileinfo(".var_export($fileinfo,TRUE)."), metadata(".var_export($metadata,TRUE).")..."); try { + $database->execute("SAVEPOINT upload"); // Fire off an event which should process the new file and add it to the db $dae = send_event(new DataUploadEvent($file, [ 'filename' => pathinfo($filename, PATHINFO_BASENAME), @@ -363,8 +364,9 @@ class DanbooruApi extends Extension } else { $page->add_http_header("Location: $newid"); } + $database->execute("RELEASE SAVEPOINT upload"); } catch (UploadException $ex) { - // Did something screw up? + $database->execute("ROLLBACK TO SAVEPOINT upload"); $page->set_code(409); $page->add_http_header("X-Danbooru-Errors: exception - " . $ex->getMessage()); } diff --git a/ext/image_hash_ban/test.php b/ext/image_hash_ban/test.php index 5ba09b80..a5d6ff22 100644 --- a/ext/image_hash_ban/test.php +++ b/ext/image_hash_ban/test.php @@ -33,12 +33,9 @@ class ImageBanTest extends ShimmiePHPUnitTestCase $this->assertEquals(404, $page->code); // Can't repost - try { + $this->assertException(UploadException::class, function () { $this->post_image("tests/pbx_screenshot.jpg", "pbx"); - $this->fail(); - } catch (UploadException $e) { - $this->assertTrue(true); - } + }); // Remove ban send_event(new RemoveImageHashBanEvent($this->hash)); diff --git a/ext/ouroboros_api/main.php b/ext/ouroboros_api/main.php index 93c34814..bd125893 100644 --- a/ext/ouroboros_api/main.php +++ b/ext/ouroboros_api/main.php @@ -321,7 +321,7 @@ class OuroborosAPI extends Extension */ protected function postCreate(OuroborosPost $post, ?string $md5 = '') { - global $config; + global $config, $database; $handler = $config->get_string(ImageConfig::UPLOAD_COLLISION_HANDLER); if (!empty($md5) && !($handler == ImageConfig::COLLISION_MERGE)) { $img = Image::by_hash($md5); @@ -383,20 +383,20 @@ class OuroborosAPI extends Extension } $meta['extension'] = pathinfo($meta['filename'], PATHINFO_EXTENSION); try { - send_event(new DataUploadEvent($meta['file'], $meta)); - $image = Image::by_hash($meta['hash']); + $database->execute("SAVEPOINT upload"); + $dae = send_event(new DataUploadEvent($meta['file'], $meta)); + $image = $dae->images[0]; if (!is_null($image)) { $this->sendResponse(200, make_link('post/view/' . $image->id), true); - return; } else { // Fail, unsupported file? $this->sendResponse(500, 'Unknown error'); - return; } + $database->execute("RELEASE SAVEPOINT upload"); } catch (UploadException $e) { + $database->execute("ROLLBACK TO SAVEPOINT upload"); // Cleanup in case shit hit the fan $this->sendResponse(500, $e->getMessage()); - return; } } diff --git a/ext/replace_file/main.php b/ext/replace_file/main.php index 8de3f9a9..dcd80296 100644 --- a/ext/replace_file/main.php +++ b/ext/replace_file/main.php @@ -69,7 +69,7 @@ class ReplaceFile extends Extension $target = warehouse_path(Image::IMAGE_DIR, $event->new_hash); if (!@copy($event->tmp_filename, $target)) { $errors = error_get_last(); - throw new UploadException( + throw new ImageReplaceException( "Failed to copy file from uploads ({$event->tmp_filename}) to archive ($target): ". "{$errors['type']} / {$errors['message']}" ); diff --git a/ext/res_limit/test.php b/ext/res_limit/test.php index 7bb9ee49..d8a51de4 100644 --- a/ext/res_limit/test.php +++ b/ext/res_limit/test.php @@ -33,12 +33,10 @@ class ResolutionLimitTest extends ShimmiePHPUnitTestCase $config->set_string("upload_ratios", "4:3 16:9"); $this->log_in_as_user(); - try { + $e = $this->assertException(UploadException::class, function () { $this->post_image("tests/pbx_screenshot.jpg", "pbx computer screenshot"); - $this->fail("Invalid-size image was allowed"); - } catch (UploadException $e) { - $this->assertEquals("Post too small", $e->getMessage()); - } + }); + $this->assertEquals("Post too small", $e->getMessage()); } public function testResLimitLarge() @@ -50,12 +48,10 @@ class ResolutionLimitTest extends ShimmiePHPUnitTestCase $config->set_int("upload_max_width", 100); $config->set_string("upload_ratios", "4:3 16:9"); - try { + $e = $this->assertException(UploadException::class, function () { $this->post_image("tests/pbx_screenshot.jpg", "pbx computer screenshot"); - $this->fail("Invalid-size image was allowed"); - } catch (UploadException $e) { - $this->assertEquals("Post too large", $e->getMessage()); - } + }); + $this->assertEquals("Post too large", $e->getMessage()); } public function testResLimitRatio() @@ -67,12 +63,10 @@ class ResolutionLimitTest extends ShimmiePHPUnitTestCase $config->set_int("upload_max_width", -1); $config->set_string("upload_ratios", "16:9"); - try { + $e = $this->assertException(UploadException::class, function () { $this->post_image("tests/pbx_screenshot.jpg", "pbx computer screenshot"); - $this->fail("Invalid-size image was allowed"); - } catch (UploadException $e) { - $this->assertEquals("Post needs to be in one of these ratios: 16:9", $e->getMessage()); - } + }); + $this->assertEquals("Post needs to be in one of these ratios: 16:9", $e->getMessage()); } # reset to defaults, otherwise this can interfere with diff --git a/ext/upload/main.php b/ext/upload/main.php index f10900ed..fa79f46a 100644 --- a/ext/upload/main.php +++ b/ext/upload/main.php @@ -315,7 +315,7 @@ class Upload extends Extension */ private function try_upload(array $file, array $tags, ?string $source = null): array { - global $page, $config; + global $page, $config, $database; // blank file boxes cause empty uploads, no need for error message if (empty($file['name'])) { @@ -337,17 +337,17 @@ class Upload extends Extension continue; } try { + $database->execute("SAVEPOINT upload"); // check if the upload was successful if ($error !== UPLOAD_ERR_OK) { throw new UploadException($this->upload_error_message($error)); } - $metadata = []; - $metadata['filename'] = pathinfo($name, PATHINFO_BASENAME); - $metadata['tags'] = $tags; - $metadata['source'] = $source; - - $event = new DataUploadEvent($tmp_name, $metadata); + $event = new DataUploadEvent($tmp_name, [ + 'filename' => pathinfo($name, PATHINFO_BASENAME), + 'tags' => $tags, + 'source' => $source, + ]); send_event($event); if (count($event->images) == 0) { throw new UploadException("MIME type not supported: " . $event->mime); @@ -355,7 +355,9 @@ class Upload extends Extension foreach($event->images as $image) { $results[] = new UploadSuccess($name, $image->id); } + $database->execute("RELEASE SAVEPOINT upload"); } catch (UploadException $ex) { + $database->execute("ROLLBACK TO SAVEPOINT upload"); $results[] = new UploadError($name, $ex->getMessage()); } } @@ -368,12 +370,13 @@ class Upload extends Extension */ private function try_transload(string $url, array $tags, string $source = null): array { - global $page, $config, $user; + global $page, $config, $user, $database; $results = []; $tmp_filename = tempnam(ini_get('upload_tmp_dir'), "shimmie_transload"); try { + $database->execute("SAVEPOINT upload"); // Fetch file $headers = fetch_url($url, $tmp_filename); if (is_null($headers)) { @@ -410,7 +413,9 @@ class Upload extends Extension foreach($event->images as $image) { $results[] = new UploadSuccess($url, $image->id); } + $database->execute("RELEASE SAVEPOINT upload"); } catch (UploadException $ex) { + $database->execute("ROLLBACK TO SAVEPOINT upload"); $results[] = new UploadError($url, $ex->getMessage()); } finally { if (file_exists($tmp_filename)) { diff --git a/ext/upload/test.php b/ext/upload/test.php index 0f0ff1df..85ab424c 100644 --- a/ext/upload/test.php +++ b/ext/upload/test.php @@ -64,11 +64,10 @@ class UploadTest extends ShimmiePHPUnitTestCase { $this->post_image("tests/pbx_screenshot.jpg", "pbx computer screenshot"); - try { + $e = $this->assertException(UploadException::class, function () { $this->post_image("tests/pbx_screenshot.jpg", "pbx computer screenshot"); - } catch (UploadException $e) { - $this->assertStringContainsString("already has hash", $e->getMessage()); - } + }); + $this->assertStringContainsString("already has hash", $e->getMessage()); } public function testRejectUnknownFiletype() @@ -81,12 +80,10 @@ class UploadTest extends ShimmiePHPUnitTestCase { // FIXME: huge.dat is rejected for other reasons; manual testing shows that this works file_put_contents("data/huge.jpg", file_get_contents("tests/pbx_screenshot.jpg") . str_repeat("U", 1024 * 1024 * 3)); - try { + $e = $this->assertException(UploadException::class, function () { $this->post_image("data/huge.jpg", "test"); - $this->fail("Uploading huge.jpg didn't fail..."); - } catch (UploadException $e) { - $this->assertEquals("File too large (3.0MB > 1.0MB)", $e->error); - } + }); unlink("data/huge.jpg"); + $this->assertEquals("File too large (3.0MB > 1.0MB)", $e->getMessage()); } }