require_relative should never be used with bazel because it reads files from the source tree instead of the sandbox runfiles tree

[noahkawasakigoogle]

I've discovered that the use of require_relative vs require actually reads files from the source tree instead of the files in the bazel sandbox runfiles tree.

Repro: https://github.com/bazel-contrib/rules_ruby/pull/224/files

In this example, you can remove the :spec_helper library from the :add test target and it will still pass even though spec_helper.rb doesnt exist in any of the runfiles. This is because require_relative loads the file directly from the source tree (your local repo) and bypasses Bazel sandbox.

If you change require_relative to require, it will fail to find the spec_helper.rb file until you add back :spec_helper as a dependency.

require always searches files from $LOAD_PATH, and in bazel rules_ruby, the deps, srcs, and bundle repo gems are always on in the list of dirs in $LOAD_PATH.

I think it might be good to add a require_relative static analysis check in rb_library to fail any rb file that has this function used in rules_ruby to avoid this silent/unintentional behavior. The result is that your bazel targets may be unknowingly using files that are not declared as dependencies. Thoughts?

[p0deje]

Ugh, I was not expecting that, but it makes sense judging by the source code of Kernel#require_relative (https://docs.ruby-lang.org/en/3.4/Kernel.html#method-i-require_relative). This is similar to aspect-build/rules_js#362 I guess.

We could solve this by monkey-patching require_relative and first checking that the file exists within the sandboxed tree, then calling the original implementation. Like any other monkey-patch, it might open a can of worms, though.

[noahkawasakigoogle]

Thats true, I think a WARNING log if the target doesnt have local = True during rb_library might be good enough. For local runs it might be fine.

I also found that using require and absolute paths to all ruby files kind of forced me to refactor all require statements to be much more readable and less vague. Kind of like how Java/Kotlin imports are always fully qualified, it just results in more clear code and no guessing where something came from.

[pauldanielm]

There are a few other things in Ruby such as this one that cause similar problems.

• Using __dir__ will use realpaths instead of the symlink path. The common pattern of using require with __dir__ will require from the source file tree instead of the sandbox runfiles tree. Example in activesupport: https://github.com/rails/rails/blob/8bf7a3d4d5760da094031681d354a4e148c2ded6/activesupport/lib/active_support/core_ext.rb#L3-L5
• Rails.root will use the realpath, so any references to Rails.root in a bazel symlink sandbox will be in the source tree instead. This is also a common pattern, e.g. https://github.com/csexton/angular-rails-presentation/blob/067c1401e6bbed7924e287e1da2f7b6f71e88eca/BOWER.md?plain=1#L29

[p0deje]

I think it's worth experimenting with monkey-patches for all these methods to prevent escaping a sandbox. Something like this could be a start, I just tested it and it works on simple scenarios:

# test.rb
module Kernel
  module_function def __dir__
    File.dirname(File.expand_path(__FILE__))
  end
end

puts __dir__

❱ mkdir 123
❱ ln -s (pwd)/test.rb (pwd)/123/test.rb
❱ ll 123
total 0
lrwxr-xr-x  1 p0deje  staff    21B Jul 26 08:36 test.rb@ -> /Users/p0deje/test.rb
❱ ruby test.rb
/Users/p0deje
❱ ruby 123/test.rb
/Users/p0deje/123

[pauldanielm]

For some reason the Kernel patch @p0deje provides does not work in my case. The problem is that __FILE__ always seems to resolve to the file that the patch is defined in, rather than the file calling __dir__. I am using MRI ruby, which might behave differently. Example:

Using:

# test.rb 
module Kernel
  module_function def __dir__
    File.dirname(File.expand_path(__FILE__))
  end
end

and using load with FILE to avoid the require_relative issue causing more confusion:

<pwd> > echo "load File.expand_path('../test.rb', File.dirname(__FILE__)); puts __dir__" > print_dir.rb
<pwd> > mkdir sandbox
<pwd> > mkdir sandbox/subdirectory
<pwd> > ln -s $(pwd)/test.rb $(pwd)/sandbox/test.rb
<pwd> > ln -s $(pwd)/print_dir.rb $(pwd)/sandbox/subdirectory/print_dir.rb
<pwd> > ruby sandbox/subdirectory/print_dir.rb
(pwd)/sandbox

We can see that (pwd)/sandbox is printed, instead of the expected (pwd)/sandbox/subdirectory.

We ended up with the following patches for require_relative and __dir__ which solve a large number of issues with Ruby's handling of symlink sandboxes for our use case.

define_method(:require_relative) do |path|
  base = caller_locations[0]&.path
  base = "" if base == nil
  base_directory = File.dirname(base)
  require File.expand_path(path, base_directory)
end

and

def __dir__
  File.dirname(caller_locations[0]&.path)
end

I am interested to hear what others think of the above patches and if there is a way to improve them in any way.

[noahkawasakigoogle]

I found that patching require_relative was unnecessary (but probably a good thing to figure out regardless) if you just rewrite all of your ruby code to use require with a fully qualified path to the ruby file by putting the root of your repo in the LOAD_PATH.

In #223 I showed this with -I. which bazel runs by including the root of your repo.

    rb_test(
        srcs = test_files,
        args = [
            "-I.",
            "-e",
            "\"ARGV.each { |f| require f }\"",
        ] + test_files_full_path,
        **kwargs
    )

The benefit here was that there were no ambiguous require relatives where you have to search relative directories to find the imported file like require_relative '../somefile' but rather very explicit code such as require 'path/to/somefile'. Not saying this is any better or worse but its a pretty easy workaround and possibly leads to cleaner code.

All the gems from the @bundle repo rules_ruby provided all worked (and many of them definitely use require_relative) require_relative wasnt necessary for those either. I believe since the gems and their versions are already managed as bazel repo resources means the require_relative behavior here doesnt apply to external gems, but only to your actual source code