Skip to content

fix(npm): support npmrc certfile + keyfile#32655

Merged
dsherret merged 3 commits intodenoland:mainfrom
seroperson:i23951-npmrc-certfile-auth
Mar 12, 2026
Merged

fix(npm): support npmrc certfile + keyfile#32655
dsherret merged 3 commits intodenoland:mainfrom
seroperson:i23951-npmrc-certfile-auth

Conversation

@seroperson
Copy link
Copy Markdown
Contributor

Closes #23951

This PR implements the missing support for certfile and keyfile options. Now when both are set, it creates an HTTP client with the client certificate attached.

Also added two new tests and a new mTLS-enabled test registry:

  • npmrc_certfile - installs a package from an https registry that requires a valid client certificate
  • npmrc_missing_certfile - verifies an error when the certfile path doesn't exist

@seroperson seroperson force-pushed the i23951-npmrc-certfile-auth branch 2 times, most recently from dc1294e to b430795 Compare March 12, 2026 02:28
@seroperson seroperson force-pushed the i23951-npmrc-certfile-auth branch from b430795 to cde33be Compare March 12, 2026 07:37
@seroperson
Copy link
Copy Markdown
Contributor Author

Force-pushed the same commit to re-trigger the CI, as previous build failed with some strange CI-related error.

Copy link
Copy Markdown
Contributor

@kajukitli kajukitli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, good fix

threading RegistryConfig all the way down to the npm cache http client is the right shape. otherwise there was no place to decide whether a request needed a client-cert-backed client.

the per-(certfile,keyfile) client cache in HttpClientProvider also makes sense — you don't want to rebuild a TLS client for every packument/tarball fetch.

nice that you covered both the happy path (mTLS registry) and the missing-certfile error path.

one minor thing: get_or_create_with_client_cert() uses to_string_lossy() for the cache key, so weird non-utf8 paths could alias. probably irrelevant in practice, but if you want to be strict you could key by PathBuf instead of lossy strings.

Copy link
Copy Markdown
Contributor

@dsherret dsherret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Thank you!

@dsherret dsherret enabled auto-merge (squash) March 12, 2026 12:24
@dsherret dsherret merged commit bbd5282 into denoland:main Mar 12, 2026
112 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

.npmrc: support certfile and keyfile auth options

3 participants