feat(plugin-encryption): Create new plugin-encryption with file download and decrypt feature #4082
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request integrates an encryption plugin into the Cisco Webex JS SDK project. It introduces new documentation and sample files, including an HTML interface and JavaScript code for handling encrypted files, user authentication, and file decryption. The changes update service endpoints in existing sample applications and improve error handling and asynchronous operations. Configuration files such as package.json, tsconfig.json, and webpack.config.js have been modified to include the new plugin in build, testing, and deployment processes. New modules, interfaces, types, and ESLint configurations specific to the encryption plugin are added, along with corresponding test suites. Additionally, the pull request updates tooling files to accommodate the plugin during automated testing and adjusts licensing information and dependency declarations accordingly. 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (16)
packages/@webex/plugin-encryption/src/encryption/index.ts (2)
16-21: Consider removing or documenting thets-ignoreandany[]usage in the constructor.While this approach works, having
ts-ignorehere may hide potential type issues, and usingany[]for constructor parameters can obscure type safety. Consider either providing more specific parameter types or clarifying why they're not strictly typed.
58-62: Ensure fallback logic is clear in documentation.Falling back to
options.keyUriandoptions.jweis helpful but might be overlooked by other developers. Consider adding comments or doc lines highlighting that keys/JWE can be passed in the URI or in the options object.packages/@webex/plugin-encryption/src/constants.ts (1)
1-3: Clarify the need for@ts-ignorewhen defining theEnumtype.Disabling TypeScript checks might obscure real issues. If possible, remove
@ts-ignoreor add a comment explaining its necessity to maintain clarity and code health.packages/@webex/plugin-encryption/src/encryption/encryption.types.ts (1)
24-26: Consider adding error types to the Promise return type.The
downloadAndDecryptFilemethod should specify potential error types that could be thrown during file download or decryption.- downloadAndDecryptFile(fileUri: string, options: FileDownloadOptions): Promise<ArrayBuffer>; + downloadAndDecryptFile(fileUri: string, options: FileDownloadOptions): Promise<ArrayBuffer | Error>;packages/@webex/plugin-encryption/src/types.ts (1)
4-11: Consider enhancing Logger type with log level enums and structured logging.The Logger type could be improved by:
- Using an enum for log levels
- Supporting structured logging with objects/errors
+export enum LogLevel { + ERROR = 'error', + WARN = 'warn', + INFO = 'info', + DEBUG = 'debug', + TRACE = 'trace' +} 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: (level: LogLevel, payload: string | Error | object) => void; + error: (payload: string | Error | object) => void; + warn: (payload: string | Error | object) => void; + info: (payload: string | Error | object) => void; + trace: (payload: string | Error | object) => void; + debug: (payload: string | Error | object) => void; };packages/@webex/plugin-encryption/jest.config.js (1)
5-6: Consider specifying transformIgnorePatterns.Empty transformIgnorePatterns might cause unnecessary transformations. Consider explicitly listing patterns to ignore.
- transformIgnorePatterns: [], + transformIgnorePatterns: [ + '/node_modules/(?!(@webex)/)', + '<rootDir>/dist/' + ],packages/webex/src/encryption.js (1)
30-40: Consider adding input validation for the config object.While the implementation is correct, consider adding validation for the config object to ensure required encryption-related settings are present.
Webex.init = function init(attrs = {}) { + // Validate encryption-specific configuration + if (attrs.config && attrs.config.credentials) { + if (!attrs.config.credentials.authorizationString) { + throw new Error('Authorization string is required for encryption functionality'); + } + } + attrs.config = merge( { sdkType: 'encryption',packages/@webex/plugin-encryption/browsers.js (2)
44-49: Remove or document commented-out browser configurations.Either remove the commented-out Firefox configurations or add a comment explaining why they are kept for future reference.
Also applies to: 79-84
24-28: Remove duplicate WebRTC flags in Chrome configuration.The flags array duplicates the settings already present in 'goog:chromeOptions.args'.
], }, - flags: [ - '--disable-features=WebRtcHideLocalIpsWithMdns', - '--use-fake-device-for-media-stream', - '--use-fake-ui-for-media-stream', - ], },packages/@webex/plugin-encryption/test/unit/spec/encryption/index.ts (1)
39-73: Consider adding more edge cases to the test suite.The test coverage is good, but consider adding these additional test cases:
- Test with malformed JWE token
- Test with invalid keyUri format
- Test with empty file content
- Test with very large file sizes
packages/@webex/plugin-encryption/README.md (2)
53-53: Fix heading level increment.The heading level jumps from h2 to h4. Update the heading level to maintain proper hierarchy.
-#### Development +### Development🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
53-53: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
15-15: Consider adding more details to the warning message.The warning message could be more informative by including details about potential instability or breaking changes.
-# WARNING: This plugin is currently under active development +# WARNING: This plugin is currently under active development and may include breaking changes in future releases. Use with caution in production environments.docs/samples/plugin-encryption/index.html (1)
1-87: New Encryption Plugin Sample Page
This new HTML file (“Encryption Kitchen Sink”) provides an interactive demonstration of the Webex encryption service. The structure (with clear sections for authentication and decryption actions) is well organized and consistent with our samples. One minor suggestion: while inline event handlers (e.g.onclick="changeEnv()"andonclick="decryptFile()") are acceptable in a sample, consider using external script event bindings for better maintainability and separation of concerns in production code.docs/samples/plugin-encryption/style.css (3)
70-77: Duplicate Font Name in Button Styles
In the rule forbutton, input[type="button"], thefont-familyis set as:
CiscoSansTT Regular, Helvetica Neue, Helvetica, sans-serif
Consider removing the duplicated"Helvetica"entry to avoid redundancy. For example, use:
CiscoSansTT Regular, Helvetica Neue, sans-serif🧰 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)
136-143: Duplicate Font Name in Input Styles
Within the styles forinput[type=text], input[type=password], input[type=email], thefont-familydeclaration also includes"Helvetica Neue, Helvetica"redundantly. Removing the duplicate"Helvetica"will simplify the declaration.
344-358: Duplicate Font in Select/Input String Styles
The selector forinput[type="string"], selectuses a similarfont-familylist with duplicate"Helvetica". Consolidating the list to remove repetition (e.g. usingCiscoSansTT Regular, Helvetica Neue, sans-serif) would be ideal.🧰 Tools
🪛 Biome (1.9.4)
[error] 353-353: 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(4 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/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/constants.ts(1 hunks)packages/@webex/plugin-encryption/src/encryption/constants.ts(1 hunks)packages/@webex/plugin-encryption/src/encryption/encryption.types.ts(1 hunks)packages/@webex/plugin-encryption/src/encryption/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/encryption/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(3 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/process
- packages/@webex/plugin-encryption/LICENSE
- packages/@webex/plugin-encryption/.eslintrc.js
- packages/@webex/plugin-encryption/tsconfig.json
- packages/@webex/plugin-encryption/src/config.ts
- packages/@webex/plugin-encryption/babel.config.js
- docs/samples/browser-plugin-meetings/app.js
🧰 Additional context used
🪛 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] 146-146: Duplicate font names are redundant and unnecessary: Helvetica
Remove duplicate font names within the property
(lint/suspicious/noDuplicateFontNames)
[error] 353-353: Duplicate font names are redundant and unnecessary: Helvetica
Remove duplicate font names within the property
(lint/suspicious/noDuplicateFontNames)
🪛 markdownlint-cli2 (0.17.2)
packages/@webex/plugin-encryption/README.md
53-53: 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 (2)
- GitHub Check: Initialize Project
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (25)
packages/@webex/plugin-encryption/src/encryption/index.ts (1)
36-39: ValidatefileUrimore comprehensively.You may want to explicitly handle edge cases like malformed URLs or empty
fileUrito avoid runtime errors fromnew URL(fileUri). Adding a quick check or try/catch can improve stability.packages/@webex/plugin-encryption/src/index.ts (2)
7-9: Validate plugin naming consistency.Using
'cypher'as the registered plugin name makes sense for encryption logic, but confirm it aligns with established naming patterns (e.g.,'encryption','crypto') for consistency across plugins.
11-11: Ensure consistent exports and usage.Exporting
Cypherby default is fine, but double-check all internal references (e.g., doc references, tests) to ensure consistent import statements for the plugin.packages/@webex/plugin-encryption/src/encryption/constants.ts (1)
2-5: LGTM! Constants are well-defined.The constants for decryption states are descriptive and follow a consistent naming pattern.
packages/@webex/plugin-encryption/src/encryption/encryption.types.ts (1)
4-23: LGTM! Well-documented interface with clear options.The
FileDownloadOptionsinterface is well-structured with clear documentation for each option. The security-sensitive parameters (jweandkeyUri) are properly documented with their requirements.packages/@webex/plugin-encryption/src/types.ts (1)
13-35: LGTM! Well-structured SDK interface with proper separation of concerns.The WebexSDK interface properly separates internal and public plugins, and includes necessary security-related methods.
packages/@webex/plugin-encryption/jest.config.js (1)
11-18: LGTM! Good coverage thresholds for security-critical code.The 85% coverage threshold for lines, functions, branches, and statements is appropriate for a security-critical encryption plugin.
packages/webex/src/encryption.js (1)
1-3: LGTM! Updated copyright year.The copyright notice is properly updated to include 2024.
packages/webex/src/webex.js (1)
30-30: LGTM! Plugin require statement correctly added.The encryption plugin is properly integrated alongside other plugins.
packages/@webex/plugin-encryption/test/unit/spec/encryption/index.ts (1)
6-88: LGTM! Well-structured test suite with good coverage.The test suite effectively covers the main functionality and error cases for the
downloadAndDecryptFilemethod.docs/samples/plugin-encryption/app.js (1)
24-28: Consider using a more secure storage mechanism for access tokens.The current implementation stores access tokens in localStorage, which is vulnerable to XSS attacks. Consider using more secure alternatives like session storage or HTTP-only cookies.
webpack.config.js (1)
42-48: LGTM! New encryption entry point added correctly.The encryption entry point is properly configured with the correct library name and type.
tooling/commands/test.js (1)
171-171: LGTM! Added encryption plugin to Mocha test runner.The plugin is correctly added to the list of packages that should be run through Mocha.
packages/@webex/internal-plugin-encryption/src/encryption.js (1)
75-84: Verify SCR location handling security implications.The code sets
scrJSON.locto an empty string before creating a new SCR instance. This could have security implications if the location is used for validation or authorization.✅ Verification successful
Let's gather additional context around how the location field (
loc) is handled in the codebase. In particular, we want to look for any conditional checks or usage oflocthat could indicate its role in security validation. I'll run a more general search for any conditional statements involving.locacross the repository.
SCR Location Handling in Encryption Is Verified as Intended and Safe
We’ve confirmed that the SCR’s location is intentionally cleared by setting
scrJSON.loc = ''before creating a new SCR viaSCR.fromJSON(). A repository-wide search shows that the encryption module (e.g., at line 195 inpackages/@webex/internal-plugin-encryption/src/encryption.js) explicitly checks for a falsyscr.locvalue. This indicates that the empty location is expected and correctly triggers the related logic (for example, as part of the newly added file service bypass). There’s no evidence that removing the location introduces an authorization or validation vulnerability.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for SCR location usage in security contexts ast-grep --pattern 'if ($_.loc) { $$$ }'Length of output: 44
Script:
#!/bin/bash # Search for conditionals that use a property named "loc" anywhere in the repository rg "if\s*\(.*\.loc" -n .Length of output: 107331
packages/@webex/test-helper-mock-webex/src/index.js (1)
18-18: LGTM! Addition of 'cypher' to nonInternalPlugins.The addition of 'cypher' to the nonInternalPlugins array is consistent with the PR objective of introducing the new encryption plugin.
docs/samples/calling/app.js (2)
184-184: Fixed syntax error by adding missing closing brace.The addition of the missing closing brace resolves a syntax error in the
generateGuestTokenfunction.
234-234: Verify the updated hydra service URL.The hydra service URL has been updated to point to the integration environment. Please confirm this is the intended environment for the service.
✅ Verification successful
Hydra Integration Endpoint Verified
The updated URL (https://hydra-intb.ciscospark.com/v1/) clearly targets an integration environment (as indicated by the “-intb” subdomain) rather than a production endpoint. Based on the available information and the absence of a distinct production URL for a “Hydra” service in the documentation, this appears to be the intended configuration.🏁 Scripts executed
The following scripts were executed for the analysis:
Web query:
What is the current production URL for Webex Hydra service?Length of output: 4007
packages/@webex/plugin-encryption/package.json (1)
30-35: Verify the version of node-scr package.Please ensure that the specified version of
node-scris the latest stable version and doesn't have any known security vulnerabilities.packages/webex/package.json (3)
20-20: New Export for Encryption Plugin
The new export entry"./encryption": "./dist/encryption.js",has been added to the exports object. This correctly exposes the encryption module for consumers of the package.
82-82: Encryption Plugin Dependency Added
The dependency on"@webex/plugin-encryption": "workspace:*"is now included. This aligns with the integration of the new encryption functionality.
94-95: New Dependency: node-scr
The addition of"node-scr": "^0.3.0"complements the encryption feature. Please verify that this dependency’s version is compatible with our use cases and does not introduce conflicts.docs/index.html (1)
85-86: New Sample Link for Encryption Plugin
The addition of the anchor link<a href="./samples/plugin-encryption/" class="list-group-item">Encryption Plugin</a>integrates the new encryption sample into our main documentation page. This improves discoverability and is consistent with the other sample links.package.json (3)
30-30: Updated Legacy Build Script
The"@legacy"script now includes@webex/plugin-encryptionin its--fromargument. This change ensures that the encryption plugin is built along with other modules in the legacy process.
40-40: Integration into TypeScript Build Process
The update to"build:tsc"now appends&& yarn workspace @webex/plugin-encryption run build, ensuring that the encryption plugin is built as part of the type-checking and compilation step.
42-42: Inclusion in Skip-Samples Build
Similarly, the"build:skip-samples"script now runs the build for@webex/plugin-encryption, integrating the plugin into the build workflow when sample files are skipped.
| try { | ||
| // Decrypt the JWE to get the SCR | ||
| const scr = await this.$webex.internal.encryption.decryptScr(keyUri, JWE); | ||
|
|
||
| // Start the download and decryption process, returning a promise | ||
|
|
||
| return this.$webex.internal.encryption.download(fileUri, scr, {useFileService}); | ||
| } catch (error) { | ||
| throw new Error(`Failed to decrypt the JWE: ${error.message}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine error handling for decryption failures.
Re-throwing errors with a new message can mask the underlying stack. Consider wrapping or preserving the original error’s stack for easier debugging:
- throw new Error(\`Failed to decrypt the JWE: \${error.message}\`);
+ throw new Error(\`Failed to decrypt the JWE: \${(error as Error).message}\nStack: \${(error as Error).stack}\`);📝 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.
| try { | |
| // Decrypt the JWE to get the SCR | |
| const scr = await this.$webex.internal.encryption.decryptScr(keyUri, JWE); | |
| // Start the download and decryption process, returning a promise | |
| return this.$webex.internal.encryption.download(fileUri, scr, {useFileService}); | |
| } catch (error) { | |
| throw new Error(`Failed to decrypt the JWE: ${error.message}`); | |
| } | |
| } | |
| try { | |
| // Decrypt the JWE to get the SCR | |
| const scr = await this.$webex.internal.encryption.decryptScr(keyUri, JWE); | |
| // Start the download and decryption process, returning a promise | |
| return this.$webex.internal.encryption.download(fileUri, scr, {useFileService}); | |
| } catch (error) { | |
| - throw new Error(`Failed to decrypt the JWE: ${error.message}`); | |
| + throw new Error(`Failed to decrypt the JWE: ${(error as Error).message}\nStack: ${(error as Error).stack}`); | |
| } | |
| } |
| async function decryptFile() { | ||
| decryptFileResult.innerText = ''; | ||
| const fileUrl = encryptedFileUrlInput.value; | ||
| const encryptedFileName = decryptedFileNameInput.value; | ||
| const mimeType = mimeTypeDropdown.value; | ||
| try { | ||
|
|
||
| const decryptedBuf = await webex.cypher.downloadAndDecryptFile(fileUrl); | ||
| const file = new File([decryptedBuf], encryptedFileName, {type: mimeType}); | ||
|
|
||
| const url = URL.createObjectURL(file); | ||
| const a = document.createElement("a"); | ||
| a.href = url; | ||
| a.download = file.name || "download"; // Use the original filename if available | ||
| document.body.appendChild(a); | ||
|
|
||
| // Trigger the download | ||
| a.click(); | ||
|
|
||
| // Cleanup | ||
| document.body.removeChild(a); | ||
| URL.revokeObjectURL(url); | ||
| decryptFileResult.innerText = ': success'; | ||
| } | ||
| catch (error) { | ||
| console.error('error decrypting file', error); | ||
| decryptFileResult.innerText = ': error'; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for invalid file URLs and MIME types.
The decryption function should validate the file URL and MIME type before proceeding. Also, consider adding cleanup in a finally block.
async function decryptFile() {
decryptFileResult.innerText = '';
const fileUrl = encryptedFileUrlInput.value;
const encryptedFileName = decryptedFileNameInput.value;
const mimeType = mimeTypeDropdown.value;
+
+ if (!fileUrl) {
+ decryptFileResult.innerText = ': error - Invalid file URL';
+ return;
+ }
+
+ if (!mimeType) {
+ decryptFileResult.innerText = ': error - Invalid MIME type';
+ return;
+ }
+
+ let objectUrl;
try {
const decryptedBuf = await webex.cypher.downloadAndDecryptFile(fileUrl);
const file = new File([decryptedBuf], encryptedFileName, {type: mimeType});
- const url = URL.createObjectURL(file);
+ objectUrl = URL.createObjectURL(file);
const a = document.createElement("a");
- a.href = url;
+ a.href = objectUrl;
a.download = file.name || "download";
document.body.appendChild(a);
a.click();
document.body.removeChild(a);
- URL.revokeObjectURL(url);
decryptFileResult.innerText = ': success';
}
catch (error) {
console.error('error decrypting file', error);
decryptFileResult.innerText = ': error';
+ } finally {
+ if (objectUrl) {
+ URL.revokeObjectURL(objectUrl);
+ }
}
}📝 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 function decryptFile() { | |
| decryptFileResult.innerText = ''; | |
| const fileUrl = encryptedFileUrlInput.value; | |
| const encryptedFileName = decryptedFileNameInput.value; | |
| const mimeType = mimeTypeDropdown.value; | |
| try { | |
| const decryptedBuf = await webex.cypher.downloadAndDecryptFile(fileUrl); | |
| const file = new File([decryptedBuf], encryptedFileName, {type: mimeType}); | |
| const url = URL.createObjectURL(file); | |
| const a = document.createElement("a"); | |
| a.href = url; | |
| a.download = file.name || "download"; // Use the original filename if available | |
| document.body.appendChild(a); | |
| // Trigger the download | |
| a.click(); | |
| // Cleanup | |
| document.body.removeChild(a); | |
| URL.revokeObjectURL(url); | |
| decryptFileResult.innerText = ': success'; | |
| } | |
| catch (error) { | |
| console.error('error decrypting file', error); | |
| decryptFileResult.innerText = ': error'; | |
| } | |
| } | |
| async function decryptFile() { | |
| decryptFileResult.innerText = ''; | |
| const fileUrl = encryptedFileUrlInput.value; | |
| const encryptedFileName = decryptedFileNameInput.value; | |
| const mimeType = mimeTypeDropdown.value; | |
| if (!fileUrl) { | |
| decryptFileResult.innerText = ': error - Invalid file URL'; | |
| return; | |
| } | |
| if (!mimeType) { | |
| decryptFileResult.innerText = ': error - Invalid MIME type'; | |
| return; | |
| } | |
| let objectUrl; | |
| 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); | |
| decryptFileResult.innerText = ': success'; | |
| } | |
| catch (error) { | |
| console.error('error decrypting file', error); | |
| decryptFileResult.innerText = ': error'; | |
| } finally { | |
| if (objectUrl) { | |
| URL.revokeObjectURL(objectUrl); | |
| } | |
| } | |
| } |
| tokenElm.addEventListener('change', (event) => { | ||
| localStorage.setItem('access-token', event.target.value); | ||
| localStorage.setItem('date', new Date().getTime() + 12 * 60 * 60 * 1000); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add token expiration validation.
The token expiration check only stores a new timestamp but doesn't validate if the stored token has expired.
tokenElm.addEventListener('change', (event) => {
+ const token = event.target.value;
+ if (!token) {
+ localStorage.removeItem('access-token');
+ localStorage.removeItem('date');
+ return;
+ }
localStorage.setItem('access-token', event.target.value);
localStorage.setItem('date', new Date().getTime() + 12 * 60 * 60 * 1000);
});Committable suggestion skipped: line range outside the PR's diff.
| if (options && options.useFileService === false) { | ||
| return Promise.resolve(fileUrl); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for file service bypass option.
The new option to bypass file service could potentially be used to access unauthorized files. Consider adding validation or logging when this option is used.
if (options && options.useFileService === false) {
+ this.logger.warn('encryption: bypassing file service - this should only be used in trusted environments');
+ if (!fileUrl.startsWith('https://')) {
+ return Promise.reject(new Error('Direct file URLs must use HTTPS'));
+ }
return Promise.resolve(fileUrl);
}📝 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 (options && options.useFileService === false) { | |
| return Promise.resolve(fileUrl); | |
| } | |
| if (options && options.useFileService === false) { | |
| this.logger.warn('encryption: bypassing file service - this should only be used in trusted environments'); | |
| if (!fileUrl.startsWith('https://')) { | |
| return Promise.reject(new Error('Direct file URLs must use HTTPS')); | |
| } | |
| return Promise.resolve(fileUrl); | |
| } |
| // first we get the scr json, then we create an SCR instance using the key json and then we create a JWE using the key jwk | ||
| return this.getKey(key, options).then((k) => | ||
| SCR.fromJSON(scr).then((encScr) => encScr.toJWE(k.jwk)) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in SCR encryption.
The SCR encryption process should handle potential errors from fromJSON and toJWE operations.
- return this.getKey(key, options).then((k) =>
- SCR.fromJSON(scr).then((encScr) => encScr.toJWE(k.jwk))
- );
+ return this.getKey(key, options)
+ .then((k) => {
+ if (!k || !k.jwk) {
+ throw new Error('Invalid key or JWK');
+ }
+ return SCR.fromJSON(scr)
+ .then((encScr) => {
+ if (!encScr) {
+ throw new Error('Failed to create SCR instance');
+ }
+ return encScr.toJWE(k.jwk);
+ });
+ });📝 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.
| // first we get the scr json, then we create an SCR instance using the key json and then we create a JWE using the key jwk | |
| return this.getKey(key, options).then((k) => | |
| SCR.fromJSON(scr).then((encScr) => encScr.toJWE(k.jwk)) | |
| ); | |
| // first we get the scr json, then we create an SCR instance using the key json and then we create a JWE using the key jwk | |
| return this.getKey(key, options) | |
| .then((k) => { | |
| if (!k || !k.jwk) { | |
| throw new Error('Invalid key or JWK'); | |
| } | |
| return SCR.fromJSON(scr) | |
| .then((encScr) => { | |
| if (!encScr) { | |
| throw new Error('Failed to create SCR instance'); | |
| } | |
| return encScr.toJWE(k.jwk); | |
| }); | |
| }); |
| @@ -0,0 +1,75 @@ | |||
| { | |||
| "name": "@webex/plugin-encryption", | |||
| "description": "", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a package description.
The description field is empty. Please add a meaningful description that explains the purpose and features of the encryption plugin.
- "description": "",
+ "description": "Encryption plugin for the Cisco Webex JS SDK providing secure file download and decryption capabilities",📝 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.
| "description": "", | |
| "description": "Encryption plugin for the Cisco Webex JS SDK providing secure file download and decryption capabilities", |
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
|
|
||
| return this.$webex.internal.encryption.download(fileUri, scr, {useFileService}); | ||
| } catch (error) { | ||
| throw new Error(`Failed to decrypt the JWE: ${error.message}`); |
There was a problem hiding this comment.
Let's add the logger and also use the conventional error message we throw in other packages
There was a problem hiding this comment.
Are we not adding the log messages like this?
LoggerProxy.log('log message.', {
module: 'file name',
method: 'method name',
});
|
Please add the coverage report. |
|
|
approve with minor comments/questions |
bhabalan
changed the title
Seeing the same set of integration test failures while trying to run the integration test locally against the next branch itself
|
3 participants
COMPLETES #<SPARK-586605>
This pull request addresses
by making the following changes
Change Type
The following scenarios were tested
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.