Zip::FileSystem::ZipFsFile#symlink? always returns false

[fbacall]

even if the entry for the given filepath is actually a symlink

https://github.com/rubyzip/rubyzip/blob/master/lib/zip/filesystem/file.rb#L200-L202

[fbacall]

I wonder if the behaviour of ftype should also be changed?

https://github.com/rubyzip/rubyzip/blob/master/lib/zip/filesystem/file.rb#L208-L210

The ftype property of a Zip::Entry

rubyzip/lib/zip/entry.rb

Lines 379 to 407 in 044759f

	def set_ftype_from_c_dir_entry
	@ftype = case @fstype
	when ::Zip::FSTYPE_UNIX
	@unix_perms = (@external_file_attributes >> 16) & 0o7777
	case (@external_file_attributes >> 28)
	when ::Zip::FILE_TYPE_DIR
	:directory
	when ::Zip::FILE_TYPE_FILE
	:file
	when ::Zip::FILE_TYPE_SYMLINK
	:symlink
	else
	# Best case guess for whether it is a file or not.
	# Otherwise this would be set to unknown and that
	# entry would never be able to be extracted.
	if name_is_directory?
	:directory
	else
	:file
	end
	end
	else
	if name_is_directory?
	:directory
	else
	:file
	end
	end
	end

can return :symlink (as a symbol)

But on the other hand, I wonder if this method is supposed to behave more like Ruby's ftype on File.stat which returns a link (as a string): https://ruby-doc.org/core-2.5.0/File/Stat.html#method-i-ftype

🤔

[hainesr]

That would seem sensible. Did File.stat change at any point, do you know?

I suppose if we were to do this then now would be the time, with version 3.0 coming up (at some point 😬 ).

[hainesr]

Having now looked this up, ftype has been returning strings since at least 1.9.3...

[fbacall]

I went down a bit of a rabbit hole looking into this.

There are quite a few other methods that could be implemented to better handle symlinks, such as stat lstat readlink. But I believe there are some path-traversal vulnerabilities around symlinks that have been addressed in the past, and I don't really want to open up that can of worms again.

[hainesr]

I agree: let's not open the symlinks can of worms right now - although I'll have to get to it at some point 😢

Do you think we should change ftype to return strings though, to match File.stat? I'm leaning towards yes. I also note that there are side effects in set_ftype_from_c_dir_entry that I don't much care for (c.f. setting @unix_perms)...

[fbacall]

It makes sense to me, but I'm not sure how much of a breaking change that would be. As well as changing to strings, it would also need to return link instead of symlink for symlinks.

[hainesr]

It would be a breaking change, but now is the time for those because next release is v3.0.

I'll see about doing this soon.

[hainesr]

Closing this issue and transferring the subsequent conversation to #532.