Add a ctor to AuthenticityToken and change class methods to instance

Do all the session manipulation in the constructor, and just operate
on a copy of the CSRF token in the instance methods. Slightly
simplifies the controller mixin code and makes the API read a bit
better.

actionpack/lib/action_controller/authenticity_token.rb

@@ -1,54 +1,66 @@
 module ActionController
   class AuthenticityToken
-    class << self
-      LENGTH = 32
-
-      def generate_masked(session)
-        one_time_pad = SecureRandom.random_bytes(LENGTH)
-        encrypted_csrf_token = xor_byte_strings(one_time_pad, master_csrf_token(session))
-        masked_token = one_time_pad + encrypted_csrf_token
-        Base64.strict_encode64(masked_token)
-      end
+    LENGTH = 32
+
+    # Note that this will modify +session+ as a side-effect if there is
+    # not a master CSRF token already present
+    def initialize(session, logger = nil)
+      session[:_csrf_token] ||= SecureRandom.base64(LENGTH)
+      @master_csrf_token = Base64.strict_decode64(session[:_csrf_token])
+      @logger = logger
+    end
 
-      def valid?(session, encoded_masked_token, logger = nil)
-        return false if encoded_masked_token.nil?
+    def generate_masked
+      # Start with some random bits
+      masked_token = SecureRandom.random_bytes(LENGTH)
 
-        masked_token = Base64.strict_decode64(encoded_masked_token)
+      raise if masked_token.length != 32
 
-        # See if it's actually a masked token or not. In order to
-        # deploy this code, we should be able to handle any unmasked
-        # tokens that we've issued without error.
-        if masked_token.length == LENGTH
-          # This is actually an unmasked token
-          if logger
-            logger.warn "The client is using an unmasked CSRF token. This " +
-              "should only happen immediately after you upgrade to masked " +
-              "tokens; if this persists, something is wrong."
-          end
+      # XOR the random bits with the real token and concatenate them
+      encrypted_csrf_token = self.class.xor_byte_strings(masked_token, @master_csrf_token)
+      masked_token.concat(encrypted_csrf_token)
 
-          masked_token == master_csrf_token(session)
+      Base64.strict_encode64(masked_token)
+    end
+
+    def valid?(encoded_masked_token)
+      return false unless encoded_masked_token
 
-        elsif masked_token.length == LENGTH * 2
-          # Split the token into the one-time pad and the encrypted
-          # value and decrypt it
-          one_time_pad = masked_token[0...LENGTH]
-          encrypted_csrf_token = masked_token[LENGTH..-1]
-          csrf_token = xor_byte_strings(one_time_pad, encrypted_csrf_token)
+      masked_token = Base64.strict_decode64(encoded_masked_token)
 
-          csrf_token == master_csrf_token(session)
+      # See if it's actually a masked token or not. In order to
+      # deploy this code, we should be able to handle any unmasked
+      # tokens that we've issued without error.
+      if masked_token.length == LENGTH
+        # This is actually an unmasked token
+        if @logger
+          @logger.warn "The client is using an unmasked CSRF token. This " +
+            "should only happen immediately after you upgrade to masked " +
+            "tokens; if this persists, something is wrong."
         end
-      end
 
-      private
+        masked_token == @master_csrf_token
 
-      def xor_byte_strings(s1, s2)
-        s1.bytes.zip(s2.bytes).map { |(c1,c2)| c1 ^ c2 }.pack('c*')
-      end
+      elsif masked_token.length == LENGTH * 2
+        # Split the token into the one-time pad and the encrypted
+        # value and decrypt it
+        one_time_pad = masked_token.first(LENGTH)
+        encrypted_csrf_token = masked_token.last(LENGTH)
+        csrf_token = self.class.xor_byte_strings(one_time_pad, encrypted_csrf_token)
+
+        csrf_token == @master_csrf_token
+
+      else
+        # Malformed token of some strange length
+        false
 
