From 9e1b33f24c7e1d8b44d9241df92de45183a31e6a Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Thu, 26 Oct 2023 12:35:45 -0600 Subject: [PATCH 1/2] desktop access: soften the disconnect error If the Windows host (RDP server) decides to terminate the connection, it often sends a message indicating why the disconnect is coming. One example of a server-initiated disconnect would be when the user clicks "log off" in the RDP session or when they restart the remote machine. We forward this message to the UI, but we put it in a big red ERROR box that makes it look like something went wrong. Errors should be reserved for unexpected disconnections, so covert the dialog to "info" level and soften the messaging. --- .../src/DesktopSession/DesktopSession.tsx | 8 ++--- .../DesktopSession.story.test.tsx.snap | 32 +++++++++---------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/web/packages/teleport/src/DesktopSession/DesktopSession.tsx b/web/packages/teleport/src/DesktopSession/DesktopSession.tsx index 051026cad56a2..50e73d7420a0e 100644 --- a/web/packages/teleport/src/DesktopSession/DesktopSession.tsx +++ b/web/packages/teleport/src/DesktopSession/DesktopSession.tsx @@ -23,7 +23,7 @@ import { ButtonSecondary, ButtonPrimary, } from 'design'; -import { Danger } from 'design/Alert'; +import { Info } from 'design/Alert'; import Dialog, { DialogHeader, DialogTitle, @@ -126,12 +126,12 @@ export function DesktopSession(props: State) { open={errorDialog.open} > - Error + Disconnected <> - {errorDialog.text}} /> - Refresh the page to try again. + {errorDialog.text}} /> + Refresh the page to reconnect. diff --git a/web/packages/teleport/src/DesktopSession/__snapshots__/DesktopSession.story.test.tsx.snap b/web/packages/teleport/src/DesktopSession/__snapshots__/DesktopSession.story.test.tsx.snap index 5735526ab4edc..09ef916a40cb4 100644 --- a/web/packages/teleport/src/DesktopSession/__snapshots__/DesktopSession.story.test.tsx.snap +++ b/web/packages/teleport/src/DesktopSession/__snapshots__/DesktopSession.story.test.tsx.snap @@ -874,7 +874,7 @@ exports[`connection error 1`] = ` overflow: auto; word-break: break-word; line-height: 1.5; - background: #FF6257; + background: #039be5; color: #000000; } @@ -985,7 +985,7 @@ exports[`connection error 1`] = ` class="c5" color="text.main" > - Error + Disconnected
some connection error
- Refresh the page to try again. + Refresh the page to reconnect.
- Error + Disconnected
some fetch error
- Refresh the page to try again. + Refresh the page to reconnect.
- Error + Disconnected
The application has detected an invalid internal application state. Please file a bug report for this issue at https://github.com/gravitational/teleport/issues/new?assignees=&labels=bug&template=bug_report.md
- Refresh the page to try again. + Refresh the page to reconnect.
- Error + Disconnected
Session disconnected for an unknown reason.
- Refresh the page to try again. + Refresh the page to reconnect.
Date: Mon, 6 Nov 2023 17:33:59 -0700 Subject: [PATCH 2/2] Render the error if we didn't get a disconnect reason With the prior commit, we successfully made server-side disconnects look less like errors, but we also made some real errors (like connection time outs) look less like errors. Now, we render an error indicator when we have an unexpected disconnect. We also add a special check for "RDP connection failed" which is used to indicate that the connection was never established succesfully. This also fixes an ancient bug where our strings had an extra unprintable byte at the end. This is because the TDP codec was interpreting the "severity" byte as part of the string, rather than reading only the length that was specified in the message. --- .../src/DesktopSession/DesktopSession.tsx | 19 +++++++++++++------ .../DesktopSession.story.test.tsx.snap | 6 +++--- web/packages/teleport/src/lib/tdp/codec.ts | 15 +++++++++++++-- 3 files changed, 29 insertions(+), 11 deletions(-) diff --git a/web/packages/teleport/src/DesktopSession/DesktopSession.tsx b/web/packages/teleport/src/DesktopSession/DesktopSession.tsx index 50e73d7420a0e..4c421a7d6e403 100644 --- a/web/packages/teleport/src/DesktopSession/DesktopSession.tsx +++ b/web/packages/teleport/src/DesktopSession/DesktopSession.tsx @@ -23,7 +23,7 @@ import { ButtonSecondary, ButtonPrimary, } from 'design'; -import { Info } from 'design/Alert'; +import { Danger, Info } from 'design/Alert'; import Dialog, { DialogHeader, DialogTitle, @@ -86,7 +86,7 @@ export function DesktopSession(props: State) { const computeErrorDialog = () => { // Websocket is closed but we haven't - // closed it on purpose or registered a fatal tdp error. + // closed it on purpose or registered a fatal TDP error. const unknownConnectionError = wsConnection === 'closed' && !disconnected && @@ -96,7 +96,7 @@ export function DesktopSession(props: State) { if (fetchAttempt.status === 'failed') { errorText = fetchAttempt.statusText || 'fetch attempt failed'; } else if (tdpConnection.status === 'failed') { - errorText = tdpConnection.statusText || 'tdp connection failed'; + errorText = tdpConnection.statusText || 'TDP connection failed'; } else if (tdpConnection.status === '') { errorText = tdpConnection.statusText || 'encountered a non-fatal error'; } else if (unknownConnectionError) { @@ -112,10 +112,15 @@ export function DesktopSession(props: State) { } const open = errorText !== ''; - return { open, text: errorText }; + return { + open, + text: errorText, + isError: unknownConnectionError || errorText === 'RDP connection failed', + }; }; const errorDialog = computeErrorDialog(); + const Alert = errorDialog.isError ? Danger : Info; if (errorDialog.open) { return ( @@ -126,11 +131,13 @@ export function DesktopSession(props: State) { open={errorDialog.open} > - Disconnected + + {errorDialog.isError ? 'Error' : 'Disconnected'} + <> - {errorDialog.text}} /> + {errorDialog.text}} /> Refresh the page to reconnect. diff --git a/web/packages/teleport/src/DesktopSession/__snapshots__/DesktopSession.story.test.tsx.snap b/web/packages/teleport/src/DesktopSession/__snapshots__/DesktopSession.story.test.tsx.snap index 09ef916a40cb4..ddbf5707a14b8 100644 --- a/web/packages/teleport/src/DesktopSession/__snapshots__/DesktopSession.story.test.tsx.snap +++ b/web/packages/teleport/src/DesktopSession/__snapshots__/DesktopSession.story.test.tsx.snap @@ -2407,7 +2407,7 @@ exports[`unintended disconnect 1`] = ` overflow: auto; word-break: break-word; line-height: 1.5; - background: #039be5; + background: #FF6257; color: #000000; } @@ -2518,7 +2518,7 @@ exports[`unintended disconnect 1`] = ` class="c5" color="text.main" > - Disconnected + Error
Session disconnected for an unknown reason.
diff --git a/web/packages/teleport/src/lib/tdp/codec.ts b/web/packages/teleport/src/lib/tdp/codec.ts index 3d590f8c94292..f67e27cbf18ea 100644 --- a/web/packages/teleport/src/lib/tdp/codec.ts +++ b/web/packages/teleport/src/lib/tdp/codec.ts @@ -825,12 +825,17 @@ export default class Codec { decodeNotification(buffer: ArrayBuffer): Notification { const dv = new DataView(buffer); let offset = 0; + offset += byteLength; // eat message type + const messageLength = dv.getUint32(offset); offset += uint32Length; // eat messageLength + const message = this.decodeStringMessage(buffer); offset += messageLength; // eat message + const severity = dv.getUint8(offset); + return { message, severity: toSeverity(severity), @@ -858,8 +863,14 @@ export default class Codec { // decodeStringMessage decodes a tdp message of the form // | message type (N) | message_length uint32 | message []byte private decodeStringMessage(buffer: ArrayBuffer): string { - const offset = byteLength + uint32Length; // eat message type and message_length - return this.decoder.decode(new Uint8Array(buffer.slice(offset))); + const dv = new DataView(buffer); + let offset = byteLength; // eat message type + const msgLength = dv.getUint32(offset); + offset += uint32Length; // eat messageLength + + return this.decoder.decode( + new Uint8Array(buffer.slice(offset, offset + msgLength)) + ); } // decodePngFrame decodes a raw tdp PNG frame message and returns it as a PngFrame