From 2780c532e113ed801fb5ae6f20bdca0a4bf10130 Mon Sep 17 00:00:00 2001 From: hezhengxu2018 <35598090+hezhengxu2018@users.noreply.github.com> Date: Mon, 23 Dec 2024 14:41:56 +0800 Subject: [PATCH] fix: incorrect request headers in proxy mode and deleted unparsable cached data (#719) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit proxy时因为一个低级的拼写错误没有正确的携带请求头，导致代理模式时返回的数据不正确。但是现在用户发起的请求中的user-agent和x-forwarded等头部信息也没有正确的携带。虽然影响不大但还是想和跑批时更新的请求做一下区分。 ## Summary by CodeRabbit - **Bug Fixes** - Enhanced error handling and logging for task execution, improving traceability. - **Improvements** - Updated HTTP header access method for better alignment with context structure. - Clarified logic for manifest retrieval based on file type, ensuring correct API usage. - Streamlined cache handling and response generation logic in package management. - Improved method visibility and organization within the cache service and controller. - Simplified task creation logic and cache removal processes in the controller. - Updated expected outcomes for cache-related operations in the test cases. --- app/core/service/ProxyCacheService.ts | 133 ++++++++++-------- app/port/controller/ProxyCacheController.ts | 39 ++--- .../package/ShowPackageController.ts | 3 +- test/core/service/ProxyCacheService.test.ts | 88 +----------- .../ProxyCacheController/index.test.ts | 17 +-- 5 files changed, 97 insertions(+), 183 deletions(-) diff --git a/app/core/service/ProxyCacheService.ts b/app/core/service/ProxyCacheService.ts index 1cfe25ec..755926c1 100644 --- a/app/core/service/ProxyCacheService.ts +++ b/app/core/service/ProxyCacheService.ts @@ -59,12 +59,28 @@ export class ProxyCacheService extends AbstractService { } async getPackageManifest(fullname: string, fileType: DIST_NAMES.FULL_MANIFESTS| DIST_NAMES.ABBREVIATED_MANIFESTS): Promise { + const isFullManifests = fileType === DIST_NAMES.FULL_MANIFESTS; const cachedStoreKey = (await this.proxyCacheRepository.findProxyCache(fullname, fileType))?.filePath; if (cachedStoreKey) { - const nfsBytes = await this.nfsAdapter.getBytes(cachedStoreKey); - const nfsString = Buffer.from(nfsBytes!).toString(); - const nfsPkgManifgest = JSON.parse(nfsString); - return nfsPkgManifgest; + try { + const nfsBytes = await this.nfsAdapter.getBytes(cachedStoreKey); + if (!nfsBytes) throw new Error('not found proxy cache, try again later.'); + + const nfsBuffer = Buffer.from(nfsBytes); + const { shasum: etag } = await calculateIntegrity(nfsBytes); + await this.cacheService.savePackageEtagAndManifests(fullname, isFullManifests, etag, nfsBuffer); + + const nfsString = nfsBuffer.toString(); + const nfsPkgManifest = JSON.parse(nfsString); + return nfsPkgManifest as AbbreviatedPackageManifestType|PackageManifestType; + } catch (error) { + /* c8 ignore next 6 */ + if (error.message.includes('not found proxy cache') || error.message.includes('Unexpected token : in JSON at')) { + await this.nfsAdapter.remove(cachedStoreKey); + await this.proxyCacheRepository.removeProxyCache(fullname, fileType); + } + throw error; + } } const manifest = await this.getRewrittenManifest(fullname, fileType); @@ -88,9 +104,19 @@ export class ProxyCacheService extends AbstractService { } const cachedStoreKey = (await this.proxyCacheRepository.findProxyCache(fullname, fileType, version))?.filePath; if (cachedStoreKey) { - const nfsBytes = await this.nfsAdapter.getBytes(cachedStoreKey); - const nfsString = Buffer.from(nfsBytes!).toString(); - return JSON.parse(nfsString) as PackageJSONType | AbbreviatedPackageJSONType; + try { + const nfsBytes = await this.nfsAdapter.getBytes(cachedStoreKey); + if (!nfsBytes) throw new Error('not found proxy cache, try again later.'); + const nfsString = Buffer.from(nfsBytes!).toString(); + return JSON.parse(nfsString) as PackageJSONType | AbbreviatedPackageJSONType; + } catch (error) { + /* c8 ignore next 6 */ + if (error.message.includes('not found proxy cache') || error.message.includes('Unexpected token : in JSON at')) { + await this.nfsAdapter.remove(cachedStoreKey); + await this.proxyCacheRepository.removeProxyCache(fullname, fileType); + } + throw error; + } } const manifest = await this.getRewrittenManifest(fullname, fileType, versionOrTag); this.backgroundTaskHelper.run(async () => { @@ -109,6 +135,27 @@ export class ProxyCacheService extends AbstractService { await this.proxyCacheRepository.removeProxyCache(fullname, fileType, version); } + replaceTarballUrl(manifest: GetSourceManifestAndCacheReturnType, fileType: T) { + const { sourceRegistry, registry } = this.config.cnpmcore; + if (isPkgManifest(fileType)) { + // pkg manifest + const versionMap = (manifest as AbbreviatedPackageManifestType|PackageManifestType)?.versions; + for (const key in versionMap) { + const versionItem = versionMap[key]; + if (versionItem?.dist?.tarball) { + versionItem.dist.tarball = versionItem.dist.tarball.replace(sourceRegistry, registry); + } + } + } else { + // pkg version manifest + const distItem = (manifest as AbbreviatedPackageJSONType | PackageJSONType).dist; + if (distItem?.tarball) { + distItem.tarball = distItem.tarball.replace(sourceRegistry, registry); + } + } + return manifest; + } + async createTask(targetName: string, options: UpdateProxyCacheTaskOptions): Promise { return await this.taskService.createTask(Task.createUpdateProxyCache(targetName, options), false) as CreateUpdateProxyCacheTask; } @@ -151,44 +198,37 @@ export class ProxyCacheService extends AbstractService { await this.taskService.finishTask(task, TaskState.Success, logs.join('\n')); } - async getRewrittenManifest(fullname:string, fileType: T, versionOrTag?:string): Promise> { + // only used by schedule task + private async getRewrittenManifest(fullname:string, fileType: T, versionOrTag?:string): Promise> { let responseResult; + const USER_AGENT = 'npm_service.cnpmjs.org/cnpmcore'; switch (fileType) { - case DIST_NAMES.FULL_MANIFESTS: - responseResult = await this.getUpstreamFullManifests(fullname); + case DIST_NAMES.FULL_MANIFESTS: { + const url = `/${encodeURIComponent(fullname)}?t=${Date.now()}&cache=0`; + responseResult = await this.getProxyResponse({ url, headers: { accept: 'application/json', 'user-agent': USER_AGENT } }, { dataType: 'json' }); break; - case DIST_NAMES.ABBREVIATED_MANIFESTS: - responseResult = await this.getUpstreamAbbreviatedManifests(fullname); + } + case DIST_NAMES.ABBREVIATED_MANIFESTS: { + const url = `/${encodeURIComponent(fullname)}?t=${Date.now()}&cache=0`; + responseResult = await this.getProxyResponse({ url, headers: { accept: ABBREVIATED_META_TYPE, 'user-agent': USER_AGENT } }, { dataType: 'json' }); break; - case DIST_NAMES.MANIFEST: - responseResult = await this.getUpstreamPackageVersionManifest(fullname, versionOrTag!); + } + case DIST_NAMES.MANIFEST: { + const url = `/${encodeURIComponent(fullname)}/${encodeURIComponent(versionOrTag!)}`; + responseResult = await this.getProxyResponse({ url, headers: { accept: 'application/json', 'user-agent': USER_AGENT } }, { dataType: 'json' }); break; - case DIST_NAMES.ABBREVIATED: - responseResult = await this.getUpstreamAbbreviatedPackageVersionManifest(fullname, versionOrTag!); + } + case DIST_NAMES.ABBREVIATED: { + const url = `/${encodeURIComponent(fullname)}/${encodeURIComponent(versionOrTag!)}`; + responseResult = await this.getProxyResponse({ url, headers: { accept: ABBREVIATED_META_TYPE, 'user-agent': USER_AGENT } }, { dataType: 'json' }); break; + } default: break; } // replace tarball url - const manifest = responseResult.data; - const { sourceRegistry, registry } = this.config.cnpmcore; - if (isPkgManifest(fileType)) { - // pkg manifest - const versionMap = manifest.versions || {}; - for (const key in versionMap) { - const versionItem = versionMap[key]; - if (versionItem?.dist?.tarball) { - versionItem.dist.tarball = versionItem.dist.tarball.replace(sourceRegistry, registry); - } - } - } else { - // pkg version manifest - const distItem = manifest.dist || {}; - if (distItem.tarball) { - distItem.tarball = distItem.tarball.replace(sourceRegistry, registry); - } - } + const manifest = this.replaceTarballUrl(responseResult.data, fileType); return manifest; } @@ -204,7 +244,7 @@ export class ProxyCacheService extends AbstractService { await this.nfsAdapter.uploadBytes(storeKey, nfsBytes); } - private async getProxyResponse(ctx: Partial, options?: HttpClientRequestOptions): Promise { + async getProxyResponse(ctx: Partial, options?: HttpClientRequestOptions): Promise { const registry = this.npmRegistry.registry; const remoteAuthToken = await this.registryManagerService.getAuthTokenByRegistryHost(registry); const authorization = this.npmRegistry.genAuthorizationHeader(remoteAuthToken); @@ -221,8 +261,8 @@ export class ProxyCacheService extends AbstractService { compressed: true, ...options, headers: { - accept: ctx.header?.accept, - 'user-agent': ctx.header?.['user-agent'], + accept: ctx.headers?.accept, + 'user-agent': ctx.headers?.['user-agent'], authorization, 'x-forwarded-for': ctx?.ip, via: `1.1, ${this.config.cnpmcore.registry}`, @@ -231,23 +271,4 @@ export class ProxyCacheService extends AbstractService { this.logger.info('[ProxyCacheService:getProxyStreamResponse] %s, status: %s', url, res.status); return res; } - - private async getUpstreamFullManifests(fullname: string): Promise { - const url = `/${encodeURIComponent(fullname)}?t=${Date.now()}&cache=0`; - return await this.getProxyResponse({ url }, { dataType: 'json' }); - } - - private async getUpstreamAbbreviatedManifests(fullname: string): Promise { - const url = `/${encodeURIComponent(fullname)}?t=${Date.now()}&cache=0`; - return await this.getProxyResponse({ url, headers: { accept: ABBREVIATED_META_TYPE } }, { dataType: 'json' }); - } - private async getUpstreamPackageVersionManifest(fullname: string, versionOrTag: string): Promise { - const url = `/${encodeURIComponent(fullname)}/${encodeURIComponent(versionOrTag)}`; - return await this.getProxyResponse({ url }, { dataType: 'json' }); - } - private async getUpstreamAbbreviatedPackageVersionManifest(fullname: string, versionOrTag: string): Promise { - const url = `/${encodeURIComponent(fullname)}/${encodeURIComponent(versionOrTag)}`; - return await this.getProxyResponse({ url, headers: { accept: ABBREVIATED_META_TYPE } }, { dataType: 'json' }); - } - } diff --git a/app/port/controller/ProxyCacheController.ts b/app/port/controller/ProxyCacheController.ts index db4dd3c4..1dbc9d1c 100644 --- a/app/port/controller/ProxyCacheController.ts +++ b/app/port/controller/ProxyCacheController.ts @@ -8,7 +8,7 @@ import { Context, EggContext, } from '@eggjs/tegg'; -import { ForbiddenError, NotFoundError, UnauthorizedError } from 'egg-errors'; +import { ForbiddenError, NotFoundError, UnauthorizedError, NotImplementedError } from 'egg-errors'; import { AbstractController } from './AbstractController'; import { ProxyCacheRepository } from '../../repository/ProxyCacheRepository'; import { Static } from 'egg-typebox-validate/typebox'; @@ -18,8 +18,8 @@ import { ProxyCacheService, isPkgManifest, } from '../../core/service/ProxyCacheService'; -import { SyncMode, PROXY_CACHE_DIR_NAME } from '../../common/constants'; -import { NFSAdapter } from '../../common/adapter/NFSAdapter'; +import { SyncMode } from '../../common/constants'; +import { CacheService } from '../../core/service/CacheService'; @HTTPController() export class ProxyCacheController extends AbstractController { @@ -28,7 +28,7 @@ export class ProxyCacheController extends AbstractController { @Inject() private readonly proxyCacheService: ProxyCacheService; @Inject() - private readonly nfsAdapter: NFSAdapter; + private readonly cacheService: CacheService; @HTTPMethod({ method: HTTPMethodEnum.GET, @@ -77,11 +77,12 @@ export class ProxyCacheController extends AbstractController { if (refreshList.length === 0) { throw new NotFoundError(); } + await this.cacheService.removeCache(fullname); const taskList = refreshList - // 仅manifests需要更新，指定版本的package.json文件发布后不会改变 + // only refresh package.json and abbreviated.json .filter(i => isPkgManifest(i.fileType)) - .map(async item => { - const task = await this.proxyCacheService.createTask( + .map(item => { + const task = this.proxyCacheService.createTask( `${item.fullname}/${item.fileType}`, { fullname: item.fullname, @@ -90,9 +91,10 @@ export class ProxyCacheController extends AbstractController { ); return task; }); + const tasks = await Promise.all(taskList); return { ok: true, - tasks: await Promise.all(taskList), + tasks, }; } @@ -100,12 +102,7 @@ export class ProxyCacheController extends AbstractController { method: HTTPMethodEnum.DELETE, path: `/-/proxy-cache/:fullname(${FULLNAME_REG_STRING})`, }) - async removeProxyCaches(@Context() ctx: EggContext, @HTTPParam() fullname: string) { - const isAdmin = await this.userRoleManager.isAdmin(ctx); - if (!isAdmin) { - throw new UnauthorizedError('only admin can do this'); - } - + async removeProxyCaches(@HTTPParam() fullname: string) { if (this.config.cnpmcore.syncMode !== SyncMode.proxy) { throw new ForbiddenError('proxy mode is not enabled'); } @@ -116,6 +113,7 @@ export class ProxyCacheController extends AbstractController { if (proxyCachesList.length === 0) { throw new NotFoundError(); } + await this.cacheService.removeCache(fullname); const removingList = proxyCachesList.map(item => { return this.proxyCacheService.removeProxyCache(item.fullname, item.fileType, item.version); }); @@ -140,17 +138,6 @@ export class ProxyCacheController extends AbstractController { throw new ForbiddenError('proxy mode is not enabled'); } - await this.proxyCacheRepository.truncateProxyCache(); - // 尝试删除proxy cache目录，若失败可手动管理 - ctx.runInBackground(async () => { - try { - await this.nfsAdapter.remove(`/${PROXY_CACHE_DIR_NAME}`); - } catch (err) { - this.logger.error('[ProxyCacheService.truncateProxyCaches] remove proxy cache dir error: %s', err); - } - }); - return { - ok: true, - }; + throw new NotImplementedError('not implemented yet'); } } diff --git a/app/port/controller/package/ShowPackageController.ts b/app/port/controller/package/ShowPackageController.ts index 75f4ba5e..d77db07d 100644 --- a/app/port/controller/package/ShowPackageController.ts +++ b/app/port/controller/package/ShowPackageController.ts @@ -72,7 +72,8 @@ export class ShowPackageController extends AbstractController { if (this.config.cnpmcore.syncMode === SyncMode.proxy) { // proxy mode const fileType = isFullManifests ? DIST_NAMES.FULL_MANIFESTS : DIST_NAMES.ABBREVIATED_MANIFESTS; - const pkgManifest = await this.proxyCacheService.getPackageManifest(fullname, fileType); + const { data: sourceManifest } = await this.proxyCacheService.getProxyResponse(ctx, { dataType: 'json' }); + const pkgManifest = this.proxyCacheService.replaceTarballUrl(sourceManifest, fileType); const nfsBytes = Buffer.from(JSON.stringify(pkgManifest)); const { shasum: etag } = await calculateIntegrity(nfsBytes); diff --git a/test/core/service/ProxyCacheService.test.ts b/test/core/service/ProxyCacheService.test.ts index 721cd8d8..76b3436c 100644 --- a/test/core/service/ProxyCacheService.test.ts +++ b/test/core/service/ProxyCacheService.test.ts @@ -127,92 +127,6 @@ describe('test/core/service/ProxyCacheService/index.test.ts', () => { }); }); - describe('getRewrittenManifest()', () => { - it('should get full package manifest', async () => { - const data = await TestUtil.readJSONFile( - TestUtil.getFixtures('registry.npmjs.org/foobar.json'), - ); - mock(proxyCacheService, 'getUpstreamFullManifests', async () => { - return { - status: 200, - data, - }; - }); - const manifest = await proxyCacheService.getRewrittenManifest( - 'foobar', - DIST_NAMES.FULL_MANIFESTS, - ); - const versionArr = Object.values(manifest.versions); - for (const i of versionArr) { - assert(i.dist.tarball.includes('http://localhost:7001')); - } - }); - - it('should get abbreviated package manifest', async () => { - const data = await TestUtil.readJSONFile( - TestUtil.getFixtures('registry.npmjs.org/abbreviated_foobar.json'), - ); - mock(proxyCacheService, 'getUpstreamAbbreviatedManifests', async () => { - return { - status: 200, - data, - }; - }); - const manifest = await proxyCacheService.getRewrittenManifest( - 'foobar', - DIST_NAMES.ABBREVIATED_MANIFESTS, - ); - const versionArr = Object.values(manifest.versions); - for (const i of versionArr) { - assert(i.dist.tarball.includes('http://localhost:7001')); - } - }); - - it('should get full package version manifest', async () => { - const data = await TestUtil.readJSONFile( - TestUtil.getFixtures('registry.npmjs.org/foobar/1.0.0/package.json'), - ); - mock(proxyCacheService, 'getUpstreamPackageVersionManifest', async () => { - return { - status: 200, - data, - }; - }); - const manifest = await proxyCacheService.getRewrittenManifest( - 'foobar', - DIST_NAMES.MANIFEST, - '1.0.0', - ); - assert(manifest.dist); - assert(manifest.dist.tarball.includes('http://localhost:7001')); - }); - - it('should get abbreviated package version manifest', async () => { - const data = await TestUtil.readJSONFile( - TestUtil.getFixtures( - 'registry.npmjs.org/foobar/1.0.0/abbreviated.json', - ), - ); - mock( - proxyCacheService, - 'getUpstreamAbbreviatedPackageVersionManifest', - async () => { - return { - status: 200, - data, - }; - }, - ); - const manifest = await proxyCacheService.getRewrittenManifest( - 'foobar', - DIST_NAMES.ABBREVIATED, - '1.0.0', - ); - assert(manifest.dist); - assert(manifest.dist.tarball.includes('http://localhost:7001')); - }); - }); - describe('removeProxyCache()', () => { it('should remove cache', async () => { await proxyCacheRepository.saveProxyCache( @@ -300,7 +214,7 @@ describe('test/core/service/ProxyCacheService/index.test.ts', () => { fileType: DIST_NAMES.FULL_MANIFESTS, }, ); - mock(proxyCacheService, 'getUpstreamFullManifests', async () => { + mock(proxyCacheService, 'getPackageVersionManifest', async () => { return { status: 200, data: await TestUtil.readJSONFile( diff --git a/test/port/controller/ProxyCacheController/index.test.ts b/test/port/controller/ProxyCacheController/index.test.ts index ab3b58ed..0406cae8 100644 --- a/test/port/controller/ProxyCacheController/index.test.ts +++ b/test/port/controller/ProxyCacheController/index.test.ts @@ -9,8 +9,6 @@ import { SyncMode } from '../../../../app/common/constants'; import { TestUtil } from '../../../TestUtil'; describe('test/port/controller/PackageVersionFileController/listFiles.test.ts', () => { - // let publisher; - // let adminUser; let proxyCacheRepository: ProxyCacheRepository; beforeEach(async () => { proxyCacheRepository = await app.getEggObject(ProxyCacheRepository); @@ -125,12 +123,6 @@ describe('test/port/controller/PackageVersionFileController/listFiles.test.ts', .expect(403); }); - it('should 403 when not login', async () => { - mock(app.config.cnpmcore, 'syncMode', SyncMode.proxy); - mock(app.config.cnpmcore, 'redirectNotFound', false); - await app.httpRequest().delete('/-/proxy-cache/foo-bar').expect(401); - }); - it('should delete all packages about "foo-bar".', async () => { mock(app.config.cnpmcore, 'syncMode', SyncMode.proxy); mock(app.config.cnpmcore, 'redirectNotFound', false); @@ -141,8 +133,10 @@ describe('test/port/controller/PackageVersionFileController/listFiles.test.ts', .set('authorization', adminUser.authorization) .expect(200); assert(res.body.ok === true); + // foo-bar assert(res.body.result.length === 2); const res1 = await app.httpRequest().get('/-/proxy-cache').expect(200); + // foobar assert(res1.body.data.length === 2); }); @@ -178,14 +172,11 @@ describe('test/port/controller/PackageVersionFileController/listFiles.test.ts', mock(app.config.cnpmcore, 'syncMode', SyncMode.proxy); mock(app.config.cnpmcore, 'redirectNotFound', false); const adminUser = await TestUtil.createAdmin(); - const res = await app + await app .httpRequest() .delete('/-/proxy-cache') .set('authorization', adminUser.authorization) - .expect(200); - assert(res.body.ok === true); - const res1 = await app.httpRequest().get('/-/proxy-cache').expect(200); - assert(res1.body.data.length === 0); + .expect(501); }); }); });