feat: add unpkg-white-list to detect sync unpkg files or not #686
Conversation
WalkthroughThe changes enhance the package version file synchronization process by introducing a whitelist mechanism for unpkg file synchronization, improving logging, and adding new configuration options. These modifications provide better control over the packages allowed for unpkg synchronization and enhance code readability and maintainability. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| @@ -54,6 +54,7 @@ export const cnpmcoreConfig: CnpmcoreConfig = { | |||
| redirectNotFound: true, | |||
| enableUnpkg: true, | |||
| enableSyncUnpkgFiles: true, | |||
| enableSyncUnpkgFilesWhiteList: false, | |||
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #686 +/- ##
==========================================
- Coverage 96.89% 96.81% -0.08%
==========================================
Files 181 181
Lines 17902 17982 +80
Branches 2325 2348 +23
==========================================
+ Hits 17346 17410 +64
- Misses 556 572 +16 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (2)
app.ts (1)
5-5: Add documentation for the newbinaryHTMLproperty.It's good practice to document new properties, especially when they are public or part of an interface. This helps other developers understand the purpose and usage of the property.
app/port/config.ts (1)
152-155: Document the new configuration propertyenableSyncUnpkgFilesWhiteList.Adding documentation for new configuration properties helps maintainers and users understand their purpose and impact on the system.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- app.ts (1 hunks)
- app/core/event/SyncPackageVersionFile.ts (2 hunks)
- app/core/service/PackageVersionFileService.ts (4 hunks)
- app/port/config.ts (1 hunks)
- app/port/controller/PackageVersionFileController.ts (1 hunks)
- app/port/controller/package/SavePackageVersionController.ts (2 hunks)
- config/config.default.ts (1 hunks)
- test/port/controller/PackageVersionFileController/listFiles.test.ts (3 hunks)
Additional comments not posted (10)
app.ts (1)
5-5: Ensure the initialization ofbinaryHTMLin the constructor aligns with its intended use.Verification successful
Ensure Proper Initialization and Usage of
binaryHTMLThe initialization and usage of
binaryHTMLin the codebase have been verified:
Declaration and Initialization:
binaryHTMLis declared in theApplicationinterface and initialized in the constructor ofCnpmcoreAppHookinapp.ts.- It is initially set to an empty string in the constructor.
Usage:
binaryHTMLis assigned a value in thedidReadymethod ofCnpmcoreAppHook, where it reads and processes the content ofbinary.html.- It is used in
BinarySyncController.tsto serve the processed HTML content.The initialization aligns with its intended use, ensuring that
binaryHTMLis properly set up before it is used.
- Locations:
app.ts: Initialization and assignment.app/port/controller/BinarySyncController.ts: Usage in theshowBinaryHTMLmethod.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `binaryHTML` is used correctly throughout the application. # Test: Search for usages of `binaryHTML`. Expect: Proper usage in context. rg --type typescript $'binaryHTML'Length of output: 68
Script:
#!/bin/bash # Description: Verify that `binaryHTML` is used correctly throughout the application. # Test: Search for usages of `binaryHTML`. Expect: Proper usage in context. rg --type ts 'binaryHTML'Length of output: 266
Script:
#!/bin/bash # Extract relevant lines from app.ts and BinarySyncController.ts for detailed inspection # Extract lines from app.ts rg --type ts --context 5 'binaryHTML' app.ts # Extract lines from BinarySyncController.ts rg --type ts --context 5 'binaryHTML' app/port/controller/BinarySyncController.tsLength of output: 1273
app/core/event/SyncPackageVersionFile.ts (1)
5-5: Confirm the necessity of importingForbiddenError.Verification successful
ForbiddenErroris used in the fileThe
ForbiddenErrorimport is indeed used within theSyncPackageVersionFile.tsfile.
ForbiddenErroris imported fromegg-errors.- It is used in a conditional check:
if (err instanceof ForbiddenError).This confirms the necessity of importing
ForbiddenError.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `ForbiddenError` is used in the file. # Test: Search for usages of `ForbiddenError`. Expect: At least one usage. rg --type typescript $'ForbiddenError' app/core/event/SyncPackageVersionFile.tsLength of output: 113
Script:
#!/bin/bash # Description: Verify that `ForbiddenError` is used in the file. # Test: Search for usages of `ForbiddenError`. Expect: At least one usage. rg --type ts $'ForbiddenError' app/core/event/SyncPackageVersionFile.tsLength of output: 158
config/config.default.ts (1)
57-57: Ensure the default value ofenableSyncUnpkgFilesWhiteListaligns with expected system behavior.app/port/controller/PackageVersionFileController.ts (1)
Line range hint
5-5: Review the simplified conditional logic for file redirection to ensure it handles all necessary cases.Verification successful
The search results indicate two instances of
ctx.redirectin thePackageVersionFileController.tsfile:
ctx.redirect(join(ctx.path, indexFile));ctx.redirect(route);Additionally, there is a comment explaining the redirection logic for directories with
index.jsfiles.Analysis:
- The first redirection handles cases where the path is joined with an
indexFile.- The second redirection handles a generic route redirection.
- The comment suggests that directories with
index.jsfiles are redirected appropriately.Conclusion:
The redirection logic appears to cover the necessary cases, including handling directories with
index.jsfiles and generic route redirections.Recommendation:
No further action is required as the redirection logic seems comprehensive.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new file redirection logic is comprehensive and does not omit important cases. # Test: Search for the redirection logic in the controller. Expect: Comprehensive handling of cases. rg --type typescript $'redirect' app/port/controller/PackageVersionFileController.tsLength of output: 118
Script:
#!/bin/bash # Description: Verify that the new file redirection logic is comprehensive and does not omit important cases. # Test: Search for the redirection logic in the controller. Expect: Comprehensive handling of cases. rg --type ts $'redirect' app/port/controller/PackageVersionFileController.tsLength of output: 293
app/port/controller/package/SavePackageVersionController.ts (1)
Line range hint
224-244: Validate the usage of thelockNameconstant in logging statements.Verification successful
lockNameis used correctly in logging statementsThe
lockNameconstant is correctly used in the logging statement within theSavePackageVersionController.tsfile.
- Line 224:
const lockName =${pkg.name}:publish;- Line 227:
this.logger.warn('[package:version:add] check lock:%s fail', lockName);Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `lockName` is used correctly in logging statements. # Test: Search for usages of `lockName` in logging. Expect: Correct and consistent usage. rg --type typescript $'lockName' app/port/controller/package/SavePackageVersionController.tsLength of output: 126
Script:
#!/bin/bash # Description: Verify that `lockName` is used correctly in logging statements. # Test: Search for usages of `lockName` in logging. Expect: Correct and consistent usage. rg --type ts 'lockName' app/port/controller/package/SavePackageVersionController.tsLength of output: 205
app/core/service/PackageVersionFileService.ts (1)
Line range hint
49-102: Ensure the new private fields and methods for handling theunpkg-white-listare secure and performant.Verification successful
The search results indicate that the
unpkgWhiteListrelated fields and methods are used for access control and synchronization withinPackageVersionFileService.ts. The methods include checks for the current version, updating the version, and validating packages and scopes against the whitelist. These operations appear to be secure and performant, with proper error handling and efficient data access patterns.
- The
#unpkgWhiteListCurrentVersionis checked and updated to ensure synchronization.- The
#unpkgWhiteListAllowPackagesand#unpkgWhiteListAllowScopesare used to validate access, with appropriate error handling usingForbiddenError.Based on the analysis, the implementation of the
unpkgWhiteListrelated methods and fields appears to be secure and performant.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the security and performance of the new `unpkg-white-list` related methods. # Test: Search for usages of the new methods and fields. Expect: Secure and efficient handling. rg --type typescript $'unpkgWhiteList' app/core/service/PackageVersionFileService.tsLength of output: 118
Script:
#!/bin/bash # Description: Verify the security and performance of the new `unpkg-white-list` related methods. # Test: Search for usages of the new methods and fields. Expect: Secure and efficient handling. rg --type ts $'unpkgWhiteList' app/core/service/PackageVersionFileService.tsLength of output: 1060
test/port/controller/PackageVersionFileController/listFiles.test.ts (4)
403-427: LGTM! This test case correctly handles the scenario where a package name is not in the white list, returning the expected 403 status and error message.
429-470: LGTM! This test case correctly handles the scenario where a package version does not match the white list, returning the expected 403 status and error message.
472-509: LGTM! This test case correctly handles the scenario where a scope is in the white list, returning the expected 200 status.
511-613: LGTM! This test case correctly handles the scenario where a package version is in the white list, returning the expected 200 status.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/core/event/SyncPackageVersionFile.ts (3 hunks)
- app/core/service/PackageVersionFileService.ts (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/core/event/SyncPackageVersionFile.ts
- app/core/service/PackageVersionFileService.ts
[skip ci] ## [3.59.0](v3.58.1...v3.59.0) (2024-05-18) ### Features * add unpkg-white-list to detect sync unpkg files or not ([#686](#686)) ([0530116](0530116))
see https://github.com/cnpm/unpkg-white-list
Summary by CodeRabbit
New Features
enableSyncUnpkgFilesWhiteListto enhance package version file synchronization.Improvements
Tests
enableSyncUnpkgFilesWhiteListconfiguration to ensure reliability.