Skip to content

Add specific message for network errors on app launch (Web UI)#25527

Merged
gabrielcorado merged 5 commits intomasterfrom
gabrielcorado/app-ssl-error-message
May 3, 2023
Merged

Add specific message for network errors on app launch (Web UI)#25527
gabrielcorado merged 5 commits intomasterfrom
gabrielcorado/app-ssl-error-message

Conversation

@gabrielcorado
Copy link
Copy Markdown
Contributor

@gabrielcorado gabrielcorado commented May 3, 2023

Related to #24255.

This PR changes the error Failed to fetch returned when launching an application (from Web UI) with a more specific message that can help users better understand why it is failing.

The error cause is better described on the issue:

The Failed to fetch error is due to the browser not trusting the self-signed certificates from the Teleport proxy (NET::ERR_CERT_AUTHORITY_INVALID on Chrome). It happens on the /web/launch page because, before redirecting the application domain, it requests the https://app.domain.dev/x-teleport-auth (responsible for setting the authentication cookies).

Screenshot of how the page will look after the change.

Screenshot 2023-05-03 at 17 45 41

@gabrielcorado gabrielcorado requested a review from r0mant May 3, 2023 01:12
@gabrielcorado gabrielcorado self-assigned this May 3, 2023
@github-actions github-actions Bot requested review from gzdunek and ryanclark May 3, 2023 01:12
Comment thread web/packages/teleport/src/AppLauncher/AppLauncher.tsx Outdated
let statusText = 'Something went wrong';
if (err instanceof Error) {

if (err instanceof TypeError) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no expert but TypeError seems broad. Would be it better to check if the error text is "Failed to fetch" like what we see is being returned in this case?

@ryanclark @kimlisa I'll defer to you on this one.

Copy link
Copy Markdown
Contributor

@kimlisa kimlisa May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

roman has a point b/c there could be other TypeError's like accessing a field in an undefined variable (seems unlikely though), however different browsers will have different error messages for network errors. tested them out:

chrome: failed to fetch
firefox: network error when attempting to fetch resource.
safari: load failed

so i think the way it is okay, as long as we are aware it could mean something else which the web console will print out the real error

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or we could just add the real error message err.message along with the status text

@r0mant r0mant requested a review from kimlisa May 3, 2023 01:21
Copy link
Copy Markdown
Contributor

@kimlisa kimlisa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after addressing roman's comment

let statusText = 'Something went wrong';
if (err instanceof Error) {

if (err instanceof TypeError) {
Copy link
Copy Markdown
Contributor

@kimlisa kimlisa May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

roman has a point b/c there could be other TypeError's like accessing a field in an undefined variable (seems unlikely though), however different browsers will have different error messages for network errors. tested them out:

chrome: failed to fetch
firefox: network error when attempting to fetch resource.
safari: load failed

so i think the way it is okay, as long as we are aware it could mean something else which the web console will print out the real error

@gabrielcorado gabrielcorado requested a review from r0mant May 3, 2023 20:53
@gabrielcorado
Copy link
Copy Markdown
Contributor Author

@r0mant I've updated the message (attached an image on the PR description to see how it looks on the page). I couldn't add a link to the error message because the component accepts only text. I'm not sure if we want to update it to have a dangerouslySetInnerHTML attribute, update the component to support children, or solve it differently.

@gabrielcorado gabrielcorado enabled auto-merge May 3, 2023 22:48
@gabrielcorado gabrielcorado added this pull request to the merge queue May 3, 2023
Merged via the queue into master with commit 9200cae May 3, 2023
@gabrielcorado gabrielcorado deleted the gabrielcorado/app-ssl-error-message branch May 3, 2023 23:04
@public-teleport-github-review-bot
Copy link
Copy Markdown

@gabrielcorado See the table below for backport results.

Branch Result
branch/v11 Create PR
branch/v12 Create PR
branch/v13 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants