fix: add https port parsing on registry matching for artifactory compatibility

[FrederickEngelhardt]

Changes

* add compatibility back for always-auth=true so the authorization is still passed to components not matching the registry urls Removed due to security issue.

• add https (:443 ) port filter in the URL parsing section so that authorization headers are added if the registry request matches.
• This fixes problems with large organizations that use and share multiple scopes across registries

Issue

1. pnpm was deleting the authorization header, making registries that act as pass-through registries fail to fetch their tarball file download when attempting to access another registry with shared scope. Removed due to security issue.
2. pnpm was skipping https ports IE :443 that artifactory was transforming. This caused a registry mismatch and it skipped authorization. This is related to URL which does not return well-known ports (0-1023) in the .ports param. It still however parses and removes the https port.

Solution

Initial fix:
1. Add a logic block to protect against deletion of authorization if always-auth is provided in .npmrc (<=Node16) or the pnpm config >=Node18. Removed due to security issue.
2. Adding a matcher for :443 fixes the port problem so that when communicating with artifactory the authorization is not removed due to url mismatch.

Possible follow-up fixes related to auth (excluded from this PR scope)

• add url parsing so that the authorization urls approved and authorization is not being passed to packages outside registry scope accidentally.
  • Example if you provided the registry=https://vllc.dev/artifactory/api/npm/npm-virtual/ adding a second key registry-root=https://vllc.dev/artifactory/api/npm/ would capture and add auth to all other registries in this structure and delete the rest.

pnpm config file

registry-root=https://vllc.dev/artifactory/api/npm/
registry=https://vllc.dev/artifactory/api/npm/npm-virtual/

Use case

This problem applies to all Artifactory registries with (>=2) registries and with scoped packages mixed across each registry. FYI this is a real world problem.

• Any usage of another registry that could have blended scope
  • Example Installing a scope of @platform could resolve to multiple registries resolved by artifactory authorization.
• Any usage of artifactory with more than 1 registry with blended scopes.

Due to a scoped package being unknown which registry to use, using the @platform:registry=https://<company_domain>/artifactory/api/npm/npm-virtual/ will not properly resolve the package correctly, this is because package-a could point to npm-virtual but package-b could point to npm-virtual-v2 and so on.

Artifactory continues to recommend in npm v9 to use the always-auth=true flag (which is deprecated in 18).

[welcome]

💖 Thanks for opening this pull request! 💖
Please be patient and we will get back to you as soon as we can.

[zkochan]

add compatibility back for always-auth=true so the authorization is still passed to components not matching the registry urls

We cannot add this back. It was removed both from npm and pnpm as it creates a security vulnerability.

[FrederickEngelhardt]

We cannot add this back. It was removed both from npm and pnpm as it creates a security vulnerability.

Can we do a registry origin check instead and then resolve the url only if it matches that domain or base path?

[zkochan]

I'll think about it later. @weswigham you were the one who reported the always-auth issue, so I would be happy to have your feedback here.

[FrederickEngelhardt]

@zkochan Since the first commit cannot be merged due to security reasons, I can remove it or make a new PR with only the HTTPS port fix. Related commit that should go in 9efa80a

This will allow for scoped packages to resolve correctly if they have the :443 port added in the request. Artifactory urls do this.

[FrederickEngelhardt]

Renamed the PR and rebase dropped the always auth changes. This PR only contains the HTTPS port parsing logic now.

[FrederickEngelhardt]

Noting that the following needs to be done to replicate the issue.

1. Add a new dependency only available within artifactory or delete the pnpm-lock.yaml and have that dependency defined within your package.json
2. Install a dependency that is published only in the artifactory scope IE pnpm install @platform/artifactory-only-platform-code
3. Each replication make sure to do a rm pnpm-lock.yaml; ../pnpm/pnpm/dev/pd.js store prune; rm -rf node_modules; ../pnpm/pnpm/dev/pd.js install in your test repo that is on the same level as the pnpm repo.

Curiously only the artifactory urls fail that are unavailable externally. I'm wondering if it's defaulting to npm registry if the package fails to validate the registry. The output pnpm-lock.yaml still points to the :443 urls so at least the registry is preserved for the actual downloading part.

[zkochan]

Can you cover this with a unit test?

[zkochan]

Please also handle the case with port :80 and http. So there are two issues, port 443 on https and 80 on http.

[zkochan]

It is probably just enough to do this:

Suggested change

	/**
	* @artifactory adds a https port in the middle of their urls and URL.parse will not return ports in that range
	*/
	if (hasHTTPS && urlObj.port === '') {
	return urlObj.href
	}
	if (urlObj.port === '') return originalUrl
	if (urlObj.port === '') return urlObj.href

it will handle both :80 and :443

[FrederickEngelhardt]

@zkochan agreed. I added tests for http|https|ws|wss and moved the removePort method to it's own file so it can be tested without exporting it.

[welcome]

Congrats on merging your first pull request! 🎉🎉🎉