NPM: fix GitHub registry not working when path is specified

[jakecoffman]

We noticed quite a few users put the path in their private registry configuration for GitHub Packages (url: npm.pkg.github.com/my-org) which causes Dependabot to fallback to using the resolved line as the registry URL. This doesn't work however, because the GItHub registry has a special /download URL it uses.

To correct that, I've added a special case for the GitHub registry where it will return just the host instead of the extra path. I tested this with the Dependabot CLI and it seems to fix the issue 🎉

[deivid-rodriguez]

Does this fallback actually work for any private registry? It feels that this should be returning just a host, not a URI dependent on each particular dependency, which is what's locked in the lockfile 🤔

[jurre]

Nice! I wondered if we might hoist this up to where we define a package's source, but I think it's a lot more risky and probably not worth it for now

[jakecoffman]

@deivid-rodriguez great question, it seems like it wouldn't work! Should it just return URI(registry_source_url).host instead?

[deivid-rodriguez]

That was my idea, yes.

Looking into this more, I think this PR may shed some light, and might have actually introduced this bug 😬.

Apparently when there's no .npmrc file but there's a lockfile, we infer a .npmrc file from the lockfile information during file fetching, and I guess that inferred file is used by the registry finder being changed by this PR.

Until #7030, this inferred file included just the host, but that PR changed that because apparently some registries need the user/org after the host (like gemfury). Turns out GitHub registry works the opposite way? 😞

So, I guess we need to special case the fallback regarding GitHub vs other private registries (for GitHub use just the host, for others use also the path up until the dependency name like in #7030).

Also, since there's a fallback in place here to use the lockfile URL, I'm not sure we even need to infer a .npmrc during file fetching at all 🤔.

[jakecoffman]

I actually rolled the code back to early march trying to bisect it and it still has this issue, so I don't think it was a recent change.

[jakecoffman]

I'll save this for tomorrow to deploy to give folks more time to think about it... also it's the end of my day 🙂

[deivid-rodriguez]

Did you try a case without a .npmrc file? Or a case with a wrong .npmrc file? My hypothesis was that some errors of this family could be due to the former, while the case you're seeing I think it's the former?

Keeping digging, even if this variable is called lockfile_registry, this is just using the source url set for the dependency, but I would not bet that this is always the raw lockfile URL, so returning just the host here may be risky for other private sources? I think this is originally set here, so maybe a better fix could be to add a special case for GitHub private registry there?

[jurre]

Should we try to find known registries using only the host first?

[deivid-rodriguez]

My intuition is that this lockfile_registry variable should be holding the proper registry url already (just host, or host plus some path depending on the type of private registry). The fact that's holding a full path to a dependency in this case I believe it may be a bug in the file parser, specific to GitHub private registries. But that's just me guessing...

[jakecoffman]

Looks like @deivid-rodriguez is right, there's even a test for this case:

dependabot-core/npm_and_yarn/spec/dependabot/npm_and_yarn/file_parser_spec.rb

Lines 241 to 254 in de36c26

	its(:requirements) do
	is_expected.to eq(
	[{
	requirement: "^2.0.1",
	file: "package.json",
	groups: ["devDependencies"],
	source: {
	type: "registry",
	url: "https://npm.pkg.github.com"
	}
	}]
	)
	end
	end

Unclear why it doesn't work sometimes, still investigating, but I think the fix will be here in the FileParser.

[jakecoffman]

Looks like this is the source of the regression: #6306

[jakecoffman]

I added back the include? check that was removed in #6306. Basically the GitHub Registry should never match credentials if it has a path because the resolved path starts with /downloads, but since I removed the include? previously, the hostname matched, and so it didn't fall through here:

dependabot-core/npm_and_yarn/lib/dependabot/npm_and_yarn/file_parser.rb

Lines 283 to 286 in b0b0dc7

	elsif (cred_url = url_for_relevant_cred(resolved_url)) then cred_url
	else
	resolved_url.split("/")[0..2].join("/")
	end

[deivid-rodriguez]

👏 👏