Add copy-to-clipboard fallback if navigator.clipboard is not available #1336
Conversation
|
Hi @lacksfish thanks for submitting these prs. We'll be reviewing it shortly. |
|
You're welcome, I'm happy to help :) Looking forward to the reviews! |
|
@coderabbitai review |
WalkthroughThe update refactors the clipboard interaction in the directive by removing the conditional check for the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/app/shared/directive/clipboard.directive.ts (1 hunks)
Additional comments: 2
src/app/shared/directive/clipboard.directive.ts (2)
- 17-17: The early return pattern is correctly used to avoid unnecessary nesting.
- 19-22: The conditional check for
navigator.clipboardis now correctly separated into two distinct method calls, improving readability and maintainability.
| private copyUsingFallbackMethod(): void { | ||
| const input = document.createElement('textarea'); | ||
| input.innerHTML = this.payload; | ||
| document.body.appendChild(input); | ||
| input.select(); | ||
|
|
||
| try { | ||
| const result = document.execCommand('copy'); | ||
| if (result) { | ||
| this.copied.emit(this.payload.toString()); | ||
| } else { | ||
| this.copied.emit('Error could not copy text.'); | ||
| } | ||
| } finally { | ||
| document.body.removeChild(input); | ||
| } | ||
| } |
There was a problem hiding this comment.
The copyUsingFallbackMethod method correctly implements the fallback mechanism using document.execCommand('copy'). However, the payload is set using innerHTML, which can be a security risk if the payload is not sanitized, as it may lead to XSS attacks.
- input.innerHTML = this.payload;
+ input.value = this.payload;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.
| private copyUsingFallbackMethod(): void { | |
| const input = document.createElement('textarea'); | |
| input.innerHTML = this.payload; | |
| document.body.appendChild(input); | |
| input.select(); | |
| try { | |
| const result = document.execCommand('copy'); | |
| if (result) { | |
| this.copied.emit(this.payload.toString()); | |
| } else { | |
| this.copied.emit('Error could not copy text.'); | |
| } | |
| } finally { | |
| document.body.removeChild(input); | |
| } | |
| } | |
| private copyUsingFallbackMethod(): void { | |
| const input = document.createElement('textarea'); | |
| input.value = this.payload; | |
| document.body.appendChild(input); | |
| input.select(); | |
| try { | |
| const result = document.execCommand('copy'); | |
| if (result) { | |
| this.copied.emit(this.payload.toString()); | |
| } else { | |
| this.copied.emit('Error could not copy text.'); | |
| } | |
| } finally { | |
| document.body.removeChild(input); | |
| } | |
| } |
There was a problem hiding this comment.
Good point, should use input.innerText or input.value
There was a problem hiding this comment.
@lacksfish You're correct that using input.value is the preferred approach for setting the text content of a textarea element, as it ensures the text is treated as raw text rather than HTML, which can prevent XSS vulnerabilities. The input.innerText could be used, but input.value is more conventional and semantically correct for form inputs. Here's the updated code:
private copyUsingFallbackMethod(): void {
const input = document.createElement('textarea');
input.value = this.payload;
document.body.appendChild(input);
input.select();
try {
const result = document.execCommand('copy');
if (result) {
this.copied.emit(this.payload.toString());
} else {
this.copied.emit('Error could not copy text.');
}
} finally {
document.body.removeChild(input);
}
}This change should address the security concern while maintaining the functionality of your clipboard directive.
| private copyUsingClipboardAPI(): void { | ||
| navigator.clipboard.writeText(this.payload.toString()) | ||
| .then(() => this.copied.emit(this.payload.toString())) | ||
| .catch((error) => this.copied.emit('Error could not copy text: ' + JSON.stringify(error))); |
There was a problem hiding this comment.
Tip
Codebase Verification
The context provided around the assignment input.innerHTML = this.payload; does not include any code that indicates sanitization of the payload. The payload is directly assigned to the innerHTML of a newly created textarea element, which is then used for the fallback copy method. This could potentially introduce security risks if the payload contains untrusted user input that is not properly sanitized.
Based on the information gathered, there is no evidence in the provided context to suggest that the payload is sanitized before being used in the copyUsingClipboardAPI method or in the fallback method.
Therefore, the review comment regarding the need to ensure that the payload is sanitized before being copied to the clipboard is valid and should be addressed to prevent potential security issues.
Analysis chain
The copyUsingClipboardAPI method is well-implemented with proper promise handling for success and error cases. Ensure that the payload is sanitized before being copied to the clipboard to prevent potential security issues.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the payload is sanitized before being passed to this method.
ast-grep --lang typescript --pattern $'this.copyUsingClipboardAPI($_)'
Length of output: 71
Script:
#!/bin/bash
# Search for the invocation of 'copyUsingClipboardAPI' method and check for payload sanitization
rg "this.copyUsingClipboardAPI\(" --type ts
Length of output: 129
Script:
#!/bin/bash
# Search for assignments or modifications to 'this.payload' to check for sanitization before it's used in 'copyUsingClipboardAPI'
rg "this.payload" --type ts
Length of output: 505
Script:
#!/bin/bash
# Examine the context around the assignment to 'input.innerHTML' to check for payload sanitization
rg -C 5 "input.innerHTML = this.payload" --type ts
Length of output: 845
There was a problem hiding this comment.
I'm open to suggestions for sanitization. I haven't looked at all places in the menus where copy is being used, for me the issue scope was not having copied invoices I created.
In the fallback, since there's gonna be a temp textarea element's content in the DOM it's important (as suggested above) to use input.innerText or input.value. I've opted for innerText and pushed the 1-line change.
Again, maybe a more simple alternative would be to not display the "copy" button when navigator.clipboard is not available.
There was a problem hiding this comment.
@lacksfish, using input.innerText instead of input.innerHTML is a good move to mitigate the risk of script injection, as it treats the content as text rather than HTML. Regarding the alternative to hide the "Copy" button when navigator.clipboard is not available, it could simplify the implementation and reduce the need for a fallback, but it would also remove functionality for users on older browsers. It's a trade-off between functionality and maintainability that should be carefully considered based on your user base and their needs.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/app/shared/directive/clipboard.directive.ts (1 hunks)
Additional comments: 3
src/app/shared/directive/clipboard.directive.ts (3)
- 17-22: The refactoring of the
onClickevent handler correctly separates the concerns of checking for the payload and determining which clipboard method to use. However, ensure that thecopyUsingFallbackMethodis only called whennavigator.clipboardis not available.- 26-41: The
copyUsingFallbackMethodusesinnerTextinstead ofinnerHTML, which is a safer choice to prevent XSS attacks. However, ensure that the payload is sanitized before being set to prevent any potential security risks.- 44-47: The
copyUsingClipboardAPImethod is implemented with proper promise handling for success and error cases. Ensure that the payload is sanitized before being copied to the clipboard to prevent potential security issues.
ShahanaFarooqui
force-pushed
the
Release-0.15.1
branch
3 times, most recently
from
8f8178a to
c2c9e74
Compare
There was a problem hiding this comment.
Changes look good but need some lint fixes. Also need to push compiled code.
I do not have write access to the fork, so I will do that in the next commit.
@lacksfish Thank you for the PR.
#1336) * add copy-to-clipboard fallback if navigator.clipboard is not available * amend copy fallback
* rm .DS_Store * Add watchfrontenddev command for npm * Fix toggle issues in sidenav (pinning and on page refresh) * Add copy-to-clipboard fallback if navigator.clipboard is not available (#1336) * add copy-to-clipboard fallback if navigator.clipboard is not available * amend copy fallback * clipboard copy lint fixes and frontend build * fix: add missing boltz state `transaction.lockupFailed` (#1349) * fix: boltzd docs link (#1354) * exit gracefully (#1356) * allow for eclair updated relayed audit format (#1363) * feat: add boltz service to cln (#1352) * lint fix * Request Params Cleanup * cln: Boltz auto-send (#1366) * Bug-fix (CLN Boltz): Hide claim tx id and routing fee for non-zero conf reverse swap * cln: Boltz auto-send - Added auto send option for Swap In - Checking compatiblity with v2.0.0 and above * Test import fixes * Update help.component.ts (#1379) Fixed broken link under "Help" -> "Node Settings" * Backend config fix (#1382) * Updating Common Application Configuration * Fixed get RTL Conf * Update Application Settings * application and settings case change * Unified config models * Default node update * 2FA and Password reset * Final application settings update * Config Settings and Authentication case fixed * Node Setting Fix * Fiat currency Symbol fix * CLN: Fiat symbol fix * All: Fiat symbol fix * Update node settings * Services UI fix * CLN: Removed child node settings * All: Removed child node settings * Test fixes * mempool links for onchain information (#1383) * Tests fix Tests fix * UI for Block Explorer Configuration (#1385) * Bump fee with mempool information (#1386) * Mempool openchannel minfee (#1388) Open channel model block if min fee is higher * Show error on login screen if rune is incorrect and getinfo throws error (#1391) * cln: Removed channel lookup call for update policy (#1392) * ECL: On-chain Transactions, Invoice and Payments pagination (#1393) Done most of the UI changes to accommodate pagination on transactions, payments and invoices tables but true pagination cannot be implemented till total number of records are missing from the API response. Once the issue ACINQ/eclair#2855 is fixed, I will uncomment pagination changes in the frontend. * lnd: Onchain CPFP (#1394) - UTXO label bug fix - Warning on utxo label for "sweep" in text. * Bug fixes after testing * Testing bug fixes (#1401) * Bug fix 2: lnd: Link channel point to explorer and show fee on close channel too * lnd: explorer link on pending channels * Node lookup link on view channel peer pubkey * Testing bug fixes (#1402) * Bug fix 2: lnd: Link channel point to explorer and show fee on close channel too * lnd: explorer link on pending channels * Node lookup link on view channel peer pubkey * test fixes * ng update to v18.0.x * Updating install with --legacy-peer-deps --------- Co-authored-by: Grzegorz Kućmierz <[email protected]> Co-authored-by: lacksfish <[email protected]> Co-authored-by: jackstar12 <[email protected]> Co-authored-by: Kilian <[email protected]> Co-authored-by: Taylor King <[email protected]> Co-authored-by: Fishcake <[email protected]> Co-authored-by: Ant <[email protected]>
Refactored the onClick event for the
Copy To Clipboardbuttons. A fallback mechanism has been introduced using the deprecateddocument.execCommand('copy')method to address browsers that lack support for the newnavigator.clipboardAPI (e.g., IE and Firefox Android).Alternatively, if the decision is not to include this fallback mechanism, it's recommended not to display the "Copy To Clipboard" buttons at all. This avoids potential issues, such as providing a non-functioning button that could unintentionally retain previously copied addresses. Note that opting for this approach would involve more extensive code changes across multiple files.
Summary by CodeRabbit