[DOC] Enhanced RDoc for FileUtils #78

BurdetteLamar · 2022-05-30T18:02:22Z

Treats:

::rm
::rm_f
::rm_r
::rm_rf
::remove_entry_secure

BurdetteLamar · 2022-06-01T15:26:32Z

@peterzhu2118, as you see, @jeremyevans and I have been working out the ways for this module's documentation. Eventually I will revisit the first ones I did, in order to make them conformant with the later ones.

lib/fileutils.rb

jeremyevans · 2022-06-01T19:39:26Z

lib/fileutils.rb

@@ -949,13 +951,27 @@ def mv(src, dest, force: nil, noop: nil, verbose: nil, secure: nil)
  alias move mv
  module_function :move

+  # Removes entries at the paths given in array +list+; returns +list+.


list is not necessarily an array, a string is accepted. However, an array is always return. The sentence should be updated to reflect that. Similar for rm_r.

lib/fileutils.rb

deivid-rodriguez · 2022-06-01T21:38:21Z

lib/fileutils.rb

-  # Remove file(s) specified in +list+.  This method cannot remove directories.
-  # All StandardErrors are ignored when the :force option is set.
+  # - <tt>force: true</tt> - attempts to remove files regardless of permissions;
+  #   ignores raised exceptions of StandardError and its descendants.


The previous description

All StandardErrors are ignored when the :force option is set.

is actually more descriptive of the current behavior. Currently the only difference between force: true and force: false is that force: true makes any StandardErrors while trying to remove files be ignored. But if a file is impossible to remove due to permissions, the removal will be attempted regardless of force, and the files won't get removed regardless of force.

For what it's worth, I think this behaviour is harmful, I opened a ruby-core ticket at https://bugs.ruby-lang.org/issues/18784. But unless the behavior is changed, the documentation should explain the current behavior.

deivid-rodriguez · 2022-06-01T21:38:57Z

lib/fileutils.rb

+  # Keyword arguments:
+  #
+  # - <tt>force: true</tt> - attempts to remove entries regardless of permissions;
+  #   ignores raised exceptions of StandardError and its descendants.


Ditto regarding force.

Also fixed.

It doesn't seem fixed, no? The sentence about permissions is still there. You did remove some stuff regarding force in other methods that take care of moving files, but I'm not sure how force behaves there, so I'm not sure if that's correct or not.

peterzhu2118 · 2022-06-01T21:47:41Z

lib/fileutils.rb

+  # May cause a local vulnerability if not called with keyword argument
+  # <tt>secure: true</tt>.


Can you add a link to the place that explains this vulnerability?

I'll be reworking all about secure and vulnerabilities, putting it into the doc for the module, and adding links as needed.

peterzhu2118 · 2022-06-01T21:49:46Z

lib/fileutils.rb

+  #   {chown(2)}[https://man7.org/linux/man-pages/man2/chown.2.html]
+  #   and {chmod(2)}[https://man7.org/linux/man-pages/man2/chmod.2.html]
+  #   in removing directories.
+  # - The owner of +path+ should be either the current proces


Typo:

Suggested change

# - The owner of +path+ should be either the current proces

# - The owner of +path+ should be either the current process

BurdetteLamar · 2022-06-01T21:57:36Z

@jeremyevans and @peterzhu2118, I'm thinking that the vulnerability/security business should be explained in the medule header so that all can link to it directly.

Also, does this vulnerability exist for methods other than those here in FileUtils?

jeremyevans · 2022-06-01T22:05:56Z

I'm not opposed to moving the warning to the module documentation and linking to it, but that needs to be done in this PR if you want to do that.

I'm not sure if this affects other methods. It should only affect recursive methods if it does.

BurdetteLamar · 2022-06-01T22:10:10Z

I'm not opposed to moving the warning to the module documentation and linking to it, but that needs to be done in this PR if you want to do that.

Understood and agreed. Will work on it.

deivid-rodriguez · 2022-06-02T13:32:36Z

lib/fileutils.rb

+  # - <tt>force: true</tt> - attempts to remove files regardless of permissions;
+  #   ignores raised exceptions of StandardError and its descendants.


