diff --git a/actionpack/lib/action_controller/metal/request_forgery_protection.rb b/actionpack/lib/action_controller/metal/request_forgery_protection.rb index c5893e0a55..02f140d2f7 100644 --- a/actionpack/lib/action_controller/metal/request_forgery_protection.rb +++ b/actionpack/lib/action_controller/metal/request_forgery_protection.rb @@ -322,13 +322,10 @@ def masked_authenticity_token(session, form_options: {}) # :doc: action_path = normalize_action_path(action) per_form_csrf_token(session, action_path, method) else - real_csrf_token(session) + global_csrf_token(session) end - one_time_pad = SecureRandom.random_bytes(AUTHENTICITY_TOKEN_LENGTH) - encrypted_csrf_token = xor_byte_strings(one_time_pad, raw_token) - masked_token = one_time_pad + encrypted_csrf_token - Base64.urlsafe_encode64(masked_token, padding: false) + mask_token(raw_token) end # Checks the client's masked token to see if it matches the @@ -358,7 +355,8 @@ def valid_authenticity_token?(session, encoded_masked_token) # :doc: elsif masked_token.length == AUTHENTICITY_TOKEN_LENGTH * 2 csrf_token = unmask_token(masked_token) - compare_with_real_token(csrf_token, session) || + compare_with_global_token(csrf_token, session) || + compare_with_real_token(csrf_token, session) || valid_per_form_csrf_token?(csrf_token, session) else false # Token is malformed. @@ -373,10 +371,21 @@ def unmask_token(masked_token) # :doc: xor_byte_strings(one_time_pad, encrypted_csrf_token) end + def mask_token(raw_token) # :doc: + one_time_pad = SecureRandom.random_bytes(AUTHENTICITY_TOKEN_LENGTH) + encrypted_csrf_token = xor_byte_strings(one_time_pad, raw_token) + masked_token = one_time_pad + encrypted_csrf_token + Base64.urlsafe_encode64(masked_token, padding: false) + end + def compare_with_real_token(token, session) # :doc: ActiveSupport::SecurityUtils.fixed_length_secure_compare(token, real_csrf_token(session)) end + def compare_with_global_token(token, session) # :doc: + ActiveSupport::SecurityUtils.fixed_length_secure_compare(token, global_csrf_token(session)) + end + def valid_per_form_csrf_token?(token, session) # :doc: if per_form_csrf_tokens correct_token = per_form_csrf_token( @@ -397,10 +406,21 @@ def real_csrf_token(session) # :doc: end def per_form_csrf_token(session, action_path, method) # :doc: + csrf_token_hmac(session, [action_path, method.downcase].join("#")) + end + + GLOBAL_CSRF_TOKEN_IDENTIFIER = "!real_csrf_token" + private_constant :GLOBAL_CSRF_TOKEN_IDENTIFIER + + def global_csrf_token(session) # :doc: + csrf_token_hmac(session, GLOBAL_CSRF_TOKEN_IDENTIFIER) + end + + def csrf_token_hmac(session, identifier) # :doc: OpenSSL::HMAC.digest( OpenSSL::Digest::SHA256.new, real_csrf_token(session), - [action_path, method.downcase].join("#") + identifier ) end diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index 7389aef51e..a9aeb28256 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -364,6 +364,8 @@ def each_pair(&block) @parameters.each_pair do |key, value| yield [key, convert_hashes_to_parameters(key, value)] end + + self end alias_method :each, :each_pair @@ -374,6 +376,8 @@ def each_value(&block) @parameters.each_pair do |key, value| yield convert_hashes_to_parameters(key, value) end + + self end # Attribute that keeps track of converted arrays, if any, to avoid double diff --git a/actionpack/test/controller/parameters/accessors_test.rb b/actionpack/test/controller/parameters/accessors_test.rb index 764b2cc292..7e8312f419 100644 --- a/actionpack/test/controller/parameters/accessors_test.rb +++ b/actionpack/test/controller/parameters/accessors_test.rb @@ -19,6 +19,18 @@ class ParametersAccessorsTest < ActiveSupport::TestCase ) end + test "each returns self" do + assert_same @params, @params.each { |_| _ } + end + + test "each_pair returns self" do + assert_same @params, @params.each_pair { |_| _ } + end + + test "each_value returns self" do + assert_same @params, @params.each_value { |_| _ } + end + test "[] retains permitted status" do @params.permit! assert_predicate @params[:person], :permitted? diff --git a/actionpack/test/controller/request_forgery_protection_test.rb b/actionpack/test/controller/request_forgery_protection_test.rb index 89bbbeefbe..25cb9f24c5 100644 --- a/actionpack/test/controller/request_forgery_protection_test.rb +++ b/actionpack/test/controller/request_forgery_protection_test.rb @@ -953,6 +953,39 @@ def test_accepts_global_csrf_token assert_response :success end + def test_does_not_return_old_csrf_token + get :index + + token = @controller.send(:form_authenticity_token) + + unmasked_token = @controller.send(:unmask_token, Base64.urlsafe_decode64(token)) + + assert_not_equal @controller.send(:real_csrf_token, session), unmasked_token + end + + def test_returns_hmacd_token + get :index + + token = @controller.send(:form_authenticity_token) + + unmasked_token = @controller.send(:unmask_token, Base64.urlsafe_decode64(token)) + + assert_equal @controller.send(:global_csrf_token, session), unmasked_token + end + + def test_accepts_old_csrf_token + get :index + + non_hmac_token = @controller.send(:mask_token, @controller.send(:real_csrf_token, session)) + + # This is required because PATH_INFO isn't reset between requests. + @request.env["PATH_INFO"] = "/per_form_tokens/post_one" + assert_nothing_raised do + post :post_one, params: { custom_authenticity_token: non_hmac_token } + end + assert_response :success + end + def test_chomps_slashes get :index, params: { form_path: "/per_form_tokens/post_one?foo=bar" } diff --git a/actionview/app/assets/javascripts/rails-ujs/utils/ajax.coffee b/actionview/app/assets/javascripts/rails-ujs/utils/ajax.coffee index 624352f49c..645144022c 100644 --- a/actionview/app/assets/javascripts/rails-ujs/utils/ajax.coffee +++ b/actionview/app/assets/javascripts/rails-ujs/utils/ajax.coffee @@ -52,9 +52,10 @@ createXHR = (options, done) -> # Sending FormData will automatically set Content-Type to multipart/form-data if typeof options.data is 'string' xhr.setRequestHeader('Content-Type', 'application/x-www-form-urlencoded; charset=UTF-8') - xhr.setRequestHeader('X-Requested-With', 'XMLHttpRequest') unless options.crossDomain - # Add X-CSRF-Token - CSRFProtection(xhr) + unless options.crossDomain + xhr.setRequestHeader('X-Requested-With', 'XMLHttpRequest') + # Add X-CSRF-Token + CSRFProtection(xhr) xhr.withCredentials = !!options.withCredentials xhr.onreadystatechange = -> done(xhr) if xhr.readyState is XMLHttpRequest.DONE diff --git a/activestorage/lib/active_storage/service/s3_service.rb b/activestorage/lib/active_storage/service/s3_service.rb index fa2ab911cd..b4fba3d6ec 100644 --- a/activestorage/lib/active_storage/service/s3_service.rb +++ b/activestorage/lib/active_storage/service/s3_service.rb @@ -80,7 +80,8 @@ def exist?(key) def url_for_direct_upload(key, expires_in:, content_type:, content_length:, checksum:) instrument :url, key: key do |payload| generated_url = object_for(key).presigned_url :put, expires_in: expires_in.to_i, - content_type: content_type, content_length: content_length, content_md5: checksum, **upload_options + content_type: content_type, content_length: content_length, content_md5: checksum + whitelist_headers: ['content-length'], **upload_options payload[:url] = generated_url diff --git a/activestorage/test/service/s3_service_test.rb b/activestorage/test/service/s3_service_test.rb index a65fe42720..5ebfd16cc3 100644 --- a/activestorage/test/service/s3_service_test.rb +++ b/activestorage/test/service/s3_service_test.rb @@ -55,6 +55,29 @@ class ActiveStorage::Service::S3ServiceTest < ActiveSupport::TestCase @service.delete key end + test "directly uploading file larger than the provided content-length does not work" do + key = SecureRandom.base58(24) + data = "Some text that is longer than the specified content length" + checksum = Digest::MD5.base64digest(data) + url = @service.url_for_direct_upload(key, expires_in: 5.minutes, content_type: "text/plain", content_length: data.size - 1, checksum: checksum) + + uri = URI.parse url + request = Net::HTTP::Put.new uri.request_uri + request.body = data + request.add_field "Content-Type", "text/plain" + request.add_field "Content-MD5", checksum + upload_result = Net::HTTP.start(uri.host, uri.port, use_ssl: true) do |http| + http.request request + end + + assert_equal "403", upload_result.code + assert_raises ActiveStorage::FileNotFoundError do + @service.download(key) + end + ensure + @service.delete key + end + test "upload a zero byte file" do blob = directly_upload_file_blob filename: "empty_file.txt", content_type: nil user = User.create! name: "DHH", avatar: blob diff --git a/activesupport/lib/active_support/cache/mem_cache_store.rb b/activesupport/lib/active_support/cache/mem_cache_store.rb index bebe87b019..6b90d91ed3 100644 --- a/activesupport/lib/active_support/cache/mem_cache_store.rb +++ b/activesupport/lib/active_support/cache/mem_cache_store.rb @@ -8,7 +8,6 @@ end require "active_support/core_ext/enumerable" -require "active_support/core_ext/marshal" require "active_support/core_ext/array/extract_options" module ActiveSupport @@ -29,14 +28,6 @@ class MemCacheStore < Store # Provide support for raw values in the local cache strategy. module LocalCacheWithRaw # :nodoc: private - def read_entry(key, **options) - entry = super - if options[:raw] && local_cache && entry - entry = deserialize_entry(entry.value) - end - entry - end - def write_entry(key, entry, **options) if options[:raw] && local_cache raw_entry = Entry.new(entry.value.to_s) @@ -195,9 +186,8 @@ def normalize_key(key, options) key end - def deserialize_entry(raw_value) - if raw_value - entry = Marshal.load(raw_value) rescue raw_value + def deserialize_entry(entry) + if entry entry.is_a?(Entry) ? entry : Entry.new(entry) end end diff --git a/activesupport/lib/active_support/cache/redis_cache_store.rb b/activesupport/lib/active_support/cache/redis_cache_store.rb index a5b1e370c2..af8b4e017e 100644 --- a/activesupport/lib/active_support/cache/redis_cache_store.rb +++ b/activesupport/lib/active_support/cache/redis_cache_store.rb @@ -74,14 +74,6 @@ def self.supports_cache_versioning? # Support raw values in the local cache strategy. module LocalCacheWithRaw # :nodoc: private - def read_entry(key, **options) - entry = super - if options[:raw] && local_cache && entry - entry = deserialize_entry(entry.value) - end - entry - end - def write_entry(key, entry, **options) if options[:raw] && local_cache raw_entry = Entry.new(serialize_entry(entry, raw: true)) @@ -352,7 +344,8 @@ def set_redis_capabilities # Read an entry from the cache. def read_entry(key, **options) failsafe :read_entry do - deserialize_entry redis.with { |c| c.get(key) } + raw = options&.fetch(:raw, false) + deserialize_entry(redis.with { |c| c.get(key) }, raw: raw) end end @@ -368,6 +361,7 @@ def read_multi_mget(*names) options = names.extract_options! options = merged_options(options) return {} if names == [] + raw = options&.fetch(:raw, false) keys = names.map { |name| normalize_key(name, options) } @@ -377,7 +371,7 @@ def read_multi_mget(*names) names.zip(values).each_with_object({}) do |(name, value), results| if value - entry = deserialize_entry(value) + entry = deserialize_entry(value, raw: raw) unless entry.nil? || entry.expired? || entry.mismatched?(normalize_version(name, options)) results[name] = entry.value end @@ -457,10 +451,13 @@ def truncate_key(key) end end - def deserialize_entry(serialized_entry) + def deserialize_entry(serialized_entry, raw:) if serialized_entry - entry = Marshal.load(serialized_entry) rescue serialized_entry - entry.is_a?(Entry) ? entry : Entry.new(entry) + if raw + Entry.new(serialized_entry) + else + Marshal.load(serialized_entry) + end end end diff --git a/activesupport/test/cache/behaviors/cache_increment_decrement_behavior.rb b/activesupport/test/cache/behaviors/cache_increment_decrement_behavior.rb index 16b7abc679..5ffc08ea3a 100644 --- a/activesupport/test/cache/behaviors/cache_increment_decrement_behavior.rb +++ b/activesupport/test/cache/behaviors/cache_increment_decrement_behavior.rb @@ -3,11 +3,11 @@ module CacheIncrementDecrementBehavior def test_increment @cache.write("foo", 1, raw: true) - assert_equal 1, @cache.read("foo").to_i + assert_equal 1, @cache.read("foo", raw: true).to_i assert_equal 2, @cache.increment("foo") - assert_equal 2, @cache.read("foo").to_i + assert_equal 2, @cache.read("foo", raw: true).to_i assert_equal 3, @cache.increment("foo") - assert_equal 3, @cache.read("foo").to_i + assert_equal 3, @cache.read("foo", raw: true).to_i missing = @cache.increment("bar") assert(missing.nil? || missing == 1) @@ -15,11 +15,11 @@ def test_increment def test_decrement @cache.write("foo", 3, raw: true) - assert_equal 3, @cache.read("foo").to_i + assert_equal 3, @cache.read("foo", raw: true).to_i assert_equal 2, @cache.decrement("foo") - assert_equal 2, @cache.read("foo").to_i + assert_equal 2, @cache.read("foo", raw: true).to_i assert_equal 1, @cache.decrement("foo") - assert_equal 1, @cache.read("foo").to_i + assert_equal 1, @cache.read("foo", raw: true).to_i missing = @cache.decrement("bar") assert(missing.nil? || missing == -1) diff --git a/activesupport/test/cache/behaviors/cache_store_behavior.rb b/activesupport/test/cache/behaviors/cache_store_behavior.rb index 9f40b0cc34..05ca9fad07 100644 --- a/activesupport/test/cache/behaviors/cache_store_behavior.rb +++ b/activesupport/test/cache/behaviors/cache_store_behavior.rb @@ -472,8 +472,8 @@ def test_race_condition_protection def test_crazy_key_characters crazy_key = "#/:*(<+=> )&$%@?;'\"\'`~-" assert @cache.write(crazy_key, "1", raw: true) - assert_equal "1", @cache.read(crazy_key) - assert_equal "1", @cache.fetch(crazy_key) + assert_equal "1", @cache.read(crazy_key, raw: true) + assert_equal "1", @cache.fetch(crazy_key, raw: true) assert @cache.delete(crazy_key) assert_equal "2", @cache.fetch(crazy_key, raw: true) { "2" } assert_equal 3, @cache.increment(crazy_key) @@ -497,7 +497,7 @@ def test_cache_hit_instrumentation @events << ActiveSupport::Notifications::Event.new(*args) end assert @cache.write(key, "1", raw: true) - assert @cache.fetch(key) { } + assert @cache.fetch(key, raw: true) { } assert_equal 1, @events.length assert_equal "cache_read.active_support", @events[0].name assert_equal :fetch, @events[0].payload[:super_operation] diff --git a/activesupport/test/cache/behaviors/encoded_key_cache_behavior.rb b/activesupport/test/cache/behaviors/encoded_key_cache_behavior.rb index 842400f4a3..2304ba94c0 100644 --- a/activesupport/test/cache/behaviors/encoded_key_cache_behavior.rb +++ b/activesupport/test/cache/behaviors/encoded_key_cache_behavior.rb @@ -8,8 +8,8 @@ module EncodedKeyCacheBehavior define_method "test_#{encoding.name.underscore}_encoded_values" do key = (+"foo").force_encoding(encoding) assert @cache.write(key, "1", raw: true) - assert_equal "1", @cache.read(key) - assert_equal "1", @cache.fetch(key) + assert_equal "1", @cache.read(key, raw: true) + assert_equal "1", @cache.fetch(key, raw: true) assert @cache.delete(key) assert_equal "2", @cache.fetch(key, raw: true) { "2" } assert_equal 3, @cache.increment(key) @@ -20,8 +20,8 @@ module EncodedKeyCacheBehavior def test_common_utf8_values key = (+"\xC3\xBCmlaut").force_encoding(Encoding::UTF_8) assert @cache.write(key, "1", raw: true) - assert_equal "1", @cache.read(key) - assert_equal "1", @cache.fetch(key) + assert_equal "1", @cache.read(key, raw: true) + assert_equal "1", @cache.fetch(key, raw: true) assert @cache.delete(key) assert_equal "2", @cache.fetch(key, raw: true) { "2" } assert_equal 3, @cache.increment(key) diff --git a/activesupport/test/cache/behaviors/local_cache_behavior.rb b/activesupport/test/cache/behaviors/local_cache_behavior.rb index e4f88de368..1a847ac711 100644 --- a/activesupport/test/cache/behaviors/local_cache_behavior.rb +++ b/activesupport/test/cache/behaviors/local_cache_behavior.rb @@ -115,7 +115,7 @@ def test_local_cache_of_increment @cache.write("foo", 1, raw: true) @peek.write("foo", 2, raw: true) @cache.increment("foo") - assert_equal 3, @cache.read("foo") + assert_equal 3, @cache.read("foo", raw: true) end end @@ -124,7 +124,7 @@ def test_local_cache_of_decrement @cache.write("foo", 1, raw: true) @peek.write("foo", 3, raw: true) @cache.decrement("foo") - assert_equal 2, @cache.read("foo") + assert_equal 2, @cache.read("foo", raw: true) end end @@ -142,9 +142,9 @@ def test_local_cache_of_read_multi @cache.with_local_cache do @cache.write("foo", "foo", raw: true) @cache.write("bar", "bar", raw: true) - values = @cache.read_multi("foo", "bar") - assert_equal "foo", @cache.read("foo") - assert_equal "bar", @cache.read("bar") + values = @cache.read_multi("foo", "bar", raw: true) + assert_equal "foo", @cache.read("foo", raw: true) + assert_equal "bar", @cache.read("bar", raw: true) assert_equal "foo", values["foo"] assert_equal "bar", values["bar"] end diff --git a/activesupport/test/cache/stores/mem_cache_store_test.rb b/activesupport/test/cache/stores/mem_cache_store_test.rb index f578714cdd..96d2680f54 100644 --- a/activesupport/test/cache/stores/mem_cache_store_test.rb +++ b/activesupport/test/cache/stores/mem_cache_store_test.rb @@ -79,7 +79,7 @@ def test_raw_values def test_raw_values_with_marshal cache = lookup_store(raw: true) cache.write("foo", Marshal.dump([])) - assert_equal [], cache.read("foo") + assert_equal Marshal.dump([]), cache.read("foo") end def test_local_cache_raw_values @@ -108,7 +108,7 @@ def test_local_cache_raw_values_with_marshal cache = lookup_store(raw: true) cache.with_local_cache do cache.write("foo", Marshal.dump([])) - assert_equal [], cache.read("foo") + assert_equal Marshal.dump([]), cache.read("foo") end end diff --git a/activesupport/test/cache/stores/redis_cache_store_test.rb b/activesupport/test/cache/stores/redis_cache_store_test.rb index 2fa5050497..b8d8cf76b5 100644 --- a/activesupport/test/cache/stores/redis_cache_store_test.rb +++ b/activesupport/test/cache/stores/redis_cache_store_test.rb @@ -17,6 +17,7 @@ class SlowRedis < Redis def get(key) if /latency/.match?(key) sleep 3 + super else super end