fix: 修复代理模式下请求头没有正确携带的问题 #719
Conversation
WalkthroughThe changes in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #719 +/- ##
=======================================
Coverage 96.46% 96.47%
=======================================
Files 188 188
Lines 18803 18841 +38
Branches 2463 2465 +2
=======================================
+ Hits 18139 18176 +37
- Misses 664 665 +1 ☔ View full report in Codecov by Sentry. |
|
Hey @hezhengxu2018, here is an example of how you can ask me to improve this pull request: @Sweep Add unit tests for the new `replaceTarballUrl` method in ProxyCacheService that verify: 📖 For more information on how to use Sweep, please read our documentation. |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
test/port/controller/ProxyCacheController/index.test.ts (1)
175-179: Update the test description to match the new behavior.The test description suggests it "should delete all packages" but the implementation now expects a 501 Not Implemented response. Consider renaming the test to something like "should return not implemented status" to better reflect the current behavior.
- it('should delete all packages about "foo-bar".', async () => { + it('should return not implemented status for truncate operation', async () => {test/core/service/ProxyCacheService.test.ts (1)
Line range hint
1-238: Add tests for header transmission in proxy mode.Given that the main PR objective is to fix header transmission issues in proxy mode, we should add test cases to verify this functionality.
Would you like me to help generate test cases that verify:
- Correct transmission of headers like "user-agent" and "x-forwarded"
- Error scenarios when headers are missing or malformed
- Integration tests for proxy mode with headers
Example test structure:
describe('proxy mode header handling', () => { it('should correctly transmit user-agent header', async () => { // Test implementation }); it('should correctly transmit x-forwarded header', async () => { // Test implementation }); it('should handle missing headers gracefully', async () => { // Test implementation }); });app/core/service/ProxyCacheService.ts (2)
64-74: Consider adding error logging before cache cleanup.The error handling for NFS operations is good, but consider logging the error before cleanup for better debugging.
} catch (error) { + this.logger.error('[ProxyCacheService] Failed to read manifest from NFS: %s', error); await this.nfsAdapter.remove(cachedStoreKey); await this.proxyCacheRepository.removeProxyCache(fullname, fileType); throw error; }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 74-74: app/core/service/ProxyCacheService.ts#L74
Added line #L74 was not covered by tests
Line range hint
235-258: Header forwarding implementation looks good.The changes correctly fix the header forwarding issue mentioned in the PR objectives. Consider adding a type for expected headers.
interface ProxyHeaders { accept?: string; 'user-agent'?: string; 'x-forwarded-for'?: string; }app/port/controller/ProxyCacheController.ts (1)
Line range hint
105-119: Security Concern: Removing Authorization Check fromremoveProxyCachesThe removal of the
@Context()parameter and the associated admin check allows any authenticated user to delete proxy caches for a package. This could lead to unauthorized cache deletions, impacting other users and potentially degrading system performance or reliability. Consider reinstating the authorization check to ensure that only authorized users (e.g., admins) can perform this operation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- app/core/service/ProxyCacheService.ts (6 hunks)
- app/port/controller/ProxyCacheController.ts (7 hunks)
- app/port/controller/package/ShowPackageController.ts (1 hunks)
- test/core/service/ProxyCacheService.test.ts (1 hunks)
- test/port/controller/ProxyCacheController/index.test.ts (2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/core/service/ProxyCacheService.ts
[warning] 74-74: app/core/service/ProxyCacheService.ts#L74
Added line #L74 was not covered by tests
[warning] 107-107: app/core/service/ProxyCacheService.ts#L107
Added line #L107 was not covered by tests
🔇 Additional comments (5)
test/port/controller/ProxyCacheController/index.test.ts (1)
136-141: LGTM! Clear and accurate test assertions.The assertions correctly verify both the deletion result and the remaining cache state, with helpful comments distinguishing between "foo-bar" and "foobar" related counts.
test/core/service/ProxyCacheService.test.ts (2)
Line range hint
217-225: Verify test coverage after mocking changes.The change from
getUpstreamFullManifeststogetPackageVersionManifestreflects architectural changes in how manifests are retrieved. However, we should ensure that the manifest retrieval functionality is still thoroughly tested.Let's verify the test coverage:
#!/bin/bash # Description: Check test coverage for manifest retrieval methods # Look for other tests that might cover the getUpstreamFullManifests functionality # Search for tests of getUpstreamFullManifests echo "Searching for getUpstreamFullManifests tests..." rg -l "getUpstreamFullManifests" "test/" # Search for additional tests of getPackageVersionManifest echo "Searching for getPackageVersionManifest tests..." rg -l "getPackageVersionManifest" "test/"
Line range hint
1-238: Verify coverage for removed getRewrittenManifest tests.The AI summary indicates that the entire
getRewrittenManifest()test suite was removed. Since this method is now private, we need to ensure its functionality is still indirectly tested through public methods.Let's verify the indirect test coverage:
✅ Verification successful
The getRewrittenManifest functionality is well covered through public methods
The search results show that
getRewrittenManifestis still thoroughly tested through its public interfaces:
- It's tested through
getPackageManifest()with both success and cache scenarios- It's tested through
getPackageVersionManifest()with both direct version and tag resolution- The implementation remains in
ProxyCacheService.tsas a private method, and its functionality is verified through these public method testsThe removal of direct
getRewrittenManifesttests is appropriate since it's now a private implementation detail, and its behavior is properly verified through the public interface tests that remain in the file.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if getRewrittenManifest functionality is still tested # Look for tests that might indirectly test this functionality # Search for the implementation of getRewrittenManifest echo "Searching for getRewrittenManifest implementation..." ast-grep --pattern 'getRewrittenManifest($$$) { $$$ }' # Search for tests that might cover this functionality echo "Searching for potential indirect tests..." rg -l "manifest.*rewrite|rewrite.*manifest" "test/"Length of output: 318
Script:
#!/bin/bash # Let's try a different approach to find the implementation and test coverage # 1. Find the implementation of getRewrittenManifest with more context echo "Searching for getRewrittenManifest implementation..." rg -A 5 "getRewrittenManifest" app/ # 2. Look for test files that mock or use this method echo "Searching for test files using getRewrittenManifest..." rg -A 5 "getRewrittenManifest" test/ # 3. Look for related test files that might cover manifest rewriting echo "Searching for related manifest tests..." rg -l "getPackageManifest|getPackageVersionManifest" test/Length of output: 5183
app/core/service/ProxyCacheService.ts (1)
126-145: LGTM! Well-structured URL rewriting logic.Clean implementation with good type safety and clear separation between package and version manifest handling.
app/port/controller/package/ShowPackageController.ts (1)
75-76: Verify that request headers are correctly forwarded in proxy modeThese changes aim to fix the issue where request headers were not properly carried in proxy mode. Please ensure that
getProxyResponsecorrectly forwards the request headers fromctxto the upstream request.Run the following script to verify that
getProxyResponseusesctx.request.headerswhen making the upstream request:✅ Verification successful
Based on the code found, I can now generate the final response:
Headers are properly forwarded in proxy mode, but with specific security considerations
Looking at the implementation of
getProxyResponsein ProxyCacheService, the service selectively forwards essential headers:
acceptheader for content negotiationuser-agentfor client identification- Adds secure proxy headers like
x-forwarded-forandvia- Includes proper authorization headers for upstream registry
The implementation follows security best practices by not blindly forwarding all headers but instead explicitly choosing which headers to forward.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `getProxyResponse` forwards headers from `ctx.request` to the upstream request. # Test: Search for the usage of `ctx.request.headers` in the `getProxyResponse` method. Expect to find forward of headers. rg -A 10 -g '*.ts' 'async getProxyResponse' | rg 'ctx\.request\.headers'Length of output: 74
Script:
#!/bin/bash # Let's first find the implementation of getProxyResponse method ast-grep --pattern 'async getProxyResponse($$$) { $$$ }' # Also search for any proxy-related header forwarding in the codebase rg -g '*.ts' -B 2 -A 5 'headers.*proxy|proxy.*headers'Length of output: 998
Script:
#!/bin/bash # Let's find the implementation of ProxyCacheService to see how it handles headers fd -e ts "ProxyCacheService" --exec cat {} # Also look for any proxy configuration or header handling in the service rg -g '*.ts' -B 3 -A 10 'class ProxyCacheService'Length of output: 21015
| // only used by schedule task | ||
| private async getRewrittenManifest<T extends DIST_NAMES>(fullname:string, fileType: T, versionOrTag?:string): Promise<GetSourceManifestAndCacheReturnType<T>> { | ||
| 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; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting common URL pattern.
The URL construction is similar across cases. Consider extracting the pattern.
+ private buildProxyUrl(fullname: string, versionOrTag?: string): string {
+ const base = `/${encodeURIComponent(fullname)}`;
+ return versionOrTag
+ ? `${base}/${encodeURIComponent(versionOrTag)}`
+ : `${base}?t=${Date.now()}&cache=0`;
+ }Committable suggestion was skipped due to low confidence.
| try { | ||
| const nfsBytes = await this.nfsAdapter.getBytes(cachedStoreKey); | ||
| const nfsString = Buffer.from(nfsBytes!).toString(); | ||
| return JSON.parse(nfsString) as PackageJSONType | AbbreviatedPackageJSONType; | ||
| } catch (error) { | ||
| /* c8 ignore next 3 */ | ||
| await this.nfsAdapter.remove(cachedStoreKey); | ||
| await this.proxyCacheRepository.removeProxyCache(fullname, fileType); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting common error handling logic.
This error handling block is duplicated from getPackageManifest. Consider extracting it into a private method.
+ private async handleNFSError(cachedStoreKey: string, fullname: string, fileType: DIST_NAMES) {
+ this.logger.error('[ProxyCacheService] Failed to read manifest from NFS: %s', error);
+ await this.nfsAdapter.remove(cachedStoreKey);
+ await this.proxyCacheRepository.removeProxyCache(fullname, fileType);
+ throw error;
+ }Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 107-107: app/core/service/ProxyCacheService.ts#L107
Added line #L107 was not covered by tests
| const nfsPkgManifgest = JSON.parse(nfsString); | ||
| return nfsPkgManifgest; | ||
| } catch (error) { | ||
| /* c8 ignore next 3 */ |
There was a problem hiding this comment.
看逻辑如果 getBytes 抛出异常，会直接清空之前存储的信息？
用其他办法来做数据清理？
There was a problem hiding this comment.
json信息解析不出来，说明存入的信息就有错误或者文件在nfs上已经删掉了。
每天凌晨三点会去刷新一下已经缓存的依赖，也算是一种清理吧，这里删除是防止依赖还没刷新但是读取已经异常了，清除了就会再次请求上游缓存一遍，不用等每天的刷新。
| @@ -90,22 +91,18 @@ export class ProxyCacheController extends AbstractController { | |||
| ); | |||
| return task; | |||
| }); | |||
| const tasks = await Promise.all(taskList); | |||
There was a problem hiding this comment.
这个接口会查询这个依赖所有缓存的json文件，但是只有manifest 和 abbrivated_manifest会更新，因为具体版本的包发布之后就不会再更改（正常情况下），所以创建的任务数量最多也只有2个。不需要特别控制并发
| const nfsPkgManifgest = JSON.parse(nfsString); | ||
| return nfsPkgManifgest; | ||
| } catch (error) { | ||
| /* c8 ignore next 3 */ |
proxy时因为一个低级的拼写错误没有正确的携带请求头，导致代理模式时返回的数据不正确。但是现在用户发起的请求中的user-agent和x-forwarded等头部信息也没有正确的携带。虽然影响不大但还是想和跑批时更新的请求做一下区分。
Summary by CodeRabbit
Bug Fixes
Improvements