I think it was not too clear what I'm suggesting. It's this:

Suggested change

# - <tt>force: true</tt> - attempts to remove files regardless of permissions;

# ignores raised exceptions of StandardError and its descendants.

# - <tt>force: true</tt> - ignores raised exceptions of StandardError and its descendants.

BurdetteLamar · 2022-06-02T20:29:14Z

@jeremyevans and @peterzhu2118, I'll be reworking all about secure and vulnerabilities, putting it into the doc for the module, and adding links as needed. So review of such text here should be deferred.

lib/fileutils.rb

BurdetteLamar · 2022-06-02T20:40:46Z

@jeremyevans, I only meant that you might like to review the other fixes; but fine, I'll ping you when all's ready.

BurdetteLamar · 2022-06-03T21:01:18Z

I have added section "Avoiding the TOCTTOU Vulnerability," which is linked from the relevant methods doc.

jeremyevans

Only a few small changes requested. I'm OK to merge after the changes are made, but please allow a day or two for @peterzhu2118 and @deivid-rodriguez to review and provide feedback.

jeremyevans · 2022-06-03T22:38:25Z

lib/fileutils.rb

+# if the source and destination are on different devices
+# (which means that the "move" is really a copy and remove):
+#
+# - FileUtils.mv  with keyword argument <tt>secure: true</tt>.


Extra space here.

jeremyevans · 2022-06-03T22:38:43Z

lib/fileutils.rb

+#
+# - FileUtils.mv  with keyword argument <tt>secure: true</tt>.
+#
+# \Method \FileUtils.remove_entry_secure remove securely


s/remove/removes/

jeremyevans · 2022-06-03T22:39:08Z

lib/fileutils.rb

+# WARNING: You must ensure that *ALL* parent directories cannot be
+# moved by other untrusted users.  For example, parent directories
+# should not be owned by untrusted users, and should not be world
+# writable except when the sticky bit set.


s/set/is set/

jeremyevans · 2022-06-03T22:40:04Z

lib/fileutils.rb

+# should not be owned by untrusted users, and should not be world
+# writable except when the sticky bit set.
+#
+# WARNING: Only the owner of the removing directory tree, or Unix super


I think this paragraph can be removed, since it's mentioned in the preceding bullet point.

olleolleolle · 2022-06-04T18:07:16Z

lib/fileutils.rb

@@ -130,9 +130,9 @@
 # if the source and destination are on different devices
 # (which means that the "move" is really a copy and remove):
 #
-# - FileUtils.mv  with keyword argument <tt>secure: true</tt>.
+# - FileUtils.mv `with keyword argument <tt>secure: true</tt>.


Guessing this backtick is a typo.

Fixed, and thanks.

peterzhu2118

Looks good!

BurdetteLamar · 2022-06-05T14:46:15Z

@deivid-rodriguez, over to you.

deivid-rodriguez · 2022-06-05T17:03:21Z

lib/fileutils.rb

-  #   ignores raised exceptions of StandardError and its descendants.
+  # Keyword arguments:
+  #
+  # - <tt>force: true</tt> - ignores raised exceptions of StandardError


My understanding of the previous documentation is that in this case, the :force flag only affects cases where copying and then removing the original source is needed, and in this case it would apply to the removal phase only. If this is correct, I think the previous documentation is more accurate, because it doesn't imply that raised exceptions during the copy phase are swallowed.

What should this say, then?

I would say what it says now, because I'm not sure whether your change improves things or not. Except thet I'd probably remove the first sentence "attempts to force the move", because in my mind it's quite unclear what that means. So I'd go with:

# - <tt>force: true</tt> - if the move includes removing +src+ (that is, if +src+ and +dest+ are on different devices), ignores raised exceptions of StandardError and its descendants.

I've made it so.

deivid-rodriguez

The force documentation should be ok now I think, thanks! :). If I ever succeed on changing the behavior I will update the docs too 💪

BurdetteLamar · 2022-06-06T15:28:38Z

@jeremyevans I've pinged you once more b/c @deivid-rodriguez requested a change that I've made since you approved. Is this now good to go?

jeremyevans

