feat(plugin-encryption): Create new plugin-encryption #4099
Conversation
…oad and decrypt feature (#4082)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (17)
packages/@webex/plugin-encryption/src/cypher/index.ts (2)
11-14: Consider defining a dedicated interface for$webexto avoid@ts-ignore.By providing a typed definition for
$webex, you can remove the@ts-ignoredirective and improve type safety and readability. This ensures that future refactoring won’t introduce hidden type errors.
79-90: Avoid losing original error context in rethrows.While you’re including the original message and stack, consider wrapping or augmenting the original error rather than creating a brand new
Errorobject. This helps preserve error identity and stack traces for debugging.+ } catch (error) { + throw this.webex.internal.encryption.wrapError(error, 'Failed to decrypt the JWE'); + }packages/@webex/plugin-encryption/src/index.ts (1)
11-11: Good practice to export default and named references.Exporting
Cypheras the default is helpful. You may also consider providing a named export for clarity and future extension with additional utility exports.packages/@webex/plugin-encryption/src/cypher/cypher.types.ts (1)
24-26: Consider additional extensibility forIEncryption.The
IEncryptioninterface currently declares only one method. If encryption or decryption features expand in the future, you might anticipate further method definitions here, improving maintainability and discoverability.packages/@webex/plugin-encryption/src/types.ts (2)
4-11: Consider enhancing the Logger type definition.The Logger type could be more flexible and better documented:
- Methods could accept Error objects and additional metadata.
- Return types should be explicitly defined.
- JSDoc comments would improve documentation.
export type Logger = { - log: (payload: string) => void; - error: (payload: string) => void; - warn: (payload: string) => void; - info: (payload: string) => void; - trace: (payload: string) => void; - debug: (payload: string) => void; + /** Log a message at the 'log' level */ + log: (payload: string | Error, ...args: unknown[]) => void; + /** Log a message at the 'error' level */ + error: (payload: string | Error, ...args: unknown[]) => void; + /** Log a message at the 'warn' level */ + warn: (payload: string | Error, ...args: unknown[]) => void; + /** Log a message at the 'info' level */ + info: (payload: string | Error, ...args: unknown[]) => void; + /** Log a message at the 'trace' level */ + trace: (payload: string | Error, ...args: unknown[]) => void; + /** Log a message at the 'debug' level */ + debug: (payload: string | Error, ...args: unknown[]) => void; };
13-35: Enhance type safety and documentation of WebexSDK interface.Consider the following improvements:
- Define specific event types instead of using string.
- Extract options type for the download method.
- Add JSDoc comments for better documentation.
+/** Supported event types for the SDK */ +type WebexEvent = 'ready' | 'initialized' | 'error'; + +/** Options for file download */ +interface DownloadOptions { + useFileService: boolean; +} + +/** Main SDK interface */ export interface WebexSDK { version: string; canAuthorize: boolean; + /** Credentials management */ credentials: { getUserToken: () => Promise<string>; getOrgId: () => string; }; ready: boolean; - once: (event: string, callBack: () => void) => void; + once: (event: WebexEvent, callBack: () => void) => void; // internal plugins internal: { encryption: { + /** Decrypt a Secure Content Resource */ decryptScr: (keyUri: string, JWE: string) => Promise<string>; + /** Download and decrypt a file */ download: ( fileUri: string, scr: string, - options: {useFileService: boolean} + options: DownloadOptions ) => Promise<ArrayBuffer>; }; }; // public plugins logger: Logger; }packages/webex/src/encryption.js (1)
28-38: Avoid mutating function parameters.The current implementation modifies the input parameter
attrs, which could lead to unexpected side effects.-Webex.init = function init(attrs = {}) { - attrs.config = merge( +Webex.init = function init(attrs = {}) { + const config = merge( { sdkType: 'encryption', }, config, - attrs.config + attrs.config || {} ); // eslint-disable-line no-param-reassign - return new Webex(attrs); + return new Webex({ ...attrs, config }); };packages/@webex/plugin-encryption/jest.config.js (1)
3-41: Consider enhancing Jest configuration for better coverage reporting.The configuration is well-structured but could benefit from:
- Source map support for accurate coverage reporting
- Configuration for handling TypeScript paths
- Setup for handling CSS/asset imports in tests
const jestConfig = { rootDir: './', transformIgnorePatterns: [], testPathIgnorePatterns: ['/node_modules/', '/dist/'], testResultsProcessor: 'jest-junit', clearMocks: true, collectCoverage: true, + moduleNameMapper: { + '\\.(css|less|scss|sass)$': 'identity-obj-proxy', + '\\.(jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)$': '<rootDir>/__mocks__/fileMock.js' + }, + transform: { + '^.+\\.(t|j)sx?$': ['@swc/jest'] + }, coverageThreshold: { global: { lines: 85, functions: 85, branches: 85, statements: 85, }, }, coverageDirectory: 'coverage', - coverageReporters: ['clover', 'json', 'lcov'], + coverageReporters: ['clover', 'json', 'lcov', 'text', 'html'], + coveragePathIgnorePatterns: [ + '/node_modules/', + '/dist/', + '/__tests__/', + '/test/' + ], reporters: [ 'default', [packages/@webex/plugin-encryption/browsers.js (1)
58-71: Consider adding documentation for Firefox debugging preferences.The Firefox configuration includes multiple debugging preferences without documentation explaining their purpose and security implications.
Add comments explaining the purpose of each preference and any security considerations:
'moz:firefoxOptions': { args: ['-start-debugger-server', '9222'], prefs: { + // Enable Chrome DevTools Protocol for debugging 'devtools.chrome.enabled': true, + // Disable connection prompt for remote debugging 'devtools.debugger.prompt-connection': false, + // Enable remote debugging capability 'devtools.debugger.remote-enabled': true, // ... other prefs }, },packages/@webex/plugin-encryption/test/unit/spec/cypher/index.ts (1)
44-50: Add test cases for file size limits and specific error messages.The error test case could be more specific about the error conditions and include validation for file size limits.
Consider adding these test cases:
it('should throw an error if file size exceeds maximum limit', async () => { const maxSizeBytes = 100 * 1024 * 1024; // 100MB const largeFile = new ArrayBuffer(maxSizeBytes + 1); webex.internal.encryption.decryptScr.mockResolvedValue('scr'); webex.internal.encryption.download.mockResolvedValue(largeFile); await expect(cypher.downloadAndDecryptFile(fileUri, options)) .rejects.toThrow('File size exceeds maximum limit of 100MB'); }); it('should provide specific error message for invalid keyUri format', async () => { await expect( cypher.downloadAndDecryptFile('https://example.com/file', { keyUri: 'invalid-format' }) ).rejects.toThrow('Invalid keyUri format. Expected format: kms://<domain>/keys/<id>'); });docs/samples/plugin-encryption/app.js (1)
120-141: Improve error handling and resource cleanup.The error handling could be more specific, and resource cleanup could be more robust.
Consider these improvements:
let objectUrl; +let tempElement; try { const decryptedBuf = await webex.cypher.downloadAndDecryptFile(fileUrl); const file = new File([decryptedBuf], encryptedFileName, {type: mimeType}); objectUrl = URL.createObjectURL(file); - const a = document.createElement("a"); - a.href = objectUrl; - a.download = file.name || "download"; - document.body.appendChild(a); - a.click(); - document.body.removeChild(a); + tempElement = document.createElement("a"); + tempElement.href = objectUrl; + tempElement.download = file.name || "download"; + tempElement.style.display = 'none'; + document.body.appendChild(tempElement); + tempElement.click(); decryptFileResult.innerText = ': success'; } catch (error) { console.error('error decrypting file', error); - decryptFileResult.innerText = ': error'; + decryptFileResult.innerText = `: error - ${error.message}`; } finally { if (objectUrl) { URL.revokeObjectURL(objectUrl); } + if (tempElement?.parentNode) { + tempElement.parentNode.removeChild(tempElement); + } }packages/@webex/internal-plugin-encryption/src/encryption.js (1)
82-82: Consider makinguseFileServiceconfigurable via class config.The default
useFileService: trueis hardcoded in the method. Consider making it configurable through the class configuration for better flexibility.- const promise = this._fetchDownloadUrl(fileUrl, {useFileService: true, ...options}) + const promise = this._fetchDownloadUrl(fileUrl, {useFileService: this.config.useFileService ?? true, ...options})packages/@webex/plugin-encryption/README.md (2)
52-52: Fix heading level inconsistency.The heading "Development" is using h4 (####) when it should be using h3 (###) to maintain proper hierarchy.
-#### Development +### Development🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
52-52: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
35-50: Enhance sample code with error handling best practices.The sample code should demonstrate proper error handling and cleanup.
try { const decryptedFileBuf = await webex.cypher.downloadAndDecryptFile(attachmentURL); // Do something with the decrypted file buffer } catch (error) { - // Handle error + console.error('Failed to decrypt file:', error.message); + throw new Error(`Decryption failed: ${error.message}`); +} finally { + // Cleanup any resources if needed }packages/@webex/plugin-encryption/developer-quickstart.md (1)
55-74: Enhance file decryption error handling and add progress indicator.The decryption function could benefit from better error handling and user feedback.
async function decryptFile(webex, encryptedFileUrl, decryptedFileName, mimeType) { + const progressElement = document.getElementById('decrypt-progress'); try { + progressElement.textContent = 'Decrypting file...'; const file = await webex.cypher.downloadAndDecryptFile({ url: encryptedFileUrl, mimeType: mimeType }); const url = window.URL.createObjectURL(file); const a = document.createElement('a'); a.style.display = 'none'; a.href = url; a.download = decryptedFileName; document.body.appendChild(a); a.click(); window.URL.revokeObjectURL(url); + progressElement.textContent = 'File decrypted and downloaded successfully'; console.log('File decrypted and downloaded successfully'); } catch (error) { + progressElement.textContent = `Error: ${error.message}`; console.error('Error decrypting file:', error); + throw error; + } finally { + document.body.removeChild(a); } }docs/samples/plugin-encryption/index.html (1)
38-38: Add proper ARIA labels and input validation.The access token input field should have proper accessibility attributes and validation.
- <input id="access-token" name="accessToken" placeholder="Access Token" value="" type="text" style="margin: 1rem 0 0.5rem 0;" required> + <input id="access-token" name="accessToken" placeholder="Access Token" value="" type="text" + aria-label="Access Token Input" + pattern="^[A-Za-z0-9-_]*\.[A-Za-z0-9-_]*\.[A-Za-z0-9-_]*$" + title="Please enter a valid JWT token" + style="margin: 1rem 0 0.5rem 0;" required>docs/samples/plugin-encryption/style.css (1)
74-74: Remove redundant font names in font-family declarations.The font-family declarations contain duplicate "Helvetica" entries, which are unnecessary.
Apply this diff to remove the redundancy:
- font-family: CiscoSansTT Regular, Helvetica Neue, Helvetica, sans-serif; + font-family: CiscoSansTT Regular, Helvetica Neue, sans-serif;Also applies to: 144-144, 351-351
🧰 Tools
🪛 Biome (1.9.4)
[error] 74-74: Duplicate font names are redundant and unnecessary: Helvetica
Remove duplicate font names within the property
(lint/suspicious/noDuplicateFontNames)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (31)
docs/index.html(1 hunks)docs/samples/browser-plugin-meetings/app.js(1 hunks)docs/samples/calling/app.js(4 hunks)docs/samples/plugin-encryption/app.js(1 hunks)docs/samples/plugin-encryption/index.html(1 hunks)docs/samples/plugin-encryption/style.css(1 hunks)package.json(2 hunks)packages/@webex/internal-plugin-encryption/src/encryption.js(3 hunks)packages/@webex/plugin-encryption/.eslintrc.js(1 hunks)packages/@webex/plugin-encryption/LICENSE(1 hunks)packages/@webex/plugin-encryption/README.md(1 hunks)packages/@webex/plugin-encryption/babel.config.js(1 hunks)packages/@webex/plugin-encryption/browsers.js(1 hunks)packages/@webex/plugin-encryption/developer-quickstart.md(1 hunks)packages/@webex/plugin-encryption/jest.config.js(1 hunks)packages/@webex/plugin-encryption/package.json(1 hunks)packages/@webex/plugin-encryption/process(1 hunks)packages/@webex/plugin-encryption/src/config.ts(1 hunks)packages/@webex/plugin-encryption/src/cypher/constants.ts(1 hunks)packages/@webex/plugin-encryption/src/cypher/cypher.types.ts(1 hunks)packages/@webex/plugin-encryption/src/cypher/index.ts(1 hunks)packages/@webex/plugin-encryption/src/index.ts(1 hunks)packages/@webex/plugin-encryption/src/types.ts(1 hunks)packages/@webex/plugin-encryption/test/unit/spec/cypher/index.ts(1 hunks)packages/@webex/plugin-encryption/tsconfig.json(1 hunks)packages/@webex/test-helper-mock-webex/src/index.js(1 hunks)packages/webex/package.json(2 hunks)packages/webex/src/encryption.js(1 hunks)packages/webex/src/webex.js(1 hunks)tooling/commands/test.js(1 hunks)webpack.config.js(2 hunks)
✅ Files skipped from review due to trivial changes (8)
- packages/@webex/plugin-encryption/tsconfig.json
- packages/@webex/plugin-encryption/src/config.ts
- packages/@webex/plugin-encryption/process
- packages/@webex/plugin-encryption/LICENSE
- packages/@webex/plugin-encryption/.eslintrc.js
- packages/@webex/plugin-encryption/babel.config.js
- packages/@webex/plugin-encryption/src/cypher/constants.ts
- docs/samples/browser-plugin-meetings/app.js
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
packages/@webex/plugin-encryption/README.md
52-52: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
🪛 Biome (1.9.4)
docs/samples/plugin-encryption/style.css
[error] 74-74: Duplicate font names are redundant and unnecessary: Helvetica
Remove duplicate font names within the property
(lint/suspicious/noDuplicateFontNames)
[error] 144-144: Duplicate font names are redundant and unnecessary: Helvetica
Remove duplicate font names within the property
(lint/suspicious/noDuplicateFontNames)
[error] 351-351: Duplicate font names are redundant and unnecessary: Helvetica
Remove duplicate font names within the property
(lint/suspicious/noDuplicateFontNames)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Initialize Project
🔇 Additional comments (23)
packages/@webex/plugin-encryption/src/cypher/index.ts (2)
37-43: Commendable documentation of usage.The inline example with explicit parameters and usage instructions helps users quickly understand how to utilize the
downloadAndDecryptFilemethod. This is an exemplary practice.
61-65: Check for repeated query parameter keys.When using
URL(fileUri), consider the case where a query parameter might appear multiple times. By default,URLSearchParamsmerges repeated keys. Make sure this behavior matches your expectations forkeyUriandJWE. If necessary, handle repeated keys explicitly.packages/@webex/plugin-encryption/src/index.ts (1)
7-9: Validate plugin name consistency.Make sure the
'cypher'plugin name is consistent across documentation, configuration, and usage. Inconsistent naming can cause confusion or runtime errors if the plugin is referenced incorrectly elsewhere.packages/webex/src/encryption.js (2)
9-19: LGTM!Good practices observed:
- Polyfill check before loading
- Clear module dependencies
- Proper error handling for polyfill loading
23-26:❓ Verification inconclusive
Verify PACKAGE_VERSION replacement during build.
Ensure that the PACKAGE_VERSION constant is correctly replaced during the build process.
🏁 Script executed:
#!/bin/bash # Description: Check if PACKAGE_VERSION is defined in webpack config # Search for PACKAGE_VERSION definition in webpack configs rg -l "PACKAGE_VERSION" | grep -i "webpack"Length of output: 41
Verify PACKAGE_VERSION Replacement in Build Process
The search for PACKAGE_VERSION within webpack configuration files did not return any results. This could mean that the constant might be replaced by a different mechanism or in another configuration file. Please manually verify that the build system (webpack, rollup, etc.) is set up to replace PACKAGE_VERSION correctly. If your repository uses a non-webpack build tool for replacement, double-check that its configuration is functioning as expected.
- Confirm that the build configuration (e.g., in webpack.config.js or other related build files) includes a mechanism to replace PACKAGE_VERSION.
- If using a custom plugin or another tool for environment variable injection, verify that it targets the PACKAGE_VERSION constant properly.
packages/webex/src/webex.js (1)
30-30: LGTM!The encryption plugin is correctly added to the list of required plugins, following the established pattern.
packages/@webex/plugin-encryption/browsers.js (1)
19-21: Review WebRTC configuration for security implications.The
WebRtcHideLocalIpsWithMdnsfeature is being disabled across multiple browser configurations. This could expose local network information in production environments.Consider enabling this feature in production or documenting the security implications:
-'--disable-features=WebRtcHideLocalIpsWithMdns', +'--disable-features=WebRtcHideLocalIpsWithMdns', // TODO: Enable in productionAlso applies to: 25-27, 38-40
packages/@webex/plugin-encryption/test/unit/spec/cypher/index.ts (1)
81-86: LGTM! Good error handling test coverage.The test properly verifies error propagation from the download method.
webpack.config.js (2)
42-48: LGTM! Good configuration for the encryption module.The webpack configuration for the encryption module is properly set up with UMD library type.
145-145: LGTM! Appropriate service URL for integration environment.The HYDRA_SERVICE_URL is correctly configured for the integration environment.
tooling/commands/test.js (1)
171-171: LGTM!The addition of
@webex/plugin-encryptionto the Mocha test handler list is appropriate for the new encryption plugin.packages/@webex/internal-plugin-encryption/src/encryption.js (3)
75-75: LGTM!Making the
downloadmethod async is appropriate as it performs asynchronous operations.
121-129: LGTM! Good security check for HTTPS.The HTTPS validation for direct file URLs is a good security practice.
201-216: LGTM! Improved error handling.The added validations for key, JWK, and SCR instance creation improve the robustness of the code.
packages/@webex/test-helper-mock-webex/src/index.js (1)
18-18: LGTM!Adding 'cypher' to
nonInternalPluginsis appropriate for the new encryption plugin's test mocking.docs/samples/calling/app.js (2)
184-184: LGTM! Fixed syntax error.Added missing closing brace to fix the syntax error in the
generateGuestTokenfunction.
234-234:❓ Verification inconclusive
Verify the new hydra service URL.
The hydra service URL has been updated from
apialpha.ciscospark.comtohydra-intb.ciscospark.com.
🌐 Web query:
Is hydra-intb.ciscospark.com the correct integration environment URL for Webex?💡 Result:
The domain
hydra-intb.ciscospark.comappears to relate to Cisco's internal integration/testing environments for Webex, though this is not explicitly documented in public resources[8].Key points:
- Internal environments like
int(integration) are typically restricted to Cisco engineering teams[8]. Public developer tools (e.g.,developer.webex.com) focus on production environments[1][2][5].- Alternative domains with
intbsuffixes (e.g.,app-hub-intb.ciscospark.com) are referenced in limited contexts[10], suggesting Cisco uses such naming conventions for non-production systems.- Guidance for non-public environments: If working with internal Cisco teams, coordinate directly with them for authentication URLs like
idbrokerbts.webex.com[8]. Public integrations should use standard OAuth endpoints likewebexapis.com/v1/access_token[1][2].For most developers, production integrations should follow official documentation using
webexapis.comdomains[1][2][5].Citations:
- 1: https://developer.webex.com/docs/integrations
- 2: https://developer.webex.com/docs/run-an-oauth-integration
- 3: https://help.webex.com/en-us/article/m0jby2/Configure-a-list-of-allowed-domains-to-access-Webex-while-on-your-corporatenetwork
- 4: https://www.webex.com/faqs.html
- 5: https://developer.cisco.com/codeexchange/github/repo/CiscoDevNet/webex-integration-sample/
- 6: https://help.webex.com/en-us/article/WBX264/How-Do-I-Allow-Webex-Meetings-Traffic-on-My-Network
- 7: https://github.com/webex/webex-js-sdk/blob/next/sauce.py
- 8: https://community.cisco.com/t5/webex-for-developers/create-integration-belongs-to-int-environment/td-p/4630547
- 9: https://www.cisco.com/c/en/us/td/docs/voice_ip_comm/cloudCollaboration/wbxt/hybridservices/hybridcalldevices/wbxhs_m_deployment-guide-for-webex-devices-hybrid-call/wbxhs_m_deployment-guide-for-webex-devices-hybrid-call_chapter_01.html
- 10: https://app-hub-intb.ciscospark.com/category/collaboration-management
Action: Verify the intended integration environment for the Hydra URL
The updated Hydra URL:
hydra: 'https://hydra-intb.ciscospark.com/v1/',appears to correspond to Cisco’s internal integration/testing environment rather than a publicly documented production endpoint. If this sample is meant for internal use, the change seems appropriate. However, if this is intended for external developers, please review whether a production URL (such as those documented on developer.webex.com) would be more appropriate.
- Confirm that internal integration endpoints are intended for this code sample.
- If external usage is expected, consider aligning with the officially documented endpoints.
packages/@webex/plugin-encryption/package.json (1)
34-34:✅ Verification successful
Verify the security of node-scr dependency.
The node-scr package version 0.3.0 appears to be outdated. Let's check for any security advisories and the latest version.
🌐 Web query:
What is the latest version of node-scr npm package? Are there any known security vulnerabilities?💡 Result:
The latest version of the node-scr npm package is 0.3.0, last published on October 21, 2020[1]. This JavaScript library helps manage Secure Content Resource (SCR) objects for encrypted content handling.
Security Status
No specific security vulnerabilities are reported for node-scr in available sources[1][2][5][6]. However, general npm security considerations apply:
- Outdated Dependencies: Last updated 4 years ago (as of 2025), raising potential maintenance concerns[1]
- Crypto Implementation Risks: Uses AES-GCM encryption which requires proper implementation to avoid side-channel attacks[1]
- JSON Exposure Warning: Contains sensitive decryption data in JSON format that shouldn't be shared externally[1]
Recommended Actions
- Monitor using
npm auditor tools like Snyk[6][8]- Consider alternatives if long-term maintenance is critical
- Follow encryption best practices when handling SCR objects[1]
While no direct exploits are documented, the package's age (4+ years without updates) suggests increased potential for undiscovered vulnerabilities compared to actively maintained packages[2][5].
Citations:
- 1: https://www.npmjs.com/package/node-scr
- 2: https://www.aquasec.com/cloud-native-academy/supply-chain-security/npm-vulnerabilities/
- 3: https://nodejs.org/en/blog/vulnerability/february-2023-security-releases
- 4: https://nodejs.org/en/learn/getting-started/an-introduction-to-the-npm-package-manager
- 5: https://www.nodejs-security.com/blog/npm-vulnerabilities-review
- 6: https://security.snyk.io/package/npm/npm
- 7: https://betterstack.com/community/questions/how-to-find-version-of-installed-npm-package/
- 8: https://docs.npmjs.com/auditing-package-dependencies-for-security-vulnerabilities/
- 9: https://blog.checkpoint.com/securing-the-cloud/review-of-recent-npm-based-vulnerabilities/
- 10: https://www.npmjs.com/package/node
Security Check for node-scr Dependency Verified
- The installed version
"node-scr": "^0.3.0"is confirmed as the latest release (last published in October 2020).- No known security vulnerabilities have been reported for this version.
- Although the package is not actively maintained, we recommend periodically running tools like
npm auditand considering long-term maintenance risks—especially if encryption security is mission-critical.packages/webex/package.json (1)
20-20: LGTM! Plugin integration looks correct.The encryption plugin is properly integrated with both the exports and dependencies.
Also applies to: 82-82
docs/samples/plugin-encryption/index.html (1)
82-83:❓ Verification inconclusive
Add integrity checks for external scripts.
External script sources should include integrity hashes to prevent tampering.
[security]- <script src="../webex.min.js"></script> - <script src="app.js"></script> + <script src="../webex.min.js" integrity="sha384-..." crossorigin="anonymous"></script> + <script src="app.js" integrity="sha384-..." crossorigin="anonymous"></script>
🏁 Script executed:
#!/bin/bash # Generate integrity hashes for the scripts sha384sum ../webex.min.js app.js | awk '{print "sha384-"$1}'Length of output: 162
Attention Required: Verify and Update Integrity Hashes for External Scripts
The external script tags in
docs/samples/plugin-encryption/index.htmlcurrently lack computed integrity attributes, leaving a potential vulnerability. The initial command usingsha384sumfailed (command not found), so the SHA‑384 values were not generated.
- Action Steps:
Recalculate Integrity Hashes: Please run an alternative command using
opensslto generate the correct hash values. For example:#!/bin/bash for file in "../webex.min.js" "app.js"; do echo "Generating integrity hash for $file..." if command -v openssl >/dev/null 2>&1; then hash=$(openssl dgst -sha384 -binary "$file" | openssl base64 -A) echo "sha384-$hash" else echo "Error: openssl is not available, please ensure it is installed." fi doneUpdate the Script Tags: Once you have the correct hash values, update the HTML as follows:
- <script src="../webex.min.js"></script> - <script src="app.js"></script> + <script src="../webex.min.js" integrity="sha384-<computed_hash>" crossorigin="anonymous"></script> + <script src="app.js" integrity="sha384-<computed_hash>" crossorigin="anonymous"></script>Please verify the outputs from the revised command and update the integrity attributes accordingly.
Manual Verification Required: Update Integrity Hashes for External Scripts
The integrity attributes for the external scripts in
docs/samples/plugin-encryption/index.htmlstill need to be computed and added. The previous attempt usingsha384sumfailed because that command wasn’t available. To generate the required SHA‑384 hashes, please run the following shell script which usesopenssl:Once you obtain the hash values, update the script tags in the HTML file as follows:
- <script src="../webex.min.js"></script> - <script src="app.js"></script> + <script src="../webex.min.js" integrity="sha384-<computed_hash>" crossorigin="anonymous"></script> + <script src="app.js" integrity="sha384-<computed_hash>" crossorigin="anonymous"></script>Replace
<computed_hash>with the actual hash values generated for each file.
🏁 Script executed:
[run_scripts] #!/bin/bash if ! command -v openssl >/dev/null 2>&1; then echo "Error: openssl command not available. Please install it before proceeding." exit 1 fi for file in "../webex.min.js" "app.js" do if [ -f "$file" ]; then echo "Generating integrity hash for $file..." hash=$(openssl dgst -sha384 -binary "$file" | openssl base64 -A) echo "$file: sha384-$hash" else echo "Error: file '$file' not found" fi doneLength of output: 310
Attention: Integrity Hashes Verification Still Pending
The current method using
opensslfailed because the command is not available in the environment. To generate the necessary SHA‑384 hashes for the external scripts in the HTML file, please try the following alternative using Python:Next Steps:
Run the above script to generate the SHA‑384 integrity hashes for
../webex.min.jsandapp.js.Once you have the computed hash values, update the HTML file
docs/samples/plugin-encryption/index.htmlas follows:- <script src="../webex.min.js"></script> - <script src="app.js"></script> + <script src="../webex.min.js" integrity="sha384-<computed_hash>" crossorigin="anonymous"></script> + <script src="app.js" integrity="sha384-<computed_hash>" crossorigin="anonymous"></script>Replace
<computed_hash>with the actual hash values output by the script.Please verify the hashes manually and update the file accordingly.
🏁 Script executed:
[run_scripts] #!/bin/bash for file in "../webex.min.js" "app.js" do if [ -f "$file" ]; then python - <<'EOF' import sys import hashlib import base64 filename = sys.argv[1] with open(filename, 'rb') as f: data = f.read() hash_val = hashlib.sha384(data).digest() print(f"{filename}: sha384-{base64.b64encode(hash_val).decode().strip()}") EOF $file else echo "Error: file '$file' not found" fi doneLength of output: 375
Action Required: Verify and Update Integrity Hashes for External Scripts
It appears that previous attempts to generate SHA‑384 hashes for
../webex.min.jsandapp.jshave failed. Please run an updated verification script that uses Python’s inline execution to generate the required integrity hashes for both files. For example, try the following:Next Steps:
Run the updated script to generate the SHA‑384 hash values manually.
Update the script tags in
docs/samples/plugin-encryption/index.htmlas shown below, replacing<computed_hash>with the actual hash values:- <script src="../webex.min.js"></script> - <script src="app.js"></script> + <script src="../webex.min.js" integrity="sha384-<computed_hash>" crossorigin="anonymous"></script> + <script src="app.js" integrity="sha384-<computed_hash>" crossorigin="anonymous"></script>Once you’ve manually verified and updated the integrity attributes, the security concern will be addressed.
🏁 Script executed:
[run_scripts] #!/bin/bash for file in "../webex.min.js" "app.js" do if [ -f "$file" ]; then python -c "import sys, hashlib, base64; f = open(sys.argv[1], 'rb'); data = f.read(); hash_val = hashlib.sha384(data).digest(); print(f'{sys.argv[1]}: sha384-'+base64.b64encode(hash_val).decode().strip())" "$file" else echo "Error: file '$file' not found" fi doneLength of output: 424
Action Required: Verify External File Paths and Update Integrity Attributes
It appears that the files referenced in
docs/samples/plugin-encryption/index.html—namely,../webex.min.jsandapp.js—were not located in the repository. Before adding integrity attributes, please verify that these files exist at the specified paths. If they do exist (or once you correct the paths), compute the SHA‑384 integrity hashes manually (using a secure local method, such as Python or a verified tool) and update the script tags accordingly. For example, the revised HTML should look like:- <script src="../webex.min.js"></script> - <script src="app.js"></script> + <script src="../webex.min.js" integrity="sha384-<computed_hash>" crossorigin="anonymous"></script> + <script src="app.js" integrity="sha384-<computed_hash>" crossorigin="anonymous"></script>Replace
<computed_hash>with the actual computed values for each script.Please address the file path issue and verify the integrity hashes manually.
docs/index.html (1)
85-86: LGTM!The new link to the Encryption Plugin sample is properly integrated into the samples list, following the established pattern.
docs/samples/plugin-encryption/style.css (1)
1-356: LGTM! Well-structured and comprehensive CSS.The CSS is well-organized with clear sections for base styles, form elements, utilities, and components. The styling provides a consistent and professional look while maintaining good maintainability.
🧰 Tools
🪛 Biome (1.9.4)
[error] 74-74: Duplicate font names are redundant and unnecessary: Helvetica
Remove duplicate font names within the property
(lint/suspicious/noDuplicateFontNames)
[error] 144-144: Duplicate font names are redundant and unnecessary: Helvetica
Remove duplicate font names within the property
(lint/suspicious/noDuplicateFontNames)
[error] 351-351: Duplicate font names are redundant and unnecessary: Helvetica
Remove duplicate font names within the property
(lint/suspicious/noDuplicateFontNames)
package.json (1)
30-30: LGTM! Build scripts properly updated for plugin-encryption integration.The changes correctly integrate the @webex/plugin-encryption plugin into the build system:
- Added to the @legacy script's workspace list
- Included in the TypeScript build process
- Added to the skip-samples build process
Also applies to: 40-40, 42-42
| if (localStorage.getItem('date') > new Date().getTime()) { | ||
| tokenElm.value = localStorage.getItem('access-token'); | ||
| } else { | ||
| localStorage.removeItem('access-token'); | ||
| } |
There was a problem hiding this comment.
Enhance token storage security.
Storing access tokens in localStorage poses security risks. Consider using more secure storage methods or implementing encryption.
Consider using session storage or implementing encryption:
-if (localStorage.getItem('date') > new Date().getTime()) {
- tokenElm.value = localStorage.getItem('access-token');
-} else {
- localStorage.removeItem('access-token');
-}
+// Use sessionStorage for better security
+if (sessionStorage.getItem('date') > new Date().getTime()) {
+ tokenElm.value = sessionStorage.getItem('access-token');
+} else {
+ sessionStorage.removeItem('access-token');
+}📝 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.
| if (localStorage.getItem('date') > new Date().getTime()) { | |
| tokenElm.value = localStorage.getItem('access-token'); | |
| } else { | |
| localStorage.removeItem('access-token'); | |
| } | |
| // Use sessionStorage for better security | |
| if (sessionStorage.getItem('date') > new Date().getTime()) { | |
| tokenElm.value = sessionStorage.getItem('access-token'); | |
| } else { | |
| sessionStorage.removeItem('access-token'); | |
| } |
| if (!fileUrl) { | ||
| decryptFileResult.innerText = ': error - Invalid file URL'; | ||
| return; | ||
| } | ||
|
|
||
| if (!mimeType) { | ||
| decryptFileResult.innerText = ': error - Invalid MIME type'; | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve input validation for file URL and MIME type.
The validation could be more robust by checking URL format and validating MIME types against a whitelist.
Add stronger validation:
-if (!fileUrl) {
+const validMimeTypes = ['application/pdf', 'image/jpeg', 'image/png'];
+if (!fileUrl || !fileUrl.startsWith('https://')) {
decryptFileResult.innerText = ': error - Invalid file URL';
return;
}
-if (!mimeType) {
+if (!mimeType || !validMimeTypes.includes(mimeType)) {
decryptFileResult.innerText = ': error - Invalid MIME type';
return;
}📝 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.
| if (!fileUrl) { | |
| decryptFileResult.innerText = ': error - Invalid file URL'; | |
| return; | |
| } | |
| if (!mimeType) { | |
| decryptFileResult.innerText = ': error - Invalid MIME type'; | |
| return; | |
| } | |
| const validMimeTypes = ['application/pdf', 'image/jpeg', 'image/png']; | |
| if (!fileUrl || !fileUrl.startsWith('https://')) { | |
| decryptFileResult.innerText = ': error - Invalid file URL'; | |
| return; | |
| } | |
| if (!mimeType || !validMimeTypes.includes(mimeType)) { | |
| decryptFileResult.innerText = ': error - Invalid MIME type'; | |
| return; | |
| } |
| "test:broken": "yarn test:style && yarn test:unit && yarn test:integration && yarn test:browser", | ||
| "test:browser": "webex-legacy-tools test --integration --runner karma", | ||
| "test:style": "eslint ./src/**/*.*", | ||
| "test:unit": "webex-legacy-tools test --unit --runner jest" |
There was a problem hiding this comment.
Test script needs correction.
The test:broken script includes browser tests, but the test:browser script is marked as broken. This inconsistency should be resolved.
- "test:broken": "yarn test:style && yarn test:unit && yarn test:integration && yarn test:browser",
+ "test": "yarn test:style && yarn test:unit && yarn test:integration",
+ "test:all": "yarn test && yarn test:browser"📝 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.
| "test:broken": "yarn test:style && yarn test:unit && yarn test:integration && yarn test:browser", | |
| "test:browser": "webex-legacy-tools test --integration --runner karma", | |
| "test:style": "eslint ./src/**/*.*", | |
| "test:unit": "webex-legacy-tools test --unit --runner jest" | |
| "test": "yarn test:style && yarn test:unit && yarn test:integration", | |
| "test:browser": "webex-legacy-tools test --integration --runner karma", | |
| "test:style": "eslint ./src/**/*.*", | |
| "test:unit": "webex-legacy-tools test --unit --runner jest", | |
| "test:all": "yarn test && yarn test:browser" |
| <legend>Decrypt files</legend> | ||
| <div class="u-mv"> | ||
| <div> | ||
| File URL: <input id="encrypted-file-url" value="" type="string" style="margin-bottom: 0.5rem;"> |
There was a problem hiding this comment.
Add input validation for file URL.
The file URL input should validate the URL format and restrict to allowed domains.
- File URL: <input id="encrypted-file-url" value="" type="string" style="margin-bottom: 0.5rem;">
+ File URL: <input id="encrypted-file-url" value="" type="url"
+ pattern="https://.*\.webex\.com/.*"
+ title="Please enter a valid Webex file URL"
+ aria-label="Encrypted File URL"
+ style="margin-bottom: 0.5rem;"
+ required>📝 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.
| File URL: <input id="encrypted-file-url" value="" type="string" style="margin-bottom: 0.5rem;"> | |
| File URL: <input id="encrypted-file-url" value="" type="url" | |
| pattern="https://.*\.webex\.com/.*" | |
| title="Please enter a valid Webex file URL" | |
| aria-label="Encrypted File URL" | |
| style="margin-bottom: 0.5rem;" | |
| required> |
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
📝 WalkthroughWalkthroughThis pull request implements extensive updates across documentation, sample projects, and internal modules to integrate and refine the Encryption Plugin for the Cisco Webex JS SDK. Changes include adding new HTML pages and links to guide users in using the encryption service, updating service endpoint URLs, and enhancing error handling and code formatting in sample projects. The modifications also extend build scripts in package.json to include the Encryption Plugin. New configuration files for ESLint, Babel, Jest, and TypeScript have been added, along with comprehensive updates to encryption functionalities such as file decryption, SCR handling, and encryption constants. Additionally, the plugin is now registered and exposed in the Webex package, and automated tests have been introduced to verify key behaviors of the encryption plugin. Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
yarn install v1.22.22 (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) ✨ Finishing Touches
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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (15)
packages/@webex/plugin-encryption/src/cypher/index.ts (2)
24-28: Remove or justify the@ts-ignoreusage.For better type safety and maintainability, it's generally preferable to resolve the underlying TypeScript error rather than ignoring it. Consider casting
this.webextoWebexSDKif that's the expected type:constructor(...args: any[]) { super(...args); - // @ts-ignore - this.$webex = this.webex; + this.$webex = this.webex as WebexSDK; }
44-90: Enhance error handling for download operations.While the current error handling for decryption is sufficient, additional checks for network or partial download failures could be beneficial to improve resiliency. For instance, you might handle HTTP errors explicitly and retry or provide a more descriptive error message if the download fails or times out.
packages/@webex/plugin-encryption/src/cypher/cypher.types.ts (1)
24-26: Add JSDoc documentation for the IEncryption interface.Consider adding JSDoc documentation to describe the purpose of the interface and its method.
+/** + * Interface for encryption-related operations. + */ interface IEncryption { + /** + * Downloads and decrypts a file from the given URI. + * @param fileUri - The URI of the file to download and decrypt. + * @param options - Options for downloading and decrypting the file. + * @returns A promise that resolves to the decrypted file data as ArrayBuffer. + */ downloadAndDecryptFile(fileUri: string, options: FileDownloadOptions): Promise<ArrayBuffer>; }packages/@webex/plugin-encryption/src/types.ts (1)
24-31: Add JSDoc documentation for encryption methods.The internal encryption methods would benefit from JSDoc documentation explaining their parameters and return values.
encryption: { + /** + * Decrypts a Secure Content Resource (SCR) using the provided key. + * @param keyUri - The URI of the key used for decryption. + * @param JWE - The JSON Web Encryption string to decrypt. + * @returns A promise that resolves to the decrypted content. + */ decryptScr: (keyUri: string, JWE: string) => Promise<string>; + /** + * Downloads a file using the provided SCR. + * @param fileUri - The URI of the file to download. + * @param scr - The Secure Content Resource for the file. + * @param options - Options for downloading the file. + * @returns A promise that resolves to the downloaded file data. + */ download: ( fileUri: string, scr: string, options: {useFileService: boolean} ) => Promise<ArrayBuffer>;packages/webex/src/encryption.js (1)
28-38: Add error handling for invalid initialization attributes.The
initmethod should validate the input attributes to prevent potential runtime errors.Webex.init = function init(attrs = {}) { + if (attrs !== null && typeof attrs !== 'object') { + throw new TypeError('attrs must be an object'); + } + attrs.config = merge( { sdkType: 'encryption', }, config, attrs.config ); // eslint-disable-line no-param-reassign return new Webex(attrs); };packages/@webex/plugin-encryption/browsers.js (1)
17-29: Extract repeated Chrome/Edge configuration flags.The Chrome and Edge configurations share identical flags. Consider extracting these into a shared constant to improve maintainability.
+const SHARED_BROWSER_FLAGS = [ + '--disable-features=WebRtcHideLocalIpsWithMdns', + '--use-fake-device-for-media-stream', + '--use-fake-ui-for-media-stream', +]; sl_chrome_latest_macOS_Catalina: { // ... 'goog:chromeOptions': { - args: [ - '--disable-features=WebRtcHideLocalIpsWithMdns', - '--use-fake-device-for-media-stream', - '--use-fake-ui-for-media-stream', - ], + args: [...SHARED_BROWSER_FLAGS], }, flags: [...SHARED_BROWSER_FLAGS], }, sl_edge_latest_Win_10: { // ... 'ms:edgeOptions': { - args: [ - '--disable-features=WebRtcHideLocalIpsWithMdns', - '--use-fake-device-for-media-stream', - '--use-fake-ui-for-media-stream', - ], + args: [...SHARED_BROWSER_FLAGS], }, },Also applies to: 36-43, 90-95
packages/@webex/plugin-encryption/test/unit/spec/cypher/index.ts (1)
39-87: Consider adding edge cases to the test suite.The current test coverage is good but could be enhanced with additional edge cases:
- Test with empty/malformed JWE
- Test with invalid keyUri format
- Test with very large files
- Test with various ArrayBuffer sizes
Example test case:
it('should throw an error if JWE is malformed', async () => { const malformedFileUri = 'https://example.com/encrypted-file?JWE=invalid&keyUri=kms://example.com/keys/1234'; await expect(cypher.downloadAndDecryptFile(malformedFileUri, options)) .rejects.toThrow('Invalid JWE format'); });docs/samples/plugin-encryption/app.js (1)
133-136: Enhance error handling with detailed messages.The current error handling only shows a generic error message. Consider providing more specific error details to help users troubleshoot issues.
catch (error) { console.error('error decrypting file', error); - decryptFileResult.innerText = ': error'; + decryptFileResult.innerText = `: error - ${error.message || 'Failed to decrypt file'}`; }packages/@webex/internal-plugin-encryption/src/encryption.js (1)
201-216: Enhanced error handling in encryptScr method.The changes improve error handling by:
- Validating the key.jwk before use
- Ensuring successful SCR instance creation
- Adding descriptive error messages and logging
Consider adding error codes to make error handling more systematic in client code.
- throw new Error('Invalid key or JWK'); + throw new Error('ERR_INVALID_KEY: Invalid key or JWK'); - throw new Error('Failed to create SCR instance'); + throw new Error('ERR_SCR_CREATION: Failed to create SCR instance');docs/samples/calling/app.js (1)
181-184: Implement the TODO for token refresh.The error handling for 401 errors is incomplete. The TODO comment indicates that access token refresh and retry logic should be implemented.
Would you like me to help implement the token refresh and retry logic for 401 errors?
packages/@webex/plugin-encryption/README.md (1)
35-50: Consider enhancing the sample code section.While the current example demonstrates basic file decryption, it would be helpful to include:
- Error handling best practices
- Examples of other common encryption operations
- Configuration options for different environments
Would you like me to provide additional code examples covering these scenarios?
packages/@webex/plugin-encryption/developer-quickstart.md (1)
22-22: Format the URL properly in markdown.The bare URL should be properly formatted in markdown.
-Navigate to https://localhost:8000/samples/plugin-encryption/ +Navigate to [https://localhost:8000/samples/plugin-encryption/](https://localhost:8000/samples/plugin-encryption/)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
22-22: Bare URL used
null(MD034, no-bare-urls)
docs/samples/plugin-encryption/index.html (1)
1-87: Improve accessibility.The page needs several accessibility improvements:
- Missing ARIA labels
- Missing form validation feedback
- Missing loading states
Apply these changes to improve accessibility:
- <button id="enableProduction" type="button" class="btn-code" onclick=changeEnv()>In Production</button> + <button id="enableProduction" type="button" class="btn-code" onclick=changeEnv() aria-pressed="true">In Production</button> - <pre id="access-token-status">Not initialized</pre> + <pre id="access-token-status" role="status" aria-live="polite">Not initialized</pre> - File URL: <input id="encrypted-file-url" value="" type="string"> + <label for="encrypted-file-url">File URL:</label> + <input id="encrypted-file-url" + value="" + type="url" + aria-label="Encrypted File URL" + aria-required="true"> - File Name: <input id="decrypted-file-name" value="my-decrypted-file.jpeg" type="string"> + <label for="decrypted-file-name">File Name:</label> + <input id="decrypted-file-name" + value="my-decrypted-file.jpeg" + type="string" + aria-label="Decrypted File Name" + aria-required="true"> - File Type: <select name="mime-types" id="mime-types"> + <label for="mime-types">File Type:</label> + <select name="mime-types" + id="mime-types" + aria-label="File MIME Type">docs/samples/plugin-encryption/style.css (2)
74-74: Remove duplicate font names in font-family declarations.The font-family declarations contain redundant font names.
- font-family: CiscoSansTT Regular, Helvetica Neue, Helvetica, sans-serif; + font-family: CiscoSansTT Regular, Helvetica Neue, sans-serif;Also applies to: 144-144, 351-351
🧰 Tools
🪛 Biome (1.9.4)
[error] 74-74: Duplicate font names are redundant and unnecessary: Helvetica
Remove duplicate font names within the property
(lint/suspicious/noDuplicateFontNames)
271-275: Standardize CSS units.The file uses a mix of px and rem units. Consider standardizing to rem for consistency.
- border:1px solid black; - padding: 5px; + border: 0.0625rem solid black; + padding: 0.3125rem; - margin: 1.5625 0; + margin: 1.5625rem 0; - padding: 1.25rem 1.25rem; + padding: 1.25rem; - border-bottom: 0.063rem solid #dddddd; + border-bottom: 0.0625rem solid #dddddd; - border-bottom: 0.125rem solid #009879; + border-bottom: 0.125rem solid #009879;Also applies to: 283-283, 298-299, 302-302, 310-310
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (31)
docs/index.html(1 hunks)docs/samples/browser-plugin-meetings/app.js(1 hunks)docs/samples/calling/app.js(4 hunks)docs/samples/plugin-encryption/app.js(1 hunks)docs/samples/plugin-encryption/index.html(1 hunks)docs/samples/plugin-encryption/style.css(1 hunks)package.json(2 hunks)packages/@webex/internal-plugin-encryption/src/encryption.js(3 hunks)packages/@webex/plugin-encryption/.eslintrc.js(1 hunks)packages/@webex/plugin-encryption/LICENSE(1 hunks)packages/@webex/plugin-encryption/README.md(1 hunks)packages/@webex/plugin-encryption/babel.config.js(1 hunks)packages/@webex/plugin-encryption/browsers.js(1 hunks)packages/@webex/plugin-encryption/developer-quickstart.md(1 hunks)packages/@webex/plugin-encryption/jest.config.js(1 hunks)packages/@webex/plugin-encryption/package.json(1 hunks)packages/@webex/plugin-encryption/process(1 hunks)packages/@webex/plugin-encryption/src/config.ts(1 hunks)packages/@webex/plugin-encryption/src/cypher/constants.ts(1 hunks)packages/@webex/plugin-encryption/src/cypher/cypher.types.ts(1 hunks)packages/@webex/plugin-encryption/src/cypher/index.ts(1 hunks)packages/@webex/plugin-encryption/src/index.ts(1 hunks)packages/@webex/plugin-encryption/src/types.ts(1 hunks)packages/@webex/plugin-encryption/test/unit/spec/cypher/index.ts(1 hunks)packages/@webex/plugin-encryption/tsconfig.json(1 hunks)packages/@webex/test-helper-mock-webex/src/index.js(1 hunks)packages/webex/package.json(2 hunks)packages/webex/src/encryption.js(1 hunks)packages/webex/src/webex.js(1 hunks)tooling/commands/test.js(1 hunks)webpack.config.js(2 hunks)
✅ Files skipped from review due to trivial changes (7)
- packages/@webex/plugin-encryption/.eslintrc.js
- packages/@webex/plugin-encryption/src/config.ts
- packages/@webex/plugin-encryption/babel.config.js
- packages/@webex/plugin-encryption/process
- packages/@webex/plugin-encryption/LICENSE
- packages/@webex/plugin-encryption/tsconfig.json
- docs/samples/browser-plugin-meetings/app.js
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
packages/@webex/plugin-encryption/developer-quickstart.md
22-22: Bare URL used
null
(MD034, no-bare-urls)
🪛 Biome (1.9.4)
docs/samples/plugin-encryption/style.css
[error] 74-74: Duplicate font names are redundant and unnecessary: Helvetica
Remove duplicate font names within the property
(lint/suspicious/noDuplicateFontNames)
[error] 144-144: Duplicate font names are redundant and unnecessary: Helvetica
Remove duplicate font names within the property
(lint/suspicious/noDuplicateFontNames)
[error] 351-351: Duplicate font names are redundant and unnecessary: Helvetica
Remove duplicate font names within the property
(lint/suspicious/noDuplicateFontNames)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Initialize Project
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (26)
packages/@webex/plugin-encryption/src/cypher/index.ts (1)
11-14: Interface implementation looks good.By extending
WebexPluginand implementing theIEncryptioninterface, we ensure a consistent contract for encryption-related features. Thenamespaceproperty also keeps the plugin well compartmentalized.packages/@webex/plugin-encryption/src/index.ts (1)
7-9: Plugin registration appears correct.The call to
registerPlugin('cypher', Cypher, { config })properly integrates the plugin into the Webex framework. This ensures the encryption capabilities are accessible throughout the application.packages/@webex/plugin-encryption/src/cypher/constants.ts (1)
1-5: Naming consistency is clear.These constants provide a clear, centralized reference for decryption states. The uppercase identifiers are conventional for constants, and the string values align with the encryption/decryption domain. Overall, this is straightforward and well-structured.
packages/@webex/plugin-encryption/src/cypher/cypher.types.ts (1)
4-23: LGTM! Well-structured interface with clear documentation.The
FileDownloadOptionsinterface is well-documented with clear JSDoc comments explaining each property's purpose and requirements.packages/@webex/plugin-encryption/src/types.ts (1)
4-11: LGTM! Well-defined Logger type with comprehensive methods.The Logger type includes all standard logging levels with consistent method signatures.
packages/webex/src/encryption.js (1)
9-13: LGTM! Proper polyfill check.The code correctly checks for and includes the Babel polyfill when needed.
packages/@webex/plugin-encryption/jest.config.js (1)
11-18:✅ Verification successful
Consider a gradual increase in coverage thresholds.
Setting an 85% coverage threshold for a new plugin might be too aggressive initially. Consider starting with a lower threshold (e.g., 70%) and gradually increasing it as the codebase matures.
🌐 Web query:
What are the recommended initial code coverage thresholds for new projects?💡 Result:
For new software projects, industry recommendations for initial code coverage thresholds generally fall between 80-100%, depending on project goals and testing philosophies:
Core Recommendations
80% Minimum for New Code
- SonarQube’s built-in Quality Gate enforces 80% coverage on new code (added/modified code) as a baseline[1][8].
- Codecov’s intermediate-level teams target 80% coverage for new features to balance rigor and practicality[4].
100% Aspiration for Maximum Confidence
- Very Good Ventures advocates enforcing 100% coverage by default for new projects to ensure every line is tested[7].
- Google recommends 90-99% per-commit coverage as exemplary, though project-wide averages may vary[9].
Implementation Strategies
Approach Threshold Use Case Tool Example New Code Focus 80% Legacy-heavy projects or gradual adoption SonarQube[1], Codecov[4] Full Coverage 100% Greenfield projects or mission-critical systems Very Good Coverage[7] Safety Net ~70% CI failure threshold Teams building CI/CD guardrails Atlassian’s guidance[8] Key Considerations
- Critical vs. Non-Critical Code: Prioritize higher thresholds (90-100%) for security-sensitive or business-critical logic[2][5].
- Tool Integration: Automate coverage tracking in CI/CD pipelines (e.g., Codecov’s
target: autosetting)[4].- Avoid Gaming Metrics: Pair coverage with other quality practices like mutation testing and peer reviews[2][5].
As Ann Campbell from SonarSource notes: “Focusing on high new-code coverage naturally improves overall quality over time”[1]. Start strict (80-100%) while the codebase is small, then adjust based on evolving complexity[6][8].
Citations:
- 1: https://community.sonarsource.com/t/best-practices-for-increasing-code-coverage/21423
- 2: https://launchdarkly.com/blog/code-coverage-what-it-is-and-why-it-matters/
- 3: https://dev.to/d_ir/do-you-aim-for-80-code-coverage-let-me-guess-which-80-it-is-1fj9
- 4: https://about.codecov.io/blog/the-5-levels-of-code-coverage-how-to-build-a-testing-culture-in-your-organization/
- 5: https://www.browserstack.com/guide/code-coverage-techniques
- 6: https://www.qodo.ai/question/what-is-a-reasonable-code-coverage-for-unit-tests-and-why/
- 7: https://verygood.ventures/blog/very-good-coverage
- 8: https://www.atlassian.com/continuous-delivery/software-testing/code-coverage
- 9: https://testing.googleblog.com/2020/08/code-coverage-best-practices.html
Coverage Thresholds in Plugin-Encryption
After reviewing industry recommendations, initial code coverage thresholds for new projects are commonly set between 80% and 100%—with many tools enforcing at least an 80% baseline on new or modified code. The current threshold of 85% fits well within these guidelines. While a gradual, phased approach to increasing coverage can be beneficial as the codebase matures, there’s no strong need to lower the threshold to 70% at this stage unless you encounter specific challenges maintaining such high coverage early on.
- Current Setting: 85% global threshold, which is in line with industry practices.
- Recommendation: Continue with the current threshold; consider a gradual adjustment only if testing challenges arise as the plugin evolves.
packages/webex/src/webex.js (1)
30-30: LGTM!The addition of the encryption plugin follows the established pattern and is correctly placed among other plugin imports.
packages/@webex/plugin-encryption/test/unit/spec/cypher/index.ts (1)
6-38: LGTM! Clean and well-structured test setup.The mock setup is comprehensive and follows best practices. The beforeEach and afterEach hooks ensure proper test isolation.
docs/samples/plugin-encryption/app.js (2)
24-28: Enhance token storage security.Storing access tokens in localStorage poses security risks. Consider using more secure storage methods or implementing encryption.
51-96: LGTM! Well-structured initialization logic.The Webex initialization logic is clean, handles environment configuration properly, and includes appropriate status updates.
webpack.config.js (1)
42-48: LGTM! New entry point for encryption plugin.The new entry point follows the established pattern and correctly configures the library name and type.
tooling/commands/test.js (1)
171-171: LGTM! Added encryption plugin to Mocha test runner.The change correctly includes the encryption plugin in the list of packages that run on Mocha, alongside other core plugins.
packages/@webex/internal-plugin-encryption/src/encryption.js (3)
75-75: LGTM! Method marked as async.The async keyword improves code readability and makes the asynchronous nature of the method explicit.
82-82: LGTM! Default useFileService option.Setting a default value for useFileService ensures consistent behavior when the option is not provided.
121-129: LGTM! Added HTTPS validation for direct file URLs.The security check ensures that direct file URLs use HTTPS, preventing potential security vulnerabilities.
packages/@webex/test-helper-mock-webex/src/index.js (1)
18-18: LGTM! Added cypher to external plugins list.The change correctly categorizes the cypher plugin as an external plugin in the mock Webex instance.
docs/samples/calling/app.js (2)
234-234: LGTM! Service endpoint updated.The hydra service endpoint URL has been updated from
apialpha.ciscospark.comtohydra-intb.ciscospark.com.
723-723: LGTM! Environment-aware configuration.The noise reduction effect options now consider the environment (prod/int) when initializing.
packages/@webex/plugin-encryption/package.json (2)
19-22: Test script needs correction.The
test:brokenscript includes browser tests, but thetest:browserscript is marked as broken. This inconsistency should be resolved.
30-35: LGTM! Well-structured dependencies.The dependencies are properly structured using workspace references for internal packages and a specific version for external dependency.
packages/@webex/plugin-encryption/developer-quickstart.md (1)
57-76: LGTM! Well-implemented file decryption.The decryption function includes proper error handling, cleanup of object URLs, and clear console messages.
packages/webex/package.json (1)
20-20: LGTM!The changes correctly integrate the new encryption plugin by:
- Adding the export path for the encryption module
- Including the plugin as a workspace dependency
Also applies to: 82-82
docs/samples/plugin-encryption/index.html (1)
55-55: Add input validation for file URL.The file URL input should validate the URL format and restrict to allowed domains.
docs/index.html (1)
85-86: LGTM!The new link to the encryption plugin sample is correctly added to the samples list.
package.json (1)
30-30: LGTM!The build scripts are correctly updated to include the encryption plugin in:
- Legacy build command
- TypeScript build command
- Skip samples build command
Also applies to: 40-40, 42-42
| <div class="u-mv"> | ||
| <p class="note">Initializes webex object and registers Webex JS SDK as a device.</p> | ||
| <button id="enableProduction" type="button" class="btn-code" onclick=changeEnv()>In Production</button><span class="text-color"> ( Click to change the environment )</span> | ||
| <input id="access-token" name="accessToken" placeholder="Access Token" value="" type="text" style="margin: 1rem 0 0.5rem 0;" required> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for access token.
The access token input should validate the token format and include security best practices.
- <input id="access-token" name="accessToken" placeholder="Access Token" value="" type="text" style="margin: 1rem 0 0.5rem 0;" required>
+ <input id="access-token"
+ name="accessToken"
+ placeholder="Access Token"
+ value=""
+ type="password"
+ autocomplete="off"
+ pattern="^[A-Za-z0-9-_=]+\.[A-Za-z0-9-_=]+\.?[A-Za-z0-9-_.+/=]*$"
+ title="Please enter a valid JWT token"
+ style="margin: 1rem 0 0.5rem 0;"
+ required>📝 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.
| <input id="access-token" name="accessToken" placeholder="Access Token" value="" type="text" style="margin: 1rem 0 0.5rem 0;" required> | |
| <input id="access-token" | |
| name="accessToken" | |
| placeholder="Access Token" | |
| value="" | |
| type="password" | |
| autocomplete="off" | |
| pattern="^[A-Za-z0-9-_=]+\.[A-Za-z0-9-_=]+\.?[A-Za-z0-9-_.+/=]*$" | |
| title="Please enter a valid JWT token" | |
| style="margin: 1rem 0 0.5rem 0;" | |
| required> |
| <button type="button" id="decrypt-my-file-btn" onclick="decryptFile()" class="btn" style="display: block;"> | ||
| <div>Download and decrypt my file<span id="decrypt-file-result"></span> </div> | ||
| </button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling UI for decryption failures.
The decryption button should include error handling UI to display any failures during the download or decryption process.
- <button type="button" id="decrypt-my-file-btn" onclick="decryptFile()" class="btn" style="display: block;">
- <div>Download and decrypt my file<span id="decrypt-file-result"></span> </div>
- </button>
+ <button type="button" id="decrypt-my-file-btn" onclick="decryptFile()" class="btn" style="display: block;">
+ <div>
+ Download and decrypt my file
+ <span id="decrypt-file-result"></span>
+ <span id="decrypt-file-error" class="error-message" aria-live="polite"></span>
+ </div>
+ </button>📝 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.
| <button type="button" id="decrypt-my-file-btn" onclick="decryptFile()" class="btn" style="display: block;"> | |
| <div>Download and decrypt my file<span id="decrypt-file-result"></span> </div> | |
| </button> | |
| <button type="button" id="decrypt-my-file-btn" onclick="decryptFile()" class="btn" style="display: block;"> | |
| <div> | |
| Download and decrypt my file | |
| <span id="decrypt-file-result"></span> | |
| <span id="decrypt-file-error" class="error-message" aria-live="polite"></span> | |
| </div> | |
| </button> |
| border-radius: 1.125rem; | ||
| height: 2.25rem; | ||
| padding: 0.375rem 1.125rem; | ||
| color: #000; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify color contrast for accessibility.
Some color combinations might not meet WCAG contrast requirements:
- Button text color (#000) on background (#dedede)
- White text on #f7644a background
- White text on #009879 background
Also applies to: 119-121, 290-293
🌐 Web query:
What is the color contrast ratio for:
1. Black text (#000000) on #dedede background
2. White text (#ffffff) on #f7644a background
3. White text (#ffffff) on #009879 background
Are these ratios WCAG AA compliant?
💡 Result:
The color contrast ratios and WCAG compliance status are calculated using the formula ( \frac{L_1 + 0{.}05}{L_2 + 0{.}05} ), where ( L_1 ) and ( L_2 ) represent relative luminance values of lighter/darker colors[7][10]. WCAG AA requires ≥4{.}5:¹ for normal text and ≥3:¹ for large text (>18pt or bold ≥14pt)[2][5][8]. Here are the results:
Black (#000000) on #DEDEDE Background
- Contrast Ratio:
19{.}56**:¹**
(Example comparison: Black/Yellow achieves 19{.}56:¹[3][8]) - WCAG Compliance:
Passes AA (19{.}56:¹ > 4{.}5:¹)[8].
White (#FFFFFF) on #F7644A Background
- Contrast Ratio:
Approximately 3{.}9:¹
(Similar example: Blue/Orange achieves 4{.}35:¹[3]). - WCAG Compliance:
- Fails AA for normal text (<4{.}5:¹).
- Passes AA for large text (≥3:¹)[8].
White (#FFFFFF) on #009879 Background
- Contrast Ratio:
Approximately 5{.}16:¹
(Example comparison: Light Green/Dark Green achieves 5{.}16:¹[10]). - WCAG Compliance:
Passes AA (≥4{.}5:¹)[10].
Summary Table
| Color Pair | Contrast Ratio | Normal Text | Large Text |
|---|---|---|---|
| Black / #DEDEDE | 19{.}56:¹ | ✅ | ✅ |
| White / #F7644A | ~3{.}9:¹ | ❌ | ✅ |
| White / #009879 | ~5{.}16:¹ | ✅ | ✅ |
Key Notes:
- Use tools like WebAIM’s Contrast Checker[8] or Deque University’s table[3] for precise calculations.
- Always verify ratios programmatically or via accessibility tools during design workflows[7][9].
Citations:
- 1: https://www.accessibility-developer-guide.com/knowledge/colours-and-contrast/how-to-calculate/
- 2: https://dequeuniversity.com/checklists/web/color-contrast
- 3: https://dequeuniversity.com/tips/color-contrast
- 4: https://dev.to/alvaromontoro/building-your-own-color-contrast-checker-4j7o
- 5: https://www.washington.edu/accesstech/checklist/contrast/
- 6: https://www.hhs.gov/sites/default/files/os-a11y-color-contrast-reference.pdf
- 7: https://www.youtube.com/watch?v=-chsX0O76aY
- 8: https://webaim.org/resources/contrastchecker/
- 9: https://216digital.com/what-is-color-contrast-and-how-does-it-affect-your-web-accessibility/
- 10: https://www.101computing.net/colour-luminance-and-contrast-ratio/
Accessibility Contrast Ratios – Action Required
- In
docs/samples/plugin-encryption/style.css(lines 85, 119–121, 290–293):- Black (#000) on #dedede: Contrast ratio is ~19.56, which comfortably meets WCAG AA requirements.
- White (#FFF) on #009879: Contrast ratio is approximately 5.16, compliant for normal text.
- White (#FFF) on #F7644A: Contrast ratio is about 3.9, failing WCAG AA criteria for normal text (requires ≥4.5) but passing for large text (≥3).
Please verify whether the white text on the #F7644A background is used for normal-sized text. If so, consider adjusting the color contrast (or text size) to ensure full accessibility compliance.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
docs/samples/plugin-encryption/index.html (2)
38-38: 🛠️ Refactor suggestionAdd input validation for access token.
The access token input should validate the token format and include security best practices.
- <input id="access-token" name="accessToken" placeholder="Access Token" value="" type="text" style="margin: 1rem 0 0.5rem 0;" required> + <input id="access-token" + name="accessToken" + placeholder="Access Token" + value="" + type="password" + autocomplete="off" + pattern="^[A-Za-z0-9-_=]+\.[A-Za-z0-9-_=]+\.?[A-Za-z0-9-_.+/=]*$" + title="Please enter a valid JWT token" + style="margin: 1rem 0 0.5rem 0;" + required>
57-57: 🛠️ Refactor suggestionAdd input validation for file URL.
The file URL input should validate the URL format and restrict to allowed domains.
- File URL: <input id="encrypted-file-url" value="" type="string" style="margin-bottom: 0.5rem;"> + File URL: <input id="encrypted-file-url" value="" type="url" + pattern="https://.*\.webex\.com/.*" + title="Please enter a valid Webex file URL" + aria-label="Encrypted File URL" + style="margin-bottom: 0.5rem;" + required>docs/samples/plugin-encryption/app.js (2)
26-30:⚠️ Potential issueEnhance token storage security.
Storing access tokens in localStorage poses security risks. Consider using more secure storage methods or implementing encryption.
Consider using session storage or implementing encryption:
-if (localStorage.getItem('date') > new Date().getTime()) { - tokenElm.value = localStorage.getItem('access-token'); -} else { - localStorage.removeItem('access-token'); -} +// Use sessionStorage for better security +if (sessionStorage.getItem('date') > new Date().getTime()) { + tokenElm.value = sessionStorage.getItem('access-token'); +} else { + sessionStorage.removeItem('access-token'); +}
132-140: 🛠️ Refactor suggestionImprove input validation for file URL and MIME type.
The validation could be more robust by checking URL format and validating MIME types against a whitelist.
Add stronger validation:
-if (!fileUrl) { +const validMimeTypes = ['application/pdf', 'image/jpeg', 'image/png']; +if (!fileUrl || !fileUrl.startsWith('https://')) { decryptFileResult.innerText = ': error - Invalid file URL'; return; } -if (!mimeType) { +if (!mimeType || !validMimeTypes.includes(mimeType)) { decryptFileResult.innerText = ': error - Invalid MIME type'; return; }
🧹 Nitpick comments (6)
docs/samples/plugin-encryption/index.html (1)
84-85: Consider adding integrity and cross-origin attributes to script tags.For better security, add integrity checks and appropriate CORS settings to external script references.
- <script src="../webex.min.js"></script> - <script src="app.js"></script> + <script src="../webex.min.js" integrity="sha384-..." crossorigin="anonymous"></script> + <script src="app.js" integrity="sha384-..." crossorigin="anonymous"></script>Note: Replace "sha384-..." with actual hash values generated for these files.
packages/@webex/plugin-encryption/developer-quickstart.md (1)
93-95: Fix malformed license URL.The license URL contains nested markdown syntax which will not render correctly.
-This project is licensed under the Cisco General Terms - see the [LICENSE]([https://www.cisco.com/c/en/us/products/end-user-license-agreement.html](https://www.cisco.com/c/dam/en_us/about/doing_business/legal/Cisco_General_Terms.pdf)) for details. +This project is licensed under the Cisco General Terms - see the [LICENSE](https://www.cisco.com/c/dam/en_us/about/doing_business/legal/Cisco_General_Terms.pdf) for details.packages/@webex/plugin-encryption/src/cypher/index.ts (1)
11-29: Consider improving TypeScript integration.The class structure looks good, but there are a few areas for improvement:
- The use of
@ts-ignoresuggests potential type safety issues- Constructor arguments use
any[]which reduces type safetyConsider refactoring to improve type safety:
- /** - * Constructs an instance of the class. - * - * - * @param {...any[]} args - The arguments to pass to the superclass constructor. - * - * - * @remarks - * This constructor calls the superclass constructor with the provided arguments. - * It also assigns the `webex` property to the `$webex` property, ignoring TypeScript errors. - */ - constructor(...args: any[]) { - super(...args); - // @ts-ignore - this.$webex = this.webex; - } + /** + * Constructs an instance of the class. + * + * @param {object} options - The options object for the plugin. + */ + constructor(options: object) { + super(options); + this.$webex = this.webex as WebexSDK; + }docs/samples/plugin-encryption/app.js (2)
100-124: Improve promise handling in register and deregister functions.Both functions use async/await but then switch to promise chaining with .then() and .catch(). This mixing of styles is confusing and doesn't properly utilize the async/await pattern.
Refactor to use consistent async/await:
async function register(){ - webex.cypher.register().then(() => { - console.log('Authentication#initWebex() :: Webex Registered'); - authStatusElm.innerText = 'Webex is ready and registered!'; - updateStatus(true); - registerBtn.disabled = true; - deregisterBtn.disabled = false; - }).catch((err) => { - console.error(`error registering webex: ${err}`); - authStatusElm.innerText = 'Error registering Webex. Check access token!'; - }); + try { + await webex.cypher.register(); + console.log('Authentication#initWebex() :: Webex Registered'); + authStatusElm.innerText = 'Webex is ready and registered!'; + updateStatus(true); + registerBtn.disabled = true; + deregisterBtn.disabled = false; + } catch (err) { + console.error(`error registering webex: ${err}`); + authStatusElm.innerText = 'Error registering Webex. Check access token!'; + } } async function deregister(){ - webex.cypher.deregister().then(() => { - console.log('Authentication#initWebex() :: Webex Deregistered'); - authStatusElm.innerText = 'Webex is ready, but not registered!'; - updateStatus(false); - registerBtn.disabled = false; - deregisterBtn.disabled = true; - }).catch((err) => { - console.error(`error deregistering webex: ${err}`); - authStatusElm.innerText = 'Error deregistering Webex. Check access token!'; - }); + try { + await webex.cypher.deregister(); + console.log('Authentication#initWebex() :: Webex Deregistered'); + authStatusElm.innerText = 'Webex is ready, but not registered!'; + updateStatus(false); + registerBtn.disabled = false; + deregisterBtn.disabled = true; + } catch (err) { + console.error(`error deregistering webex: ${err}`); + authStatusElm.innerText = 'Error deregistering Webex. Check access token!'; + } }
142-162: Add file size validation and improve user feedback.Consider adding validation for the file size and providing more detailed user feedback about decryption progress.
let objectUrl; try { + decryptFileResult.innerText = ': downloading...'; const decryptedBuf = await webex.cypher.downloadAndDecryptFile(fileUrl); + + // Check file size - prevent unnecessarily large files + if (decryptedBuf.byteLength > 50 * 1024 * 1024) { // 50MB limit + decryptFileResult.innerText = ': error - File too large'; + return; + } + + decryptFileResult.innerText = ': creating file...'; const file = new File([decryptedBuf], encryptedFileName, {type: mimeType}); objectUrl = URL.createObjectURL(file); const a = document.createElement("a"); a.href = objectUrl; a.download = file.name || "download"; document.body.appendChild(a); a.click(); document.body.removeChild(a); decryptFileResult.innerText = ': success'; } catch (error) { console.error('error decrypting file', error); - decryptFileResult.innerText = ': error'; + decryptFileResult.innerText = `: error - ${error.message || 'Failed to decrypt file'}`; } finally { if (objectUrl) { URL.revokeObjectURL(objectUrl); } }packages/@webex/plugin-encryption/test/unit/spec/cypher/index.ts (1)
55-55: Fix spacing in object spread syntax.There's an extra space after the comma in the object spread syntax.
- const customOptions = {...options ,keyUri: 'kms://example.com/keys/1234', jwe: 'eyJhbGci'}; + const customOptions = {...options, keyUri: 'kms://example.com/keys/1234', jwe: 'eyJhbGci'};
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
docs/samples/calling/app.js(4 hunks)docs/samples/plugin-encryption/app.js(1 hunks)docs/samples/plugin-encryption/index.html(1 hunks)packages/@webex/plugin-encryption/README.md(1 hunks)packages/@webex/plugin-encryption/developer-quickstart.md(1 hunks)packages/@webex/plugin-encryption/package.json(1 hunks)packages/@webex/plugin-encryption/src/cypher/index.ts(1 hunks)packages/@webex/plugin-encryption/src/cypher/types.ts(1 hunks)packages/@webex/plugin-encryption/src/types.ts(1 hunks)packages/@webex/plugin-encryption/test/unit/spec/cypher/index.ts(1 hunks)packages/webex/src/encryption.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/@webex/plugin-encryption/package.json
- docs/samples/calling/app.js
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
packages/@webex/plugin-encryption/README.md
58-58: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Initialize Project
🔇 Additional comments (17)
packages/@webex/plugin-encryption/src/types.ts (3)
4-11: LGTM! Logger type definition is well-structured.The Logger type provides a comprehensive set of logging methods with consistent parameter signatures.
13-30: LGTM! The IWebexInternal interface is clear and well-documented.The interface cleanly separates mercury, device, and encryption functionality, with appropriate method signatures and return types.
32-45: LGTM! The WebexSDK interface definition is comprehensive.The interface captures all necessary properties and methods for the SDK, with appropriate typing and documentation comments.
packages/webex/src/encryption.js (3)
9-13: LGTM! Appropriate polyfill handling.The code correctly checks for and loads Babel polyfill if needed, ensuring compatibility across different environments.
15-17: LGTM! Proper plugin dependencies.The code correctly requires both the internal encryption plugin (marked as required) and the public encryption plugin.
29-39: LGTM! Well-implemented initialization method.The
initmethod properly merges configurations with the correct precedence order, and sets the SDK type to 'encryption'.docs/samples/plugin-encryption/index.html (1)
74-76: Add error handling UI for decryption failures.The decryption button should include error handling UI to display any failures during the download or decryption process.
- <button type="button" id="decrypt-my-file-btn" onclick="decryptFile()" class="btn" style="display: block;"> - <div>Download and decrypt my file<span id="decrypt-file-result"></span> </div> - </button> + <button type="button" id="decrypt-my-file-btn" onclick="decryptFile()" class="btn" style="display: block;"> + <div> + Download and decrypt my file + <span id="decrypt-file-result"></span> + <span id="decrypt-file-error" class="error-message" aria-live="polite"></span> + </div> + </button>packages/@webex/plugin-encryption/src/cypher/types.ts (2)
4-23: Well-structured interface with appropriate documentation.The
FileDownloadOptionsinterface is well defined with clear JSDoc comments explaining each optional parameter. The naming is intuitive and the optionality of parameters is appropriate for the use case.
24-26: Well-defined encryption interface.The
IEncryptioninterface correctly defines the contract for the downloadAndDecryptFile method with appropriate parameter types and return type.packages/@webex/plugin-encryption/developer-quickstart.md (1)
1-92: Well-structured documentation with clear examples.The documentation provides a comprehensive guide for using the Webex encryption service, with clear sections on prerequisites, installation, and usage. The code examples are well-formatted and include helpful comments.
packages/@webex/plugin-encryption/src/cypher/index.ts (1)
89-135: Well-implemented file download and decryption method.The method handles parameter validation, error cases, and has good documentation with examples. Parsing of the URL parameters is done correctly, and appropriate error messages are provided.
One minor suggestion to improve error message security:
throw new Error( - `Failed to decrypt the JWE: ${(error as Error).message}\nStack: ${(error as Error).stack}` + `Failed to decrypt the JWE: ${(error as Error).message}` );Including the full stack trace in error messages could potentially expose sensitive information to clients.
packages/@webex/plugin-encryption/test/unit/spec/cypher/index.ts (6)
1-5: Well-structured imports and dependencies.The imports are clean and properly organized, separating external dependencies from local imports.
6-39: Good test setup with comprehensive mocking.The test setup correctly uses MockWebex and properly mocks all necessary methods and components. The beforeEach and afterEach hooks ensure proper test isolation.
41-89: Comprehensive test coverage for downloadAndDecryptFile method.The tests for this method are thorough, covering:
- Error cases when required parameters are missing
- Parameter extraction from different sources
- Successful decryption and download
- Error handling for both decryption and download failures
The tests ensure that the method integrates correctly with the internal encryption module.
91-112: Complete test coverage for device registration.The tests cover all important scenarios including successful registration, already registered devices, and error handling.
114-137: Well-structured tests for device deregistration.The tests appropriately verify deregistration behavior in various scenarios, including error handling and the case when a device is not registered.
1-138: Excellent overall test structure and implementation.This test file demonstrates good testing practices:
- Proper setup and cleanup
- Comprehensive test coverage for all methods
- Testing of edge cases and error scenarios
- Clear test descriptions that explain what each test verifies
- Appropriate mocking of dependencies
The tests will effectively validate the functionality of the Cypher class in the encryption plugin.
|
|
||
| ## License | ||
|
|
||
| This project is licensed under the Cisco General Terms - see the [LICENSE]([https://www.cisco.com/c/en/us/products/end-user-license-agreement.html](https://www.cisco.com/c/dam/en_us/about/doing_business/legal/Cisco_General_Terms.pdf)) for details. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix malformed license link.
The license link is improperly formatted with nested square brackets, which will cause rendering issues.
-This project is licensed under the Cisco General Terms - see the [LICENSE]([https://www.cisco.com/c/en/us/products/end-user-license-agreement.html](https://www.cisco.com/c/dam/en_us/about/doing_business/legal/Cisco_General_Terms.pdf)) for details.
+This project is licensed under the Cisco General Terms - see the [LICENSE](https://www.cisco.com/c/dam/en_us/about/doing_business/legal/Cisco_General_Terms.pdf) for details.📝 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.
| This project is licensed under the Cisco General Terms - see the [LICENSE]([https://www.cisco.com/c/en/us/products/end-user-license-agreement.html](https://www.cisco.com/c/dam/en_us/about/doing_business/legal/Cisco_General_Terms.pdf)) for details. | |
| This project is licensed under the Cisco General Terms - see the [LICENSE](https://www.cisco.com/c/dam/en_us/about/doing_business/legal/Cisco_General_Terms.pdf) for details. |
| }); | ||
| ``` | ||
|
|
||
| #### Development |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix heading level hierarchy.
The heading level jumps from h2 to h4, which breaks the document hierarchy. Heading levels should only increment by one level at a time.
-#### Development
+## DevelopmentCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
58-58: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
| async register() { | ||
| if (this.registered) { | ||
| this.$webex.logger.info('Cypher: webex.internal.device.register already done'); | ||
|
|
||
| return Promise.resolve(); | ||
| } | ||
|
|
||
| return this.$webex.internal.device | ||
| .register() | ||
| .then(() => { | ||
| this.$webex.logger.info('Cypher: webex.internal.device.register successful'); | ||
| this.registered = true; | ||
| }) | ||
| .catch((error) => { | ||
| this.$webex.logger.error(`Error occurred during device.register() ${error}`); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in register method.
The method catches errors but still resolves the promise successfully, which could lead to silent failures. The calling code will think registration was successful when it wasn't.
async register() {
if (this.registered) {
this.$webex.logger.info('Cypher: webex.internal.device.register already done');
return Promise.resolve();
}
return this.$webex.internal.device
.register()
.then(() => {
this.$webex.logger.info('Cypher: webex.internal.device.register successful');
this.registered = true;
})
.catch((error) => {
this.$webex.logger.error(`Error occurred during device.register() ${error}`);
+ throw error; // Re-throw the error to propagate it to the caller
});
}📝 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.
| async register() { | |
| if (this.registered) { | |
| this.$webex.logger.info('Cypher: webex.internal.device.register already done'); | |
| return Promise.resolve(); | |
| } | |
| return this.$webex.internal.device | |
| .register() | |
| .then(() => { | |
| this.$webex.logger.info('Cypher: webex.internal.device.register successful'); | |
| this.registered = true; | |
| }) | |
| .catch((error) => { | |
| this.$webex.logger.error(`Error occurred during device.register() ${error}`); | |
| }); | |
| } | |
| async register() { | |
| if (this.registered) { | |
| this.$webex.logger.info('Cypher: webex.internal.device.register already done'); | |
| return Promise.resolve(); | |
| } | |
| return this.$webex.internal.device | |
| .register() | |
| .then(() => { | |
| this.$webex.logger.info('Cypher: webex.internal.device.register successful'); | |
| this.registered = true; | |
| }) | |
| .catch((error) => { | |
| this.$webex.logger.error(`Error occurred during device.register() ${error}`); | |
| throw error; // Re-throw the error to propagate it to the caller | |
| }); | |
| } |
| async deregister() { | ||
| if (!this.registered) { | ||
| this.$webex.logger.info('Cypher: webex.internal.device.deregister already done'); | ||
|
|
||
| return Promise.resolve(); | ||
| } | ||
|
|
||
| return this.$webex.internal.device | ||
| .unregister() | ||
| .then(() => { | ||
| this.$webex.logger.info('Cypher: webex.internal.device.deregister successful'); | ||
| this.registered = false; | ||
| }) | ||
| .catch((error) => { | ||
| this.$webex.logger.error(`Error occurred during device.deregister() ${error}`); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in deregister method.
Similar to the register method, errors are caught and logged but the promise still resolves successfully, which could lead to silent failures.
async deregister() {
if (!this.registered) {
this.$webex.logger.info('Cypher: webex.internal.device.deregister already done');
return Promise.resolve();
}
return this.$webex.internal.device
.unregister()
.then(() => {
this.$webex.logger.info('Cypher: webex.internal.device.deregister successful');
this.registered = false;
})
.catch((error) => {
this.$webex.logger.error(`Error occurred during device.deregister() ${error}`);
+ throw error; // Re-throw the error to propagate it to the caller
});
}📝 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.
| async deregister() { | |
| if (!this.registered) { | |
| this.$webex.logger.info('Cypher: webex.internal.device.deregister already done'); | |
| return Promise.resolve(); | |
| } | |
| return this.$webex.internal.device | |
| .unregister() | |
| .then(() => { | |
| this.$webex.logger.info('Cypher: webex.internal.device.deregister successful'); | |
| this.registered = false; | |
| }) | |
| .catch((error) => { | |
| this.$webex.logger.error(`Error occurred during device.deregister() ${error}`); | |
| }); | |
| } | |
| async deregister() { | |
| if (!this.registered) { | |
| this.$webex.logger.info('Cypher: webex.internal.device.deregister already done'); | |
| return Promise.resolve(); | |
| } | |
| return this.$webex.internal.device | |
| .unregister() | |
| .then(() => { | |
| this.$webex.logger.info('Cypher: webex.internal.device.deregister successful'); | |
| this.registered = false; | |
| }) | |
| .catch((error) => { | |
| this.$webex.logger.error(`Error occurred during device.deregister() ${error}`); | |
| throw error; // Re-throw the error to propagate it to the caller | |
| }); | |
| } |
| function changeEnv() { | ||
| enableProd = !enableProd; | ||
| enableProduction.innerHTML = enableProd ? 'In Production' : 'In Integration'; | ||
| } |
There was a problem hiding this comment.
Fix undefined DOM element reference.
The function refers to enableProduction which is not defined in the provided code.
function changeEnv() {
enableProd = !enableProd;
- enableProduction.innerHTML = enableProd ? 'In Production' : 'In Integration';
+ // Ensure the element exists before updating it
+ const enableProductionElm = document.querySelector('#enableProduction');
+ if (enableProductionElm) {
+ enableProductionElm.innerHTML = enableProd ? 'In Production' : 'In Integration';
+ }
}📝 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.
| function changeEnv() { | |
| enableProd = !enableProd; | |
| enableProduction.innerHTML = enableProd ? 'In Production' : 'In Integration'; | |
| } | |
| function changeEnv() { | |
| enableProd = !enableProd; | |
| // Ensure the element exists before updating it | |
| const enableProductionElm = document.querySelector('#enableProduction'); | |
| if (enableProductionElm) { | |
| enableProductionElm.innerHTML = enableProd ? 'In Production' : 'In Integration'; | |
| } | |
| } |
COMPLETES #https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-580068
This pull request addresses
Creation of plugin-encryption
by making the following changes
Added new plugin-encryption
Change Type
The following scenarios were tested
Tested that we can download and decrypt files using the plugin
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.