feat: support cjs and esm both by tshy #54
Conversation
BREAKING CHANGE: drop Node.js < 18.19.0 support part of eggjs/egg#3644
WalkthroughThe updates encompass various improvements and refinements to a project. Major changes include switching ESLint configurations to support TypeScript, updating Node.js versions in CI workflows, revising the gitignore patterns, rebranding the package name, enhancing cookies handling, and refactoring with TypeScript. New TypeScript configuration files, error classes, and functionalities for encryption and decryption have been added, along with comprehensive unit tests for the new features. Changes
Sequence Diagram(s)sequenceDiagram
participant UserAgent as User Agent
participant CookieManager as Cookie Manager
participant EncryptionModule as Encryption Module
participant VerificationModule as Verification Module
UserAgent->>CookieManager: Request with cookies
CookieManager->>EncryptionModule: Encrypt cookie data
EncryptionModule-->>CookieManager: Return encrypted data
CookieManager-->>UserAgent: Respond with Set-Cookie header
UserAgent->>CookieManager: Request with encrypted cookie
CookieManager->>VerificationModule: Verify cookie
VerificationModule-->>CookieManager: Return verification status
CookieManager-->>UserAgent: Respond with verified data
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@types/node@10.17.60, npm/egg-bin@1.11.1, npm/eslint-config-egg@8.1.2, npm/eslint@7.32.0, npm/git-contributor@2.1.5, npm/scmp@2.1.0, npm/ts-node@7.0.1, npm/typescript@3.9.10, npm/utility@1.18.0 |
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring: Next stepsTake a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #54 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 4 5 +1
Lines 261 610 +349
Branches 70 131 +61
==========================================
+ Hits 261 610 +349 ☔ View full report in Codecov by Sentry. |
|
@SocketSecurity ignore npm/egg-bin@6.10.0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (3)
test/keygrip.test.ts (1)
45-61: Verify backward compatibility of encryption methods.The test "should decrypt key encrypted by createCipher without error" checks for backward compatibility with deprecated
crypto.createCipher. Ensure that the deprecation is documented and consider usingcreateCipherivfor new implementations.test/cookie.test.ts (1)
Line range hint
68-83: Handling ofmaxAgeandexpiresattributes needs clarification.The tests for
maxAgeandexpiresprovide good coverage but could be clearer in documenting the precedence and interaction between these two attributes. Consider adding comments or documentation in the test to clarify this behavior for future maintainers.src/cookies.ts (1)
Line range hint
32-217: Comprehensive review of the Cookies class.The
Cookiesclass is robust and handles a variety of cookie operations with attention to security and functionality. However, consider the following improvements:
- The handling of secure cookies and the integration with the
Keygripclass for signing and encryption are well-implemented.- The method
_getcould be refactored for clarity and efficiency.- The handling of
SameSiteandpartitionedattributes in thesetmethod is complex and could benefit from further documentation or simplification.Overall, the class functions well but could be improved in terms of readability and maintainability.
- if (opts.sameSite && typeof opts.sameSite === 'string' && opts.sameSite.toLowerCase() === 'none') { + // Refactor this block for clarity and add documentation
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (18)
- .eslintrc (1 hunks)
- .github/workflows/nodejs.yml (2 hunks)
- .gitignore (1 hunks)
- README.md (3 hunks)
- README.zh-CN.md (2 hunks)
- benchmark/index.cjs (2 hunks)
- package.json (1 hunks)
- src/cookie.ts (1 hunks)
- src/cookies.ts (8 hunks)
- src/error.ts (1 hunks)
- src/index.ts (1 hunks)
- src/keygrip.ts (1 hunks)
- test/cookie.test.ts (6 hunks)
- test/cookies.test.ts (30 hunks)
- test/cookies.ts (1 hunks)
- test/helper.ts (1 hunks)
- test/keygrip.test.ts (1 hunks)
- tsconfig.json (1 hunks)
Files skipped from review due to trivial changes (6)
- .github/workflows/nodejs.yml
- .gitignore
- README.zh-CN.md
- benchmark/index.cjs
- src/index.ts
- tsconfig.json
Additional context used
LanguageTool
README.md
[uncategorized] ~21-~21: This verb does not appear to agree with the subject. Consider using a different form. (AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
Context: ...l be encrypted base on keys. Anyone who don't have the keys are unable to know the...
[uncategorized] ~21-~21: This verb does not appear to agree with the subject. Consider using a different form. (AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
Context: ...on keys. Anyone who don't have the keys are unable to know the original cookie's va...
[uncategorized] ~36-~36: The preposition “on” seems more likely in this position than the preposition “in”. (AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_ON)
Context: ...heck [Browsers all had some limitation in cookie's length](http://browsercookieli...
Biome
src/cookie.ts
[error] 10-10: Unexpected control character(s) in regular expression: \u0009 (lint/suspicious/noControlCharactersInRegex)
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
Additional comments not posted (19)
.eslintrc (1)
2-5: Updated ESLint configuration for TypeScript and node prefix enforcement.The changes in the ESLint configuration extend support for TypeScript-specific linting and enforce node prefix rules, aligning with the project's adoption of TypeScript and stricter code quality standards.
src/error.ts (1)
1-6: IntroducedCookieErrorclass extendingError.The new
CookieErrorclass provides a structured way to handle cookie-specific errors, enhancing error management in the library. The implementation correctly sets the error name to the class name, which is a good practice for error handling.test/helper.ts (1)
1-9: Utility function to get test fixtures path.The function
getFixturesuses modern ES modules syntax (node:pathandnode:url) and properly calculates the path to the fixtures directory. This is a useful utility for accessing test files, improving maintainability and readability of tests.test/cookies.ts (1)
1-35: RefactoredcreateCookiefunction to support dynamic cookie creation.This function efficiently sets up a cookie context and allows for flexible cookie creation with customizable security and key options. The use of modern JavaScript features like default parameters and spread operators enhances code clarity and flexibility.
package.json (1)
2-71: Comprehensive updates topackage.jsonfor project reconfiguration.The changes reflect the transition to a scoped package name (
@eggjs/cookies), update dependencies, and introduce new scripts and configurations for TypeScript and publishing. The explicit support for Node.js version>= 18.19.0aligns with the PR's objectives to drop support for older Node.js versions.README.md (1)
Line range hint
1-58: Documentation updates to reflect new package scope and features.The documentation has been thoroughly updated to reflect the change to a scoped package name and the introduction of new features like encrypted cookies. The examples and links have been correctly updated, which is crucial for user guidance and SEO.
Tools
LanguageTool
[uncategorized] ~21-~21: This verb does not appear to agree with the subject. Consider using a different form. (AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
Context: ...l be encrypted base on keys. Anyone who don't have the keys are unable to know the...
[uncategorized] ~21-~21: This verb does not appear to agree with the subject. Consider using a different form. (AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
Context: ...on keys. Anyone who don't have the keys are unable to know the original cookie's va...test/keygrip.test.ts (2)
22-30: Encryption and decryption tests are well-implemented.The tests for encryption and decryption correctly handle multiple keys and verify that the encrypted data can be decrypted correctly. This is a good practice for ensuring the robustness of cryptographic functions.
63-69: Signing and verification functionality is correctly tested.Tests for signing and verifying data ensure that signatures are correctly verified across different keys. This is crucial for security implementations.
src/cookie.ts (1)
78-146: Review of the Cookie class.The
Cookieclass is well-implemented with comprehensive attribute handling and validation. However, consider the following improvements:
- The use of regular expressions for validating
name,value,path, anddomainenhances security by preventing injection attacks.- The
toHeadermethod constructs the cookie header string correctly, but be cautious with the handling of potentially null attributes.Overall, the class is robust and adheres to good security practices.
test/cookie.test.ts (4)
6-9: Tests for invalid cookie attributes are correctly implemented.The tests ensure that invalid characters in cookie names, values, paths, and domains throw the appropriate errors. This is crucial for maintaining the integrity of cookie handling.
13-13: Test for setting expires attribute correctly handles null values.The test ensures that the
expiresattribute is set to 0 when the cookie value is not provided, which is a good practice for cookie expiration management.
23-30: Header construction tests are comprehensive.The tests for the
toHeadermethod cover various scenarios, including secure cookies and cookies with different attributes. This comprehensive testing ensures that the header string is constructed correctly under different conditions.
43-54: Test for conditional attribute inclusion in headers.The tests for not including
pathandhttponlywhen they are set to null or false are well-implemented. This ensures that the cookie header conforms to specified attributes.test/cookies.test.ts (6)
5-12: Test case correctly simulates encryption error when keys are not present.The test appropriately throws and catches an error when trying to encrypt without keys, validating the error message correctly.
31-36: Test case correctly checks for keygrip caching.The use of
assert.equalto verify that the same keygrip instance is used across multiple calls is a good practice for ensuring performance optimizations are effective.
Line range hint
36-50: Test case correctly handles failed encryption scenario.The test effectively simulates a failed decryption due to key mismatch by setting and retrieving a cookie with different keys, correctly expecting
undefined.
Line range hint
230-237: Test case correctly handles the cookie limit exceed event.The asynchronous test pattern using
done()is appropriately used to handle the event-driven nature of thecookieLimitExceedevent, ensuring that the event is emitted and handled correctly when the cookie value exceeds the limit.
Line range hint
245-251: Test case correctly ensures immutability of options object.The test effectively ensures that the options object passed to the
setmethod is not modified, which is crucial for avoiding unintended side effects.
Line range hint
257-265: Test case correctly applies default cookie options withsameSite=lax.The test properly sets and verifies the default cookie options, ensuring that the
sameSite=laxsetting is correctly applied and reflected in the response headers.
| @@ -32,13 +33,14 @@ cookies.get('foo', { encrypt: true }); | |||
|
|
|||
| ## Cookie Length Check | |||
|
|
|||
| [Browsers all had some limitation in cookie's length](http://browsercookielimits.squawky.net/), so if set a cookie with an extremely long value(> 4093), egg-cookies will emit an `cookieLimitExceed` event. You can listen to this event and record. | |||
| [Browsers all had some limitation in cookie's length](http://browsercookielimits.squawky.net/), so if set a cookie with an extremely long value(> 4093), `@eggjs/cookies` will emit an `cookieLimitExceed` event. You can listen to this event and record. | |||
There was a problem hiding this comment.
Preposition correction in cookie length check section.
The preposition used in the description of cookie length limitations should be corrected for grammatical accuracy.
- [Browsers all had some limitation in cookie's length]
+ [Browsers all had some limitation on cookie's length]Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [Browsers all had some limitation in cookie's length](http://browsercookielimits.squawky.net/), so if set a cookie with an extremely long value(> 4093), `@eggjs/cookies` will emit an `cookieLimitExceed` event. You can listen to this event and record. | |
| [Browsers all had some limitation on cookie's length](http://browsercookielimits.squawky.net/), so if set a cookie with an extremely long value(> 4093), `@eggjs/cookies` will emit an `cookieLimitExceed` event. You can listen to this event and record. |
Tools
LanguageTool
[uncategorized] ~36-~36: The preposition “on” seems more likely in this position than the preposition “in”. (AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_ON)
Context: ...heck [Browsers all had some limitation in cookie's length](http://browsercookieli...
|
|
||
| Extends [pillarjs/cookies](https://github.com/pillarjs/cookies) to adapt koa and egg with some additional features. | ||
|
|
||
| ## Encrypt | ||
|
|
||
| egg-cookies provide an alternative `encrypt` mode like `signed`. An encrypt cookie's value will be encrypted base on keys. Anyone who don't have the keys are unable to know the original cookie's value. | ||
| `@eggjs/cookies` provide an alternative `encrypt` mode like `signed`. An encrypt cookie's value will be encrypted base on keys. Anyone who don't have the keys are unable to know the original cookie's value. |
There was a problem hiding this comment.
Grammatical corrections needed in encryption description.
There are grammatical errors in the description of the encryption feature. The verb agreements need correction for clarity and professionalism.
- will be encrypted base on keys. Anyone who don't have the keys are unable to know the original cookie's value.
+ will be encrypted based on keys. Anyone who doesn't have the keys is unable to know the original cookie's value.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| `@eggjs/cookies` provide an alternative `encrypt` mode like `signed`. An encrypt cookie's value will be encrypted base on keys. Anyone who don't have the keys are unable to know the original cookie's value. | |
| `@eggjs/cookies` provide an alternative `encrypt` mode like `signed`. An encrypt cookie's value will be encrypted based on keys. Anyone who doesn't have the keys is unable to know the original cookie's value. |
Tools
LanguageTool
[uncategorized] ~21-~21: This verb does not appear to agree with the subject. Consider using a different form. (AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
Context: ...l be encrypted base on keys. Anyone who don't have the keys are unable to know the...
[uncategorized] ~21-~21: This verb does not appear to agree with the subject. Consider using a different form. (AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
Context: ...on keys. Anyone who don't have the keys are unable to know the original cookie's va...
test/keygrip.test.ts
Outdated
| assert.throws(() => { | ||
| new (Keygrip as any)(); | ||
| }, /keys must be provided and should be an array/); | ||
| assert.throws(() => { | ||
| new Keygrip([]); | ||
| }, /keys must be provided and should be an array/); | ||
| assert.throws(() => { | ||
| new Keygrip('hello' as any); | ||
| }, /keys must be provided and should be an array/); |
There was a problem hiding this comment.
Ensure consistent error handling in key validation tests.
The repeated error message "keys must be provided and should be an array" is used in different scenarios. Consider using distinct messages for each case to improve debuggability.
- }, /keys must be provided and should be an array/);
+ }, /keys array cannot be empty/);
- }, /keys must be provided and should be an array/);
+ }, /keys must be an array/);Committable suggestion was skipped due to low confidence.
test/keygrip.test.ts
Outdated
| it('should signed and verify failed return -1', () => { | ||
| const keygrip = new Keygrip([ 'foo', 'bar' ]); | ||
| const newKeygrip = new Keygrip([ 'another' ]); | ||
|
|
||
| const signed = keygrip.sign('hello'); | ||
| assert.equal(keygrip.verify('hello', signed), 0); | ||
| assert.equal(newKeygrip.verify('hello', signed), -1); |
There was a problem hiding this comment.
Add more robust testing for signature verification failures.
The test "should signed and verify failed return -1" checks for failed verifications but could be expanded to test various edge cases, such as invalid signatures or tampered data.
+ const tamperedSigned = 'tampered' + signed;
+ assert.equal(newKeygrip.verify('hello', tamperedSigned), -1);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should signed and verify failed return -1', () => { | |
| const keygrip = new Keygrip([ 'foo', 'bar' ]); | |
| const newKeygrip = new Keygrip([ 'another' ]); | |
| const signed = keygrip.sign('hello'); | |
| assert.equal(keygrip.verify('hello', signed), 0); | |
| assert.equal(newKeygrip.verify('hello', signed), -1); | |
| it('should signed and verify failed return -1', () => { | |
| const keygrip = new Keygrip([ 'foo', 'bar' ]); | |
| const newKeygrip = new Keygrip([ 'another' ]); | |
| const signed = keygrip.sign('hello'); | |
| assert.equal(keygrip.verify('hello', signed), 0); | |
| assert.equal(newKeygrip.verify('hello', signed), -1); | |
| const tamperedSigned = 'tampered' + signed; | |
| assert.equal(newKeygrip.verify('hello', tamperedSigned), -1); |
src/cookie.ts
Outdated
| * field-vchar = VCHAR / obs-text | ||
| * obs-text = %x80-FF | ||
| */ | ||
| const fieldContentRegExp = /^[\u0009\u0020-\u007e\u0080-\u00ff]+$/; // eslint-disable-line no-control-regex |
There was a problem hiding this comment.
Remove control characters from regular expressions.
The use of control characters in regular expressions is flagged as a potential issue. Consider removing these if they are not necessary for your validation logic.
- const fieldContentRegExp = /^[\u0009\u0020-\u007e\u0080-\u00ff]+$/; // eslint-disable-line no-control-regex
+ const fieldContentRegExp = /^[\u0020-\u007e\u0080-\u00ff]+$/; // Removed control character \u0009Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fieldContentRegExp = /^[\u0009\u0020-\u007e\u0080-\u00ff]+$/; // eslint-disable-line no-control-regex | |
| const fieldContentRegExp = /^[\u0020-\u007e\u0080-\u00ff]+$/; // Removed control character \u0009 |
Tools
Biome
[error] 10-10: Unexpected control character(s) in regular expression: \u0009 (lint/suspicious/noControlCharactersInRegex)
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- .github/workflows/nodejs.yml (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/nodejs.yml
[skip ci] ## [3.0.0](v2.10.0...v3.0.0) (2024-06-23) ### ⚠ BREAKING CHANGES * drop Node.js < 18.19.0 support part of eggjs/egg#3644 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced comprehensive support for TypeScript in project configurations. - Added new cookie management functionalities, including setting, encryption, and validation. - Added support for `Keygrip` class for cryptographic operations. - **Documentation** - Updated package name in README files from `egg-cookies` to `@eggjs/cookies`. - Adjusted code snippets and URLs in documentation to reflect the new package name. - **Tests** - Enhanced test suite with additional test cases for cookie encryption, caching, and error handling. - Added new test files for `Keygrip` and cookie functionalities. - **Chores** - Updated `.gitignore` to include new patterns for ignoring files. - Improved CI workflow with updated Node.js versions and Codecov token integration. - Updated dependencies and scripts in `package.json` to align with the new package structure and TypeScript support. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * support cjs and esm both by tshy ([#54](#54)) ([12db545](12db545))
BREAKING CHANGE: drop Node.js < 18.19.0 support
part of eggjs/egg#3644
Summary by CodeRabbit
New Features
Cookieclass for enhanced cookie management.Keygripclass for cryptographic operations.Bug Fixes
Documentation
egg-cookiesto@eggjs/cookiesin the README files.Tests
Chores
.gitignoreto exclude cache and build files.tsconfig.jsonfor TypeScript configuration.