-      def master_csrf_token(session)
-        session[:_csrf_token] ||= SecureRandom.base64(LENGTH)
-        Base64.strict_decode64(session[:_csrf_token])
       end
     end
+
+    def self.xor_byte_strings(s1, s2)
+      raise "#{s1.length} #{s2.length}" if s1.length != s2.length
+
+      s1.bytes.zip(s2.bytes).map! { |c1, c2| c1 ^ c2 }.pack('c*')
+    end
   end
 end

actionpack/lib/action_controller/metal/request_forgery_protection.rb

@@ -185,13 +185,13 @@ def verify_authenticity_token
       # * Does the X-CSRF-Token header match the form_authenticity_token
       def verified_request?
         !protect_against_forgery? || request.get? || request.head? ||
-          AuthenticityToken.valid?(session, params[request_forgery_protection_token], logger) ||
-          AuthenticityToken.valid?(session, request.headers['X-CSRF-Token'], logger)
+          authenticity_token.valid?(params[request_forgery_protection_token]) ||
+          authenticity_token.valid?(request.headers['X-CSRF-Token'])
       end
 
       # Sets the token value for the current session.
       def form_authenticity_token
-        AuthenticityToken.generate_masked(session)
+        authenticity_token.generate_masked
       end
 
       # The form's authenticity parameter. Override to provide your own.
@@ -203,5 +203,9 @@ def form_authenticity_param
       def protect_against_forgery?
         allow_forgery_protection
       end
+
+      def authenticity_token
+        AuthenticityToken.new(session, logger)
+      end
   end
 end

actionpack/test/controller/authenticity_token_test.rb

@@ -2,102 +2,104 @@
 require 'active_support/log_subscriber/test_helper'
 
 class AuthenticityTokenTest < ActiveSupport::TestCase
-  test 'should generate a master token that is random' do
+  test 'should generate a master token that is random as a side-effect' do
     first_session = {}
-    ActionController::AuthenticityToken.generate_masked(first_session)
+    ActionController::AuthenticityToken.new(first_session)
 
     second_session = {}
-    ActionController::AuthenticityToken.generate_masked(second_session)
+    ActionController::AuthenticityToken.new(second_session)
 
     refute_equal first_session[:_csrf_token], second_session[:_csrf_token]
   end
 
   test 'should generate a master token that is a 32-byte base64 string' do
     session = {}
-    ActionController::AuthenticityToken.generate_masked(session)
+    ActionController::AuthenticityToken.new(session)
     bytes = Base64.strict_decode64(session[:_csrf_token])
     assert_equal 32, bytes.length
   end
 
   test 'should generate masked tokens that are 64-byte base64 strings' do
-    masked_token = ActionController::AuthenticityToken.generate_masked({})
+    masked_token = ActionController::AuthenticityToken.new({}).generate_masked
     bytes = Base64.strict_decode64(masked_token)
     assert_equal 64, bytes.length
   end
 
   test 'should save a new master token to the session if none is present' do
     session = {}
-    ActionController::AuthenticityToken.generate_masked(session)
+    ActionController::AuthenticityToken.new(session)
     refute_nil session[:_csrf_token]
   end
 
   test 'should not overwrite an existing master token' do
     existing = SecureRandom.base64(32)
     session = {:_csrf_token => existing}
-    ActionController::AuthenticityToken.generate_masked(session)
+    ActionController::AuthenticityToken.new(session)
     assert_equal existing, session[:_csrf_token]
   end
 
   test 'should generate masked tokens that are different each time' do
     session = {}
-    first = ActionController::AuthenticityToken.generate_masked(session)
-    second = ActionController::AuthenticityToken.generate_masked(session)
+    first = ActionController::AuthenticityToken.new(session).generate_masked
+    second = ActionController::AuthenticityToken.new(session).generate_masked
     refute_equal first, second
   end
 
   test 'should be able to verify a masked token' do
     session = {}
