You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2020/12/18 22:25:42 UTC
[GitHub] [guacamole-server] myjimmy opened a new pull request #317: GUACAMOLE-1186: Maintain the clipboard content of the remote server.
myjimmy opened a new pull request #317:
URL: https://github.com/apache/guacamole-server/pull/317
When users copy any data within the remote server, the guacamole maintains the clipboard content of the remote server unless users copy any text in the client computer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] myjimmy commented on a change in pull request #317: GUACAMOLE-1186: Maintain the clipboard content of the remote server.
Posted by GitBox <gi...@apache.org>.
myjimmy commented on a change in pull request #317:
URL: https://github.com/apache/guacamole-server/pull/317#discussion_r547297801
##########
File path: src/protocols/rdp/channels/cliprdr.c
##########
@@ -283,7 +283,8 @@ static UINT guac_rdp_cliprdr_format_list(CliprdrClientContext* cliprdr,
if (guac_rdp_cliprdr_format_supported(format_list, CF_TEXT))
return guac_rdp_cliprdr_send_format_data_request(cliprdr, CF_TEXT);
- /* Ignore any unsupported data */
+ /* Suppose that any unsupported data is raw data (in this case, CF_RAW) */
+ guac_rdp_cliprdr_send_format_data_request(cliprdr, CF_RAW);
Review comment:
> why are we requesting that the RDP server send us that data?
Because we need to reset the local clipboard. That's, to get the opportunity to reset the local clipboard when users copy any data other than text.
The added new code of `guac_rdp_cliprdr_format_list()` function let the remote server to call `guac_rdp_cliprdr_format_data_response()` function according to **"Remote Desktop Protocol: Clipboard Virtual Channel Extension"**.
If users copy any kind of data other than text, so we can use `guac_rdp_cliprdr_format_data_response()` function to reset the local clipboard.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] myjimmy commented on a change in pull request #317: GUACAMOLE-1186: Maintain the clipboard content of the remote server.
Posted by GitBox <gi...@apache.org>.
myjimmy commented on a change in pull request #317:
URL: https://github.com/apache/guacamole-server/pull/317#discussion_r547099769
##########
File path: src/protocols/rdp/channels/cliprdr.c
##########
@@ -283,7 +283,8 @@ static UINT guac_rdp_cliprdr_format_list(CliprdrClientContext* cliprdr,
if (guac_rdp_cliprdr_format_supported(format_list, CF_TEXT))
return guac_rdp_cliprdr_send_format_data_request(cliprdr, CF_TEXT);
- /* Ignore any unsupported data */
+ /* Suppose that any unsupported data is raw data (in this case, CF_RAW) */
+ guac_rdp_cliprdr_send_format_data_request(cliprdr, CF_RAW);
Review comment:
guac_rdp_cliprdr_format_list() function is called when copying any data in the remote server.
In this function, we need to tell the remote server whether the remote client supports the clipboard format of the remote clipboard.
Users can copy any kind of data including text in the remote server.
Because I thought that CF_RAW indicates any kind of clipboard data, I selected the clipboard format as CF_RAW.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] myjimmy commented on a change in pull request #317: GUACAMOLE-1186: Maintain the clipboard content of the remote server.
Posted by GitBox <gi...@apache.org>.
myjimmy commented on a change in pull request #317:
URL: https://github.com/apache/guacamole-server/pull/317#discussion_r547099769
##########
File path: src/protocols/rdp/channels/cliprdr.c
##########
@@ -283,7 +283,8 @@ static UINT guac_rdp_cliprdr_format_list(CliprdrClientContext* cliprdr,
if (guac_rdp_cliprdr_format_supported(format_list, CF_TEXT))
return guac_rdp_cliprdr_send_format_data_request(cliprdr, CF_TEXT);
- /* Ignore any unsupported data */
+ /* Suppose that any unsupported data is raw data (in this case, CF_RAW) */
+ guac_rdp_cliprdr_send_format_data_request(cliprdr, CF_RAW);
Review comment:
guac_rdp_cliprdr_format_list() function is called when copying any data in the remote server.
In this function, we need to tell the remote server whether the remote client supports the clipboard format of the remote clipboard.
Users can copy any kind of data including text in the remote server.
Because I thought that CF_RAW indicates any kind of clipboard data, I selected the clipboard format as CF_RAW.
Then, we can reduce many IF statements and make this function simple.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #317: GUACAMOLE-1186: Maintain the clipboard content of the remote server.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #317:
URL: https://github.com/apache/guacamole-server/pull/317#discussion_r547071757
##########
File path: src/protocols/rdp/channels/cliprdr.c
##########
@@ -283,7 +283,8 @@ static UINT guac_rdp_cliprdr_format_list(CliprdrClientContext* cliprdr,
if (guac_rdp_cliprdr_format_supported(format_list, CF_TEXT))
return guac_rdp_cliprdr_send_format_data_request(cliprdr, CF_TEXT);
- /* Ignore any unsupported data */
+ /* Suppose that any unsupported data is raw data (in this case, CF_RAW) */
+ guac_rdp_cliprdr_send_format_data_request(cliprdr, CF_RAW);
Review comment:
Why?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] myjimmy commented on a change in pull request #317: GUACAMOLE-1186: Maintain the clipboard content of the remote server.
Posted by GitBox <gi...@apache.org>.
myjimmy commented on a change in pull request #317:
URL: https://github.com/apache/guacamole-server/pull/317#discussion_r548052800
##########
File path: src/protocols/rdp/channels/cliprdr.c
##########
@@ -614,6 +619,11 @@ int guac_rdp_clipboard_end_handler(guac_user* user, guac_stream* stream) {
if (clipboard == NULL)
return 0;
+ /* Don't need to send it to the remote server, if the local clipboard is
+ * empty */
+ if (clipboard->clipboard->length == 0)
+ return 0;
Review comment:
Is this related to this pull request?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] necouchman commented on a change in pull request #317: GUACAMOLE-1186: Maintain the clipboard content of the remote server.
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #317:
URL: https://github.com/apache/guacamole-server/pull/317#discussion_r547298953
##########
File path: src/protocols/rdp/channels/cliprdr.c
##########
@@ -283,7 +283,8 @@ static UINT guac_rdp_cliprdr_format_list(CliprdrClientContext* cliprdr,
if (guac_rdp_cliprdr_format_supported(format_list, CF_TEXT))
return guac_rdp_cliprdr_send_format_data_request(cliprdr, CF_TEXT);
- /* Ignore any unsupported data */
+ /* Suppose that any unsupported data is raw data (in this case, CF_RAW) */
+ guac_rdp_cliprdr_send_format_data_request(cliprdr, CF_RAW);
Review comment:
Yes, but why reset the local clipboard? Why is resetting the local clipboard the solution to not erasing what's on the remote clipboard?
##########
File path: src/protocols/rdp/channels/cliprdr.c
##########
@@ -614,6 +619,11 @@ int guac_rdp_clipboard_end_handler(guac_user* user, guac_stream* stream) {
if (clipboard == NULL)
return 0;
+ /* Don't need to send it to the remote server, if the local clipboard is
+ * empty */
+ if (clipboard->clipboard->length == 0)
+ return 0;
Review comment:
I'm not sure this logic is correct. What is someone wants to clear the clipboard and zero it out, removing all contents?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] myjimmy commented on a change in pull request #317: GUACAMOLE-1186: Maintain the clipboard content of the remote server.
Posted by GitBox <gi...@apache.org>.
myjimmy commented on a change in pull request #317:
URL: https://github.com/apache/guacamole-server/pull/317#discussion_r548052800
##########
File path: src/protocols/rdp/channels/cliprdr.c
##########
@@ -614,6 +619,11 @@ int guac_rdp_clipboard_end_handler(guac_user* user, guac_stream* stream) {
if (clipboard == NULL)
return 0;
+ /* Don't need to send it to the remote server, if the local clipboard is
+ * empty */
+ if (clipboard->clipboard->length == 0)
+ return 0;
Review comment:
Sorry, I can't understand your intention.
Is this related to this pull request?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] myjimmy commented on a change in pull request #317: GUACAMOLE-1186: Maintain the clipboard content of the remote server.
Posted by GitBox <gi...@apache.org>.
myjimmy commented on a change in pull request #317:
URL: https://github.com/apache/guacamole-server/pull/317#discussion_r547306132
##########
File path: src/protocols/rdp/channels/cliprdr.c
##########
@@ -283,7 +283,8 @@ static UINT guac_rdp_cliprdr_format_list(CliprdrClientContext* cliprdr,
if (guac_rdp_cliprdr_format_supported(format_list, CF_TEXT))
return guac_rdp_cliprdr_send_format_data_request(cliprdr, CF_TEXT);
- /* Ignore any unsupported data */
+ /* Suppose that any unsupported data is raw data (in this case, CF_RAW) */
+ guac_rdp_cliprdr_send_format_data_request(cliprdr, CF_RAW);
Review comment:
In the Apache Guacamole, the process related to the clipboard has one cyclic loop.
If the remote clipboard is changed, the content of the remote clipboard is reflected in the local clipboard.
This produces one regression to reflect the local clipboard to the remote clipboard, again.
So, I thought that we have to reset the local clipboard when copying any data other than text in the remote server.
Then, we can check the length of the local clipboard in `guac_rdp_clipboard_end_handler()` function to decide whether we maintain the clipboard content of the remote server.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #317: GUACAMOLE-1186: Maintain the clipboard content of the remote server.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #317:
URL: https://github.com/apache/guacamole-server/pull/317#discussion_r548476115
##########
File path: src/protocols/rdp/channels/cliprdr.c
##########
@@ -614,6 +619,11 @@ int guac_rdp_clipboard_end_handler(guac_user* user, guac_stream* stream) {
if (clipboard == NULL)
return 0;
+ /* Don't need to send it to the remote server, if the local clipboard is
+ * empty */
+ if (clipboard->clipboard->length == 0)
+ return 0;
Review comment:
Yes. In response to Nick asking noting that this change would remove the ability for a user to clear the remote clipboard, you said:
> No, it can never happen. I think that we don't need to update the remote clipboard when there is no data in the local clipboard.
On the contrary, from the above link:
* It can happen.
* It is desirable to be able to do so for the sake of privacy.
If the local clipboard is intentionally cleared, the remote clipboard needs to be cleared, as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #317: GUACAMOLE-1186: Maintain the clipboard content of the remote server.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #317:
URL: https://github.com/apache/guacamole-server/pull/317#discussion_r547886760
##########
File path: src/protocols/rdp/channels/cliprdr.c
##########
@@ -614,6 +619,11 @@ int guac_rdp_clipboard_end_handler(guac_user* user, guac_stream* stream) {
if (clipboard == NULL)
return 0;
+ /* Don't need to send it to the remote server, if the local clipboard is
+ * empty */
+ if (clipboard->clipboard->length == 0)
+ return 0;
Review comment:
It looks like it can:
https://www.windowscentral.com/how-clear-clipboard-data-shortcut-windows-10
Testing the suggestion at the link above (running `echo off | clip` from within the Windows command line), that does indeed clear the clipboard, and Guacamole currently handles that correctly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] myjimmy commented on a change in pull request #317: GUACAMOLE-1186: Maintain the clipboard content of the remote server.
Posted by GitBox <gi...@apache.org>.
myjimmy commented on a change in pull request #317:
URL: https://github.com/apache/guacamole-server/pull/317#discussion_r547297801
##########
File path: src/protocols/rdp/channels/cliprdr.c
##########
@@ -283,7 +283,8 @@ static UINT guac_rdp_cliprdr_format_list(CliprdrClientContext* cliprdr,
if (guac_rdp_cliprdr_format_supported(format_list, CF_TEXT))
return guac_rdp_cliprdr_send_format_data_request(cliprdr, CF_TEXT);
- /* Ignore any unsupported data */
+ /* Suppose that any unsupported data is raw data (in this case, CF_RAW) */
+ guac_rdp_cliprdr_send_format_data_request(cliprdr, CF_RAW);
Review comment:
> why are we requesting that the RDP server send us that data?
Because we need to reset the local clipboard. That's, to get the opportunity to reset the local clipboard when users copy any data other than text.
The added new code of `guac_rdp_cliprdr_format_list()` function let the remote server to call `guac_rdp_cliprdr_format_data_response()` function according to **"Remote Desktop Protocol: Clipboard Virtual Channel Extension"**.
If users copy any kind of data other than text, so we can use `guac_rdp_cliprdr_format_data_response()` function to reset the local clipboard.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] myjimmy commented on a change in pull request #317: GUACAMOLE-1186: Maintain the clipboard content of the remote server.
Posted by GitBox <gi...@apache.org>.
myjimmy commented on a change in pull request #317:
URL: https://github.com/apache/guacamole-server/pull/317#discussion_r548052800
##########
File path: src/protocols/rdp/channels/cliprdr.c
##########
@@ -614,6 +619,11 @@ int guac_rdp_clipboard_end_handler(guac_user* user, guac_stream* stream) {
if (clipboard == NULL)
return 0;
+ /* Don't need to send it to the remote server, if the local clipboard is
+ * empty */
+ if (clipboard->clipboard->length == 0)
+ return 0;
Review comment:
Sorry, I can't understand this.
Is this related to this pull request?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #317: GUACAMOLE-1186: Maintain the clipboard content of the remote server.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #317:
URL: https://github.com/apache/guacamole-server/pull/317#discussion_r548476115
##########
File path: src/protocols/rdp/channels/cliprdr.c
##########
@@ -614,6 +619,11 @@ int guac_rdp_clipboard_end_handler(guac_user* user, guac_stream* stream) {
if (clipboard == NULL)
return 0;
+ /* Don't need to send it to the remote server, if the local clipboard is
+ * empty */
+ if (clipboard->clipboard->length == 0)
+ return 0;
Review comment:
Yes. In response to Nick noting that this change would remove the ability for a user to clear the remote clipboard, you said:
> No, it can never happen. I think that we don't need to update the remote clipboard when there is no data in the local clipboard.
On the contrary, from the above link:
* It can happen.
* It is desirable to be able to do so for the sake of privacy.
If the local clipboard is intentionally cleared, the remote clipboard needs to be cleared, as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] myjimmy commented on a change in pull request #317: GUACAMOLE-1186: Maintain the clipboard content of the remote server.
Posted by GitBox <gi...@apache.org>.
myjimmy commented on a change in pull request #317:
URL: https://github.com/apache/guacamole-server/pull/317#discussion_r547309949
##########
File path: src/protocols/rdp/channels/cliprdr.c
##########
@@ -614,6 +619,11 @@ int guac_rdp_clipboard_end_handler(guac_user* user, guac_stream* stream) {
if (clipboard == NULL)
return 0;
+ /* Don't need to send it to the remote server, if the local clipboard is
+ * empty */
+ if (clipboard->clipboard->length == 0)
+ return 0;
Review comment:
No, it can never happen. I think that we don't need to update the remote clipboard when there is no data in the local clipboard.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] mike-jumper commented on pull request #317: GUACAMOLE-1186: Maintain the clipboard content of the remote server.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #317:
URL: https://github.com/apache/guacamole-server/pull/317#issuecomment-750107665
I'm not against altering things such that things work cleanly even if you attempt to copy data that Guacamole itself does not currently handle, but I don't think this is the correct approach. This will remove the ability to clear the clipboard, and looks like it will result in requesting the RDP server send us data that we will simply drop (it would be better to just not request it).
You mentioned:
> Because we need to reset the local clipboard. That's, to get the opportunity to reset the local clipboard when users copy any data other than text.
There is no need to request the RDP server send us _anything_ in order to do that. If you want to reset the local clipboard, you can just reset it. That said, I don't think we should be resetting the local clipboard as a means of achieving what you're trying to achieve.
I'd think the solution to the difficulty you're encountering with images would be:
* Don't clear the clipboard if unknown data is received (behave as if nothing has changed)
* Don't send the local clipboard to the RDP server unless it's changed locally.
I believe the server is already doing the former, and the client _should_ actually already be doing the latter. If either of those are not the case, then perhaps correcting that would correct this cleanly?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] necouchman commented on a change in pull request #317: GUACAMOLE-1186: Maintain the clipboard content of the remote server.
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #317:
URL: https://github.com/apache/guacamole-server/pull/317#discussion_r546251100
##########
File path: src/protocols/rdp/channels/cliprdr.c
##########
@@ -614,6 +619,11 @@ int guac_rdp_clipboard_end_handler(guac_user* user, guac_stream* stream) {
if (clipboard == NULL)
return 0;
+ /* Don't need to send it to the remote server, if the local clipboard is
+ * empty */
+ if (clipboard->clipboard->length == 0)
+ return 0;
Review comment:
I'm not sure this logic is correct. What if someone wants to clear the clipboard and zero it out, removing all contents?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] myjimmy commented on a change in pull request #317: GUACAMOLE-1186: Maintain the clipboard content of the remote server.
Posted by GitBox <gi...@apache.org>.
myjimmy commented on a change in pull request #317:
URL: https://github.com/apache/guacamole-server/pull/317#discussion_r547306958
##########
File path: src/protocols/rdp/channels/cliprdr.c
##########
@@ -283,7 +283,8 @@ static UINT guac_rdp_cliprdr_format_list(CliprdrClientContext* cliprdr,
if (guac_rdp_cliprdr_format_supported(format_list, CF_TEXT))
return guac_rdp_cliprdr_send_format_data_request(cliprdr, CF_TEXT);
- /* Ignore any unsupported data */
+ /* Suppose that any unsupported data is raw data (in this case, CF_RAW) */
+ guac_rdp_cliprdr_send_format_data_request(cliprdr, CF_RAW);
Review comment:
Also, you can see the following image for the workflow when copying any data in the remote server:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #317: GUACAMOLE-1186: Maintain the clipboard content of the remote server.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #317:
URL: https://github.com/apache/guacamole-server/pull/317#discussion_r547175176
##########
File path: src/protocols/rdp/channels/cliprdr.c
##########
@@ -283,7 +283,8 @@ static UINT guac_rdp_cliprdr_format_list(CliprdrClientContext* cliprdr,
if (guac_rdp_cliprdr_format_supported(format_list, CF_TEXT))
return guac_rdp_cliprdr_send_format_data_request(cliprdr, CF_TEXT);
- /* Ignore any unsupported data */
+ /* Suppose that any unsupported data is raw data (in this case, CF_RAW) */
+ guac_rdp_cliprdr_send_format_data_request(cliprdr, CF_RAW);
Review comment:
If the format of the data in the remote clipboard is not supported (is not handled by one of the `if` statements above), why are we requesting that the RDP server send us that data?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org