New language looks fine.

Treats: ::rm ::rm_f ::rm_r ::rm_rf ::remove_entry_secure ruby/fileutils@ce2a438d75

Enhanced RDoc for FileUtils

4c67c3e

BurdetteLamar requested a review from jeremyevans May 30, 2022 18:02

BurdetteLamar added the documentation label May 30, 2022

BurdetteLamar requested a review from peterzhu2118 June 1, 2022 15:24

jeremyevans requested changes Jun 1, 2022

View reviewed changes

BurdetteLamar added 2 commits June 1, 2022 13:54

Enhanced RDoc

65f22f4

Enhanced RDoc

8e653bd

BurdetteLamar requested a review from jeremyevans June 1, 2022 21:01

jeremyevans requested changes Jun 1, 2022

View reviewed changes

lib/fileutils.rb Outdated Show resolved Hide resolved

lib/fileutils.rb Outdated Show resolved Hide resolved

deivid-rodriguez reviewed Jun 1, 2022

View reviewed changes

peterzhu2118 reviewed Jun 1, 2022

View reviewed changes

deivid-rodriguez reviewed Jun 2, 2022

View reviewed changes

BurdetteLamar added 2 commits June 2, 2022 13:04

Enhanced RDoc

71672b4

Enhanced RDoc

fabcf44

BurdetteLamar requested review from peterzhu2118 and jeremyevans June 2, 2022 20:29

jeremyevans requested changes Jun 2, 2022

View reviewed changes

lib/fileutils.rb Outdated Show resolved Hide resolved

Enhanced RDoc

cec2e7b

BurdetteLamar requested a review from jeremyevans June 3, 2022 21:01

jeremyevans requested changes Jun 3, 2022

View reviewed changes

Enhanced RDoc

4a9a0f8

olleolleolle reviewed Jun 4, 2022

View reviewed changes

Enhanced RDoc

d7fd008

peterzhu2118 approved these changes Jun 5, 2022

View reviewed changes

deivid-rodriguez reviewed Jun 5, 2022

View reviewed changes

Enhanced RDoc

5163525

BurdetteLamar requested review from jeremyevans and peterzhu2118 June 5, 2022 17:32

deivid-rodriguez reviewed Jun 5, 2022

View reviewed changes

jeremyevans approved these changes Jun 6, 2022

View reviewed changes

BurdetteLamar merged commit ce2a438 into ruby:master Jun 6, 2022

matzbot pushed a commit to ruby/ruby that referenced this pull request Jun 6, 2022

[ruby/fileutils] [DOC] Enhanced RDoc for FileUtils (ruby/fileutils#78)

b737998

Treats: ::rm ::rm_f ::rm_r ::rm_rf ::remove_entry_secure ruby/fileutils@ce2a438d75

		# May cause a local vulnerability if not called with keyword argument
		# <tt>secure: true</tt>.

	# - The owner of +path+ should be either the current proces
	# - The owner of +path+ should be either the current process

		# - <tt>force: true</tt> - attempts to remove files regardless of permissions;
		# ignores raised exceptions of StandardError and its descendants.

	# - <tt>force: true</tt> - attempts to remove files regardless of permissions;
	# ignores raised exceptions of StandardError and its descendants.
	# - <tt>force: true</tt> - ignores raised exceptions of StandardError and its descendants.

[DOC] Enhanced RDoc for FileUtils #78

[DOC] Enhanced RDoc for FileUtils #78

Conversation

BurdetteLamar commented May 30, 2022

BurdetteLamar commented Jun 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BurdetteLamar commented Jun 1, 2022

jeremyevans commented Jun 1, 2022

BurdetteLamar commented Jun 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BurdetteLamar commented Jun 2, 2022

BurdetteLamar commented Jun 2, 2022

BurdetteLamar commented Jun 3, 2022

jeremyevans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterzhu2118 left a comment

Choose a reason for hiding this comment

BurdetteLamar commented Jun 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deivid-rodriguez left a comment

Choose a reason for hiding this comment

BurdetteLamar commented Jun 6, 2022

jeremyevans left a comment

Choose a reason for hiding this comment