fix: add https port parsing on registry matching for artifactory compatibility by FrederickEngelhardt · Pull Request #6864 · pnpm/pnpm

FrederickEngelhardt · 2023-07-26T05:09:30Z

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 · 2023-07-26T05:09:32Z

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

zkochan · 2023-07-26T09:33:04Z

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 · 2023-07-26T09:45:08Z

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 · 2023-07-26T11:51:59Z

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 · 2023-07-26T18:24:28Z

@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.

…tion for thoses urls

FrederickEngelhardt · 2023-07-26T19:01:18Z

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

FrederickEngelhardt · 2023-07-26T19:13:15Z

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

Add a new dependency only available within artifactory or delete the pnpm-lock.yaml and have that dependency defined within your package.json
Install a dependency that is published only in the artifactory scope IE pnpm install @platform/artifactory-only-platform-code
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 · 2023-07-26T19:36:50Z

Can you cover this with a unit test?

zkochan · 2023-07-26T23:31:09Z

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

zkochan · 2023-07-26T23:33:02Z

+  /**
+   * @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


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

@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.

…tion for thoses urls

…breaking-artifactory-virtual-registries

…tion for thoses urls

welcome · 2023-07-27T14:18:32Z

Congrats on merging your first pull request! 🎉🎉🎉

…tifactory compatibility (#6864) Co-authored-by: Frederick Engelhardt <frederick.engelhardt@bestbuy.com> Co-authored-by: Zoltan Kochan <z@kochan.io>

Related PR: - pnpm/pnpm#6864

FrederickEngelhardt requested a review from zkochan as a code owner July 26, 2023 05:09

FrederickEngelhardt force-pushed the fix/pnpm-issues-with-breaking-artifactory-virtual-registries branch from 078f8d6 to 80dae65 Compare July 26, 2023 05:22

fix(https-port-validation): add https port matcher to allow authoriza…

fb68e29

…tion for thoses urls

FrederickEngelhardt force-pushed the fix/pnpm-issues-with-breaking-artifactory-virtual-registries branch from 9efa80a to fb68e29 Compare July 26, 2023 18:42

FrederickEngelhardt changed the title ~~feat(always-auth): add back support for always-auth for artifactory compatibility~~ feat(always-auth): add https port parsing on registry matching for artifactory compatibility Jul 26, 2023

zkochan reviewed Jul 26, 2023

View reviewed changes

zkochan added 4 commits July 27, 2023 15:04

fix(https-port-validation): add https port matcher to allow authoriza…

76fe821

…tion for thoses urls

Merge remote-tracking branch 'origin/main' into fix/pnpm-issues-with-…

ebb193c

…breaking-artifactory-virtual-registries

fix(https-port-validation): add https port matcher to allow authoriza…

dfc72e0

…tion for thoses urls

fix(https-port-validation): add https port matcher to allow authoriza…

3af99bb

…tion for thoses urls

zkochan approved these changes Jul 27, 2023

View reviewed changes

zkochan merged commit aa20818 into pnpm:main Jul 27, 2023

FrederickEngelhardt changed the title ~~feat(always-auth): add https port parsing on registry matching for artifactory compatibility~~ feat(always-auth): add http|https|ws|wss port parsing on registry matching for artifactory compatibility Jul 27, 2023

FrederickEngelhardt changed the title ~~feat(always-auth): add http|https|ws|wss port parsing on registry matching for artifactory compatibility~~ feat(always-auth): add https port parsing on registry matching for artifactory compatibility Jul 27, 2023

zkochan changed the title ~~feat(always-auth): add https port parsing on registry matching for artifactory compatibility~~ fix: add https port parsing on registry matching for artifactory compatibility Jul 30, 2023

zkochan mentioned this pull request Jul 30, 2023

fix: update pnpm to fix auth issue in artifactory teambit/bit#7714

Merged

zkochan added a commit to teambit/bit that referenced this pull request Jul 30, 2023

fix: update pnpm to fix auth issue in artifactory (#7714)

1cfbb6f

Related PR: - pnpm/pnpm#6864

zkochan added a commit to teambit/bit that referenced this pull request Aug 7, 2023

fix: update pnpm to fix auth issue in artifactory (#7714)

257412e

Related PR: - pnpm/pnpm#6864

luvkapur pushed a commit to teambit/bit that referenced this pull request Aug 21, 2023

fix: update pnpm to fix auth issue in artifactory (#7714)

11dc3b8

Related PR: - pnpm/pnpm#6864

brunojppb mentioned this pull request Aug 23, 2023

tarball URLs are not stable when using a custom NPM registry with a port number #6725

Closed

rainbow180 mentioned this pull request Mar 5, 2024

Cannot authenticate to private registry #7742

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

fix: add https port parsing on registry matching for artifactory compatibility#6864
zkochan merged 5 commits intopnpm:mainfrom
FrederickEngelhardt:fix/pnpm-issues-with-breaking-artifactory-virtual-registries

FrederickEngelhardt commented Jul 26, 2023 •

edited

Loading

Uh oh!

welcome Bot commented Jul 26, 2023

Uh oh!

zkochan commented Jul 26, 2023 •

edited

Loading

Uh oh!

FrederickEngelhardt commented Jul 26, 2023

Uh oh!

zkochan commented Jul 26, 2023

Uh oh!

FrederickEngelhardt commented Jul 26, 2023 •

edited

Loading

Uh oh!

FrederickEngelhardt commented Jul 26, 2023

Uh oh!

FrederickEngelhardt commented Jul 26, 2023 •

edited

Loading

Uh oh!

zkochan commented Jul 26, 2023

Uh oh!

zkochan commented Jul 26, 2023 •

edited

Loading

Uh oh!

zkochan Jul 26, 2023

Uh oh!

FrederickEngelhardt Jul 27, 2023

Uh oh!

welcome Bot commented Jul 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

FrederickEngelhardt commented Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Issue

Solution

Use case

Uh oh!

welcome Bot commented Jul 26, 2023

Uh oh!

zkochan commented Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FrederickEngelhardt commented Jul 26, 2023

Uh oh!

zkochan commented Jul 26, 2023

Uh oh!

FrederickEngelhardt commented Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FrederickEngelhardt commented Jul 26, 2023

Uh oh!

FrederickEngelhardt commented Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zkochan commented Jul 26, 2023

Uh oh!

zkochan commented Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zkochan Jul 26, 2023

Choose a reason for hiding this comment

Uh oh!

FrederickEngelhardt Jul 27, 2023

Choose a reason for hiding this comment

Uh oh!

welcome Bot commented Jul 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

FrederickEngelhardt commented Jul 26, 2023 •

edited

Loading

zkochan commented Jul 26, 2023 •

edited

Loading

FrederickEngelhardt commented Jul 26, 2023 •

edited

Loading

FrederickEngelhardt commented Jul 26, 2023 •

edited

Loading

zkochan commented Jul 26, 2023 •

edited

Loading