From 53f1951ba9094d579629c0ee5812b992dd15b0f7 Mon Sep 17 00:00:00 2001 From: Shubhangi Singh Date: Wed, 22 Oct 2025 05:56:30 +0000 Subject: [PATCH 1/8] WIP --- .../acceptance/storage/file_test.rb | 31 +++++++++ .../lib/google/cloud/storage/file.rb | 63 +++++++++++++++++++ 2 files changed, 94 insertions(+) diff --git a/google-cloud-storage/acceptance/storage/file_test.rb b/google-cloud-storage/acceptance/storage/file_test.rb index 248b724e5a8e..2abb743229a6 100644 --- a/google-cloud-storage/acceptance/storage/file_test.rb +++ b/google-cloud-storage/acceptance/storage/file_test.rb @@ -1040,4 +1040,35 @@ expect { uploaded_file.retention = retention }.must_raise Google::Cloud::PermissionDeniedError end end + + describe "restricting downloading files to current working directory" do + let(:original) { File.new(files[:logo][:path]) } + + def upload_logo + bucket.create_file(original, "CloudLogo.png") + end + + def assert_security_error(path, expected_message) + uploaded = upload_logo + + error = expect do + uploaded.download(path) + end.must_raise SecurityError + + _(error.message).must_match expected_message + uploaded.delete + end + + it "raises error when downloading to an absolute path" do + assert_security_error "/absolute/path/to/file.png", /Absolute path not allowed in user input/ + end + + it "raises error when downloading to a path with parent directory traversal" do + assert_security_error "../parent/directory/traversal/file.png", /Directory traversal attempt detected./ + end + + it "raises error when downloading to a nested path with parent directory traversal" do + assert_security_error "test/../../parent/directory/traversal/file.png", /Directory traversal attempt detected./ + end + end end diff --git a/google-cloud-storage/lib/google/cloud/storage/file.rb b/google-cloud-storage/lib/google/cloud/storage/file.rb index 8df9f7b13a5a..108dcf95faf9 100644 --- a/google-cloud-storage/lib/google/cloud/storage/file.rb +++ b/google-cloud-storage/lib/google/cloud/storage/file.rb @@ -1065,6 +1065,8 @@ def download path = nil, verify: :md5, encryption_key: nil, range: nil, if path.nil? path = StringIO.new path.set_encoding "ASCII-8BIT" + else + safe_path_for_download path end file, resp = service.download_file bucket, name, path, @@ -2270,6 +2272,67 @@ def gzip_decompress local_file end end + + # Validates a user-supplied path for downloading a file. This method is + # intended to prevent directory traversal attacks by ensuring the final + # resolved path is within the current working directory. + # + # @param [String] user_supplied_path The local path where the file will be + # downloaded. This path must be relative to the current working + # directory. + # + # @raise [ArgumentError] If the provided `user_supplied_path` is not a + # String. + # @raise [SecurityError] If the `user_supplied_path` is an absolute path + # or if it resolves to a location outside of the current working + # directory. + # + # @return [String] The expanded, validated, and safe local path for the + # download. + # @example + # # Assuming the current working directory is /home/user + # safe_path = safe_path_for_download "downloads/file.txt" + # # safe_path will be "/home/user/downloads/file.txt" + # + # # The following would raise a SecurityError: + # # safe_path_for_download "/etc/passwd" + # # safe_path_for_download "../../../etc/passwd" + # + + def safe_path_for_download user_supplied_path + # binding.pry + + # Allow StringIO and Tempfile (in test env) to pass through + if user_supplied_path.is_a?(StringIO) || + (ENV['TEST'] && user_supplied_path.is_a?(Tempfile)) || + (ENV['TEST'] && user_supplied_path.is_a?(String) && user_supplied_path.start_with?("/tmp")) + + return user_supplied_path + end + + # Ensure the input is a String. + unless user_supplied_path.is_a?(String) + raise ArgumentError, "Path must be a String: #{user_supplied_path.class}" + end + + # Disallow if path is absolute. + # binding.pry + if user_supplied_path.start_with?('/', ::File::SEPARATOR) || user_supplied_path =~ /^[a-zA-Z]:/ + raise SecurityError, "Absolute path not allowed in user input: #{user_supplied_path}" + end + + # Resolve path against the current working directory. + base_dir = Dir.pwd + download_path = ::File.expand_path user_supplied_path, base_dir + + # Prevent directory traversal outside the base directory + unless download_path.start_with? base_dir + raise SecurityError, "Directory traversal attempt detected." + end + + download_path + end + ## # Yielded to a block to accumulate changes for a patch request. class Updater < File From 0978a84eabc558abde7eb5892ea76e3ffafd4d6e Mon Sep 17 00:00:00 2001 From: Shubhangi Singh Date: Wed, 22 Oct 2025 07:59:56 +0000 Subject: [PATCH 2/8] wip --- .../lib/google/cloud/storage/file.rb | 41 ++++++++++--------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/google-cloud-storage/lib/google/cloud/storage/file.rb b/google-cloud-storage/lib/google/cloud/storage/file.rb index 108dcf95faf9..0d2980641807 100644 --- a/google-cloud-storage/lib/google/cloud/storage/file.rb +++ b/google-cloud-storage/lib/google/cloud/storage/file.rb @@ -1066,7 +1066,7 @@ def download path = nil, verify: :md5, encryption_key: nil, range: nil, path = StringIO.new path.set_encoding "ASCII-8BIT" else - safe_path_for_download path + path = safe_path_for_download path end file, resp = service.download_file bucket, name, path, @@ -2300,35 +2300,38 @@ def gzip_decompress local_file # def safe_path_for_download user_supplied_path - # binding.pry - # Allow StringIO and Tempfile (in test env) to pass through - if user_supplied_path.is_a?(StringIO) || - (ENV['TEST'] && user_supplied_path.is_a?(Tempfile)) || - (ENV['TEST'] && user_supplied_path.is_a?(String) && user_supplied_path.start_with?("/tmp")) - - return user_supplied_path - end + # Allow StringIO to pass through + return user_supplied_path if user_supplied_path.is_a? StringIO - # Ensure the input is a String. - unless user_supplied_path.is_a?(String) - raise ArgumentError, "Path must be a String: #{user_supplied_path.class}" + # Allow Tempfile and /tmp paths in test env to pass through + if ENV["TEST"] && + (user_supplied_path.is_a?(Tempfile) || + (user_supplied_path.is_a?(String) && user_supplied_path.start_with?("/tmp/"))) + return user_supplied_path end # Disallow if path is absolute. - # binding.pry - if user_supplied_path.start_with?('/', ::File::SEPARATOR) || user_supplied_path =~ /^[a-zA-Z]:/ + path_obj = Pathname.new user_supplied_path + if path_obj.absolute? raise SecurityError, "Absolute path not allowed in user input: #{user_supplied_path}" end # Resolve path against the current working directory. - base_dir = Dir.pwd - download_path = ::File.expand_path user_supplied_path, base_dir - - # Prevent directory traversal outside the base directory - unless download_path.start_with? base_dir + base_dir_path = Pathname.new Dir.pwd + download_path_obj = (base_dir_path + path_obj).cleanpath + + # Prevent directory traversal outside the base directory + begin + relative = download_path_obj.relative_path_from base_dir_path + if relative.to_s.start_with?("..") + raise SecurityError, "Directory traversal attempt detected." + end + rescue ArgumentError + # This can happen on Windows with different drives, which means it's outside. raise SecurityError, "Directory traversal attempt detected." end + download_path = download_path_obj.to_s download_path end From e7c9c234f52d1de74463cbca803500ac35f4b889 Mon Sep 17 00:00:00 2001 From: Shubhangi Singh Date: Wed, 22 Oct 2025 08:47:30 +0000 Subject: [PATCH 3/8] fix --- .../lib/google/cloud/storage/file.rb | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/google-cloud-storage/lib/google/cloud/storage/file.rb b/google-cloud-storage/lib/google/cloud/storage/file.rb index 0d2980641807..d4ae0ab4566a 100644 --- a/google-cloud-storage/lib/google/cloud/storage/file.rb +++ b/google-cloud-storage/lib/google/cloud/storage/file.rb @@ -2305,8 +2305,7 @@ def safe_path_for_download user_supplied_path return user_supplied_path if user_supplied_path.is_a? StringIO # Allow Tempfile and /tmp paths in test env to pass through - if ENV["TEST"] && - (user_supplied_path.is_a?(Tempfile) || + if (user_supplied_path.is_a?(Tempfile) || (user_supplied_path.is_a?(String) && user_supplied_path.start_with?("/tmp/"))) return user_supplied_path end @@ -2321,19 +2320,17 @@ def safe_path_for_download user_supplied_path base_dir_path = Pathname.new Dir.pwd download_path_obj = (base_dir_path + path_obj).cleanpath - # Prevent directory traversal outside the base directory + # Prevent directory traversal outside the base directory begin - relative = download_path_obj.relative_path_from base_dir_path - if relative.to_s.start_with?("..") + relative = download_path_obj.relative_path_from base_dir_path + if relative.to_s.start_with? ".." raise SecurityError, "Directory traversal attempt detected." end rescue ArgumentError # This can happen on Windows with different drives, which means it's outside. raise SecurityError, "Directory traversal attempt detected." end - download_path = download_path_obj.to_s - - download_path + download_path_obj.to_s end ## From 5ccb76a5d9d1f5c89ba41ba53137a67baf9af75a Mon Sep 17 00:00:00 2001 From: Shubhangi Singh Date: Wed, 22 Oct 2025 17:25:28 +0000 Subject: [PATCH 4/8] wip --- .../lib/google/cloud/storage/file.rb | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/google-cloud-storage/lib/google/cloud/storage/file.rb b/google-cloud-storage/lib/google/cloud/storage/file.rb index d4ae0ab4566a..f31747931aab 100644 --- a/google-cloud-storage/lib/google/cloud/storage/file.rb +++ b/google-cloud-storage/lib/google/cloud/storage/file.rb @@ -2301,13 +2301,18 @@ def gzip_decompress local_file def safe_path_for_download user_supplied_path + temp_regex = /\A#{Regexp.escape(Dir.tmpdir)}/ + # Allow StringIO to pass through return user_supplied_path if user_supplied_path.is_a? StringIO # Allow Tempfile and /tmp paths in test env to pass through - if (user_supplied_path.is_a?(Tempfile) || - (user_supplied_path.is_a?(String) && user_supplied_path.start_with?("/tmp/"))) - return user_supplied_path + # if user_supplied_path.is_a?(Tempfile) || + # (user_supplied_path.is_a?(String) && user_supplied_path.start_with?("/tmp/")) + # return user_supplied_path + # end + if user_supplied_path.is_a?(Tempfile) || user_supplied_path =~ temp_regex + return user_supplied_path end # Disallow if path is absolute. @@ -2322,7 +2327,7 @@ def safe_path_for_download user_supplied_path # Prevent directory traversal outside the base directory begin - relative = download_path_obj.relative_path_from base_dir_path + relative = download_path_obj.relative_path_from base_dir_path if relative.to_s.start_with? ".." raise SecurityError, "Directory traversal attempt detected." end From 9a6552da10ad791e0a903367f06b510706cd71ba Mon Sep 17 00:00:00 2001 From: Shubhangi Singh Date: Mon, 27 Oct 2025 12:09:50 +0000 Subject: [PATCH 5/8] adding unit test cases --- .../lib/google/cloud/storage/file.rb | 11 ++------ .../test/google/cloud/storage/file_test.rb | 26 +++++++++++++++++++ 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/google-cloud-storage/lib/google/cloud/storage/file.rb b/google-cloud-storage/lib/google/cloud/storage/file.rb index f31747931aab..e3ca156b33c9 100644 --- a/google-cloud-storage/lib/google/cloud/storage/file.rb +++ b/google-cloud-storage/lib/google/cloud/storage/file.rb @@ -2280,9 +2280,6 @@ def gzip_decompress local_file # @param [String] user_supplied_path The local path where the file will be # downloaded. This path must be relative to the current working # directory. - # - # @raise [ArgumentError] If the provided `user_supplied_path` is not a - # String. # @raise [SecurityError] If the `user_supplied_path` is an absolute path # or if it resolves to a location outside of the current working # directory. @@ -2301,18 +2298,14 @@ def gzip_decompress local_file def safe_path_for_download user_supplied_path - temp_regex = /\A#{Regexp.escape(Dir.tmpdir)}/ + temp_regex = /\A#{Regexp.escape Dir.tmpdir }/ # Allow StringIO to pass through return user_supplied_path if user_supplied_path.is_a? StringIO # Allow Tempfile and /tmp paths in test env to pass through - # if user_supplied_path.is_a?(Tempfile) || - # (user_supplied_path.is_a?(String) && user_supplied_path.start_with?("/tmp/")) - # return user_supplied_path - # end if user_supplied_path.is_a?(Tempfile) || user_supplied_path =~ temp_regex - return user_supplied_path + return user_supplied_path end # Disallow if path is absolute. diff --git a/google-cloud-storage/test/google/cloud/storage/file_test.rb b/google-cloud-storage/test/google/cloud/storage/file_test.rb index 4b52e9954b82..f147694755e2 100644 --- a/google-cloud-storage/test/google/cloud/storage/file_test.rb +++ b/google-cloud-storage/test/google/cloud/storage/file_test.rb @@ -653,6 +653,32 @@ def file.crc32c; "crc32c="; end mock.verify end end + + describe "restricting downloading files to current working directory" do + + def assert_security_error(path, expected_message) + error = expect do + file.download path + end.must_raise SecurityError + _(error.message).must_match expected_message + end + + it "raises error when downloading to an absolute path" do + assert_security_error "/absolute/path/to/file.png", /Absolute path not allowed in user input/ + end + + it "raises error when downloading to a path with parent directory traversal" do + assert_security_error "../parent/directory/traversal/file.png", /Directory traversal attempt detected./ + end + + it "raises error when downloading to a nested path with parent directory traversal" do + assert_security_error "test/../../parent/directory/traversal/file.png", /Directory traversal attempt detected./ + end + + it "raises error when downloading to a nested path with parent directory traversal" do + assert_security_error "test/../../parent/directory/traversal/file.png", /Directory traversal attempt detected./ + end + end end describe "File#copy" do From b92bfdc5be635798ef5d0ae3c83ec7c3d666f2df Mon Sep 17 00:00:00 2001 From: Shubhangi Singh Date: Mon, 27 Oct 2025 12:13:30 +0000 Subject: [PATCH 6/8] lint fix --- google-cloud-storage/lib/google/cloud/storage/file.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google-cloud-storage/lib/google/cloud/storage/file.rb b/google-cloud-storage/lib/google/cloud/storage/file.rb index e3ca156b33c9..40095288bce7 100644 --- a/google-cloud-storage/lib/google/cloud/storage/file.rb +++ b/google-cloud-storage/lib/google/cloud/storage/file.rb @@ -2298,7 +2298,7 @@ def gzip_decompress local_file def safe_path_for_download user_supplied_path - temp_regex = /\A#{Regexp.escape Dir.tmpdir }/ + temp_regex = /\A#{Regexp.escape Dir.tmpdir}/ # Allow StringIO to pass through return user_supplied_path if user_supplied_path.is_a? StringIO From b7b77053a07a7e6c61aa6af4860dbaee609fd218 Mon Sep 17 00:00:00 2001 From: Shubhangi Singh Date: Mon, 27 Oct 2025 12:16:31 +0000 Subject: [PATCH 7/8] lix fix --- google-cloud-storage/test/google/cloud/storage/file_test.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/google-cloud-storage/test/google/cloud/storage/file_test.rb b/google-cloud-storage/test/google/cloud/storage/file_test.rb index f147694755e2..1bb4b44ed187 100644 --- a/google-cloud-storage/test/google/cloud/storage/file_test.rb +++ b/google-cloud-storage/test/google/cloud/storage/file_test.rb @@ -674,10 +674,6 @@ def assert_security_error(path, expected_message) it "raises error when downloading to a nested path with parent directory traversal" do assert_security_error "test/../../parent/directory/traversal/file.png", /Directory traversal attempt detected./ end - - it "raises error when downloading to a nested path with parent directory traversal" do - assert_security_error "test/../../parent/directory/traversal/file.png", /Directory traversal attempt detected./ - end end end From d7e63fdca48686b34febc520a01b5a8da1e78322 Mon Sep 17 00:00:00 2001 From: Shubhangi Singh Date: Mon, 27 Oct 2025 12:17:50 +0000 Subject: [PATCH 8/8] fix lint --- google-cloud-storage/lib/google/cloud/storage/file.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/google-cloud-storage/lib/google/cloud/storage/file.rb b/google-cloud-storage/lib/google/cloud/storage/file.rb index 40095288bce7..edfaacc9341d 100644 --- a/google-cloud-storage/lib/google/cloud/storage/file.rb +++ b/google-cloud-storage/lib/google/cloud/storage/file.rb @@ -2297,7 +2297,6 @@ def gzip_decompress local_file # def safe_path_for_download user_supplied_path - temp_regex = /\A#{Regexp.escape Dir.tmpdir}/ # Allow StringIO to pass through