From 7feca531b1cddd815785bc8793f129ba392510aa Mon Sep 17 00:00:00 2001
From: Stephen von Takach <steve@place.technology>
Date: Thu, 7 Sep 2023 17:37:11 +1000
Subject: [PATCH] feat(uploads): improve upload validation (#360)

provide more descriptive errors
---
 shard.lock                                  |  4 +--
 spec/controllers/uploads_spec.cr            | 11 ++++----
 src/placeos-rest-api/controllers/uploads.cr | 31 +++++++++++++++++----
 3 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/shard.lock b/shard.lock
index 0c65b18e..a8586ed5 100644
--- a/shard.lock
+++ b/shard.lock
@@ -255,7 +255,7 @@ shards:
 
   placeos-frontend-loader:
     git: https://github.com/placeos/frontend-loader.git
-    version: 2.7.1+git.commit.7ba696750e3876082b39476e0c399ce3890f8669
+    version: 2.7.1+git.commit.352a520740fe85d9517e41325e883a8edf104b2b
 
   placeos-log-backend:
     git: https://github.com/place-labs/log-backend.git
@@ -263,7 +263,7 @@ shards:
 
   placeos-models:
     git: https://github.com/placeos/models.git
-    version: 9.17.3
+    version: 9.18.0
 
   placeos-resource:
     git: https://github.com/place-labs/resource.git
diff --git a/spec/controllers/uploads_spec.cr b/spec/controllers/uploads_spec.cr
index 2726df38..2d34c62f 100644
--- a/spec/controllers/uploads_spec.cr
+++ b/spec/controllers/uploads_spec.cr
@@ -2,6 +2,10 @@ require "../helper"
 
 module PlaceOS::Api
   describe Uploads do
+    ::Spec.before_each do
+      Model::Storage.clear
+    end
+
     it "new should return the Storage Provider" do
       Model::Generator.storage.save!
       params = HTTP::Params.encode({
@@ -29,7 +33,7 @@ module PlaceOS::Api
 
       resp = client.get("#{Uploads.base_route}/new?#{params}",
         headers: Spec::Authentication.headers)
-      resp.status_code.should eq(401)
+      resp.status_code.should eq(400)
       JSON.parse(resp.body).as_h["error"].as_s.should eq("File extension not allowed")
     end
 
@@ -47,12 +51,11 @@ module PlaceOS::Api
       resp = client.post(Uploads.base_route,
         body: params.to_json,
         headers: Spec::Authentication.headers)
-      resp.status_code.should eq(401)
+      resp.status_code.should eq(400)
       JSON.parse(resp.body).as_h["error"].as_s.should eq("File extension not allowed")
     end
 
     it "post should return the pre-signed signature" do
-      Model::Storage.clear
       Model::Generator.storage.save!
       params = {
         "file_name" => "some_file_name.jpg",
@@ -75,7 +78,6 @@ module PlaceOS::Api
     end
 
     it "post should return the pre-signed signature for multi-part" do
-      Model::Storage.clear
       Model::Generator.storage.save!
       params = {
         "file_name" => "some_file_name.jpg",
@@ -99,7 +101,6 @@ module PlaceOS::Api
     end
 
     it "should handle upload visibility" do
-      Model::Storage.clear
       Model::Generator.storage.save!
       params = {
         "file_name"   => "some_file_name.jpg",
diff --git a/src/placeos-rest-api/controllers/uploads.cr b/src/placeos-rest-api/controllers/uploads.cr
index 6a9ce2b0..8f4cdb22 100644
--- a/src/placeos-rest-api/controllers/uploads.cr
+++ b/src/placeos-rest-api/controllers/uploads.cr
@@ -288,13 +288,34 @@ module PlaceOS::Api
     end
 
     def allowed?(file_name, file_mime)
-      storage.check_file_ext(File.extname(file_name))
+      if !Model::Upload.safe_filename?(file_name)
+        raise AC::Route::Param::ValueError.new(
+          "filename contains unsupported characters or words",
+          "file_name"
+        )
+      end
+
+      begin
+        storage.check_file_ext(File.extname(file_name))
+      rescue error : PlaceOS::Model::Error
+        raise AC::Route::Param::ValueError.new(
+          error.message,
+          "file_name",
+          storage.ext_filter.join(",")
+        )
+      end
+
       if mime = file_mime
-        storage.check_file_mime(mime)
+        begin
+          storage.check_file_mime(mime)
+        rescue error : PlaceOS::Model::Error
+          raise AC::Route::Param::ValueError.new(
+            error.message,
+            "file_mime",
+            storage.mime_filter.join(",")
+          )
+        end
       end
-    rescue ex : PlaceOS::Model::Error
-      Log.error(exception: ex) { {file_name: file_name, mime_type: file_mime} }
-      raise Error::Unauthorized.new(ex.message || "Invalid file extension or mime type")
     end
   end
 end