feat(conversation): config encrypt decrypt payload transforms #3977
Conversation
WalkthroughThe changes introduce new configuration properties for handling encryption and decryption in the Changes
Possibly related PRs
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 (
|
robstax
changed the title
![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 (7)
packages/@webex/internal-plugin-conversation/src/config.js (2)
42-44: Fix incorrect documentation commentThe comment for
includeEncryptionTransformsincorrectly mentions "decryption" instead of "encryption".- * config value to perform decryption on outbound conversations and activities + * config value to perform encryption on outbound conversations and activities + * @type {Boolean}
36-45: Consider grouping encryption-related configurationsThe new properties are logically related to existing encryption settings like
keepEncryptedPropertiesanddecryptionFailureMessage, but they're separated in the config object. Consider grouping them together for better maintainability.export default { conversation: { allowedInboundTags: { 'webex-mention': ['data-object-type', 'data-object-id', 'data-object-url'], }, allowedOutboundTags: { 'webex-mention': ['data-object-type', 'data-object-id', 'data-object-url'], }, // eslint-disable-next-line no-empty-function inboundProcessFunc: () => {}, // eslint-disable-next-line no-empty-function outboundProcessFunc: () => {}, allowedInboundStyles: [], allowedOutboundStyles: [], thumbnailMaxHeight: 960, thumbnailMaxWidth: 640, + // Encryption-related settings + includeDecryptionTransforms: true, + includeEncryptionTransforms: true, keepEncryptedProperties: false, decryptionFailureMessage: 'This message cannot be decrypted', - includeDecryptionTransforms: true, - includeEncryptionTransforms: true, }, };packages/@webex/internal-plugin-conversation/src/index.js (3)
Line range hint
134-146: Ensure proper handling of outbound encryptionThe outbound transform chain includes encryption steps that should be conditional based on the new configuration:
return ctx .transform('normalizeObject', object) .then(() => ctx.transform('encryptObject', object)) .then(() => ctx.transform('encryptKmsMessage', object));Consider refactoring this to:
- Check configuration before applying encryption transforms
- Make the encryption steps optional in the transform chain
- Document the behavior when encryption is disabled
Line range hint
147-161: Ensure proper handling of inbound decryptionSimilar to the outbound transform, the inbound transform chain includes decryption that should be conditional:
return ctx .transform('decryptObject', object) .then(() => ctx.transform('normalizeObject', object));Consider:
- Adding configuration checks before applying decryption
- Making decryption optional in the transform chain
- Documenting the behavior when decryption is disabled
Line range hint
1-316: Consider documenting the encryption/decryption behaviorThe changes introduce conditional encryption/decryption, which is a significant architectural change. Consider:
- Adding JSDoc comments explaining the transform behavior with and without encryption/decryption
- Documenting any assumptions about external encryption/decryption handling
- Adding examples of how to properly configure and use the plugin with external encryption
packages/@webex/internal-plugin-conversation/test/unit/spec/conversation.js (1)
927-957: Enhance test coverage for transform verification.While the test covers various configuration combinations, it could be improved by:
- Adding explicit assertions for empty transforms
- Verifying the exact transform count
- Adding assertions for transform order
Consider adding these additional assertions:
it.each([...])('%s should include the correct transforms based on configuration', async ({includeDecryptionTransforms, includeEncryptionTransforms, expectedTransforms}) => { setup({includeDecryptionTransforms, includeEncryptionTransforms}); webex.trigger('ready'); const actualTransforms = webex.config.payloadTransformer.transforms; // Verify transform count assert.lengthOf(actualTransforms, expectedTransforms.length, 'should have correct number of transforms'); // Verify transform inclusion assert.isTrue( checkTransforms(actualTransforms, expectedTransforms), 'should include expected transforms' ); // If no transforms expected, verify array is empty if (expectedTransforms.length === 0) { assert.isEmpty(actualTransforms, 'should have no transforms'); } // Verify transform order if both are included if (includeEncryptionTransforms && includeDecryptionTransforms) { assert.isTrue( actualTransforms.every((transform, index) => transform.name === expectedTransforms[index].name ), 'transforms should be in correct order' ); } } );packages/@webex/internal-plugin-conversation/src/conversation.js (1)
82-89: Consider performance implications of transform initialization.The current implementation concatenates arrays on every initialization. For better performance, consider using a more efficient approach.
Consider using a Set or maintaining a single array:
initialize() { this.listenToOnce(this.webex, 'ready', () => { + const transforms = new Set(this.webex.config.payloadTransformer.transforms); + + if (this.config.includeDecryptionTransforms) { + decryptionTransforms.forEach(t => transforms.add(t)); + } + if (this.config.includeEncryptionTransforms) { + encryptionTransforms.forEach(t => transforms.add(t)); + } + + this.webex.config.payloadTransformer.transforms = Array.from(transforms); }); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/@webex/internal-plugin-conversation/src/config.js(1 hunks)packages/@webex/internal-plugin-conversation/src/conversation.js(2 hunks)packages/@webex/internal-plugin-conversation/src/index.js(1 hunks)packages/@webex/internal-plugin-conversation/test/unit/spec/conversation.js(5 hunks)
🔇 Additional comments (7)
packages/@webex/internal-plugin-conversation/src/config.js (1)
36-45: Verify security implications of disabling encryption/decryption
The ability to disable encryption/decryption transforms could potentially create security vulnerabilities if not properly handled by the embedding application. Please ensure:
- The documentation clearly states the security implications
- There are appropriate warnings when these are disabled
- The application validates that alternative encryption is in place when disabled
Let's check for any existing security documentation or warnings:
packages/@webex/internal-plugin-conversation/src/index.js (3)
Line range hint 71-92: Review the conversation list decryption logic
The normalizeConversationListAndBindDecrypters transform binds a decrypt function to each item. This needs to respect the new configuration options for conditional decryption.
Let's verify the integration with config options:
Line range hint 1-316: Verify encryption/decryption transform dependencies
The file shows removal of encryption/decryption transform imports but still contains multiple references to these operations in the transform functions. We need to ensure these operations are properly handled when the transforms are conditionally included.
Let's verify the dependencies and usage:
316-316: Review transform registration order
The transforms array maintains a specific order of operations. Ensure that making encryption/decryption optional doesn't break dependencies between transforms.
Let's analyze transform dependencies:
packages/@webex/internal-plugin-conversation/test/unit/spec/conversation.js (2)
18-19: LGTM! Clean import additions.
The new imports for encryption and decryption transforms are properly placed and align with the new test suite requirements.
949-949:
Fix misleading test description.
The test description "sets initFailed to true when collectPreauthCatalog errors" doesn't match what the test is actually verifying (transform inclusion based on configuration).
- '%s sets initFailed to true when collectPreauthCatalog errors',
+ '%s should include the correct transforms based on configuration',Likely invalid or redundant comment.
packages/@webex/internal-plugin-conversation/src/conversation.js (1)
28-29: Verify the security implications of the transform imports.
The imported encryption and decryption transforms are critical security components. We should ensure these modules are properly audited and tested.
✅ Verification successful
The imported transform modules are properly secured and tested
After analyzing the codebase, I can confirm that:
- Both encryption and decryption transforms are part of the internal plugin system with proper copyright and licensing.
- The transforms implement comprehensive security measures:
- Proper key management and validation
- Encrypted property handling
- Secure error handling that doesn't leak sensitive information
- The code includes unit tests that verify the encryption behavior, particularly for sensitive operations.
- The transforms are used internally within the
@webexnamespace, limiting exposure.
The security implications of these imports are well-managed through:
- Proper encapsulation within the internal plugin system
- Comprehensive encryption/decryption transforms with proper key handling
- Test coverage for critical security paths
- Secure error handling patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security tests and audit logs for the transform modules
# Check for security tests
rg -A 5 "describe.*security|test.*security" "./encryption-transforms.js" "./decryption-transforms.js"
# Check for recent security audits in git history
git log -p --grep="security audit" -- "./encryption-transforms.js" "./decryption-transforms.js"
Length of output: 327
Script:
#!/bin/bash
# First, let's find the actual paths of these transform files
fd "encryption-transforms.js|decryption-transforms.js"
# Then let's check the content of these files for security-related code
rg -l "security|encrypt|decrypt" $(fd "encryption-transforms.js|decryption-transforms.js")
# Look for test files related to these transforms
fd "encryption.*test|decryption.*test"
Length of output: 889
Script:
#!/bin/bash
# Let's examine the content of the transform files and their tests
echo "=== Encryption Transforms ==="
cat packages/@webex/internal-plugin-conversation/src/encryption-transforms.js
echo -e "\n=== Decryption Transforms ==="
cat packages/@webex/internal-plugin-conversation/src/decryption-transforms.js
echo -e "\n=== Encryption Tests ==="
cat packages/@webex/internal-plugin-conversation/test/unit/spec/encryption-transforms.js
Length of output: 35484
packages/@webex/internal-plugin-conversation/test/unit/spec/conversation.js
Outdated
Show resolved
Hide resolved
packages/@webex/internal-plugin-conversation/src/conversation.js
Outdated
Show resolved
Hide resolved
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
robstax
force-pushed
the
config-encrypt-decrypt
branch
from
d420089
to
499afa0
Compare
| .then(() => { | ||
| assert.deepEqual(conversation.sendReaction.args[0][1].recipients, expected); | ||
| }); | ||
| const expected = {items: [{id: recipientId, objectType: 'person'}]}; |
There was a problem hiding this comment.
most of these changes are just whitespace/formatting changes, except for the added new describe block
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/internal-plugin-conversation/src/conversation.js(2 hunks)packages/@webex/internal-plugin-conversation/test/unit/spec/conversation.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/internal-plugin-conversation/src/conversation.js
🔇 Additional comments (3)
packages/@webex/internal-plugin-conversation/test/unit/spec/conversation.js (3)
18-19: LGTM: New imports for transform testing
The imports for encryption and decryption transforms are correctly placed and will be used in the new test suite.
43-44: LGTM: Improved test readability
The refactoring improves readability by extracting expected values into variables, making the tests more maintainable.
Also applies to: 57-58, 81-82, 95-96
905-959: LGTM: Well-structured test suite for payload transforms
The test suite is well-organized with:
- Clear setup helper function
- Utility function for transform verification
- Comprehensive test coverage using parameterized tests
- Clear assertions validating transform configuration
packages/@webex/internal-plugin-conversation/test/unit/spec/conversation.js
Show resolved
Hide resolved
This pull request addresses
there are use cases where we want to use conversation plugin, but not encryption/decryption, such as when iframed in another app that handles encrypt/decrypt
by making the following changes
there are two new config values to conditionally add encryption and decryption transforms. by default these are true.
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
manual testing linking with web client
unit tests
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores