From fc6c0e9b7f037c57f6e442d73a8c7f0744a55e6c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mih=C3=A1ly=20Lengyel?= <mihaly@lengyel.tech>
Date: Sun, 27 Oct 2024 02:16:41 +0100
Subject: [PATCH] fix: self review test fixes (#955)

* feat: better error handling for invalid login/out challenges

* ci: make ci job only run if necessary
---
 .circleci/config.yml                          |  6 +++++
 .../oauth2provider/api/implementation.js      |  3 +++
 .../oauth2provider/recipeImplementation.js    | 22 +++++++++++++-----
 lib/build/recipe/oauth2provider/types.d.ts    |  9 +++++---
 .../oauth2provider/api/implementation.ts      |  4 ++++
 .../oauth2provider/recipeImplementation.ts    | 23 ++++++++++++++-----
 lib/ts/recipe/oauth2provider/types.ts         |  5 +++-
 7 files changed, 56 insertions(+), 16 deletions(-)

diff --git a/.circleci/config.yml b/.circleci/config.yml
index 99df7aa8a..9f4c608fe 100644
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -64,6 +64,12 @@ jobs:
 workflows:
     version: 2
     tagged-build:
+        when: # One must be true to trigger
+            or:
+                - equal: [true, <<pipeline.parameters.force>>]
+                - matches: { pattern: "dev-v[0-9]+(\\.[0-9]+)*", value: << pipeline.git.tag >> }
+                - matches: { pattern: "v[0-9]+(\\.[0-9]+)*", value: << pipeline.git.tag >> }
+                - equal: ["master", << pipeline.git.branch >>]
         jobs:
             - publish:
                   context:
diff --git a/lib/build/recipe/oauth2provider/api/implementation.js b/lib/build/recipe/oauth2provider/api/implementation.js
index fa4232d17..1abde6a64 100644
--- a/lib/build/recipe/oauth2provider/api/implementation.js
+++ b/lib/build/recipe/oauth2provider/api/implementation.js
@@ -169,6 +169,9 @@ function getAPIImplementation() {
                 challenge: logoutChallenge,
                 userContext,
             });
+            if ("error" in response) {
+                return response;
+            }
             const res = await utils_1.handleLogoutInternalRedirects({
                 response,
                 recipeImplementation: options.recipeImplementation,
diff --git a/lib/build/recipe/oauth2provider/recipeImplementation.js b/lib/build/recipe/oauth2provider/recipeImplementation.js
index d1f440e85..f40da4bce 100644
--- a/lib/build/recipe/oauth2provider/recipeImplementation.js
+++ b/lib/build/recipe/oauth2provider/recipeImplementation.js
@@ -803,12 +803,14 @@ function getRecipeInterface(
                     };
                 } else {
                     // Accept the logout challenge immediately as there is no supertokens session
-                    redirectTo = (
-                        await this.acceptLogoutRequest({
-                            challenge: logoutChallenge,
-                            userContext: input.userContext,
-                        })
-                    ).redirectTo;
+                    const acceptLogoutResponse = await this.acceptLogoutRequest({
+                        challenge: logoutChallenge,
+                        userContext: input.userContext,
+                    });
+                    if ("error" in acceptLogoutResponse) {
+                        return acceptLogoutResponse;
+                    }
+                    return { redirectTo: acceptLogoutResponse.redirectTo };
                 }
             }
             // CASE 2 or 3 (See above notes)
@@ -831,6 +833,14 @@ function getRecipeInterface(
                 {},
                 input.userContext
             );
