Skip to content

Fix percent-encoding gaps in npm_and_yarn and bun MetadataFinders#14711

Merged
v-HaripriyaC merged 7 commits intochp/brokendownlinks-npm-yarnfrom
copilot/fix-percent-encoding-issues
Apr 14, 2026
Merged

Fix percent-encoding gaps in npm_and_yarn and bun MetadataFinders#14711
v-HaripriyaC merged 7 commits intochp/brokendownlinks-npm-yarnfrom
copilot/fix-percent-encoding-issues

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 13, 2026

PR #14638 removed the deprecated URI::DEFAULT_PARSER.escape from npm_and_yarn MetadataFinder's dependency_url, but left normalize_registry_url (called via configured_registry_from_credentials) without any space-encoding. Credentials with raw spaces in their registry URL — a supported, tested scenario — would produce invalid HTTP requests on the credentials path. The bun MetadataFinder also had the identical unencoded maintainer_changes bug that #14638 fixed for npm_and_yarn.

Changes

npm_and_yarn/lib/dependabot/npm_and_yarn/metadata_finder.rb

  • normalize_registry_url now strips whitespace and encodes spaces as %20, matching RegistryFinder#prepare_registry_url:
    def normalize_registry_url(registry)
      return nil unless registry
      registry = registry.strip.gsub(/\s+/, "%20")
      return registry if registry.start_with?("http")
      "https://#{registry}"
    end

bun/lib/dependabot/bun/metadata_finder.rb

  • Added require "cgi", CHARS_REQUIRING_ENCODING constant, and encode_npm_releaser private helper (identical to the npm_and_yarn fix)
  • Updated maintainer_changes to use encode_npm_releaser for the URL portion; display text intentionally left unencoded
  • Same normalize_registry_url space-encoding fix applied for consistency (forward-compatible with eventual URI::DEFAULT_PARSER.escape removal)

Tests & fixtures

  • npm_and_yarn spec: new dependency_url test for a replaces-base credential with spaces in the registry URL
  • bun spec: new maintainer_changes test asserting %20-encoded link when releaser name contains spaces
  • bun/spec/fixtures/npm_responses/npm-package-json-lint.json copied from npm_and_yarn to back the new bun fixture test

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • api.launchpad.net
    • Triggering command: /usr/bin/add-apt-repository add-apt-repository -y ppa:git-core/ppa grep rity.crt -q la/emSign_Root_CA_-_G1.crt e u3_amd64.deb (dns block)
  • ports.ubuntu.com
    • Triggering command: /usr/lib/apt/methods/http /usr/lib/apt/methods/http (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Original prompt

Context

PR #14638 (chp/brokendownlinks-npm-yarn) correctly removes the deprecated URI::DEFAULT_PARSER.escape call from MetadataFinder#dependency_url and replaces the npm releaser encoding with a proper RFC 3986-compliant helper. However, during review we identified a regression gap:

Problem

The configured_registry_from_credentials method in npm_and_yarn/lib/dependabot/npm_and_yarn/metadata_finder.rb calls normalize_registry_url, which does not percent-encode spaces or other unsafe characters. Previously, URI::DEFAULT_PARSER.escape in dependency_url served as a safety net for this path.

The RegistryFinder class handles this correctly via prepare_registry_url which does url&.strip&.gsub(/\s+/, "%20"), but MetadataFinder has its own configured_registry_from_credentials that bypasses RegistryFinder entirely. This path is hit when new_source is nil (dependencies resolved from the default registry) and a replaces-base credential is configured.

Existing specs in registry_finder_spec.rb prove that credentials with spaces are a supported scenario:

context "with no rc and with credentials containing spaces" do
  let(:credentials) do
    [Dependabot::Credential.new(
      {
        "type" => "npm_registry",
        "registry" => "http://example.com/registry  with spaces",
        "replaces-base" => true
      }
    )]
  end
  it { is_expected.to eq("http://example.com/registry%20with%20spaces") }
end

Additionally, the bun ecosystem's metadata_finder.rb has the same unencoded maintainer_changes bug that PR #14638 fixes for npm_and_yarn, but was not addressed.

Required Changes

1. Fix normalize_registry_url in npm_and_yarn/lib/dependabot/npm_and_yarn/metadata_finder.rb

Update normalize_registry_url to also strip and encode spaces, matching the behavior of RegistryFinder#prepare_registry_url:

def normalize_registry_url(registry)
  return nil unless registry
  registry = registry.strip.gsub(/\s+/, "%20")
  return registry if registry.start_with?("http")
  "https://#{registry}"
end

2. Add a test case in npm_and_yarn/spec/dependabot/npm_and_yarn/metadata_finder_spec.rb

Add a test for the source_url (or dependency_url) path that exercises configured_registry_from_credentials with a credential containing spaces, verifying that the URL is properly encoded.

3. Apply the same encode_npm_releaser fix to bun/lib/dependabot/bun/metadata_finder.rb

The bun metadata finder at bun/lib/dependabot/bun/metadata_finder.rb has the identical bug in maintainer_changes (lines 30-38):

def maintainer_changes
  return unless npm_releaser
  return unless npm_listing.dig("time", dependency.version)
  return if previous_releasers&.include?(npm_releaser)

  "This version was pushed to npm by " \
    "[#{npm_releaser}](https://www.npmjs.com/~#{npm_releaser}), a new " \
    "releaser for #{dependency.name} since your current version."
end

Apply the same pattern used in the npm_and_yarn fix:

  • Add require "cgi" at the top
  • Add the CHARS_REQUIRING_ENCODING constant
  • Add the encode_npm_releaser private helper method
  • Update maintainer_changes to use encode_npm_releaser for the URL portion

4. Fix normalize_registry_url in bun/lib/dependabot/bun/metadata_finder.rb too

The bun metadata finder has the same normalize_registry_url that doesn't encode spaces. Apply the same fix as in npm_and_yarn.

5. Add test coverage for bun in bun/spec/dependabot/bun/metadata_finder_spec.rb

Add similar test cases for the bun ecosystem:

  • Test that maintainer_changes properly encodes releaser names with spaces
  • Test encode_npm_releaser private helper

6. Add the same npm response fixture for bun tests

Copy or reference the npm-package-json-lint.json fixture so bun tests can verify the maintainer encoding behavior.

Important Notes

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: Can you review the changes?

Assistant:

I'll load the PR review abilities and then gather the necessary context to review this pull request.

Assistant: Now let me check if CI is all green:

Assistant: Here's my review of PR #14638:


PR Summary

percent-encode npm releaser names in Maintainer changes section by @v-HaripriyaC

Fixes #14595...

This pull request was created from Copilot chat.

Copilot AI changed the title [WIP] Fix percent-encoding for registry URLs in MetadataFinder Fix percent-encoding gaps in npm_and_yarn and bun MetadataFinders Apr 13, 2026
Copilot AI requested a review from kbukum1 April 13, 2026 20:08
@kbukum1 kbukum1 marked this pull request as ready for review April 13, 2026 20:13
@kbukum1 kbukum1 requested a review from a team as a code owner April 13, 2026 20:13
@v-HaripriyaC v-HaripriyaC merged commit d24e7eb into chp/brokendownlinks-npm-yarn Apr 14, 2026
54 of 57 checks passed
@v-HaripriyaC v-HaripriyaC deleted the copilot/fix-percent-encoding-issues branch April 14, 2026 01:21
v-HaripriyaC pushed a commit that referenced this pull request Apr 15, 2026
…4711)

