fix: admin recovery CSRF & duplicate form elements (#2846)

ory · Oct 30, 2022 · de80b7f · de80b7f
1 parent 411cd79
commit de80b7f
Show file tree

Hide file tree

Showing 3 changed files with 63 additions and 7 deletions.
diff --git a/...stAdminStrategy-case=form_should_not_contain_email_field_when_creating_recovery_code.json b/...stAdminStrategy-case=form_should_not_contain_email_field_when_creating_recovery_code.json
@@ -0,0 +1,40 @@
+[
+  {
+    "type": "input",
+    "group": "code",
+    "attributes": {
+      "name": "code",
+      "type": "number",
+      "required": true,
+      "disabled": false,
+      "node_type": "input"
+    },
+    "messages": [],
+    "meta": {
+      "label": {
+        "id": 1070006,
+        "text": "Verify code",
+        "type": "info"
+      }
+    }
+  },
+  {
+    "type": "input",
+    "group": "code",
+    "attributes": {
+      "name": "method",
+      "type": "submit",
+      "value": "code",
+      "disabled": false,
+      "node_type": "input"
+    },
+    "messages": [],
+    "meta": {
+      "label": {
+        "id": 1070005,
+        "text": "Submit",
+        "type": "info"
+      }
+    }
+  }
+]
diff --git a/selfservice/strategy/code/strategy_recovery.go b/selfservice/strategy/code/strategy_recovery.go
@@ -168,6 +168,7 @@ func (s *Strategy) createRecoveryCode(w http.ResponseWriter, r *http.Request, _
 	}
 	flow.DangerousSkipCSRFCheck = true
 	flow.State = recovery.StateEmailSent
+	flow.UI.Nodes = node.Nodes{}
 	flow.UI.Nodes.Append(node.NewInputField("code", nil, node.CodeGroup, node.InputAttributeTypeNumber, node.WithRequiredInputAttribute).
 		WithMetaLabel(text.NewInfoNodeLabelVerifyOTP()),
 	)
@@ -270,9 +271,13 @@ func (s *Strategy) Recover(w http.ResponseWriter, r *http.Request, f *recovery.F
 	}
 	ctx := r.Context()
 
-	// If a CSRF violation occurs the flow is most likely FUBAR, as the user either lost the CSRF token, or an attack occured.
-	// In this case, we just issue a new flow and "abandon" the old flow.
-	if err := flow.EnsureCSRF(s.deps, r, f.Type, s.deps.Config().DisableAPIFlowEnforcement(ctx), s.deps.GenerateCSRFToken, body.CSRFToken); err != nil {
+	if f.DangerousSkipCSRFCheck {
+		s.deps.Logger().
+			WithRequest(r).
+			Debugf("A recovery flow with `DangerousSkipCSRFCheck` set has been submitted, skipping anti-CSRF measures.")
+	} else if err := flow.EnsureCSRF(s.deps, r, f.Type, s.deps.Config().DisableAPIFlowEnforcement(ctx), s.deps.GenerateCSRFToken, body.CSRFToken); err != nil {
+		// If a CSRF violation occurs the flow is most likely FUBAR, as the user either lost the CSRF token, or an attack occured.
+		// In this case, we just issue a new flow and "abandon" the old flow.
 		return s.retryRecoveryFlowWithError(w, r, flow.TypeBrowser, err)
 	}
 

diff --git a/selfservice/strategy/code/strategy_recovery_test.go b/selfservice/strategy/code/strategy_recovery_test.go
@@ -134,12 +134,9 @@ func TestAdminStrategy(t *testing.T) {
 
 		action := gjson.GetBytes(body, "ui.action").String()
 		require.NotEmpty(t, action)
-		csrfToken := gjson.GetBytes(body, "ui.nodes.#(attributes.name==csrf_token).attributes.value").String()
-		require.NotEmpty(t, csrfToken)
 
 		res, err = publicTS.Client().PostForm(action, url.Values{
-			"csrf_token": {csrfToken},
-			"code":       {code},
+			"code": {code},
 		})
 		assert.Equal(t, http.StatusOK, res.StatusCode)
 
@@ -229,6 +226,20 @@ func TestAdminStrategy(t *testing.T) {
 
 		assertMessage(t, body, "The recovery code is invalid or has already been used. Please try again.")
 	})
+
+	t.Run("case=form should not contain email field when creating recovery code", func(t *testing.T) {
+		email := strings.ToLower(testhelpers.RandomEmail())
+		i := createIdentityToRecover(t, reg, email)
+
+		c1, _, err := createCode(i.ID.String(), pointerx.String("1h"))
+		require.NoError(t, err)
+
+		res, err := http.Get(c1.RecoveryLink)
+		require.NoError(t, err)
+		body := ioutilx.MustReadAll(res.Body)
+
+		snapshotx.SnapshotT(t, json.RawMessage(gjson.GetBytes(body, "ui.nodes").String()))
+	})
 }
 
 const (