-    masked_token = ActionController::AuthenticityToken.generate_masked(session)
-    assert ActionController::AuthenticityToken.valid?(session, masked_token)
+    token = ActionController::AuthenticityToken.new(session)
+    masked_token = token.generate_masked
+    assert token.valid?(masked_token)
   end
 
   test 'should be able to verify an unmasked (master) token' do
     # Generate a master token
     session = {}
-    ActionController::AuthenticityToken.generate_masked(session)
-    assert ActionController::AuthenticityToken.valid?(session, session[:_csrf_token])
+    token = ActionController::AuthenticityToken.new(session)
+    assert token.valid?(session[:_csrf_token])
   end
 
   test 'should warn when verifying an unmasked token' do
     logger = ActiveSupport::LogSubscriber::TestHelper::MockLogger.new
 
     session = {}
-    ActionController::AuthenticityToken.generate_masked(session)
-    ActionController::AuthenticityToken.valid?(session, session[:_csrf_token], logger)
+    token = ActionController::AuthenticityToken.new(session, logger)
+    token.valid?(session[:_csrf_token])
 
     assert_equal 1, logger.logged(:warn).size
     assert_match(/unmasked CSRF token/, logger.logged(:warn).last)
   end
 
   test 'should reject an invalid unmasked token' do
     session = {}
-    ActionController::AuthenticityToken.generate_masked(session)
-    refute ActionController::AuthenticityToken.valid?(session, SecureRandom.base64(32))
+    token = ActionController::AuthenticityToken.new(session)
+    refute token.valid?(SecureRandom.base64(32))
   end
 
   test 'should reject an invalid masked token' do
     session = {}
-    ActionController::AuthenticityToken.generate_masked(session)
-    refute ActionController::AuthenticityToken.valid?(session, SecureRandom.base64(64))
+    token = ActionController::AuthenticityToken.new(session)
+    refute token.valid?(SecureRandom.base64(64))
   end
 
   test 'should reject a token from a different session' do
     old_session = {}
-    old_masked_token = ActionController::AuthenticityToken.generate_masked(old_session)
+    old_masked_token = ActionController::AuthenticityToken.new(old_session).generate_masked
 
     new_session = {}
-    refute ActionController::AuthenticityToken.valid?(new_session, old_masked_token)
+    new_token = ActionController::AuthenticityToken.new(new_session)
+    refute new_token.valid?(old_masked_token)
   end
 
   test 'should reject a nil token' do
-    refute ActionController::AuthenticityToken.valid?({}, nil)
+    refute ActionController::AuthenticityToken.new({}).valid?(nil)
   end
 
   test 'should reject an empty token' do
-    refute ActionController::AuthenticityToken.valid?({}, '')
+    refute ActionController::AuthenticityToken.new({}).valid?('')
   end
 
   test 'should reject a malformed token' do
-    refute ActionController::AuthenticityToken.valid?({}, SecureRandom.base64(42))
+    refute ActionController::AuthenticityToken.new({}).valid?(SecureRandom.base64(42))
   end
 end

actionpack/test/controller/request_forgery_protection_test.rb

@@ -101,11 +101,17 @@ def form_authenticity_param
 # common test methods
 module RequestForgeryProtectionTests
   def setup
-    # Pin the one-time pad to a fixed value to get predictable CSRF tokens
+    # Pin the RNG to a fixed value to get predictable CSRF tokens
+    # HACK Seed in the same value many times b/c the caller is going
+    # to modify it.
     one_time_pad = SecureRandom.random_bytes(32)
-    SecureRandom.stubs(:random_bytes).returns(one_time_pad)
+    SecureRandom.stubs(:random_bytes)
+      .returns(one_time_pad.dup)
+      .then.returns(one_time_pad.dup)
+      .then.returns(one_time_pad.dup)
+      .then.returns(one_time_pad.dup)
 
-    @token = ActionController::AuthenticityToken.generate_masked(session)
+    @token = ActionController::AuthenticityToken.new(session).generate_masked
 
     ActionController::Base.request_forgery_protection_token = :custom_authenticity_token
   end