SECURITY: Clear webauthn challenge from session after authenticating

lib/discourse_webauthn.rb

@@ -8,6 +8,7 @@ module DiscourseWebauthn
   # -257 - RS256 (Windows Hello supported alg.)
   SUPPORTED_ALGORITHMS = COSE::Algorithm.registered_algorithm_ids.freeze
   VALID_ATTESTATION_FORMATS = %w[none packed fido-u2f].freeze
+  CHALLENGE_EXPIRY = 5.minutes
 
   class SecurityKeyError < StandardError
   end
@@ -64,16 +65,32 @@ module DiscourseWebauthn
   # are challenging the user that has a security key, and
   # they must respond with a valid webauthn response and
   # credentials.
+  #
+  # @param user [User] the user to stage the challenge for
+  # @param secure_session [SecureSession] the session to store the challenge in
   def self.stage_challenge(user, secure_session)
-    ::DiscourseWebauthn::ChallengeGenerator.generate.commit_to_session(secure_session, user)
+    ::DiscourseWebauthn::ChallengeGenerator.generate.commit_to_session(
+      secure_session,
+      user,
+      expires: CHALLENGE_EXPIRY,
+    )
+  end
+
+  ##
+  # Clears the challenge from the user's secure session.
+  #
+  # @param user [User] the user to clear the challenge for
+  # @param secure_session [SecureSession] the session to clear the challenge from
+  def self.clear_challenge(user, secure_session)
+    secure_session[self.session_challenge_key(user)] = nil
   end
 
   def self.allowed_credentials(user, secure_session)
     return {} if !user.security_keys_enabled?
-    credential_ids = user.second_factor_security_key_credential_ids
+
     {
-      allowed_credential_ids: credential_ids,
-      challenge: secure_session[self.session_challenge_key(user)],
+      allowed_credential_ids: user.second_factor_security_key_credential_ids,
+      challenge: self.challenge(user, secure_session),
     }
   end
 

lib/discourse_webauthn/authentication_service.rb

@@ -111,6 +111,7 @@ module DiscourseWebauthn
 
       # 26. Success! Update the last used at time for the key (credentialRecord).
       security_key.update(last_used: Time.zone.now)
+      clear_challenge
 
       # Return security key record so controller can use it to update the session
       security_key

lib/discourse_webauthn/base_validation_service.rb

@@ -19,6 +19,10 @@ module DiscourseWebauthn
       raise(ChallengeMismatchError, I18n.t("webauthn.validation.challenge_mismatch_error"))
     end
 
+    def clear_challenge
+      DiscourseWebauthn.clear_challenge(@current_user, @session)
+    end
+
     def validate_origin
       return if origin_match?
       raise(InvalidOriginError, I18n.t("webauthn.validation.invalid_origin_error"))

lib/discourse_webauthn/challenge_generator.rb

@@ -8,9 +8,8 @@ module DiscourseWebauthn
         @challenge = params[:challenge]
       end
 
-      def commit_to_session(secure_session, user)
-        secure_session[DiscourseWebauthn.session_challenge_key(user)] = @challenge
-
+      def commit_to_session(secure_session, user, expires: nil)
+        secure_session.set(DiscourseWebauthn.session_challenge_key(user), @challenge, expires:)
         self
       end
     end

spec/lib/concern/second_factor_manager_spec.rb

@@ -165,7 +165,7 @@ RSpec.describe SecondFactorManager do
 
   describe "#authenticate_second_factor" do
     let(:params) { {} }
-    let(:secure_session) { {} }
+    let(:secure_session) { SecureSession.new("some-prefix") }
 
     context "when neither security keys nor totp/backup codes are enabled" do
       before { disable_security_key && disable_totp }

spec/lib/discourse_webauthn/challenge_generator_spec.rb

@@ -12,7 +12,7 @@ RSpec.describe DiscourseWebauthn::ChallengeGenerator do
       let(:user) { Fabricate(:user) }
 
       it "stores the challenge in the provided session object" do
-        secure_session = {}
+        secure_session = SecureSession.new("some-prefix")
         generated_session = DiscourseWebauthn::ChallengeGenerator.generate
         generated_session.commit_to_session(secure_session, user)
 

spec/lib/discourse_webauthn/discourse_webauthn_spec.rb

@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 RSpec.describe DiscourseWebauthn do
+  fab!(:user)
+
   describe "#origin" do
     it "returns the current hostname" do
       expect(DiscourseWebauthn.origin).to eq("http://test.localhost")
@@ -13,4 +15,34 @@ RSpec.describe DiscourseWebauthn do
       end
     end
   end
+
+  describe ".stage_challenge" do
+    let(:secure_session) { SecureSession.new("some-prefix") }
+
+    it "stores the challenge in the provided session object with the right expiry" do
+      described_class.stage_challenge(user, secure_session)
+      key = described_class.session_challenge_key(user)
+
+      expect(secure_session[key]).to be_present
+
+      expect(secure_session.ttl(key)).to be_within_one_second_of(
+        DiscourseWebauthn::CHALLENGE_EXPIRY,
+      )
+    end
+  end
+
+  describe ".clear_challenge" do
+    let(:secure_session) { SecureSession.new("some-prefix") }
+
+    it "clears the challenge from the provided session object" do
+      described_class.stage_challenge(user, secure_session)
+      key = described_class.session_challenge_key(user)
+
+      expect(secure_session[key]).to be_present
+
+      described_class.clear_challenge(user, secure_session)
+
+      expect(secure_session[key]).to be_nil
+    end
+  end
 end

spec/requests/session_controller_spec.rb

@@ -2058,11 +2058,14 @@ RSpec.describe SessionController do
         end
 
         before do
-          simulate_localhost_webauthn_challenge
           DiscourseWebauthn.stubs(:origin).returns("http://localhost:3000")
 
           # store challenge in secure session by failing login once
           post "/session.json", params: { login: user.username, password: "myawesomepassword" }
+
+          read_secure_session[
+            DiscourseWebauthn.session_challenge_key(user)
+          ] = valid_security_key_challenge_data[:challenge]
         end
 
         context "when the security key params are blank and a random second factor token is provided" do
@@ -2120,10 +2123,23 @@ RSpec.describe SessionController do
 
             expect(response.status).to eq(200)
             expect(response.parsed_body["error"]).not_to be_present
+
             user.reload
 
             expect(session[:current_user_id]).to eq(user.id)
             expect(user.user_auth_tokens.count).to eq(1)
+
+            post "/session.json",
+                 params: {
+                   login: user.username,
+                   password: "myawesomepassword",
+                   second_factor_token: valid_security_key_auth_post_data,
+                   second_factor_method: UserSecondFactor.methods[:security_key],
+                 }
+
+            expect(response.parsed_body["error"]).to eq(
+              I18n.t("webauthn.validation.challenge_mismatch_error"),
+            )
           end
         end