From b85ab09d61f6c38ee54ab6e8d47ab23b20c0e04e Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Tue, 31 Oct 2017 15:48:37 -0700 Subject: [PATCH 1/7] Fix offline mirror file collision with internal registries and scoped packages When using artifactory as an internal npm registry in conjunction with scoped packages, yarn install was failing (about 50% of the time) with messages of the form Hashes don't match when extracting file "https://artifactory.internal.site:443/artifactory/api/npm/external-mirror/@types/react/-/react-15.6.4.tgz". Expected "3bb57bd43183a05919ceb025a264287348f47e9d" but got "da39a3ee5e6b4b0d3255bfef95601890afd80709" The problem was that yarn was writing both @types/react-15.6.4.tgz and react-15.6.4.tgz to the same location within the offline mirror due to making an assumption about no additional leading paths before the scope name in the URL -- an assumption which was true for registry.npmjs.org, but not for artifactory npm repositories. --- __tests__/fetchers.js | 22 ++++++++++++++++++++++ src/fetchers/tarball-fetcher.js | 15 ++++++++++----- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/__tests__/fetchers.js b/__tests__/fetchers.js index 6b173de948..57de5c1fa1 100644 --- a/__tests__/fetchers.js +++ b/__tests__/fetchers.js @@ -233,3 +233,25 @@ test('TarballFetcher.fetch properly stores tarball of scoped package in offline const exists = await fs.exists(path.join(offlineMirrorDir, '@exponent-configurator-1.0.2.tgz')); expect(exists).toBe(true); }); + +test('TarballFetcher.tarball mirror path correctly chosen for scoped package resolved from internal artifactory registry', async () => { + const dir = await mkdir('tarball-fetcher'); + const offlineMirrorDir = await mkdir('offline-mirror'); + + const config = await Config.create(); + config.registries.npm.config['yarn-offline-mirror'] = offlineMirrorDir; + + const fetcher = new TarballFetcher( + dir, + { + type: 'tarball', + hash: '6f0ab73cdd7b82d8e81e80838b49e9e4c7fbcc44', + reference: + 'https://artifactory.internal.site:443/artifactory/api/npm/external-mirror/@exponent/configurator/-/configurator-1.0.2.tgz', + registry: 'npm', + }, + config, + ); + + expect(fetcher.getTarballMirrorPath()).toBe(path.join(offlineMirrorDir, '@exponent-configurator-1.0.2.tgz')); +}); diff --git a/src/fetchers/tarball-fetcher.js b/src/fetchers/tarball-fetcher.js index c01cc45cd5..b7c2ba62a7 100644 --- a/src/fetchers/tarball-fetcher.js +++ b/src/fetchers/tarball-fetcher.js @@ -44,13 +44,18 @@ export default class TarballFetcher extends BaseFetcher { return null; } - // handle scoped packages const pathParts = pathname.replace(/^\//, '').split(/\//g); + const tarballBasename = pathParts[pathParts.length - 1]; - const packageFilename = - pathParts.length >= 2 && pathParts[0][0] === '@' - ? `${pathParts[0]}-${pathParts[pathParts.length - 1]}` // scopped - : `${pathParts[pathParts.length - 1]}`; + // handle scoped packages; the scope is the 4th-to-last path, as + // seen in the following examples: + // 'https://registry.npmjs.org/@exponent/configurator/-/configurator-1.0.2.tgz' + // 'https://artifactory.internal.site:443/' + + // 'artifactory/api/npm/release-repo/@exponent/configurator/-/configurator-1.0.2.tgz' + + const scopeName = pathParts.length >= 4 ? pathParts[pathParts.length - 4] : ''; + const hasScopeName = scopeName !== '' && scopeName[0] == '@'; + const packageFilename = hasScopeName ? `${scopeName}-${tarballBasename}` : tarballBasename; return this.config.getOfflineMirrorPath(packageFilename); } From 2ecb4593f2e02bb6e0c2e4a6082a34675cdc8212 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 6 Dec 2017 17:27:39 +0000 Subject: [PATCH 2/7] Use regex for stronger guarantees --- src/fetchers/tarball-fetcher.js | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/fetchers/tarball-fetcher.js b/src/fetchers/tarball-fetcher.js index 93b5356198..e5d0e4d977 100644 --- a/src/fetchers/tarball-fetcher.js +++ b/src/fetchers/tarball-fetcher.js @@ -17,6 +17,8 @@ const stream = require('stream'); const gunzip = require('gunzip-maybe'); const invariant = require('invariant'); +const RE_URL_NAME_MATCH = /\/(?:@([^/]+)\/)?[^/]+\/-\/([^/]+)$/; + export default class TarballFetcher extends BaseFetcher { async setupMirrorFromCache(): Promise { const tarballMirrorPath = this.getTarballMirrorPath(); @@ -44,18 +46,14 @@ export default class TarballFetcher extends BaseFetcher { return null; } - const pathParts = pathname.replace(/^\//, '').split(/\//g); - const tarballBasename = pathParts[pathParts.length - 1]; + const match = RE_URL_NAME_MATCH.exec(pathname); - // handle scoped packages; the scope is the 4th-to-last path, as - // seen in the following examples: - // 'https://registry.npmjs.org/@exponent/configurator/-/configurator-1.0.2.tgz' - // 'https://artifactory.internal.site:443/' + - // 'artifactory/api/npm/release-repo/@exponent/configurator/-/configurator-1.0.2.tgz' + if (!match) { + return null; + } - const scopeName = pathParts.length >= 4 ? pathParts[pathParts.length - 4] : ''; - const hasScopeName = scopeName !== '' && scopeName[0] == '@'; - const packageFilename = hasScopeName ? `${scopeName}-${tarballBasename}` : tarballBasename; + const [, scope, tarballBasename] = match; + const packageFilename = scope ? `${scope}-${tarballBasename}` : tarballBasename; return this.config.getOfflineMirrorPath(packageFilename); } From 61c4ec1b26c761c031bf61a013892960df236a67 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 6 Dec 2017 17:36:31 +0000 Subject: [PATCH 3/7] Move '@' into the scope name --- src/fetchers/tarball-fetcher.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fetchers/tarball-fetcher.js b/src/fetchers/tarball-fetcher.js index e5d0e4d977..6c57ddd3dc 100644 --- a/src/fetchers/tarball-fetcher.js +++ b/src/fetchers/tarball-fetcher.js @@ -17,7 +17,7 @@ const stream = require('stream'); const gunzip = require('gunzip-maybe'); const invariant = require('invariant'); -const RE_URL_NAME_MATCH = /\/(?:@([^/]+)\/)?[^/]+\/-\/([^/]+)$/; +const RE_URL_NAME_MATCH = /\/(?:(@[^/]+)\/)?[^/]+\/-\/([^/]+)$/; export default class TarballFetcher extends BaseFetcher { async setupMirrorFromCache(): Promise { From f1d3bd2fa3b57ddef330aaa47f31bed27cc5d740 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 6 Dec 2017 23:14:39 +0000 Subject: [PATCH 4/7] extend the regex --- src/fetchers/tarball-fetcher.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fetchers/tarball-fetcher.js b/src/fetchers/tarball-fetcher.js index 6c57ddd3dc..18271ca587 100644 --- a/src/fetchers/tarball-fetcher.js +++ b/src/fetchers/tarball-fetcher.js @@ -17,7 +17,7 @@ const stream = require('stream'); const gunzip = require('gunzip-maybe'); const invariant = require('invariant'); -const RE_URL_NAME_MATCH = /\/(?:(@[^/]+)\/)?[^/]+\/-\/([^/]+)$/; +const RE_URL_NAME_MATCH = /\/(?:(@[^/]+)\/)?[^/]+\/-\/(?:@[^/]+\/)?([^/]+)$/; export default class TarballFetcher extends BaseFetcher { async setupMirrorFromCache(): Promise { From a0886e9dec8e92566fd600fc928063a87a72656e Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Thu, 7 Dec 2017 10:57:37 +0000 Subject: [PATCH 5/7] Add fallback mechanism when regex fails --- src/fetchers/tarball-fetcher.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/fetchers/tarball-fetcher.js b/src/fetchers/tarball-fetcher.js index 18271ca587..e11ce55197 100644 --- a/src/fetchers/tarball-fetcher.js +++ b/src/fetchers/tarball-fetcher.js @@ -48,13 +48,15 @@ export default class TarballFetcher extends BaseFetcher { const match = RE_URL_NAME_MATCH.exec(pathname); - if (!match) { - return null; + let packageFilename; + if (match) { + const [, scope, tarballBasename] = match; + packageFilename = scope ? `${scope}-${tarballBasename}` : tarballBasename; + } else { + // fallback to base name + packageFilename = path.basename(pathname); } - const [, scope, tarballBasename] = match; - const packageFilename = scope ? `${scope}-${tarballBasename}` : tarballBasename; - return this.config.getOfflineMirrorPath(packageFilename); } From 3dfa3e2fbd6a25d7de31d727e9c49802484ecad1 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Thu, 7 Dec 2017 11:39:30 +0000 Subject: [PATCH 6/7] prefer match over exec --- src/fetchers/tarball-fetcher.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fetchers/tarball-fetcher.js b/src/fetchers/tarball-fetcher.js index e11ce55197..0ea45c4234 100644 --- a/src/fetchers/tarball-fetcher.js +++ b/src/fetchers/tarball-fetcher.js @@ -46,7 +46,7 @@ export default class TarballFetcher extends BaseFetcher { return null; } - const match = RE_URL_NAME_MATCH.exec(pathname); + const match = pathname.match(RE_URL_NAME_MATCH); let packageFilename; if (match) { From 93b6545833f200122691b340c3d572f9d1e19b0d Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Thu, 7 Dec 2017 11:48:55 +0000 Subject: [PATCH 7/7] Add another test case --- __tests__/fetchers.js | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/__tests__/fetchers.js b/__tests__/fetchers.js index 5be0db2b17..e5ec555b24 100644 --- a/__tests__/fetchers.js +++ b/__tests__/fetchers.js @@ -256,7 +256,7 @@ test('TarballFetcher.fetch properly stores tarball of scoped package in offline expect(exists).toBe(true); }); -test('TarballFetcher.tarball mirror path correctly chosen for scoped package resolved from internal artifactory registry', async () => { +test('TarballFetcher.fetch properly stores tarball for scoped package resolved from artifactory registry', async () => { const dir = await mkdir('tarball-fetcher'); const offlineMirrorDir = await mkdir('offline-mirror'); @@ -277,3 +277,25 @@ test('TarballFetcher.tarball mirror path correctly chosen for scoped package res expect(fetcher.getTarballMirrorPath()).toBe(path.join(offlineMirrorDir, '@exponent-configurator-1.0.2.tgz')); }); + +test('TarballFetcher.fetch properly stores tarball for scoped package resolved from new style URLs', async () => { + const dir = await mkdir('tarball-fetcher'); + const offlineMirrorDir = await mkdir('offline-mirror'); + + const config = await Config.create(); + config.registries.npm.config['yarn-offline-mirror'] = offlineMirrorDir; + + const fetcher = new TarballFetcher( + dir, + { + type: 'tarball', + hash: '6f0ab73cdd7b82d8e81e80838b49e9e4c7fbcc44', + reference: + 'https://artifactory.internal.site:443/artifactory/api/npm/external-mirror/@exponent/configurator/-/@exponent/configurator-1.0.2.tgz', + registry: 'npm', + }, + config, + ); + + expect(fetcher.getTarballMirrorPath()).toBe(path.join(offlineMirrorDir, '@exponent-configurator-1.0.2.tgz')); +});