+            if (resp.status !== "OK") {
+                return {
+                    status: "ERROR",
+                    statusCode: resp.statusCode,
+                    error: resp.error,
+                    errorDescription: resp.errorDescription,
+                };
+            }
             const redirectTo = getUpdatedRedirectTo(appInfo, resp.redirectTo);
             if (redirectTo.endsWith("/fallbacks/logout/callback")) {
                 return {
diff --git a/lib/build/recipe/oauth2provider/types.d.ts b/lib/build/recipe/oauth2provider/types.d.ts
index 8c4f49b7e..2b5dbf8e9 100644
--- a/lib/build/recipe/oauth2provider/types.d.ts
+++ b/lib/build/recipe/oauth2provider/types.d.ts
@@ -354,9 +354,12 @@ export declare type RecipeInterface = {
     acceptLogoutRequest(input: {
         challenge: string;
         userContext: UserContext;
-    }): Promise<{
-        redirectTo: string;
-    }>;
+    }): Promise<
+        | {
+              redirectTo: string;
+          }
+        | ErrorOAuth2
+    >;
     rejectLogoutRequest(input: {
         challenge: string;
         userContext: UserContext;
diff --git a/lib/ts/recipe/oauth2provider/api/implementation.ts b/lib/ts/recipe/oauth2provider/api/implementation.ts
index 7a13bee92..28dd37a0c 100644
--- a/lib/ts/recipe/oauth2provider/api/implementation.ts
+++ b/lib/ts/recipe/oauth2provider/api/implementation.ts
@@ -185,6 +185,10 @@ export default function getAPIImplementation(): APIInterface {
                 userContext,
             });
 
+            if ("error" in response) {
+                return response;
+            }
+
             const res = await handleLogoutInternalRedirects({
                 response,
                 recipeImplementation: options.recipeImplementation,
diff --git a/lib/ts/recipe/oauth2provider/recipeImplementation.ts b/lib/ts/recipe/oauth2provider/recipeImplementation.ts
index 1af9d5611..ee2b1a99f 100644
--- a/lib/ts/recipe/oauth2provider/recipeImplementation.ts
+++ b/lib/ts/recipe/oauth2provider/recipeImplementation.ts
@@ -819,12 +819,14 @@ export default function getRecipeInterface(
                     };
                 } else {
                     // Accept the logout challenge immediately as there is no supertokens session
-                    redirectTo = (
-                        await this.acceptLogoutRequest({
-                            challenge: logoutChallenge,
-                            userContext: input.userContext,
-                        })
-                    ).redirectTo;
+                    const acceptLogoutResponse = await this.acceptLogoutRequest({
+                        challenge: logoutChallenge,
+                        userContext: input.userContext,
+                    });
+                    if ("error" in acceptLogoutResponse) {
+                        return acceptLogoutResponse;
+                    }
+                    return { redirectTo: acceptLogoutResponse.redirectTo };
                 }
             }
 
@@ -851,6 +853,15 @@ export default function getRecipeInterface(
                 input.userContext
             );
 
+            if (resp.status !== "OK") {
+                return {
+                    status: "ERROR",
+                    statusCode: resp.statusCode,
+                    error: resp.error,
+                    errorDescription: resp.errorDescription,
+                };
+            }
+
             const redirectTo = getUpdatedRedirectTo(appInfo, resp.redirectTo);
 
             if (redirectTo.endsWith("/fallbacks/logout/callback")) {
diff --git a/lib/ts/recipe/oauth2provider/types.ts b/lib/ts/recipe/oauth2provider/types.ts
index e73880482..e6b5a7fb9 100644
--- a/lib/ts/recipe/oauth2provider/types.ts
+++ b/lib/ts/recipe/oauth2provider/types.ts
@@ -412,7 +412,10 @@ export type RecipeInterface = {
         shouldTryRefresh: boolean;
         userContext: UserContext;
     }): Promise<{ redirectTo: string } | ErrorOAuth2>;
-    acceptLogoutRequest(input: { challenge: string; userContext: UserContext }): Promise<{ redirectTo: string }>;
+    acceptLogoutRequest(input: {
+        challenge: string;
+        userContext: UserContext;
+    }): Promise<{ redirectTo: string } | ErrorOAuth2>;
     rejectLogoutRequest(input: { challenge: string; userContext: UserContext }): Promise<{ status: "OK" }>;
 };