-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fallback to copy symlinks on Windows #9296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
0365027
c814924
1c8bc39
180c269
de64429
7df9a0c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| require "win32/process" | ||
| require "rbconfig" | ||
|
|
||
| testuser = "testuser" | ||
| testpassword = "Password123+" | ||
|
|
||
| # Remove a previous test user if present | ||
| # See https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/net-user | ||
| system("net user #{testuser} /del 2>NUL") | ||
| # Create a new non-admin user | ||
| system("net user #{testuser} \"#{testpassword}\" /add") | ||
|
|
||
| pinfo = nil | ||
| IO.pipe do |stdout_read, stdout_write| | ||
| cmd = ARGV.join(" ") | ||
| env = { | ||
| "TMP" => "#{Dir.pwd}/tmp", | ||
| "TEMP" => "#{Dir.pwd}/tmp" | ||
| } | ||
| pinfo = Process.create command_line: cmd, | ||
| with_logon: testuser, | ||
| password: testpassword, | ||
| cwd: Dir.pwd, | ||
| environment: ENV.to_h.merge(env).map{|k,v| "#{k}=#{v}" }, | ||
| startup_info: { stdout: stdout_write, stderr: stdout_write } | ||
|
|
||
| stdout_write.close | ||
| stdout_read.each_line do |line| | ||
| puts(line) | ||
| end | ||
| end | ||
|
|
||
| # Wait for process to terminate | ||
| sleep 1 while !(ecode=Process.get_exitcode(pinfo.process_id)) | ||
|
|
||
| exit ecode |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -466,7 +466,7 @@ def extract_tar_gz(io, destination_dir, pattern = "*") # :nodoc: | |
|
|
||
| symlinks.each do |name, target, destination, real_destination| | ||
| if File.exist?(real_destination) | ||
| File.symlink(target, destination) | ||
| create_symlink(target, destination) | ||
| else | ||
| alert_warning "#{@spec.full_name} ships with a dangling symlink named #{name} pointing to missing #{target} file. Ignoring" | ||
| end | ||
|
|
@@ -721,6 +721,21 @@ def limit_read(io, name, limit) | |
| raise Gem::Package::FormatError, "#{name} is too big (over #{limit} bytes)" if bytes.size > limit | ||
| bytes | ||
| end | ||
|
|
||
| if Gem.win_platform? | ||
| # Create a symlink and fallback to copy the file or directory on Windows, | ||
| # where symlink creation needs special privileges in form of the Developer Mode. | ||
| def create_symlink(old_name, new_name) | ||
| File.symlink(old_name, new_name) | ||
| rescue Errno::EACCES | ||
| from = File.expand_path(old_name, File.dirname(new_name)) | ||
| FileUtils.cp_r(from, new_name) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we be worried about gems that include a symlink where the target is a system path (either maliciously or by mistake) and we'd end up coping recursively its whole content?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| end | ||
| else | ||
| def create_symlink(old_name, new_name) | ||
| File.symlink(old_name, new_name) | ||
| end | ||
| end | ||
| end | ||
|
|
||
| require_relative "package/digest_io" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -175,6 +175,9 @@ def test_add_files | |
| end | ||
|
|
||
| def test_add_files_symlink | ||
| unless symlink_supported? | ||
| omit("symlink - developer mode must be enabled on Windows") | ||
| end | ||
| spec = Gem::Specification.new | ||
| spec.files = %w[lib/code.rb lib/code_sym.rb lib/code_sym2.rb] | ||
|
|
||
|
|
@@ -185,16 +188,8 @@ def test_add_files_symlink | |
| end | ||
|
|
||
| # NOTE: 'code.rb' is correct, because it's relative to lib/code_sym.rb | ||
| begin | ||
| File.symlink("code.rb", "lib/code_sym.rb") | ||
| File.symlink("../lib/code.rb", "lib/code_sym2.rb") | ||
| rescue Errno::EACCES => e | ||
| if Gem.win_platform? | ||
| pend "symlink - must be admin with no UAC on Windows" | ||
| else | ||
| raise e | ||
| end | ||
| end | ||
| File.symlink("code.rb", "lib/code_sym.rb") | ||
| File.symlink("../lib/code.rb", "lib/code_sym2.rb") | ||
|
|
||
| package = Gem::Package.new "bogus.gem" | ||
| package.spec = spec | ||
|
|
@@ -583,25 +578,45 @@ def test_extract_tar_gz_symlink_relative_path | |
| tar.add_symlink "lib/foo.rb", "../relative.rb", 0o644 | ||
| end | ||
|
|
||
| begin | ||
| package.extract_tar_gz tgz_io, @destination | ||
| rescue Errno::EACCES => e | ||
| if Gem.win_platform? | ||
| pend "symlink - must be admin with no UAC on Windows" | ||
| else | ||
| raise e | ||
| end | ||
| end | ||
| package.extract_tar_gz tgz_io, @destination | ||
|
|
||
| extracted = File.join @destination, "lib/foo.rb" | ||
| assert_path_exist extracted | ||
| assert_equal "../relative.rb", | ||
| File.readlink(extracted) | ||
| if symlink_supported? | ||
| assert_equal "../relative.rb", | ||
| File.readlink(extracted) | ||
| end | ||
| assert_equal "hi", | ||
| File.read(extracted), | ||
| "should read file content either by following symlink or on Windows by reading copy" | ||
| end | ||
|
|
||
| def test_extract_tar_gz_symlink_directory | ||
| package = Gem::Package.new @gem | ||
| package.verify | ||
|
|
||
| tgz_io = util_tar_gz do |tar| | ||
| tar.add_symlink "link", "lib/orig", 0o644 | ||
| tar.mkdir "lib", 0o755 | ||
| tar.mkdir "lib/orig", 0o755 | ||
| tar.add_file "lib/orig/file.rb", 0o644 do |io| | ||
| io.write "ok" | ||
| end | ||
| end | ||
|
|
||
| package.extract_tar_gz tgz_io, @destination | ||
| extracted = File.join @destination, "link/file.rb" | ||
| assert_path_exist extracted | ||
| if symlink_supported? | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small nitpick, maybe it would be easier without this condition and just check that reading the file works and the content is "ok", regardless if it's a symlink or not
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is a valid addition: If symlinks are supported then a symlink should actually be created. So I wouldn't remove it. |
||
| assert_equal "lib/orig", | ||
| File.readlink(File.dirname(extracted)) | ||
| end | ||
| assert_equal "ok", | ||
| File.read(extracted) | ||
| end | ||
|
|
||
| def test_extract_symlink_into_symlink_dir | ||
| omit "Symlinks not supported or not enabled" unless symlink_supported? | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this test be skipped? Since we now support extracting symlinks even is symlink is not supported
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test fails if |
||
| package = Gem::Package.new @gem | ||
| tgz_io = util_tar_gz do |tar| | ||
| tar.mkdir "lib", 0o755 | ||
|
|
@@ -665,14 +680,10 @@ def test_extract_symlink_parent | |
| destination_subdir = File.join @destination, "subdir" | ||
| FileUtils.mkdir_p destination_subdir | ||
|
|
||
| expected_exceptions = Gem.win_platform? ? [Gem::Package::SymlinkError, Errno::EACCES] : [Gem::Package::SymlinkError] | ||
|
|
||
| e = assert_raise(*expected_exceptions) do | ||
| e = assert_raise(Gem::Package::SymlinkError) do | ||
| package.extract_tar_gz tgz_io, destination_subdir | ||
| end | ||
|
|
||
| pend "symlink - must be admin with no UAC on Windows" if Errno::EACCES === e | ||
|
|
||
| assert_equal("installing symlink 'lib/link' pointing to parent path #{@destination} of " \ | ||
| "#{destination_subdir} is not allowed", e.message) | ||
|
|
||
|
|
@@ -700,14 +711,10 @@ def test_extract_symlink_parent_doesnt_delete_user_dir | |
| tar.add_symlink "link/dir", ".", 16_877 | ||
| end | ||
|
|
||
| expected_exceptions = Gem.win_platform? ? [Gem::Package::SymlinkError, Errno::EACCES] : [Gem::Package::SymlinkError] | ||
|
|
||
| e = assert_raise(*expected_exceptions) do | ||
| e = assert_raise(Gem::Package::SymlinkError) do | ||
| package.extract_tar_gz tgz_io, destination_subdir | ||
| end | ||
|
|
||
| pend "symlink - must be admin with no UAC on Windows" if Errno::EACCES === e | ||
|
|
||
| assert_equal("installing symlink 'link' pointing to parent path #{destination_user_dir} of " \ | ||
| "#{destination_subdir} is not allowed", e.message) | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.