fix(npm): registry inferring should include the full registry path

[yeikel]

Fixes #6804

[yeikel]

I believe that this test was incorrect

[yeikel]

I created a test org to double check this

Trying to pull any dependency from https://npm.fury.io/ would fail in this scenario

[deivid-rodriguez]

Thanks for trying this in real life! 💪

[deivid-rodriguez]

Nice fix!

I do agree that the issue is probably not exploited, but I think we can probably keep the previous approach of properly parsing URIs to avoid it. I can suggest something tomorrow.

[deivid-rodriguez]

Thanks for trying this in real life! 💪

[yeikel]

I do agree that the issue is probably not exploited, but I think we can probably keep the previous approach of properly parsing URIs to avoid it. I can suggest something tomorrow.

I do not know what's best in Ruby as I am new to the language, but in the JVM and other languages like C#, this try/catch/recover pattern is not great as it is quite slow to unwind the stack. I expect a similar impact in Ruby but I do not know if it is a good practice or not

In the current code, we are running this logic for every single dependency declared and that can take a lot of time for a project with many dependencies(that is very common in the NPM world to begin with)

[deivid-rodriguez]

I think it's fine in CRuby. But more importantly, I think it's best to focus on the bug fix rather than mixing in performance related patches. If the problem you mention is an issue, we could handle it separately.

Let me suggest this alternative patch that keeps the existing approach of parsing registries into well structured URIs:

diff --git a/npm_and_yarn/lib/dependabot/npm_and_yarn/file_fetcher.rb b/npm_and_yarn/lib/dependabot/npm_and_yarn/file_fetcher.rb
index 4ef88467c..10f75aa33 100644
--- a/npm_and_yarn/lib/dependabot/npm_and_yarn/file_fetcher.rb
+++ b/npm_and_yarn/lib/dependabot/npm_and_yarn/file_fetcher.rb
@@ -90,7 +90,7 @@ module Dependabot
         return @inferred_npmrc = nil unless npmrc.nil? && package_lock
 
         known_registries = []
-        JSON.parse(package_lock.content).fetch("dependencies", {}).each do |_name, details|
+        JSON.parse(package_lock.content).fetch("dependencies", {}).each do |dependency_name, details|
           resolved = details.fetch("resolved", "https://registry.npmjs.org")
           begin
             uri = URI.parse(resolved)
@@ -99,8 +99,24 @@ module Dependabot
             # This can happen if resolved is false, for instance.
             next
           end
+
+          next unless uri.scheme && uri.host
+
           # Check for scheme since path dependencies will not have one
-          known_registries << "#{uri.scheme}://#{uri.host}" if uri.scheme && uri.host
+          known_registry = "#{uri.scheme}://#{uri.host}"
+
+          path = uri.path
+          if path
+            index = path.index(dependency_name)
+
+            if index
+              registry_base_path = path[0...index].delete_suffix("/")
+
+              known_registry << registry_base_path
+            end
+          end
+
+          known_registries << known_registry
         end
 
         if known_registries.uniq.length == 1 && known_registries.first != "https://registry.npmjs.org"

Using this approach is how I discovered yesterday that the test lockfile was incorrect, because the tests you added do not pass with this patch due to the dependency name vs registry url discrepancy.

[yeikel]

@deivid-rodriguez Let me know if you have any additional feedback :)

Thanks again

[yeikel]

@jeffwidman I would appreciate your input here as well

[deivid-rodriguez]

Sorry for the delay @yeikel, this looks good to me, but I'd like to extract the malformed URI handling changes/discussion to a separate PR/issue if that's ok.

[deivid-rodriguez]

@yeikel This looks ready from my side and I'm pretty confident about it since you have taken the time to actually try this on a real registry, but I'm not super familiar with how private registries are setup in general, so let me ask for a second review here from another teammate just in case. Thanks!

[pavera]

LGTM