dependabot / dependabot-core

🤖 Dependabot's core logic for creating update PRs.
https://docs.github.com/en/code-security/dependabot
MIT License
4.72k stars 1.02k forks source link

Unexpected failure specifying https url in an npm credential #5570

Open randymarsh77 opened 2 years ago

randymarsh77 commented 2 years ago

Specifying a credential for a private npm registry using a url starting with https:// is invalid, since the logic results in querying a url like https://https://actual.registry.etc

This code is "the problem": https://github.com/dependabot/dependabot-core/blob/83043c7d61ec39cc2e7145dcd87982a1c3378124/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/registry_finder.rb#L104-L113

Notice that any http scheme is stripped from lockfile_registry, but not detailed_registry.

It's easy enough to workaround this by omitting the scheme from the registry url when specifying credentials, though this seems like an unfortunate pitfall.

The result is that the request fails in an unhandled way and the npm details are empty, so there is no error, but no updates will be found.

It seems like it would be perfectly safe to perform the replacement on both of those strings. (I tested this replacement and the code worked the same as omitting https:// from my credential.)

Package ecosystem npm

jeffwidman commented 2 years ago

Thanks for debugging!

This seems reasonable to me, want to submit a PR with the fix?

andrcuns commented 2 years ago

FYI, I'm fairly certain similar issue exists with docker registry credentials as well.

github-actions[bot] commented 2 months ago

👋 This issue has been marked as stale because it has been open for 2 years with no activity. You can comment on the issue to hold stalebot off for a while, or do nothing. If you do nothing, this issue will be closed eventually by the stalebot. Please see CONTRIBUTING.md for more policy details.

abdulapopoola commented 2 months ago

@sachin-sandhu , can you please help with a fix?

sachin-sandhu commented 2 months ago

Hi @randymarsh77 , can you give us the .npmrc file format that resulted in an error, is it similar to following

registry=https://npm.fury.io/dependabot/ https://npm.fury.io/dependabot/:_authToken=xxxxxx

sachin-sandhu commented 1 month ago

Hi @randymarsh77 , gentle reminder https://github.com/dependabot/dependabot-core/issues/5570#issuecomment-2339442879

randymarsh77 commented 1 month ago

I don't believe I ever had an issue when using an npmrc. I had an issue using the dependabot-core library with the following code:

credentials = [
  {
    'type' => 'git_source',
    'host' => github_host,
    'username' => 'x-access-token',
    'password' => github_token
  }
]
azure_npm_token = options[:'az-npm-token']
unless azure_npm_token.nil? || azure_npm_token.empty?
  credentials.append(
    {
      'type' => 'npm_registry',
      'registry' => 'https://pkgs.dev.azure.com/REDACTED/Packages/_packaging/REDACTED/npm/registry/',
      'token' => azure_npm_token
    }
  )
end

# ...

# Maybe this part?

root_update_group = DependabotAction::UpdateGroup.new(
  source: Dependabot::Source.new(
    provider: 'github',
    hostname: github_host,
    api_endpoint: github_api_endpoint,
    repo: repo,
    directory: '.',
    branch: current_branch
  ),
  credentials: credentials,
  group_name: group_name,
  use_semantic_release_format: use_semantic_release_format,
  github_token: github_token,
  github_api_endpoint: github_api_endpoint
)

# ...

# Or, maybe this part?

fetcher = Dependabot::FileFetchers.for_package_manager(package_manager).new(
  source: source,
  credentials: credentials
)
files = fetcher.files
commit = fetcher.commit

# ...

# But, probably this part based on where I tracked the bug to:

checker = Dependabot::UpdateCheckers.for_package_manager(package_manager).new(
  dependency: dep,
  dependency_files: files,
  credentials: credentials
)

next if checker.up_to_date?

In any case, I just removed https:// from that string. I also noted that this is a "problem", quoted because it might be intentional that you must pass an http endpoint and omit the scheme as registry, but it sure looked like an oversight based on the surrounding code that handled the http scheme being included.