Escape URIs like the editor does and harden Dir.glob against metacharacters in paths

lib/ruby_indexer/lib/ruby_indexer/uri.rb

@@ -15,10 +15,14 @@ class Generic
     class << self
       #: (path: String, ?fragment: String?, ?scheme: String, ?load_path_entry: String?) -> URI::Generic
       def from_path(path:, fragment: nil, scheme: "file", load_path_entry: nil)
-        # This unsafe regex is the same one used in the URI::RFC2396_REGEXP class with the exception of the fact that we
-        # do not include colon as a safe character. VS Code URIs always escape colons and we need to ensure we do the
-        # same to avoid inconsistencies in our URIs, which are used to identify resources
-        unsafe_regex = %r{[^\-_.!~*'()a-zA-Z\d;/?@&=+$,\[\]]}
+        # Our URIs are used to identify resources, so they must be escaped exactly the way the editor escapes them.
+        # Otherwise the same file ends up tracked under two different URIs: one built here during indexing and one
+        # received from the editor, which results in duplicate index entries (see Shopify/ruby-lsp#3639).
+        #
+        # VS Code's `vscode-uri` only leaves the RFC 3986 unreserved characters unescaped (`A-Za-z0-9-._~`), plus the
+        # forward slash in paths. Everything else, including characters that RFC 2396 considers safe such as `(`, `)`,
+        # `!`, `'`, `*`, `$`, `&`, `+`, `,`, `;`, `=`, `@`, `[`, `]` and `:`, is percent-encoded
+        unsafe_regex = %r{[^\-_.~a-zA-Z\d/]}
 
         # On Windows, if the path begins with the disk name, we need to add a leading slash to make it a valid URI
         escaped_path = if /^[A-Z]:/i.match?(path)

lib/ruby_indexer/test/uri_test.rb

@@ -81,5 +81,100 @@ def test_from_path_with_unicode_characters
       assert_equal(path, uri.to_standardized_path)
       assert_equal("file:///path/with/unicode/%E6%96%87%E4%BB%B6.rb", uri.to_s)
     end
+
+    def test_from_path_with_brackets
+      uri = URI::Generic.from_path(path: "/some/path/[id].rb")
+      assert_equal("file:///some/path/%5Bid%5D.rb", uri.to_s)
+    end
+
+    def test_from_path_with_braces
+      uri = URI::Generic.from_path(path: "/some/path/{slug}.rb")
+      assert_equal("file:///some/path/%7Bslug%7D.rb", uri.to_s)
+    end
+
+    def test_round_trip_with_brackets
+      path = "/some/path/[id].rb"
+      uri = URI::Generic.from_path(path: path)
+      assert_equal(path, uri.to_standardized_path)
+    end
+
+    def test_round_trip_with_braces
+      path = "/some/path/{slug}.rb"
+      uri = URI::Generic.from_path(path: path)
+      assert_equal(path, uri.to_standardized_path)
+    end
+
+    def test_from_path_with_parentheses
+      uri = URI::Generic.from_path(path: "/some/path/(id).rb")
+      assert_equal("file:///some/path/%28id%29.rb", uri.to_s)
+    end
+
+    def test_round_trip_with_parentheses
+      path = "/some/path/(id).rb"
+      uri = URI::Generic.from_path(path: path)
+      assert_equal(path, uri.to_standardized_path)
+    end
+
+    def test_round_trip_with_spaces_inside_brackets
+      path = "/some/path/[id page].rb"
+      uri = URI::Generic.from_path(path: path)
+      assert_equal(path, uri.to_standardized_path)
+      assert_equal("file:///some/path/%5Bid%20page%5D.rb", uri.to_s)
+    end
+
+    def test_round_trip_with_spaces_inside_braces
+      path = "/some/path/{slug name}.rb"
+      uri = URI::Generic.from_path(path: path)
+      assert_equal(path, uri.to_standardized_path)
+      assert_equal("file:///some/path/%7Bslug%20name%7D.rb", uri.to_s)
+    end
+
+    def test_round_trip_with_spaces_inside_parentheses
+      path = "/some/path/file (copy).rb"
+      uri = URI::Generic.from_path(path: path)
+      assert_equal(path, uri.to_standardized_path)
+      assert_equal("file:///some/path/file%20%28copy%29.rb", uri.to_s)
+    end
+
+    # The editor and the server must escape paths identically, otherwise the same file is tracked under two different
+    # URIs and the index ends up with duplicate entries (Shopify/ruby-lsp#3639). These expectations were captured from
+    # the `vscode-uri` package, which is what VS Code uses to build the URIs it sends us
+    def test_from_path_escapes_every_character_the_editor_escapes
+      {
+        "/some/path/[id].rb" => "file:///some/path/%5Bid%5D.rb",
+        "/some/path/{slug}.rb" => "file:///some/path/%7Bslug%7D.rb",
+        "/some/path/(id).rb" => "file:///some/path/%28id%29.rb",
+        "/some/path/a&b.rb" => "file:///some/path/a%26b.rb",
+        "/some/path/a,b.rb" => "file:///some/path/a%2Cb.rb",
+        "/some/path/a+b.rb" => "file:///some/path/a%2Bb.rb",
+        "/some/path/a=b.rb" => "file:///some/path/a%3Db.rb",
+        "/some/path/a@b.rb" => "file:///some/path/a%40b.rb",
+        "/some/path/a;b.rb" => "file:///some/path/a%3Bb.rb",
+        "/some/path/a!b.rb" => "file:///some/path/a%21b.rb",
+        "/some/path/a*b.rb" => "file:///some/path/a%2Ab.rb",
+        "/some/path/a$b.rb" => "file:///some/path/a%24b.rb",
+        "/some/path/it's.rb" => "file:///some/path/it%27s.rb",
+        "/some/path/with space.rb" => "file:///some/path/with%20space.rb",
+      }.each do |path, expected|
+        uri = URI::Generic.from_path(path: path)
+        assert_equal(expected, uri.to_s)
+        assert_equal(path, uri.to_standardized_path)
+      end
+    end
+
+    def test_from_path_does_not_escape_unreserved_characters
+      path = "/some/path/a-b_c.d~e.rb"
+      uri = URI::Generic.from_path(path: path)
+      assert_equal("file:///some/path/a-b_c.d~e.rb", uri.to_s)
+      assert_equal(path, uri.to_standardized_path)
+    end
+
+    def test_from_path_with_question_mark
+      # A question mark used to be treated as safe, which made `URI::Generic.build` reject the escaped path for not
+      # being a valid absolute path component
+      uri = URI::Generic.from_path(path: "/some/path/what?.rb")
+      assert_equal("file:///some/path/what%3F.rb", uri.to_s)
+      assert_equal("/some/path/what?.rb", uri.to_standardized_path)
+    end
   end
 end

lib/ruby_lsp/addon.rb

@@ -62,7 +62,9 @@ def load_addons(global_state, outgoing_queue, include_project_addons: true)
             nil
           end
 
-          project_addons = Dir.glob("#{global_state.workspace_path}/**/ruby_lsp/**/addon.rb")
+          project_addons = Dir.glob("**/ruby_lsp/**/addon.rb", base: global_state.workspace_path).map! do |relative_path|
+            File.join(global_state.workspace_path, relative_path)
+          end
           project_addons.reject! do |path|
             (bundle_path && path.start_with?(bundle_path)) || gem_installation_path?(path)
           end

lib/ruby_lsp/listeners/completion.rb

@@ -457,10 +457,11 @@ def complete_require_relative(node)
         # if the path is not a directory, glob all possible next characters
         # for example ../somethi| (where | is the cursor position)
         # should find files for ../somethi*/
+        escaped_content = RubyLsp.escape_glob_metacharacters(content)
         path_query = if content.end_with?("/") || content.empty?
-          "#{content}**/*.rb"
+          "#{escaped_content}**/*.rb"
         else
-          "{#{content}*/**/*.rb,**/#{content}*.rb}"
+          "{#{escaped_content}*/**/*.rb,**/#{escaped_content}*.rb}"
         end
 
         Dir.glob(path_query, File::FNM_PATHNAME | File::FNM_EXTGLOB, base: origin_dir).sort!.each do |path|

lib/ruby_lsp/listeners/test_style.rb

@@ -35,9 +35,10 @@ def resolve_test_commands(items)
               if children.empty?
                 full_files.concat(
                   Dir.glob(
-                    "#{path}/**/{*_test,test_*,*_spec}.rb",
+                    "**/{*_test,test_*,*_spec}.rb",
                     File::Constants::FNM_EXTGLOB | File::Constants::FNM_PATHNAME,
-                  ).map! { |p| Shellwords.escape(p) },
+                    base: path,
+                  ).map! { |p| Shellwords.escape(File.join(path, p)) },
                 )
               end
             elsif tags.include?("test_file")

lib/ruby_lsp/requests/go_to_relevant_file.rb

@@ -36,9 +36,10 @@ def perform
       #: -> Array[String]
       def find_relevant_paths
         patterns = relevant_filename_patterns
+        root = search_root
 
         candidate_paths = patterns.flat_map do |pattern|
-          Dir.glob(File.join(search_root, "**", pattern))
+          Dir.glob(File.join("**", pattern), base: root).map! { |p| File.join(root, p) }
         end
 
         return [] if candidate_paths.empty?
@@ -89,22 +90,25 @@ def relevant_filename_patterns
           # Test file -> find implementation
           base = input_basename.gsub(TEST_PATTERN, "")
           parent_dir = File.basename(File.dirname(@path))
+          escaped_base = RubyLsp.escape_glob_metacharacters(base)
+          escaped_parent_dir = RubyLsp.escape_glob_metacharacters(parent_dir)
 
           # If test file is in a directory matching the implementation name
           # (e.g., go_to_relevant_file/test_go_to_relevant_file_a.rb)
           # return patterns for both the base file name and the parent directory name
           if base.include?(parent_dir) && base != parent_dir
-            ["#{base}#{extension}", "#{parent_dir}#{extension}"]
+            ["#{escaped_base}#{extension}", "#{escaped_parent_dir}#{extension}"]
           else
-            ["#{base}#{extension}"]
+            ["#{escaped_base}#{extension}"]
           end
         else
           # Implementation file -> find tests (including in matching directory)
+          escaped_basename = RubyLsp.escape_glob_metacharacters(input_basename)
           [
-            "{#{TEST_PREFIX_GLOB}}#{input_basename}#{extension}",
-            "#{input_basename}{#{TEST_SUFFIX_GLOB}}#{extension}",
-            "#{input_basename}/{#{TEST_PREFIX_GLOB}}*#{extension}",
-            "#{input_basename}/*{#{TEST_SUFFIX_GLOB}}#{extension}",
+            "{#{TEST_PREFIX_GLOB}}#{escaped_basename}#{extension}",
+            "#{escaped_basename}{#{TEST_SUFFIX_GLOB}}#{extension}",
+            "#{escaped_basename}/{#{TEST_PREFIX_GLOB}}*#{extension}",
+            "#{escaped_basename}/*{#{TEST_SUFFIX_GLOB}}#{extension}",
           ]
         end
       end

lib/ruby_lsp/requests/references.rb

@@ -60,7 +60,8 @@ def perform
         reference_target = create_reference_target(target, node_context)
         return @locations unless reference_target
 
-        Dir.glob(File.join(@global_state.workspace_path, "**/*.rb")).each do |path|
+        Dir.glob("**/*.rb", base: @global_state.workspace_path).each do |relative_path|
+          path = File.join(@global_state.workspace_path, relative_path)
           uri = URI::Generic.from_path(path: path)
           # If the document is being managed by the client, then we should use whatever is present in the store instead
           # of reading from disk

lib/ruby_lsp/requests/rename.rb

@@ -129,7 +129,8 @@ def collect_file_renames(fully_qualified_name, document_changes)
       def collect_text_edits(target, name)
         changes = {}
 
-        Dir.glob(File.join(@global_state.workspace_path, "**/*.rb")).each do |path|
+        Dir.glob("**/*.rb", base: @global_state.workspace_path).each do |relative_path|
+          path = File.join(@global_state.workspace_path, relative_path)
           uri = URI::Generic.from_path(path: path)
           # If the document is being managed by the client, then we should use whatever is present in the store instead
           # of reading from disk

lib/ruby_lsp/utils.rb

@@ -21,6 +21,21 @@ module RubyLsp
   GUESSED_TYPES_URL = "https://shopify.github.io/ruby-lsp/#guessed-types"
   TEST_PATH_PATTERN = "**/{test,spec,features}/**/{*_test.rb,test_*.rb,*_spec.rb,*.feature}"
 
+  class << self
+    # Escape characters that have special meaning in Dir.glob patterns. The comma is included because callers may
+    # interpolate the result into a brace group, where an unescaped comma would split the alternation
+    #
+    # This only takes effect on POSIX. `Dir.glob` does not support backslash escaping on Windows, where the backslash
+    # is a path separator, so there a path containing metacharacters still fails to match. Nothing regresses because
+    # of it, since the substitution is a no-op for the metacharacter-free paths that are the common case. Prefer
+    # `Dir.glob`'s `base:` parameter whenever the value is a path rather than a fragment interpolated into a pattern,
+    # because that treats the path literally on every platform
+    #: (String str) -> String
+    def escape_glob_metacharacters(str)
+      str.gsub(/[\[\]{}*?,\\]/) { |c| "\\#{c}" }
+    end
+  end
+
   # Request delegation for embedded languages is not yet standardized into the language server specification. Here we
   # use this custom error class as a way to return a signal to the client that the request should be delegated to the
   # language server for the host language. The support for delegation is custom built on the client side, so each editor

test/addon_test.rb

@@ -191,6 +191,38 @@ def version
       end
     end
 
+    def test_project_specific_addons_in_a_workspace_path_with_glob_metacharacters
+      Dir.mktmpdir do |dir|
+        # Routing conventions like Rails or Next.js produce directory names such as `[id]` or `{slug}`, which are
+        # `Dir.glob` metacharacters. They must be treated as literal path segments
+        workspace = File.join(dir, "[id]", "{slug}")
+        addon_dir = File.join(workspace, "lib", "ruby_lsp", "test_addon")
+        FileUtils.mkdir_p(addon_dir)
+        File.write(File.join(addon_dir, "addon.rb"), <<~RUBY)
+          class GlobMetacharacterProjectAddon < RubyLsp::Addon
+            def activate(global_state, outgoing_queue)
+            end
+
+            def name
+              "Glob Metacharacter Project Addon"
+            end
+
+            def version
+              "0.1.0"
+            end
+          end
+        RUBY
+
+        @global_state.apply_options({
+          workspaceFolders: [{ uri: URI::Generic.from_path(path: workspace).to_s }],
+        })
+        Addon.load_addons(@global_state, @outgoing_queue)
+
+        addon = Addon.get("Glob Metacharacter Project Addon", "0.1.0") #: as untyped
+        assert_equal("Glob Metacharacter Project Addon", addon.name)
+      end
+    end
+
     def test_loading_project_addons_when_the_project_has_no_gemfile
       Dir.mktmpdir do |dir|
         addon_dir = File.join(dir, "lib", "ruby_lsp", "test_addon")