fix: don't assume the login challenge to be a UUID

For compatibility with ory/hydra#3515, which now encodes the whole flow in the login challenge, we cannot further assume that the challenge is a UUID.
ory · Jun 15, 2023 · a412d48 · a412d48
1 parent 84d14ed
commit a412d48
Show file tree

Hide file tree

Showing 5 changed files with 41 additions and 16 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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",
         )
       })
     })