fix(auth): SyntaxError not being considered Network Error (aws#5512)

## Problem:

There were some previous changes that caused the existing implementation
of a `SyntaxError` to not be considered a Network Error. And during
token refresh we were seeing SyntaxErrors still appearing

## Solution:

Create a single class which wraps a `SyntaxError`, called
`AwsClientResponseError`. Under the hood of a SyntaxError is the real
error that originates from a failed AWS SDK Client response, this is how
we determine if it should be an `AwsClientResponseError`. This new class
can only be created if given the correct SyntaxError instance to
`AwsClientResponseError.instanceIf()`.

Then we can simply check if an error matches by using `instanceif
AwsClientResponseError`

Any existing code that was related to this scenario has all been
centralized in to the AwsClientResponseError class.

---

<!--- REMINDER: Ensure that your PR meets the guidelines in
CONTRIBUTING.md -->

License: I confirm that my contribution is made under the terms of the
Apache 2.0 license.

---------

Signed-off-by: Nikolas Komonen <[email protected]>

Author: nkomonen-amazon

packages/amazonq/.changes/next-release/Bug Fix-54ae3b04-fe8b-40e8-866a-7f9f73f3c7cb.json

@@ -0,0 +1,4 @@
+{
+	"type": "Bug Fix",
+	"description": "Auth: `SyntaxError` causing unexpected SSO logout"
+}

packages/core/src/auth/sso/clients.ts

@@ -28,7 +28,7 @@ import { pageableToCollection, partialClone } from '../../shared/utilities/colle
 import { assertHasProps, isNonNullable, RequiredProps, selectFrom } from '../../shared/utilities/tsUtils'
 import { getLogger } from '../../shared/logger'
 import { SsoAccessTokenProvider } from './ssoAccessTokenProvider'
-import { ToolkitError, getHttpStatusCode, getReasonFromSyntaxError, isClientFault } from '../../shared/errors'
+import { AwsClientResponseError, isClientFault } from '../../shared/errors'
 import { DevSettings } from '../../shared/settings'
 import { SdkError } from '@aws-sdk/types'
 import { HttpRequest, HttpResponse } from '@smithy/protocol-http'
@@ -90,20 +90,8 @@ export class OidcClient {
         try {
             response = await this.client.createToken(request as CreateTokenRequest)
         } catch (err) {
-            // In rare cases the SDK client may get unexpected data from its API call.
-            // This will throw a SyntaxError that contains the returned data.
-            if (err instanceof SyntaxError) {
-                getLogger().error(`createToken: SSOIDC Client received an unexpected non-JSON response: %O`, err)
-                throw new ToolkitError(
-                    `SDK Client unexpected error response: data response code: ${getHttpStatusCode(err)}, data reason: ${getReasonFromSyntaxError(err)}`,
-                    {
-                        code: `${getHttpStatusCode(err)}`,
-                        cause: err,
-                    }
-                )
-            } else {
-                throw err
-            }
+            const newError = AwsClientResponseError.instanceIf(err)
+            throw newError
         }
         assertHasProps(response, 'accessToken', 'expiresIn')
 

packages/core/src/codewhispererChat/controllers/chat/controller.ts

@@ -48,7 +48,7 @@ import { LspController } from '../../../amazonq/lsp/lspController'
 import { CodeWhispererSettings } from '../../../codewhisperer/util/codewhispererSettings'
 import { getSelectedCustomization } from '../../../codewhisperer/util/customizationUtil'
 import { FeatureConfigProvider } from '../../../shared/featureConfig'
-import { getHttpStatusCode, getReasonFromSyntaxError } from '../../../shared/errors'
+import { getHttpStatusCode, AwsClientResponseError } from '../../../shared/errors'
 
 export interface ChatControllerMessagePublishers {
     readonly processPromptChatMessage: MessagePublisher<PromptMessage>
@@ -299,7 +299,7 @@ export class ChatController {
             errorMessage = e.toUpperCase()
         } else if (e instanceof SyntaxError) {
             // Workaround to handle case when LB returns web-page with error and our client doesn't return proper exception
-            errorMessage = getReasonFromSyntaxError(e) ?? defaultMessage
+            errorMessage = AwsClientResponseError.tryExtractReasonFromSyntaxError(e) ?? defaultMessage
         } else if (e instanceof CodeWhispererStreamingServiceException) {
             errorMessage = e.message
             requestID = e.$metadata.requestId

packages/core/src/shared/errors.ts

@@ -789,11 +789,11 @@ export function isNetworkError(err?: unknown): err is Error & { code: string } {
     if (
         isVSCodeProxyError(err) ||
         isSocketTimeoutError(err) ||
-        isHttpSyntaxError(err) ||
         isEnoentError(err) ||
         isEaccesError(err) ||
         isEbadfError(err) ||
-        isEconnRefusedError(err)
+        isEconnRefusedError(err) ||
+        err instanceof AwsClientResponseError
     ) {
         return true
     }
@@ -847,18 +847,6 @@ function isSocketTimeoutError(err: Error): boolean {
     return isError(err, 'TimeoutError', 'Connection timed out after')
 }
 
-/**
- * Expected JSON response from HTTP request, but got an error HTML error page instead.
- *
- * IMPORTANT:
- *
- * This function is influenced by {@link getReasonFromSyntaxError()} since it modifies the error
- * message with the real underlying reason, instead of the default "Unexpected token" message.
- */
-function isHttpSyntaxError(err: Error): boolean {
-    return isError(err, 'SyntaxError', 'SDK Client unexpected error response')
-}
-
 /**
  * We were seeing errors of ENOENT for the oidc FQDN (eg: oidc.us-east-1.amazonaws.com) during the SSO flow.
  * Our assumption is that this is an intermittent error.
@@ -887,30 +875,69 @@ function isError(err: Error, id: string, messageIncludes: string = '') {
 }
 
 /**
- * AWS SDK clients may rarely make requests that results in something other than JSON data
- * being returned (e.g html). This will cause the client to throw a SyntaxError as a result
+ * AWS SDK clients make requests with the expected result to be JSON data.
+ * But in some cases the request may fail and result in an error HTML page being returned instead
+ * of the JSON. This will cause the client to throw a `SyntaxError` as a result
  * of attempt to deserialize the non-JSON data.
- * While the contents of the response may contain sensitive information, there may be a reason
- * for failure embedded. This function attempts to extract that reason.
  *
- * Example error message before extracting:
- * "Unexpected token '<', "<html><bod"... is not valid JSON Deserialization error: to see the raw response, inspect the hidden field {error}.$response on this object."
+ * But within the `SyntaxError` instance is the real reason for the failure.
+ * This class attempts to extract the underlying issue from the SyntaxError.
  *
- * If the reason cannot be found or the error is not a SyntaxError, return undefined.
+ * Example SyntaxError message before extracting the underlying issue:
+ *  - "Unexpected token '<', "<html><bod"... is not valid JSON Deserialization error: to see the raw response, inspect the hidden field {error}.$response on this object."
+ * Once we extract the real error message from the hidden field, `$response.reason`, we get messages similar to:
+ *  - "SDK Client unexpected error response: data response code: 403, data reason: Forbidden | Unexpected ..."
  */
-export function getReasonFromSyntaxError(err: Error): string | undefined {
-    if (!(err instanceof SyntaxError)) {
-        return undefined
+export class AwsClientResponseError extends Error {
+    /** Use {@link instanceIf} to create instance. */
+    protected constructor(err: unknown) {
+        const underlyingErrorMsg = AwsClientResponseError.tryExtractReasonFromSyntaxError(err)
+
+        /**
+         * This condition should never be hit since {@link AwsClientResponseError.instanceIf}
+         * is the only way to create an instance of this class, due to the constructor not being public.
+         *
+         * The following only exists to make the type checker happy.
+         */
+        if (!(underlyingErrorMsg && err instanceof Error)) {
+            throw Error(`Cannot create AwsClientResponseError from ${JSON.stringify(err)}}`)
+        }
+
+        super(underlyingErrorMsg)
     }
 
-    if (hasKey(err, '$response')) {
-        const response = err['$response']
-        if (response && hasKey(response, 'reason')) {
-            return response['reason'] as string
+    /**
+     * Resolves an instance of {@link AwsClientResponseError} if the given error matches certain criteria.
+     * Otherwise the original error is returned.
+     */
+    static instanceIf<T>(err: T): AwsClientResponseError | T {
+        const reason = AwsClientResponseError.tryExtractReasonFromSyntaxError(err)
+        if (reason && reason.startsWith('SDK Client unexpected error response: data response code:')) {
+            getLogger().debug(`Creating AwsClientResponseError from SyntaxError: %O`, err)
+            return new AwsClientResponseError(err)
         }
+        return err
     }
 
-    return undefined
+    /**
+     * Returns the true underlying error message from a `SyntaxError`, if possible.
+     * Otherwise returning undefined.
+     */
+    static tryExtractReasonFromSyntaxError(err: unknown): string | undefined {
+        if (!(err instanceof SyntaxError)) {
+            return undefined
+        }
+
+        // See the class docstring to explain how we know the existence of the following keys
+        if (hasKey(err, '$response')) {
+            const response = err['$response']
+            if (response && hasKey(response, 'reason')) {
+                return response['reason'] as string
+            }
+        }
+
+        return undefined
+    }
 }
 
 /**

packages/core/src/test/shared/errors.test.ts

@@ -15,6 +15,7 @@ import {
     isNetworkError,
     resolveErrorMessageToDisplay,
     scrubNames,
+    AwsClientResponseError,
     ToolkitError,
     tryRun,
     UnknownError,
@@ -482,6 +483,13 @@ describe('util', function () {
         assert.deepStrictEqual(getTelemetryReasonDesc(toolkitError), 'ToolkitError Message | Cause Message x/x/x/x.txt')
     })
 
+    function makeSyntaxErrorWithSdkClientError() {
+        const syntaxError: Error = new SyntaxError('The SyntaxError message')
+        // Under the hood of a SyntaxError may be a hidden field with the real reason for the failure
+        ;(syntaxError as any)['$response'] = { reason: 'SDK Client unexpected error response: data response code: 500' }
+        return syntaxError
+    }
+
     it('isNetworkError()', function () {
         assert.deepStrictEqual(
             isNetworkError(new Error('Failed to establish a socket connection to proxies BLAH BLAH BLAH')),
@@ -493,15 +501,33 @@ describe('util', function () {
             false,
             'Incorrectly indicated as network error'
         )
-        let err = new Error('SDK Client unexpected error response: Blah Blah Blah')
-        err.name = 'SyntaxError'
-        assert.deepStrictEqual(isNetworkError(err), true, 'Did not indicate SyntaxError as network error')
 
-        err = new Error('getaddrinfo ENOENT oidc.us-east-1.amazonaws.com')
+        const awsClientResponseError = AwsClientResponseError.instanceIf(makeSyntaxErrorWithSdkClientError())
+        assert.deepStrictEqual(
+            isNetworkError(awsClientResponseError),
+            true,
+            'Did not indicate SyntaxError as network error'
+        )
+
+        const err = new Error('getaddrinfo ENOENT oidc.us-east-1.amazonaws.com')
         ;(err as any).code = 'ENOENT'
         assert.deepStrictEqual(isNetworkError(err), true, 'Did not indicate ENOENT error as network error')
     })
 
+    it('AwsClientResponseError', function () {
+        const syntaxError = makeSyntaxErrorWithSdkClientError()
+
+        assert.deepStrictEqual(
+            AwsClientResponseError.tryExtractReasonFromSyntaxError(syntaxError),
+            'SDK Client unexpected error response: data response code: 500'
+        )
+        const responseError = AwsClientResponseError.instanceIf(syntaxError)
+        assert(!(responseError instanceof SyntaxError))
+        assert(responseError instanceof Error)
+        assert(responseError instanceof AwsClientResponseError)
+        assert(responseError.message === 'SDK Client unexpected error response: data response code: 500')
+    })
+
     it('scrubNames()', async function () {
         const fakeUser = 'jdoe123'
         assert.deepStrictEqual(scrubNames('', fakeUser), '')

packages/toolkit/.changes/next-release/Bug Fix-48f605af-ae0d-477f-b1bb-1691b64ad96c.json

@@ -0,0 +1,4 @@
+{
+	"type": "Bug Fix",
+	"description": "Auth: `SyntaxError` causing unexpected SSO logout"
+}