From 1a5dbbf4307fa2a6175651012bb1577c9baeb036 Mon Sep 17 00:00:00 2001 From: Jarek Radosz Date: Tue, 7 Jun 2022 13:00:25 +0200 Subject: [PATCH] FIX: Correctly handle invalid auth cookies (#16995) Previously it would blow up on invalid utf byte sequences. This was a source of spec flakiness. --- lib/auth/default_current_user_provider.rb | 9 ++++++--- spec/lib/middleware/anonymous_cache_spec.rb | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index 9c3dd60d5e1..70087e5302e 100644 --- a/lib/auth/default_current_user_provider.rb +++ b/lib/auth/default_current_user_provider.rb @@ -77,8 +77,9 @@ class Auth::DefaultCurrentUserProvider ] def self.find_v0_auth_cookie(request) - cookie = request.cookies[TOKEN_COOKIE].presence - if cookie && cookie.size == TOKEN_SIZE + cookie = request.cookies[TOKEN_COOKIE] + + if cookie&.valid_encoding? && cookie.present? && cookie.size == TOKEN_SIZE cookie end end @@ -88,8 +89,10 @@ class Auth::DefaultCurrentUserProvider env[DECRYPTED_AUTH_COOKIE] = begin request = ActionDispatch::Request.new(env) + cookie = request.cookies[TOKEN_COOKIE] + # don't even initialize a cookie jar if we don't have a cookie at all - if request.cookies[TOKEN_COOKIE].present? + if cookie&.valid_encoding? && cookie.present? request.cookie_jar.encrypted[TOKEN_COOKIE]&.with_indifferent_access end end diff --git a/spec/lib/middleware/anonymous_cache_spec.rb b/spec/lib/middleware/anonymous_cache_spec.rb index 596732b81e0..977f90b3c68 100644 --- a/spec/lib/middleware/anonymous_cache_spec.rb +++ b/spec/lib/middleware/anonymous_cache_spec.rb @@ -29,6 +29,7 @@ describe Middleware::AnonymousCache do it "is true if it has an invalid auth cookie" do cookie = create_auth_cookie(token: SecureRandom.hex, issued_at: 5.minutes.ago) cookie = swap_2_different_characters(cookie) + cookie.prepend("%a0%a1") # an invalid byte sequence expect(new_helper("HTTP_COOKIE" => "jack=1; _t=#{cookie}; jill=2").cacheable?).to eq(true) end @@ -376,5 +377,4 @@ describe Middleware::AnonymousCache do expect(@status).to eq(403) end end - end