fix: keep the CSRF secret for the session

The dead-letter list renders one Retry form per row, and a browser keeps
pages open in other tabs. Rotating the session secret on the first
submission answered 403 to every other form the same page had already
rendered, so retrying a second dead letter appeared to be forbidden
until the operator reloaded.

Single use is not what a CSRF token provides. It proves the request came
from a page this session was served; the per-request mask is what keeps
the value on the wire from repeating, which is the property rotation
looked like it was adding.

The mount check now forges a token of the right length so it reaches the
comparison rather than being turned away by the length check, which is
what a wrong-but-well-formed token would do.

Author: cardmagic

lib/solid_objects/web/csrf_protection.rb

@@ -64,9 +64,13 @@ def valid?(env, given)
         token = decode(given)
         return false unless token
 
-        # The session token is replaced whether or not this comparison
-        # succeeds, so a token cannot be replayed after it is spent.
-        session[:csrf] = SecureRandom.base64(TOKEN_BYTES)
+        # The secret is not rotated here. A page renders one Retry form per
+        # dead letter, and a browser keeps pages open in other tabs, so
+        # spending the secret on the first submission would answer 403 to
+        # every other form already rendered. Single use is not what a CSRF
+        # token provides: it proves the request came from a page this session
+        # was served, and the per-request mask below is what keeps the value
+        # on the wire from repeating.
         matches?(token, stored)
       end
 

test/dummy/web_mount_check.rb

@@ -50,7 +50,9 @@ def pause(request, path, cookie, token)
   request.post("#{path}/pause", options)
 end
 
-forged = pause(request, detail_path, cookie, "forged")
+# Well formed and the right length, so it reaches the comparison rather than
+# being turned away by the length check on the way in.
+forged = pause(request, detail_path, cookie, SecureRandom.base64(32))
 forged_paused = instance.reload.paused_at
 
 paused = pause(request, detail_path, cookie, token)

test/integration/web_test.rb

@@ -157,6 +157,45 @@ class WebTest < WebTestCase
     assert_nil instance.reload.paused_at
   end
 
+  test "rejects a token that was not derived from this session" do
+    instance = create_instance
+    get("/")
+
+    response = request(
+      "/instances/#{instance.id}/pause",
+      method: "POST",
+      params: { "authenticity_token" => SecureRandom.base64(32) }
+    )
+
+    assert_equal 403, response.status
+    assert_nil instance.reload.paused_at
+  end
+
+  # The dead-letter list renders one Retry form per row, and a browser keeps
+  # pages open in other tabs. Spending the session secret on the first
+  # submission would answer 403 to every other form the same page rendered.
+  test "accepts every form a page rendered, not only the first" do
+    instance = create_instance
+    token = rendered_token("/instances/#{instance.id}")
+
+    paused = submit("/instances/#{instance.id}/pause", token)
+    resumed = submit("/instances/#{instance.id}/resume", token)
+
+    assert_equal 302, paused.status
+    assert_equal 302, resumed.status, "a second form from the same page must still be accepted"
+    assert_nil instance.reload.paused_at
+  end
+
+  test "issues a differently masked token on each request" do
+    instance = create_instance
+    first = rendered_token("/instances/#{instance.id}")
+    second = rendered_token("/instances/#{instance.id}")
+
+    refute_equal first, second, "an unchanging token on the wire is what masking prevents"
+    assert_equal 302, submit("/instances/#{instance.id}/pause", first).status
+    assert_equal 302, submit("/instances/#{instance.id}/resume", second).status
+  end
+
   test "lists ready and claimed mailbox messages" do
     instance = create_instance
     mark_ready(create_message(instance, operation: "increment"))

test/web_test_helper.rb

@@ -45,6 +45,18 @@ def post_without_csrf_token(path, params = {})
     request(path, method: "POST", params:)
   end
 
+  # The masked token a page actually put in its forms, rather than the session
+  # value behind it.
+  # @rbs (String) -> String
+  def rendered_token(path)
+    get(path).body[/name="authenticity_token" value="([^"]+)"/, 1]
+  end
+
+  # @rbs (String, String, ?Hash[String, untyped]) -> Rack::MockResponse
+  def submit(path, token, params = {})
+    request(path, method: "POST", params: params.merge("authenticity_token" => token))
+  end
+
   # @rbs (String, method: String, params: Hash[String, untyped]) -> Rack::MockResponse
   def request(path, method:, params:)
     environment = Rack::MockRequest.env_for(