From afc9fc8c186878147f4df6206926ca33bb289971 Mon Sep 17 00:00:00 2001 From: Bojidar Marinov Date: Tue, 14 Apr 2020 23:08:14 +0300 Subject: [PATCH] Remove download hashes (#209) * Remove download hashes, as they caused too much hassle without much security * Make usage of git commits compulsory for non-moderators That way, we can be sure that users would download the asset as it was accepted, at least when working with large repository providers. Note that moderators still need to be able to input invalid hashes when using the "Custom" provider; and allowing a bit of flexibility would help in the long run. --- API.md | 2 +- data/data.sql | 1 - data/migration-7.sql | 2 ++ src/Helpers/Utils.php | 8 +++---- src/queries.php | 10 +++----- src/routes/asset.php | 38 +---------------------------- src/routes/asset_edit.php | 45 +++++++++++------------------------ templates/_asset_fields.phtml | 7 +++--- templates/asset.phtml | 10 -------- templates/asset_edit.phtml | 2 -- 10 files changed, 28 insertions(+), 97 deletions(-) create mode 100644 data/migration-7.sql diff --git a/API.md b/API.md index 87b699e..1647f4a 100644 --- a/API.md +++ b/API.md @@ -223,6 +223,7 @@ Example result: Notes: * The `cost` field is the license. Other asset libraries may put the price there and supply a download URL which requires authentication. +* The `download_hash` field is always empty and is kept for compatibility only. * The download URL is generated based on the download commit and the browse URL.
@@ -405,7 +406,6 @@ Moderator-only. Put an edit in review. It is impossible to change it after this. ```json { "token": "…", - "hash": "new sha256 hash" } ``` Successful result: the asset edit, without the original asset. diff --git a/data/data.sql b/data/data.sql index b8bef36..60c531f 100644 --- a/data/data.sql +++ b/data/data.sql @@ -18,7 +18,6 @@ CREATE TABLE `as_assets` ( `support_level` tinyint(4) NOT NULL, `download_provider` tinyint(4) NOT NULL, `download_commit` varchar(2048) NOT NULL, - `download_hash` text NOT NULL, `browse_url` varchar(1024) NOT NULL, `issues_url` varchar(1024) NOT NULL, `icon_url` varchar(1024) NOT NULL, diff --git a/data/migration-7.sql b/data/migration-7.sql new file mode 100644 index 0000000..00c71c4 --- /dev/null +++ b/data/migration-7.sql @@ -0,0 +1,2 @@ + +ALTER TABLE `as_assets` DROP `download_hash`; diff --git a/src/Helpers/Utils.php b/src/Helpers/Utils.php index 02bb561..51fa330 100644 --- a/src/Helpers/Utils.php +++ b/src/Helpers/Utils.php @@ -23,11 +23,9 @@ class Utils $warning[] = "\"$repo_url\" doesn't look correct; it probably shouldn't end in .git. $warning_suffix"; } if ($provider != 'Custom') { - if ($commit == 'master') { - $light_warning[] = "Giving 'master' (or any other branch name) as the commit to be downloaded is not recommended, since it would invalidate the asset when you push a new version (as we ensure the version is kept the same via a sha256 hash of the zip). You can try using tags instead.\n"; - } - if (sizeof(preg_grep('/\/|\\|\:|^\.|\ |\^|\~|\?|\*|\[|^\@$|\@\{/', [$commit])) != 0) { - $light_warning[] = "The inputted download commit is not a valid git ref; please ensure you aren't giving a full URL. (If your tag includes '/' in its name, consider escaping it as '%2F')\n"; + // Git commits are either 40 (SHA1) or 64 (SHA2) hex characters + if (sizeof(preg_grep('/^[a-f0-9]{40}([a-f0-9]{24})?$/', [$commit])) == 0) { + $warning[] = "Using git tags or branches is no longer supported. Please give a full git commit hash instead.\n"; } } switch ($provider) { diff --git a/src/queries.php b/src/queries.php index 817b316..06086e0 100644 --- a/src/queries.php +++ b/src/queries.php @@ -71,7 +71,7 @@ return [ OR username LIKE :filter )', - 'get_one' => 'SELECT asset_id, category_type, title, username as author, user_id as author_id, version, version_string, category, category_id, godot_version, rating, cost, description, support_level, download_provider, download_commit, download_hash, browse_url, issues_url, icon_url, preview_id, `as_asset_previews`.type, link, thumbnail, searchable, modify_date FROM `as_assets` + 'get_one' => 'SELECT asset_id, category_type, title, username as author, user_id as author_id, version, version_string, category, category_id, godot_version, rating, cost, description, support_level, download_provider, download_commit, browse_url, issues_url, icon_url, preview_id, `as_asset_previews`.type, link, thumbnail, searchable, modify_date FROM `as_assets` LEFT JOIN `as_categories` USING (category_id) LEFT JOIN `as_users` USING (user_id) LEFT JOIN `as_asset_previews` USING (asset_id) @@ -83,12 +83,12 @@ return [ 'apply_creational_edit' => 'INSERT INTO `as_assets` SET user_id=:user_id, title=:title, description=:description, category_id=:category_id, godot_version=:godot_version, version_string=:version_string, cost=:cost, - download_provider=:download_provider, download_commit=:download_commit, download_hash=:download_hash, browse_url=:browse_url, issues_url=:issues_url, icon_url=:icon_url, + download_provider=:download_provider, download_commit=:download_commit, browse_url=:browse_url, issues_url=:issues_url, icon_url=:icon_url, version=0+:update_version, support_level=:support_level, rating=0, searchable=TRUE', 'apply_edit' => 'UPDATE `as_assets` SET title=COALESCE(:title, title), description=COALESCE(:description, description), category_id=COALESCE(:category_id, category_id), godot_version=COALESCE(:godot_version, godot_version), version_string=COALESCE(:version_string, version_string), cost=COALESCE(:cost, cost), - download_provider=COALESCE(:download_provider, download_provider), download_commit=COALESCE(:download_commit, download_commit), download_hash=COALESCE(:download_hash, download_hash), browse_url=COALESCE(:browse_url, browse_url), issues_url=COALESCE(:issues_url, issues_url), icon_url=COALESCE(:icon_url, icon_url), + download_provider=COALESCE(:download_provider, download_provider), download_commit=COALESCE(:download_commit, download_commit), browse_url=COALESCE(:browse_url, browse_url), issues_url=COALESCE(:issues_url, issues_url), icon_url=COALESCE(:icon_url, icon_url), version=version+:update_version WHERE asset_id=:asset_id', @@ -104,10 +104,6 @@ return [ SET support_level=:support_level WHERE asset_id=:asset_id', - 'set_download_hash' => 'UPDATE `as_assets` - SET download_hash=:download_hash - WHERE asset_id=:asset_id', - 'delete' => 'UPDATE `as_assets` SET searchable=FALSE WHERE asset_id=:asset_id', 'undelete' => 'UPDATE `as_assets` SET searchable=TRUE WHERE asset_id=:asset_id' ], diff --git a/src/routes/asset.php b/src/routes/asset.php index bb58b5c..15b3a65 100644 --- a/src/routes/asset.php +++ b/src/routes/asset.php @@ -219,6 +219,7 @@ $get_asset = function ($request, $response, $args) { } $asset_info['previews'] = $previews; + $asset_info['download_hash'] = ''; return $response->withJson($asset_info, 200); }; @@ -268,43 +269,6 @@ $app->post('/asset/{id:[0-9]+}/support_level', function ($request, $response, $a ], 200); }); -// Change download hash of an asset -$app->post('/asset/{id:[0-9]+}/download_hash', function ($request, $response, $args) { - $body = $request->getParsedBody(); - - $error = $this->utils->ensureLoggedIn(false, $response, $body, $user); - $error = $this->utils->errorResponseIfNotUserHasLevel($error, $response, $user, 'moderator'); - $error = $this->utils->errorResponseIfMissingOrNotString($error, $response, $body, 'hash'); - if ($error) { - return $response; - } - - $body['hash'] = trim($body['hash']); - if (sizeof(preg_grep('/^[a-f0-9]{64}$/', [$body['hash']])) == 0) { - return $response->withJson([ - 'error' => 'Invalid hash given. Expected 64 lowercase hexadecimal digits.', - ]); - } - - - $query = $this->queries['asset']['set_download_hash']; - - $query->bindValue(':asset_id', (int) $args['id'], PDO::PARAM_INT); - $query->bindValue(':download_hash', $body['hash']); - - $query->execute(); - - $error = $this->utils->errorResponseIfQueryBad(false, $response, $query); - if ($error) { - return $response; - } - - return $response->withJson([ - 'changed' => true, - 'url' => 'asset/' . $args['id'], - ], 200); -}); - /* * Delete asset from library */ diff --git a/src/routes/asset_edit.php b/src/routes/asset_edit.php index c01ed66..2a4840e 100644 --- a/src/routes/asset_edit.php +++ b/src/routes/asset_edit.php @@ -101,10 +101,22 @@ function _insert_asset_edit_fields($c, $error, &$response, $query, $body, $requi ); } } + if (isset($body['icon_url'])) { $icon_url = $body['icon_url']; if (sizeof(preg_grep('/^https?:\/\/.+?\.(png|jpg|jpeg)$/i', [$icon_url])) == 0) { - $warning[] = "\"$icon_url\" doesn't look correct; it should be similar to \"http://.\". Make sure the icon URL is correct.\n"; + $warning[] = "\"$icon_url\" doesn't look correct; it should be similar to \"http://.\". Make sure the icon URL is correct."; + } + } + + if (isset($body['download_commit'])) { + // Git commits are either 40 (SHA1) or 64 (SHA2) hex characters + if (sizeof(preg_grep('/^[a-f0-9]{40}([a-f0-9]{24})?$/', [$body['download_commit']])) == 0) { + $error = $c->utils->ensureLoggedIn($error, $response, $body, $user); + $error = $c->utils->errorResponseIfNotUserHasLevel($error, $response, $user, 'moderator', 'Using git tags or branches is no longer supported. Please give a full git commit hash instead.'); + if ($error) { + return $response; + } } } @@ -737,36 +749,7 @@ $app->post('/asset/edit/{id:[0-9]+}/accept', function ($request, $response, $arg } } - if ($update_version) { - $error = $this->utils->errorResponseIfMissingOrNotString(false, $response, $body, 'hash'); - if ($error) { - return $response; - } - - $body['hash'] = trim($body['hash']); - if (sizeof(preg_grep('/^[a-f0-9]{64}$/', [$body['hash']])) == 0) { - return $response->withJson([ - 'error' => 'Invalid hash given. Expected 64 lowercase hexadecimal digits.', - ]); - } - - $query->bindValue(':update_version', 1, PDO::PARAM_INT); - $query->bindValue(':download_hash', $body['hash']); - } else { - if (isset($body['hash']) && trim($body['hash']) != '') { - $body['hash'] = trim($body['hash']); - if (sizeof(preg_grep('/^[a-f0-9]{64}$/', [$body['hash']])) == 0) { - return $response->withJson([ - 'error' => 'Invalid hash given. Expected either nothing or 64 lowercase hexadecimal digits.', - ]); - } - $query->bindValue(':update_version', 1, PDO::PARAM_INT); - $query->bindValue(':download_hash', $body['hash']); - } else { - $query->bindValue(':update_version', 0, PDO::PARAM_INT); - $query->bindValue(':download_hash', null, PDO::PARAM_NULL); - } - } + $query->bindValue(':update_version', (int) $update_version, PDO::PARAM_INT); $this->db->beginTransaction(); diff --git a/templates/_asset_fields.phtml b/templates/_asset_fields.phtml index ebcf93d..71aaf17 100644 --- a/templates/_asset_fields.phtml +++ b/templates/_asset_fields.phtml @@ -171,10 +171,11 @@ $_asset_values = array_merge([
- +
- - The commit or tag that should be downloaded. If using a tag name, make sure that you actually created the tag on your repository. + pattern="[a-f0-9]{40}([a-f0-9]{24})?" > + The commit hash that should be downloaded. If using a tag name, make sure that you actually created the tag on your repository. + Expected format: 40 or 64 hexadecimal digits, fully specifying a git commit. = $constants['user_type']['moderator'])) { ?> When using the Custom provider, this is the download URL. diff --git a/templates/asset.phtml b/templates/asset.phtml index 061343f..ae61eea 100644 --- a/templates/asset.phtml +++ b/templates/asset.phtml @@ -55,7 +55,6 @@ Recent Edits

-

Sha256 Hash:

$preview) { ?> @@ -94,15 +93,6 @@
-
- -
- - - - -
-
diff --git a/templates/asset_edit.phtml b/templates/asset_edit.phtml index 43ef403..121a7af 100644 --- a/templates/asset_edit.phtml +++ b/templates/asset_edit.phtml @@ -184,8 +184,6 @@ $preview_field_names = [
- -