From a412d48fc483ea0adfe6b0d75ea3b1da062eabe0 Mon Sep 17 00:00:00 2001
From: Henning Perl <henning.perl@ory.sh>
Date: Wed, 14 Jun 2023 21:05:55 +0200
Subject: [PATCH] fix: don't assume the login challenge to be a UUID

For compatibility with https://github.com/ory/hydra/pull/3515, which
now encodes the whole flow in the login challenge, we cannot further
assume that the challenge is a UUID.
---
 hydra/hydra.go                                  |  7 ++++++-
 hydra/hydra_test.go                             |  9 +++++++++
 .../cccccccc-dda4-4700-9e42-35731f2af91e.json   | 17 +++++++++++++++++
 .../testdata/20230614205200_testdata.sql        |  8 ++++++++
 .../profiles/oidc-provider/error.spec.ts        | 16 +---------------
 5 files changed, 41 insertions(+), 16 deletions(-)
 create mode 100644 persistence/sql/migratest/fixtures/login_flow/cccccccc-dda4-4700-9e42-35731f2af91e.json
 create mode 100644 persistence/sql/migratest/testdata/20230614205200_testdata.sql

diff --git a/hydra/hydra.go b/hydra/hydra.go
index e26d32d77834..695eeed55e91 100644
--- a/hydra/hydra.go
+++ b/hydra/hydra.go
@@ -50,7 +50,12 @@ func GetLoginChallengeID(conf *config.Config, r *http.Request) (sqlxx.NullString
 		return "", errors.WithStack(herodot.ErrInternalServerError.WithReason("refusing to parse login_challenge query parameter because " + config.ViperKeyOAuth2ProviderURL + " is invalid or unset"))
 	}
 
-	return sqlxx.NullString(r.URL.Query().Get("login_challenge")), nil
+	loginChallenge := r.URL.Query().Get("login_challenge")
+	if loginChallenge == "" {
+		return "", errors.WithStack(herodot.ErrBadRequest.WithReason("the login_challenge parameter is present but empty"))
+	}
+
+	return sqlxx.NullString(loginChallenge), nil
 }
 
 func (h *DefaultHydra) getAdminURL(ctx context.Context) (string, error) {
diff --git a/hydra/hydra_test.go b/hydra/hydra_test.go
index 49f99c38957e..c00ec8bf2f4b 100644
--- a/hydra/hydra_test.go
+++ b/hydra/hydra_test.go
@@ -64,6 +64,15 @@ func TestGetLoginChallengeID(t *testing.T) {
 			want:      "",
 			assertErr: assert.NoError,
 		},
+		{
+			name: "empty login challenge; hydra is configured",
+			args: args{
+				conf: configWithHydra,
+				r:    requestFromChallenge(""),
+			},
+			want:      "",
+			assertErr: assert.Error,
+		},
 		{
 			name: "login_challenge is present; Hydra is not configured",
 			args: args{
diff --git a/persistence/sql/migratest/fixtures/login_flow/cccccccc-dda4-4700-9e42-35731f2af91e.json b/persistence/sql/migratest/fixtures/login_flow/cccccccc-dda4-4700-9e42-35731f2af91e.json
new file mode 100644
index 000000000000..438fb4005e14
--- /dev/null
+++ b/persistence/sql/migratest/fixtures/login_flow/cccccccc-dda4-4700-9e42-35731f2af91e.json
@@ -0,0 +1,17 @@
+{
+  "id": "cccccccc-dda4-4700-9e42-35731f2af91e",
+  "oauth2_login_challenge": "challenge data",
+  "type": "api",
+  "expires_at": "2013-10-07T08:23:19Z",
+  "issued_at": "2013-10-07T08:23:19Z",
+  "request_url": "http://kratos:4433/self-service/browser/flows/login",
+  "ui": {
+    "action": "",
+    "method": "",
+    "nodes": null
+  },
+  "created_at": "2013-10-07T08:23:19Z",
+  "updated_at": "2013-10-07T08:23:19Z",
+  "refresh": false,
+  "requested_aal": "aal1"
+}
diff --git a/persistence/sql/migratest/testdata/20230614205200_testdata.sql b/persistence/sql/migratest/testdata/20230614205200_testdata.sql
new file mode 100644
index 000000000000..c047c59a3115
--- /dev/null
+++ b/persistence/sql/migratest/testdata/20230614205200_testdata.sql
@@ -0,0 +1,8 @@
+INSERT INTO selfservice_login_flows (id, nid, request_url, issued_at, expires_at, active_method, csrf_token, created_at,
+                                     updated_at, forced, type, ui, internal_context, oauth2_login_challenge_data)
+VALUES ('cccccccc-dda4-4700-9e42-35731f2af91e',
+        '884f556e-eb3a-4b9f-bee3-11345642c6c0',
+        'http://kratos:4433/self-service/browser/flows/login',
+        '2013-10-07 08:23:19', '2013-10-07 08:23:19', '',
+        'fpeVSZ9ZH7YvUkhXsOVEIssxbfauh5lcoQSYxTcN0XkMneg1L42h+HtvisjlNjBF4ElcD2jApCHoJYq2u9sVWg==',
+        '2013-10-07 08:23:19', '2013-10-07 08:23:19', false, 'api', '{}', '{"foo":"bar"}', 'challenge data');
diff --git a/test/e2e/cypress/integration/profiles/oidc-provider/error.spec.ts b/test/e2e/cypress/integration/profiles/oidc-provider/error.spec.ts
index ec5a6321718f..39afd7a9c5c5 100644
--- a/test/e2e/cypress/integration/profiles/oidc-provider/error.spec.ts
+++ b/test/e2e/cypress/integration/profiles/oidc-provider/error.spec.ts
@@ -15,21 +15,7 @@ context("OpenID Provider", () => {
       cy.get(`[data-testid="ui/error/message"]`).then((c) => {
         cy.wrap(c[0].textContent).should(
           "contain",
-          "the login_challenge parameter is present but invalid or zero UUID",
-        )
-      })
-    })
-  })
-
-  it("should fail with zero login_challenge", () => {
-    cy.visit(
-      express.login + "?login_challenge=00000000-0000-0000-0000-000000000000",
-      { failOnStatusCode: false },
-    ).then((d) => {
-      cy.get(`[data-testid="ui/error/message"]`).then((c) => {
-        cy.wrap(c[0].textContent).should(
-          "contain",
-          "the login_challenge parameter is present but invalid or zero UUID",
+          "the login_challenge parameter is present but empty",
         )
       })
     })