* Initial plan

* Fix percent-encoding issues in npm_and_yarn and bun MetadataFinders

Agent-Logs-Url: https://github.com/dependabot/dependabot-core/sessions/13261c22-8e0e-42fe-a06d-e8a7c3b7ad76

Co-authored-by: kbukum1 <171620528+kbukum1@users.noreply.github.com>

* Address code review: remove private method test from bun spec

Agent-Logs-Url: https://github.com/dependabot/dependabot-core/sessions/13261c22-8e0e-42fe-a06d-e8a7c3b7ad76

Co-authored-by: kbukum1 <171620528+kbukum1@users.noreply.github.com>

* parametric tests added, fixed some edge case as well

* ade sure the fix is for bun

* added some more fixes

* added small fixe

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: kbukum1 <171620528+kbukum1@users.noreply.github.com>
Co-authored-by: v-HaripriyaC <v-haripriyac@microsoft.com>
v-HaripriyaC pushed a commit that referenced this pull request Apr 15, 2026
…4711)

* Initial plan

* Fix percent-encoding issues in npm_and_yarn and bun MetadataFinders

Agent-Logs-Url: https://github.com/dependabot/dependabot-core/sessions/13261c22-8e0e-42fe-a06d-e8a7c3b7ad76

Co-authored-by: kbukum1 <171620528+kbukum1@users.noreply.github.com>

* Address code review: remove private method test from bun spec

Agent-Logs-Url: https://github.com/dependabot/dependabot-core/sessions/13261c22-8e0e-42fe-a06d-e8a7c3b7ad76

Co-authored-by: kbukum1 <171620528+kbukum1@users.noreply.github.com>

* parametric tests added, fixed some edge case as well

* ade sure the fix is for bun

* added some more fixes

* added small fixe

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: kbukum1 <171620528+kbukum1@users.noreply.github.com>
Co-authored-by: v-HaripriyaC <v-haripriyac@microsoft.com>
v-HaripriyaC pushed a commit that referenced this pull request Apr 15, 2026
…4711)

* Initial plan

* Fix percent-encoding issues in npm_and_yarn and bun MetadataFinders

Agent-Logs-Url: https://github.com/dependabot/dependabot-core/sessions/13261c22-8e0e-42fe-a06d-e8a7c3b7ad76

Co-authored-by: kbukum1 <171620528+kbukum1@users.noreply.github.com>

* Address code review: remove private method test from bun spec

Agent-Logs-Url: https://github.com/dependabot/dependabot-core/sessions/13261c22-8e0e-42fe-a06d-e8a7c3b7ad76

Co-authored-by: kbukum1 <171620528+kbukum1@users.noreply.github.com>

* parametric tests added, fixed some edge case as well

* ade sure the fix is for bun

* added some more fixes

* added small fixe

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: kbukum1 <171620528+kbukum1@users.noreply.github.com>
Co-authored-by: v-HaripriyaC <v-haripriyac@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants