From e5de522ed0b01a2806423dbe2bdddbd5c44e673e Mon Sep 17 00:00:00 2001 From: Antti Hukkanen Date: Thu, 7 Nov 2024 20:16:50 +0200 Subject: [PATCH] Add a test case for the issue detected with cookie overflow The test ensures that the ID token is not stored within the cookies as that caused the cookie overflow. --- .../helsinki_profile/test/oidc_server.rb | 36 ++++++++++++--- .../omniauth_callbacks_controller_spec.rb | 44 ++++++++++++++++--- spec/spec_helper.rb | 3 +- 3 files changed, 69 insertions(+), 14 deletions(-) diff --git a/lib/decidim/helsinki_profile/test/oidc_server.rb b/lib/decidim/helsinki_profile/test/oidc_server.rb index 51447ec..6f68f37 100644 --- a/lib/decidim/helsinki_profile/test/oidc_server.rb +++ b/lib/decidim/helsinki_profile/test/oidc_server.rb @@ -118,18 +118,42 @@ def jwt(payload = {}) ) end - def token(payload = {}) + def token(payload = {}, id_token_data = nil) # We need to issue the same kid for the JSON::JWK key as the server keys # that are generated through JWT::JWK. We use JSON::JWK in the specs to # control better the signing of the keys. kid = JSON::JWK.new(jwk_rsa_keys.first, kid: jwks.first.kid) - jwt = jwt(payload).sign(kid) + access_token = jwt(payload).sign(kid) + id_token = + if id_token_data.is_a?(Hash) + JSON::JWT.new( + # https://openid.net/specs/openid-connect-core-1_0.html#IDToken + { + iss: access_token[:iss], + sub: access_token[:sub], + aud: access_token[:aud], + exp: access_token[:exp], + iat: access_token[:iat], + **id_token_data + } + ).sign(kid) + end expires_at = 30.minutes.from_now - Rack::OAuth2::AccessToken::Bearer.new( - access_token: jwt.to_s, - expires_in: (expires_at - Time.now.utc).to_i - ) + token_attrs = { + client: "client", # required by OpenIDConnect::AccessToken + access_token: access_token.to_s, + expires_in: (expires_at - Time.now.utc).to_i, + id_token: id_token&.to_s + } + # Use OpenIDConnect::AccessToken instead of + # Rack::OAuth2::AccessToken::Bearer in order to use the `id_token` + # attribute. + OpenIDConnect::AccessToken.new(**token_attrs.compact).tap do |token| + [:client, :raw_attributes].each do |attr| + token.send(:remove_instance_variable, :"@#{attr}") + end + end end def userinfo(authorization) diff --git a/spec/controllers/decidim/helsinki_profile/omniauth_callbacks_controller_spec.rb b/spec/controllers/decidim/helsinki_profile/omniauth_callbacks_controller_spec.rb index a17d0c7..4998026 100644 --- a/spec/controllers/decidim/helsinki_profile/omniauth_callbacks_controller_spec.rb +++ b/spec/controllers/decidim/helsinki_profile/omniauth_callbacks_controller_spec.rb @@ -27,14 +27,14 @@ { env: { "rack.session" => request.session, - "rack.session.options" => request.session.options, - "omniauth-helsinki.id_token" => id_token + "rack.session.options" => request.session.options } } end - let(:id_token) { "omniauth_id_token" } let(:omniauth_state) { request.session["omniauth.state"] } + let(:omniauth_nonce) { request.session["omniauth.nonce"] } let(:code) { SecureRandom.hex(16) } + let(:id_token_payload) { { nonce: omniauth_nonce, groups: ["testing"] } } before do profile_api.register_profile(profile) @@ -86,7 +86,9 @@ info = Decidim::HelsinkiProfile::SessionInfo.find_by(user: Decidim::User.last) expect(info).to be_a(Decidim::HelsinkiProfile::SessionInfo) - expect(info.id_token).to eq(id_token) + + decoded = JSON::JWT.decode(info.id_token, :skip_verification).to_h + expect(decoded.symbolize_keys!).to include(id_token_payload) end context "when the session has a pending redirect" do @@ -99,6 +101,24 @@ expect(response).to redirect_to("/processes") end end + + # This spec is just to ensure that the ID token is not stored inside a + # cookie as that would cause a cookie overflow exception + # (ActionDispatch::Cookies::CookieOverflow) in case it exceeds 4kB in + # size. + context "when the id token is very long" do + # Generate a payload of at least 4kB. + let(:id_token_payload) do + { nonce: omniauth_nonce, groups: 4.times.map { |num| (97 + num).chr * 1024 } } + end + + it "does not cause an exception" do + user = Decidim::User.last + + expect(user.sign_in_count).to eq(1) + expect(response).to redirect_to("/") + end + end end context "when storing the email address" do @@ -182,9 +202,10 @@ expect(Decidim::HelsinkiProfile::SessionInfo.find_by(id: previous_info.id)).to be_nil info = Decidim::HelsinkiProfile::SessionInfo.find_by(user: Decidim::User.last) - expect(info).to be_a(Decidim::HelsinkiProfile::SessionInfo) - expect(info.id_token).to eq(id_token) + + decoded = JSON::JWT.decode(info.id_token, :skip_verification).to_h + expect(decoded.symbolize_keys!).to include(id_token_payload) end it "redirects to the root path" do @@ -239,7 +260,16 @@ name: "helsinki_idp" ) expect(authorization).to be_nil - expect(response).to redirect_to("/users/auth/helsinki/logout?id_token_hint=#{id_token}") + + info = Decidim::HelsinkiProfile::SessionInfo.find_by(user: confirmed_user) + expect(info).to be_nil + + redirect_uri = URI.parse(response.location) + redirect_params = Rack::Utils.parse_query(redirect_uri.query).symbolize_keys! + + expect(redirect_uri.host).to eq(organization.host) + expect(redirect_uri.path).to eq("/users/auth/helsinki/logout") + expect(redirect_params).to match(id_token_hint: /\A[^\s]+\z/) expect(flash[:alert]).to eq( "Another user has already been identified using this identity. Please sign out and sign in again directly using Helsinki profile." ) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index ffc4e8a..a829910 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -107,10 +107,11 @@ scope: form_data["scope"] } token_payload[:sub] = token_sub if respond_to?(:token_sub) && token_sub + idt_payload = id_token_payload if respond_to?(:id_token_payload) { headers: { "Content-Type" => "application/json" }, - body: auth_server.token(token_payload).to_json + body: auth_server.token(token_payload, idt_payload).to_json } end end